[PATCH 0/3] detecting delete/modechange conflicts

[PATCH 0/3] detecting delete/modechange conflicts

[Jeff King]

I was surprised to find that:

  # base commit
  echo base >file &&
  git add file &&
  git commit -m base &&

  # one side changes mode
  chmod +x file &&
  git commit -am executable &&

  # the other deletes the file
  git checkout -b other HEAD^ &&
  git rm file &&
  git commit -am removed &&

  git merge master

silently completes the merge, dropping the mode change. We detect
delete/modify conflicts, but not delete/modechange conflicts.  The
trivial index merge does catch it, but both the resolve and recursive
strategies resolve it silently in favor of the deletion.

After looking through the history and the list archive, I don't _think_
this was intentional, and we simply missed the case in both places. But
maybe somebody else knows something I don't. It seems like we should be
punting to the user under the general principle of stupid and safe
merges.

  [1/3]: t6031: move triple-rename test to t3030
  [2/3]: t6031: generalize for recursive and resolve strategies
  [3/3]: merge: detect delete/modechange conflict

-Peff

[PATCH 1/3] t6031: move triple-rename test to t3030

[Jeff King]

The t6031 test was introduced to check filemode handling of
merge-recursive. Much later, an unrelated test was tacked on
to look at renames and d/f conflicts. This test does not
depend on anything that happened before (it actually blows
away any existing content in the test repo). Let's move it
to t3030, where there are more related tests.

Signed-off-by: Jeff King <peff@peff.net>
---
I didn't actually look all that closely at what it does and what t3030
does to see if there is overlap, and we could simply get rid of this.
But it _definitely_ doesn't belong in t6031, so this is at least a step
forward.

 t/t3030-merge-recursive.sh | 30 ++++++++++++++++++++++++++++++
 t/t6031-merge-recursive.sh | 31 -------------------------------
 2 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 82e1854..6224187 100755
--- a/t/t3030-merge-recursive.sh
+++ b/t/t3030-merge-recursive.sh
@@ -629,5 +629,35 @@ test_expect_failure 'merge-recursive rename vs. rename/symlink' '
 	test_cmp expected actual
 '
 
+test_expect_success 'merging with triple rename across D/F conflict' '
+	git reset --hard HEAD &&
+	git checkout -b main &&
+	git rm -rf . &&
+
+	echo "just a file" >sub1 &&
+	mkdir -p sub2 &&
+	echo content1 >sub2/file1 &&
+	echo content2 >sub2/file2 &&
+	echo content3 >sub2/file3 &&
+	mkdir simple &&
+	echo base >simple/bar &&
+	git add -A &&
+	test_tick &&
+	git commit -m base &&
+
+	git checkout -b other &&
+	echo more >>simple/bar &&
+	test_tick &&
+	git commit -a -m changesimplefile &&
+
+	git checkout main &&
+	git rm sub1 &&
+	git mv sub2 sub1 &&
+	test_tick &&
+	git commit -m changefiletodir &&
+
+	test_tick &&
+	git merge other
+'
 
 test_done
diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
index 6464a16..4053bd9 100755
--- a/t/t6031-merge-recursive.sh
+++ b/t/t6031-merge-recursive.sh
@@ -53,35 +53,4 @@ test_expect_success FILEMODE 'verify executable bit on file' '
 	test -x file2
 '
 
-test_expect_success 'merging with triple rename across D/F conflict' '
-	git reset --hard HEAD &&
-	git checkout -b main &&
-	git rm -rf . &&
-
-	echo "just a file" >sub1 &&
-	mkdir -p sub2 &&
-	echo content1 >sub2/file1 &&
-	echo content2 >sub2/file2 &&
-	echo content3 >sub2/file3 &&
-	mkdir simple &&
-	echo base >simple/bar &&
-	git add -A &&
-	test_tick &&
-	git commit -m base &&
-
-	git checkout -b other &&
-	echo more >>simple/bar &&
-	test_tick &&
-	git commit -a -m changesimplefile &&
-
-	git checkout main &&
-	git rm sub1 &&
-	git mv sub2 sub1 &&
-	test_tick &&
-	git commit -m changefiletodir &&
-
-	test_tick &&
-	git merge other
-'
-
 test_done
