[v2,0/8] submodule: convert the rest of 'update' to C

Message

Atharva Raykar Sept. 16, 2021, 10:32 a.m. UTC

NOTE: This series uses ar/submodule-run-update-procedure, which is now part of
'next'. [1]

Since v1:
  I have incorporated Ævar's suggestions, and attempted to make this easier to
  review.

This series builds upon the previous conversion work on 'submodule update' and
moves out all of that shell logic in 'git-submodule.sh' into
'builtin/submodule--helper.c'. Even though this patch series looks a bit long, a
lot of it is preparatory patches and cleanup of unused functions that result
from this conversion. The real action happens at [5/8].

As with the other series, the goal is to be a faithful conversion, with no
change in behaviour.

This would be the last command whose logic would be moved into C, other than
'submodule add', whose patches have been sent already.

After this works out, we can invert the shell-C relationship and make
'submodule' a proper C builtin.

Fetch-it-Via:
git fetch https://github.com/tfidfwastaken/git submodule-update-list-2

[1] https://lore.kernel.org/git/[e-mail]/

Atharva Raykar (8):
  submodule--helper: split up ensure_core_worktree()
  submodule--helper: get remote names from any repository
  submodule--helper: rename helpers for update-clone
  submodule--helper: refactor get_submodule_displaypath()
  submodule: move core cmd_update() logic to C
  submodule--helper: remove update-clone subcommand
  submodule--helper: remove unused helpers
  submodule--helper: rename helper functions

 builtin/submodule--helper.c | 767 +++++++++++++++++++++---------------
 git-submodule.sh            | 145 +------
 2 files changed, 458 insertions(+), 454 deletions(-)

