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

[Atharva Raykar <raykar.ath@gmail.com>]

Junio C Hamano <gitster@pobox.com> writes:
>
> 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.

Understood. Apologies for the lack of updates on this topic, I will work
on organising this patch (and this series) better. I've taken a note of
the conflicts with other topics as well.

I don't mind the other topics being prioritized over this one. At the
moment I am not too confident about being able to quickly iterate over
this series. I'll try my best anyway.