| Message ID | 20210614125157.99426-4-raykar.ath@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | submodule--helper: introduce subcommands for sh to C conversion | expand |
Atharva Raykar <raykar.ath@gmail.com> writes: > --- > builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++ > git-submodule.sh | 28 +-------- > 2 files changed, 120 insertions(+), 27 deletions(-) > I do not have enough expertise to judge the entire content of this patch. I would like, however, to propose a slight code change for the sake of readability. > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 6dffaeb6cb..c4b2aa6537 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix) > return 0; > } > > +static void configure_added_submodule(struct add_data *add_data) > +{ > + char *key, *submod_pathspec = NULL; > + struct child_process add_submod = CHILD_PROCESS_INIT; > + struct child_process add_gitmodules = CHILD_PROCESS_INIT; > + int pathspec_key_exists, activate = 0; > + > + key = xstrfmt("submodule.%s.url", add_data->sm_name); > + git_config_set_gently(key, add_data->realrepo); > + free(key); > + > + add_submod.git_cmd = 1; > + strvec_pushl(&add_submod.args, "add", > + "--no-warn-embedded-repo", NULL); > + if (add_data->force) > + strvec_push(&add_submod.args, "--force"); > + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); > + > + if (run_command(&add_submod)) > + die(_("Failed to add submodule '%s'"), add_data->sm_path); > + > + key = xstrfmt("submodule.%s.path", add_data->sm_name); > + config_set_in_gitmodules_file_gently(key, add_data->sm_path); > + free(key); This above three lines of code is very similar to the two operations that follows (including the one inside the `if (add_data->branch)` condition. So [ ... ] > + key = xstrfmt("submodule.%s.url", add_data->sm_name); > + config_set_in_gitmodules_file_gently(key, add_data->repo); > + free(key); > + if (add_data->branch) { > + key = xstrfmt("submodule.%s.branch", add_data->sm_path); > + config_set_in_gitmodules_file_gently(key, add_data->branch); > + free(key); > + } > + [ ... ] it might be worth to write a small wrapper that will perform: (1) `xstrfmt()` on the specified config section, (2) set the configuration in the file and (3) free()'ing the variable inside the wrapper. Thus, most of these code will become one liners that is easier to read (given the function is properly named :) ). After abstracting the code on the wrapper, this code will become something like: function_properly_named("submodule.%s.path", add_data->sm_name, add_data->sm_path); function_properly_named("submodule.%s.url", add_data->sm_name, add_data->repo); if (add_data->branch) function_properly_named("submodule.%s.branch", add_data->sm_path, add_data->branch); Just as an example, here's a diff to demonstrate the argument: -- >8 -- diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 65f79fbd53..48ea909f51 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix) return 0; } +void add_config_in_submodules_file(const char *keyfmt, const char *submodule, + const char *value) +{ + char *key = xstrfmt(keyfmt, submodule); + config_set_in_gitmodules_file_gently(key, value); + free(key); +} + static void configure_added_submodule(struct add_data *add_data) { char *key, *submod_pathspec = NULL; @@ -2955,17 +2963,10 @@ static void configure_added_submodule(struct add_data *add_data) if (run_command(&add_submod)) die(_("Failed to add submodule '%s'"), add_data->sm_path); - key = xstrfmt("submodule.%s.path", add_data->sm_name); - config_set_in_gitmodules_file_gently(key, add_data->sm_path); - free(key); - key = xstrfmt("submodule.%s.url", add_data->sm_name); - config_set_in_gitmodules_file_gently(key, add_data->repo); - free(key); - if (add_data->branch) { - key = xstrfmt("submodule.%s.branch", add_data->sm_path); - config_set_in_gitmodules_file_gently(key, add_data->branch); - free(key); - } + add_config_in_submodules_file("submodule.%s.path", add_data->sm_name, add_data->sm_path); + add_config_in_submodules_file("submodule.%s.url", add_data->sm_name, add_data->repo); + if (add_data->branch) + add_config_in_submodules_file("submodule.%s.branch", add_data->sm_path, add_data->branch); add_gitmodules.git_cmd = 1; strvec_pushl(&add_gitmodules.args, -- >8 -- A proper name than "add_config_in_submodules_file" should be considered - I'm not very good in naming things. These change does (should) not change the behavior of code, even though I believe it make the code simpler to read, I do not have strong opinions about it. So, take this proposal as you wish.
On Mon, Jun 14, 2021 at 3:53 PM Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > Just as an example, here's a diff to demonstrate the argument: > -- >8 -- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix) > +void add_config_in_submodules_file(const char *keyfmt, const char *submodule, > + const char *value) > +{ > + char *key = xstrfmt(keyfmt, submodule); > + config_set_in_gitmodules_file_gently(key, value); > + free(key); > +} The new function should be `static`, of course.
On 15-Jun-2021, at 01:21, Rafael Silva <rafaeloliveira.cs@gmail.com> wrote: > > > Atharva Raykar <raykar.ath@gmail.com> writes: > >> --- >> builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++ >> git-submodule.sh | 28 +-------- >> 2 files changed, 120 insertions(+), 27 deletions(-) >> > > I do not have enough expertise to judge the entire content of this > patch. I would like, however, to propose a slight code change for the > sake of readability. > >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 6dffaeb6cb..c4b2aa6537 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix) >> return 0; >> } >> >> +static void configure_added_submodule(struct add_data *add_data) >> +{ >> + char *key, *submod_pathspec = NULL; >> + struct child_process add_submod = CHILD_PROCESS_INIT; >> + struct child_process add_gitmodules = CHILD_PROCESS_INIT; >> + int pathspec_key_exists, activate = 0; >> + >> + key = xstrfmt("submodule.%s.url", add_data->sm_name); >> + git_config_set_gently(key, add_data->realrepo); >> + free(key); >> + >> + add_submod.git_cmd = 1; >> + strvec_pushl(&add_submod.args, "add", >> + "--no-warn-embedded-repo", NULL); >> + if (add_data->force) >> + strvec_push(&add_submod.args, "--force"); >> + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); >> + >> + if (run_command(&add_submod)) >> + die(_("Failed to add submodule '%s'"), add_data->sm_path); >> + >> + key = xstrfmt("submodule.%s.path", add_data->sm_name); >> + config_set_in_gitmodules_file_gently(key, add_data->sm_path); >> + free(key); > > This above three lines of code is very similar to the two operations that > follows (including the one inside the `if (add_data->branch)` > condition. So [ ... ] > >> + key = xstrfmt("submodule.%s.url", add_data->sm_name); >> + config_set_in_gitmodules_file_gently(key, add_data->repo); >> + free(key); >> + if (add_data->branch) { >> + key = xstrfmt("submodule.%s.branch", add_data->sm_path); >> + config_set_in_gitmodules_file_gently(key, add_data->branch); >> + free(key); >> + } >> + > > [ ... ] it might be worth to write a small wrapper that will perform: (1) > `xstrfmt()` on the specified config section, (2) set the configuration > in the file and (3) free()'ing the variable inside the wrapper. Thus, > most of these code will become one liners that is easier to read (given > the function is properly named :) ). > > After abstracting the code on the wrapper, this code will become > something like: > > > function_properly_named("submodule.%s.path", add_data->sm_name, add_data->sm_path); > function_properly_named("submodule.%s.url", add_data->sm_name, add_data->repo); > if (add_data->branch) > function_properly_named("submodule.%s.branch", add_data->sm_path, add_data->branch); > > > Just as an example, here's a diff to demonstrate the argument: > > -- >8 -- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 65f79fbd53..48ea909f51 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix) > return 0; > } > > +void add_config_in_submodules_file(const char *keyfmt, const char *submodule, > + const char *value) > +{ > + char *key = xstrfmt(keyfmt, submodule); > + config_set_in_gitmodules_file_gently(key, value); > + free(key); > +} > + > static void configure_added_submodule(struct add_data *add_data) > { > char *key, *submod_pathspec = NULL; > @@ -2955,17 +2963,10 @@ static void configure_added_submodule(struct add_data *add_data) > if (run_command(&add_submod)) > die(_("Failed to add submodule '%s'"), add_data->sm_path); > > - key = xstrfmt("submodule.%s.path", add_data->sm_name); > - config_set_in_gitmodules_file_gently(key, add_data->sm_path); > - free(key); > - key = xstrfmt("submodule.%s.url", add_data->sm_name); > - config_set_in_gitmodules_file_gently(key, add_data->repo); > - free(key); > - if (add_data->branch) { > - key = xstrfmt("submodule.%s.branch", add_data->sm_path); > - config_set_in_gitmodules_file_gently(key, add_data->branch); > - free(key); > - } > + add_config_in_submodules_file("submodule.%s.path", add_data->sm_name, add_data->sm_path); > + add_config_in_submodules_file("submodule.%s.url", add_data->sm_name, add_data->repo); > + if (add_data->branch) > + add_config_in_submodules_file("submodule.%s.branch", add_data->sm_path, add_data->branch); > > add_gitmodules.git_cmd = 1; > strvec_pushl(&add_gitmodules.args, > > -- >8 -- > > A proper name than "add_config_in_submodules_file" should be considered - I'm > not very good in naming things. > > These change does (should) not change the behavior of code, even though > I believe it make the code simpler to read, I do not have strong > opinions about it. So, take this proposal as you wish. I agree with you, this will make the code simpler to read. It also made me realise one thing that I did not replicate exactly from the shell code. The original shell code calls 'module_config()', which does an extra check to see if writing to '.gitmodules' is okay. I did not perform this check, and including that in the wrapper you propose will be a good idea. > -- > Thanks > Rafael
Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Jun 14, 2021 at 3:53 PM Rafael Silva > <rafaeloliveira.cs@gmail.com> wrote: >> Just as an example, here's a diff to demonstrate the argument: >> -- >8 -- >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> @@ -2934,6 +2934,14 @@ static int add_clone(int argc, const char **argv, const char *prefix) >> +void add_config_in_submodules_file(const char *keyfmt, const char *submodule, >> + const char *value) >> +{ >> + char *key = xstrfmt(keyfmt, submodule); >> + config_set_in_gitmodules_file_gently(key, value); >> + free(key); >> +} > > The new function should be `static`, of course. Good catch! Indeed, it should be `static`.
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 6dffaeb6cb..c4b2aa6537 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2935,6 +2935,124 @@ static int add_clone(int argc, const char **argv, const char *prefix) return 0; } +static void configure_added_submodule(struct add_data *add_data) +{ + char *key, *submod_pathspec = NULL; + struct child_process add_submod = CHILD_PROCESS_INIT; + struct child_process add_gitmodules = CHILD_PROCESS_INIT; + int pathspec_key_exists, activate = 0; + + key = xstrfmt("submodule.%s.url", add_data->sm_name); + git_config_set_gently(key, add_data->realrepo); + free(key); + + add_submod.git_cmd = 1; + strvec_pushl(&add_submod.args, "add", + "--no-warn-embedded-repo", NULL); + if (add_data->force) + strvec_push(&add_submod.args, "--force"); + strvec_pushl(&add_submod.args, "--", add_data->sm_path, NULL); + + if (run_command(&add_submod)) + die(_("Failed to add submodule '%s'"), add_data->sm_path); + + key = xstrfmt("submodule.%s.path", add_data->sm_name); + config_set_in_gitmodules_file_gently(key, add_data->sm_path); + free(key); + key = xstrfmt("submodule.%s.url", add_data->sm_name); + config_set_in_gitmodules_file_gently(key, add_data->repo); + free(key); + if (add_data->branch) { + key = xstrfmt("submodule.%s.branch", add_data->sm_path); + config_set_in_gitmodules_file_gently(key, add_data->branch); + free(key); + } + + add_gitmodules.git_cmd = 1; + strvec_pushl(&add_gitmodules.args, + "add", "--force", "--", ".gitmodules", NULL); + + if (run_command(&add_gitmodules)) + die(_("Failed to register submodule '%s'"), add_data->sm_path); + + /* + * NEEDSWORK: In a multi-working-tree world this needs to be + * set in the per-worktree config. + */ + pathspec_key_exists = !git_config_get_string("submodule.active", + &submod_pathspec); + if (pathspec_key_exists && !submod_pathspec) { + warning(_("The submodule.active configuration exists, but the " + "pathspec was unset. If the submodule is not already " + "active, the value of submodule.%s.active will be " + "be set to 'true'."), add_data->sm_name); + activate = 1; + } + + /* + * If submodule.active does not exist, or if the pathspec was unset, + * we will activate this module unconditionally. + * + * Otherwise, we ask is_submodule_active(), which iterates + * through all the values of 'submodule.active' to determine + * if this module is already active. + */ + if (!pathspec_key_exists || activate || + !is_submodule_active(the_repository, add_data->sm_path)) { + key = xstrfmt("submodule.%s.active", add_data->sm_name); + git_config_set_gently(key, "true"); + free(key); + } +} + +static int add_config(int argc, const char **argv, const char *prefix) +{ + int force = 0; + struct add_data add_data = ADD_DATA_INIT; + + struct option options[] = { + OPT_STRING('b', "branch", &add_data.branch, + N_("branch"), + N_("branch of repository to store in " + "the submodule configuration")), + OPT_STRING(0, "url", &add_data.repo, + N_("string"), + N_("url to clone submodule from")), + OPT_STRING(0, "resolved-url", &add_data.realrepo, + N_("string"), + N_("url to clone the submodule from, after it has " + "been dereferenced relative to parent's url, " + "in the case where <url> is a relative url")), + OPT_STRING(0, "path", &add_data.sm_path, + N_("path"), + N_("where the new submodule will be cloned to")), + OPT_STRING(0, "name", &add_data.sm_name, + N_("string"), + N_("name of the new submodule")), + OPT__FORCE(&force, N_("allow adding an otherwise ignored submodule path"), + PARSE_OPT_NOCOMPLETE), + OPT_END() + }; + + const char *const usage[] = { + N_("git submodule--helper add-config " + "[--force|-f] [--branch|-b <branch>] " + "--url <url> --resolved-url <resolved-url> " + "--path <path> --name <name>"), + NULL + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + + if (argc != 0) + usage_with_options(usage, options); + + add_data.force = !!force; + configure_added_submodule(&add_data); + + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2948,6 +3066,7 @@ static struct cmd_struct commands[] = { {"name", module_name, 0}, {"clone", module_clone, 0}, {"add-clone", add_clone, 0}, + {"add-config", add_config, 0}, {"update-module-mode", module_update_module_mode, 0}, {"update-clone", update_clone, 0}, {"ensure-core-worktree", ensure_core_worktree, 0}, diff --git a/git-submodule.sh b/git-submodule.sh index f71e1e5495..9826378fa6 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -242,33 +242,7 @@ cmd_add() fi git submodule--helper add-clone ${GIT_QUIET:+--quiet} ${force:+"--force"} ${progress:+"--progress"} ${branch:+--branch "$branch"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"--dissociate"} ${depth:+"$depth"} || exit - git config submodule."$sm_name".url "$realrepo" - - git add --no-warn-embedded-repo $force "$sm_path" || - die "$(eval_gettext "Failed to add submodule '\$sm_path'")" - - git submodule--helper config submodule."$sm_name".path "$sm_path" && - git submodule--helper config submodule."$sm_name".url "$repo" && - if test -n "$branch" - then - git submodule--helper config submodule."$sm_name".branch "$branch" - fi && - git add --force .gitmodules || - die "$(eval_gettext "Failed to register submodule '\$sm_path'")" - - # NEEDSWORK: In a multi-working-tree world, this needs to be - # set in the per-worktree config. - if git config --get submodule.active >/dev/null - then - # If the submodule being adding isn't already covered by the - # current configured pathspec, set the submodule's active flag - if ! git submodule--helper is-active "$sm_path" - then - git config submodule."$sm_name".active "true" - fi - else - git config submodule."$sm_name".active "true" - fi + git submodule--helper add-config ${force:+--force} ${branch:+--branch "$branch"} --url "$repo" --resolved-url "$realrepo" --path "$sm_path" --name "$sm_name" } #
Add a new "add-config" subcommand to `git submodule--helper` with the goal of converting part of the shell code in git-submodule.sh related to `git submodule add` into C code. This new subcommand sets the configuration variables of a newly added submodule, by registering the url in local git config, as well as the submodule name and path in the .gitmodules file. It also sets 'submodule.<name>.active' to "true" if the submodule path has not already been covered by any pathspec specified in 'submodule.active'. This is meant to be a faithful conversion from shell to C, with only one minor change: A warning is emitted if no value is specified in 'submodule.active', ie, the config looks like: "[submodule] active\n", because it is an invalid configuration. It would be helpful to let the user know that the pathspec is unset, and the value of 'submodule.<name>.active' might be set to 'true' so that they can rectify their configuration and prevent future surprises (especially given that the latter variable has a higher priority than the former). The structure of the conditional to check if we need to set the 'active' toggle looks different from the shell version -- but behaves the same. The change was made to decrease code duplication. A comment has been added to explain that only one value of 'submodule.active' is obtained to check if we need to call is_submodule_active() at all. This is part of a series of changes that will result in all of 'submodule add' being converted to C. Signed-off-by: Atharva Raykar <raykar.ath@gmail.com> Mentored-by: Christian Couder <christian.couder@gmail.com> Mentored-by: Shourya Shukla <shouryashukla.oo@gmail.com> Based-on-patch-by: Shourya Shukla <shouryashukla.oo@gmail.com> Based-on-patch-by: Prathamesh Chavan <pc44800@gmail.com> --- builtin/submodule--helper.c | 119 ++++++++++++++++++++++++++++++++++++ git-submodule.sh | 28 +-------- 2 files changed, 120 insertions(+), 27 deletions(-)