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

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

[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

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

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.

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(-)

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

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

[Antonio Ospite]

On Fri,  5 Oct 2018 15:45:57 -0700
Stefan Beller <sbeller@google.com> 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.
> 
> 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>

> ---
> 
> 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?

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

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> 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.

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

[Junio C Hamano]

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".

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

[Stefan Beller]

> 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:
>
> 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

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

[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.

It claimed that grep would still need some explicit handling, but that is
not the call to repo_read_gitmodules (applying this patch on top of
ff6f1f564c4 still keep the test suite happy, specifically
t7814-grep-recurse-submodules, which contains a test
"grep history with moved submoules")

The special handling is the call to gitmodules_config_oid which was added
already in 74ed43711f (grep: enable recurse-submodules to work on
<tree> objects, 2016-12-16), but then was still named
gitmodules_config_sha1.

Signed-off-by: Stefan Beller <sbeller@google.com>
Acked-by: Antonio Ospite <ao2@ao2.it>
---

This is a resend of origin/sb/grep-submodule-cleanup,
and I think picking ff6f1f564c4 as the base for the series would
also be appropriate.

Stefan


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

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

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

[Jonathan Tan]

> It claimed that grep would still need some explicit handling, but that is
> not the call to repo_read_gitmodules (applying this patch on top of
> ff6f1f564c4 still keep the test suite happy, specifically
> t7814-grep-recurse-submodules, which contains a test
> "grep history with moved submoules")

Firstly, spelling of "remove" and "superfluous" in the commit title.

I don't think the "grep history with moved submodules" test exercises
much. That test only tests the superproject > submodule case, but we
need a superproject > submodule > sub-submodule case, because what is
being removed is a call to repo_read_gitmodules() on a repository
("struct repository submodule") that has a superproject ("struct
repository *superproject"). In other words, we need a submodule that has
its own gitmodules.

Alternatively, it would be fine if someone could point out where the
.gitmodules file is lazily loaded when grep_submodule() is invoked. I
couldn't find it, although I wasn't looking very hard. I did look at the
invocation of repo_submodule_init() (right before the removed lines),
which indeed calls repo_read_gitmodules() indirectly through
submodule_from_path(), but that is called on the superproject, whereas
what is being removed is a call on the submodule.

> The special handling is the call to gitmodules_config_oid which was added
> already in 74ed43711f (grep: enable recurse-submodules to work on
> <tree> objects, 2016-12-16), but then was still named
> gitmodules_config_sha1.

If you're stating that gitmodules_config_oid() is where the .gitmodules
file is lazily loaded, it doesn't seem to be that way, because that
function works only on the_repository (at least on 'master' and 'next').

> This is a resend of origin/sb/grep-submodule-cleanup,
> and I think picking ff6f1f564c4 as the base for the series would
> also be appropriate.

Any particular reason why you suggest that commit (which is more than a
year old)? It seems that basing this on 'master' is fine.

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

[Stefan Beller]

On Tue, Oct 9, 2018 at 5:10 PM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > It claimed that grep would still need some explicit handling, but that is
> > not the call to repo_read_gitmodules (applying this patch on top of
> > ff6f1f564c4 still keep the test suite happy, specifically
> > t7814-grep-recurse-submodules, which contains a test
> > "grep history with moved submoules")
>
> Firstly, spelling of "remove" and "superfluous" in the commit title.
>
> I don't think the "grep history with moved submodules" test exercises
> much. That test only tests the superproject > submodule case, but we
> need a superproject > submodule > sub-submodule case, because what is
> being removed is a call to repo_read_gitmodules() on a repository
> ("struct repository submodule") that has a superproject ("struct
> repository *superproject"). In other words, we need a submodule that has
> its own gitmodules.

Right; we do have a test 'grep and nested submodules', which still passes.
I added another test, that would grep through nested submodules in
the history (not checked out), but that would not work on nested submodules
with or without this patch applied. (As the nested submodule is not checked
out, is_submodule_active(repo, path) would return false and we'd not dive
into the nested submodule.

I looked into ao/submodule-wo-gitmodules-checked-out, as that touches
this area of code as well and promises to allow working with submodules
when .gitmodules is not checked out, it doesn't help this use case, either.

That is (as Antonio diagnosed), due to get_oid not working with a repository
handle, yet.

> > The special handling is the call to gitmodules_config_oid which was added
> > already in 74ed43711f (grep: enable recurse-submodules to work on
> > <tree> objects, 2016-12-16), but then was still named
> > gitmodules_config_sha1.
>
> If you're stating that gitmodules_config_oid() is where the .gitmodules
> file is lazily loaded, it doesn't seem to be that way, because that
> function works only on the_repository (at least on 'master' and 'next').

yes, that is why nested submodules do not work currently when they
are not in the working tree.

>
> > This is a resend of origin/sb/grep-submodule-cleanup,
> > and I think picking ff6f1f564c4 as the base for the series would
> > also be appropriate.
>
> Any particular reason why you suggest that commit (which is more than a
> year old)? It seems that basing this on 'master' is fine.

After more analysis, I think we'd want to wait for Antonios series to land
and then build on top of that, while also getting get_oid converted.

Regarding this patch, let's retract it for now and revisit it once we have
more submodule infrastructure working.

Thanks,
Stefan