Merge branch 'jk/delete-modechange-conflict' into maint

Merging a branch that removes a path and another that changes the
mode bits on the same path should have conflicted at the path, but
it didn't and silently favoured the removal.

* jk/delete-modechange-conflict:
  merge: detect delete/modechange conflict
  t6031: generalize for recursive and resolve strategies
  t6031: move triple-rename test to t3030

5 files changed, 144 insertions, 89 deletions

diff --git a/git-merge-one-file.sh b/git-merge-one-file.sh
index 07dfeb8df4..cdc02af517 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 44d85bea4b..b446df9c1d 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1531,13 +1531,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)
@@ -1723,8 +1727,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/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh
index 82e18548c3..6224187632 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-filemode.sh b/t/t6031-merge-filemode.sh
new file mode 100755
index 0000000000..7d06461f13
--- /dev/null
+++ b/t/t6031-merge-filemode.sh
@@ -0,0 +1,100 @@
+#!/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_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
diff --git a/t/t6031-merge-recursive.sh b/t/t6031-merge-recursive.sh
deleted file mode 100755
index 6464a16a19..0000000000
--- a/t/t6031-merge-recursive.sh
+++ /dev/null
@@ -1,87 +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_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