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

[Glen Choo <chooglen@google.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:

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

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