git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to C

From
BWBrandon Williams <bmwill@google.com>
Date
Jul 24, 2017, 21:52 UTC
Message-ID
<20170724215220.GC92874@google.com>
In-Reply-To
<20170724203454.13947-6-pc44800@gmail.com>
On 07/25, Prathamesh Chavan wrote:
Show 43 quoted lines
> Port the submodule subcommand 'sync' from shell to C using the same
> mechanism as that used for porting submodule subcommand 'status'.
> Hence, here the function cmd_sync() is ported from shell to C.
> This is done by introducing three functions: module_sync(),
> sync_submodule() and print_default_remote().
> 
> The function print_default_remote() is introduced for getting
> the default remote as stdout.
> 
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
> In this new version of patch, following changes were made:
> * the code use to die when sub->url was found to be NULL. This was not a
>   correct translation of code. It was corrected by using an empty string
>   instead of sub->url.
> * a process was used in the previous patch for registering the submodule
>   url. This was avoided by the suggested changes on the previous patch.
> * some nits were also corrected.
>  
>  builtin/submodule--helper.c | 183 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  56 +-------------
>  2 files changed, 184 insertions(+), 55 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index b39828174..2d1d3984d 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -44,6 +44,20 @@ static char *get_default_remote(void)
>  	return ret;
>  }
>  
> +static int print_default_remote(int argc, const char **argv, const char *prefix)
> +{
> +	const char *remote;
> +
> +	if (argc != 1)
> +		die(_("submodule--helper print-default-remote takes no arguments"));
> +
> +	remote = get_default_remote();
> +	if (remote)
> +		puts(remote);
Any reason why puts is used instead of a printf function?
Show 53 quoted lines
> +
> +	return 0;
> +}
> +
>  static int starts_with_dot_slash(const char *str)
>  {
>  	return str[0] == '.' && is_dir_sep(str[1]);
> @@ -379,6 +393,25 @@ static void module_list_active(struct module_list *list)
>  	*list = active_modules;
>  }
>  
> +static char *get_up_path(const char *path)
> +{
> +	int i;
> +	struct strbuf sb = STRBUF_INIT;
> +
> +	for (i = count_slashes(path); i; i--)
> +		strbuf_addstr(&sb, "../");
> +
> +	/*
> +	 * Check if 'path' ends with slash or not
> +	 * for having the same output for dir/sub_dir
> +	 * and dir/sub_dir/
> +	 */
> +	if (!is_dir_sep(path[strlen(path) - 1]))
> +		strbuf_addstr(&sb, "../");
> +
> +	return strbuf_detach(&sb, NULL);
> +}
> +
>  static int module_list(int argc, const char **argv, const char *prefix)
>  {
>  	int i;
> @@ -729,6 +762,154 @@ static int module_name(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct sync_cb {
> +	const char *prefix;
> +	unsigned int quiet: 1;
> +	unsigned int recursive: 1;
> +};
> +#define SYNC_CB_INIT { NULL, 0, 0 }
> +
> +static void sync_submodule(const struct cache_entry *list_item, void *cb_data)
> +{
> +	struct sync_cb *info = cb_data;
> +	const struct submodule *sub;
> +	char *sub_key, *remote_key;
> +	char *sub_origin_url, *super_config_url, *displaypath;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	struct strbuf sub_repo_path = STRBUF_INIT;

Not the most important but you could be a lot more efficient here with your variables. You could allocate a single strbuf and reuse it for 'sub_key'. 'remote_key', 'sb', and 'sub_repo_path' as it doesn't look like you need any of those two at the same time.

Show 6 quoted lines
> +	char *sub_config_path = NULL;
> +
> +	if (!is_submodule_active(the_repository, list_item->name))
> +		return;
> +
> +	sub = submodule_from_path(null_sha1, list_item->name);

Since is_submodule_active also calls into the submodule-config subsystem to retrieve a submodule this call should never return a NULL ptr...so it may be safer to return instead of proceeding if 'sub' ends up being null here.

Show 56 quoted lines
> +
> +	if (sub && sub->url) {
> +		if (starts_with_dot_dot_slash(sub->url) || starts_with_dot_slash(sub->url)) {
> +			char *remote_url, *up_path;
> +			char *remote = get_default_remote();
> +			char *remote_key = xstrfmt("remote.%s.url", remote);
> +
> +			if (git_config_get_string(remote_key, &remote_url))
> +				remote_url = xgetcwd();
> +
> +			up_path = get_up_path(list_item->name);
> +			sub_origin_url = relative_url(remote_url, sub->url, up_path);
> +			super_config_url = relative_url(remote_url, sub->url, NULL);
> +
> +			free(remote);
> +			free(remote_key);
> +			free(up_path);
> +			free(remote_url);
> +		} else {
> +			sub_origin_url = xstrdup(sub->url);
> +			super_config_url = xstrdup(sub->url);
> +		}
> +	} else {
> +		sub_origin_url = "";
> +		super_config_url = "";
> +	}
> +
> +	displaypath = get_submodule_displaypath(list_item->name, info->prefix);
> +
> +	if (!info->quiet)
> +		printf(_("Synchronizing submodule url for '%s'\n"),
> +			 displaypath);
> +
> +	sub_key = xstrfmt("submodule.%s.url", sub->name);
> +	if (git_config_set_gently(sub_key, super_config_url))
> +		die(_("failed to register url for submodule path '%s'"),
> +		      displaypath);
> +
> +	if (!is_submodule_populated_gently(list_item->name, NULL))
> +		goto cleanup;
> +
> +	prepare_submodule_repo_env(&cp.env_array);
> +	cp.git_cmd = 1;
> +	cp.dir = list_item->name;
> +	argv_array_pushl(&cp.args, "submodule--helper",
> +			 "print-default-remote", NULL);
> +
> +	if (capture_command(&cp, &sb, 0))
> +		die(_("failed to get the default remote for submodule '%s'"),
> +		      list_item->name);
> +
> +	strbuf_strip_suffix(&sb, "\n");
> +	remote_key = xstrfmt("remote.%s.url", sb.buf);
> +	strbuf_release(&sb);
> +
> +	submodule_to_gitdir(&sub_repo_path, list_item->name);

This function (submodule_to_gitdir) has some poor characteristics in that it will succeed even if there isn't a configured submodule but there just happens to be a submodule at the provided path...but it looks like none of them will affect this as the fist thing this function does is check if the submodule is active before preceding.

Show 15 quoted lines
> +	sub_config_path = xstrfmt("%s/config", sub_repo_path.buf);
> +	strbuf_release(&sub_repo_path);
> +
> +	if (git_config_set_in_file_gently(sub_config_path, remote_key, sub_origin_url))
> +		die(_("failed to update remote for submodule '%s'"),
> +		      list_item->name);
> +
> +	if (info->recursive) {
> +		struct child_process cpr = CHILD_PROCESS_INIT;
> +
> +		cpr.git_cmd = 1;
> +		cpr.dir = list_item->name;
> +		prepare_submodule_repo_env(&cpr.env_array);
> +
> +		argv_array_pushl(&cpr.args, "--super-prefix", displaypath,
Same comment as the previous patch here.
Show 139 quoted lines
> +				 "submodule--helper", "sync", "--recursive",
> +				 NULL);
> +
> +		if (info->quiet)
> +			argv_array_push(&cpr.args, "--quiet");
> +
> +		if (run_command(&cpr))
> +			die(_("failed to recurse into submodule '%s'"),
> +			      list_item->name);
> +	}
> +
> +cleanup:
> +	free(sub_key);
> +	free(super_config_url);
> +	free(displaypath);
> +	free(sub_config_path);
> +	free(sub_origin_url);
> +}
> +
> +static int module_sync(int argc, const char **argv, const char *prefix)
> +{
> +	struct sync_cb info = SYNC_CB_INIT;
> +	struct pathspec pathspec;
> +	struct module_list list = MODULE_LIST_INIT;
> +	int quiet = 0;
> +	int recursive = 0;
> +
> +	struct option module_sync_options[] = {
> +		OPT__QUIET(&quiet, N_("Suppress output of synchronizing submodule url")),
> +		OPT_BOOL(0, "recursive", &recursive,
> +			N_("Recurse into nested submodules")),
> +		OPT_END()
> +	};
> +
> +	const char *const git_submodule_helper_usage[] = {
> +		N_("git submodule--helper sync [--quiet] [--recursive] [<path>]"),
> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, module_sync_options,
> +			     git_submodule_helper_usage, 0);
> +
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +		return 1;
> +
> +	info.prefix = prefix;
> +	info.quiet = !!quiet;
> +	info.recursive = !!recursive;
> +
> +	gitmodules_config();
> +	for_each_submodule_list(list, sync_submodule, &info);
> +
> +	return 0;
> +}
> +
>  static int clone_submodule(const char *path, const char *gitdir, const char *url,
>  			   const char *depth, struct string_list *reference,
>  			   int quiet, int progress)
> @@ -1457,6 +1638,8 @@ static struct cmd_struct commands[] = {
>  	{"print-name-rev", print_name_rev, 0},
>  	{"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},
>  	{"remote-branch", resolve_remote_submodule_branch, 0},
>  	{"push-check", push_check, 0},
>  	{"absorb-git-dirs", absorb_git_dirs, SUPPORT_SUPER_PREFIX},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 51b057d82..6bfc5e17d 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -1037,63 +1037,9 @@ cmd_sync()
>  			;;
>  		esac
>  	done
> -	cd_to_toplevel
> -	{
> -		git submodule--helper list --prefix "$wt_prefix" "$@" ||
> -		echo "#unmatched" $?
> -	} |
> -	while read -r mode sha1 stage sm_path
> -	do
> -		die_if_unmatched "$mode" "$sha1"
> -
> -		# skip inactive submodules
> -		if ! git submodule--helper is-active "$sm_path"
> -		then
> -			continue
> -		fi
> -
> -		name=$(git submodule--helper name "$sm_path")
> -		url=$(git config -f .gitmodules --get submodule."$name".url)
> -
> -		# Possibly a url relative to parent
> -		case "$url" in
> -		./*|../*)
> -			# rewrite foo/bar as ../.. to find path from
> -			# submodule work tree to superproject work tree
> -			up_path="$(printf '%s\n' "$sm_path" | sed "s/[^/][^/]*/../g")" &&
> -			# guarantee a trailing /
> -			up_path=${up_path%/}/ &&
> -			# path from submodule work tree to submodule origin repo
> -			sub_origin_url=$(git submodule--helper resolve-relative-url "$url" "$up_path") &&
> -			# path from superproject work tree to submodule origin repo
> -			super_config_url=$(git submodule--helper resolve-relative-url "$url") || exit
> -			;;
> -		*)
> -			sub_origin_url="$url"
> -			super_config_url="$url"
> -			;;
> -		esac
>  
> -		displaypath=$(git submodule--helper relative-path "$prefix$sm_path" "$wt_prefix")
> -		say "$(eval_gettext "Synchronizing submodule url for '\$displaypath'")"
> -		git config submodule."$name".url "$super_config_url"
> -
> -		if test -e "$sm_path"/.git
> -		then
> -		(
> -			sanitize_submodule_env
> -			cd "$sm_path"
> -			remote=$(get_default_remote)
> -			git config remote."$remote".url "$sub_origin_url"
> +	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
>  
> -			if test -n "$recursive"
> -			then
> -				prefix="$prefix$sm_path/"
> -				eval cmd_sync
> -			fi
> -		)
> -		fi
> -	done
>  }
>  
>  cmd_absorbgitdirs()
> -- 
> 2.13.0
> 
-- 
Brandon Williams
Previous: Prathamesh ChavanNext: Prathamesh Chavan
Message 9 of 41 in “Update: Week 10”
  1. Prathamesh ChavanJul 24, 2017
  2. [GSoC][PATCH 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 24, 2017
  3. [GSoC][PATCH 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 24, 2017
  4. [GSoC][PATCH 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 24, 2017
  5. Brandon WilliamsJul 24, 2017
  6. [GSoC][PATCH 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 24, 2017
  7. Brandon WilliamsJul 24, 2017
  8. [GSoC][PATCH 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 24, 2017
  9. Brandon WilliamsJul 24, 2017
  10. [GSoC][PATCH 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 24, 2017
  11. Brandon WilliamsJul 24, 2017
  12. [GSoC][PATCH 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 24, 2017
  13. [GSoC][PATCH 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 24, 2017
  14. Brandon WilliamsJul 25, 2017
  15. [GSoC][PATCH 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 24, 2017
  16. Brandon WilliamsJul 25, 2017
  17. [GSoC][PATCH 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 24, 2017
  18. Brandon WilliamsJul 25, 2017
  19. [GSoC][PATCH 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 24, 2017
  20. [GSoC][PATCH 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 24, 2017
  21. Brandon WilliamsJul 25, 2017
  22. [GSoC][PATCH 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 24, 2017
  23. Brandon WilliamsJul 25, 2017
  24. [GSoC][PATCH v2 00/13] Update: Week 10Prathamesh Chavan, Jul 29, 2017
  25. [GSoC][PATCH v2 01/13] submodule--helper: introduce get_submodule_displaypath()Prathamesh Chavan, Jul 29, 2017
  26. [GSoC][PATCH v2 02/13] submodule--helper: introduce for_each_submodule_list()Prathamesh Chavan, Jul 29, 2017
  27. [GSoC][PATCH v2 03/13] submodule: port set_name_rev() from shell to CPrathamesh Chavan, Jul 29, 2017
  28. [GSoC][PATCH v2 04/13] submodule: port submodule subcommand 'status' from shell to CPrathamesh Chavan, Jul 29, 2017
  29. Christian CouderJul 30, 2017
  30. [GSoC][PATCH v2 05/13] submodule: port submodule subcommand 'sync' from shell to CPrathamesh Chavan, Jul 29, 2017
  31. [GSoC][PATCH v2 06/13] submodule: port submodule subcommand 'deinit' from shell to CPrathamesh Chavan, Jul 29, 2017
  32. [GSoC][PATCH v2 07/13] diff: change scope of the function count_lines()Prathamesh Chavan, Jul 29, 2017
  33. [GSoC][PATCH v2 08/13] submodule: port submodule subcommand 'summary' from shell to CPrathamesh Chavan, Jul 29, 2017
  34. Christian CouderJul 30, 2017
  35. Prathamesh ChavanJul 30, 2017
  36. [GSoC][PATCH v2 09/13] submodule foreach: correct '$path' in nested submodules from a subdirectoryPrathamesh Chavan, Jul 29, 2017
  37. [GSoC][PATCH v2 10/13] submodule foreach: document '$sm_path' instead of '$path'Prathamesh Chavan, Jul 29, 2017
  38. [GSoC][PATCH v2 11/13] submodule foreach: clarify the '$toplevel' variable documentationPrathamesh Chavan, Jul 29, 2017
  39. [GSoC][PATCH v2 12/13] submodule foreach: document variable '$displaypath'Prathamesh Chavan, Jul 29, 2017
  40. [GSoC][PATCH v2 13/13] submodule: port submodule subcommand 'foreach' from shell to CPrathamesh Chavan, Jul 29, 2017
  41. Brandon WilliamsJul 31, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.