iter_changes, delta consistency and revert

[Aaron Bentley]

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

Robert Collins wrote:
> 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.

I'm not sure what you mean by "applied to an inventory delta".  I think
you mean if an inventory delta was generated from it?  If so, you have
choice in how you generate paths.  Generating all the paths in an
inventory delta should be pretty cheap in bulk, since it's just a matter
of concatenating the Inventory.name attributes that you already have.

> 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.

True, but it is recoverable.

> 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'

Can you expand on that description of merge?  The TreeTransform code is
very robust, and it will create missing parent directories as needed,
for example.

>  - 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.

Please fix this.  iter_changes is supposed to be symmetric.

> I have thought of of a few options:
>  - inspect the iter_changes output inside revert, and discard things
>    that are not needed.

As far as I'm concerned, revert is using iter_changes exactly the way it
was meant to be used.  It may be that inventory delta generation needs
something similar to, but not the same as, iter_changes.  Perhaps both
could be built on the same primitive.

>  - 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.

Revert is built on the TreeTransform code, and therefore cannot produce
an inconsistent transform at a logical level.  Whatever bugs there are
in the way TreeTransform generates inventory deltas need to be fixed
once, and should stay fixed forever.

> 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

I don't.  In my thinking, I rely on the fact that iter_changes is
symmetric, and it comes as a shock that this has changed.

Having iter_changes emit things that haven't changed is bad enough.
Having it emit things that *have changed* but should be ignored is very bad.

I think we're running into trouble because we're changing the original
contract of iter_changes into something much more complex.  I think we
should re-examine whether iter_changes is actually the issue here.
Maybe we need a new API that gives the data inventory deltas need, instead.

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

iEYEARECAAYFAkp4PJEACgkQ0F+nu1YWqI1ATQCffUE+yxhV4ClmIirn3JtW7TSB
PhoAniJuVhFm5uYPOqS3vBj2mKQjpJhS
=siwX
-----END PGP SIGNATURE-----