Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes

From: Jeff King <peff@peff.net>
Date: Aug 21, 2023, 21:00 UTC
Message-ID: <20230821210058.GD1800481@coredump.intra.peff.net>
In-Reply-To: <20230818235932.3253552-6-gitster@pobox.com>

On Fri, Aug 18, 2023 at 04:59:32PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> The output from "--raw", "--name-status", and "--name-only" modes in
> "git diff" does depend on and does not reflect how certain different
> contents are considered equal, unlike "--patch" and "--stat" output
> modes do, when used with options like "-w" (another way of thinking
> about it is that it is not like we recompute the hash of the blob
> after removing all whitespaces to show "git diff --raw -w" output).
> 
> But that is not an excuse for "git diff --exit-code --raw" to fail
> to report differences with its exit status, when used with options
> like "-w".  Make sure the command exits with 1 when these options
> report paths that are different.

I think s/with options like/without options like/ in the final paragraph?

I like this approach much better than the earlier round. It brings the output and exit code of "--raw", etc, into harmony. I am open to the argument that if you ask for "--raw -w", we should start doing content-level comparisons (since otherwise the "-w" is somewhat useless). But even if we went that route, it should affect both the output and the exit code. So this patch would be a prerequisite anyway.

And I say "I am open to" and not "I think it is a good idea" above, because I suspect there are many lurking corner cases. The hash output you mentioned is one. But also, what "--raw --patch -w" does right now is sensible (show the raw output, but also show the whitespace-ignoring patch), and would be impossible if "-w" affected "--raw".

Show 15 quoted lines

> diff --git a/diff.c b/diff.c
> index da965ff688..78f4e7518f 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -4744,6 +4744,10 @@ void diff_setup_done(struct diff_options *options)
>  	else
>  		options->prefix_length = 0;
>  
> +	/*
> +	 * --name-only, --name-status, --checkdiff, and -s
> +	 * turn other output format off.
> +	 */
>  	if (options->output_format & (DIFF_FORMAT_NAME |
>  				      DIFF_FORMAT_NAME_STATUS |
>  				      DIFF_FORMAT_CHECKDIFF |

OK, this hunk is just documenting the current state, not changing anything.

Show 7 quoted lines

> @@ -6072,6 +6076,8 @@ static void flush_one_pair(struct diff_filepair *p, struct diff_options *opt)
>  		fprintf(opt->file, "%s", diff_line_prefix(opt));
>  		write_name_quoted(name_a, opt->file, opt->line_termination);
>  	}
> +
> +	opt->found_changes = 1;
>  }

This seems deceptively simple. :) The key here is that flush_one_pair() is called only for the formats that are otherwise broken (because they are not looking at the content). I think this is a good spot to put the fix, as any similar formats would hopefully get added to the conditional in diff_flush() that puts us here.

Alternatively, we could put it in the caller, like so:

diff --git a/diff.c b/diff.c
index 78f4e7518f..e7281e75eb 100644
--- a/diff.c
+++ b/diff.c
@@ -6528,6 +6528,7 @@ void diff_flush(struct diff_options *options)
 			if (check_pair_status(p))
 				flush_one_pair(p, options);
 		}
+		options->found_changes = !!q->nr;
 		separator++;
 	}
 

which matches the diffstat technique (I almost thought we could share
the code, but for the diffstat we are counting what ends up in the
diffstat struct; it does not clean out the original diff_queue when it
sees a noop pair).

I don't see a real reason to prefer one over the other.

> -for opts in --patch --quiet -s --stat --shortstat --dirstat=lines
> +for opt_res in --patch --quiet -s --stat --shortstat --dirstat=lines \
> +	       --raw! --name-only! --name-status!
>  do
> +	opts=${opt_res%!} expect_failure=
> +	test "$opts" = "$opt_res" ||
> +		expect_failure="test_expect_code 1"

Cute "!" convention. :)

-Peff