| Message ID | 20210728115304.80643-1-raykar.ath@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [GSoC,v2] submodule--helper: introduce add-config subcommand | expand |
Hi Atharva, On 28/07/21 5:23 pm, Atharva Raykar wrote: > Add a new "add-config" subcommand to `git submodule--helper` with the > goal of converting part of the shell code in git-submodule.sh related to > `git submodule add` into C code. This new subcommand sets the > configuration variables of a newly added submodule, by registering the > url in local git config, as well as the submodule name and path in the > .gitmodules file. It also sets 'submodule.<name>.active' to "true" if > the submodule path has not already been covered by any pathspec > specified in 'submodule.active'. > > This is meant to be a faithful conversion from shell to C, with only one > minor change: A warning is emitted if no value is specified in > 'submodule.active', ie, the config looks like: "[submodule] active\n", > because it is an invalid configuration. It would be helpful to let the > user know that the pathspec is unset, and the value of > 'submodule.<name>.active' might be set to 'true' so that they can > rectify their configuration and prevent future surprises (especially > given that the latter variable has a higher priority than the former). > v2 doesn't have the warning that this paragraph describes. So, this could be dropped. > [ snip ] > > A comment has been > added to explain that only one value of 'submodule.active' is obtained > to check if we need to call is_submodule_active() at all. > This could be me likely not understanding this properly. Anyways, where is this comment in the code? I only see a comment about how 'is_submodule_active' iterates over all values. I couldn't find any "one value" reference in it. > Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> > Mentored-by: Christian Couder <christian.couder@gmail.com> > Mentored-by: Shourya Shukla <periperidip@gmail.com> > Based-on-patch-by: Shourya Shukla <periperidip@gmail.com> > Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com> > --- > > Changes since v1: > > * Remove the extra handling for the case where submodule.active is valueless, as > Junio pointed out that it is better dealt with in a cleanup patch. > > * Do not discard error returns from 'config_submodule_in_gitmodules()', and also > ensure that any calls to it in 'configure_added_submodule()' die on failure, > like with the original shell porcelain. > > * Style fixes. > > builtin/submodule--helper.c | 120 ++++++++++++++++++++++++++++++++++++ > git-submodule.sh | 28 +-------- > 2 files changed, 121 insertions(+), 27 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 862053c9f2..60b47492cb 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2936,6 +2936,125 @@ static int add_clone(int argc, const char **argv, const char *prefix) > return 0; > } > > +static int config_submodule_in_gitmodules(const char *name, const char *var, const char *value) > +{ > + char *key; > + int ret; > + > + if (!is_writing_gitmodules_ok()) > + die(_("please make sure that the .gitmodules file is in the working tree")); > + > + key = xstrfmt("submodule.%s.%s", name, var); > + ret = config_set_in_gitmodules_file_gently(key, value); > + free(key); > + > + return ret; > +} > + > +static void configure_added_submodule(struct add_data *add_data) > +{ > + char *key; > + char *val = NULL; > + struct child_process add_submod = CHILD_PROCESS_INIT; > + struct child_process add_gitmodules = CHILD_PROCESS_INIT; > + > + key = xstrfmt("submodule.%s.url", add_data->sm_name); > + git_config_set_gently(key, add_data->realrepo); > + free(key); > + > + add_submod.git_cmd = 1; > + strvec_pushl(&add_submod.args, "add", > + "--no-warn-embedded-repo", NULL); > + if (add_data->force) > + strvec_push(&add_submod.args, "--force"); > + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); > + > + if (run_command(&add_submod)) > + die(_("Failed to add submodule '%s'"), add_data->sm_path); > + > + if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || > + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo)) > + die(_("Failed to register submodule '%s'"), add_data->sm_path); > + > + if (add_data->branch) > + if (config_submodule_in_gitmodules(add_data->sm_name, > + "branch", add_data->branch)) > + die(_("Failed to register submodule '%s'"), add_data->sm_path); > + > + add_gitmodules.git_cmd = 1; > + strvec_pushl(&add_gitmodules.args, > + "add", "--force", "--", ".gitmodules", NULL); > + > + if (run_command(&add_gitmodules)) > + die(_("Failed to register submodule '%s'"), add_data->sm_path); > + We could restructure this portion like so ... -- 8< -- add_gitmodules.git_cmd = 1; strvec_pushl(&add_gitmodules.args, "add", "--force", "--", ".gitmodules", NULL); if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo) || (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name, "branch", add_data->branch)) || run_command(&add_gitmodules)) die(_("Failed to register submodule '%s'"), add_data->sm_path); -- >8 -- .. to avoid the redundant "Failed to register submodule ..." error message. Whether the restructured version has poor readability or not is debatable, though. > + /* > + * NEEDSWORK: In a multi-working-tree world this needs to be > + * set in the per-worktree config. > + * It might be a good idea to differentiate the NEEDSWORK comment from an informative comment about the code snippet. Also, you could add another NEEDSWORK/TODO comment regarding the change to 'is_submodule_active' which you mention before[1]. [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/ > + * If submodule.active does not exist, or if the pathspec was unset, > + * we will activate this module unconditionally. > + * > + * Otherwise, we ask is_submodule_active(), which iterates > + * through all the values of 'submodule.active' to determine > + * if this module is already active. > + */ > + if (git_config_get_string("submodule.active", &val) || > + !is_submodule_active(the_repository, add_data->sm_path)) { > + key = xstrfmt("submodule.%s.active", add_data->sm_name); > + git_config_set_gently(key, "true"); > + free(key); > + } It might be a good idea to expand this condition similar to the scripted version, to retain the following comment which seems like a useful one to keep. > [ snip ] > > - if git config --get submodule.active >/dev/null > - then > - # If the submodule being adding isn't already covered by the > - # current configured pathspec, set the submodule's active flag > - if ! git submodule--helper is-active "$sm_path" > - then > - git config submodule."$sm_name".active "true" > - fi > - else > - git config submodule."$sm_name".active "true" > - fi > + git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name" > } > > # >
On 29/07/21 11:05 pm, Atharva Raykar wrote: > (apologies for the reflowed text, seems to only happen when replying to > this message?? Won't affect this response much though) > In case you're using thunderbird then you could see if the following helps: http://kb.mozillazine.org/Plain_text_e-mail_%28Thunderbird%29#Flowed_format > On 29/07/21 01:21, Kaartic Sivaraam wrote: >> Hi Atharva, >> >> On 28/07/21 5:23 pm, Atharva Raykar wrote: >>> Add a new "add-config" subcommand to `git submodule--helper` with the >>> goal of converting part of the shell code in git-submodule.sh related to >>> `git submodule add` into C code. This new subcommand sets the >>> configuration variables of a newly added submodule, by registering the >>> url in local git config, as well as the submodule name and path in the >>> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if >>> the submodule path has not already been covered by any pathspec >>> specified in 'submodule.active'. >>> >>> This is meant to be a faithful conversion from shell to C, with only one >>> minor change: A warning is emitted if no value is specified in >>> 'submodule.active', ie, the config looks like: "[submodule] active\n", >>> because it is an invalid configuration. It would be helpful to let the >>> user know that the pathspec is unset, and the value of >>> 'submodule.<name>.active' might be set to 'true' so that they can >>> rectify their configuration and prevent future surprises (especially >>> given that the latter variable has a higher priority than the former). >>> >> >> v2 doesn't have the warning that this paragraph describes. So, this could >> be dropped. > > My bad, looks like I forgot to edit the commit message. > >>> [ snip ] >>> >>> A comment has been >>> added to explain that only one value of 'submodule.active' is obtained >>> to check if we need to call is_submodule_active() at all. >>> >> >> This could be me likely not understanding this properly. Anyways, where >> is this comment in the code? I only see a comment about how >> 'is_submodule_active' >> iterates over all values. I couldn't find any "one value" reference in it. > > Looks like my comment does not explain it clearly. It would have made > more sense to start the comment with "If there is no value found for > submodule.active", but I think instead of modifying that comment (which > is clear enough as it is), I'll make the commit message better, by > removing the mention of the "we check one value". > > It seems like the line: > > if (git_config_get_string("submodule.active", &val) > > makes it clear that a single string is being queried first. The larger > point was about why that conditional was needed, if we were going to > call 'is_submodule_active()' to retrieve the value anyway. > Ah. Now I get the idea. A rephrasing might indeed make this clear. >>> + if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || >>> + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo)) >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); >>> + >>> + if (add_data->branch) >>> + if (config_submodule_in_gitmodules(add_data->sm_name, >>> + "branch", add_data->branch)) >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); >>> + >>> + add_gitmodules.git_cmd = 1; >>> + strvec_pushl(&add_gitmodules.args, >>> + "add", "--force", "--", ".gitmodules", NULL); >>> + >>> + if (run_command(&add_gitmodules)) >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); >>> + >> >> We could restructure this portion like so ... >> >> -- 8< -- >> add_gitmodules.git_cmd = 1; >> strvec_pushl(&add_gitmodules.args, >> "add", "--force", "--", ".gitmodules", NULL); >>> >> if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || >> config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo) || >> (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name, >> "branch", add_data->branch)) || >> run_command(&add_gitmodules)) >> die(_("Failed to register submodule '%s'"), >> add_data->sm_path); >> -- >8 -- >> >> .. to avoid the redundant "Failed to register submodule ..." error message. >> Whether the restructured version has poor readability or not is debatable, though. > > Yeah, I felt the redundancy in this case was okay, I find that big > conditional rather hard to read. > I tried to make it as easy to read as possible but its a really long one indeed. So, I could understand. But the redundancy bothered me a bit ;-) >>> + /* >>> + * NEEDSWORK: In a multi-working-tree world this needs to be >>> + * set in the per-worktree config. >>> + * >> >> It might be a good idea to differentiate the NEEDSWORK comment from an >> informative comment about the code snippet. > > Okay. I suppose you mean give this part it's own closing delimiter and > start the next line with a new multiline comment. > Yeah. I did mean this. > If you meant something else, do let me know. > >> Also, you could add another NEEDSWORK/TODO comment regarding the change >> to 'is_submodule_active' which you mention before[1]. >> >> [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/ > > Good point. I'll add it. > >>> + * If submodule.active does not exist, or if the pathspec was unset, >>> + * we will activate this module unconditionally. >>> + * >>> + * Otherwise, we ask is_submodule_active(), which iterates >>> + * through all the values of 'submodule.active' to determine >>> + * if this module is already active. >>> + */ >>> + if (git_config_get_string("submodule.active", &val) || >>> + !is_submodule_active(the_repository, add_data->sm_path)) { >>> + key = xstrfmt("submodule.%s.active", add_data->sm_name); >>> + git_config_set_gently(key, "true"); >>> + free(key); >>> + } >> >> It might be a good idea to expand this condition similar to the scripted version, >> to retain the following comment which seems like a useful one to keep. > > I felt that this version had less redundant code, and hence seemed more > readable than the expanded conditional in shell. > > For comparison this is the same code imitating the shell version: > > if (!git_config_get_string("submodule.active", &var) && var) { > > /* > * If the submodule being added isn't already covered by the > * current configured pathspec, set the submodule's active flag > */ > if (!is_submodule_active(the_repository, info->sm_path)) { > key = xstrfmt("submodule.%s.active", info->sm_name); > git_config_set_gently(key, "true"); > free(key); > } > > } else { > key = xstrfmt("submodule.%s.active", info->sm_name); > git_config_set_gently(key, "true"); > free(key); > } > > It repeats the string allocation and freeing, and also is a lot more > code to parse mentally while reading. The shorter version that I used > does not feel more "clever" to me than this either. > > As for the comment, I felt that the new one I introduced (Otherwise, we > ask ...) covers the same ground. > I think the comment you introduced only mentions that 'is_submodule_active' iterates over configs to determine that a submodule is active. It doesn't mention that we set the submodule's active flag if the submodule is not covered by the current configured pathspec, which is what the original tries to convey. Correct me if I missed anything. > I am open to reverting to the expanded conditional, but it would be nice > if you could help me understand the motivation behind why it should be done. > I'm not against short-circuiting the conditional. I suggested expanding the conditional so that we get a structure similar to the scripted version. That way we could keep the original comment close to the inside conditional where it felt relevant :) >>> [ snip ] >>> >>> - if git config --get submodule.active >/dev/null >>> - then >>> - # If the submodule being adding isn't already covered by the >>> - # current configured pathspec, set the submodule's active flag >>> - if ! git submodule--helper is-active "$sm_path" >>> - then >>> - git config submodule."$sm_name".active "true" >>> - fi >>> - else >>> - git config submodule."$sm_name".active "true" >>> - fi >>> + git submodule--helper add-config ${force:+--force} >>> ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" >>> --path "$sm_path" --name "$sm_name" >>> } >>> # >>> >> >
On Fri, Jul 30, 2021 at 1:00 AM Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote: > > On 29/07/21 11:05 pm, Atharva Raykar wrote: > > (apologies for the reflowed text, seems to only happen when replying to > > this message?? Won't affect this response much though) > > > > In case you're using thunderbird then you could see if the following helps: > > http://kb.mozillazine.org/Plain_text_e-mail_%28Thunderbird%29#Flowed_format Yeah, I have pretty much been following the setup that is in the git-format-patch [1] documentation. It worked fine until the last couple of days. The mailing list is now rejecting all my mails. My guess is because Thunderbird is forcing a 'Content-Transfer-Encoding: 7-bit' which I read causes problems with this list [2]. Strangely, so far, this header is added only when I send mails to git@vger.kernel.org, not elsewhere. (sending this from GMail for now) Here's the error message: ------8<------8<------8<------ <git@vger.kernel.org>: host 23.128.96.18[23.128.96.18] said: 550 5.7.1 Content-Policy reject msg: Wrong MIME labeling on 8-bit character texts. BF:<H 0>; S229739AbhG2RfR (in reply to end of DATA command) ------8<------8<------8<------ I'll try fixing my mail situation today, and if I still have problems, I'll bring it up on a separate thread. [1] https://git-scm.com/docs/git-format-patch#_approach_1_add_on [2] https://lore.kernel.org/git/alpine.DEB.2.20.1611031554100.3108@virtualbox/ > > On 29/07/21 01:21, Kaartic Sivaraam wrote: > >> Hi Atharva, > >> > >> On 28/07/21 5:23 pm, Atharva Raykar wrote: > >>> Add a new "add-config" subcommand to `git submodule--helper` with the > >>> goal of converting part of the shell code in git-submodule.sh related to > >>> `git submodule add` into C code. This new subcommand sets the > >>> configuration variables of a newly added submodule, by registering the > >>> url in local git config, as well as the submodule name and path in the > >>> .gitmodules file. It also sets 'submodule.<name>.active' to "true" if > >>> the submodule path has not already been covered by any pathspec > >>> specified in 'submodule.active'. > >>> > >>> This is meant to be a faithful conversion from shell to C, with only one > >>> minor change: A warning is emitted if no value is specified in > >>> 'submodule.active', ie, the config looks like: "[submodule] active\n", > >>> because it is an invalid configuration. It would be helpful to let the > >>> user know that the pathspec is unset, and the value of > >>> 'submodule.<name>.active' might be set to 'true' so that they can > >>> rectify their configuration and prevent future surprises (especially > >>> given that the latter variable has a higher priority than the former). > >>> > >> > >> v2 doesn't have the warning that this paragraph describes. So, this could > >> be dropped. > > > > My bad, looks like I forgot to edit the commit message. > > > >>> [ snip ] > >>> > >>> A comment has been > >>> added to explain that only one value of 'submodule.active' is obtained > >>> to check if we need to call is_submodule_active() at all. > >>> > >> > >> This could be me likely not understanding this properly. Anyways, where > >> is this comment in the code? I only see a comment about how > >> 'is_submodule_active' > >> iterates over all values. I couldn't find any "one value" reference in it. > > > > Looks like my comment does not explain it clearly. It would have made > > more sense to start the comment with "If there is no value found for > > submodule.active", but I think instead of modifying that comment (which > > is clear enough as it is), I'll make the commit message better, by > > removing the mention of the "we check one value". > > > > It seems like the line: > > > > if (git_config_get_string("submodule.active", &val) > > > > makes it clear that a single string is being queried first. The larger > > point was about why that conditional was needed, if we were going to > > call 'is_submodule_active()' to retrieve the value anyway. > > > > Ah. Now I get the idea. A rephrasing might indeed make this clear. > > >>> + if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || > >>> + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >>> + if (add_data->branch) > >>> + if (config_submodule_in_gitmodules(add_data->sm_name, > >>> + "branch", add_data->branch)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >>> + add_gitmodules.git_cmd = 1; > >>> + strvec_pushl(&add_gitmodules.args, > >>> + "add", "--force", "--", ".gitmodules", NULL); > >>> + > >>> + if (run_command(&add_gitmodules)) > >>> + die(_("Failed to register submodule '%s'"), add_data->sm_path); > >>> + > >> > >> We could restructure this portion like so ... > >> > >> -- 8< -- > >> add_gitmodules.git_cmd = 1; > >> strvec_pushl(&add_gitmodules.args, > >> "add", "--force", "--", ".gitmodules", NULL); > >>> > >> if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || > >> config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo) || > >> (add_data->branch && config_submodule_in_gitmodules(add_data->sm_name, > >> "branch", add_data->branch)) || > >> run_command(&add_gitmodules)) > >> die(_("Failed to register submodule '%s'"), > >> add_data->sm_path); > >> -- >8 -- > >> > >> .. to avoid the redundant "Failed to register submodule ..." error message. > >> Whether the restructured version has poor readability or not is debatable, though. > > > > Yeah, I felt the redundancy in this case was okay, I find that big > > conditional rather hard to read. > > > > I tried to make it as easy to read as possible but its a really long one > indeed. So, I could understand. But the redundancy bothered me a bit ;-) > > >>> + /* > >>> + * NEEDSWORK: In a multi-working-tree world this needs to be > >>> + * set in the per-worktree config. > >>> + * > >> > >> It might be a good idea to differentiate the NEEDSWORK comment from an > >> informative comment about the code snippet. > > > > Okay. I suppose you mean give this part it's own closing delimiter and > > start the next line with a new multiline comment. > > > > Yeah. I did mean this. > > > If you meant something else, do let me know. > > > >> Also, you could add another NEEDSWORK/TODO comment regarding the change > >> to 'is_submodule_active' which you mention before[1]. > >> > >> [1]: https://public-inbox.org/git/a6de518a-d4a2-5a2b-28e2-ca8b62f2c85b@gmail.com/ > > > > Good point. I'll add it. > > > >>> + * If submodule.active does not exist, or if the pathspec was unset, > >>> + * we will activate this module unconditionally. > >>> + * > >>> + * Otherwise, we ask is_submodule_active(), which iterates > >>> + * through all the values of 'submodule.active' to determine > >>> + * if this module is already active. > >>> + */ > >>> + if (git_config_get_string("submodule.active", &val) || > >>> + !is_submodule_active(the_repository, add_data->sm_path)) { > >>> + key = xstrfmt("submodule.%s.active", add_data->sm_name); > >>> + git_config_set_gently(key, "true"); > >>> + free(key); > >>> + } > >> > >> It might be a good idea to expand this condition similar to the scripted version, > >> to retain the following comment which seems like a useful one to keep. > > > > I felt that this version had less redundant code, and hence seemed more > > readable than the expanded conditional in shell. > > > > For comparison this is the same code imitating the shell version: > > > > if (!git_config_get_string("submodule.active", &var) && var) { > > > > /* > > * If the submodule being added isn't already covered by the > > * current configured pathspec, set the submodule's active flag > > */ > > if (!is_submodule_active(the_repository, info->sm_path)) { > > key = xstrfmt("submodule.%s.active", info->sm_name); > > git_config_set_gently(key, "true"); > > free(key); > > } > > > > } else { > > key = xstrfmt("submodule.%s.active", info->sm_name); > > git_config_set_gently(key, "true"); > > free(key); > > } > > > > It repeats the string allocation and freeing, and also is a lot more > > code to parse mentally while reading. The shorter version that I used > > does not feel more "clever" to me than this either. > > > > As for the comment, I felt that the new one I introduced (Otherwise, we > > ask ...) covers the same ground. > > > > I think the comment you introduced only mentions that 'is_submodule_active' > iterates over configs to determine that a submodule is active. It doesn't mention > that we set the submodule's active flag if the submodule is not covered by the > current configured pathspec, which is what the original tries to convey. > Correct me if I missed anything. > > > I am open to reverting to the expanded conditional, but it would be nice > > if you could help me understand the motivation behind why it should be done. > > > > I'm not against short-circuiting the conditional. I suggested expanding the conditional > so that we get a structure similar to the scripted version. That way we could keep the > original comment close to the inside conditional where it felt relevant :) Ah okay, so the reason is so that we could keep the structure similar to retain the comment? Okay, I'll change that. > >>> [ snip ] > >>> > >>> - if git config --get submodule.active >/dev/null > >>> - then > >>> - # If the submodule being adding isn't already covered by the > >>> - # current configured pathspec, set the submodule's active flag > >>> - if ! git submodule--helper is-active "$sm_path" > >>> - then > >>> - git config submodule."$sm_name".active "true" > >>> - fi > >>> - else > >>> - git config submodule."$sm_name".active "true" > >>> - fi > >>> + git submodule--helper add-config ${force:+--force} > >>> ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" > >>> --path "$sm_path" --name "$sm_name" > >>> } > >>> # > >>> > >> > > > > > -- > Sivaraam
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 862053c9f2..60b47492cb 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2936,6 +2936,125 @@ static int add_clone(int argc, const char **argv, const char *prefix) return 0; } +static int config_submodule_in_gitmodules(const char *name, const char *var, const char *value) +{ + char *key; + int ret; + + if (!is_writing_gitmodules_ok()) + die(_("please make sure that the .gitmodules file is in the working tree")); + + key = xstrfmt("submodule.%s.%s", name, var); + ret = config_set_in_gitmodules_file_gently(key, value); + free(key); + + return ret; +} + +static void configure_added_submodule(struct add_data *add_data) +{ + char *key; + char *val = NULL; + struct child_process add_submod = CHILD_PROCESS_INIT; + struct child_process add_gitmodules = CHILD_PROCESS_INIT; + + key = xstrfmt("submodule.%s.url", add_data->sm_name); + git_config_set_gently(key, add_data->realrepo); + free(key); + + add_submod.git_cmd = 1; + strvec_pushl(&add_submod.args, "add", + "--no-warn-embedded-repo", NULL); + if (add_data->force) + strvec_push(&add_submod.args, "--force"); + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); + + if (run_command(&add_submod)) + die(_("Failed to add submodule '%s'"), add_data->sm_path); + + if (config_submodule_in_gitmodules(add_data->sm_name, "path", add_data->sm_path) || + config_submodule_in_gitmodules(add_data->sm_name, "url", add_data->repo)) + die(_("Failed to register submodule '%s'"), add_data->sm_path); + + if (add_data->branch) + if (config_submodule_in_gitmodules(add_data->sm_name, + "branch", add_data->branch)) + die(_("Failed to register submodule '%s'"), add_data->sm_path); + + add_gitmodules.git_cmd = 1; + strvec_pushl(&add_gitmodules.args, + "add", "--force", "--", ".gitmodules", NULL); + + if (run_command(&add_gitmodules)) + die(_("Failed to register submodule '%s'"), add_data->sm_path); + + /* + * NEEDSWORK: In a multi-working-tree world this needs to be + * set in the per-worktree config. + * + * If submodule.active does not exist, or if the pathspec was unset, + * we will activate this module unconditionally. + * + * Otherwise, we ask is_submodule_active(), which iterates + * through all the values of 'submodule.active' to determine + * if this module is already active. + */ + if (git_config_get_string("submodule.active", &val) || + !is_submodule_active(the_repository, add_data->sm_path)) { + key = xstrfmt("submodule.%s.active", add_data->sm_name); + git_config_set_gently(key, "true"); + free(key); + } +} + +static int add_config(int argc, const char **argv, const char *prefix) +{ + int force = 0; + struct add_data add_data = ADD_DATA_INIT; + + struct option options[] = { + OPT_STRING('b', "branch", &add_data.branch, + N_("branch"), + N_("branch of repository to store in " + "the submodule configuration")), + OPT_STRING(0, "url", &add_data.repo, + N_("string"), + N_("url to clone submodule from")), + OPT_STRING(0, "resolved-url", &add_data.realrepo, + N_("string"), + N_("url to clone the submodule from, after it has " + "been dereferenced relative to parent's url, " + "in the case where <url> is a relative url")), + OPT_STRING(0, "path", &add_data.sm_path, + N_("path"), + N_("where the new submodule will be cloned to")), + OPT_STRING(0, "name", &add_data.sm_name, + N_("string"), + N_("name of the new submodule")), + OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"), + PARSE_OPT_NOCOMPLETE), + OPT_END() + }; + + const char *const usage[] = { + N_("git submodule--helper add-config " + "[--force|-f] [--branch|-b <branch>] " + "--url <url> --resolved-url <resolved-url> " + "--path <path> --name <name>"), + NULL + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + + if (argc) + usage_with_options(usage, options); + + add_data.force = !!force; + configure_added_submodule(&add_data); + + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2949,6 +3068,7 @@ static struct cmd_struct commands[] = { {"name", module_name, 0}, {"clone", module_clone, 0}, {"add-clone", add_clone, 0}, + {"add-config", add_config, 0}, {"update-module-mode", module_update_module_mode, 0}, {"update-clone", update_clone, 0}, {"ensure-core-worktree", ensure_core_worktree, 0}, diff --git a/git-submodule.sh b/git-submodule.sh index 053daf3724..f713cb113c 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -242,33 +242,7 @@ cmd_add() fi git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit - git config submodule."$sm_name".url "$realrepo" - - git add --no-warn-embedded-repo $force "$sm_path" || - die "fatal: $(eval_gettext "Failed to add submodule '\$sm_path'")" - - git submodule--helper config submodule."$sm_name".path "$sm_path" && - git submodule--helper config submodule."$sm_name".url "$repo" && - if test -n "$branch" - then - git submodule--helper config submodule."$sm_name".branch "$branch" - fi && - git add --force .gitmodules || - die "fatal: $(eval_gettext "Failed to register submodule '\$sm_path'")" - - # NEEDSWORK: In a multi-working-tree world, this needs to be - # set in the per-worktree config. - if git config --get submodule.active >/dev/null - then - # If the submodule being adding isn't already covered by the - # current configured pathspec, set the submodule's active flag - if ! git submodule--helper is-active "$sm_path" - then - git config submodule."$sm_name".active "true" - fi - else - git config submodule."$sm_name".active "true" - fi + git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name" } #
Add a new "add-config" subcommand to `git submodule--helper` with the goal of converting part of the shell code in git-submodule.sh related to `git submodule add` into C code. This new subcommand sets the configuration variables of a newly added submodule, by registering the url in local git config, as well as the submodule name and path in the .gitmodules file. It also sets 'submodule.<name>.active' to "true" if the submodule path has not already been covered by any pathspec specified in 'submodule.active'. This is meant to be a faithful conversion from shell to C, with only one minor change: A warning is emitted if no value is specified in 'submodule.active', ie, the config looks like: "[submodule] active\n", because it is an invalid configuration. It would be helpful to let the user know that the pathspec is unset, and the value of 'submodule.<name>.active' might be set to 'true' so that they can rectify their configuration and prevent future surprises (especially given that the latter variable has a higher priority than the former). The structure of the conditional to check if we need to set the 'active' toggle looks different from the shell version -- but behaves the same. The change was made to decrease code duplication. A comment has been added to explain that only one value of 'submodule.active' is obtained to check if we need to call is_submodule_active() at all. Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <periperidip@gmail.com> Based-on-patch-by: Shourya Shukla <periperidip@gmail.com> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com> --- Changes since v1: * Remove the extra handling for the case where submodule.active is valueless, as Junio pointed out that it is better dealt with in a cleanup patch. * Do not discard error returns from 'config_submodule_in_gitmodules()', and also ensure that any calls to it in 'configure_added_submodule()' die on failure, like with the original shell porcelain. * Style fixes. builtin/submodule--helper.c | 120 ++++++++++++++++++++++++++++++++++++ git-submodule.sh | 28 +-------- 2 files changed, 121 insertions(+), 27 deletions(-)