From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 17 Oct 2025 18:22:37 GMT
Subject: Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents
Message-ID: <xmqqzf9pz8uq.fsf@gitster.g>
In-Reply-To: <20251017083641.GB4073661@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> Looking at that patch, my biggest concern is: are we missing other spots
>> that need to special-case the dry_run setting? Because it's a regression
>> in a maint release, I'm tempted to say we should do the dumbest possible
>> thing that covers all cases and just revert this hunk from the original
>> patch, like:
>
> Here it is with a commit message and test, in case that is helpful.
>
> -- >8 --
> Subject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents
> ...
> I didn't test, but I also wondered if this might be necessary to avoid
> actual external diff programs from spewing to stdout. Looking at
> run_external_diff(), we do:
>
>   int quiet = !(o->output_format & DIFF_FORMAT_PATCH);
>   [...]
>   cmd.no_stdout = quiet;
>
> so I _think_ it should be OK even without this patch. But again, I like
> the extra layer of protection here.

I do like this direction, in addition I really do appreciate your
thought above on optimizing ext-diff and textconv away when they are
not necessary (obviously outside the scope of the regression fix).

I also wonder if we want to get rid of the new code related to the
"dry-run" mode that we no longer have to use.  But as a regression
fix that wants to be minimum, I think this patch stops at the right
place.

Thanks.


>  diff.c                | 9 +++++++++
>  t/t4035-diff-quiet.sh | 4 ++++
>  2 files changed, 13 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index 87fa16b730..687206f353 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6890,6 +6890,15 @@ void diff_flush(struct diff_options *options)
>  	if (output_format & DIFF_FORMAT_NO_OUTPUT &&
>  	    options->flags.exit_with_status &&
>  	    options->flags.diff_from_contents) {
> +		/*
> +		 * run diff_flush_patch for the exit status. setting
> +		 * options->file to /dev/null should be safe, because we
> +		 * aren't supposed to produce any output anyway.
> +		 */
> +		diff_free_file(options);
> +		options->file = xfopen("/dev/null", "w");
> +		options->close_file = 1;
> +		options->color_moved = 0;
>  		for (i = 0; i < q->nr; i++) {
>  			struct diff_filepair *p = q->queue[i];
>  			if (check_pair_status(p))
> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index 0352bf81a9..35eaf0855f 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
>  	test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
>  	test_line_count = 0 cnt
>  '
> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '
> +	test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
> +	test_line_count = 0 cnt
> +'
>  test_expect_success 'git diff-files' '
>  	test_expect_code 0 git diff-files --quiet >cnt &&
>  	test_line_count = 0 cnt