Re: Regression in `git diff --quiet HEAD` when a new file is staged

From

Jeff King <peff@peff.net>

Date

Oct 23, 2025, 12:15 UTC

Message-ID

<20251023121525.GB1123594@coredump.intra.peff.net>

In-Reply-To

<20251023120101.GA1123594@coredump.intra.peff.net>

On Thu, Oct 23, 2025 at 08:01:01AM -0400, Jeff King wrote:

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

diff --git a/diff.c b/diff.c
index 9b8d658b9e..c9d3aaeb0f 100644
--- a/diff.c
+++ b/diff.c
@@ -6175,14 +6175,34 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
 /* return 1 if any change is found; otherwise, return 0 */
 static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)
 {
+	static FILE *dev_null;
+	FILE *saved_file = o->file;
+	int saved_color_moved = o->color_moved;
 	int saved_dry_run = o->dry_run;
 	int saved_found_changes = o->found_changes;
 	int ret;
 
+	/*
+	 * As an extra precaution against code sending output to o->file even
+	 * when o->dry_run is set, redirect to /dev/null.
+	 *
+	 * We cache the /dev/null filehandle forever, effectively leaking it.
+	 * Gross, but it's O(1) gross-ness. A better solution would perhaps be
+	 * stuffing it into o->cached_dev_null or something, and freeing it
+	 * with the rest of the diff options.
+	 */
+	if (!dev_null)
+		dev_null = xfopen("/dev/null", "w");
+
+	o->file = dev_null;
+	/* TODO check if this is actually doing anything! */
+	o->color_moved = 0;
 	o->dry_run = 1;
 	o->found_changes = 0;
 	diff_flush_patch(p, o);
 	ret = o->found_changes;
+	o->file = saved_file;
+	o->color_moved = saved_color_moved;
 	o->dry_run = saved_dry_run;
 	o->found_changes |= saved_found_changes;
 	return ret;
@@ -6876,15 +6896,6 @@ 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))

And you can see the difference with the tests Lidong added in t4013, or
just with this simple sequence:

  git init
  echo content >file
  git add file
  git commit -m foo
  git.compile show -w --name-status

Without either the /dev/null redirection above (or the actual dry_run
fixes), you get a bogus "diff --git" header in the output.

So mulling over that for a moment...if we are going to teach all code
paths that look at o->file to check o->dry_run, why do we need a
/dev/null redirection at all? Can't we just set o->file to NULL, and
that is the clue that we do not want output?

I know that is more intricate, and not what we want to do for the
immediate regression fix. But in the long run it makes more sense to me.
We get rid of the extra flag, and any code that does the wrong thing (by
trying to write to o->file) will blow up horribly with a segfault rather
than quietly produce wrong output.

-Peff