threads / patch / 62079

patch, 2 partsdiff: report copies and renames as changes in run_diff_cmd()

Subject: [PATCH 1/2] diff: report copies and renames as changes in run_diff_cmd()

## tl;dr

4 messages between Sep 8, 2024 and Sep 9, 2024. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

René Scharfe· Sep 8, 2024, 07:05 UTC · lore

The diff machinery has two ways to detect changes to set the exit code: Just comparing hashes and comparing blob contents. The latter is needed if certain changes have to be ignored, e.g. with --ignore-space-change or --ignore-matching-lines. It's enabled by the diff_options flag diff_from_contents.

The slower mode has never considered copies and renames to be changes, which is inconsistent with the quicker one. Fix it. Even if we ignore the file contents (because it's empty or contains only ignored lines), there's still the meta data change of adding or changing a filename, so we need to report it in the exit code.

d7b97b7185 (diff: let external diffs report that changes are uninteresting, 2024-06-09) set diff_from_contents if external diff programs are allowed. This is the default e.g. for git diff, and so that change exposed the inconsistency much more widely.

Reported-by: Jorge Luis Martinez Gomez <jol@jol.dev>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c                 |  3 +++
 t/t4017-diff-retval.sh | 16 ++++++++++++++++
 2 files changed, 19 insertions(+)

Show changes to 2 files +19 −1

diff.c, t/t4017-diff-retval.sh

diff --git a/diff.c b/diff.c
index 4035a9374d..1d2057d4cb 100644
--- a/diff.c
+++ b/diff.c
@@ -4587,6 +4587,9 @@ static void run_diff_cmd(const struct external_diff *pgm,
 		builtin_diff(name, other ? other : name,
 			     one, two, xfrm_msg, must_show_header,
 			     o, complete_rewrite);
+		if (p->status == DIFF_STATUS_COPIED ||
+		    p->status == DIFF_STATUS_RENAMED)
+			o->found_changes = 1;
 	} else {
 		fprintf(o->file, "* Unmerged path %s\n", name);
 		o->found_changes = 1;
diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index f439f469bd..9a4f578614 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -143,4 +143,20 @@ test_expect_success 'option errors are not confused by --exit-code' '
 	grep '^usage:' err
 '

+for option in --exit-code --quiet
+do
+	test_expect_success "git diff $option returns 1 for copied file" "
+		git reset --hard &&
+		cp a copy &&
+		git add copy &&
+		test_expect_code 1 git diff $option --cached --find-copies-harder
+	"
+
+	test_expect_success "git diff $option returns 1 for renamed file" "
+		git reset --hard &&
+		git mv a renamed &&
+		test_expect_code 1 git diff $option --cached
+	"
+done
+
 test_done
--
2.46.0

René Scharfe· Sep 8, 2024, 07:08 UTC · re: René Scharfe · lore

[PATCH 2/2] diff: report dirty submodules as changes in builtin_diff()

The slower mode as never considered submodules (and subrepos) as changes with --submodule=diff or --submodule=log, which is inconsistent with --submodule=short (the default). Fix it.

Reported-by: David Hull <david.hull@friendbuy.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 diff.c                 |  2 ++
 t/t4017-diff-retval.sh | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+)

Show changes to 2 files +23 −1

diff.c, t/t4017-diff-retval.sh

diff --git a/diff.c b/diff.c
index 1d2057d4cb..472479eb10 100644
--- a/diff.c
+++ b/diff.c
@@ -3565,6 +3565,7 @@ static void builtin_diff(const char *name_a,
 		show_submodule_diff_summary(o, one->path ? one->path : two->path,
 				&one->oid, &two->oid,
 				two->dirty_submodule);
+		o->found_changes = 1;
 		return;
 	} else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
 		   (!one->mode || S_ISGITLINK(one->mode)) &&
@@ -3573,6 +3574,7 @@ static void builtin_diff(const char *name_a,
 		show_submodule_inline_diff(o, one->path ? one->path : two->path,
 				&one->oid, &two->oid,
 				two->dirty_submodule);
+		o->found_changes = 1;
 		return;
 	}

diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
index 9a4f578614..d644310e22 100755
--- a/t/t4017-diff-retval.sh
+++ b/t/t4017-diff-retval.sh
@@ -159,4 +159,25 @@ do
 	"
 done

+test_expect_success 'setup dirty subrepo' '
+	git reset --hard &&
+	test_create_repo subrepo &&
+	test_commit -C subrepo subrepo-file &&
+	test_tick &&
+	git add subrepo &&
+	git commit -m subrepo &&
+	test_commit -C subrepo another-subrepo-file
+'
+
+for option in --exit-code --quiet
+do
+	for submodule_format in diff log short
+	do
+		opts="$option --submodule=$submodule_format" &&
+		test_expect_success "git diff $opts returns 1 for dirty subrepo" "
+			test_expect_code 1 git diff $opts
+		"
+	done
+done
+
 test_done
--
2.46.0

Phillip Wood· Sep 9, 2024, 15:16 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] diff: report copies and renames as changes in run_diff_cmd()

