From: Junio C Hamano <gitster@pobox.com>
Date: Thu, 23 Oct 2025 13:35:05 GMT
Subject: Re: Regression in `git diff --quiet HEAD` when a new file is staged
Message-ID: <xmqqms5hwxkm.fsf@gitster.g>
In-Reply-To: <20251023120101.GA1123594@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> That is because you are trying to redirect to /dev/null once at the
> beginning of the loop. But the loop is effectively:
>
>   for each pair
>     check for content changes with diff_flush_patch_quietly();
>     output actual pair data with flush_one_pair();
>
> We want the redirection to /dev/null for the first part of the loop
> body, but not the second. So you have to do the redirection inside the
> loop.

Yeah, my bad.  Lidong noticed the same thing.

> I agree that opening /dev/null over and over is silly. But we can reuse
> the same filehandle for each one. I.e., like:
>
> diff --git a/diff.c b/diff.c
> index dac3ea9e01..e903afcf04 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6835,11 +6835,11 @@ void diff_flush(struct diff_options *options)
>  		/*
>  		 * make sure diff_Flush_patch_quietly() to be silent.
>  		 */
> -		FILE *saved_file = options->file;
> +		FILE *dev_null = NULL;
>  		int saved_color_moved = options->color_moved;
>  
>  		if (options->flags.diff_from_contents) {
> -			options->file = xfopen("/dev/null", "w");
> +			dev_null = xfopen("/dev/null", "w");
>  			options->color_moved = 0;
>  		}
>  		for (i = 0; i < q->nr; i++) {
> @@ -6848,15 +6848,20 @@ void diff_flush(struct diff_options *options)
>  			if (!check_pair_status(p))
>  				continue;
>  
> -			if (options->flags.diff_from_contents &&
> -			    !diff_flush_patch_quietly(p, options))
> -				continue;
> +			if (options->flags.diff_from_contents) {
> +				FILE *saved_file = options->file;
> +				int r;
> +				options->file = dev_null;
> +				r = diff_flush_patch_quietly(p, options);
> +				options->file = saved_file;
> +				if (!r)
> +					continue;
> +			}
>  
>  			flush_one_pair(p, options);
>  		}
>  		if (options->flags.diff_from_contents) {
> -			fclose(options->file);
> -			options->file = saved_file;
> +			fclose(dev_null);
>  			options->color_moved = saved_color_moved;
>  		}
>  		separator++;
>
> You could even imagine diff_flush_patch_quietly() saving the /dev/null
> descriptor in a static variable and effectively leaking it (or if we
> want to be more structured, cached inside the diff_options struct). And
> then the callers do not have to worry about it at all.

That would be bigger change than a regression fix warrants, so let's
leave it out, but let me use the above to replace my botched
attempt.

Thanks, both of you.