diff mbox series

[v3,08/13] submodule--helper run-update-procedure: learn --remote

Message ID 20220303005727.69270-9-chooglen@google.com (mailing list archive)
State Superseded
Headers show
Series submodule: convert parts of 'update' to C | expand

Commit Message

Glen Choo March 3, 2022, 12:57 a.m. UTC 
Teach run-update-procedure to handle --remote instead of parsing
--remote in git-submodule.sh. As a result, "git submodule--helper
[print-default-remote|remote-branch]" have no more callers, so remove
them.

Signed-off-by: Glen Choo <chooglen@google.com>
---
 builtin/submodule--helper.c | 56 +++++++++++++++----------------------
 git-submodule.sh            | 30 +-------------------
 2 files changed, 23 insertions(+), 63 deletions(-)

Comments

Junio C Hamano March 3, 2022, 9:35 p.m. UTC | #1 
Glen Choo <chooglen@google.com> writes:

> @@ -3033,6 +3004,25 @@ static int update_submodule2(struct update_data *update_data)
>  		die(_("Unable to find current revision in submodule path '%s'"),
>  			update_data->displaypath);
>  
> +	if (update_data->remote) {
> +		char *remote_name = get_default_remote_submodule(update_data->sm_path);
> +		const char *branch = remote_submodule_branch(update_data->sm_path);
> +		char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
> +
> +		if (!update_data->nofetch) {
> +			if (fetch_in_submodule(update_data->sm_path, update_data->depth,
> +					      0, NULL))
> +				die(_("Unable to fetch in submodule path '%s'"),
> +				    update_data->sm_path);
> +		}
> +
> +		if (resolve_gitlink_ref(update_data->sm_path, remote_ref, &update_data->oid))
> +			die(_("Unable to find %s revision in submodule path '%s'"),
> +			    remote_ref, update_data->sm_path);
> +
> +		free(remote_ref);
> +	}

This, and ...

> @@ -409,21 +395,6 @@ cmd_update()
>  			just_cloned=
>  		fi
>  
> -		if test -n "$remote"
> -		then
> -			branch=$(git submodule--helper remote-branch "$sm_path")
> -			if test -z "$nofetch"
> -			then
> -				# Fetch remote before determining tracking $sha1
> -				fetch_in_submodule "$sm_path" $depth ||
> -				die "fatal: $(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
> -			fi
> -			remote_name=$(sanitize_submodule_env; cd "$sm_path" && git submodule--helper print-default-remote)
> -			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
> -				git rev-parse --verify "${remote_name}/${branch}") ||
> -			die "fatal: $(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
> -		fi

... this change would "fix" the temporary breakage [05/13] may have
brought us.  From the series structure's point of view, doing
[05/13] after this would be more sensible.  If we leave the call to
"displaypath" still in the script after this series, [05/13] or its
equivalent should mention that ensure-core-worktree does not matter
for that particular call when it delays the call to it by moving it
to the beginning of the update_submodule2() from the much earlier
part of the script.

>  		out=$(git submodule--helper run-update-procedure \
>  			  ${wt_prefix:+--prefix "$wt_prefix"} \
>  			  ${GIT_QUIET:+--quiet} \
> @@ -434,6 +405,7 @@ cmd_update()
>  			  ${update:+--update "$update"} \
>  			  ${prefix:+--recursive-prefix "$prefix"} \
>  			  ${sha1:+--oid "$sha1"} \
> +			  ${remote:+--remote} \
>  			  "--" \
>  			  "$sm_path")
Glen Choo March 4, 2022, 9:29 p.m. UTC | #2 
Junio C Hamano <gitster@pobox.com> writes:

> Glen Choo <chooglen@google.com> writes:
>
>> @@ -3033,6 +3004,25 @@ static int update_submodule2(struct update_data *update_data)
>>  		die(_("Unable to find current revision in submodule path '%s'"),
>>  			update_data->displaypath);
>>  
>> +	if (update_data->remote) {
>> +		char *remote_name = get_default_remote_submodule(update_data->sm_path);
>> +		const char *branch = remote_submodule_branch(update_data->sm_path);
>> +		char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
>> +
>> +		if (!update_data->nofetch) {
>> +			if (fetch_in_submodule(update_data->sm_path, update_data->depth,
>> +					      0, NULL))
>> +				die(_("Unable to fetch in submodule path '%s'"),
>> +				    update_data->sm_path);
>> +		}
>> +
>> +		if (resolve_gitlink_ref(update_data->sm_path, remote_ref, &update_data->oid))
>> +			die(_("Unable to find %s revision in submodule path '%s'"),
>> +			    remote_ref, update_data->sm_path);
>> +
>> +		free(remote_ref);
>> +	}
>
> This, and ...
>
>> @@ -409,21 +395,6 @@ cmd_update()
>>  			just_cloned=
>>  		fi
>>  
>> -		if test -n "$remote"
>> -		then
>> -			branch=$(git submodule--helper remote-branch "$sm_path")
>> -			if test -z "$nofetch"
>> -			then
>> -				# Fetch remote before determining tracking $sha1
>> -				fetch_in_submodule "$sm_path" $depth ||
>> -				die "fatal: $(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
>> -			fi
>> -			remote_name=$(sanitize_submodule_env; cd "$sm_path" && git submodule--helper print-default-remote)
>> -			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
>> -				git rev-parse --verify "${remote_name}/${branch}") ||
>> -			die "fatal: $(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
>> -		fi
>
> ... this change would "fix" the temporary breakage [05/13] may have
> brought us.  From the series structure's point of view, doing
> [05/13] after this would be more sensible.  If we leave the call to
> "displaypath" still in the script after this series, [05/13] or its
> equivalent should mention that ensure-core-worktree does not matter
> for that particular call when it delays the call to it by moving it
> to the beginning of the update_submodule2() from the much earlier
> part of the script.
>

