[PATCH] diff: tighten interaction between -w and --exit-code
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 16, 2023, 23:45 UTC
- Message-ID
- <xmqqv8ded018.fsf@gitster.g>
- In-Reply-To
- <xmqqbkfh8nu3.fsf@gitster.g>
The "diff" family of commands have "--exit-code" option to make them report if they "saw" changes with their exit code, but it did not work when options in the "ignore space" family is used *and* the output mode did not involve the "--patch" format output, unless the output is totally suppressed with "-s".
Internally, the diff machinery grabs sets of paths from the two sides being compared that record different blob objects, optionally matches these paths up for rename processing, and compares the blob objects pair-wise. When options like "-w" are *not* involved, the machinery can say that it see a difference immediately after noticing that the object names of the blobs at paths being compared are different. In essence, this means that a non-empty diff_queue[] means the command should exit with 1 under "--exit-code" mode.
But when options that may make two different blobs compare as equivalents, like "-w" that ignores whitespace differences, are in effect, the blobs need to be compared for their contents, as if we were generating a patch, even when the user is only interested in "--exit-code" and not an actual differences.
There are two special case tweaks in the existing code. One is that the codepath to compute "--patch" output sets .found_changes bit only when it sees a "real" change (whose definition is loosened when "-w" is in effect to ignore whitespace-only changes), and uses that bit, instead of whether diff_queue[] has any paths, to decide the exit status. The other is that when "-s" (no output) is combined with "-w", the machinery calls the same "--patch" codepath but redirecting the output to void, only for the .found_changes bit.
That is fine, until somebody comes and tries to combine options like "--stat", "--name-only", "--name-status", etc. with "-w". Because the second special case above to run a fallback "--patch" computation does not kick in for these other output modes, .found_changes bit is not updated correctly.
To fix this, do two things:
* The codepath to generate "--stat" output already calls the underlying xdiff machinery with appropriate options like "-w", but it did not update .found_changes bit. Fixing "--stat -w" combined with "--exit-code" thus becomes just the matter of adding these missing .found_changes assignment.
* For generating "--name-only", "--name-status", etc., the code does not look into the contents of the blob objects at all. For now, extend the special case used for "-s -w --exit-code" to run a silent "--patch" computation to set the .found_changes bit correctly.
Not that the latter may still not be correct in that a path whose contents have no differences other than whitespace changes would still show up in the "diff -w --name-only --exit-code" output, even though the exit status may say there is no differences. Arguably this is better than status quo, even though it still is wrong.
Reported-by: Paul Watson <pwatson2@wellmed.net> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Junio C Hamano <gitster@pobox.com> writes:
Show 23 quoted lines
> Also, this is not limited to the no-index mode. > > $ echo one >1 > $ git add 1 > $ echo two >1 > $ git diff --exit-code --numstat 1; echo "<<$?>>" > 1 1 1 > <<1>> > $ git diff --exit-code --numstat -w 1; echo "<<$?>>" > 1 1 1 > <<0>> > > So the minimum reproduction seems to be > > * the diff machinery is asked to do --exit-code (no-index > implicitly does it) > * -w is used > * -p is *not* used > * to compare two different files. > > Thanks for a bug report. > > Patches welcome ;-)
So I ended up looking into this myself X-<.
diff.c | 22 +++++++++- t/t4015-diff-whitespace.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 119 insertions(+), 3 deletions(-)
diff --git c/diff.c w/diff.c index ee3eb629e3..38b57b589f 100644 --- c/diff.c +++ w/diff.c @@ -3795,6 +3795,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, } else { data->added = diff_filespec_size(o->repo, two); data->deleted = diff_filespec_size(o->repo, one); + o->found_changes = 1; } } @@ -3803,6 +3804,7 @@ static void builtin_diffstat(const char *name_a, const char *name_b, diff_populate_filespec(o->repo, two, NULL); data->deleted = count_lines(one->data, one->size); data->added = count_lines(two->data, two->size); + o->found_changes = 1; } else if (may_differ) { @@ -3828,6 +3830,11 @@ static void builtin_diffstat(const char *name_a, const char *name_b, diffstat_consume, diffstat, &xpp, &xecfg)) die("unable to generate diffstat for %s", one->path); + /* Do this before cancelling the no-op diffstat below */ + if (diffstat->files[diffstat->nr - 1]->added || + diffstat->files[diffstat->nr - 1]->deleted) + o->found_changes = 1; + if (DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two)) { struct diffstat_file *file = diffstat->files[diffstat->nr - 1]; @@ -4832,6 +4839,11 @@ void diff_setup_done(struct diff_options *options) else options->prefix_length = 0; + /* + * This is how "diff --name-status -p --stat --raw" becomes + * equivalent to "diff --name-status", which may be + * unintuitive. + */ if (options->output_format & (DIFF_FORMAT_NAME | DIFF_FORMAT_NAME_STATUS | DIFF_FORMAT_CHECKDIFF | @@ -6684,13 +6696,19 @@ void diff_flush(struct diff_options *options) separator++; } - if (output_format & DIFF_FORMAT_NO_OUTPUT && + if (((output_format & DIFF_FORMAT_NO_OUTPUT) || + /* these compute .found_changes properly */ + !(output_format & (DIFF_FORMAT_DIFFSTAT| + DIFF_FORMAT_SHORTSTAT| + DIFF_FORMAT_NUMSTAT| + DIFF_FORMAT_DIRSTAT| + DIFF_FORMAT_PATCH))) && 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. + * aren't supposed to produce any output from here. */ diff_free_file(options); options->file = xfopen("/dev/null", "w"); diff --git c/t/t4015-diff-whitespace.sh w/t/t4015-diff-whitespace.sh index b298f220e0..1b944d77e4 100755 --- c/t/t4015-diff-whitespace.sh +++ w/t/t4015-diff-whitespace.sh @@ -1,7 +1,7 @@ #!/bin/sh # # Copyright (c) 2006 Johannes E. Schindelin -# +# Copyright (c) 2023 Google LLC test_description='Test special whitespace in diff engine. @@ -11,6 +11,104 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh +test_expect_success 'exit status with -w --name-only (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --name-only --exit-code x +' + +test_expect_success 'exit status with -w --name-only (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --name-only --exit-code x +' + +test_expect_success 'exit status with -w --raw (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --raw --exit-code x +' + +test_expect_success 'exit status with -w --raw (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --raw --exit-code x +' + +test_expect_success 'exit status with -w --stat (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --stat --exit-code x +' + +test_expect_success 'exit status with -w --stat (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --stat --exit-code x +' + +test_expect_success 'exit status with -w --shortstat (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --shortstat --exit-code x +' + +test_expect_success 'exit status with -w --shortstat (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --shortstat --exit-code x +' + +test_expect_success 'exit status with -w --quiet (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --quiet --exit-code x +' + +test_expect_success 'exit status with -w --quiet (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --quiet --exit-code x +' + +test_expect_success 'exit status with -w --summary (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w --summary --exit-code x +' + +test_expect_success 'exit status with -w --summary (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w --summary --exit-code x +' + +test_expect_success 'exit status with -w -s (different)' ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w -s --exit-code x +' + +test_expect_success 'exit status with -w -s (different but equivalent)' ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w -s --exit-code x +' + test_expect_success "Ray Lehtiniemi's example" ' cat <<-\EOF >x && do {