Re: [PATCH v5 4/4] submodule: record superproject gitdir during 'update'

From: Emily Shaffer <emilyshaffer@google.com>
Date: Nov 8, 2021, 23:21 UTC
Message-ID: <YYmw8vEyFnQpe58z@google.com>
In-Reply-To: <211105.861r3vhtot.gmgdl@evledraar.gmail.com>

On Fri, Nov 05, 2021 at 09:43:56AM +0100, Ævar Arnfjörð Bjarmason wrote:

Show 29 quoted lines

> 
> 
> On Thu, Nov 04 2021, Junio C Hamano wrote:
> 
> > Emily Shaffer <emilyshaffer@google.com> writes:
> >
> >> A recorded hint path to the superproject's gitdir might be added during
> >> 'git submodule add', but in some cases - like submodules which were
> >> created before 'git submodule add' learned to record that info - it might
> >> be useful to update the hint. Let's do it during 'git submodule
> >> update', when we already have a handle to the superproject while calling
> >> operations on the submodules.
> >
> > We are hearing repeated mention of "cache" and "hint".  Do we ever
> > invalidate it, or if we have such a record, do we blindly trust it
> > and use it without verifying if it is still fresh?
> >
> > Also, this step and the previous step both say we record gitdir on
> > their title, but we instead record common dir.  Whichever is the
> > right choice to record, let's be consistent.
> 
> I had similar (AFAICT still unaddressed) feedback on the v2[1]. I'd lost
> track of this series, and see one reason is that the In-Reply-Chain was
> broken between v3..v4.
> 
> I.e. it seems to me that this whole thing started as a way to avoid
> shellscript overhead by calling git-rev-parse from git-submodule.sh, but
> now that the relevant bits are moved to C we could just call some
> slightly adjusted code in setup.c.

No, that is not the case. It is the case that `git -C .. rev-parse --git-dir` is *very* expensive in the case where `../` is not, in fact, a gitdir; when I attempted another series which relied on finding the parent superproject's gitdir in this way, our testsuite took something like 5x longer to run than before. In other words, the expensive part is not the shelling out overhead - the expensive part is searching up the entire filesystem directory structure in the worst-case ("we are not a submodule") scenario. This is still needed, even with 'git-submodule.sh' moving to C.

Show 12 quoted lines

> 
> Maybe I'm entirely wrong, but I think if I am that this series has a
> commit message gap between the "hint" and "cache" and this really *does*
> need to be written at clone/init/update time in some way that I've
> missed.
> 
> Otherwise I still don't really get it, sorry. I.e. maybe the relevant
> code in setup.c really does need caching, or maybe it doesn't anymore,
> and this cache-via-config is a hold-over from when figuring out the
> parent repo implied needing to shell out somewhere for every operation.
> 
> 1. https://lore.kernel.org/git/87r1cnfkqx.fsf@evledraar.gmail.com/