Skip to contentSkip to conversation

diff --no-index: fix -R with file/directory conflicts

PATCH In review Haokai Ding via GitGitGadget 4 participants 4 messages Sep 18 – Sep 21

lore
Share & tools
lore

Haokai Ding via GitGitGadget

author
#0 lore raw

PATCH 1/1 diff --no-index: fix -R with file/directory conflicts

From: Haokai Ding <[email protected]>

When a path is a file on one side and a directory on the other, queue_diff() queues the file separately before recursing into the directory. This early queue entry bypasses the reverse_diff handling used for ordinary files.

As a result, comparing directories d and e where d/sub is a file and e/sub/file is another file reports both paths as deleted with -R. Reversing the operands reports both paths as added instead.

Swap the filespecs of the early queue entry when reverse_diff is set, so that -R reverses the file change as well as the directory contents. Add regression tests for both directions.

Signed-off-by Haokai Ding <[email protected]>

---

    diff --no-index: fix -R with file/directory conflicts

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2232%2FHaokaiDing%2Ffix%2Fno-index-reverse-file-directory-mail-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2232/HaokaiDing/fix/no-index-reverse-file-directory-mail-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2232

 diff-no-index.c          |  2 ++
 t/t4053-diff-no-index.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)
diff-no-index.c +2 -0
index f320424f05..a9151cc27a 100644
@@ -188,6 +188,8 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,
 			mode1 = 0;
 		}
 		/* emit that file */
+		if (o->flags.reverse_diff)
+			SWAP(d1, d2);
 		diff_queue(&diff_queued_diff, d1, d2);
 
 		/* and then let the entire directory be created or deleted */
t/t4053-diff-no-index.sh +18 -0
index 8e0394cf5a..c3c703f4f4 100755
@@ -120,6 +120,24 @@ test_expect_success 'turning a file into a directory' '
 	)
 '
 
+test_expect_success 'reverse diff when turning a file into a directory' '
+	(
+		cd non/git &&
+		printf "A\td/sub\nD\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status d e >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'reverse diff when turning a directory into a file' '
+	(
+		cd non/git &&
+		printf "D\td/sub\nA\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status e d >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'diff from repo subdir shows real paths (explicit)' '
 	echo "diff --git a/../../non/git/a b/../../non/git/b" >expect &&
 	test_expect_code 1 \

base-commit: d38352cd43ab9745686d697872408bc3249a153f

Signature · 1 line
gitgitgadget
#1 lore raw
Quoted 32 lines · Haokai Ding via GitGitGadget

From: Haokai Ding <[email protected]>

When a path is a file on one side and a directory on the other, queue_diff() queues the file separately before recursing into the directory. This early queue entry bypasses the reverse_diff handling used for ordinary files.

As a result, comparing directories d and e where d/sub is a file and e/sub/file is another file reports both paths as deleted with -R. Reversing the operands reports both paths as added instead.

Swap the filespecs of the early queue entry when reverse_diff is set, so that -R reverses the file change as well as the directory contents. Add regression tests for both directions.

Signed-off-by Haokai Ding <[email protected]>

---

    diff --no-index: fix -R with file/directory conflicts

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2232%2FHaokaiDing%2Ffix%2Fno-index-reverse-file-directory-mail-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2232/HaokaiDing/fix/no-index-reverse-file-directory-mail-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2232

 diff-no-index.c          |  2 ++
 t/t4053-diff-no-index.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)
diff-no-index.c +0 -0
index f320424f05..a9151cc27a 100644

diff-no-index.c static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop, · line 192

Quoted 5 lines
diff-no-index.c +2 -0
@@ -188,6 +188,8 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,
 			mode1 = 0;
 		}
 		/* emit that file */
+		if (o->flags.reverse_diff)
+			SWAP(d1, d2);

Good find!

Quoted 17 lines
diff-no-index.c +0 -0
 		diff_queue(&diff_queued_diff, d1, d2);
 
 		/* and then let the entire directory be created or deleted */
t/t4053-diff-no-index.sh +6 -0
index 8e0394cf5a..c3c703f4f4 100755
@@ -120,6 +120,24 @@ test_expect_success 'turning a file into a directory' '
 	)
 '
 
