I mentioned (out-of-mailing-list) that I was still looking at this, but
that the big sh -> c conversion is quite challenging for me to parse
personally. I'm still looking at it, but it will take some time..
I'm of the opinion that this patch would be a lot easier to review if it
were broken up into more patches, but it has always looked like this
[1]. The only real difference from [1] to this version is that this also
removes all of the dead code, which doesn't really hinder reviewability.
I don't think you have to go through the effort of splitting it up -
after all you were able to review [1], so given enough time I should
be able to read through this patch too :) That said, I wish that we
already had a split up patch for at least 2 other reasons:
- Junio pointed out that this conflicts with
es/superproject-aware-submodules [2]. I'm not sure which should be
based on which. If this does end up being based on
es/superproject-aware-submodules, it would probably be easier to
rebase as a series of smaller patches. Atharva noted that the
conflicts are mild though, so maybe it's not so bad.
- Besides making sure that the sh -> c is faithful, a thorough review
should hopefully catch unintentional mistakes. The size of this patch
makes such mistakes difficult to spot. For instance, here's something
I spotted only after trying to split the patch myself..
> +static int module_update(int argc, const char **argv, const char *prefix)
> +{
> + const char *update = NULL;
> + struct pathspec pathspec;
> + struct update_data opt = UPDATE_DATA_INIT;
> +
> + struct option module_update_clone_options[] = {
[...]
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper update-clone [--prefix=<path>] [<path>...]"),
> + NULL
> + };
> +
> + update_clone_config_from_gitmodules(&opt.max_jobs);
> + git_config(git_update_clone_config, &opt.max_jobs);
Notice that we copy-pasted the option parsing from update-clone into
module_update() but forgot to update the names.
My ideal patch organization would be something like:
- wrap some existing command in "git submodule--helper update" (probably
run-update-procedure)
- absorb the surrounding sh code into "git submodule--helper
update" one command at-a-time i.e. deprecating and removing the
commands one at a time - instead of deprecating and removing them all
at once (like this patch), or deprecating all at once and removing
them one at a time (like v1).
I don't know if it's feasible or not; Atharva noted upthread that there
are some technical reasons why some things can be done in-process and
some cannot, but it might be a useful exercise.
Here's what I propose:
- If you think this alternative organization would be helpful for you
too, I will attempt it. This will take a while, but by the end you and
I will have effectively reviewed all of the code, so it should be easy
to finish up the review.
- Otherwise e.g. maybe this is a huge waste of time, or you're already
really confident in the correctness of the sh -> c when you reviewed
the original patch, etc, I'll just review this patch as-is. I'd
appreciate any tips and tricks that might help :)
- Orthogonal to patch organization, I'm still not sure if this will be
rebased on es/superproject-aware-submodules or vice-versa, and I don't
want either of us to sink too much effort before knowing the answer.
[1] https://lore.kernel.org/git/20210907115932.36068-7-raykar.ath@xxxxxxxxx/
[2] https://lore.kernel.org/git/YWiXL+plA7GHfuVv@xxxxxxxxxx/
