Re: [PATCH v2 5/8] submodule: move core cmd_update() logic to C

[Junio C Hamano <gitster@pobox.com>]

Atharva Raykar <raykar.ath@gmail.com> writes:

> This patch completes the conversion past the flag parsing of
> `submodule update` by introducing a helper subcommand called
> `submodule--helper update`. The behaviour of `submodule update` should
> remain the same after this patch.
>
> We add more fields to the `struct update_data` that are required by
> `struct submodule_update_clone` to be able to perform a clone, when that
> is needed to be done.
>
> Recursing on a submodule is done by calling a subprocess that launches
> `submodule--helper update`, with a modified `--recursive-prefix` and
> `--prefix` parameter.
>
> We also introduce `update_submodules()` and `update_submodule()` which
> are quite similar to `update_clone_submodules()` and
> `update_clone_submodule()`, and will supersede them.
>
> When the `--init` flag is passed to the subcommand, we do not spawn a
> new subprocess and call `submodule--helper init` on the submodule paths,
> because the Git machinery is not able to pick up the configuration
> changes introduced by that init call[1]. So we instead run the
> `init_submodule_cb()` callback over each submodule directly.
>
> This introduces another problem, because there is no mechanism to pass
> the superproject path prefix (ie, `--super-prefix`) without starting a
> new git process. This field is required for obtaining the display path
> for that is used by the command's output messages. So let's add a field
> into the `init_cb` struct that lets us pass this information to
> `init_submodule()`, which will now also take an explicit 'superprefix'
> argument.
>
> While we are at it, we also remove the fetch_in_submodule() shell
> function since it is no longer used anywhere.
>
> [1] https://lore.kernel.org/git/CAP8UFD0NCQ5w_3GtT_xHr35i7h8BuLX4UcHNY6VHPGREmDVObA@mail.gmail.com/
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <periperidip@gmail.com>
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> ---
>  builtin/submodule--helper.c | 505 ++++++++++++++++++++++++++++++------
>  git-submodule.sh            | 145 +----------
>  2 files changed, 433 insertions(+), 217 deletions(-)

It seems that this step does too many things in one step and makes
it harder than necessary for readers to understand what is going on.

For example, as far as I can see, the change to optionally allow
passing superprefix to init_submodule() can be made without anything
else in this step (i.e. "we allow superprefix from the parameter to
override what get_super_prefix() would give us, but at this step,
everybody passes NULL and the behaviour is unchanged" would be one
such step), later to be followed by a change that involves passing a
non-NULL value there, and it would become a lot easier to see who
passes a non-NULL super prefix, what value is passed and for what
purpose, with such an organization.

A 650+ lines single patch like this buries the details and forces
the readers to disect them out.  The change that teaches
run_update_command() to instead use capture_command() and gives the
error message to its callers may be another example of what may be
better done as an independent step before tying the whole thing
together.

It is larger than what I comfortably can spend my time on while
tending patches from other topics, so I'll skip reviewing this step
for now.

Thanks.