{"thread":{"id":"49496","subject":"[PATCH] builtin/grep.c: remote superflous submodule code","startedAt":"2018-10-05T22:46:04Z","lastAt":"2018-10-09T00:14:15Z","messageCount":5,"participants":["Stefan Beller","Antonio Ospite","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"359758","messageId":"20181005224557.31420-1-sbeller@google.com","threadId":"49496","inReplyTo":null,"subject":"[PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-05T22:45:57Z","receivedAt":"2018-10-05T22:46:04Z","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\nCleanup the only remaining caller to repo_read_gitmodules outside of\nsubmodule.c\n\nSigned-off-by: Stefan Beller \n---\n\nAntonio Ospite writes:\n> BTW, with Stefan Beller we also identified some unneeded code which\n> could have been removed to alleviate the issue, but that would not have\n> solved it completely; so, I am not removing the unnecessary call to\n> repo_read_gitmodules() builtin/grep.c in this series, possibly this can\n> become a stand-alone change.\n\nHere is the stand-alone change.\n\nThe patch [1] contains the lines as deleted below in the context lines\nbut they would not conflict as there is one empty line between the changes\nin this patch in [1].\n\n[1] https://public-inbox.org/git/20181005130601.15879-10-ao2@ao2.it/\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":"359765","messageId":"20181006105959.47f8ccf38281d8bdd07448e7@ao2.it","threadId":"49496","inReplyTo":"20181005224557.31420-1-sbeller@google.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Antonio Ospite","fromEmail":"ao2@ao2.it","sentAt":"2018-10-06T08:59:59Z","receivedAt":"2018-10-06T09:03:25Z","isPatch":true,"sender":{"key":"ao2@ao2.it","avatar":"https://avatars.githubusercontent.com/u/1249395?v=4"},"body":"On Fri, 5 Oct 2018 15:45:57 -0700\nStefan Beller wrote:\n\n> In f9ee2fcdfa (grep: recurse in-process using 'struct repository',\n> 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep\n> to simplify the submodule handling.\n> \n> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules\n> file, 2017-08-03) this is no longer necessary, but that commit did not\n> cleanup the whole tree, but just show cased the new way how to deal with\n> submodules in ls-files.\n> \n> Cleanup the only remaining caller to repo_read_gitmodules outside of\n> submodule.c\n> \n> Signed-off-by: Stefan Beller \n\nNot sure if I am entitled to formally ack it, but:\n\nAcked-by: Antonio Ospite \n\n> ---\n> \n> Antonio Ospite writes:\n> > BTW, with Stefan Beller we also identified some unneeded code which\n> > could have been removed to alleviate the issue, but that would not have\n> > solved it completely; so, I am not removing the unnecessary call to\n> > repo_read_gitmodules() builtin/grep.c in this series, possibly this can\n> > become a stand-alone change.\n> \n> Here is the stand-alone change.\n>\n\nThank you for sending it.\n\nCiao,\n Antonio\n\n-- \nAntonio Ospite\nhttps://ao2.it\nhttps://twitter.com/ao2it\n\nA: Because it messes up the order in which people normally read text.\n See http://en.wikipedia.org/wiki/Posting_style\nQ: Why is top-posting such a bad thing?\n"},{"id":"359781","messageId":"xmqqlg7avbtt.fsf@gitster-ct.c.googlers.com","threadId":"49496","inReplyTo":"20181005224557.31420-1-sbeller@google.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-07T00:29:18Z","receivedAt":"2018-10-07T00:29:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> In f9ee2fcdfa (grep: recurse in-process using 'struct repository',\n> 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep\n> to simplify the submodule handling.\n>\n> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules\n> file, 2017-08-03) this is no longer necessary, but that commit did not\n> cleanup the whole tree, but just show cased the new way how to deal with\n> submodules in ls-files.\n>\n> Cleanup the only remaining caller to repo_read_gitmodules outside of\n> submodule.c\n\nWell, submodule-config.c has its implementation and another caller,\nwhich technically is outside submodule.c ;-) repo_read_gitmodules\nhas two more callers in unpack-trees.c these days, so perhaps we can\ndo without this last paragraph.\n\n"},{"id":"359782","messageId":"xmqqh8hyvbnd.fsf@gitster-ct.c.googlers.com","threadId":"49496","inReplyTo":"20181005224557.31420-1-sbeller@google.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-10-07T00:33:10Z","receivedAt":"2018-10-07T00:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules\n> file, 2017-08-03) this is no longer necessary, but that commit did not\n> cleanup the whole tree, but just show cased the new way how to deal with\n> submodules in ls-files.\n\nThe log message of the above one singles out \"grep\" as a special\ncase and explalins why it did not touch, by the way. You probably\nneed to explain the reason why \"this is no longer necessary\" a bit\nbetter than the above---as it stands, it is \"ff6f1f564c4 said it\nstill is necessary, I say it is not\".\n"},{"id":"359885","messageId":"CAGZ79kaxDch0qGWV612it+kLFhnfVhh+f_97kBf=KJFL-CJJOw@mail.gmail.com","threadId":"49496","inReplyTo":"xmqqh8hyvbnd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] builtin/grep.c: remote superflous submodule code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-09T00:14:00Z","receivedAt":"2018-10-09T00:14:15Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Well, submodule-config.c has its implementation and another caller,\n> which technically is outside submodule.c ;-)\n\ni.e. there is a typo in my commit message.\nI meant to say submodule-config.c\n\n> repo_read_gitmodules\n> has two more callers in unpack-trees.c these days, so perhaps we can\n> do without this last paragraph.\n\nGah, looking at that code, did we have any reason to rush that series?\nc.f. https://public-inbox.org/git/20170811171811.GC1472@book.hvoigt.net/\n\n\nOn Sat, Oct 6, 2018 at 5:33 PM Junio C Hamano wrote:\n>\n> Stefan Beller writes:\n>\n> > After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules\n> > file, 2017-08-03) this is no longer necessary, but that commit did not\n> > cleanup the whole tree, but just show cased the new way how to deal with\n> > submodules in ls-files.\n>\n> The log message of the above one singles out \"grep\" as a special\n> case and explalins why it did not touch, by the way. You probably\n> need to explain the reason why \"this is no longer necessary\" a bit\n> better than the above---as it stands, it is \"ff6f1f564c4 said it\n> still is necessary, I say it is not\".\n\nThat is true.\n\nFor grep, the reason seems to be, that we check is_submodule_active\nbased off the index, i.e. using\n module = submodule_from_path(repo, &null_oid, path);\nas the deciding factor, which falls in line with lazyloading.\n\nHowever the use of the specialized gitmodules_config_oid\nin grep is also guarded by the same commit ff6f1f564c4.\n\nGoing back to the use case of unpack-trees.c,\nI think that we need to keep it there as alternatives\nseem to be more complicated.\n\nSo I guess I'll just resend with a better commit message.\n\nThanks,\nStefan\n"}]}