Re: [PATCH] diff: restore redirection to /dev/null for diff_from_contents
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 17, 2025, 18:22 UTC
- Message-ID
- <xmqqzf9pz8uq.fsf@gitster.g>
- In-Reply-To
- <20251017083641.GB4073661@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 21 quoted lines
>> Looking at that patch, my biggest concern is: are we missing other spots >> that need to special-case the dry_run setting? Because it's a regression >> in a maint release, I'm tempted to say we should do the dumbest possible >> thing that covers all cases and just revert this hunk from the original >> patch, like: > > Here it is with a commit message and test, in case that is helpful. > > -- >8 -- > Subject: [PATCH] diff: restore redirection to /dev/null for diff_from_contents > ... > I didn't test, but I also wondered if this might be necessary to avoid > actual external diff programs from spewing to stdout. Looking at > run_external_diff(), we do: > > int quiet = !(o->output_format & DIFF_FORMAT_PATCH); > [...] > cmd.no_stdout = quiet; > > so I _think_ it should be OK even without this patch. But again, I like > the extra layer of protection here.
I do like this direction, in addition I really do appreciate your thought above on optimizing ext-diff and textconv away when they are not necessary (obviously outside the scope of the regression fix).
I also wonder if we want to get rid of the new code related to the "dry-run" mode that we no longer have to use. But as a regression fix that wants to be minimum, I think this patch stops at the right place.
Thanks.
Show 39 quoted lines
> diff.c | 9 +++++++++
> t/t4035-diff-quiet.sh | 4 ++++
> 2 files changed, 13 insertions(+)
>
> diff --git a/diff.c b/diff.c
> index 87fa16b730..687206f353 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -6890,6 +6890,15 @@ 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;
> + options->color_moved = 0;
> for (i = 0; i < q->nr; i++) {
> struct diff_filepair *p = q->queue[i];
> if (check_pair_status(p))
> diff --git a/t/t4035-diff-quiet.sh b/t/t4035-diff-quiet.sh
> index 0352bf81a9..35eaf0855f 100755
> --- a/t/t4035-diff-quiet.sh
> +++ b/t/t4035-diff-quiet.sh
> @@ -50,6 +50,10 @@ test_expect_success 'git diff-tree HEAD HEAD' '
> test_expect_code 0 git diff-tree --quiet HEAD HEAD >cnt &&
> test_line_count = 0 cnt
> '
> +test_expect_success 'git diff-tree -w HEAD^ HEAD' '
> + test_expect_code 1 git diff-tree --quiet -w HEAD^ HEAD >cnt &&
> + test_line_count = 0 cnt
> +'
> test_expect_success 'git diff-files' '
> test_expect_code 0 git diff-files --quiet >cnt &&
> test_line_count = 0 cnt