Re: [GSoC] [PATCH v2] submodule--helper: run update procedures from C

[Atharva Raykar <raykar.ath@gmail.com>]

(I am resending this email, because my client mangled the whitespaces
due to a misconfiguration. Please ignore the my previous message.)

Shourya Shukla <periperidip@gmail.com> writes:

> Le lun. 2 août 2021 à 18:36, Atharva Raykar <raykar.ath@gmail.com> a écrit :
>>
>> Add a new submodule--helper subcommand `run-update-procedure` that runs
>> the update procedure if the SHA1 of the submodule does not match what
>> the superproject expects.
>>
>> This is an intermediate change that works towards total conversion of
>> `submodule update` from shell to C.
>>
>> Specific error codes are returned so that the shell script calling the
>> subcommand can take a decision on the control flow, and preserve the
>> error messages across subsequent recursive calls of `cmd_update`.
>>
>> This patch could have been approached differently, by first changing the
>> `is_tip_reachable` and `fetch_in_submodule` shell functions to be
>> `submodule--helper` subcommands, and then following up with a patch that
>> introduces the `run-update-procedure` subcommand. We have not done it
>> like that because those functions are trivial enough to convert directly
>> along with these other changes. This lets us avoid the boilerplate and
>> the cleanup patches that will need to be introduced in following that
>> approach.
>
> I feel that this part is more suitable for a cover letter rather than the commit
> message itself. It is a useful piece of info though.

Okay, that seems right, the message does seem a bit too context-sensitive.

>> This change is more focused on doing a faithful conversion, so for now we
>> are not too concerned with trying to reduce subprocess spawns.
>>
>> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Shourya Shukla <periperidip@gmail.com>
>> ---
>>
>> Notable changes since v1:
>>
>> * Modified the code structure in
>>   submodule--helper.c:run_update_command(), while fixing problems with
>>   the translation marks.
>>
>> * Renamed '--sha1' and '--subsha1' options to '--oid' and '--suboid' to
>>   since the argument is parsed into an object_id struct, not plain sha1
>>   data.
>>
>> * Used option callbacks to parse the SHA1 arguments directly.
>>
>> * Moved the conditional out of 'do_run_update_procedure()'.
>>
>> Feedback required:
>>
>> Ævar felt that it would be clearer to populate the 'fatal' messages
>> after the run_command() operation in 'run_update_command()', to make it
>> more readable [1]. I have attempted something like that here, and it has led
>> to a lot more duplicated 'switch' statements, which feels suboptimal.
>> I'd appreciate suggestions to make it more legible.
>>
>> [1] https://lore.kernel.org/git/87r1fps63r.fsf@evledraar.gmail.com/
>>
>> Fetch-it-Via:
>> git fetch https://github.com/tfidfwastaken/git submodule-run-update-proc-list-2
>>
>>  builtin/submodule--helper.c | 253 ++++++++++++++++++++++++++++++++++++
>>  git-submodule.sh            | 106 +++++----------
>>  2 files changed, 286 insertions(+), 73 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index d55f6262e9..b9c40324d0 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2029,6 +2029,20 @@ struct submodule_update_clone {
>>         .max_jobs = 1, \
>>  }
>>
>> +struct update_data {
>> +       const char *recursive_prefix;
>> +       const char *sm_path;
>> +       const char *displaypath;
>> +       struct object_id oid;
>> +       struct object_id suboid;
>> +       struct submodule_update_strategy update_strategy;
>> +       int depth;
>> +       unsigned int force: 1;
>> +       unsigned int quiet: 1;
>> +       unsigned int nofetch: 1;
>> +       unsigned int just_cloned: 1;
>> +};
>> +#define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }
>>
>>  static void next_submodule_warn_missing(struct submodule_update_clone *suc,
>>                 struct strbuf *out, const char *displaypath)
>> @@ -2282,6 +2296,175 @@ static int git_update_clone_config(const char *var, const char *value,
>>         return 0;
>>  }
>> +
>> +static int run_update_command(struct update_data *ud, int subforce)
>> +{
>> +       struct child_process cp = CHILD_PROCESS_INIT;
>> +       char *oid = oid_to_hex(&ud->oid);
>> +       int must_die_on_failure = 0;
>> +
>> +       cp.dir = xstrdup(ud->sm_path);
>> +       switch (ud->update_strategy.type) {
>> +       case SM_UPDATE_CHECKOUT:
>> +               cp.git_cmd = 1;
>> +               strvec_pushl(&cp.args, "checkout", "-q", NULL);
>
> Would it be possible to add the 'if' statement above just before the
> 'switch' (or after,
> whichever seems okay) since this is common amongst (almost) all the cases?

I'll try it on once, if it makes the code more readable, I'll include it in the
reroll.

>> +               if (subforce)
>> +                       strvec_push(&cp.args, "-f");
>> +               break;
>> +       case SM_UPDATE_REBASE:
>> +               cp.git_cmd = 1;
>> +               strvec_push(&cp.args, "rebase");
>> +               if (ud->quiet)
>> +                       strvec_push(&cp.args, "--quiet");
>> +               must_die_on_failure = 1;
>> +               break;
>> +       case SM_UPDATE_MERGE:
>> +               cp.git_cmd = 1;
>> +               strvec_push(&cp.args, "merge");
>> +               if (ud->quiet)
>> +                       strvec_push(&cp.args, "--quiet");
>> +               must_die_on_failure = 1;
>> +               break;
>> +       case SM_UPDATE_COMMAND:
>> +               /* NOTE: this does not handle quoted arguments */
>> +               strvec_split(&cp.args, ud->update_strategy.command);
>> +               must_die_on_failure = 1;
>> +               break;
>> +       case SM_UPDATE_UNSPECIFIED:
>> +       case SM_UPDATE_NONE:
>> +               BUG("update strategy should have been specified");
>> +       }
>
> If the original did not bug out, do we need to? The documentation does
> not mention
> this as well:
> https://git-scm.com/docs/git-submodule#Documentation/git-submodule.txt-none

This was how the original shell porcelain did it:

case "$update_module" in
checkout)
	command="git checkout $subforce -q"
	die_msg="$(eval_gettext "Unable to checkout '\$sha1' in submodule path '\$displaypath'")"
	say_msg="$(eval_gettext "Submodule path '\$displaypath': checked out '\$sha1'")"
	;;
