[PATCH v7 00/20] submodule: convert the rest of 'update' to C

From: Glen Choo <chooglen@google.com>
To: git@vger.kernel.org
Cc: "Glen Choo" <chooglen@google.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Atharva Raykar" <raykar.ath@gmail.com>,
	"Christian Couder" <christian.couder@gmail.com>,
	"Emily Shaffer" <emilyshaffer@google.com>,
	"Jonathan Nieder" <jrn@google.com>,
	"Kaartic Sivaraam" <kaartic.sivaraam@gmail.com>,
	pc44800@gmail.com, "Shourya Shukla" <periperidip@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v7 00/20] submodule: convert the rest of 'update' to C
Date: Thu, 10 Feb 2022 17:28:13 +0800	[thread overview]
Message-ID: <20220210092833.55360-1-chooglen@google.com> (raw)
In-Reply-To: 20220208083952.35036-1-chooglen@google.com

This reroll contains another 'easy' preparatory patch and the fixups I
alluded to in v6 [1]. This isn't the split-up I described in the
footnote of v6, but it gets the big patch (patch 17) to what I think is
a reviewable state.

The diff between v7 and v5 is no longer just NEEDSWORK comments, but I
think it is easier to reason about. Patch 17 resembles v5 the most (I
will include a diff in a reply to that patch); everything after patch 17
is fixups (I did not squash them in because they would grow the diff
even more).

I will also leave a review on patch 17 since the changes were not
originally authored by me.

[1] https://lore.kernel.org/git/20220208083952.35036-1-chooglen@google.com

Changes in v7:
- Split the last patch of v6 (the big one) into patches 16-17.
- Patch 16 moves logic out of run_update_procedure() (because the
  command is going away), removing some noise from patch 17. This makes
  the update_strategy parsing easier to reason about, but at the cost of
  growing the diff vis-a-vis v5
- Patches 18-20 are fixups that address NEEDSWORK comments from earlier
  patches. Once maintaining a small diff vis-a-vis v5 stops making
  sense, I will squash them in.

Atharva Raykar (6):
  submodule--helper: get remote names from any repository
  submodule--helper: refactor get_submodule_displaypath()
  submodule--helper: allow setting superprefix for init_submodule()
  submodule--helper: run update using child process struct
  builtin/submodule--helper.c: reformat designated initializers
  submodule: move core cmd_update() logic to C

Glen Choo (11):
  submodule--helper: remove update-module-mode
  submodule--helper: reorganize code for sh to C conversion
  submodule--helper run-update-procedure: remove --suboid
  submodule--helper run-update-procedure: learn --remote
  submodule--helper: remove ensure-core-worktree
  submodule--helper update-clone: learn --init
  submodule--helper: move functions around
  submodule--helper: reduce logic in run_update_procedure()
  fixup! submodule--helper run-update-procedure: remove --suboid
  fixup! submodule--helper run-update-procedure: learn --remote
  fixup! submodule: move core cmd_update() logic to C

Ævar Arnfjörð Bjarmason (3):
  builtin/submodule--helper.c: rename option variables to "opt"
  submodule--helper: don't use bitfield indirection for parse_options()
  submodule tests: test for init and update failure output

 builtin/submodule--helper.c    | 695 ++++++++++++++++++---------------
 git-submodule.sh               | 153 +-------
 t/t7406-submodule-update.sh    |  14 +-
 t/t7408-submodule-reference.sh |  14 +-
 4 files changed, 423 insertions(+), 453 deletions(-)

Range-diff against v6:
 1:  79c522cf56 =  1:  86ffb53742 submodule--helper: get remote names from any repository
 2:  0b97034d89 =  2:  2a40266b7a submodule--helper: refactor get_submodule_displaypath()
 3:  fedbed87a3 =  3:  cd851c8eb5 submodule--helper: allow setting superprefix for init_submodule()
 4:  8e7868c5d1 =  4:  bfe5cad136 submodule--helper: run update using child process struct
 5:  3dee4b9b15 =  5:  72c257fdbf builtin/submodule--helper.c: reformat designated initializers
 6:  e2cc866e6b =  6:  4b5f703fde builtin/submodule--helper.c: rename option variables to "opt"
 7:  4831695dc6 =  7:  f1d21f5b1c submodule--helper: don't use bitfield indirection for parse_options()
 8:  c2dadfffb2 =  8:  9d32a73fc3 submodule tests: test for init and update failure output
 9:  79ceb88dee =  9:  087bf43aba submodule--helper: remove update-module-mode
