{"thread":{"id":"42684","subject":"[PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","startedAt":"2016-06-21T21:33:27Z","lastAt":"2016-06-22T19:17:30Z","messageCount":9,"participants":["Charles Bailey","Junio C Hamano","Eric Sunshine","Duy Nguyen"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"289814","messageId":"20160621211412.28752-2-charles@hashpling.org","threadId":"42684","inReplyTo":"20160621211412.28752-1-charles@hashpling.org","subject":"[PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2016-06-21T21:14:12Z","receivedAt":"2016-06-21T21:33:27Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nThis reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e and adds an\nalternative fix to maintain the -L --cached behavior.\n\n4d5520053 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, despite the fact that these files are tracked.\n\nThe content in the index of a file for which the \"intent to add\" bit is\nset is considered indeterminate and not empty. For most grep queries we\nwant these to behave the same, however for -L --cached (files without a\nmatch) we don't want to respond positively for \"intent to add\" files as\ntheir contents are indeterminate. This is in contrast to files with\nempty contents in the index (no lines implies no matches for any grep\nquery expression) which should be reported in the output of a grep -L\n--cached invocation.\n\nAdd tests to cover this case and a few related cases which previously\nlacked coverage.\n\nHelped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n\nIs \"Helped-by\" an appropriate attribution in this case?\n\nPersonally, I could have gone either way on the -L --cached\nfunctionality but I know that Duy has put a lot more thought into\n\"intent-to-add\" entries so I trust his judgement.\n\n builtin/grep.c  |  4 ++--\n t/t7810-grep.sh | 38 ++++++++++++++++++++++++++++++++++++++\n 2 files changed, 40 insertions(+), 2 deletions(-)\n\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}\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex c4302ed..6c7ccb3 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -1364,4 +1364,42 @@ 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_expect_success 'grep does not report i-t-a with -L --cached' '\n+\techo \"intend to add this\" >intend-to-add &&\n+\tgit add -N intend-to-add &&\n+\tgit ls-files | grep -v \"^intend-to-add\\$\" >expected &&\n+\tgit grep -L --cached \"nonexistent_string\" >actual &&\n+\ttest_cmp expected actual &&\n+\tgit rm -f intend-to-add\n+'\n+\n test_done\n-- \n2.8.2.311.gee88674\n\n"},{"id":"289815","messageId":"20160621211412.28752-1-charles@hashpling.org","threadId":"42684","inReplyTo":null,"subject":"[PATCH v2 1/2] Fix duplicated test name","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2016-06-21T21:14:11Z","receivedAt":"2016-06-21T21:33:32Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"Signed-off-by: Charles Bailey <charles@hashpling.org>\n---\n\nSpotted while testing t7810-grep and grep \"i-t-a\" fixes.\n\n t/t7810-grep.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\nindex 1e72971..c4302ed 100755\n--- a/t/t7810-grep.sh\n+++ b/t/t7810-grep.sh\n@@ -353,7 +353,7 @@ test_expect_success 'grep -l -C' '\n cat >expected <<EOF\n file:5\n EOF\n-test_expect_success 'grep -l -C' '\n+test_expect_success 'grep -c -C' '\n \tgit grep -c -C1 foo >actual &&\n \ttest_cmp expected actual\n '\n-- \n2.8.2.311.gee88674\n\n"},{"id":"289819","messageId":"xmqqinx2nonl.fsf@gitster.mtv.corp.google.com","threadId":"42684","inReplyTo":"20160621211412.28752-2-charles@hashpling.org","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-21T22:49:18Z","receivedAt":"2016-06-21T22:50:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Charles Bailey <charles@hashpling.org> writes:\n\n> Is \"Helped-by\" an appropriate attribution in this case?\n\nSure.\n\n> diff --git a/builtin/grep.c b/builtin/grep.c\n> index 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\nOK, so this function handles searching in either the index or the\nworking tree.\n\nThe first hunk used to unconditionally discard paths marked as\ni-t-a, even when we are looking at the working tree, which is\nclearly useless, and we stop rejecting i-t-a paths too early, which\nis good.\n\nThe second hunk is for \"grep --cached\" but also covers two other\ncases.  What are these?\n\nCE_VALID is used by \"Assume unchanged\".  Because the user promised\nthat s/he will take responsibility of keeping the working tree\ncontents in sync with what is in the index by not modifying it, even\nwhen we are not doing \"grep --cached\", we pick up the contents from\nthe index and look for the string in there, instead of going to the\nworking tree.  In other words, even though at the mechanical level\nwe are looking into the index, logically we are searching in the\nworking tree.  Is it sensible to skip i-t-a entries in that case?\n\nI think the same discussion would apply to CE_SKIP_WORKTREE (see\n\"Skip-worktree bit\" in Documentation/git-update-index.txt).\n\nSo I wonder if a better change would be more like\n\n\tfor (...) {\n        \tif (!S_ISREG(ce->ce_mode))\n                \tcontinue; /* not a regular file */\n\t\tif (!ce_path_match(ce, pathspec, NULL)\n                \tcontinue; /* uninteresting */\n+\t\tif (cached && ce_intent_to_add(ce))\n+\t\t\tcontinue; /* path not yet in the index */\n\t\t        \n\t\tif (cached || ...)\n                \tUNCHANGED FROM THE ORIGINAL\n\nperhaps?\n\nI actually think (ce->ce_flags & CE_VALID) case should go to working\ntree for performance, but that is a separate topic.\n\n> +test_expect_success 'grep can find things only in the work tree' '\n> +\ttouch work-tree-only &&\n\nPlease do not use \"touch\" if the reason to use the command is *not*\nto muck with the timestamp, e.g. to create an empty file.\n\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_expect_success 'grep does not report i-t-a with -L --cached' '\n> +\techo \"intend to add this\" >intend-to-add &&\n> +\tgit add -N intend-to-add &&\n> +\tgit ls-files | grep -v \"^intend-to-add\\$\" >expected &&\n> +\tgit grep -L --cached \"nonexistent_string\" >actual &&\n> +\ttest_cmp expected actual &&\n> +\tgit rm -f intend-to-add\n> +'\n> +\n>  test_done\n"},{"id":"289822","messageId":"CAPig+cQ4CxRo460dcTJJtV_dPH8i5HC76_gpTv8attEZ8sdMZw@mail.gmail.com","threadId":"42684","inReplyTo":"20160621211412.28752-2-charles@hashpling.org","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-06-22T01:13:10Z","receivedAt":"2016-06-22T01:13:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Jun 21, 2016 at 5:14 PM, Charles Bailey <charles@hashpling.org> wrote:\n> From: Charles Bailey <cbailey32@bloomberg.net>\n>\n> This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e and adds an\n> alternative fix to maintain the -L --cached behavior.\n\nIt is common to provide some context along with the (shortened) commit\nID. For instance:\n\n    This reverts 4d55200 (grep: make it clear i-t-a entries are\n    ignored, 2015-12-27) and adds ...\n\n> 4d5520053 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, despite the fact that these files are tracked.\n> [...]\n> Helped-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n> Signed-off-by: Charles Bailey <cbailey32@bloomberg.net>\n> ---\n>\n> Is \"Helped-by\" an appropriate attribution in this case?\n\nVery much so.\n\n> diff --git a/t/t7810-grep.sh b/t/t7810-grep.sh\n> @@ -1364,4 +1364,42 @@ test_expect_success 'grep --color -e A --and -e B -p with context' '\n> +test_expect_success 'grep can find things only in the work tree' '\n> +       touch work-tree-only &&\n\nAvoid 'touch' if the timestamp of the file has no significance. Use '>' instead:\n\n    >work-tree-only &&\n\n> +       git add work-tree-only &&\n> +       echo \"find in work tree\" >work-tree-only &&\n> +       git grep --quiet \"find in work tree\" &&\n> +       test_must_fail git grep --quiet --cached \"find in work tree\" &&\n> +       test_must_fail git grep --quiet \"find in work tree\" HEAD &&\n> +       git rm -f work-tree-only\n\nIf any statement before this cleanup code fails, then the cleanup will\nnever take place (due to the &&-chain). To ensure cleanup regardless\nof test outcome, instead use test_when_finished() at the beginning of\nthe test:\n\n    test_when_finished \"git rm -f work-tree-only\" &&\n\nSame applies to other added tests.\n\n> +'\n> +\n> +test_expect_success 'grep can find things only in the work tree (i-t-a)' '\n> +       echo \"intend to add this\" >intend-to-add &&\n> +       git add -N intend-to-add &&\n> +       git grep --quiet \"intend to add this\" &&\n> +       test_must_fail git grep --quiet --cached \"intend to add this\" &&\n> +       test_must_fail git grep --quiet \"intend to add this\" HEAD &&\n> +       git rm -f intend-to-add\n> +'\n> +\n> +test_expect_success 'grep can find things only in the index' '\n> +       echo \"only in the index\" >cache-this &&\n> +       git add cache-this &&\n> +       rm cache-this &&\n> +       test_must_fail git grep --quiet \"only in the index\" &&\n> +       git grep --quiet --cached \"only in the index\" &&\n> +       test_must_fail git grep --quiet \"only in the index\" HEAD &&\n> +       git rm --cached cache-this\n> +'\n> +\n> +test_expect_success 'grep does not report i-t-a with -L --cached' '\n> +       echo \"intend to add this\" >intend-to-add &&\n> +       git add -N intend-to-add &&\n> +       git ls-files | grep -v \"^intend-to-add\\$\" >expected &&\n> +       git grep -L --cached \"nonexistent_string\" >actual &&\n> +       test_cmp expected actual &&\n> +       git rm -f intend-to-add\n> +'\n> +\n>  test_done\n"},{"id":"289859","messageId":"CACsJy8Biaowr-XoaJgOCXjDGre==CzeSyADftCAdzxFHoxrZAQ@mail.gmail.com","threadId":"42684","inReplyTo":"CAPig+cQ4CxRo460dcTJJtV_dPH8i5HC76_gpTv8attEZ8sdMZw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-22T16:01:44Z","receivedAt":"2016-06-22T16:02:18Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 22, 2016 at 3:13 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:\n> On Tue, Jun 21, 2016 at 5:14 PM, Charles Bailey <charles@hashpling.org> wrote:\n>> From: Charles Bailey <cbailey32@bloomberg.net>\n>>\n>> This reverts commit 4d552005323034c1d6311796ac1074e9a4b4b57e and adds an\n>> alternative fix to maintain the -L --cached behavior.\n>\n> It is common to provide some context along with the (shortened) commit\n> ID. For instance:\n>\n>     This reverts 4d55200 (grep: make it clear i-t-a entries are\n>     ignored, 2015-12-27) and adds ...\n\nAnd that could be produced with some git alias like\n\ngit config alias.one 'show -s --date=short --pretty='format:%h (%s - %ad)'\n\nNo point in manually copy/pasting the context.\n-- \nDuy\n"},{"id":"289860","messageId":"CACsJy8C9Dh_Owr3UFJnCtvXserG4V-e1ws8ZY52ME1yr+fefOw@mail.gmail.com","threadId":"42684","inReplyTo":"xmqqinx2nonl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-22T16:11:35Z","receivedAt":"2016-06-22T16:12:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 22, 2016 at 12:49 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> @@ -396,7 +396,7 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int\n>>                * cache version instead\n>>                */\n>>               if (cached || (ce->ce_flags & CE_VALID) || ce_skip_worktree(ce)) {\n>> -                     if (ce_stage(ce))\n>> +                     if (ce_stage(ce) || ce_intent_to_add(ce))\n>>                               continue;\n>>                       hit |= grep_sha1(opt, ce->sha1, ce->name, 0, ce->name);\n>>               }\n>\n> OK, so this function handles searching in either the index or the\n> working tree.\n>\n> The first hunk used to unconditionally discard paths marked as\n> i-t-a, even when we are looking at the working tree, which is\n> clearly useless, and we stop rejecting i-t-a paths too early, which\n> is good.\n>\n> The second hunk is for \"grep --cached\" but also covers two other\n> cases.  What are these?\n>\n> CE_VALID is used by \"Assume unchanged\".  Because the user promised\n> that s/he will take responsibility of keeping the working tree\n> contents in sync with what is in the index by not modifying it, even\n> when we are not doing \"grep --cached\", we pick up the contents from\n> the index and look for the string in there, instead of going to the\n> working tree.  In other words, even though at the mechanical level\n> we are looking into the index, logically we are searching in the\n> working tree.  Is it sensible to skip i-t-a entries in that case?\n>\n> I think the same discussion would apply to CE_SKIP_WORKTREE (see\n> \"Skip-worktree bit\" in Documentation/git-update-index.txt).\n>\n> So I wonder if a better change would be more like\n>\n>         for (...) {\n>                 if (!S_ISREG(ce->ce_mode))\n>                         continue; /* not a regular file */\n>                 if (!ce_path_match(ce, pathspec, NULL)\n>                         continue; /* uninteresting */\n> +               if (cached && ce_intent_to_add(ce))\n> +                       continue; /* path not yet in the index */\n>\n>                 if (cached || ...)\n>                         UNCHANGED FROM THE ORIGINAL\n>\n> perhaps?\n\nI did wonder a bit about these cases. But, can i-t-a really be\ncombined with CE_VALID or CE_SKIP_WORKTREE? CE_SKIP_... is\nautomatically set and should not cover i-t-a entries imo (I didn't\ncheck the implementation). CE_VALID is about real entries, yes you\ncould do \"git update-index --assume-unchanged <ita-path>\" but it does\nnot feel right to me.\n\nIf cached is false and ce_ita() is true and either CE_VALID or\nCE_SKIP_WORKTREE is set, we would continue to grep an _empty_ SHA-1.\nBut I think we should grep_file() instead, at least for CE_VALID.\n-- \nDuy\n"},{"id":"289877","messageId":"xmqqlh1xm7c5.fsf@gitster.mtv.corp.google.com","threadId":"42684","inReplyTo":"CACsJy8C9Dh_Owr3UFJnCtvXserG4V-e1ws8ZY52ME1yr+fefOw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-22T18:00:58Z","receivedAt":"2016-06-22T18:01:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>> So I wonder if a better change would be more like\n>>\n>>         for (...) {\n>>                 if (!S_ISREG(ce->ce_mode))\n>>                         continue; /* not a regular file */\n>>                 if (!ce_path_match(ce, pathspec, NULL)\n>>                         continue; /* uninteresting */\n>> +               if (cached && ce_intent_to_add(ce))\n>> +                       continue; /* path not yet in the index */\n>>\n>>                 if (cached || ...)\n>>                         UNCHANGED FROM THE ORIGINAL\n>>\n>> perhaps?\n>\n> I did wonder a bit about these cases. But, can i-t-a really be\n> combined with CE_VALID or CE_SKIP_WORKTREE? CE_SKIP_... is\n> automatically set and should not cover i-t-a entries imo (I didn't\n> check the implementation). CE_VALID is about real entries, yes you\n> could do \"git update-index --assume-unchanged <ita-path>\" but it does\n> not feel right to me.\n\nYeah but we know people are stupid^W^Wdo unexpected things ;-)\n\n> If cached is false and ce_ita() is true and either CE_VALID or\n> CE_SKIP_WORKTREE is set, we would continue to grep an _empty_ SHA-1.\n> But I think we should grep_file() instead, at least for CE_VALID.\n\nYes, that is the breakage I noticed in the patch under discussion\nand that I wanted to fix in the \"I wonder if a better change would\nbe...\" version.\n"},{"id":"289880","messageId":"CACsJy8Acb+Hx1R66hcHQ7gNQ6TmKoUzC7Ar2PpSPkQeKM1EY8w@mail.gmail.com","threadId":"42684","inReplyTo":"xmqqlh1xm7c5.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-06-22T18:32:42Z","receivedAt":"2016-06-22T18:33:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Jun 22, 2016 at 8:00 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Duy Nguyen <pclouds@gmail.com> writes:\n>\n>>> So I wonder if a better change would be more like\n>>>\n>>>         for (...) {\n>>>                 if (!S_ISREG(ce->ce_mode))\n>>>                         continue; /* not a regular file */\n>>>                 if (!ce_path_match(ce, pathspec, NULL)\n>>>                         continue; /* uninteresting */\n>>> +               if (cached && ce_intent_to_add(ce))\n>>> +                       continue; /* path not yet in the index */\n>>>\n>>>                 if (cached || ...)\n>>>                         UNCHANGED FROM THE ORIGINAL\n>>>\n>>> perhaps?\n>>\n>> I did wonder a bit about these cases. But, can i-t-a really be\n>> combined with CE_VALID or CE_SKIP_WORKTREE? CE_SKIP_... is\n>> automatically set and should not cover i-t-a entries imo (I didn't\n>> check the implementation). CE_VALID is about real entries, yes you\n>> could do \"git update-index --assume-unchanged <ita-path>\" but it does\n>> not feel right to me.\n>\n> Yeah but we know people are stupid^W^Wdo unexpected things ;-)\n>\n>> If cached is false and ce_ita() is true and either CE_VALID or\n>> CE_SKIP_WORKTREE is set, we would continue to grep an _empty_ SHA-1.\n>> But I think we should grep_file() instead, at least for CE_VALID.\n>\n> Yes, that is the breakage I noticed in the patch under discussion\n> and that I wanted to fix in the \"I wonder if a better change would\n> be...\" version.\n\nHeh.. I did guess that. Since neither solution is complete, I'm in\nfavor of Charles's and assume that i-t-a forces to ignore CE_SKIP and\nCE_SKIP_WORKTREE. I could wait for people to come back complaining,\nthen we know there are real users in very obscure cases and will fix\nit then.\n-- \nDuy\n"},{"id":"289889","messageId":"xmqq8txxm3ss.fsf@gitster.mtv.corp.google.com","threadId":"42684","inReplyTo":"CACsJy8Acb+Hx1R66hcHQ7gNQ6TmKoUzC7Ar2PpSPkQeKM1EY8w@mail.gmail.com","subject":"Re: [PATCH v2 2/2] grep: fix grepping for \"intent to add\" files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-06-22T19:17:23Z","receivedAt":"2016-06-22T19:17:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen <pclouds@gmail.com> writes:\n\n>>> If cached is false and ce_ita() is true and either CE_VALID or\n>>> CE_SKIP_WORKTREE is set, we would continue to grep an _empty_ SHA-1.\n>>> But I think we should grep_file() instead, at least for CE_VALID.\n>>\n>> Yes, that is the breakage I noticed in the patch under discussion\n>> and that I wanted to fix in the \"I wonder if a better change would\n>> be...\" version.\n>\n> Heh.. I did guess that. Since neither solution is complete, I'm in\n> favor of Charles's and assume that i-t-a forces to ignore CE_SKIP and\n> CE_SKIP_WORKTREE. I could wait for people to come back complaining,\n> then we know there are real users in very obscure cases and will fix\n> it then.\n\nI said something that can be misunderstood.  I meant \"I wonder if ...\"\nversion is correct.  Charles's has the bugs you mentioned and I\nwanted to fix them by sending the \"I wonder if...\" version out.\n\nBut you seem to have misread my statement as \"A bug is in my version\nand I want to fix that bug in my version\".  That is not what I\nmeant.\n"}]}