[PATCH] Re: Feature Request: 'bzr mv --after' to tell bzr that file(s) have already been moved in the working tree

[John Arbash Meinel]

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

Hanns-Steffen Eichenberg wrote:
> This patch is a draft.
> 
> 
> On 11/3/06, *John Arbash Meinel* <john at arbash-meinel.com
> <mailto:john at arbash-meinel.com>> wrote:
> 
> 
>     I thought I saw you mention on IRC you actually wrote a patch for this,
>     which I would be happy to look over.
> 
> 
> Here it is. Since this is my first work for bzr and my first lines of
> python for more than 3 years i would be happy if
> you can give the code a thorough review.
> 
> What i have done:
>   - modified the move code and the command line documentation  
>   - some tests on the command line with a real world project
> 
> What i have not done:
>   - written any tests, because i do not know where and how. I would
> appreciate some help here
>   - run 'bzr selftest', because the selftest of 'bzr.dev' seems to hang
> on my machine
> 
> 
> The code was developed on WinXP. Did not had time to test it on my Linux
> box.
> 
> 
> Ciao,
>   Steffen
> 


Here are a few comments:

1) I guess I spoke to soon in the other thread, since here indeed is a
patch. Thank you for this. The functionality seems to be there. We need
to have tests for everything, though. If you aren't comfortable writing
them, I'm certainly willing to help you through. Or you can always punt
and ask me to do it if you don't have the time.

I highly recommend learning to write tests along with code. I'm not 100%
dedicated to test-driven yet, but test-as-you-go is really nice.
Test-driven can be really good too, I just don't always use it.
(Basically, to know your code works, you have to run it anyway, so you
might as well put that in a real test, and then in the future you will
be aware if that test ever fails).

2) We generally submit patches as text files (not .zip, etc) because
that way we can review them inside an email, rather than downloading it,
unzipping it, opening another editor, and the copying code chunks back
to do comments.

3) If you read HACKING, you can see some of our style guides, but the
important ones are:

   1) Use spaces, not tabs to indent
   2) Wrap lines at 79 characters

4) You changed the parameter 'to_name' to 'to_dir'. I agree that it is
more descriptive, but because in python any parameter can be accessed by
its' name, we have to maintain API compatibility. (eg
move(from_paths=[], to_name='foo'))

However, we can do this:

def move(self, from_paths, to_dir=None, after=False, **kwargs):
...

if to_dir is None:
  to_name = kwargs.get('to_name', None)
  if to_name is None:
    raise ? ('You must supply a target directory')
  else:
    symbol_versioning.warn('The parameter to_name was deprecated'
			   ' in version 0.13. Use to_dir instead',
                           DeprecationWarning)
    to_dir = to_name


I'm not sure what the exact exception should be. Python would raise an
TypeError if you didn't supply a required argument. Which seems good
enough here. It is a programmer error, not a runtime error. (We
generally avoid raising builtin errors, but in this case it makes sense)


5) This section:
+            tuple = {}
+            tuple['from_rel']  = from_rel
+            tuple['from_id']   = from_id
+            tuple['from_tail'] = from_tail
+            tuple['from_parent_id'] = from_parent_id
+            tuple['to_rel']    = to_rel
+            tuple['to_tail']   = from_tail   # from_tail == to_tail
+            tuple['to_parent_id'] = to_dir_id
+            from_to_tuples.append(tuple)

First, you call it a 'tuple' but it is a dictionary. (Python has a tuple
type (1, 2, 3))

Second, PEP8 requests that you don't add whitespace before your '='
sign. I understand why you like it, but for consistency (and to prevent
people going back and forth changing this sort of thing) we've decided
to follow PEP8's recommendations.

Third, I think you have enough stuff going on here that a real object
would be better than a dictionary. We won't be creating so many of them
that a dictionary performance would matter. And I think it ends up being
clearer. And if you want to keep the naming, you still can, it just becomes:

class _RenameEntry(object):

  def __init__(self, from_rel, from_id, from_tail, from_parent_id,
               to_rel, to_tail, to_parent_id):
     self.from_rel = from_rel
...
     self.only_change_inv = None

...

   entry = _RenamEntry(from_rel=from_rel,
                       from_id=from_id,
                       from_tail=from_tail,
                       ...
                      )

6) All of the mutter() statements should be change:
+        mutter("  from_id   {%s}" % from_id)
+        mutter("  from_rel  %r" % from_rel)
+        mutter("  to_rel    %r" % to_rel)
+        mutter("  to_dir    %r" % to_dir)
+        mutter("  to_dir_id {%s}" % to_dir_id)
+

This wasn't your bug, they were already incorrect. But at the least,
these should be:
+        mutter("  from_id   {%s}", from_id)
+        mutter("  from_rel  %r", from_rel)
+        mutter("  to_rel    %r", to_rel)
+        mutter("  to_dir    %r", to_dir)
+        mutter("  to_dir_id {%s}", to_dir_id)
+

Otherwise if there was a '%' symbol in one of the filenames, it would
cause mutter() to puke, because mutter() also does a string expansion
with the arguments. (Mutter should be updated to not puke, but this is
still incorrect).

Further, I prefer to see stuff like this in a single call, so something
like:

mutter('rename one:  {%s}\n'
       '  from rel:  %r\n'
       '  to_rel:    %r\n'
       '  to_dir:    %r\n'
       '  to_dir_id: {%s}\n',
       from_id, from_rel, to_rel, to_dir, to_dir_id)


7) I also discovered that your bundle used 'dos' line endings (CRLF).
This is a bug with how we interact with Windows (win defaults to setting
stdin/stdout/etc as text mode, when we expect binary mode). But it means
that you should use 'bzr bundle --output=filename'

I hope this isn't more feedback than you wanted. I think the general
flow of what you did was good, just some tweaks to make it fit with our
coding style, etc.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.2.1 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFFT0xuJdeBCYSNAAMRAuDsAJ9avvaiRCgFMpc8cG2RUI5IGZzpEACfVJZQ
XSdpWRqcguUx9D1bk18DPv0=
=f7jI
-----END PGP SIGNATURE-----