Re: [RFC PATCH 0/8] Get rid of "git --super-prefix"

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Nov 14 2022, Glen Choo wrote:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>> As noted in the CL above I included this because I see you're keen to
>> include it, but I'm personally a bit "meh" on it. I.e. it's just
>> renaming an existing unrelated option, although being able to use
>> OPT__SUPER_PREFIX() makes it slightly nicer.
>>
>> As post-cleanups go I think removing the "submodule_prefix" from the
>> "struct repository" would make more sense, and maybe it's worth peeling
>> off the 10/10 to include in such a post-cleanup series? I.e. the below
>> on top of all of this works, and reduces allocations and cargo-culting
>> around the submodule API.
>
> As a first impression I'm not particularly keen on this, since it makes
> perfect sense to me to have a repo->submodule_prefix, especially when
> recursing into N-level deep submodules...

This is mainly a quick experiment I drafted up, with that context...

>> -- >8 --
>> Subject: [PATCH] repo & submodule API: stop carrying "repo->submodule_prefix"
>>
>> As this change shows the "submodule_prefix" field to "struct
>> repository" added in 96dc883b3cd (repository: enable initialization of
>> submodules, 2017-06-22) was only used by "ls-files" and "grep". Let's
>> have those two carry forward the "super_prefix" instead.
>>
>> Having every user of "struct repository" needing to worry about this
>> created a mismatch in the API where e.g. "grep" would re-compute a
>> "name_base_len" which we knew before. Now we use a "struct strbuf" in
>> the "struct grep_opt" there instead, so we'll know the length
>> already. This simplifies "grep_cache()" and "grep_tree()".
>>
>> We're also deleting cargo-culted code that the previous API foisted
>> upon us. In 605f0ec1350 (submodule: use submodule repos for object
>> lookup, 2018-12-14) the "Mark it as a submodule" code was added to
>> "open_submodule()", but the resulting xstrdup()'d "submodule_prefix"
>> was never used by anything.
>
> (As an aside, I think open_submodule() should have been replaced by
> repo_submodule_init().)
>
> In which case, yes it isn't used by anything in that code path, but
> being meticulous about maintaining .super_prefix means that other
> callers could use it if they wanted to, which might be crucial once we
> start plumb "struct repository" deeper and deeper and...

...I just don't see the point of maintaining an API with this sort of
reach for just two callers, when it's a bad fit for what those two
callers need.

>>
>> Still, removing this field might not be a good move, as the
>> "super_prefix" might be a common enough case in the future, e.g. when
>> eventually migrating the "submodule--helper" users[1] to run
>> in-process.
>>
>> As the "grep" example demonstrates I don't think that's the
>> case. There instead of xstrdup()-ing all the way down we're now
>> carrying a single "super_prefix" in the form of a "struct strbuf". As
>> we recurse we then append to it, and strbuf_setlen() it back when we
>> we recurse out of that submodule. This is similar to how e.g. the
>> "read_tree_at()" API works.
>
> This technique might no longer be so appealing. We _could_ pass both
> "struct repository" and "super_prefix", but that seems odd given that
> the super prefix is tied to the repository.

FWIW I don't even think a "super_prefix" being tied to a repository is a
useful abstraction level in general.

E.g. I've got some local hacky patches to make "git ls-tree" able to
recurse down submodules, and for that code you usually just want to
print the prefix to the current *dir*, and having to print the prefix to
the current module, then the current dir-within-module, then the file
just makes things needlessly complex.

> But that's just a first impression anyway. I don't mind taking another
> look if this gets a standalone review.

*nod*