+test_expect_success 'reverse diff when turning a file into a directory' '
+	(
+		cd non/git &&
+		printf "A\td/sub\nD\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status d e >actual &&
+		test_cmp expect actual

t/t4053-diff-no-index.sh test_expect_success 'turning a file into a directory' ' · line 138

Quoted 10 lines
t/t4053-diff-no-index.sh +10 -0
+	)
+'
+
+test_expect_success 'reverse diff when turning a directory into a file' '
+	(
+		cd non/git &&
+		printf "D\td/sub\nA\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status e d >actual &&
+		test_cmp expect actual
+	)

Looks good. I like the explicit exit code expectation.

Quoted 7 lines
t/t4053-diff-no-index.sh +2 -0
+'
+
 test_expect_success 'diff from repo subdir shows real paths (explicit)' '
 	echo "diff --git a/../../non/git/a b/../../non/git/b" >expect &&
 	test_expect_code 1 \

base-commit: d38352cd43ab9745686d697872408bc3249a153f

Reviewed-by on v1 1/1
#2 lore raw
In reply to Haokai Ding via GitGitGadget
Quoted 11 lines · Haokai Ding via GitGitGadget

From: Haokai Ding <[email protected]>

When a path is a file on one side and a directory on the other, queue_diff() queues the file separately before recursing into the directory. This early queue entry bypasses the reverse_diff handling used for ordinary files.

As a result, comparing directories d and e where d/sub is a file and e/sub/file is another file reports both paths as deleted with -R. Reversing the operands reports both paths as added instead.

Quoted 6 lines

Swap the filespecs of the early queue entry when reverse_diff is set, so that -R reverses the file change as well as the directory contents. Add regression tests for both directions.

Signed-off-by Haokai Ding <[email protected]>

---

Thanks for fixing!

Reviewed-by Jacob Keller <[email protected]>
Quoted 54 lines
    diff --no-index: fix -R with file/directory conflicts

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2232%2FHaokaiDing%2Ffix%2Fno-index-reverse-file-directory-mail-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2232/HaokaiDing/fix/no-index-reverse-file-directory-mail-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2232

 diff-no-index.c          |  2 ++
 t/t4053-diff-no-index.sh | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+)
diff-no-index.c +2 -0
index f320424f05..a9151cc27a 100644
@@ -188,6 +188,8 @@ static int queue_diff(struct diff_options *o, const struct git_hash_algo *algop,
 			mode1 = 0;
 		}
 		/* emit that file */
+		if (o->flags.reverse_diff)
+			SWAP(d1, d2);
 		diff_queue(&diff_queued_diff, d1, d2);
 
 		/* and then let the entire directory be created or deleted */
t/t4053-diff-no-index.sh +18 -0
index 8e0394cf5a..c3c703f4f4 100755
@@ -120,6 +120,24 @@ test_expect_success 'turning a file into a directory' '
 	)
 '
 
+test_expect_success 'reverse diff when turning a file into a directory' '
+	(
+		cd non/git &&
+		printf "A\td/sub\nD\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status d e >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'reverse diff when turning a directory into a file' '
+	(
+		cd non/git &&
+		printf "D\td/sub\nA\te/sub/file\n" >expect &&
+		test_expect_code 1 git diff --no-index -R --name-status e d >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'diff from repo subdir shows real paths (explicit)' '
 	echo "diff --git a/../../non/git/a b/../../non/git/b" >expect &&
 	test_expect_code 1 \

base-commit: d38352cd43ab9745686d697872408bc3249a153f

#3 lore raw
Quoted 16 lines · Jacob Keller
Quoted 15 lines · Haokai Ding via GitGitGadget

From: Haokai Ding <[email protected]>

When a path is a file on one side and a directory on the other, queue_diff() queues the file separately before recursing into the directory. This early queue entry bypasses the reverse_diff handling used for ordinary files.

As a result, comparing directories d and e where d/sub is a file and e/sub/file is another file reports both paths as deleted with -R. Reversing the operands reports both paths as added instead.

Swap the filespecs of the early queue entry when reverse_diff is set, so that -R reverses the file change as well as the directory contents. Add regression tests for both directions.

Quoted 6 lines
Quoted 2 lines
Signed-off-by Haokai Ding <[email protected]>

---

Thanks for fixing!

Reviewed-by Jacob Keller <[email protected]>

Thanks, both. Will queue.