[GSoC] [PATCH 0/2] submodule--helper: introduce subcommands for sh to C conversion

[GSoC] [PATCH 0/2] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

This patch series introduces the `add clone` and `add config`
subcommands in `git submodule--helper` with the goal of converting part
of the shell code in `git-submodule.sh` related to `git submodule add`
into C code.

The first patch [1/2] has already been sent to the mailing list before:
https://lore.kernel.org/git/20210604110524.84326-1-raykar.ath@gmail.com/
No changes have been made to it since then.

Because patch [2/2] depends on changes introduced in [1/2] I am sending
them together as a series.

The eventual goal is to replace all of the shell code with equivalent C
code. Subsequent patches will replace all of shell the code past the
flag parsing of `cmd_add()` into a single call to subcommand
`submodule--helper add` which will make use of the functions introduced
in these two patches.

Link to hosted Git repository for containing these patches:
https://github.com/tfidfwastaken/git/commits/submodule-add-in-c

Atharva Raykar (2):
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 312 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  66 +-------
 2 files changed, 314 insertions(+), 64 deletions(-)

-- 
2.31.1

[GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule
name has been supplied with a name that clashes with a local submodule,
the message shown to the user ("A git directory for 'foo' is found
locally...") is prepended with "error" for clarity.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 199 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +------
 2 files changed, 200 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..c9cb535312 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static char *parse_token(char **begin, const char *end, int *tok_len)
+{
+	char *tok_start, *pos = *begin;
+	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
+		pos++;
+	tok_start = *begin;
+	*tok_len = pos - *begin;
+	*begin = pos + 1;
+	return tok_start;
+}
+
+static char *get_next_line(char *const begin, const char *const end)
+{
+	char *pos = begin;
+	while (pos != end && *pos++ != '\n');
+	return pos;
+}
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *line;
+		char *begin = sb_remote_out.buf;
+		char *end = sb_remote_out.buf + sb_remote_out.len;
+		while (begin != end && (line = get_next_line(begin, end))) {
+			int namelen = 0, urllen = 0, taillen = 0;
+			char *name = parse_token(&begin, line, &namelen);
+			char *url = parse_token(&begin, line, &urllen);
+			char *tail = parse_token(&begin, line, &taillen);
+			if (!memcmp(tail, "(fetch)", 7))
+				fprintf(output, "  %.*s\t%.*s\n",
+					namelen, name, urllen, url);
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
+			printf(_("Adding existing path at '%s' to index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct strvec clone_args = STRVEC_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				error(_("A git directory for '%s' is found "
+					"locally with remote(s):"), add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				fprintf(stderr,
+					_("If you want to reuse this local git "
+					  "directory instead of cloning again from\n"
+					  "  %s\n"
+					  "use the '--force' option. If the local git "
+					  "directory is not the correct repo\n"
+					  "or if you are unsure what this means, choose "
+					  "another name with the '--name' option.\n"),
+					add_data->realrepo);
+				free(submod_gitdir_path);
+				return 1;
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
+			     add_data->sm_name, "--url", add_data->realrepo, NULL);
+		if (add_data->quiet)
+			strvec_push(&clone_args, "--quiet");
+		if (add_data->progress)
+			strvec_push(&clone_args, "--progress");
+		if (add_data->prefix)
+			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
+		if (add_data->reference_path)
+			strvec_pushl(&clone_args, "--reference",
+				     add_data->reference_path, NULL);
+		if (add_data->dissociate)
+			strvec_push(&clone_args, "--dissociate");
+		if (add_data->depth >= 0)
+			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
+
+		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
+			strvec_clear(&clone_args);
+			return -1;
+		}
+		strvec_clear(&clone_args);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &add_data.prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT_BOOL('f', "force", &force,
+			 N_("allow adding an otherwise ignored submodule path")),
+		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
+		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
+		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n".

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 113 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +--------
 2 files changed, 114 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c9cb535312..92d105be02 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2943,6 +2943,118 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	key = xstrfmt("submodule.%s.path", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
+	free(key);
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->repo);
+	free(key);
+	if (add_data->branch) {
+		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
+		config_set_in_gitmodules_file_gently(key, add_data->branch);
+		free(key);
+	}
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec)
+		warning(_("The submodule.active configuration exists, but "
+			  "no pathspec was specified. Setting the value of "
+			  "submodule.%s.active to 'true'."), add_data->sm_name);
+
+	/*
+	 * If submodule.active does not exist, we will activate this
+	 * module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_BOOL('f', "force", &force,
+			 N_("allow adding an otherwise ignored submodule path")),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2956,6 +3068,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1

Re: [GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand

[Bagas Sanjaya]

Hi,

On 05/06/21 18.39, Atharva Raykar wrote:
> +	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit

What is the exit code you mean in case of error?

>   	git config submodule."$sm_name".url "$realrepo"
>   
>   	git add --no-warn-embedded-repo $force "$sm_path" ||
> 

Is `git` executable that found in $PATH used? I have both 
distro-packaged Git and custom-compiled Git installed, and I would like 
the latter to be able to use git-submodule from its own install prefix 
(/path/to/git-prefix or whatsever).

-- 
An old man doll... just what I always wanted! - Clara

Re: [GSoC] [PATCH 1/2] submodule--helper: introduce add-clone subcommand

[Christian Couder]

Hi,

On Sun, Jun 6, 2021 at 5:38 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:
>
> Hi,
>
> On 05/06/21 18.39, Atharva Raykar wrote:
> > +     git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
>
> What is the exit code you mean in case of error?

The exit code of the above is the same as the exit code of what's
before `|| exit`.

For example:

$ ( ( exit 10 ) || exit ); echo $?
10

> >       git config submodule."$sm_name".url "$realrepo"
> >
> >       git add --no-warn-embedded-repo $force "$sm_path" ||
>
> Is `git` executable that found in $PATH used? I have both
> distro-packaged Git and custom-compiled Git installed, and I would like
> the latter to be able to use git-submodule from its own install prefix
> (/path/to/git-prefix or whatsever).

This is a different issue than what this patch is doing.

Re: [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand

[Christian Couder]

On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@gmail.com> wrote:
>
> Add a new "add-config" subcommand to `git submodule--helper` with the
> goal of converting part of the shell code in git-submodule.sh related to
> `git submodule add` into C code. This new subcommand sets the
> configuration variables of a newly added submodule, by registering the
> url in local git config, as well as the submodule name and path in the
> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if
> the submodule path has not already been covered by any pathspec
> specified in 'submodule.active'.
>
> This is meant to be a faithful conversion from shell to C, with only one
> minor change: A warning is emitted if no value is specified in
> 'submodule.active', ie, the config looks like: "[submodule] active\n".

Maybe explaining a bit how this warning is useful could help reviewers
here. Especially what could happen if no value is specified in
'submodule.active'?

> The structure of the conditional to check if we need to set the 'active'
> toggle looks different from the shell version -- but behaves the same.
> The change was made to decrease code duplication. A comment has been
> added to explain that only one value of 'submodule.active' is obtained
> to check if we need to call is_submodule_active() at all.
>
> This is part of a series of changes that will result in all of
> 'submodule add' being converted to C.

[...]


> +       /*
> +        * NEEDSWORK: In a multi-working-tree world this needs to be
> +        * set in the per-worktree config.
> +        */
> +       pathspec_key_exists = !git_config_get_string("submodule.active",
> +                                                    &submod_pathspec);
> +       if (pathspec_key_exists && !submod_pathspec)
> +               warning(_("The submodule.active configuration exists, but "
> +                         "no pathspec was specified. Setting the value of "
> +                         "submodule.%s.active to 'true'."), add_data->sm_name);

It's not very clear that we will actually set
'submodule.<name>.active' to true below as it depends on the result of
calling is_submodule_active(), and anyway is_submodule_active() will
check again if "submodule.active" is set, which is wasteful.

Maybe we could set a variable, called for example "activate" here,
with something like:

       if (pathspec_key_exists && !submod_pathspec) {
               warning(...);
               activate = 1;
      }

and below use a check like:

       if (!pathspec_key_exists || activate ||
           !is_submodule_active(the_repository, add_data->sm_path)) {
...

> +       /*
> +        * If submodule.active does not exist, we will activate this
> +        * module unconditionally.
> +        *
> +        * Otherwise, we ask is_submodule_active(), which iterates
> +        * through all the values of 'submodule.active' to determine
> +        * if this module is already active.
> +        */
> +       if (!pathspec_key_exists ||
> +           !is_submodule_active(the_repository, add_data->sm_path)) {
> +               key = xstrfmt("submodule.%s.active", add_data->sm_name);
> +               git_config_set_gently(key, "true");
> +               free(key);
> +       }
> +}

The rest looks good to me! Thanks!

Re: [GSoC] [PATCH 2/2] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

On 07-Jun-2021, at 14:54, Christian Couder <christian.couder@gmail.com> wrote:
> 
> On Sat, Jun 5, 2021 at 1:42 PM Atharva Raykar <raykar.ath@gmail.com> wrote:
>> 
>> Add a new "add-config" subcommand to `git submodule--helper` with the
>> goal of converting part of the shell code in git-submodule.sh related to
>> `git submodule add` into C code. This new subcommand sets the
>> configuration variables of a newly added submodule, by registering the
>> url in local git config, as well as the submodule name and path in the
>> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if
>> the submodule path has not already been covered by any pathspec
>> specified in 'submodule.active'.
>> 
>> This is meant to be a faithful conversion from shell to C, with only one
>> minor change: A warning is emitted if no value is specified in
>> 'submodule.active', ie, the config looks like: "[submodule] active\n".
> 
> Maybe explaining a bit how this warning is useful could help reviewers
> here. Especially what could happen if no value is specified in
> 'submodule.active'?

Will do.

For now I'll leave an explanation here as well, so that those
who might see this thread can know the motivation behind it.

(I'll make it more concise in my cover letter of v2)

Junio in his review of Shourya's patch[1] said:
> When a user has "[submodule] active" in his or her
> configuration file, it is a configuration error.  When Git reads
> "submodule.active" configuration variable to make a decision (like
> the above code) and finds that the user has such an error, the user
> would appreciate if the error is pointed out, so that it can be
> corrected, rather than silently ignored.

Git might set 'submodule.<name>.active = true' in the above case, and
since it has a higher priority[2] than the 'submodule.active' switch,
it would be useful to let the user know of this change, and the incorrect
configuration that led to this behaviour, so that they may be able to
remedy it, and avoid surprises later on.

[1] https://lore.kernel.org/git/xmqqo8isxefz.fsf@gitster.c.googlers.com/
[2] https://git-scm.com/docs/gitsubmodules#_active_submodules

>> The structure of the conditional to check if we need to set the 'active'
>> toggle looks different from the shell version -- but behaves the same.
>> The change was made to decrease code duplication. A comment has been
>> added to explain that only one value of 'submodule.active' is obtained
>> to check if we need to call is_submodule_active() at all.
>> 
>> This is part of a series of changes that will result in all of
>> 'submodule add' being converted to C.
> 
> [...]
> 
> 
>> +       /*
>> +        * NEEDSWORK: In a multi-working-tree world this needs to be
>> +        * set in the per-worktree config.
>> +        */
>> +       pathspec_key_exists = !git_config_get_string("submodule.active",
>> +                                                    &submod_pathspec);
>> +       if (pathspec_key_exists && !submod_pathspec)
>> +               warning(_("The submodule.active configuration exists, but "
>> +                         "no pathspec was specified. Setting the value of "
>> +                         "submodule.%s.active to 'true'."), add_data->sm_name);
> 
> It's not very clear that we will actually set
> 'submodule.<name>.active' to true below as it depends on the result of
> calling is_submodule_active()

Hmm, I see the issue.

Would it be more accurate to say this:

"The submodule.active configuration exists, but no pathspec
was specified. If the module is not already active, the value
of 'submodule.<name>.active' will be set to 'true'."

> , and anyway is_submodule_active() will
> check again if "submodule.active" is set, which is wasteful.
> 
> Maybe we could set a variable, called for example "activate" here,
> with something like:
> 
>       if (pathspec_key_exists && !submod_pathspec) {
>               warning(...);
>               activate = 1;
>      }
> 
> and below use a check like:
> 
>       if (!pathspec_key_exists || activate ||
>           !is_submodule_active(the_repository, add_data->sm_path)) {

Got it. Thanks for suggesting this improvement!

> ...
> 
>> +       /*
>> +        * If submodule.active does not exist, we will activate this
>> +        * module unconditionally.
>> +        *
>> +        * Otherwise, we ask is_submodule_active(), which iterates
>> +        * through all the values of 'submodule.active' to determine
>> +        * if this module is already active.
>> +        */
>> +       if (!pathspec_key_exists ||
>> +           !is_submodule_active(the_repository, add_data->sm_path)) {
>> +               key = xstrfmt("submodule.%s.active", add_data->sm_name);
>> +               git_config_set_gently(key, "true");
>> +               free(key);
>> +       }
>> +}
> 
> The rest looks good to me! Thanks!

:^)

[GSoC] [PATCH v2 0/2] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

I have elaborated the commit message more, to explain why a warning is emitted
for an empty value in 'submodule.active'. The warning has been worded more
accurately than before.

An unnecessary extra check for 'submodule.active' has been avoided.

I have included a range diff, in case that is useful.

My fork containing these changes can be found at:
https://github.com/tfidfwastaken/git/commits/submodule-add-in-c

Emily and Jonathan: To be on the safe side, I have CC'd you so you know where I
keep my changes, and we can avoid potential conflicts, as I believe you are
working on this area as well. Just so you know, every week, I update the link to
all my ongoing work at:
https://atharvaraykar.me/gitnotes/#my-public-gitgit-branches

Atharva Raykar (2):
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 315 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  66 +-------
 2 files changed, 317 insertions(+), 64 deletions(-)

Range-diff against v1:
1:  398bfa713d = 1:  4374ebb6b1 submodule--helper: introduce add-clone subcommand
2:  f9954cfcf7 ! 2:  406022d0f7 submodule--helper: introduce add-config subcommand
    @@ Commit message
     
         This is meant to be a faithful conversion from shell to C, with only one
         minor change: A warning is emitted if no value is specified in
    -    'submodule.active', ie, the config looks like: "[submodule] active\n".
    +    'submodule.active', ie, the config looks like: "[submodule] active\n",
    +    because it is an invalid configuration. It would be helpful to let the
    +    user know that the pathspec is unset, and the value of
    +    'submodule.<name>.active' might be set to 'true' so that they can
    +    rectify their configuration and prevent future surprises (especially
    +    given that the latter variable has a higher priority than the former).
     
         The structure of the conditional to check if we need to set the 'active'
         toggle looks different from the shell version -- but behaves the same.
    @@ builtin/submodule--helper.c: static int add_clone(int argc, const char **argv, c
     +	char *key, *submod_pathspec = NULL;
     +	struct child_process add_submod = CHILD_PROCESS_INIT;
     +	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
    -+	int pathspec_key_exists;
    ++	int pathspec_key_exists, activate = 0;
     +
     +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
     +	git_config_set_gently(key, add_data->realrepo);
    @@ builtin/submodule--helper.c: static int add_clone(int argc, const char **argv, c
     +	 */
     +	pathspec_key_exists = !git_config_get_string("submodule.active",
     +						     &submod_pathspec);
    -+	if (pathspec_key_exists && !submod_pathspec)
    -+		warning(_("The submodule.active configuration exists, but "
    -+			  "no pathspec was specified. Setting the value of "
    -+			  "submodule.%s.active to 'true'."), add_data->sm_name);
    ++	if (pathspec_key_exists && !submod_pathspec) {
    ++		warning(_("The submodule.active configuration exists, but the "
    ++			  "pathspec was unset. If the submodule is not already "
    ++			  "active, the value of submodule.%s.active will be "
    ++			  "be set to 'true'."), add_data->sm_name);
    ++		activate = 1;
    ++	}
     +
     +	/*
    -+	 * If submodule.active does not exist, we will activate this
    -+	 * module unconditionally.
    ++	 * If submodule.active does not exist, or if the pathspec was unset,
    ++	 * we will activate this module unconditionally.
     +	 *
     +	 * Otherwise, we ask is_submodule_active(), which iterates
     +	 * through all the values of 'submodule.active' to determine
     +	 * if this module is already active.
     +	 */
    -+	if (!pathspec_key_exists ||
    ++	if (!pathspec_key_exists || activate ||
     +	    !is_submodule_active(the_repository, add_data->sm_path)) {
     +		key = xstrfmt("submodule.%s.active", add_data->sm_name);
     +		git_config_set_gently(key, "true");
-- 
2.31.1

[GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.

This is part of a series of changes that will result in all of 'submodule add'
being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 199 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +------
 2 files changed, 200 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..c9cb535312 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static char *parse_token(char **begin, const char *end, int *tok_len)
+{
+	char *tok_start, *pos = *begin;
+	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
+		pos++;
+	tok_start = *begin;
+	*tok_len = pos - *begin;
+	*begin = pos + 1;
+	return tok_start;
+}
+
+static char *get_next_line(char *const begin, const char *const end)
+{
+	char *pos = begin;
+	while (pos != end && *pos++ != '\n');
+	return pos;
+}
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *line;
+		char *begin = sb_remote_out.buf;
+		char *end = sb_remote_out.buf + sb_remote_out.len;
+		while (begin != end && (line = get_next_line(begin, end))) {
+			int namelen = 0, urllen = 0, taillen = 0;
+			char *name = parse_token(&begin, line, &namelen);
+			char *url = parse_token(&begin, line, &urllen);
+			char *tail = parse_token(&begin, line, &taillen);
+			if (!memcmp(tail, "(fetch)", 7))
+				fprintf(output, "  %.*s\t%.*s\n",
+					namelen, name, urllen, url);
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
+			printf(_("Adding existing path at '%s' to index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct strvec clone_args = STRVEC_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				error(_("A git directory for '%s' is found "
+					"locally with remote(s):"), add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				fprintf(stderr,
+					_("If you want to reuse this local git "
+					  "directory instead of cloning again from\n"
+					  "  %s\n"
+					  "use the '--force' option. If the local git "
+					  "directory is not the correct repo\n"
+					  "or if you are unsure what this means, choose "
+					  "another name with the '--name' option.\n"),
+					add_data->realrepo);
+				free(submod_gitdir_path);
+				return 1;
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
+			     add_data->sm_name, "--url", add_data->realrepo, NULL);
+		if (add_data->quiet)
+			strvec_push(&clone_args, "--quiet");
+		if (add_data->progress)
+			strvec_push(&clone_args, "--progress");
+		if (add_data->prefix)
+			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
+		if (add_data->reference_path)
+			strvec_pushl(&clone_args, "--reference",
+				     add_data->reference_path, NULL);
+		if (add_data->dissociate)
+			strvec_push(&clone_args, "--dissociate");
+		if (add_data->depth >= 0)
+			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
+
+		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
+			strvec_clear(&clone_args);
+			return -1;
+		}
+		strvec_clear(&clone_args);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &add_data.prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT_BOOL('f', "force", &force,
+			 N_("allow adding an otherwise ignored submodule path")),
+		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
+		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
+		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[GSoC] [PATCH v2 2/2] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 116 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +--------
 2 files changed, 117 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c9cb535312..d7225f503d 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2943,6 +2943,121 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists, activate = 0;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	key = xstrfmt("submodule.%s.path", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
+	free(key);
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->repo);
+	free(key);
+	if (add_data->branch) {
+		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
+		config_set_in_gitmodules_file_gently(key, add_data->branch);
+		free(key);
+	}
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}
+
+	/*
+	 * If submodule.active does not exist, or if the pathspec was unset,
+	 * we will activate this module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists || activate ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_BOOL('f', "force", &force,
+			 N_("allow adding an otherwise ignored submodule path")),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2956,6 +3071,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Đoàn Trần Công Danh]

On 2021-06-08 15:26:54+0530, Atharva Raykar <raykar.ath@gmail.com> wrote:
> Let's add a new "add-clone" subcommand to `git submodule--helper` with
> the goal of converting part of the shell code in git-submodule.sh
> related to `git submodule add` into C code. This new subcommand clones
> the repository that is to be added, and checks out to the appropriate
> branch.
> 
> This is meant to be a faithful conversion that leaves the behaviour of
> 'submodule add' unchanged. The only minor change is that if a submodule name has
> been supplied with a name that clashes with a local submodule, the message shown
> to the user ("A git directory for 'foo' is found locally...") is prepended with
> "error" for clarity.
> 
> This is part of a series of changes that will result in all of 'submodule add'
> being converted to C.
> 
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
> ---
>  builtin/submodule--helper.c | 199 ++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  38 +------
>  2 files changed, 200 insertions(+), 37 deletions(-)
> 
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..c9cb535312 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  	return !!ret;
>  }
>  
> +struct add_data {
> +	const char *prefix;
> +	const char *branch;
> +	const char *reference_path;
> +	const char *sm_path;
> +	const char *sm_name;
> +	const char *repo;
> +	const char *realrepo;
> +	int depth;
> +	unsigned int force: 1;
> +	unsigned int quiet: 1;
> +	unsigned int progress: 1;
> +	unsigned int dissociate: 1;
> +};
> +#define ADD_DATA_INIT { .depth = -1 }
> +
> +static char *parse_token(char **begin, const char *end, int *tok_len)
> +{
> +	char *tok_start, *pos = *begin;
> +	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
> +		pos++;
> +	tok_start = *begin;
> +	*tok_len = pos - *begin;
> +	*begin = pos + 1;
> +	return tok_start;
> +}
> +
> +static char *get_next_line(char *const begin, const char *const end)
> +{
> +	char *pos = begin;
> +	while (pos != end && *pos++ != '\n');
> +	return pos;
> +}

On my first glance, this function looks like a reinvention of strchr(3).
Except that, this function also has a second parameter for "end".
Maybe it has a specical use-case?

> +
> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
> +{
> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
> +	struct strbuf sb_remote_out = STRBUF_INIT;
> +
> +	cp_remote.git_cmd = 1;
> +	strvec_pushf(&cp_remote.env_array,
> +		     "GIT_DIR=%s", git_dir_path);
> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
> +		char *line;
> +		char *begin = sb_remote_out.buf;
> +		char *end = sb_remote_out.buf + sb_remote_out.len;
> +		while (begin != end && (line = get_next_line(begin, end))) {

And this is the only use-case.  Because you also want to check if you
reached the last token or not.  I guess you really meant to write:

	while ((line = strchr(begin, '\n')) != NULL) {

Anyway, I would name the "line" variable as "nextline"

> +			int namelen = 0, urllen = 0, taillen = 0;
> +			char *name = parse_token(&begin, line, &namelen);
> +			char *url = parse_token(&begin, line, &urllen);
> +			char *tail = parse_token(&begin, line, &taillen);
> +			if (!memcmp(tail, "(fetch)", 7))
> +				fprintf(output, "  %.*s\t%.*s\n",
> +					namelen, name, urllen, url);

I think this whole block is better replaced with strip_suffix_mem and
fprintf.

Overral I would replace the block inside capture_command with:

-----8<-----
	char *nextline;
	char *line = sb_remote_out.buf;
	while ((nextline = strchr(line, '\n')) != NULL) {
		size_t len = nextline - line;
		if (strip_suffix_mem(line, &len, "(fetch)"))
			fprintf(output, "  %.*s\n", (int)len, line);
		line = nextline + 1;
	}
---->8-----

And get rid of parse_token and get_next_line functions.



> +		}
> +	}
> +
> +	strbuf_release(&sb_remote_out);
> +}
> +
> +static int add_submodule(const struct add_data *add_data)
> +{
> +	char *submod_gitdir_path;
> +	/* perhaps the path already exists and is already a git repo, else clone it */
> +	if (is_directory(add_data->sm_path)) {
> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
> +			printf(_("Adding existing path at '%s' to index\n"),
> +			       add_data->sm_path);
> +		else
> +			die(_("'%s' already exists and is not a valid git repo"),
> +			    add_data->sm_path);
> +		free(submod_gitdir_path);
> +	} else {
> +		struct strvec clone_args = STRVEC_INIT;
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
> +
> +		if (is_directory(submod_gitdir_path)) {
> +			if (!add_data->force) {
> +				error(_("A git directory for '%s' is found "
> +					"locally with remote(s):"), add_data->sm_name);

We don't capitalise first character of error message.
IOW, downcase "A git".

Well, it's bug-for-bug with shell implementation, so it doesn't matter much, anyway.

> +				show_fetch_remotes(stderr, add_data->sm_name,
> +						   submod_gitdir_path);
> +				fprintf(stderr,
> +					_("If you want to reuse this local git "
> +					  "directory instead of cloning again from\n"
> +					  "  %s\n"
> +					  "use the '--force' option. If the local git "
> +					  "directory is not the correct repo\n"
> +					  "or if you are unsure what this means, choose "
> +					  "another name with the '--name' option.\n"),
> +					add_data->realrepo);

Is there any reason we can't use "error" here?

> +				free(submod_gitdir_path);
> +				return 1;
> +			} else {
> +				printf(_("Reactivating local git directory for "
> +					 "submodule '%s'\n"), add_data->sm_name);
> +			}
> +		}
> +		free(submod_gitdir_path);
> +
> +		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
> +			     add_data->sm_name, "--url", add_data->realrepo, NULL);
> +		if (add_data->quiet)
> +			strvec_push(&clone_args, "--quiet");
> +		if (add_data->progress)
> +			strvec_push(&clone_args, "--progress");
> +		if (add_data->prefix)
> +			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
> +		if (add_data->reference_path)
> +			strvec_pushl(&clone_args, "--reference",
> +				     add_data->reference_path, NULL);
> +		if (add_data->dissociate)
> +			strvec_push(&clone_args, "--dissociate");
> +		if (add_data->depth >= 0)
> +			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
> +
> +		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
> +			strvec_clear(&clone_args);
> +			return -1;
> +		}
> +		strvec_clear(&clone_args);
> +
> +		prepare_submodule_repo_env(&cp.env_array);
> +		cp.git_cmd = 1;
> +		cp.dir = add_data->sm_path;
> +		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
> +
> +		if (add_data->branch) {
> +			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
> +			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
> +		}
> +
> +		if (run_command(&cp))
> +			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
> +	}
> +	return 0;
> +}
> +
> +static int add_clone(int argc, const char **argv, const char *prefix)
> +{
> +	int force = 0, quiet = 0, dissociate = 0, progress = 0;
> +	struct add_data add_data = ADD_DATA_INIT;
> +
> +	struct option options[] = {
> +		OPT_STRING('b', "branch", &add_data.branch,
> +			   N_("branch"),
> +			   N_("branch of repository to checkout on cloning")),
> +		OPT_STRING(0, "prefix", &add_data.prefix,
> +			   N_("path"),
> +			   N_("alternative anchor for relative paths")),
> +		OPT_STRING(0, "path", &add_data.sm_path,
> +			   N_("path"),
> +			   N_("where the new submodule will be cloned to")),
> +		OPT_STRING(0, "name", &add_data.sm_name,
> +			   N_("string"),
> +			   N_("name of the new submodule")),
> +		OPT_STRING(0, "url", &add_data.realrepo,
> +			   N_("string"),
> +			   N_("url where to clone the submodule from")),
> +		OPT_STRING(0, "reference", &add_data.reference_path,
> +			   N_("repo"),
> +			   N_("reference repository")),
> +		OPT_BOOL(0, "dissociate", &dissociate,
> +			 N_("use --reference only while cloning")),
> +		OPT_INTEGER(0, "depth", &add_data.depth,
> +			    N_("depth for shallow clones")),
> +		OPT_BOOL(0, "progress", &progress,
> +			 N_("force cloning progress")),
> +		OPT_BOOL('f', "force", &force,
> +			 N_("allow adding an otherwise ignored submodule path")),

We have OPT__FORCE, too.

> +		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),

And, please downcase "Suppress".

> +		OPT_END()
> +	};
> +
> +	const char *const usage[] = {
> +		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
> +		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
> +		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),

I think it's too crowded here, I guess it's better to write:

	N_("git submodule--helper add-clone [<options>...] --url <url> --path <path> --name <name>"),

> +		NULL
> +	};
> +
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);

