[PATCH 0/4] diff dry-run cleanups

[PATCH 0/4] diff dry-run cleanups

[Jeff King]

Since we were talking about this topic so much this week, I wanted to
collect these patches before they fell out of my head. They would go on
top of ly/diff-name-only-with-diff-from-content.

  [1/4]: diff: send external diff output to diff_options.file

    This one is actually a bug-fix that we found along the way, and
    could be applied completely independently. AFAICT the bug has been
    there since 2008, though, so probably not super urgent. ;)

  [2/4]: diff: drop save/restore of color_moved in dry-run mode

    This one is hopefully a no-brainer simplification.

  [3/4]: diff: replace diff_options.dry_run flag with NULL file
  [4/4]: diff: drop dry-run redirection to /dev/null

    And then these ones clean up our extra layer of fixes, converting
    any missed dry_run checks into segfaults. But we hope there aren't
    any after Lidong's fixes. :) So possibly controversial, but I think
    the end result is nicer to read.

 diff.c                   | 56 +++++++++++-----------------------------
 diff.h                   |  2 --
 t/t4020-diff-external.sh | 10 +++++++
 3 files changed, 25 insertions(+), 43 deletions(-)

-Peff

[PATCH 1/4] diff: send external diff output to diff_options.file

[Jeff King]

Diff output usually goes to the process stdout, but it can be redirected
with the "--output" option. We store this in the "file" pointer of
diff_options, and all of the diff code should write there instead of to
stdout.

But there's one spot we missed: running an external diff cmd. We don't
redirect its output at all, so it just defaults to the stdout of the
parent process. We should instead point its stdout at our output file.
There are a few caveats to watch out for when doing so:

  - The stdout field takes a descriptor, not a FILE pointer. We can pull
    out the descriptor with fileno().

  - The run-command API always closes the stdout descriptor we pass to
    it. So we must duplicate it (otherwise we break the FILE pointer,
    since it now points to a closed descriptor).

  - We don't need to worry about closing our dup'd descriptor, since the
    point is that run-command will do it for us (even in the case of an
    error). But we do need to make sure we skip the dup() if we set
    no_stdout (because then run-command will not look at it at all).

  - When the output is going to stdout, it would not be wrong to dup()
    the descriptor, but we don't need to. We can skip that extra work
    with a simple pointer comparison.

  - It seems like you'd need to fflush() the descriptor before handing
    off a copy to the child process to prevent out-of-order writes. But
    that was true even before this patch! It works because run-command
    always calls fflush(NULL) before running the child.

The new test shows the breakage (and fix). The need for duplicating the
descriptor doesn't need a new test; that is covered by the later test
"GIT_EXTERNAL_DIFF with more than one changed files".

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c                   |  5 ++++-
 t/t4020-diff-external.sh | 10 ++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/diff.c b/diff.c