Hi René

On 08/09/2024 08:05, René Scharfe wrote:

Show 16 quoted lines

> The diff machinery has two ways to detect changes to set the exit code:
> Just comparing hashes and comparing blob contents.  The latter is needed
> if certain changes have to be ignored, e.g. with --ignore-space-change
> or --ignore-matching-lines.  It's enabled by the diff_options flag
> diff_from_contents.
> 
> The slower mode has never considered copies and renames to be changes,
> which is inconsistent with the quicker one.  Fix it.  Even if we ignore
> the file contents (because it's empty or contains only ignored lines),
> there's still the meta data change of adding or changing a filename, so
> we need to report it in the exit code.
> 
> d7b97b7185 (diff: let external diffs report that changes are
> uninteresting, 2024-06-09) set diff_from_contents if external diff
> programs are allowed.  This is the default e.g. for git diff, and so
> that change exposed the inconsistency much more widely.

Thanks for fixing this - both patches looks good to me.

Best Wishes

Phillip

Show 49 quoted lines

> Reported-by: Jorge Luis Martinez Gomez <jol@jol.dev>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>   diff.c                 |  3 +++
>   t/t4017-diff-retval.sh | 16 ++++++++++++++++
>   2 files changed, 19 insertions(+)
> 
> diff --git a/diff.c b/diff.c
> index 4035a9374d..1d2057d4cb 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4587,6 +4587,9 @@ static void run_diff_cmd(const struct external_diff *pgm,
>   		builtin_diff(name, other ? other : name,
>   			     one, two, xfrm_msg, must_show_header,
>   			     o, complete_rewrite);
> +		if (p->status == DIFF_STATUS_COPIED ||
> +		    p->status == DIFF_STATUS_RENAMED)
> +			o->found_changes = 1;
>   	} else {
>   		fprintf(o->file, "* Unmerged path %s\n", name);
>   		o->found_changes = 1;
> diff --git a/t/t4017-diff-retval.sh b/t/t4017-diff-retval.sh
> index f439f469bd..9a4f578614 100755
> --- a/t/t4017-diff-retval.sh
> +++ b/t/t4017-diff-retval.sh
> @@ -143,4 +143,20 @@ test_expect_success 'option errors are not confused by --exit-code' '
>   	grep '^usage:' err
>   '
> 
> +for option in --exit-code --quiet
> +do
> +	test_expect_success "git diff $option returns 1 for copied file" "
> +		git reset --hard &&
> +		cp a copy &&
> +		git add copy &&
> +		test_expect_code 1 git diff $option --cached --find-copies-harder
> +	"
> +
> +	test_expect_success "git diff $option returns 1 for renamed file" "
> +		git reset --hard &&
> +		git mv a renamed &&
> +		test_expect_code 1 git diff $option --cached
> +	"
> +done
> +
>   test_done
> --
> 2.46.0
>

Junio C Hamano· Sep 9, 2024, 16:59 UTC · re: Phillip Wood · lore

Re: [PATCH 1/2] diff: report copies and renames as changes in run_diff_cmd()

Phillip Wood <phillip.wood123@gmail.com> writes:

Show 10 quoted lines

>> which is inconsistent with the quicker one.  Fix it.  Even if we ignore
>> the file contents (because it's empty or contains only ignored lines),
>> there's still the meta data change of adding or changing a filename, so
>> we need to report it in the exit code.
>> d7b97b7185 (diff: let external diffs report that changes are
>> uninteresting, 2024-06-09) set diff_from_contents if external diff
>> programs are allowed.  This is the default e.g. for git diff, and so
>> that change exposed the inconsistency much more widely.
>
> Thanks for fixing this - both patches looks good to me.

Yup, thanks, both.