Range-diff against v1:
 1:  2cfdc0e10a <  -:  ---------- submodule--helper: get remote names from any repository
 -:  ---------- >  1:  f83a5b7f34 submodule--helper: split up ensure_core_worktree()
 2:  be83ba7fdb !  2:  7f4e24ce25 submodule--helper: introduce get_default_remote_submodule()
    @@ Metadata
     Author: Atharva Raykar <[e-mail]>
     
      ## Commit message ##
    -    submodule--helper: introduce get_default_remote_submodule()
    +    submodule--helper: get remote names from any repository
     
    -    Before 8ef1d2b549 (submodule--helper: get remote names from any
    -    repository, 2021-07-20), it was not possible to directly retrieve a
    -    submodule's remote name within the same process, because
    -    `get_default_remote()` used only knew about the current repository.
    +    `get_default_remote()` retrieves the name of a remote by resolving the
    +    refs from of the current repository's ref store.
    +
    +    Thus in order to use it for retrieving the remote name of a submodule,
    +    we have to start a new subprocess which runs from the submodule
    +    directory.
    +
    +    Let's instead introduce a function called `repo_get_default_remote()`
    +    which takes any repository object and retrieves the remote accordingly.
    +
    +    `get_default_remote()` is then defined as a call to
    +    `repo_get_default_remote()` with 'the_repository' passed to it.
     
         Now that we have `repo_get_default_remote()`, we no longer have to start
         a subprocess that called `submodule--helper get-default-remote` from
         within the submodule directory.
     
    -    Let's make a function called `get_default_remote_submodule()` which
    +    So let's make a function called `get_default_remote_submodule()` which
         takes a submodule path, and returns the default remote for that
         submodule, all within the same process.
     
    @@ Commit message
         Signed-off-by: Atharva Raykar <[e-mail]>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: static char *repo_get_default_remote(struct repository *repo, const char *refnam
    +@@
    + typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
    + 				  void *cb_data);
    + 
    +-static char *get_default_remote(void)
    ++static char *repo_get_default_remote(struct repository *repo, const char *refname)
    + {
    + 	char *dest = NULL, *ret;
    + 	struct strbuf sb = STRBUF_INIT;
    +-	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
    + 
    + 	if (!refname)
    + 		die(_("No such ref: %s"), "HEAD");
    +@@ builtin/submodule--helper.c: static char *get_default_remote(void)
    + 		die(_("Expecting a full ref name, got %s"), refname);
    + 
    + 	strbuf_addf(&sb, "branch.%s.remote", refname);
    +-	if (git_config_get_string(sb.buf, &dest))
    ++	if (repo_config_get_string(repo, sb.buf, &dest))
    + 		ret = xstrdup("origin");
    + 	else
    + 		ret = dest;
    +@@ builtin/submodule--helper.c: static char *get_default_remote(void)
      	return ret;
      }
      
    @@ builtin/submodule--helper.c: static char *repo_get_default_remote(struct reposit
     +	return repo_get_default_remote(&subrepo, refname);
     +}
     +
    - static char *get_default_remote(void)
    ++static char *get_default_remote(void)
    ++{
    ++	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
    ++	return repo_get_default_remote(the_repository, refname);
    ++}
    ++
    + static int print_default_remote(int argc, const char **argv, const char *prefix)
      {
    - 	const char *refname = resolve_ref_unsafe("HEAD", 0, NULL, NULL);
    + 	char *remote;
     @@ builtin/submodule--helper.c: static void sync_submodule(const char *path, const char *prefix,
      	char *remote_key = NULL;
      	char *sub_origin_url, *super_config_url, *displaypath;
 3:  8981b2c4c5 =  3:  390596ee7c submodule--helper: rename helpers for update-clone
 4:  420d792f99 =  4:  2a2ddcac91 submodule--helper: refactor get_submodule_displaypath()
 5:  6c4696b70a !  5:  832020a290 submodule: move core cmd_update() logic to C
    @@ Commit message
         `init_submodule()`, which will now also take an explicit 'superprefix'
         argument.
     
    +    While we are at it, we also remove the fetch_in_submodule() shell
    +    function since it is no longer used anywhere.
    +
         [1] https://lore.kernel.org/git/[e-mail]/
     
         Mentored-by: Christian Couder <[e-mail]>
    @@ builtin/submodule--helper.c: static char *compute_submodule_clone_url(const char
      	unsigned int flags;
      };
     -#define INIT_CB_INIT { NULL, 0 }
    -+#define INIT_CB_INIT { NULL, NULL, 0 }
    ++#define INIT_CB_INIT { 0 }
      
      static void init_submodule(const char *path, const char *prefix,
     -			   unsigned int flags)
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +	 *   checkout involve more straightforward sequential I/O.
     +	 * - the listener can avoid doing any work if fetching failed.
     +	 */
    -+	if (suc.quickstop)
    ++	if (suc.quickstop) {
    ++		string_list_clear(&update_data->references, 0);
     +		return 1;
    ++	}
     +
     +	for (i = 0; i < suc.update_clone_nr; i++) {
     +		struct update_clone_data ucd = suc.update_clone[i];
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +			res = 1;
     +	}
     +
    ++	string_list_clear(&update_data->references, 0);
     +	return res;
     +}
     +
    @@ builtin/submodule--helper.c: static struct cmd_struct commands[] = {
      	{"run-update-procedure", run_update_procedure, 0},
     
      ## git-submodule.sh ##
    +@@ git-submodule.sh: cmd_deinit()
    + 	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${force:+--force} ${deinit_all:+--all} -- "$@"
    + }
    + 
    +-# usage: fetch_in_submodule <module_path> [<depth>] [<sha1>]
    +-# Because arguments are positional, use an empty string to omit <depth>
    +-# but include <sha1>.
    +-fetch_in_submodule () (
    +-	sanitize_submodule_env &&
    +-	cd "$1" &&
    +-	if test $# -eq 3
    +-	then
    +-		echo "$3" | git fetch ${GIT_QUIET:+--quiet} --stdin ${2:+"$2"}
    +-	else
    +-		git fetch ${GIT_QUIET:+--quiet} ${2:+"$2"}
    +-	fi
    +-)
    +-
    + #
    + # Update each submodule path to correct revision, using clone and checkout as needed
    + #
     @@ git-submodule.sh: cmd_update()
      		shift
      	done
 6:  524ae77c3f <  -:  ---------- submodule: remove fetch_in_submodule shell function
 7:  ea56f7319a =  6:  fb3fa8174a submodule--helper: remove update-clone subcommand
 8:  10a62172a2 <  -:  ---------- submodule--helper: remove update-module-mode subcommand
 9:  dbbe5d3f53 <  -:  ---------- submodule--helper: remove shell interface to ensure_core_worktree()
10:  a015af3a16 <  -:  ---------- submodule--helper: remove print-default-remote subcommand
11:  f5a7ba1405 <  -:  ---------- submodule--helper: remove relative-path subcommand
12:  9f54eb5972 !  7:  364f72f870 submodule--helper: remove run-update-procedure subcommand
    @@ Metadata
     Author: Atharva Raykar <[e-mail]>
     
      ## Commit message ##
    -    submodule--helper: remove run-update-procedure subcommand
    +    submodule--helper: remove unused helpers
     
    -    The subcommand 'submodule--helper run-update-procedure' is no longer
    -    needed after the conversion of the bulk of 'update' to C.
    -
    -    While we are at it, let's rename 'do_run_update_procedure()' to
    -    'run_update_procedure()' to reflect the fact that it is no longer a
    -    utility function meant to be wrapped in another function.
    +    These helpers were useful back when 'submodule update' had most of its
    +    logic in shell. Now that they will never be invoked, let us remove them.
     
         Mentored-by: Christian Couder <[e-mail]>
         Mentored-by: Shourya Shukla <[e-mail]>
         Signed-off-by: Atharva Raykar <[e-mail]>
     
      ## builtin/submodule--helper.c ##
    -@@ builtin/submodule--helper.c: static int run_update_command(struct update_data *ud, int subforce, struct strin
    - 	return 0;
    +@@ builtin/submodule--helper.c: static char *get_default_remote(void)
    + 	return repo_get_default_remote(the_repository, refname);
      }
      
    --static int do_run_update_procedure(struct update_data *ud, struct string_list *err)
    -+static int run_update_procedure(struct update_data *ud, struct string_list *err)
    +-static int print_default_remote(int argc, const char **argv, const char *prefix)
    +-{
    +-	char *remote;
    +-
    +-	if (argc != 1)
    +-		die(_("submodule--helper print-default-remote takes no arguments"));
    +-
    +-	remote = get_default_remote();
    +-	if (remote)
    +-		printf("%s\n", remote);
    +-
    +-	free(remote);
    +-	return 0;
    +-}
    +-
    + static int starts_with_dot_slash(const char *str)
      {
    - 	int subforce = is_null_oid(&ud->suboid) || ud->force;
    + 	return str[0] == '.' && is_dir_sep(str[1]);
    +@@ builtin/submodule--helper.c: static void determine_submodule_update_strategy(struct repository *r,
    + 	free(key);
    + }
      
    +-static int module_update_module_mode(int argc, const char **argv, const char *prefix)
    +-{
    +-	const char *path, *update = NULL;
    +-	int just_cloned;
    +-	struct submodule_update_strategy update_strategy = { .type = SM_UPDATE_CHECKOUT };
    +-
    +-	if (argc < 3 || argc > 4)
    +-		die("submodule--helper update-module-clone expects <just-cloned> <path> [<update>]");
    +-
    +-	just_cloned = git_config_int("just_cloned", argv[1]);
    +-	path = argv[2];
    +-
    +-	if (argc == 4)
    +-		update = argv[3];
    +-
    +-	determine_submodule_update_strategy(the_repository,
    +-					    just_cloned, path, update,
    +-					    &update_strategy);
    +-	fputs(submodule_strategy_to_string(&update_strategy), stdout);
    +-
    +-	return 0;
    +-}
    +-
    + struct update_clone_data {
    + 	const struct submodule *sub;
    + 	struct object_id oid;
     @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_data *ud, struct string_list *e
      	return run_update_command(ud, subforce, err);
      }
    @@ builtin/submodule--helper.c: static int do_run_update_procedure(struct update_da
     -
     -	return 3;
     -}
    +-
    +-static int resolve_relative_path(int argc, const char **argv, const char *prefix)
    +-{
    +-	struct strbuf sb = STRBUF_INIT;
    +-	if (argc != 3)
    +-		die("submodule--helper relative-path takes exactly 2 arguments, got %d", argc);
    +-
    +-	printf("%s", relative_path(argv[1], argv[2], &sb));
    +-	strbuf_release(&sb);
    +-	return 0;
    +-}
     -
      static const char *remote_submodule_branch(const char *path)
      {
      	const struct submodule *sub;
    -@@ builtin/submodule--helper.c: static int update_submodule(struct update_data *update_data)
    +@@ builtin/submodule--helper.c: static void do_ensure_core_worktree(const char *path)
      	}
    + }
      
    - 	if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force)
    --		if (do_run_update_procedure(update_data, &err))
    -+		if (run_update_procedure(update_data, &err))
    - 			return 1;
    - 
    - 	if (update_data->recursive) {
    +-static int ensure_core_worktree(int argc, const char **argv, const char *prefix)
    +-{
    +-	const char *path;
    +-
    +-	if (argc != 2)
    +-		BUG("submodule--helper ensure-core-worktree <path>");
    +-
    +-	path = argv[1];
    +-	do_ensure_core_worktree(path);
    +-
    +-	return 0;
    +-}
    +-
    + static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
    + {
    + 	int i;
     @@ builtin/submodule--helper.c: static struct cmd_struct commands[] = {
      	{"clone", module_clone, 0},
      	{"add-clone", add_clone, 0},
      	{"update", module_update, 0},
    +-	{"update-module-mode", module_update_module_mode, 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},
      	{"resolve-relative-url-test", resolve_relative_url_test, 0},
      	{"foreach", module_foreach, SUPPORT_SUPER_PREFIX},
    + 	{"init", module_init, SUPPORT_SUPER_PREFIX},
    + 	{"status", module_status, SUPPORT_SUPER_PREFIX},
    +-	{"print-default-remote", print_default_remote, 0},
    + 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
    + 	{"deinit", module_deinit, 0},
    + 	{"summary", module_summary, SUPPORT_SUPER_PREFIX},
 -:  ---------- >  8:  ca48dd452c submodule--helper: rename helper functions