From: Elijah Newren <newren@gmail.com>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
Junio C Hamano <gitster@pobox.com>,
ben.humphreys@atlassian.com,
Ben Humphreys <behumphreys@atlassian.com>
Subject: Re: [PATCH] merge-recursive: restore accidentally dropped setting of path
Date: Tue, 4 Jun 2019 13:22:50 -0700 [thread overview]
Message-ID: <CABPp-BFF4B+YoMyfxVDKPmob1wVarvz02BTiCEaPGH8FHJ9ATQ@mail.gmail.com> (raw)
In-Reply-To: <20190604072614.26885-1-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.
next prev parent reply other threads:[~2019-06-04 20:23 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-03 20:23 [ANNOUNCE] Git v2.22.0-rc3 Junio C Hamano
2019-06-04 1:32 ` Ben Humphreys
2019-06-04 2:30 ` SZEDER Gábor
2019-06-04 7:26 ` [PATCH] merge-recursive: restore accidentally dropped setting of path Elijah Newren
2019-06-04 8:33 ` Ben Humphreys
2019-06-04 13:14 ` SZEDER Gábor
2019-06-04 20:14 ` Elijah Newren
2019-06-04 20:22 ` Elijah Newren [this message]
2019-06-04 20:27 ` [PATCH v2] " Elijah Newren
2019-06-04 21:07 ` SZEDER Gábor
2019-06-04 21:33 ` Junio C Hamano
2019-06-04 22:48 ` Elijah Newren
2019-06-04 1:47 ` [ANNOUNCE] Git v2.22.0-rc3 Bhaskar Chowdhury
2019-06-04 14:45 ` Git for Windows v2.22.0-rc3, was " Johannes Schindelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CABPp-BFF4B+YoMyfxVDKPmob1wVarvz02BTiCEaPGH8FHJ9ATQ@mail.gmail.com \
--to=newren@gmail.com \
--cc=behumphreys@atlassian.com \
--cc=ben.humphreys@atlassian.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=szeder.dev@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).