git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Emily Shaffer <emilyshaffer@google.com>
Cc: Junio C Hamano <gitster@pobox.com>,
	Git Mailing List <git@vger.kernel.org>
Subject: Re: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case
Date: Tue, 6 Aug 2019 14:16:25 -0700
Message-ID: <CABPp-BFZig73jXKkKNaz=YpT4enB3_ARQ1KTz_ttPYobkY6Bhg@mail.gmail.com> (raw)
In-Reply-To: <20190806202829.GB26909@google.com>

On Tue, Aug 6, 2019 at 1:28 PM Emily Shaffer <emilyshaffer@google.com> wrote:
>
> On Mon, Aug 05, 2019 at 03:33:50PM -0700, Elijah Newren wrote:
> > 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().
...
> Thanks for picking this up and sorry I didn't end up sending anything -
> priority shifts on this end. :)

I totally understand.

...
> > +             git checkout C &&
> > +             test_must_fail git -c merge.directoryRenames=conflict merge B &&
> > +             git add b/x &&
> > +             test_tick &&
> > +             git commit -m "C" &&
>
> Then we merge C with B which places B as a mutual ancestor of D as well
> as C.
>
> > +
> > +
> > +             git checkout D &&
> > +             test_must_fail git -c merge.directoryRenames=conflict merge A &&
>
> Now we do the same thing merging A with D, which means that D has
> ancestors B from branching and A from merge, and C has ancestors A from
> branching and B from merge. So D and C have two closest ancestors
> (criss-cross merge).
>
> > +             git add b/x &&
> > +             mkdir a &&
> > +             git mv b/x a/x &&
>
> Now D adds contention over a/x and b/x (which were both mentioned in the
> ancestry too) to induce a conflict... or is this adding a resolution
> which can be decided on automatically? I guess later you are looking to
> make sure no CONFLICT still exists in the output, so you must be
> resolving the conflict here?

Yes, we are resolving the conflict for D by choosing to reject the
directory rename, placing 'x' in a/x instead of b/x.

When merge.directoryRenames=conflict, it'll make only b/x be present
in the working directory and mark it as conflicted (i.e. assume the
directory rename is probably the right resolution, but print a warning
and mark the index as needing to be updated to verify -- this allows
e.g. "git add -u" to do the "right thing").  For commit C, we just did
a "git add b/x" to accept the directory rename.  For commit D, we
wanted to say we didn't want the directory rename which you'd first
guess would be "git mv b/x a/x" BUT: (1) a/x has unstaged entries
which will cause git-mv to fail, and (2) directory a/ didn't exist --
both of these issues had to be corrected before running git-mv.

> > +             test_tick &&
> > +             git commit -m "D"
> > +     )
> > +'
> > +
> > +test_expect_success '13e-check: directory rename detection in recursive case' '
> > +     (
> > +             cd 13e &&
> > +
> > +             git checkout --quiet D^0 &&
> > +
> > +             git -c merge.directoryRenames=conflict merge -s recursive C^0 >out 2>err &&
>
> Now we finally do the recursive merge - C and D have equally likely
> ancestors A and B, and A and B have a rename conflict.
>
> > +
> > +             test_i18ngrep ! CONFLICT out &&
> > +             test_i18ngrep ! BUG: err &&
>
> The BUG is gone. But should it not use i18ngrep? BUG() isn't localized.

Technically, yes, you're right. However, this line's purpose isn't
correctness of the test but documentation for the person reading the
testcase about what it's real original purpose was; my real test was
the "test_must_be_empty err" check I have below it, but I added this
line just to document the intent better.  I kind of like the
"CONFLICT" and "BUG" lines looking similar just so the reader of the
testcase doesn't have to try to reason through why they are different,
although I guess it does present the problem that more careful readers
like yourself might do a double take.

If folks find it more readable to use regular grep instead of
test_i18ngrep, I can change this line as well as the "core dumped"
check immediately below over to regular grep.

> > +             test_i18ngrep ! core.dumped err &&
> > +             test_must_be_empty err &&
> > +
> > +             git ls-files >paths &&
> > +             ! grep a/x paths &&
> Finally, make sure that a/x has been truly disappeared...
>
> > +             grep b/x paths
> ...and b/x is the only x left standing.


Thanks for taking a look.  :-)

  reply	other threads:[~2019-08-06 21:16 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
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 [this message]
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:
  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-BFZig73jXKkKNaz=YpT4enB3_ARQ1KTz_ttPYobkY6Bhg@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=emilyshaffer@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git