On Mon, Nov 14 2022, Glen Choo wrote: > Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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*
