{"thread":{"id":"56607","subject":"[PATCH] grep: demonstrate bug with textconv attributes and submodules","startedAt":"2021-09-28T17:08:36Z","lastAt":"2021-09-29T12:24:38Z","messageCount":5,"participants":["Matheus Tavares","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"437329","messageId":"8c266e58dede247b2c97ad2870c7c24c3b35ed55.1632848754.git.matheus.bernardino@usp.br","threadId":"56607","inReplyTo":null,"subject":"[PATCH] grep: demonstrate bug with textconv attributes and submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-28T17:08:27Z","receivedAt":"2021-09-28T17:08:36Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In some circumstances, \"git grep --textconv --recurse-submodules\"\nignores the textconv attributes from the submodules and erroneuosly\napply the attributes defined in the superproject on the submodules'\nfiles. The textconv cache is also saved on the superproject, even for\nsubmodule objects.\n\nA fix for these problems will probably require at least three changes:\n\n- Some textconv and attributes functions (as well as their callees) will\n  have to be adjusted to work with arbitrary repositories. Note that\n  \"fill_textconv()\", for example, already receives a \"struct repository\"\n  but it writes the textconv cache using \"write_loose_object()\", which\n  implicitly works on \"the_repository\".\n\n- grep.c functions will have to call textconv/userdiff routines passing\n  the \"repo\" field from \"struct grep_source\" instead of the one from\n  \"struct grep_opt\". The latter always points to \"the_repository\" on\n  \"git grep\" executions (see its initialization in builtin/grep.c), but\n  the former points to the correct repository that each source (an\n  object, file, or buffer) comes from.\n\n- \"userdiff_find_by_path()\" might need to use a different attributes\n  stack for each repository it works on or reset its internal static\n  stack when the repository is changed throughout the calls.\n\nFor now, let's add some tests to demonstrate these problems, and also\nupdate a NEEDSWORK comment in grep.h that mentions this bug to reference\nthe added tests.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n grep.h                             |   6 +-\n t/t7814-grep-recurse-submodules.sh | 103 +++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.h b/grep.h\nindex 128007db65..3b63bd0253 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -128,9 +128,9 @@ struct grep_opt {\n \t * instead.\n \t *\n \t * This is potentially the cause of at least one bug - \"git grep\"\n-\t * ignoring the textconv attributes from submodules. See [1] for more\n-\t * information.\n-\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n+\t * using the textconv attributes from the superproject on the\n+\t * submodules. See the failing \"git grep --textconv\" tests in\n+\t * t7814-grep-recurse-submodules.sh for more information.\n \t */\n \tstruct repository *repo;\n \ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 3172f5b936..cfbaee3851 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -441,4 +441,107 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n \ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n \ttest_must_be_empty actual\n '\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\trm .gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\tsuper_attr=\"$(git rev-parse --git-path info/attributes)\" &&\n+\ttest_when_finished \"rm -f \\\"$super_attr\\\"\" &&\n+\techo \"a diff=d2x\" >\"$super_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Note: what currently prevents this test from passing is not that the\n+# .gitattributes file from \"./submodule\" is being ignored, but that it is being\n+# propagated to the nested \"./submodule/sub\" files.\n+#\n+test_expect_failure 'grep --textconv corectly reads submodule .gitattributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv corectly reads submodule .gitattributes (from index)' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\tgit -C submodule add .gitattributes &&\n+\trm submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv corectly reads submodule .git/info/attributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\n+\tsubmodule_attr=\"$(git -C submodule rev-parse --path-format=absolute --git-path info/attributes)\" &&\n+\ttest_when_finished \"rm -f \\\"$submodule_attr\\\"\" &&\n+\techo \"a diff=d2x\" >\"$submodule_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep saves textconv cache in the appropriated repository' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x_cached.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\ttest_config_global diff.d2x_cached.cachetextconv true &&\n+\techo \"a diff=d2x_cached\" >submodule/.gitattributes &&\n+\n+\t# We only read/write to the textconv cache when grepping from an OID,\n+\t# as the working tree file might have modifications.\n+\tgit grep --textconv --cached --recurse-submodules x &&\n+\n+\tsuper_textconv_cache=\"$(git rev-parse --git-path refs/notes/textconv/d2x_cached)\" &&\n+\tsub_textconv_cache=\"$(git -C submodule rev-parse \\\n+\t\t\t--path-format=absolute --git-path refs/notes/textconv/d2x_cached)\" &&\n+\ttest_path_is_missing \"$super_textconv_cache\" &&\n+\ttest_path_is_file \"$sub_textconv_cache\"\n+'\n+\n test_done\n-- \n2.33.0\n\n"},{"id":"437330","messageId":"CAPig+cS1xrXHBbxwcwL_WKdnU9_MSTZffMtn3FxdUGuQX85XbA@mail.gmail.com","threadId":"56607","inReplyTo":"8c266e58dede247b2c97ad2870c7c24c3b35ed55.1632848754.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] grep: demonstrate bug with textconv attributes and submodules","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-28T17:15:56Z","receivedAt":"2021-09-28T17:16:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Sep 28, 2021 at 1:08 PM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n> In some circumstances, \"git grep --textconv --recurse-submodules\"\n> ignores the textconv attributes from the submodules and erroneuosly\n> apply the attributes defined in the superproject on the submodules'\n> files. The textconv cache is also saved on the superproject, even for\n> submodule objects.\n\ns/erroneuosly/erroneously/\n\nAlso, perhaps: s/apply/applies/\n\n> Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> @@ -441,4 +441,107 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n> +test_expect_failure 'grep --textconv corectly reads submodule .gitattributes' '\n\nHere and in remaining newly added tests: s/corectly/correctly/\n\n> +test_expect_failure 'grep saves textconv cache in the appropriated repository' '\n\ns/appropriated/appropriate/\n"},{"id":"437332","messageId":"CAHd-oW4aQtY8gBG_UhcgU6kfbkBX1TkjHBTLnNJsCGJdMrBfhQ@mail.gmail.com","threadId":"56607","inReplyTo":"8c266e58dede247b2c97ad2870c7c24c3b35ed55.1632848754.git.matheus.bernardino@usp.br","subject":"Re: [PATCH] grep: demonstrate bug with textconv attributes and submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-28T17:16:57Z","receivedAt":"2021-09-28T17:17:12Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Sep 28, 2021 at 2:08 PM Matheus Tavares\n<matheus.bernardino@usp.br> wrote:\n>\n> Subject: [PATCH] grep: demonstrate bug with textconv attributes and submodules\n\nI probably should have referenced (or linked to) the thread this came\nfrom: https://lore.kernel.org/git/87zgryylfx.fsf@evledraar.gmail.com/\n"},{"id":"437333","messageId":"CAHd-oW4fNNp+A_fFeqbTeOwDV+ss0-0x=rt0HOi2m1txrh+1ZQ@mail.gmail.com","threadId":"56607","inReplyTo":"CAPig+cS1xrXHBbxwcwL_WKdnU9_MSTZffMtn3FxdUGuQX85XbA@mail.gmail.com","subject":"Re: [PATCH] grep: demonstrate bug with textconv attributes and submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-28T17:18:34Z","receivedAt":"2021-09-28T17:18:48Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Sep 28, 2021 at 2:16 PM Eric Sunshine <sunshine@sunshineco.com> wrote:\n>\n> On Tue, Sep 28, 2021 at 1:08 PM Matheus Tavares\n> <matheus.bernardino@usp.br> wrote:\n> > In some circumstances, \"git grep --textconv --recurse-submodules\"\n> > ignores the textconv attributes from the submodules and erroneuosly\n> > apply the attributes defined in the superproject on the submodules'\n> > files. The textconv cache is also saved on the superproject, even for\n> > submodule objects.\n>\n> s/erroneuosly/erroneously/\n>\n> Also, perhaps: s/apply/applies/\n>\n> > Signed-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n> > diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\n> > @@ -441,4 +441,107 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n> > +test_expect_failure 'grep --textconv corectly reads submodule .gitattributes' '\n>\n> Here and in remaining newly added tests: s/corectly/correctly/\n>\n> > +test_expect_failure 'grep saves textconv cache in the appropriated repository' '\n>\n> s/appropriated/appropriate/\n\nOops, I should have been more careful with the writing. Thanks for\ncatching those!\n"},{"id":"437454","messageId":"8c8932465cd2fb2f0d4d6d9a5b86e51e2a72865b.1632918166.git.matheus.bernardino@usp.br","threadId":"56607","inReplyTo":"8c266e58dede247b2c97ad2870c7c24c3b35ed55.1632848754.git.matheus.bernardino@usp.br","subject":"[PATCH v2] grep: demonstrate bug with textconv attributes and submodules","fromName":"Matheus Tavares","fromEmail":"matheus.bernardino@usp.br","sentAt":"2021-09-29T12:24:25Z","receivedAt":"2021-09-29T12:24:38Z","isPatch":true,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"In some circumstances, \"git grep --textconv --recurse-submodules\"\nignores the textconv attributes from the submodules and erroneously\napplies the attributes defined in the superproject on the submodules'\nfiles. The textconv cache is also saved on the superproject, even for\nsubmodule objects.\n\nA fix for these problems will probably require at least three changes:\n\n- Some textconv and attributes functions (as well as their callees) will\n  have to be adjusted to work with arbitrary repositories. Note that\n  \"fill_textconv()\", for example, already receives a \"struct repository\"\n  but it writes the textconv cache using \"write_loose_object()\", which\n  implicitly works on \"the_repository\".\n\n- grep.c functions will have to call textconv/userdiff routines passing\n  the \"repo\" field from \"struct grep_source\" instead of the one from\n  \"struct grep_opt\". The latter always points to \"the_repository\" on\n  \"git grep\" executions (see its initialization in builtin/grep.c), but\n  the former points to the correct repository that each source (an\n  object, file, or buffer) comes from.\n\n- \"userdiff_find_by_path()\" might need to use a different attributes\n  stack for each repository it works on or reset its internal static\n  stack when the repository is changed throughout the calls.\n\nFor now, let's add some tests to demonstrate these problems, and also\nupdate a NEEDSWORK comment in grep.h that mentions this bug to reference\nthe added tests.\n\nSigned-off-by: Matheus Tavares <matheus.bernardino@usp.br>\n---\n\nChanged in v2: fixed typos in commit message and test names\n\n grep.h                             |   6 +-\n t/t7814-grep-recurse-submodules.sh | 103 +++++++++++++++++++++++++++++\n 2 files changed, 106 insertions(+), 3 deletions(-)\n\ndiff --git a/grep.h b/grep.h\nindex 128007db65..3b63bd0253 100644\n--- a/grep.h\n+++ b/grep.h\n@@ -128,9 +128,9 @@ struct grep_opt {\n \t * instead.\n \t *\n \t * This is potentially the cause of at least one bug - \"git grep\"\n-\t * ignoring the textconv attributes from submodules. See [1] for more\n-\t * information.\n-\t * [1] https://lore.kernel.org/git/CAHd-oW5iEQarYVxEXoTG-ua2zdoybTrSjCBKtO0YT292fm0NQQ@mail.gmail.com/\n+\t * using the textconv attributes from the superproject on the\n+\t * submodules. See the failing \"git grep --textconv\" tests in\n+\t * t7814-grep-recurse-submodules.sh for more information.\n \t */\n \tstruct repository *repo;\n \ndiff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh\nindex 3172f5b936..058e5d0c96 100755\n--- a/t/t7814-grep-recurse-submodules.sh\n+++ b/t/t7814-grep-recurse-submodules.sh\n@@ -441,4 +441,107 @@ test_expect_success 'grep --recurse-submodules with --cached ignores worktree mo\n \ttest_must_fail git grep --recurse-submodules --cached \"A modified line in submodule\" >actual 2>&1 &&\n \ttest_must_be_empty actual\n '\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .gitattributes (from index) does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\trm .gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv: superproject .git/info/attributes does not affect submodules' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\tsuper_attr=\"$(git rev-parse --git-path info/attributes)\" &&\n+\ttest_when_finished \"rm -f \\\"$super_attr\\\"\" &&\n+\techo \"a diff=d2x\" >\"$super_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\ta:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# Note: what currently prevents this test from passing is not that the\n+# .gitattributes file from \"./submodule\" is being ignored, but that it is being\n+# propagated to the nested \"./submodule/sub\" files.\n+#\n+test_expect_failure 'grep --textconv correctly reads submodule .gitattributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv correctly reads submodule .gitattributes (from index)' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\techo \"a diff=d2x\" >submodule/.gitattributes &&\n+\tgit -C submodule add .gitattributes &&\n+\trm submodule/.gitattributes &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep --textconv correctly reads submodule .git/info/attributes' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\n+\tsubmodule_attr=\"$(git -C submodule rev-parse --path-format=absolute --git-path info/attributes)\" &&\n+\ttest_when_finished \"rm -f \\\"$submodule_attr\\\"\" &&\n+\techo \"a diff=d2x\" >\"$submodule_attr\" &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tsubmodule/a:(1|2)x(3|4)\n+\tEOF\n+\tgit grep --textconv --recurse-submodules x >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'grep saves textconv cache in the appropriate repository' '\n+\treset_and_clean &&\n+\ttest_config_global diff.d2x_cached.textconv \"sed -e \\\"s/d/x/\\\"\" &&\n+\ttest_config_global diff.d2x_cached.cachetextconv true &&\n+\techo \"a diff=d2x_cached\" >submodule/.gitattributes &&\n+\n+\t# We only read/write to the textconv cache when grepping from an OID,\n+\t# as the working tree file might have modifications.\n+\tgit grep --textconv --cached --recurse-submodules x &&\n+\n+\tsuper_textconv_cache=\"$(git rev-parse --git-path refs/notes/textconv/d2x_cached)\" &&\n+\tsub_textconv_cache=\"$(git -C submodule rev-parse \\\n+\t\t\t--path-format=absolute --git-path refs/notes/textconv/d2x_cached)\" &&\n+\ttest_path_is_missing \"$super_textconv_cache\" &&\n+\ttest_path_is_file \"$sub_textconv_cache\"\n+'\n+\n test_done\n-- \n2.33.0\n\n"}]}