Shourya Shukla <periperidip@xxxxxxxxx> writes:
Le lun. 2 août 2021 à 18:36, Atharva Raykar
<raykar.ath@xxxxxxxxx> 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@xxxxxxxxx>
Mentored-by: Christian Couder <christian.couder@xxxxxxxxx>
Mentored-by: Shourya Shukla <periperidip@xxxxxxxxx>
---
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@xxxxxxxxxxxxxxxxxxx/
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.
