Junio C Hamano <gitster@xxxxxxxxx> 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.
