From 57c2b6cc86edd29fa2d30bc53b4a476e0621619c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Oct 2025 13:06:49 -0400 Subject: [PATCH 1/5] diff: send external diff output to diff_options.file 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 Signed-off-by: Junio C Hamano --- 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- actual && + test_must_be_empty stdout && + test_cmp expect actual +' + test_expect_success SYMLINKS 'typechange diff' ' rm -f file && ln -s elif file && From 0152831d96af40277b0b69ef39e9c31b623dc753 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Oct 2025 13:07:53 -0400 Subject: [PATCH 2/5] diff: drop save/restore of color_moved in dry-run mode 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 Signed-off-by: Junio C Hamano --- 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)) From b2b5ad514d62ba26b3cfa65104d81c2d19552789 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Oct 2025 13:08:53 -0400 Subject: [PATCH 3/5] diff: replace diff_options.dry_run flag with NULL file 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 Signed-off-by: Junio C Hamano --- 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; From 1ad2760020bf426edd01ccec467da14c0f92cf2e Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Oct 2025 13:09:25 -0400 Subject: [PATCH 4/5] diff: drop dry-run redirection to /dev/null 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 Signed-off-by: Junio C Hamano --- 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)) From 2ecb8857e7785b6b27887164cb1aca67ce0b114a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Fri, 24 Oct 2025 13:25:07 -0400 Subject: [PATCH 5/5] diff: simplify run_external_diff() quiet logic 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 Signed-off-by: Junio C Hamano --- 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));