* git bug report @ 2023-08-04 16:46 Paul Watson 2023-08-04 17:28 ` rsbecker 2023-08-08 17:07 ` Junio C Hamano 0 siblings, 2 replies; 52+ messages in thread From: Paul Watson @ 2023-08-04 16:46 UTC (permalink / raw) To: git Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue. What did you do before the bug happened? (Steps to reproduce your issue) 9:43:55.45 2023-08-04 C:\src\t\scripts>TYPE .\t1.txt this is t1.txt 9:43:57.92 2023-08-04 C:\src\t\scripts>TYPE .\t2.txt this is t2.txt 9:43:58.04 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index --ignore-all-space --shortstat .\t1.txt .\t2.txt 1 file changed, 1 insertion(+), 1 deletion(-) 9:43:58.14 2023-08-04 C:\src\t\scripts>ECHO %ERRORLEVEL% 0 What did you expect to happen? (Expected behavior) I expected that the exit code from `git diff` would be 1, or something non-zero because the files are different. What happened instead? (Actual behavior) The exit code was zero (0). What's different between what you expected and what actually happened? Zero (0) is not one (1). :-) Anything else you want to add: Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.39.2.windows.1 cpu: x86_64 built from commit: a82fa99b36ddfd643e61ed45e52abe314687df67 sizeof-long: 4 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Windows 10.0 19044 compiler info: gnuc: 12.2 libc info: no libc information available $SHELL (typically, interactive shell): <unset> PowerShell 7.3.6 Console on Windows 10 [Enabled Hooks] This e-mail, including attachments, may include confidential and/or proprietary information, and may be used only by the person or entity to which it is addressed. If the reader of this e-mail is not the intended recipient or intended recipient’s authorized agent, the reader is hereby notified that any dissemination, distribution or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender by replying to this message and delete this e-mail immediately. ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: git bug report 2023-08-04 16:46 git bug report Paul Watson @ 2023-08-04 17:28 ` rsbecker 2023-08-04 17:48 ` [EXTERNAL] " Paul Watson 2023-08-08 17:07 ` Junio C Hamano 1 sibling, 1 reply; 52+ messages in thread From: rsbecker @ 2023-08-04 17:28 UTC (permalink / raw) To: 'Paul Watson', git On Friday, August 4, 2023 12:46 PM, Paul Watson wrote: >Thank you for filling out a Git bug report! >Please answer the following questions to help us understand your issue. > >What did you do before the bug happened? (Steps to reproduce your issue) > >9:43:55.45 2023-08-04 C:\src\t\scripts>TYPE .\t1.txt this is t1.txt > >9:43:57.92 2023-08-04 C:\src\t\scripts>TYPE .\t2.txt this is t2.txt > >9:43:58.04 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit- >code --no-index --ignore-all-space --shortstat .\t1.txt .\t2.txt >1 file changed, 1 insertion(+), 1 deletion(-) Can you retry this without --shortstat so we can see what git is reporting as a difference. Thanks, Randall ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [EXTERNAL] RE: git bug report 2023-08-04 17:28 ` rsbecker @ 2023-08-04 17:48 ` Paul Watson 2023-08-04 18:12 ` rsbecker 0 siblings, 1 reply; 52+ messages in thread From: Paul Watson @ 2023-08-04 17:48 UTC (permalink / raw) To: rsbecker, git -----Original Message----- From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> Sent: Friday, August 4, 2023 12:29 >> Can you retry this without --shortstat so we can see what git is >> reporting as a difference. The problem appears to be the use of --shortstat. When --shortstat is not used, the exit code for different files is one (1), as expected. 12:46:56.88 2023-08-04 C:\src\t\scripts>TYPE t1.txt this is t1.txt this is t1.txt this is t1.txt 12:47:01.88 2023-08-04 C:\src\t\scripts>type t2.txt this is t2.txt this is t2.txt this is t2.txt 12:47:06.41 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index --ignore-all-space --shortstat .\t1.txt .\t2.txt 1 file changed, 3 insertions(+), 3 deletions(-) 12:47:18.22 2023-08-04 C:\src\t\scripts>ECHO %ERRORLEVEL% 0 12:47:20.61 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index --ignore-all-space .\t1.txt .\t2.txt diff --git a/./t1.txt b/./t2.txt index 6dfb884..66812c5 100644 --- a/./t1.txt +++ b/./t2.txt @@ -1,3 +1,3 @@ -this is t1.txt -this is t1.txt -this is t1.txt +this is t2.txt +this is t2.txt +this is t2.txt 12:47:27.71 2023-08-04 C:\src\t\scripts>ECHO %ERRORLEVEL% 1 12:47:30.32 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --quiet --exit-code --no-index --ignore-all-space .\t1.txt .\t2.txt 12:47:36.58 2023-08-04 C:\src\t\scripts>ECHO %ERRORLEVEL% 1 This e-mail, including attachments, may include confidential and/or proprietary information, and may be used only by the person or entity to which it is addressed. If the reader of this e-mail is not the intended recipient or intended recipient’s authorized agent, the reader is hereby notified that any dissemination, distribution or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender by replying to this message and delete this e-mail immediately. ^ permalink raw reply related [flat|nested] 52+ messages in thread
* RE: [EXTERNAL] RE: git bug report 2023-08-04 17:48 ` [EXTERNAL] " Paul Watson @ 2023-08-04 18:12 ` rsbecker 2023-08-07 15:46 ` Paul Watson 0 siblings, 1 reply; 52+ messages in thread From: rsbecker @ 2023-08-04 18:12 UTC (permalink / raw) To: 'Paul Watson', git On Friday, August 4, 2023 1:49 PM, Paul Watson wrote: >From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> >Sent: Friday, August 4, 2023 12:29 > >>> Can you retry this without --shortstat so we can see what git is >>> reporting as a difference. > >The problem appears to be the use of --shortstat. When --shortstat is not used, the >exit code for different files is one (1), as expected. I am not convinced this is a defect. The result of --shortstat is to report that there are differences, so it is possible that 0 is correct here. Others might have a different opinion. I can recreate without including any other options. ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [EXTERNAL] RE: git bug report 2023-08-04 18:12 ` rsbecker @ 2023-08-07 15:46 ` Paul Watson 2023-08-08 13:07 ` Paul Watson 0 siblings, 1 reply; 52+ messages in thread From: Paul Watson @ 2023-08-07 15:46 UTC (permalink / raw) To: rsbecker, git From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> > I am not convinced this is a defect. The result of --shortstat is to > report that there are differences, so it is possible that 0 is correct > here. Others might have a different opinion. I can recreate without > including any other options. The code has already done the work to identify if there are differences. Why would it not return a meaningful exit code? This e-mail, including attachments, may include confidential and/or proprietary information, and may be used only by the person or entity to which it is addressed. If the reader of this e-mail is not the intended recipient or intended recipient’s authorized agent, the reader is hereby notified that any dissemination, distribution or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender by replying to this message and delete this e-mail immediately. ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [EXTERNAL] RE: git bug report 2023-08-07 15:46 ` Paul Watson @ 2023-08-08 13:07 ` Paul Watson 2023-08-08 13:28 ` rsbecker 0 siblings, 1 reply; 52+ messages in thread From: Paul Watson @ 2023-08-08 13:07 UTC (permalink / raw) To: rsbecker, git >From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> > >> I am not convinced this is a defect. The result of --shortstat is to >> report that there are differences, so it is possible that 0 is correct >> here. Others might have a different opinion. I can recreate without >> including any other options. > > The code has already done the work to identify if there are differences. > Why would it not return a meaningful exit code? Also, since the --exit-code switch has been specified, it should set the exit code accordingly. This e-mail, including attachments, may include confidential and/or proprietary information, and may be used only by the person or entity to which it is addressed. If the reader of this e-mail is not the intended recipient or intended recipient’s authorized agent, the reader is hereby notified that any dissemination, distribution or copying of this e-mail is prohibited. If you have received this e-mail in error, please notify the sender by replying to this message and delete this e-mail immediately. ^ permalink raw reply [flat|nested] 52+ messages in thread
* RE: [EXTERNAL] RE: git bug report 2023-08-08 13:07 ` Paul Watson @ 2023-08-08 13:28 ` rsbecker 0 siblings, 0 replies; 52+ messages in thread From: rsbecker @ 2023-08-08 13:28 UTC (permalink / raw) To: git On Tuesday, August 8, 2023 9:08 AM, Paul Watson wrote: >>From: rsbecker@nexbridge.com <rsbecker@nexbridge.com> >> >>> I am not convinced this is a defect. The result of --shortstat is to >>> report that there are differences, so it is possible that 0 is >>> correct here. Others might have a different opinion. I can recreate >>> without including any other options. >> >> The code has already done the work to identify if there are differences. > Why >would it not return a meaningful exit code? > >Also, since the --exit-code switch has been specified, it should set the exit code >accordingly. I think this is a decision for the maintainers, whether I agree or disagree. I have to defer to others. --Randall ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: git bug report 2023-08-04 16:46 git bug report Paul Watson 2023-08-04 17:28 ` rsbecker @ 2023-08-08 17:07 ` Junio C Hamano 2023-08-16 23:45 ` [PATCH] diff: tighten interaction between -w and --exit-code Junio C Hamano 1 sibling, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-08 17:07 UTC (permalink / raw) To: Paul Watson; +Cc: git Paul Watson <pwatson2@wellmed.net> writes: > 9:43:55.45 2023-08-04 C:\src\t\scripts>TYPE .\t1.txt > this is t1.txt > 9:43:57.92 2023-08-04 C:\src\t\scripts>TYPE .\t2.txt > this is t2.txt > > 9:43:58.04 2023-08-04 C:\src\t\scripts>"C:\Program Files\Git\cmd\git.exe" diff --exit-code --no-index --ignore-all-space --shortstat .\t1.txt .\t2.txt > 1 file changed, 1 insertion(+), 1 deletion(-) > > 9:43:58.14 2023-08-04 C:\src\t\scripts>ECHO %ERRORLEVEL% > 0 This is not specific to Windows port and it can be reproduced on a random Linux box. $ echo one >1 $ echo two >2 $ git diff --no-index --shortstat 1 2; echo "<<$?>>" 1 file changed, 1 insertion(+), 1 deletion(-) <<1>> $ git diff --no-index --shortstat -w 1 2; echo "<<$?>>" 1 file changed, 1 insertion(+), 1 deletion(-) <<0>> Note that I omitted "--exit-code" in the above reproduction, as it is always used in the no-index mode. There seems to be interaction with "-w" and not using "-p", as this is not limited to "--shortstat". $ git diff --no-index -w --stat 1 2; echo "<<$?>>" 1 => 2 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) <<0>> $ git diff --no-index -w --numstat 1 2; echo "<<$?>>" 1 1 1 => 2 <<0>> All of the above that exits with 0 status will exit with 1 when -p is added to the command line. 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 ;-) ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH] diff: tighten interaction between -w and --exit-code 2023-08-08 17:07 ` Junio C Hamano @ 2023-08-16 23:45 ` Junio C Hamano 2023-08-17 5:10 ` Jeff King 0 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-16 23:45 UTC (permalink / raw) To: git; +Cc: Paul Watson 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: > 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 { ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] diff: tighten interaction between -w and --exit-code 2023-08-16 23:45 ` [PATCH] diff: tighten interaction between -w and --exit-code Junio C Hamano @ 2023-08-17 5:10 ` Jeff King 2023-08-17 16:12 ` Junio C Hamano 0 siblings, 1 reply; 52+ messages in thread From: Jeff King @ 2023-08-17 5:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Paul Watson On Wed, Aug 16, 2023 at 04:45:23PM -0700, Junio C Hamano wrote: > 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. Nicely explained overall, but one hunk of the patch left me wondering... > @@ -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; > + So this is checking whether any lines were added/deleted to see if the stat was a noop. But what about non-content bits, like mode changes? E.g., the tests below all fail (but would pass without "-w"): diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 1b944d77e4..54e56ad911 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -25,6 +25,14 @@ test_expect_success 'exit status with -w --name-only (different but equivalent)' git diff -w --name-only --exit-code x ' +test_expect_success 'exit status with -w --name-only (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --name-only --exit-code +' + test_expect_success 'exit status with -w --raw (different)' ' echo foo >x && git add x && @@ -39,6 +47,14 @@ test_expect_success 'exit status with -w --raw (different but equivalent)' ' git diff -w --raw --exit-code x ' +test_expect_success 'exit status with -w --raw (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --raw --exit-code +' + test_expect_success 'exit status with -w --stat (different)' ' echo foo >x && git add x && @@ -53,6 +69,14 @@ test_expect_success 'exit status with -w --stat (different but equivalent)' ' git diff -w --stat --exit-code x ' +test_expect_success 'exit status with -w --stat (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --stat --exit-code +' + test_expect_success 'exit status with -w --shortstat (different)' ' echo foo >x && git add x && @@ -67,6 +91,14 @@ test_expect_success 'exit status with -w --shortstat (different but equivalent)' git diff -w --shortstat --exit-code x ' +test_expect_success 'exit status with -w --shortstat (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --shortstat --exit-code +' + test_expect_success 'exit status with -w --quiet (different)' ' echo foo >x && git add x && @@ -81,6 +113,14 @@ test_expect_success 'exit status with -w --quiet (different but equivalent)' ' git diff -w --quiet --exit-code x ' +test_expect_success 'exit status with -w --quiet (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --quiet --exit-code +' + test_expect_success 'exit status with -w --summary (different)' ' echo foo >x && git add x && @@ -95,6 +135,14 @@ test_expect_success 'exit status with -w --summary (different but equivalent)' ' git diff -w --summary --exit-code x ' +test_expect_success 'exit status with -w --summary (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w --summary --exit-code +' + test_expect_success 'exit status with -w -s (different)' ' echo foo >x && git add x && @@ -109,6 +157,14 @@ test_expect_success 'exit status with -w -s (different but equivalent)' ' git diff -w -s --exit-code x ' +test_expect_success 'exit status with -w -s (mode change)' ' + test_when_finished "git reset" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w -s --exit-code x +' + test_expect_success "Ray Lehtiniemi's example" ' cat <<-\EOF >x && do { For the diffstat case, I think we could check the mode here, but there are other cases (e.g., adding or deleting an empty file). The code right below the hunk I quoted seems to try to deal with that (the "cancelling the no-op" your comment mentions). I'm not sure if we want something like this: diff --git a/diff.c b/diff.c index 38b57b589f..1dbfdaeff0 100644 --- a/diff.c +++ b/diff.c @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b, && one->mode == two->mode) { free_diffstat_file(file); diffstat->nr--; + } else { + o->found_changes = 1; } } } but I haven't dug too far (and of course all of the other options also need something similar to catch this case). -Peff ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH] diff: tighten interaction between -w and --exit-code 2023-08-17 5:10 ` Jeff King @ 2023-08-17 16:12 ` Junio C Hamano 2023-08-17 19:49 ` Jeff King 0 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 16:12 UTC (permalink / raw) To: Jeff King; +Cc: git, Paul Watson Jeff King <peff@peff.net> writes: > For the diffstat case, I think we could check the mode here, but there > are other cases (e.g., adding or deleting an empty file). The code right > below the hunk I quoted seems to try to deal with that (the "cancelling > the no-op" your comment mentions). I'm not sure if we want something > like this: > > diff --git a/diff.c b/diff.c > index 38b57b589f..1dbfdaeff0 100644 > --- a/diff.c > +++ b/diff.c > @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b, > && one->mode == two->mode) { > free_diffstat_file(file); > diffstat->nr--; > + } else { > + o->found_changes = 1; > } > } > } That is much better. In all cases where the above diffstat->nr-- is not reached and diffstat is kept is where we found changes, so an even simpler solution that fundamentally cannot go wrong would be to see "diffstat->nr" at the end (i.e. "are we going to show diffstat for *any* filepair?"). If it is non-zero, we did find a difference. Then we do not have to wonder if that else clause is in the right place, or we have to do something similar to the above for cases where DIFF_FILE_VALID() is not true for both sides (i.e. creation or deletion). Thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] diff: tighten interaction between -w and --exit-code 2023-08-17 16:12 ` Junio C Hamano @ 2023-08-17 19:49 ` Jeff King 2023-08-17 19:56 ` Junio C Hamano 0 siblings, 1 reply; 52+ messages in thread From: Jeff King @ 2023-08-17 19:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Paul Watson On Thu, Aug 17, 2023 at 09:12:09AM -0700, Junio C Hamano wrote: > > diff --git a/diff.c b/diff.c > > index 38b57b589f..1dbfdaeff0 100644 > > --- a/diff.c > > +++ b/diff.c > > @@ -3853,6 +3853,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b, > > && one->mode == two->mode) { > > free_diffstat_file(file); > > diffstat->nr--; > > + } else { > > + o->found_changes = 1; > > } > > } > > } > > That is much better. In all cases where the above diffstat->nr-- is > not reached and diffstat is kept is where we found changes, so an > even simpler solution that fundamentally cannot go wrong would be to > see "diffstat->nr" at the end (i.e. "are we going to show diffstat > for *any* filepair?"). If it is non-zero, we did find a difference. Yeah, without having really dug into the problem too far, that does sound a lot better. I also wonder to what degree you could apply the same strategy to other formats (I guess it depends on them removing whitespace-only changes from a structure). From the test I posted earlier, it does look like many of them have the same blind spots for mode-only changes (and I suspect addition/removal of empty files is another corner case to check). -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] diff: tighten interaction between -w and --exit-code 2023-08-17 19:49 ` Jeff King @ 2023-08-17 19:56 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 0 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 19:56 UTC (permalink / raw) To: Jeff King; +Cc: git, Paul Watson Jeff King <peff@peff.net> writes: > Yeah, without having really dug into the problem too far, that does > sound a lot better. I also wonder to what degree you could apply the > same strategy to other formats (I guess it depends on them removing > whitespace-only changes from a structure). From the test I posted > earlier, it does look like many of them have the same blind spots for > mode-only changes (and I suspect addition/removal of empty files is > another corner case to check). I have something cooking. Stay tuned, without getting excited too much ;-) Thanks. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" 2023-08-17 19:56 ` Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 1/5] diff: --dirstat leakfix Junio C Hamano ` (6 more replies) 0 siblings, 7 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git So here is the reworked series. The previous attempt mostly correspond to [4/5] of this series, but the approach has been quite updated, so here is marked as "v2" but I omitted the interdiff as it would be more or less useless. The first patch is not part of the main theme, but because I'll be adding the first use of "--dirstat" to t4015 that has been leak sanitizer clean, and the "--dirstat" codepath has known leaks, I am plugging the leaks as a preliminary step to avoid having to mark t4015 as leak sanitizer unclean again. The second patch flips around order of processing without changing the meaning. After this change, blocks that compute output that also take contents into account are grouped together, before the fallback code for output formats that do not look at the contents to compute their result. It is purely done as a clean-up. The earlier series claimed that "--exit-code -w" is reliable as long as "--patch" output is used, but it turns out that there are corner case holes in the "--patch" codepath. The third patch fixes them. The fourth patch teachs "--stat" codepath to help "--exit-code -w"; as it looks at contents to produce its output, we note the fact that we found (or did not find) differences, and use that for the exit code computation. The fifth patch deals with other output modes that do not look at contents for their output to reuse the fallback code. Junio C Hamano (5): diff: --dirstat leakfix 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 diff: teach "--name-status" and friends to honor "--exit-code -w" diff.c | 71 +++++++++++++++++++++++++++----------- t/t4015-diff-whitespace.sh | 36 ++++++++++++++++++- t/t4047-diff-dirstat.sh | 2 ++ 3 files changed, 87 insertions(+), 22 deletions(-) -- 2.42.0-rc2 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 1/5] diff: --dirstat leakfix 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 2/5] diff: move the fallback "--exit-code" code down Junio C Hamano ` (5 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git The dirstat_dir structure holds a list of files that had "damages" and is used to summarize the change by directory. It was allocated, used, and then left behind, leaking. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 14 ++++++++++++-- t/t4047-diff-dirstat.sh | 2 ++ 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/diff.c b/diff.c index 648f6717a5..03d0cfc700 100644 --- a/diff.c +++ b/diff.c @@ -2977,6 +2977,7 @@ static void show_dirstat(struct diff_options *options) unsigned long changed; struct dirstat_dir dir; struct diff_queue_struct *q = &diff_queued_diff; + struct dirstat_file *to_free; dir.files = NULL; dir.alloc = 0; @@ -3060,13 +3061,17 @@ static void show_dirstat(struct diff_options *options) dir.nr++; } + to_free = dir.files; + /* This can happen even with many files, if everything was renames */ if (!changed) - return; + goto free_return; /* Show all directories with more than x% of the changes */ QSORT(dir.files, dir.nr, dirstat_compare); gather_dirstat(options, &dir, changed, "", 0); +free_return: + free(to_free); } static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *options) @@ -3074,6 +3079,7 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o int i; unsigned long changed; struct dirstat_dir dir; + struct dirstat_file *to_free; if (data->nr == 0) return; @@ -3104,13 +3110,17 @@ static void show_dirstat_by_line(struct diffstat_t *data, struct diff_options *o dir.nr++; } + to_free = dir.files; + /* This can happen even with many files, if everything was renames */ if (!changed) - return; + goto free_return; /* Show all directories with more than x% of the changes */ QSORT(dir.files, dir.nr, dirstat_compare); gather_dirstat(options, &dir, changed, "", 0); +free_return: + free(to_free); } static void free_diffstat_file(struct diffstat_file *f) diff --git a/t/t4047-diff-dirstat.sh b/t/t4047-diff-dirstat.sh index 7fec2cb9cd..70224c3da1 100755 --- a/t/t4047-diff-dirstat.sh +++ b/t/t4047-diff-dirstat.sh @@ -1,6 +1,8 @@ #!/bin/sh test_description='diff --dirstat tests' + +TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh # set up two commits where the second commit has these files -- 2.42.0-rc2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 2/5] diff: move the fallback "--exit-code" code down 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 1/5] diff: --dirstat leakfix Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano ` (4 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git When "--exit-code" is asked and the code cannot just answer by comparing the object names on both sides but need 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 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. Currently, this fallback invocation of "--patch" output is done after the "--stat" output format and its friends and before the "--patch" and internal callback logic. Move it to the end of the sequence to clarify the fallback status of this code block. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/diff.c b/diff.c index 03d0cfc700..ce9c8272c7 100644 --- a/diff.c +++ b/diff.c @@ -6555,6 +6555,21 @@ void diff_flush(struct diff_options *options) separator++; } + if (output_format & DIFF_FORMAT_PATCH) { + if (separator) { + emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); + if (options->stat_sep) + /* attach patch instead of inline */ + emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, + NULL, 0, 0); + } + + diff_flush_patch_all_file_pairs(options); + } + + if (output_format & DIFF_FORMAT_CALLBACK) + options->format_callback(q, options, options->format_callback_data); + if (output_format & DIFF_FORMAT_NO_OUTPUT && options->flags.exit_with_status && options->flags.diff_from_contents) { @@ -6576,21 +6591,6 @@ void diff_flush(struct diff_options *options) } } - if (output_format & DIFF_FORMAT_PATCH) { - if (separator) { - emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); - if (options->stat_sep) - /* attach patch instead of inline */ - emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, - NULL, 0, 0); - } - - diff_flush_patch_all_file_pairs(options); - } - - if (output_format & DIFF_FORMAT_CALLBACK) - options->format_callback(q, options, options->format_callback_data); - for (i = 0; i < q->nr; i++) diff_free_filepair(q->queue[i]); free_queue: -- 2.42.0-rc2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 1/5] diff: --dirstat leakfix Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 2/5] diff: move the fallback "--exit-code" code down Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-18 21:15 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 4/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano ` (3 subsequent siblings) 6 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git The codepath to notice the content-level changes, taking certaion no-op changes like "ignore whitespace" into account, forgot that a mode-only change is still a change. This resulted in $ git diff --patch --exit-code -w to exit with status 0 even when there is such a mode-only change, breaking both "--patch" and "--quiet" output formats. Teach the builtin_diff() codepath that creation and deletion as well as mode changes are all interesting changes. Note that the test specifically checks removal of an empty file, because if there is anything in the preimage (i.e. the removed file is not empty), the removal would still trigger textual patch output and the codepath for that does update .found_changes bit to report that it found an interesting change. We need to make sure that the .found_changes bit is set even without triggering textual patch output. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 3 +++ t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index ce9c8272c7..de18364902 100644 --- a/diff.c +++ b/diff.c @@ -3501,18 +3501,21 @@ static void builtin_diff(const char *name_a, strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset); if (xfrm_msg) strbuf_addstr(&header, xfrm_msg); + o->found_changes = 1; must_show_header = 1; } else if (lbl[1][0] == '/') { strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset); if (xfrm_msg) strbuf_addstr(&header, xfrm_msg); + o->found_changes = 1; must_show_header = 1; } else { if (one->mode != two->mode) { strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset); strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset); + o->found_changes = 1; must_show_header = 1; } if (xfrm_msg) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index f3e20dd5bb..943ad252d4 100755 --- a/t/t4015-diff-whitespace.sh +++ b/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,39 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh +for opts in --patch --quiet -s +do + + test_expect_success "status with $opts (different)" ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w $opts --exit-code x + ' + + test_expect_success "status with $opts (mode differs)" ' + test_when_finished "git update-index --chmod=-x x" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w $opts --exit-code x + ' + + test_expect_success "status with $opts (removing an empty file)" ' + : >x && + git add x && + rm x && + test_expect_code 1 git diff -w $opts --exit-code -- x + ' + + test_expect_success "status with $opts (different but equivalent)" ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w $opts --exit-code x + ' +done + test_expect_success "Ray Lehtiniemi's example" ' cat <<-\EOF >x && do { -- 2.42.0-rc2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" 2023-08-17 22:29 ` [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano @ 2023-08-18 21:15 ` Junio C Hamano 0 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 21:15 UTC (permalink / raw) To: git Junio C Hamano <gitster@pobox.com> writes: > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > index f3e20dd5bb..943ad252d4 100755 > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -11,6 +11,39 @@ TEST_PASSES_SANITIZE_LEAK=true > + ... > + test_expect_success "status with $opts (mode differs)" ' > + test_when_finished "git update-index --chmod=-x x" && > + echo foo >x && > + git add x && > + git update-index --chmod=+x x && > + test_expect_code 1 git diff -w $opts --exit-code x > + ' Apparently this one needs to skipped on a filesystem without support for the executable bit. cf. https://github.com/git/git/actions/runs/5897310248/job/15996914969#step:5:218 I'll give POSIXPERM prerequisite to this test piece. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v2 4/5] diff: teach "--stat -w --exit-code" to notice differences 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano ` (2 preceding siblings ...) 2023-08-17 22:29 ` [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 5/5] diff: teach "--name-status" and friends to honor "--exit-code -w" Junio C Hamano ` (2 subsequent siblings) 6 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git When options like "-w" is used while "--exit-code" option is in effect, instead of the usual "do we have any filepair whose preimage and postimage have different <mode,object>?" check, we need to compare the contents of the blobs, taking into account that certain changes are considered no-op. With the previous step, we taught "--patch" codepath to set the .found_changes bit correctly, even for a change that only affects the mode and not object. The "--stat" codepath, however, did not set the .found_changes bit at all. This lead to $ git diff --stat -w --exit-code for a change that does have an outout 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 0-line deleted" change) to fix it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 1 + t/t4015-diff-whitespace.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index de18364902..7213237675 100644 --- a/diff.c +++ b/diff.c @@ -6905,6 +6905,7 @@ void compute_diffstat(struct diff_options *options, if (check_pair_status(p)) diff_flush_stat(p, options, diffstat); } + options->found_changes = !!diffstat->nr; } void diff_addremove(struct diff_options *options, diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 943ad252d4..355d96aa14 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh -for opts in --patch --quiet -s +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines do test_expect_success "status with $opts (different)" ' -- 2.42.0-rc2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v2 5/5] diff: teach "--name-status" and friends to honor "--exit-code -w" 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano ` (3 preceding siblings ...) 2023-08-17 22:29 ` [PATCH v2 4/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano @ 2023-08-17 22:29 ` Junio C Hamano 2023-08-17 22:37 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano 6 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:29 UTC (permalink / raw) To: git We have fallback code that is used when "-s -w --exit-code" options are used together to compute the exit code, because the exit code must take the whitespace-ignoring comparison into account over the contents, but with "-s", the normal codepath does not even have to look at the contents at all. The fallback code simply runs a "git diff --patch" while sending the output to "/dev/null". The codepaths for some other output modes, like "--name-status" and "--raw", share the same trait as "-s" in that they do not look at the contents for their output generation. Extend the fallback code to cover these output modes as well. Note that they 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 may be wrong. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 23 +++++++++++++++++++---- t/t4015-diff-whitespace.sh | 3 ++- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index 7213237675..3bb9d11bfe 100644 --- a/diff.c +++ b/diff.c @@ -6573,13 +6573,28 @@ void diff_flush(struct diff_options *options) if (output_format & DIFF_FORMAT_CALLBACK) options->format_callback(q, options, options->format_callback_data); - 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. + * We need to inspect the contents, not just object + * names, to determine the exit status, but the usual + * processing for the output format specified does not + * have to work with and does not look at the + * contents. Run an extra and silent "diff --patch" + * but discard the output to /dev/null, so that we + * would set the .found_changes bit correctly. + * + * We can safely close and discard the original output + * file here, since all that is left to do from this + * point is to return (we don't do the FORMAT_PATCH + * thing below). */ diff_free_file(options); options->file = xfopen("/dev/null", "w"); diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 355d96aa14..412d20181c 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -11,7 +11,8 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh -for opts in --patch --quiet -s --stat --shortstat --dirstat=lines +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines \ + --name-only --raw --name-status --summary do test_expect_success "status with $opts (different)" ' -- 2.42.0-rc2 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano ` (4 preceding siblings ...) 2023-08-17 22:29 ` [PATCH v2 5/5] diff: teach "--name-status" and friends to honor "--exit-code -w" Junio C Hamano @ 2023-08-17 22:37 ` Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano 6 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-17 22:37 UTC (permalink / raw) To: git Junio C Hamano <gitster@pobox.com> writes: > The first patch is not part of the main theme, but because I'll be > adding the first use of "--dirstat" to t4015 that has been leak > sanitizer clean, and the "--dirstat" codepath has known leaks, I am > plugging the leaks as a preliminary step to avoid having to mark > t4015 as leak sanitizer unclean again. I completely forgot that I already have done 1e1dcb2a (Merge branch 'jc/dirstat-plug-leaks', 2023-05-15) in Git 2.41 timeframe. I was basing my work on Git 2.38 maintenance track and needed [1/5] but in newer codebase it is not needed. ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 0/5] fix interactions with "-w" and "--exit-code" 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano ` (5 preceding siblings ...) 2023-08-17 22:37 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 1/5] diff: move the fallback "--exit-code" code down Junio C Hamano ` (5 more replies) 6 siblings, 6 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git 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 -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 1/5] diff: move the fallback "--exit-code" code down 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-21 20:41 ` Jeff King 2023-08-18 23:59 ` [PATCH v3 2/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano ` (4 subsequent siblings) 5 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git When "--exit-code" is asked and the code cannot just answer by 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 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 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 "--patch" and internal callback logic. Move it to the end of the sequence to clarify the fallback status of this code block. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/diff.c b/diff.c index d52db685f7..0ce678fc06 100644 --- a/diff.c +++ b/diff.c @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options) separator++; } + if (output_format & DIFF_FORMAT_PATCH) { + if (separator) { + emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); + if (options->stat_sep) + /* attach patch instead of inline */ + emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, + NULL, 0, 0); + } + + diff_flush_patch_all_file_pairs(options); + } + + if (output_format & DIFF_FORMAT_CALLBACK) + options->format_callback(q, options, options->format_callback_data); + if (output_format & DIFF_FORMAT_NO_OUTPUT && options->flags.exit_with_status && options->flags.diff_from_contents) { @@ -6572,21 +6587,6 @@ void diff_flush(struct diff_options *options) } } - if (output_format & DIFF_FORMAT_PATCH) { - if (separator) { - emit_diff_symbol(options, DIFF_SYMBOL_SEPARATOR, NULL, 0, 0); - if (options->stat_sep) - /* attach patch instead of inline */ - emit_diff_symbol(options, DIFF_SYMBOL_STAT_SEP, - NULL, 0, 0); - } - - diff_flush_patch_all_file_pairs(options); - } - - if (output_format & DIFF_FORMAT_CALLBACK) - options->format_callback(q, options, options->format_callback_data); - for (i = 0; i < q->nr; i++) diff_free_filepair(q->queue[i]); free_queue: -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 1/5] diff: move the fallback "--exit-code" code down 2023-08-18 23:59 ` [PATCH v3 1/5] diff: move the fallback "--exit-code" code down Junio C Hamano @ 2023-08-21 20:41 ` Jeff King 0 siblings, 0 replies; 52+ messages in thread From: Jeff King @ 2023-08-21 20:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, Aug 18, 2023 at 04:59:28PM -0700, Junio C Hamano wrote: > diff --git a/diff.c b/diff.c > index d52db685f7..0ce678fc06 100644 > --- a/diff.c > +++ b/diff.c > @@ -6551,6 +6551,21 @@ void diff_flush(struct diff_options *options) > separator++; > } > > + if (output_format & DIFF_FORMAT_PATCH) { > + if (separator) { This step makes sense, but I did a double-take when looking at the patch, because it is moving code _up_ rather than _down_. But the problem is that the block you moved was larger than the intervening bit, so the diff chose to flip-flop the context and changed bits. Obviously orthogonal to your series, but I wonder if there's a way to convince Git to show what actually happened. I don't think this is really a heuristic or algorithm problem. Seeing the pre- and post-images, it can't know whether it was "move A up" or "move B down", and the "real" diff is simply much larger in this case. -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 2/5] diff: mode-only change should be noticed by "--patch -w --exit-code" 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 1/5] diff: move the fallback "--exit-code" code down Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano ` (3 subsequent siblings) 5 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git 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 $ git diff --patch --exit-code -w to exit with status 0 even when there is such a mode-only change, breaking both "--patch" and "--quiet" output formats. Teach the builtin_diff() codepath that creation and deletion as well as mode changes are all interesting changes. Note that the test specifically checks removal of an empty file, because if there is anything in the preimage (i.e. the removed file is not empty), the removal would still trigger textual patch output and the codepath for that does update .found_changes bit to report that it found an interesting change. We need to make sure that the .found_changes bit is set even without triggering textual patch output. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 3 +++ t/t4015-diff-whitespace.sh | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 0ce678fc06..998d7ae20c 100644 --- a/diff.c +++ b/diff.c @@ -3497,18 +3497,21 @@ static void builtin_diff(const char *name_a, strbuf_addf(&header, "%s%snew file mode %06o%s\n", line_prefix, meta, two->mode, reset); if (xfrm_msg) strbuf_addstr(&header, xfrm_msg); + o->found_changes = 1; must_show_header = 1; } else if (lbl[1][0] == '/') { strbuf_addf(&header, "%s%sdeleted file mode %06o%s\n", line_prefix, meta, one->mode, reset); if (xfrm_msg) strbuf_addstr(&header, xfrm_msg); + o->found_changes = 1; must_show_header = 1; } else { if (one->mode != two->mode) { strbuf_addf(&header, "%s%sold mode %06o%s\n", line_prefix, meta, one->mode, reset); strbuf_addf(&header, "%s%snew mode %06o%s\n", line_prefix, meta, two->mode, reset); + o->found_changes = 1; must_show_header = 1; } if (xfrm_msg) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index f3e20dd5bb..02731dccb9 100755 --- a/t/t4015-diff-whitespace.sh +++ b/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,39 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh +for opts in --patch --quiet -s +do + + test_expect_success "status with $opts (different)" ' + echo foo >x && + git add x && + echo bar >x && + test_expect_code 1 git diff -w $opts --exit-code x + ' + + test_expect_success POSIXPERM "status with $opts (mode differs)" ' + test_when_finished "git update-index --chmod=-x x" && + echo foo >x && + git add x && + git update-index --chmod=+x x && + test_expect_code 1 git diff -w $opts --exit-code x + ' + + test_expect_success "status with $opts (removing an empty file)" ' + : >x && + git add x && + rm x && + test_expect_code 1 git diff -w $opts --exit-code -- x + ' + + test_expect_success "status with $opts (different but equivalent)" ' + echo foo >x && + git add x && + echo " foo" >x && + git diff -w $opts --exit-code x + ' +done + test_expect_success "Ray Lehtiniemi's example" ' cat <<-\EOF >x && do { -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 1/5] diff: move the fallback "--exit-code" code down Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 2/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-21 20:45 ` Jeff King 2023-08-18 23:59 ` [PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason Junio C Hamano ` (2 subsequent siblings) 5 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git When options like "-w" is used while "--exit-code" option is in effect, instead of the usual "do we have any filepair whose preimage and postimage have different <mode,object>?" check, we need to compare the contents of the blobs, taking into account that certain changes are considered no-op. With the previous step, we taught "--patch" codepath to set the .found_changes bit correctly, even for a change that only affects the mode and not object. The "--stat" codepath, however, did not set the .found_changes bit at all. This lead to $ git diff --stat -w --exit-code 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 0-line deleted" change) to fix it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 1 + t/t4015-diff-whitespace.sh | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/diff.c b/diff.c index 998d7ae20c..da965ff688 100644 --- a/diff.c +++ b/diff.c @@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options, if (check_pair_status(p)) diff_flush_stat(p, options, diffstat); } + options->found_changes = !!diffstat->nr; } void diff_addremove(struct diff_options *options, diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 02731dccb9..230a89b951 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh -for opts in --patch --quiet -s +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines do test_expect_success "status with $opts (different)" ' -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences 2023-08-18 23:59 ` [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano @ 2023-08-21 20:45 ` Jeff King 0 siblings, 0 replies; 52+ messages in thread From: Jeff King @ 2023-08-21 20:45 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, Aug 18, 2023 at 04:59:30PM -0700, Junio C Hamano wrote: > 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 > 0-line deleted" change) to fix it. > [...] > diff --git a/diff.c b/diff.c > index 998d7ae20c..da965ff688 100644 > --- a/diff.c > +++ b/diff.c > @@ -6901,6 +6901,7 @@ void compute_diffstat(struct diff_options *options, > if (check_pair_status(p)) > diff_flush_stat(p, options, diffstat); > } > + options->found_changes = !!diffstat->nr; > } Makes sense, and this is delightfully simple. I wondered if we needed to remove any code setting found_changes via builtin_diffstat(), but it does not seem to exist in the first place. ;) > diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh > index 02731dccb9..230a89b951 100755 > --- a/t/t4015-diff-whitespace.sh > +++ b/t/t4015-diff-whitespace.sh > @@ -11,7 +11,7 @@ TEST_PASSES_SANITIZE_LEAK=true > . ./test-lib.sh > . "$TEST_DIRECTORY"/lib-diff.sh > > -for opts in --patch --quiet -s > +for opts in --patch --quiet -s --stat --shortstat --dirstat=lines > do I guess this would fix --numstat, too, though there may be diminishing returns in checking every format if we know it is just hitting the same code paths (though perhaps one could argue that --shortstat is already uninteresting for the same reason). -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* [PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano ` (2 preceding siblings ...) 2023-08-18 23:59 ` [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes Junio C Hamano 2023-08-21 21:02 ` [PATCH v3 0/5] fix interactions with "-w" and "--exit-code" Jeff King 5 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git "diff-tree -b --exit-code" without "--patch" exits with 0 status, not because it finds that the two input files are equivalent while ignoring whitespaces, but because the implied "--raw" mode always exits with 0 when whitespace tweaking options like "-b" and "-w" are given, which is a long-standing bug. We are about to fix it so that "--raw" and friends report the differences with the exit status (even though they ignore the whitespace tweaking options when producing their output), which will make this test, which succeeded for a wrong reason, start failing. Remove it. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- t/t4040-whitespace-status.sh | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/t/t4040-whitespace-status.sh b/t/t4040-whitespace-status.sh index e70e020ae9..eec3d73dc2 100755 --- a/t/t4040-whitespace-status.sh +++ b/t/t4040-whitespace-status.sh @@ -28,8 +28,7 @@ test_expect_success 'diff-tree --exit-code' ' test_expect_success 'diff-tree -b --exit-code' ' git diff -b --exit-code HEAD^ HEAD && - git diff-tree -b -p --exit-code HEAD^ HEAD && - git diff-tree -b --exit-code HEAD^ HEAD + git diff-tree -b -p --exit-code HEAD^ HEAD ' test_expect_success 'diff-index --cached --exit-code' ' -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano ` (3 preceding siblings ...) 2023-08-18 23:59 ` [PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason Junio C Hamano @ 2023-08-18 23:59 ` Junio C Hamano 2023-08-21 21:00 ` Jeff King 2023-08-21 21:02 ` [PATCH v3 0/5] fix interactions with "-w" and "--exit-code" Jeff King 5 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-18 23:59 UTC (permalink / raw) To: git 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. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- diff.c | 6 ++++++ t/t4015-diff-whitespace.sh | 8 ++++++-- 2 files changed, 12 insertions(+), 2 deletions(-) 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 | @@ -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; } static void show_file_mode_name(struct diff_options *opt, const char *newdelete, struct diff_filespec *fs) diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh index 230a89b951..7fcffa4b11 100755 --- a/t/t4015-diff-whitespace.sh +++ b/t/t4015-diff-whitespace.sh @@ -11,8 +11,12 @@ TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh . "$TEST_DIRECTORY"/lib-diff.sh -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" test_expect_success "status with $opts (different)" ' echo foo >x && @@ -40,7 +44,7 @@ do echo foo >x && git add x && echo " foo" >x && - git diff -w $opts --exit-code x + $expect_failure git diff -w $opts --exit-code x ' done -- 2.42.0-rc2-7-gf9972720e9 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-18 23:59 ` [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes Junio C Hamano @ 2023-08-21 21:00 ` Jeff King 2023-08-21 21:08 ` Jeff King ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Jeff King @ 2023-08-21 21:00 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Fri, Aug 18, 2023 at 04:59:32PM -0700, Junio C Hamano wrote: > 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". > 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. > @@ -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 ^ permalink raw reply related [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-21 21:00 ` Jeff King @ 2023-08-21 21:08 ` Jeff King 2023-08-21 22:23 ` Jeff King 2023-08-21 22:16 ` Junio C Hamano 2023-08-21 22:26 ` Junio C Hamano 2 siblings, 1 reply; 52+ messages in thread From: Jeff King @ 2023-08-21 21:08 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote: > 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. Actually, on second look these are not quite the same. Yours only triggers if check_pair_status() is true. So something like --diff-filter should affect both output and exit code. Yours gets that right, and mine does not. Sorry for the noise. :) -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-21 21:08 ` Jeff King @ 2023-08-21 22:23 ` Jeff King 0 siblings, 0 replies; 52+ messages in thread From: Jeff King @ 2023-08-21 22:23 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Aug 21, 2023 at 05:08:05PM -0400, Jeff King wrote: > On Mon, Aug 21, 2023 at 05:00:58PM -0400, Jeff King wrote: > > > 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. > > Actually, on second look these are not quite the same. Yours only > triggers if check_pair_status() is true. So something like > --diff-filter should affect both output and exit code. Yours gets that > right, and mine does not. Sorry for the noise. :) Sorry, I'm dumb again. That is not where diff-filter is handled (it is handled by diffcore, duh). check_pair_status() is only checking for DIFF_STATUS_UNKNOWN. I'm not sure when that would ever be set, but it seems like we should be matching the "if it is output" logic, which is what you get by calling flush_one_pair(). So yours is definitely preferable, even if I don't understand the possible differences. ;) -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-21 21:00 ` Jeff King 2023-08-21 21:08 ` Jeff King @ 2023-08-21 22:16 ` Junio C Hamano 2023-08-21 22:26 ` Junio C Hamano 2 siblings, 0 replies; 52+ messages in thread From: Junio C Hamano @ 2023-08-21 22:16 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: > 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++; > } Yup, I suspect they amount to the same thing in practice, but I couldn't come up with a good explanation to give casual readers of the conditional call to flush_one_pair() a few lines above why this is correct. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-21 21:00 ` Jeff King 2023-08-21 21:08 ` Jeff King 2023-08-21 22:16 ` Junio C Hamano @ 2023-08-21 22:26 ` Junio C Hamano 2023-08-22 1:30 ` Jeff King 2 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2023-08-21 22:26 UTC (permalink / raw) To: Jeff King; +Cc: git Jeff King <peff@peff.net> writes: >> ... >> 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? Sorry, I am confused. "diff --raw --exit-code" without "-w" reports with its exit status that it found differences just fine. When "-w" is given, without this patch, it always reports 0. What I wanted to convey was ... "--raw" and friends deliberately ignore "-w" and other "look at contents" options. The fact they do ignore the "look at contents" options is not a good excuse for "diff --raw -w --exit-code" to also ignore the request to report the differences with its exit status. "diff --raw --exit-code" does report the differences as requested, and we should do the same when given "-w". I guess the two have no logical connection so the latter sentence is not making sense, with its "with" kept as-is or changed to "without". ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes 2023-08-21 22:26 ` Junio C Hamano @ 2023-08-22 1:30 ` Jeff King 0 siblings, 0 replies; 52+ messages in thread From: Jeff King @ 2023-08-22 1:30 UTC (permalink / raw) To: Junio C Hamano; +Cc: git On Mon, Aug 21, 2023 at 03:26:37PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> ... > >> 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? > > Sorry, I am confused. "diff --raw --exit-code" without "-w" reports > with its exit status that it found differences just fine. When "-w" > is given, without this patch, it always reports 0. I think I just got confused by the multiple negatives "not an excuse to fail to...". > What I wanted to convey was ... > > "--raw" and friends deliberately ignore "-w" and other "look at > contents" options. > > The fact they do ignore the "look at contents" options is not a > good excuse for "diff --raw -w --exit-code" to also ignore the > request to report the differences with its exit status. "diff > --raw --exit-code" does report the differences as requested, and > we should do the same when given "-w". Yes, that conveys it more clearly (to me, anyway). -Peff ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH v3 0/5] fix interactions with "-w" and "--exit-code" 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano ` (4 preceding siblings ...) 2023-08-18 23:59 ` [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes Junio C Hamano @ 2023-08-21 21:02 ` Jeff King 5 siblings, 0 replies; 52+ messages in thread From: Jeff King @ 2023-08-21 21:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: git 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 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Git bug report @ 2022-10-10 23:09 Camden Narzt 0 siblings, 0 replies; 52+ messages in thread From: Camden Narzt @ 2022-10-10 23:09 UTC (permalink / raw) To: git What did you do before the bug happened? (Steps to reproduce your issue) Enabled core.fsmonitor in ~/.gitconfig What did you expect to happen? (Expected behavior) Faster git status, or at the very least not slower. What happened instead? (Actual behavior) Significantly slower git status. What's different between what you expected and what actually happened? With the fsmonitor git status takes like 2-8s when normally it's under 1s, I expected it to be faster than normal. Anything else you want to add: I've tried apple's git (used to generate this report) and the one from homebrew (2.38.0). The repo doesn’t seem to matter much, some are a bit faster than others, but they’re all really slow. I tried to track down the slowness using trace2.perfTarget and from that it seemed to spend a lot of time doing fsync. [System Info] git version: git version 2.37.0 (Apple Git-136) cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Darwin 21.6.0 Darwin Kernel Version 21.6.0: Mon Aug 22 20:17:10 PDT 2022; root:xnu-8020.140.49~2/RELEASE_X86_64 x86_64 compiler info: clang: 14.0.0 (clang-1400.0.29.201) libc info: no libc information available $SHELL (typically, interactive shell): /usr/local/bin/bash [Enabled Hooks] ^ permalink raw reply [flat|nested] 52+ messages in thread
* Git Bug Report @ 2022-04-14 7:22 Randall Alfaro 0 siblings, 0 replies; 52+ messages in thread From: Randall Alfaro @ 2022-04-14 7:22 UTC (permalink / raw) To: git [-- Attachment #1: Type: text/plain, Size: 306 bytes --] Hello Git Community. While using the Git tool on windows and a custom path for a private SSH (due to some policies and guidelines I've got to follow), I encountered a particular issue. I think this is related to a bug, I attach the template generated by git bugreport down below. Regards! Randall Alfaro [-- Attachment #2: git-bugreport-2022-04-14-0107.txt --] [-- Type: text/plain, Size: 1662 bytes --] Thank you for filling out a Git bug report! Please answer the following questions to help us understand your issue. What did you do before the bug happened? (Steps to reproduce your issue) R/ Set up GIT_SSH_COMMAND or core.sshCommand What did you expect to happen? (Expected behavior) R/ To be able to fetch/pull/push What happened instead? (Actual behavior) R/ "Disallowed command" was obtained by the server What's different between what you expected and what actually happened? R/ Any GIT command worked. Anything else you want to add: While debugging and troubleshooting, I noticed that the command "git-upload-pack" was getting sent two times. This is the command sent to the server after connecting to ssh (with -vvv) git-upload-pack '<PRIVATE PATH>' -o SendEnv=GIT_PROTOCOL -p <PRIVATE_PORT> git@<PRIVATE URL> git-upload-pack '<PRIVATE PATH>' After debugging as well the normal behavior when ssh is used but without GIT_SSH_COMMAND or core.sshCommand (by using the .ssh folder and config), the command is indeed only sent one time. My hypothesis is that this might be a bug, and sending the command two times might be causing GitLab to respond with the "Disallowed command" error. Please review the rest of the bug report below. You can delete any lines you don't wish to share. [System Info] git version: git version 2.35.2.windows.1 cpu: x86_64 built from commit: 518ccba2352ce721cabbbf2933869c3c3313d1c3 sizeof-long: 4 sizeof-size_t: 8 shell-path: /bin/sh feature: fsmonitor--daemon uname: Windows 10.0 19042 compiler info: gnuc: 11.2 libc info: no libc information available $SHELL (typically, interactive shell): <unset> [Enabled Hooks] ^ permalink raw reply [flat|nested] 52+ messages in thread
* Git Bug report @ 2011-10-04 21:24 Federico Lucifredi 2011-10-05 6:11 ` Johannes Sixt 2011-10-05 7:22 ` Fredrik Gustafsson 0 siblings, 2 replies; 52+ messages in thread From: Federico Lucifredi @ 2011-10-04 21:24 UTC (permalink / raw) To: git Hello Git list, Found a minor bug in git today - the error message reported is not correct when trying to access a repo that is not accessible permission-wise: > federico@skyplex:/etc$ git log > fatal: Not a git repository (or any of the parent directories): .git with correct access permissions, everything works as expected. > federico@skyplex:/etc$ sudo git log > commit 10a1d0eefcc100a513a9dff46839cff2c4f9b5a0 > Author: root <root@skyplex> > Date: Mon Oct 3 16:53:33 2011 -0400 > > saving uncommitted changes in /etc prior to apt run > > commit 2abb2b397631c7f48757bbcb029ebc38e37659d6 > Author: federico <federico@skyplex> > Date: Mon Oct 3 16:50:16 2011 -0400 > > updating firefox packages next >federico@skyplex:/etc$ > drwx------ 8 root root 4096 2011-10-03 16:53 .git That's it... I am not subscribed to the list, CC me in reply as needed. Best -Federico -- _________________________________________ -- "'Problem' is a bleak word for challenge" - Richard Fish (Federico L. Lucifredi) - federico at canonical.com - GnuPG 0x4A73884C ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-04 21:24 Git Bug report Federico Lucifredi @ 2011-10-05 6:11 ` Johannes Sixt 2011-10-05 18:32 ` Federico Lucifredi 2011-10-05 7:22 ` Fredrik Gustafsson 1 sibling, 1 reply; 52+ messages in thread From: Johannes Sixt @ 2011-10-05 6:11 UTC (permalink / raw) To: Federico Lucifredi; +Cc: git Am 10/4/2011 23:24, schrieb Federico Lucifredi: > Hello Git list, > Found a minor bug in git today - the error message reported is not > correct when trying to access a repo that is not accessible > permission-wise: > >> federico@skyplex:/etc$ git log >> fatal: Not a git repository (or any of the parent directories): .git > > with correct access permissions, everything works as expected. And the correct error message is...? >> drwx------ 8 root root 4096 2011-10-03 16:53 .git Assuming that you expected something like this: fatal: .git: permission denied it is hard to argue that a directory that happens to be named .git, but was sealed by its owner should be assumed to be a git repository, albeit one that we do not have access to. "Not a git repository" is an equally justifyable error message, IMHO. -- Hannes ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-05 6:11 ` Johannes Sixt @ 2011-10-05 18:32 ` Federico Lucifredi 0 siblings, 0 replies; 52+ messages in thread From: Federico Lucifredi @ 2011-10-05 18:32 UTC (permalink / raw) To: Johannes Sixt; +Cc: git On Wed, 2011-10-05 at 08:11 +0200, Johannes Sixt wrote: > >> federico@skyplex:/etc$ git log > >> fatal: Not a git repository (or any of the parent directories): .git > > > > with correct access permissions, everything works as expected. > > And the correct error message is...? ".git: permission denied" -- no need to be fatal (there could be ../.git, etc - fatal comes only if those don't exist). It could fail silently if one of the parents exists, but in the fatal scenario, I should be told that it was by permission denied. Currently, I am told there is no git repo where I know there to be one, which means "what happened to my repo" is the next question in the user's head. "it's there but it is broke" is the subtext one gets from this error. it should be "it is there, I cannot get to it". > >> drwx------ 8 root root 4096 2011-10-03 16:53 .git > > Assuming that you expected something like this: > > fatal: .git: permission denied > > it is hard to argue that a directory that happens to be named .git, but > was sealed by its owner should be assumed to be a git repository, albeit > one that we do not have access to. "Not a git repository" is an equally > justifyable error message, IMHO. An error message should help resolve the error in question, not grade the user's smarts. Here I as a user had an expectation that there was a git repository for /etc ("I set up one!") and figured the permission issue with my own wits (well, I did not need to because it was /etc, but in the general perm-denied case I would have had to), by looking at the dir because the message gave me no useful information on the cause of the problem. Thanks -Federico -- _________________________________________ -- "'Problem' is a bleak word for challenge" - Richard Fish (Federico L. Lucifredi) - federico at canonical.com - GnuPG 0x4A73884C ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-04 21:24 Git Bug report Federico Lucifredi 2011-10-05 6:11 ` Johannes Sixt @ 2011-10-05 7:22 ` Fredrik Gustafsson 2011-10-05 16:49 ` Junio C Hamano 1 sibling, 1 reply; 52+ messages in thread From: Fredrik Gustafsson @ 2011-10-05 7:22 UTC (permalink / raw) To: Federico Lucifredi; +Cc: git On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote: > Found a minor bug in git today - the error message reported is not > correct when trying to access a repo that is not accessible > permission-wise: > > > federico@skyplex:/etc$ git log > > fatal: Not a git repository (or any of the parent directories): .git > > with correct access permissions, everything works as expected. So if: .git/ is a directory with not enought permissions. ../.git/ is a directory with enought permissions. git would today use ../.git. You suggest that git instead would die because a .git/ exists? (I'm not saying this is wrong or right). -- Med vänliga hälsningar Fredrik Gustafsson E-post: iveqy@iveqy.com Tel. nr.: 0733 60 82 74 ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-05 7:22 ` Fredrik Gustafsson @ 2011-10-05 16:49 ` Junio C Hamano 2011-10-05 21:56 ` Nguyen Thai Ngoc Duy 2011-10-06 0:33 ` SZEDER Gábor 0 siblings, 2 replies; 52+ messages in thread From: Junio C Hamano @ 2011-10-05 16:49 UTC (permalink / raw) To: Fredrik Gustafsson; +Cc: Federico Lucifredi, git Fredrik Gustafsson <iveqy@iveqy.com> writes: > On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote: >> Found a minor bug in git today - the error message reported is not >> correct when trying to access a repo that is not accessible >> permission-wise: >> >> > federico@skyplex:/etc$ git log >> > fatal: Not a git repository (or any of the parent directories): .git >> >> with correct access permissions, everything works as expected. > > So if: > .git/ is a directory with not enought permissions. > ../.git/ is a directory with enought permissions. > > git would today use ../.git. You suggest that git instead would die > because a .git/ exists? (I'm not saying this is wrong or right). For that matter, if you have .git/ that is a directory but is not a repository, and ../.git/ that is, the same situation would arise. I do not think we should die because .git/ is not a git repository. I do not know if we should even warn about it. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-05 16:49 ` Junio C Hamano @ 2011-10-05 21:56 ` Nguyen Thai Ngoc Duy 2011-10-06 0:33 ` SZEDER Gábor 1 sibling, 0 replies; 52+ messages in thread From: Nguyen Thai Ngoc Duy @ 2011-10-05 21:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Fredrik Gustafsson, Federico Lucifredi, git On Thu, Oct 6, 2011 at 3:49 AM, Junio C Hamano <gitster@pobox.com> wrote: >> So if: >> .git/ is a directory with not enough permissions. >> ../.git/ is a directory with enough permissions. >> >> git would today use ../.git. You suggest that git instead would die >> because a .git/ exists? (I'm not saying this is wrong or right). > > For that matter, if you have .git/ that is a directory but is not a > repository, and ../.git/ that is, the same situation would arise. I do not > think we should die because .git/ is not a git repository. I do not know > if we should even warn about it. Probably not. On the other hand we should show user how we ignored .git if we find no good repository in the end. So maybe it's a good idea to queue up warnings and only print before git calls die("Not a repository"). -- Duy ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-05 16:49 ` Junio C Hamano 2011-10-05 21:56 ` Nguyen Thai Ngoc Duy @ 2011-10-06 0:33 ` SZEDER Gábor 2011-10-06 0:44 ` Junio C Hamano 1 sibling, 1 reply; 52+ messages in thread From: SZEDER Gábor @ 2011-10-06 0:33 UTC (permalink / raw) To: Junio C Hamano; +Cc: Fredrik Gustafsson, Federico Lucifredi, git On Wed, Oct 05, 2011 at 09:49:00AM -0700, Junio C Hamano wrote: > Fredrik Gustafsson <iveqy@iveqy.com> writes: > > > On Tue, Oct 04, 2011 at 05:24:03PM -0400, Federico Lucifredi wrote: > >> Found a minor bug in git today - the error message reported is not > >> correct when trying to access a repo that is not accessible > >> permission-wise: > >> > >> > federico@skyplex:/etc$ git log > >> > fatal: Not a git repository (or any of the parent directories): .git > >> > >> with correct access permissions, everything works as expected. > > > > So if: > > .git/ is a directory with not enought permissions. > > ../.git/ is a directory with enought permissions. > > > > git would today use ../.git. You suggest that git instead would die > > because a .git/ exists? (I'm not saying this is wrong or right). > > For that matter, if you have .git/ that is a directory but is not a > repository, and ../.git/ that is, the same situation would arise. I do not > think we should die because .git/ is not a git repository. I do not know > if we should even warn about it. And what about unreadable .git files? ~/tmp/git/outside$ git init Initialized empty Git repository in /home/szeder/tmp/git/outside/.git/ ~/tmp/git/outside$ mkdir inside repo ~/tmp/git/outside$ cd inside/ ~/tmp/git/outside/inside$ git init --separate-git-dir=../repo Initialized empty Git repository in /home/szeder/tmp/git/outside/repo/ ~/tmp/git/outside/inside$ git rev-parse --git-dir /home/szeder/tmp/git/outside/repo ~/tmp/git/outside/inside$ chmod a-r .git ~/tmp/git/outside/inside$ git rev-parse --git-dir fatal: Error opening '.git': Permission denied Or a non-gitfile .git file? ~/tmp/git/outside/inside$ chmod a+r .git ~/tmp/git/outside/inside$ echo foo >.git ~/tmp/git/outside/inside$ git rev-parse --git-dir fatal: Invalid gitfile format: .git Shouldn't these also be ignored? Best, Gábor ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 0:33 ` SZEDER Gábor @ 2011-10-06 0:44 ` Junio C Hamano 2011-10-06 1:09 ` SZEDER Gábor 0 siblings, 1 reply; 52+ messages in thread From: Junio C Hamano @ 2011-10-06 0:44 UTC (permalink / raw) To: SZEDER Gábor; +Cc: Fredrik Gustafsson, Federico Lucifredi, git SZEDER Gábor <szeder@ira.uka.de> writes: > And what about unreadable .git files? Having then inside a working tree is so sick that I do not think it deserves consideration. Please don't troll immediately after a big release. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 0:44 ` Junio C Hamano @ 2011-10-06 1:09 ` SZEDER Gábor [not found] ` <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com> 2011-10-06 16:48 ` Phil Hord 0 siblings, 2 replies; 52+ messages in thread From: SZEDER Gábor @ 2011-10-06 1:09 UTC (permalink / raw) To: Junio C Hamano; +Cc: Fredrik Gustafsson, Federico Lucifredi, git On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote: > SZEDER Gábor <szeder@ira.uka.de> writes: > > > And what about unreadable .git files? > > Having then inside a working tree is so sick that I do not think it > deserves consideration. I'm not sure why is this any different than having a .git directory that is not a repository inside a working tree. > Please don't troll immediately after a big release. I didn't mean to troll; it just happened that I came across this issue this weekend while trying to optimize the bash completion code... ^ permalink raw reply [flat|nested] 52+ messages in thread
[parent not found: <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com>]
* Re: Git Bug report [not found] ` <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com> @ 2011-10-06 16:22 ` Junio C Hamano 2011-10-06 16:26 ` Matthieu Moy ` (2 more replies) 0 siblings, 3 replies; 52+ messages in thread From: Junio C Hamano @ 2011-10-06 16:22 UTC (permalink / raw) To: Phil Hord; +Cc: SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi Phil Hord <phil.hord@gmail.com> writes: > On Oct 5, 2011 9:14 PM, "SZEDER Gábor" <szeder@ira.uka.de> wrote: >> >> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote: >> > SZEDER Gábor <szeder@ira.uka.de> writes: >> > >> > > And what about unreadable .git files? >> > >> > Having then inside a working tree is so sick that I do not think it >> > deserves consideration. >> >> I'm not sure why is this any different than having a .git directory >> that is not a repository inside a working tree. > > What should happen here? Ignore it and keep searching? Or fail? > > I just added some common gitfile detection code and I noticed that the > oddball case now is the one that dies on error rather than continuing to > search for alternate explanations. I left the oddball behavior assuming it > is desireable, but now you have me rethinking it. Yeah, after thinking about it a bit more, whenever we see ".git" during the upward discovery process, we should always warn if we know it is _not_ a GIT_DIR before looking for another ".git" at higher levels, as anything in that directory cannot be added. If we cannot tell if it is or is not a GIT_DIR, we should error out---the reason we cannot tell most likely is because we cannot read it, and such a file, if it is not a GIT_DIR, cannot be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR, we cannot use it to record updates or inspect existing history. How's that sound as a guideline? ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 16:22 ` Junio C Hamano @ 2011-10-06 16:26 ` Matthieu Moy 2011-10-06 16:54 ` Phil Hord 2011-10-06 22:57 ` Aaron Schrab 2 siblings, 0 replies; 52+ messages in thread From: Matthieu Moy @ 2011-10-06 16:26 UTC (permalink / raw) To: Junio C Hamano Cc: Phil Hord, SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi Junio C Hamano <gitster@pobox.com> writes: > If we cannot tell if it is or is not > a GIT_DIR, we should error out---the reason we cannot tell most likely is > because we cannot read it, and such a file, if it is not a GIT_DIR, cannot > be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR, > we cannot use it to record updates or inspect existing history. Plus, the user may have removed the permission on the .git directory by mistake, and it would be very surprising behavior if git ran without complaining using a higher level GIT_DIR (i.e. a more or less arbitrary repo as far as the user is concerned ...) > How's that sound as a guideline? Sounds reasonable, yes. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 16:22 ` Junio C Hamano 2011-10-06 16:26 ` Matthieu Moy @ 2011-10-06 16:54 ` Phil Hord 2011-10-06 22:57 ` Aaron Schrab 2 siblings, 0 replies; 52+ messages in thread From: Phil Hord @ 2011-10-06 16:54 UTC (permalink / raw) To: Junio C Hamano Cc: SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi On Thu, Oct 6, 2011 at 12:22 PM, Junio C Hamano <gitster@pobox.com> wrote: > Phil Hord <phil.hord@gmail.com> writes: > >> On Oct 5, 2011 9:14 PM, "SZEDER Gábor" <szeder@ira.uka.de> wrote: >>> >>> On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote: >>> > SZEDER Gábor <szeder@ira.uka.de> writes: >>> > >>> > > And what about unreadable .git files? >>> > >>> > Having then inside a working tree is so sick that I do not think it >>> > deserves consideration. >>> >>> I'm not sure why is this any different than having a .git directory >>> that is not a repository inside a working tree. >> >> What should happen here? Ignore it and keep searching? Or fail? >> >> I just added some common gitfile detection code and I noticed that the >> oddball case now is the one that dies on error rather than continuing to >> search for alternate explanations. I left the oddball behavior assuming it >> is desireable, but now you have me rethinking it. > > Yeah, after thinking about it a bit more, whenever we see ".git" during > the upward discovery process, we should always warn if we know it is _not_ > a GIT_DIR before looking for another ".git" at higher levels, as anything > in that directory cannot be added. If we cannot tell if it is or is not > a GIT_DIR, we should error out---the reason we cannot tell most likely is > because we cannot read it, and such a file, if it is not a GIT_DIR, cannot > be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR, > we cannot use it to record updates or inspect existing history. > > How's that sound as a guideline? Ok. Three cases, then: if .git is valid, we use it. If .git is bogus, we warn about it and keep searching. If .git is unverifiable (permissions, IO-fail, etc.), we die. Phil ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 16:22 ` Junio C Hamano 2011-10-06 16:26 ` Matthieu Moy 2011-10-06 16:54 ` Phil Hord @ 2011-10-06 22:57 ` Aaron Schrab 2 siblings, 0 replies; 52+ messages in thread From: Aaron Schrab @ 2011-10-06 22:57 UTC (permalink / raw) To: Junio C Hamano Cc: Phil Hord, SZEDER Gábor, git, Fredrik Gustafsson, Federico Lucifredi At 09:22 -0700 06 Oct 2011, Junio C Hamano <gitster@pobox.com> wrote: >Yeah, after thinking about it a bit more, whenever we see ".git" during >the upward discovery process, we should always warn if we know it is _not_ >a GIT_DIR before looking for another ".git" at higher levels, as anything >in that directory cannot be added. If we cannot tell if it is or is not >a GIT_DIR, we should error out---the reason we cannot tell most likely is >because we cannot read it, and such a file, if it is not a GIT_DIR, cannot >be tracked in the real GIT_DIR at a higher level, and if it is a GIT_DIR, >we cannot use it to record updates or inspect existing history. Yes, I think that sounds like a good idea. That should also solve a related problem that I noticed while checking out the current behaviour. Currently if the .git directory of a submodule is inaccessible running `git status` from anywhere in the parent repository (including within the submodule) will cause git to recursively call itself until enough resources are used to prevent further forking. I then tried this with the patch from earlier in the thread applied, but with the call to error() changed to call die() instead. With that change it quickly failed with a useful error message. ^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: Git Bug report 2011-10-06 1:09 ` SZEDER Gábor [not found] ` <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com> @ 2011-10-06 16:48 ` Phil Hord 1 sibling, 0 replies; 52+ messages in thread From: Phil Hord @ 2011-10-06 16:48 UTC (permalink / raw) To: SZEDER Gábor Cc: Junio C Hamano, Fredrik Gustafsson, Federico Lucifredi, git On 10/5/11, SZEDER Gábor <szeder@ira.uka.de> wrote: > On Wed, Oct 05, 2011 at 05:44:54PM -0700, Junio C Hamano wrote: >> SZEDER Gábor <szeder@ira.uka.de> writes: >> >> > And what about unreadable .git files? >> >> Having then inside a working tree is so sick that I do not think it >> deserves consideration. > > I'm not sure why is this any different than having a .git directory > that is not a repository inside a working tree. What should happen here? Ignore it and keep searching? Or fail? I just added some common gitfile detection code and I noticed that the oddball case now is the one that dies on error rather than continuing to search for alternate explanations. I left the oddball behavior assuming it is desireable, but now you have me rethinking it. Phil ^ permalink raw reply [flat|nested] 52+ messages in thread
end of thread, other threads:[~2023-08-22 1:30 UTC | newest] Thread overview: 52+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-08-04 16:46 git bug report Paul Watson 2023-08-04 17:28 ` rsbecker 2023-08-04 17:48 ` [EXTERNAL] " Paul Watson 2023-08-04 18:12 ` rsbecker 2023-08-07 15:46 ` Paul Watson 2023-08-08 13:07 ` Paul Watson 2023-08-08 13:28 ` rsbecker 2023-08-08 17:07 ` Junio C Hamano 2023-08-16 23:45 ` [PATCH] diff: tighten interaction between -w and --exit-code Junio C Hamano 2023-08-17 5:10 ` Jeff King 2023-08-17 16:12 ` Junio C Hamano 2023-08-17 19:49 ` Jeff King 2023-08-17 19:56 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 1/5] diff: --dirstat leakfix Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 2/5] diff: move the fallback "--exit-code" code down Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 3/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano 2023-08-18 21:15 ` Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 4/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano 2023-08-17 22:29 ` [PATCH v2 5/5] diff: teach "--name-status" and friends to honor "--exit-code -w" Junio C Hamano 2023-08-17 22:37 ` [PATCH v2 0/5] fix interactions with "-w" and "--exit-code" Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 " Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 1/5] diff: move the fallback "--exit-code" code down Junio C Hamano 2023-08-21 20:41 ` Jeff King 2023-08-18 23:59 ` [PATCH v3 2/5] diff: mode-only change should be noticed by "--patch -w --exit-code" Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 3/5] diff: teach "--stat -w --exit-code" to notice differences Junio C Hamano 2023-08-21 20:45 ` Jeff King 2023-08-18 23:59 ` [PATCH v3 4/5] t4040: remove test that succeeded for a wrong reason Junio C Hamano 2023-08-18 23:59 ` [PATCH v3 5/5] diff: the -w option breaks --exit-code for --raw and other output modes Junio C Hamano 2023-08-21 21:00 ` Jeff King 2023-08-21 21:08 ` Jeff King 2023-08-21 22:23 ` Jeff King 2023-08-21 22:16 ` Junio C Hamano 2023-08-21 22:26 ` Junio C Hamano 2023-08-22 1:30 ` Jeff King 2023-08-21 21:02 ` [PATCH v3 0/5] fix interactions with "-w" and "--exit-code" Jeff King -- strict thread matches above, loose matches on Subject: below -- 2022-10-10 23:09 Git bug report Camden Narzt 2022-04-14 7:22 Git Bug Report Randall Alfaro 2011-10-04 21:24 Git Bug report Federico Lucifredi 2011-10-05 6:11 ` Johannes Sixt 2011-10-05 18:32 ` Federico Lucifredi 2011-10-05 7:22 ` Fredrik Gustafsson 2011-10-05 16:49 ` Junio C Hamano 2011-10-05 21:56 ` Nguyen Thai Ngoc Duy 2011-10-06 0:33 ` SZEDER Gábor 2011-10-06 0:44 ` Junio C Hamano 2011-10-06 1:09 ` SZEDER Gábor [not found] ` <CABURp0qCsKG2oOxLw4BfU8UM=9V+pigd69ZK=TZVwetBPqjuiA@mail.gmail.com> 2011-10-06 16:22 ` Junio C Hamano 2011-10-06 16:26 ` Matthieu Moy 2011-10-06 16:54 ` Phil Hord 2011-10-06 22:57 ` Aaron Schrab 2011-10-06 16:48 ` Phil Hord
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).