[PATCH] combine-diff: honor --relative when printing paths

[PATCH] combine-diff: honor --relative when printing paths

[Muhammed Dilshad A]

During a merge conflict, "git diff --relative" in a subdirectory still
prints paths from the repository root. Combined raw output does the same.

Strip the requested prefix when printing these paths. Keep the original
paths for reading files from the working tree.

Make the known failure in t4045 a passing test, and cover explicit
prefixes, --no-relative, raw and NUL-separated output, quoted filenames
and renamed parent paths.

Signed-off-by: Muhammed Dilshad A <dilsheddilu123@gmail.com>
---
 combine-diff.c           | 19 +++++++++---
 t/t4038-diff-combined.sh | 22 ++++++++++++++
 t/t4045-diff-relative.sh | 62 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 98 insertions(+), 5 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 717d537688..78d2852cce 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -902,6 +902,14 @@ static void reuse_combine_diff(struct sline *sline, unsigned long cnt,
 	sline->p_lno[i] = sline->p_lno[j];
 }
 
+static const char *strip_relative_prefix(const struct diff_options *opt,
+					const char *path)
+{
+	if (opt->prefix && skip_prefix(path, opt->prefix, &path) && *path == '/')
+		path++;
+	return path;
+}
+
 static void dump_quoted_path(const char *head,
 			     const char *prefix,
 			     const char *path,
@@ -932,6 +940,7 @@ static void show_combined_header(struct combine_diff_path *elem,
 	const char *b_prefix = opt->b_prefix ? opt->b_prefix : "b/";
 	const char *c_meta = diff_get_color_opt(opt, DIFF_METAINFO);
 	const char *c_reset = diff_get_color_opt(opt, DIFF_RESET);
+	const char *name = strip_relative_prefix(opt, elem->path);
 	const char *abb;
 	int added = 0;
 	int deleted = 0;
@@ -942,7 +951,7 @@ static void show_combined_header(struct combine_diff_path *elem,
 		show_log(rev);
 
 	dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
-			 "", elem->path, line_prefix, c_meta, c_reset);
+			 "", name, line_prefix, c_meta, c_reset);
 	printf("%s%sindex ", line_prefix, c_meta);
 	for (i = 0; i < num_parent; i++) {
 		abb = repo_find_unique_abbrev(the_repository,
@@ -987,6 +996,7 @@ static void show_combined_header(struct combine_diff_path *elem,
 			const char *path = elem->parent[i].path ?
 					   elem->parent[i].path :
 					   elem->path;
+			path = strip_relative_prefix(opt, path);
 			if (elem->parent[i].status == DIFF_STATUS_ADDED)
 				dump_quoted_path("--- ", "", "/dev/null",
 						 line_prefix, c_meta, c_reset);
@@ -999,14 +1009,14 @@ static void show_combined_header(struct combine_diff_path *elem,
 			dump_quoted_path("--- ", "", "/dev/null",
 					 line_prefix, c_meta, c_reset);
 		else
-			dump_quoted_path("--- ", a_prefix, elem->path,
+			dump_quoted_path("--- ", a_prefix, name,
 					 line_prefix, c_meta, c_reset);
 	}
 	if (deleted)
 		dump_quoted_path("+++ ", "", "/dev/null",
 				 line_prefix, c_meta, c_reset);
 	else
-		dump_quoted_path("+++ ", b_prefix, elem->path,
+		dump_quoted_path("+++ ", b_prefix, name,
 				 line_prefix, c_meta, c_reset);
 }
 
@@ -1270,9 +1280,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 			const char *path = p->parent[i].path ?
 					   p->parent[i].path :
 					   p->path;
+			path = strip_relative_prefix(opt, path);
 			write_name_quoted(path, stdout, inter_name_termination);
 		}
-	write_name_quoted(p->path, stdout, line_termination);
+	write_name_quoted(strip_relative_prefix(opt, p->path), stdout, line_termination);
 }
 
 /*
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index e11b711388..2575c06360 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -492,6 +492,28 @@ test_expect_success '--combined-all-paths and --cc' '
 	test_cmp expect actual
 '
 
+test_expect_success '--combined-all-paths and --raw with relative renamed paths' '
+	cat <<-EOF >expect &&
+	::100644 100644 100644 $side1cf $side2cf $mergedf RR	side1c	side2c	merged
+	EOF
+	git diff-tree -c -M --raw --combined-all-paths \
+		--relative=filename- HEAD >actual.tmp &&
+	sed 1d <actual.tmp >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--combined-all-paths and --cc with relative renamed paths' '
+	cat <<-\EOF >expect &&
+	--- a/side1c
+	--- a/side2c
+	+++ b/merged
+	EOF
+	git diff-tree --cc -M --combined-all-paths \
+		--relative=filename- HEAD >actual.tmp &&
+	grep ^[-+][-+][-+] <actual.tmp >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success FUNNYNAMES 'setup for --combined-all-paths with funny names' '
 	git branch side1d &&
 	git branch side2d &&
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 167be0bdcc..f105ddcfd1 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -223,7 +223,7 @@ test_expect_success 'diff --relative --name-only with change in subdir' '
 	test_cmp expected out
 '
 
-test_expect_failure 'diff --relative with change in subdir' '
+test_expect_success 'diff --relative with change in subdir' '
 	git switch br3 &&
 	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
 	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
@@ -242,6 +242,44 @@ test_expect_failure 'diff --relative with change in subdir' '
 	++>>>>>>> br1
 	EOF
 	git -C subdir diff --relative >out &&
+	test_cmp expected out &&
+	git diff --relative=subdir >out &&
+	test_cmp expected out &&
+	git diff --relative=subdir/ >out &&
+	test_cmp expected out &&
+	sed "s,file0,dir/file0,g" expected >expected-short &&
+	git diff --relative=sub >out &&
+	test_cmp expected-short out &&
+	sed "s,file0,subdir/file0,g" expected >expected-full &&
+	git diff --relative=subdir --no-relative -- subdir/file0 >out &&
+	test_cmp expected-full out
+'
+
+test_expect_success 'combined raw diff with relative paths' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\tfile0\n" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git -C subdir diff --cc --raw --relative >out &&
+	test_cmp expected out &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\0file0\0" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git diff --cc --raw -z --relative=subdir >out &&
+	test_cmp expected out
+'
+
+test_expect_success 'combined raw diff lists all relative paths' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\tfile0\tfile0\tfile0\n" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git diff --cc --raw --combined-all-paths --relative=subdir >out &&
 	test_cmp expected out
 '
 
@@ -254,4 +292,26 @@ test_expect_success 'diff --relative --cached with change in subdir' '
 	test_cmp expected out
 '
 
+test_expect_success FUNNYNAMES 'combined diff quotes relative paths' '
+	test_create_repo quoted &&
+	(
+		cd quoted &&
+		mkdir subdir &&
+		test_commit --no-tag base "subdir/quoted\"file" base &&
+		git switch -c side &&
+		test_commit --no-tag side "subdir/quoted\"file" side &&
+		git switch -c other HEAD^ &&
+		test_commit --no-tag other "subdir/quoted\"file" other &&
+		test_must_fail git merge side &&
+		cat >expected <<-\EOF &&
+		diff --cc "quoted\"file"
+		--- "a/quoted\"file"
+		+++ "b/quoted\"file"
+		EOF
+		git -C subdir diff --relative >out &&
+		sed -n "/^diff --cc /p; /^--- /p; /^+++ /p" out >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done

base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.55.0

Re: [PATCH] combine-diff: honor --relative when printing paths

[Junio C Hamano]

Muhammed Dilshad A <dilsheddilu123@gmail.com> writes:

> +static const char *strip_relative_prefix(const struct diff_options *opt,
> +					const char *path)
> +{
> +	if (opt->prefix && skip_prefix(path, opt->prefix, &path) && *path == '/')
> +		path++;
> +	return path;
> +}

That's quite a long line.  Read about coding guidelines in our
Documentation/ directory.

Also, do callers guarantee that path may have only up to one
trailing slashes and never two or more?

> @@ -932,6 +940,7 @@ static void show_combined_header(struct combine_diff_path *elem,
>  	const char *b_prefix = opt->b_prefix ? opt->b_prefix : "b/";
>  	const char *c_meta = diff_get_color_opt(opt, DIFF_METAINFO);
>  	const char *c_reset = diff_get_color_opt(opt, DIFF_RESET);
> +	const char *name = strip_relative_prefix(opt, elem->path);
>  	const char *abb;
>  	int added = 0;
>  	int deleted = 0;
> @@ -942,7 +951,7 @@ static void show_combined_header(struct combine_diff_path *elem,
>  		show_log(rev);
>  
>  	dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
> -			 "", elem->path, line_prefix, c_meta, c_reset);
> +			 "", name, line_prefix, c_meta, c_reset);
>  	printf("%s%sindex ", line_prefix, c_meta);
>  	for (i = 0; i < num_parent; i++) {
>  		abb = repo_find_unique_abbrev(the_repository,
> @@ -987,6 +996,7 @@ static void show_combined_header(struct combine_diff_path *elem,
>  			const char *path = elem->parent[i].path ?
>  					   elem->parent[i].path :
>  					   elem->path;
> +			path = strip_relative_prefix(opt, path);

When a rename is involved (e.g., originally the contents was in
here/file we have made our changes in place, while the other side
moved the file to there/file and made changes there, these were
matched up and are shown as a merge into here/file.  Wouldn't a
elem->parent[].path point at here/file while another points at
there/file in such a case?  What should happen when our prefix is in
"here/"?  I know "here/file" should become "file", but what about
"there/file" that they bring into the picture?

Not striping anything does give consistent result and would not
mislead the readers as long as they understand --relative is
ignored.  Contrasting to that, "we strip if the path is inside our
prefix, but otherwise we give full path" would give ambiguous
output, wouldn't it?

[PATCH v2 0/2] combine-diff: honor relative paths consistently

[Muhammed Dilshad A]

Hi Junio,

Thanks for the review. I checked the path handling and reproduced the
cross-directory rename case you described.

With plain -M, the outside source is filtered before rename detection.
With --follow, though, Git searches for renames with an unfiltered tree
comparison, so an outside parent name can reach the combined output.
The revised patch prints there/file as ../there/file when the prefix is
here/. All displayed names then use the same base. The tests cover both
discovery paths, including raw and NUL-separated output.

The index rejects repeated separators, but tree entry parsing does not
enforce the same check. The helper now skips all separators at the prefix
boundary, so it does not rely on there being only one. Explicit prefix
arguments stay literal, matching ordinary diff's filtering behavior.
I also wrapped the added C lines to fit the coding guidelines.

While checking the two discovery paths, I found that the fast multi-tree
scan bypasses the relative-prefix filter entirely. Patch 2 fixes that
separately and adds tests for outside paths and repeated separators in
an explicit prefix.

Changes since v1:

* Use relative_path() for parent names outside the prefix while keeping
  ordinary diff's literal-prefix behavior for matching names.
* Preserve /dev/null, skip all boundary separators, and wrap long lines.
* Add cross-directory rename tests and the separate fast-scan fix.

The developer build with SANITIZE=leak succeeds. The affected suites pass
all 77 normal tests with SHA-1 and SHA-256. A separate run with
LSAN_OPTIONS=detect_leaks=1 also passes without a leak report. The existing
three-parent coalescing failure in t4038 remains an expected failure.

Muhammed Dilshad A (2):
  combine-diff: honor --relative when printing paths
  combine-diff: filter the fast scan by the relative prefix

 combine-diff.c           |  70 ++++++++++++++++++---
 t/t4038-diff-combined.sh | 130 +++++++++++++++++++++++++++++++++++++++
 t/t4045-diff-relative.sh |  62 ++++++++++++++++++-
 3 files changed, 251 insertions(+), 11 deletions(-)


base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd
-- 
2.55.0

[PATCH v2 1/2] combine-diff: honor --relative when printing paths

[Muhammed Dilshad A]

During a merge conflict, "git diff --relative" in a subdirectory still
prints repository-root paths in combined output. Combined raw output has
the same problem.

Print all names relative to the requested prefix, keeping the stored
paths for reading file contents. Preserve ordinary diff's literal-prefix
behavior for matching names, and use relative_path() for renamed parent
names outside the prefix. With --relative=here/, there/file is shown as
../there/file instead of mixing root-relative and relative names.

The outside-parent case occurs with --follow, which searches for renames
using an unfiltered tree comparison. Add cross-directory tests for that
case, including raw and NUL-separated output. Keep /dev/null unchanged
and skip all separators at the prefix boundary.

Turn the known failure in t4045 into a passing test and retain coverage
for explicit prefixes, --no-relative and quoted filenames.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Muhammed Dilshad A <dilsheddilu123@gmail.com>
---
 combine-diff.c           |  52 +++++++++++++++----
 t/t4038-diff-combined.sh | 109 +++++++++++++++++++++++++++++++++++++++
 t/t4045-diff-relative.sh |  62 +++++++++++++++++++++-
 3 files changed, 212 insertions(+), 11 deletions(-)

diff --git a/combine-diff.c b/combine-diff.c
index 717d537688..d615471717 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -2,6 +2,7 @@
 #define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
+#include "abspath.h"
 #include "odb.h"
 #include "commit.h"
 #include "convert.h"
@@ -10,6 +11,7 @@
 #include "environment.h"
 #include "hex.h"
 #include "object-name.h"
+#include "path.h"
 #include "quote.h"
 #include "xdiff-interface.h"
 #include "xdiff/xmacros.h"
@@ -902,19 +904,39 @@ static void reuse_combine_diff(struct sline *sline, unsigned long cnt,
 	sline->p_lno[i] = sline->p_lno[j];
 }
 
-static void dump_quoted_path(const char *head,
+static const char *relative_combined_path(const struct diff_options *opt,
+					 const char *path, struct strbuf *sb)
+{
+	if (!opt->prefix || is_absolute_path(path))
+		return path;
+
+	/* Match ordinary diff's literal-prefix handling inside the prefix. */
+	if (skip_prefix(path, opt->prefix, &path)) {
+		while (*path == '/')
+			path++;
+		return path;
+	}
+
+	return relative_path(path, opt->prefix, sb);
+}
+
+static void dump_quoted_path(const struct diff_options *opt,
+			     const char *head,
 			     const char *prefix,
 			     const char *path,
 			     const char *line_prefix,
 			     const char *c_meta, const char *c_reset)
 {
 	static struct strbuf buf = STRBUF_INIT;
+	struct strbuf relative = STRBUF_INIT;
 
+	path = relative_combined_path(opt, path, &relative);
 	strbuf_reset(&buf);
 	strbuf_addstr(&buf, line_prefix);
 	strbuf_addstr(&buf, c_meta);
 	strbuf_addstr(&buf, head);
 	quote_two_c_style(&buf, prefix, path, 0);
+	strbuf_release(&relative);
 	strbuf_addstr(&buf, c_reset);
 	puts(buf.buf);
 }
@@ -941,7 +963,7 @@ static void show_combined_header(struct combine_diff_path *elem,
 	if (rev->loginfo && !rev->no_commit_id)
 		show_log(rev);
 
-	dump_quoted_path(dense ? "diff --cc " : "diff --combined ",
+	dump_quoted_path(opt, dense ? "diff --cc " : "diff --combined ",
 			 "", elem->path, line_prefix, c_meta, c_reset);
 	printf("%s%sindex ", line_prefix, c_meta);
 	for (i = 0; i < num_parent; i++) {
@@ -988,25 +1010,25 @@ static void show_combined_header(struct combine_diff_path *elem,
 					   elem->parent[i].path :
 					   elem->path;
 			if (elem->parent[i].status == DIFF_STATUS_ADDED)
-				dump_quoted_path("--- ", "", "/dev/null",
+				dump_quoted_path(opt, "--- ", "", "/dev/null",
 						 line_prefix, c_meta, c_reset);
 			else
-				dump_quoted_path("--- ", a_prefix, path,
+				dump_quoted_path(opt, "--- ", a_prefix, path,
 						 line_prefix, c_meta, c_reset);
 		}
 	} else {
 		if (added)
-			dump_quoted_path("--- ", "", "/dev/null",
+			dump_quoted_path(opt, "--- ", "", "/dev/null",
 					 line_prefix, c_meta, c_reset);
 		else
-			dump_quoted_path("--- ", a_prefix, elem->path,
+			dump_quoted_path(opt, "--- ", a_prefix, elem->path,
 					 line_prefix, c_meta, c_reset);
 	}
 	if (deleted)
-		dump_quoted_path("+++ ", "", "/dev/null",
+		dump_quoted_path(opt, "+++ ", "", "/dev/null",
 				 line_prefix, c_meta, c_reset);
 	else
-		dump_quoted_path("+++ ", b_prefix, elem->path,
+		dump_quoted_path(opt, "+++ ", b_prefix, elem->path,
 				 line_prefix, c_meta, c_reset);
 }
 
@@ -1225,6 +1247,16 @@ static void show_patch_diff(struct combine_diff_path *elem, int num_parent,
 	free(sline);
 }
 
+static void write_combined_path(const struct diff_options *opt,
+				const char *path, int termination)
+{
+	struct strbuf relative = STRBUF_INIT;
+
+	path = relative_combined_path(opt, path, &relative);
+	write_name_quoted(path, stdout, termination);
+	strbuf_release(&relative);
+}
+
 static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct rev_info *rev)
 {
 	struct diff_options *opt = &rev->diffopt;
@@ -1270,9 +1302,9 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
 			const char *path = p->parent[i].path ?
 					   p->parent[i].path :
 					   p->path;
-			write_name_quoted(path, stdout, inter_name_termination);
+			write_combined_path(opt, path, inter_name_termination);
 		}
-	write_name_quoted(p->path, stdout, line_termination);
+	write_combined_path(opt, p->path, line_termination);
 }
 
 /*
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index e11b711388..21eeb4fbcb 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -492,6 +492,28 @@ test_expect_success '--combined-all-paths and --cc' '
 	test_cmp expect actual
 '
 
+test_expect_success '--combined-all-paths and --raw with relative renamed paths' '
+	cat <<-EOF >expect &&
+	::100644 100644 100644 $side1cf $side2cf $mergedf RR	side1c	side2c	merged
+	EOF
+	git diff-tree -c -M --raw --combined-all-paths \
+		--relative=filename- HEAD >actual.tmp &&
+	sed 1d <actual.tmp >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success '--combined-all-paths and --cc with relative renamed paths' '
+	cat <<-\EOF >expect &&
+	--- a/side1c
+	--- a/side2c
+	+++ b/merged
+	EOF
+	git diff-tree --cc -M --combined-all-paths \
+		--relative=filename- HEAD >actual.tmp &&
+	grep ^[-+][-+][-+] <actual.tmp >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success FUNNYNAMES 'setup for --combined-all-paths with funny names' '
 	git branch side1d &&
 	git branch side2d &&
@@ -543,4 +565,91 @@ test_expect_success FUNNYNAMES '--combined-all-paths and --cc and funny names' '
 	test_cmp expect actual
 '
 
+test_expect_success 'setup rename across relative prefix' '
+	test_create_repo cross-prefix &&
+	(
+		cd cross-prefix &&
+		mkdir here there &&
+		test_seq 1 8 >here/file &&
+		echo base >there/extra &&
+		git add here/file there/extra &&
+		git commit -m base &&
+		git tag base &&
+		git switch -c ours &&
+		test_seq 1 10 >here/file &&
+		echo ours >there/extra &&
+		git add here/file there/extra &&
+		git commit -m ours &&
+		git switch -c theirs base &&
+		git mv here/file there/file &&
+		test_seq 1 9 >there/file &&
+		echo ten >>there/file &&
+		echo theirs >there/extra &&
+		git add there/file there/extra &&
+		git commit -m theirs &&
+		git switch ours &&
+		test_must_fail git merge --no-commit theirs &&
+		git rm -f there/file &&
+		mkdir -p here &&
+		test_seq 1 9 >here/file &&
+		echo ten >>here/file &&
+		echo eleven >>here/file &&
+		echo merged >there/extra &&
+		git add here/file there/extra &&
+		git commit -m merged
+	)
+'
+
+test_expect_success 'combined relative diff excludes an outside rename source' '
+	cat >expect <<-\EOF &&
+	diff --cc file
+	--- a/file
+	--- /dev/null
+	+++ b/file
+	EOF
+	git -C cross-prefix diff-tree --no-commit-id --cc -M \
+		--combined-all-paths --relative=here/ HEAD >actual.tmp &&
+	sed -n "/^diff --cc /p; /^--- /p; /^+++ /p" actual.tmp >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'combined raw relative diff excludes an outside rename source' '
+	ours_oid=$(git -C cross-prefix rev-parse HEAD^:here/file) &&
+	merged_oid=$(git -C cross-prefix rev-parse HEAD:here/file) &&
+	printf "::100644 000000 100644 %s %s %s MA\tfile\tfile\tfile\n" \
+		"$ours_oid" "$ZERO_OID" "$merged_oid" >expect &&
+	git -C cross-prefix diff-tree --no-commit-id -c -M --raw \
+		--combined-all-paths --relative=here/ HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'combined relative diff follows an outside rename source' '
+	cat >expect <<-\EOF &&
+	diff --cc file
+	--- a/file
+	--- a/../there/file
+	+++ b/file
+	EOF
+	git -C cross-prefix diff-tree --no-commit-id --cc -M --follow \
+		--combined-all-paths --relative=here/ HEAD -- here/file >actual.tmp &&
+	sed -n "/^diff --cc /p; /^--- /p; /^+++ /p" actual.tmp >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'combined raw relative diff follows an outside rename source' '
+	ours_oid=$(git -C cross-prefix rev-parse HEAD^:here/file) &&
+	theirs_oid=$(git -C cross-prefix rev-parse HEAD^2:there/file) &&
+	merged_oid=$(git -C cross-prefix rev-parse HEAD:here/file) &&
+	printf "::100644 100644 100644 %s %s %s MR\tfile\t../there/file\tfile\n" \
+		"$ours_oid" "$theirs_oid" "$merged_oid" >expect &&
+	git -C cross-prefix diff-tree --no-commit-id -c -M --follow --raw \
+		--combined-all-paths --relative=here/ HEAD -- here/file >actual &&
+	test_cmp expect actual &&
+	printf "::100644 100644 100644 %s %s %s MR\0file\0../there/file\0file\0" \
+		"$ours_oid" "$theirs_oid" "$merged_oid" >expect &&
+	git -C cross-prefix diff-tree --no-commit-id -c -M --follow --raw -z \
+		--combined-all-paths --relative=here/ HEAD -- here/file >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t4045-diff-relative.sh b/t/t4045-diff-relative.sh
index 167be0bdcc..f105ddcfd1 100755
--- a/t/t4045-diff-relative.sh
+++ b/t/t4045-diff-relative.sh
@@ -223,7 +223,7 @@ test_expect_success 'diff --relative --name-only with change in subdir' '
 	test_cmp expected out
 '
 
-test_expect_failure 'diff --relative with change in subdir' '
+test_expect_success 'diff --relative with change in subdir' '
 	git switch br3 &&
 	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
 	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
@@ -242,6 +242,44 @@ test_expect_failure 'diff --relative with change in subdir' '
 	++>>>>>>> br1
 	EOF
 	git -C subdir diff --relative >out &&
+	test_cmp expected out &&
+	git diff --relative=subdir >out &&
+	test_cmp expected out &&
+	git diff --relative=subdir/ >out &&
+	test_cmp expected out &&
+	sed "s,file0,dir/file0,g" expected >expected-short &&
+	git diff --relative=sub >out &&
+	test_cmp expected-short out &&
+	sed "s,file0,subdir/file0,g" expected >expected-full &&
+	git diff --relative=subdir --no-relative -- subdir/file0 >out &&
+	test_cmp expected-full out
+'
+
+test_expect_success 'combined raw diff with relative paths' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\tfile0\n" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git -C subdir diff --cc --raw --relative >out &&
+	test_cmp expected out &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\0file0\0" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git diff --cc --raw -z --relative=subdir >out &&
+	test_cmp expected out
+'
+
+test_expect_success 'combined raw diff lists all relative paths' '
+	git switch br3 &&
+	br1_blob=$(git rev-parse --short --verify br1:subdir/file0) &&
+	br3_blob=$(git rev-parse --short --verify br3:subdir/file0) &&
+	test_when_finished "git merge --abort" &&
+	test_must_fail git merge br1 &&
+	printf "::100644 100644 100644 %s %s 0000000 MM\tfile0\tfile0\tfile0\n" \
+		"$br3_blob" "$br1_blob" >expected &&
+	git diff --cc --raw --combined-all-paths --relative=subdir >out &&
 	test_cmp expected out
 '
 
@@ -254,4 +292,26 @@ test_expect_success 'diff --relative --cached with change in subdir' '
 	test_cmp expected out
 '
 
+test_expect_success FUNNYNAMES 'combined diff quotes relative paths' '
+	test_create_repo quoted &&
+	(
+		cd quoted &&
+		mkdir subdir &&
+		test_commit --no-tag base "subdir/quoted\"file" base &&
+		git switch -c side &&
+		test_commit --no-tag side "subdir/quoted\"file" side &&
+		git switch -c other HEAD^ &&
+		test_commit --no-tag other "subdir/quoted\"file" other &&
+		test_must_fail git merge side &&
+		cat >expected <<-\EOF &&
+		diff --cc "quoted\"file"
+		--- "a/quoted\"file"
+		+++ "b/quoted\"file"
+		EOF
+		git -C subdir diff --relative >out &&
+		sed -n "/^diff --cc /p; /^--- /p; /^+++ /p" out >actual &&
+		test_cmp expected actual
+	)
+'
+
 test_done
-- 
2.55.0

[PATCH v2 2/2] combine-diff: filter the fast scan by the relative prefix

[Muhammed Dilshad A]

The pairwise scan filters paths by --relative before looking for renames.
The fast scan bypasses those callbacks, so it can show changes outside
the requested prefix.

Filter the fast scan's paths by the same literal prefix and release the
discarded records. Add tests for excluding outside paths and for keeping
explicit prefixes with repeated separators literal, as ordinary diff
does.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Muhammed Dilshad A <dilsheddilu123@gmail.com>
---
 combine-diff.c           | 18 ++++++++++++++++++
 t/t4038-diff-combined.sh | 21 +++++++++++++++++++++
 2 files changed, 39 insertions(+)

diff --git a/combine-diff.c b/combine-diff.c
index d615471717..e8a3bd1ee9 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1474,6 +1474,24 @@ static struct combine_diff_path *find_paths_multitree(
 
 	strbuf_release(&base);
 	free(parents_oid);
+
+	/* Match the prefix filtering used by the pairwise scan. */
+	if (opt->prefix) {
+		struct combine_diff_path **tail = &paths;
+
+		while (*tail) {
+			struct combine_diff_path *p = *tail;
+
+			if (starts_with(p->path, opt->prefix)) {
+				tail = &p->next;
+				continue;
+			}
+			*tail = p->next;
+			for (i = 0; i < nparent; i++)
+				free(p->parent[i].path);
+			free(p);
+		}
+	}
 	return paths;
 }
 
diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh
index 21eeb4fbcb..50df3dfb46 100755
--- a/t/t4038-diff-combined.sh
+++ b/t/t4038-diff-combined.sh
@@ -652,4 +652,25 @@ test_expect_success 'combined raw relative diff follows an outside rename source
 	test_cmp expect actual
 '
 
+test_expect_success 'fast combined relative diff excludes outside paths' '
+	ours_oid=$(git -C cross-prefix rev-parse HEAD^:here/file) &&
+	merged_oid=$(git -C cross-prefix rev-parse HEAD:here/file) &&
+	printf "::100644 000000 100644 %s %s %s MA\tfile\tfile\tfile\n" \
+		"$ours_oid" "$ZERO_OID" "$merged_oid" >expect &&
+	git -C cross-prefix diff-tree --no-commit-id -c --raw --no-renames \
+		--combined-all-paths --relative=here/ HEAD >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'combined diff keeps explicit relative prefixes literal' '
+	git -C cross-prefix diff HEAD^ HEAD --relative=here// >actual &&
+	test_must_be_empty actual &&
+	git -C cross-prefix diff-tree --no-commit-id --cc -M \
+		--combined-all-paths --relative=here// HEAD >actual &&
+	test_must_be_empty actual &&
+	git -C cross-prefix diff-tree --no-commit-id --cc --no-renames \
+		--combined-all-paths --relative=here// HEAD >actual &&
+	test_must_be_empty actual
+'
+
 test_done
-- 
2.55.0

Re: [PATCH v2 0/2] combine-diff: honor relative paths consistently

[Junio C Hamano]

Muhammed Dilshad A <dilsheddilu123@gmail.com> writes:

> The revised patch prints there/file as ../there/file when the prefix is
> here/. All displayed names then use the same base. The tests cover both
> discovery paths, including raw and NUL-separated output.

This sounds like the most sensible behaviour, within the constraint
that --relative must give a relative path to the prefix.

> The index rejects repeated separators, but tree entry parsing does not
> enforce the same check. The helper now skips all separators at the prefix
> boundary, so it does not rely on there being only one. Explicit prefix
> arguments stay literal, matching ordinary diff's filtering behavior.
> I also wrapped the added C lines to fit the coding guidelines.

Do *not* respond to review comments in your cover letter.  Nobody
reading the above, other than those who have seen our earlier
exchange of you sending v1 patch with I commenting on it, would not
know what you are talking about in the above, and especially what is
so special about "repeated separators" without context.  The cover
letter should aim to welcome even those late-comming reviewers who
missed an earlier round.

Review response should be done as a response to a review message,
unrelated to your rerolled patches.

> While checking the two discovery paths, I found that the fast multi-tree
> scan bypasses the relative-prefix filter entirely. Patch 2 fixes that
> separately and adds tests for outside paths and repeated separators in
> an explicit prefix.

Great.

>
> Changes since v1:
>
> * Use relative_path() for parent names outside the prefix while keeping
>   ordinary diff's literal-prefix behavior for matching names.
> * Preserve /dev/null, skip all boundary separators, and wrap long lines.
> * Add cross-directory rename tests and the separate fast-scan fix.
>
> The developer build with SANITIZE=leak succeeds. The affected suites pass
> all 77 normal tests with SHA-1 and SHA-256. A separate run with
> LSAN_OPTIONS=detect_leaks=1 also passes without a leak report. The existing
> three-parent coalescing failure in t4038 remains an expected failure.
>
> Muhammed Dilshad A (2):
>   combine-diff: honor --relative when printing paths
>   combine-diff: filter the fast scan by the relative prefix
>
>  combine-diff.c           |  70 ++++++++++++++++++---
>  t/t4038-diff-combined.sh | 130 +++++++++++++++++++++++++++++++++++++++
>  t/t4045-diff-relative.sh |  62 ++++++++++++++++++-
>  3 files changed, 251 insertions(+), 11 deletions(-)
>
>
> base-commit: 6de20f6092dcf9bdb1c8efe03db4b70c82b423dd

Re: [PATCH v2 0/2] combine-diff: honor relative paths consistently

[Muhammed Dilshad A]

Hi Junio,

Resending in plain text after the list rejected my earlier reply.

Understood. I'll keep responses to review comments in separate replies
and make future cover letters introduce the series for readers who have
not seen the earlier discussion.

Regards,
Dilshad