index 22415aecee..39029cc096 100644
--- a/diff.c
+++ b/diff.c
@@ -4457,7 +4457,10 @@ static void run_external_diff(const struct external_diff *pgm,
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 	cmd.use_shell = 1;
-	cmd.no_stdout = quiet;
+	if (quiet)
+		cmd.no_stdout = 1;
+	else if (o->file != stdout)
+		cmd.out = xdup(fileno(o->file));
 	rc = run_command(&cmd);
 	if (!pgm->trust_exit_code && rc == 0)
 		o->found_changes = 1;
diff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh
index c8a23d5148..7ec5854f74 100755
--- a/t/t4020-diff-external.sh
+++ b/t/t4020-diff-external.sh
@@ -44,6 +44,16 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '
 
 '
 
+test_expect_success 'GIT_EXTERNAL_DIFF and --output' '
+	cat >expect <<-EOF &&
+	file $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644
+	EOF
+	GIT_EXTERNAL_DIFF=echo git diff --output=out >stdout &&
+	cut -d" " -f1,3- <out >actual &&
+	test_must_be_empty stdout &&
+	test_cmp expect actual
+'
+
 test_expect_success SYMLINKS 'typechange diff' '
 	rm -f file &&
 	ln -s elif file &&
-- 
2.51.1.797.g1148beab57

[PATCH 2/4] diff: drop save/restore of color_moved in dry-run mode

[Jeff King]

When running a dry-run content-level diff to check whether a "--quiet"
diff has any changes, we have always unset the color_moved variable
since the feature was added in 2e2d5ac184 (diff.c: color moved lines
differently, 2017-06-30). The reasoning is not given explicitly there,
but presumably the idea is that since color_moved requires a lot of
extra computation to match lines but does not actually affect the
found_changes flag, we want to skip it.

Later, in 3da4413dbc (diff: make sure the other caller of
diff_flush_patch_quietly() is silent, 2025-10-22) we copied the same
idea for other dry-run diffs.

But neither spot actually needs to reset this flag at all, because
diff_flush_patch() will not ever compute color_moved. Nor could it, as
it is only looking at a single file-pair, and we detect moves across
files. So color_moved is checked only when we are actually doing real
DIFF_FORMAT_PATCH output, and call diff_flush_patch_all_file_pairs().

So we can get rid of these extra lines to save and restore the
color_moved flag without changing the behavior at all. (Note that there
is no "restore" to drop for the second caller, as we know at that point
we are not generating any output and can just leave the feature
disabled).

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/diff.c b/diff.c
index 39029cc096..d83d898702 100644
--- a/diff.c
+++ b/diff.c
@@ -6839,11 +6839,9 @@ void diff_flush(struct diff_options *options)
 		 * make sure diff_Flush_patch_quietly() to be silent.
 		 */
 		FILE *dev_null = NULL;
-		int saved_color_moved = options->color_moved;
 
 		if (options->flags.diff_from_contents) {
 			dev_null = xfopen("/dev/null", "w");
-			options->color_moved = 0;
 		}
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
@@ -6865,7 +6863,6 @@ void diff_flush(struct diff_options *options)
 		}
 		if (options->flags.diff_from_contents) {
 			fclose(dev_null);
-			options->color_moved = saved_color_moved;
 		}
 		separator++;
 	}
@@ -6925,7 +6922,6 @@ void diff_flush(struct diff_options *options)
 		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))
-- 
2.51.1.797.g1148beab57

[PATCH 3/4] diff: replace diff_options.dry_run flag with NULL file

[Jeff King]

We introduced a dry_run flag to diff_options in b55e6d36eb (diff: ensure
consistent diff behavior with ignore options, 2025-08-08), with the idea
that the lower-level diff code could skip output when it is set.

As we saw with the bugs fixed by 3ed5d8bd73 (diff: stop output garbled
message in dry run mode, 2025-10-20), it is easy to miss spots. In the
end, we located all of them by checking where diff_options.file is used.

That suggests another possible approach: we can replace the dry_run
boolean with a NULL pointer for "file", as we know that using "file" in
dry_run mode would always be an error. This turns any missed spots from
producing extra output[1] into a segfault. Which is less forgiving, but
that is the point: this is indicative of a programming error, and
complaining loudly and immediately is good.

[1] We protect ourselves against garbled output as a separate step,
    courtesy of 623f7af284 (diff: restore redirection to /dev/null for
    diff_from_contents, 2025-10-17). So in that sense this patch can
    only introduce user-visible errors (since any "bugs" were going to
    /dev/null before), but the idea is to catch them rather than quietly
    send garbage to /dev/null.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 16 ++++++++--------
 diff.h |  2 --
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index d83d898702..a8d50fb1fc 100644
--- a/diff.c
+++ b/diff.c
@@ -1351,7 +1351,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,
 	int len = eds->len;
 	unsigned flags = eds->flags;
 