10:  6fe25e24da = 10:  4eb2893a19 submodule--helper: reorganize code for sh to C conversion
11:  52c997f97b = 11:  c08e7781e3 submodule--helper run-update-procedure: remove --suboid
12:  61a5b02472 = 12:  2419c37184 submodule--helper run-update-procedure: learn --remote
13:  f76627a078 = 13:  6691fd3648 submodule--helper: remove ensure-core-worktree
14:  2d93c4232b = 14:  d2c9c356e9 submodule--helper update-clone: learn --init
15:  d4899c5635 ! 15:  c8945fcc6f submodule--helper: move functions around
    @@ Metadata
      ## Commit message ##
         submodule--helper: move functions around

    -    The next commit will change the internals of several functions and
    +    A subsequent commit will change the internals of several functions and
         arrange them in a more logical manner. Move these functions to their
         final positions so that the diff is smaller.

 -:  ---------- > 16:  10af533ae0 submodule--helper: reduce logic in run_update_procedure()
16:  edf752da8d ! 17:  19143f6009 submodule: move core cmd_update() logic to C
    @@ builtin/submodule--helper.c: struct submodule_update_clone {
      };
      #define SUBMODULE_UPDATE_CLONE_INIT { \
      	.list = MODULE_LIST_INIT, \
    -@@ builtin/submodule--helper.c: struct submodule_update_clone {
    - }
    - 
    - struct update_data {
    -+	const char *prefix;
    - 	const char *recursive_prefix;
    - 	const char *sm_path;
    - 	const char *displaypath;
     @@ builtin/submodule--helper.c: struct update_data {
      	struct object_id suboid;
      	struct submodule_update_strategy update_strategy;
    @@ builtin/submodule--helper.c: struct update_data {

      static void next_submodule_warn_missing(struct submodule_update_clone *suc,
     @@ builtin/submodule--helper.c: 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;
    -+	struct submodule_update_strategy strategy = SUBMODULE_UPDATE_STRATEGY_INIT;
    -+
    -+	if (ud->update_strategy.type == SM_UPDATE_UNSPECIFIED || ud->just_cloned)
    -+		determine_submodule_update_strategy(the_repository, ud->just_cloned,
    -+						    ud->sm_path, NULL, &strategy);
    -+	else
    -+		strategy = ud->update_strategy;
    - 
    --	switch (ud->update_strategy.type) {
    -+	switch (strategy.type) {
    - 	case SM_UPDATE_CHECKOUT:
    - 		cp.git_cmd = 1;
    - 		strvec_pushl(&cp.args, "checkout", "-q", NULL);
    -@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce)
    - 		break;
    - 	case SM_UPDATE_COMMAND:
    - 		cp.use_shell = 1;
    --		strvec_push(&cp.args, ud->update_strategy.command);
    -+		strvec_push(&cp.args, strategy.command);
    - 		must_die_on_failure = 1;
    - 		break;
    - 	default:
    - 		BUG("unexpected update strategy type: %s",
    --		    submodule_strategy_to_string(&ud->update_strategy));
    -+		    submodule_strategy_to_string(&strategy));
    - 	}
    - 	strvec_push(&cp.args, oid);
    - 
    - 	cp.dir = xstrdup(ud->sm_path);
    - 	prepare_submodule_repo_env(&cp.env_array);
      	if (run_command(&cp)) {
    --		switch (ud->update_strategy.type) {
    -+		switch (strategy.type) {
    + 		switch (ud->update_strategy.type) {
      		case SM_UPDATE_CHECKOUT:
     -			printf(_("Unable to checkout '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u
      		case SM_UPDATE_REBASE:
     -			printf(_("Unable to rebase '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Unable to rebase '%s' in submodule path '%s'"),
    ++			die_message(_("Unable to rebase '%s' in submodule path '%s'"),
     +			    oid, ud->displaypath);
      			break;
      		case SM_UPDATE_MERGE:
     -			printf(_("Unable to merge '%s' in submodule path '%s'"),
     -			       oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Unable to merge '%s' in submodule path '%s'"),
    ++			die_message(_("Unable to merge '%s' in submodule path '%s'"),
     +			    oid, ud->displaypath);
      			break;
      		case SM_UPDATE_COMMAND:
     -			printf(_("Execution of '%s %s' failed in submodule path '%s'"),
     -			       ud->update_strategy.command, oid, ud->displaypath);
    -+			if (!must_die_on_failure)
    -+				break;
    -+			die(_("Execution of '%s %s' failed in submodule path '%s'"),
    -+			    strategy.command, oid, ud->displaypath);
    ++			die_message(_("Execution of '%s %s' failed in submodule path '%s'"),
    ++			    ud->update_strategy.command, oid, ud->displaypath);
      			break;
      		default:
      			BUG("unexpected update strategy type: %s",
    --			    submodule_strategy_to_string(&ud->update_strategy));
    -+			    submodule_strategy_to_string(&strategy));
    + 			    submodule_strategy_to_string(&ud->update_strategy));
      		}
     -		/*
     -		 * NEEDSWORK: We are currently printing to stdout with error
    @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *u
     -		 * properly. Once we start handling the error messages within
     -		 * C, we should use die() instead.
     -		 */
    --		if (must_die_on_failure)
    + 		if (must_die_on_failure)
     -			return 2;
     -		/*
     -		 * This signifies to the caller in shell that the command
     -		 * failed without dying
     -		 */
    ++			exit(128);
     +
     +		/* the command failed, but update must continue */
      		return 1;
      	}

    --	switch (ud->update_strategy.type) {
     +	if (ud->quiet)
     +		return 0;
     +
    -+	switch (strategy.type) {
    + 	switch (ud->update_strategy.type) {
      	case SM_UPDATE_CHECKOUT:
      		printf(_("Submodule path '%s': checked out '%s'\n"),
    - 		       ud->displaypath, oid);
     @@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce)
    - 		break;
    - 	case SM_UPDATE_COMMAND:
    - 		printf(_("Submodule path '%s': '%s %s'\n"),
    --		       ud->displaypath, ud->update_strategy.command, oid);
    -+		       ud->displaypath, strategy.command, oid);
    - 		break;
    - 	default:
    - 		BUG("unexpected update strategy type: %s",
    --		    submodule_strategy_to_string(&ud->update_strategy));
    -+		    submodule_strategy_to_string(&strategy));
    - 	}
    - 
      	return 0;
      }

    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -static int update_submodule2(struct update_data *update_data);
     -static int run_update_procedure(int argc, const char **argv, const char *prefix)
     -{
    --	char *prefixed_path, *update = NULL;
     -	struct update_data opt = UPDATE_DATA_INIT;
     -
     -	struct option options[] = {
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -		OPT_BOOL(0, "just-cloned", &opt.just_cloned,
     -			 N_("overrides update mode in case the repository is a fresh clone")),
     -		OPT_INTEGER(0, "depth", &opt.depth, N_("depth for shallow fetch")),
    --		OPT_STRING(0, "prefix", &prefix,
    +-		OPT_STRING(0, "prefix", &opt.prefix,
     -			   N_("path"),
     -			   N_("path into the working tree")),
    --		OPT_STRING(0, "update", &update,
    +-		OPT_STRING(0, "update", &opt.update_default,
     -			   N_("string"),
     -			   N_("rebase, merge, checkout or none")),
     -		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix, N_("path"),
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -
     -	opt.sm_path = argv[0];
     -
    --	if (opt.recursive_prefix)
    --		prefixed_path = xstrfmt("%s%s", opt.recursive_prefix, opt.sm_path);
    --	else
    --		prefixed_path = xstrdup(opt.sm_path);
    --
    --	opt.displaypath = get_submodule_displaypath(prefixed_path, prefix);
    --
    --	determine_submodule_update_strategy(the_repository, opt.just_cloned,
    --					    opt.sm_path, update,
    --					    &opt.update_strategy);
    --
    --	free(prefixed_path);
     -	return update_submodule2(&opt);
     -}
     -
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     -static int update_submodule2(struct update_data *update_data)
     +static void update_data_to_args(struct update_data *update_data, struct strvec *args)
     +{
    -+	const char *update = submodule_strategy_to_string(&update_data->update_strategy);
    -+
     +	strvec_pushl(args, "submodule--helper", "update", "--recursive", NULL);
     +	strvec_pushf(args, "--jobs=%d", update_data->max_jobs);
    ++	/*
    ++	 * NEEDSWORK: the equivalent code in git-submodule.sh does not
    ++	 * pass --prefix, so this shouldn't either
    ++	*/
     +	if (update_data->prefix)
     +		strvec_pushl(args, "--prefix", update_data->prefix, NULL);
     +	if (update_data->recursive_prefix)
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +		strvec_push(args, "--require-init");
     +	if (update_data->depth)
     +		strvec_pushf(args, "--depth=%d", update_data->depth);
    -+	if (update)
    -+		strvec_pushl(args, "--update", update, NULL);
    ++	if (update_data->update_default)
    ++		strvec_pushl(args, "--update", update_data->update_default, NULL);
     +	if (update_data->references.nr) {
     +		struct string_list_item *item;
     +		for_each_string_list_item(item, &update_data->references)
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +
     +static int update_submodule(struct update_data *update_data)
      {
    -+	char *prefixed_path;
    -+
    - 	ensure_core_worktree(update_data->sm_path);
    + 	char *prefixed_path;

    -+	if (update_data->recursive_prefix)
    -+		prefixed_path = xstrfmt("%s%s", update_data->recursive_prefix,
    -+					update_data->sm_path);
    -+	else
    -+		prefixed_path = xstrdup(update_data->sm_path);
    -+
    -+	update_data->displaypath = get_submodule_displaypath(prefixed_path,
    -+							     update_data->prefix);
    -+	free(prefixed_path);
    -+
    - 	/* NEEDSWORK: fix the style issues e.g. braces */
    - 	if (update_data->just_cloned) {
    - 		oidcpy(&update_data->suboid, null_oid());
     @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *update_data)
      	}

    @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *up
     -		return do_run_update_procedure(update_data);
     +		if (run_update_procedure(update_data))
     +			return 1;
    - 
    --	return 3;
    ++
     +	if (update_data->recursive) {
     +		struct child_process cp = CHILD_PROCESS_INIT;
     +		struct update_data next = *update_data;
    @@ builtin/submodule--helper.c: static int update_submodule2(struct update_data *up
     +		cp.git_cmd = 1;
     +		prepare_submodule_repo_env(&cp.env_array);
     +		update_data_to_args(&next, &cp.args);
    -+
    + 
    +-	return 3;
     +		/* die() if child process die()'d */
     +		res = run_command(&cp);
     +		if (!res)
    @@ builtin/submodule--helper.c: static int update_submodules(struct submodule_updat
     -static int update_clone(int argc, const char **argv, const char *prefix)
     +static int module_update(int argc, const char **argv, const char *prefix)
      {
    - 	const char *update = NULL;
    +-	const char *update = NULL;
      	struct pathspec pathspec;
     -	struct submodule_update_clone opt = SUBMODULE_UPDATE_CLONE_INIT;
     +	struct update_data opt = UPDATE_DATA_INIT;
    @@ builtin/submodule--helper.c: static int update_submodules(struct submodule_updat
      			   N_("path"),
      			   N_("path into the working tree")),
      		OPT_STRING(0, "recursive-prefix", &opt.recursive_prefix,
    -@@ builtin/submodule--helper.c: static int update_clone(int argc, const char **argv, const char *prefix)
    + 			   N_("path"),
    + 			   N_("path into the working tree, across nested "
    + 			      "submodule boundaries")),
    +-		OPT_STRING(0, "update", &update,
    ++		OPT_STRING(0, "update", &opt.update_default,
      			   N_("string"),
      			   N_("rebase, merge, checkout or none")),
      		OPT_STRING_LIST(0, "reference", &opt.references, N_("repo"),
    @@ builtin/submodule--helper.c: static int update_clone(int argc, const char **argv
     +	oidcpy(&opt.oid, null_oid());
     +	oidcpy(&opt.suboid, null_oid());

    - 	if (update)
    +-	if (update)
     -		if (parse_submodule_update_strategy(update, &opt.update) < 0)
    -+		if (parse_submodule_update_strategy(update,
    ++	if (opt.update_default)
    ++		if (parse_submodule_update_strategy(opt.update_default,
     +						    &opt.update_strategy) < 0)
      			die(_("bad value for update parameter"));

    @@ builtin/submodule--helper.c: static struct cmd_struct commands[] = {
      	{"init", module_init, SUPPORT_SUPER_PREFIX},

      ## git-submodule.sh ##
    +@@ git-submodule.sh: single_branch=
    + jobs=
    + recommend_shallow=
    + 
    ++# NEEDSWORK this is now unused
    + die_if_unmatched ()
    + {
    + 	if test "$1" = "#unmatched"
     @@ git-submodule.sh: cmd_update()
      		shift
      	done

     -	{
     -	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper update-clone \
    ++	# NEEDSWORK --super-prefix isn't actually supported by this
    ++	# command - we just pass the $prefix to --recursive-prefix.
     +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper update \
      		${GIT_QUIET:+--quiet} \
     -		${progress:+"--progress"} \
 -:  ---------- > 18:  9d0afc60ec fixup! submodule--helper run-update-procedure: remove --suboid
 -:  ---------- > 19:  e700861239 fixup! submodule--helper run-update-procedure: learn --remote
 -:  ---------- > 20:  aef1a03b44 fixup! submodule: move core cmd_update() logic to C

base-commit: b23dac905bde28da47543484320db16312c87551
-- 
2.33.GIT