{"thread":{"id":"64382","subject":"[PATCH 0/4] diff dry-run cleanups","startedAt":"2025-10-24T17:05:24Z","lastAt":"2025-10-24T17:25:08Z","messageCount":7,"participants":["Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"529619","messageId":"20251024170522.GA2344972@coredump.intra.peff.net","threadId":"64382","inReplyTo":null,"subject":"[PATCH 0/4] diff dry-run cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:05:22Z","receivedAt":"2025-10-24T17:05:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since we were talking about this topic so much this week, I wanted to\ncollect these patches before they fell out of my head. They would go on\ntop of ly/diff-name-only-with-diff-from-content.\n\n  [1/4]: diff: send external diff output to diff_options.file\n\n    This one is actually a bug-fix that we found along the way, and\n    could be applied completely independently. AFAICT the bug has been\n    there since 2008, though, so probably not super urgent. ;)\n\n  [2/4]: diff: drop save/restore of color_moved in dry-run mode\n\n    This one is hopefully a no-brainer simplification.\n\n  [3/4]: diff: replace diff_options.dry_run flag with NULL file\n  [4/4]: diff: drop dry-run redirection to /dev/null\n\n    And then these ones clean up our extra layer of fixes, converting\n    any missed dry_run checks into segfaults. But we hope there aren't\n    any after Lidong's fixes. :) So possibly controversial, but I think\n    the end result is nicer to read.\n\n diff.c                   | 56 +++++++++++-----------------------------\n diff.h                   |  2 --\n t/t4020-diff-external.sh | 10 +++++++\n 3 files changed, 25 insertions(+), 43 deletions(-)\n\n-Peff\n"},{"id":"529620","messageId":"20251024170649.GA2345184@coredump.intra.peff.net","threadId":"64382","inReplyTo":"20251024170522.GA2344972@coredump.intra.peff.net","subject":"[PATCH 1/4] diff: send external diff output to diff_options.file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:06:49Z","receivedAt":"2025-10-24T17:06:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Diff output usually goes to the process stdout, but it can be redirected\nwith the \"--output\" option. We store this in the \"file\" pointer of\ndiff_options, and all of the diff code should write there instead of to\nstdout.\n\nBut there's one spot we missed: running an external diff cmd. We don't\nredirect its output at all, so it just defaults to the stdout of the\nparent process. We should instead point its stdout at our output file.\nThere are a few caveats to watch out for when doing so:\n\n  - The stdout field takes a descriptor, not a FILE pointer. We can pull\n    out the descriptor with fileno().\n\n  - The run-command API always closes the stdout descriptor we pass to\n    it. So we must duplicate it (otherwise we break the FILE pointer,\n    since it now points to a closed descriptor).\n\n  - We don't need to worry about closing our dup'd descriptor, since the\n    point is that run-command will do it for us (even in the case of an\n    error). But we do need to make sure we skip the dup() if we set\n    no_stdout (because then run-command will not look at it at all).\n\n  - When the output is going to stdout, it would not be wrong to dup()\n    the descriptor, but we don't need to. We can skip that extra work\n    with a simple pointer comparison.\n\n  - It seems like you'd need to fflush() the descriptor before handing\n    off a copy to the child process to prevent out-of-order writes. But\n    that was true even before this patch! It works because run-command\n    always calls fflush(NULL) before running the child.\n\nThe new test shows the breakage (and fix). The need for duplicating the\ndescriptor doesn't need a new test; that is covered by the later test\n\"GIT_EXTERNAL_DIFF with more than one changed files\".\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c                   |  5 ++++-\n t/t4020-diff-external.sh | 10 ++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex 22415aecee..39029cc096 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4457,7 +4457,10 @@ static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tcmd.no_stdout = quiet;\n+\tif (quiet)\n+\t\tcmd.no_stdout = 1;\n+\telse if (o->file != stdout)\n+\t\tcmd.out = xdup(fileno(o->file));\n \trc = run_command(&cmd);\n \tif (!pgm->trust_exit_code && rc == 0)\n \t\to->found_changes = 1;\ndiff --git a/t/t4020-diff-external.sh b/t/t4020-diff-external.sh\nindex c8a23d5148..7ec5854f74 100755\n--- a/t/t4020-diff-external.sh\n+++ b/t/t4020-diff-external.sh\n@@ -44,6 +44,16 @@ test_expect_success 'GIT_EXTERNAL_DIFF environment and --no-ext-diff' '\n \n '\n \n+test_expect_success 'GIT_EXTERNAL_DIFF and --output' '\n+\tcat >expect <<-EOF &&\n+\tfile $(git rev-parse --verify HEAD:file) 100644 file $(test_oid zero) 100644\n+\tEOF\n+\tGIT_EXTERNAL_DIFF=echo git diff --output=out >stdout &&\n+\tcut -d\" \" -f1,3- <out >actual &&\n+\ttest_must_be_empty stdout &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success SYMLINKS 'typechange diff' '\n \trm -f file &&\n \tln -s elif file &&\n-- \n2.51.1.797.g1148beab57\n\n"},{"id":"529621","messageId":"20251024170753.GB2345184@coredump.intra.peff.net","threadId":"64382","inReplyTo":"20251024170522.GA2344972@coredump.intra.peff.net","subject":"[PATCH 2/4] diff: drop save/restore of color_moved in dry-run mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:07:53Z","receivedAt":"2025-10-24T17:07:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When running a dry-run content-level diff to check whether a \"--quiet\"\ndiff has any changes, we have always unset the color_moved variable\nsince the feature was added in 2e2d5ac184 (diff.c: color moved lines\ndifferently, 2017-06-30). The reasoning is not given explicitly there,\nbut presumably the idea is that since color_moved requires a lot of\nextra computation to match lines but does not actually affect the\nfound_changes flag, we want to skip it.\n\nLater, in 3da4413dbc (diff: make sure the other caller of\ndiff_flush_patch_quietly() is silent, 2025-10-22) we copied the same\nidea for other dry-run diffs.\n\nBut neither spot actually needs to reset this flag at all, because\ndiff_flush_patch() will not ever compute color_moved. Nor could it, as\nit is only looking at a single file-pair, and we detect moves across\nfiles. So color_moved is checked only when we are actually doing real\nDIFF_FORMAT_PATCH output, and call diff_flush_patch_all_file_pairs().\n\nSo we can get rid of these extra lines to save and restore the\ncolor_moved flag without changing the behavior at all. (Note that there\nis no \"restore\" to drop for the second caller, as we know at that point\nwe are not generating any output and can just leave the feature\ndisabled).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 4 ----\n 1 file changed, 4 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 39029cc096..d83d898702 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6839,11 +6839,9 @@ void diff_flush(struct diff_options *options)\n \t\t * make sure diff_Flush_patch_quietly() to be silent.\n \t\t */\n \t\tFILE *dev_null = NULL;\n-\t\tint saved_color_moved = options->color_moved;\n \n \t\tif (options->flags.diff_from_contents) {\n \t\t\tdev_null = xfopen(\"/dev/null\", \"w\");\n-\t\t\toptions->color_moved = 0;\n \t\t}\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n@@ -6865,7 +6863,6 @@ void diff_flush(struct diff_options *options)\n \t\t}\n \t\tif (options->flags.diff_from_contents) {\n \t\t\tfclose(dev_null);\n-\t\t\toptions->color_moved = saved_color_moved;\n \t\t}\n \t\tseparator++;\n \t}\n@@ -6925,7 +6922,6 @@ void diff_flush(struct diff_options *options)\n \t\tdiff_free_file(options);\n \t\toptions->file = xfopen(\"/dev/null\", \"w\");\n \t\toptions->close_file = 1;\n-\t\toptions->color_moved = 0;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n-- \n2.51.1.797.g1148beab57\n\n"},{"id":"529622","messageId":"20251024170853.GC2345184@coredump.intra.peff.net","threadId":"64382","inReplyTo":"20251024170522.GA2344972@coredump.intra.peff.net","subject":"[PATCH 3/4] diff: replace diff_options.dry_run flag with NULL file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:08:53Z","receivedAt":"2025-10-24T17:08:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We introduced a dry_run flag to diff_options in b55e6d36eb (diff: ensure\nconsistent diff behavior with ignore options, 2025-08-08), with the idea\nthat the lower-level diff code could skip output when it is set.\n\nAs we saw with the bugs fixed by 3ed5d8bd73 (diff: stop output garbled\nmessage in dry run mode, 2025-10-20), it is easy to miss spots. In the\nend, we located all of them by checking where diff_options.file is used.\n\nThat suggests another possible approach: we can replace the dry_run\nboolean with a NULL pointer for \"file\", as we know that using \"file\" in\ndry_run mode would always be an error. This turns any missed spots from\nproducing extra output[1] into a segfault. Which is less forgiving, but\nthat is the point: this is indicative of a programming error, and\ncomplaining loudly and immediately is good.\n\n[1] We protect ourselves against garbled output as a separate step,\n    courtesy of 623f7af284 (diff: restore redirection to /dev/null for\n    diff_from_contents, 2025-10-17). So in that sense this patch can\n    only introduce user-visible errors (since any \"bugs\" were going to\n    /dev/null before), but the idea is to catch them rather than quietly\n    send garbage to /dev/null.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 16 ++++++++--------\n diff.h |  2 --\n 2 files changed, 8 insertions(+), 10 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex d83d898702..a8d50fb1fc 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -1351,7 +1351,7 @@ static void emit_diff_symbol_from_struct(struct diff_options *o,\n \tint len = eds->len;\n \tunsigned flags = eds->flags;\n \n-\tif (o->dry_run)\n+\tif (!o->file)\n \t\treturn;\n \n \tswitch (s) {\n@@ -3765,9 +3765,9 @@ static void builtin_diff(const char *name_a,\n \n \t\tif (o->word_diff)\n \t\t\tinit_diff_words_data(&ecbdata, o, one, two);\n-\t\tif (o->dry_run) {\n+\t\tif (!o->file) {\n \t\t\t/*\n-\t\t\t * Unlike the !dry_run case, we need to ignore the\n+\t\t\t * Unlike the normal output case, we need to ignore the\n \t\t\t * return value from xdi_diff_outf() here, because\n \t\t\t * xdi_diff_outf() takes non-zero return from its\n \t\t\t * callback function as a sign of error and returns\n@@ -4423,7 +4423,7 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;\n+\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || !o->file;\n \tint rc;\n \n \t/*\n@@ -4621,7 +4621,7 @@ static void run_diff_cmd(const struct external_diff *pgm,\n \t\t    p->status == DIFF_STATUS_RENAMED)\n \t\t\to->found_changes = 1;\n \t} else {\n-\t\tif (!o->dry_run)\n+\t\tif (o->file)\n \t\t\tfprintf(o->file, \"* Unmerged path %s\\n\", name);\n \t\to->found_changes = 1;\n \t}\n@@ -6199,15 +6199,15 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)\n /* return 1 if any change is found; otherwise, return 0 */\n static int diff_flush_patch_quietly(struct diff_filepair *p, struct diff_options *o)\n {\n-\tint saved_dry_run = o->dry_run;\n+\tFILE *saved_file = o->file;\n \tint saved_found_changes = o->found_changes;\n \tint ret;\n \n-\to->dry_run = 1;\n+\to->file = NULL;\n \to->found_changes = 0;\n \tdiff_flush_patch(p, o);\n \tret = o->found_changes;\n-\to->dry_run = saved_dry_run;\n+\to->file = saved_file;\n \to->found_changes |= saved_found_changes;\n \treturn ret;\n }\ndiff --git a/diff.h b/diff.h\nindex 2fa256c3ef..31eedd5c0c 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -408,8 +408,6 @@ struct diff_options {\n \t#define COLOR_MOVED_WS_ERROR (1<<0)\n \tunsigned color_moved_ws_handling;\n \n-\tbool dry_run;\n-\n \tstruct repository *repo;\n \tstruct strmap *additional_path_headers;\n \n-- \n2.51.1.797.g1148beab57\n\n"},{"id":"529623","messageId":"20251024170925.GD2345184@coredump.intra.peff.net","threadId":"64382","inReplyTo":"20251024170522.GA2344972@coredump.intra.peff.net","subject":"[PATCH 4/4] diff: drop dry-run redirection to /dev/null","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:09:25Z","receivedAt":"2025-10-24T17:09:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"As an added protection against dry-run diffs accidentally producing\noutput, we redirect diff_options.file to /dev/null. But as of the\nprevious patch, this now does nothing, since dry-run diffs are\nimplemented by setting \"file\" to NULL.\n\nSo we can drop this extra code with no change in behavior. This is\neffectively a revert of 623f7af284 (diff: restore redirection to\n/dev/null for diff_from_contents, 2025-10-17) and 3da4413dbc (diff: make\nsure the other caller of diff_flush_patch_quietly() is silent,\n2025-10-22), but:\n\n  1. We get a conflict because we already dropped the color_moved\n     handling in an earlier patch. But we just resolve the conflicts to\n     \"theirs\" (removing all of the code).\n\n  2. We retain the test from 623f7af284.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n diff.c | 31 +++----------------------------\n 1 file changed, 3 insertions(+), 28 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex a8d50fb1fc..9169ccfaa9 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6835,35 +6835,18 @@ void diff_flush(struct diff_options *options)\n \t\t\t     DIFF_FORMAT_NAME |\n \t\t\t     DIFF_FORMAT_NAME_STATUS |\n \t\t\t     DIFF_FORMAT_CHECKDIFF)) {\n-\t\t/*\n-\t\t * make sure diff_Flush_patch_quietly() to be silent.\n-\t\t */\n-\t\tFILE *dev_null = NULL;\n-\n-\t\tif (options->flags.diff_from_contents) {\n-\t\t\tdev_null = xfopen(\"/dev/null\", \"w\");\n-\t\t}\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \n \t\t\tif (!check_pair_status(p))\n \t\t\t\tcontinue;\n \n-\t\t\tif (options->flags.diff_from_contents) {\n-\t\t\t\tFILE *saved_file = options->file;\n-\t\t\t\tint found_changes;\n+\t\t\tif (options->flags.diff_from_contents &&\n+\t\t\t    !diff_flush_patch_quietly(p, options))\n+\t\t\t\tcontinue;\n \n-\t\t\t\toptions->file = dev_null;\n-\t\t\t\tfound_changes = diff_flush_patch_quietly(p, options);\n-\t\t\t\toptions->file = saved_file;\n-\t\t\t\tif (!found_changes)\n-\t\t\t\t\tcontinue;\n-\t\t\t}\n \t\t\tflush_one_pair(p, options);\n \t\t}\n-\t\tif (options->flags.diff_from_contents) {\n-\t\t\tfclose(dev_null);\n-\t\t}\n \t\tseparator++;\n \t}\n \n@@ -6914,14 +6897,6 @@ void diff_flush(struct diff_options *options)\n \tif (output_format & DIFF_FORMAT_NO_OUTPUT &&\n \t    options->flags.exit_with_status &&\n \t    options->flags.diff_from_contents) {\n-\t\t/*\n-\t\t * run diff_flush_patch for the exit status. setting\n-\t\t * options->file to /dev/null should be safe, because we\n-\t\t * aren't supposed to produce any output anyway.\n-\t\t */\n-\t\tdiff_free_file(options);\n-\t\toptions->file = xfopen(\"/dev/null\", \"w\");\n-\t\toptions->close_file = 1;\n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n \t\t\tif (check_pair_status(p))\n-- \n2.51.1.797.g1148beab57\n"},{"id":"529624","messageId":"xmqqcy6cp637.fsf@gitster.g","threadId":"64382","inReplyTo":"20251024170853.GC2345184@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] diff: replace diff_options.dry_run flag with NULL file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-24T17:22:52Z","receivedAt":"2025-10-24T17:22:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> We introduced a dry_run flag to diff_options in b55e6d36eb (diff: ensure\n> consistent diff behavior with ignore options, 2025-08-08), with the idea\n> that the lower-level diff code could skip output when it is set.\n>\n> As we saw with the bugs fixed by 3ed5d8bd73 (diff: stop output garbled\n> message in dry run mode, 2025-10-20), it is easy to miss spots. In the\n> end, we located all of them by checking where diff_options.file is used.\n>\n> That suggests another possible approach: we can replace the dry_run\n> boolean with a NULL pointer for \"file\", as we know that using \"file\" in\n> dry_run mode would always be an error. This turns any missed spots from\n> producing extra output[1] into a segfault. Which is less forgiving, but\n> that is the point: this is indicative of a programming error, and\n> complaining loudly and immediately is good.\n\nMakes sense.\n"},{"id":"529625","messageId":"20251024172507.GA3081793@coredump.intra.peff.net","threadId":"64382","inReplyTo":"20251024170522.GA2344972@coredump.intra.peff.net","subject":"[PATCH 5/4] diff: simplify run_external_diff() quiet logic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-10-24T17:25:07Z","receivedAt":"2025-10-24T17:25:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We'd sometimes end up in run_external_diff() to do a dry-run diff (e.g.,\nto find content-level changes for --quiet). We recognize this quiet mode\nby seeing the lack of DIFF_FORMAT_PATCH in the output format.\n\nBut since introducing an explicit dry-run check via 3ed5d8bd73 (diff:\nstop output garbled message in dry run mode, 2025-10-20), this logic can\nnever trigger. We can only get to this function by calling\ndiff_flush_patch(), and that comes from only two places:\n\n  1. A dry-run flush comes from diff_flush_patch_quietly(), which is\n     always in dry-run mode (so the other half of our \"||\" is true\n     anyway).\n\n  2. A regular flush comes from diff_flush_patch_all_file_pairs(),\n     which is only called when output_format has DIFF_FORMAT_PATCH in\n     it.\n\nSo we can simplify our \"quiet\" condition to just checking dry-run mode\n(which used to be a specific flag, but recently became just a NULL\n\"file\" pointer). And since it's so simple, we can just do that inline.\nThis makes the logic about o->file more obvious, since we handle the\nNULL and non-stdout cases next to each other.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nHere's one more that I forgot to poke at before sending out the others.\n\n diff.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 9169ccfaa9..a1961526c0 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4423,7 +4423,6 @@ static void run_external_diff(const struct external_diff *pgm,\n {\n \tstruct child_process cmd = CHILD_PROCESS_INIT;\n \tstruct diff_queue_struct *q = &diff_queued_diff;\n-\tint quiet = !(o->output_format & DIFF_FORMAT_PATCH) || !o->file;\n \tint rc;\n \n \t/*\n@@ -4432,7 +4431,7 @@ static void run_external_diff(const struct external_diff *pgm,\n \t * external diff program lacks the ability to tell us whether\n \t * it's empty then we consider it non-empty without even asking.\n \t */\n-\tif (!pgm->trust_exit_code && quiet) {\n+\tif (!pgm->trust_exit_code && !o->file) {\n \t\to->found_changes = 1;\n \t\treturn;\n \t}\n@@ -4457,7 +4456,7 @@ static void run_external_diff(const struct external_diff *pgm,\n \tdiff_free_filespec_data(one);\n \tdiff_free_filespec_data(two);\n \tcmd.use_shell = 1;\n-\tif (quiet)\n+\tif (!o->file)\n \t\tcmd.no_stdout = 1;\n \telse if (o->file != stdout)\n \t\tcmd.out = xdup(fileno(o->file));\n-- \n2.51.1.797.g1148beab57\n\n"}]}