| Message ID | 20230818235932.3253552-1-gitster@pobox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix interactions with "-w" and "--exit-code" | expand |
On Fri, Aug 18, 2023 at 04:59:27PM -0700, Junio C Hamano wrote: > Paul Watson reported that "diff --no-index --exit-code > --ignore-all-space" does not work when used with "--shortstat". It > turns out that this is not limited to "--no-index" mode, and it is > not limited to "--shortstat". Anything that does not use the "--patch" > machinery to discover the content level differences ignored --exit-code > when used with options like "-w" and always exited with 0. In fact, > even the "--patch" machinery was slightly faulty in corner cases. > > And here is another round to fix it. > Previous one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/ This looks good to me. I left some musings, but I don't think there is anything that merits a re-roll (you might want to fix-up a commit message typo locally). Thanks for running with it. It's much nicer than v1. :) -Peff
Paul Watson reported that "diff --no-index --exit-code --ignore-all-space" does not work when used with "--shortstat". It turns out that this is not limited to "--no-index" mode, and it is not limited to "--shortstat". Anything that does not use the "--patch" machinery to discover the content level differences ignored --exit-code when used with options like "-w" and always exited with 0. In fact, even the "--patch" machinery was slightly faulty in corner cases. And here is another round to fix it. Previous one is at https://lore.kernel.org/git/20230817222949.3835424-1-gitster@pobox.com/ The interdiff is not all that interesting. One patch dropped, two patches stay the same, one patch has one-line change, and the final one patch has been completely reworked. Here is the summary: * The first patch about dirstat leak-fix is now gone. The series has instead been rebased on top of the official "dirstat leak-fix" series that was merged in Git 2.41. * The next patch (preliminary code clean-up) stays the same. It is now [1/5] instead of being the second step. * The next patch is to fix the corner case bug of "--patch" machinery. The code stays the same, but the tests were asking the filesystem to do something impossible when they do not have executable bit support, so a prerequisite has been added to work around them. * The next patch is to fix "--stat -w --exit-code", which stays the same. * The last step is completely new. v2 took an approach to reuse the "--patch" machinery even for "--raw" and other modes, but it would mean that "diff --raw --exit-code" may exit with 0 even when it has different paths to report, which is confusing. v3 changes the approach to align the exit status with the presence of reported paths that are different better. "--raw" has ignored "-w" when producing its output. It should ignore "-w" when reporting differences with its exit code, instead of always exiting with 0. Junio C Hamano (5): diff: move the fallback "--exit-code" code down diff: mode-only change should be noticed by "--patch -w --exit-code" diff: teach "--stat -w --exit-code" to notice differences t4040: remove test that succeeded for a wrong reason diff: the -w option breaks --exit-code for --raw and other output modes diff.c | 40 ++++++++++++++++++++++-------------- t/t4015-diff-whitespace.sh | 39 ++++++++++++++++++++++++++++++++++- t/t4040-whitespace-status.sh | 3 +-- 3 files changed, 64 insertions(+), 18 deletions(-) Range-diff against v2: 1: 65ff4655a2 < -: ---------- diff: --dirstat leakfix 2: 533f974c9b ! 1: df869ac550 diff: move the fallback "--exit-code" code down @@ Commit message diff: move the fallback "--exit-code" code down When "--exit-code" is asked and the code cannot just answer by - comparing the object names on both sides but need to inspect and + comparing the object names on both sides but needs to inspect and compare the contents, there are two ways that the result is found out. Some output modes, like "--stat" and "--patch", inherently have to - inspect the contents in order to _show_ the differences in the way + inspect the contents in order to show the differences in the way they do. The codepaths for these modes set the .found_changes bit as they compute what to show. However, other output modes do not need to inspect the contents to show the differences in the way they do. The most notable example - is "--quiet", which does not need to compute any output. When they - are asked to report "--exit-code", they run the codepaths for the - "--patch" output with their output redirected to "/dev/null", only - to set the .found_changes bit. + is "--quiet", which does not need to compute any output to show. + When they are asked to report "--exit-code", they run the codepaths + for the "--patch" output with their output redirected to "/dev/null", + only to set the .found_changes bit. Currently, this fallback invocation of "--patch" output is done after the "--stat" output format and its friends and before the 3: 89338b9302 ! 2: b349ad5278 diff: mode-only change should be noticed by "--patch -w --exit-code" @@ Metadata ## Commit message ## diff: mode-only change should be noticed by "--patch -w --exit-code" - The codepath to notice the content-level changes, taking certaion + The codepath to notice the content-level changes, taking certain no-op changes like "ignore whitespace" into account, forgot that a mode-only change is still a change. This resulted in @@ t/t4015-diff-whitespace.sh: TEST_PASSES_SANITIZE_LEAK=true + test_expect_code 1 git diff -w $opts --exit-code x + ' + -+ test_expect_success "status with $opts (mode differs)" ' ++ test_expect_success POSIXPERM "status with $opts (mode differs)" ' + test_when_finished "git update-index --chmod=-x x" && + echo foo >x && + git add x && 4: 8fc63f4a04 ! 3: be50b023a8 diff: teach "--stat -w --exit-code" to notice differences @@ Commit message $ git diff --stat -w --exit-code - for a change that does have an outout to exit with status 0. + for a change that does have an output to exit with status 0. Set the bit by inspecting the list of paths the diffstat output is given for (a mode-only change will still appear as a "0-line added 5: 200593e9e0 < -: ---------- diff: teach "--name-status" and friends to honor "--exit-code -w" -: ---------- > 4: d1a6fa7d17 t4040: remove test that succeeded for a wrong reason -: ---------- > 5: 573f810cce diff: the -w option breaks --exit-code for --raw and other output modes base-commit: 83973981eb475ce90f829f8a5bd6ea99cd3bbd8e