-- 
2.6.2.481.g6ca35c3

[PATCH 2/3] t6031: generalize for recursive and resolve strategies

[Jeff King]

This script tests the filemode handling of merge-recursive,
but we do not test the same thing for merge-resolve. Let's
generalize the script a little:

  1. Break out the setup steps for each test into a separate
     snippet.

  2. For each test, run it twice; once with "-s recursive"
     and once with "-s resolve". We can avoid repeating
     ourselves by adding a function.

  3. Since we have a nice abstracted function, we can make
     our tests more thorough by testing both directions
     (change on "ours" versus "theirs").

This improves our test coverage, and will make this the
place to add more tests related to merging mode changes.

Signed-off-by: Jeff King <peff@peff.net>
---
Sadly I changed too much for this to count as a rename (mostly due to
the new indentation). Oh well, the rename diff was not especially
readable, and it is probably just as well to read the new content as a
single blob.

 t/t6031-merge-filemode.sh  | 77 ++++++++++++++++++++++++++++++++++++++++++++++
 t/t6031-merge-recursive.sh | 56 ---------------------------------
 2 files changed, 77 insertions(+), 56 deletions(-)
 create mode 100755 t/t6031-merge-filemode.sh
 delete mode 100755 t/t6031-merge-recursive.sh

diff --git a/t/t6031-merge-filemode.sh b/t/t6031-merge-filemode.sh
new file mode 100755
index 0000000..c6896e6
--- /dev/null
+++ b/t/t6031-merge-filemode.sh
@@ -0,0 +1,77 @@
+#!/bin/sh
+
+test_description='merge: handle file mode'
+. ./test-lib.sh
+
+test_expect_success 'set up mode change in one branch' '
+	: >file1 &&
+	git add file1 &&
+	git commit -m initial &&
+	git checkout -b a1 master &&
+	: >dummy &&
+	git add dummy &&
+	git commit -m a &&
+	git checkout -b b1 master &&
+	test_chmod +x file1 &&
+	git add file1 &&
+	git commit -m b1
+'
+
+do_one_mode () {
+	strategy=$1
+	us=$2
+	them=$3
+	test_expect_success "resolve single mode change ($strategy, $us)" '
+		git checkout -f $us &&
+		git merge -s $strategy $them &&
+		git ls-files -s file1 | grep ^100755
+	'
+
+	test_expect_success FILEMODE "verify executable bit on file ($strategy, $us)" '
+		test -x file1
+	'
+}
+
+do_one_mode recursive a1 b1
+do_one_mode recursive b1 a1
+do_one_mode resolve a1 b1
+do_one_mode resolve b1 a1
+
+test_expect_success 'set up mode change in both branches' '
+	git reset --hard HEAD &&
+	git checkout -b a2 master &&
+	: >file2 &&
+	H=$(git hash-object file2) &&
+	test_chmod +x file2 &&
+	git commit -m a2 &&
+	git checkout -b b2 master &&
+	: >file2 &&
+	git add file2 &&
+	git commit -m b2 &&
+	{
+		echo "100755 $H 2	file2"
+		echo "100644 $H 3	file2"
+	} >expect
+'
+
+do_both_modes () {
+	strategy=$1
+	test_expect_success "detect conflict on double mode change ($strategy)" '
+		git reset --hard &&
+		git checkout -f a2 &&
+		test_must_fail git merge -s $strategy b2 &&
+		git ls-files -u >actual &&
+		test_cmp actual expect &&
+		git ls-files -s file2 | grep ^100755
+	'
+
+	test_expect_success FILEMODE "verify executable bit on file ($strategy)" '
+		test -x file2
+	'
+}
+
+# both sides are equivalent, so no need to run both ways
+do_both_modes recursive
+do_both_modes resolve
+
+test_done
diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
deleted file mode 100755
index 4053bd9..0000000
--- a/t/t6031-merge-recursive.sh
+++ /dev/null
@@ -1,56 +0,0 @@
-#!/bin/sh
-
-test_description='merge-recursive: handle file mode'
-. ./test-lib.sh
-
-test_expect_success 'mode change in one branch: keep changed version' '
-	: >file1 &&
-	git add file1 &&
-	git commit -m initial &&
-	git checkout -b a1 master &&
-	: >dummy &&
-	git add dummy &&
-	git commit -m a &&
-	git checkout -b b1 master &&
-	test_chmod +x file1 &&
-	git add file1 &&
-	git commit -m b1 &&
-	git checkout a1 &&
-	git merge-recursive master -- a1 b1 &&
-	git ls-files -s file1 | grep ^100755
-'
-
-test_expect_success FILEMODE 'verify executable bit on file' '
-	test -x file1
-'
-
-test_expect_success 'mode change in both branches: expect conflict' '
-	git reset --hard HEAD &&
-	git checkout -b a2 master &&
-	: >file2 &&
-	H=$(git hash-object file2) &&
-	test_chmod +x file2 &&
-	git commit -m a2 &&
-	git checkout -b b2 master &&
-	: >file2 &&
-	git add file2 &&
-	git commit -m b2 &&
-	git checkout a2 &&
-	(
-		git merge-recursive master -- a2 b2
-		test $? = 1
-	) &&
-	git ls-files -u >actual &&
-	(
-		echo "100755 $H 2	file2"
-		echo "100644 $H 3	file2"
-	) >expect &&
-	test_cmp actual expect &&
-	git ls-files -s file2 | grep ^100755
-'
-
-test_expect_success FILEMODE 'verify executable bit on file' '
-	test -x file2
-'
-
-test_done
-- 
2.6.2.481.g6ca35c3

