git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Emily Shaffer <emilyshaffer@google.com>,
	Elijah Newren <newren@gmail.com>
Subject: [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case
Date: Mon,  5 Aug 2019 15:33:50 -0700
Message-ID: <20190805223350.27504-1-newren@gmail.com> (raw)
In-Reply-To: <20190726220928.GG113966@google.com>

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 <emilyshaffer@google.com>
Signed-off-by: Elijah Newren <newren@gmail.com>
---

I really should introduce constants like
  DETECT_DIRECTORY_RENAMES_NEVER    = 0
  DETECT_DIRECTORY_RENAMES_CONFLICT = 1
  DETECT_DIRECTORY_RENAMES_YES      = 2
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.

(This fix does merge down cleanly to master, next, and pu.)

 merge-recursive.c                   |   3 +-
 t/t6043-merge-rename-directories.sh | 111 ++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index d2e380b7ed..c7691d9b54 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2856,7 +2856,8 @@ static int detect_and_process_renames(struct merge_options *opt,
 	head_pairs = get_diffpairs(opt, common, head);
 	merge_pairs = get_diffpairs(opt, common, merge);
 
-	if (opt->detect_directory_renames) {
+	if ((opt->detect_directory_renames == 2) ||
+	    (opt->detect_directory_renames == 1 && !opt->call_depth)) {
 		dir_re_head = get_directory_renames(head_pairs);
 		dir_re_merge = get_directory_renames(merge_pairs);
 
diff --git a/t/t6043-merge-rename-directories.sh b/t/t6043-merge-rename-directories.sh
index 50b7543483..c966147d5d 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -4403,4 +4403,115 @@ test_expect_success '13d-check(info): messages for rename/rename(1to1) via dual
 	)
 '
 
+# 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
+#   Expected: b/{z,y,x}  (sort of; see below for why this might not be expected)
+#
+#   NOTES: 'X' represents a virtual merge base.  With the default of
+#          directory rename detection yielding conflicts, merging A and B
+#          results in a conflict complaining about whether 'x' should be
+#          under 'a/' or 'b/'.  However, when creating the virtual merge
+#          base 'X', since virtual merge bases need to be written out as a
+#          tree, we cannot have a conflict, so some resolution has to be
+#          picked.
+#
+#          In choosing the right resolution, it's worth noting here that
+#          commits C & D are merges of A & B that choose different
+#          locations for 'x' (i.e. they resolve the conflict differently),
+#          and so it would be nice when merging C & D if git could detect
+#          this difference of opinion and report a conflict.  But the only
+#          way to do so that I can think of would be to have the virtual
+#          merge base place 'x' in some directory other than either 'a/' or
+#          'b/', which seems a little weird -- especially since it'd result
+#          in a rename/rename(1to2) conflict with a source path that never
+#          existed in any version.
+#
+#          So, for now, when directory rename detection is set to
+#          'conflict' just avoid doing directory rename detection at all in
+#          the recursive case.  This will not allow us to detect a conflict
+#          in the outer merge for this special kind of setup, but it at
+#          least avoids hitting a BUG().
+#
+test_expect_success '13e-setup: directory rename detection in recursive case' '
+	test_create_repo 13e &&
+	(
+		cd 13e &&
+
+		mkdir a &&
+		echo z >a/z &&
+		echo y >a/y &&
+		git add a &&
+		test_tick &&
+		git commit -m "O" &&
+
+		git branch O &&
+		git branch A &&
+		git branch B &&
+
+		git checkout A &&
+		git mv a/ b/ &&
+		test_tick &&
+		git commit -m "A" &&
+
+		git checkout B &&
+		echo x >a/x &&
+		git add a &&
+		test_tick &&
+		git commit -m "B" &&
+
+		git branch C A &&
+		git branch D B &&
+
+		git checkout C &&
+		test_must_fail git -c merge.directoryRenames=conflict merge B &&
+		git add b/x &&
+		test_tick &&
+		git commit -m "C" &&
+
+
+		git checkout D &&
+		test_must_fail git -c merge.directoryRenames=conflict merge A &&
+		git add b/x &&
+		mkdir a &&
+		git mv b/x a/x &&
+		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 &&
+
+		test_i18ngrep ! CONFLICT out &&
+		test_i18ngrep ! BUG: err &&
+		test_i18ngrep ! core.dumped err &&
+		test_must_be_empty err &&
+
+		git ls-files >paths &&
+		! grep a/x paths &&
+		grep b/x paths
+	)
+'
+
 test_done
-- 
2.22.0.246.g5ddf3d502a


  reply	other threads:[~2019-08-05 22:33 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 ` Elijah Newren [this message]
2019-08-06 16:57   ` [PATCH 1/1] merge-recursive: avoid directory rename detection in recursive case 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
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=20190805223350.27504-1-newren@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