{"thread":{"id":"26752","subject":"[PATCH] diff-lib.c: Fix diff-files --diff-filter --quiet exit code","startedAt":"2011-03-16T16:56:33Z","lastAt":"2011-03-17T11:15:07Z","messageCount":5,"participants":["Jakob Pfender","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"163494","messageId":"4D80EBC1.7010105@elegosoft.com","threadId":"26752","inReplyTo":null,"subject":"[PATCH] diff-lib.c: Fix diff-files --diff-filter --quiet exit code","fromName":"Jakob Pfender","fromEmail":"jpfender@elegosoft.com","sentAt":"2011-03-16T16:56:33Z","receivedAt":"2011-03-16T16:56:33Z","isPatch":true,"sender":{"key":"jpfender@elegosoft.com","avatar":null},"body":"Given the following status:\n\n\t$ git status -s\n\t M bar\n\tUU foo\n\nThere is an unexpected difference in the return code of git diff-files\nwhen used with --quiet as opposed to --exit-code:\n\n\t$ git diff-files --diff-filter=U --quiet\n\t$ echo $?\n\t0\n\n\t$ git diff-files --diff-filter=U --exit-code\n\tfoo>\n\t$ echo $?\n\t1\n\nNotice the different return codes. Now, according to the documentation,\n--quiet implies --return-code. However, the return code of the former is\nclearly not correct if --return-code was supposed to be on.\n\nThis patch removes a useless bit of code that caused the return codes to\ndiffer.\n\nSigned-off-by: Jakob Pfender \n---\n diff-lib.c | 4 ----\n 1 files changed, 0 insertions(+), 4 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 392ce2b..a7aa42b 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -102,10 +102,6 @@ int run_diff_files(struct rev_info *revs, unsigned \nint option)\n \t\tint changed;\n \t\tunsigned dirty_submodule = 0;\n\n-\t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK) &&\n-\t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n-\t\t\tbreak;\n-\n \t\tif (!ce_path_match(ce, revs->prune_data))\n \t\t\tcontinue;\n\n-- \n1.7.0.4\n"},{"id":"163515","messageId":"7v62ri7oqm.fsf@alter.siamese.dyndns.org","threadId":"26752","inReplyTo":"4D80EBC1.7010105@elegosoft.com","subject":"Re: [PATCH] diff-lib.c: Fix diff-files --diff-filter --quiet exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-16T21:13:37Z","receivedAt":"2011-03-16T21:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakob Pfender writes:\n\n> Given the following status:\n>\n> \t$ git status -s\n> \t M bar\n> \tUU foo\n>\n> There is an unexpected difference in the return code of git diff-files\n> when used with --quiet as opposed to --exit-code:\n>\n> \t$ git diff-files --diff-filter=U --quiet\n> \t$ echo $?\n> \t0\n>\n> \t$ git diff-files --diff-filter=U --exit-code\n> \tfoo>\n> \t$ echo $?\n> \t1\n>\n> Notice the different return codes. Now, according to the documentation,\n> --quiet implies --return-code. However, the return code of the former is\n> clearly not correct if --return-code was supposed to be on.\n>\n> This patch removes a useless bit of code that caused the return codes to\n> differ.\n\nHow did you determine it is \"useless bit\"?\n\nThe code notices that the caller, by specifying --quiet, does not want any\ndetails of the changes and instead wants to know if there is a change or\nnot. And it breaks out of the loop because it already found what it\nwanted to know, namely, there is a change.\n\nWhen you have a post-process filter (like -w or -S), the path we found to\nbe different here may be uninteresting and there may be no output (hence\nwe should exit with status 0). So it is true that the optimization you\nare removing needs to be disabled in _some_ situations, and the current\ncode doesn't, and it needs fixing.\n\nBut is it a justifiable fix to disable the optimization for even the\nnormal cases?\n\n> Signed-off-by: Jakob Pfender \n> ---\n> diff-lib.c | 4 ----\n> 1 files changed, 0 insertions(+), 4 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 392ce2b..a7aa42b 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -102,10 +102,6 @@ int run_diff_files(struct rev_info *revs,\n> unsigned int option)\n> \t\tint changed;\n> \t\tunsigned dirty_submodule = 0;\n>\n> -\t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK) &&\n> -\t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n> -\t\t\tbreak;\n> -\n> \t\tif (!ce_path_match(ce, revs->prune_data))\n> \t\t\tcontinue;\n"},{"id":"163519","messageId":"7vwrjy670r.fsf_-_@alter.siamese.dyndns.org","threadId":"26752","inReplyTo":"7v62ri7oqm.fsf@alter.siamese.dyndns.org","subject":"Re* [PATCH] diff-lib.c: Fix diff-files --diff-filter --quiet exit code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-16T22:21:40Z","receivedAt":"2011-03-16T22:21:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The code notices that the caller, by specifying --quiet, does not want any\n> details of the changes and instead wants to know if there is a change or\n> not. And it breaks out of the loop because it already found what it\n> wanted to know, namely, there is a change.\n>\n> When you have a post-process filter (like -w or -S), the path we found to\n> be different here may be uninteresting and there may be no output (hence\n> we should exit with status 0). So it is true that the optimization you\n> are removing needs to be disabled in _some_ situations, and the current\n> code doesn't, and it needs fixing.\n\nWhat I was hinting at was perhaps to do it something like the attached; it\nis totally untested, and merely for illustration purposes.\n\nI didn't even try to cover -w/-S which may or may not involve inspecting\nDIFF_FROM_CONTENTS bit in the options.\n\n diff-lib.c | 3 ++-\n 1 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 1e22992..2870de4 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -103,7 +103,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tunsigned dirty_submodule = 0;\n \n \t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK) &&\n-\t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n+\t\t !revs->diffopt.filter &&\n+\t\t DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n \t\t\tbreak;\n \n \t\tif (!ce_path_match(ce, &revs->prune_data))\n"},{"id":"163525","messageId":"7vmxku64sh.fsf_-_@alter.siamese.dyndns.org","threadId":"26752","inReplyTo":"7vwrjy670r.fsf_-_@alter.siamese.dyndns.org","subject":"[PATCH] diff --quiet: disable optimization when --diff-filter=X is used","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-03-16T23:09:50Z","receivedAt":"2011-03-16T23:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The code notices that the caller does not want any detail of the changes\nand only wants to know if there is a change or not by specifying --quiet.\nAnd it breaks out of the loop when it knows it already found any change.\n\nWhen you have a post-process filter (e.g. --diff-filter), however, the\npath we found to be different in the previous round and set HAS_CHANGES\nbit may end up being uninteresting, and there may be no output at the end.\nThe optimization needs to be disabled for such case.\n\nNote that the f245194 (diff: change semantics of \"ignore whitespace\"\noptions, 2009-05-22) already disables this optimization by refraining\nfrom setting HAS_CHANGES when post-process filters that need to inspect\nthe contents of the files (e.g. -S, -w) in diff_change() function.\n\nSigned-off-by: Junio C Hamano \n---\n\n * This time with tests, on top of 90b1994 (diff: Rename QUIET internal\n option to QUICK, 2009-05-23), which was the last commit in the series\n that introduced the incorrect optimization in the first place.\n\n Note that the test script was renamed to t/t4040 in today's codebase,\n but it merges cleanly.\n\n diff-lib.c | 3 ++-\n t/t4037-whitespace-status.sh | 7 +++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex b7813af..bfa6503 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -74,7 +74,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\tint changed;\n \n \t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK) &&\n-\t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n+\t\t !revs->diffopt.filter &&\n+\t\t DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n \t\t\tbreak;\n \n \t\tif (!ce_path_match(ce, revs->prune_data))\ndiff --git a/t/t4037-whitespace-status.sh b/t/t4037-whitespace-status.sh\nindex a30b03b..abc4934 100755\n--- a/t/t4037-whitespace-status.sh\n+++ b/t/t4037-whitespace-status.sh\n@@ -60,4 +60,11 @@ test_expect_success 'diff-files -b -p --exit-code' '\n \tgit diff-files -b -p --exit-code\n '\n \n+test_expect_success 'diff-files --diff-filter --quiet' '\n+\tgit reset --hard &&\n+\trm a/d &&\n+\techo x >>b/e &&\n+\ttest_must_fail git diff-files --diff-filter=M --quiet\n+'\n+\n test_done\n-- \n1.7.4.1.430.g5aa4d\n"},{"id":"163552","messageId":"4D81ED3B.3000500@elegosoft.com","threadId":"26752","inReplyTo":"7vmxku64sh.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] diff --quiet: disable optimization when --diff-filter=X is used","fromName":"Jakob Pfender","fromEmail":"jpfender@elegosoft.com","sentAt":"2011-03-17T11:15:07Z","receivedAt":"2011-03-17T11:15:07Z","isPatch":true,"sender":{"key":"jpfender@elegosoft.com","avatar":null},"body":"\n\nOn 03/17/2011 12:09 AM, Junio C Hamano wrote:\n> The code notices that the caller does not want any detail of the changes\n> and only wants to know if there is a change or not by specifying --quiet.\n> And it breaks out of the loop when it knows it already found any change.\n>\n> When you have a post-process filter (e.g. --diff-filter), however, the\n> path we found to be different in the previous round and set HAS_CHANGES\n> bit may end up being uninteresting, and there may be no output at the end.\n> The optimization needs to be disabled for such case.\n>\n> Note that the f245194 (diff: change semantics of \"ignore whitespace\"\n> options, 2009-05-22) already disables this optimization by refraining\n> from setting HAS_CHANGES when post-process filters that need to inspect\n> the contents of the files (e.g. -S, -w) in diff_change() function.\n\nThat fixes it nicely, thank you.\n\n>\n> Signed-off-by: Junio C Hamano\n> ---\n>\n> * This time with tests, on top of 90b1994 (diff: Rename QUIET internal\n> option to QUICK, 2009-05-23), which was the last commit in the series\n> that introduced the incorrect optimization in the first place.\n>\n> Note that the test script was renamed to t/t4040 in today's codebase,\n> but it merges cleanly.\n>\n> diff-lib.c | 3 ++-\n> t/t4037-whitespace-status.sh | 7 +++++++\n> 2 files changed, 9 insertions(+), 1 deletions(-)\n>\n> diff --git a/diff-lib.c b/diff-lib.c\n> index b7813af..bfa6503 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -74,7 +74,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n> \t\tint changed;\n>\n> \t\tif (DIFF_OPT_TST(&revs->diffopt, QUICK)&&\n> -\t\t\tDIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n> +\t\t !revs->diffopt.filter&&\n> +\t\t DIFF_OPT_TST(&revs->diffopt, HAS_CHANGES))\n> \t\t\tbreak;\n>\n> \t\tif (!ce_path_match(ce, revs->prune_data))\n> diff --git a/t/t4037-whitespace-status.sh b/t/t4037-whitespace-status.sh\n> index a30b03b..abc4934 100755\n> --- a/t/t4037-whitespace-status.sh\n> +++ b/t/t4037-whitespace-status.sh\n> @@ -60,4 +60,11 @@ test_expect_success 'diff-files -b -p --exit-code' '\n> \tgit diff-files -b -p --exit-code\n> '\n>\n> +test_expect_success 'diff-files --diff-filter --quiet' '\n> +\tgit reset --hard&&\n> +\trm a/d&&\n> +\techo x>>b/e&&\n> +\ttest_must_fail git diff-files --diff-filter=M --quiet\n> +'\n> +\n> test_done\n"}]}