[PATCH 3/3] merge: detect delete/modechange conflict

[Jeff King]

If one side deletes a file and the other changes its
content, we notice and report a conflict. However, if
instead of changing the content, we change only the mode,
the merge does not notice (and the mode change is silently
dropped).

The trivial index merge notices the problem and correctly
leaves the conflict in the index, but both merge-recursive
and merge-one-file will silently resolve this in favor of
the deletion.  In many cases that is a sane resolution, but
we should be punting to the user whenever there is any
question. So let's detect and treat this as a conflict (in
both strategies).

Signed-off-by: Jeff King <peff@peff.net>
---
The text for merge-one-file looks really gross and unlike
our usual error messages, but it matches the similar add/add
case lower in the script. I don't know if its worth cleaning
those messages up, as I doubt many people use "-s resolve"
these days. But even if we do, that should be a separate
series.

 git-merge-one-file.sh     |  8 ++++++++
 merge-recursive.c         |  8 ++++++--
 t/t6031-merge-filemode.sh | 23 +++++++++++++++++++++++
 3 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 07dfeb8..cdc02af 100755
--- a/git-merge-one-file.sh
+++ b/git-merge-one-file.sh
@@ -38,6 +38,14 @@ case "${1:-.}${2:-.}${3:-.}" in
 # Deleted in both or deleted in one and unchanged in the other
 #
 "$1.." | "$1.$1" | "$1$1.")
+	if { test -z "$6" && test "$5" != "$7"; } ||
+	   { test -z "$7" && test "$5" != "$6"; }
+	then
+		echo "ERROR: File $4 deleted on one branch but had its" >&2
+		echo "ERROR: permissions changed on the other." >&2
+		exit 1
+	fi
+
 	if test -n "$2"
 	then
 		echo "Removing $4"
