iter_changes, delta consistency and revert

[Robert Collins]

So, I'm trying to land this iter_changes change. Theres one failure that
has me concerned. I've attached the diff for it below.

Now, its a bit hard to read like this, or even in context, so let me
make it more obvious:

bzr init
mkdir -p a/d/e
mkdir f
touch a/b a/c a/e f/g f/h f/i
bzr add
bzr commit -m 1
bzr mv a/b f/b
bzr mv a/d/e f/e
bzr mv f/g a/g
bzr mv f/h h
bzr mv f j
bzr st a

Now, the last command before my patch does this:
$ bzr st a
renamed:
  a/b => j/b
  a/d/e/ => j/e/
  f/g => a/g

but after applying the patch:
$ bzr st a
renamed:
  a/b => j/b
  a/d/e/ => j/e/
  f/ => j/
  f/g => a/g

Now, you may be wondering why 'f->j' is in the status output. Its there
because without it the path of j/b, if applied to an inventory delta,
would be wrong. This matter for dirstate and other path based storage
systems. It also matters for humans, if we say we commit j/b, but
actually commit a/b, thats really confusing - at best.

The problem turns up in revert though. Revert uses iter_changes to
determine what to revert, and the inclusion of j in the output causes it
to revert the f->j rename when 'bzr revert a' is invoked. At a human
level, I would in fact expect 'revert a' to grab the things I'd renamed
out of a and put them back, and not to touch other things that look like
they are incidentally connected.

There is some friction here:
 - revert wants 'enough changes to put specified things back the way
   they were'
 - commit and merge want 'enough changes so that the output tree looks
   like the user expects and isn't corrupted'
 - status wants to 'show what commit will do'
 - I think revert is using iter_changes in a symmetrical way, but
   inventory deltas built on iter_changes are not symmetric, and the
   iter_changes work I've been doing is likewise not symmetric.
   Specifically tree.iter_changes(basis, specific=foo) != 
   basis.iter_changes(tree, specific=foo), where expansion for 
   consistency is concerned.

I have thought of of a few options:
 - inspect the iter_changes output inside revert, and discard things
   that are not needed.
 - make consistent expansion an optional flag to iter_changes and have
   revert use that.

However, we've had a number of bugs (going from memory) to do with
revert and dirstate; I'm not at all sure that this is a coincidence -
the very bug I'm fixing, to ensure consistent deltas, may well prevent
those bugs in revert.

I'd rather not have more flags to iter_changes, I think being
conservative and expanding as needed is the safest option. I think
revert is well placed as a special-case (it doesn't need consistency in
output, as its applying things in reverse) to manually filter the
transform it uses back down - by using a simple check that one of the
paths of each item it gets are inside the specified paths.

For now, I've made the failing test a KnownFailure, and would like to
land the changes. This is the bulk of the work, and can be tuned in
various directions quite easily.

=== modified file 'bzrlib/tests/blackbox/test_revert.py'
--- bzrlib/tests/blackbox/test_revert.py        2009-03-23 14:59:43
+0000
+++ bzrlib/tests/blackbox/test_revert.py        2009-08-04 00:59:44
+0000
@@ -96,7 +96,9 @@
         self.failUnlessExists('a/b')
         self.failUnlessExists('a/d')
         self.failIfExists('a/g')
-        self.failUnlessExists('j')
+        self.expectFailure(
+            "j is in the delta revert applies because j was renamed
too",
+            self.failUnlessExists, 'j')
         self.failUnlessExists('h')
         self.run_bzr('revert f')
         self.failIfExists('j')

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090804/873fde20/attachment.pgp