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

On Wed, Nov 09 2022, Glen Choo wrote:

... (more below)...

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?

I agree with your:

	"[...]"This is the only way the prefix is used in "git
	submodule--helper", and all of its subcommands already conform
	to this behavior (even the ones that weren't marked
	SUPPORT_SUPER_PREFIX) because they all use
	get_submodule_displaypath() when printing paths to
	submodules.[...]"

In that it's clear that e.g. if "add" were added to the list of sub-commands that used "git --super-prefix" *and* we started calling that recursively with the same method that "get_submodule_displaypath()" makes it easier to construct such paths (which is also the case after my series, just with the *_sp() variant).

But I don't see how that extends to an argument that we should be blurring the lines about what does and doesn't need it *now*, which as I pointed out in my "now we'll accept it" is what that commit is doing: https://lore.kernel.org/git/221109.867d04rfnt.gmgdl@evledraar.gmail.com/

But they don't need it, and some of them will never need it. E.g. I don't see how it would make sense for "set-url" and "get-url" ever to get a "--recursive" option, those are inherently non-recursive (if they're going to make any sense, you *could* set all your submodule URLs to the same thing, but what's the use in that?).

But also re my: https://lore.kernel.org/git/221110.86cz9vpvqk.gmgdl@evledraar.gmail.com/; I think that *if* we add new recursion we're just as likely to not use any "--submodule-prefix"-like functionality, but to instead just walk the tree, constructing repo objects along the way. As my 8/8 summarizes that's what we're doing in "grep", "ls-files" etc.

Still, if it was easier etc. to just provide this for the entire command, even if all subcommands wouldn't need it I wouldn't mind it. But as noted above it's trivial to add it on demand with "OPT__SUPER_PREFIX()", you don't need to reason about globals etc. So I just don't see the benefit...

I you don't need to take much of a look, because on "master" they don't have a "SUPPORT_SUPER_PREFIX" next to their listing at the bottom of submodule--helper.c, ditto for the "if" chain I have in "next". I.e. none of the ones listed in the "struct option options" as OPT_SUBCOMMAND() use "git --super-prefix", except if they're listed in the "if" chain below.

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.

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.

Right, and just to add to that: This is the super_prefix==NULL case in get_submodule_displaypath() on "master" (i.e. aside from either of our topics here).

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.

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.