{"thread":{"id":"42628","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","startedAt":"2016-06-16T09:49:06Z","lastAt":"2016-06-16T19:58:14Z","messageCount":8,"participants":["Charles Bailey","Duy Nguyen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"289348","messageId":"20160616094749.GA20681@hashpling.org","threadId":"42628","inReplyTo":"20160616074709.GA24412@duynguyen-vnpc.vn.dektech.internal","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2016-06-16T09:47:49Z","receivedAt":"2016-06-16T09:49:06Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, Jun 16, 2016 at 02:47:09PM +0700, Duy Nguyen wrote:\n> I don't think revert is right. It rather needs a re-fix like below.\n> Basically we want grep_file() to run as normal, but grep_sha1()\n> (i.e. git grep --cached) should ignore i-t-a entries, because empty\n> SHA-1 is not the right content to grep. It does not matter in positive\n> matching, sure, but it may in -v cache.\n\nYou don't think the revert is correct or you don't think the revert is\nsufficient? (I wasn't able to find a test case which proved that the\nchange to line 399 was necessary, so perhaps I don't understand.)\n\nI would have thought that grepping the empty SHA-1 would be correct for\nwith or without -v. An \"intent to add\" file has no content in the index\nso I would expect it to have zero matching and zero non-matching lines\nfor any grep --cached query?\n\nOr is this an efficiency and not a correctness concern?\n\nCharles.\n"},{"id":"289353","messageId":"CACsJy8Bp6Mv2D1QCsR6MWhW2XMedo2svQKHBrx8AgA1Le56Grw@mail.gmail.com","threadId":"42628","inReplyTo":"20160616094749.GA20681@hashpling.org","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-16T10:57:18Z","receivedAt":"2016-06-16T10:57:52Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 16, 2016 at 4:47 PM, Charles Bailey wrote:\n> On Thu, Jun 16, 2016 at 02:47:09PM +0700, Duy Nguyen wrote:\n>> I don't think revert is right. It rather needs a re-fix like below.\n>> Basically we want grep_file() to run as normal, but grep_sha1()\n>> (i.e. git grep --cached) should ignore i-t-a entries, because empty\n>> SHA-1 is not the right content to grep. It does not matter in positive\n>> matching, sure, but it may in -v cache.\n>\n> You don't think the revert is correct or you don't think the revert is\n> sufficient? (I wasn't able to find a test case which proved that the\n> change to line 399 was necessary, so perhaps I don't understand.)\n\nOK insufficient.\n\n> I would have thought that grepping the empty SHA-1 would be correct for\n> with or without -v. An \"intent to add\" file has no content in the index\n> so I would expect it to have zero matching and zero non-matching lines\n> for any grep --cached query?\n>\n> Or is this an efficiency and not a correctness concern?\n\n\"git grep --cached\" searches file content that will be committed by\n\"git commit\" (no -a). An i-t-a entry will not be committed (you would\nneed \"git add\" first, or do \"git commit -a\"). So if I say \"search\namong the to-be-committed file content, list files that do not match\nabc\" (git grep -l -v --cached abc), the i-t-a entry will show up\nbecause its fake content is empty (i.e. not contain \"abc\"), even\nthough it's not in the \"to-be-committed\" list. So yeah, correctness\nissue.\n-- \nDuy\n"},{"id":"289355","messageId":"20160616114452.GA21930@hashpling.org","threadId":"42628","inReplyTo":"CACsJy8Bp6Mv2D1QCsR6MWhW2XMedo2svQKHBrx8AgA1Le56Grw@mail.gmail.com","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2016-06-16T11:44:52Z","receivedAt":"2016-06-16T11:45:27Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Thu, Jun 16, 2016 at 05:57:18PM +0700, Duy Nguyen wrote:\n> \n> \"git grep --cached\" searches file content that will be committed by\n> \"git commit\" (no -a). An i-t-a entry will not be committed (you would\n> need \"git add\" first, or do \"git commit -a\"). So if I say \"search\n> among the to-be-committed file content, list files that do not match\n> abc\" (git grep -l -v --cached abc), the i-t-a entry will show up\n> because its fake content is empty (i.e. not contain \"abc\"), even\n> though it's not in the \"to-be-committed\" list. So yeah, correctness\n> issue.\n\nOK, I think there is an issue there but it's not with \"-l -v --cached\"\nbut rather with \"-L --cached\". If my understanding is correct, \"-l -v\"\nmeans \"has a line that doesn't match\" whereas \"-L\" means \"has no line\nthat matches\".\n\nDoes this sound correct? I'll try adding a new test.\n"},{"id":"289356","messageId":"CACsJy8AdHMdj9y6WnqqNyEaaW-OHhqR3KTN0Gvd8sPqnbnaY+g@mail.gmail.com","threadId":"42628","inReplyTo":"20160616114452.GA21930@hashpling.org","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-16T12:11:29Z","receivedAt":"2016-06-16T12:12:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 16, 2016 at 6:44 PM, Charles Bailey wrote:\n> On Thu, Jun 16, 2016 at 05:57:18PM +0700, Duy Nguyen wrote:\n>>\n>> \"git grep --cached\" searches file content that will be committed by\n>> \"git commit\" (no -a). An i-t-a entry will not be committed (you would\n>> need \"git add\" first, or do \"git commit -a\"). So if I say \"search\n>> among the to-be-committed file content, list files that do not match\n>> abc\" (git grep -l -v --cached abc), the i-t-a entry will show up\n>> because its fake content is empty (i.e. not contain \"abc\"), even\n>> though it's not in the \"to-be-committed\" list. So yeah, correctness\n>> issue.\n>\n> OK, I think there is an issue there but it's not with \"-l -v --cached\"\n> but rather with \"-L --cached\". If my understanding is correct, \"-l -v\"\n> means \"has a line that doesn't match\" whereas \"-L\" means \"has no line\n> that matches\".\n>\n> Does this sound correct? I'll try adding a new test.\n\nYeah \"-L --cached\" should work the same, I think.\n-- \nDuy\n"},{"id":"289377","messageId":"xmqq7fdp57gg.fsf@gitster.mtv.corp.google.com","threadId":"42628","inReplyTo":"20160616065324.GA14967@hashpling.org","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-16T18:12:15Z","receivedAt":"2016-06-16T18:12:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey writes:\n\n> http://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358\n>\n> http://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002\n>\n> Unless I've misunderstood the conversation and commit message, the\n> referenced commit was supposed to be a \"code as a comment\" commit with\n> no change in observable behavior\n\nThanks for a pointer; 276358 claims that ce->ce_mode would be zero\nfor path added with \"git add -N path\", but I do not think it is\ncorrect.\n\nThe updated behaviour is more understandable. With \"add -N\", the\nuser said \"Just keep an eye on this path, I cannot decide what the\ncontents for this path in the index should be at this moment\".\ngrep_cache() that checks the contents in the index cannot say what\nis in the index, because the contents is not yet there.\n"},{"id":"289378","messageId":"xmqq37od576e.fsf@gitster.mtv.corp.google.com","threadId":"42628","inReplyTo":"CACsJy8Bp6Mv2D1QCsR6MWhW2XMedo2svQKHBrx8AgA1Le56Grw@mail.gmail.com","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-16T18:18:17Z","receivedAt":"2016-06-16T18:18:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> You don't think the revert is correct or you don't think the revert is\n>> sufficient? (I wasn't able to find a test case which proved that the\n>> change to line 399 was necessary, so perhaps I don't understand.)\n>\n> OK insufficient.\n>\n>> I would have thought that grepping the empty SHA-1 would be correct for\n>> with or without -v. An \"intent to add\" file has no content in the index\n>> so I would expect it to have zero matching and zero non-matching lines\n>> for any grep --cached query?\n>>\n>> Or is this an efficiency and not a correctness concern?\n>\n> \"git grep --cached\" searches file content that will be committed by\n> \"git commit\" (no -a). An i-t-a entry will not be committed (you would\n> need \"git add\" first, or do \"git commit -a\"). So if I say \"search\n> among the to-be-committed file content, list files that do not match\n> abc\" (git grep -l -v --cached abc), the i-t-a entry will show up\n> because its fake content is empty (i.e. not contain \"abc\"), even\n> though it's not in the \"to-be-committed\" list. So yeah, correctness\n> issue.\n\nOK, sounds like a good start for a proper log message for the fix.\nThanks for hashing it all out before I got to the end of the thread\n;-)\n"},{"id":"289391","messageId":"20160616074709.GA24412@duynguyen-vnpc.vn.dektech.internal","threadId":"42628","inReplyTo":"20160616065324.GA14967@hashpling.org","subject":"Re: [PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-16T07:47:09Z","receivedAt":"2016-06-16T19:57:42Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jun 16, 2016 at 07:53:24AM +0100, Charles Bailey wrote:\n> From: Charles Bailey \n> \n> This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e.\n> \n> This commit caused 'git grep' to no longer find matches in new files in\n> the working tree where the corresponding index entry had the \"intent to\n> add\" bit set.\n\nI don't think revert is right. It rather needs a re-fix like below.\nBasically we want grep_file() to run as normal, but grep_sha1()\n(i.e. git grep --cached) should ignore i-t-a entries, because empty\nSHA-1 is not the right content to grep. It does not matter in positive\nmatching, sure, but it may in -v cache.\n\n-- 8< --\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 462e607..ae73831 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tconst struct cache_entry *ce = active_cache[nr];\n-\t\tif (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))\n+\t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, pathspec, NULL))\n \t\t\tcontinue;\n@@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \t\t * cache version instead\n \t\t */\n \t\tif (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {\n-\t\t\tif (ce_stage(ce))\n+\t\t\tif (ce_stage(ce) || ce_intent_to_add(ce))\n \t\t\t\tcontinue;\n \t\t\thit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);\n \t\t}\n-- 8< --\n--\nDuy\n"},{"id":"289396","messageId":"20160616065324.GA14967@hashpling.org","threadId":"42628","inReplyTo":null,"subject":"[PATCH] grep: fix grepping for \"intent to add\" files","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2016-06-16T06:53:24Z","receivedAt":"2016-06-16T19:58:14Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey \n\nThis reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e.\n\nThis commit caused 'git grep' to no longer find matches in new files in\nthe working tree where the corresponding index entry had the \"intent to\nadd\" bit set.\n\nAdd tests to cover this case and a few related cases which previously\nlacked coverage.\n\nSigned-off-by: Charles Bailey \n---\n\nOriginally discussed:\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/272363/focus=276358\n\nhttp://thread.gmane.org/gmane.comp.version-control.git/283001/focus=283002\n\nUnless I've misunderstood the conversation and commit message, the\nreferenced commit was supposed to be a \"code as a comment\" commit with\nno change in observable behavior however a user was surprised that 'git\ngrep' couldn't find something that regular grep could, despite the file\nbeing tracked - albeit new and \"intended to add\".\n\n builtin/grep.c | 2 +-\n t/t7810-grep.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 462e607..d5aacba 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -386,7 +386,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n \n \tfor (nr = 0; nr < active_nr; nr++) {\n \t\tconst struct cache_entry *ce = active_cache[nr];\n-\t\tif (!S_ISREG(ce->ce_mode) || ce_intent_to_add(ce))\n+\t\tif (!S_ISREG(ce->ce_mode))\n \t\t\tcontinue;\n \t\tif (!ce_path_match(ce, pathspec, NULL))\n \t\t\tcontinue;\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 1e72971..eae731a 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1364,4 +1364,33 @@ test_expect_success 'grep --color -e A --and -e B -p with context' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'grep can find things only in the work tree' '\n+\ttouch work-tree-only &&\n+\tgit add work-tree-only &&\n+\techo \"find in work tree\" >work-tree-only &&\n+\tgit grep --quiet \"find in work tree\" &&\n+\ttest_must_fail git grep --quiet --cached \"find in work tree\" &&\n+\ttest_must_fail git grep --quiet \"find in work tree\" HEAD &&\n+\tgit rm -f work-tree-only\n+'\n+\n+test_expect_success 'grep can find things only in the work tree (i-t-a)' '\n+\techo \"intend to add this\" >intend-to-add &&\n+\tgit add -N intend-to-add &&\n+\tgit grep --quiet \"intend to add this\" &&\n+\ttest_must_fail git grep --quiet --cached \"intend to add this\" &&\n+\ttest_must_fail git grep --quiet \"intend to add this\" HEAD &&\n+\tgit rm -f intend-to-add\n+'\n+\n+test_expect_success 'grep can find things only in the index' '\n+\techo \"only in the index\" >cache-this &&\n+\tgit add cache-this &&\n+\trm cache-this &&\n+\ttest_must_fail git grep --quiet \"only in the index\" &&\n+\tgit grep --quiet --cached \"only in the index\" &&\n+\ttest_must_fail git grep --quiet \"only in the index\" HEAD &&\n+\tgit rm --cached cache-this\n+'\n+\n test_done\n-- \n2.8.2.311.gee88674\n\n"}]}