diff-lib.c: Fix diff-files --diff-filter --quiet exit code
PATCH 1/1 diff-lib.c: Fix diff-files --diff-filter --quiet exit code
Given the following status:
$ git status -s M bar UU foo
There is an unexpected difference in the return code of git diff-files when used with --quiet as opposed to --exit-code:
$ git diff-files --diff-filter=U --quiet $ echo $? 0
$ git diff-files --diff-filter=U --exit-code <usual output, lots of 0's and U<TAB>foo> $ echo $? 1
Notice the different return codes. Now, according to the documentation, --quiet implies --return-code. However, the return code of the former is clearly not correct if --return-code was supposed to be on.
This patch removes a useless bit of code that caused the return codes to differ.
--- diff-lib.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)
diff-lib.c +0 -4
@@ -102,10 +102,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)int changed;unsigned dirty_submodule =0 ; -if (DIFF_OPT_TST (&revs->diffopt,QUICK ) && -DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES )) -break ; -if (!ce_path_match (ce, revs->prune_data))continue ;
Signature · 1 line
1.7.0.4
Quoted 18 lines · Jakob Pfender
Given the following status:
$ git status -s M bar UU fooThere is an unexpected difference in the return code of git diff-files when used with --quiet as opposed to --exit-code:
$ git diff-files --diff-filter=U --quiet $ echo $? 0$ git diff-files --diff-filter=U --exit-code <usual output, lots of 0's and U<TAB>foo> $ echo $? 1
Quoted 6 lines
Notice the different return codes. Now, according to the documentation, --quiet implies --return-code. However, the return code of the former is clearly not correct if --return-code was supposed to be on.
This patch removes a useless bit of code that caused the return codes to differ.
How did you determine it is "useless bit"?
The code notices that the caller, by specifying --quiet, does not want any details of the changes and instead wants to know if there is a change or not. And it breaks out of the loop because it already found what it wanted to know, namely, there is a change.
When you have a post-process filter (like -w or -S), the path we found to be different here may be uninteresting and there may be no output (hence we should exit with status 0). So it is true that the optimization you are removing needs to be disabled in _some_ situations, and the current code doesn't, and it needs fixing.
But is it a justifiable fix to disable the optimization for even the normal cases?
Quoted 20 lines
Signed-off-by Jakob Pfender <[email protected]>--- diff-lib.c | 4 ---- 1 files changed, 0 insertions(+), 4 deletions(-)diff-lib.c +0
-4@@ -102,10 +102,6 @@ int run_diff_files(struct rev_info *revs, unsigned int option)int changed;unsigned dirty_submodule =0 ; -if (DIFF_OPT_TST (&revs->diffopt,QUICK ) && -DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES )) -break ; -if (!ce_path_match (ce, revs->prune_data))continue ;
Re* [PATCH] diff-lib.c: Fix diff-files --diff-filter --quiet exit code
Quoted 10 lines · Junio C Hamano
The code notices that the caller, by specifying --quiet, does not want any details of the changes and instead wants to know if there is a change or not. And it breaks out of the loop because it already found what it wanted to know, namely, there is a change.
When you have a post-process filter (like -w or -S), the path we found to be different here may be uninteresting and there may be no output (hence we should exit with status 0). So it is true that the optimization you are removing needs to be disabled in _some_ situations, and the current code doesn't, and it needs fixing.
What I was hinting at was perhaps to do it something like the attached; it is totally untested, and merely for illustration purposes.
I didn't even try to cover -w/-S which may or may not involve inspecting DIFF_FROM_CONTENTS bit in the options.
diff-lib.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff-lib.c +2 -1
@@ -103,7 +103,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)unsigned dirty_submodule =0 ;if (DIFF_OPT_TST (&revs->diffopt,QUICK ) && -DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES )) + !revs->diffopt.filter && +DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES ))break ;if (!ce_path_match (ce, &revs->prune_data))
PATCH 1/1 diff --quiet: disable optimization when --diff-filter=X is used
The code notices that the caller does not want any detail of the changes and only wants to know if there is a change or not by specifying --quiet. And it breaks out of the loop when it knows it already found any change.
When you have a post-process filter (e.g. --diff-filter), however, the path we found to be different in the previous round and set HAS_CHANGES bit may end up being uninteresting, and there may be no output at the end. The optimization needs to be disabled for such case.
Note that the f245194 (diff: change semantics of "ignore whitespace" options, 2009-05-22) already disables this optimization by refraining from setting HAS_CHANGES when post-process filters that need to inspect the contents of the files (e.g. -S, -w) in diff_change() function.
---
* This time with tests, on top of 90b1994 (diff: Rename QUIET internal option to QUICK, 2009-05-23), which was the last commit in the series that introduced the incorrect optimization in the first place.
Note that the test script was renamed to t/t4040 in today's codebase, but it merges cleanly.
diff-lib.c | 3 ++- t/t4037-whitespace-status.sh | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletions(-)
diff-lib.c +2 -1
@@ -74,7 +74,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)int changed;if (DIFF_OPT_TST (&revs->diffopt,QUICK ) && -DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES )) + !revs->diffopt.filter && +DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES ))break ;if (!ce_path_match (ce, revs->prune_data))
t/t4037-whitespace-status.sh +7 -0
@@ -60,4 +60,11 @@ test_expect_success 'diff-files -b -p --exit-code' ' git diff-files -b -p --exit-code ' +test_expect_success 'diff-files --diff-filter --quiet' ' + git reset --hard && + rm a/d && + echo x >>b/e && + test_must_fail git diff-files --diff-filter=M --quiet +' + test_done
Signature · 1 line
1.7.4.1.430.g5aa4d
Re: [PATCH] diff --quiet: disable optimization when --diff-filter=X is used
Quoted 9 lines · Junio C Hamano
The code notices that the caller does not want any detail of the changes and only wants to know if there is a change or not by specifying --quiet. And it breaks out of the loop when it knows it already found any change.
When you have a post-process filter (e.g. --diff-filter), however, the path we found to be different in the previous round and set
HAS_CHANGESbit may end up being uninteresting, and there may be no output at the end. The optimization needs to be disabled for such case.
Quoted 4 lines
Note that the f245194 (diff: change semantics of "ignore whitespace" options, 2009-05-22) already disables this optimization by refraining from setting
HAS_CHANGESwhen post-process filters that need to inspect the contents of the files (e.g. -S, -w) indiff_change()function.
That fixes it nicely, thank you.
Quoted 45 lines
Signed-off-by Junio C Hamano<[email protected]>---
* This time with tests, on top of 90b1994 (diff: Rename QUIET internal option to QUICK, 2009-05-23), which was the last commit in the series that introduced the incorrect optimization in the first place.
Note that the test script was renamed to t/t4040 in today's codebase, but it merges cleanly.
diff-lib.c | 3 ++- t/t4037-whitespace-status.sh | 7 +++++++ 2 files changed, 9 insertions(+), 1 deletions(-)diff-lib.c +2
-1@@ -74,7 +74,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)int changed;if (DIFF_OPT_TST (&revs->diffopt,QUICK )&& -DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES )) + !revs->diffopt.filter&& +DIFF_OPT_TST (&revs->diffopt,HAS_CHANGES ))break ;if (!ce_path_match (ce, revs->prune_data))t/t4037-whitespace-status.sh +7
-0@@ -60,4 +60,11 @@ test_expect_success 'diff-files -b -p --exit-code' ' git diff-files -b -p --exit-code ' +test_expect_success 'diff-files --diff-filter --quiet' ' + git reset --hard&& + rm a/d&& + echo x>>b/e&& + test_must_fail git diff-files --diff-filter=M --quiet +' + test_done