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

From: Glen Choo <chooglen@google.com>
Date: Nov 10, 2022, 00:45 UTC
Message-ID: <kl6ltu373ae5.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <RFC-cover-0.8-00000000000-20221109T192315Z-avarab@gmail.com>

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:

Show 6 quoted lines

> 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.

>
> 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.

> Whereas here there's no renaming of the option, but:
>
>  * For "submodule--helper" only the sub-commands that need it take the
>    option, it's not an option to "submodule--helper" itself.

In writing [1], I ended up convincing myself that it isn't just that all of "submodule--helper" _does_ support "--super-prefix", but that all of it _should_ support "--super-prefix". I'll have to take another look.

At the very least, the subcommands that are just entrypoints for git-submodule.sh should support it, since they all need to print submodule paths in a semi-consistent way. I still think it would be nice for these to take a top level flag.

There are other subcommands that are implementation details of other commands that need to run in a submodule because of assumptions the_repository, e.g. create-branch, push-check. Maybe these don't need "--super-prefix", I'll take another look.

I'm not sure if there are others.

(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 :))

As you noted (somewhere) in the series, only commands called recursively need "--super-prefix", because otherwise "submodule--helper" is called from the root of the superproject and the 'prefix' is already well-known. I didn't make this argument because it was hard to word, so I'm glad you mentioned it.

[1] https://lore.kernel.org/git/20221109004708.97668-2-chooglen@google.com/

Show 9 quoted lines

>  * 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.

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.