Re: [PATCH v5 9/9] submodule: move core cmd_update() logic to C

[Glen Choo <chooglen@google.com>]

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@gmail.com/
[2] https://lore.kernel.org/git/YWiXL+plA7GHfuVv@google.com/