Pardon my late response, I had been occupied with other things.
Junio C Hamano <gitster@xxxxxxxxx> writes:
> Atharva Raykar <raykar.ath@xxxxxxxxx> writes:
> [...]
>> Signed-off-by: Atharva Raykar <raykar.ath@xxxxxxxxx>
>> Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
>> Mentored-by: Shourya Shukla <periperidip@xxxxxxxxx>
>
> Keep trailer lines in chronological order. The mentors mentored,
> the patch was written and finally you signed it off.
Okay.
>> +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);
>> + 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);
>
> Indeed this doesn't. I think cp.use_shell would be the right way to
> run this.
>
> Study what happens before run_command_v_opt_cd_env() with
> RUN_USING_SHELL calls run_command() and make something similar
> happen here, instead of doing a manual command line splitting.
>
> Side note: run_command_v_opt_cd_env() with RUN_USING_SHELL
> is used in places like diff.c::run_external_diff() to invoke
> an external diff command, ll-merge.c::ll_ext_merge() to
> invoke a user-defined low level merge driver,
> sequencer.c::do_exec() to invoke 'x cmd' you write in the
> todo list during an "rebase -i" session.
Thanks for the pointers, the details helped. I'll handle this more
correctly in the next version.
>> + must_die_on_failure = 1;
>> + break;
>> + case SM_UPDATE_NONE:
>> + BUG("this should have been handled before. How did we reach here?");
>> + break;
>> + case SM_UPDATE_UNSPECIFIED:
>> + BUG("update strategy should have been specified");
>
> These two case arms are not a faithful conversion from the original,
> but because you do not carry around a random string from the caller
> and instead have parsed enums, it cannot be ;-) But it makes me
> wonder why we want these two cases separate. Isn't it a BUG() if
> anything other than what we handled (i.e. prepared cp.args for)
> already is in ud->update_strategy.type? IOW, wouldn't it be more
> forward looking to do
>
> default:
> BUG("unexpected ud->update_strategy.type (%d)",
> ud->update_strategy.type (%d)");
>
> or something? That way, if we ever come up with a new update
> strategy and forget to update this part, we will catch such a bug
> fairly quickly.
The original intention for separating the cases was to differentiate the
cause for the invalid state, but your proposed suggestion is a lot
better. I'll address this.
>> + }
>> +
>> + 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;
>
> The messages here correspond to what is assigned to $die_message in
> the original. Note that they are emitted to the standard error
> stream.
>
> I suspect that these should be "printf()" followed by a call to
> exit() with some non-zero value (see below).
I also notice another major lapse in conversion. In the shell porcelain,
the "checkout" mode should not die out at all, instead it should print
out the error message.
My code tries to die() on the checkout mode (in a case arm that will
never be activated), and does not ever print the checkout failure
message at all. I will fix this in the re-roll.
(Will address more of this below...)
>> + case SM_UPDATE_NONE:
>> + BUG("this should have been handled before. How did we reach here?");
>> + break;
>> + case SM_UPDATE_UNSPECIFIED:
>> + BUG("update strategy should have been specified");
>> + }
>
> The same comment applies to the last two case arms of this switch
> statement and the next one, too. I think we just should catch
> "everything else" with a simple "default:" label.
>
> Also, don't omit the "break;" from the last case arm in a switch
> statement. It harms the long-term help of the code---the last case
> arm may not forever stay to be the last one.
>
>> + }
>> + /*
>> + * 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_NONE:
>> + BUG("this should have been handled before. How did we reach here?");
>> + break;
>> + case SM_UPDATE_UNSPECIFIED:
>> + BUG("update strategy should have been specified");
>
> Likewise here.
Okay.
>> + }
>> +
>> + 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,
>
> OK. Combining these into a single statement like
>
> if (!is_tip_reachable(...) &&
> fetch_in_submodule(...) &&
> !ud->quiet)
> fprintf_ln(...
>
> would reduce the indentation level, but the way the conditional is
> structured may convey the flow of the thought better, i.e.
>
> if we need to fetch,
> try to fetch and if that fails,
> report failure.
>
> On the other hand, if we take that line of thought to the extreme,
> the check for !ud->quiet should belong to another level of if
> statement so perhaps the more concise version I showed above might
> be an overall win. I dunno.
Right. I agree with you, mainly because there are many other predicates
in my previous conversions that were done with short-circuited &&'s, so
might as well stick to what has been my convention.
>> + _("Unable to fetch in submodule path '%s'; "
>> + "trying to directly fetch %s:"),
>> + ud->displaypath, oid_to_hex(&ud->oid));
>> + /*
>> + * Now we tried the usual fetch, but `oid` may
>> + * not be reachable from any of the refs.
>> + */
>> + if (!is_tip_reachable(ud->sm_path, &ud->oid))
>> + if (fetch_in_submodule(ud->sm_path, ud->depth, ud->quiet, &ud->oid))
>> + die(_("Fetched in submodule path '%s', but it did not "
>> + "contain %s. Direct fetching of that commit failed."),
>> + ud->displaypath, oid_to_hex(&ud->oid));
>
> Likewise.
>
>> + }
>> [...]
>> +
>> + if ((!is_null_oid(&update_data.oid) && !is_null_oid(&update_data.suboid) &&
>> + !oideq(&update_data.oid, &update_data.suboid)) ||
>> + is_null_oid(&update_data.suboid) || update_data.force)
>> + return do_run_update_procedure(&update_data);
>
> The original does the update procedure if $sha1 and $subsha1 are
> different or if $force option is given. The rewritten seems to skip
> the update when .oid is NULL and .suboid is not NULL; intended?
Unintended. I initially implemented this with raw chars until I
discovered the object_id API. So this was the result of me
indiscriminately substituting NULL checks with the OID equivalents. I
realise this is not needed anymore, and we can simplify that to:
if (!oideq(&update_data.oid, &update_data.suboid) || update_data.force)
> I understand that the division of labour between this function and
> do_run_update_procedure() is for the former to only exist to
> interface with the script side by populating the update_data
> structure, and the latter implements the logic to run update
> procedure. I was a bit surprised that this conditional is
> here, not at the very beginning of the callee.
Ævar pointed out that since this function just had one caller, I could
move the whole 'if' outside it for now, which would save me one level of
indentation within that function, and make it easier to parse. With the
conditional being simplified to what I showed above, I think it can
still be justified?
Even in the series that will follow this, we would still have only one
caller for this function.
>> + return 3;
>> +}
>> +
>> static int resolve_relative_path(int argc, const char **argv, const char *prefix)
>> {
>> struct strbuf sb = STRBUF_INIT;
>> @@ -2951,6 +3209,7 @@ static struct cmd_struct commands[] = {
>> {"add-clone", add_clone, 0},
>> {"update-module-mode", module_update_module_mode, 0},
>> {"update-clone", update_clone, 0},
>> + {"run-update-procedure", run_update_procedure, 0},
>> {"ensure-core-worktree", ensure_core_worktree, 0},
>> {"relative-path", resolve_relative_path, 0},
>> {"resolve-relative-url", resolve_relative_url, 0},
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index dbd2ec2050..d8e30d1afa 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -519,14 +512,13 @@ cmd_update()
>>
>> git submodule--helper ensure-core-worktree "$sm_path" || exit 1
>>
>> - update_module=$(git submodule--helper update-module-mode $just_cloned "$sm_path" $update)
>> -
>> displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
>
> On the other side of the API boundary, update_data.displaypath is
> populated by value computed in C. It is a bit unfortunate that we
> still need to compute it here and risk the two to drift apart.
Yes, and I had some trouble figuring out a clean separation boundary,
and this compromise felt like the best one. The best I can do is assure
you that the patch series following this change solves this issue
entirely, as it moves all of the shell code you see here into the C
helper, and thus we only compute this value once.
So there should be no worry about drift, as I will not give any chance
to introduce it at all :)
>> if test $just_cloned -eq 1
>> then
>> subsha1=
>> else
>> + just_cloned=
>> subsha1=$(sanitize_submodule_env; cd "$sm_path" &&
>> git rev-parse --verify HEAD) ||
>> die "fatal: $(eval_gettext "Unable to find current revision in submodule path '\$displaypath'")"
>> @@ -547,70 +539,38 @@ cmd_update()
>> die "fatal: $(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
>> fi
>>
>> - if test "$subsha1" != "$sha1" || test -n "$force"
>> - then
>> - subforce=$force
>> - # If we don't already have a -f flag and the submodule has never been checked out
>> - if test -z "$subsha1" && test -z "$force"
>> - then
>> - subforce="-f"
>> - fi
>> + out=$(git submodule--helper run-update-procedure \
>> + ${wt_prefix:+--prefix "$wt_prefix"} \
>> + ${GIT_QUIET:+--quiet} \
>> + ${force:+--force} \
>> + ${just_cloned:+--just-cloned} \
>> + ${nofetch:+--no-fetch} \
>> + ${depth:+"$depth"} \
>> + ${update:+--update "$update"} \
>> + ${prefix:+--recursive-prefix "$prefix"} \
>> + ${sha1:+--oid "$sha1"} \
>> + ${subsha1:+--suboid "$subsha1"} \
>> + "--" \
>> + "$sm_path")
>
> We'd just show errors directly to the standard error stream from
> submodule--helper, but what comes from the printf in the switch
> statement at the end of run_update_command() is captured in $out
> variable. Notably, the messages from die()s in the second switch
> statement in run_update_command() are not captured in $out here.
>
>> - if test -z "$nofetch"
>> - then
>> - # Run fetch only if $sha1 isn't present or it
>> - # is not reachable from a ref.
>> - is_tip_reachable "$sm_path" "$sha1" ||
>> - fetch_in_submodule "$sm_path" $depth ||
>> - say "$(eval_gettext "Unable to fetch in submodule path '\$displaypath'; trying to directly fetch \$sha1:")"
>> -
>> - # Now we tried the usual fetch, but $sha1 may
>> - # not be reachable from any of the refs
>> - is_tip_reachable "$sm_path" "$sha1" ||
>> - fetch_in_submodule "$sm_path" "$depth" "$sha1" ||
>> - die "fatal: $(eval_gettext "Fetched in submodule path '\$displaypath', but it did not contain \$sha1. Direct fetching of that commit failed.")"
>> - fi
>> -
>> - must_die_on_failure=
>> - case "$update_module" in
>> - checkout)
>> - command="git checkout $subforce -q"
>> - die_msg="fatal: $(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="fatal: $(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="fatal: $(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="fatal: $(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 "fatal: $(eval_gettext "Invalid update mode '$update_module' for submodule path '$path'")"
>> - esac
>> -
>> - if (sanitize_submodule_env; cd "$sm_path" && $command "$sha1")
>> - then
>> - say "$say_msg"
>> - elif test -n "$must_die_on_failure"
>> - then
>> - die_with_status 2 "$die_msg"
>> - else
>> - err="${err};$die_msg"
>> - continue
>> - fi
>> - fi
>> + # exit codes for run-update-procedure:
>> + # 0: update was successful, say command output
>> + # 128: subcommand died during execution
>> + # 1: update procedure failed, but should not die
>> + # 3: no update procedure was run
>> + res="$?"
>> + case $res in
>> + 0)
>> + say "$out"
>> + ;;
>
> And the case where there is no error is quite straight-forward. We
> just emit what we saw in the standard output stream of the helper.
>
>> + 128)
>> + exit $res
>> + ;;
>> + 1)
>> + err="${err};$out"
>
> This part is dubious. In the original, $err accumulates what is in
> $die_msg, which are things like "fatal: Unable to rebase ...", but
> with this patch, what used to be the contents of $die_msg are given
> to die() after we see run_command() fail, and would have sent to the
> standard error stream, not captured in $out here, no?
Yes, this is bad. This error slipped past the test suite that I was too
reliant on. Even if it did work, it would not have printed the error
messages for the "checkout" mode. I will fix all of these issues.
Printing to stdout with error return ought to fix it, as you suggested.
>> + continue
>> + ;;
>> + esac
>>
>> if test -n "$recursive"
>> then
>
>
> Thanks.
Thanks for the thorough review.
