{"thread":{"id":"55791","subject":"[PATCH][GSoC] submodule: introduce add-clone helper for submodule add","startedAt":"2021-05-28T08:13:12Z","lastAt":"2021-06-04T12:02:43Z","messageCount":11,"participants":["Atharva Raykar","Christian Couder","Shourya Shukla"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"425748","messageId":"20210528081224.69163-1-raykar.ath@gmail.com","threadId":"55791","inReplyTo":null,"subject":"[PATCH][GSoC] submodule: introduce add-clone helper for submodule add","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-05-28T08:12:24Z","receivedAt":"2021-05-28T08:13:12Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Convert the the shell code that performs the cloning of the repository that is\nto be added, and checks out to the appropriate branch.\n\nThis is meant to be a faithful conversion that leaves the behaviour of\n'submodule add' unchanged. The only minor change is that if a submodule name has\nbeen supplied with a name that clashes with a local submodule, the message shown\nto the user (\"A git directory for 'foo' is found locally...\") is prepended with\n\"error\" for clarity.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nThis is part of a series of changes that will result in all of 'submodule add'\nbeing converted to C, which is a more familiar language for Git developers, and\npaves the way to improve performance and portability.\n\nI have made this patch based on Shourya's patch[1]. I have decided to send the\nchanges in smaller, more reviewable parts. The add-clone subcommand of\nsubmodule--helper is an intermediate change, while I work on translating all of\nthe code. So in the next few patches, this helper subcommand is likely to be\nremoved as its functionality would be invoked from the C code itself.\n\n[1] https://lore.kernel.org/git/20201214231939.644175-1-periperidip@gmail.com/\n\n builtin/submodule--helper.c | 221 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +------\n 2 files changed, 222 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..39a844b0b1 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2745,6 +2745,226 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \treturn !!ret;\n }\n \n+struct add_data {\n+\tconst char *prefix;\n+\tconst char *branch;\n+\tconst char *reference_path;\n+\tconst char *sm_path;\n+\tconst char *sm_name;\n+\tconst char *repo;\n+\tconst char *realrepo;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+};\n+#define ADD_DATA_INIT { 0 }\n+\n+static char *parse_token(char **begin, const char *end)\n+{\n+\tint size;\n+\tchar *token, *pos = *begin;\n+\twhile (pos != end && (*pos != ' ' && *pos != '\\t' && *pos != '\\n'))\n+\t\tpos++;\n+\tsize = pos - *begin;\n+\ttoken = xstrndup(*begin, size);\n+\t*begin = pos + 1;\n+\treturn token;\n+}\n+\n+static char *get_next_line(char *const begin, const char *const end)\n+{\n+\tchar *pos = begin;\n+\twhile (pos != end && *pos++ != '\\n');\n+\treturn pos;\n+}\n+\n+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n+{\n+\tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb_remote_out = STRBUF_INIT;\n+\n+\tcp_remote.git_cmd = 1;\n+\tstrvec_pushf(&cp_remote.env_array,\n+\t\t \"GIT_DIR=%s\", git_dir_path);\n+\tstrvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n+\tstrvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n+\tif (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n+\t\tchar *next_line, *name, *url, *tail;\n+\t\tchar *begin = sb_remote_out.buf;\n+\t\tchar *end = sb_remote_out.buf + sb_remote_out.len;\n+\t\twhile (begin != end &&\n+\t\t (next_line = get_next_line(begin, end))) {\n+\t\t\tname = parse_token(&begin, next_line);\n+\t\t\turl = parse_token(&begin, next_line);\n+\t\t\ttail = parse_token(&begin, next_line);\n+\t\t\tif (!memcmp(tail, \"(fetch)\", 7))\n+\t\t\t\tfprintf(output, \" %s\\t%s\\n\", name, url);\n+\t\t\tfree(url);\n+\t\t\tfree(name);\n+\t\t\tfree(tail);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb_remote_out);\n+}\n+\n+static int add_submodule(const struct add_data *info)\n+{\n+\tchar *submod_gitdir_path;\n+\t/* perhaps the path already exists and is already a git repo, else clone it */\n+\tif (is_directory(info->sm_path)) {\n+\t\tprintf(\"sm_path=%s\\n\", info->sm_path);\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", info->sm_path);\n+\t\tif (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))\n+\t\t\tprintf(_(\"Adding existing path at '%s' to index\\n\"),\n+\t\t\t info->sm_path);\n+\t\telse\n+\t\t\tdie(_(\"'%s' already exists and is not a valid git repo\"),\n+\t\t\t info->sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\n+\t\tstruct strvec clone_args = STRVEC_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tsubmod_gitdir_path = xstrfmt(\".git/modules/%s\", info->sm_name);\n+\n+\t\tif (is_directory(submod_gitdir_path)) {\n+\t\t\tif (!info->force) {\n+\t\t\t\terror(_(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\"locally with remote(s):\"), info->sm_name);\n+\t\t\t\tshow_fetch_remotes(stderr, info->sm_name,\n+\t\t\t\t\t\t submod_gitdir_path);\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t_(\"If you want to reuse this local git \"\n+\t\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t\t \" %s\\n\"\n+\t\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t\tinfo->realrepo);\n+\t\t\t\tfree(submod_gitdir_path);\n+\t\t\t\treturn 1;\n+\t\t\t} else {\n+\t\t\t\tprintf(_(\"Reactivating local git directory for \"\n+\t\t\t\t\t \"submodule '%s'\\n\"), info->sm_name);\n+\t\t\t}\n+\t\t}\n+\t\tfree(submod_gitdir_path);\n+\n+\t\tstrvec_push(&clone_args, \"clone\");\n+\n+\t\tif (info->quiet)\n+\t\t\tstrvec_push(&clone_args, \"--quiet\");\n+\n+\t\tif (info->progress)\n+\t\t\tstrvec_push(&clone_args, \"--progress\");\n+\n+\t\tif (info->prefix)\n+\t\t\tstrvec_pushl(&clone_args, \"--prefix\", info->prefix, NULL);\n+\n+\t\tstrvec_pushl(&clone_args, \"--path\", info->sm_path, \"--name\",\n+\t\t\t info->sm_name, \"--url\", info->realrepo, NULL);\n+\n+\t\tif (info->reference_path)\n+\t\t\tstrvec_pushl(&clone_args, \"--reference\",\n+\t\t\t\t info->reference_path, NULL);\n+\n+\t\tif (info->dissociate)\n+\t\t\tstrvec_push(&clone_args, \"--dissociate\");\n+\n+\t\tif (info->depth >= 0)\n+\t\t\tstrvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n+\n+\t\tif (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n+\t\t\tstrvec_clear(&clone_args);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tstrvec_clear(&clone_args);\n+\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.git_cmd = 1;\n+\t\tcp.dir = info->sm_path;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-f\", \"-q\", NULL);\n+\n+\t\tif (info->branch) {\n+\t\t\tstrvec_pushl(&cp.args, \"-B\", info->branch, NULL);\n+\t\t\tstrvec_pushf(&cp.args, \"origin/%s\", info->branch);\n+\t\t}\n+\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"unable to checkout submodule '%s'\"), info->sm_path);\n+\t}\n+\treturn 0;\n+}\n+\n+static int add_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *branch = NULL, *sm_path = NULL;\n+\tconst char *wt_prefix = NULL, *realrepo = NULL;\n+\tconst char *reference = NULL, *sm_name = NULL;\n+\tint force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0;\n+\tstruct add_data info = ADD_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING('b', \"branch\", &branch,\n+\t\t\t N_(\"branch\"),\n+\t\t\t N_(\"branch of repository to checkout on cloning\")),\n+\t\tOPT_STRING(0, \"prefix\", &wt_prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_STRING(0, \"path\", &sm_path,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"where the new submodule will be cloned to\")),\n+\t\tOPT_STRING(0, \"name\", &sm_name,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"name of the new submodule\")),\n+\t\tOPT_STRING(0, \"url\", &realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url where to clone the submodule from\")),\n+\t\tOPT_STRING(0, \"reference\", &reference,\n+\t\t\t N_(\"repo\"),\n+\t\t\t N_(\"reference repository\")),\n+\t\tOPT_BOOL(0, \"dissociate\", &dissociate,\n+\t\t\t N_(\"use --reference only while cloning\")),\n+\t\tOPT_INTEGER(0, \"depth\", &depth,\n+\t\t\t N_(\"depth for shallow clones\")),\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"force cloning progress\")),\n+\t\tOPT_BOOL('f', \"force\", &force,\n+\t\t\t N_(\"allow adding an otherwise ignored submodule path\")),\n+\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper clone [--prefix=] [--quiet] [--force] \"\n+\t\t \"[--reference ] [--depth ] [-b|--branch ]\"\n+\t\t \"--url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tinfo.prefix = prefix;\n+\tinfo.sm_name = sm_name;\n+\tinfo.sm_path = sm_path;\n+\tinfo.realrepo = realrepo;\n+\tinfo.reference_path = reference;\n+\tinfo.branch = branch;\n+\tinfo.depth = depth;\n+\tinfo.progress = !!progress;\n+\tinfo.dissociate = !!dissociate;\n+\tinfo.force = !!force;\n+\tinfo.quiet = !!quiet;\n+\n+\tif (add_submodule(&info))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2757,6 +2977,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n+\t{\"add-clone\", add_clone, 0},\n \t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..f71e1e5495 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,43 +241,7 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n \tfi\n \n-\t# perhaps the path exists and is already a git repo, else clone it\n-\tif test -e \"$sm_path\"\n-\tthen\n-\t\tif test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\teval_gettextln \"Adding existing repo at '\\$sm_path' to the index\"\n-\t\telse\n-\t\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists and is not a valid git repo\")\"\n-\t\tfi\n-\n-\telse\n-\t\tif test -d \".git/modules/$sm_name\"\n-\t\tthen\n-\t\t\tif test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\teval_gettextln >&2 \"A git directory for '\\$sm_name' is found locally with remote(s):\"\n-\t\t\t\tGIT_DIR=\".git/modules/$sm_name\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\" \", -e s,' (fetch)',, >&2\n-\t\t\t\tdie \"$(eval_gettextln \"\\\n-If you want to reuse this local git directory instead of cloning again from\n- \\$realrepo\n-use the '--force' option. If the local git directory is not the correct repo\n-or you are unsure what this means choose another name with the '--name' option.\")\"\n-\t\t\telse\n-\t\t\t\teval_gettextln \"Reactivating local git directory for submodule '\\$sm_name'.\"\n-\t\t\tfi\n-\t\tfi\n-\t\tgit 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\n-\t\t(\n-\t\t\tsanitize_submodule_env\n-\t\t\tcd \"$sm_path\" &&\n-\t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n-\t\t\tcase \"$branch\" in\n-\t\t\t'') git checkout -f -q ;;\n-\t\t\t?*) git checkout -f -q -B \"$branch\" \"origin/$branch\" ;;\n-\t\t\tesac\n-\t\t) || die \"$(eval_gettext \"Unable to checkout submodule '\\$sm_path'\")\"\n-\tfi\n+\tgit 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\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-- \n2.31.1\n\n"},{"id":"426170","messageId":"CAP8UFD3SMghGb0y0jKuLScrKqqHgZFDxW1c97MwoEz+1hXt1hA@mail.gmail.com","threadId":"55791","inReplyTo":"20210528081224.69163-1-raykar.ath@gmail.com","subject":"Re: [PATCH][GSoC] submodule: introduce add-clone helper for submodule add","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-06-01T22:10:13Z","receivedAt":"2021-06-01T22:10:29Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 28, 2021 at 10:13 AM Atharva Raykar wrote:\n>\n> Convert the the shell code that performs the cloning of the repository that is\n\ns/the the/the/\n\n> to be added, and checks out to the appropriate branch.\n\nSomething a bit more explicit might make things easier to understand.\nFor example:\n\n\"Let's add a new \"add-clone\" subcommand to `git submodule--helper`\nwith the goal of converting part of the shell code in git-submodule.sh\nrelated to `git submodule add` into C code. This new subcommand clones\nthe repository that is to be added, and checks out to the appropriate\nbranch.\"\n\nThen a simpler title could be:\n\n\"submodule--helper: introduce add-clone subcommand\"\n\n> This is meant to be a faithful conversion that leaves the behaviour of\n> 'submodule add' unchanged. The only minor change is that if a submodule name has\n> been supplied with a name that clashes with a local submodule, the message shown\n> to the user (\"A git directory for 'foo' is found locally...\") is prepended with\n> \"error\" for clarity.\n\nGood.\n\n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n> ---\n>\n> This is part of a series of changes that will result in all of 'submodule add'\n> being converted to C, which is a more familiar language for Git developers, and\n> paves the way to improve performance and portability.\n>\n> I have made this patch based on Shourya's patch[1]. I have decided to send the\n> changes in smaller, more reviewable parts. The add-clone subcommand of\n> submodule--helper is an intermediate change, while I work on translating all of\n> the code. So in the next few patches, this helper subcommand is likely to be\n> removed as its functionality would be invoked from the C code itself.\n\nIt might be a good idea to let us know how many such new subcommands\nyou'd like to introduce before removing them.\n\nAnyway I think it's a good idea to send changes in smaller, more\neasily reviewable parts. Hopefully this way more work will end up\nbeing merged.\n\n> [1] https://lore.kernel.org/git/20201214231939.644175-1-periperidip@gmail.com/\n>\n> builtin/submodule--helper.c | 221 ++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 38 +------\n> 2 files changed, 222 insertions(+), 37 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index d55f6262e9..39a844b0b1 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2745,6 +2745,226 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n> return !!ret;\n> }\n>\n> +struct add_data {\n> + const char *prefix;\n> + const char *branch;\n> + const char *reference_path;\n> + const char *sm_path;\n> + const char *sm_name;\n> + const char *repo;\n> + const char *realrepo;\n> + int depth;\n> + unsigned int force: 1;\n> + unsigned int quiet: 1;\n> + unsigned int progress: 1;\n> + unsigned int dissociate: 1;\n> +};\n> +#define ADD_DATA_INIT { 0 }\n> +\n> +static char *parse_token(char **begin, const char *end)\n> +{\n> + int size;\n> + char *token, *pos = *begin;\n> + while (pos != end && (*pos != ' ' && *pos != '\\t' && *pos != '\\n'))\n> + pos++;\n> + size = pos - *begin;\n> + token = xstrndup(*begin, size);\n> + *begin = pos + 1;\n> + return token;\n> +}\n> +\n> +static char *get_next_line(char *const begin, const char *const end)\n> +{\n> + char *pos = begin;\n> + while (pos != end && *pos++ != '\\n');\n> + return pos;\n> +}\n> +\n> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n> +{\n> + struct child_process cp_remote = CHILD_PROCESS_INIT;\n> + struct strbuf sb_remote_out = STRBUF_INIT;\n> +\n> + cp_remote.git_cmd = 1;\n> + strvec_pushf(&cp_remote.env_array,\n> + \"GIT_DIR=%s\", git_dir_path);\n> + strvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n> + strvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n> + char *next_line, *name, *url, *tail;\n\nMaybe name, url and tail could be declared in the while loop below\nwhere they are used.\n\n> + char *begin = sb_remote_out.buf;\n> + char *end = sb_remote_out.buf + sb_remote_out.len;\n> + while (begin != end &&\n> + (next_line = get_next_line(begin, end))) {\n\nIt would be nice if the above 2 lines could be reduced into just one\nline. Maybe renaming \"next_line\" to just \"line\" could help with that.\n\n> + name = parse_token(&begin, next_line);\n> + url = parse_token(&begin, next_line);\n> + tail = parse_token(&begin, next_line);\n> + if (!memcmp(tail, \"(fetch)\", 7))\n> + fprintf(output, \" %s\\t%s\\n\", name, url);\n> + free(url);\n> + free(name);\n> + free(tail);\n> + }\n> + }\n> +\n> + strbuf_release(&sb_remote_out);\n> +}\n> +\n> +static int add_submodule(const struct add_data *info)\n> +{\n> + char *submod_gitdir_path;\n> + /* perhaps the path already exists and is already a git repo, else clone it */\n> + if (is_directory(info->sm_path)) {\n> + printf(\"sm_path=%s\\n\", info->sm_path);\n\nIs this a leftover debug statement?\n\n> + submod_gitdir_path = xstrfmt(\"%s/.git\", info->sm_path);\n> + if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))\n> + printf(_(\"Adding existing path at '%s' to index\\n\"),\n> + info->sm_path);\n> + else\n> + die(_(\"'%s' already exists and is not a valid git repo\"),\n> + info->sm_path);\n> + free(submod_gitdir_path);\n> + } else {\n> + struct strvec clone_args = STRVEC_INIT;\n> + struct child_process cp = CHILD_PROCESS_INIT;\n> + submod_gitdir_path = xstrfmt(\".git/modules/%s\", info->sm_name);\n> +\n> + if (is_directory(submod_gitdir_path)) {\n> + if (!info->force) {\n> + error(_(\"A git directory for '%s' is found \"\n> + \"locally with remote(s):\"), info->sm_name);\n> + show_fetch_remotes(stderr, info->sm_name,\n> + submod_gitdir_path);\n> + fprintf(stderr,\n> + _(\"If you want to reuse this local git \"\n> + \"directory instead of cloning again from\\n\"\n> + \" %s\\n\"\n> + \"use the '--force' option. If the local git \"\n> + \"directory is not the correct repo\\n\"\n> + \"or if you are unsure what this means, choose \"\n> + \"another name with the '--name' option.\\n\"),\n> + info->realrepo);\n> + free(submod_gitdir_path);\n> + return 1;\n> + } else {\n> + printf(_(\"Reactivating local git directory for \"\n> + \"submodule '%s'\\n\"), info->sm_name);\n> + }\n> + }\n> + free(submod_gitdir_path);\n> +\n> + strvec_push(&clone_args, \"clone\");\n> +\n> + if (info->quiet)\n> + strvec_push(&clone_args, \"--quiet\");\n> +\n> + if (info->progress)\n> + strvec_push(&clone_args, \"--progress\");\n> +\n> + if (info->prefix)\n> + strvec_pushl(&clone_args, \"--prefix\", info->prefix, NULL);\n> +\n> + strvec_pushl(&clone_args, \"--path\", info->sm_path, \"--name\",\n> + info->sm_name, \"--url\", info->realrepo, NULL);\n\nMaybe this unconditional strvec_pushl(...) could be squashed into the\nstrvec_push(&clone_args, \"clone\") above.\n\n> + if (info->reference_path)\n> + strvec_pushl(&clone_args, \"--reference\",\n> + info->reference_path, NULL);\n> +\n> + if (info->dissociate)\n> + strvec_push(&clone_args, \"--dissociate\");\n> +\n\nBlank lines since the above strvec_push(&clone_args, \"clone\") could\nperhaps be removed.\n\n> + if (info->depth >= 0)\n> + strvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n> +\n> + if (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n> + strvec_clear(&clone_args);\n> + return -1;\n> + }\n> + strvec_clear(&clone_args);\n> +\n> + prepare_submodule_repo_env(&cp.env_array);\n> + cp.git_cmd = 1;\n> + cp.dir = info->sm_path;\n> + strvec_pushl(&cp.args, \"checkout\", \"-f\", \"-q\", NULL);\n> +\n> + if (info->branch) {\n> + strvec_pushl(&cp.args, \"-B\", info->branch, NULL);\n> + strvec_pushf(&cp.args, \"origin/%s\", info->branch);\n> + }\n> +\n> + if (run_command(&cp))\n> + die(_(\"unable to checkout submodule '%s'\"), info->sm_path);\n> + }\n> + return 0;\n> +}\n> +\n> +static int add_clone(int argc, const char **argv, const char *prefix)\n> +{\n> + const char *branch = NULL, *sm_path = NULL;\n> + const char *wt_prefix = NULL, *realrepo = NULL;\n> + const char *reference = NULL, *sm_name = NULL;\n> + int force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0;\n> + struct add_data info = ADD_DATA_INIT;\n> +\n> + struct option options[] = {\n> + OPT_STRING('b', \"branch\", &branch,\n> + N_(\"branch\"),\n> + N_(\"branch of repository to checkout on cloning\")),\n> + OPT_STRING(0, \"prefix\", &wt_prefix,\n> + N_(\"path\"),\n> + N_(\"alternative anchor for relative paths\")),\n> + OPT_STRING(0, \"path\", &sm_path,\n> + N_(\"path\"),\n> + N_(\"where the new submodule will be cloned to\")),\n> + OPT_STRING(0, \"name\", &sm_name,\n> + N_(\"string\"),\n> + N_(\"name of the new submodule\")),\n> + OPT_STRING(0, \"url\", &realrepo,\n> + N_(\"string\"),\n> + N_(\"url where to clone the submodule from\")),\n> + OPT_STRING(0, \"reference\", &reference,\n> + N_(\"repo\"),\n> + N_(\"reference repository\")),\n> + OPT_BOOL(0, \"dissociate\", &dissociate,\n> + N_(\"use --reference only while cloning\")),\n> + OPT_INTEGER(0, \"depth\", &depth,\n> + N_(\"depth for shallow clones\")),\n> + OPT_BOOL(0, \"progress\", &progress,\n> + N_(\"force cloning progress\")),\n> + OPT_BOOL('f', \"force\", &force,\n> + N_(\"allow adding an otherwise ignored submodule path\")),\n> + OPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n> + OPT_END()\n> + };\n> +\n> + const char *const usage[] = {\n> + N_(\"git submodule--helper clone [--prefix=] [--quiet] [--force] \"\n\ns/clone/add-clone/\n\n> + \"[--reference ] [--depth ] [-b|--branch ]\"\n> + \"--url --path --name \"),\n\nThe --progress and --dissociate options seem to be missing.\n\n> + NULL\n> + };\n"},{"id":"426206","messageId":"38AEA0B4-FCF7-4123-9412-98C1394972B0@gmail.com","threadId":"55791","inReplyTo":"CAP8UFD3SMghGb0y0jKuLScrKqqHgZFDxW1c97MwoEz+1hXt1hA@mail.gmail.com","subject":"Re: [PATCH][GSoC] submodule: introduce add-clone helper for submodule add","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-02T07:55:07Z","receivedAt":"2021-06-02T07:55:15Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 02-Jun-2021, at 03:40, Christian Couder wrote:\n> \n> On Fri, May 28, 2021 at 10:13 AM Atharva Raykar wrote:\n>> \n>> Convert the the shell code that performs the cloning of the repository that is\n> \n> s/the the/the/\n> \n>> to be added, and checks out to the appropriate branch.\n> \n> Something a bit more explicit might make things easier to understand.\n> For example:\n> \n> \"Let's add a new \"add-clone\" subcommand to `git submodule--helper`\n> with the goal of converting part of the shell code in git-submodule.sh\n> related to `git submodule add` into C code. This new subcommand clones\n> the repository that is to be added, and checks out to the appropriate\n> branch.\"\n> \n> Then a simpler title could be:\n> \n> \"submodule--helper: introduce add-clone subcommand\"\n\nGreat suggestions. I'll update my commit message.\n\n>> This is meant to be a faithful conversion that leaves the behaviour of\n>> 'submodule add' unchanged. The only minor change is that if a submodule name has\n>> been supplied with a name that clashes with a local submodule, the message shown\n>> to the user (\"A git directory for 'foo' is found locally...\") is prepended with\n>> \"error\" for clarity.\n> \n> Good.\n> \n>> Signed-off-by: Atharva Raykar \n>> Mentored-by: Christian Couder \n>> Mentored-by: Shourya Shukla \n>> Based-on-patch-by: Shourya Shukla \n>> Based-on-patch-by: Prathamesh Chavan \n>> ---\n>> \n>> This is part of a series of changes that will result in all of 'submodule add'\n>> being converted to C, which is a more familiar language for Git developers, and\n>> paves the way to improve performance and portability.\n>> \n>> I have made this patch based on Shourya's patch[1]. I have decided to send the\n>> changes in smaller, more reviewable parts. The add-clone subcommand of\n>> submodule--helper is an intermediate change, while I work on translating all of\n>> the code. So in the next few patches, this helper subcommand is likely to be\n>> removed as its functionality would be invoked from the C code itself.\n> \n> It might be a good idea to let us know how many such new subcommands\n> you'd like to introduce before removing them.\n\nI'll add that in my description of v2.\n\n> Anyway I think it's a good idea to send changes in smaller, more\n> easily reviewable parts. Hopefully this way more work will end up\n> being merged.\n> \n>> [...]\n>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n>> +{\n>> + struct child_process cp_remote = CHILD_PROCESS_INIT;\n>> + struct strbuf sb_remote_out = STRBUF_INIT;\n>> +\n>> + cp_remote.git_cmd = 1;\n>> + strvec_pushf(&cp_remote.env_array,\n>> + \"GIT_DIR=%s\", git_dir_path);\n>> + strvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n>> + strvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n>> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n>> + char *next_line, *name, *url, *tail;\n> \n> Maybe name, url and tail could be declared in the while loop below\n> where they are used.\n\nWill do. Just to better understand your intent, is the reason to\ndo this to make the declarations closer to usage, for the sake of\nbetter readability?\n\nI've not yet fully developed a taste for good C style, so I wanted\nto ask, which one looks better to you in these?\n\n/* Sample 1 */\nwhile (begin != end && (line = get_next_line(begin, end))) {\n\tchar *name, *url, *tail;\n\tname = parse_token(&begin, next_line);\n\turl = parse_token(&begin, next_line);\n\ttail = parse_token(&begin, next_line);\n\t...\n}\n\n/* Sample 2 */\nwhile (begin != end && (line = get_next_line(begin, end))) {\n\tchar *name = parse_token(&begin, next_line);\n\tchar *url = parse_token(&begin, next_line);\n\tchar *tail = parse_token(&begin, next_line);\n\t...\n}\n\n>> + char *begin = sb_remote_out.buf;\n>> + char *end = sb_remote_out.buf + sb_remote_out.len;\n>> + while (begin != end &&\n>> + (next_line = get_next_line(begin, end))) {\n> \n> It would be nice if the above 2 lines could be reduced into just one\n> line. Maybe renaming \"next_line\" to just \"line\" could help with that.\n\nNoted.\n\n>> + name = parse_token(&begin, next_line);\n>> + url = parse_token(&begin, next_line);\n>> + tail = parse_token(&begin, next_line);\n>> + if (!memcmp(tail, \"(fetch)\", 7))\n>> + fprintf(output, \" %s\\t%s\\n\", name, url);\n>> + free(url);\n>> + free(name);\n>> + free(tail);\n>> + }\n>> + }\n>> +\n>> + strbuf_release(&sb_remote_out);\n>> +}\n>> +\n>> +static int add_submodule(const struct add_data *info)\n>> +{\n>> + char *submod_gitdir_path;\n>> + /* perhaps the path already exists and is already a git repo, else clone it */\n>> + if (is_directory(info->sm_path)) {\n>> + printf(\"sm_path=%s\\n\", info->sm_path);\n> \n> Is this a leftover debug statement?\n\nNope, at least not _my_ leftover debug statement.\n\nI saw it in git-submodule.sh here, so I preserved it:\n\n-\t# perhaps the path exists and is already a git repo, else clone it\n-\tif test -e \"$sm_path\"\n-\t...\n\nPersonally, I found that comment quite useful when I was trying to\nunderstand the shell version, because at a glance I immediately\nknew what the intention of the big block of code was, that followed\nthe comment.\n\nPerhaps it could be broken into many functions to make it more\nreadable without needing a comment, but that is outside the scope\nof this particular patch, which is aiming for a faithful conversion.\n\n>> + submod_gitdir_path = xstrfmt(\"%s/.git\", info->sm_path);\n>> + if (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))\n>> + printf(_(\"Adding existing path at '%s' to index\\n\"),\n>> + info->sm_path);\n>> + else\n>> + die(_(\"'%s' already exists and is not a valid git repo\"),\n>> + info->sm_path);\n>> + free(submod_gitdir_path);\n>> + } else {\n>> + struct strvec clone_args = STRVEC_INIT;\n>> + struct child_process cp = CHILD_PROCESS_INIT;\n>> + submod_gitdir_path = xstrfmt(\".git/modules/%s\", info->sm_name);\n>> +\n>> + if (is_directory(submod_gitdir_path)) {\n>> + if (!info->force) {\n>> + error(_(\"A git directory for '%s' is found \"\n>> + \"locally with remote(s):\"), info->sm_name);\n>> + show_fetch_remotes(stderr, info->sm_name,\n>> + submod_gitdir_path);\n>> + fprintf(stderr,\n>> + _(\"If you want to reuse this local git \"\n>> + \"directory instead of cloning again from\\n\"\n>> + \" %s\\n\"\n>> + \"use the '--force' option. If the local git \"\n>> + \"directory is not the correct repo\\n\"\n>> + \"or if you are unsure what this means, choose \"\n>> + \"another name with the '--name' option.\\n\"),\n>> + info->realrepo);\n>> + free(submod_gitdir_path);\n>> + return 1;\n>> + } else {\n>> + printf(_(\"Reactivating local git directory for \"\n>> + \"submodule '%s'\\n\"), info->sm_name);\n>> + }\n>> + }\n>> + free(submod_gitdir_path);\n>> +\n>> + strvec_push(&clone_args, \"clone\");\n>> +\n>> + if (info->quiet)\n>> + strvec_push(&clone_args, \"--quiet\");\n>> +\n>> + if (info->progress)\n>> + strvec_push(&clone_args, \"--progress\");\n>> +\n>> + if (info->prefix)\n>> + strvec_pushl(&clone_args, \"--prefix\", info->prefix, NULL);\n>> +\n>> + strvec_pushl(&clone_args, \"--path\", info->sm_path, \"--name\",\n>> + info->sm_name, \"--url\", info->realrepo, NULL);\n> \n> Maybe this unconditional strvec_pushl(...) could be squashed into the\n> strvec_push(&clone_args, \"clone\") above.\n\nGot it.\n\n>> + if (info->reference_path)\n>> + strvec_pushl(&clone_args, \"--reference\",\n>> + info->reference_path, NULL);\n>> +\n>> + if (info->dissociate)\n>> + strvec_push(&clone_args, \"--dissociate\");\n>> +\n> \n> Blank lines since the above strvec_push(&clone_args, \"clone\") could\n> perhaps be removed.\n\nWill do.\n\n>> [...]\n>> + const char *const usage[] = {\n>> + N_(\"git submodule--helper clone [--prefix=] [--quiet] [--force] \"\n> \n> s/clone/add-clone/\n> \n>> + \"[--reference ] [--depth ] [-b|--branch ]\"\n>> + \"--url --path --name \"),\n> \n> The --progress and --dissociate options seem to be missing.\n\nThanks, will fix.\n\n>> + NULL\n>> + };\n\n"},{"id":"426208","messageId":"44C3C05A-1BA8-42BF-8614-1BA859050AEE@gmail.com","threadId":"55791","inReplyTo":"38AEA0B4-FCF7-4123-9412-98C1394972B0@gmail.com","subject":"Re: [PATCH][GSoC] submodule: introduce add-clone helper for submodule add","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-02T08:18:50Z","receivedAt":"2021-06-02T08:19:01Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\n\n> On 02-Jun-2021, at 13:25, Atharva Raykar wrote:\n> \n> [...]\n> I've not yet fully developed a taste for good C style, so I wanted\n> to ask, which one looks better to you in these?\n> \n> /* Sample 1 */\n> while (begin != end && (line = get_next_line(begin, end))) {\n> \tchar *name, *url, *tail;\n> \tname = parse_token(&begin, next_line);\n> \turl = parse_token(&begin, next_line);\n> \ttail = parse_token(&begin, next_line);\n> \t...\n> }\n> \n> /* Sample 2 */\n> while (begin != end && (line = get_next_line(begin, end))) {\n> \tchar *name = parse_token(&begin, next_line);\n> \tchar *url = parse_token(&begin, next_line);\n> \tchar *tail = parse_token(&begin, next_line);\n> \t...\n> }\n\nAlso ignore the error here, assume: s/next_line/line/\n(Anyway, my question was about style)\n"},{"id":"426233","messageId":"20210602131259.50350-1-raykar.ath@gmail.com","threadId":"55791","inReplyTo":"20210528081224.69163-1-raykar.ath@gmail.com","subject":"[PATCH v2] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-02T13:12:59Z","receivedAt":"2021-06-02T13:14:29Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Let's add a new \"add-clone\" subcommand to `git submodule--helper` with\nthe goal of converting part of the shell code in git-submodule.sh\nrelated to `git submodule add` into C code. This new subcommand clones\nthe repository that is to be added, and checks out to the appropriate\nbranch.\n\nThis is meant to be a faithful conversion that leaves the behaviour of\n'submodule add' unchanged. The only minor change is that if a submodule name has\nbeen supplied with a name that clashes with a local submodule, the message shown\nto the user (\"A git directory for 'foo' is found locally...\") is prepended with\n\"error\" for clarity.\n\nThis is part of a series of changes that will result in all of 'submodule add'\nbeing converted to C.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nThis is part of a series of changes that will result in all of 'submodule add'\nbeing converted to C, which is a more familiar language for Git developers, and\npaves the way to improve performance and portability.\n\nI have made this patch based on Shourya's patch[1]. I have decided to send the\nchanges in smaller, more reviewable parts. The add-clone subcommand of\nsubmodule--helper is an intermediate change, while I work on translating all of\nthe code.\n\nAnother subcommand called 'add-config' will also be added in a separate patch\nthat handles the configuration on adding the module.\n\nAfter those two changes look good enough, I will be converting whatever is left\nof 'git submodule add' in the git-submodule.sh past the flag parsing into C code\nby having one helper subcommand called 'git submodule--helper add' that will\nincorporate the functionality of the other two helpers, as well. In that patch,\nthe 'add-clone' and 'add-config' subcommands will be removed from the commands\narray, as they will be called from within the C code itself.\n\nChanges since v1:\n * Fixed typos, and made commit message more explicit\n * Fixed incorrect usage string\n * Some style changes were made\n\n builtin/submodule--helper.c | 212 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +------\n 2 files changed, 213 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..bbbb42088b 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2745,6 +2745,217 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \treturn !!ret;\n }\n \n+struct add_data {\n+\tconst char *prefix;\n+\tconst char *branch;\n+\tconst char *reference_path;\n+\tconst char *sm_path;\n+\tconst char *sm_name;\n+\tconst char *repo;\n+\tconst char *realrepo;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+};\n+#define ADD_DATA_INIT { 0 }\n+\n+static char *parse_token(char **begin, const char *end)\n+{\n+\tint size;\n+\tchar *token, *pos = *begin;\n+\twhile (pos != end && (*pos != ' ' && *pos != '\\t' && *pos != '\\n'))\n+\t\tpos++;\n+\tsize = pos - *begin;\n+\ttoken = xstrndup(*begin, size);\n+\t*begin = pos + 1;\n+\treturn token;\n+}\n+\n+static char *get_next_line(char *const begin, const char *const end)\n+{\n+\tchar *pos = begin;\n+\twhile (pos != end && *pos++ != '\\n');\n+\treturn pos;\n+}\n+\n+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n+{\n+\tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb_remote_out = STRBUF_INIT;\n+\n+\tcp_remote.git_cmd = 1;\n+\tstrvec_pushf(&cp_remote.env_array,\n+\t\t \"GIT_DIR=%s\", git_dir_path);\n+\tstrvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n+\tstrvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n+\tif (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n+\t\tchar *line;\n+\t\tchar *begin = sb_remote_out.buf;\n+\t\tchar *end = sb_remote_out.buf + sb_remote_out.len;\n+\t\twhile (begin != end && (line = get_next_line(begin, end))) {\n+\t\t\tchar *name, *url, *tail;\n+\t\t\tname = parse_token(&begin, line);\n+\t\t\turl = parse_token(&begin, line);\n+\t\t\ttail = parse_token(&begin, line);\n+\t\t\tif (!memcmp(tail, \"(fetch)\", 7))\n+\t\t\t\tfprintf(output, \" %s\\t%s\\n\", name, url);\n+\t\t\tfree(url);\n+\t\t\tfree(name);\n+\t\t\tfree(tail);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb_remote_out);\n+}\n+\n+static int add_submodule(const struct add_data *info)\n+{\n+\tchar *submod_gitdir_path;\n+\t/* perhaps the path already exists and is already a git repo, else clone it */\n+\tif (is_directory(info->sm_path)) {\n+\t\tprintf(\"sm_path=%s\\n\", info->sm_path);\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", info->sm_path);\n+\t\tif (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))\n+\t\t\tprintf(_(\"Adding existing path at '%s' to index\\n\"),\n+\t\t\t info->sm_path);\n+\t\telse\n+\t\t\tdie(_(\"'%s' already exists and is not a valid git repo\"),\n+\t\t\t info->sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\n+\t\tstruct strvec clone_args = STRVEC_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tsubmod_gitdir_path = xstrfmt(\".git/modules/%s\", info->sm_name);\n+\n+\t\tif (is_directory(submod_gitdir_path)) {\n+\t\t\tif (!info->force) {\n+\t\t\t\terror(_(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\"locally with remote(s):\"), info->sm_name);\n+\t\t\t\tshow_fetch_remotes(stderr, info->sm_name,\n+\t\t\t\t\t\t submod_gitdir_path);\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t_(\"If you want to reuse this local git \"\n+\t\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t\t \" %s\\n\"\n+\t\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t\tinfo->realrepo);\n+\t\t\t\tfree(submod_gitdir_path);\n+\t\t\t\treturn 1;\n+\t\t\t} else {\n+\t\t\t\tprintf(_(\"Reactivating local git directory for \"\n+\t\t\t\t\t \"submodule '%s'\\n\"), info->sm_name);\n+\t\t\t}\n+\t\t}\n+\t\tfree(submod_gitdir_path);\n+\n+\t\tstrvec_pushl(&clone_args, \"clone\", \"--path\", info->sm_path, \"--name\",\n+\t\t\t info->sm_name, \"--url\", info->realrepo, NULL);\n+\t\tif (info->quiet)\n+\t\t\tstrvec_push(&clone_args, \"--quiet\");\n+\t\tif (info->progress)\n+\t\t\tstrvec_push(&clone_args, \"--progress\");\n+\t\tif (info->prefix)\n+\t\t\tstrvec_pushl(&clone_args, \"--prefix\", info->prefix, NULL);\n+\t\tif (info->reference_path)\n+\t\t\tstrvec_pushl(&clone_args, \"--reference\",\n+\t\t\t\t info->reference_path, NULL);\n+\t\tif (info->dissociate)\n+\t\t\tstrvec_push(&clone_args, \"--dissociate\");\n+\t\tif (info->depth >= 0)\n+\t\t\tstrvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n+\t\tif (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n+\t\t\tstrvec_clear(&clone_args);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tstrvec_clear(&clone_args);\n+\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.git_cmd = 1;\n+\t\tcp.dir = info->sm_path;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-f\", \"-q\", NULL);\n+\n+\t\tif (info->branch) {\n+\t\t\tstrvec_pushl(&cp.args, \"-B\", info->branch, NULL);\n+\t\t\tstrvec_pushf(&cp.args, \"origin/%s\", info->branch);\n+\t\t}\n+\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"unable to checkout submodule '%s'\"), info->sm_path);\n+\t}\n+\treturn 0;\n+}\n+\n+static int add_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tconst char *branch = NULL, *sm_path = NULL;\n+\tconst char *wt_prefix = NULL, *realrepo = NULL;\n+\tconst char *reference = NULL, *sm_name = NULL;\n+\tint force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0;\n+\tstruct add_data info = ADD_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING('b', \"branch\", &branch,\n+\t\t\t N_(\"branch\"),\n+\t\t\t N_(\"branch of repository to checkout on cloning\")),\n+\t\tOPT_STRING(0, \"prefix\", &wt_prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_STRING(0, \"path\", &sm_path,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"where the new submodule will be cloned to\")),\n+\t\tOPT_STRING(0, \"name\", &sm_name,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"name of the new submodule\")),\n+\t\tOPT_STRING(0, \"url\", &realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url where to clone the submodule from\")),\n+\t\tOPT_STRING(0, \"reference\", &reference,\n+\t\t\t N_(\"repo\"),\n+\t\t\t N_(\"reference repository\")),\n+\t\tOPT_BOOL(0, \"dissociate\", &dissociate,\n+\t\t\t N_(\"use --reference only while cloning\")),\n+\t\tOPT_INTEGER(0, \"depth\", &depth,\n+\t\t\t N_(\"depth for shallow clones\")),\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"force cloning progress\")),\n+\t\tOPT_BOOL('f', \"force\", &force,\n+\t\t\t N_(\"allow adding an otherwise ignored submodule path\")),\n+\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add-clone [--prefix=] [--quiet] [--force] \"\n+\t\t \"[--reference ] [--depth ] [-b|--branch ]\"\n+\t\t \"[--progress] [--dissociate] --url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tinfo.prefix = prefix;\n+\tinfo.sm_name = sm_name;\n+\tinfo.sm_path = sm_path;\n+\tinfo.realrepo = realrepo;\n+\tinfo.reference_path = reference;\n+\tinfo.branch = branch;\n+\tinfo.depth = depth;\n+\tinfo.progress = !!progress;\n+\tinfo.dissociate = !!dissociate;\n+\tinfo.force = !!force;\n+\tinfo.quiet = !!quiet;\n+\n+\tif (add_submodule(&info))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2757,6 +2968,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n+\t{\"add-clone\", add_clone, 0},\n \t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..f71e1e5495 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,43 +241,7 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n \tfi\n \n-\t# perhaps the path exists and is already a git repo, else clone it\n-\tif test -e \"$sm_path\"\n-\tthen\n-\t\tif test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\teval_gettextln \"Adding existing repo at '\\$sm_path' to the index\"\n-\t\telse\n-\t\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists and is not a valid git repo\")\"\n-\t\tfi\n-\n-\telse\n-\t\tif test -d \".git/modules/$sm_name\"\n-\t\tthen\n-\t\t\tif test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\teval_gettextln >&2 \"A git directory for '\\$sm_name' is found locally with remote(s):\"\n-\t\t\t\tGIT_DIR=\".git/modules/$sm_name\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\" \", -e s,' (fetch)',, >&2\n-\t\t\t\tdie \"$(eval_gettextln \"\\\n-If you want to reuse this local git directory instead of cloning again from\n- \\$realrepo\n-use the '--force' option. If the local git directory is not the correct repo\n-or you are unsure what this means choose another name with the '--name' option.\")\"\n-\t\t\telse\n-\t\t\t\teval_gettextln \"Reactivating local git directory for submodule '\\$sm_name'.\"\n-\t\t\tfi\n-\t\tfi\n-\t\tgit 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\n-\t\t(\n-\t\t\tsanitize_submodule_env\n-\t\t\tcd \"$sm_path\" &&\n-\t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n-\t\t\tcase \"$branch\" in\n-\t\t\t'') git checkout -f -q ;;\n-\t\t\t?*) git checkout -f -q -B \"$branch\" \"origin/$branch\" ;;\n-\t\t\tesac\n-\t\t) || die \"$(eval_gettext \"Unable to checkout submodule '\\$sm_path'\")\"\n-\tfi\n+\tgit 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\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-- \n2.31.1\n\n"},{"id":"426403","messageId":"CAP8UFD01-VJpUEGg3cEG7X=xU0KCv1AEgq2n_qhk=U+rXV5mvA@mail.gmail.com","threadId":"55791","inReplyTo":"20210602131259.50350-1-raykar.ath@gmail.com","subject":"Re: [PATCH v2] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2021-06-04T08:21:02Z","receivedAt":"2021-06-04T08:21:28Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Wed, Jun 2, 2021 at 3:13 PM Atharva Raykar wrote:\n\n> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n> +{\n> + struct child_process cp_remote = CHILD_PROCESS_INIT;\n> + struct strbuf sb_remote_out = STRBUF_INIT;\n> +\n> + cp_remote.git_cmd = 1;\n> + strvec_pushf(&cp_remote.env_array,\n> + \"GIT_DIR=%s\", git_dir_path);\n> + strvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n> + strvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n> + char *line;\n> + char *begin = sb_remote_out.buf;\n> + char *end = sb_remote_out.buf + sb_remote_out.len;\n> + while (begin != end && (line = get_next_line(begin, end))) {\n> + char *name, *url, *tail;\n> + name = parse_token(&begin, line);\n> + url = parse_token(&begin, line);\n> + tail = parse_token(&begin, line);\n\nSorry for not replying to your earlier message, but I think it's a bit\nbetter to save a line with:\n\n char *name = parse_token(&begin, line);\n char *url = parse_token(&begin, line);\n char *tail = parse_token(&begin, line);\n\n> + if (!memcmp(tail, \"(fetch)\", 7))\n> + fprintf(output, \" %s\\t%s\\n\", name, url);\n> + free(url);\n> + free(name);\n> + free(tail);\n> + }\n> + }\n> +\n> + strbuf_release(&sb_remote_out);\n> +}\n> +\n> +static int add_submodule(const struct add_data *info)\n> +{\n> + char *submod_gitdir_path;\n> + /* perhaps the path already exists and is already a git repo, else clone it */\n> + if (is_directory(info->sm_path)) {\n> + printf(\"sm_path=%s\\n\", info->sm_path);\n\nI don't see which shell code the above printf(...) instruction is\nreplacing. That's why I asked if it's some debugging leftover.\n\n[...]\n\n> + if (info->dissociate)\n> + strvec_push(&clone_args, \"--dissociate\");\n> + if (info->depth >= 0)\n> + strvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n\nIt's ok if there is a blank line here.\n\n> + if (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n> + strvec_clear(&clone_args);\n> + return -1;\n> + }\n> + strvec_clear(&clone_args);\n\n> +static int add_clone(int argc, const char **argv, const char *prefix)\n> +{\n> + const char *branch = NULL, *sm_path = NULL;\n> + const char *wt_prefix = NULL, *realrepo = NULL;\n> + const char *reference = NULL, *sm_name = NULL;\n> + int force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0;\n> + struct add_data info = ADD_DATA_INIT;\n\nMaybe: s/info/add_data/\n\nAlso it seems that in many cases it's a bit wasteful to use new\nvariables for option parsing and then to copy them into the add_data\nstruct when the field of the add_data struct could be used directly\nfor option parsing...\n\n> + struct option options[] = {\n> + OPT_STRING('b', \"branch\", &branch,\n\n...for example, here maybe `&add_data.branch` could be used instead of\n`&branch`...\n\n> + N_(\"branch\"),\n> + N_(\"branch of repository to checkout on cloning\")),\n\n[...]\n\n> + info.branch = branch;\n\n...so that the above line would not be needed.\n"},{"id":"426404","messageId":"11A04D37-41CB-43D7-B237-3BFA10B1313A@gmail.com","threadId":"55791","inReplyTo":"CAP8UFD01-VJpUEGg3cEG7X=xU0KCv1AEgq2n_qhk=U+rXV5mvA@mail.gmail.com","subject":"Re: [PATCH v2] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-04T09:47:48Z","receivedAt":"2021-06-04T09:48:05Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 04-Jun-2021, at 13:51, Christian Couder wrote:\n> \n> On Wed, Jun 2, 2021 at 3:13 PM Atharva Raykar wrote:\n> \n>> +static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n>> +{\n>> + struct child_process cp_remote = CHILD_PROCESS_INIT;\n>> + struct strbuf sb_remote_out = STRBUF_INIT;\n>> +\n>> + cp_remote.git_cmd = 1;\n>> + strvec_pushf(&cp_remote.env_array,\n>> + \"GIT_DIR=%s\", git_dir_path);\n>> + strvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n>> + strvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n>> + if (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n>> + char *line;\n>> + char *begin = sb_remote_out.buf;\n>> + char *end = sb_remote_out.buf + sb_remote_out.len;\n>> + while (begin != end && (line = get_next_line(begin, end))) {\n>> + char *name, *url, *tail;\n>> + name = parse_token(&begin, line);\n>> + url = parse_token(&begin, line);\n>> + tail = parse_token(&begin, line);\n> \n> Sorry for not replying to your earlier message, but I think it's a bit\n> better to save a line with:\n> \n> char *name = parse_token(&begin, line);\n> char *url = parse_token(&begin, line);\n> char *tail = parse_token(&begin, line);\n\nAlright.\n\n>> + if (!memcmp(tail, \"(fetch)\", 7))\n>> + fprintf(output, \" %s\\t%s\\n\", name, url);\n>> + free(url);\n>> + free(name);\n>> + free(tail);\n>> + }\n>> + }\n>> +\n>> + strbuf_release(&sb_remote_out);\n>> +}\n>> +\n>> +static int add_submodule(const struct add_data *info)\n>> +{\n>> + char *submod_gitdir_path;\n>> + /* perhaps the path already exists and is already a git repo, else clone it */\n>> + if (is_directory(info->sm_path)) {\n>> + printf(\"sm_path=%s\\n\", info->sm_path);\n> \n> I don't see which shell code the above printf(...) instruction is\n> replacing. That's why I asked if it's some debugging leftover.\n\nOh, my bad. It is a leftover debugging statement. Please excuse\nmy temporary blindness to it (:\n\n> [...]\n> \n>> + if (info->dissociate)\n>> + strvec_push(&clone_args, \"--dissociate\");\n>> + if (info->depth >= 0)\n>> + strvec_pushf(&clone_args, \"--depth=%d\", info->depth);\n> \n> It's ok if there is a blank line here.\n\nOK. Makes sense.\n\n>> + if (module_clone(clone_args.nr, clone_args.v, info->prefix)) {\n>> + strvec_clear(&clone_args);\n>> + return -1;\n>> + }\n>> + strvec_clear(&clone_args);\n> \n>> +static int add_clone(int argc, const char **argv, const char *prefix)\n>> +{\n>> + const char *branch = NULL, *sm_path = NULL;\n>> + const char *wt_prefix = NULL, *realrepo = NULL;\n>> + const char *reference = NULL, *sm_name = NULL;\n>> + int force = 0, quiet = 0, dissociate = 0, depth = -1, progress = 0;\n>> + struct add_data info = ADD_DATA_INIT;\n> \n> Maybe: s/info/add_data/\n\n'info' was the local convention for naming similar structures that\nheld the flag values (like summary_cb, module_cb, deinit_cb etc).\n\nThe exception to the above is 'struct submodule_update_clone', which\nwas named as 'suc'. It did not follow the *_cb naming convention,\npresumably because it was not used as a parameter passed to any\n*_cb() function.\n\nSince 'struct add_data' is more similar to the latter (as it is not\nused in any callback function) I guess it would be okay to name it\ndifferently and more descriptively as 'add_data'?\n\n> Also it seems that in many cases it's a bit wasteful to use new\n> variables for option parsing and then to copy them into the add_data\n> struct when the field of the add_data struct could be used directly\n> for option parsing...\n> \n>> + struct option options[] = {\n>> + OPT_STRING('b', \"branch\", &branch,\n> \n> ...for example, here maybe `&add_data.branch` could be used instead of\n> `&branch`...\n\nI thought of this too, but decided to stick to the surrounding\nconvention, where a new variable is used and then assigned to the\nstruct.\n\nI had a looked at the file again, and turns out...\n\n\tOPT_STRING_LIST(0, \"reference\", &suc.references, N_(\"repo\"),\n\t\t N_(\"reference repository\")),\n\tOPT_BOOL(0, \"dissociate\", &suc.dissociate,\n\t\t N_(\"use --reference only while cloning\")),\n\tOPT_STRING(0, \"depth\", &suc.depth, \"\",\n\t\t N_(\"create a shallow clone truncated to the \"\n\t\t \"specified number of revisions\")),\n\n... update_clone() is the exception again.\n\nSo there is precedent, and I'd rather follow what you suggested,\nbecause that looks much better to me, and saves a lot of redundant\ncode.\n\n>> + N_(\"branch\"),\n>> + N_(\"branch of repository to checkout on cloning\")),\n> \n> [...]\n> \n>> + info.branch = branch;\n> \n> ...so that the above line would not be needed.\n\nYes, although I might still need to use an extra variable for\nbooleans, like 'progress' or 'dissociate', because of the need\nto use !! to make it either 1 or 0. I am not too familiar with\nwhy doing that would be important in this context, but since\nthis is the convention, I'll keep it intact."},{"id":"426407","messageId":"20210604110524.84326-1-raykar.ath@gmail.com","threadId":"55791","inReplyTo":"11A04D37-41CB-43D7-B237-3BFA10B1313A@gmail.com","subject":"[PATCH v3] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-04T11:05:24Z","receivedAt":"2021-06-04T11:06:45Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Let's add a new \"add-clone\" subcommand to `git submodule--helper` with\nthe goal of converting part of the shell code in git-submodule.sh\nrelated to `git submodule add` into C code. This new subcommand clones\nthe repository that is to be added, and checks out to the appropriate\nbranch.\n\nThis is meant to be a faithful conversion that leaves the behaviour of\n'submodule add' unchanged. The only minor change is that if a submodule name has\nbeen supplied with a name that clashes with a local submodule, the message shown\nto the user (\"A git directory for 'foo' is found locally...\") is prepended with\n\"error\" for clarity.\n\nThis is part of a series of changes that will result in all of 'submodule add'\nbeing converted to C.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nChanges since v2:\n * Remove printf debug statement that was accidentally inserted into the final\n patch\n * Rename 'struct add_data info' to the more descriptive\n 'struct add_data add_data'\n * Remove unnecessary variables while parsing flags, and insert into the struct\n members directly\n * Eliminate extra heap allocation via 'xstrndup()' in parse_token()\n (I learnt this trick from Junio's comment on Shourya's v2 review of a similar\n patch :^) )\n\n builtin/submodule--helper.c | 199 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +------\n 2 files changed, 200 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex d55f6262e9..c9cb535312 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2745,6 +2745,204 @@ static int module_set_branch(int argc, const char **argv, const char *prefix)\n \treturn !!ret;\n }\n \n+struct add_data {\n+\tconst char *prefix;\n+\tconst char *branch;\n+\tconst char *reference_path;\n+\tconst char *sm_path;\n+\tconst char *sm_name;\n+\tconst char *repo;\n+\tconst char *realrepo;\n+\tint depth;\n+\tunsigned int force: 1;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+};\n+#define ADD_DATA_INIT { .depth = -1 }\n+\n+static char *parse_token(char **begin, const char *end, int *tok_len)\n+{\n+\tchar *tok_start, *pos = *begin;\n+\twhile (pos != end && (*pos != ' ' && *pos != '\\t' && *pos != '\\n'))\n+\t\tpos++;\n+\ttok_start = *begin;\n+\t*tok_len = pos - *begin;\n+\t*begin = pos + 1;\n+\treturn tok_start;\n+}\n+\n+static char *get_next_line(char *const begin, const char *const end)\n+{\n+\tchar *pos = begin;\n+\twhile (pos != end && *pos++ != '\\n');\n+\treturn pos;\n+}\n+\n+static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n+{\n+\tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n+\tstruct strbuf sb_remote_out = STRBUF_INIT;\n+\n+\tcp_remote.git_cmd = 1;\n+\tstrvec_pushf(&cp_remote.env_array,\n+\t\t \"GIT_DIR=%s\", git_dir_path);\n+\tstrvec_push(&cp_remote.env_array, \"GIT_WORK_TREE=.\");\n+\tstrvec_pushl(&cp_remote.args, \"remote\", \"-v\", NULL);\n+\tif (!capture_command(&cp_remote, &sb_remote_out, 0)) {\n+\t\tchar *line;\n+\t\tchar *begin = sb_remote_out.buf;\n+\t\tchar *end = sb_remote_out.buf + sb_remote_out.len;\n+\t\twhile (begin != end && (line = get_next_line(begin, end))) {\n+\t\t\tint namelen = 0, urllen = 0, taillen = 0;\n+\t\t\tchar *name = parse_token(&begin, line, &namelen);\n+\t\t\tchar *url = parse_token(&begin, line, &urllen);\n+\t\t\tchar *tail = parse_token(&begin, line, &taillen);\n+\t\t\tif (!memcmp(tail, \"(fetch)\", 7))\n+\t\t\t\tfprintf(output, \" %.*s\\t%.*s\\n\",\n+\t\t\t\t\tnamelen, name, urllen, url);\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&sb_remote_out);\n+}\n+\n+static int add_submodule(const struct add_data *add_data)\n+{\n+\tchar *submod_gitdir_path;\n+\t/* perhaps the path already exists and is already a git repo, else clone it */\n+\tif (is_directory(add_data->sm_path)) {\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", add_data->sm_path);\n+\t\tif (is_directory(submod_gitdir_path) || file_exists(submod_gitdir_path))\n+\t\t\tprintf(_(\"Adding existing path at '%s' to index\\n\"),\n+\t\t\t add_data->sm_path);\n+\t\telse\n+\t\t\tdie(_(\"'%s' already exists and is not a valid git repo\"),\n+\t\t\t add_data->sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\n+\t\tstruct strvec clone_args = STRVEC_INIT;\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tsubmod_gitdir_path = xstrfmt(\".git/modules/%s\", add_data->sm_name);\n+\n+\t\tif (is_directory(submod_gitdir_path)) {\n+\t\t\tif (!add_data->force) {\n+\t\t\t\terror(_(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\"locally with remote(s):\"), add_data->sm_name);\n+\t\t\t\tshow_fetch_remotes(stderr, add_data->sm_name,\n+\t\t\t\t\t\t submod_gitdir_path);\n+\t\t\t\tfprintf(stderr,\n+\t\t\t\t\t_(\"If you want to reuse this local git \"\n+\t\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t\t \" %s\\n\"\n+\t\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t\tadd_data->realrepo);\n+\t\t\t\tfree(submod_gitdir_path);\n+\t\t\t\treturn 1;\n+\t\t\t} else {\n+\t\t\t\tprintf(_(\"Reactivating local git directory for \"\n+\t\t\t\t\t \"submodule '%s'\\n\"), add_data->sm_name);\n+\t\t\t}\n+\t\t}\n+\t\tfree(submod_gitdir_path);\n+\n+\t\tstrvec_pushl(&clone_args, \"clone\", \"--path\", add_data->sm_path, \"--name\",\n+\t\t\t add_data->sm_name, \"--url\", add_data->realrepo, NULL);\n+\t\tif (add_data->quiet)\n+\t\t\tstrvec_push(&clone_args, \"--quiet\");\n+\t\tif (add_data->progress)\n+\t\t\tstrvec_push(&clone_args, \"--progress\");\n+\t\tif (add_data->prefix)\n+\t\t\tstrvec_pushl(&clone_args, \"--prefix\", add_data->prefix, NULL);\n+\t\tif (add_data->reference_path)\n+\t\t\tstrvec_pushl(&clone_args, \"--reference\",\n+\t\t\t\t add_data->reference_path, NULL);\n+\t\tif (add_data->dissociate)\n+\t\t\tstrvec_push(&clone_args, \"--dissociate\");\n+\t\tif (add_data->depth >= 0)\n+\t\t\tstrvec_pushf(&clone_args, \"--depth=%d\", add_data->depth);\n+\n+\t\tif (module_clone(clone_args.nr, clone_args.v, add_data->prefix)) {\n+\t\t\tstrvec_clear(&clone_args);\n+\t\t\treturn -1;\n+\t\t}\n+\t\tstrvec_clear(&clone_args);\n+\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.git_cmd = 1;\n+\t\tcp.dir = add_data->sm_path;\n+\t\tstrvec_pushl(&cp.args, \"checkout\", \"-f\", \"-q\", NULL);\n+\n+\t\tif (add_data->branch) {\n+\t\t\tstrvec_pushl(&cp.args, \"-B\", add_data->branch, NULL);\n+\t\t\tstrvec_pushf(&cp.args, \"origin/%s\", add_data->branch);\n+\t\t}\n+\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"unable to checkout submodule '%s'\"), add_data->sm_path);\n+\t}\n+\treturn 0;\n+}\n+\n+static int add_clone(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 0, quiet = 0, dissociate = 0, progress = 0;\n+\tstruct add_data add_data = ADD_DATA_INIT;\n+\n+\tstruct option options[] = {\n+\t\tOPT_STRING('b', \"branch\", &add_data.branch,\n+\t\t\t N_(\"branch\"),\n+\t\t\t N_(\"branch of repository to checkout on cloning\")),\n+\t\tOPT_STRING(0, \"prefix\", &add_data.prefix,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"alternative anchor for relative paths\")),\n+\t\tOPT_STRING(0, \"path\", &add_data.sm_path,\n+\t\t\t N_(\"path\"),\n+\t\t\t N_(\"where the new submodule will be cloned to\")),\n+\t\tOPT_STRING(0, \"name\", &add_data.sm_name,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"name of the new submodule\")),\n+\t\tOPT_STRING(0, \"url\", &add_data.realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url where to clone the submodule from\")),\n+\t\tOPT_STRING(0, \"reference\", &add_data.reference_path,\n+\t\t\t N_(\"repo\"),\n+\t\t\t N_(\"reference repository\")),\n+\t\tOPT_BOOL(0, \"dissociate\", &dissociate,\n+\t\t\t N_(\"use --reference only while cloning\")),\n+\t\tOPT_INTEGER(0, \"depth\", &add_data.depth,\n+\t\t\t N_(\"depth for shallow clones\")),\n+\t\tOPT_BOOL(0, \"progress\", &progress,\n+\t\t\t N_(\"force cloning progress\")),\n+\t\tOPT_BOOL('f', \"force\", &force,\n+\t\t\t N_(\"allow adding an otherwise ignored submodule path\")),\n+\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add-clone [--prefix=] [--quiet] [--force] \"\n+\t\t \"[--reference ] [--depth ] [-b|--branch ]\"\n+\t\t \"[--progress] [--dissociate] --url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tadd_data.progress = !!progress;\n+\tadd_data.dissociate = !!dissociate;\n+\tadd_data.force = !!force;\n+\tadd_data.quiet = !!quiet;\n+\n+\tif (add_submodule(&add_data))\n+\t\treturn 1;\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2757,6 +2955,7 @@ static struct cmd_struct commands[] = {\n \t{\"list\", module_list, 0},\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n+\t{\"add-clone\", add_clone, 0},\n \t{\"update-module-mode\", module_update_module_mode, 0},\n \t{\"update-clone\", update_clone, 0},\n \t{\"ensure-core-worktree\", ensure_core_worktree, 0},\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..f71e1e5495 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,43 +241,7 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n \tfi\n \n-\t# perhaps the path exists and is already a git repo, else clone it\n-\tif test -e \"$sm_path\"\n-\tthen\n-\t\tif test -d \"$sm_path\"/.git || test -f \"$sm_path\"/.git\n-\t\tthen\n-\t\t\teval_gettextln \"Adding existing repo at '\\$sm_path' to the index\"\n-\t\telse\n-\t\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists and is not a valid git repo\")\"\n-\t\tfi\n-\n-\telse\n-\t\tif test -d \".git/modules/$sm_name\"\n-\t\tthen\n-\t\t\tif test -z \"$force\"\n-\t\t\tthen\n-\t\t\t\teval_gettextln >&2 \"A git directory for '\\$sm_name' is found locally with remote(s):\"\n-\t\t\t\tGIT_DIR=\".git/modules/$sm_name\" GIT_WORK_TREE=. git remote -v | grep '(fetch)' | sed -e s,^,\" \", -e s,' (fetch)',, >&2\n-\t\t\t\tdie \"$(eval_gettextln \"\\\n-If you want to reuse this local git directory instead of cloning again from\n- \\$realrepo\n-use the '--force' option. If the local git directory is not the correct repo\n-or you are unsure what this means choose another name with the '--name' option.\")\"\n-\t\t\telse\n-\t\t\t\teval_gettextln \"Reactivating local git directory for submodule '\\$sm_name'.\"\n-\t\t\tfi\n-\t\tfi\n-\t\tgit 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\n-\t\t(\n-\t\t\tsanitize_submodule_env\n-\t\t\tcd \"$sm_path\" &&\n-\t\t\t# ash fails to wordsplit ${branch:+-b \"$branch\"...}\n-\t\t\tcase \"$branch\" in\n-\t\t\t'') git checkout -f -q ;;\n-\t\t\t?*) git checkout -f -q -B \"$branch\" \"origin/$branch\" ;;\n-\t\t\tesac\n-\t\t) || die \"$(eval_gettext \"Unable to checkout submodule '\\$sm_path'\")\"\n-\tfi\n+\tgit 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\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-- \n2.31.1\n\n"},{"id":"426408","messageId":"97DA3479-7E78-4EC8-BBD0-72869803E9D0@gmail.com","threadId":"55791","inReplyTo":"20210604110524.84326-1-raykar.ath@gmail.com","subject":"Re: [PATCH v3] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-04T11:16:06Z","receivedAt":"2021-06-04T11:17:24Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 04-Jun-2021, at 16:35, Atharva Raykar wrote:\n> \n> Let's add a new \"add-clone\" subcommand to `git submodule--helper` with\n> the goal of converting part of the shell code in git-submodule.sh\n> related to `git submodule add` into C code. This new subcommand clones\n> the repository that is to be added, and checks out to the appropriate\n> branch.\n> \n> This is meant to be a faithful conversion that leaves the behaviour of\n> 'submodule add' unchanged. The only minor change is that if a submodule name has\n> been supplied with a name that clashes with a local submodule, the message shown\n> to the user (\"A git directory for 'foo' is found locally...\") is prepended with\n> \"error\" for clarity.\n> \n> This is part of a series of changes that will result in all of 'submodule add'\n> being converted to C.\n> \n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n> ---\n> \n> Changes since v2:\n> * Remove printf debug statement that was accidentally inserted into the final\n> patch\n> * Rename 'struct add_data info' to the more descriptive\n> 'struct add_data add_data'\n> * Remove unnecessary variables while parsing flags, and insert into the struct\n> members directly\n> * Eliminate extra heap allocation via 'xstrndup()' in parse_token()\n> (I learnt this trick from Junio's comment on Shourya's v2 review of a similar\n> patch :^) )\n\nI forgot to mention, but this patch can be fetched via GitHub from:\n\nhttps://github.com/tfidfwastaken/git/tree/submodule-add-in-c-add-clone-v3"},{"id":"426409","messageId":"CAP6+3T1hN5mvWBe9-hziw=XGOugJ3ah=LVEDwOM5XY2uiZPkOQ@mail.gmail.com","threadId":"55791","inReplyTo":"20210602131259.50350-1-raykar.ath@gmail.com","subject":"Re: [PATCH v2] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Shourya Shukla","fromEmail":"shouryashukla.oo@gmail.com","sentAt":"2021-06-04T11:37:46Z","receivedAt":"2021-06-04T11:38:13Z","isPatch":true,"sender":{"key":"shouryashukla.oo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43680618?v=4"},"body":"Hi Atharva!\n\nOn Wed, Jun 2, 2021 at 6:43 PM Atharva Raykar wrote:\n>\n> Let's add a new \"add-clone\" subcommand to `git submodule--helper` with\n> the goal of converting part of the shell code in git-submodule.sh\n> related to `git submodule add` into C code. This new subcommand clones\n> the repository that is to be added, and checks out to the appropriate\n> branch.\n>\n> This is meant to be a faithful conversion that leaves the behaviour of\n> 'submodule add' unchanged. The only minor change is that if a submodule name has\n> been supplied with a name that clashes with a local submodule, the message shown\n> to the user (\"A git directory for 'foo' is found locally...\") is prepended with\n> \"error\" for clarity.\n\nIt would be better if commit messages are limited to 72 columns\n(characters) per line.\nThough you can obviously write longer lines on the list no problem.\n\n> This is part of a series of changes that will result in all of 'submodule add'\n> being converted to C.\n>\n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n\n\nI and others before me used to sign off the previous authors using\n'Signed-off-by:'. This trailer\nhas not been used yet so I am not sure if it should be used though I\nprefer this over the former.\nMaybe Christian could comment here?\n\n> This is part of a series of changes that will result in all of 'submodule add'\n> being converted to C, which is a more familiar language for Git developers, and\n> paves the way to improve performance and portability.\n>\n> I have made this patch based on Shourya's patch[1]. I have decided to send the\n> changes in smaller, more reviewable parts. The add-clone subcommand of\n> submodule--helper is an intermediate change, while I work on translating all of\n> the code.\n>\n> Another subcommand called 'add-config' will also be added in a separate patch\n> that handles the configuration on adding the module.\n>\n> After those two changes look good enough, I will be converting whatever is left\n> of 'git submodule add' in the git-submodule.sh past the flag parsing into C code\n> by having one helper subcommand called 'git submodule--helper add' that will\n> incorporate the functionality of the other two helpers, as well. In that patch,\n> the 'add-clone' and 'add-config' subcommands will be removed from the commands\n> array, as they will be called from within the C code itself.\n\nSeems like a good approach! BTW, if this \"extra\" message is a bit long\nlike the one above, then\nyou can put it in a cover letter instead. If people really want to\nread this extra information\nthey will read it in a cover letter as well.\n\nJust supply the '--cover-letter' option when executing the 'git\nformat-patch' command.\n\n> Changes since v1:\n> * Fixed typos, and made commit message more explicit\n> * Fixed incorrect usage string\n> * Some style changes were made\n\nTo save yourself the trouble of sieving the \"top\" or \"noteworthy\" changes from\nthe new version, you could instead just print the 'range-diff' between\nthe two versions.\n\nYou can do:\n'git range-diff b1~n1..b1 b2~n2..b2'\n\nWhere:\n\n- 'b1' is the first branch; 'n1' is the number of top commits you are\ntaking from 'b1' for\n comparison.\n\n- 'b2' is the second branch; 'n2' is the number of top commits you are\ntaking from 'b2' for\n comparison.\n\nIt will print a very detailed output showing what differences were\nthere commit-wise\namongst the two branches. This can be put at the end of the cover\nletter. Though, this\nisn't necessary if your way seems better to you.\n\nBTW, it would be helpful if you could send mails addressed to me on my\nother email .\n"},{"id":"426410","messageId":"F10D45CC-033E-45F5-B1A6-CA757D3EB6F2@gmail.com","threadId":"55791","inReplyTo":"CAP6+3T1hN5mvWBe9-hziw=XGOugJ3ah=LVEDwOM5XY2uiZPkOQ@mail.gmail.com","subject":"Re: [PATCH v2] [GSoC] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-06-04T12:02:25Z","receivedAt":"2021-06-04T12:02:43Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 04-Jun-2021, at 17:07, Shourya Shukla wrote:\n> \n> Hi Atharva!\n> \n> On Wed, Jun 2, 2021 at 6:43 PM Atharva Raykar wrote:\n>> \n>> Let's add a new \"add-clone\" subcommand to `git submodule--helper` with\n>> the goal of converting part of the shell code in git-submodule.sh\n>> related to `git submodule add` into C code. This new subcommand clones\n>> the repository that is to be added, and checks out to the appropriate\n>> branch.\n>> \n>> This is meant to be a faithful conversion that leaves the behaviour of\n>> 'submodule add' unchanged. The only minor change is that if a submodule name has\n>> been supplied with a name that clashes with a local submodule, the message shown\n>> to the user (\"A git directory for 'foo' is found locally...\") is prepended with\n>> \"error\" for clarity.\n> \n> It would be better if commit messages are limited to 72 columns\n> (characters) per line.\n> Though you can obviously write longer lines on the list no problem.\n\nGood catch. My auto-fill settings had got switched to length 80.\nI'll be careful next time.\n\n>> This is part of a series of changes that will result in all of 'submodule add'\n>> being converted to C.\n>> \n>> Signed-off-by: Atharva Raykar \n>> Mentored-by: Christian Couder \n>> Mentored-by: Shourya Shukla \n>> Based-on-patch-by: Shourya Shukla \n>> Based-on-patch-by: Prathamesh Chavan \n> \n> \n> I and others before me used to sign off the previous authors using\n> 'Signed-off-by:'. This trailer\n> has not been used yet so I am not sure if it should be used though I\n> prefer this over the former.\n> Maybe Christian could comment here?\n\nYeah, I wasn't sure if I should include an S.o.B without explicit\nacknowledgement from you and Prathamesh.\n\n>> This is part of a series of changes that will result in all of 'submodule add'\n>> being converted to C, which is a more familiar language for Git developers, and\n>> paves the way to improve performance and portability.\n>> \n>> I have made this patch based on Shourya's patch[1]. I have decided to send the\n>> changes in smaller, more reviewable parts. The add-clone subcommand of\n>> submodule--helper is an intermediate change, while I work on translating all of\n>> the code.\n>> \n>> Another subcommand called 'add-config' will also be added in a separate patch\n>> that handles the configuration on adding the module.\n>> \n>> After those two changes look good enough, I will be converting whatever is left\n>> of 'git submodule add' in the git-submodule.sh past the flag parsing into C code\n>> by having one helper subcommand called 'git submodule--helper add' that will\n>> incorporate the functionality of the other two helpers, as well. In that patch,\n>> the 'add-clone' and 'add-config' subcommands will be removed from the commands\n>> array, as they will be called from within the C code itself.\n> \n> Seems like a good approach! BTW, if this \"extra\" message is a bit long\n> like the one above, then\n> you can put it in a cover letter instead. If people really want to\n> read this extra information\n> they will read it in a cover letter as well.\n> \n> Just supply the '--cover-letter' option when executing the 'git\n> format-patch' command.\n\nNot too familiar with the convention here on how long a description\nwarrants a cover letter. Generally in the mailing list I found\n[PATCH 0/1] labels far more uncommon than [PATCH] for single patch\nchanges, so I went with the common case.\n\n>> Changes since v1:\n>> * Fixed typos, and made commit message more explicit\n>> * Fixed incorrect usage string\n>> * Some style changes were made\n> \n> To save yourself the trouble of sieving the \"top\" or \"noteworthy\" changes from\n> the new version, you could instead just print the 'range-diff' between\n> the two versions.\n> \n> You can do:\n> 'git range-diff b1~n1..b1 b2~n2..b2'\n> \n> Where:\n> \n> - 'b1' is the first branch; 'n1' is the number of top commits you are\n> taking from 'b1' for\n> comparison.\n> \n> - 'b2' is the second branch; 'n2' is the number of top commits you are\n> taking from 'b2' for\n> comparison.\n> \n> It will print a very detailed output showing what differences were\n> there commit-wise\n> amongst the two branches. This can be put at the end of the cover\n> letter. Though, this\n> isn't necessary if your way seems better to you.\n\nThanks for the tip. I felt since this change was mostly about code\nstyle and naming, a range diff for it felt a little extra.\n\nI liked it more when you used it in a previous v2 of your patch,\nwhere the changes were more significant.\n\n> BTW, it would be helpful if you could send mails addressed to me on my\n> other email .\n\nGot it.\n\n"}]}