mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Elijah Newren <>
To: Junio C Hamano <>
Cc: Git Mailing List <>,
	Emily Shaffer <>,
	Derrick Stolee <>
Subject: Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case
Date: Tue, 6 Aug 2019 10:26:24 -0700	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Tue, Aug 6, 2019 at 9:57 AM Junio C Hamano <> wrote:
> Elijah Newren <> writes:
> > Ever since commit 8c8e5bd6eb33 ("merge-recursive: switch directory
> > rename detection default", 2019-04-05), the default handling with
> > directory rename detection was to report a conflict and leave unstaged
> > entries in the index.  However, when creating a virtual merge base in
> > the recursive case, we absolutely need a tree, and the only way a tree
> > can be written is if we have no unstaged entries -- otherwise we hit a
> > BUG().
> >
> > There are a few fixes possible here which at least fix the BUG(), but
> > none of them seem optimal for other reasons; see the comments with the
> > new testcase 13e in t6043 for details (which testcase triggered a BUG()
> > prior to this patch).  As such, just opt for a very conservative and
> > simple choice that is still relatively reasonable: have the recursive
> > case treat 'conflict' as 'false' for opt->detect_directory_renames.
> >
> > Reported-by: Emily Shaffer <>
> > Signed-off-by: Elijah Newren <>
> > ---
> >
> > I really should introduce constants like
> How would they compare with MERGE_DIRECTORY_RENAMES_* macros
> I see at the tip of 'pu'?  init_merge_options() seems to read
> one of those values from the "repo settings" and copies it to
> the detect_directory_renames field, so I am reading that they
> must be identical.

Indeed, it looks like Stolee has done my work for me -- though I would
suspect that his change won't be merged down to maint, and I am
assuming that my patch might be.  If you don't want to merge it down
to maint, I can rebase on Stolee's patch; just let me know.  (And if
you do want to merge it down to maint, I can send a subsequent patch
to modify and adopt Stolee's naming.)

> > and then use them in the code to make it clearer, but I wanted to make
> > the code change as simple and contained as possible given that this is
> > built on maint, fixes a BUG() and we're already in -rc1.
> >
> > I know this bug doesn't satisfy the normal criteria for making it into
> > 2.23 (it's a bug that was present in 2.22 rather than a regression in
> > 2.23), but given that it's a BUG() condition, I was hoping it is
> > important and safe enough to include anyway.
> I do agree that it is sensible to avoid doing any funky thing during
> the virtual base merges, whose result is much less observable (hence
> harder to form the right mental model in end user's head) than the
> outermost merge.  Do we want to allow this for inner merges when the
> setting is 2?  Wouldn't that hit the same BUG()?

No, it doesn't hit the same BUG().  Hitting the BUG() only came from
having unstaged entries at the end of an inner merge, which only came
from the 'conflict' setting for directory rename detection.  Having
directory rename detection completely on (detect the rename and accept
it as the resolution), or completely off (don't detect directory
renames, i.e. leave old pathname as the resolution) both have well
defined resolutions.

But that doesn't completely answer your question about whether we want
to have directory rename detection for the inner merges, it just
suggests we have to avoid 'conflict' as a setting in that case.  Let
me look at the same testcase:

> > +# Testcase 13e, directory rename in virtual merge base
> > +#
> > +# This testcase has a slightly different setup than all the above cases, in
> > +# order to include a recursive case:
> > +#
> > +#      A   C
> > +#      o - o
> > +#     / \ / \
> > +#  O o   X   ?
> > +#     \ / \ /
> > +#      o   o
> > +#      B   D
> > +#
> > +#   Commit O: a/{z,y}
> > +#   Commit A: b/{z,y}
> > +#   Commit B: a/{z,y,x}
> > +#   Commit C: b/{z,y,x}
> > +#   Commit D: b/{z,y}, a/x

Here, if the user has directory rename detection fully on
(opt->detect_directory_renames == 2), then the fact that commits C and
D resolved the merge differently is perhaps more surprising, because
the default would be to just accept the rename.  That means one side
(commit C) is likely to have just taken the default resolution, but
someone (whoever created commit D) made a manual effort to undo the
directory rename.  Now, if the virtual merge base doesn't do directory
rename detection, then it'll match commit D which undid the directory
rename detection, and result in commit C winning and using the
directory rename.  That seems like the wrong choice to me.

So, I think that if opt->detect_directory_renames == 2, then we really
do want to use directory rename detection on the virtual merge base so
that we don't silently miss someone manually undoing the directory
rename detection on one side of history.

The trickier choice comes when opt->detect_directory_renames == 1 (or
'conflict') because then we don't have a sane
avoid-accidentally-matching-one-side solution.  I'm not sure there's a
"best" choice for this case, but we should certainly at least avoid a
BUG().  The simple ways to do that were to translate 'conflict' to
either 'false' or 'true'.  I can't give a good rationale for why I
picked 'false' over 'true' in that case with this patch; either seem
equally good, but 'false' has a slight performance advantage over
'true' so I went with it.

  reply	other threads:[~2019-08-06 17:26 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-07-26 22:09 BUG() during criss-cross merge with directory rename and deleted file Emily Shaffer
2019-08-05 22:33 ` [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case Elijah Newren
2019-08-06 16:57   ` Junio C Hamano
2019-08-06 17:26     ` Elijah Newren [this message]
2019-08-06 17:49     ` Junio C Hamano
2019-08-06 17:26   ` Junio C Hamano
2019-08-06 17:29     ` Elijah Newren
2019-08-06 20:28   ` Emily Shaffer
2019-08-06 21:16     ` Elijah Newren
2019-08-06 21:54       ` Emily Shaffer
2019-08-08 11:00       ` Jeff King

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:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* 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

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).