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

On Fri, Aug 5, 2011 at 11:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

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.