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

Junio C Hamano [off-list ref] 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.