grep: fix grepping for "intent to add" files
PATCH 1/1 grep: fix grepping for "intent to add" files
From: Charles Bailey <[email protected]>
This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e.
This commit caused 'git grep' to no longer find matches in new files in the working tree where the corresponding index entry had the "intent to add" bit set.
Add tests to cover this case and a few related cases which previously lacked coverage.
---
Originally discussed:
http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358
http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002
Unless I've misunderstood the conversation and commit message, the referenced commit was supposed to be a "code as a comment" commit with no change in observable behavior however a user was surprised that 'git grep' couldn't find something that regular grep could, despite the file being tracked - albeit new and "intended to add".
builtin/grep.c | 2 +- t/t7810-grep.sh | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-)
builtin/grep.c +1 -1
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, intfor (nr =0 ; nr < active_nr; nr++) {const struct cache_entry *ce = active_cache[nr]; -if (!S_ISREG (ce->ce_mode) ||ce_intent_to_add (ce)) +if (!S_ISREG (ce->ce_mode))continue ;if (!ce_path_match (ce, pathspec,NULL ))continue ;
t/t7810-grep.sh +29 -0
@@ -1364,4 +1364,33 @@ test_expect_success 'grep --color -e A --and -e B -p with context' ' test_cmp expected actual ' +test_expect_success 'grep can find things only in the work tree' ' + touch work-tree-only && + git add work-tree-only && + echo "find in work tree" >work-tree-only && + git grep --quiet "find in work tree" && + test_must_fail git grep --quiet --cached "find in work tree" && + test_must_fail git grep --quiet "find in work tree" HEAD && + git rm -f work-tree-only +' + +test_expect_success 'grep can find things only in the work tree (i-t-a)' ' + echo "intend to add this" >intend-to-add && + git add -N intend-to-add && + git grep --quiet "intend to add this" && + test_must_fail git grep --quiet --cached "intend to add this" && + test_must_fail git grep --quiet "intend to add this" HEAD && + git rm -f intend-to-add +' + +test_expect_success 'grep can find things only in the index' ' + echo "only in the index" >cache-this && + git add cache-this && + rm cache-this && + test_must_fail git grep --quiet "only in the index" && + git grep --quiet --cached "only in the index" && + test_must_fail git grep --quiet "only in the index" HEAD && + git rm --cached cache-this +' + test_done
Signature · 1 line
2.8.2.311.gee88674
Quoted 7 lines · Charles Bailey
From: Charles Bailey <[email protected]>
This reverts commit
4d552005323034c1d6311796ac1074e9a4b4b57e.This commit caused 'git grep' to no longer find matches in new files in the working tree where the corresponding index entry had the "intent to add" bit set.
I don't think revert is right. It rather needs a re-fix like below. Basically we want grep_file() to run as normal, but grep_sha1() (i.e. git grep --cached) should ignore i-t-a entries, because empty SHA-1 is not the right content to grep. It does not matter in positive matching, sure, but it may in -v cache.
-- 8< --
builtin/grep.c +2 -2
@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, intfor (nr =0 ; nr < active_nr; nr++) {const struct cache_entry *ce = active_cache[nr]; -if (!S_ISREG (ce->ce_mode) ||ce_intent_to_add (ce)) +if (!S_ISREG (ce->ce_mode))continue ;if (!ce_path_match (ce, pathspec,NULL ))continue ;
@@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int* cache version instead */ if (cached || (ce->ce_flags &CE_VALID ) ||ce_skip_worktree (ce)) { -if (ce_stage (ce)) +if (ce_stage (ce) ||ce_intent_to_add (ce))continue ; hit |=grep_sha1 (opt, ce->sha1, ce->name,0 , ce->name); }
-- 8< --
--
Duy
Quoted 5 lines · Duy Nguyen
I don't think revert is right. It rather needs a re-fix like below. Basically we want
grep_file()to run as normal, butgrep_sha1()(i.e. git grep --cached) should ignore i-t-a entries, because empty SHA-1 is not the right content to grep. It does not matter in positive matching, sure, but it may in -v cache.
You don't think the revert is correct or you don't think the revert is sufficient? (I wasn't able to find a test case which proved that the change to line 399 was necessary, so perhaps I don't understand.)
I would have thought that grepping the empty SHA-1 would be correct for with or without -v. An "intent to add" file has no content in the index so I would expect it to have zero matching and zero non-matching lines for any grep --cached query?
Or is this an efficiency and not a correctness concern?
Charles.
Quoted 10 lines · Charles Bailey
Quoted 5 lines · Duy Nguyen
I don't think revert is right. It rather needs a re-fix like below. Basically we want
grep_file()to run as normal, butgrep_sha1()(i.e. git grep --cached) should ignore i-t-a entries, because empty SHA-1 is not the right content to grep. It does not matter in positive matching, sure, but it may in -v cache.You don't think the revert is correct or you don't think the revert is sufficient? (I wasn't able to find a test case which proved that the change to line 399 was necessary, so perhaps I don't understand.)
OK insufficient.
Quoted 6 lines
I would have thought that grepping the empty SHA-1 would be correct for with or without -v. An "intent to add" file has no content in the index so I would expect it to have zero matching and zero non-matching lines for any grep --cached query?
Or is this an efficiency and not a correctness concern?
"git grep --cached" searches file content that will be committed by "git commit" (no -a). An i-t-a entry will not be committed (you would need "git add" first, or do "git commit -a"). So if I say "search among the to-be-committed file content, list files that do not match abc" (git grep -l -v --cached abc), the i-t-a entry will show up because its fake content is empty (i.e. not contain "abc"), even though it's not in the "to-be-committed" list. So yeah, correctness issue.
Signature · 1 line
Duy
Quoted 9 lines · Duy Nguyen
"git grep --cached" searches file content that will be committed by "git commit" (no -a). An i-t-a entry will not be committed (you would need "git add" first, or do "git commit -a"). So if I say "search among the to-be-committed file content, list files that do not match abc" (git grep -l -v --cached abc), the i-t-a entry will show up because its fake content is empty (i.e. not contain "abc"), even though it's not in the "to-be-committed" list. So yeah, correctness issue.
OK, I think there is an issue there but it's not with "-l -v --cached" but rather with "-L --cached". If my understanding is correct, "-l -v" means "has a line that doesn't match" whereas "-L" means "has no line that matches".
Does this sound correct? I'll try adding a new test.
Quoted 11 lines · Charles Bailey
Quoted 9 lines · Duy Nguyen
"git grep --cached" searches file content that will be committed by "git commit" (no -a). An i-t-a entry will not be committed (you would need "git add" first, or do "git commit -a"). So if I say "search among the to-be-committed file content, list files that do not match abc" (git grep -l -v --cached abc), the i-t-a entry will show up because its fake content is empty (i.e. not contain "abc"), even though it's not in the "to-be-committed" list. So yeah, correctness issue.
Quoted 6 lines
OK, I think there is an issue there but it's not with "-l -v --cached" but rather with "-L --cached". If my understanding is correct, "-l -v" means "has a line that doesn't match" whereas "-L" means "has no line that matches".
Does this sound correct? I'll try adding a new test.
Yeah "-L --cached" should work the same, I think.
Signature · 1 line
Duy
Quoted 13 lines · Duy Nguyen
Quoted 3 lines
You don't think the revert is correct or you don't think the revert is sufficient? (I wasn't able to find a test case which proved that the change to line 399 was necessary, so perhaps I don't understand.)
OK insufficient.
Quoted 6 lines
I would have thought that grepping the empty SHA-1 would be correct for with or without -v. An "intent to add" file has no content in the index so I would expect it to have zero matching and zero non-matching lines for any grep --cached query?
Or is this an efficiency and not a correctness concern?
Quoted 8 lines
"git grep --cached" searches file content that will be committed by "git commit" (no -a). An i-t-a entry will not be committed (you would need "git add" first, or do "git commit -a"). So if I say "search among the to-be-committed file content, list files that do not match abc" (git grep -l -v --cached abc), the i-t-a entry will show up because its fake content is empty (i.e. not contain "abc"), even though it's not in the "to-be-committed" list. So yeah, correctness issue.
OK, sounds like a good start for a proper log message for the fix. Thanks for hashing it all out before I got to the end of the thread ;-)
Quoted 7 lines · Charles Bailey
http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358
http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002
Unless I've misunderstood the conversation and commit message, the referenced commit was supposed to be a "code as a comment" commit with no change in observable behavior
Thanks for a pointer; 276358 claims that ce->ce_mode would be zero for path added with "git add -N path", but I do not think it is correct.
The updated behaviour is more understandable. With "add -N", the user said "Just keep an eye on this path, I cannot decide what the contents for this path in the index should be at this moment". grep_cache() that checks the contents in the index cannot say what is in the index, because the contents is not yet there.