{"thread":{"id":"56146","subject":"[GSoC] [PATCH] submodule--helper: introduce add-config subcommand","startedAt":"2021-07-22T11:22:21Z","lastAt":"2021-08-05T18:25:42Z","messageCount":13,"participants":["Atharva Raykar","Ævar Arnfjörð Bjarmason","Junio C Hamano","Kaartic Sivaraam"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430905","messageId":"20210722112143.97944-1-raykar.ath@gmail.com","threadId":"56146","inReplyTo":null,"subject":"[GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T11:21:43Z","receivedAt":"2021-07-22T11:22:21Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new \"add-config\" subcommand to `git submodule--helper` with the\ngoal of converting part of the shell code in git-submodule.sh related to\n`git submodule add` into C code. This new subcommand sets the\nconfiguration variables of a newly added submodule, by registering the\nurl in local git config, as well as the submodule name and path in the\n.gitmodules file. It also sets 'submodule..active' to \"true\" if\nthe submodule path has not already been covered by any pathspec\nspecified in 'submodule.active'.\n\nThis is meant to be a faithful conversion from shell to C, with only one\nminor change: A warning is emitted if no value is specified in\n'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\nbecause it is an invalid configuration. It would be helpful to let the\nuser know that the pathspec is unset, and the value of\n'submodule..active' might be set to 'true' so that they can\nrectify their configuration and prevent future surprises (especially\ngiven that the latter variable has a higher priority than the former).\n\nThe structure of the conditional to check if we need to set the 'active'\ntoggle looks different from the shell version -- but behaves the same.\nThe change was made to decrease code duplication. A comment has been\nadded to explain that only one value of 'submodule.active' is obtained\nto check if we need to call is_submodule_active() at all.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nThis patch depends on changes introduced in 83913dd5b6 (t7400: test failure to\nadd submodule in tracked path, 2021-06-18), which belongs to the\nar/submodule-add (2021-07-12) series[1].\n\nThis changes in this patch are no different from the one in my first\nsubmodule-add conversion series[2] which has already got some reviews before.\n\n[1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/\n[2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/\n\n builtin/submodule--helper.c | 125 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 28 +-------\n 2 files changed, 126 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 862053c9f2..9658804d24 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n+{\n+\tchar *key;\n+\n+\tif (!is_writing_gitmodules_ok())\n+\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n+\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n+\tconfig_set_in_gitmodules_file_gently(key, value);\n+\tfree(key);\n+}\n+\n+static void configure_added_submodule(struct add_data *add_data)\n+{\n+\tchar *key, *submod_pathspec = NULL;\n+\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n+\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n+\tint pathspec_key_exists, activate = 0;\n+\n+\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n+\tgit_config_set_gently(key, add_data->realrepo);\n+\tfree(key);\n+\n+\tadd_submod.git_cmd = 1;\n+\tstrvec_pushl(&add_submod.args, \"add\",\n+\t\t \"--no-warn-embedded-repo\", NULL);\n+\tif (add_data->force)\n+\t\tstrvec_push(&add_submod.args, \"--force\");\n+\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n+\n+\tif (run_command(&add_submod))\n+\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n+\n+\tconfig_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path);\n+\tconfig_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo);\n+\tif (add_data->branch)\n+\t\tconfig_submodule_in_gitmodules(add_data->sm_name,\n+\t\t\t\t\t \"branch\", add_data->branch);\n+\n+\tadd_gitmodules.git_cmd = 1;\n+\tstrvec_pushl(&add_gitmodules.args,\n+\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n+\n+\tif (run_command(&add_gitmodules))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\t/*\n+\t * NEEDSWORK: In a multi-working-tree world this needs to be\n+\t * set in the per-worktree config.\n+\t */\n+\tpathspec_key_exists = !git_config_get_string(\"submodule.active\",\n+\t\t\t\t\t\t &submod_pathspec);\n+\tif (pathspec_key_exists && !submod_pathspec) {\n+\t\twarning(_(\"The submodule.active configuration exists, but the \"\n+\t\t\t \"pathspec was unset. If the submodule is not already \"\n+\t\t\t \"active, the value of submodule.%s.active will be \"\n+\t\t\t \"be set to 'true'.\"), add_data->sm_name);\n+\t\tactivate = 1;\n+\t}\n+\n+\t/*\n+\t * If submodule.active does not exist, or if the pathspec was unset,\n+\t * we will activate this module unconditionally.\n+\t *\n+\t * Otherwise, we ask is_submodule_active(), which iterates\n+\t * through all the values of 'submodule.active' to determine\n+\t * if this module is already active.\n+\t */\n+\tif (!pathspec_key_exists || activate ||\n+\t !is_submodule_active(the_repository, add_data->sm_path)) {\n+\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t}\n+}\n+\n+static int add_config(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 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 store in \"\n+\t\t\t \"the submodule configuration\")),\n+\t\tOPT_STRING(0, \"url\", &add_data.repo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone submodule from\")),\n+\t\tOPT_STRING(0, \"resolved-url\", &add_data.realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone the submodule from, after it has \"\n+\t\t\t \"been dereferenced relative to parent's url, \"\n+\t\t\t \"in the case where is a relative url\")),\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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add-config \"\n+\t\t \"[--force|-f] [--branch|-b ] \"\n+\t\t \"--url --resolved-url \"\n+\t\t \"--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.force = !!force;\n+\tconfigure_added_submodule(&add_data);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2949,6 +3073,7 @@ static struct cmd_struct commands[] = {\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n \t{\"add-clone\", add_clone, 0},\n+\t{\"add-config\", add_config, 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 053daf3724..f713cb113c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -242,33 +242,7 @@ cmd_add()\n \tfi\n \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-\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-\tif test -n \"$branch\"\n-\tthen\n-\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n-\tfi &&\n-\tgit add --force .gitmodules ||\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-\tif git config --get submodule.active >/dev/null\n-\tthen\n-\t\t# If the submodule being adding isn't already covered by the\n-\t\t# current configured pathspec, set the submodule's active flag\n-\t\tif ! git submodule--helper is-active \"$sm_path\"\n-\t\tthen\n-\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\t\tfi\n-\telse\n-\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\tfi\n+\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n }\n \n #\n-- \n2.32.0\n\n"},{"id":"430908","messageId":"DE4FABF8-574C-4D01-872A-CE13E3975AD0@gmail.com","threadId":"56146","inReplyTo":"20210722112143.97944-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T11:41:11Z","receivedAt":"2021-07-22T11:41:23Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 22-Jul-2021, at 16:51, Atharva Raykar wrote:\n> \n> Add a new \"add-config\" subcommand to `git submodule--helper` with the\n> goal of converting part of the shell code in git-submodule.sh related to\n> `git submodule add` into C code. This new subcommand sets the\n> configuration variables of a newly added submodule, by registering the\n> url in local git config, as well as the submodule name and path in the\n> .gitmodules file. It also sets 'submodule..active' to \"true\" if\n> the submodule path has not already been covered by any pathspec\n> specified in 'submodule.active'.\n> \n> This is meant to be a faithful conversion from shell to C, with only one\n> minor change: A warning is emitted if no value is specified in\n> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n> because it is an invalid configuration. It would be helpful to let the\n> user know that the pathspec is unset, and the value of\n> 'submodule..active' might be set to 'true' so that they can\n> rectify their configuration and prevent future surprises (especially\n> given that the latter variable has a higher priority than the former).\n> \n> The structure of the conditional to check if we need to set the 'active'\n> toggle looks different from the shell version -- but behaves the same.\n> The change was made to decrease code duplication. A comment has been\n> added to explain that only one value of 'submodule.active' is obtained\n> to check if we need to call is_submodule_active() at all.\n> \n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n> ---\n> \n> This patch depends on changes introduced in 83913dd5b6 (t7400: test failure to\n> add submodule in tracked path, 2021-06-18), which belongs to the\n> ar/submodule-add (2021-07-12) series[1].\n> \n> This changes in this patch are no different from the one in my first\n> submodule-add conversion series[2] which has already got some reviews before.\n> \n> [1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/\n> [2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/\n\nI forgot to mention, you can fetch this change through:\ngit fetch https://github.com/tfidfwastaken/git.git submodule-helper-add-config-1\n\n"},{"id":"430910","messageId":"87sg06tvab.fsf@evledraar.gmail.com","threadId":"56146","inReplyTo":"20210722112143.97944-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-22T11:50:14Z","receivedAt":"2021-07-22T11:58:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 22 2021, Atharva Raykar wrote:\n\n> +static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n> +{\n> +\tchar *key;\n> +\n> +\tif (!is_writing_gitmodules_ok())\n> +\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> +\n> +\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n> +\tconfig_set_in_gitmodules_file_gently(key, value);\n> +\tfree(key);\n> +}\n\nJust a small point not per-se to do with this patch, but aren't all\ncallers of config_set_in_gitmodules_file_gently() wanting to prefix\nthigs with \"submodule.\"? Looks like its API could be simplified a bit\nwith that xstrfmt() and free() inside that function.\n\n> +static void configure_added_submodule(struct add_data *add_data)\n> +{\n> +\tchar *key, *submod_pathspec = NULL;\n> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n> +\tint pathspec_key_exists, activate = 0;\n\nUsual style is to have different variables on different lines, unless\nthey're closely related (like \"int i, j\"), so \"char *key;\\n char\n*submod[...]\" in this case.\n\n> +\n> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n> +\tgit_config_set_gently(key, add_data->realrepo);\n> +\tfree(key);\n> +\n> +\tadd_submod.git_cmd = 1;\n> +\tstrvec_pushl(&add_submod.args, \"add\",\n> +\t\t \"--no-warn-embedded-repo\", NULL);\n> +\tif (add_data->force)\n> +\t\tstrvec_push(&add_submod.args, \"--force\");\n> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n> +\n> +\tif (run_command(&add_submod))\n> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n> +\n> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path);\n> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo);\n> +\tif (add_data->branch)\n> +\t\tconfig_submodule_in_gitmodules(add_data->sm_name,\n> +\t\t\t\t\t \"branch\", add_data->branch);\n> +\n> +\tadd_gitmodules.git_cmd = 1;\n> +\tstrvec_pushl(&add_gitmodules.args,\n> +\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n> +\n> +\tif (run_command(&add_gitmodules))\n> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n\nLooks good at a glance.\n\n> +\t/*\n> +\t * NEEDSWORK: In a multi-working-tree world this needs to be\n> +\t * set in the per-worktree config.\n> +\t */\n\nSo should we have a failing test for that scenario, or...? (Update: but\nread ahead...)\n\n> +static int add_config(int argc, const char **argv, const char *prefix)\n> +{\n> +\tint force = 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 store in \"\n> +\t\t\t \"the submodule configuration\")),\n> +\t\tOPT_STRING(0, \"url\", &add_data.repo,\n> +\t\t\t N_(\"string\"),\n> +\t\t\t N_(\"url to clone submodule from\")),\n> +\t\tOPT_STRING(0, \"resolved-url\", &add_data.realrepo,\n> +\t\t\t N_(\"string\"),\n> +\t\t\t N_(\"url to clone the submodule from, after it has \"\n> +\t\t\t \"been dereferenced relative to parent's url, \"\n> +\t\t\t \"in the case where is a relative url\")),\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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n> +\t\t\t PARSE_OPT_NOCOMPLETE),\n> +\t\tOPT_END()\n> +\t};\n> +\n> +\tconst char *const usage[] = {\n> +\t\tN_(\"git submodule--helper add-config \"\n> +\t\t \"[--force|-f] [--branch|-b ] \"\n> +\t\t \"--url --resolved-url \"\n> +\t\t \"--path --name \"),\n> +\t\tNULL\n> +\t};\n\nI'd say consider adding this as a \"static\" earlier in the file, but it's\nan established pattern in this file, so let's keep it.\n\n> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n\nIt's fine to omit it for a helper, but we're being non-pedantic about\nchecking mandatory options here. Would do it in a \"real\" built-in, but\nfor internal use it's fine.\n\n> +\tif (argc != 0)\n\nStyle: if (!argc)\n\n> +\t\tusage_with_options(usage, options);\n> +\n> +\tadd_data.force = !!force;\n> +\tconfigure_added_submodule(&add_data);\n> +\n> +\treturn 0;\n> +}\n> +\n> #define SUPPORT_SUPER_PREFIX (1<<0)\n> \n> struct cmd_struct {\n> @@ -2949,6 +3073,7 @@ static struct cmd_struct commands[] = {\n> \t{\"name\", module_name, 0},\n> \t{\"clone\", module_clone, 0},\n> \t{\"add-clone\", add_clone, 0},\n> +\t{\"add-config\", add_config, 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},\n> diff --git a/git-submodule.sh b/git-submodule.sh\n> index 053daf3724..f713cb113c 100755\n> --- a/git-submodule.sh\n> +++ b/git-submodule.sh\n> @@ -242,33 +242,7 @@ cmd_add()\n> \tfi\n> \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> -\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> -\tif test -n \"$branch\"\n> -\tthen\n> -\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n> -\tfi &&\n> -\tgit add --force .gitmodules ||\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\nAh, this is the NEEDSWORK comment, just copied to the C code...\n\n> -\tif git config --get submodule.active >/dev/null\n> -\tthen\n> -\t\t# If the submodule being adding isn't already covered by the\n> -\t\t# current configured pathspec, set the submodule's active flag\n> -\t\tif ! git submodule--helper is-active \"$sm_path\"\n> -\t\tthen\n> -\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n> -\t\tfi\n> -\telse\n> -\t\tgit config submodule.\"$sm_name\".active \"true\"\n> -\tfi\n> +\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n> }\n> \n\nVery nice to have this simplified.\n\nWould be good to split this very long line across multiple lines\nthough...\n"},{"id":"430930","messageId":"4DEF3E9A-C6FA-4402-8182-D4E618CF2CCC@gmail.com","threadId":"56146","inReplyTo":"87sg06tvab.fsf@evledraar.gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T13:28:38Z","receivedAt":"2021-07-22T13:28:49Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 22-Jul-2021, at 17:20, Ævar Arnfjörð Bjarmason wrote:\n> \n> \n> On Thu, Jul 22 2021, Atharva Raykar wrote:\n> \n>> +static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n>> +{\n>> +\tchar *key;\n>> +\n>> +\tif (!is_writing_gitmodules_ok())\n>> +\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n>> +\n>> +\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n>> +\tconfig_set_in_gitmodules_file_gently(key, value);\n>> +\tfree(key);\n>> +}\n> \n> Just a small point not per-se to do with this patch, but aren't all\n> callers of config_set_in_gitmodules_file_gently() wanting to prefix\n> thigs with \"submodule.\"? Looks like its API could be simplified a bit\n> with that xstrfmt() and free() inside that function.\n\nYes, you are right, all the callers prefix with \"submodule.\". Changing\nthe API to what you suggested, would require some special handling in\n'submodule--helper:module_config()', which will be dead code after the\nwhole series is done, so probably a better time to change it would be\nduring the cleanup after the conversion.\n\n>> +static void configure_added_submodule(struct add_data *add_data)\n>> +{\n>> +\tchar *key, *submod_pathspec = NULL;\n>> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n>> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n>> +\tint pathspec_key_exists, activate = 0;\n> \n> Usual style is to have different variables on different lines, unless\n> they're closely related (like \"int i, j\"), so \"char *key;\\n char\n> *submod[...]\" in this case.\n\nOkay.\n\n>> +\n>> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n>> +\tgit_config_set_gently(key, add_data->realrepo);\n>> +\tfree(key);\n>> +\n>> +\tadd_submod.git_cmd = 1;\n>> +\tstrvec_pushl(&add_submod.args, \"add\",\n>> +\t\t \"--no-warn-embedded-repo\", NULL);\n>> +\tif (add_data->force)\n>> +\t\tstrvec_push(&add_submod.args, \"--force\");\n>> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n>> +\n>> +\tif (run_command(&add_submod))\n>> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n>> +\n>> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path);\n>> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo);\n>> +\tif (add_data->branch)\n>> +\t\tconfig_submodule_in_gitmodules(add_data->sm_name,\n>> +\t\t\t\t\t \"branch\", add_data->branch);\n>> +\n>> +\tadd_gitmodules.git_cmd = 1;\n>> +\tstrvec_pushl(&add_gitmodules.args,\n>> +\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n>> +\n>> +\tif (run_command(&add_gitmodules))\n>> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> \n> Looks good at a glance.\n> \n>> +\t/*\n>> +\t * NEEDSWORK: In a multi-working-tree world this needs to be\n>> +\t * set in the per-worktree config.\n>> +\t */\n> \n> So should we have a failing test for that scenario, or...? (Update: but\n> read ahead...)\n> \n>> +static int add_config(int argc, const char **argv, const char *prefix)\n>> +{\n>> +\tint force = 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 store in \"\n>> +\t\t\t \"the submodule configuration\")),\n>> +\t\tOPT_STRING(0, \"url\", &add_data.repo,\n>> +\t\t\t N_(\"string\"),\n>> +\t\t\t N_(\"url to clone submodule from\")),\n>> +\t\tOPT_STRING(0, \"resolved-url\", &add_data.realrepo,\n>> +\t\t\t N_(\"string\"),\n>> +\t\t\t N_(\"url to clone the submodule from, after it has \"\n>> +\t\t\t \"been dereferenced relative to parent's url, \"\n>> +\t\t\t \"in the case where is a relative url\")),\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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n>> +\t\t\t PARSE_OPT_NOCOMPLETE),\n>> +\t\tOPT_END()\n>> +\t};\n>> +\n>> +\tconst char *const usage[] = {\n>> +\t\tN_(\"git submodule--helper add-config \"\n>> +\t\t \"[--force|-f] [--branch|-b ] \"\n>> +\t\t \"--url --resolved-url \"\n>> +\t\t \"--path --name \"),\n>> +\t\tNULL\n>> +\t};\n> \n> I'd say consider adding this as a \"static\" earlier in the file, but it's\n> an established pattern in this file, so let's keep it.\n> \n>> +\targc = parse_options(argc, argv, prefix, options, usage, 0);\n> \n> It's fine to omit it for a helper, but we're being non-pedantic about\n> checking mandatory options here. Would do it in a \"real\" built-in, but\n> for internal use it's fine.\n> \n>> +\tif (argc != 0)\n> \n> Style: if (!argc)\n\nWill fix.\n\n>> +\t\tusage_with_options(usage, options);\n>> +\n>> +\tadd_data.force = !!force;\n>> +\tconfigure_added_submodule(&add_data);\n>> +\n>> +\treturn 0;\n>> +}\n>> +\n>> #define SUPPORT_SUPER_PREFIX (1<<0)\n>> \n>> struct cmd_struct {\n>> @@ -2949,6 +3073,7 @@ static struct cmd_struct commands[] = {\n>> \t{\"name\", module_name, 0},\n>> \t{\"clone\", module_clone, 0},\n>> \t{\"add-clone\", add_clone, 0},\n>> +\t{\"add-config\", add_config, 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},\n>> diff --git a/git-submodule.sh b/git-submodule.sh\n>> index 053daf3724..f713cb113c 100755\n>> --- a/git-submodule.sh\n>> +++ b/git-submodule.sh\n>> @@ -242,33 +242,7 @@ cmd_add()\n>> \tfi\n>> \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>> -\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>> -\tif test -n \"$branch\"\n>> -\tthen\n>> -\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n>> -\tfi &&\n>> -\tgit add --force .gitmodules ||\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> \n> Ah, this is the NEEDSWORK comment, just copied to the C code...\n> \n>> -\tif git config --get submodule.active >/dev/null\n>> -\tthen\n>> -\t\t# If the submodule being adding isn't already covered by the\n>> -\t\t# current configured pathspec, set the submodule's active flag\n>> -\t\tif ! git submodule--helper is-active \"$sm_path\"\n>> -\t\tthen\n>> -\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n>> -\t\tfi\n>> -\telse\n>> -\t\tgit config submodule.\"$sm_name\".active \"true\"\n>> -\tfi\n>> +\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n>> }\n>> \n> \n> Very nice to have this simplified.\n> \n> Would be good to split this very long line across multiple lines\n> though...\n\nI followed the established pattern of not splitting the lines from the\nprevious conversions ('submodule--helper update-clone' being the\nexception in that file).\n\nIn this case, I felt it should be fine because a follow-up series that\ncompletes the full conversion will get rid of that line entirely.\n\n"},{"id":"430931","messageId":"A7C71B08-8C7C-4D30-88FF-58B3080A76EF@gmail.com","threadId":"56146","inReplyTo":"20210722112143.97944-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-22T13:31:03Z","receivedAt":"2021-07-22T13:31:14Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 22-Jul-2021, at 16:51, Atharva Raykar wrote:\n> [...]\n> This patch depends on changes introduced in 83913dd5b6 (t7400: test failure to\n> add submodule in tracked path, 2021-06-18), which belongs to the\n> ar/submodule-add (2021-07-12) series[1].\n> \n> This changes in this patch are no different from the one in my first\n> submodule-add conversion series[2] which has already got some reviews before.\n> \n> [1] https://lore.kernel.org/git/20210710074801.19917-1-raykar.ath@gmail.com/\n> [2] https://lore.kernel.org/git/20210615145745.33382-4-raykar.ath@gmail.com/\n\nA correction:\n\nThis patch depends on that same topic, but a different commit, which is\n559e49fe5c (submodule: prefix die messages with 'fatal', 2021-07-08)."},{"id":"431096","messageId":"xmqq4kckn4x1.fsf@gitster.g","threadId":"56146","inReplyTo":"20210722112143.97944-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-23T20:36:42Z","receivedAt":"2021-07-23T20:36:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Atharva Raykar writes:\n\n> This is meant to be a faithful conversion from shell to C, with only one\n> minor change: A warning is emitted if no value is specified in\n> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n\n... meaning that submodule.active is *not* a boolean.\n\nIn scripted porcelain, I think we let \"submodule--helper is-active\"\nto inspect its value(s), which may end up feeding a NULL as one of\nthe pathspec elements when calling parse_pathspec(), so this may\neven be a bugfix. In any case, I think \"submodule--helper\nis-active\" is where such a fix should happen and in the longer term,\nthe code that says \"if submodule.active exists, ask is-active and\nset submodule.*.active accordingly, otherwise activate everything\"\nwe see in this patch should be simplified to always ask is-active\nand let is-active worry about cases like missing submodule.active\nand submodule.active being valueless true, so let's not worry too\nmuch about what happens in this patch, because it needs to be\ncleaned up anyway after the dust settles.\n\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 862053c9f2..9658804d24 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n> +{\n> +\tchar *key;\n> +\n> +\tif (!is_writing_gitmodules_ok())\n> +\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> +\n> +\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n> +\tconfig_set_in_gitmodules_file_gently(key, value);\n\nThis uses _gently() to avoid dying, but does it discard error return\nand hide it from our callers?\n\n> +\tfree(key);\n> +}\n> +\n> +static void configure_added_submodule(struct add_data *add_data)\n> +{\n> +\tchar *key, *submod_pathspec = NULL;\n> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n> +\tint pathspec_key_exists, activate = 0;\n> +\n> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n> +\tgit_config_set_gently(key, add_data->realrepo);\n> +\tfree(key);\n> +\n> +\tadd_submod.git_cmd = 1;\n> +\tstrvec_pushl(&add_submod.args, \"add\",\n> +\t\t \"--no-warn-embedded-repo\", NULL);\n> +\tif (add_data->force)\n> +\t\tstrvec_push(&add_submod.args, \"--force\");\n> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n> +\n> +\tif (run_command(&add_submod))\n> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n> +\n> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path);\n> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo);\n> +\tif (add_data->branch)\n> +\t\tconfig_submodule_in_gitmodules(add_data->sm_name,\n> +\t\t\t\t\t \"branch\", add_data->branch);\n\nA failure in any of the above in the scripted version used to result\nin \"failed to register submodule\" error, but they are now ignored.\nIntended?\n\n> +\tadd_gitmodules.git_cmd = 1;\n> +\tstrvec_pushl(&add_gitmodules.args,\n> +\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n> +\n> +\tif (run_command(&add_gitmodules))\n> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n> +\t/*\n> +\t * NEEDSWORK: In a multi-working-tree world this needs to be\n> +\t * set in the per-worktree config.\n> +\t */\n> +\tpathspec_key_exists = !git_config_get_string(\"submodule.active\",\n> +\t\t\t\t\t\t &submod_pathspec);\n> +\tif (pathspec_key_exists && !submod_pathspec) {\n> +\t\twarning(_(\"The submodule.active configuration exists, but the \"\n> +\t\t\t \"pathspec was unset. If the submodule is not already \"\n> +\t\t\t \"active, the value of submodule.%s.active will be \"\n> +\t\t\t \"be set to 'true'.\"), add_data->sm_name);\n> +\t\tactivate = 1;\n> +\t}\n> +\n> +\t/*\n> +\t * If submodule.active does not exist, or if the pathspec was unset,\n> +\t * we will activate this module unconditionally.\n> +\t *\n> +\t * Otherwise, we ask is_submodule_active(), which iterates\n> +\t * through all the values of 'submodule.active' to determine\n> +\t * if this module is already active.\n> +\t */\n> +\tif (!pathspec_key_exists || activate ||\n> +\t !is_submodule_active(the_repository, add_data->sm_path)) {\n> +\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> +\t\tgit_config_set_gently(key, \"true\");\n> +\t\tfree(key);\n> +\t}\n\nThis is the part I discussed earlier. I think this \"optimize so\nthat we can avoid calling is_submodule_active()\" should go away in\nthe long run. In the current code, is_submodule_active() needs to\nfind out the value of submodule.active itself anyway, so the\nshort-circuit is not working as an optimization.\n\nOther than the \"what happens when we see errors?\" issue, the patch\nlooks quite straight-forward rewrite from the scripted version.\n\nThanks.\n"},{"id":"431111","messageId":"a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com","threadId":"56146","inReplyTo":"xmqq4kckn4x1.fsf@gitster.g","subject":"Re: [GSoC] [PATCH] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-24T09:59:55Z","receivedAt":"2021-07-24T10:00:04Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On 24/07/21 02:06, Junio C Hamano wrote:\n> Atharva Raykar writes:\n> \n>> This is meant to be a faithful conversion from shell to C, with only one\n>> minor change: A warning is emitted if no value is specified in\n>> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n> \n> ... meaning that submodule.active is *not* a boolean.\n> \n> In scripted porcelain, I think we let \"submodule--helper is-active\"\n> to inspect its value(s), which may end up feeding a NULL as one of\n> the pathspec elements when calling parse_pathspec(), so this may\n> even be a bugfix. In any case, I think \"submodule--helper\n> is-active\" is where such a fix should happen and in the longer term,\n> the code that says \"if submodule.active exists, ask is-active and\n> set submodule.*.active accordingly, otherwise activate everything\"\n> we see in this patch should be simplified to always ask is-active\n> and let is-active worry about cases like missing submodule.active\n> and submodule.active being valueless true, so let's not worry too\n> much about what happens in this patch, because it needs to be\n> cleaned up anyway after the dust settles.\n\nOkay, that makes sense. I'll remove the extra warning and special\nhandling and make it a bug-for-bug conversion for now, so that the\ncleanup can be handled afterwards. It will probably be more fitting to\nhave this change 'is_submodule_active()' afterwards. I'll maybe add a\nNEEDSWORK comment for now?\n\n>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n>> index 862053c9f2..9658804d24 100644\n>> --- a/builtin/submodule--helper.c\n>> +++ b/builtin/submodule--helper.c\n>> @@ -2936,6 +2936,130 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n>> \treturn 0;\n>> }\n>> \n>> +static void config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n>> +{\n>> +\tchar *key;\n>> +\n>> +\tif (!is_writing_gitmodules_ok())\n>> +\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n>> +\n>> +\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n>> +\tconfig_set_in_gitmodules_file_gently(key, value);\n> \n> This uses _gently() to avoid dying, but does it discard error return\n> and hide it from our callers?\n> \n>> +\tfree(key);\n>> +}\n>> +\n>> +static void configure_added_submodule(struct add_data *add_data)\n>> +{\n>> +\tchar *key, *submod_pathspec = NULL;\n>> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n>> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n>> +\tint pathspec_key_exists, activate = 0;\n>> +\n>> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n>> +\tgit_config_set_gently(key, add_data->realrepo);\n>> +\tfree(key);\n>> +\n>> +\tadd_submod.git_cmd = 1;\n>> +\tstrvec_pushl(&add_submod.args, \"add\",\n>> +\t\t \"--no-warn-embedded-repo\", NULL);\n>> +\tif (add_data->force)\n>> +\t\tstrvec_push(&add_submod.args, \"--force\");\n>> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n>> +\n>> +\tif (run_command(&add_submod))\n>> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n>> +\n>> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path);\n>> +\tconfig_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo);\n>> +\tif (add_data->branch)\n>> +\t\tconfig_submodule_in_gitmodules(add_data->sm_name,\n>> +\t\t\t\t\t \"branch\", add_data->branch);\n> \n> A failure in any of the above in the scripted version used to result\n> in \"failed to register submodule\" error, but they are now ignored.\n> Intended?\n\nThis was not intended. I think I did not notice those expressions were\nchained in the scripted version. I'll fix this.\n\n>> +\tadd_gitmodules.git_cmd = 1;\n>> +\tstrvec_pushl(&add_gitmodules.args,\n>> +\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n>> +\n>> +\tif (run_command(&add_gitmodules))\n>> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n>> +\n>> +\t/*\n>> +\t * NEEDSWORK: In a multi-working-tree world this needs to be\n>> +\t * set in the per-worktree config.\n>> +\t */\n>> +\tpathspec_key_exists = !git_config_get_string(\"submodule.active\",\n>> +\t\t\t\t\t\t &submod_pathspec);\n>> +\tif (pathspec_key_exists && !submod_pathspec) {\n>> +\t\twarning(_(\"The submodule.active configuration exists, but the \"\n>> +\t\t\t \"pathspec was unset. If the submodule is not already \"\n>> +\t\t\t \"active, the value of submodule.%s.active will be \"\n>> +\t\t\t \"be set to 'true'.\"), add_data->sm_name);\n>> +\t\tactivate = 1;\n>> +\t}\n>> +\n>> +\t/*\n>> +\t * If submodule.active does not exist, or if the pathspec was unset,\n>> +\t * we will activate this module unconditionally.\n>> +\t *\n>> +\t * Otherwise, we ask is_submodule_active(), which iterates\n>> +\t * through all the values of 'submodule.active' to determine\n>> +\t * if this module is already active.\n>> +\t */\n>> +\tif (!pathspec_key_exists || activate ||\n>> +\t !is_submodule_active(the_repository, add_data->sm_path)) {\n>> +\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n>> +\t\tgit_config_set_gently(key, \"true\");\n>> +\t\tfree(key);\n>> +\t}\n> \n> This is the part I discussed earlier. I think this \"optimize so\n> that we can avoid calling is_submodule_active()\" should go away in\n> the long run. In the current code, is_submodule_active() needs to\n> find out the value of submodule.active itself anyway, so the\n> short-circuit is not working as an optimization.\n\nAgreed.\n\n> Other than the \"what happens when we see errors?\" issue, the patch\n> looks quite straight-forward rewrite from the scripted version.\n> \n> Thanks.\n> \n\n"},{"id":"431395","messageId":"20210728115304.80643-1-raykar.ath@gmail.com","threadId":"56146","inReplyTo":"20210722112143.97944-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v2] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-28T11:53:04Z","receivedAt":"2021-07-28T11:53:52Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new \"add-config\" subcommand to `git submodule--helper` with the\ngoal of converting part of the shell code in git-submodule.sh related to\n`git submodule add` into C code. This new subcommand sets the\nconfiguration variables of a newly added submodule, by registering the\nurl in local git config, as well as the submodule name and path in the\n.gitmodules file. It also sets 'submodule..active' to \"true\" if\nthe submodule path has not already been covered by any pathspec\nspecified in 'submodule.active'.\n\nThis is meant to be a faithful conversion from shell to C, with only one\nminor change: A warning is emitted if no value is specified in\n'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\nbecause it is an invalid configuration. It would be helpful to let the\nuser know that the pathspec is unset, and the value of\n'submodule..active' might be set to 'true' so that they can\nrectify their configuration and prevent future surprises (especially\ngiven that the latter variable has a higher priority than the former).\n\nThe structure of the conditional to check if we need to set the 'active'\ntoggle looks different from the shell version -- but behaves the same.\nThe change was made to decrease code duplication. A comment has been\nadded to explain that only one value of 'submodule.active' is obtained\nto check if we need to call is_submodule_active() at all.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nChanges since v1:\n\n* Remove the extra handling for the case where submodule.active is valueless, as\n Junio pointed out that it is better dealt with in a cleanup patch.\n\n* Do not discard error returns from 'config_submodule_in_gitmodules()', and also\n ensure that any calls to it in 'configure_added_submodule()' die on failure,\n like with the original shell porcelain.\n\n* Style fixes.\n\n builtin/submodule--helper.c | 120 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 28 +--------\n 2 files changed, 121 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 862053c9f2..60b47492cb 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2936,6 +2936,125 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n+{\n+\tchar *key;\n+\tint ret;\n+\n+\tif (!is_writing_gitmodules_ok())\n+\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n+\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n+\tret = config_set_in_gitmodules_file_gently(key, value);\n+\tfree(key);\n+\n+\treturn ret;\n+}\n+\n+static void configure_added_submodule(struct add_data *add_data)\n+{\n+\tchar *key;\n+\tchar *val = NULL;\n+\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n+\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n+\n+\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n+\tgit_config_set_gently(key, add_data->realrepo);\n+\tfree(key);\n+\n+\tadd_submod.git_cmd = 1;\n+\tstrvec_pushl(&add_submod.args, \"add\",\n+\t\t \"--no-warn-embedded-repo\", NULL);\n+\tif (add_data->force)\n+\t\tstrvec_push(&add_submod.args, \"--force\");\n+\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n+\n+\tif (run_command(&add_submod))\n+\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n+\n+\tif (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n+\t config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\tif (add_data->branch)\n+\t\tif (config_submodule_in_gitmodules(add_data->sm_name,\n+\t\t\t\t\t\t \"branch\", add_data->branch))\n+\t\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\tadd_gitmodules.git_cmd = 1;\n+\tstrvec_pushl(&add_gitmodules.args,\n+\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n+\n+\tif (run_command(&add_gitmodules))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\t/*\n+\t * NEEDSWORK: In a multi-working-tree world this needs to be\n+\t * set in the per-worktree config.\n+\t *\n+\t * If submodule.active does not exist, or if the pathspec was unset,\n+\t * we will activate this module unconditionally.\n+\t *\n+\t * Otherwise, we ask is_submodule_active(), which iterates\n+\t * through all the values of 'submodule.active' to determine\n+\t * if this module is already active.\n+\t */\n+\tif (git_config_get_string(\"submodule.active\", &val) ||\n+\t !is_submodule_active(the_repository, add_data->sm_path)) {\n+\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t}\n+}\n+\n+static int add_config(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 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 store in \"\n+\t\t\t \"the submodule configuration\")),\n+\t\tOPT_STRING(0, \"url\", &add_data.repo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone submodule from\")),\n+\t\tOPT_STRING(0, \"resolved-url\", &add_data.realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone the submodule from, after it has \"\n+\t\t\t \"been dereferenced relative to parent's url, \"\n+\t\t\t \"in the case where is a relative url\")),\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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add-config \"\n+\t\t \"[--force|-f] [--branch|-b ] \"\n+\t\t \"--url --resolved-url \"\n+\t\t \"--path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc)\n+\t\tusage_with_options(usage, options);\n+\n+\tadd_data.force = !!force;\n+\tconfigure_added_submodule(&add_data);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2949,6 +3068,7 @@ static struct cmd_struct commands[] = {\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n \t{\"add-clone\", add_clone, 0},\n+\t{\"add-config\", add_config, 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 053daf3724..f713cb113c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -242,33 +242,7 @@ cmd_add()\n \tfi\n \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-\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-\tif test -n \"$branch\"\n-\tthen\n-\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n-\tfi &&\n-\tgit add --force .gitmodules ||\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-\tif git config --get submodule.active >/dev/null\n-\tthen\n-\t\t# If the submodule being adding isn't already covered by the\n-\t\t# current configured pathspec, set the submodule's active flag\n-\t\tif ! git submodule--helper is-active \"$sm_path\"\n-\t\tthen\n-\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\t\tfi\n-\telse\n-\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\tfi\n+\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n }\n \n #\n-- \n2.32.0\n\n"},{"id":"431430","messageId":"07070c45-5761-b67e-59b1-aa90f8cd877b@gmail.com","threadId":"56146","inReplyTo":"20210728115304.80643-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: introduce add-config subcommand","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-07-28T19:51:05Z","receivedAt":"2021-07-28T19:51:18Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"Hi Atharva,\n\nOn 28/07/21 5:23 pm, Atharva Raykar wrote:\n> Add a new \"add-config\" subcommand to `git submodule--helper` with the\n> goal of converting part of the shell code in git-submodule.sh related to\n> `git submodule add` into C code. This new subcommand sets the\n> configuration variables of a newly added submodule, by registering the\n> url in local git config, as well as the submodule name and path in the\n> .gitmodules file. It also sets 'submodule..active' to \"true\" if\n> the submodule path has not already been covered by any pathspec\n> specified in 'submodule.active'.\n> \n> This is meant to be a faithful conversion from shell to C, with only one\n> minor change: A warning is emitted if no value is specified in\n> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n> because it is an invalid configuration. It would be helpful to let the\n> user know that the pathspec is unset, and the value of\n> 'submodule..active' might be set to 'true' so that they can\n> rectify their configuration and prevent future surprises (especially\n> given that the latter variable has a higher priority than the former).\n> \n\nv2 doesn't have the warning that this paragraph describes. So, this could\nbe dropped.\n\n> [ snip ]\n>\n> A comment has been\n> added to explain that only one value of 'submodule.active' is obtained\n> to check if we need to call is_submodule_active() at all.\n>\n\nThis could be me likely not understanding this properly. Anyways, where\nis this comment in the code? I only see a comment about how 'is_submodule_active'\niterates over all values. I couldn't find any \"one value\" reference in it.\n \n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n> ---\n> \n> Changes since v1:\n> \n> * Remove the extra handling for the case where submodule.active is valueless, as\n> Junio pointed out that it is better dealt with in a cleanup patch.\n> \n> * Do not discard error returns from 'config_submodule_in_gitmodules()', and also\n> ensure that any calls to it in 'configure_added_submodule()' die on failure,\n> like with the original shell porcelain.\n> \n> * Style fixes.\n> \n> builtin/submodule--helper.c | 120 ++++++++++++++++++++++++++++++++++++\n> git-submodule.sh | 28 +--------\n> 2 files changed, 121 insertions(+), 27 deletions(-)\n> \n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 862053c9f2..60b47492cb 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -2936,6 +2936,125 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n> \treturn 0;\n> }\n> \n> +static int config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n> +{\n> +\tchar *key;\n> +\tint ret;\n> +\n> +\tif (!is_writing_gitmodules_ok())\n> +\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n> +\n> +\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n> +\tret = config_set_in_gitmodules_file_gently(key, value);\n> +\tfree(key);\n> +\n> +\treturn ret;\n> +}\n> +\n> +static void configure_added_submodule(struct add_data *add_data)\n> +{\n> +\tchar *key;\n> +\tchar *val = NULL;\n> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n> +\n> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n> +\tgit_config_set_gently(key, add_data->realrepo);\n> +\tfree(key);\n> +\n> +\tadd_submod.git_cmd = 1;\n> +\tstrvec_pushl(&add_submod.args, \"add\",\n> +\t\t \"--no-warn-embedded-repo\", NULL);\n> +\tif (add_data->force)\n> +\t\tstrvec_push(&add_submod.args, \"--force\");\n> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n> +\n> +\tif (run_command(&add_submod))\n> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n> +\n\n> +\tif (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n> +\t config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n> +\tif (add_data->branch)\n> +\t\tif (config_submodule_in_gitmodules(add_data->sm_name,\n> +\t\t\t\t\t\t \"branch\", add_data->branch))\n> +\t\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n> +\tadd_gitmodules.git_cmd = 1;\n> +\tstrvec_pushl(&add_gitmodules.args,\n> +\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n> +\n> +\tif (run_command(&add_gitmodules))\n> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n\nWe could restructure this portion like so ...\n\n-- 8< --\n add_gitmodules.git_cmd = 1;\n strvec_pushl(&add_gitmodules.args,\n \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n \n if (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo) ||\n (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name,\n \"branch\", add_data->branch)) ||\n run_command(&add_gitmodules))\n die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n-- >8 --\n\n.. to avoid the redundant \"Failed to register submodule ...\" error message.\nWhether the restructured version has poor readability or not is debatable, though.\n \n> +\t/*\n> +\t * NEEDSWORK: In a multi-working-tree world this needs to be\n> +\t * set in the per-worktree config.\n> +\t *\n\nIt might be a good idea to differentiate the NEEDSWORK comment from an informative\ncomment about the code snippet.\n\nAlso, you could add another NEEDSWORK/TODO comment regarding the change\nto 'is_submodule_active' which you mention before[1].\n\n[1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/\n\n> +\t * If submodule.active does not exist, or if the pathspec was unset,\n> +\t * we will activate this module unconditionally.\n> +\t *\n> +\t * Otherwise, we ask is_submodule_active(), which iterates\n> +\t * through all the values of 'submodule.active' to determine\n> +\t * if this module is already active.\n> +\t */\n> +\tif (git_config_get_string(\"submodule.active\", &val) ||\n> +\t !is_submodule_active(the_repository, add_data->sm_path)) {\n> +\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> +\t\tgit_config_set_gently(key, \"true\");\n> +\t\tfree(key);\n> +\t}\n\nIt might be a good idea to expand this condition similar to the scripted version,\nto retain the following comment which seems like a useful one to keep.\n\n> [ snip ]\n>\n> -\tif git config --get submodule.active >/dev/null\n> -\tthen\n> -\t\t# If the submodule being adding isn't already covered by the\n> -\t\t# current configured pathspec, set the submodule's active flag\n> -\t\tif ! git submodule--helper is-active \"$sm_path\"\n> -\t\tthen\n> -\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n> -\t\tfi\n> -\telse\n> -\t\tgit config submodule.\"$sm_name\".active \"true\"\n> -\tfi\n\n> +\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n> }\n> \n> #\n> \n\n-- \nSivaraam\n"},{"id":"431511","messageId":"251ef131-bdd1-3881-659e-3caf20b65a53@gmail.com","threadId":"56146","inReplyTo":"d206fa7a-a450-552b-824c-518ee481c480@gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: introduce add-config subcommand","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-07-29T19:30:45Z","receivedAt":"2021-07-29T19:30:54Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 29/07/21 11:05 pm, Atharva Raykar wrote:\n> (apologies for the reflowed text, seems to only happen when replying to\n> this message?? Won't affect this response much though)\n> \n\nIn case you're using thunderbird then you could see if the following helps:\n\nhttp://kb.mozillazine.org/Plain_text_e-mail_%28Thunderbird%29#Flowed_format\n\n> On 29/07/21 01:21, Kaartic Sivaraam wrote:\n>> Hi Atharva,\n>>\n>> On 28/07/21 5:23 pm, Atharva Raykar wrote:\n>>> Add a new \"add-config\" subcommand to `git submodule--helper` with the\n>>> goal of converting part of the shell code in git-submodule.sh related to\n>>> `git submodule add` into C code. This new subcommand sets the\n>>> configuration variables of a newly added submodule, by registering the\n>>> url in local git config, as well as the submodule name and path in the\n>>> .gitmodules file. It also sets 'submodule..active' to \"true\" if\n>>> the submodule path has not already been covered by any pathspec\n>>> specified in 'submodule.active'.\n>>>\n>>> This is meant to be a faithful conversion from shell to C, with only one\n>>> minor change: A warning is emitted if no value is specified in\n>>> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n>>> because it is an invalid configuration. It would be helpful to let the\n>>> user know that the pathspec is unset, and the value of\n>>> 'submodule..active' might be set to 'true' so that they can\n>>> rectify their configuration and prevent future surprises (especially\n>>> given that the latter variable has a higher priority than the former).\n>>>\n>>\n>> v2 doesn't have the warning that this paragraph describes. So, this could\n>> be dropped.\n> \n> My bad, looks like I forgot to edit the commit message.\n> \n>>> [ snip ]\n>>>\n>>> A comment has been\n>>> added to explain that only one value of 'submodule.active' is obtained\n>>> to check if we need to call is_submodule_active() at all.\n>>>\n>>\n>> This could be me likely not understanding this properly. Anyways, where\n>> is this comment in the code? I only see a comment about how\n>> 'is_submodule_active'\n>> iterates over all values. I couldn't find any \"one value\" reference in it.\n> \n> Looks like my comment does not explain it clearly. It would have made\n> more sense to start the comment with \"If there is no value found for\n> submodule.active\", but I think instead of modifying that comment (which\n> is clear enough as it is), I'll make the commit message better, by\n> removing the mention of the \"we check one value\".\n> \n> It seems like the line:\n> \n> \tif (git_config_get_string(\"submodule.active\", &val)\n> \n> makes it clear that a single string is being queried first. The larger\n> point was about why that conditional was needed, if we were going to\n> call 'is_submodule_active()' to retrieve the value anyway.\n> \n\nAh. Now I get the idea. A rephrasing might indeed make this clear.\n\n>>> +    if (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n>>> +        config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n>>> +        die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n>>> +\n>>> +    if (add_data->branch)\n>>> +        if (config_submodule_in_gitmodules(add_data->sm_name,\n>>> +                           \"branch\", add_data->branch))\n>>> +            die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n>>> +\n>>> +    add_gitmodules.git_cmd = 1;\n>>> +    strvec_pushl(&add_gitmodules.args,\n>>> +             \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n>>> +\n>>> +    if (run_command(&add_gitmodules))\n>>> +        die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n>>> +\n>>\n>> We could restructure this portion like so ...\n>>\n>> -- 8< --\n>>         add_gitmodules.git_cmd = 1;\n>>         strvec_pushl(&add_gitmodules.args,\n>>                      \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n>>> \n>>         if (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n>>             config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo) ||\n>>             (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name,\n>>                                                                 \"branch\", add_data->branch)) ||\n>>             run_command(&add_gitmodules))\n>>                 die(_(\"Failed to register submodule '%s'\"),\n>> add_data->sm_path);\n>> -- >8 --\n>>\n>> .. to avoid the redundant \"Failed to register submodule ...\" error message.\n>> Whether the restructured version has poor readability or not is debatable, though.\n> \n> Yeah, I felt the redundancy in this case was okay, I find that big\n> conditional rather hard to read.\n> \n\nI tried to make it as easy to read as possible but its a really long one\nindeed. So, I could understand. But the redundancy bothered me a bit ;-)\n\n>>> +    /*\n>>> +     * NEEDSWORK: In a multi-working-tree world this needs to be\n>>> +     * set in the per-worktree config.\n>>> +     *\n>>\n>> It might be a good idea to differentiate the NEEDSWORK comment from an\n>> informative comment about the code snippet.\n> \n> Okay. I suppose you mean give this part it's own closing delimiter and\n> start the next line with a new multiline comment.\n> \n\nYeah. I did mean this.\n\n> If you meant something else, do let me know.\n> \n>> Also, you could add another NEEDSWORK/TODO comment regarding the change\n>> to 'is_submodule_active' which you mention before[1].\n>>\n>> [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/\n> \n> Good point. I'll add it.\n> \n>>> +     * If submodule.active does not exist, or if the pathspec was unset,\n>>> +     * we will activate this module unconditionally.\n>>> +     *\n>>> +     * Otherwise, we ask is_submodule_active(), which iterates\n>>> +     * through all the values of 'submodule.active' to determine\n>>> +     * if this module is already active.\n>>> +     */\n>>> +    if (git_config_get_string(\"submodule.active\", &val) ||\n>>> +        !is_submodule_active(the_repository, add_data->sm_path)) {\n>>> +        key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n>>> +        git_config_set_gently(key, \"true\");\n>>> +        free(key);\n>>> +    }\n>>\n>> It might be a good idea to expand this condition similar to the scripted version,\n>> to retain the following comment which seems like a useful one to keep.\n> \n> I felt that this version had less redundant code, and hence seemed more\n> readable than the expanded conditional in shell.\n> \n> For comparison this is the same code imitating the shell version:\n> \n> if (!git_config_get_string(\"submodule.active\", &var) && var) {\n> \n> \t/*\n> \t * If the submodule being added isn't already covered by the\n> \t * current configured pathspec, set the submodule's active flag\n> \t */\n> \tif (!is_submodule_active(the_repository, info->sm_path)) {\n> \t\tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n> \t\tgit_config_set_gently(key, \"true\");\n> \t\tfree(key);\n> \t}\n> \n> } else {\n> \tkey = xstrfmt(\"submodule.%s.active\", info->sm_name);\n> \tgit_config_set_gently(key, \"true\");\n> \tfree(key);\n> }\n> \n> It repeats the string allocation and freeing, and also is a lot more\n> code to parse mentally while reading. The shorter version that I used\n> does not feel more \"clever\" to me than this either.\n> \n> As for the comment, I felt that the new one I introduced (Otherwise, we\n> ask ...) covers the same ground.\n> \n\nI think the comment you introduced only mentions that 'is_submodule_active'\niterates over configs to determine that a submodule is active. It doesn't mention\nthat we set the submodule's active flag if the submodule is not covered by the\ncurrent configured pathspec, which is what the original tries to convey.\nCorrect me if I missed anything.\n\n> I am open to reverting to the expanded conditional, but it would be nice\n> if you could help me understand the motivation behind why it should be done.\n> \n\nI'm not against short-circuiting the conditional. I suggested expanding the conditional\nso that we get a structure similar to the scripted version. That way we could keep the\noriginal comment close to the inside conditional where it felt relevant :)\n\n>>> [ snip ]\n>>>\n>>> -    if git config --get submodule.active >/dev/null\n>>> -    then\n>>> -        # If the submodule being adding isn't already covered by the\n>>> -        # current configured pathspec, set the submodule's active flag\n>>> -        if ! git submodule--helper is-active \"$sm_path\"\n>>> -        then\n>>> -            git config submodule.\"$sm_name\".active \"true\"\n>>> -        fi\n>>> -    else\n>>> -        git config submodule.\"$sm_name\".active \"true\"\n>>> -    fi\n>>> +    git submodule--helper add-config ${force:+--force}\n>>> ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\"\n>>> --path \"$sm_path\" --name \"$sm_name\"\n>>>   }\n>>>     #\n>>>\n>>\n> \n\n\n-- \nSivaraam\n"},{"id":"431540","messageId":"CADi-XoRNRrtC6bQ-DETj=0Bmy=WJzv3mk++QkrpDOZ6THGhaZQ@mail.gmail.com","threadId":"56146","inReplyTo":"251ef131-bdd1-3881-659e-3caf20b65a53@gmail.com","subject":"Re: [GSoC] [PATCH v2] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-07-30T06:22:10Z","receivedAt":"2021-07-30T06:22:24Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"On Fri, Jul 30, 2021 at 1:00 AM Kaartic Sivaraam\n wrote:\n>\n> On 29/07/21 11:05 pm, Atharva Raykar wrote:\n> > (apologies for the reflowed text, seems to only happen when replying to\n> > this message?? Won't affect this response much though)\n> >\n>\n> In case you're using thunderbird then you could see if the following helps:\n>\n> http://kb.mozillazine.org/Plain_text_e-mail_%28Thunderbird%29#Flowed_format\n\nYeah, I have pretty much been following the setup that is in the\ngit-format-patch [1] documentation. It worked fine until the last couple of\ndays. The mailing list is now rejecting all my mails. My guess is because\nThunderbird is forcing a 'Content-Transfer-Encoding: 7-bit' which I read causes\nproblems with this list [2]. Strangely, so far, this header is added only when\nI send mails to git@vger.kernel.org, not elsewhere.\n(sending this from GMail for now)\n\nHere's the error message:\n------8<------8<------8<------\n\n: host 23.128.96.18[23.128.96.18] said: 550 5.7.1\n Content-Policy reject msg: Wrong MIME labeling on 8-bit character texts.\n BF:; S229739AbhG2RfR (in reply to end of DATA command)\n\n------8<------8<------8<------\n\nI'll try fixing my mail situation today, and if I still have problems, I'll\nbring it up on a separate thread.\n\n[1] https://git-scm.com/docs/git-format-patch#_approach_1_add_on\n[2] https://lore.kernel.org/git/alpine.DEB.2.20.1611031554100.3108@virtualbox/\n\n> > On 29/07/21 01:21, Kaartic Sivaraam wrote:\n> >> Hi Atharva,\n> >>\n> >> On 28/07/21 5:23 pm, Atharva Raykar wrote:\n> >>> Add a new \"add-config\" subcommand to `git submodule--helper` with the\n> >>> goal of converting part of the shell code in git-submodule.sh related to\n> >>> `git submodule add` into C code. This new subcommand sets the\n> >>> configuration variables of a newly added submodule, by registering the\n> >>> url in local git config, as well as the submodule name and path in the\n> >>> .gitmodules file. It also sets 'submodule..active' to \"true\" if\n> >>> the submodule path has not already been covered by any pathspec\n> >>> specified in 'submodule.active'.\n> >>>\n> >>> This is meant to be a faithful conversion from shell to C, with only one\n> >>> minor change: A warning is emitted if no value is specified in\n> >>> 'submodule.active', ie, the config looks like: \"[submodule] active\\n\",\n> >>> because it is an invalid configuration. It would be helpful to let the\n> >>> user know that the pathspec is unset, and the value of\n> >>> 'submodule..active' might be set to 'true' so that they can\n> >>> rectify their configuration and prevent future surprises (especially\n> >>> given that the latter variable has a higher priority than the former).\n> >>>\n> >>\n> >> v2 doesn't have the warning that this paragraph describes. So, this could\n> >> be dropped.\n> >\n> > My bad, looks like I forgot to edit the commit message.\n> >\n> >>> [ snip ]\n> >>>\n> >>> A comment has been\n> >>> added to explain that only one value of 'submodule.active' is obtained\n> >>> to check if we need to call is_submodule_active() at all.\n> >>>\n> >>\n> >> This could be me likely not understanding this properly. Anyways, where\n> >> is this comment in the code? I only see a comment about how\n> >> 'is_submodule_active'\n> >> iterates over all values. I couldn't find any \"one value\" reference in it.\n> >\n> > Looks like my comment does not explain it clearly. It would have made\n> > more sense to start the comment with \"If there is no value found for\n> > submodule.active\", but I think instead of modifying that comment (which\n> > is clear enough as it is), I'll make the commit message better, by\n> > removing the mention of the \"we check one value\".\n> >\n> > It seems like the line:\n> >\n> > if (git_config_get_string(\"submodule.active\", &val)\n> >\n> > makes it clear that a single string is being queried first. The larger\n> > point was about why that conditional was needed, if we were going to\n> > call 'is_submodule_active()' to retrieve the value anyway.\n> >\n>\n> Ah. Now I get the idea. A rephrasing might indeed make this clear.\n>\n> >>> + if (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n> >>> + config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n> >>> + die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> >>> +\n> >>> + if (add_data->branch)\n> >>> + if (config_submodule_in_gitmodules(add_data->sm_name,\n> >>> + \"branch\", add_data->branch))\n> >>> + die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> >>> +\n> >>> + add_gitmodules.git_cmd = 1;\n> >>> + strvec_pushl(&add_gitmodules.args,\n> >>> + \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n> >>> +\n> >>> + if (run_command(&add_gitmodules))\n> >>> + die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> >>> +\n> >>\n> >> We could restructure this portion like so ...\n> >>\n> >> -- 8< --\n> >> add_gitmodules.git_cmd = 1;\n> >> strvec_pushl(&add_gitmodules.args,\n> >> \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n> >>>\n> >> if (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n> >> config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo) ||\n> >> (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name,\n> >> \"branch\", add_data->branch)) ||\n> >> run_command(&add_gitmodules))\n> >> die(_(\"Failed to register submodule '%s'\"),\n> >> add_data->sm_path);\n> >> -- >8 --\n> >>\n> >> .. to avoid the redundant \"Failed to register submodule ...\" error message.\n> >> Whether the restructured version has poor readability or not is debatable, though.\n> >\n> > Yeah, I felt the redundancy in this case was okay, I find that big\n> > conditional rather hard to read.\n> >\n>\n> I tried to make it as easy to read as possible but its a really long one\n> indeed. So, I could understand. But the redundancy bothered me a bit ;-)\n>\n> >>> + /*\n> >>> + * NEEDSWORK: In a multi-working-tree world this needs to be\n> >>> + * set in the per-worktree config.\n> >>> + *\n> >>\n> >> It might be a good idea to differentiate the NEEDSWORK comment from an\n> >> informative comment about the code snippet.\n> >\n> > Okay. I suppose you mean give this part it's own closing delimiter and\n> > start the next line with a new multiline comment.\n> >\n>\n> Yeah. I did mean this.\n>\n> > If you meant something else, do let me know.\n> >\n> >> Also, you could add another NEEDSWORK/TODO comment regarding the change\n> >> to 'is_submodule_active' which you mention before[1].\n> >>\n> >> [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/\n> >\n> > Good point. I'll add it.\n> >\n> >>> + * If submodule.active does not exist, or if the pathspec was unset,\n> >>> + * we will activate this module unconditionally.\n> >>> + *\n> >>> + * Otherwise, we ask is_submodule_active(), which iterates\n> >>> + * through all the values of 'submodule.active' to determine\n> >>> + * if this module is already active.\n> >>> + */\n> >>> + if (git_config_get_string(\"submodule.active\", &val) ||\n> >>> + !is_submodule_active(the_repository, add_data->sm_path)) {\n> >>> + key = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n> >>> + git_config_set_gently(key, \"true\");\n> >>> + free(key);\n> >>> + }\n> >>\n> >> It might be a good idea to expand this condition similar to the scripted version,\n> >> to retain the following comment which seems like a useful one to keep.\n> >\n> > I felt that this version had less redundant code, and hence seemed more\n> > readable than the expanded conditional in shell.\n> >\n> > For comparison this is the same code imitating the shell version:\n> >\n> > if (!git_config_get_string(\"submodule.active\", &var) && var) {\n> >\n> > /*\n> > * If the submodule being added isn't already covered by the\n> > * current configured pathspec, set the submodule's active flag\n> > */\n> > if (!is_submodule_active(the_repository, info->sm_path)) {\n> > key = xstrfmt(\"submodule.%s.active\", info->sm_name);\n> > git_config_set_gently(key, \"true\");\n> > free(key);\n> > }\n> >\n> > } else {\n> > key = xstrfmt(\"submodule.%s.active\", info->sm_name);\n> > git_config_set_gently(key, \"true\");\n> > free(key);\n> > }\n> >\n> > It repeats the string allocation and freeing, and also is a lot more\n> > code to parse mentally while reading. The shorter version that I used\n> > does not feel more \"clever\" to me than this either.\n> >\n> > As for the comment, I felt that the new one I introduced (Otherwise, we\n> > ask ...) covers the same ground.\n> >\n>\n> I think the comment you introduced only mentions that 'is_submodule_active'\n> iterates over configs to determine that a submodule is active. It doesn't mention\n> that we set the submodule's active flag if the submodule is not covered by the\n> current configured pathspec, which is what the original tries to convey.\n> Correct me if I missed anything.\n>\n> > I am open to reverting to the expanded conditional, but it would be nice\n> > if you could help me understand the motivation behind why it should be done.\n> >\n>\n> I'm not against short-circuiting the conditional. I suggested expanding the conditional\n> so that we get a structure similar to the scripted version. That way we could keep the\n> original comment close to the inside conditional where it felt relevant :)\n\nAh okay, so the reason is so that we could keep the structure similar\nto retain the\ncomment? Okay, I'll change that.\n\n> >>> [ snip ]\n> >>>\n> >>> - if git config --get submodule.active >/dev/null\n> >>> - then\n> >>> - # If the submodule being adding isn't already covered by the\n> >>> - # current configured pathspec, set the submodule's active flag\n> >>> - if ! git submodule--helper is-active \"$sm_path\"\n> >>> - then\n> >>> - git config submodule.\"$sm_name\".active \"true\"\n> >>> - fi\n> >>> - else\n> >>> - git config submodule.\"$sm_name\".active \"true\"\n> >>> - fi\n> >>> + git submodule--helper add-config ${force:+--force}\n> >>> ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\"\n> >>> --path \"$sm_path\" --name \"$sm_name\"\n> >>> }\n> >>> #\n> >>>\n> >>\n> >\n>\n>\n> --\n> Sivaraam\n"},{"id":"431645","messageId":"20210801063352.50813-1-raykar.ath@gmail.com","threadId":"56146","inReplyTo":"20210728115304.80643-1-raykar.ath@gmail.com","subject":"[GSoC] [PATCH v3] submodule--helper: introduce add-config subcommand","fromName":"Atharva Raykar","fromEmail":"raykar.ath@gmail.com","sentAt":"2021-08-01T06:33:52Z","receivedAt":"2021-08-01T06:34:16Z","isPatch":true,"sender":{"key":"raykar.ath@gmail.com","avatar":"https://avatars.githubusercontent.com/u/24277692?v=4"},"body":"Add a new \"add-config\" subcommand to `git submodule--helper` with the\ngoal of converting part of the shell code in git-submodule.sh related to\n`git submodule add` into C code. This new subcommand sets the\nconfiguration variables of a newly added submodule, by registering the\nurl in local git config, as well as the submodule name and path in the\n.gitmodules file. It also sets 'submodule..active' to \"true\" if\nthe submodule path has not already been covered by any pathspec\nspecified in 'submodule.active'.\n\nThis is meant to be a faithful conversion from shell to C, although we\nadd comments to areas that could be improved in future patches, after\nthe conversion has settled.\n\nSigned-off-by: Atharva Raykar \nMentored-by: Christian Couder \nMentored-by: Shourya Shukla \nBased-on-patch-by: Shourya Shukla \nBased-on-patch-by: Prathamesh Chavan \n---\n\nChanges since v2:\n\n* Change the commit message, which erroneously still spoke about a warning that\n has since been removed.\n\n* Change the structure of the code that checks if a submodule is active, so that\n it more closely resembles the conditional in the original shell version, and\n preserves the original comments.\n\n* Add NEEDSWORK comments, one to state the future intention of removing the\n check for 'submodule.active' before anyway calling is_submodule_active().\n The other comment is to state the intention of adding a warning in a future\n patch when 'is_submodule_active()' reads a valueless 'submodule.active'.\n\nFetch-it-via:\ngit fetch https://github.com/tfidfwastaken/git.git submodule-helper-add-config-3\n\n builtin/submodule--helper.c | 128 ++++++++++++++++++++++++++++++++++++\n git-submodule.sh | 28 +-------\n submodule.c | 5 ++\n 3 files changed, 134 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 862053c9f2..791ceeb63e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -2936,6 +2936,133 @@ static int add_clone(int argc, const char **argv, const char *prefix)\n \treturn 0;\n }\n \n+static int config_submodule_in_gitmodules(const char *name, const char *var, const char *value)\n+{\n+\tchar *key;\n+\tint ret;\n+\n+\tif (!is_writing_gitmodules_ok())\n+\t\tdie(_(\"please make sure that the .gitmodules file is in the working tree\"));\n+\n+\tkey = xstrfmt(\"submodule.%s.%s\", name, var);\n+\tret = config_set_in_gitmodules_file_gently(key, value);\n+\tfree(key);\n+\n+\treturn ret;\n+}\n+\n+static void configure_added_submodule(struct add_data *add_data)\n+{\n+\tchar *key;\n+\tchar *val = NULL;\n+\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n+\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n+\n+\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n+\tgit_config_set_gently(key, add_data->realrepo);\n+\tfree(key);\n+\n+\tadd_submod.git_cmd = 1;\n+\tstrvec_pushl(&add_submod.args, \"add\",\n+\t\t \"--no-warn-embedded-repo\", NULL);\n+\tif (add_data->force)\n+\t\tstrvec_push(&add_submod.args, \"--force\");\n+\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n+\n+\tif (run_command(&add_submod))\n+\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n+\n+\tif (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n+\t config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\tif (add_data->branch)\n+\t\tif (config_submodule_in_gitmodules(add_data->sm_name,\n+\t\t\t\t\t\t \"branch\", add_data->branch))\n+\t\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\tadd_gitmodules.git_cmd = 1;\n+\tstrvec_pushl(&add_gitmodules.args,\n+\t\t \"add\", \"--force\", \"--\", \".gitmodules\", NULL);\n+\n+\tif (run_command(&add_gitmodules))\n+\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n+\n+\t/*\n+\t * NEEDSWORK: In a multi-working-tree world this needs to be\n+\t * set in the per-worktree config.\n+\t */\n+\t/*\n+\t * NEEDSWORK: In the longer run, we need to get rid of this\n+\t * pattern of querying \"submodule.active\" before calling\n+\t * is_submodule_active(), since that function needs to find\n+\t * out the value of \"submodule.active\" again anyway.\n+\t */\n+\tif (!git_config_get_string(\"submodule.active\", &val) && val) {\n+\t\t/*\n+\t\t * If the submodule being added isn't already covered by the\n+\t\t * current configured pathspec, set the submodule's active flag\n+\t\t */\n+\t\tif (!is_submodule_active(the_repository, add_data->sm_path)) {\n+\t\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\t\tgit_config_set_gently(key, \"true\");\n+\t\t\tfree(key);\n+\t\t}\n+\t} else {\n+\t\tkey = xstrfmt(\"submodule.%s.active\", add_data->sm_name);\n+\t\tgit_config_set_gently(key, \"true\");\n+\t\tfree(key);\n+\t}\n+}\n+\n+static int add_config(int argc, const char **argv, const char *prefix)\n+{\n+\tint force = 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 store in \"\n+\t\t\t \"the submodule configuration\")),\n+\t\tOPT_STRING(0, \"url\", &add_data.repo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone submodule from\")),\n+\t\tOPT_STRING(0, \"resolved-url\", &add_data.realrepo,\n+\t\t\t N_(\"string\"),\n+\t\t\t N_(\"url to clone the submodule from, after it has \"\n+\t\t\t \"been dereferenced relative to parent's url, \"\n+\t\t\t \"in the case where is a relative url\")),\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__FORCE(&force, N_(\"allow adding an otherwise ignored submodule path\"),\n+\t\t\t PARSE_OPT_NOCOMPLETE),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const usage[] = {\n+\t\tN_(\"git submodule--helper add-config \"\n+\t\t \"[--force|-f] [--branch|-b ] \"\n+\t\t \"--url --resolved-url \"\n+\t\t \"--path --name \"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, options, usage, 0);\n+\n+\tif (argc)\n+\t\tusage_with_options(usage, options);\n+\n+\tadd_data.force = !!force;\n+\tconfigure_added_submodule(&add_data);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -2949,6 +3076,7 @@ static struct cmd_struct commands[] = {\n \t{\"name\", module_name, 0},\n \t{\"clone\", module_clone, 0},\n \t{\"add-clone\", add_clone, 0},\n+\t{\"add-config\", add_config, 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 053daf3724..f713cb113c 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -242,33 +242,7 @@ cmd_add()\n \tfi\n \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-\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-\tif test -n \"$branch\"\n-\tthen\n-\t\tgit submodule--helper config submodule.\"$sm_name\".branch \"$branch\"\n-\tfi &&\n-\tgit add --force .gitmodules ||\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-\tif git config --get submodule.active >/dev/null\n-\tthen\n-\t\t# If the submodule being adding isn't already covered by the\n-\t\t# current configured pathspec, set the submodule's active flag\n-\t\tif ! git submodule--helper is-active \"$sm_path\"\n-\t\tthen\n-\t\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\t\tfi\n-\telse\n-\t\tgit config submodule.\"$sm_name\".active \"true\"\n-\tfi\n+\tgit submodule--helper add-config ${force:+--force} ${branch:+--branch \"$branch\"} --url \"$repo\" --resolved-url \"$realrepo\" --path \"$sm_path\" --name \"$sm_name\"\n }\n \n #\ndiff --git a/submodule.c b/submodule.c\nindex 0b1d9c1dde..8577667773 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -237,6 +237,11 @@ int option_parse_recurse_submodules_worktree_updater(const struct option *opt,\n /*\n * Determine if a submodule has been initialized at a given 'path'\n */\n+/*\n+ * NEEDSWORK: Emit a warning if submodule.active exists, but is valueless,\n+ * ie, the config looks like: \"[submodule] active\\n\".\n+ * Since that is an invalid pathspec, we should inform the user.\n+ */\n int is_submodule_active(struct repository *repo, const char *path)\n {\n \tint ret = 0;\n-- \n2.32.0\n\n"},{"id":"432121","messageId":"1013ed94-ad4f-6ec8-09fb-772edea4ea05@gmail.com","threadId":"56146","inReplyTo":"20210801063352.50813-1-raykar.ath@gmail.com","subject":"Re: [GSoC] [PATCH v3] submodule--helper: introduce add-config subcommand","fromName":"Kaartic Sivaraam","fromEmail":"kaartic.sivaraam@gmail.com","sentAt":"2021-08-05T18:25:28Z","receivedAt":"2021-08-05T18:25:42Z","isPatch":true,"sender":{"key":"kaartic.sivaraam@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12448084?v=4"},"body":"On 01/08/21 12:03 pm, Atharva Raykar wrote:\n> Add a new \"add-config\" subcommand to `git submodule--helper` with the\n> goal of converting part of the shell code in git-submodule.sh related to\n> `git submodule add` into C code. This new subcommand sets the\n> configuration variables of a newly added submodule, by registering the\n> url in local git config, as well as the submodule name and path in the\n> .gitmodules file. It also sets 'submodule..active' to \"true\" if\n> the submodule path has not already been covered by any pathspec\n> specified in 'submodule.active'.\n> \n> This is meant to be a faithful conversion from shell to C, although we\n> add comments to areas that could be improved in future patches, after\n> the conversion has settled.\n> \n> Signed-off-by: Atharva Raykar \n> Mentored-by: Christian Couder \n> Mentored-by: Shourya Shukla \n> Based-on-patch-by: Shourya Shukla \n> Based-on-patch-by: Prathamesh Chavan \n> ---\n>\n\nThe v3 mostly looks good to me. Just one style nit:\n\n> [ ... ]\n>\n> +static void configure_added_submodule(struct add_data *add_data)\n> +{\n> +\tchar *key;\n> +\tchar *val = NULL;\n> +\tstruct child_process add_submod = CHILD_PROCESS_INIT;\n> +\tstruct child_process add_gitmodules = CHILD_PROCESS_INIT;\n> +\n> +\tkey = xstrfmt(\"submodule.%s.url\", add_data->sm_name);\n> +\tgit_config_set_gently(key, add_data->realrepo);\n> +\tfree(key);\n> +\n> +\tadd_submod.git_cmd = 1;\n> +\tstrvec_pushl(&add_submod.args, \"add\",\n> +\t\t \"--no-warn-embedded-repo\", NULL);\n> +\tif (add_data->force)\n> +\t\tstrvec_push(&add_submod.args, \"--force\");\n> +\tstrvec_pushl(&add_submod.args, \"--\", add_data->sm_path, NULL);\n> +\n> +\tif (run_command(&add_submod))\n> +\t\tdie(_(\"Failed to add submodule '%s'\"), add_data->sm_path);\n> +\n> +\tif (config_submodule_in_gitmodules(add_data->sm_name, \"path\", add_data->sm_path) ||\n> +\t config_submodule_in_gitmodules(add_data->sm_name, \"url\", add_data->repo))\n> +\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n> +\tif (add_data->branch)\n> +\t\tif (config_submodule_in_gitmodules(add_data->sm_name,\n> +\t\t\t\t\t\t \"branch\", add_data->branch))\n> +\t\t\tdie(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n> +\n\nAs the body of if(add->branch) has a nested if in it and totally spans 3 lines, it might\nbe a good idea to wrap it in braces like so:\n\n if (add_data->branch) {\n if (config_submodule_in_gitmodules(add_data->sm_name,\n \"branch\", add_data->branch))\n die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n }\n\n\n... or collapse both conditionals into a single if like so:\n\n if (add_data->branch &&\n config_submodule_in_gitmodules(add_data->sm_name, \"branch\", add_data->branch))\n die(_(\"Failed to register submodule '%s'\"), add_data->sm_path);\n\n-- \nSivaraam\n"}]}