Re: [PATCH] merge-recursive: restore accidentally dropped setting of path

[Elijah Newren <newren@gmail.com>]

On Tue, Jun 4, 2019 at 12:26 AM Elijah Newren <newren@gmail.com> wrote:
> On Mon, Jun 3, 2019 at 7:30 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:

> > > -     filespec_from_entry(&tmp, ci->ren1->src_entry, other_stage);
> > > -     tmp.path = a->path;
> >
> > Note that 'tmp.path' used to be set ...
> >
> > > -
> > >       prev_path_desc = xstrfmt("version of %s from %s", path, a->path);
> > > -     if (merge_mode_and_contents(opt, a, c, &tmp,
> >
> > ... and that this 'tmp' used to become 'b' in
> > merge_mode_and_contents() and then in merge_3way().
> >
> > > +     if (merge_mode_and_contents(opt, a, c,
> > > +                                 &ci->ren1->src_entry->stages[other_stage],
> > >                                   prev_path_desc,
> > >                                   opt->branch1, opt->branch2,
> > >                                   1 + opt->call_depth * 2, &mfi))
> > >               return -1;
> > >       free(prev_path_desc);
> >
> >
> > This one-liner patch below the issue, the merge fails with conflicts
> > as expected, but, honestly, I have no idea what I am doing :)  At
> > least the test suite still passes, but that might not mean all that
> > much since it missed this issue in the first place...
> >
> > diff --git a/merge-recursive.c b/merge-recursive.c
> > index a7bcfcbeb4..d2e380b7ed 100644
> > --- a/merge-recursive.c
> > +++ b/merge-recursive.c
> > @@ -1660,6 +1660,7 @@ static int handle_rename_add(struct merge_options *opt,
> >                c->path, add_branch);
> >
> >         prev_path_desc = xstrfmt("version of %s from %s", path, a->path);
> > +       ci->ren1->src_entry->stages[other_stage].path = a->path;
> >         if (merge_mode_and_contents(opt, a, c,
> >                                     &ci->ren1->src_entry->stages[other_stage],
> >                                     prev_path_desc,
> >
> >
>
> This analysis and patch are correct; I somehow deleted the setting of the
> path here in what should have been a straightforward conversion.
>
> I've tried to look through every other callsite to merge_3way to see
> if any others fail to set the paths; there's a dozen or two of them.
> I think this was the only one that was missed, but honestly I'm
> exhausted right now and not sure I'm thinking straight.  So I'll
> recheck tomorrow and do a bunch more testing.

I've rechecked pretty thoroughly and yeah this was the only one that
was missed.  If anyone wants to double check me, here's some notes:

merge_3way has the following callers:

merge_3way (1 caller)
  <- merge_mode_and_contents (7 callers, 2x from handle_rename_rename_2to1)
     <- merge_mode_and_contents (to flip args into canonical order)
     <- handle_rename_add (1 caller)
        <- process_entry
     <- handle_rename_rename_1to2 (1 caller)
        <- process_entry
     <- handle_rename_rename_2to1 (1 caller)
        <- process_entry
     <- handle_content_merge (2 caller)
        <- process_entry
        <- handle_rename_normal (1 caller)
          <- process_entry
     <- handle_file_collision (6 callers, 2x from handle_rename_rename_1to2)
        <- handle_file_collision (to flip args into canonical order)
        <- handle_rename_add (1 caller)
           <- process_entry
        <- handle_rename_rename_1to2 (1 caller)
           <- process_entry
        <- handle_rename_rename_2to1 (1 caller)
           <- process_entry
        <- process_entry

From this, it's clear that just about everything starts in
process_entry().  process_entry() is pretty meticulous about setting
[oab]->path; it does so near the beginning of the function and then
updates for each of the rename cases to make sure it has the basic
values right.  I've audited all those and they are clean.  So the big
question is which functions pass something other than the [oab]
diff_filespec that they were passed; checking into that the answer is:

  handle_rename_rename_2to1 (3 different places, but path set in all of them)
  handle_rename_rename_1to2 (2 different places, but path set in all of them)
  handle_rename_add         (2 different places; one missed setting a path)

So, this is indeed the one case that was missed, and as SZEDER showed,
it was in the original but the conversion just somehow dropped the
simple one liner.  So I'll send an updated patch making the small
tweaks suggested by SZEDER, add a Tested-by for Ben since he retested
with his extra testcases for us, and then we'll be good for 2.22.0.

While doing this I also found a couple other small cleanups and
improvements, but I'll submit those after 2.22.0 is out; let's keep
the regression fix as minimal as possible.