Re: [PATCH 00/48] Handling more corner cases in merge-recursive.c

[Elijah Newren <newren@gmail.com>]

On Fri, Aug 5, 2011 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
> But why do you care about the original index (i.e. the starting state of
> our side) in the first place? If your algorithm depends on the original
> index, wouldn't that mean you would screw up the same merge if the user
> merged branches in the opposite direction?

No, it does not mess up the same merge in the opposite direction.  The
code in question (with some local modifications) is

       if (mfi.clean && !df_conflict_remains &&
           sha_eq(mfi.sha, a_sha) && mfi.mode == a_mode) {
               output(o, 3, "Skipped %s (merged same as existing)", path);
               /*
                * The content merge resulted in the same file contents we
                * already had.  We can return early if those file contents
                * are recorded at the correct path (which may not be true
                * if the merge involves a rename).
                */
               if (was_tracked(&o->original_index, path)) {
                       add_cacheinfo(mfi.mode, mfi.sha, path,
                                     0 /*stage*/, 1 /*refresh*/, 0 /*options*/)
                       return mfi.clean;
               }
       } else
                output(o, 2, "Auto-merging %s", path);

And the merge is:
  BASE: original-path: v1
  HEAD: original-path: v2
  SIDE: renamed-path: v1

So, HEAD did have the correct contents that we wanted, as checked in
the first if.  However, we cannot bail early because those contents
were recorded at the wrong path.  Repeating the merge in the other
direction would simply show that we don't have the right contents
(a_sha != mfi.sha).  Only if we have both the right contents and have
it at the right path should we exit early.

If we use "&the_index" instead of "&o->original_index", which is what
my previous series essentially did, then we get the wrong answer about
whether the contents are recorded at the necessary path, i.e.
was_tracked() lies to us.


Now, your gut feel and question here does turn out to be
important...in a different case.  You'll note that I'm passing an
index to was_tracked(); that's because there is one case (the call to
was_tracked() from would_lose_untracked()) where it is important that
we use the index as modified by unpack_trees rather than the original
index and not doing so causes a bug depending on the direction things
are merged.  I added a big comment in the code about that one.

> I think the fact you have a path "two" at stage 0, combined with the two
> diffs you ran for rename detection between the common ancestor and two
> branches, should be enough to decide if one branch added the path or moved
> it from elsewhere (in which case the common ancestor would not have that
> path), or it kept the path at the original place (with or without content
> change), no?

You are correct.  I just need to reorder the patch series somewhat
(other patches added the passing of the relevant diff_filepair
information to merge_content, which is needed to do these checks you
suggest), and then I can change the was_tracked() call to instead
compare pathnames.