[PATCH v1 0/2] Incremental rewrite of git-submodules

[PATCH v1 0/2] Incremental rewrite of git-submodules

[Prathamesh Chavan]

The patches [1] and [2] concerning the porting of submodule
subcommands: sync and deinit were updated in accoudance with
the changes made in one of such similar portings made earlier
for submodule subcommand status[3]. Following are the changes
made:

* It was observed that the number of params increased a lot due to flags
  like quiet, recursive, cached, etc, and keeping in mind the future
  subcommand's ported functions as well, a single unsigned int called
  flags was introduced to store all of these flags, instead of having
  parameter for each one.

* To accomodate the possiblity of a direct call to the functions
  deinit_submodule() and sync_submodule(), callback functions were
  introduced.

As before you can find this series at: 
https://github.com/pratham-pc/git/commits/patch-series-2

And its build report is available at: 
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-2
Build #195

[1]: https://public-inbox.org/git/20170807211900.15001-6-pc44800@gmail.com/
[2]: https://public-inbox.org/git/20170807211900.15001-7-pc44800@gmail.com/
[3]: https://public-inbox.org/git/20171006132415.2876-4-pc44800@gmail.com/

Prathamesh Chavan (2):
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C

 builtin/submodule--helper.c | 345 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 112 +-------------
 2 files changed, 347 insertions(+), 110 deletions(-)

-- 
2.14.2

[PATCH v1 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

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 four functions: module_sync(),
sync_submodule(), sync_submodule_cb() 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>
---
 builtin/submodule--helper.c | 192 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  57 +------------
 2 files changed, 193 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..dd7737acd 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -50,6 +50,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)
+		printf("%s\n", remote);
+
+	return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
 	return str[0] == '.' && is_dir_sep(str[1]);
@@ -358,6 +372,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;
@@ -718,6 +751,163 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct sync_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+
+#define SYNC_CB_INIT { NULL, 0 }
+
+static void sync_submodule(const char *path, const char *prefix,
+			   unsigned int flags)
+{
+	const struct submodule *sub;
+	char *remote_key = NULL;
+	char *sub_origin_url, *super_config_url, *displaypath;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *sub_config_path = NULL;
+
+	if (!is_submodule_active(the_repository, path))
+		return;
+
+	sub = submodule_from_path(&null_oid, path);
+
+	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();
+			strbuf_addf(&sb, "remote.%s.url", remote);
+
+			if (git_config_get_string(sb.buf, &remote_url))
+				remote_url = xgetcwd();
+
+			up_path = get_up_path(path);
+			sub_origin_url = relative_url(remote_url, sub->url, up_path);
+			super_config_url = relative_url(remote_url, sub->url, NULL);
+
+			free(remote);
+			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(path, prefix);
+
+	if (!(flags & OPT_QUIET))
+		printf(_("Synchronizing submodule url for '%s'\n"),
+			 displaypath);
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (git_config_set_gently(sb.buf, super_config_url))
+		die(_("failed to register url for submodule path '%s'"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.dir = path;
+	argv_array_pushl(&cp.args, "submodule--helper",
+			 "print-default-remote", NULL);
+
+	strbuf_reset(&sb);
+	if (capture_command(&cp, &sb, 0))
+		die(_("failed to get the default remote for submodule '%s'"),
+		      path);
+
+	strbuf_strip_suffix(&sb, "\n");
+	remote_key = xstrfmt("remote.%s.url", sb.buf);
+
+	strbuf_reset(&sb);
+	submodule_to_gitdir(&sb, path);
+	strbuf_addstr(&sb, "/config");
+
+	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+		die(_("failed to update remote for submodule '%s'"),
+		      path);
+
+	if (flags & OPT_RECURSIVE) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_push(&cpr.args, "--super-prefix");
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "sync",
+				 "--recursive", NULL);
+
+		if (flags & OPT_QUIET)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      path);
+	}
+
+cleanup:
+	strbuf_release(&sb);
+	free(remote_key);
+	free(super_config_url);
+	free(displaypath);
+	free(sub_config_path);
+	free(sub_origin_url);
+}
+
+static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data)
+{
+	struct sync_cb *info = cb_data;
+	sync_submodule(list_item->name, info->prefix, info->flags);
+
+}
+
+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;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (recursive)
+		info.flags |= OPT_RECURSIVE;
+
+	for_each_listed_submodule(&list, sync_submodule_cb, &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)
@@ -1498,6 +1688,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 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 156255a9e..0825cae14 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,63 +1036,8 @@ 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"
-
-			if test -n "$recursive"
-			then
-				prefix="$prefix$sm_path/"
-				eval cmd_sync
-			fi
-		)
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 cmd_absorbgitdirs()
-- 
2.14.2

[PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into four
functions: module_deinit(), for_each_listed_submodule(),
deinit_submodule() and deinit_submodule_cb().

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  55 +---------------
 2 files changed, 154 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index dd7737acd..54b0e46fc 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
 #define OPT_RECURSIVE (1 << 2)
+#define OPT_FORCE (1 << 3)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 				  void *cb_data);
@@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct deinit_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+#define DEINIT_CB_INIT { NULL, 0 }
+
+static void deinit_submodule(const char *path, const char *prefix,
+			     unsigned int flags)
+{
+	const struct submodule *sub;
+	char *displaypath = NULL;
+	struct child_process cp_config = CHILD_PROCESS_INIT;
+	struct strbuf sb_config = STRBUF_INIT;
+	char *sub_git_dir = xstrfmt("%s/.git", path);
+	mode_t mode = 0777;
+
+	sub = submodule_from_path(&null_oid, path);
+
+	if (!sub || !sub->name)
+		goto cleanup;
+
+	displaypath = get_submodule_displaypath(path, prefix);
+
+	/* remove the submodule work tree (unless the user already did it) */
+	if (is_directory(path)) {
+		struct stat st;
+		/*
+		 * protect submodules containing a .git directory
+		 * NEEDSWORK: automatically call absorbgitdirs before
+		 * warning/die.
+		 */
+		if (is_directory(sub_git_dir))
+			die(_("Submodule work tree '%s' contains a .git "
+			      "directory use 'rm -rf' if you really want "
+			      "to remove it including all of its history"),
+			      displaypath);
+
+		if (!(flags & OPT_FORCE)) {
+			struct child_process cp_rm = CHILD_PROCESS_INIT;
+			cp_rm.git_cmd = 1;
+			argv_array_pushl(&cp_rm.args, "rm", "-qn",
+					 path, NULL);
+
+			if (run_command(&cp_rm))
+				die(_("Submodule work tree '%s' contains local "
+				      "modifications; use '-f' to discard them"),
+				      displaypath);
+		}
+
+		if (!lstat(path, &st)) {
+			struct strbuf sb_rm = STRBUF_INIT;
+			const char *format;
+
+			strbuf_addstr(&sb_rm, path);
+
+			if (!remove_dir_recursively(&sb_rm, 0))
+				format = _("Cleared directory '%s'\n");
+			else
+				format = _("Could not remove submodule work tree '%s'\n");
+
+			if (!(flags & OPT_QUIET))
+				printf(format, displaypath);
+
+			mode = st.st_mode;
+
+			strbuf_release(&sb_rm);
+		}
+	}
+
+	if (mkdir(path, mode))
+		die_errno(_("could not create empty submodule directory %s"),
+		      displaypath);
+
+	cp_config.git_cmd = 1;
+	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
+	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
+
+	/* remove the .git/config entries (unless the user already did it) */
+	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
+		char *sub_key = xstrfmt("submodule.%s", sub->name);
+		/*
+		 * remove the whole section so we have a clean state when
+		 * the user later decides to init this submodule again
+		 */
+		git_config_rename_section_in_file(NULL, sub_key, NULL);
+		if (!(flags & OPT_QUIET))
+			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
+				 sub->name, sub->url, displaypath);
+		free(sub_key);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_git_dir);
+	strbuf_release(&sb_config);
+}
+
+static void deinit_submodule_cb(const struct cache_entry *list_item,
+				void *cb_data)
+{
+	struct deinit_cb *info = cb_data;
+	deinit_submodule(list_item->name, info->prefix, info->flags);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+	struct deinit_cb info = DEINIT_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int force = 0;
+	int all = 0;
+
+	struct option module_deinit_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT__FORCE(&force, N_("Remove submodule working trees even if they contain local changes")),
+		OPT_BOOL(0, "all", &all, N_("Unregister all submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_deinit_options,
+			     git_submodule_helper_usage, 0);
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.prefix = prefix;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (force)
+		info.flags |= OPT_FORCE;
+
+	if (all && argc) {
+		error("pathspec and --all are incompatible");
+		usage_with_options(git_submodule_helper_usage,
+				   module_deinit_options);
+	}
+
+	if (!argc && !all)
+		die(_("Use '--all' if you really want to deinitialize all submodules"));
+
+	for_each_listed_submodule(&list, deinit_submodule_cb, &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)
@@ -1690,6 +1842,7 @@ static struct cmd_struct commands[] = {
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
+	{"deinit", module_deinit, 0},
 	{"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 0825cae14..24914963c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -428,60 +428,7 @@ cmd_deinit()
 		shift
 	done
 
-	if test -n "$deinit_all" && test "$#" -ne 0
-	then
-		echo >&2 "$(eval_gettext "pathspec and --all are incompatible")"
-		usage
-	fi
-	if test $# = 0 && test -z "$deinit_all"
-	then
-		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
-	fi
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-
-		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
-
-		# Remove the submodule work tree (unless the user already did it)
-		if test -d "$sm_path"
-		then
-			# Protect submodules containing a .git directory
-			if test -d "$sm_path/.git"
-			then
-				die "$(eval_gettext "\
-Submodule work tree '\$displaypath' contains a .git directory
-(use 'rm -rf' if you really want to remove it including all of its history)")"
-			fi
-
-			if test -z "$force"
-			then
-				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
-			fi
-			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
-		fi
-
-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
-
-		# Remove the .git/config entries (unless the user already did it)
-		if test -n "$(git config --get-regexp submodule."$name\.")"
-		then
-			# Remove the whole section so we have a clean state when
-			# the user later decides to init this submodule again
-			url=$(git config submodule."$name".url)
-			git config --remove-section submodule."$name" 2>/dev/null &&
-			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
 }
 
 is_tip_reachable () (
-- 
2.14.2

Re: [PATCH v1 0/2] Incremental rewrite of git-submodules

[Stefan Beller]

On Tue, Jan 9, 2018 at 9:57 AM, Prathamesh Chavan <pc44800@gmail.com> wrote:
> The patches [1] and [2] concerning the porting of submodule
> subcommands: sync and deinit were updated in accoudance with
> the changes made in one of such similar portings made earlier
> for submodule subcommand status[3]. Following are the changes
> made:
>
> * It was observed that the number of params increased a lot due to flags
>   like quiet, recursive, cached, etc, and keeping in mind the future
>   subcommand's ported functions as well, a single unsigned int called
>   flags was introduced to store all of these flags, instead of having
>   parameter for each one.
>
> * To accomodate the possiblity of a direct call to the functions
>   deinit_submodule() and sync_submodule(), callback functions were
>   introduced.
>
> As before you can find this series at:
> https://github.com/pratham-pc/git/commits/patch-series-2
>
> And its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: patch-series-2
> Build #195

Cool!

I have reviewed both patches and found them good to apply;

Thanks,
Stefan

Re: [PATCH v1 0/2] Incremental rewrite of git-submodules

[Brandon Williams]

On 01/09, Prathamesh Chavan wrote:
> The patches [1] and [2] concerning the porting of submodule
> subcommands: sync and deinit were updated in accoudance with
> the changes made in one of such similar portings made earlier
> for submodule subcommand status[3]. Following are the changes
> made:

The two patches look good to me.  Thanks for continuing this work!

> 
> * It was observed that the number of params increased a lot due to flags
>   like quiet, recursive, cached, etc, and keeping in mind the future
>   subcommand's ported functions as well, a single unsigned int called
>   flags was introduced to store all of these flags, instead of having
>   parameter for each one.

This is unfortunate.  The use of a flag word or using bit-fields are
essentially equivalent so its unfortunate that the conversion to using
one or the other caused review churn.  My own preference would be to use
bit-fields ;)  I also noticed that the flags you are using start with
OPT_* which conflict with the parse-options namespace, sorry for not
catching this when a few of your older patches made it into master.
This isn't a big deal since no symbols look to collide so I am not
suggesting you change this since I would prefer to eliminate more
unnecessary review churn on this series.

> 
> * To accomodate the possiblity of a direct call to the functions
>   deinit_submodule() and sync_submodule(), callback functions were
>   introduced.
> 
> As before you can find this series at: 
> https://github.com/pratham-pc/git/commits/patch-series-2
> 
> And its build report is available at: 
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: patch-series-2
> Build #195
> 
> [1]: https://public-inbox.org/git/20170807211900.15001-6-pc44800@gmail.com/
> [2]: https://public-inbox.org/git/20170807211900.15001-7-pc44800@gmail.com/
> [3]: https://public-inbox.org/git/20171006132415.2876-4-pc44800@gmail.com/
> 
> Prathamesh Chavan (2):
>   submodule: port submodule subcommand 'sync' from shell to C
>   submodule: port submodule subcommand 'deinit' from shell to C
> 
>  builtin/submodule--helper.c | 345 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 112 +-------------
>  2 files changed, 347 insertions(+), 110 deletions(-)
> 
> -- 
> 2.14.2
> 

-- 
Brandon Williams

Re: [PATCH v1 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> +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)
> +		printf("%s\n", remote);
> +
> +	return 0;
> +}

This is called directly from main and return immediately after
printing, so a small leak of remote does not matter, I guess.

> +static void sync_submodule(const char *path, const char *prefix,
> +			   unsigned int flags)
> +{
> +	const struct submodule *sub;
> +	char *remote_key = NULL;
> +	char *sub_origin_url, *super_config_url, *displaypath;
> +	struct strbuf sb = STRBUF_INIT;
> +	struct child_process cp = CHILD_PROCESS_INIT;
> +	char *sub_config_path = NULL;
> +
> +	if (!is_submodule_active(the_repository, path))
> +		return;
> +
> +	sub = submodule_from_path(&null_oid, path);
> +
> +	if (sub && sub->url) {
> +		if (starts_with_dot_dot_slash(sub->url) || starts_with_dot_slash(sub->url)) {

Not a big deal, but other codepaths seem to fold this pattern into
two lines, i.e.

		if (starts_with_dot_dot_slash(sub->url) ||
		    starts_with_dot_slash(sub->url)) {

> +			sub_origin_url = relative_url(remote_url, sub->url, up_path);
> +			super_config_url = relative_url(remote_url, sub->url, NULL);

On this side, these two are allocated memory that need to be freed.

> +		} else {
> +			sub_origin_url = xstrdup(sub->url);
> +			super_config_url = xstrdup(sub->url);

This side as well.

> +		}
> +	} else {
> +		sub_origin_url = "";
> +		super_config_url = "";

But not these.  You have free() of these two at the end of this
function, which will break things.

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> The same mechanism is used even for porting this submodule
> subcommand, as used in the ported subcommands till now.
> The function cmd_deinit in split up after porting into four
> functions: module_deinit(), for_each_listed_submodule(),
> deinit_submodule() and deinit_submodule_cb().
>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Stefan Beller <sbeller@google.com>
> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  55 +---------------
>  2 files changed, 154 insertions(+), 54 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index dd7737acd..54b0e46fc 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -20,6 +20,7 @@
>  #define OPT_QUIET (1 << 0)
>  #define OPT_CACHED (1 << 1)
>  #define OPT_RECURSIVE (1 << 2)
> +#define OPT_FORCE (1 << 3)
>  
>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
>  				  void *cb_data);
> @@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +struct deinit_cb {
> +	const char *prefix;
> +	unsigned int flags;
> +};
> +#define DEINIT_CB_INIT { NULL, 0 }
> +
> +static void deinit_submodule(const char *path, const char *prefix,
> +			     unsigned int flags)
> +{
> +	const struct submodule *sub;
> +	char *displaypath = NULL;
> +	struct child_process cp_config = CHILD_PROCESS_INIT;
> +	struct strbuf sb_config = STRBUF_INIT;
> +	char *sub_git_dir = xstrfmt("%s/.git", path);
> +	mode_t mode = 0777;
> +
> +	sub = submodule_from_path(&null_oid, path);
> +
> +	if (!sub || !sub->name)
> +		goto cleanup;
> +
> +	displaypath = get_submodule_displaypath(path, prefix);
> +
> +	/* remove the submodule work tree (unless the user already did it) */
> +	if (is_directory(path)) {
> +		struct stat st;
> +		/*
> +		 * protect submodules containing a .git directory
> +		 * NEEDSWORK: automatically call absorbgitdirs before
> +		 * warning/die.
> +		 */

I guess that you mean "instead of dying, automatically call absorb
and (possibly) warn"?  That sounds like a sensible improvement.

> +		if (is_directory(sub_git_dir))
> +			die(_("Submodule work tree '%s' contains a .git "
> +			      "directory use 'rm -rf' if you really want "
> +			      "to remove it including all of its history"),

This changes the message text by removing () around "use ... history",
which I do not think you intended to do.

> +			      displaypath);
> +
> +		if (!(flags & OPT_FORCE)) {
> +			struct child_process cp_rm = CHILD_PROCESS_INIT;
> +			cp_rm.git_cmd = 1;
> +			argv_array_pushl(&cp_rm.args, "rm", "-qn",
> +					 path, NULL);
> +
> +			if (run_command(&cp_rm))
> +				die(_("Submodule work tree '%s' contains local "
> +				      "modifications; use '-f' to discard them"),
> +				      displaypath);
> +		}
> +
> +		if (!lstat(path, &st)) {

What is this if statement doing here?  It does not make sense,
especially without an 'else' clause on the other side, at least to
me.

At this point in the flow, the code has already determined that path
is a directory above before starting to check if it has ".git/"
immediately below it, or trying to run "git rm" in the dry run mode
to see if it yields an error, so at this point lstat() should
succeed (and would say it is a directory).  I would sort-of
understand it if this "if()" has an "else" clause to act on an
error, but that is not something the original does not do, so I am
not sure if it belongs to a "rewrite to C" patch.

> +			struct strbuf sb_rm = STRBUF_INIT;
> +			const char *format;
> +
> +			strbuf_addstr(&sb_rm, path);
> +
> +			if (!remove_dir_recursively(&sb_rm, 0))
> +				format = _("Cleared directory '%s'\n");
> +			else
> +				format = _("Could not remove submodule work tree '%s'\n");
> +
> +			if (!(flags & OPT_QUIET))
> +				printf(format, displaypath);
> +
> +			mode = st.st_mode;
> +
> +			strbuf_release(&sb_rm);
> +		}
> +	}

If the reason is "avoid losing the original directory mode by
removing and recreating", then you should be able to do much better
by using REMOVE_DIR_KEEP_TOPLEVEL in the above "do we still have a
directory?  if so get rid of working tree contents" thing.  And the
call to mkdir() below can be placed in the else clause of that
check, i.e. "the user has removed the directory as well, but there
should be an empty directory even for a de-initialized submodule"
side of this.

That of course does not have to be part of "rewrite to C" patch.  In
fact, it probably should come as a follow-up improvement after the
dust settles.

> +	if (mkdir(path, mode))
> +		die_errno(_("could not create empty submodule directory %s"),
> +		      displaypath);
> + ...
> +
> +static int module_deinit(int argc, const char **argv, const char *prefix)
> +{
> +...
> +	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
> +		BUG("module_list_compute should not choke on empty pathspec");
> +...
> +	if (all && argc) {
> +		error("pathspec and --all are incompatible");
> +		usage_with_options(git_submodule_helper_usage,
> +				   module_deinit_options);
> +	}
> +
> +	if (!argc && !all)
> +		die(_("Use '--all' if you really want to deinitialize all submodules"));

Shouldn't these two checks come before we call module_list_compute()?  

> +
> +	for_each_listed_submodule(&list, deinit_submodule_cb, &info);
> +
> +	return 0;
> +}

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

On Wed, Jan 10, 2018 at 2:54 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Prathamesh Chavan <pc44800@gmail.com> writes:
>
>> The same mechanism is used even for porting this submodule
>> subcommand, as used in the ported subcommands till now.
>> The function cmd_deinit in split up after porting into four
>> functions: module_deinit(), for_each_listed_submodule(),
>> deinit_submodule() and deinit_submodule_cb().
>>
>> Mentored-by: Christian Couder <christian.couder@gmail.com>
>> Mentored-by: Stefan Beller <sbeller@google.com>
>> Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
>> ---
>>  builtin/submodule--helper.c | 153 ++++++++++++++++++++++++++++++++++++++++++++
>>  git-submodule.sh            |  55 +---------------
>>  2 files changed, 154 insertions(+), 54 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index dd7737acd..54b0e46fc 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -20,6 +20,7 @@
>>  #define OPT_QUIET (1 << 0)
>>  #define OPT_CACHED (1 << 1)
>>  #define OPT_RECURSIVE (1 << 2)
>> +#define OPT_FORCE (1 << 3)
>>
>>  typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
>>                                 void *cb_data);
>> @@ -908,6 +909,157 @@ static int module_sync(int argc, const char **argv, const char *prefix)
>>       return 0;
>>  }
>>
>> +struct deinit_cb {
>> +     const char *prefix;
>> +     unsigned int flags;
>> +};
>> +#define DEINIT_CB_INIT { NULL, 0 }
>> +
>> +static void deinit_submodule(const char *path, const char *prefix,
>> +                          unsigned int flags)
>> +{
>> +     const struct submodule *sub;
>> +     char *displaypath = NULL;
>> +     struct child_process cp_config = CHILD_PROCESS_INIT;
>> +     struct strbuf sb_config = STRBUF_INIT;
>> +     char *sub_git_dir = xstrfmt("%s/.git", path);
>> +     mode_t mode = 0777;
>> +
>> +     sub = submodule_from_path(&null_oid, path);
>> +
>> +     if (!sub || !sub->name)
>> +             goto cleanup;
>> +
>> +     displaypath = get_submodule_displaypath(path, prefix);
>> +
>> +     /* remove the submodule work tree (unless the user already did it) */
>> +     if (is_directory(path)) {
>> +             struct stat st;
>> +             /*
>> +              * protect submodules containing a .git directory
>> +              * NEEDSWORK: automatically call absorbgitdirs before
>> +              * warning/die.
>> +              */
>
> I guess that you mean "instead of dying, automatically call absorb
> and (possibly) warn"?  That sounds like a sensible improvement.
>
>> +             if (is_directory(sub_git_dir))
>> +                     die(_("Submodule work tree '%s' contains a .git "
>> +                           "directory use 'rm -rf' if you really want "
>> +                           "to remove it including all of its history"),
>
> This changes the message text by removing () around "use ... history",
> which I do not think you intended to do.
>
>> +                           displaypath);
>> +
>> +             if (!(flags & OPT_FORCE)) {
>> +                     struct child_process cp_rm = CHILD_PROCESS_INIT;
>> +                     cp_rm.git_cmd = 1;
>> +                     argv_array_pushl(&cp_rm.args, "rm", "-qn",
>> +                                      path, NULL);
>> +
>> +                     if (run_command(&cp_rm))
>> +                             die(_("Submodule work tree '%s' contains local "
>> +                                   "modifications; use '-f' to discard them"),
>> +                                   displaypath);
>> +             }
>> +
>> +             if (!lstat(path, &st)) {
>
> What is this if statement doing here?  It does not make sense,
> especially without an 'else' clause on the other side, at least to
> me.
>
> At this point in the flow, the code has already determined that path
> is a directory above before starting to check if it has ".git/"
> immediately below it, or trying to run "git rm" in the dry run mode
> to see if it yields an error, so at this point lstat() should
> succeed (and would say it is a directory).  I would sort-of
> understand it if this "if()" has an "else" clause to act on an
> error, but that is not something the original does not do, so I am
> not sure if it belongs to a "rewrite to C" patch.
>
>> +                     struct strbuf sb_rm = STRBUF_INIT;
>> +                     const char *format;
>> +
>> +                     strbuf_addstr(&sb_rm, path);
>> +
>> +                     if (!remove_dir_recursively(&sb_rm, 0))
>> +                             format = _("Cleared directory '%s'\n");
>> +                     else
>> +                             format = _("Could not remove submodule work tree '%s'\n");
>> +
>> +                     if (!(flags & OPT_QUIET))
>> +                             printf(format, displaypath);
>> +
>> +                     mode = st.st_mode;
>> +
>> +                     strbuf_release(&sb_rm);
>> +             }
>> +     }
>
> If the reason is "avoid losing the original directory mode by
> removing and recreating", then you should be able to do much better
> by using REMOVE_DIR_KEEP_TOPLEVEL in the above "do we still have a
> directory?  if so get rid of working tree contents" thing.  And the
> call to mkdir() below can be placed in the else clause of that
> check, i.e. "the user has removed the directory as well, but there
> should be an empty directory even for a de-initialized submodule"
> side of this.
>
> That of course does not have to be part of "rewrite to C" patch.  In
> fact, it probably should come as a follow-up improvement after the
> dust settles.
>
Firstly, thanks a lot for taking time and reviewing the patches.
I have a few queries about the above changes to be made in the
"rewrite to C" patch.

Function lstat() was used for mainly getting the mode for the to be
created new directory.
And since sometimes st.st_mode may be containing garbage value, a new variable
mode was introduced with initial value 0777.

Thanks for pointing out that we can introduce the flag REMOVE_DIR_KEEP_TOPLEVEL
which solves the issue. And for the case where no directory exists: we
create an empty
directory.Since this won't be similar to what happens in the shell
script, this change
can be included in a saperate patch as an imporvement. But till the
dust settles, does the
current patch serve the purpose? (After imporving over the other
points being pointed above)

Thanks,
Prathamesh Chavan

>> +     if (mkdir(path, mode))
>> +             die_errno(_("could not create empty submodule directory %s"),
>> +                   displaypath);
>> + ...
>> +
>> +static int module_deinit(int argc, const char **argv, const char *prefix)
>> +{
>> +...
>> +     if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
>> +             BUG("module_list_compute should not choke on empty pathspec");
>> +...
>> +     if (all && argc) {
>> +             error("pathspec and --all are incompatible");
>> +             usage_with_options(git_submodule_helper_usage,
>> +                                module_deinit_options);
>> +     }
>> +
>> +     if (!argc && !all)
>> +             die(_("Use '--all' if you really want to deinitialize all submodules"));
>
> Shouldn't these two checks come before we call module_list_compute()?
>
>> +
>> +     for_each_listed_submodule(&list, deinit_submodule_cb, &info);
>> +
>> +     return 0;
>> +}

Re: [PATCH v1 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> Thanks for pointing out that we can introduce the flag REMOVE_DIR_KEEP_TOPLEVEL
> which solves the issue. And for the case where no directory exists: we
> create an empty
> directory.Since this won't be similar to what happens in the shell
> script, this change
> can be included in a saperate patch as an imporvement.

Exactly.  The way the shell script does it is to _always_ honor
user's umask and recreate the directory, so before that separate
improvement, tweaking "mode" based on the returned value from an
extra lstat() is an unneeded change of behaviour.  Just passing 0777
and let mkdir() take the umask into account to come up with the
final permission bits is more in line with the original scripted
version, I would think.

Thanks.

[PATCH v2 0/2] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Changes made to the previous version of the patch series[1]:

* Since later on with certain patches, the number of bit-parameters to
  be passed to a few functions depend on many parameters, I prefered
  using a single flag bit.

* Memory-leak of the variable 'remote' in the function:
  print_default_remote() was avoided.

* Additional condition were introduced while freeing the variables:
  sub_origin_url and super_config_url.

* print messages and comments in the deinit_submodule function were
  corrected as suggested in previous review of this patch[2].

* Call to the function lstat() for identifying the directory mode was
  avoided and instead 0777 was used. An additional improvement is to be
  made over this patch, but since the improvement can not directly be
  part of the "rewirte in C", the patch would be floated saperately on
  the mailing list.

As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-2

And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-2
Build #196

[1]: https://public-inbox.org/git/20180109175703.4793-1-pc44800@gmail.com/ 
[2]: https://public-inbox.org/git/xmqq7esq4tf6.fsf@gitster.mtv.corp.google.com/

Prathamesh Chavan (2):
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C

 builtin/submodule--helper.c | 342 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 112 +--------------
 2 files changed, 344 insertions(+), 110 deletions(-)

-- 
2.15.1

[PATCH v2 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

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 four functions: module_sync(),
sync_submodule(), sync_submodule_cb() 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>
---
 builtin/submodule--helper.c | 195 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  57 +------------
 2 files changed, 196 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..eb6f96981 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -50,6 +50,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)
+		printf("%s\n", remote);
+
+	return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
 	return str[0] == '.' && is_dir_sep(str[1]);
@@ -358,6 +372,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;
@@ -718,6 +751,166 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct sync_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+
+#define SYNC_CB_INIT { NULL, 0 }
+
+static void sync_submodule(const char *path, const char *prefix,
+			   unsigned int flags)
+{
+	const struct submodule *sub;
+	char *remote_key = NULL;
+	char *sub_origin_url, *super_config_url, *displaypath;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *sub_config_path = NULL;
+
+	if (!is_submodule_active(the_repository, path))
+		return;
+
+	sub = submodule_from_path(&null_oid, path);
+
+	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();
+			strbuf_addf(&sb, "remote.%s.url", remote);
+
+			if (git_config_get_string(sb.buf, &remote_url))
+				remote_url = xgetcwd();
+
+			up_path = get_up_path(path);
+			sub_origin_url = relative_url(remote_url, sub->url, up_path);
+			super_config_url = relative_url(remote_url, sub->url, NULL);
+
+			free(remote);
+			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(path, prefix);
+
+	if (!(flags & OPT_QUIET))
+		printf(_("Synchronizing submodule url for '%s'\n"),
+			 displaypath);
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (git_config_set_gently(sb.buf, super_config_url))
+		die(_("failed to register url for submodule path '%s'"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.dir = path;
+	argv_array_pushl(&cp.args, "submodule--helper",
+			 "print-default-remote", NULL);
+
+	strbuf_reset(&sb);
+	if (capture_command(&cp, &sb, 0))
+		die(_("failed to get the default remote for submodule '%s'"),
+		      path);
+
+	strbuf_strip_suffix(&sb, "\n");
+	remote_key = xstrfmt("remote.%s.url", sb.buf);
+
+	strbuf_reset(&sb);
+	submodule_to_gitdir(&sb, path);
+	strbuf_addstr(&sb, "/config");
+
+	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+		die(_("failed to update remote for submodule '%s'"),
+		      path);
+
+	if (flags & OPT_RECURSIVE) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_push(&cpr.args, "--super-prefix");
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "sync",
+				 "--recursive", NULL);
+
+		if (flags & OPT_QUIET)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      path);
+	}
+
+cleanup:
+	if (strlen(super_config_url))
+		free(super_config_url);
+	if (strlen(sub_origin_url))
+		free(sub_origin_url);
+	strbuf_release(&sb);
+	free(remote_key);
+	free(displaypath);
+	free(sub_config_path);
+}
+
+static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data)
+{
+	struct sync_cb *info = cb_data;
+	sync_submodule(list_item->name, info->prefix, info->flags);
+
+}
+
+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;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (recursive)
+		info.flags |= OPT_RECURSIVE;
+
+	for_each_listed_submodule(&list, sync_submodule_cb, &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)
@@ -1498,6 +1691,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 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 156255a9e..0825cae14 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,63 +1036,8 @@ 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"
-
-			if test -n "$recursive"
-			then
-				prefix="$prefix$sm_path/"
-				eval cmd_sync
-			fi
-		)
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 cmd_absorbgitdirs()
-- 
2.15.1

[PATCH v2 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into four
functions: module_deinit(), for_each_listed_submodule(),
deinit_submodule() and deinit_submodule_cb().

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 147 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  55 +----------------
 2 files changed, 148 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index eb6f96981..b93e1d50b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
 #define OPT_RECURSIVE (1 << 2)
+#define OPT_FORCE (1 << 3)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 				  void *cb_data);
@@ -911,6 +912,151 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct deinit_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+#define DEINIT_CB_INIT { NULL, 0 }
+
+static void deinit_submodule(const char *path, const char *prefix,
+			     unsigned int flags)
+{
+	const struct submodule *sub;
+	char *displaypath = NULL;
+	struct child_process cp_config = CHILD_PROCESS_INIT;
+	struct strbuf sb_config = STRBUF_INIT;
+	char *sub_git_dir = xstrfmt("%s/.git", path);
+
+	sub = submodule_from_path(&null_oid, path);
+
+	if (!sub || !sub->name)
+		goto cleanup;
+
+	displaypath = get_submodule_displaypath(path, prefix);
+
+	/* remove the submodule work tree (unless the user already did it) */
+	if (is_directory(path)) {
+		struct strbuf sb_rm = STRBUF_INIT;
+		const char *format;
+
+		/*
+		 * protect submodules containing a .git directory
+		 * NEEDSWORK: instead of dying, automatically call
+		 * absorbgitdirs and (possibly) warn.
+		 */
+		if (is_directory(sub_git_dir))
+			die(_("Submodule work tree '%s' contains a .git "
+			      "directory (use 'rm -rf' if you really want "
+			      "to remove it including all of its history)"),
+			    displaypath);
+
+		if (!(flags & OPT_FORCE)) {
+			struct child_process cp_rm = CHILD_PROCESS_INIT;
+			cp_rm.git_cmd = 1;
+			argv_array_pushl(&cp_rm.args, "rm", "-qn",
+					 path, NULL);
+
+			if (run_command(&cp_rm))
+				die(_("Submodule work tree '%s' contains local "
+				      "modifications; use '-f' to discard them"),
+				      displaypath);
+		}
+
+		strbuf_addstr(&sb_rm, path);
+
+		if (!remove_dir_recursively(&sb_rm, 0))
+			format = _("Cleared directory '%s'\n");
+		else
+			format = _("Could not remove submodule work tree '%s'\n");
+
+		if (!(flags & OPT_QUIET))
+			printf(format, displaypath);
+
+		strbuf_release(&sb_rm);
+	}
+
+	if (mkdir(path, 0777))
+		die_errno(_("could not create empty submodule directory %s"),
+		      displaypath);
+
+	cp_config.git_cmd = 1;
+	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
+	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
+
+	/* remove the .git/config entries (unless the user already did it) */
+	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
+		char *sub_key = xstrfmt("submodule.%s", sub->name);
+		/*
+		 * remove the whole section so we have a clean state when
+		 * the user later decides to init this submodule again
+		 */
+		git_config_rename_section_in_file(NULL, sub_key, NULL);
+		if (!(flags & OPT_QUIET))
+			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
+				 sub->name, sub->url, displaypath);
+		free(sub_key);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_git_dir);
+	strbuf_release(&sb_config);
+}
+
+static void deinit_submodule_cb(const struct cache_entry *list_item,
+				void *cb_data)
+{
+	struct deinit_cb *info = cb_data;
+	deinit_submodule(list_item->name, info->prefix, info->flags);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+	struct deinit_cb info = DEINIT_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int force = 0;
+	int all = 0;
+
+	struct option module_deinit_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT__FORCE(&force, N_("Remove submodule working trees even if they contain local changes")),
+		OPT_BOOL(0, "all", &all, N_("Unregister all submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_deinit_options,
+			     git_submodule_helper_usage, 0);
+
+	if (all && argc) {
+		error("pathspec and --all are incompatible");
+		usage_with_options(git_submodule_helper_usage,
+				   module_deinit_options);
+	}
+
+	if (!argc && !all)
+		die(_("Use '--all' if you really want to deinitialize all submodules"));
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.prefix = prefix;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (force)
+		info.flags |= OPT_FORCE;
+
+	for_each_listed_submodule(&list, deinit_submodule_cb, &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)
@@ -1693,6 +1839,7 @@ static struct cmd_struct commands[] = {
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
+	{"deinit", module_deinit, 0},
 	{"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 0825cae14..24914963c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -428,60 +428,7 @@ cmd_deinit()
 		shift
 	done
 
-	if test -n "$deinit_all" && test "$#" -ne 0
-	then
-		echo >&2 "$(eval_gettext "pathspec and --all are incompatible")"
-		usage
-	fi
-	if test $# = 0 && test -z "$deinit_all"
-	then
-		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
-	fi
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-
-		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
-
-		# Remove the submodule work tree (unless the user already did it)
-		if test -d "$sm_path"
-		then
-			# Protect submodules containing a .git directory
-			if test -d "$sm_path/.git"
-			then
-				die "$(eval_gettext "\
-Submodule work tree '\$displaypath' contains a .git directory
-(use 'rm -rf' if you really want to remove it including all of its history)")"
-			fi
-
-			if test -z "$force"
-			then
-				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
-			fi
-			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
-		fi
-
-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
-
-		# Remove the .git/config entries (unless the user already did it)
-		if test -n "$(git config --get-regexp submodule."$name\.")"
-		then
-			# Remove the whole section so we have a clean state when
-			# the user later decides to init this submodule again
-			url=$(git config submodule."$name".url)
-			git config --remove-section submodule."$name" 2>/dev/null &&
-			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
 }
 
 is_tip_reachable () (
-- 
2.15.1

Re: [PATCH v2 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> +		} else {
> +			sub_origin_url = xstrdup(sub->url);
> +			super_config_url = xstrdup(sub->url);
> +		}
> +	} else {
> +		sub_origin_url = "";
> +		super_config_url = "";
> +	}
> + ...
> +cleanup:
> +	if (strlen(super_config_url))
> +		free(super_config_url);
> +	if (strlen(sub_origin_url))
> +		free(sub_origin_url);

The above is ugly and veriy likely to be wrong; imagine that
sub->url was an empty string to begin with.

Doing xstrdup("") before assigning the constant to *_url would be a
lot more sensible and maintainable solution for things like this.

Re: [PATCH v2 0/2] Incremental rewrite of git-submodules

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> Changes made to the previous version of the patch series[1]:
>
> * Since later on with certain patches, the number of bit-parameters to
>   be passed to a few functions depend on many parameters, I prefered
>   using a single flag bit.

I am not quite getting what you meant to say here.

> * Memory-leak of the variable 'remote' in the function:
>   print_default_remote() was avoided.

avoided how?  I am not quite getting what you meant to say here.

> * Additional condition were introduced while freeing the variables:
>   sub_origin_url and super_config_url.

As I said, I do not think the change goes into the right direction.

Re: [PATCH v2 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> +	/* remove the submodule work tree (unless the user already did it) */
> +	if (is_directory(path)) {
> +		struct strbuf sb_rm = STRBUF_INIT;
> +		const char *format;
> +
> +		/*
> +		 * protect submodules containing a .git directory
> +		 * NEEDSWORK: instead of dying, automatically call
> +		 * absorbgitdirs and (possibly) warn.
> +		 */
> +		if (is_directory(sub_git_dir))
> +			die(_("Submodule work tree '%s' contains a .git "
> +			      "directory (use 'rm -rf' if you really want "
> +			      "to remove it including all of its history)"),
> +			    displaypath);
> +
> +		if (!(flags & OPT_FORCE)) {
> +			struct child_process cp_rm = CHILD_PROCESS_INIT;
> +			cp_rm.git_cmd = 1;
> +			argv_array_pushl(&cp_rm.args, "rm", "-qn",
> +					 path, NULL);
> +
> +			if (run_command(&cp_rm))
> +				die(_("Submodule work tree '%s' contains local "
> +				      "modifications; use '-f' to discard them"),
> +				      displaypath);
> +		}
> +
> +		strbuf_addstr(&sb_rm, path);
> +
> +		if (!remove_dir_recursively(&sb_rm, 0))
> +			format = _("Cleared directory '%s'\n");
> +		else
> +			format = _("Could not remove submodule work tree '%s'\n");
> +
> +		if (!(flags & OPT_QUIET))
> +			printf(format, displaypath);
> +
> +		strbuf_release(&sb_rm);
> +	}
> +
> +	if (mkdir(path, 0777))
> +		die_errno(_("could not create empty submodule directory %s"),
> +		      displaypath);

If path was a directory (which presumably is the normal case) and
recursive removal fails (i.e. when the code says "Could not remove"),
this mkdir() would also fail with EEXIST.

In such a case, the original code did not die and instead continued
to remove the entries for the submodule from the configuration.
This "rewritten" version dies, leaving the stale configuration for
the submodule we failed to get rid of from the working tree.

I offhand do not know which one of these error case behaviours is
more useful; the user needs to do something (e.g. loosening the perm
in some paths in the submodule that prevented "rm -rf" from working
with "chmod u+w sub/some/path" and removing it manually) to recover
in either case, and cleaning as much as possible by removing the
configuration entries even when this mkdir() fails would probably be
a better behaviour, as long as the command as a whole exits with non
zero status to signal an error.

[PATCH v3 0/2] Incremental rewrite of git-submodules

[Prathamesh Chavan]

Changes in v3:

* For the variables: super_config_url and sub_origin_url, xstrdup() was used
  while assigning "" to them, before freeing.

* In case of the function deinit_submodule, since the orignal code doesn't die
  upon failure of the function mkdir(), printf was used instead of die_errno.

As before you can find this series at:
https://github.com/pratham-pc/git/commits/patch-series-2

And its build report is available at:
https://travis-ci.org/pratham-pc/git/builds/
Branch: patch-series-2
Build #197

Prathamesh Chavan (2):
  submodule: port submodule subcommand 'sync' from shell to C
  submodule: port submodule subcommand 'deinit' from shell to C

 builtin/submodule--helper.c | 340 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            | 112 +--------------
 2 files changed, 342 insertions(+), 110 deletions(-)

-- 
2.15.1

[PATCH v3 1/2] submodule: port submodule subcommand 'sync' from shell to C

[Prathamesh Chavan]

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 four functions: module_sync(),
sync_submodule(), sync_submodule_cb() 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>
---
 builtin/submodule--helper.c | 193 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  57 +------------
 2 files changed, 194 insertions(+), 56 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index a5c4a8a69..745d070ea 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -50,6 +50,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)
+		printf("%s\n", remote);
+
+	return 0;
+}
+
 static int starts_with_dot_slash(const char *str)
 {
 	return str[0] == '.' && is_dir_sep(str[1]);
@@ -358,6 +372,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;
@@ -718,6 +751,164 @@ static int module_name(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct sync_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+
+#define SYNC_CB_INIT { NULL, 0 }
+
+static void sync_submodule(const char *path, const char *prefix,
+			   unsigned int flags)
+{
+	const struct submodule *sub;
+	char *remote_key = NULL;
+	char *sub_origin_url, *super_config_url, *displaypath;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+	char *sub_config_path = NULL;
+
+	if (!is_submodule_active(the_repository, path))
+		return;
+
+	sub = submodule_from_path(&null_oid, path);
+
+	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();
+			strbuf_addf(&sb, "remote.%s.url", remote);
+
+			if (git_config_get_string(sb.buf, &remote_url))
+				remote_url = xgetcwd();
+
+			up_path = get_up_path(path);
+			sub_origin_url = relative_url(remote_url, sub->url, up_path);
+			super_config_url = relative_url(remote_url, sub->url, NULL);
+
+			free(remote);
+			free(up_path);
+			free(remote_url);
+		} else {
+			sub_origin_url = xstrdup(sub->url);
+			super_config_url = xstrdup(sub->url);
+		}
+	} else {
+		sub_origin_url = xstrdup("");
+		super_config_url = xstrdup("");
+	}
+
+	displaypath = get_submodule_displaypath(path, prefix);
+
+	if (!(flags & OPT_QUIET))
+		printf(_("Synchronizing submodule url for '%s'\n"),
+			 displaypath);
+
+	strbuf_reset(&sb);
+	strbuf_addf(&sb, "submodule.%s.url", sub->name);
+	if (git_config_set_gently(sb.buf, super_config_url))
+		die(_("failed to register url for submodule path '%s'"),
+		      displaypath);
+
+	if (!is_submodule_populated_gently(path, NULL))
+		goto cleanup;
+
+	prepare_submodule_repo_env(&cp.env_array);
+	cp.git_cmd = 1;
+	cp.dir = path;
+	argv_array_pushl(&cp.args, "submodule--helper",
+			 "print-default-remote", NULL);
+
+	strbuf_reset(&sb);
+	if (capture_command(&cp, &sb, 0))
+		die(_("failed to get the default remote for submodule '%s'"),
+		      path);
+
+	strbuf_strip_suffix(&sb, "\n");
+	remote_key = xstrfmt("remote.%s.url", sb.buf);
+
+	strbuf_reset(&sb);
+	submodule_to_gitdir(&sb, path);
+	strbuf_addstr(&sb, "/config");
+
+	if (git_config_set_in_file_gently(sb.buf, remote_key, sub_origin_url))
+		die(_("failed to update remote for submodule '%s'"),
+		      path);
+
+	if (flags & OPT_RECURSIVE) {
+		struct child_process cpr = CHILD_PROCESS_INIT;
+
+		cpr.git_cmd = 1;
+		cpr.dir = path;
+		prepare_submodule_repo_env(&cpr.env_array);
+
+		argv_array_push(&cpr.args, "--super-prefix");
+		argv_array_pushf(&cpr.args, "%s/", displaypath);
+		argv_array_pushl(&cpr.args, "submodule--helper", "sync",
+				 "--recursive", NULL);
+
+		if (flags & OPT_QUIET)
+			argv_array_push(&cpr.args, "--quiet");
+
+		if (run_command(&cpr))
+			die(_("failed to recurse into submodule '%s'"),
+			      path);
+	}
+
+cleanup:
+	free(super_config_url);
+	free(sub_origin_url);
+	strbuf_release(&sb);
+	free(remote_key);
+	free(displaypath);
+	free(sub_config_path);
+}
+
+static void sync_submodule_cb(const struct cache_entry *list_item, void *cb_data)
+{
+	struct sync_cb *info = cb_data;
+	sync_submodule(list_item->name, info->prefix, info->flags);
+
+}
+
+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;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (recursive)
+		info.flags |= OPT_RECURSIVE;
+
+	for_each_listed_submodule(&list, sync_submodule_cb, &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)
@@ -1498,6 +1689,8 @@ static struct cmd_struct commands[] = {
 	{"resolve-relative-url-test", resolve_relative_url_test, 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 156255a9e..0825cae14 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -1036,63 +1036,8 @@ 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"
-
-			if test -n "$recursive"
-			then
-				prefix="$prefix$sm_path/"
-				eval cmd_sync
-			fi
-		)
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper sync ${GIT_QUIET:+--quiet} ${recursive:+--recursive} "$@"
 }
 
 cmd_absorbgitdirs()
-- 
2.15.1

[PATCH v3 2/2] submodule: port submodule subcommand 'deinit' from shell to C

[Prathamesh Chavan]

The same mechanism is used even for porting this submodule
subcommand, as used in the ported subcommands till now.
The function cmd_deinit in split up after porting into four
functions: module_deinit(), for_each_listed_submodule(),
deinit_submodule() and deinit_submodule_cb().

Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Stefan Beller <sbeller@google.com>
Signed-off-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 147 ++++++++++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  55 +----------------
 2 files changed, 148 insertions(+), 54 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 745d070ea..b1daca995 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -20,6 +20,7 @@
 #define OPT_QUIET (1 << 0)
 #define OPT_CACHED (1 << 1)
 #define OPT_RECURSIVE (1 << 2)
+#define OPT_FORCE (1 << 3)
 
 typedef void (*each_submodule_fn)(const struct cache_entry *list_item,
 				  void *cb_data);
@@ -909,6 +910,151 @@ static int module_sync(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+struct deinit_cb {
+	const char *prefix;
+	unsigned int flags;
+};
+#define DEINIT_CB_INIT { NULL, 0 }
+
+static void deinit_submodule(const char *path, const char *prefix,
+			     unsigned int flags)
+{
+	const struct submodule *sub;
+	char *displaypath = NULL;
+	struct child_process cp_config = CHILD_PROCESS_INIT;
+	struct strbuf sb_config = STRBUF_INIT;
+	char *sub_git_dir = xstrfmt("%s/.git", path);
+
+	sub = submodule_from_path(&null_oid, path);
+
+	if (!sub || !sub->name)
+		goto cleanup;
+
+	displaypath = get_submodule_displaypath(path, prefix);
+
+	/* remove the submodule work tree (unless the user already did it) */
+	if (is_directory(path)) {
+		struct strbuf sb_rm = STRBUF_INIT;
+		const char *format;
+
+		/*
+		 * protect submodules containing a .git directory
+		 * NEEDSWORK: instead of dying, automatically call
+		 * absorbgitdirs and (possibly) warn.
+		 */
+		if (is_directory(sub_git_dir))
+			die(_("Submodule work tree '%s' contains a .git "
+			      "directory (use 'rm -rf' if you really want "
+			      "to remove it including all of its history)"),
+			    displaypath);
+
+		if (!(flags & OPT_FORCE)) {
+			struct child_process cp_rm = CHILD_PROCESS_INIT;
+			cp_rm.git_cmd = 1;
+			argv_array_pushl(&cp_rm.args, "rm", "-qn",
+					 path, NULL);
+
+			if (run_command(&cp_rm))
+				die(_("Submodule work tree '%s' contains local "
+				      "modifications; use '-f' to discard them"),
+				      displaypath);
+		}
+
+		strbuf_addstr(&sb_rm, path);
+
+		if (!remove_dir_recursively(&sb_rm, 0))
+			format = _("Cleared directory '%s'\n");
+		else
+			format = _("Could not remove submodule work tree '%s'\n");
+
+		if (!(flags & OPT_QUIET))
+			printf(format, displaypath);
+
+		strbuf_release(&sb_rm);
+	}
+
+	if (mkdir(path, 0777))
+		printf(_("could not create empty submodule directory %s"),
+		      displaypath);
+
+	cp_config.git_cmd = 1;
+	argv_array_pushl(&cp_config.args, "config", "--get-regexp", NULL);
+	argv_array_pushf(&cp_config.args, "submodule.%s\\.", sub->name);
+
+	/* remove the .git/config entries (unless the user already did it) */
+	if (!capture_command(&cp_config, &sb_config, 0) && sb_config.len) {
+		char *sub_key = xstrfmt("submodule.%s", sub->name);
+		/*
+		 * remove the whole section so we have a clean state when
+		 * the user later decides to init this submodule again
+		 */
+		git_config_rename_section_in_file(NULL, sub_key, NULL);
+		if (!(flags & OPT_QUIET))
+			printf(_("Submodule '%s' (%s) unregistered for path '%s'\n"),
+				 sub->name, sub->url, displaypath);
+		free(sub_key);
+	}
+
+cleanup:
+	free(displaypath);
+	free(sub_git_dir);
+	strbuf_release(&sb_config);
+}
+
+static void deinit_submodule_cb(const struct cache_entry *list_item,
+				void *cb_data)
+{
+	struct deinit_cb *info = cb_data;
+	deinit_submodule(list_item->name, info->prefix, info->flags);
+}
+
+static int module_deinit(int argc, const char **argv, const char *prefix)
+{
+	struct deinit_cb info = DEINIT_CB_INIT;
+	struct pathspec pathspec;
+	struct module_list list = MODULE_LIST_INIT;
+	int quiet = 0;
+	int force = 0;
+	int all = 0;
+
+	struct option module_deinit_options[] = {
+		OPT__QUIET(&quiet, N_("Suppress submodule status output")),
+		OPT__FORCE(&force, N_("Remove submodule working trees even if they contain local changes")),
+		OPT_BOOL(0, "all", &all, N_("Unregister all submodules")),
+		OPT_END()
+	};
+
+	const char *const git_submodule_helper_usage[] = {
+		N_("git submodule deinit [--quiet] [-f | --force] [--all | [--] [<path>...]]"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, module_deinit_options,
+			     git_submodule_helper_usage, 0);
+
+	if (all && argc) {
+		error("pathspec and --all are incompatible");
+		usage_with_options(git_submodule_helper_usage,
+				   module_deinit_options);
+	}
+
+	if (!argc && !all)
+		die(_("Use '--all' if you really want to deinitialize all submodules"));
+
+	if (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)
+		BUG("module_list_compute should not choke on empty pathspec");
+
+	info.prefix = prefix;
+	if (quiet)
+		info.flags |= OPT_QUIET;
+	if (force)
+		info.flags |= OPT_FORCE;
+
+	for_each_listed_submodule(&list, deinit_submodule_cb, &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)
@@ -1691,6 +1837,7 @@ static struct cmd_struct commands[] = {
 	{"status", module_status, SUPPORT_SUPER_PREFIX},
 	{"print-default-remote", print_default_remote, 0},
 	{"sync", module_sync, SUPPORT_SUPER_PREFIX},
+	{"deinit", module_deinit, 0},
 	{"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 0825cae14..24914963c 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -428,60 +428,7 @@ cmd_deinit()
 		shift
 	done
 
-	if test -n "$deinit_all" && test "$#" -ne 0
-	then
-		echo >&2 "$(eval_gettext "pathspec and --all are incompatible")"
-		usage
-	fi
-	if test $# = 0 && test -z "$deinit_all"
-	then
-		die "$(eval_gettext "Use '--all' if you really want to deinitialize all submodules")"
-	fi
-
-	{
-		git submodule--helper list --prefix "$wt_prefix" "$@" ||
-		echo "#unmatched" $?
-	} |
-	while read -r mode sha1 stage sm_path
-	do
-		die_if_unmatched "$mode" "$sha1"
-		name=$(git submodule--helper name "$sm_path") || exit
-
-		displaypath=$(git submodule--helper relative-path "$sm_path" "$wt_prefix")
-
-		# Remove the submodule work tree (unless the user already did it)
-		if test -d "$sm_path"
-		then
-			# Protect submodules containing a .git directory
-			if test -d "$sm_path/.git"
-			then
-				die "$(eval_gettext "\
-Submodule work tree '\$displaypath' contains a .git directory
-(use 'rm -rf' if you really want to remove it including all of its history)")"
-			fi
-
-			if test -z "$force"
-			then
-				git rm -qn "$sm_path" ||
-				die "$(eval_gettext "Submodule work tree '\$displaypath' contains local modifications; use '-f' to discard them")"
-			fi
-			rm -rf "$sm_path" &&
-			say "$(eval_gettext "Cleared directory '\$displaypath'")" ||
-			say "$(eval_gettext "Could not remove submodule work tree '\$displaypath'")"
-		fi
-
-		mkdir "$sm_path" || say "$(eval_gettext "Could not create empty submodule directory '\$displaypath'")"
-
-		# Remove the .git/config entries (unless the user already did it)
-		if test -n "$(git config --get-regexp submodule."$name\.")"
-		then
-			# Remove the whole section so we have a clean state when
-			# the user later decides to init this submodule again
-			url=$(git config submodule."$name".url)
-			git config --remove-section submodule."$name" 2>/dev/null &&
-			say "$(eval_gettext "Submodule '\$name' (\$url) unregistered for path '\$displaypath'")"
-		fi
-	done
+	git ${wt_prefix:+-C "$wt_prefix"} submodule--helper deinit ${GIT_QUIET:+--quiet} ${prefix:+--prefix "$prefix"} ${force:+--force} ${deinit_all:+--all} "$@"
 }
 
 is_tip_reachable () (
-- 
2.15.1

Re: [PATCH v3 0/2] Incremental rewrite of git-submodules

[Junio C Hamano]

Prathamesh Chavan <pc44800@gmail.com> writes:

> Changes in v3:
>
> * For the variables: super_config_url and sub_origin_url, xstrdup() was used
>   while assigning "" to them, before freeing.
>
> * In case of the function deinit_submodule, since the orignal code doesn't die
>   upon failure of the function mkdir(), printf was used instead of die_errno.
>
> As before you can find this series at:
> https://github.com/pratham-pc/git/commits/patch-series-2
>
> And its build report is available at:
> https://travis-ci.org/pratham-pc/git/builds/
> Branch: patch-series-2
> Build #197
>
> Prathamesh Chavan (2):
>   submodule: port submodule subcommand 'sync' from shell to C
>   submodule: port submodule subcommand 'deinit' from shell to C
>
>  builtin/submodule--helper.c | 340 ++++++++++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            | 112 +--------------
>  2 files changed, 342 insertions(+), 110 deletions(-)

Looks sensible.  Thanks.