threads / patch / 49496

patchbuiltin/grep.c: remote superflous submodule code

Subject: [PATCH] builtin/grep.c: remote superflous submodule code

## tl;dr

5 messages between Oct 5, 2018 and Oct 9, 2018. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Stefan Beller· Oct 5, 2018, 22:45 UTC · lore

In f9ee2fcdfa (grep: recurse in-process using 'struct repository', 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep to simplify the submodule handling.

After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules file, 2017-08-03) this is no longer necessary, but that commit did not cleanup the whole tree, but just show cased the new way how to deal with submodules in ls-files.

Cleanup the only remaining caller to repo_read_gitmodules outside of submodule.c

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Antonio Ospite writes:

Show 5 quoted lines

> BTW, with Stefan Beller we also identified some unneeded code which
> could have been removed to alleviate the issue, but that would not have
> solved it completely; so, I am not removing the unnecessary call to
> repo_read_gitmodules() builtin/grep.c in this series, possibly this can
> become a stand-alone change.

Here is the stand-alone change.

The patch [1] contains the lines as deleted below in the context lines but they would not conflict as there is one empty line between the changes in this patch in [1].

[1] https://public-inbox.org/git/20181005130601.15879-10-ao2@ao2.it/

 builtin/grep.c | 2 --
 1 file changed, 2 deletions(-)

Show changes to builtin/grep.c +0 −2

diff --git a/builtin/grep.c b/builtin/grep.c
index 601f801158..a6272b9c2f 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -427,8 +427,6 @@ static int grep_submodule(struct grep_opt *opt, struct repository *superproject,
 	if (repo_submodule_init(&submodule, superproject, path))
 		return 0;
 
-	repo_read_gitmodules(&submodule);
-
 	/*
 	 * NEEDSWORK: This adds the submodule's object directory to the list of
 	 * alternates for the single in-memory object store.  This has some bad

-- 
2.19.0

Antonio Ospite· Oct 6, 2018, 08:59 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/grep.c: remote superflous submodule code

On Fri, 5 Oct 2018 15:45:57 -0700 Stefan Beller <sbeller@google.com> wrote:

Show 13 quoted lines

> In f9ee2fcdfa (grep: recurse in-process using 'struct repository',
> 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep
> to simplify the submodule handling.
> 
> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules
> file, 2017-08-03) this is no longer necessary, but that commit did not
> cleanup the whole tree, but just show cased the new way how to deal with
> submodules in ls-files.
> 
> Cleanup the only remaining caller to repo_read_gitmodules outside of
> submodule.c
> 
> Signed-off-by: Stefan Beller <sbeller@google.com>

Not sure if I am entitled to formally ack it, but:

Acked-by: Antonio Ospite <ao2@ao2.it>

Show 11 quoted lines

> ---
> 
> Antonio Ospite writes:
> > BTW, with Stefan Beller we also identified some unneeded code which
> > could have been removed to alleviate the issue, but that would not have
> > solved it completely; so, I am not removing the unnecessary call to
> > repo_read_gitmodules() builtin/grep.c in this series, possibly this can
> > become a stand-alone change.
> 
> Here is the stand-alone change.
>

Thank you for sending it.

Ciao,
   Antonio

-- 
Antonio Ospite
https://ao2.it
https://twitter.com/ao2it

A: Because it messes up the order in which people normally read text.
   See http://en.wikipedia.org/wiki/Posting_style
Q: Why is top-posting such a bad thing?

Junio C Hamano· Oct 7, 2018, 00:29 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/grep.c: remote superflous submodule code

Stefan Beller <sbeller@google.com> writes:

Show 11 quoted lines

> In f9ee2fcdfa (grep: recurse in-process using 'struct repository',
> 2017-08-02), we introduced a call to repo_read_gitmodules in builtin/grep
> to simplify the submodule handling.
>
> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules
> file, 2017-08-03) this is no longer necessary, but that commit did not
> cleanup the whole tree, but just show cased the new way how to deal with
> submodules in ls-files.
>
> Cleanup the only remaining caller to repo_read_gitmodules outside of
> submodule.c

Well, submodule-config.c has its implementation and another caller, which technically is outside submodule.c ;-) repo_read_gitmodules has two more callers in unpack-trees.c these days, so perhaps we can do without this last paragraph.

Junio C Hamano· Oct 7, 2018, 00:33 UTC · re: Stefan Beller · lore

Re: [PATCH] builtin/grep.c: remote superflous submodule code

Stefan Beller <sbeller@google.com> writes:

> After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules
> file, 2017-08-03) this is no longer necessary, but that commit did not
> cleanup the whole tree, but just show cased the new way how to deal with
> submodules in ls-files.

The log message of the above one singles out "grep" as a special case and explalins why it did not touch, by the way. You probably need to explain the reason why "this is no longer necessary" a bit better than the above---as it stands, it is "ff6f1f564c4 said it still is necessary, I say it is not".

Stefan Beller· Oct 9, 2018, 00:14 UTC · re: Junio C Hamano · lore

Re: [PATCH] builtin/grep.c: remote superflous submodule code

> Well, submodule-config.c has its implementation and another caller,
> which technically is outside submodule.c ;-)

i.e. there is a typo in my commit message. I meant to say submodule-config.c

>  repo_read_gitmodules
> has two more callers in unpack-trees.c these days, so perhaps we can
> do without this last paragraph.

Gah, looking at that code, did we have any reason to rush that series? c.f. https://public-inbox.org/git/20170811171811.GC1472@book.hvoigt.net/

On Sat, Oct 6, 2018 at 5:33 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 13 quoted lines

>
> Stefan Beller <sbeller@google.com> writes:
>
> > After ff6f1f564c4 (submodule-config: lazy-load a repository's .gitmodules
> > file, 2017-08-03) this is no longer necessary, but that commit did not
> > cleanup the whole tree, but just show cased the new way how to deal with
> > submodules in ls-files.
>
> The log message of the above one singles out "grep" as a special
> case and explalins why it did not touch, by the way.  You probably
> need to explain the reason why "this is no longer necessary" a bit
> better than the above---as it stands, it is "ff6f1f564c4 said it
> still is necessary, I say it is not".

That is true.

For grep, the reason seems to be, that we check is_submodule_active
based off the index, i.e. using
   module = submodule_from_path(repo, &null_oid, path);
as the deciding factor, which falls in line with lazyloading.

However the use of the specialized gitmodules_config_oid in grep is also guarded by the same commit ff6f1f564c4.

Going back to the use case of unpack-trees.c, I think that we need to keep it there as alternatives seem to be more complicated.

So I guess I'll just resend with a better commit message.

Thanks, Stefan