[MERGE] remerge doesn't clobber unrelated conflicts

[Aaron Bentley]

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

John Arbash Meinel wrote:

> Thanks for doing this. I'm have a little bit of trouble understanding
> what the actual fix is, because some of your comments seem to conflict
> with the code. Can you do a brief overview of what has changed?

Sure thing.  The transform_tree operation was ignoring existing
conflicts when it updated the conflict list.  The actual bug was in
Merger, which was taking the list of conflicts from the merge operation,
and setting the working tree conflicts list to that.

There was a second bug: cmd_remerge wasn't clearing the conflicts lists
of files it was remerging.  Now, it does that too.

> ...
> 
> @@ -2186,6 +2187,8 @@
>              base_tree = repository.revision_tree(base_revision)
>              other_tree = repository.revision_tree(pending_merges[0])
>              interesting_ids = None
> +            new_conflicts = []
> +            conflicts = tree.conflicts()
> 
> ^- What are these for? In case nothing else happens? They just don't
> seem to be used.

new_conflicts is used as the input to tree.set_conflicts. (though it
will be overwritten if file_list is not None).

conflicts is used for calling select_conflicts on, to generate a fresh
conflicts list that doesn't include the conflicts for selected files.

> +                new_conflicts = conflicts.select_conflicts(tree,
> file_list)[0]
>              transform_tree(tree, tree.basis_tree(), interesting_ids)
> +            tree.set_conflicts(ConflictList(new_conflicts))
> +            conflicts = merge_inner(tree.branch, other_tree, base_tree,
> +                                    this_tree=tree,
> +                                    interesting_ids = interesting_ids,
> +                                    other_rev_id=pending_merges[0],
> +                                    merge_type=merge_type,
> +                                    show_base=show_base,
> +                                    reprocess=reprocess)
> 
> A little bit of PEP8 here, arguments aren't supposed to have spaces for
> parameters, so:
> interesting_ids=interesting_ids
> not
> interesting_ids = interesting_ids.

Oh, missed that one.

> Am I correct in saying that the only thing that changed was the spacing?

Correct.

> @@ -207,6 +207,7 @@
>          """Select the conflicts associated with paths in a tree.
> 
>          File-ids are also used for this.
> +        :return: a pair of ConflictLists: (not_selected, selected)
>          """
>          path_set = set(paths)
>          ids = {}
> 
> ^- You change the comment, but I don't see any 'return' section being
> changed. I think you didn't actually change it in this way.

That's right.  It was a documentation improvement drive-by.  I had to go
double-check the code to make sure that was what it did, so I thought I
should document it so no one else had to check the code.

> === modified file bzrlib/merge.py
> --- bzrlib/merge.py
> +++ bzrlib/merge.py
> @@ -390,7 +390,9 @@
>              results = self.tt.apply()> 
> 

>              self.write_modified(results)
>              try:
> -
> working_tree.set_conflicts(ConflictList(self.cooked_conflicts))
> +                new_conflicts = (self.cooked_conflicts +
> +                                 list(working_tree.conflicts()))
> +                working_tree.set_conflicts(ConflictList(new_conflicts))
>              except UnsupportedOperation:
>                  pass
>          finally:
> 
> ^- And this seems to indicated that you are combining the conflicts, not
> returning 2 sets.

Yes, I'm combining the conflicts.  I don't think I said I was returning
two sets anywhere.  (This is Merge3Merger.__init__, not
ConflictList.select_conflicts.)

> I'm a little concerned that you would get double
> entries for any files that you remerge. Though somehow you must be
> working around this.

Well, in the general case, it's pretty crazy to perform a merge on a
file that already has conflicts, but if you did, it would make sense to
combine the conflicts.  For example, a file might have a DuplicateID
conflict, but adding a TextConflict shouldn't overwrite the DuplicateID.

Now it could be argued that we shouldn't have two identical conflicts
for the same file, so I'd be willing to special-case that one.

But in the specific case of remerge, we've already done
tree.set_conflicts() to clear all the conflicts for all files we're
remerging.  So there would have to be a bug in select_conflicts for this
to produce duplicate conflicts.


> +    def test_merge_inner_conflicts(self):
> +        tree_a = self.make_branch_and_tree('a')
> +        tree_a.set_conflicts(ConflictList([TextConflict('patha')]))
> +        merge_inner(tree_a.branch, tree_a, tree_a, tree_a)
> +        self.assertEqual(len(tree_a.conflicts()), 1)
> 
> ^- What are you testing here. That merge_inner() doesn't effect the
> conflict list?

Right.  merge_inner is what tree_transform was calling that was
resetting the conflict list.  It calls Merger, but calling Merger is a
big pain, so I thought merge_inner was the best thing to test.

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

iD8DBQFEpyOo0F+nu1YWqI0RAq2OAKCI5bgZsUVpycQEoG+l0W8s5MNtygCdFQy7
YbykTpYeQc4I8QxqdBLCPtw=
=NLte
-----END PGP SIGNATURE-----