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

> On Wed, Feb 02 2022, Glen Choo wrote:
>
>> - 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.
>
> I think it makes sense to get this series through first, i.e. the
> (supposedly) no-behavior-changing one, and then one that introduces new
> submodule behavior.
>
> Particularly because for es/superproject-aware-submodules the main
> selling point is a performance improvement, which as I noted in the
> review for it I've been unable to observe once the C<->sh layer goes
> away.
>
> I'm not saying it's not there, just that I don't think it's been shown
> so far, IIRC there was some reference to some Google-internal network FS
> that might or might not be helped by it...
>
>> - 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 do think atomic changes that don't leave dead code for removal later
> are easier to read & reason about, whatever else is reorganized.
>
> I.e. not to have something where we replace all the running code, and
> then remove already-unused code later.
>
> On that topic, I noticed this series could/should have [1] fixed up into
> it.
>
>> - 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.
>
> I think it might, but I really don't know. We'll just have to see, so if
> you want to take a stab at it that would be great.
>
> Maybe it's a good way forward. E.g. as af first small step we could turn:
>
>     while read -r quickabort sha1 just_cloned sm_path
>     [...]
>     die_if_unmatched "$quickabort" "$sha1"
>
> into version where we fold that die_if_unmatched() logic into the C
> code, and then ensure-core-worktree etc.