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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi René

On 08/09/2024 08:05, René Scharfe wrote:
> 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

> 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
>