From above usage, I think url, path, name is required, should we have a check for them, here?
> +
> +	add_data.progress = !!progress;
> +	add_data.dissociate = !!dissociate;
> +	add_data.force = !!force;
> +	add_data.quiet = !!quiet;
> +
> +	if (add_submodule(&add_data))
> +		return 1;
> +
> +	return 0;
> +}
> +
>  #define SUPPORT_SUPER_PREFIX (1<<0)
>  
>  struct cmd_struct {
> @@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
>  	{"list", module_list, 0},
>  	{"name", module_name, 0},
>  	{"clone", module_clone, 0},
> +	{"add-clone", add_clone, 0},
>  	{"update-module-mode", module_update_module_mode, 0},
>  	{"update-clone", update_clone, 0},
>  	{"ensure-core-worktree", ensure_core_worktree, 0},
> diff --git a/git-submodule.sh b/git-submodule.sh
> index 4678378424..f71e1e5495 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -241,43 +241,7 @@ cmd_add()
>  		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
>  	fi
>  
> -	# perhaps the path exists and is already a git repo, else clone it
> -	if test -e "$sm_path"
> -	then
> -		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
> -		then
> -			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
> -		else
> -			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
> -		fi
> -
> -	else
> -		if test -d ".git/modules/$sm_name"
> -		then
> -			if test -z "$force"
> -			then
> -				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
> -				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
> -				die "$(eval_gettextln "\
> -If you want to reuse this local git directory instead of cloning again from
> -  \$realrepo
> -use the '--force' option. If the local git directory is not the correct repo
> -or you are unsure what this means choose another name with the '--name' option.")"
> -			else
> -				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
> -			fi
> -		fi
> -		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> -		(
> -			sanitize_submodule_env
> -			cd "$sm_path" &&
> -			# ash fails to wordsplit ${branch:+-b "$branch"...}
> -			case "$branch" in
> -			'') git checkout -f -q ;;
> -			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> -			esac
> -		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
> -	fi
> +	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
>  	git config submodule."$sm_name".url "$realrepo"
>  
>  	git add --no-warn-embedded-repo $force "$sm_path" ||
> -- 
> 2.31.1
> 

-- 
Danh

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Junio C Hamano]

Just a bit of random comments, leaving the full review to mentors.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..c9cb535312 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  	return !!ret;
>  }
>  
> +struct add_data {
> +	const char *prefix;
> +	const char *branch;
> +	const char *reference_path;
> +	const char *sm_path;
> +	const char *sm_name;
> +	const char *repo;
> +	const char *realrepo;
> +	int depth;
> +	unsigned int force: 1;
> +	unsigned int quiet: 1;
> +	unsigned int progress: 1;
> +	unsigned int dissociate: 1;
> +};
> +#define ADD_DATA_INIT { .depth = -1 }
> +
> +static char *parse_token(char **begin, const char *end, int *tok_len)
> +{
> +	char *tok_start, *pos = *begin;

Make it a habit to have a blank line between the initial block
of declarations and the first statement.

> +	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
> +		pos++;
> +	tok_start = *begin;
> +	*tok_len = pos - *begin;
> +	*begin = pos + 1;
> +	return tok_start;
> +}
> +static char *get_next_line(char *const begin, const char *const end)
> +{
> +	char *pos = begin;
> +	while (pos != end && *pos++ != '\n');

Write an empty loop on two lines, like this:

	while (... condition ...)
		; /* keep scanning */

If there is a NUL byte between begin and end, this keeps going and
the resulting string will contain one.  Is that a problem?

> +	return pos;
> +}

In general, this project is mature enough that we should question
ourselves if there is already a suitable line parser we can reuse
when tempted to write another one.

> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
> +{
> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
> +	struct strbuf sb_remote_out = STRBUF_INIT;
> +
> +	cp_remote.git_cmd = 1;
> +	strvec_pushf(&cp_remote.env_array,
> +		     "GIT_DIR=%s", git_dir_path);
> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
> +		char *line;
> +		char *begin = sb_remote_out.buf;
> +		char *end = sb_remote_out.buf + sb_remote_out.len;
> +		while (begin != end && (line = get_next_line(begin, end))) {

OK, so this tries to parse output from "git remote -v", so NUL will
not be an issue at all.  We will get a string that is NUL terminated
and has zero or more lines, terminated with LFs.

If that is the case, I think it is far easier to read without
a custom get-next-line wrapper, e.g.

	for (this_line = begin;
	     *this_line;
	     this_line = next_line) {
		next_line = strchrnul(this_line, '\n');
		... process bytes between this_line..next_line ...
	}                

> +			int namelen = 0, urllen = 0, taillen = 0;
> +			char *name = parse_token(&begin, line, &namelen);

Similarly, consider if strcspn() is useful in implementing
parse_token().  See how existing code uses the standard system
function with

	$ git grep strcspn \*.c

> +			char *url = parse_token(&begin, line, &urllen);
> +			char *tail = parse_token(&begin, line, &taillen);
> +			if (!memcmp(tail, "(fetch)", 7))

At this point do we know there are enough number of bytes after
tail[0] to allow us to do this comparison safely?  Otherwise,

			if (starts_with(tail, "(fetch)")

may be preferrable.

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

On 08-Jun-2021, at 18:02, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> 
>> [...]
>> +static char *get_next_line(char *const begin, const char *const end)
>> +{
>> +	char *pos = begin;
>> +	while (pos != end && *pos++ != '\n');
>> +	return pos;
>> +}
> 
> On my first glance, this function looks like a reinvention of strchr(3).
> Except that, this function also has a second parameter for "end".
> Maybe it has a specical use-case?
> 
>> +
>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
>> +{
>> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
>> +	struct strbuf sb_remote_out = STRBUF_INIT;
>> +
>> +	cp_remote.git_cmd = 1;
>> +	strvec_pushf(&cp_remote.env_array,
>> +		     "GIT_DIR=%s", git_dir_path);
>> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
>> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
>> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
>> +		char *line;
>> +		char *begin = sb_remote_out.buf;
>> +		char *end = sb_remote_out.buf + sb_remote_out.len;
>> +		while (begin != end && (line = get_next_line(begin, end))) {
> 
> And this is the only use-case.  Because you also want to check if you
> reached the last token or not.  I guess you really meant to write:
> 
> 	while ((line = strchr(begin, '\n')) != NULL) {
> 
> Anyway, I would name the "line" variable as "nextline"
> 
>> +			int namelen = 0, urllen = 0, taillen = 0;
>> +			char *name = parse_token(&begin, line, &namelen);
>> +			char *url = parse_token(&begin, line, &urllen);
>> +			char *tail = parse_token(&begin, line, &taillen);
>> +			if (!memcmp(tail, "(fetch)", 7))
>> +				fprintf(output, "  %.*s\t%.*s\n",
>> +					namelen, name, urllen, url);
> 
> I think this whole block is better replaced with strip_suffix_mem and
> fprintf.
> 
> Overral I would replace the block inside capture_command with:
> 
> -----8<-----
> 	char *nextline;
> 	char *line = sb_remote_out.buf;
> 	while ((nextline = strchr(line, '\n')) != NULL) {
> 		size_t len = nextline - line;
> 		if (strip_suffix_mem(line, &len, "(fetch)"))
> 			fprintf(output, "  %.*s\n", (int)len, line);
> 		line = nextline + 1;
> 	}
> ---->8-----
> 
> And get rid of parse_token and get_next_line functions.

That looks much simpler. Thanks!

I realised that all the token parsing I do is not really necessary.
What I really want to do is "If this line ends with '(fetch)',
print it, but without the '(fetch)'", and I think your version
captures that succinctly.

>> +		}
>> +	}
>> +
>> +	strbuf_release(&sb_remote_out);
>> +}
>> +
>> +static int add_submodule(const struct add_data *add_data)
>> +{
>> +	char *submod_gitdir_path;
>> +	/* perhaps the path already exists and is already a git repo, else clone it */
>> +	if (is_directory(add_data->sm_path)) {
>> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
>> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
>> +			printf(_("Adding existing path at '%s' to index\n"),
>> +			       add_data->sm_path);
>> +		else
>> +			die(_("'%s' already exists and is not a valid git repo"),
>> +			    add_data->sm_path);
>> +		free(submod_gitdir_path);
>> +	} else {
>> +		struct strvec clone_args = STRVEC_INIT;
>> +		struct child_process cp = CHILD_PROCESS_INIT;
>> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
>> +
>> +		if (is_directory(submod_gitdir_path)) {
>> +			if (!add_data->force) {
>> +				error(_("A git directory for '%s' is found "
>> +					"locally with remote(s):"), add_data->sm_name);
> 
> We don't capitalise first character of error message.
> IOW, downcase "A git".

Got it.

> Well, it's bug-for-bug with shell implementation, so it doesn't matter much, anyway.

While it is meant to be a faithful implementation, I think this
is a good opportunity to make minor style fixes.

>> +				show_fetch_remotes(stderr, add_data->sm_name,
>> +						   submod_gitdir_path);
>> +				fprintf(stderr,
>> +					_("If you want to reuse this local git "
>> +					  "directory instead of cloning again from\n"
>> +					  "  %s\n"
>> +					  "use the '--force' option. If the local git "
>> +					  "directory is not the correct repo\n"
>> +					  "or if you are unsure what this means, choose "
>> +					  "another name with the '--name' option.\n"),
>> +					add_data->realrepo);
> 
> Is there any reason we can't use "error" here?

The message in its entirety looks like this:

error: A git directory for 'test' is found locally with remote(s):
  origin	git@github.com:tfidfwastaken/abc.git
If you want to reuse this local git directory instead of cloning again from
  git@github.com:tfidfwastaken/test.git
use the '--force' option. If the local git directory is not the correct repo
or if you are unsure what this means, choose another name with the '--name' option.

Since the 'error:' is already there in the first line, having it
prepended before 'If you want to reuse...' felt redundant to me.

Besides, it's more of an informational message about what a user
can do next, rather than a message that signifies an error.

If there is a preferred convention or label for such messages,
I can use that. The shell version did not have any such thing though.

>> [...]
>> +	struct option options[] = {
>> +		OPT_STRING('b', "branch", &add_data.branch,
>> +			   N_("branch"),
>> +			   N_("branch of repository to checkout on cloning")),
>> +		OPT_STRING(0, "prefix", &add_data.prefix,
>> +			   N_("path"),
>> +			   N_("alternative anchor for relative paths")),
>> +		OPT_STRING(0, "path", &add_data.sm_path,
>> +			   N_("path"),
>> +			   N_("where the new submodule will be cloned to")),
>> +		OPT_STRING(0, "name", &add_data.sm_name,
>> +			   N_("string"),
>> +			   N_("name of the new submodule")),
>> +		OPT_STRING(0, "url", &add_data.realrepo,
>> +			   N_("string"),
>> +			   N_("url where to clone the submodule from")),
>> +		OPT_STRING(0, "reference", &add_data.reference_path,
>> +			   N_("repo"),
>> +			   N_("reference repository")),
>> +		OPT_BOOL(0, "dissociate", &dissociate,
>> +			 N_("use --reference only while cloning")),
>> +		OPT_INTEGER(0, "depth", &add_data.depth,
>> +			    N_("depth for shallow clones")),
>> +		OPT_BOOL(0, "progress", &progress,
>> +			 N_("force cloning progress")),
>> +		OPT_BOOL('f', "force", &force,
>> +			 N_("allow adding an otherwise ignored submodule path")),
> 
> We have OPT__FORCE, too.

Will switch over to that.

>> +		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> 
> And, please downcase "Suppress".

OK.

>> +		OPT_END()
>> +	};
>> +
>> +	const char *const usage[] = {
>> +		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
>> +		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
>> +		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
> 
> I think it's too crowded here, I guess it's better to write:
> 
> 	N_("git submodule--helper add-clone [<options>...] --url <url> --path <path> --name <name>"),

OK. It shouldn't be an issue to shorten it, because this is not
user-facing, and is only ever used within 'cmd_add()'.

>> +		NULL
>> +	};
>> +
>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> 
> From above usage, I think url, path, name is required, should we have a check for them, here?

We could. The reason why I was not too rigorous about this is
because I plan to eliminate the shell interface for this helper
eventually and call add-clone from within C, in the next few
patches.

But this is a small ask, and I can just add a quick check just
to be extra safe, so I'll do it.

>> +
>> +	add_data.progress = !!progress;
>> +	add_data.dissociate = !!dissociate;
>> +	add_data.force = !!force;
>> +	add_data.quiet = !!quiet;
>> +
>> +	if (add_submodule(&add_data))
>> +		return 1;
>> +
>> +	return 0;
>> +}
>> +
>> #define SUPPORT_SUPER_PREFIX (1<<0)
>> 
>> struct cmd_struct {
>> @@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
>> 	{"list", module_list, 0},
>> 	{"name", module_name, 0},
>> 	{"clone", module_clone, 0},
>> +	{"add-clone", add_clone, 0},
>> 	{"update-module-mode", module_update_module_mode, 0},
>> 	{"update-clone", update_clone, 0},
>> 	{"ensure-core-worktree", ensure_core_worktree, 0},
>> diff --git a/git-submodule.sh b/git-submodule.sh
>> index 4678378424..f71e1e5495 100755
>> --- a/git-submodule.sh
>> +++ b/git-submodule.sh
>> @@ -241,43 +241,7 @@ cmd_add()
>> 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
>> 	fi
>> 
>> -	# perhaps the path exists and is already a git repo, else clone it
>> -	if test -e "$sm_path"
>> -	then
>> -		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
>> -		then
>> -			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
>> -		else
>> -			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
>> -		fi
>> -
>> -	else
>> -		if test -d ".git/modules/$sm_name"
>> -		then
>> -			if test -z "$force"
>> -			then
>> -				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
>> -				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
>> -				die "$(eval_gettextln "\
>> -If you want to reuse this local git directory instead of cloning again from
>> -  \$realrepo
>> -use the '--force' option. If the local git directory is not the correct repo
>> -or you are unsure what this means choose another name with the '--name' option.")"
>> -			else
>> -				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
>> -			fi
>> -		fi
>> -		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
>> -		(
>> -			sanitize_submodule_env
>> -			cd "$sm_path" &&
>> -			# ash fails to wordsplit ${branch:+-b "$branch"...}
>> -			case "$branch" in
>> -			'') git checkout -f -q ;;
>> -			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
>> -			esac
>> -		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
>> -	fi
>> +	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
>> 	git config submodule."$sm_name".url "$realrepo"
>> 
>> 	git add --no-warn-embedded-repo $force "$sm_path" ||
>> -- 
>> 2.31.1
>> 
> 
> -- 
> Danh

Thanks for the comments!

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

On 09-Jun-2021, at 09:54, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Just a bit of random comments, leaving the full review to mentors.
> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index d55f6262e9..c9cb535312 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>> 	return !!ret;
>> }
>> 
>> +struct add_data {
>> +	const char *prefix;
>> +	const char *branch;
>> +	const char *reference_path;
>> +	const char *sm_path;
>> +	const char *sm_name;
>> +	const char *repo;
>> +	const char *realrepo;
>> +	int depth;
>> +	unsigned int force: 1;
>> +	unsigned int quiet: 1;
>> +	unsigned int progress: 1;
>> +	unsigned int dissociate: 1;
>> +};
>> +#define ADD_DATA_INIT { .depth = -1 }
>> +
>> +static char *parse_token(char **begin, const char *end, int *tok_len)
>> +{
>> +	char *tok_start, *pos = *begin;
> 
> Make it a habit to have a blank line between the initial block
> of declarations and the first statement.
> 
>> +	while (pos != end && (*pos != ' ' && *pos != '\t' && *pos != '\n'))
>> +		pos++;
>> +	tok_start = *begin;
>> +	*tok_len = pos - *begin;
>> +	*begin = pos + 1;
>> +	return tok_start;
>> +}
>> +static char *get_next_line(char *const begin, const char *const end)
>> +{
>> +	char *pos = begin;
>> +	while (pos != end && *pos++ != '\n');
> 
> Write an empty loop on two lines, like this:
> 
> 	while (... condition ...)
> 		; /* keep scanning */

OK.

> If there is a NUL byte between begin and end, this keeps going and
> the resulting string will contain one.  Is that a problem?
> 
>> +	return pos;
>> +}
> 
> In general, this project is mature enough that we should question
> ourselves if there is already a suitable line parser we can reuse
> when tempted to write another one.

I will keep this in mind.

>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
>> +{
>> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
>> +	struct strbuf sb_remote_out = STRBUF_INIT;
>> +
>> +	cp_remote.git_cmd = 1;
>> +	strvec_pushf(&cp_remote.env_array,
>> +		     "GIT_DIR=%s", git_dir_path);
>> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
>> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
>> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
>> +		char *line;
>> +		char *begin = sb_remote_out.buf;
>> +		char *end = sb_remote_out.buf + sb_remote_out.len;
>> +		while (begin != end && (line = get_next_line(begin, end))) {
> 
> OK, so this tries to parse output from "git remote -v", so NUL will
> not be an issue at all.  We will get a string that is NUL terminated
> and has zero or more lines, terminated with LFs.
> 
> If that is the case, I think it is far easier to read without
> a custom get-next-line wrapper, e.g.
> 
> 	for (this_line = begin;
> 	     *this_line;
> 	     this_line = next_line) {
> 		next_line = strchrnul(this_line, '\n');
> 		... process bytes between this_line..next_line ...
> 	}                
> 
>> +			int namelen = 0, urllen = 0, taillen = 0;
>> +			char *name = parse_token(&begin, line, &namelen);
> 
> Similarly, consider if strcspn() is useful in implementing
> parse_token().  See how existing code uses the standard system
> function with
> 
> 	$ git grep strcspn \*.c
> 
>> +			char *url = parse_token(&begin, line, &urllen);
>> +			char *tail = parse_token(&begin, line, &taillen);
>> +			if (!memcmp(tail, "(fetch)", 7))
> 
> At this point do we know there are enough number of bytes after
> tail[0] to allow us to do this comparison safely?  Otherwise,
> 
> 			if (starts_with(tail, "(fetch)")
> 
> may be preferrable.

This solution is definitely an improvement over what I was doing.

That said, I like Danh's suggestion[1] more, because it eliminates the
need for parsing tokens entirely.

The fundamental thing that piece of code was meant to do is:

"If this line ends with '(fetch)', print the line, but without the '(fetch)'"

Parsing tokens only to put them back together through fprintf() may
not be necessary for this usage, so using strchr() with
strip_suffix_mem() should do the trick.

[1] https://lore.kernel.org/git/YL9jTFAoEBP+mDA2@danh.dev/

Thanks for the comments :^)

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Đoàn Trần Công Danh]

On 2021-06-09 16:01:40+0530, Atharva Raykar <raykar.ath@gmail.com> wrote:
> On 08-Jun-2021, at 18:02, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
> > 
> >> [...]
> >> +static char *get_next_line(char *const begin, const char *const end)
> >> +{
> >> +	char *pos = begin;
> >> +	while (pos != end && *pos++ != '\n');
> >> +	return pos;
> >> +}
> > 
> > On my first glance, this function looks like a reinvention of strchr(3).
> > Except that, this function also has a second parameter for "end".
> > Maybe it has a specical use-case?
> > 
> >> +
> >> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
> >> +{
> >> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
> >> +	struct strbuf sb_remote_out = STRBUF_INIT;
> >> +
> >> +	cp_remote.git_cmd = 1;
> >> +	strvec_pushf(&cp_remote.env_array,
> >> +		     "GIT_DIR=%s", git_dir_path);
> >> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
> >> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
> >> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
> >> +		char *line;
> >> +		char *begin = sb_remote_out.buf;
> >> +		char *end = sb_remote_out.buf + sb_remote_out.len;
> >> +		while (begin != end && (line = get_next_line(begin, end))) {
> > 
> > And this is the only use-case.  Because you also want to check if you
> > reached the last token or not.  I guess you really meant to write:
> > 
> > 	while ((line = strchr(begin, '\n')) != NULL) {
> > 
> > Anyway, I would name the "line" variable as "nextline"
> > 
> >> +			int namelen = 0, urllen = 0, taillen = 0;
> >> +			char *name = parse_token(&begin, line, &namelen);
> >> +			char *url = parse_token(&begin, line, &urllen);
> >> +			char *tail = parse_token(&begin, line, &taillen);
> >> +			if (!memcmp(tail, "(fetch)", 7))
> >> +				fprintf(output, "  %.*s\t%.*s\n",
> >> +					namelen, name, urllen, url);
> > 
> > I think this whole block is better replaced with strip_suffix_mem and
> > fprintf.
> > 
> > Overral I would replace the block inside capture_command with:
> > 
> > -----8<-----
> > 	char *nextline;
> > 	char *line = sb_remote_out.buf;
> > 	while ((nextline = strchr(line, '\n')) != NULL) {
> > 		size_t len = nextline - line;
> > 		if (strip_suffix_mem(line, &len, "(fetch)"))
> > 			fprintf(output, "  %.*s\n", (int)len, line);

Fix-up for my suggestion:

To be bug-for-bug with shell implementation, it should be:

		if (strip_suffix_mem(line, &len, " (fetch)"))

> > 		line = nextline + 1;
> > 	}
> > ---->8-----
> > 
> > And get rid of parse_token and get_next_line functions.
> 
> That looks much simpler. Thanks!
> 
> I realised that all the token parsing I do is not really necessary.
> What I really want to do is "If this line ends with '(fetch)',
> print it, but without the '(fetch)'", and I think your version
> captures that succinctly.
> 
> >> +		}
> >> +	}
> >> +
> >> +	strbuf_release(&sb_remote_out);
> >> +}
> >> +
> >> +static int add_submodule(const struct add_data *add_data)
> >> +{
> >> +	char *submod_gitdir_path;
> >> +	/* perhaps the path already exists and is already a git repo, else clone it */
> >> +	if (is_directory(add_data->sm_path)) {
> >> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
> >> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
> >> +			printf(_("Adding existing path at '%s' to index\n"),
> >> +			       add_data->sm_path);
> >> +		else
> >> +			die(_("'%s' already exists and is not a valid git repo"),
> >> +			    add_data->sm_path);
> >> +		free(submod_gitdir_path);
> >> +	} else {
> >> +		struct strvec clone_args = STRVEC_INIT;
> >> +		struct child_process cp = CHILD_PROCESS_INIT;
> >> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
> >> +
> >> +		if (is_directory(submod_gitdir_path)) {
> >> +			if (!add_data->force) {
> >> +				error(_("A git directory for '%s' is found "
> >> +					"locally with remote(s):"), add_data->sm_name);
> > 
> > We don't capitalise first character of error message.
> > IOW, downcase "A git".
> 
> Got it.
> 
> > Well, it's bug-for-bug with shell implementation, so it doesn't matter much, anyway.
> 
> While it is meant to be a faithful implementation, I think this
> is a good opportunity to make minor style fixes.
> 
> >> +				show_fetch_remotes(stderr, add_data->sm_name,
> >> +						   submod_gitdir_path);
> >> +				fprintf(stderr,
> >> +					_("If you want to reuse this local git "
> >> +					  "directory instead of cloning again from\n"
> >> +					  "  %s\n"
> >> +					  "use the '--force' option. If the local git "
> >> +					  "directory is not the correct repo\n"
> >> +					  "or if you are unsure what this means, choose "
> >> +					  "another name with the '--name' option.\n"),
> >> +					add_data->realrepo);
> > 
> > Is there any reason we can't use "error" here?
> 
> The message in its entirety looks like this:
> 
> error: A git directory for 'test' is found locally with remote(s):
>   origin	git@github.com:tfidfwastaken/abc.git
> If you want to reuse this local git directory instead of cloning again from
>   git@github.com:tfidfwastaken/test.git
> use the '--force' option. If the local git directory is not the correct repo
> or if you are unsure what this means, choose another name with the '--name' option.
> 
> Since the 'error:' is already there in the first line, having it
> prepended before 'If you want to reuse...' felt redundant to me.
> 
> Besides, it's more of an informational message about what a user
> can do next, rather than a message that signifies an error.
> 
> If there is a preferred convention or label for such messages,
> I can use that. The shell version did not have any such thing though.
> 
> >> [...]
> >> +	struct option options[] = {
> >> +		OPT_STRING('b', "branch", &add_data.branch,
> >> +			   N_("branch"),
> >> +			   N_("branch of repository to checkout on cloning")),
> >> +		OPT_STRING(0, "prefix", &add_data.prefix,
> >> +			   N_("path"),
> >> +			   N_("alternative anchor for relative paths")),
> >> +		OPT_STRING(0, "path", &add_data.sm_path,
> >> +			   N_("path"),
> >> +			   N_("where the new submodule will be cloned to")),
> >> +		OPT_STRING(0, "name", &add_data.sm_name,
> >> +			   N_("string"),
> >> +			   N_("name of the new submodule")),
> >> +		OPT_STRING(0, "url", &add_data.realrepo,
> >> +			   N_("string"),
> >> +			   N_("url where to clone the submodule from")),
> >> +		OPT_STRING(0, "reference", &add_data.reference_path,
> >> +			   N_("repo"),
> >> +			   N_("reference repository")),
> >> +		OPT_BOOL(0, "dissociate", &dissociate,
> >> +			 N_("use --reference only while cloning")),
> >> +		OPT_INTEGER(0, "depth", &add_data.depth,
> >> +			    N_("depth for shallow clones")),
> >> +		OPT_BOOL(0, "progress", &progress,
> >> +			 N_("force cloning progress")),
> >> +		OPT_BOOL('f', "force", &force,
> >> +			 N_("allow adding an otherwise ignored submodule path")),
> > 
> > We have OPT__FORCE, too.
> 
> Will switch over to that.
> 
> >> +		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
> > 
> > And, please downcase "Suppress".
> 
> OK.
> 
> >> +		OPT_END()
> >> +	};
> >> +
> >> +	const char *const usage[] = {
> >> +		N_("git submodule--helper add-clone [--prefix=<path>] [--quiet] [--force] "
> >> +		   "[--reference <repository>] [--depth <depth>] [-b|--branch <branch>]"
> >> +		   "[--progress] [--dissociate] --url <url> --path <path> --name <name>"),
> > 
> > I think it's too crowded here, I guess it's better to write:
> > 
> > 	N_("git submodule--helper add-clone [<options>...] --url <url> --path <path> --name <name>"),
> 
> OK. It shouldn't be an issue to shorten it, because this is not
> user-facing, and is only ever used within 'cmd_add()'.
> 
> >> +		NULL
> >> +	};
> >> +
> >> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> > 
> > From above usage, I think url, path, name is required, should we have a check for them, here?
> 
> We could. The reason why I was not too rigorous about this is
> because I plan to eliminate the shell interface for this helper
> eventually and call add-clone from within C, in the next few
> patches.
> 
> But this is a small ask, and I can just add a quick check just
> to be extra safe, so I'll do it.
> 
> >> +
> >> +	add_data.progress = !!progress;
> >> +	add_data.dissociate = !!dissociate;
> >> +	add_data.force = !!force;
> >> +	add_data.quiet = !!quiet;
> >> +
> >> +	if (add_submodule(&add_data))
> >> +		return 1;
> >> +
> >> +	return 0;
> >> +}
> >> +
> >> #define SUPPORT_SUPER_PREFIX (1<<0)
> >> 
> >> struct cmd_struct {
> >> @@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {
> >> 	{"list", module_list, 0},
> >> 	{"name", module_name, 0},
> >> 	{"clone", module_clone, 0},
> >> +	{"add-clone", add_clone, 0},
> >> 	{"update-module-mode", module_update_module_mode, 0},
> >> 	{"update-clone", update_clone, 0},
> >> 	{"ensure-core-worktree", ensure_core_worktree, 0},
> >> diff --git a/git-submodule.sh b/git-submodule.sh
> >> index 4678378424..f71e1e5495 100755
> >> --- a/git-submodule.sh
> >> +++ b/git-submodule.sh
> >> @@ -241,43 +241,7 @@ cmd_add()
> >> 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
> >> 	fi
> >> 
> >> -	# perhaps the path exists and is already a git repo, else clone it
> >> -	if test -e "$sm_path"
> >> -	then
> >> -		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
> >> -		then
> >> -			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
> >> -		else
> >> -			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
> >> -		fi
> >> -
> >> -	else
> >> -		if test -d ".git/modules/$sm_name"
> >> -		then
> >> -			if test -z "$force"
> >> -			then
> >> -				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
> >> -				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
> >> -				die "$(eval_gettextln "\
> >> -If you want to reuse this local git directory instead of cloning again from
> >> -  \$realrepo
> >> -use the '--force' option. If the local git directory is not the correct repo
> >> -or you are unsure what this means choose another name with the '--name' option.")"
> >> -			else
> >> -				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
> >> -			fi
> >> -		fi
> >> -		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> >> -		(
> >> -			sanitize_submodule_env
> >> -			cd "$sm_path" &&
> >> -			# ash fails to wordsplit ${branch:+-b "$branch"...}
> >> -			case "$branch" in
> >> -			'') git checkout -f -q ;;
> >> -			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
> >> -			esac
> >> -		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
> >> -	fi
> >> +	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
> >> 	git config submodule."$sm_name".url "$realrepo"
> >> 
> >> 	git add --no-warn-embedded-repo $force "$sm_path" ||
> >> -- 
> >> 2.31.1
> >> 
> > 
> > -- 
> > Danh
> 
> Thanks for the comments!

-- 
Danh

Re: [GSoC] [PATCH v2 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

On 09-Jun-2021, at 18:36, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:
>>> 
>>> Overral I would replace the block inside capture_command with:
>>> 
>>> -----8<-----
>>> 	char *nextline;
>>> 	char *line = sb_remote_out.buf;
>>> 	while ((nextline = strchr(line, '\n')) != NULL) {
>>> 		size_t len = nextline - line;
>>> 		if (strip_suffix_mem(line, &len, "(fetch)"))
>>> 			fprintf(output, "  %.*s\n", (int)len, line);
> 
> Fix-up for my suggestion:
> 
> To be bug-for-bug with shell implementation, it should be:
> 
> 		if (strip_suffix_mem(line, &len, " (fetch)"))

That is very subtle, and I would have definitely missed it.
Thanks.

[GSoC] [PATCH v3 0/2] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

Notable changes since v2:
 - In show_fetch_remotes(), remove the get_next_line() and parse_token()
   in favour of a simpler solution that uses strchr() and
   strip_suffix_mem()
 - Use OPT__FORCE() instead of OPT_BOOL for '--force' flags
 - Add checks for number of arguments in the helper subcommands
 - Simplify usage string for add-clone and make error messages start in
   lowercase.

Atharva Raykar (2):
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 299 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  66 +-------
 2 files changed, 301 insertions(+), 64 deletions(-)

-- 
2.31.1

[PATCH v3 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.

This is part of a series of changes that will result in all of 'submodule add'
being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 builtin/submodule--helper.c | 180 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +-------
 2 files changed, 181 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..21b2e9fb14 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2745,6 +2745,185 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *next_line;
+		char *line = sb_remote_out.buf;
+		while ((next_line = strchr(line, '\n')) != NULL) {
+			size_t len = next_line - line;
+			if (strip_suffix_mem(line, &len, " (fetch)"))
+				fprintf(output, "  %.*s\n", (int)len, line);
+			line = next_line + 1;
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
+			printf(_("Adding existing path at '%s' to index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct strvec clone_args = STRVEC_INIT;
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				error(_("a git directory for '%s' is found "
+					"locally with remote(s):"), add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				fprintf(stderr,
+					_("If you want to reuse this local git "
+					  "directory instead of cloning again from\n"
+					  "  %s\n"
+					  "use the '--force' option. If the local git "
+					  "directory is not the correct repo\n"
+					  "or if you are unsure what this means, choose "
+					  "another name with the '--name' option.\n"),
+					add_data->realrepo);
+				free(submod_gitdir_path);
+				return 1;
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
+			     add_data->sm_name, "--url", add_data->realrepo, NULL);
+		if (add_data->quiet)
+			strvec_push(&clone_args, "--quiet");
+		if (add_data->progress)
+			strvec_push(&clone_args, "--progress");
+		if (add_data->prefix)
+			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
+		if (add_data->reference_path)
+			strvec_pushl(&clone_args, "--reference",
+				     add_data->reference_path, NULL);
+		if (add_data->dissociate)
+			strvec_push(&clone_args, "--dissociate");
+		if (add_data->depth >= 0)
+			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
+
+		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
+			strvec_clear(&clone_args);
+			return -1;
+		}
+		strvec_clear(&clone_args);
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &add_data.prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [<options>...] "
+		   "--url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2757,6 +2936,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[PATCH v3 2/2] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +--------
 2 files changed, 120 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 21b2e9fb14..a50276138b 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2924,6 +2924,124 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists, activate = 0;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	key = xstrfmt("submodule.%s.path", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
+	free(key);
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->repo);
+	free(key);
+	if (add_data->branch) {
+		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
+		config_set_in_gitmodules_file_gently(key, add_data->branch);
+		free(key);
+	}
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}
+
+	/*
+	 * If submodule.active does not exist, or if the pathspec was unset,
+	 * we will activate this module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists || activate ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2937,6 +3055,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1

Re: [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand

[Junio C Hamano]

Atharva Raykar <raykar.ath@gmail.com> writes:

> Let's add a new "add-clone" subcommand to `git submodule--helper` with
> the goal of converting part of the shell code in git-submodule.sh
> related to `git submodule add` into C code. This new subcommand clones
> the repository that is to be added, and checks out to the appropriate
> branch.
>
> This is meant to be a faithful conversion that leaves the behaviour of
> 'submodule add' unchanged. The only minor change is that if a submodule name has
> been supplied with a name that clashes with a local submodule, the message shown
> to the user ("A git directory for 'foo' is found locally...") is prepended with
> "error" for clarity.
>
> This is part of a series of changes that will result in all of 'submodule add'
> being converted to C.
>
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> Mentored-by: Christian Couder <christian.couder@gmail.com>
> Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
> Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  builtin/submodule--helper.c | 180 ++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  38 +-------
>  2 files changed, 181 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..21b2e9fb14 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2745,6 +2745,185 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
>  	return !!ret;
>  }
>  
> +struct add_data {
> +	const char *prefix;
> +	const char *branch;
> +	const char *reference_path;
> +	const char *sm_path;
> +	const char *sm_name;
> +	const char *repo;
> +	const char *realrepo;
> +	int depth;
> +	unsigned int force: 1;
> +	unsigned int quiet: 1;
> +	unsigned int progress: 1;
> +	unsigned int dissociate: 1;
> +};
> +#define ADD_DATA_INIT { .depth = -1 }
> +
> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
> +{
> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
> +	struct strbuf sb_remote_out = STRBUF_INIT;
> +
> +	cp_remote.git_cmd = 1;
> +	strvec_pushf(&cp_remote.env_array,
> +		     "GIT_DIR=%s", git_dir_path);
> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
> +		char *next_line;
> +		char *line = sb_remote_out.buf;
> +		while ((next_line = strchr(line, '\n')) != NULL) {
> +			size_t len = next_line - line;
> +			if (strip_suffix_mem(line, &len, " (fetch)"))
> +				fprintf(output, "  %.*s\n", (int)len, line);
> +			line = next_line + 1;

Good.  That's a handy helper to use here.

> +		}
> +	}
> +
> +	strbuf_release(&sb_remote_out);
> +}
> +
> +static int add_submodule(const struct add_data *add_data)
> +{
> +	char *submod_gitdir_path;
> +
> +	/* perhaps the path already exists and is already a git repo, else clone it */
> +	if (is_directory(add_data->sm_path)) {
> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))

This is a faithful translation from the scripted Porcelain that did
not have access to our C internals that can do a lot better job at
deciding if a given "$path", which has ".git" directly or file below
it, is the top of a working tree that is governed by that ".git"
thing.

I wonder if setup.c::is_nonbare_repository_dir() can be used here.
You really want to see add_data->sm_path is the top of the working
tree of a submodule, so requiring only that ".git" thing is either
a directory or a file, which was the cheap thing the scripted version
could do without expending too much engineering effort, is a shame
when you are now writing in C and all the infrastructure to do much
better job is available to you.

> +			printf(_("Adding existing path at '%s' to index\n"),
> +			       add_data->sm_path);

Unintended change to the message here; you lost "the".

> +		else
> +			die(_("'%s' already exists and is not a valid git repo"),
> +			    add_data->sm_path);
> +		free(submod_gitdir_path);
> +	} else {
> +		struct strvec clone_args = STRVEC_INIT;
> +		struct child_process cp = CHILD_PROCESS_INIT;
> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
> +
> +		if (is_directory(submod_gitdir_path)) {
> +			if (!add_data->force) {
> +				error(_("a git directory for '%s' is found "
> +					"locally with remote(s):"), add_data->sm_name);

The original is a mere eval_gettextln, so a faithful conversion
would be to emit this to the standard error stream, without letting
error() to add "error: " prefix and downcasing "A git directory".

> +				show_fetch_remotes(stderr, add_data->sm_name,
> +						   submod_gitdir_path);
> +				fprintf(stderr,
> +					_("If you want to reuse this local git "
> +					  "directory instead of cloning again from\n"
> +					  "  %s\n"
> +					  "use the '--force' option. If the local git "
> +					  "directory is not the correct repo\n"
> +					  "or if you are unsure what this means, choose "
> +					  "another name with the '--name' option.\n"),
> +					add_data->realrepo);
> +				free(submod_gitdir_path);
> +				return 1;

The updated caller in the scripted Porcelain just silently exits
upon an error, so the above message that used to be given to die()
needs to be emitted by us like this.  You may want to do die(),
instead of fprintf(stderr) followed by return 1, to be a more
faithful conversion.

> +			} else {
> +				printf(_("Reactivating local git directory for "
> +					 "submodule '%s'\n"), add_data->sm_name);
> +			}
> +		}
> +		free(submod_gitdir_path);

And at this point, the original scripted Porcelain used to call "git
submodule--helper clone".

> +		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
> +			     add_data->sm_name, "--url", add_data->realrepo, NULL);
> +		if (add_data->quiet)
> +			strvec_push(&clone_args, "--quiet");
> +		if (add_data->progress)
> +			strvec_push(&clone_args, "--progress");
> +		if (add_data->prefix)
> +			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
> +		if (add_data->reference_path)
> +			strvec_pushl(&clone_args, "--reference",
> +				     add_data->reference_path, NULL);
> +		if (add_data->dissociate)
> +			strvec_push(&clone_args, "--dissociate");
> +		if (add_data->depth >= 0)
> +			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
> +
> +		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
> +			strvec_clear(&clone_args);
> +			return -1;
> +		}
> +		strvec_clear(&clone_args);

All of the above does the equivalent of it, but having to "unparse"
members of the add_data structure and stuff clone_args, only to be
parsed by parse_options() call in module_clone(), is somewhat
unsatisfactory.  I think this series would be helped greatly if
there were an additional step that splits module_clone() into two,
factoring out everything that happens after module_clone() calls
parse_options() into a helper function.  module_clone() will call
that helper function, and then at this step, instead of the above
"unparse and then call module_clone()", this place will call the
same helper function.

In any case, after doing "git submodule--helper clone", the original
scripted version does "git checkout" in the newly cloned working
tree.  I wonder if the "module_clone" is done with "--no-checkout"
option---cloning and letting a commit checked out, and checking out
another commit immediately after it, feels somewhat wasteful.

> +		prepare_submodule_repo_env(&cp.env_array);
> +		cp.git_cmd = 1;
> +		cp.dir = add_data->sm_path;
> +		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
> +
> +		if (add_data->branch) {
> +			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
> +			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
> +		}
> +
> +		if (run_command(&cp))
> +			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
> +	}
> +	return 0;
> +}

Other than all the commented parts, this looks fairly faithful
conversion from the part of the scripted Porcelain that has been
lost by the same patch.

> +static int add_clone(int argc, const char **argv, const char *prefix)
> +{
> +	int force = 0, quiet = 0, dissociate = 0, progress = 0;
> +	struct add_data add_data = ADD_DATA_INIT;
> + ...
> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
> +
> +	if (argc != 0)
> +		usage_with_options(usage, options);
> +
> +	add_data.progress = !!progress;
> +	add_data.dissociate = !!dissociate;
> +	add_data.force = !!force;
> +	add_data.quiet = !!quiet;
> +
> +	if (add_submodule(&add_data))
> +		return 1;
> +
> +	return 0;
> +}

As you wrote the add_clone() vs add_submodule() split of labour, the
reason this one is much more nicely organized than module_clone()
should be obvious to you.  This part of the design of this patch is
very well done.

The "additional step" I hinted earlier is exactly that---by making
the guts of the feature add_submodule() to work on already parsed
data and a separate wrapper add_clone() that interfaces to the
scripts to parse the command line arguments into a shape that is
more appropriate for the inner function, you can later call the
inner function add_submodule() directly without going through the
unparsing-then-parsing dance from other places.  If module_clone()
were split into two in a similar way, then the place to call it from
add_submodule() would become much cleaner.

Thanks.

Re: [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

On 11-Jun-2021, at 11:40, Junio C Hamano <gitster@pobox.com> wrote:
> 
>> [...]
>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
>> +{
>> +	struct child_process cp_remote = CHILD_PROCESS_INIT;
>> +	struct strbuf sb_remote_out = STRBUF_INIT;
>> +
>> +	cp_remote.git_cmd = 1;
>> +	strvec_pushf(&cp_remote.env_array,
>> +		     "GIT_DIR=%s", git_dir_path);
>> +	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
>> +	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
>> +	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
>> +		char *next_line;
>> +		char *line = sb_remote_out.buf;
>> +		while ((next_line = strchr(line, '\n')) != NULL) {
>> +			size_t len = next_line - line;
>> +			if (strip_suffix_mem(line, &len, " (fetch)"))
>> +				fprintf(output, "  %.*s\n", (int)len, line);
>> +			line = next_line + 1;
> 
> Good.  That's a handy helper to use here.
> 
>> +		}
>> +	}
>> +
>> +	strbuf_release(&sb_remote_out);
>> +}
>> +
>> +static int add_submodule(const struct add_data *add_data)
>> +{
>> +	char *submod_gitdir_path;
>> +
>> +	/* perhaps the path already exists and is already a git repo, else clone it */
>> +	if (is_directory(add_data->sm_path)) {
>> +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
>> +		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
> 
> This is a faithful translation from the scripted Porcelain that did
> not have access to our C internals that can do a lot better job at
> deciding if a given "$path", which has ".git" directly or file below
> it, is the top of a working tree that is governed by that ".git"
> thing.
> 
> I wonder if setup.c::is_nonbare_repository_dir() can be used here.
> You really want to see add_data->sm_path is the top of the working
> tree of a submodule, so requiring only that ".git" thing is either
> a directory or a file, which was the cheap thing the scripted version
> could do without expending too much engineering effort, is a shame
> when you are now writing in C and all the infrastructure to do much
> better job is available to you.

Yes, that looks like the right function to use. My version just
poorly reinvented that wheel. Will fix.

>> +			printf(_("Adding existing path at '%s' to index\n"),
>> +			       add_data->sm_path);
> 
> Unintended change to the message here; you lost "the".

Will change.

>> +		else
>> +			die(_("'%s' already exists and is not a valid git repo"),
>> +			    add_data->sm_path);
>> +		free(submod_gitdir_path);
>> +	} else {
>> +		struct strvec clone_args = STRVEC_INIT;
>> +		struct child_process cp = CHILD_PROCESS_INIT;
>> +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
>> +
>> +		if (is_directory(submod_gitdir_path)) {
>> +			if (!add_data->force) {
>> +				error(_("a git directory for '%s' is found "
>> +					"locally with remote(s):"), add_data->sm_name);
> 
> The original is a mere eval_gettextln, so a faithful conversion
> would be to emit this to the standard error stream, without letting
> error() to add "error: " prefix and downcasing "A git directory".

This was an intentional change on my part, which was motivated by
Sivaraam's message on a similar patch[1]. I agreed with his point
that it is better to be explicit to the user that an error has
occurred.

I think the larger concern I have to balance here is what level of
"faithfulness" should I be having with this conversion. My current
thoughts on this are:

 - Functionality and behaviour should be as similar as possible.

 - If there is an *obvious* bug that can be fixed in translation,
   then I should fix it.

 - If there are error messages that can be made more clear and
   consistent with the rest of the Porcelain, then I'll make the
   required change.

If you want me to turn up the faithfulness all the way up, so that
the error messages are identical word-for-word, I don't mind doing
that. I just personally felt that this conversion would be a good
opportunity to make the error messages consistent with the rest of
Git.

(I'll concede that I'm not sure what "consistent with the rest
of Git" would exactly mean, so I'll be happy if any reviewers will
guide me on those kind of changes).

If I do go down the "reproduce messages exactly" path, it should
also follow that I not include this warning in [2/2], as it is
not in the original shell script:

+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}

This was something that you suggested including, back when Shourya was
working on this[2]. So I used that as the signal that lightly altering
the messages emitted would be acceptable. I'm willing to change my
boundaries though.

[1] https://lore.kernel.org/git/5899bf6e-f61f-8a19-196d-d38d611dc037@gmail.com/
[2] https://lore.kernel.org/git/xmqqo8isxefz.fsf@gitster.c.googlers.com/

>> +				show_fetch_remotes(stderr, add_data->sm_name,
>> +						   submod_gitdir_path);
>> +				fprintf(stderr,
>> +					_("If you want to reuse this local git "
>> +					  "directory instead of cloning again from\n"
>> +					  "  %s\n"
>> +					  "use the '--force' option. If the local git "
>> +					  "directory is not the correct repo\n"
>> +					  "or if you are unsure what this means, choose "
>> +					  "another name with the '--name' option.\n"),
>> +					add_data->realrepo);
>> +				free(submod_gitdir_path);
>> +				return 1;
> 
> The updated caller in the scripted Porcelain just silently exits
> upon an error, so the above message that used to be given to die()
> needs to be emitted by us like this.  You may want to do die(),
> instead of fprintf(stderr) followed by return 1, to be a more
> faithful conversion.

Okay. Will change.

>> +			} else {
>> +				printf(_("Reactivating local git directory for "
>> +					 "submodule '%s'\n"), add_data->sm_name);
>> +			}
>> +		}
>> +		free(submod_gitdir_path);
> 
> And at this point, the original scripted Porcelain used to call "git
> submodule--helper clone".
> 
>> +		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
>> +			     add_data->sm_name, "--url", add_data->realrepo, NULL);
>> +		if (add_data->quiet)
>> +			strvec_push(&clone_args, "--quiet");
>> +		if (add_data->progress)
>> +			strvec_push(&clone_args, "--progress");
>> +		if (add_data->prefix)
>> +			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
>> +		if (add_data->reference_path)
>> +			strvec_pushl(&clone_args, "--reference",
>> +				     add_data->reference_path, NULL);
>> +		if (add_data->dissociate)
>> +			strvec_push(&clone_args, "--dissociate");
>> +		if (add_data->depth >= 0)
>> +			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
>> +
>> +		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
>> +			strvec_clear(&clone_args);
>> +			return -1;
>> +		}
>> +		strvec_clear(&clone_args);
> 
> All of the above does the equivalent of it, but having to "unparse"
> members of the add_data structure and stuff clone_args, only to be
> parsed by parse_options() call in module_clone(), is somewhat
> unsatisfactory.  I think this series would be helped greatly if
> there were an additional step that splits module_clone() into two,
> factoring out everything that happens after module_clone() calls
> parse_options() into a helper function.  module_clone() will call
> that helper function, and then at this step, instead of the above
> "unparse and then call module_clone()", this place will call the
> same helper function.

This is the right idea. I will do this.

> In any case, after doing "git submodule--helper clone", the original
> scripted version does "git checkout" in the newly cloned working
> tree.  I wonder if the "module_clone" is done with "--no-checkout"
> option---cloning and letting a commit checked out, and checking out
> another commit immediately after it, feels somewhat wasteful.

I just had a look: module_clone() already uses "--no-checkout".
So this should not be a problem.

>> +		prepare_submodule_repo_env(&cp.env_array);
>> +		cp.git_cmd = 1;
>> +		cp.dir = add_data->sm_path;
>> +		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
>> +
>> +		if (add_data->branch) {
>> +			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
>> +			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
>> +		}
>> +
>> +		if (run_command(&cp))
>> +			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
>> +	}
>> +	return 0;
>> +}
> 
> Other than all the commented parts, this looks fairly faithful
> conversion from the part of the scripted Porcelain that has been
> lost by the same patch.
> 
>> +static int add_clone(int argc, const char **argv, const char *prefix)
>> +{
>> +	int force = 0, quiet = 0, dissociate = 0, progress = 0;
>> +	struct add_data add_data = ADD_DATA_INIT;
>> + ...
>> +	argc = parse_options(argc, argv, prefix, options, usage, 0);
>> +
>> +	if (argc != 0)
>> +		usage_with_options(usage, options);
>> +
>> +	add_data.progress = !!progress;
>> +	add_data.dissociate = !!dissociate;
>> +	add_data.force = !!force;
>> +	add_data.quiet = !!quiet;
>> +
>> +	if (add_submodule(&add_data))
>> +		return 1;
>> +
>> +	return 0;
>> +}
> 
> As you wrote the add_clone() vs add_submodule() split of labour, the
> reason this one is much more nicely organized than module_clone()
> should be obvious to you.  This part of the design of this patch is
> very well done.
> 
> The "additional step" I hinted earlier is exactly that---by making
> the guts of the feature add_submodule() to work on already parsed
> data and a separate wrapper add_clone() that interfaces to the
> scripts to parse the command line arguments into a shape that is
> more appropriate for the inner function, you can later call the
> inner function add_submodule() directly without going through the
> unparsing-then-parsing dance from other places.  If module_clone()
> were split into two in a similar way, then the place to call it from
> add_submodule() would become much cleaner.

This definitely sounds like the way to go. I'll be on it.

> Thanks.

Thanks for your suggestions. They were very helpful.

Re: [PATCH v3 1/2] submodule--helper: introduce add-clone subcommand

[Junio C Hamano]

Atharva Raykar <raykar.ath@gmail.com> writes:

> I think the larger concern I have to balance here is what level of
> "faithfulness" should I be having with this conversion. My current
> thoughts on this are:
>
>  - Functionality and behaviour should be as similar as possible.
>
>  - If there is an *obvious* bug that can be fixed in translation,
>    then I should fix it.
>
>  - If there are error messages that can be made more clear and
>    consistent with the rest of the Porcelain, then I'll make the
>    required change.

I have been assuming that we would draw the line between the second
one and the third one, that is, as little end-user visible behaviour
changes as possible, unless the behaviour being changed is a clear
bugfix, during the conversion.  Polishing the results into an even
better shape can and should be done after the initial conversion is
completed.  Switching from die() to fprintf() with error return would
be very desirable in the end result, but that would be a bit distracting
to read during the review of the initial conversion.

Thanks.

[GSoC] [PATCH v4 0/3] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

This version modifies 'module_clone()' and separates the flag parsing from the
actual cloning logic. This allows us to use the functionality of
'submodule--helper clone' without needing to push arguments to a strvec from
'add_submodule'. We use a new struct called `module_clone_data` instead.

Because this change involves moving the contents of 'module_clone()' to
'clone_submodule()', the whole function had to be relocated further down so that
all the helpers it calls are available to it.

Other changes include making error output match more closely to the shell
version, and better usage of the C API
('is_directory()' -> 'is_nonbare_repo_dir()')

Atharva Raykar (3):
  submodule--helper: refactor module_clone()
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 536 ++++++++++++++++++++++++++++--------
 git-submodule.sh            |  66 +----
 2 files changed, 425 insertions(+), 177 deletions(-)

Range-diff against v3:
-:  ---------- > 1:  11d035ce75 submodule--helper: refactor module_clone()
1:  3b5f7bec7c ! 2:  c85701b79a submodule--helper: introduce add-clone subcommand
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +static int add_submodule(const struct add_data *add_data)
     +{
     +	char *submod_gitdir_path;
    ++	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
     +
     +	/* perhaps the path already exists and is already a git repo, else clone it */
     +	if (is_directory(add_data->sm_path)) {
    ++		struct strbuf sm_path = STRBUF_INIT;
    ++		strbuf_addstr(&sm_path, add_data->sm_path);
     +		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
    -+		if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))
    -+			printf(_("Adding existing path at '%s' to index\n"),
    ++		if (is_nonbare_repository_dir(&sm_path))
    ++			printf(_("Adding existing repo at '%s' to the index\n"),
     +			       add_data->sm_path);
     +		else
     +			die(_("'%s' already exists and is not a valid git repo"),
     +			    add_data->sm_path);
    ++		strbuf_release(&sm_path);
     +		free(submod_gitdir_path);
     +	} else {
    -+		struct strvec clone_args = STRVEC_INIT;
     +		struct child_process cp = CHILD_PROCESS_INIT;
     +		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
     +
     +		if (is_directory(submod_gitdir_path)) {
     +			if (!add_data->force) {
    -+				error(_("a git directory for '%s' is found "
    -+					"locally with remote(s):"), add_data->sm_name);
    ++				fprintf(stderr, _("A git directory for '%s' is found "
    ++						  "locally with remote(s):"),
    ++					add_data->sm_name);
     +				show_fetch_remotes(stderr, add_data->sm_name,
     +						   submod_gitdir_path);
    -+				fprintf(stderr,
    -+					_("If you want to reuse this local git "
    -+					  "directory instead of cloning again from\n"
    -+					  "  %s\n"
    -+					  "use the '--force' option. If the local git "
    -+					  "directory is not the correct repo\n"
    -+					  "or if you are unsure what this means, choose "
    -+					  "another name with the '--name' option.\n"),
    -+					add_data->realrepo);
     +				free(submod_gitdir_path);
    -+				return 1;
    ++				die(_("If you want to reuse this local git "
    ++				      "directory instead of cloning again from\n"
    ++				      "  %s\n"
    ++				      "use the '--force' option. If the local git "
    ++				      "directory is not the correct repo\n"
    ++				      "or if you are unsure what this means, choose "
    ++				      "another name with the '--name' option.\n"),
    ++				    add_data->realrepo);
     +			} else {
     +				printf(_("Reactivating local git directory for "
     +					 "submodule '%s'\n"), add_data->sm_name);
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +		}
     +		free(submod_gitdir_path);
     +
    -+		strvec_pushl(&clone_args, "clone", "--path", add_data->sm_path, "--name",
    -+			     add_data->sm_name, "--url", add_data->realrepo, NULL);
    -+		if (add_data->quiet)
    -+			strvec_push(&clone_args, "--quiet");
    -+		if (add_data->progress)
    -+			strvec_push(&clone_args, "--progress");
    -+		if (add_data->prefix)
    -+			strvec_pushl(&clone_args, "--prefix", add_data->prefix, NULL);
    ++		clone_data.prefix = add_data->prefix;
    ++		clone_data.path = add_data->sm_path;
    ++		clone_data.name = add_data->sm_name;
    ++		clone_data.url = add_data->realrepo;
    ++		clone_data.quiet = add_data->quiet;
    ++		clone_data.progress = add_data->progress;
     +		if (add_data->reference_path)
    -+			strvec_pushl(&clone_args, "--reference",
    -+				     add_data->reference_path, NULL);
    -+		if (add_data->dissociate)
    -+			strvec_push(&clone_args, "--dissociate");
    ++			string_list_append(&clone_data.reference,
    ++					   xstrdup(add_data->reference_path));
    ++		clone_data.dissociate = add_data->dissociate;
     +		if (add_data->depth >= 0)
    -+			strvec_pushf(&clone_args, "--depth=%d", add_data->depth);
    ++			clone_data.depth = xstrfmt("%d", add_data->depth);
     +
    -+		if (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {
    -+			strvec_clear(&clone_args);
    ++		if (clone_submodule(&clone_data))
     +			return -1;
    -+		}
    -+		strvec_clear(&clone_args);
     +
     +		prepare_submodule_repo_env(&cp.env_array);
     +		cp.git_cmd = 1;
2:  a2a6b4d74c = 3:  6532b4ae11 submodule--helper: introduce add-config subcommand
-- 
2.31.1

[PATCH v4 1/3] submodule--helper: refactor module_clone()

[Atharva Raykar]

Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.
---
 builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
 1 file changed, 128 insertions(+), 113 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..321b623d81 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int clone_submodule(const char *path, const char *gitdir, const char *url,
-			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress, int single_branch)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	strvec_push(&cp.args, "clone");
-	strvec_push(&cp.args, "--no-checkout");
-	if (quiet)
-		strvec_push(&cp.args, "--quiet");
-	if (progress)
-		strvec_push(&cp.args, "--progress");
-	if (depth && *depth)
-		strvec_pushl(&cp.args, "--depth", depth, NULL);
-	if (reference->nr) {
-		struct string_list_item *item;
-		for_each_string_list_item(item, reference)
-			strvec_pushl(&cp.args, "--reference",
-				     item->string, NULL);
-	}
-	if (dissociate)
-		strvec_push(&cp.args, "--dissociate");
-	if (gitdir && *gitdir)
-		strvec_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
-	if (single_branch >= 0)
-		strvec_push(&cp.args, single_branch ?
-					  "--single-branch" :
-					  "--no-single-branch");
-
-	strvec_push(&cp.args, "--");
-	strvec_push(&cp.args, url);
-	strvec_push(&cp.args, path);
-
-	cp.git_cmd = 1;
-	prepare_submodule_repo_env(&cp.env_array);
-	cp.no_stdin = 1;
-
-	return run_command(&cp);
-}
+struct module_clone_data {
+	const char* prefix;
+	const char* path;
+	const char* name;
+	const char* url;
+	const char* depth;
+	struct string_list reference;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+	unsigned int require_init: 1;
+	int single_branch;
+};
+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }
 
 struct submodule_alternate_setup {
 	const char *submodule_name;
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,
 	free(error_strategy);
 }
 
+static int clone_submodule(struct module_clone_data *clone_data)
+{
+	char *p, *sm_gitdir;
+	char *sm_alternate = NULL, *error_strategy = NULL;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), clone_data->name);
+	sm_gitdir = absolute_pathdup(sb.buf);
+	strbuf_reset(&sb);
+
+	if (!is_absolute_path(clone_data->path)) {
+		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
+		clone_data->path = strbuf_detach(&sb, NULL);
+	} else {
+		clone_data->path = xstrdup(clone_data->path);
+	}
+
+	if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)
+		die(_("refusing to create/use '%s' in another submodule's "
+		      "git dir"), sm_gitdir);
+
+	if (!file_exists(sm_gitdir)) {
+		if (safe_create_leading_directories_const(sm_gitdir) < 0)
+			die(_("could not create directory '%s'"), sm_gitdir);
+
+		prepare_possible_alternates(clone_data->name, &clone_data->reference);
+
+		strvec_push(&cp.args, "clone");
+		strvec_push(&cp.args, "--no-checkout");
+		if (clone_data->quiet)
+			strvec_push(&cp.args, "--quiet");
+		if (clone_data->progress)
+			strvec_push(&cp.args, "--progress");
+		if (clone_data->depth && *(clone_data->depth))
+			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
+		if (clone_data->reference.nr) {
+			struct string_list_item *item;
+			for_each_string_list_item(item, &clone_data->reference)
+				strvec_pushl(&cp.args, "--reference",
+					     item->string, NULL);
+		}
+		if (clone_data->dissociate)
+			strvec_push(&cp.args, "--dissociate");
+		if (sm_gitdir && *sm_gitdir)
+			strvec_pushl(&cp.args, "--separate-git-dir", sm_gitdir, NULL);
+		if (clone_data->single_branch >= 0)
+			strvec_push(&cp.args, clone_data->single_branch ?
+				    "--single-branch" :
+				    "--no-single-branch");
+
+		strvec_push(&cp.args, "--");
+		strvec_push(&cp.args, clone_data->url);
+		strvec_push(&cp.args, clone_data->path);
+
+		cp.git_cmd = 1;
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.no_stdin = 1;
+
+		if(run_command(&cp))
+			die(_("clone of '%s' into submodule path '%s' failed"),
+			    clone_data->url, clone_data->path);
+	} else {
+		if (clone_data->require_init && !access(clone_data->path, X_OK) &&
+		    !is_empty_dir(clone_data->path))
+			die(_("directory not empty: '%s'"), clone_data->path);
+		if (safe_create_leading_directories_const(clone_data->path) < 0)
+			die(_("could not create directory '%s'"), clone_data->path);
+		strbuf_addf(&sb, "%s/index", sm_gitdir);
+		unlink_or_warn(sb.buf);
+		strbuf_reset(&sb);
+	}
+
+	connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);
+
+	p = git_pathdup_submodule(clone_data->path, "config");
+	if (!p)
+		die(_("could not get submodule directory for '%s'"), clone_data->path);
+
+	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+	git_config_get_string("submodule.alternateLocation", &sm_alternate);
+	if (sm_alternate)
+		git_config_set_in_file(p, "submodule.alternateLocation",
+				       sm_alternate);
+	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+	if (error_strategy)
+		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+				       error_strategy);
+
+	free(sm_alternate);
+	free(error_strategy);
+
+	strbuf_release(&sb);
+	free(sm_gitdir);
+	free(p);
+	return 0;
+}
+
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-	const char *name = NULL, *url = NULL, *depth = NULL;
-	int quiet = 0;
-	int progress = 0;
-	char *p, *path = NULL, *sm_gitdir;
-	struct strbuf sb = STRBUF_INIT;
-	struct string_list reference = STRING_LIST_INIT_NODUP;
-	int dissociate = 0, require_init = 0;
-	char *sm_alternate = NULL, *error_strategy = NULL;
-	int single_branch = -1;
+	int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
 
 	struct option module_clone_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_STRING(0, "prefix", &clone_data.prefix,
 			   N_("path"),
 			   N_("alternative anchor for relative paths")),
-		OPT_STRING(0, "path", &path,
+		OPT_STRING(0, "path", &clone_data.path,
 			   N_("path"),
 			   N_("where the new submodule will be cloned to")),
-		OPT_STRING(0, "name", &name,
+		OPT_STRING(0, "name", &clone_data.name,
 			   N_("string"),
 			   N_("name of the new submodule")),
-		OPT_STRING(0, "url", &url,
+		OPT_STRING(0, "url", &clone_data.url,
 			   N_("string"),
 			   N_("url where to clone the submodule from")),
-		OPT_STRING_LIST(0, "reference", &reference,
+		OPT_STRING_LIST(0, "reference", &clone_data.reference,
 			   N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &depth,
+		OPT_STRING(0, "depth", &clone_data.depth,
 			   N_("string"),
 			   N_("depth for shallow clones")),
 		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &single_branch,
+		OPT_BOOL(0, "single-branch", &clone_data.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
-	if (argc || !url || !path || !*path)
+	clone_data.dissociate = !!dissociate;
+	clone_data.quiet = !!quiet;
+	clone_data.progress = !!progress;
+	clone_data.require_init = !!require_init;
+
+	if (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))
 		usage_with_options(git_submodule_helper_usage,
 				   module_clone_options);
 
-	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
-	sm_gitdir = absolute_pathdup(sb.buf);
-	strbuf_reset(&sb);
-
-	if (!is_absolute_path(path)) {
-		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
-		path = strbuf_detach(&sb, NULL);
-	} else
-		path = xstrdup(path);
-
-	if (validate_submodule_git_dir(sm_gitdir, name) < 0)
-		die(_("refusing to create/use '%s' in another submodule's "
-			"git dir"), sm_gitdir);
-
-	if (!file_exists(sm_gitdir)) {
-		if (safe_create_leading_directories_const(sm_gitdir) < 0)
-			die(_("could not create directory '%s'"), sm_gitdir);
-
-		prepare_possible_alternates(name, &reference);
-
-		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress, single_branch))
-			die(_("clone of '%s' into submodule path '%s' failed"),
-			    url, path);
-	} else {
-		if (require_init && !access(path, X_OK) && !is_empty_dir(path))
-			die(_("directory not empty: '%s'"), path);
-		if (safe_create_leading_directories_const(path) < 0)
-			die(_("could not create directory '%s'"), path);
-		strbuf_addf(&sb, "%s/index", sm_gitdir);
-		unlink_or_warn(sb.buf);
-		strbuf_reset(&sb);
-	}
-
-	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
-
-	p = git_pathdup_submodule(path, "config");
-	if (!p)
-		die(_("could not get submodule directory for '%s'"), path);
-
-	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
-	git_config_get_string("submodule.alternateLocation", &sm_alternate);
-	if (sm_alternate)
-		git_config_set_in_file(p, "submodule.alternateLocation",
-					   sm_alternate);
-	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
-	if (error_strategy)
-		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
-					   error_strategy);
-
-	free(sm_alternate);
-	free(error_strategy);
-
-	strbuf_release(&sb);
-	free(sm_gitdir);
-	free(path);
-	free(p);
+	clone_submodule(&clone_data);
 	return 0;
 }
 
-- 
2.31.1

[PATCH v4 2/3] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.

This is part of a series of changes that will result in all of 'submodule add'
being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 builtin/submodule--helper.c | 176 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +-------
 2 files changed, 177 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 321b623d81..6dffaeb6cb 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2760,6 +2760,181 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *next_line;
+		char *line = sb_remote_out.buf;
+		while ((next_line = strchr(line, '\n')) != NULL) {
+			size_t len = next_line - line;
+			if (strip_suffix_mem(line, &len, " (fetch)"))
+				fprintf(output, "  %.*s\n", (int)len, line);
+			line = next_line + 1;
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
+
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		struct strbuf sm_path = STRBUF_INIT;
+		strbuf_addstr(&sm_path, add_data->sm_path);
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_nonbare_repository_dir(&sm_path))
+			printf(_("Adding existing repo at '%s' to the index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		strbuf_release(&sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				fprintf(stderr, _("A git directory for '%s' is found "
+						  "locally with remote(s):"),
+					add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				free(submod_gitdir_path);
+				die(_("If you want to reuse this local git "
+				      "directory instead of cloning again from\n"
+				      "  %s\n"
+				      "use the '--force' option. If the local git "
+				      "directory is not the correct repo\n"
+				      "or if you are unsure what this means, choose "
+				      "another name with the '--name' option.\n"),
+				    add_data->realrepo);
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		clone_data.prefix = add_data->prefix;
+		clone_data.path = add_data->sm_path;
+		clone_data.name = add_data->sm_name;
+		clone_data.url = add_data->realrepo;
+		clone_data.quiet = add_data->quiet;
+		clone_data.progress = add_data->progress;
+		if (add_data->reference_path)
+			string_list_append(&clone_data.reference,
+					   xstrdup(add_data->reference_path));
+		clone_data.dissociate = add_data->dissociate;
+		if (add_data->depth >= 0)
+			clone_data.depth = xstrfmt("%d", add_data->depth);
+
+		if (clone_submodule(&clone_data))
+			return -1;
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &add_data.prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [<options>...] "
+		   "--url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2772,6 +2947,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[PATCH v4 3/3] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +--------
 2 files changed, 120 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6dffaeb6cb..c4b2aa6537 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists, activate = 0;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	key = xstrfmt("submodule.%s.path", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
+	free(key);
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	config_set_in_gitmodules_file_gently(key, add_data->repo);
+	free(key);
+	if (add_data->branch) {
+		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
+		config_set_in_gitmodules_file_gently(key, add_data->branch);
+		free(key);
+	}
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}
+
+	/*
+	 * If submodule.active does not exist, or if the pathspec was unset,
+	 * we will activate this module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists || activate ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2948,6 +3066,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1

Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand

[Rafael Silva]

Atharva Raykar <raykar.ath@gmail.com> writes:

> ---
>  builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++
>  git-submodule.sh            |  28 +--------
>  2 files changed, 120 insertions(+), 27 deletions(-)
>

I do not have enough expertise to judge the entire content of this
patch. I would like, however, to propose a slight code change for the
sake of readability.

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 6dffaeb6cb..c4b2aa6537 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  
> +static void configure_added_submodule(struct add_data *add_data)
> +{
> +	char *key, *submod_pathspec = NULL;
> +	struct child_process add_submod = CHILD_PROCESS_INIT;
> +	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
> +	int pathspec_key_exists, activate = 0;
> +
> +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
> +	git_config_set_gently(key, add_data->realrepo);
> +	free(key);
> +
> +	add_submod.git_cmd = 1;
> +	strvec_pushl(&add_submod.args, "add",
> +		     "--no-warn-embedded-repo", NULL);
> +	if (add_data->force)
> +		strvec_push(&add_submod.args, "--force");
> +	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
> +
> +	if (run_command(&add_submod))
> +		die(_("Failed to add submodule '%s'"), add_data->sm_path);
> +
> +	key = xstrfmt("submodule.%s.path", add_data->sm_name);
> +	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
> +	free(key);

This above three lines of code is very similar to the two operations that
follows (including the one inside the `if (add_data->branch)`
condition. So [ ... ]

> +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
> +	config_set_in_gitmodules_file_gently(key, add_data->repo);
> +	free(key);
> +	if (add_data->branch) {
> +		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
> +		config_set_in_gitmodules_file_gently(key, add_data->branch);
> +		free(key);
> +	}
> +

[ ... ] it might be worth to write a small wrapper that will perform: (1)
`xstrfmt()` on the specified config section, (2) set the configuration
in the file and (3) free()'ing the variable inside the wrapper. Thus,
most of these code will become one liners that is easier to read (given
the function is properly named :) ).

After abstracting the code on the wrapper, this code will become
something like:


    function_properly_named("submodule.%s.path", add_data->sm_name, add_data->sm_path);
    function_properly_named("submodule.%s.url", add_data->sm_name, add_data->repo);
    if (add_data->branch)
         function_properly_named("submodule.%s.branch", add_data->sm_path, add_data->branch);


Just as an example, here's a diff to demonstrate the argument:

-- >8 --
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 65f79fbd53..48ea909f51 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+void add_config_in_submodules_file(const char *keyfmt, const char *submodule,
+			  const char *value)
+{
+	char *key = xstrfmt(keyfmt, submodule);
+	config_set_in_gitmodules_file_gently(key, value);
+	free(key);
+}
+
 static void configure_added_submodule(struct add_data *add_data)
 {
 	char *key, *submod_pathspec = NULL;
@@ -2955,17 +2963,10 @@ static void configure_added_submodule(struct add_data *add_data)
 	if (run_command(&add_submod))
 		die(_("Failed to add submodule '%s'"), add_data->sm_path);
 
-	key = xstrfmt("submodule.%s.path", add_data->sm_name);
-	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
-	free(key);
-	key = xstrfmt("submodule.%s.url", add_data->sm_name);
-	config_set_in_gitmodules_file_gently(key, add_data->repo);
-	free(key);
-	if (add_data->branch) {
-		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
-		config_set_in_gitmodules_file_gently(key, add_data->branch);
-		free(key);
-	}
+	add_config_in_submodules_file("submodule.%s.path", add_data->sm_name, add_data->sm_path);
+	add_config_in_submodules_file("submodule.%s.url", add_data->sm_name, add_data->repo);
+	if (add_data->branch)
+		add_config_in_submodules_file("submodule.%s.branch", add_data->sm_path, add_data->branch);
 
 	add_gitmodules.git_cmd = 1;
 	strvec_pushl(&add_gitmodules.args,

-- >8 --

A proper name than "add_config_in_submodules_file" should be considered - I'm
not very good in naming things.

These change does (should) not change the behavior of code, even though
I believe it make the code simpler to read, I do not have strong
opinions about it. So, take this proposal as you wish. 

-- 
Thanks
Rafael

Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand

[Eric Sunshine]

On Mon, Jun 14, 2021 at 3:53 PM Rafael Silva
<rafaeloliveira.cs@gmail.com> wrote:
> Just as an example, here's a diff to demonstrate the argument:
> -- >8 --
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix)
> +void add_config_in_submodules_file(const char *keyfmt, const char *submodule,
> +                         const char *value)
> +{
> +       char *key = xstrfmt(keyfmt, submodule);
> +       config_set_in_gitmodules_file_gently(key, value);
> +       free(key);
> +}

The new function should be `static`, of course.

Re: [PATCH v4 1/3] submodule--helper: refactor module_clone()

[Junio C Hamano]

Atharva Raykar <raykar.ath@gmail.com> writes:

> Separate out the core logic of module_clone() from the flag
> parsing---this way we can call the equivalent of the `submodule--helper
> clone` subcommand directly within C, without needing to push arguments
> in a strvec.
> ---

Missing sign-off?

>  builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
>  1 file changed, 128 insertions(+), 113 deletions(-)
>
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index d55f6262e9..321b623d81 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>  	return 0;
>  }
>  


> +struct module_clone_data {
> +	const char* prefix;

Asterisk sticks to identifier not type in this codebase, i.e.

	const char *prefix;

Other than that, the split makes sense, especially for the purpose
of more easily reusing this helper without having to unparse and
reparse the option strings.

Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

On 15-Jun-2021, at 01:21, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote:
> 
> 
> Atharva Raykar <raykar.ath@gmail.com> writes:
> 
>> ---
>> builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++
>> git-submodule.sh            |  28 +--------
>> 2 files changed, 120 insertions(+), 27 deletions(-)
>> 
> 
> I do not have enough expertise to judge the entire content of this
> patch. I would like, however, to propose a slight code change for the
> sake of readability.
> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 6dffaeb6cb..c4b2aa6537 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix)
>> 	return 0;
>> }
>> 
>> +static void configure_added_submodule(struct add_data *add_data)
>> +{
>> +	char *key, *submod_pathspec = NULL;
>> +	struct child_process add_submod = CHILD_PROCESS_INIT;
>> +	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
>> +	int pathspec_key_exists, activate = 0;
>> +
>> +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
>> +	git_config_set_gently(key, add_data->realrepo);
>> +	free(key);
>> +
>> +	add_submod.git_cmd = 1;
>> +	strvec_pushl(&add_submod.args, "add",
>> +		     "--no-warn-embedded-repo", NULL);
>> +	if (add_data->force)
>> +		strvec_push(&add_submod.args, "--force");
>> +	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
>> +
>> +	if (run_command(&add_submod))
>> +		die(_("Failed to add submodule '%s'"), add_data->sm_path);
>> +
>> +	key = xstrfmt("submodule.%s.path", add_data->sm_name);
>> +	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
>> +	free(key);
> 
> This above three lines of code is very similar to the two operations that
> follows (including the one inside the `if (add_data->branch)`
> condition. So [ ... ]
> 
>> +	key = xstrfmt("submodule.%s.url", add_data->sm_name);
>> +	config_set_in_gitmodules_file_gently(key, add_data->repo);
>> +	free(key);
>> +	if (add_data->branch) {
>> +		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
>> +		config_set_in_gitmodules_file_gently(key, add_data->branch);
>> +		free(key);
>> +	}
>> +
> 
> [ ... ] it might be worth to write a small wrapper that will perform: (1)
> `xstrfmt()` on the specified config section, (2) set the configuration
> in the file and (3) free()'ing the variable inside the wrapper. Thus,
> most of these code will become one liners that is easier to read (given
> the function is properly named :) ).
> 
> After abstracting the code on the wrapper, this code will become
> something like:
> 
> 
>    function_properly_named("submodule.%s.path", add_data->sm_name, add_data->sm_path);
>    function_properly_named("submodule.%s.url", add_data->sm_name, add_data->repo);
>    if (add_data->branch)
>         function_properly_named("submodule.%s.branch", add_data->sm_path, add_data->branch);
> 
> 
> Just as an example, here's a diff to demonstrate the argument:
> 
> -- >8 --
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 65f79fbd53..48ea909f51 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix)
> 	return 0;
> }
> 
> +void add_config_in_submodules_file(const char *keyfmt, const char *submodule,
> +			  const char *value)
> +{
> +	char *key = xstrfmt(keyfmt, submodule);
> +	config_set_in_gitmodules_file_gently(key, value);
> +	free(key);
> +}
> +
> static void configure_added_submodule(struct add_data *add_data)
> {
> 	char *key, *submod_pathspec = NULL;
> @@ -2955,17 +2963,10 @@ static void configure_added_submodule(struct add_data *add_data)
> 	if (run_command(&add_submod))
> 		die(_("Failed to add submodule '%s'"), add_data->sm_path);
> 
> -	key = xstrfmt("submodule.%s.path", add_data->sm_name);
> -	config_set_in_gitmodules_file_gently(key, add_data->sm_path);
> -	free(key);
> -	key = xstrfmt("submodule.%s.url", add_data->sm_name);
> -	config_set_in_gitmodules_file_gently(key, add_data->repo);
> -	free(key);
> -	if (add_data->branch) {
> -		key = xstrfmt("submodule.%s.branch", add_data->sm_path);
> -		config_set_in_gitmodules_file_gently(key, add_data->branch);
> -		free(key);
> -	}
> +	add_config_in_submodules_file("submodule.%s.path", add_data->sm_name, add_data->sm_path);
> +	add_config_in_submodules_file("submodule.%s.url", add_data->sm_name, add_data->repo);
> +	if (add_data->branch)
> +		add_config_in_submodules_file("submodule.%s.branch", add_data->sm_path, add_data->branch);
> 
> 	add_gitmodules.git_cmd = 1;
> 	strvec_pushl(&add_gitmodules.args,
> 
> -- >8 --
> 
> A proper name than "add_config_in_submodules_file" should be considered - I'm
> not very good in naming things.
> 
> These change does (should) not change the behavior of code, even though
> I believe it make the code simpler to read, I do not have strong
> opinions about it. So, take this proposal as you wish. 

I agree with you, this will make the code simpler to read. It also
made me realise one thing that I did not replicate exactly from the
shell code.

The original shell code calls 'module_config()', which does an extra
check to see if writing to '.gitmodules' is okay. I did not perform
this check, and including that in the wrapper you propose will be a
good idea.

> -- 
> Thanks
> Rafael

Re: [PATCH v4 1/3] submodule--helper: refactor module_clone()

[Atharva Raykar]

On 15-Jun-2021, at 09:21, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Atharva Raykar <raykar.ath@gmail.com> writes:
> 
>> Separate out the core logic of module_clone() from the flag
>> parsing---this way we can call the equivalent of the `submodule--helper
>> clone` subcommand directly within C, without needing to push arguments
>> in a strvec.
>> ---
> 
> Missing sign-off?

Yes, I missed out all the trailers.

>> builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
>> 1 file changed, 128 insertions(+), 113 deletions(-)
>> 
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index d55f6262e9..321b623d81 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
>> 	return 0;
>> }
>> 
> 
> 
>> +struct module_clone_data {
>> +	const char* prefix;
> 
> Asterisk sticks to identifier not type in this codebase, i.e.
> 
> 	const char *prefix;
> 

Will fix and resend.

> Other than that, the split makes sense, especially for the purpose
> of more easily reusing this helper without having to unparse and
> reparse the option strings.
> 

Re: [PATCH v4 3/3] submodule--helper: introduce add-config subcommand

[Rafael Silva]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Mon, Jun 14, 2021 at 3:53 PM Rafael Silva
> <rafaeloliveira.cs@gmail.com> wrote:
>> Just as an example, here's a diff to demonstrate the argument:
>> -- >8 --
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix)
>> +void add_config_in_submodules_file(const char *keyfmt, const char *submodule,
>> +                         const char *value)
>> +{
>> +       char *key = xstrfmt(keyfmt, submodule);
>> +       config_set_in_gitmodules_file_gently(key, value);
>> +       free(key);
>> +}
>
> The new function should be `static`, of course.

Good catch! Indeed, it should be `static`. 

-- 
Thanks
Rafael

[GSoC] [PATCH v5 0/3] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

Only two changes since v4:
 - Add missing trailers and s.o.b in [1/3]
 - In [3/3] introduce a wrapper function called 'config_submodule_in_gitmodules'
   that sets the 'submodule.<name>.<var>' configuration, and before doing so,
   checks if it is okay to write to '.gitmodules', which was the original
   behaviour of the shell version.

Atharva Raykar (3):
  submodule--helper: refactor module_clone()
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 542 ++++++++++++++++++++++++++++--------
 git-submodule.sh            |  66 +----
 2 files changed, 431 insertions(+), 177 deletions(-)

-- 
2.31.1

[PATCH v5 1/3] submodule--helper: refactor module_clone()

[Atharva Raykar]

Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Suggested-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
 1 file changed, 128 insertions(+), 113 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..ae246a35f9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int clone_submodule(const char *path, const char *gitdir, const char *url,
-			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress, int single_branch)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	strvec_push(&cp.args, "clone");
-	strvec_push(&cp.args, "--no-checkout");
-	if (quiet)
-		strvec_push(&cp.args, "--quiet");
-	if (progress)
-		strvec_push(&cp.args, "--progress");
-	if (depth && *depth)
-		strvec_pushl(&cp.args, "--depth", depth, NULL);
-	if (reference->nr) {
-		struct string_list_item *item;
-		for_each_string_list_item(item, reference)
-			strvec_pushl(&cp.args, "--reference",
-				     item->string, NULL);
-	}
-	if (dissociate)
-		strvec_push(&cp.args, "--dissociate");
-	if (gitdir && *gitdir)
-		strvec_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
-	if (single_branch >= 0)
-		strvec_push(&cp.args, single_branch ?
-					  "--single-branch" :
-					  "--no-single-branch");
-
-	strvec_push(&cp.args, "--");
-	strvec_push(&cp.args, url);
-	strvec_push(&cp.args, path);
-
-	cp.git_cmd = 1;
-	prepare_submodule_repo_env(&cp.env_array);
-	cp.no_stdin = 1;
-
-	return run_command(&cp);
-}
+struct module_clone_data {
+	const char *prefix;
+	const char *path;
+	const char *name;
+	const char *url;
+	const char *depth;
+	struct string_list reference;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+	unsigned int require_init: 1;
+	int single_branch;
+};
+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }
 
 struct submodule_alternate_setup {
 	const char *submodule_name;
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,
 	free(error_strategy);
 }
 
-static int module_clone(int argc, const char **argv, const char *prefix)
+static int clone_submodule(struct module_clone_data *clone_data)
 {
-	const char *name = NULL, *url = NULL, *depth = NULL;
-	int quiet = 0;
-	int progress = 0;
-	char *p, *path = NULL, *sm_gitdir;
-	struct strbuf sb = STRBUF_INIT;
-	struct string_list reference = STRING_LIST_INIT_NODUP;
-	int dissociate = 0, require_init = 0;
+	char *p, *sm_gitdir;
 	char *sm_alternate = NULL, *error_strategy = NULL;
-	int single_branch = -1;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), clone_data->name);
+	sm_gitdir = absolute_pathdup(sb.buf);
+	strbuf_reset(&sb);
+
+	if (!is_absolute_path(clone_data->path)) {
+		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
+		clone_data->path = strbuf_detach(&sb, NULL);
+	} else {
+		clone_data->path = xstrdup(clone_data->path);
+	}
+
+	if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)
+		die(_("refusing to create/use '%s' in another submodule's "
+		      "git dir"), sm_gitdir);
+
+	if (!file_exists(sm_gitdir)) {
+		if (safe_create_leading_directories_const(sm_gitdir) < 0)
+			die(_("could not create directory '%s'"), sm_gitdir);
+
+		prepare_possible_alternates(clone_data->name, &clone_data->reference);
+
+		strvec_push(&cp.args, "clone");
+		strvec_push(&cp.args, "--no-checkout");
+		if (clone_data->quiet)
+			strvec_push(&cp.args, "--quiet");
+		if (clone_data->progress)
+			strvec_push(&cp.args, "--progress");
+		if (clone_data->depth && *(clone_data->depth))
+			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
+		if (clone_data->reference.nr) {
+			struct string_list_item *item;
+			for_each_string_list_item(item, &clone_data->reference)
+				strvec_pushl(&cp.args, "--reference",
+					     item->string, NULL);
+		}
+		if (clone_data->dissociate)
+			strvec_push(&cp.args, "--dissociate");
+		if (sm_gitdir && *sm_gitdir)
+			strvec_pushl(&cp.args, "--separate-git-dir", sm_gitdir, NULL);
+		if (clone_data->single_branch >= 0)
+			strvec_push(&cp.args, clone_data->single_branch ?
+				    "--single-branch" :
+				    "--no-single-branch");
+
+		strvec_push(&cp.args, "--");
+		strvec_push(&cp.args, clone_data->url);
+		strvec_push(&cp.args, clone_data->path);
+
+		cp.git_cmd = 1;
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.no_stdin = 1;
+
+		if(run_command(&cp))
+			die(_("clone of '%s' into submodule path '%s' failed"),
+			    clone_data->url, clone_data->path);
+	} else {
+		if (clone_data->require_init && !access(clone_data->path, X_OK) &&
+		    !is_empty_dir(clone_data->path))
+			die(_("directory not empty: '%s'"), clone_data->path);
+		if (safe_create_leading_directories_const(clone_data->path) < 0)
+			die(_("could not create directory '%s'"), clone_data->path);
+		strbuf_addf(&sb, "%s/index", sm_gitdir);
+		unlink_or_warn(sb.buf);
+		strbuf_reset(&sb);
+	}
+
+	connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);
+
+	p = git_pathdup_submodule(clone_data->path, "config");
+	if (!p)
+		die(_("could not get submodule directory for '%s'"), clone_data->path);
+
+	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+	git_config_get_string("submodule.alternateLocation", &sm_alternate);
+	if (sm_alternate)
+		git_config_set_in_file(p, "submodule.alternateLocation",
+				       sm_alternate);
+	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+	if (error_strategy)
+		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+				       error_strategy);
+
+	free(sm_alternate);
+	free(error_strategy);
+
+	strbuf_release(&sb);
+	free(sm_gitdir);
+	free(p);
+	return 0;
+}
+
+static int module_clone(int argc, const char **argv, const char *prefix)
+{
+	int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
 
 	struct option module_clone_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_STRING(0, "prefix", &clone_data.prefix,
 			   N_("path"),
 			   N_("alternative anchor for relative paths")),
-		OPT_STRING(0, "path", &path,
+		OPT_STRING(0, "path", &clone_data.path,
 			   N_("path"),
 			   N_("where the new submodule will be cloned to")),
-		OPT_STRING(0, "name", &name,
+		OPT_STRING(0, "name", &clone_data.name,
 			   N_("string"),
 			   N_("name of the new submodule")),
-		OPT_STRING(0, "url", &url,
+		OPT_STRING(0, "url", &clone_data.url,
 			   N_("string"),
 			   N_("url where to clone the submodule from")),
-		OPT_STRING_LIST(0, "reference", &reference,
+		OPT_STRING_LIST(0, "reference", &clone_data.reference,
 			   N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &depth,
+		OPT_STRING(0, "depth", &clone_data.depth,
 			   N_("string"),
 			   N_("depth for shallow clones")),
 		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &single_branch,
+		OPT_BOOL(0, "single-branch", &clone_data.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
-	if (argc || !url || !path || !*path)
+	clone_data.dissociate = !!dissociate;
+	clone_data.quiet = !!quiet;
+	clone_data.progress = !!progress;
+	clone_data.require_init = !!require_init;
+
+	if (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))
 		usage_with_options(git_submodule_helper_usage,
 				   module_clone_options);
 
-	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
-	sm_gitdir = absolute_pathdup(sb.buf);
-	strbuf_reset(&sb);
-
-	if (!is_absolute_path(path)) {
-		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
-		path = strbuf_detach(&sb, NULL);
-	} else
-		path = xstrdup(path);
-
-	if (validate_submodule_git_dir(sm_gitdir, name) < 0)
-		die(_("refusing to create/use '%s' in another submodule's "
-			"git dir"), sm_gitdir);
-
-	if (!file_exists(sm_gitdir)) {
-		if (safe_create_leading_directories_const(sm_gitdir) < 0)
-			die(_("could not create directory '%s'"), sm_gitdir);
-
-		prepare_possible_alternates(name, &reference);
-
-		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress, single_branch))
-			die(_("clone of '%s' into submodule path '%s' failed"),
-			    url, path);
-	} else {
-		if (require_init && !access(path, X_OK) && !is_empty_dir(path))
-			die(_("directory not empty: '%s'"), path);
-		if (safe_create_leading_directories_const(path) < 0)
-			die(_("could not create directory '%s'"), path);
-		strbuf_addf(&sb, "%s/index", sm_gitdir);
-		unlink_or_warn(sb.buf);
-		strbuf_reset(&sb);
-	}
-
-	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
-
-	p = git_pathdup_submodule(path, "config");
-	if (!p)
-		die(_("could not get submodule directory for '%s'"), path);
-
-	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
-	git_config_get_string("submodule.alternateLocation", &sm_alternate);
-	if (sm_alternate)
-		git_config_set_in_file(p, "submodule.alternateLocation",
-					   sm_alternate);
-	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
-	if (error_strategy)
-		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
-					   error_strategy);
-
-	free(sm_alternate);
-	free(error_strategy);
-
-	strbuf_release(&sb);
-	free(sm_gitdir);
-	free(path);
-	free(p);
+	clone_submodule(&clone_data);
 	return 0;
 }
 
-- 
2.31.1

[PATCH v5 2/3] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.

This is part of a series of changes that will result in all of 'submodule add'
being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 builtin/submodule--helper.c | 176 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +-------
 2 files changed, 177 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ae246a35f9..82c3a9aaa5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2760,6 +2760,181 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *next_line;
+		char *line = sb_remote_out.buf;
+		while ((next_line = strchr(line, '\n')) != NULL) {
+			size_t len = next_line - line;
+			if (strip_suffix_mem(line, &len, " (fetch)"))
+				fprintf(output, "  %.*s\n", (int)len, line);
+			line = next_line + 1;
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
+
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		struct strbuf sm_path = STRBUF_INIT;
+		strbuf_addstr(&sm_path, add_data->sm_path);
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_nonbare_repository_dir(&sm_path))
+			printf(_("Adding existing repo at '%s' to the index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		strbuf_release(&sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				fprintf(stderr, _("A git directory for '%s' is found "
+						  "locally with remote(s):"),
+					add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				free(submod_gitdir_path);
+				die(_("If you want to reuse this local git "
+				      "directory instead of cloning again from\n"
+				      "  %s\n"
+				      "use the '--force' option. If the local git "
+				      "directory is not the correct repo\n"
+				      "or if you are unsure what this means, choose "
+				      "another name with the '--name' option.\n"),
+				    add_data->realrepo);
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		clone_data.prefix = add_data->prefix;
+		clone_data.path = add_data->sm_path;
+		clone_data.name = add_data->sm_name;
+		clone_data.url = add_data->realrepo;
+		clone_data.quiet = add_data->quiet;
+		clone_data.progress = add_data->progress;
+		if (add_data->reference_path)
+			string_list_append(&clone_data.reference,
+					   xstrdup(add_data->reference_path));
+		clone_data.dissociate = add_data->dissociate;
+		if (add_data->depth >= 0)
+			clone_data.depth = xstrfmt("%d", add_data->depth);
+
+		if (clone_submodule(&clone_data))
+			return -1;
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &add_data.prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [<options>...] "
+		   "--url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2772,6 +2947,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[PATCH v5 3/3] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 125 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +-------
 2 files changed, 126 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 82c3a9aaa5..e6a75f9f7e 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2935,6 +2935,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)
+{
+	char *key;
+
+	if (!is_writing_gitmodules_ok())
+		die(_("please make sure that the .gitmodules file is in the working tree"));
+
+	key = xstrfmt("submodule.%s.%s", name, var);
+	config_set_in_gitmodules_file_gently(key, value);
+	free(key);
+}
+
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists, activate = 0;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path);
+	config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo);
+	if (add_data->branch)
+		config_submodule_in_gitmodules(add_data->sm_name,
+					       "branch", add_data->branch);
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}
+
+	/*
+	 * If submodule.active does not exist, or if the pathspec was unset,
+	 * we will activate this module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists || activate ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2948,6 +3072,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1

[GSoC] [PATCH v6 0/3] submodule--helper: introduce subcommands for sh to C conversion

[Atharva Raykar]

In [2/3]:
I noticed that my previous code fails to parse prefixes properly when it has not
been explicitly stated by the subcommand caller.

Atharva Raykar (3):
  submodule--helper: refactor module_clone()
  submodule--helper: introduce add-clone subcommand
  submodule--helper: introduce add-config subcommand

 builtin/submodule--helper.c | 543 ++++++++++++++++++++++++++++--------
 git-submodule.sh            |  66 +----
 2 files changed, 432 insertions(+), 177 deletions(-)

Range-diff against v5:
1:  068568f997 = 1:  068568f997 submodule--helper: refactor module_clone()
1:  10734cf91a ! 2:  06b2075580 submodule--helper: introduce add-clone subcommand
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +		OPT_STRING('b', "branch", &add_data.branch,
     +			   N_("branch"),
     +			   N_("branch of repository to checkout on cloning")),
    -+		OPT_STRING(0, "prefix", &add_data.prefix,
    ++		OPT_STRING(0, "prefix", &prefix,
     +			   N_("path"),
     +			   N_("alternative anchor for relative paths")),
     +		OPT_STRING(0, "path", &add_data.sm_path,
    @@ builtin/submodule--helper.c: static int module_set_branch(int argc, const char *
     +	if (argc != 0)
     +		usage_with_options(usage, options);
     +
    ++	add_data.prefix = prefix;
     +	add_data.progress = !!progress;
     +	add_data.dissociate = !!dissociate;
     +	add_data.force = !!force;
2:  383f0b6217 = 3:  3e827a3858 submodule--helper: introduce add-config subcommand

--
2.31.1

[PATCH v6 1/3] submodule--helper: refactor module_clone()

[Atharva Raykar]

Separate out the core logic of module_clone() from the flag
parsing---this way we can call the equivalent of the `submodule--helper
clone` subcommand directly within C, without needing to push arguments
in a strvec.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Suggested-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------
 1 file changed, 128 insertions(+), 113 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index d55f6262e9..ae246a35f9 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
-static int clone_submodule(const char *path, const char *gitdir, const char *url,
-			   const char *depth, struct string_list *reference, int dissociate,
-			   int quiet, int progress, int single_branch)
-{
-	struct child_process cp = CHILD_PROCESS_INIT;
-
-	strvec_push(&cp.args, "clone");
-	strvec_push(&cp.args, "--no-checkout");
-	if (quiet)
-		strvec_push(&cp.args, "--quiet");
-	if (progress)
-		strvec_push(&cp.args, "--progress");
-	if (depth && *depth)
-		strvec_pushl(&cp.args, "--depth", depth, NULL);
-	if (reference->nr) {
-		struct string_list_item *item;
-		for_each_string_list_item(item, reference)
-			strvec_pushl(&cp.args, "--reference",
-				     item->string, NULL);
-	}
-	if (dissociate)
-		strvec_push(&cp.args, "--dissociate");
-	if (gitdir && *gitdir)
-		strvec_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
-	if (single_branch >= 0)
-		strvec_push(&cp.args, single_branch ?
-					  "--single-branch" :
-					  "--no-single-branch");
-
-	strvec_push(&cp.args, "--");
-	strvec_push(&cp.args, url);
-	strvec_push(&cp.args, path);
-
-	cp.git_cmd = 1;
-	prepare_submodule_repo_env(&cp.env_array);
-	cp.no_stdin = 1;
-
-	return run_command(&cp);
-}
+struct module_clone_data {
+	const char *prefix;
+	const char *path;
+	const char *name;
+	const char *url;
+	const char *depth;
+	struct string_list reference;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+	unsigned int require_init: 1;
+	int single_branch;
+};
+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }
 
 struct submodule_alternate_setup {
 	const char *submodule_name;
@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,
 	free(error_strategy);
 }
 
+static int clone_submodule(struct module_clone_data *clone_data)
+{
+	char *p, *sm_gitdir;
+	char *sm_alternate = NULL, *error_strategy = NULL;
+	struct strbuf sb = STRBUF_INIT;
+	struct child_process cp = CHILD_PROCESS_INIT;
+
+	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), clone_data->name);
+	sm_gitdir = absolute_pathdup(sb.buf);
+	strbuf_reset(&sb);
+
+	if (!is_absolute_path(clone_data->path)) {
+		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), clone_data->path);
+		clone_data->path = strbuf_detach(&sb, NULL);
+	} else {
+		clone_data->path = xstrdup(clone_data->path);
+	}
+
+	if (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)
+		die(_("refusing to create/use '%s' in another submodule's "
+		      "git dir"), sm_gitdir);
+
+	if (!file_exists(sm_gitdir)) {
+		if (safe_create_leading_directories_const(sm_gitdir) < 0)
+			die(_("could not create directory '%s'"), sm_gitdir);
+
+		prepare_possible_alternates(clone_data->name, &clone_data->reference);
+
+		strvec_push(&cp.args, "clone");
+		strvec_push(&cp.args, "--no-checkout");
+		if (clone_data->quiet)
+			strvec_push(&cp.args, "--quiet");
+		if (clone_data->progress)
+			strvec_push(&cp.args, "--progress");
+		if (clone_data->depth && *(clone_data->depth))
+			strvec_pushl(&cp.args, "--depth", clone_data->depth, NULL);
+		if (clone_data->reference.nr) {
+			struct string_list_item *item;
+			for_each_string_list_item(item, &clone_data->reference)
+				strvec_pushl(&cp.args, "--reference",
+					     item->string, NULL);
+		}
+		if (clone_data->dissociate)
+			strvec_push(&cp.args, "--dissociate");
+		if (sm_gitdir && *sm_gitdir)
+			strvec_pushl(&cp.args, "--separate-git-dir", sm_gitdir, NULL);
+		if (clone_data->single_branch >= 0)
+			strvec_push(&cp.args, clone_data->single_branch ?
+				    "--single-branch" :
+				    "--no-single-branch");
+
+		strvec_push(&cp.args, "--");
+		strvec_push(&cp.args, clone_data->url);
+		strvec_push(&cp.args, clone_data->path);
+
+		cp.git_cmd = 1;
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.no_stdin = 1;
+
+		if(run_command(&cp))
+			die(_("clone of '%s' into submodule path '%s' failed"),
+			    clone_data->url, clone_data->path);
+	} else {
+		if (clone_data->require_init && !access(clone_data->path, X_OK) &&
+		    !is_empty_dir(clone_data->path))
+			die(_("directory not empty: '%s'"), clone_data->path);
+		if (safe_create_leading_directories_const(clone_data->path) < 0)
+			die(_("could not create directory '%s'"), clone_data->path);
+		strbuf_addf(&sb, "%s/index", sm_gitdir);
+		unlink_or_warn(sb.buf);
+		strbuf_reset(&sb);
+	}
+
+	connect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);
+
+	p = git_pathdup_submodule(clone_data->path, "config");
+	if (!p)
+		die(_("could not get submodule directory for '%s'"), clone_data->path);
+
+	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
+	git_config_get_string("submodule.alternateLocation", &sm_alternate);
+	if (sm_alternate)
+		git_config_set_in_file(p, "submodule.alternateLocation",
+				       sm_alternate);
+	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
+	if (error_strategy)
+		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
+				       error_strategy);
+
+	free(sm_alternate);
+	free(error_strategy);
+
+	strbuf_release(&sb);
+	free(sm_gitdir);
+	free(p);
+	return 0;
+}
+
 static int module_clone(int argc, const char **argv, const char *prefix)
 {
-	const char *name = NULL, *url = NULL, *depth = NULL;
-	int quiet = 0;
-	int progress = 0;
-	char *p, *path = NULL, *sm_gitdir;
-	struct strbuf sb = STRBUF_INIT;
-	struct string_list reference = STRING_LIST_INIT_NODUP;
-	int dissociate = 0, require_init = 0;
-	char *sm_alternate = NULL, *error_strategy = NULL;
-	int single_branch = -1;
+	int dissociate = 0, quiet = 0, progress = 0, require_init = 0;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
 
 	struct option module_clone_options[] = {
-		OPT_STRING(0, "prefix", &prefix,
+		OPT_STRING(0, "prefix", &clone_data.prefix,
 			   N_("path"),
 			   N_("alternative anchor for relative paths")),
-		OPT_STRING(0, "path", &path,
+		OPT_STRING(0, "path", &clone_data.path,
 			   N_("path"),
 			   N_("where the new submodule will be cloned to")),
-		OPT_STRING(0, "name", &name,
+		OPT_STRING(0, "name", &clone_data.name,
 			   N_("string"),
 			   N_("name of the new submodule")),
-		OPT_STRING(0, "url", &url,
+		OPT_STRING(0, "url", &clone_data.url,
 			   N_("string"),
 			   N_("url where to clone the submodule from")),
-		OPT_STRING_LIST(0, "reference", &reference,
+		OPT_STRING_LIST(0, "reference", &clone_data.reference,
 			   N_("repo"),
 			   N_("reference repository")),
 		OPT_BOOL(0, "dissociate", &dissociate,
 			   N_("use --reference only while cloning")),
-		OPT_STRING(0, "depth", &depth,
+		OPT_STRING(0, "depth", &clone_data.depth,
 			   N_("string"),
 			   N_("depth for shallow clones")),
 		OPT__QUIET(&quiet, "Suppress output for cloning a submodule"),
@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 			   N_("force cloning progress")),
 		OPT_BOOL(0, "require-init", &require_init,
 			   N_("disallow cloning into non-empty directory")),
-		OPT_BOOL(0, "single-branch", &single_branch,
+		OPT_BOOL(0, "single-branch", &clone_data.single_branch,
 			 N_("clone only one branch, HEAD or --branch")),
 		OPT_END()
 	};
@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_clone_options,
 			     git_submodule_helper_usage, 0);
 
-	if (argc || !url || !path || !*path)
+	clone_data.dissociate = !!dissociate;
+	clone_data.quiet = !!quiet;
+	clone_data.progress = !!progress;
+	clone_data.require_init = !!require_init;
+
+	if (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))
 		usage_with_options(git_submodule_helper_usage,
 				   module_clone_options);
 
-	strbuf_addf(&sb, "%s/modules/%s", get_git_dir(), name);
-	sm_gitdir = absolute_pathdup(sb.buf);
-	strbuf_reset(&sb);
-
-	if (!is_absolute_path(path)) {
-		strbuf_addf(&sb, "%s/%s", get_git_work_tree(), path);
-		path = strbuf_detach(&sb, NULL);
-	} else
-		path = xstrdup(path);
-
-	if (validate_submodule_git_dir(sm_gitdir, name) < 0)
-		die(_("refusing to create/use '%s' in another submodule's "
-			"git dir"), sm_gitdir);
-
-	if (!file_exists(sm_gitdir)) {
-		if (safe_create_leading_directories_const(sm_gitdir) < 0)
-			die(_("could not create directory '%s'"), sm_gitdir);
-
-		prepare_possible_alternates(name, &reference);
-
-		if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
-				    quiet, progress, single_branch))
-			die(_("clone of '%s' into submodule path '%s' failed"),
-			    url, path);
-	} else {
-		if (require_init && !access(path, X_OK) && !is_empty_dir(path))
-			die(_("directory not empty: '%s'"), path);
-		if (safe_create_leading_directories_const(path) < 0)
-			die(_("could not create directory '%s'"), path);
-		strbuf_addf(&sb, "%s/index", sm_gitdir);
-		unlink_or_warn(sb.buf);
-		strbuf_reset(&sb);
-	}
-
-	connect_work_tree_and_git_dir(path, sm_gitdir, 0);
-
-	p = git_pathdup_submodule(path, "config");
-	if (!p)
-		die(_("could not get submodule directory for '%s'"), path);
-
-	/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */
-	git_config_get_string("submodule.alternateLocation", &sm_alternate);
-	if (sm_alternate)
-		git_config_set_in_file(p, "submodule.alternateLocation",
-					   sm_alternate);
-	git_config_get_string("submodule.alternateErrorStrategy", &error_strategy);
-	if (error_strategy)
-		git_config_set_in_file(p, "submodule.alternateErrorStrategy",
-					   error_strategy);
-
-	free(sm_alternate);
-	free(error_strategy);
-
-	strbuf_release(&sb);
-	free(sm_gitdir);
-	free(path);
-	free(p);
+	clone_submodule(&clone_data);
 	return 0;
 }
 
-- 
2.31.1

[PATCH v6 2/3] submodule--helper: introduce add-clone subcommand

[Atharva Raykar]

Let's add a new "add-clone" subcommand to `git submodule--helper` with
the goal of converting part of the shell code in git-submodule.sh
related to `git submodule add` into C code. This new subcommand clones
the repository that is to be added, and checks out to the appropriate
branch.

This is meant to be a faithful conversion that leaves the behaviour of
'submodule add' unchanged. The only minor change is that if a submodule name has
been supplied with a name that clashes with a local submodule, the message shown
to the user ("A git directory for 'foo' is found locally...") is prepended with
"error" for clarity.

This is part of a series of changes that will result in all of 'submodule add'
being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
---
 builtin/submodule--helper.c | 177 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  38 +-------
 2 files changed, 178 insertions(+), 37 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index ae246a35f9..6d52a73a57 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2760,6 +2760,182 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)
 	return !!ret;
 }
 
+struct add_data {
+	const char *prefix;
+	const char *branch;
+	const char *reference_path;
+	const char *sm_path;
+	const char *sm_name;
+	const char *repo;
+	const char *realrepo;
+	int depth;
+	unsigned int force: 1;
+	unsigned int quiet: 1;
+	unsigned int progress: 1;
+	unsigned int dissociate: 1;
+};
+#define ADD_DATA_INIT { .depth = -1 }
+
+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)
+{
+	struct child_process cp_remote = CHILD_PROCESS_INIT;
+	struct strbuf sb_remote_out = STRBUF_INIT;
+
+	cp_remote.git_cmd = 1;
+	strvec_pushf(&cp_remote.env_array,
+		     "GIT_DIR=%s", git_dir_path);
+	strvec_push(&cp_remote.env_array, "GIT_WORK_TREE=.");
+	strvec_pushl(&cp_remote.args, "remote", "-v", NULL);
+	if (!capture_command(&cp_remote, &sb_remote_out, 0)) {
+		char *next_line;
+		char *line = sb_remote_out.buf;
+		while ((next_line = strchr(line, '\n')) != NULL) {
+			size_t len = next_line - line;
+			if (strip_suffix_mem(line, &len, " (fetch)"))
+				fprintf(output, "  %.*s\n", (int)len, line);
+			line = next_line + 1;
+		}
+	}
+
+	strbuf_release(&sb_remote_out);
+}
+
+static int add_submodule(const struct add_data *add_data)
+{
+	char *submod_gitdir_path;
+	struct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;
+
+	/* perhaps the path already exists and is already a git repo, else clone it */
+	if (is_directory(add_data->sm_path)) {
+		struct strbuf sm_path = STRBUF_INIT;
+		strbuf_addstr(&sm_path, add_data->sm_path);
+		submod_gitdir_path = xstrfmt("%s/.git", add_data->sm_path);
+		if (is_nonbare_repository_dir(&sm_path))
+			printf(_("Adding existing repo at '%s' to the index\n"),
+			       add_data->sm_path);
+		else
+			die(_("'%s' already exists and is not a valid git repo"),
+			    add_data->sm_path);
+		strbuf_release(&sm_path);
+		free(submod_gitdir_path);
+	} else {
+		struct child_process cp = CHILD_PROCESS_INIT;
+		submod_gitdir_path = xstrfmt(".git/modules/%s", add_data->sm_name);
+
+		if (is_directory(submod_gitdir_path)) {
+			if (!add_data->force) {
+				fprintf(stderr, _("A git directory for '%s' is found "
+						  "locally with remote(s):"),
+					add_data->sm_name);
+				show_fetch_remotes(stderr, add_data->sm_name,
+						   submod_gitdir_path);
+				free(submod_gitdir_path);
+				die(_("If you want to reuse this local git "
+				      "directory instead of cloning again from\n"
+				      "  %s\n"
+				      "use the '--force' option. If the local git "
+				      "directory is not the correct repo\n"
+				      "or if you are unsure what this means, choose "
+				      "another name with the '--name' option.\n"),
+				    add_data->realrepo);
+			} else {
+				printf(_("Reactivating local git directory for "
+					 "submodule '%s'\n"), add_data->sm_name);
+			}
+		}
+		free(submod_gitdir_path);
+
+		clone_data.prefix = add_data->prefix;
+		clone_data.path = add_data->sm_path;
+		clone_data.name = add_data->sm_name;
+		clone_data.url = add_data->realrepo;
+		clone_data.quiet = add_data->quiet;
+		clone_data.progress = add_data->progress;
+		if (add_data->reference_path)
+			string_list_append(&clone_data.reference,
+					   xstrdup(add_data->reference_path));
+		clone_data.dissociate = add_data->dissociate;
+		if (add_data->depth >= 0)
+			clone_data.depth = xstrfmt("%d", add_data->depth);
+
+		if (clone_submodule(&clone_data))
+			return -1;
+
+		prepare_submodule_repo_env(&cp.env_array);
+		cp.git_cmd = 1;
+		cp.dir = add_data->sm_path;
+		strvec_pushl(&cp.args, "checkout", "-f", "-q", NULL);
+
+		if (add_data->branch) {
+			strvec_pushl(&cp.args, "-B", add_data->branch, NULL);
+			strvec_pushf(&cp.args, "origin/%s", add_data->branch);
+		}
+
+		if (run_command(&cp))
+			die(_("unable to checkout submodule '%s'"), add_data->sm_path);
+	}
+	return 0;
+}
+
+static int add_clone(int argc, const char **argv, const char *prefix)
+{
+	int force = 0, quiet = 0, dissociate = 0, progress = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to checkout on cloning")),
+		OPT_STRING(0, "prefix", &prefix,
+			   N_("path"),
+			   N_("alternative anchor for relative paths")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT_STRING(0, "url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url where to clone the submodule from")),
+		OPT_STRING(0, "reference", &add_data.reference_path,
+			   N_("repo"),
+			   N_("reference repository")),
+		OPT_BOOL(0, "dissociate", &dissociate,
+			 N_("use --reference only while cloning")),
+		OPT_INTEGER(0, "depth", &add_data.depth,
+			    N_("depth for shallow clones")),
+		OPT_BOOL(0, "progress", &progress,
+			 N_("force cloning progress")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT__QUIET(&quiet, "suppress output for cloning a submodule"),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-clone [<options>...] "
+		   "--url <url> --path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.prefix = prefix;
+	add_data.progress = !!progress;
+	add_data.dissociate = !!dissociate;
+	add_data.force = !!force;
+	add_data.quiet = !!quiet;
+
+	if (add_submodule(&add_data))
+		return 1;
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2772,6 +2948,7 @@ static struct cmd_struct commands[] = {
 	{"list", module_list, 0},
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
+	{"add-clone", add_clone, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index 4678378424..f71e1e5495 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -241,43 +241,7 @@ cmd_add()
 		die "$(eval_gettext "'$sm_name' is not a valid submodule name")"
 	fi
 
-	# perhaps the path exists and is already a git repo, else clone it
-	if test -e "$sm_path"
-	then
-		if test -d "$sm_path"/.git || test -f "$sm_path"/.git
-		then
-			eval_gettextln "Adding existing repo at '\$sm_path' to the index"
-		else
-			die "$(eval_gettext "'\$sm_path' already exists and is not a valid git repo")"
-		fi
-
-	else
-		if test -d ".git/modules/$sm_name"
-		then
-			if test -z "$force"
-			then
-				eval_gettextln >&2 "A git directory for '\$sm_name' is found locally with remote(s):"
-				GIT_DIR=".git/modules/$sm_name" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,"  ", -e s,' (fetch)',, >&2
-				die "$(eval_gettextln "\
-If you want to reuse this local git directory instead of cloning again from
-  \$realrepo
-use the '--force' option. If the local git directory is not the correct repo
-or you are unsure what this means choose another name with the '--name' option.")"
-			else
-				eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
-			fi
-		fi
-		git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"--progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-		(
-			sanitize_submodule_env
-			cd "$sm_path" &&
-			# ash fails to wordsplit ${branch:+-b "$branch"...}
-			case "$branch" in
-			'') git checkout -f -q ;;
-			?*) git checkout -f -q -B "$branch" "origin/$branch" ;;
-			esac
-		) || die "$(eval_gettext "Unable to checkout submodule '\$sm_path'")"
-	fi
+	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
 	git config submodule."$sm_name".url "$realrepo"
 
 	git add --no-warn-embedded-repo $force "$sm_path" ||
-- 
2.31.1

[PATCH v6 3/3] submodule--helper: introduce add-config subcommand

[Atharva Raykar]

Add a new "add-config" subcommand to `git submodule--helper` with the
goal of converting part of the shell code in git-submodule.sh related to
`git submodule add` into C code. This new subcommand sets the
configuration variables of a newly added submodule, by registering the
url in local git config, as well as the submodule name and path in the
.gitmodules file. It also sets 'submodule.<name>.active' to "true" if
the submodule path has not already been covered by any pathspec
specified in 'submodule.active'.

This is meant to be a faithful conversion from shell to C, with only one
minor change: A warning is emitted if no value is specified in
'submodule.active', ie, the config looks like: "[submodule] active\n",
because it is an invalid configuration. It would be helpful to let the
user know that the pathspec is unset, and the value of
'submodule.<name>.active' might be set to 'true' so that they can
rectify their configuration and prevent future surprises (especially
given that the latter variable has a higher priority than the former).

The structure of the conditional to check if we need to set the 'active'
toggle looks different from the shell version -- but behaves the same.
The change was made to decrease code duplication. A comment has been
added to explain that only one value of 'submodule.active' is obtained
to check if we need to call is_submodule_active() at all.

This is part of a series of changes that will result in all of
'submodule add' being converted to C.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
Mentored-by: Christian Couder <christian.couder@gmail.com>
Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com>
Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com>
---
 builtin/submodule--helper.c | 125 ++++++++++++++++++++++++++++++++++++
 git-submodule.sh            |  28 +-------
 2 files changed, 126 insertions(+), 27 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 6d52a73a57..bce1e06d74 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)
 	return 0;
 }
 
+static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)
+{
+	char *key;
+
+	if (!is_writing_gitmodules_ok())
+		die(_("please make sure that the .gitmodules file is in the working tree"));
+
+	key = xstrfmt("submodule.%s.%s", name, var);
+	config_set_in_gitmodules_file_gently(key, value);
+	free(key);
+}
+
+static void configure_added_submodule(struct add_data *add_data)
+{
+	char *key, *submod_pathspec = NULL;
+	struct child_process add_submod = CHILD_PROCESS_INIT;
+	struct child_process add_gitmodules = CHILD_PROCESS_INIT;
+	int pathspec_key_exists, activate = 0;
+
+	key = xstrfmt("submodule.%s.url", add_data->sm_name);
+	git_config_set_gently(key, add_data->realrepo);
+	free(key);
+
+	add_submod.git_cmd = 1;
+	strvec_pushl(&add_submod.args, "add",
+		     "--no-warn-embedded-repo", NULL);
+	if (add_data->force)
+		strvec_push(&add_submod.args, "--force");
+	strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL);
+
+	if (run_command(&add_submod))
+		die(_("Failed to add submodule '%s'"), add_data->sm_path);
+
+	config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path);
+	config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo);
+	if (add_data->branch)
+		config_submodule_in_gitmodules(add_data->sm_name,
+					       "branch", add_data->branch);
+
+	add_gitmodules.git_cmd = 1;
+	strvec_pushl(&add_gitmodules.args,
+		     "add", "--force", "--", ".gitmodules", NULL);
+
+	if (run_command(&add_gitmodules))
+		die(_("Failed to register submodule '%s'"), add_data->sm_path);
+
+	/*
+	 * NEEDSWORK: In a multi-working-tree world this needs to be
+	 * set in the per-worktree config.
+	 */
+	pathspec_key_exists = !git_config_get_string("submodule.active",
+						     &submod_pathspec);
+	if (pathspec_key_exists && !submod_pathspec) {
+		warning(_("The submodule.active configuration exists, but the "
+			  "pathspec was unset. If the submodule is not already "
+			  "active, the value of submodule.%s.active will be "
+			  "be set to 'true'."), add_data->sm_name);
+		activate = 1;
+	}
+
+	/*
+	 * If submodule.active does not exist, or if the pathspec was unset,
+	 * we will activate this module unconditionally.
+	 *
+	 * Otherwise, we ask is_submodule_active(), which iterates
+	 * through all the values of 'submodule.active' to determine
+	 * if this module is already active.
+	 */
+	if (!pathspec_key_exists || activate ||
+	    !is_submodule_active(the_repository, add_data->sm_path)) {
+		key = xstrfmt("submodule.%s.active", add_data->sm_name);
+		git_config_set_gently(key, "true");
+		free(key);
+	}
+}
+
+static int add_config(int argc, const char **argv, const char *prefix)
+{
+	int force = 0;
+	struct add_data add_data = ADD_DATA_INIT;
+
+	struct option options[] = {
+		OPT_STRING('b', "branch", &add_data.branch,
+			   N_("branch"),
+			   N_("branch of repository to store in "
+			      "the submodule configuration")),
+		OPT_STRING(0, "url", &add_data.repo,
+			   N_("string"),
+			   N_("url to clone submodule from")),
+		OPT_STRING(0, "resolved-url", &add_data.realrepo,
+			   N_("string"),
+			   N_("url to clone the submodule from, after it has "
+			      "been dereferenced relative to parent's url, "
+			      "in the case where <url> is a relative url")),
+		OPT_STRING(0, "path", &add_data.sm_path,
+			   N_("path"),
+			   N_("where the new submodule will be cloned to")),
+		OPT_STRING(0, "name", &add_data.sm_name,
+			   N_("string"),
+			   N_("name of the new submodule")),
+		OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"),
+			   PARSE_OPT_NOCOMPLETE),
+		OPT_END()
+	};
+
+	const char *const usage[] = {
+		N_("git submodule--helper add-config "
+		   "[--force|-f] [--branch|-b <branch>] "
+		   "--url <url> --resolved-url <resolved-url> "
+		   "--path <path> --name <name>"),
+		NULL
+	};
+
+	argc = parse_options(argc, argv, prefix, options, usage, 0);
+
+	if (argc != 0)
+		usage_with_options(usage, options);
+
+	add_data.force = !!force;
+	configure_added_submodule(&add_data);
+
+	return 0;
+}
+
 #define SUPPORT_SUPER_PREFIX (1<<0)
 
 struct cmd_struct {
@@ -2949,6 +3073,7 @@ static struct cmd_struct commands[] = {
 	{"name", module_name, 0},
 	{"clone", module_clone, 0},
 	{"add-clone", add_clone, 0},
+	{"add-config", add_config, 0},
 	{"update-module-mode", module_update_module_mode, 0},
 	{"update-clone", update_clone, 0},
 	{"ensure-core-worktree", ensure_core_worktree, 0},
diff --git a/git-submodule.sh b/git-submodule.sh
index f71e1e5495..9826378fa6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -242,33 +242,7 @@ cmd_add()
 	fi
 
 	git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit
-	git config submodule."$sm_name".url "$realrepo"
-
-	git add --no-warn-embedded-repo $force "$sm_path" ||
-	die "$(eval_gettext "Failed to add submodule '\$sm_path'")"
-
-	git submodule--helper config submodule."$sm_name".path "$sm_path" &&
-	git submodule--helper config submodule."$sm_name".url "$repo" &&
-	if test -n "$branch"
-	then
-		git submodule--helper config submodule."$sm_name".branch "$branch"
-	fi &&
-	git add --force .gitmodules ||
-	die "$(eval_gettext "Failed to register submodule '\$sm_path'")"
-
-	# NEEDSWORK: In a multi-working-tree world, this needs to be
-	# set in the per-worktree config.
-	if git config --get submodule.active >/dev/null
-	then
-		# If the submodule being adding isn't already covered by the
-		# current configured pathspec, set the submodule's active flag
-		if ! git submodule--helper is-active "$sm_path"
-		then
-			git config submodule."$sm_name".active "true"
-		fi
-	else
-		git config submodule."$sm_name".active "true"
-	fi
+	git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name"
 }
 
 #
-- 
2.31.1