Re: [PATCH 4/5] merge_recursive: Fix renames across paths below D/F conflicts

On Tue, Jun 29, 2010 at 7:36 AM, Alex Riesen [off-list ref] wrote:

On Tue, Jun 29, 2010 at 14:52, Elijah Newren [off-list ref] wrote:

quoted

Alex: I think the basic idea is just that the rename logic isn't aware
that there may be higher stage entries in the index due to D/F
conflicts; by checking for such cases and marking the entry as not
processed, it allows process_entry() later to look at it and handle
those higher stages.  But I'm not sure if that's the right way to
handle it, or if just having process_renames() should take care of
clearing out the higher stage entries, or if something else entirely
should be done.

Nor am I. You may be still off by some commits in detecting the authorship :)
This code was seldom touched since it was written (by Johannes). It has
survived in this sorry state all (at least my) attempts to fix it. OTOH I never
tried really hard. Maybe because the D/F conflicts are rare and are relatively
simple to work around.

I cannot say much about your change... Are you sure about D/F conflict
detection, though? You just test if target mode not 0.

Well, as far as this particular if-block is concerned, blame suggests
that you and Miklos were responsible (I apologize if gmail screws up
and inserts line wrapping)::

$ git blame -C -C -L 1020,1038 merge-recursive.c
9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1020)
                 if (mfi.clean &&
9047ebbc (Miklos Vajna  2008-08-12 18:45:14 +0200 1021)
                     sha_eq(mfi.sha, ren1->pair->two->sha1) &&
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1022)
                     mfi.mode == ren1->pair->two->mode) {
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1023)
                         /*
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1024)
                          * This messaged is part of
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1025)
                          * t6022 test. If you change
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1026)
                          * it update the test too.
8a359819 (Alex Riesen   2007-04-25 22:07:45 +0200 1027)
                          */
8a2fce18 (Miklos Vajna  2008-08-25 16:25:57 +0200 1028)
                         output(o, 3, "Skipped %s (merged same as
existing)", ren1_
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1029)
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1030)
                         /* If this was a rename across a path
involved
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1031)
                          * in a D/F conflict, there may be more work
to
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1032)
                          * do.
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1033)
                          */
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1034)
                         for (i=1; i<=3; ++i) {
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1035)
                                 if (ren1->dst_entry->stages[i].mode)
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1036)
                                         ren1->dst_entry->processed =
0;
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1037)
                         }
de4d7dc3 (Elijah Newren 2010-06-28 09:38:58 -0600 1038)
                 } else {

With D/F conflicts, the files would be loaded into higher stages in
the index (before it gets to process_renames()), which I detected via
a non-zero mode.  If there's a different way I should be checking for
higher stage entries that still need to be resolved, I'd be happy to
use it.