Re: [PATCH] diff: stop output garbled message in dry run mode
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 17, 2025, 12:07 UTC
- Message-ID
- <4ff55fc5-7880-b8bf-257f-3186552e9c36@gmx.de>
- In-Reply-To
- <pull.2071.git.git.1760671049113.gitgitgadget@gmail.com>
Hi,
On Fri, 17 Oct 2025, Lidong Yan via GitGitGadget wrote:
Show 25 quoted lines
> From: Lidong Yan <yldhome2d2@gmail.com> > > In dry run mode, diff_flush_patch() should not produce any output. > However, in commit b55e6d36eb (diff: ensure consistent diff behavior > with ignore options, 2025-08-08), only the output during the > comparison of two file contents was suppressed. For file deletions > or mode changes, diff_flush_patch() still produces output. In > run_extern_diff(), set quiet to true if in dry run mode. In > emit_diff_symbol_from_struct(), directly return if in dry run mode. > > Signed-off-by: Lidong Yan <yldhome2d2@gmail.com> > > [...] > > diff --git a/diff.c b/diff.c > index 87fa16b730..4baf9b535e 100644 > --- a/diff.c > +++ b/diff.c > @@ -1351,6 +1351,9 @@ static void emit_diff_symbol_from_struct(struct diff_options *o, > int len = eds->len; > unsigned flags = eds->flags; > > + if (o->dry_run) > + return; > +
Very good. This is a minimal change that covers all of the `emit_*()` calls (except for `checkdiff_consume()`, but if the `--check` code path is entered under `o->dry_run`, it is debatable whether or not it should output something, therefore we could claim that this is "by design").
I do see a still-unguarded `fprintf(o->file, ...)` call in `run_diff_cmd()`, but as far as I can see, this call is not in any code path where `dry_run` is set. Granted, this is quite tedious to reason about and requires considerable cognitive load to analyze, but judging from past attempts to land patches that simplify logic e.g. in https://lore.kernel.org/git/pull.1888.git.1743079429.gitgitgadget@gmail.com/ I have concluded that core reviewers on this mailing list delight too much in such analyses to be interested in making Git's code easier to reason about.
Show 36 quoted lines
> switch (s) {
> case DIFF_SYMBOL_NO_LF_EOF:
> context = diff_get_color_opt(o, DIFF_CONTEXT);
> @@ -4420,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);
> + int quiet = !(o->output_format & DIFF_FORMAT_PATCH) || o->dry_run;
> int rc;
>
> /*
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 55a06eadb3..25fa452656 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -661,6 +661,27 @@ test_expect_success 'diff -I<regex>: ignore matching file' '
> test_grep ! "file1" actual
> '
>
> +test_expect_success 'diff -I<regex>: ignore all content changes' '
> + test_when_finished "git rm -f file1 file2" &&
> + : >file1 &&
> + git add file1 &&
> + : >file2 &&
> + git add file2 &&
> +
> + rm -f file1 file2 &&
> + mkdir file2 &&
> + test_diff_no_content_changes () {
> + git diff $1 --ignore-blank-lines -I".*" >actual &&
> + test_line_count = 2 actual &&
> + test_grep "file1" actual &&
> + test_grep "file2" actual &&
> + test_grep ! "diff --git" actual
> + } &&Nice! While this function obviously is not strictly scoped to this test case (it will still be defined when the next test case is executed), it is wonderful to see the structure that helps readers along.
Show 10 quoted lines
> + test_diff_no_content_changes "--raw" &&
> + test_diff_no_content_changes "--name-only" &&
> + test_diff_no_content_changes "--name-status"
> +'
> +
> # check_prefix <patch> <src> <dst>
> # check only lines with paths to avoid dependency on exact oid/contents
> check_prefix () {
>
> base-commit: 143f58ef7535f8f8a80d810768a18bdf3807de26Thank you for fixing this so quickly! From my point of view, this is ready to go. I will integrate this patch into Git for Windows v2.51.1 (which I am sadly forced to release on a Friday).
Ciao, Johannes