| Message ID | 20200508062136.15257-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] submodule: port subcommand 'set-url' from shell to C | expand |
Hi Shourya, It looks good to me except for one tiny nit: On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote: > Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to > 'submodule--helper.c' and call the latter via 'git-submodule.sh'. > > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++ > git-submodule.sh | 22 +--------------------- > 2 files changed, 38 insertions(+), 21 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 1a4b391c88..8bc7b4cfa6 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix) > usage_with_options(git_submodule_helper_usage, module_config_options); > } > > +static int module_set_url(int argc, const char **argv, const char *prefix) > +{ > + int quiet = 0; > + const char *newurl; > + const char *path; > + char* config_name; The asterisk should be stuck with the name, not the type, similar to how you wrote it above. > + > + struct option options[] = { > + OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")), > + OPT_END() > + }; > + const char *const usage[] = { > + N_("git submodule--helper set-url [--quiet] <path> <newurl>"), > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, options, usage, 0); > + > + path = argv[0]; > + newurl = argv[1]; > + > + if (argc != 2 || !path || !newurl) { > + usage_with_options(usage, options); > + return 1; > + } > + > + config_name = xstrfmt("submodule.%s.url", path); > + > + config_set_in_gitmodules_file_gently(config_name, newurl); > + sync_submodule(path, prefix, quiet ? OPT_QUIET : 0); > + > + free(config_name); > + > + return 0; > +} > + > #define SUPPORT_SUPER_PREFIX (1<<0) > > struct cmd_struct {
Denton Liu <liu.denton@gmail.com> writes: > Hi Shourya, > > It looks good to me except for one tiny nit: > > On Fri, May 08, 2020 at 11:51:36AM +0530, Shourya Shukla wrote: >> Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to >> 'submodule--helper.c' and call the latter via 'git-submodule.sh'. >> >> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> >> --- >> builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++ >> git-submodule.sh | 22 +--------------------- >> 2 files changed, 38 insertions(+), 21 deletions(-) >> >> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c >> index 1a4b391c88..8bc7b4cfa6 100644 >> --- a/builtin/submodule--helper.c >> +++ b/builtin/submodule--helper.c >> @@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix) >> usage_with_options(git_submodule_helper_usage, module_config_options); >> } >> >> +static int module_set_url(int argc, const char **argv, const char *prefix) >> +{ >> + int quiet = 0; >> + const char *newurl; >> + const char *path; >> + char* config_name; > > The asterisk should be stuck with the name, not the type, similar to how > you wrote it above. Right. >> + >> + struct option options[] = { >> + OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")), >> + OPT_END() >> + }; >> + const char *const usage[] = { >> + N_("git submodule--helper set-url [--quiet] <path> <newurl>"), >> + NULL >> + }; >> + >> + argc = parse_options(argc, argv, prefix, options, usage, 0); >> + >> + path = argv[0]; >> + newurl = argv[1]; >> + >> + if (argc != 2 || !path || !newurl) { Checking argc at this point is too late to protect against the potential out-of-bounds access we have already made to argv[0] and argv[1].
Denton Liu <liu.denton@gmail.com> writes: >> + if (argc != 2 || !path || !newurl) { >> + usage_with_options(usage, options); >> + return 1; It is embarrassing that nobody noticed that usage_with_options() is NORETURN; return 1 has no effect here. >> + } >> + >> + config_name = xstrfmt("submodule.%s.url", path); >> + >> + config_set_in_gitmodules_file_gently(config_name, newurl); >> + sync_submodule(path, prefix, quiet ? OPT_QUIET : 0); >> + >> + free(config_name); >> + >> + return 0; >> +} >> + >> #define SUPPORT_SUPER_PREFIX (1<<0) >> >> struct cmd_struct {
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 1a4b391c88..8bc7b4cfa6 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2246,6 +2246,42 @@ static int module_config(int argc, const char **argv, const char *prefix) usage_with_options(git_submodule_helper_usage, module_config_options); } +static int module_set_url(int argc, const char **argv, const char *prefix) +{ + int quiet = 0; + const char *newurl; + const char *path; + char* config_name; + + struct option options[] = { + OPT__QUIET(&quiet, N_("Suppress output for setting url of a submodule")), + OPT_END() + }; + const char *const usage[] = { + N_("git submodule--helper set-url [--quiet] <path> <newurl>"), + NULL + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + + path = argv[0]; + newurl = argv[1]; + + if (argc != 2 || !path || !newurl) { + usage_with_options(usage, options); + return 1; + } + + config_name = xstrfmt("submodule.%s.url", path); + + config_set_in_gitmodules_file_gently(config_name, newurl); + sync_submodule(path, prefix, quiet ? OPT_QUIET : 0); + + free(config_name); + + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2276,6 +2312,7 @@ static struct cmd_struct commands[] = { {"is-active", is_active, 0}, {"check-name", check_name, 0}, {"config", module_config, 0}, + {"set-url", module_set_url, 0}, }; int cmd_submodule__helper(int argc, const char **argv, const char *prefix) diff --git a/git-submodule.sh b/git-submodule.sh index 08e0439df0..39ebdf25b5 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -805,27 +805,7 @@ cmd_set_url() { shift done - if test $# -ne 2 - then - usage - fi - - # we can't use `git submodule--helper name` here because internally, it - # hashes the path so a trailing slash could lead to an unintentional no match - name="$(git submodule--helper list "$1" | cut -f2)" - if test -z "$name" - then - exit 1 - fi - - url="$2" - if test -z "$url" - then - exit 1 - fi - - git submodule--helper config submodule."$name".url "$url" - git submodule--helper sync ${GIT_QUIET:+--quiet} "$name" + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-url ${GIT_QUIET:+--quiet} -- "$@" } #
Convert submodule subcommand 'set-url' to a builtin. Port 'set-url' to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- builtin/submodule--helper.c | 37 +++++++++++++++++++++++++++++++++++++ git-submodule.sh | 22 +--------------------- 2 files changed, 38 insertions(+), 21 deletions(-)