[MERGE] enhanced mv command

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Marius Kruger wrote:
> On 1/12/07, *Aaron Bentley* <aaron.bentley at utoronto.ca
>     It is spurious changes that I object to; both adding and removing
>     whitespace fall into that category, as does changing the execute bit.
>     (changing the names of the errors comes close, but I wouldn't consider
>     it *spurious*, per se, just out-of-place in this bundle)
> 
> 
> I don't really worry if it is spurious or not, since the code is properly
> version controlled nobody could argue who the true owner is or what
> changed.

That stuff isn't my primary concern.  My concerns are
1. It introduces merge conflicts.  Of course, any kind of change
introduces merge conflicts, but spurious changes are not worth the merge
conflicts they produce.  Spurious changes are also frequently broader
than real changes-- removing trailing whitespace from a file might
change 10% of the lines, evenly distributed across the file, while
semantic changes would only change lines in particular sections.

2. It impedes review.  When I run a diff, I want to see only the
semantic changes, not spurious reformatting.

> What is more important to me is if it is an improvement or a deterioration:
> * Removing trailing white space is an improvement as it gets you closer
>    to the (unenforced) convention of not having trailing white space in
> code.

I don't agree that it's an improvement.  Does it change how the code
looks?  Does it change what it does?  The baseline rule is simple: if
you're not changing what code does, don't change it at all.

> * Something which would be useful for me is to have a gui annotate,
> which could
>    drill down when you click on a line: Say the line's revisoin is 10,
> do a 'bzr annotate -r9'.

Improvements to Gannotate are always welcome.  I'm the nominal
maintainer, but I'm not very experienced with GTK.  Currently, when you
double-click a line, it shows you the diff from the revision that
introduced that line, which is also useful.

I should point out though, that automatically detecting the previous
version of a line is problematic.  A line may replace
nothing(insertion), or one line, or multiple lines.  Commonly, multiple
lines replace multiple lines, so there's no clear relationship.

>     > I think after this thing is merged I should start looking at doing a
>     > patch which:
>     > 1) removes all the trailing white space once and for all and
>     > 2) introduce a test which checks that nobody violates this ever
>     again.
>     >
>     > And I might also write a plugin which warns you that you are
>     > adding trailing white space when you commit (even failing the
>     commit if
>     > you choose).
> 
>     The last sounds the most appealing to me.  Spurious whitespace changes
>     have harmful effects on merging, and I have several outstanding branches
>     at the moment, so I would object to 1).
> 
> If I write a test without fixing it, the test will fail and thus (I
> think) you would
> not be able to merge it in, as it will start failing all over.

"The last" being "And I might also write a plugin which warns you that
you are adding trailing white space when you commit"

> What I was thinking is also providing a small python script which helps
> you remove
> all trailing white space in all the .py files.
> Then all diverging branches must just apply it to themselves first before
> attempting to merge back in.

This still breaks merge-format=weave, because that relies on annotations.

>     Well, it takes time to adjust one's habits.
> 
> but its sped up if its enforced

Certainly.

>     >     ^^^ Is it possible to catch more specific errors
>     here?  BzrError and
>     >     OSError are very broad.
>     >
>     > (Again I didn't write this, so don't be too hard on me)
>     > I think its ok, as we just want to rollback and raise it again.
>     > It is done like that elsewere in the file too.
> 
>     Really, I don't see why anyone would do this.  Either you want to catch
>     *all* exceptions and roll back, or you want to catch specific
>     exceptions, and only roll back on those.  In particular, OSError does
>     not necessarily indicate that a rename failed.  You would have to wrap
>     the try/catch around osutils.rename in order to know whether the rename
>     failed.
> 
> 
> I don't really know what to do about this.

Well, catching BzrError is Just Wrong.  Why BzrError and not, say,
KeyboardInterrupt or IOError?

Moving the BzrRenameFailedError into _move_entry would allow you to
accurately detect failed renames.

But I have to ask, why would you try to roll back a rename that failed?
 If it failed, then there's nothing to roll back, right?

>     If we're going to add assert*None, it would make sense to provide a
>     default message.  
> 
> 
> I had that in for both, but after working with it a while, I realized that
> a default message is pretty uselese like  "obj is None",
> so I'd rather force a proper message, which could cast some light on the
> context.

If you look at assertIs, the message is slightly more useful than that:

"file-id" is not None

for example.  When something isn't the expected value, it can be useful
to see what its value is.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFFp53c0F+nu1YWqI0RAkieAJ9FriEffOqwpBmi/Dn51Lk9otDDVwCfUG02
Ky8DXqGtc18DfY+lnbWjp0A=
=LlqO
-----END PGP SIGNATURE-----