[MERGE] iter-changes based commit

[Ian Clatworthy]

Robert Collins wrote:
> Here's what I'd like to see:
> 
> +        # Note: A smart commit iterator is in the pipeline. Until it
> +        # arrives, we special case initial commit and non-merge commits
> +        # for performance.
> +        if len(self.parents) <= 1:
> +            self._populate_from_tree(specific_files)
> +        else:
> +            self._populate_from_inventory(specific_files)
> 
> One code path. There is no need to special case here using _iter_changes
> as a broader 'iterate the tree efficiently including data the inventory
> does not know about' tool. Do any tests fail when there is one code
> path ?

It appears not. The reporting of merge results has a minor reorder
change, but otherwise, all is good.

> This case:
> +            # Skip unknowns unless strict mode
> +            if versioned_flags == (False,False):
> +                if self.strict:
> +                    raise StrictCommitFailed()
> +                else:
> +                    continue
> 
> Add a comment:
>                      # Could version the file just-in-time here.
> Also, I think its going to lead to a bug as it stands in N-parent trees,
> where a versioned in the 2nd parent tree but unversioned in this tree,
> does not get reported at all but it probably should... and this is one
> of the things iter_changes isn't great at as an interface so we should
> tweak it - really we want a vector of all the parent versioned flags.

That's bad. In the worse case, we'd be replacing unknown files in the
working tree with ones from a merge tree - data loss. Some questions:

1. Should we abort the commit in this case?
2. Is it the reponsibility of merge or commit to detect and report this?
   Seems like maybe both?
3. Off the top of your head, is the bug in the current code or have I
   introduced it?

> (This is what my commit-ready iterator does and why it may be easier to
> work with). 

Yes.

> Test wise, I have a few pet corner cases I don't know if we test today:
> 
> basis:
> /foo foo-id
> /foo/gam gam-id
> /foo/quux quux-id
> 
> current:
> /bar foo-id (change path, remove child)
> /bar/gam gam-id (unchanged)
> /gam quux-id (change the name)
> 
> 'commit foo' or 'commit bar' should both record new versions of foo-id
> and quux-id at /bar and /gam and the existing version of gam-id
> at /bar/gam
> 
> 'commit gam' should record a new version of quux-id at /gam and the
> existing versions of foo-id and gam-id at /bar and /bar/gam
> 
> 'commit foo/gam' or 'commit bar/gam' should error when allow_unchanged
> is False.
> 
> with an extra parent:
> original tree:
> /bar foo-id at rev1
> 
> basis:
> /foo foo-id at rev-2
> 
> parent-2:
> /foo foo-id at rev-3
> 
> current:
> /foo foo-id
> 
> Committing foo records foo-id at rev-4 even though no changes were made
> against any parent.
> 
> (see my commit builder tests that went in a couple of days back for low
> level tests that this works as designed). They include the full setup -
> make a tree, add the path and commit, then branch and do a change,
> commit, make the same change commit, then merge both and commit.

Umm - I had a look back through bzr.dev and couldn't see the change you
meant. Can you be more explicit and mention the renvo or files? Or maybe
it's a different branch?

> I think that if you were to fix the two external bugs to my iterator it
> would fit well with this branch and help make it mergable.

I plan to look at those next week.

Thanks for the feedback,
Ian C.