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

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

On Thu, Nov 04 2021, Junio C Hamano wrote:

quoted

Emily Shaffer [off-list ref] writes:

quoted

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.

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