I should've read this in full before sending my reply on [05/13]. Makes
sense, thanks for the pointer :)

>>  		out=$(git submodule--helper run-update-procedure \
>>  			  ${wt_prefix:+--prefix "$wt_prefix"} \
>>  			  ${GIT_QUIET:+--quiet} \
>> @@ -434,6 +405,7 @@ cmd_update()
>>  			  ${update:+--update "$update"} \
>>  			  ${prefix:+--recursive-prefix "$prefix"} \
>>  			  ${sha1:+--oid "$sha1"} \
>> +			  ${remote:+--remote} \
>>  			  "--" \
>>  			  "$sm_path")
diff mbox series

Patch

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 3a96c35b86..99341fb343 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -72,21 +72,6 @@  static char *get_default_remote(void)
 	return repo_get_default_remote(the_repository);
 }
 
-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)
 {
 	return str[0] == '.' && is_dir_sep(str[1]);
@@ -2027,6 +2012,7 @@  struct update_data {
 	unsigned int quiet;
 	unsigned int nofetch;
 	unsigned int just_cloned;
+	unsigned int remote;
 };
 #define UPDATE_DATA_INIT { .update_strategy = SUBMODULE_UPDATE_STRATEGY_INIT }
 
@@ -2603,6 +2589,8 @@  static int run_update_procedure(int argc, const char **argv, const char *prefix)
 		OPT_CALLBACK_F(0, "oid", &update_data.oid, N_("sha1"),
 			       N_("SHA1 expected by superproject"), PARSE_OPT_NONEG,
 			       parse_opt_object_id),
+		OPT_BOOL(0, "remote", &update_data.remote,
+			 N_("use SHA-1 of submodule's remote tracking branch")),
 		OPT_END()
 	};
 
@@ -2682,23 +2670,6 @@  static const char *remote_submodule_branch(const char *path)
 	return branch;
 }
 
-static int resolve_remote_submodule_branch(int argc, const char **argv,
-		const char *prefix)
-{
-	const char *ret;
-	struct strbuf sb = STRBUF_INIT;
-	if (argc != 2)
-		die("submodule--helper remote-branch takes exactly one arguments, got %d", argc);
-
-	ret = remote_submodule_branch(argv[1]);
-	if (!ret)
-		die("submodule %s doesn't exist", argv[1]);
-
-	printf("%s", ret);
-	strbuf_release(&sb);
-	return 0;
-}
-
 static int push_check(int argc, const char **argv, const char *prefix)
 {
 	struct remote *remote;
@@ -3033,6 +3004,25 @@  static int update_submodule2(struct update_data *update_data)
 		die(_("Unable to find current revision in submodule path '%s'"),
 			update_data->displaypath);
 
+	if (update_data->remote) {
+		char *remote_name = get_default_remote_submodule(update_data->sm_path);
+		const char *branch = remote_submodule_branch(update_data->sm_path);
+		char *remote_ref = xstrfmt("refs/remotes/%s/%s", remote_name, branch);
+
+		if (!update_data->nofetch) {
+			if (fetch_in_submodule(update_data->sm_path, update_data->depth,
+					      0, NULL))
+				die(_("Unable to fetch in submodule path '%s'"),
+				    update_data->sm_path);
+		}
+
+		if (resolve_gitlink_ref(update_data->sm_path, remote_ref, &update_data->oid))
+			die(_("Unable to find %s revision in submodule path '%s'"),
+			    remote_ref, update_data->sm_path);
+
+		free(remote_ref);
+	}
+
 	if (!oideq(&update_data->oid, &update_data->suboid) || update_data->force)
 		return do_run_update_procedure(update_data);
 
@@ -3431,11 +3421,9 @@  static struct cmd_struct commands[] = {
 	{"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},
-	{"remote-branch", resolve_remote_submodule_branch, 0},
 	{"push-check", push_check, 0},
 	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
 	{"is-active", is_active, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 458ce73ac6..23ebd90892 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -247,20 +247,6 @@  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
 #
@@ -409,21 +395,6 @@  cmd_update()
 			just_cloned=
 		fi
 
-		if test -n "$remote"
-		then
-			branch=$(git submodule--helper remote-branch "$sm_path")
-			if test -z "$nofetch"
-			then
-				# Fetch remote before determining tracking $sha1
-				fetch_in_submodule "$sm_path" $depth ||
-				die "fatal: $(eval_gettext "Unable to fetch in submodule path '\$sm_path'")"
-			fi
-			remote_name=$(sanitize_submodule_env; cd "$sm_path" && git submodule--helper print-default-remote)
-			sha1=$(sanitize_submodule_env; cd "$sm_path" &&
-				git rev-parse --verify "${remote_name}/${branch}") ||
-			die "fatal: $(eval_gettext "Unable to find current \${remote_name}/\${branch} revision in submodule path '\$sm_path'")"
-		fi
-
 		out=$(git submodule--helper run-update-procedure \
 			  ${wt_prefix:+--prefix "$wt_prefix"} \
 			  ${GIT_QUIET:+--quiet} \
@@ -434,6 +405,7 @@  cmd_update()
 			  ${update:+--update "$update"} \
 			  ${prefix:+--recursive-prefix "$prefix"} \
 			  ${sha1:+--oid "$sha1"} \
+			  ${remote:+--remote} \
 			  "--" \
 			  "$sm_path")