[PATCH 3/4] diff: replace diff_options.dry_run flag with NULL file
- From
Jeff King <peff@peff.net>
- Date
- Oct 24, 2025, 17:08 UTC
- Message-ID
- <20251024170853.GC2345184@coredump.intra.peff.net>
- In-Reply-To
- <20251024170522.GA2344972@coredump.intra.peff.net>
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