{"thread":{"id":"56057","subject":"[GSoC] [PATCH 0/3] submodule add: partial conversion to C","startedAt":"2021-07-06T18:20:08Z","lastAt":"2021-10-24T06:05:40Z","messageCount":34,"participants":["Atharva Raykar","Junio C Hamano","Đoàn Trần Công Danh","Kaartic Sivaraam","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"429305","messageId":"20210706181936.34087-1-raykar.ath@gmail.com","threadId":"56057","inReplyTo":null,"subject":"[GSoC] [PATCH 0/3] submodule add: partial conversion to C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-06T18:19:33Z","receivedAt":"2021-07-06T18:20:08Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"This is based on a previous series I sent[1] which got a bunch of reviews at the\ntime from Christian, Danh, Junio, Rafael and Eric.\n\nThis series only includes changes pertaining to the 'add-clone' subcommand, and\nthe patches have remained the same as before, except for the inclusion of a\ncustom die routine 'submodule_die()' that makes all the die()'s behave identical\nto the shell version of 'die'. I am particularly interested in opinions about\nthis change.\n\nA test has been added to ensure no regressions are introduced during the\nconversion.\n\nI am holding on to a full conversion of submodule add[2], and sending only a\nportion of it, so that the parts that already have been seen before on the list\nand refined can be merged. I then plan to send the rest of the patches that\nbuild on top of this series.\n\nYou can fetch this series from: https://github.com/tfidfwastaken/git.git\n\n[1]: https://lore.kernel.org/git/20210615145745.33382-1-raykar.ath@gmail.com/\n[2]: https://github.com/tfidfwastaken/git/commits/submodule-helper-add-6\n\nAtharva Raykar (3):\n t7400: test failure to add submodule in tracked path\n submodule--helper: refactor module_clone()\n submodule--helper: introduce add-clone subcommand\n\n builtin/submodule--helper.c | 428 ++++++++++++++++++++++++++----------\n git-submodule.sh | 38 +---\n t/t7400-submodule-basic.sh | 11 +\n 3 files changed, 327 insertions(+), 150 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"429306","messageId":"20210706181936.34087-2-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210706181936.34087-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH 1/3] t7400: test failure to add submodule in tracked path","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-06T18:19:34Z","receivedAt":"2021-07-06T18:20:15Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a test to ensure failure on adding a submodule to a directory with\ntracked contents in the index.\n\nAs we are going to refactor and port to C some parts of `git submodule\nadd`, let's add a test to help ensure no regression is introduced.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nBased-on-patch-by: Shourya Shukla \nMentored-by: Shourya Shukla \n---\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..7aa7fefdfa 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n \t)\n '\n \n+test_expect_success 'submodule add to path with tracked content fails' '\n+\t(\n+\t\tcd addtest &&\n+\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n+\t\tmkdir dir-tracked &&\n+\t\ttest_commit foo dir-tracked/bar &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'submodule add to reconfigure existing submodule with --force' '\n \t(\n \t\tcd addtest-ignore &&\n-- \n2.32.0\n\n"},{"id":"429307","messageId":"20210706181936.34087-3-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210706181936.34087-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH 2/3] submodule--helper: refactor module_clone()","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-06T18:19:35Z","receivedAt":"2021-07-06T18:20:17Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Separate out the core logic of module_clone() from the flag\nparsing---this way we can call the equivalent of the `submodule--helper\nclone` subcommand directly within C, without needing to push arguments\nin a strvec.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nSuggested-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------\n 1 file changed, 128 insertions(+), 113 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ae6174ab05..320f4252fe 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static int clone_submodule(const char *path, const char *gitdir, const char *url,\n-\t\t\t const char *depth, struct string_list *reference, int dissociate,\n-\t\t\t int quiet, int progress, int single_branch)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\tstrvec_push(&cp.args, \"clone\");\n-\tstrvec_push(&cp.args, \"--no-checkout\");\n-\tif (quiet)\n-\t\tstrvec_push(&cp.args, \"--quiet\");\n-\tif (progress)\n-\t\tstrvec_push(&cp.args, \"--progress\");\n-\tif (depth && *depth)\n-\t\tstrvec_pushl(&cp.args, \"--depth\", depth, NULL);\n-\tif (reference->nr) {\n-\t\tstruct string_list_item *item;\n-\t\tfor_each_string_list_item(item, reference)\n-\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n-\t\t\t\t item->string, NULL);\n-\t}\n-\tif (dissociate)\n-\t\tstrvec_push(&cp.args, \"--dissociate\");\n-\tif (gitdir && *gitdir)\n-\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n-\tif (single_branch >= 0)\n-\t\tstrvec_push(&cp.args, single_branch ?\n-\t\t\t\t\t \"--single-branch\" :\n-\t\t\t\t\t \"--no-single-branch\");\n-\n-\tstrvec_push(&cp.args, \"--\");\n-\tstrvec_push(&cp.args, url);\n-\tstrvec_push(&cp.args, path);\n-\n-\tcp.git_cmd = 1;\n-\tprepare_submodule_repo_env(&cp.env_array);\n-\tcp.no_stdin = 1;\n-\n-\treturn run_command(&cp);\n-}\n+struct module_clone_data {\n+\tconst char *prefix;\n+\tconst char *path;\n+\tconst char *name;\n+\tconst char *url;\n+\tconst char *depth;\n+\tstruct string_list reference;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+\tunsigned int require_init: 1;\n+\tint single_branch;\n+};\n+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }\n \n struct submodule_alternate_setup {\n \tconst char *submodule_name;\n@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,\n \tfree(error_strategy);\n }\n \n+static int clone_submodule(struct module_clone_data *clone_data)\n+{\n+\tchar *p, *sm_gitdir;\n+\tchar *sm_alternate = NULL, *error_strategy = NULL;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), clone_data->name);\n+\tsm_gitdir = absolute_pathdup(sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\tif (!is_absolute_path(clone_data->path)) {\n+\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), clone_data->path);\n+\t\tclone_data->path = strbuf_detach(&sb, NULL);\n+\t} else {\n+\t\tclone_data->path = xstrdup(clone_data->path);\n+\t}\n+\n+\tif (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)\n+\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n+\t\t \"git dir\"), sm_gitdir);\n+\n+\tif (!file_exists(sm_gitdir)) {\n+\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n+\n+\t\tprepare_possible_alternates(clone_data->name, &clone_data->reference);\n+\n+\t\tstrvec_push(&cp.args, \"clone\");\n+\t\tstrvec_push(&cp.args, \"--no-checkout\");\n+\t\tif (clone_data->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tif (clone_data->progress)\n+\t\t\tstrvec_push(&cp.args, \"--progress\");\n+\t\tif (clone_data->depth && *(clone_data->depth))\n+\t\t\tstrvec_pushl(&cp.args, \"--depth\", clone_data->depth, NULL);\n+\t\tif (clone_data->reference.nr) {\n+\t\t\tstruct string_list_item *item;\n+\t\t\tfor_each_string_list_item(item, &clone_data->reference)\n+\t\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n+\t\t\t\t\t item->string, NULL);\n+\t\t}\n+\t\tif (clone_data->dissociate)\n+\t\t\tstrvec_push(&cp.args, \"--dissociate\");\n+\t\tif (sm_gitdir && *sm_gitdir)\n+\t\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", sm_gitdir, NULL);\n+\t\tif (clone_data->single_branch >= 0)\n+\t\t\tstrvec_push(&cp.args, clone_data->single_branch ?\n+\t\t\t\t \"--single-branch\" :\n+\t\t\t\t \"--no-single-branch\");\n+\n+\t\tstrvec_push(&cp.args, \"--\");\n+\t\tstrvec_push(&cp.args, clone_data->url);\n+\t\tstrvec_push(&cp.args, clone_data->path);\n+\n+\t\tcp.git_cmd = 1;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.no_stdin = 1;\n+\n+\t\tif(run_command(&cp))\n+\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n+\t\t\t clone_data->url, clone_data->path);\n+\t} else {\n+\t\tif (clone_data->require_init && !access(clone_data->path, X_OK) &&\n+\t\t !is_empty_dir(clone_data->path))\n+\t\t\tdie(_(\"directory not empty: '%s'\"), clone_data->path);\n+\t\tif (safe_create_leading_directories_const(clone_data->path) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), clone_data->path);\n+\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n+\t\tunlink_or_warn(sb.buf);\n+\t\tstrbuf_reset(&sb);\n+\t}\n+\n+\tconnect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);\n+\n+\tp = git_pathdup_submodule(clone_data->path, \"config\");\n+\tif (!p)\n+\t\tdie(_(\"could not get submodule directory for '%s'\"), clone_data->path);\n+\n+\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n+\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\tif (sm_alternate)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n+\t\t\t\t sm_alternate);\n+\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\tif (error_strategy)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n+\t\t\t\t error_strategy);\n+\n+\tfree(sm_alternate);\n+\tfree(error_strategy);\n+\n+\tstrbuf_release(&sb);\n+\tfree(sm_gitdir);\n+\tfree(p);\n+\treturn 0;\n+}\n+\n static int module_clone(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *name = NULL, *url = NULL, *depth = NULL;\n-\tint quiet = 0;\n-\tint progress = 0;\n-\tchar *p, *path = NULL, *sm_gitdir;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct string_list reference = STRING_LIST_INIT_NODUP;\n-\tint dissociate = 0, require_init = 0;\n-\tchar *sm_alternate = NULL, *error_strategy = NULL;\n-\tint single_branch = -1;\n+\tint dissociate = 0, quiet = 0, progress = 0, require_init = 0;\n+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n \n \tstruct option module_clone_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\tOPT_STRING(0, \"prefix\", &clone_data.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"alternative anchor for relative paths\")),\n-\t\tOPT_STRING(0, \"path\", &path,\n+\t\tOPT_STRING(0, \"path\", &clone_data.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\", &name,\n+\t\tOPT_STRING(0, \"name\", &clone_data.name,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"name of the new submodule\")),\n-\t\tOPT_STRING(0, \"url\", &url,\n+\t\tOPT_STRING(0, \"url\", &clone_data.url,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"url where to clone the submodule from\")),\n-\t\tOPT_STRING_LIST(0, \"reference\", &reference,\n+\t\tOPT_STRING_LIST(0, \"reference\", &clone_data.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_STRING(0, \"depth\", &depth,\n+\t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n \t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\n \t\t\t N_(\"disallow cloning into non-empty directory\")),\n-\t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n+\t\tOPT_BOOL(0, \"single-branch\", &clone_data.single_branch,\n \t\t\t N_(\"clone only one branch, HEAD or --branch\")),\n \t\tOPT_END()\n \t};\n@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n-\tif (argc || !url || !path || !*path)\n+\tclone_data.dissociate = !!dissociate;\n+\tclone_data.quiet = !!quiet;\n+\tclone_data.progress = !!progress;\n+\tclone_data.require_init = !!require_init;\n+\n+\tif (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n-\tsm_gitdir = absolute_pathdup(sb.buf);\n-\tstrbuf_reset(&sb);\n-\n-\tif (!is_absolute_path(path)) {\n-\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), path);\n-\t\tpath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tpath = xstrdup(path);\n-\n-\tif (validate_submodule_git_dir(sm_gitdir, name) < 0)\n-\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n-\t\t\t\"git dir\"), sm_gitdir);\n-\n-\tif (!file_exists(sm_gitdir)) {\n-\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n-\n-\t\tprepare_possible_alternates(name, &reference);\n-\n-\t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,\n-\t\t\t\t quiet, progress, single_branch))\n-\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n-\t\t\t url, path);\n-\t} else {\n-\t\tif (require_init && !access(path, X_OK) && !is_empty_dir(path))\n-\t\t\tdie(_(\"directory not empty: '%s'\"), path);\n-\t\tif (safe_create_leading_directories_const(path) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), path);\n-\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n-\t\tunlink_or_warn(sb.buf);\n-\t\tstrbuf_reset(&sb);\n-\t}\n-\n-\tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n-\n-\tp = git_pathdup_submodule(path, \"config\");\n-\tif (!p)\n-\t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n-\n-\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n-\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n-\tif (sm_alternate)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n-\t\t\t\t\t sm_alternate);\n-\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n-\tif (error_strategy)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n-\t\t\t\t\t error_strategy);\n-\n-\tfree(sm_alternate);\n-\tfree(error_strategy);\n-\n-\tstrbuf_release(&sb);\n-\tfree(sm_gitdir);\n-\tfree(path);\n-\tfree(p);\n+\tclone_submodule(&clone_data);\n \treturn 0;\n }\n \n-- \n2.32.0\n\n"},{"id":"429308","messageId":"20210706181936.34087-4-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210706181936.34087-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH 3/3] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-06T18:19:36Z","receivedAt":"2021-07-06T18:20:21Z","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.\n\nThe 'die' that is used in git-submodule.sh is not the same as the\n'die()' in C--the latter prefixes with 'fatal:' and exits with an error\ncode of 128, while the shell die exits with code 1.\n\nIntroduce a custom die routine, that can be used by converted\nsubcommands to emulate the shell 'die'.\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 \nHelped-by: Đoàn Trần Công Danh \n---\n builtin/submodule--helper.c | 187 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +-------\n 2 files changed, 188 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 320f4252fe..1f2673139f 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -30,6 +30,14 @@\n typedef void (*each_submodule_fn)(const struct cache_entry *list_item,\n \t\t\t\t void *cb_data);\n \n+static NORETURN void submodule_die(const char *err, va_list params)\n+{\n+\tvfprintf(stderr, err, params);\n+\tfputc('\\n', stderr);\n+\tfflush(stderr);\n+\texit(1);\n+}\n+\n static char *get_default_remote(void)\n {\n \tchar *dest = NULL, *ret;\n@@ -2760,6 +2768,184 @@ 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 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;\n+\t\tchar *line = sb_remote_out.buf;\n+\t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n+\t\t\tsize_t len = next_line - line;\n+\t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n+\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n+\t\t\tline = next_line + 1;\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+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n+\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\tstruct strbuf sm_path = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sm_path, add_data->sm_path);\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", add_data->sm_path);\n+\t\tif (is_nonbare_repository_dir(&sm_path))\n+\t\t\tprintf(_(\"Adding existing repo at '%s' to the 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\tstrbuf_release(&sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\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\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\t \"locally with remote(s):\"),\n+\t\t\t\t\tadd_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\tfree(submod_gitdir_path);\n+\t\t\t\tdie(_(\"If you want to reuse this local git \"\n+\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t \" %s\\n\"\n+\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t add_data->realrepo);\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\tclone_data.prefix = add_data->prefix;\n+\t\tclone_data.path = add_data->sm_path;\n+\t\tclone_data.name = add_data->sm_name;\n+\t\tclone_data.url = add_data->realrepo;\n+\t\tclone_data.quiet = add_data->quiet;\n+\t\tclone_data.progress = add_data->progress;\n+\t\tif (add_data->reference_path)\n+\t\t\tstring_list_append(&clone_data.reference,\n+\t\t\t\t\t xstrdup(add_data->reference_path));\n+\t\tclone_data.dissociate = add_data->dissociate;\n+\t\tif (add_data->depth >= 0)\n+\t\t\tclone_data.depth = xstrfmt(\"%d\", add_data->depth);\n+\n+\t\tif (clone_submodule(&clone_data))\n+\t\t\treturn -1;\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\", &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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\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 [...] \"\n+\t\t \"--url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 0)\n+\t\tusage_with_options(usage, options);\n+\n+\tadd_data.prefix = prefix;\n+\tadd_data.progress = !!progress;\n+\tadd_data.dissociate = !!dissociate;\n+\tadd_data.force = !!force;\n+\tadd_data.quiet = !!quiet;\n+\n+\tset_die_routine(submodule_die);\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@@ -2772,6 +2958,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.32.0\n\n"},{"id":"429443","messageId":"xmqqr1g9ew2f.fsf@gitster.g","threadId":"56057","inReplyTo":"20210706181936.34087-4-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH 3/3] submodule--helper: introduce add-clone subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-07T19:57:28Z","receivedAt":"2021-07-07T19:57:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\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.\n\nMakes sense.\n\n> The 'die' that is used in git-submodule.sh is not the same as the\n> 'die()' in C--the latter prefixes with 'fatal:' and exits with an error\n> code of 128, while the shell die exits with code 1.\n>\n> Introduce a custom die routine, that can be used by converted\n> subcommands to emulate the shell 'die'.\n\nI suspect that installing this with set_die_routine() might be going\ntoo far. If some of the lower-level helper routines we call from\nhere have to die (e.g. our call results in xmalloc() getting called\nand we run out of memory), die() called there will also end up\ncalling our submodule_die(), not just new calls to die() you are\nadding in this patch. Calling submodule_die() directly from the\ncode you convert from the scripted version where we used to call die\nof the scripted version would be fine, though.\n\nI suspect that it would be OK to use the standard die() instead,\nwith the minimum adjustment as needed, namely, we may have to\n\n * Adjust the messages the scripted version of the caller gave to\n the scripted version of die, if needed (e.g. if the scripted\n version added \"fatal:\" prefix itself to compensate for the lack\n of it in the scripted \"die\", we can drop the prefix and call the\n standard die());\n\n * Adjust the tests if they care about the differences between\n exiting 128 and 1.\n\n> +static NORETURN void submodule_die(const char *err, va_list params)\n> +{\n> +\tvfprintf(stderr, err, params);\n> +\tfputc('\\n', stderr);\n> +\tfflush(stderr);\n> +\texit(1);\n> +}\n\n\nOther than that, all three patches looked quite reasonable.\n\nThanks.\n"},{"id":"429473","messageId":"AED3D118-7613-45BE-824A-1350872A489D@gmail.com","threadId":"56057","inReplyTo":"xmqqr1g9ew2f.fsf@gitster.g","subject":"Re: [GSoC] [PATCH 3/3] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T06:45:25Z","receivedAt":"2021-07-08T06:45:33Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 08-Jul-2021, at 01:27, Junio C Hamano wrote:\n> \n> Atharva Raykar writes:\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.\n> \n> Makes sense.\n> \n>> The 'die' that is used in git-submodule.sh is not the same as the\n>> 'die()' in C--the latter prefixes with 'fatal:' and exits with an error\n>> code of 128, while the shell die exits with code 1.\n>> \n>> Introduce a custom die routine, that can be used by converted\n>> subcommands to emulate the shell 'die'.\n> \n> I suspect that installing this with set_die_routine() might be going\n> too far. If some of the lower-level helper routines we call from\n> here have to die (e.g. our call results in xmalloc() getting called\n> and we run out of memory), die() called there will also end up\n> calling our submodule_die(), not just new calls to die() you are\n> adding in this patch. Calling submodule_die() directly from the\n> code you convert from the scripted version where we used to call die\n> of the scripted version would be fine, though.\n> \n> I suspect that it would be OK to use the standard die() instead,\n> with the minimum adjustment as needed, namely, we may have to\n> \n> * Adjust the messages the scripted version of the caller gave to\n> the scripted version of die, if needed (e.g. if the scripted\n> version added \"fatal:\" prefix itself to compensate for the lack\n> of it in the scripted \"die\", we can drop the prefix and call the\n> standard die());\n> \n> * Adjust the tests if they care about the differences between\n> exiting 128 and 1.\n\nOkay, will do. The latter will not affect the tests, but the inclusion\nof a 'fatal:' prefix will require me to adjust one test that checks for\nthe error message.\n\n"},{"id":"429476","messageId":"20210708095533.26226-1-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210706181936.34087-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2 0/4] submodule add: partial conversion to C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T09:55:29Z","receivedAt":"2021-07-08T09:55:50Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Changes since v1:\n * A custom die routine is no longer used. Instead, the shell script is changed\n to prefix all die messages with 'fatal' to match what the standard 'die()'\n function in C will output when converted.\n\nFetch-It-Via:\ngit fetch https://github.com/tfidfwastaken/git.git submodule-helper-add-clone-2\n\nAtharva Raykar (4):\n t7400: test failure to add submodule in tracked path\n submodule: prefix die messages with 'fatal'\n submodule--helper: refactor module_clone()\n submodule--helper: introduce add-clone subcommand\n\n builtin/submodule--helper.c | 418 ++++++++++++++++++++++++++----------\n git-submodule.sh | 76 ++-----\n t/t7400-submodule-basic.sh | 13 +-\n t/t7406-submodule-update.sh | 10 +-\n 4 files changed, 342 insertions(+), 175 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"429477","messageId":"20210708095533.26226-2-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210708095533.26226-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2 1/4] t7400: test failure to add submodule in tracked path","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T09:55:30Z","receivedAt":"2021-07-08T09:55:52Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a test to ensure failure on adding a submodule to a directory with\ntracked contents in the index.\n\nAs we are going to refactor and port to C some parts of `git submodule\nadd`, let's add a test to help ensure no regression is introduced.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nBased-on-patch-by: Shourya Shukla \nMentored-by: Shourya Shukla \n---\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..7aa7fefdfa 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n \t)\n '\n \n+test_expect_success 'submodule add to path with tracked content fails' '\n+\t(\n+\t\tcd addtest &&\n+\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n+\t\tmkdir dir-tracked &&\n+\t\ttest_commit foo dir-tracked/bar &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'submodule add to reconfigure existing submodule with --force' '\n \t(\n \t\tcd addtest-ignore &&\n-- \n2.32.0\n\n"},{"id":"429478","messageId":"20210708095533.26226-3-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210708095533.26226-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2 2/4] submodule: prefix die messages with 'fatal'","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T09:55:31Z","receivedAt":"2021-07-08T09:55:58Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"The standard `die()` function that is used in C code prefixes all the\nmessages passed to it with 'fatal: '. This does not happen with the\n`die` used in 'git-submodule.sh'.\n\nLet's prefix each of the shell die messages with 'fatal: ' so that when\nthey are converted to C code, the error messages stay the same as before\nthe conversion.\n\nNote that the shell version of `die` exits with error code 1, while the\nC version exits with error code 128. In practice, this does not change\nany behaviour, as no functionality in 'submodule add' and 'submodule\nupdate' relies on the value of the exit code.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \n---\n git-submodule.sh | 38 ++++++++++++++++++-------------------\n t/t7400-submodule-basic.sh | 4 ++--\n t/t7406-submodule-update.sh | 10 +++++-----\n 3 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..b887daa8a1 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -147,7 +147,7 @@ cmd_add()\n \n \tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n \tthen\n-\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n+\t\t die \"$(eval_gettext \"fatal: please make sure that the .gitmodules file is in the working tree\")\"\n \tfi\n \n \tif test -n \"$reference_path\"\n@@ -176,7 +176,7 @@ cmd_add()\n \tcase \"$repo\" in\n \t./*|../*)\n \t\ttest -z \"$wt_prefix\" ||\n-\t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n+\t\tdie \"$(gettext \"fatal: Relative path can only be used from the toplevel of the working tree\")\"\n \n \t\t# dereference source url relative to parent's url\n \t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n@@ -186,7 +186,7 @@ cmd_add()\n \t\trealrepo=$repo\n \t\t;;\n \t*)\n-\t\tdie \"$(eval_gettext \"repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n+\t\tdie \"$(eval_gettext \"fatal: repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n \t;;\n \tesac\n \n@@ -205,17 +205,17 @@ cmd_add()\n \tif test -z \"$force\"\n \tthen\n \t\tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n+\t\tdie \"$(eval_gettext \"fatal: '\\$sm_path' already exists in the index\")\"\n \telse\n \t\tgit ls-files -s \"$sm_path\" | sane_grep -v \"^160000\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n+\t\tdie \"$(eval_gettext \"fatal: '\\$sm_path' already exists in the index and is not a submodule\")\"\n \tfi\n \n \tif test -d \"$sm_path\" &&\n \t\ttest -z $(git -C \"$sm_path\" rev-parse --show-cdup 2>/dev/null)\n \tthen\n \t git -C \"$sm_path\" rev-parse --verify -q HEAD >/dev/null ||\n-\t die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n+\t die \"$(eval_gettext \"fatal: '\\$sm_path' does not have a commit checked out\")\"\n \tfi\n \n \tif test -z \"$force\"\n@@ -238,7 +238,7 @@ cmd_add()\n \n \tif ! git submodule--helper check-name \"$sm_name\"\n \tthen\n-\t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n+\t\tdie \"$(eval_gettext \"fatal: '$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@@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-\tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n+\tdie \"$(eval_gettext \"fatal: Failed to add submodule '\\$sm_path'\")\"\n \n \tgit submodule--helper config submodule.\"$sm_name\".path \"$sm_path\" &&\n \tgit submodule--helper config submodule.\"$sm_name\".url \"$repo\" &&\n@@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n \tfi &&\n \tgit add --force .gitmodules ||\n-\tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n+\tdie \"$(eval_gettext \"fatal: Failed to register submodule '\\$sm_path'\")\"\n \n \t# NEEDSWORK: In a multi-working-tree world, this needs to be\n \t# set in the per-worktree config.\n@@ -565,7 +565,7 @@ cmd_update()\n \t\telse\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\t\tdie \"$(eval_gettext \"fatal: Unable to find current revision in submodule path '\\$displaypath'\")\"\n \t\tfi\n \n \t\tif test -n \"$remote\"\n@@ -575,12 +575,12 @@ cmd_update()\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n+\t\t\t\tdie \"$(eval_gettext \"fatal: Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\t\tfi\n \t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && git submodule--helper print-default-remote)\n \t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n+\t\t\tdie \"$(eval_gettext \"fatal: Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n \t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n@@ -604,36 +604,36 @@ cmd_update()\n \t\t\t\t# not be reachable from any of the refs\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n \t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n+\t\t\t\tdie \"$(eval_gettext \"fatal: Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n \t\t\tfi\n \n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\n \t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n \t\t\t\t;;\n \t\t\trebase)\n \t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\tmerge)\n \t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t!*)\n \t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n+\t\t\t\tdie \"$(eval_gettext \"fatal: Invalid update mode '$update_module' for submodule path '$path'\")\"\n \t\t\tesac\n \n \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n@@ -660,7 +660,7 @@ cmd_update()\n \t\t\tres=$?\n \t\t\tif test $res -gt 0\n \t\t\tthen\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Failed to recurse into submodule path '\\$displaypath'\")\"\n \t\t\t\tif test $res -ne 2\n \t\t\t\tthen\n \t\t\t\t\terr=\"${err};$die_msg\"\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7aa7fefdfa..cb1b8e35db 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n \n test_expect_success 'add aborts on repository with no commits' '\n \tcat >expect <<-\\EOF &&\n-\t'\"'repo-no-commits'\"' does not have a commit checked out\n+\tfatal: '\"'repo-no-commits'\"' does not have a commit checked out\n \tEOF\n \tgit init repo-no-commits &&\n \ttest_must_fail git submodule add ../a ./repo-no-commits 2>actual &&\n@@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n test_expect_success 'submodule add to path with tracked content fails' '\n \t(\n \t\tcd addtest &&\n-\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n+\t\techo \"fatal: '\\''dir-tracked'\\'' already exists in the index\" >expect &&\n \t\tmkdir dir-tracked &&\n \t\ttest_commit foo dir-tracked/bar &&\n \t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f4f61fe554..11cccbb333 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -448,7 +448,7 @@ test_expect_success 'fsck detects command in .gitmodules' '\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n \n test_expect_success 'submodule update - command in .git/config catches failure' '\n@@ -465,7 +465,7 @@ test_expect_success 'submodule update - command in .git/config catches failure'\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n EOF\n \n test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '\n@@ -484,7 +484,7 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n \tcat >expect <<-EOF &&\n-\tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n+\tfatal: Execution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n \tEOF\n \trm -rf super/submodule &&\n \ttest_must_fail git -C super submodule update 2>actual &&\n@@ -493,8 +493,8 @@ test_expect_success 'submodule update - command run for initial population of su\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n-Failed to recurse into submodule path '../super'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n+fatal: Failed to recurse into submodule path '../super'\n EOF\n \n test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '\n-- \n2.32.0\n\n"},{"id":"429479","messageId":"20210708095533.26226-4-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210708095533.26226-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2 3/4] submodule--helper: refactor module_clone()","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T09:55:32Z","receivedAt":"2021-07-08T09:56:01Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Separate out the core logic of module_clone() from the flag\nparsing---this way we can call the equivalent of the `submodule--helper\nclone` subcommand directly within C, without needing to push arguments\nin a strvec.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nSuggested-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------\n 1 file changed, 128 insertions(+), 113 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ae6174ab05..320f4252fe 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static int clone_submodule(const char *path, const char *gitdir, const char *url,\n-\t\t\t const char *depth, struct string_list *reference, int dissociate,\n-\t\t\t int quiet, int progress, int single_branch)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\tstrvec_push(&cp.args, \"clone\");\n-\tstrvec_push(&cp.args, \"--no-checkout\");\n-\tif (quiet)\n-\t\tstrvec_push(&cp.args, \"--quiet\");\n-\tif (progress)\n-\t\tstrvec_push(&cp.args, \"--progress\");\n-\tif (depth && *depth)\n-\t\tstrvec_pushl(&cp.args, \"--depth\", depth, NULL);\n-\tif (reference->nr) {\n-\t\tstruct string_list_item *item;\n-\t\tfor_each_string_list_item(item, reference)\n-\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n-\t\t\t\t item->string, NULL);\n-\t}\n-\tif (dissociate)\n-\t\tstrvec_push(&cp.args, \"--dissociate\");\n-\tif (gitdir && *gitdir)\n-\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n-\tif (single_branch >= 0)\n-\t\tstrvec_push(&cp.args, single_branch ?\n-\t\t\t\t\t \"--single-branch\" :\n-\t\t\t\t\t \"--no-single-branch\");\n-\n-\tstrvec_push(&cp.args, \"--\");\n-\tstrvec_push(&cp.args, url);\n-\tstrvec_push(&cp.args, path);\n-\n-\tcp.git_cmd = 1;\n-\tprepare_submodule_repo_env(&cp.env_array);\n-\tcp.no_stdin = 1;\n-\n-\treturn run_command(&cp);\n-}\n+struct module_clone_data {\n+\tconst char *prefix;\n+\tconst char *path;\n+\tconst char *name;\n+\tconst char *url;\n+\tconst char *depth;\n+\tstruct string_list reference;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+\tunsigned int require_init: 1;\n+\tint single_branch;\n+};\n+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }\n \n struct submodule_alternate_setup {\n \tconst char *submodule_name;\n@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,\n \tfree(error_strategy);\n }\n \n+static int clone_submodule(struct module_clone_data *clone_data)\n+{\n+\tchar *p, *sm_gitdir;\n+\tchar *sm_alternate = NULL, *error_strategy = NULL;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), clone_data->name);\n+\tsm_gitdir = absolute_pathdup(sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\tif (!is_absolute_path(clone_data->path)) {\n+\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), clone_data->path);\n+\t\tclone_data->path = strbuf_detach(&sb, NULL);\n+\t} else {\n+\t\tclone_data->path = xstrdup(clone_data->path);\n+\t}\n+\n+\tif (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)\n+\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n+\t\t \"git dir\"), sm_gitdir);\n+\n+\tif (!file_exists(sm_gitdir)) {\n+\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n+\n+\t\tprepare_possible_alternates(clone_data->name, &clone_data->reference);\n+\n+\t\tstrvec_push(&cp.args, \"clone\");\n+\t\tstrvec_push(&cp.args, \"--no-checkout\");\n+\t\tif (clone_data->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tif (clone_data->progress)\n+\t\t\tstrvec_push(&cp.args, \"--progress\");\n+\t\tif (clone_data->depth && *(clone_data->depth))\n+\t\t\tstrvec_pushl(&cp.args, \"--depth\", clone_data->depth, NULL);\n+\t\tif (clone_data->reference.nr) {\n+\t\t\tstruct string_list_item *item;\n+\t\t\tfor_each_string_list_item(item, &clone_data->reference)\n+\t\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n+\t\t\t\t\t item->string, NULL);\n+\t\t}\n+\t\tif (clone_data->dissociate)\n+\t\t\tstrvec_push(&cp.args, \"--dissociate\");\n+\t\tif (sm_gitdir && *sm_gitdir)\n+\t\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", sm_gitdir, NULL);\n+\t\tif (clone_data->single_branch >= 0)\n+\t\t\tstrvec_push(&cp.args, clone_data->single_branch ?\n+\t\t\t\t \"--single-branch\" :\n+\t\t\t\t \"--no-single-branch\");\n+\n+\t\tstrvec_push(&cp.args, \"--\");\n+\t\tstrvec_push(&cp.args, clone_data->url);\n+\t\tstrvec_push(&cp.args, clone_data->path);\n+\n+\t\tcp.git_cmd = 1;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.no_stdin = 1;\n+\n+\t\tif(run_command(&cp))\n+\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n+\t\t\t clone_data->url, clone_data->path);\n+\t} else {\n+\t\tif (clone_data->require_init && !access(clone_data->path, X_OK) &&\n+\t\t !is_empty_dir(clone_data->path))\n+\t\t\tdie(_(\"directory not empty: '%s'\"), clone_data->path);\n+\t\tif (safe_create_leading_directories_const(clone_data->path) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), clone_data->path);\n+\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n+\t\tunlink_or_warn(sb.buf);\n+\t\tstrbuf_reset(&sb);\n+\t}\n+\n+\tconnect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);\n+\n+\tp = git_pathdup_submodule(clone_data->path, \"config\");\n+\tif (!p)\n+\t\tdie(_(\"could not get submodule directory for '%s'\"), clone_data->path);\n+\n+\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n+\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\tif (sm_alternate)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n+\t\t\t\t sm_alternate);\n+\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\tif (error_strategy)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n+\t\t\t\t error_strategy);\n+\n+\tfree(sm_alternate);\n+\tfree(error_strategy);\n+\n+\tstrbuf_release(&sb);\n+\tfree(sm_gitdir);\n+\tfree(p);\n+\treturn 0;\n+}\n+\n static int module_clone(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *name = NULL, *url = NULL, *depth = NULL;\n-\tint quiet = 0;\n-\tint progress = 0;\n-\tchar *p, *path = NULL, *sm_gitdir;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct string_list reference = STRING_LIST_INIT_NODUP;\n-\tint dissociate = 0, require_init = 0;\n-\tchar *sm_alternate = NULL, *error_strategy = NULL;\n-\tint single_branch = -1;\n+\tint dissociate = 0, quiet = 0, progress = 0, require_init = 0;\n+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n \n \tstruct option module_clone_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\tOPT_STRING(0, \"prefix\", &clone_data.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"alternative anchor for relative paths\")),\n-\t\tOPT_STRING(0, \"path\", &path,\n+\t\tOPT_STRING(0, \"path\", &clone_data.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\", &name,\n+\t\tOPT_STRING(0, \"name\", &clone_data.name,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"name of the new submodule\")),\n-\t\tOPT_STRING(0, \"url\", &url,\n+\t\tOPT_STRING(0, \"url\", &clone_data.url,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"url where to clone the submodule from\")),\n-\t\tOPT_STRING_LIST(0, \"reference\", &reference,\n+\t\tOPT_STRING_LIST(0, \"reference\", &clone_data.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_STRING(0, \"depth\", &depth,\n+\t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n \t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\n \t\t\t N_(\"disallow cloning into non-empty directory\")),\n-\t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n+\t\tOPT_BOOL(0, \"single-branch\", &clone_data.single_branch,\n \t\t\t N_(\"clone only one branch, HEAD or --branch\")),\n \t\tOPT_END()\n \t};\n@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n-\tif (argc || !url || !path || !*path)\n+\tclone_data.dissociate = !!dissociate;\n+\tclone_data.quiet = !!quiet;\n+\tclone_data.progress = !!progress;\n+\tclone_data.require_init = !!require_init;\n+\n+\tif (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n-\tsm_gitdir = absolute_pathdup(sb.buf);\n-\tstrbuf_reset(&sb);\n-\n-\tif (!is_absolute_path(path)) {\n-\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), path);\n-\t\tpath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tpath = xstrdup(path);\n-\n-\tif (validate_submodule_git_dir(sm_gitdir, name) < 0)\n-\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n-\t\t\t\"git dir\"), sm_gitdir);\n-\n-\tif (!file_exists(sm_gitdir)) {\n-\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n-\n-\t\tprepare_possible_alternates(name, &reference);\n-\n-\t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,\n-\t\t\t\t quiet, progress, single_branch))\n-\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n-\t\t\t url, path);\n-\t} else {\n-\t\tif (require_init && !access(path, X_OK) && !is_empty_dir(path))\n-\t\t\tdie(_(\"directory not empty: '%s'\"), path);\n-\t\tif (safe_create_leading_directories_const(path) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), path);\n-\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n-\t\tunlink_or_warn(sb.buf);\n-\t\tstrbuf_reset(&sb);\n-\t}\n-\n-\tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n-\n-\tp = git_pathdup_submodule(path, \"config\");\n-\tif (!p)\n-\t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n-\n-\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n-\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n-\tif (sm_alternate)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n-\t\t\t\t\t sm_alternate);\n-\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n-\tif (error_strategy)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n-\t\t\t\t\t error_strategy);\n-\n-\tfree(sm_alternate);\n-\tfree(error_strategy);\n-\n-\tstrbuf_release(&sb);\n-\tfree(sm_gitdir);\n-\tfree(path);\n-\tfree(p);\n+\tclone_submodule(&clone_data);\n \treturn 0;\n }\n \n-- \n2.32.0\n\n"},{"id":"429480","messageId":"20210708095533.26226-5-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210708095533.26226-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2 4/4] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-08T09:55:33Z","receivedAt":"2021-07-08T09:56:05Z","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'cmd_add()' script unchanged.\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 \nHelped-by: Đoàn Trần Công Danh \n---\n builtin/submodule--helper.c | 177 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +-------\n 2 files changed, 178 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 320f4252fe..862053c9f2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2760,6 +2760,182 @@ 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 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;\n+\t\tchar *line = sb_remote_out.buf;\n+\t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n+\t\t\tsize_t len = next_line - line;\n+\t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n+\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n+\t\t\tline = next_line + 1;\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+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n+\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\tstruct strbuf sm_path = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sm_path, add_data->sm_path);\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", add_data->sm_path);\n+\t\tif (is_nonbare_repository_dir(&sm_path))\n+\t\t\tprintf(_(\"Adding existing repo at '%s' to the 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\tstrbuf_release(&sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\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\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\t \"locally with remote(s):\"),\n+\t\t\t\t\tadd_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\tfree(submod_gitdir_path);\n+\t\t\t\tdie(_(\"If you want to reuse this local git \"\n+\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t \" %s\\n\"\n+\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t add_data->realrepo);\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\tclone_data.prefix = add_data->prefix;\n+\t\tclone_data.path = add_data->sm_path;\n+\t\tclone_data.name = add_data->sm_name;\n+\t\tclone_data.url = add_data->realrepo;\n+\t\tclone_data.quiet = add_data->quiet;\n+\t\tclone_data.progress = add_data->progress;\n+\t\tif (add_data->reference_path)\n+\t\t\tstring_list_append(&clone_data.reference,\n+\t\t\t\t\t xstrdup(add_data->reference_path));\n+\t\tclone_data.dissociate = add_data->dissociate;\n+\t\tif (add_data->depth >= 0)\n+\t\t\tclone_data.depth = xstrfmt(\"%d\", add_data->depth);\n+\n+\t\tif (clone_submodule(&clone_data))\n+\t\t\treturn -1;\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\", &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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\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 [...] \"\n+\t\t \"--url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 0)\n+\t\tusage_with_options(usage, options);\n+\n+\tadd_data.prefix = prefix;\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@@ -2772,6 +2948,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 b887daa8a1..a65928d70e 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,43 +241,7 @@ cmd_add()\n \t\tdie \"$(eval_gettext \"fatal: '$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.32.0\n\n"},{"id":"429492","messageId":"xmqqo8bcbzrx.fsf@gitster.g","threadId":"56057","inReplyTo":"20210708095533.26226-3-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v2 2/4] submodule: prefix die messages with 'fatal'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T15:17:54Z","receivedAt":"2021-07-08T15:17:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\n\n> The standard `die()` function that is used in C code prefixes all the\n> messages passed to it with 'fatal: '. This does not happen with the\n> `die` used in 'git-submodule.sh'.\n>\n> Let's prefix each of the shell die messages with 'fatal: ' so that when\n> they are converted to C code, the error messages stay the same as before\n> the conversion.\n\nSounds good. More importantly, the error messages from the\nresulting system would become more uniform---after all, the end\nusers would not care if scripted part of the system is emitting the\nerror messages, or the message comes from a built-in version.\n"},{"id":"429547","messageId":"YOhirWqj7ajsqlYw@danh.dev","threadId":"56057","inReplyTo":"20210708095533.26226-3-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v2 2/4] submodule: prefix die messages with 'fatal'","fromName":"Đoàn Trần Công Danh","fromEmail":"congdanhqx@gmail.com","sentAt":"2021-07-09T14:52:29Z","receivedAt":"2021-07-09T14:52:36Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2021-07-08 15:25:31+0530, Atharva Raykar wrote:\n> The standard `die()` function that is used in C code prefixes all the\n> messages passed to it with 'fatal: '. This does not happen with the\n> `die` used in 'git-submodule.sh'.\n> \n> Let's prefix each of the shell die messages with 'fatal: ' so that when\n> they are converted to C code, the error messages stay the same as before\n> the conversion.\n\nThat sounds good.\n\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -147,7 +147,7 @@ cmd_add()\n> \n> \tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n> \tthen\n> -\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n> +\t\t die \"$(eval_gettext \"fatal: please make sure that the .gitmodules file is in the working tree\")\"\n\nExcept that, \"fatal: \" isn't subjected to translation. And this will\ncreate new translatable item for translator. Perhaps:\n\n-\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n+\t\t die \"fatal: $(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n\n-- Danh\n\n> \tfi\n> \n> \tif test -n \"$reference_path\"\n> @@ -176,7 +176,7 @@ cmd_add()\n> \tcase \"$repo\" in\n> \t./*|../*)\n> \t\ttest -z \"$wt_prefix\" ||\n> -\t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n> +\t\tdie \"$(gettext \"fatal: Relative path can only be used from the toplevel of the working tree\")\"\n> \n> \t\t# dereference source url relative to parent's url\n> \t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n> @@ -186,7 +186,7 @@ cmd_add()\n> \t\trealrepo=$repo\n> \t\t;;\n> \t*)\n> -\t\tdie \"$(eval_gettext \"repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n> +\t\tdie \"$(eval_gettext \"fatal: repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n> \t;;\n> \tesac\n> \n> @@ -205,17 +205,17 @@ cmd_add()\n> \tif test -z \"$force\"\n> \tthen\n> \t\tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n> -\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n> +\t\tdie \"$(eval_gettext \"fatal: '\\$sm_path' already exists in the index\")\"\n> \telse\n> \t\tgit ls-files -s \"$sm_path\" | sane_grep -v \"^160000\" > /dev/null 2>&1 &&\n> -\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n> +\t\tdie \"$(eval_gettext \"fatal: '\\$sm_path' already exists in the index and is not a submodule\")\"\n> \tfi\n> \n> \tif test -d \"$sm_path\" &&\n> \t\ttest -z $(git -C \"$sm_path\" rev-parse --show-cdup 2>/dev/null)\n> \tthen\n> \t git -C \"$sm_path\" rev-parse --verify -q HEAD >/dev/null ||\n> -\t die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n> +\t die \"$(eval_gettext \"fatal: '\\$sm_path' does not have a commit checked out\")\"\n> \tfi\n> \n> \tif test -z \"$force\"\n> @@ -238,7 +238,7 @@ cmd_add()\n> \n> \tif ! git submodule--helper check-name \"$sm_name\"\n> \tthen\n> -\t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n> +\t\tdie \"$(eval_gettext \"fatal: '$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> @@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n> \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n> \n> \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n> -\tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n> +\tdie \"$(eval_gettext \"fatal: Failed to add submodule '\\$sm_path'\")\"\n> \n> \tgit submodule--helper config submodule.\"$sm_name\".path \"$sm_path\" &&\n> \tgit submodule--helper config submodule.\"$sm_name\".url \"$repo\" &&\n> @@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n> \t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n> \tfi &&\n> \tgit add --force .gitmodules ||\n> -\tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n> +\tdie \"$(eval_gettext \"fatal: Failed to register submodule '\\$sm_path'\")\"\n> \n> \t# NEEDSWORK: In a multi-working-tree world, this needs to be\n> \t# set in the per-worktree config.\n> @@ -565,7 +565,7 @@ cmd_update()\n> \t\telse\n> \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n> \t\t\t\tgit rev-parse --verify HEAD) ||\n> -\t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n> +\t\t\tdie \"$(eval_gettext \"fatal: Unable to find current revision in submodule path '\\$displaypath'\")\"\n> \t\tfi\n> \n> \t\tif test -n \"$remote\"\n> @@ -575,12 +575,12 @@ cmd_update()\n> \t\t\tthen\n> \t\t\t\t# Fetch remote before determining tracking $sha1\n> \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n> -\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n> +\t\t\t\tdie \"$(eval_gettext \"fatal: Unable to fetch in submodule path '\\$sm_path'\")\"\n> \t\t\tfi\n> \t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && git submodule--helper print-default-remote)\n> \t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n> \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n> -\t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n> +\t\t\tdie \"$(eval_gettext \"fatal: Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n> \t\tfi\n> \n> \t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n> @@ -604,36 +604,36 @@ cmd_update()\n> \t\t\t\t# not be reachable from any of the refs\n> \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n> \t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n> -\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n> +\t\t\t\tdie \"$(eval_gettext \"fatal: Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n> \t\t\tfi\n> \n> \t\t\tmust_die_on_failure=\n> \t\t\tcase \"$update_module\" in\n> \t\t\tcheckout)\n> \t\t\t\tcommand=\"git checkout $subforce -q\"\n> -\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> +\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n> \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n> \t\t\t\t;;\n> \t\t\trebase)\n> \t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n> -\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n> +\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n> \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n> \t\t\t\tmust_die_on_failure=yes\n> \t\t\t\t;;\n> \t\t\tmerge)\n> \t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n> -\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n> +\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n> \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n> \t\t\t\tmust_die_on_failure=yes\n> \t\t\t\t;;\n> \t\t\t!*)\n> \t\t\t\tcommand=\"${update_module#!}\"\n> -\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n> +\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n> \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n> \t\t\t\tmust_die_on_failure=yes\n> \t\t\t\t;;\n> \t\t\t*)\n> -\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n> +\t\t\t\tdie \"$(eval_gettext \"fatal: Invalid update mode '$update_module' for submodule path '$path'\")\"\n> \t\t\tesac\n> \n> \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n> @@ -660,7 +660,7 @@ cmd_update()\n> \t\t\tres=$?\n> \t\t\tif test $res -gt 0\n> \t\t\tthen\n> -\t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n> +\t\t\t\tdie_msg=\"$(eval_gettext \"fatal: Failed to recurse into submodule path '\\$displaypath'\")\"\n> \t\t\t\tif test $res -ne 2\n> \t\t\t\tthen\n> \t\t\t\t\terr=\"${err};$die_msg\"\n> diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\n> index 7aa7fefdfa..cb1b8e35db 100755\n> --- a/t/t7400-submodule-basic.sh\n> +++ b/t/t7400-submodule-basic.sh\n> @@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n> \n> test_expect_success 'add aborts on repository with no commits' '\n> \tcat >expect <<-\\EOF &&\n> -\t'\"'repo-no-commits'\"' does not have a commit checked out\n> +\tfatal: '\"'repo-no-commits'\"' does not have a commit checked out\n> \tEOF\n> \tgit init repo-no-commits &&\n> \ttest_must_fail git submodule add ../a ./repo-no-commits 2>actual &&\n> @@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n> test_expect_success 'submodule add to path with tracked content fails' '\n> \t(\n> \t\tcd addtest &&\n> -\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n> +\t\techo \"fatal: '\\''dir-tracked'\\'' already exists in the index\" >expect &&\n> \t\tmkdir dir-tracked &&\n> \t\ttest_commit foo dir-tracked/bar &&\n> \t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\n> diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\n> index f4f61fe554..11cccbb333 100755\n> --- a/t/t7406-submodule-update.sh\n> +++ b/t/t7406-submodule-update.sh\n> @@ -448,7 +448,7 @@ test_expect_success 'fsck detects command in .gitmodules' '\n> '\n> \n> cat << EOF >expect\n> -Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> +fatal: Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n> EOF\n> \n> test_expect_success 'submodule update - command in .git/config catches failure' '\n> @@ -465,7 +465,7 @@ test_expect_success 'submodule update - command in .git/config catches failure'\n> '\n> \n> cat << EOF >expect\n> -Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n> +fatal: Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n> EOF\n> \n> test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '\n> @@ -484,7 +484,7 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n> \n> test_expect_success 'submodule update - command run for initial population of submodule' '\n> \tcat >expect <<-EOF &&\n> -\tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n> +\tfatal: Execution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n> \tEOF\n> \trm -rf super/submodule &&\n> \ttest_must_fail git -C super submodule update 2>actual &&\n> @@ -493,8 +493,8 @@ test_expect_success 'submodule update - command run for initial population of su\n> '\n> \n> cat << EOF >expect\n> -Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n> -Failed to recurse into submodule path '../super'\n> +fatal: Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n> +fatal: Failed to recurse into submodule path '../super'\n> EOF\n> \n> test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '\n> -- \n> 2.32.0\n> \n\n-- \nDanh\n"},{"id":"429590","messageId":"20210710074801.19917-1-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210708095533.26226-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3 0/4] submodule add: partial conversion to C","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:47:57Z","receivedAt":"2021-07-10T07:48:19Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Changes since v2:\n * In 2/4, the prefix 'fatal' is left out of the gettext functions so that it\n will not create new translatable items for the translators.\n\nAtharva Raykar (4):\n t7400: test failure to add submodule in tracked path\n submodule: prefix die messages with 'fatal'\n submodule--helper: refactor module_clone()\n submodule--helper: introduce add-clone subcommand\n\n builtin/submodule--helper.c | 418 ++++++++++++++++++++++++++----------\n git-submodule.sh | 76 ++-----\n t/t7400-submodule-basic.sh | 13 +-\n t/t7406-submodule-update.sh | 10 +-\n 4 files changed, 342 insertions(+), 175 deletions(-)\n\n-- \n2.32.0\n\n"},{"id":"429591","messageId":"20210710074801.19917-2-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210710074801.19917-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3 1/4] t7400: test failure to add submodule in tracked path","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:47:58Z","receivedAt":"2021-07-10T07:48:24Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a test to ensure failure on adding a submodule to a directory with\ntracked contents in the index.\n\nAs we are going to refactor and port to C some parts of `git submodule\nadd`, let's add a test to help ensure no regression is introduced.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nBased-on-patch-by: Shourya Shukla \nMentored-by: Shourya Shukla \n---\n t/t7400-submodule-basic.sh | 11 +++++++++++\n 1 file changed, 11 insertions(+)\n\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex a924fdb7a6..7aa7fefdfa 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -196,6 +196,17 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n \t)\n '\n \n+test_expect_success 'submodule add to path with tracked content fails' '\n+\t(\n+\t\tcd addtest &&\n+\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n+\t\tmkdir dir-tracked &&\n+\t\ttest_commit foo dir-tracked/bar &&\n+\t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_expect_success 'submodule add to reconfigure existing submodule with --force' '\n \t(\n \t\tcd addtest-ignore &&\n-- \n2.32.0\n\n"},{"id":"429592","messageId":"20210710074801.19917-3-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210710074801.19917-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3 2/4] submodule: prefix die messages with 'fatal'","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:47:59Z","receivedAt":"2021-07-10T07:48:28Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"The standard `die()` function that is used in C code prefixes all the\nmessages passed to it with 'fatal: '. This does not happen with the\n`die` used in 'git-submodule.sh'.\n\nLet's prefix each of the shell die messages with 'fatal: ' so that when\nthey are converted to C code, the error messages stay the same as before\nthe conversion.\n\nNote that the shell version of `die` exits with error code 1, while the\nC version exits with error code 128. In practice, this does not change\nany behaviour, as no functionality in 'submodule add' and 'submodule\nupdate' relies on the value of the exit code.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \n---\n git-submodule.sh | 38 ++++++++++++++++++-------------------\n t/t7400-submodule-basic.sh | 4 ++--\n t/t7406-submodule-update.sh | 10 +++++-----\n 3 files changed, 26 insertions(+), 26 deletions(-)\n\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex 4678378424..69bcb4fab2 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -147,7 +147,7 @@ cmd_add()\n \n \tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n \tthen\n-\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n+\t\t die \"fatal: $(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n \tfi\n \n \tif test -n \"$reference_path\"\n@@ -176,7 +176,7 @@ cmd_add()\n \tcase \"$repo\" in\n \t./*|../*)\n \t\ttest -z \"$wt_prefix\" ||\n-\t\tdie \"$(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n+\t\tdie \"fatal: $(gettext \"Relative path can only be used from the toplevel of the working tree\")\"\n \n \t\t# dereference source url relative to parent's url\n \t\trealrepo=$(git submodule--helper resolve-relative-url \"$repo\") || exit\n@@ -186,7 +186,7 @@ cmd_add()\n \t\trealrepo=$repo\n \t\t;;\n \t*)\n-\t\tdie \"$(eval_gettext \"repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n+\t\tdie \"fatal: $(eval_gettext \"repo URL: '\\$repo' must be absolute or begin with ./|../\")\"\n \t;;\n \tesac\n \n@@ -205,17 +205,17 @@ cmd_add()\n \tif test -z \"$force\"\n \tthen\n \t\tgit ls-files --error-unmatch \"$sm_path\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n+\t\tdie \"fatal: $(eval_gettext \"'\\$sm_path' already exists in the index\")\"\n \telse\n \t\tgit ls-files -s \"$sm_path\" | sane_grep -v \"^160000\" > /dev/null 2>&1 &&\n-\t\tdie \"$(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n+\t\tdie \"fatal: $(eval_gettext \"'\\$sm_path' already exists in the index and is not a submodule\")\"\n \tfi\n \n \tif test -d \"$sm_path\" &&\n \t\ttest -z $(git -C \"$sm_path\" rev-parse --show-cdup 2>/dev/null)\n \tthen\n \t git -C \"$sm_path\" rev-parse --verify -q HEAD >/dev/null ||\n-\t die \"$(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n+\t die \"fatal: $(eval_gettext \"'\\$sm_path' does not have a commit checked out\")\"\n \tfi\n \n \tif test -z \"$force\"\n@@ -238,7 +238,7 @@ cmd_add()\n \n \tif ! git submodule--helper check-name \"$sm_name\"\n \tthen\n-\t\tdie \"$(eval_gettext \"'$sm_name' is not a valid submodule name\")\"\n+\t\tdie \"fatal: $(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@@ -281,7 +281,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \tgit config submodule.\"$sm_name\".url \"$realrepo\"\n \n \tgit add --no-warn-embedded-repo $force \"$sm_path\" ||\n-\tdie \"$(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n+\tdie \"fatal: $(eval_gettext \"Failed to add submodule '\\$sm_path'\")\"\n \n \tgit submodule--helper config submodule.\"$sm_name\".path \"$sm_path\" &&\n \tgit submodule--helper config submodule.\"$sm_name\".url \"$repo\" &&\n@@ -290,7 +290,7 @@ or you are unsure what this means choose another name with the '--name' option.\"\n \t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n \tfi &&\n \tgit add --force .gitmodules ||\n-\tdie \"$(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n+\tdie \"fatal: $(eval_gettext \"Failed to register submodule '\\$sm_path'\")\"\n \n \t# NEEDSWORK: In a multi-working-tree world, this needs to be\n \t# set in the per-worktree config.\n@@ -565,7 +565,7 @@ cmd_update()\n \t\telse\n \t\t\tsubsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify HEAD) ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n+\t\t\tdie \"fatal: $(eval_gettext \"Unable to find current revision in submodule path '\\$displaypath'\")\"\n \t\tfi\n \n \t\tif test -n \"$remote\"\n@@ -575,12 +575,12 @@ cmd_update()\n \t\t\tthen\n \t\t\t\t# Fetch remote before determining tracking $sha1\n \t\t\t\tfetch_in_submodule \"$sm_path\" $depth ||\n-\t\t\t\tdie \"$(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n+\t\t\t\tdie \"fatal: $(eval_gettext \"Unable to fetch in submodule path '\\$sm_path'\")\"\n \t\t\tfi\n \t\t\tremote_name=$(sanitize_submodule_env; cd \"$sm_path\" && git submodule--helper print-default-remote)\n \t\t\tsha1=$(sanitize_submodule_env; cd \"$sm_path\" &&\n \t\t\t\tgit rev-parse --verify \"${remote_name}/${branch}\") ||\n-\t\t\tdie \"$(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n+\t\t\tdie \"fatal: $(eval_gettext \"Unable to find current \\${remote_name}/\\${branch} revision in submodule path '\\$sm_path'\")\"\n \t\tfi\n \n \t\tif test \"$subsha1\" != \"$sha1\" || test -n \"$force\"\n@@ -604,36 +604,36 @@ cmd_update()\n \t\t\t\t# not be reachable from any of the refs\n \t\t\t\tis_tip_reachable \"$sm_path\" \"$sha1\" ||\n \t\t\t\tfetch_in_submodule \"$sm_path\" \"$depth\" \"$sha1\" ||\n-\t\t\t\tdie \"$(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n+\t\t\t\tdie \"fatal: $(eval_gettext \"Fetched in submodule path '\\$displaypath', but it did not contain \\$sha1. Direct fetching of that commit failed.\")\"\n \t\t\tfi\n \n \t\t\tmust_die_on_failure=\n \t\t\tcase \"$update_module\" in\n \t\t\tcheckout)\n \t\t\t\tcommand=\"git checkout $subforce -q\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to checkout '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': checked out '\\$sha1'\")\"\n \t\t\t\t;;\n \t\t\trebase)\n \t\t\t\tcommand=\"git rebase ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to rebase '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': rebased into '\\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\tmerge)\n \t\t\t\tcommand=\"git merge ${GIT_QUIET:+--quiet}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Unable to merge '\\$sha1' in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': merged in '\\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t!*)\n \t\t\t\tcommand=\"${update_module#!}\"\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Execution of '\\$command \\$sha1' failed in submodule path '\\$displaypath'\")\"\n \t\t\t\tsay_msg=\"$(eval_gettext \"Submodule path '\\$displaypath': '\\$command \\$sha1'\")\"\n \t\t\t\tmust_die_on_failure=yes\n \t\t\t\t;;\n \t\t\t*)\n-\t\t\t\tdie \"$(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n+\t\t\t\tdie \"fatal: $(eval_gettext \"Invalid update mode '$update_module' for submodule path '$path'\")\"\n \t\t\tesac\n \n \t\t\tif (sanitize_submodule_env; cd \"$sm_path\" && $command \"$sha1\")\n@@ -660,7 +660,7 @@ cmd_update()\n \t\t\tres=$?\n \t\t\tif test $res -gt 0\n \t\t\tthen\n-\t\t\t\tdie_msg=\"$(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n+\t\t\t\tdie_msg=\"fatal: $(eval_gettext \"Failed to recurse into submodule path '\\$displaypath'\")\"\n \t\t\t\tif test $res -ne 2\n \t\t\t\tthen\n \t\t\t\t\terr=\"${err};$die_msg\"\ndiff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh\nindex 7aa7fefdfa..cb1b8e35db 100755\n--- a/t/t7400-submodule-basic.sh\n+++ b/t/t7400-submodule-basic.sh\n@@ -51,7 +51,7 @@ test_expect_success 'submodule update aborts on missing gitmodules url' '\n \n test_expect_success 'add aborts on repository with no commits' '\n \tcat >expect <<-\\EOF &&\n-\t'\"'repo-no-commits'\"' does not have a commit checked out\n+\tfatal: '\"'repo-no-commits'\"' does not have a commit checked out\n \tEOF\n \tgit init repo-no-commits &&\n \ttest_must_fail git submodule add ../a ./repo-no-commits 2>actual &&\n@@ -199,7 +199,7 @@ test_expect_success 'submodule add to .gitignored path with --force' '\n test_expect_success 'submodule add to path with tracked content fails' '\n \t(\n \t\tcd addtest &&\n-\t\techo \"'\\''dir-tracked'\\'' already exists in the index\" >expect &&\n+\t\techo \"fatal: '\\''dir-tracked'\\'' already exists in the index\" >expect &&\n \t\tmkdir dir-tracked &&\n \t\ttest_commit foo dir-tracked/bar &&\n \t\ttest_must_fail git submodule add \"$submodurl\" dir-tracked >actual 2>&1 &&\ndiff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh\nindex f4f61fe554..11cccbb333 100755\n--- a/t/t7406-submodule-update.sh\n+++ b/t/t7406-submodule-update.sh\n@@ -448,7 +448,7 @@ test_expect_success 'fsck detects command in .gitmodules' '\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path 'submodule'\n EOF\n \n test_expect_success 'submodule update - command in .git/config catches failure' '\n@@ -465,7 +465,7 @@ test_expect_success 'submodule update - command in .git/config catches failure'\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path '../submodule'\n EOF\n \n test_expect_success 'submodule update - command in .git/config catches failure -- subdirectory' '\n@@ -484,7 +484,7 @@ test_expect_success 'submodule update - command in .git/config catches failure -\n \n test_expect_success 'submodule update - command run for initial population of submodule' '\n \tcat >expect <<-EOF &&\n-\tExecution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n+\tfatal: Execution of '\\''false $submodulesha1'\\'' failed in submodule path '\\''submodule'\\''\n \tEOF\n \trm -rf super/submodule &&\n \ttest_must_fail git -C super submodule update 2>actual &&\n@@ -493,8 +493,8 @@ test_expect_success 'submodule update - command run for initial population of su\n '\n \n cat << EOF >expect\n-Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n-Failed to recurse into submodule path '../super'\n+fatal: Execution of 'false $submodulesha1' failed in submodule path '../super/submodule'\n+fatal: Failed to recurse into submodule path '../super'\n EOF\n \n test_expect_success 'recursive submodule update - command in .git/config catches failure -- subdirectory' '\n-- \n2.32.0\n\n"},{"id":"429593","messageId":"20210710074801.19917-4-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210710074801.19917-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3 3/4] submodule--helper: refactor module_clone()","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:48:00Z","receivedAt":"2021-07-10T07:48:31Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Separate out the core logic of module_clone() from the flag\nparsing---this way we can call the equivalent of the `submodule--helper\nclone` subcommand directly within C, without needing to push arguments\nin a strvec.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nSuggested-by: Junio C Hamano \n---\n builtin/submodule--helper.c | 241 +++++++++++++++++++-----------------\n 1 file changed, 128 insertions(+), 113 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ae6174ab05..320f4252fe 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1658,45 +1658,20 @@ static int module_deinit(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n-static int clone_submodule(const char *path, const char *gitdir, const char *url,\n-\t\t\t const char *depth, struct string_list *reference, int dissociate,\n-\t\t\t int quiet, int progress, int single_branch)\n-{\n-\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\n-\tstrvec_push(&cp.args, \"clone\");\n-\tstrvec_push(&cp.args, \"--no-checkout\");\n-\tif (quiet)\n-\t\tstrvec_push(&cp.args, \"--quiet\");\n-\tif (progress)\n-\t\tstrvec_push(&cp.args, \"--progress\");\n-\tif (depth && *depth)\n-\t\tstrvec_pushl(&cp.args, \"--depth\", depth, NULL);\n-\tif (reference->nr) {\n-\t\tstruct string_list_item *item;\n-\t\tfor_each_string_list_item(item, reference)\n-\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n-\t\t\t\t item->string, NULL);\n-\t}\n-\tif (dissociate)\n-\t\tstrvec_push(&cp.args, \"--dissociate\");\n-\tif (gitdir && *gitdir)\n-\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", gitdir, NULL);\n-\tif (single_branch >= 0)\n-\t\tstrvec_push(&cp.args, single_branch ?\n-\t\t\t\t\t \"--single-branch\" :\n-\t\t\t\t\t \"--no-single-branch\");\n-\n-\tstrvec_push(&cp.args, \"--\");\n-\tstrvec_push(&cp.args, url);\n-\tstrvec_push(&cp.args, path);\n-\n-\tcp.git_cmd = 1;\n-\tprepare_submodule_repo_env(&cp.env_array);\n-\tcp.no_stdin = 1;\n-\n-\treturn run_command(&cp);\n-}\n+struct module_clone_data {\n+\tconst char *prefix;\n+\tconst char *path;\n+\tconst char *name;\n+\tconst char *url;\n+\tconst char *depth;\n+\tstruct string_list reference;\n+\tunsigned int quiet: 1;\n+\tunsigned int progress: 1;\n+\tunsigned int dissociate: 1;\n+\tunsigned int require_init: 1;\n+\tint single_branch;\n+};\n+#define MODULE_CLONE_DATA_INIT { .reference = STRING_LIST_INIT_NODUP, .single_branch = -1 }\n \n struct submodule_alternate_setup {\n \tconst char *submodule_name;\n@@ -1802,37 +1777,128 @@ static void prepare_possible_alternates(const char *sm_name,\n \tfree(error_strategy);\n }\n \n+static int clone_submodule(struct module_clone_data *clone_data)\n+{\n+\tchar *p, *sm_gitdir;\n+\tchar *sm_alternate = NULL, *error_strategy = NULL;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\n+\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), clone_data->name);\n+\tsm_gitdir = absolute_pathdup(sb.buf);\n+\tstrbuf_reset(&sb);\n+\n+\tif (!is_absolute_path(clone_data->path)) {\n+\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), clone_data->path);\n+\t\tclone_data->path = strbuf_detach(&sb, NULL);\n+\t} else {\n+\t\tclone_data->path = xstrdup(clone_data->path);\n+\t}\n+\n+\tif (validate_submodule_git_dir(sm_gitdir, clone_data->name) < 0)\n+\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n+\t\t \"git dir\"), sm_gitdir);\n+\n+\tif (!file_exists(sm_gitdir)) {\n+\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n+\n+\t\tprepare_possible_alternates(clone_data->name, &clone_data->reference);\n+\n+\t\tstrvec_push(&cp.args, \"clone\");\n+\t\tstrvec_push(&cp.args, \"--no-checkout\");\n+\t\tif (clone_data->quiet)\n+\t\t\tstrvec_push(&cp.args, \"--quiet\");\n+\t\tif (clone_data->progress)\n+\t\t\tstrvec_push(&cp.args, \"--progress\");\n+\t\tif (clone_data->depth && *(clone_data->depth))\n+\t\t\tstrvec_pushl(&cp.args, \"--depth\", clone_data->depth, NULL);\n+\t\tif (clone_data->reference.nr) {\n+\t\t\tstruct string_list_item *item;\n+\t\t\tfor_each_string_list_item(item, &clone_data->reference)\n+\t\t\t\tstrvec_pushl(&cp.args, \"--reference\",\n+\t\t\t\t\t item->string, NULL);\n+\t\t}\n+\t\tif (clone_data->dissociate)\n+\t\t\tstrvec_push(&cp.args, \"--dissociate\");\n+\t\tif (sm_gitdir && *sm_gitdir)\n+\t\t\tstrvec_pushl(&cp.args, \"--separate-git-dir\", sm_gitdir, NULL);\n+\t\tif (clone_data->single_branch >= 0)\n+\t\t\tstrvec_push(&cp.args, clone_data->single_branch ?\n+\t\t\t\t \"--single-branch\" :\n+\t\t\t\t \"--no-single-branch\");\n+\n+\t\tstrvec_push(&cp.args, \"--\");\n+\t\tstrvec_push(&cp.args, clone_data->url);\n+\t\tstrvec_push(&cp.args, clone_data->path);\n+\n+\t\tcp.git_cmd = 1;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tcp.no_stdin = 1;\n+\n+\t\tif(run_command(&cp))\n+\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n+\t\t\t clone_data->url, clone_data->path);\n+\t} else {\n+\t\tif (clone_data->require_init && !access(clone_data->path, X_OK) &&\n+\t\t !is_empty_dir(clone_data->path))\n+\t\t\tdie(_(\"directory not empty: '%s'\"), clone_data->path);\n+\t\tif (safe_create_leading_directories_const(clone_data->path) < 0)\n+\t\t\tdie(_(\"could not create directory '%s'\"), clone_data->path);\n+\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n+\t\tunlink_or_warn(sb.buf);\n+\t\tstrbuf_reset(&sb);\n+\t}\n+\n+\tconnect_work_tree_and_git_dir(clone_data->path, sm_gitdir, 0);\n+\n+\tp = git_pathdup_submodule(clone_data->path, \"config\");\n+\tif (!p)\n+\t\tdie(_(\"could not get submodule directory for '%s'\"), clone_data->path);\n+\n+\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n+\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n+\tif (sm_alternate)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n+\t\t\t\t sm_alternate);\n+\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n+\tif (error_strategy)\n+\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n+\t\t\t\t error_strategy);\n+\n+\tfree(sm_alternate);\n+\tfree(error_strategy);\n+\n+\tstrbuf_release(&sb);\n+\tfree(sm_gitdir);\n+\tfree(p);\n+\treturn 0;\n+}\n+\n static int module_clone(int argc, const char **argv, const char *prefix)\n {\n-\tconst char *name = NULL, *url = NULL, *depth = NULL;\n-\tint quiet = 0;\n-\tint progress = 0;\n-\tchar *p, *path = NULL, *sm_gitdir;\n-\tstruct strbuf sb = STRBUF_INIT;\n-\tstruct string_list reference = STRING_LIST_INIT_NODUP;\n-\tint dissociate = 0, require_init = 0;\n-\tchar *sm_alternate = NULL, *error_strategy = NULL;\n-\tint single_branch = -1;\n+\tint dissociate = 0, quiet = 0, progress = 0, require_init = 0;\n+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n \n \tstruct option module_clone_options[] = {\n-\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\tOPT_STRING(0, \"prefix\", &clone_data.prefix,\n \t\t\t N_(\"path\"),\n \t\t\t N_(\"alternative anchor for relative paths\")),\n-\t\tOPT_STRING(0, \"path\", &path,\n+\t\tOPT_STRING(0, \"path\", &clone_data.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\", &name,\n+\t\tOPT_STRING(0, \"name\", &clone_data.name,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"name of the new submodule\")),\n-\t\tOPT_STRING(0, \"url\", &url,\n+\t\tOPT_STRING(0, \"url\", &clone_data.url,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"url where to clone the submodule from\")),\n-\t\tOPT_STRING_LIST(0, \"reference\", &reference,\n+\t\tOPT_STRING_LIST(0, \"reference\", &clone_data.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_STRING(0, \"depth\", &depth,\n+\t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n \t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n@@ -1840,7 +1906,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\n \t\t\t N_(\"disallow cloning into non-empty directory\")),\n-\t\tOPT_BOOL(0, \"single-branch\", &single_branch,\n+\t\tOPT_BOOL(0, \"single-branch\", &clone_data.single_branch,\n \t\t\t N_(\"clone only one branch, HEAD or --branch\")),\n \t\tOPT_END()\n \t};\n@@ -1856,67 +1922,16 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \targc = parse_options(argc, argv, prefix, module_clone_options,\n \t\t\t git_submodule_helper_usage, 0);\n \n-\tif (argc || !url || !path || !*path)\n+\tclone_data.dissociate = !!dissociate;\n+\tclone_data.quiet = !!quiet;\n+\tclone_data.progress = !!progress;\n+\tclone_data.require_init = !!require_init;\n+\n+\tif (argc || !clone_data.url || !clone_data.path || !*(clone_data.path))\n \t\tusage_with_options(git_submodule_helper_usage,\n \t\t\t\t module_clone_options);\n \n-\tstrbuf_addf(&sb, \"%s/modules/%s\", get_git_dir(), name);\n-\tsm_gitdir = absolute_pathdup(sb.buf);\n-\tstrbuf_reset(&sb);\n-\n-\tif (!is_absolute_path(path)) {\n-\t\tstrbuf_addf(&sb, \"%s/%s\", get_git_work_tree(), path);\n-\t\tpath = strbuf_detach(&sb, NULL);\n-\t} else\n-\t\tpath = xstrdup(path);\n-\n-\tif (validate_submodule_git_dir(sm_gitdir, name) < 0)\n-\t\tdie(_(\"refusing to create/use '%s' in another submodule's \"\n-\t\t\t\"git dir\"), sm_gitdir);\n-\n-\tif (!file_exists(sm_gitdir)) {\n-\t\tif (safe_create_leading_directories_const(sm_gitdir) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), sm_gitdir);\n-\n-\t\tprepare_possible_alternates(name, &reference);\n-\n-\t\tif (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,\n-\t\t\t\t quiet, progress, single_branch))\n-\t\t\tdie(_(\"clone of '%s' into submodule path '%s' failed\"),\n-\t\t\t url, path);\n-\t} else {\n-\t\tif (require_init && !access(path, X_OK) && !is_empty_dir(path))\n-\t\t\tdie(_(\"directory not empty: '%s'\"), path);\n-\t\tif (safe_create_leading_directories_const(path) < 0)\n-\t\t\tdie(_(\"could not create directory '%s'\"), path);\n-\t\tstrbuf_addf(&sb, \"%s/index\", sm_gitdir);\n-\t\tunlink_or_warn(sb.buf);\n-\t\tstrbuf_reset(&sb);\n-\t}\n-\n-\tconnect_work_tree_and_git_dir(path, sm_gitdir, 0);\n-\n-\tp = git_pathdup_submodule(path, \"config\");\n-\tif (!p)\n-\t\tdie(_(\"could not get submodule directory for '%s'\"), path);\n-\n-\t/* setup alternateLocation and alternateErrorStrategy in the cloned submodule if needed */\n-\tgit_config_get_string(\"submodule.alternateLocation\", &sm_alternate);\n-\tif (sm_alternate)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateLocation\",\n-\t\t\t\t\t sm_alternate);\n-\tgit_config_get_string(\"submodule.alternateErrorStrategy\", &error_strategy);\n-\tif (error_strategy)\n-\t\tgit_config_set_in_file(p, \"submodule.alternateErrorStrategy\",\n-\t\t\t\t\t error_strategy);\n-\n-\tfree(sm_alternate);\n-\tfree(error_strategy);\n-\n-\tstrbuf_release(&sb);\n-\tfree(sm_gitdir);\n-\tfree(path);\n-\tfree(p);\n+\tclone_submodule(&clone_data);\n \treturn 0;\n }\n \n-- \n2.32.0\n\n"},{"id":"429594","messageId":"20210710074801.19917-5-raykar.ath@gmail.com","threadId":"56057","inReplyTo":"20210710074801.19917-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3 4/4] submodule--helper: introduce add-clone subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:48:01Z","receivedAt":"2021-07-10T07:48:36Z","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'cmd_add()' script unchanged.\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 \nHelped-by: Đoàn Trần Công Danh \n---\n builtin/submodule--helper.c | 177 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 38 +-------\n 2 files changed, 178 insertions(+), 37 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 320f4252fe..862053c9f2 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2760,6 +2760,182 @@ 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 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;\n+\t\tchar *line = sb_remote_out.buf;\n+\t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n+\t\t\tsize_t len = next_line - line;\n+\t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n+\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n+\t\t\tline = next_line + 1;\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+\tstruct module_clone_data clone_data = MODULE_CLONE_DATA_INIT;\n+\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\tstruct strbuf sm_path = STRBUF_INIT;\n+\t\tstrbuf_addstr(&sm_path, add_data->sm_path);\n+\t\tsubmod_gitdir_path = xstrfmt(\"%s/.git\", add_data->sm_path);\n+\t\tif (is_nonbare_repository_dir(&sm_path))\n+\t\t\tprintf(_(\"Adding existing repo at '%s' to the 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\tstrbuf_release(&sm_path);\n+\t\tfree(submod_gitdir_path);\n+\t} else {\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\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\t \"locally with remote(s):\"),\n+\t\t\t\t\tadd_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\tfree(submod_gitdir_path);\n+\t\t\t\tdie(_(\"If you want to reuse this local git \"\n+\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t \" %s\\n\"\n+\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t \"or if you are unsure what this means, choose \"\n+\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t add_data->realrepo);\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\tclone_data.prefix = add_data->prefix;\n+\t\tclone_data.path = add_data->sm_path;\n+\t\tclone_data.name = add_data->sm_name;\n+\t\tclone_data.url = add_data->realrepo;\n+\t\tclone_data.quiet = add_data->quiet;\n+\t\tclone_data.progress = add_data->progress;\n+\t\tif (add_data->reference_path)\n+\t\t\tstring_list_append(&clone_data.reference,\n+\t\t\t\t\t xstrdup(add_data->reference_path));\n+\t\tclone_data.dissociate = add_data->dissociate;\n+\t\tif (add_data->depth >= 0)\n+\t\t\tclone_data.depth = xstrfmt(\"%d\", add_data->depth);\n+\n+\t\tif (clone_submodule(&clone_data))\n+\t\t\treturn -1;\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\", &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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\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 [...] \"\n+\t\t \"--url --path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc != 0)\n+\t\tusage_with_options(usage, options);\n+\n+\tadd_data.prefix = prefix;\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@@ -2772,6 +2948,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 69bcb4fab2..053daf3724 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -241,43 +241,7 @@ cmd_add()\n \t\tdie \"fatal: $(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.32.0\n\n"},{"id":"429595","messageId":"598E78EB-48B1-4C2E-BD89-90EF003A15F6@gmail.com","threadId":"56057","inReplyTo":"YOhirWqj7ajsqlYw@danh.dev","subject":"Re: [GSoC] [PATCH v2 2/4] submodule: prefix die messages with 'fatal'","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-10T07:52:16Z","receivedAt":"2021-07-10T07:52:24Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\n\n> On 09-Jul-2021, at 20:22, Đoàn Trần Công Danh wrote:\n> \n> On 2021-07-08 15:25:31+0530, Atharva Raykar wrote:\n>> The standard `die()` function that is used in C code prefixes all the\n>> messages passed to it with 'fatal: '. This does not happen with the\n>> `die` used in 'git-submodule.sh'.\n>> \n>> Let's prefix each of the shell die messages with 'fatal: ' so that when\n>> they are converted to C code, the error messages stay the same as before\n>> the conversion.\n> \n> That sounds good.\n> \n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -147,7 +147,7 @@ cmd_add()\n>> \n>> \tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n>> \tthen\n>> -\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n>> +\t\t die \"$(eval_gettext \"fatal: please make sure that the .gitmodules file is in the working tree\")\"\n> \n> Except that, \"fatal: \" isn't subjected to translation. And this will\n> create new translatable item for translator. Perhaps:\n> \n> -\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n> +\t\t die \"fatal: $(eval_gettext \"please make sure that the .gitmodules file is in the working tree\")\"\n\nOkay, I have made the change. I was wondering if there any specific\nreason as to why 'fatal' should not be translated? Is it because\nan intermediate change like this should not create more work for\ntranslators? \n\n\n"},{"id":"429611","messageId":"ED07F10B-BE44-4BCC-873A-73688683CCF4@gmail.com","threadId":"56057","inReplyTo":"598E78EB-48B1-4C2E-BD89-90EF003A15F6@gmail.com","subject":"Re: [GSoC] [PATCH v2 2/4] submodule: prefix die messages with 'fatal'","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-07-10T12:04:30Z","receivedAt":"2021-07-10T12:04:34Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi Atharva,\n\n\nOn 10 ஜூலை, 2021 பிற்பகல் 1:22:16 IST, Atharva Raykar wrote:\n>\n>\n>> On 09-Jul-2021, at 20:22, Đoàn Trần Công Danh \n>wrote:\n>> \n>> On 2021-07-08 15:25:31+0530, Atharva Raykar \n>wrote:\n>>> The standard `die()` function that is used in C code prefixes all\n>the\n>>> messages passed to it with 'fatal: '. This does not happen with the\n>>> `die` used in 'git-submodule.sh'.\n>>> \n>>> Let's prefix each of the shell die messages with 'fatal: ' so that\n>when\n>>> they are converted to C code, the error messages stay the same as\n>before\n>>> the conversion.\n>> \n>> That sounds good.\n>> \n>>> --- a/git-submodule.sh\n>>> +++ b/git-submodule.sh\n>>> @@ -147,7 +147,7 @@ cmd_add()\n>>> \n>>> \tif ! git submodule--helper config --check-writeable >/dev/null 2>&1\n>>> \tthen\n>>> -\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file\n>is in the working tree\")\"\n>>> +\t\t die \"$(eval_gettext \"fatal: please make sure that the\n>.gitmodules file is in the working tree\")\"\n>> \n>> Except that, \"fatal: \" isn't subjected to translation. And this will\n>> create new translatable item for translator. Perhaps:\n>> \n>> -\t\t die \"$(eval_gettext \"please make sure that the .gitmodules file\n>is in the working tree\")\"\n>> +\t\t die \"fatal: $(eval_gettext \"please make sure that the .gitmodules\n>file is in the working tree\")\"\n>\n>Okay, I have made the change. I was wondering if there any specific\n>reason as to why 'fatal' should not be translated? Is it because\n>an intermediate change like this should not create more work for\n>translators? \n\nYes. That's likely the intention.\n\n-- \nSivaraam\n\nSent from my Android device with K-9 Mail. Please excuse my brevity.\n"},{"id":"431022","messageId":"YPqkHs47VDFBNZ0Z@coredump.intra.peff.net","threadId":"56057","inReplyTo":"20210710074801.19917-5-raykar.ath@gmail.com","subject":"[PATCH] submodule: drop unused sm_name parameter from show_fetch_remotes()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-23T11:12:30Z","receivedAt":"2021-07-23T11:12:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jul 10, 2021 at 01:18:01PM +0530, Atharva Raykar wrote:\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;\n> +\t\tchar *line = sb_remote_out.buf;\n> +\t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n> +\t\t\tsize_t len = next_line - line;\n> +\t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n> +\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n> +\t\t\tline = next_line + 1;\n> +\t\t}\n> +\t}\n> +\n> +\tstrbuf_release(&sb_remote_out);\n> +}\n\nThe sm_name parameter is not used here. I don't think it's a bug; we\njust don't need it (there's a message that mentions the name, but it\nhappens right before we call the function). Maybe this should go on top\nof ar/submodule-add?\n\n-- >8 --\nSubject: submodule: drop unused sm_name parameter from show_fetch_remotes()\n\nThis parameter has not been used since the function was introduced in\n8c8195e9c3 (submodule--helper: introduce add-clone subcommand,\n2021-07-10).\n\nSigned-off-by: Jeff King \n---\n builtin/submodule--helper.c | 5 ++---\n 1 file changed, 2 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex ed4a50c78e..1e65ff599e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2782,7 +2782,7 @@ struct add_data {\n };\n #define ADD_DATA_INIT { .depth = -1 }\n \n-static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n+static void show_fetch_remotes(FILE *output, const char *git_dir_path)\n {\n \tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n \tstruct strbuf sb_remote_out = STRBUF_INIT;\n@@ -2833,8 +2833,7 @@ static int add_submodule(const struct add_data *add_data)\n \t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n \t\t\t\t\t\t \"locally with remote(s):\"),\n \t\t\t\t\tadd_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\tshow_fetch_remotes(stderr, submod_gitdir_path);\n \t\t\t\tfree(submod_gitdir_path);\n \t\t\t\tdie(_(\"If you want to reuse this local git \"\n \t\t\t\t \"directory instead of cloning again from\\n\"\n-- \n2.32.0.784.g92e169d3d7\n\n"},{"id":"431060","messageId":"E30F287A-0E19-45CD-8CA7-1FDA4DF20C61@gmail.com","threadId":"56057","inReplyTo":"YPqkHs47VDFBNZ0Z@coredump.intra.peff.net","subject":"Re: [PATCH] submodule: drop unused sm_name parameter from show_fetch_remotes()","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-23T17:12:19Z","receivedAt":"2021-07-23T17:12:35Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 23-Jul-2021, at 16:42, Jeff King wrote:\n> \n> On Sat, Jul 10, 2021 at 01:18:01PM +0530, Atharva Raykar wrote:\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;\n>> +\t\tchar *line = sb_remote_out.buf;\n>> +\t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n>> +\t\t\tsize_t len = next_line - line;\n>> +\t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n>> +\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n>> +\t\t\tline = next_line + 1;\n>> +\t\t}\n>> +\t}\n>> +\n>> +\tstrbuf_release(&sb_remote_out);\n>> +}\n> \n> The sm_name parameter is not used here. I don't think it's a bug; we\n> just don't need it (there's a message that mentions the name, but it\n> happens right before we call the function). Maybe this should go on top\n> of ar/submodule-add?\n> \n> -- >8 --\n> Subject: submodule: drop unused sm_name parameter from show_fetch_remotes()\n> \n> This parameter has not been used since the function was introduced in\n> 8c8195e9c3 (submodule--helper: introduce add-clone subcommand,\n> 2021-07-10).\n> \n> Signed-off-by: Jeff King \n> ---\n> builtin/submodule--helper.c | 5 ++---\n> 1 file changed, 2 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index ed4a50c78e..1e65ff599e 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2782,7 +2782,7 @@ struct add_data {\n> };\n> #define ADD_DATA_INIT { .depth = -1 }\n> \n> -static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n> +static void show_fetch_remotes(FILE *output, const char *git_dir_path)\n> {\n> \tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n> \tstruct strbuf sb_remote_out = STRBUF_INIT;\n> @@ -2833,8 +2833,7 @@ static int add_submodule(const struct add_data *add_data)\n> \t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n> \t\t\t\t\t\t \"locally with remote(s):\"),\n> \t\t\t\t\tadd_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\tshow_fetch_remotes(stderr, submod_gitdir_path);\n> \t\t\t\tfree(submod_gitdir_path);\n> \t\t\t\tdie(_(\"If you want to reuse this local git \"\n> \t\t\t\t \"directory instead of cloning again from\\n\"\n> -- \n> 2.32.0.784.g92e169d3d7\n> \n\nYes, this is definitely an oversight on my part, and it looks like this\ntopic has already made it to 'next'.\n\nThanks for the fix.\n\n"},{"id":"431222","messageId":"xmqqzgu8j3t7.fsf@gitster.g","threadId":"56057","inReplyTo":"E30F287A-0E19-45CD-8CA7-1FDA4DF20C61@gmail.com","subject":"Re: [PATCH] submodule: drop unused sm_name parameter from show_fetch_remotes()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-26T19:03:16Z","receivedAt":"2021-07-26T19:03:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\n\n> Yes, this is definitely an oversight on my part, and it looks like this\n> topic has already made it to 'next'.\n>\n> Thanks for the fix.\n\nThanks, both.\n\n\n"},{"id":"432127","messageId":"20210805192803.679948-1-kaartic.sivaraam@gmail.com","threadId":"56057","inReplyTo":"20210710074801.19917-5-raykar.ath@gmail.com","subject":"[PATCH] submodule--helper: fix incorrect newlines in an error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-08-05T19:28:03Z","receivedAt":"2021-08-05T19:28:39Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"A refactoring[1] done as part of the recent conversion of\n'git submodule add' to builtin, changed the error message\nshown when a Git directory already exists locally for a submodule\nname. Before the refactoring, the error used to appear like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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 --- END OF OUTPUT ---\n\nAfter the refactoring the error started appearing like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub\n fatal: If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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\n --- END OF OUTPUT ---\n\nAs one could observe the remote information is printed along with the\nfirst line rather than on its own line. Also, there's an additional\nnewline following output.\n\nMake the error message consistent with the error message that used to be\nprinted before the refactoring.\n\n[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n\nSigned-off-by: Kaartic Sivaraam \n---\n\nEven with this patch, the error message is still not fully consistent with the one that\nused to be printed before the refactoring. Here's the diff:\n\n3c3\n< If you want to reuse this local git directory instead of cloning again from\n---\n> fatal: If you want to reuse this local git directory instead of cloning again from\n6c6\n< or you are unsure what this means choose another name with the '--name' option.\n---\n> or if you are unsure what this means, choose another name with the '--name' option.\n\n\nThe first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix\nmade sense in other cases, I wonder if it's helpful in this case as the message being\nprinted is an informative one. Should we avoid using 'die' to print this message?\n\nThe second part of the diff shows that there's some small grammatcial tweaks in the last\nline. While I appreciate the intention, I'm not very sure if this change is a strict\nimprovement. I wonder about this as the original sounded good enough to me and thus it\nfeels like the change in message is triggering unnecesssary translation work. Should\nwe avoid the change? Or does it actually seem like an improvement to the message?\n\n\nbuiltin/submodule--helper.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 3cbde305f3..560be07091 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2824,7 +2824,7 @@ static int add_submodule(const struct add_data *add_data)\n \t\tif (is_directory(submod_gitdir_path)) {\n \t\t\tif (!add_data->force) {\n \t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n-\t\t\t\t\t\t \"locally with remote(s):\"),\n+\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n \t\t\t\t\tadd_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@@ -2835,7 +2835,7 @@ static int add_submodule(const struct add_data *add_data)\n \t\t\t\t \"use the '--force' option. If the local git \"\n \t\t\t\t \"directory is not the correct repo\\n\"\n \t\t\t\t \"or if you are unsure what this means, choose \"\n-\t\t\t\t \"another name with the '--name' option.\\n\"),\n+\t\t\t\t \"another name with the '--name' option.\"),\n \t\t\t\t add_data->realrepo);\n \t\t\t} else {\n \t\t\t\tprintf(_(\"Reactivating local git directory for \"\n-- \n2.32.0.385.g8c8534732c.dirty\n\n"},{"id":"432153","messageId":"m28s1fuluy.fsf@gmail.com","threadId":"56057","inReplyTo":"20210805192803.679948-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH] submodule--helper: fix incorrect newlines in an error message","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-06T06:29:41Z","receivedAt":"2021-08-06T06:29:50Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\nKaartic Sivaraam writes:\n\n> A refactoring[1] done as part of the recent conversion of\n> 'git submodule add' to builtin, changed the error message\n> shown when a Git directory already exists locally for a submodule\n> name. Before the refactoring, the error used to appear like so:\n>\n> [...]\n>\n> As one could observe the remote information is printed along with the\n> first line rather than on its own line. Also, there's an additional\n> newline following output.\n>\n> Make the error message consistent with the error message that used to be\n> printed before the refactoring.\n\nThanks for catching this and sending a patch!\n\n> [1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n>\n> Signed-off-by: Kaartic Sivaraam \n> ---\n>\n> Even with this patch, the error message is still not fully consistent with the one that\n> used to be printed before the refactoring. Here's the diff:\n>\n> 3c3\n> < If you want to reuse this local git directory instead of cloning again from\n> ---\n>> fatal: If you want to reuse this local git directory instead of cloning again from\n> 6c6\n> < or you are unsure what this means choose another name with the '--name' option.\n> ---\n>> or if you are unsure what this means, choose another name with the '--name' option.\n>\n>\n> The first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix\n> made sense in other cases, I wonder if it's helpful in this case as the message being\n> printed is an informative one. Should we avoid using 'die' to print this message?\n\nI had initially implemented that message as an fprintf() with return for\nthe same reason, but Junio suggested we die() instead to keep the\nconversion more faithful to the original [1].\n\nAlthough now that I think of it, it feels like a tradeoff between\nfaithfulness to the original code and faithfulness to the original\nbehaviour. I also think this change is fairly inconsequential and easily\nreversible so I am fine with it being done either way.\n\n[1] https://lore.kernel.org/git/xmqqk0n03k84.fsf@gitster.g/\n\n> The second part of the diff shows that there's some small grammatcial tweaks in the last\n> line. While I appreciate the intention, I'm not very sure if this change is a strict\n> improvement. I wonder about this as the original sounded good enough to me and thus it\n> feels like the change in message is triggering unnecesssary translation work. Should\n> we avoid the change? Or does it actually seem like an improvement to the message?\n\nI don't think that extra 'if' was intended. I think it's better to avoid\nthe change I inadvertently introduced.\n\n> builtin/submodule--helper.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 3cbde305f3..560be07091 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2824,7 +2824,7 @@ static int add_submodule(const struct add_data *add_data)\n> \t\tif (is_directory(submod_gitdir_path)) {\n> \t\t\tif (!add_data->force) {\n> \t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n> -\t\t\t\t\t\t \"locally with remote(s):\"),\n> +\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n> \t\t\t\t\tadd_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> @@ -2835,7 +2835,7 @@ static int add_submodule(const struct add_data *add_data)\n> \t\t\t\t \"use the '--force' option. If the local git \"\n> \t\t\t\t \"directory is not the correct repo\\n\"\n> \t\t\t\t \"or if you are unsure what this means, choose \"\n> -\t\t\t\t \"another name with the '--name' option.\\n\"),\n> +\t\t\t\t \"another name with the '--name' option.\"),\n> \t\t\t\t add_data->realrepo);\n> \t\t\t} else {\n> \t\t\t\tprintf(_(\"Reactivating local git directory for \"\n\n\n---\nAtharva Raykar\nಅಥರ್ವ ರಾಯ್ಕರ್\nअथर्व रायकर\n"},{"id":"432194","messageId":"1f7c1d28-4482-b6db-17f0-edbc6934acda@gmail.com","threadId":"56057","inReplyTo":"m28s1fuluy.fsf@gmail.com","subject":"Re: [PATCH] submodule--helper: fix incorrect newlines in an error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-08-06T19:07:55Z","receivedAt":"2021-08-06T19:08:08Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 06/08/21 11:59 am, Atharva Raykar wrote:\n> \n> Kaartic Sivaraam writes:\n> \n>> A refactoring[1] done as part of the recent conversion of\n>> 'git submodule add' to builtin, changed the error message\n>> shown when a Git directory already exists locally for a submodule\n>> name. Before the refactoring, the error used to appear like so:\n>>\n>> [...]\n>>\n>> As one could observe the remote information is printed along with the\n>> first line rather than on its own line. Also, there's an additional\n>> newline following output.\n>>\n>> Make the error message consistent with the error message that used to be\n>> printed before the refactoring.\n> \n> Thanks for catching this and sending a patch!\n> \n>> [1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n>>\n>> Signed-off-by: Kaartic Sivaraam \n>> ---\n>>\n>> Even with this patch, the error message is still not fully consistent with the one that\n>> used to be printed before the refactoring. Here's the diff:\n>>\n>> 3c3\n>> < If you want to reuse this local git directory instead of cloning again from\n>> ---\n>>> fatal: If you want to reuse this local git directory instead of cloning again from\n>> 6c6\n>> < or you are unsure what this means choose another name with the '--name' option.\n>> ---\n>>> or if you are unsure what this means, choose another name with the '--name' option.\n>>\n>>\n>> The first part shows that it is additionally prefixed with 'fatal: '. While the 'fatal :' prefix\n>> made sense in other cases, I wonder if it's helpful in this case as the message being\n>> printed is an informative one. Should we avoid using 'die' to print this message?\n> \n> I had initially implemented that message as an fprintf() with return for\n> the same reason, but Junio suggested we die() instead to keep the\n> conversion more faithful to the original [1].\n> \n> Although now that I think of it, it feels like a tradeoff between\n> faithfulness to the original code and faithfulness to the original\n> behaviour. I also think this change is fairly inconsequential and easily\n> reversible so I am fine with it being done either way.\n> \n> [1] https://lore.kernel.org/git/xmqqk0n03k84.fsf@gitster.g/\n> \n\nI remember this discussion. But ...\n\n A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n fatal: If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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\n... I'm not able to understand how the 'fatal: ' prefix in the _middle_ of this\ninformation message would help the reader in any way :-/\n\nIf it had occurred at the beginning of the message that would've been different\nand likely useful too. I'll try sending a re-roll with a new change that moves\nthe fatal to the beginning of the message and see how it goes.\n\n>> The second part of the diff shows that there's some small grammatcial tweaks in the last\n>> line. While I appreciate the intention, I'm not very sure if this change is a strict\n>> improvement. I wonder about this as the original sounded good enough to me and thus it\n>> feels like the change in message is triggering unnecesssary translation work. Should\n>> we avoid the change? Or does it actually seem like an improvement to the message?\n> \n> I don't think that extra 'if' was intended. I think it's better to avoid\n> the change I inadvertently introduced.\n> \n\nI guess I spoke too soon. Regardless of the additional \"... if ...\" in the message,\nthe conversion might introduce translation work as the variable substitution varies\nbetween shell and C. There isn't much we could do about this. I'm not sure if\ntranslators use any strategy to avoid redundant work during conversions like these.\nJiang Xin (Cc-ed) might be able to shed some light.\n\nThat said, I'll still revert the extra 'if' as you say it was introduced inadvertently.\n\n-- \nSivaraam\n"},{"id":"436318","messageId":"20210918193116.310575-1-kaartic.sivaraam@gmail.com","threadId":"56057","inReplyTo":"20210805192803.679948-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2 0/1] submodule: corret an incorrectly formatted error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-09-18T19:31:15Z","receivedAt":"2021-09-18T19:31:50Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi all,\n\nFinally got a chance to send the v2 of this one.\n\nChanges in v2:\n\n- Removed the 'if' from the message as Atharva mentioned that it was\n added inadvertently\n\n- Moved 'fatal:' prefix from middle of the message to the beginning.\n\n- Expanded the commit message to also mention the output after the change\n\nFor reference, the v1 could be found here:\n\n https://public-inbox.org/git/20210805192803.679948-1-kaartic.sivaraam@gmail.com/\n\n... and the range-diff against v1 could be found below. Reviews appreciated.\n\n--\nSivaraam\n\n\nKaartic Sivaraam (1):\n submodule--helper: fix incorrect newlines in an error message\n\n builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------\n 1 file changed, 22 insertions(+), 14 deletions(-)\n\nRange-diff against v1:\n1: c00617bc03 ! 1: c6daed7a92 submodule--helper: fix incorrect newlines in an error message\n @@ Commit message\n \n After the refactoring the error started appearing like so:\n \n - --- START OF OUTPUT ---\n + --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub\n fatal: If you want to reuse this local git directory instead of cloning again from\n @@ Commit message\n Make the error message consistent with the error message that used to be\n printed before the refactoring.\n \n + This also moves the 'fatal:' prefix that appears in the middle of the\n + error message to the first line as it would more appropriate to have\n + it in the first line. The output after the change would look like:\n +\n + --- START OF OUTPUT ---\n + $ git submodule add ../sub/ subm\n + fatal: A git directory for 'subm' is found locally with remote(s):\n + origin /me/git-repos-for-test/sub\n + If you want to reuse this local git directory instead of cloning again from\n + /me/git-repos-for-test/sub\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 + --- END OF OUTPUT ---\n +\n [1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n \n ## builtin/submodule--helper.c ##\n +@@ builtin/submodule--helper.c: struct add_data {\n + };\n + #define ADD_DATA_INIT { .depth = -1 }\n + \n +-static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n ++static void show_fetch_remotes(struct strbuf *msg, 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 +@@ builtin/submodule--helper.c: static void show_fetch_remotes(FILE *output, const char *sm_name, const char *gi\n + \t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n + \t\t\tsize_t len = next_line - line;\n + \t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n +-\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n ++\t\t\t\tstrbuf_addf(msg, \" %.*s\\n\", (int)len, line);\n + \t\t\tline = next_line + 1;\n + \t\t}\n + \t}\n @@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)\n + \n \t\tif (is_directory(submod_gitdir_path)) {\n \t\t\tif (!add_data->force) {\n - \t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n +-\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n -\t\t\t\t\t\t \"locally with remote(s):\"),\n -+\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n - \t\t\t\t\tadd_data->sm_name);\n - \t\t\t\tshow_fetch_remotes(stderr, add_data->sm_name,\n +-\t\t\t\t\tadd_data->sm_name);\n +-\t\t\t\tshow_fetch_remotes(stderr, add_data->sm_name,\n ++\t\t\t\tstruct strbuf msg = STRBUF_INIT;\n ++\t\t\t\tchar *die_msg;\n ++\n ++\t\t\t\tstrbuf_addf(&msg, _(\"A git directory for '%s' is found \"\n ++\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n ++\t\t\t\t\t add_data->sm_name);\n ++\n ++\t\t\t\tshow_fetch_remotes(&msg, add_data->sm_name,\n \t\t\t\t\t\t submod_gitdir_path);\n -@@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add_data)\n - \t\t\t\t \"use the '--force' option. If the local git \"\n - \t\t\t\t \"directory is not the correct repo\\n\"\n - \t\t\t\t \"or if you are unsure what this means, choose \"\n + \t\t\t\tfree(submod_gitdir_path);\n +-\t\t\t\tdie(_(\"If you want to reuse this local git \"\n +-\t\t\t\t \"directory instead of cloning again from\\n\"\n +-\t\t\t\t \" %s\\n\"\n +-\t\t\t\t \"use the '--force' option. If the local git \"\n +-\t\t\t\t \"directory is not the correct repo\\n\"\n +-\t\t\t\t \"or if you are unsure what this means, choose \"\n -\t\t\t\t \"another name with the '--name' option.\\n\"),\n -+\t\t\t\t \"another name with the '--name' option.\"),\n - \t\t\t\t add_data->realrepo);\n +-\t\t\t\t add_data->realrepo);\n ++\n ++\t\t\t\tstrbuf_addf(&msg, _(\"If you want to reuse this local git \"\n ++\t\t\t\t\t\t \"directory instead of cloning again from\\n\"\n ++\t\t\t\t\t\t \" %s\\n\"\n ++\t\t\t\t\t\t \"use the '--force' option. If the local git \"\n ++\t\t\t\t\t\t \"directory is not the correct repo\\n\"\n ++\t\t\t\t\t\t \"or you are unsure what this means choose \"\n ++\t\t\t\t\t\t \"another name with the '--name' option.\"),\n ++\t\t\t\t\t add_data->realrepo);\n ++\n ++\t\t\t\tdie_msg = strbuf_detach(&msg, NULL);\n ++\t\t\t\tdie(\"%s\", die_msg);\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-- \n2.32.0.385.gc00617bc03.dirty\n\n"},{"id":"436319","messageId":"20210918193116.310575-2-kaartic.sivaraam@gmail.com","threadId":"56057","inReplyTo":"20210918193116.310575-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v2 1/1] submodule--helper: fix incorrect newlines in an error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-09-18T19:31:16Z","receivedAt":"2021-09-18T19:31:58Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"A refactoring[1] done as part of the recent conversion of\n'git submodule add' to builtin, changed the error message\nshown when a Git directory already exists locally for a submodule\nname. Before the refactoring, the error used to appear like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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 --- END OF OUTPUT ---\n\nAfter the refactoring the error started appearing like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub\n fatal: If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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\n --- END OF OUTPUT ---\n\nAs one could observe the remote information is printed along with the\nfirst line rather than on its own line. Also, there's an additional\nnewline following output.\n\nMake the error message consistent with the error message that used to be\nprinted before the refactoring.\n\nThis also moves the 'fatal:' prefix that appears in the middle of the\nerror message to the first line as it would more appropriate to have\nit in the first line. The output after the change would look like:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n fatal: A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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 --- END OF OUTPUT ---\n\n[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n\nSigned-off-by: Kaartic Sivaraam \n---\n builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------\n 1 file changed, 22 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 414fcb63ea..236da214c6 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2775,7 +2775,7 @@ struct add_data {\n };\n #define ADD_DATA_INIT { .depth = -1 }\n \n-static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n+static void show_fetch_remotes(struct strbuf *msg, 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@@ -2791,7 +2791,7 @@ static void show_fetch_remotes(FILE *output, const char *sm_name, const char *gi\n \t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n \t\t\tsize_t len = next_line - line;\n \t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n-\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n+\t\t\t\tstrbuf_addf(msg, \" %.*s\\n\", (int)len, line);\n \t\t\tline = next_line + 1;\n \t\t}\n \t}\n@@ -2823,20 +2823,28 @@ static int add_submodule(const struct add_data *add_data)\n \n \t\tif (is_directory(submod_gitdir_path)) {\n \t\t\tif (!add_data->force) {\n-\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n-\t\t\t\t\t\t \"locally with remote(s):\"),\n-\t\t\t\t\tadd_data->sm_name);\n-\t\t\t\tshow_fetch_remotes(stderr, add_data->sm_name,\n+\t\t\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\t\t\tchar *die_msg;\n+\n+\t\t\t\tstrbuf_addf(&msg, _(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n+\t\t\t\t\t add_data->sm_name);\n+\n+\t\t\t\tshow_fetch_remotes(&msg, add_data->sm_name,\n \t\t\t\t\t\t submod_gitdir_path);\n \t\t\t\tfree(submod_gitdir_path);\n-\t\t\t\tdie(_(\"If you want to reuse this local git \"\n-\t\t\t\t \"directory instead of cloning again from\\n\"\n-\t\t\t\t \" %s\\n\"\n-\t\t\t\t \"use the '--force' option. If the local git \"\n-\t\t\t\t \"directory is not the correct repo\\n\"\n-\t\t\t\t \"or if you are unsure what this means, choose \"\n-\t\t\t\t \"another name with the '--name' option.\\n\"),\n-\t\t\t\t add_data->realrepo);\n+\n+\t\t\t\tstrbuf_addf(&msg, _(\"If you want to reuse this local git \"\n+\t\t\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t\t\t \" %s\\n\"\n+\t\t\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t\t\t \"or you are unsure what this means choose \"\n+\t\t\t\t\t\t \"another name with the '--name' option.\"),\n+\t\t\t\t\t add_data->realrepo);\n+\n+\t\t\t\tdie_msg = strbuf_detach(&msg, NULL);\n+\t\t\t\tdie(\"%s\", die_msg);\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-- \n2.32.0.385.gc00617bc03.dirty\n\n"},{"id":"436447","messageId":"xmqqzgs7azlq.fsf@gitster.g","threadId":"56057","inReplyTo":"20210918193116.310575-2-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v2 1/1] submodule--helper: fix incorrect newlines in an error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-20T18:09:05Z","receivedAt":"2021-09-20T18:11:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> A refactoring[1] done as part of the recent conversion of\n> 'git submodule add' to builtin, changed the error message\n> shown when a Git directory already exists locally for a submodule\n> name. Before the refactoring, the error used to appear like so:\n> ...\n> As one could observe the remote information is printed along with the\n> first line rather than on its own line. Also, there's an additional\n> newline following output.\n>\n> Make the error message consistent with the error message that used to be\n> printed before the refactoring.\n\nMakes sense. Atharva, an ack?\n"},{"id":"436644","messageId":"m27df9lvm1.fsf@gmail.com","threadId":"56057","inReplyTo":"20210918193116.310575-2-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v2 1/1] submodule--helper: fix incorrect newlines in an error message","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-09-21T16:47:51Z","receivedAt":"2021-09-21T16:52:13Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\nKaartic Sivaraam writes:\n\n> A refactoring[1] done as part of the recent conversion of\n> 'git submodule add' to builtin, changed the error message\n> shown when a Git directory already exists locally for a submodule\n> name. Before the refactoring, the error used to appear like so:\n>\n> --- START OF OUTPUT ---\n> $ git submodule add ../sub/ subm\n> A git directory for 'subm' is found locally with remote(s):\n> origin /me/git-repos-for-test/sub\n> If you want to reuse this local git directory instead of cloning again from\n> /me/git-repos-for-test/sub\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> --- END OF OUTPUT ---\n>\n> After the refactoring the error started appearing like so:\n>\n> --- START OF OUTPUT ---\n> $ git submodule add ../sub/ subm\n> A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub\n> fatal: If you want to reuse this local git directory instead of cloning again from\n> /me/git-repos-for-test/sub\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>\n> --- END OF OUTPUT ---\n>\n> As one could observe the remote information is printed along with the\n> first line rather than on its own line. Also, there's an additional\n> newline following output.\n>\n> Make the error message consistent with the error message that used to be\n> printed before the refactoring.\n>\n> This also moves the 'fatal:' prefix that appears in the middle of the\n> error message to the first line as it would more appropriate to have\n> it in the first line. The output after the change would look like:\n>\n> --- START OF OUTPUT ---\n> $ git submodule add ../sub/ subm\n> fatal: A git directory for 'subm' is found locally with remote(s):\n> origin /me/git-repos-for-test/sub\n> If you want to reuse this local git directory instead of cloning again from\n> /me/git-repos-for-test/sub\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> --- END OF OUTPUT ---\n>\n> [1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n>\n> Signed-off-by: Kaartic Sivaraam \n> ---\n> builtin/submodule--helper.c | 36 ++++++++++++++++++++++--------------\n> 1 file changed, 22 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 414fcb63ea..236da214c6 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2775,7 +2775,7 @@ struct add_data {\n> };\n> #define ADD_DATA_INIT { .depth = -1 }\n>\n> -static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n> +static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)\n\nI like the change from using a strbuf instead of passing the output\nstream and printing to it. But maybe we should rename this function, now\nthat it doesn't really 'show' anything? Probably something like\n'append_fetch_remotes()'?\n\n> {\n> \tstruct child_process cp_remote = CHILD_PROCESS_INIT;\n> \tstruct strbuf sb_remote_out = STRBUF_INIT;\n> @@ -2791,7 +2791,7 @@ static void show_fetch_remotes(FILE *output, const char *sm_name, const char *gi\n> \t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n> \t\t\tsize_t len = next_line - line;\n> \t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n> -\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n> +\t\t\t\tstrbuf_addf(msg, \" %.*s\\n\", (int)len, line);\n> \t\t\tline = next_line + 1;\n> \t\t}\n> \t}\n> @@ -2823,20 +2823,28 @@ static int add_submodule(const struct add_data *add_data)\n>\n> \t\tif (is_directory(submod_gitdir_path)) {\n> \t\t\tif (!add_data->force) {\n> -\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n> -\t\t\t\t\t\t \"locally with remote(s):\"),\n> -\t\t\t\t\tadd_data->sm_name);\n> -\t\t\t\tshow_fetch_remotes(stderr, add_data->sm_name,\n> +\t\t\t\tstruct strbuf msg = STRBUF_INIT;\n> +\t\t\t\tchar *die_msg;\n> +\n> +\t\t\t\tstrbuf_addf(&msg, _(\"A git directory for '%s' is found \"\n> +\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n> +\t\t\t\t\t add_data->sm_name);\n> +\n> +\t\t\t\tshow_fetch_remotes(&msg, add_data->sm_name,\n> \t\t\t\t\t\t submod_gitdir_path);\n> \t\t\t\tfree(submod_gitdir_path);\n> -\t\t\t\tdie(_(\"If you want to reuse this local git \"\n> -\t\t\t\t \"directory instead of cloning again from\\n\"\n> -\t\t\t\t \" %s\\n\"\n> -\t\t\t\t \"use the '--force' option. If the local git \"\n> -\t\t\t\t \"directory is not the correct repo\\n\"\n> -\t\t\t\t \"or if you are unsure what this means, choose \"\n> -\t\t\t\t \"another name with the '--name' option.\\n\"),\n> -\t\t\t\t add_data->realrepo);\n> +\n> +\t\t\t\tstrbuf_addf(&msg, _(\"If you want to reuse this local git \"\n> +\t\t\t\t\t\t \"directory instead of cloning again from\\n\"\n> +\t\t\t\t\t\t \" %s\\n\"\n> +\t\t\t\t\t\t \"use the '--force' option. If the local git \"\n> +\t\t\t\t\t\t \"directory is not the correct repo\\n\"\n> +\t\t\t\t\t\t \"or you are unsure what this means choose \"\n> +\t\t\t\t\t\t \"another name with the '--name' option.\"),\n> +\t\t\t\t\t add_data->realrepo);\n> +\n> +\t\t\t\tdie_msg = strbuf_detach(&msg, NULL);\n> +\t\t\t\tdie(\"%s\", die_msg);\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\nOther than that this patch is an improvement. Thanks for fixing this!\n"},{"id":"436645","messageId":"m24kadlvef.fsf@gmail.com","threadId":"56057","inReplyTo":"xmqqzgs7azlq.fsf@gitster.g","subject":"Re: [PATCH v2 1/1] submodule--helper: fix incorrect newlines in an error message","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-09-21T16:52:21Z","receivedAt":"2021-09-21T16:56:48Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"\nJunio C Hamano writes:\n\n> Kaartic Sivaraam writes:\n>\n>> A refactoring[1] done as part of the recent conversion of\n>> 'git submodule add' to builtin, changed the error message\n>> shown when a Git directory already exists locally for a submodule\n>> name. Before the refactoring, the error used to appear like so:\n>> ...\n>> As one could observe the remote information is printed along with the\n>> first line rather than on its own line. Also, there's an additional\n>> newline following output.\n>>\n>> Make the error message consistent with the error message that used to be\n>> printed before the refactoring.\n>\n> Makes sense. Atharva, an ack?\n\nSorry for the delay in looking into this, I just left a comment. After\nthat minor nit is addressed, it's an ack for me :-)\n"},{"id":"439448","messageId":"20211023125722.125933-1-kaartic.sivaraam@gmail.com","threadId":"56057","inReplyTo":"m27df9lvm1.fsf@gmail.com","subject":"[PATCH v3 0/1] submodule: correct an incorrectly formatted error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-10-23T12:57:21Z","receivedAt":"2021-10-23T12:58:14Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi Atharva,\n\nSorry for the delay in sending this. Got held up with other work.\n\nOn 21/09/21 10:17 pm, Atharva Raykar wrote:\n>>\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 414fcb63ea..236da214c6 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2775,7 +2775,7 @@ struct add_data {\n>> };\n>> #define ADD_DATA_INIT { .depth = -1 }\n>>\n>> -static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n>> +static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)\n> \n> I like the change from using a strbuf instead of passing the output\n> stream and printing to it. But maybe we should rename this function, now\n> that it doesn't really 'show' anything? Probably something like\n> 'append_fetch_remotes()'?\n\nThat's a good point. I've taken your suggestion into account in this v3.\n\nFind the details of the v3 of this patch below.\n\nChanges since v2:\n\n- Renamed the helper function name to be more appropriate, upon suggestion.\n\nAlso, I rebased my local branch over the latest 'master'. So, this should apply\ncleanly over 'master'.\n\nFor reference, the v2 could be found here:\n\n https://public-inbox.org/git/20210918193116.310575-1-kaartic.sivaraam@gmail.com/\n\n... and the range-diff against v2 could be found below.\n\n--\nSivaraam\n\n\nKaartic Sivaraam (1):\n submodule--helper: fix incorrect newlines in an error message\n\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++--------------\n 1 file changed, 23 insertions(+), 14 deletions(-)\n\nRange-diff against v2:\n1: 95cbe38be3 ! 1: 7c4887ccf5 submodule--helper: fix incorrect newlines in an error message\n @@ builtin/submodule--helper.c: struct add_data {\n #define ADD_DATA_INIT { .depth = -1 }\n \n -static void show_fetch_remotes(FILE *output, const char *git_dir_path)\n -+static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)\n ++static void append_fetch_remotes(struct strbuf *msg, 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 @@ builtin/submodule--helper.c: static int add_submodule(const struct add_data *add\n +\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n +\t\t\t\t\t add_data->sm_name);\n +\n -+\t\t\t\tshow_fetch_remotes(&msg, add_data->sm_name,\n -+\t\t\t\t\t\t submod_gitdir_path);\n ++\t\t\t\tappend_fetch_remotes(&msg, add_data->sm_name,\n ++\t\t\t\t\t\t submod_gitdir_path);\n \t\t\t\tfree(submod_gitdir_path);\n -\t\t\t\tdie(_(\"If you want to reuse this local git \"\n -\t\t\t\t \"directory instead of cloning again from\\n\"\n-- \n2.33.1.1058.gd3b4e01def\n\n"},{"id":"439449","messageId":"20211023125722.125933-2-kaartic.sivaraam@gmail.com","threadId":"56057","inReplyTo":"20211023125722.125933-1-kaartic.sivaraam@gmail.com","subject":"[PATCH v3 1/1] submodule--helper: fix incorrect newlines in an error message","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-10-23T12:57:22Z","receivedAt":"2021-10-23T12:58:16Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"A refactoring[1] done as part of the recent conversion of\n'git submodule add' to builtin, changed the error message\nshown when a Git directory already exists locally for a submodule\nname. Before the refactoring, the error used to appear like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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 --- END OF OUTPUT ---\n\nAfter the refactoring the error started appearing like so:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n A git directory for 'subm' is found locally with remote(s): origin /me/git-repos-for-test/sub\n fatal: If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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\n --- END OF OUTPUT ---\n\nAs one could observe the remote information is printed along with the\nfirst line rather than on its own line. Also, there's an additional\nnewline following output.\n\nMake the error message consistent with the error message that used to be\nprinted before the refactoring.\n\nThis also moves the 'fatal:' prefix that appears in the middle of the\nerror message to the first line as it would more appropriate to have\nit in the first line. The output after the change would look like:\n\n --- START OF OUTPUT ---\n $ git submodule add ../sub/ subm\n fatal: A git directory for 'subm' is found locally with remote(s):\n origin /me/git-repos-for-test/sub\n If you want to reuse this local git directory instead of cloning again from\n /me/git-repos-for-test/sub\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 --- END OF OUTPUT ---\n\n[1]: https://lore.kernel.org/git/20210710074801.19917-5-raykar.ath@gmail.com/#t\n\nSigned-off-by: Kaartic Sivaraam \n---\n builtin/submodule--helper.c | 37 +++++++++++++++++++++++--------------\n 1 file changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 6298cbdd4e..37661e2789 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2999,7 +2999,7 @@ struct add_data {\n };\n #define ADD_DATA_INIT { .depth = -1 }\n \n-static void show_fetch_remotes(FILE *output, const char *git_dir_path)\n+static void append_fetch_remotes(struct strbuf *msg, 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@@ -3015,7 +3015,7 @@ static void show_fetch_remotes(FILE *output, const char *git_dir_path)\n \t\twhile ((next_line = strchr(line, '\\n')) != NULL) {\n \t\t\tsize_t len = next_line - line;\n \t\t\tif (strip_suffix_mem(line, &len, \" (fetch)\"))\n-\t\t\t\tfprintf(output, \" %.*s\\n\", (int)len, line);\n+\t\t\t\tstrbuf_addf(msg, \" %.*s\\n\", (int)len, line);\n \t\t\tline = next_line + 1;\n \t\t}\n \t}\n@@ -3047,19 +3047,28 @@ static int add_submodule(const struct add_data *add_data)\n \n \t\tif (is_directory(submod_gitdir_path)) {\n \t\t\tif (!add_data->force) {\n-\t\t\t\tfprintf(stderr, _(\"A git directory for '%s' is found \"\n-\t\t\t\t\t\t \"locally with remote(s):\"),\n-\t\t\t\t\tadd_data->sm_name);\n-\t\t\t\tshow_fetch_remotes(stderr, submod_gitdir_path);\n+\t\t\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\t\t\tchar *die_msg;\n+\n+\t\t\t\tstrbuf_addf(&msg, _(\"A git directory for '%s' is found \"\n+\t\t\t\t\t\t \"locally with remote(s):\\n\"),\n+\t\t\t\t\t add_data->sm_name);\n+\n+\t\t\t\tappend_fetch_remotes(&msg, add_data->sm_name,\n+\t\t\t\t\t\t submod_gitdir_path);\n \t\t\t\tfree(submod_gitdir_path);\n-\t\t\t\tdie(_(\"If you want to reuse this local git \"\n-\t\t\t\t \"directory instead of cloning again from\\n\"\n-\t\t\t\t \" %s\\n\"\n-\t\t\t\t \"use the '--force' option. If the local git \"\n-\t\t\t\t \"directory is not the correct repo\\n\"\n-\t\t\t\t \"or if you are unsure what this means, choose \"\n-\t\t\t\t \"another name with the '--name' option.\\n\"),\n-\t\t\t\t add_data->realrepo);\n+\n+\t\t\t\tstrbuf_addf(&msg, _(\"If you want to reuse this local git \"\n+\t\t\t\t\t\t \"directory instead of cloning again from\\n\"\n+\t\t\t\t\t\t \" %s\\n\"\n+\t\t\t\t\t\t \"use the '--force' option. If the local git \"\n+\t\t\t\t\t\t \"directory is not the correct repo\\n\"\n+\t\t\t\t\t\t \"or you are unsure what this means choose \"\n+\t\t\t\t\t\t \"another name with the '--name' option.\"),\n+\t\t\t\t\t add_data->realrepo);\n+\n+\t\t\t\tdie_msg = strbuf_detach(&msg, NULL);\n+\t\t\t\tdie(\"%s\", die_msg);\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-- \n2.33.1.1058.gd3b4e01def\n\n"},{"id":"439481","messageId":"xmqq7de3555c.fsf@gitster.g","threadId":"56057","inReplyTo":"20211023125722.125933-1-kaartic.sivaraam@gmail.com","subject":"Re: [PATCH v3 0/1] submodule: correct an incorrectly formatted error message","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-10-24T06:05:35Z","receivedAt":"2021-10-24T06:05:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kaartic Sivaraam writes:\n\n> Hi Atharva,\n>\n> Sorry for the delay in sending this. Got held up with other work.\n>\n> On 21/09/21 10:17 pm, Atharva Raykar wrote:\n>>>\n>>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>>> index 414fcb63ea..236da214c6 100644\n>>> --- a/builtin/submodule--helper.c\n>>> +++ b/builtin/submodule--helper.c\n>>> @@ -2775,7 +2775,7 @@ struct add_data {\n>>> };\n>>> #define ADD_DATA_INIT { .depth = -1 }\n>>>\n>>> -static void show_fetch_remotes(FILE *output, const char *sm_name, const char *git_dir_path)\n>>> +static void show_fetch_remotes(struct strbuf *msg, const char *sm_name, const char *git_dir_path)\n>> \n>> I like the change from using a strbuf instead of passing the output\n>> stream and printing to it. But maybe we should rename this function, now\n>> that it doesn't really 'show' anything? Probably something like\n>> 'append_fetch_remotes()'?\n>\n> That's a good point. I've taken your suggestion into account in this v3.\n>\n> Find the details of the v3 of this patch below.\n\nLooking good.\n\nLet's declare victory and merge it down to 'next' and then to\n'master'.\n\nThanks, both. Will replace.\n"}]}