{"thread":{"id":"49518","subject":"[PATCH] builtin/grep.c: remote superflous submodule code","startedAt":"2018-10-09T18:36:06Z","lastAt":"2018-10-10T22:49:27Z","messageCount":3,"participants":["Stefan Beller","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359935","messageId":"20181009183549.195316-1-sbeller@google.com","threadId":"49518","inReplyTo":null,"subject":"[PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-09T18:35:49Z","receivedAt":"2018-10-09T18:36:06Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In f9ee2fcdfa (grep: recurse in-process using 'struct repository',\n2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep\nto simplify the submodule handling.\n\nAfter ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules\nfile, 2017-08-03) this is no longer necessary, but that commit did not\ncleanup the whole tree, but just show cased the new way how to deal with\nsubmodules in ls-files.\n\nIt claimed that grep would still need some explicit handling, but that is\nnot the call to repo_read_gitmodules (applying this patch on top of\nff6f1f564c4 still keep the test suite happy, specifically\nt7814-grep-recurse-submodules, which contains a test\n\"grep history with moved submoules\")\n\nThe special handling is the call to gitmodules_config_oid which was added\nalready in 74ed43711f (grep: enable recurse-submodules to work on\n<tree> objects, 2016-12-16), but then was still named\ngitmodules_config_sha1.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nAcked-by: Antonio Ospite <ao2@ao2.it>\n---\n\nThis is a resend of origin/sb/grep-submodule-cleanup,\nand I think picking ff6f1f564c4 as the base for the series would\nalso be appropriate.\n\nStefan\n\n\n builtin/grep.c | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 601f801158..a6272b9c2f 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -427,8 +427,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,\n \tif (repo_submodule_init(&submodule, superproject, path))\n \t\treturn 0;\n \n-\trepo_read_gitmodules(&submodule);\n-\n \t/*\n \t * NEEDSWORK: This adds the submodule's object directory to the list of\n \t * alternates for the single in-memory object store.  This has some bad\n-- \n2.19.0\n\n"},{"id":"359966","messageId":"20181010001037.74709-1-jonathantanmy@google.com","threadId":"49518","inReplyTo":"20181009183549.195316-1-sbeller@google.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-10T00:10:37Z","receivedAt":"2018-10-10T00:10:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> It claimed that grep would still need some explicit handling, but that is\n> not the call to repo_read_gitmodules (applying this patch on top of\n> ff6f1f564c4 still keep the test suite happy, specifically\n> t7814-grep-recurse-submodules, which contains a test\n> \"grep history with moved submoules\")\n\nFirstly, spelling of \"remove\" and \"superfluous\" in the commit title.\n\nI don't think the \"grep history with moved submodules\" test exercises\nmuch. That test only tests the superproject > submodule case, but we\nneed a superproject > submodule > sub-submodule case, because what is\nbeing removed is a call to repo_read_gitmodules() on a repository\n(\"struct repository submodule\") that has a superproject (\"struct\nrepository *superproject\"). In other words, we need a submodule that has\nits own gitmodules.\n\nAlternatively, it would be fine if someone could point out where the\n.gitmodules file is lazily loaded when grep_submodule() is invoked. I\ncouldn't find it, although I wasn't looking very hard. I did look at the\ninvocation of repo_submodule_init() (right before the removed lines),\nwhich indeed calls repo_read_gitmodules() indirectly through\nsubmodule_from_path(), but that is called on the superproject, whereas\nwhat is being removed is a call on the submodule.\n\n> The special handling is the call to gitmodules_config_oid which was added\n> already in 74ed43711f (grep: enable recurse-submodules to work on\n> <tree> objects, 2016-12-16), but then was still named\n> gitmodules_config_sha1.\n\nIf you're stating that gitmodules_config_oid() is where the .gitmodules\nfile is lazily loaded, it doesn't seem to be that way, because that\nfunction works only on the_repository (at least on 'master' and 'next').\n\n> This is a resend of origin/sb/grep-submodule-cleanup,\n> and I think picking ff6f1f564c4 as the base for the series would\n> also be appropriate.\n\nAny particular reason why you suggest that commit (which is more than a\nyear old)? It seems that basing this on 'master' is fine.\n"},{"id":"360102","messageId":"CAGZ79kbZ35OEh=2JiZuOHgG-P3a3PeSP5hgrXX-tQh9feOQsjQ@mail.gmail.com","threadId":"49518","inReplyTo":"20181010001037.74709-1-jonathantanmy@google.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-10T22:49:13Z","receivedAt":"2018-10-10T22:49:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 9, 2018 at 5:10 PM Jonathan Tan <jonathantanmy@google.com> wrote:\n>\n> > It claimed that grep would still need some explicit handling, but that is\n> > not the call to repo_read_gitmodules (applying this patch on top of\n> > ff6f1f564c4 still keep the test suite happy, specifically\n> > t7814-grep-recurse-submodules, which contains a test\n> > \"grep history with moved submoules\")\n>\n> Firstly, spelling of \"remove\" and \"superfluous\" in the commit title.\n>\n> I don't think the \"grep history with moved submodules\" test exercises\n> much. That test only tests the superproject > submodule case, but we\n> need a superproject > submodule > sub-submodule case, because what is\n> being removed is a call to repo_read_gitmodules() on a repository\n> (\"struct repository submodule\") that has a superproject (\"struct\n> repository *superproject\"). In other words, we need a submodule that has\n> its own gitmodules.\n\nRight; we do have a test 'grep and nested submodules', which still passes.\nI added another test, that would grep through nested submodules in\nthe history (not checked out), but that would not work on nested submodules\nwith or without this patch applied. (As the nested submodule is not checked\nout, is_submodule_active(repo, path) would return false and we'd not dive\ninto the nested submodule.\n\nI looked into ao/submodule-wo-gitmodules-checked-out, as that touches\nthis area of code as well and promises to allow working with submodules\nwhen .gitmodules is not checked out, it doesn't help this use case, either.\n\nThat is (as Antonio diagnosed), due to get_oid not working with a repository\nhandle, yet.\n\n> > The special handling is the call to gitmodules_config_oid which was added\n> > already in 74ed43711f (grep: enable recurse-submodules to work on\n> > <tree> objects, 2016-12-16), but then was still named\n> > gitmodules_config_sha1.\n>\n> If you're stating that gitmodules_config_oid() is where the .gitmodules\n> file is lazily loaded, it doesn't seem to be that way, because that\n> function works only on the_repository (at least on 'master' and 'next').\n\nyes, that is why nested submodules do not work currently when they\nare not in the working tree.\n\n>\n> > This is a resend of origin/sb/grep-submodule-cleanup,\n> > and I think picking ff6f1f564c4 as the base for the series would\n> > also be appropriate.\n>\n> Any particular reason why you suggest that commit (which is more than a\n> year old)? It seems that basing this on 'master' is fine.\n\nAfter more analysis, I think we'd want to wait for Antonios series to land\nand then build on top of that, while also getting get_oid converted.\n\nRegarding this patch, let's retract it for now and revisit it once we have\nmore submodule infrastructure working.\n\nThanks,\nStefan\n"}]}