builtin/grep.c: remote superflous submodule code

PATCH In review Stefan Beller 3 participants 5 messages Oct 5, 2018 – Oct 9, 2018

Share & tools

Stefan Beller

author

2018-10-05 22:46 UTC #0 lore raw

PATCH 1/1 builtin/grep.c: remote superflous submodule code

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 <[email protected]>

---

Quoted 5 lines · Antonio Ospite

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/[email protected]/

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

builtin/grep.c +0 -2

index 601f801158..a6272b9c2f 100644
@@ -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

Signature · 1 line

2.19.0

Antonio Ospite

Acked-by on v1 1/1

2018-10-06 09:03 UTC #1 lore raw

Quoted 9 lines · On Fri, 5 Oct 2018 15:45:57 -0700 Stefan Beller <[email protected]> wrote:

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.

Quoted 4 lines

Cleanup the only remaining caller to repo_read_gitmodules outside of submodule.c
Signed-off-by Stefan Beller <[email protected]>

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

Acked-by Antonio Ospite <[email protected]>

Quoted 11 lines

---
Quoted 5 lines · Antonio Ospite
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

Signature · 7 lines

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

2018-10-07 00:29 UTC #2 lore raw

In reply to Stefan Beller

Quoted 11 lines · Stefan Beller

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

2018-10-07 00:33 UTC #3 lore raw

In reply to Stefan Beller

Quoted 4 lines · Stefan Beller

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

author

2018-10-09 00:14 UTC #4 lore raw

Quoted 2 lines

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

Quoted 3 lines

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/[email protected]/

Quoted 8 lines · Junio C Hamano

Quoted 4 lines · Stefan Beller
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.

Quoted 5 lines

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