diff --git a/merge-recursive.c b/merge-recursive.c
index a5e74d8..21e680a 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1530,13 +1530,17 @@ static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)
 }
 
 static int blob_unchanged(const unsigned char *o_sha,
+			  unsigned o_mode,
 			  const unsigned char *a_sha,
+			  unsigned a_mode,
 			  int renormalize, const char *path)
 {
 	struct strbuf o = STRBUF_INIT;
 	struct strbuf a = STRBUF_INIT;
 	int ret = 0; /* assume changed for safety */
 
+	if (a_mode != o_mode)
+		return 0;
 	if (sha_eq(o_sha, a_sha))
 		return 1;
 	if (!renormalize)
@@ -1722,8 +1726,8 @@ static int process_entry(struct merge_options *o,
 	} else if (o_sha && (!a_sha || !b_sha)) {
 		/* Case A: Deleted in one */
 		if ((!a_sha && !b_sha) ||
-		    (!b_sha && blob_unchanged(o_sha, a_sha, normalize, path)) ||
-		    (!a_sha && blob_unchanged(o_sha, b_sha, normalize, path))) {
+		    (!b_sha && blob_unchanged(o_sha, o_mode, a_sha, a_mode, normalize, path)) ||
+		    (!a_sha && blob_unchanged(o_sha, o_mode, b_sha, b_mode, normalize, path))) {
 			/* Deleted in both or deleted in one and
 			 * unchanged in the other */
 			if (a_sha)
diff --git a/t/t6031-merge-filemode.sh b/t/t6031-merge-filemode.sh
index c6896e6..7d06461 100755
--- a/t/t6031-merge-filemode.sh
+++ b/t/t6031-merge-filemode.sh
@@ -74,4 +74,27 @@ do_both_modes () {
 do_both_modes recursive
 do_both_modes resolve
 
+test_expect_success 'set up delete/modechange scenario' '
+	git reset --hard &&
+	git checkout -b deletion master &&
+	git rm file1 &&
+	git commit -m deletion
+'
+
+do_delete_modechange () {
+	strategy=$1
+	us=$2
+	them=$3
+	test_expect_success "detect delete/modechange conflict ($strategy, $us)" '
+		git reset --hard &&
+		git checkout $us &&
+		test_must_fail git merge -s $strategy $them
+	'
+}
+
+do_delete_modechange recursive b1 deletion
+do_delete_modechange recursive deletion b1
+do_delete_modechange resolve b1 deletion
+do_delete_modechange resolve deletion b1
+
 test_done
-- 
2.6.2.481.g6ca35c3

Re: [PATCH 0/3] detecting delete/modechange conflicts

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> After looking through the history and the list archive, I don't _think_
> this was intentional, and we simply missed the case in both places. But
> maybe somebody else knows something I don't. It seems like we should be
> punting to the user under the general principle of stupid and safe
> merges.

Yes, I do not recall ever discussing and agreeing with Linus that we
should resolve to deletion over mode change, and I agree that it
would be very likely that this never came up in practice simply
because in real life removal is already rare, mode change is rarer,
and these happening to the same path in the same timeperiod to
matter in merges is even more rare.

We should definitely signal a conflict.

>   [1/3]: t6031: move triple-rename test to t3030
>   [2/3]: t6031: generalize for recursive and resolve strategies
>   [3/3]: merge: detect delete/modechange conflict
>
> -Peff

Re: [PATCH 0/3] detecting delete/modechange conflicts

[Jeff King]

On Mon, Oct 26, 2015 at 02:46:42PM -0700, Junio C Hamano wrote:

> > After looking through the history and the list archive, I don't _think_
> > this was intentional, and we simply missed the case in both places. But
> > maybe somebody else knows something I don't. It seems like we should be
> > punting to the user under the general principle of stupid and safe
> > merges.
> 
> Yes, I do not recall ever discussing and agreeing with Linus that we
> should resolve to deletion over mode change, and I agree that it
> would be very likely that this never came up in practice simply
> because in real life removal is already rare, mode change is rarer,
> and these happening to the same path in the same timeperiod to
> matter in merges is even more rare.
> 
> We should definitely signal a conflict.

Thanks, that matches my thinking exactly.

BTW, this came up because libgit2 does signal the conflict, and we are
regression-testing a switch from merge-resolve over to libgit2 to power
GitHub's "merge" button. Run it on enough test cases and you will find
one of everything. :)

-Peff