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

From: Jonathan Tan <jonathantanmy@google.com>
Date: Oct 10, 2018, 00:10 UTC
Message-ID: <20181010001037.74709-1-jonathantanmy@google.com>
In-Reply-To: <20181009183549.195316-1-sbeller@google.com>

Show 5 quoted lines

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