-	if (o->dry_run)
+	if (!o->file)
 		return;
 
 	switch (s) {
@@ -3765,9 +3765,9 @@ static void builtin_diff(const char *name_a,
 
 		if (o->word_diff)
 			init_diff_words_data(&ecbdata, o, one, two);
-		if (o->dry_run) {
+		if (!o->file) {
 			/*
-			 * Unlike the !dry_run case, we need to ignore the
+			 * Unlike the normal output case, we need to ignore the
 			 * return value from xdi_diff_outf() here, because
 			 * xdi_diff_outf() takes non-zero return from its
 			 * callback function as a sign of error and returns
@@ -4423,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	int quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;
+	int quiet = !(o->output_format & DIFF_FORMAT_PATCH) || !o->file;
 	int rc;
 
 	/*
@@ -4621,7 +4621,7 @@ static void run_diff_cmd(const struct external_diff *pgm,
 		    p->status == DIFF_STATUS_RENAMED)
 			o->found_changes = 1;
 	} else {
-		if (!o->dry_run)
+		if (o->file)
 			fprintf(o->file, "* Unmerged path %s\n", name);
 		o->found_changes = 1;
 	}
@@ -6199,15 +6199,15 @@ 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)
 {
-	int saved_dry_run = o->dry_run;
+	FILE *saved_file = o->file;
 	int saved_found_changes = o->found_changes;
 	int ret;
 
-	o->dry_run = 1;
+	o->file = NULL;
 	o->found_changes = 0;
 	diff_flush_patch(p, o);
 	ret = o->found_changes;
-	o->dry_run = saved_dry_run;
+	o->file = saved_file;
 	o->found_changes |= saved_found_changes;
 	return ret;
 }
diff --git a/diff.h b/diff.h
index 2fa256c3ef..31eedd5c0c 100644
--- a/diff.h
+++ b/diff.h
@@ -408,8 +408,6 @@ struct diff_options {
 	#define COLOR_MOVED_WS_ERROR (1<<0)
 	unsigned color_moved_ws_handling;
 
-	bool dry_run;
-
 	struct repository *repo;
 	struct strmap *additional_path_headers;
 
-- 
2.51.1.797.g1148beab57

Re: [PATCH 3/4] diff: replace diff_options.dry_run flag with NULL file

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> We introduced a dry_run flag to diff_options in b55e6d36eb (diff: ensure
> consistent diff behavior with ignore options, 2025-08-08), with the idea
> that the lower-level diff code could skip output when it is set.
>
> As we saw with the bugs fixed by 3ed5d8bd73 (diff: stop output garbled
> message in dry run mode, 2025-10-20), it is easy to miss spots. In the
> end, we located all of them by checking where diff_options.file is used.
>
> That suggests another possible approach: we can replace the dry_run
> boolean with a NULL pointer for "file", as we know that using "file" in
> dry_run mode would always be an error. This turns any missed spots from
> producing extra output[1] into a segfault. Which is less forgiving, but
> that is the point: this is indicative of a programming error, and
> complaining loudly and immediately is good.

Makes sense.

[PATCH 4/4] diff: drop dry-run redirection to /dev/null

[Jeff King]

As an added protection against dry-run diffs accidentally producing
output, we redirect diff_options.file to /dev/null. But as of the
previous patch, this now does nothing, since dry-run diffs are
implemented by setting "file" to NULL.

So we can drop this extra code with no change in behavior. This is
effectively a revert of 623f7af284 (diff: restore redirection to
/dev/null for diff_from_contents, 2025-10-17) and 3da4413dbc (diff: make
sure the other caller of diff_flush_patch_quietly() is silent,
2025-10-22), but:

  1. We get a conflict because we already dropped the color_moved
     handling in an earlier patch. But we just resolve the conflicts to
     "theirs" (removing all of the code).

  2. We retain the test from 623f7af284.

Signed-off-by: Jeff King <peff@peff.net>
---
 diff.c | 31 +++----------------------------
 1 file changed, 3 insertions(+), 28 deletions(-)

diff --git a/diff.c b/diff.c
index a8d50fb1fc..9169ccfaa9 100644
--- a/diff.c
+++ b/diff.c
@@ -6835,35 +6835,18 @@ void diff_flush(struct diff_options *options)
 			     DIFF_FORMAT_NAME |
 			     DIFF_FORMAT_NAME_STATUS |
 			     DIFF_FORMAT_CHECKDIFF)) {
-		/*
-		 * make sure diff_Flush_patch_quietly() to be silent.
-		 */
-		FILE *dev_null = NULL;
-
-		if (options->flags.diff_from_contents) {
-			dev_null = xfopen("/dev/null", "w");
-		}
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 
 			if (!check_pair_status(p))
 				continue;
 
-			if (options->flags.diff_from_contents) {
-				FILE *saved_file = options->file;
-				int found_changes;
+			if (options->flags.diff_from_contents &&
+			    !diff_flush_patch_quietly(p, options))
+				continue;
 
-				options->file = dev_null;
-				found_changes = diff_flush_patch_quietly(p, options);
-				options->file = saved_file;
-				if (!found_changes)
-					continue;
-			}
 			flush_one_pair(p, options);
 		}
-		if (options->flags.diff_from_contents) {
-			fclose(dev_null);
-		}
 		separator++;
 	}
 
@@ -6914,14 +6897,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;
 		for (i = 0; i < q->nr; i++) {
 			struct diff_filepair *p = q->queue[i];
 			if (check_pair_status(p))
-- 
2.51.1.797.g1148beab57

[PATCH 5/4] diff: simplify run_external_diff() quiet logic

[Jeff King]

We'd sometimes end up in run_external_diff() to do a dry-run diff (e.g.,
to find content-level changes for --quiet). We recognize this quiet mode
by seeing the lack of DIFF_FORMAT_PATCH in the output format.

But since introducing an explicit dry-run check via 3ed5d8bd73 (diff:
stop output garbled message in dry run mode, 2025-10-20), this logic can
never trigger. We can only get to this function by calling
diff_flush_patch(), and that comes from only two places:

  1. A dry-run flush comes from diff_flush_patch_quietly(), which is
     always in dry-run mode (so the other half of our "||" is true
     anyway).

  2. A regular flush comes from diff_flush_patch_all_file_pairs(),
     which is only called when output_format has DIFF_FORMAT_PATCH in
     it.

So we can simplify our "quiet" condition to just checking dry-run mode
(which used to be a specific flag, but recently became just a NULL
"file" pointer). And since it's so simple, we can just do that inline.
This makes the logic about o->file more obvious, since we handle the
NULL and non-stdout cases next to each other.

Signed-off-by: Jeff King <peff@peff.net>
---
Here's one more that I forgot to poke at before sending out the others.

 diff.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/diff.c b/diff.c
index 9169ccfaa9..a1961526c0 100644
--- a/diff.c
+++ b/diff.c
@@ -4423,7 +4423,6 @@ static void run_external_diff(const struct external_diff *pgm,
 {
 	struct child_process cmd = CHILD_PROCESS_INIT;
 	struct diff_queue_struct *q = &diff_queued_diff;
-	int quiet = !(o->output_format & DIFF_FORMAT_PATCH) || !o->file;
 	int rc;
 
 	/*
@@ -4432,7 +4431,7 @@ static void run_external_diff(const struct external_diff *pgm,
 	 * external diff program lacks the ability to tell us whether
 	 * it's empty then we consider it non-empty without even asking.
 	 */
-	if (!pgm->trust_exit_code && quiet) {
+	if (!pgm->trust_exit_code && !o->file) {
 		o->found_changes = 1;
 		return;
 	}
@@ -4457,7 +4456,7 @@ static void run_external_diff(const struct external_diff *pgm,
 	diff_free_filespec_data(one);
 	diff_free_filespec_data(two);
 	cmd.use_shell = 1;
-	if (quiet)
+	if (!o->file)
 		cmd.no_stdout = 1;
 	else if (o->file != stdout)
 		cmd.out = xdup(fileno(o->file));
-- 
2.51.1.797.g1148beab57