[PATCH] Re: Feature Request: 'bzr mv --after' to tell bzr that file(s) have already been moved in the working tree
Hanns-Steffen Eichenberg
scameronde at googlemail.com
Mon Nov 6 20:04:05 GMT 2006
Wow, you guys are fantastic. I didn't expected that much response.
On 11/6/06, Aaron Bentley <aaron.bentley at utoronto.ca> wrote:
>
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Hanns-Steffen Eichenberg wrote:
>
> > 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.
>
> Here's a review of the actual content. I hope it doesn't come off as
> too nitpicky.
No. I asked for it.
Some style things:
> - - please wrap your lines at 79 characters.
> - - "independent" has no "a" in it
> - - the "after" option should be described in terms of the resulting
> behavior, e.g. "move only the bzr identifier of the file (file has
> already been moved)"
will fix it.
> === modified file .bzrignore //
> last-changed:eichenbs at gilpnb051b-20061103151123
> > ... -dcb18125ed861bdc
> > --- .bzrignore
> > +++ .bzrignore
> > @@ -30,4 +30,6 @@
> > ./tools/win32/bzr.iss
> > ./dist
> > # performance history data file
> > -./.perf_history
> > \ No newline at end of file
> > +./.perf_history
> > +make_bat.last
> > +make_bat.py
>
> What are make_bat.last and make_bat.py?
that is a script written by Alexander Belchenko to make it easier
to switch bzr versions under WinXP.
will move that into my user ignore file.
> + for tuple in tree.move(rel_names[:-1], rel_names[-1], after):
> > + self.outf.write("%s => %s\n" % (tuple['from_rel'],
> tuple['to_rel']))
>
> John already pointed out there's a built-in tuple type, but I don't
> think he mentioned that "tuple" is the name of a built-in function.
> Assigning to "tuple" overides that built-in function in the "run"
> method's scope. In this case, it's not dangerous, but can be considered
> bad form.
Definately bad form. Will fix it.
Also, because we try to preserve API compatibility, we can't just change
> the return type of tree.move. If a new return type is desired, we can
> create a new method, or add a parameter to tree.move to enable the new
> output. There needs to be at least one release that supports the old
> and new behaviors, with the old behavior causing a deprecation warning
> to be emitted.
I will change that, rename the new method and deprecate the old one.
> === modified file bzrlib/workingtree.py
> > --- bzrlib/workingtree.py
> > +++ bzrlib/workingtree.py
> > @@ -989,7 +989,7 @@
> > stack.pop()
> >
> > @needs_tree_write_lock
> > - def move(self, from_paths, to_name):
> > + def move(self, from_paths, to_dir, after=False):
>
> I wonder whether after should support 3 values:
> True: move only the inventory
> False: always move both inventory and file
> None: auto-detect whether to move file
The logic that has been discussed at the beginning of that thread
is different. Autodetect is always on, but will fail under certain
circumstancec.
The 'after' flag has more of a 'force' semantic, to tell move that a certain
case is not an error. The name 'after' is borrowed from Mercury.
> """Rename files.
> >
> > to_name must exist in the inventory.
> > @@ -1000,112 +1000,243 @@
> > Note that to_name is only the last component of the new name;
> > this doesn't change the directory.
> >
> > - This returns a list of (from_path, to_path) pairs for each
> > - entry that is moved.
> > + for each entry in from_paths the move mode will be determined
> independantly.
> > +
> > + The first mode is moving the file in the filesystem and update
> the inventory.
> > + The second mode is only update the inventory without touching
> the file on the
> > + filesystem. This is the new mode introduced in version 0.13.
> > +
> > + move uses the second mode if 'after == True' and the target is
> not
> > + versioned but present in the working tree.
> > +
> > + move uses the second mode if 'after == False' and the source is
> versioned but
> > + no longer in the working tree, and the target is not versioned
> but present in the
> > + working tree.
> > +
> > + rename_one uses the first mode if 'after == False' and the
> source is versioned
> > + and present in the working tree, and the target is not
> versioned and not present
> > + in the working tree.
>
>
> I don't think the doctext for WorkingTree.move should describe
> WorkingTree.rename_one. (Cut and paste error?)
Yep.
> + # create from-to tuples
>
> I think a lot of the need for from-to tuples would be eliminated if
> _determine_mv_mode worked on a single entry at a time.
>
> i.e. its signature would be:
> _determine_mv_mode(from_rel, from_id, to_rel)
Did it that way in my first version, changed it to operate on a list
instead. There are
two reasons for it.
1) performance. i did not want to call a method inside of a loop if i can
have
the method make that loop.
2) do the checks first, move later. So i have at least two loops: one for
the checks
and one for the moves. Because both loops operate more or less on the
same
data, i wanted to 'cache' that data. That is what the tuples are for.
I used dictionaries instead of real tuples, because i augment the data
in
the check-method and tuples are immutable. As John has said already, i
should
have used a ValueObject instead. I guess i have written to many Ruby
scripts in
the past and have been spoiled a bit. I will change the dictionary to a
ValueObject.
> + for from_rel in from_paths:
> > + from_tail = splitpath(from_rel)[-1]
> > + from_id = inv.path2id(from_rel)
> > + from_entry = inv[from_id]
>
> ^^^ This looks like it would fail messily whenever from_id is None. You
> probably want to raise an error if from_id is None.
You are right. But this is not the place to raise an error. I must set that
entry
to None instead.
You have failing test cases, and this is one reason why.
>
> Specifically,
>
> bzrlib.tests.blackbox.test_mv.TestMove.test_mv_unversioned
> bzrlib.tests.blackbox.test_mv.TestMove.test_mv_nonexisting
>
> are failing because they're raising the wrong error now.
>
>
> You should be able to run just those tests with "bzr selftest
> test_mv_unversioned" and "test_mv_nonexisting".
I will try it.
> + # move pairs.
> > original_modified = self._inventory_is_modified
> > try:
> > - if len(from_paths):
> > + if len(from_paths): # i do not understand this line
> (scameronde)
>
> The line is equivalent to 'if len(from_paths) > 0:'. I think we could
> actually exit early in that case.
I understand the logic expression, but i do not understand why the
modified flag should not be set if 'len(from_path) <= 0' ??
> + def _move(self, from_to_tuples):
> > + """Moves a list of files.
> > +
> > + The flag 'only_change_inv' is expected for each file to move
> > + determines if only the inventory or if both,
> > + inventory and filesystem, have to be updated.
> > + """
>
> This method is not entirely general, in that it's possible for the
> from_to_tuples to name the same file as both a source and a target,
> which would cause data loss. But since it's a private method and none
> of the callers can accept input like that, it's fine.
No, i think the method should handle that case. You never know who will use
this method in the future. It is only on line of code to make it more safe.
Thank you
for identifying that bug. Will fix it.
> + for tuple in from_to_tuples:
> > + from_rel = tuple['from_rel']
> > + from_id = tuple['from_id']
> > + to_rel = tuple['to_rel']
> > + to_tail = tuple['to_tail']
> > + to_parent_id = tuple['to_parent_id']
> > + only_change_inv = tuple['only_change_inv']
>
> I am seeing this pattern a wee bit too often. If this were a real
> tuple, you could do:
>
> for from_rel, from_id, to_rel, to_tail, to_parent_id, only_change_inv\
> in from_to_tuples:
>
> and then skip right to the good part.
see statement above.
> + def _rollback_move(self, moved):
> > + """Try to rollback a previous move in case of an error in the
> filesystem.
> > + """
>
> I don't have a specific refactoring in mind, but this looks awfully
> similar to _move, and there must be a way to unify them.
The only way that comes to my mind now is passing a flag to the
method. But i consider changing the semantic of a method with a
flag bad practice, so i will not do that. Maybe you have a better idea?
Aaron
Thank you for the thorough review!
Ciao,
Steffen
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20061106/b9cc738b/attachment.htm
More information about the bazaar
mailing list