rebase)
	command="git rebase ${GIT_QUIET:+--quiet}"
	die_msg="$(eval_gettext "Unable to rebase '\$sha1' in submodule path '\$displaypath'")"
	say_msg="$(eval_gettext "Submodule path '\$displaypath': rebased into '\$sha1'")"
	must_die_on_failure=yes
	;;
merge)
	command="git merge ${GIT_QUIET:+--quiet}"
	die_msg="$(eval_gettext "Unable to merge '\$sha1' in submodule path '\$displaypath'")"
	say_msg="$(eval_gettext "Submodule path '\$displaypath': merged in '\$sha1'")"
	must_die_on_failure=yes
	;;
!*)
	command="${update_module#!}"
	die_msg="$(eval_gettext "Execution of '\$command \$sha1' failed in submodule path '\$displaypath'")"
	say_msg="$(eval_gettext "Submodule path '\$displaypath': '\$command \$sha1'")"
	must_die_on_failure=yes
	;;
*)
	die "$(eval_gettext "Invalid update mode '$update_module' for submodule path '$path'")"
esac

The fallthrough case used to die, but I noticed that this branch will never get
activated. This is because the 'update-clone' helper will not output any entry
that has the update mode set to 'none', and thus the `while` loop that contains
this code would never run.

Which is why I decided to BUG out on that case, because that state should never
be reached. But I see the source of confusion, and maybe I should have different
BUG messages for SM_UPDATE_UNSPECIFIED and SM_UPDATE_NONE. The latter should
probably say "should have been handled by update-clone".

>> +
>> +       strvec_push(&cp.args, oid);
>> +
>> +       prepare_submodule_repo_env(&cp.env_array);
>> +
>> +       if (run_command(&cp)) {
>> +               if (must_die_on_failure) {
>> +                       switch (ud->update_strategy.type) {
>> +                       case SM_UPDATE_CHECKOUT:
>> +                               die(_("Unable to checkout '%s' in submodule path '%s'"),
>> +                                     oid, ud->displaypath);
>> +                               break;
>> +                       case SM_UPDATE_REBASE:
>> +                               die(_("Unable to rebase '%s' in submodule path '%s'"),
>> +                                     oid, ud->displaypath);
>> +                               break;
>> +                       case SM_UPDATE_MERGE:
>> +                               die(_("Unable to merge '%s' in submodule path '%s'"),
>> +                                     oid, ud->displaypath);
>> +                               break;
>> +                       case SM_UPDATE_COMMAND:
>> +                               die(_("Execution of '%s %s' failed in submodule path '%s'"),
>> +                                     ud->update_strategy.command, oid, ud->displaypath);
>> +                               break;
>> +                       case SM_UPDATE_UNSPECIFIED:
>> +                       case SM_UPDATE_NONE:
>> +                               BUG("update strategy should have been specified");
>> +                       }
>> +               }
>> +               /*
>> +                * This signifies to the caller in shell that
>> +                * the command failed without dying
>> +                */
>> +               return 1;
>> +       }
>> +
>> +       switch (ud->update_strategy.type) {
>> +       case SM_UPDATE_CHECKOUT:
>> +               printf(_("Submodule path '%s': checked out '%s'\n"),
>> +                      ud->displaypath, oid);
>> +               break;
>> +       case SM_UPDATE_REBASE:
>> +               printf(_("Submodule path '%s': rebased into '%s'\n"),
>> +                      ud->displaypath, oid);
>> +               break;
>> +       case SM_UPDATE_MERGE:
>> +               printf(_("Submodule path '%s': merged in '%s'\n"),
>> +                      ud->displaypath, oid);
>> +               break;
>> +       case SM_UPDATE_COMMAND:
>> +               printf(_("Submodule path '%s': '%s %s'\n"),
>> +                      ud->displaypath, ud->update_strategy.command, oid);
>> +               break;
>> +       case SM_UPDATE_UNSPECIFIED:
>> +       case SM_UPDATE_NONE:
>> +               BUG("update strategy should have been specified");
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int do_run_update_procedure(struct update_data *ud)
>> +{
>> +       int subforce = is_null_oid(&ud->suboid) || ud->force;
>> +
>> +       if (!ud->nofetch) {
>> +               /*
>> +                * Run fetch only if `oid` isn't present or it
>> +                * is not reachable from a ref.
>> +                */
>> +               if (!is_tip_reachable(ud->sm_path, &ud->oid))
>> +                       if (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, NULL) &&
>> +                           !ud->quiet)
>> +                               fprintf_ln(stderr,
>> +                                          _("Unable to fetch in submodule path '%s'; "
>> +                                            "trying to directly fetch %s:"),
>> +                                          ud->displaypath, oid_to_hex(&ud->oid));
>
> I was wondering if an OID is invalid, will it be counted as
> unreachable and vice-versa?
> If that is the case then that would simplify the work.

Could you elaborate? I'm not sure what you mean by 'invalid' in this context. I
don't think this code will receive any kind of malformed oid--they come from
'update-clone' which handles it correctly.

As far as I can tell, the only way to check if a particular OID is unreachable
is when we check if all the refs cannot find it.