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

> On Wed, Nov 09 2022, Glen Choo wrote:
>
>> Thanks for the series! I haven't fully figured out where I stand on
>> this, but I can share some initial thoughts and comparisons to my RFC.
>>
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>>
>>> An RFC alternative to Glen's [1], and what I *thought* he might be be
>>> going for in the earlier discussion[2].
>>>
>>> The difference is that in Glen's there's no "git --super-prefix", but
>>> rather each set of commands still using it ("submodule--helper",
>>> "read-tree" etc.) geit their own command-level option.
>>
>> Yes, and a secondary intent was to give exact definitions and a shared
>> implementation to the command-level options instead of having each new
>> command figure out what to do every time a similar use case pops up.
>
> ... (more below)...
>
>>>
>>> But it still works substantially the same, in that we're juggling a
>>> global variable that we set, and read out later somewhere down the
>>> stack.
>>
>> Yes, intentionally so. I was under the assumption that the various
>> prefixes would still be used, and that adding them to commands will be a
>> necessary evil, so it was better to have them share a single
>> implementation.
>
> I'm not sure I understand what you mean here exactly, but as an example
> that might clarify things: In my "foreach" patch:
> https://lore.kernel.org/git/RFC-patch-3.8-4858e2ad0ed-20221109T192315Z-avarab@gmail.com/
>
> I'm making use of "OPT__SUPER_PREFIX()" to share the things that need to
> be shared, and after that we ferry the "super_prefix" from
> module_foreach() down through "struct foreach_cb" to
> "runcommand_in_submodule_cb()", and that's all the places we need it.
>
> With how OPT_SUBCOMMAND() works we'd need a file-level static variable
> (or similar) if we took the "--super-prefix" in the
> cmd_submodule__helper() instead, which we could do, and it would have
> pretty much the same effect.
>
> But I don't really see the advantage in any one individiual case. Isn't
> it just trading that one repeated "OPT__SUPER_PREFIX()" line for more
> clarity about who needs what?

> We can be sure it's just clone/update/foreach/status/sync/absorbgitdirs,
> because if the others are invoked with the --super-prefix they'll die().
>
> Which aside from the specific approaches in either of our topics is, I
> think, a really nice thing to have. I.e. it makes reasoning about the
> code easier if we can confidently say "this is a feature with global
> effect, but it only has an impact on this subset of stuff".
>
> Which is an aspect of your proposed RFC that I don't like in
> isolation. I.e. if/when we're going to do the eventual refactoring that
> I've done in my RFC we'd need to reason that we need to change "foreach"
> and not "create-branch" or whatever, which the current whitelisting
> really helps us with.

>
>> (As an aside, when you remove git-submodule.sh, I wonder if we should
>> split up submodule--helper along this dual-use line? e.g. the ones that
>> are entrypoints could be moved to "builtin/submodule.c", and the
>> implementation details can stay in "builtin/submodule--helper.c". Or
>> maybe you're already one step ahead of me here :))
>
> Yes, I'm ahead of you there.
>
> The ones that are purely internal, such as "push-check" etc. are still
> going to be invoked as "submodule--helper push-check".
>
> Before my RFC topic here I was planning on having both a "foreach" and
> "submodule--helper-foreach", as seen in:
> https://lore.kernel.org/git/221104.86wn8bzeus.gmgdl@evledraar.gmail.com/
>
> That was because I:
>
>  1. Didn't want to make "foreach"'s "--super-prefix" interface "public",
>     which it isn't now, as it's a purely internal thing we're trying to
>     eventually get rid of.
>  2. Didn't want to list all of "submodule--helper" as accepting it, as
>     some of it doesn't need it (e.g. "push-check").
>
> So having a "git submodule foreach" *not* accept it and it then
> recursing to an internal "git submodule--helper-foreach" which did
> accept it seemed like the least sucky thing to do.
>
> But if I rebase those patches on my RFC topic here we can invoke both as
> just "git submodule foreach".
>
> Pedantically, that's also exposing a "private" interface to the
> public. But:
>
>  * I'm listing it as OPT_HIDDEN, so we're not showing it outside fo
>    --help-all.

>  * More importantly, you need to provide that specific flag, it's not
>    accepting it "at a distance" via an env variable, which was the main
>    thing I was being paranod about.
>
>    Or rather. Even if *I knew* that "read-tree" or whatever wasn't
>    invoking a hook, which then invoked "submodule foreach" down the
>    line, and that "foreach" was slurping up the env var, I wanted to
>    leave it in a state where the next maintainer of this code didn't
>    have to worry about that.

>> [1] https://lore.kernel.org/git/20221109004708.97668-2-chooglen@google.com/
>>
>>>  * There's no passing of the "super_prefix" as a global, instead we
>>>    pass it all the way along until we recurse to ourselves. For
>>>    "submodule--helper" this is quite straightforward.
>>>
>>>  * Then in 8/8 we're left with just "read-tree" needing the remaining
>>>    "--super-prefix", and we likewise don't pass it as a global, but
>>>    instead add it to the "struct unpack_trees_options", which will
>>>    pass it all the way down into unpack-trees.c and entry.c, until
>>>    we're going to recursively invoke another "read-tree".
>>
>> I worry a little about two "necessary evils":
>>
>> - (As stated earlier) we may have to add "--super-prefix" or similar to
>>   more commands
>> - We may need to read "--super-prefix" from many parts of the code,
>>   since many parts might print paths.
>>
>> Having globals makes both of these cases easier, and is quite a bit
>> closer to the original implementation of "--super-prefix" (so your
>> characterization of only getting to a halfway point is accurate). This
>> was mostly to stave off opposition that it would tedious to add new
>> per-command "--super-prefix"-es, but if nobody else cares, maybe it's ok
>> to get of the globals.
>>
>> If we do want to pass a context object around, we probably have to be
>> more principled about it (e.g. in 8/8 I notice that checkout_stage()
>> doesn't receive the context object and we resort to passing NULL
>> instead), but we'd want that anyway if we want Git to move towards being
>> more library-like.
>
> Quick aside: Yes, that unlink_entry(ce) in builtin/checkout.c is now
> unlink_entry(ce, NULL).
>
> The API had only 3 users, and two potentially cared about the
> super_prefix. So I just amended the existing function, we could also add
> a unlink_entry_sp() or whatever.

>
>> It's quite likely that if any new "--super-prefix"-es are added, they
>> would be added by a Googler (even the original ones were ;)), so I can
>> probably go through the roadmap and figure out how costly these extra
>> "--super-prefix"-es might be.
>
> I don't really see that happening, but let me just grant that point for
> the sake of argument: 
>
> Next month we're going to add a new "--super-prefix"-like use, and we're
> going to want some combinatino of a global, setenv()/getenv() etc. back,
> because it's the "sparse" code or whatever, and it'll be daunting to
> pass the state through 30 levels of callstack, we'll want to bypass it.
>
> I still don't see how it follows that we'd want to keep any of structure
> for this, which I think my series shows none of the current code needs.
>
> We can easily add such functionality back, adding a global variable
> isn't hard, and I think if anything such a future future would benefit
> from having to explain why it needs global state to do this sort of
> thing, when these other commands and sub-commands don't.