| Message ID | 20200513201737.55778-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] submodule: port subcommand 'set-branch' from shell to C | expand |
-Cc: chriscool@tuxfamily.org (seems redundant; Also, .mailmap confirms that christian.couder@gmail.com is Christian's preference) On 14-05-2020 01:47, Shourya Shukla wrote: > > The extra '$branch' on line 752 was because of Christian's help after > reference from TLDP's Parameter Subsitution documentation: > https://www.tldp.org/LDP/abs/html/parameter-substitution.html > For those who lack the context, during the conversion of the script Shourya faced an issue where the '--branch' argument did not work as expected. He described the issue in a private e-mail where Christian pointed out the following (quoting his reply hoping he doesn't mind): > On Tue, May 12, 2020 at 5:55 PM Christian Couder <christian.couder@gmail.com> wrote: >> >> In your commit (in submodule.sh line 781) there is: >> >> `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix >> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} >> ${branch:+--branch} ${default:+--default} -- "$@"` > > Actually the issue might come from the above. I think it should use ${branch:+--branch $branch} instead of ${branch:+--branch}. > > See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html That's why Shourya mentions the '$branch' as extra. Of course, that's how it is supposed to be in the first place :)
Hi Shourya, On Thu, May 14, 2020 at 01:47:36AM +0530, Shourya Shukla wrote: > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. > > Mentored-by: Christian Couder <chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > Here is another conversion, this time it is 'set-branch'. It passes all > the tests in t7419. I am aware that there are some repetitve parts in > the conversion as well as variables which can be named better. I would > love everyone's suggestion on this and how this can be made better. > > The extra '$branch' on line 752 was because of Christian's help after > reference from TLDP's Parameter Subsitution documentation: > https://www.tldp.org/LDP/abs/html/parameter-substitution.html > > Similarly, I had to change a coouple of other lines in the shell version > so as to make it compatible with the C version. > > Thank you so much Christian and Kaartic for the mentoring, this wouldn't > have been possible otherwise :) > > builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++ > git-submodule.sh | 31 ++------------------ > 2 files changed, 60 insertions(+), 29 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index f50745a03f..5a8815b76e 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2284,6 +2284,63 @@ static int module_set_url(int argc, const char **argv, const char *prefix) > return 0; > } > > +static int module_set_branch(int argc, const char **argv, const char *prefix) > +{ > + int quiet = 0, opt_branch = 0, opt_default = 0; > + const char *newbranch; nit: I would call this new_branch > + const char *path; > + char *config_name; > + > + struct option options[] = { > + OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")), > + OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")), > + OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")), This should use OPT_STRING and accept a string argument instead of using the implicit command-line ordering. > + OPT_END() > + }; > + const char *const usage[] = { > + N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"), > + N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"), > + NULL > + }; > + > + argc = parse_options(argc, argv, prefix, options, usage, 0); > + > + if ((!opt_branch && !opt_default)) > + die(_("At least one of --branch and --default required")); Error messages in Git are generally written without capitalising the first letter of the sentence. > + > + if (opt_branch) { > + if (opt_default) > + die(_("--branch and --default do not make sense")); This error message should be qualified, perhaps with something like "do not make sense together". > + > + newbranch = argv[0]; > + path = argv[1]; These assignments are incorrect since we haven't check argc yet. Also, they're redundant since you have the assignments in the if statement below. Also, if you do the OPT_STRING thing as described above, you can do the `path = ...` outside of the if-statement since it'll be common to both -d and -b. > + > + if (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1])) > + usage_with_options(usage, options); > + > + config_name = xstrfmt("submodule.%s.branch", path); > + config_set_in_gitmodules_file_gently(config_name, newbranch); > + > + printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); The original function did not print anything. We shouldn't alter the behaviour if we're just porting it over so we should delete this. > + free(config_name); > + } > + > + if (opt_default) { > + path = argv[0]; > + > + if (argc != 1 || !(path = argv[0])) > + usage_with_options(usage, options); > + > + config_name = xstrfmt("submodule.%s.branch", path); > + config_set_in_gitmodules_file_gently(config_name, NULL); > + > + printf(_("Default tracking branch set to 'master' successfully\n")); > + free(config_name); > + } The same arguments for the above apply to this case too. Actually, the only place where they both really differ is in the call to config_set_in_gitmodules_file_gently(). Can you do all of your argument checks together? Something like if (!!new_branch == opt_default) usage_with_options(usage, options); Then the call to config_set_in_gitmodules_file_gently() could look like this: config_name = xstrfmt("submodule.%s.branch", path); config_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL); free(config_name); and we wouldn't need the ifs at all. > + > + return 0; > +} > + > #define SUPPORT_SUPER_PREFIX (1<<0) > > struct cmd_struct { > @@ -2315,6 +2372,7 @@ static struct cmd_struct commands[] = { > {"check-name", check_name, 0}, > {"config", module_config, 0}, > {"set-url", module_set_url, 0}, > + {"set-branch", module_set_branch, 0}, > }; > > int cmd_submodule__helper(int argc, const char **argv, const char *prefix) > diff --git a/git-submodule.sh b/git-submodule.sh > index 39ebdf25b5..2438ef576e 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -719,7 +719,6 @@ cmd_update() > # $@ = requested path > # > cmd_set_branch() { > - unset_branch=false > branch= > > while test $# -ne 0 > @@ -729,7 +728,7 @@ cmd_set_branch() { > # we don't do anything with this but we need to accept it > ;; > -d|--default) > - unset_branch=true > + default=1 > ;; > -b|--branch) > case "$2" in '') usage ;; esac > @@ -750,33 +749,7 @@ cmd_set_branch() { > shift > done > > - if test $# -ne 1 > - 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 > - > - test -n "$branch"; has_branch=$? > - test "$unset_branch" = true; has_unset_branch=$? > - > - if test $((!$has_branch != !$has_unset_branch)) -eq 0 > - then > - usage > - fi > - > - if test $has_branch -eq 0 > - then > - git submodule--helper config submodule."$name".branch "$branch" > - else > - git submodule--helper config --unset submodule."$name".branch > - fi > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" The shell script portion looks good. Thanks, Denton > } > > # > -- > 2.26.2 >
On 14/05 06:10, Denton Liu wrote: > > + const char *path; > > + char *config_name; > > + > > + struct option options[] = { > > + OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")), > > + OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")), > > + OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")), > > This should use OPT_STRING and accept a string argument instead of using > the implicit command-line ordering. I actually was not able to understand the point of this change until I tried it out myself. It has made the code more aethetic as well as less redundant. Thanks a ton! > > + OPT_END() > > + }; > > + const char *const usage[] = { > > + N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"), > > + N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"), > > + NULL > > + }; > > + > > + argc = parse_options(argc, argv, prefix, options, usage, 0); > > + > > + if ((!opt_branch && !opt_default)) > > + die(_("At least one of --branch and --default required")); > > Error messages in Git are generally written without capitalising the > first letter of the sentence. Corrected. BTW, many other subcommands have this problem (the error messages as well as the options start with a caps and end with a fullstop). Should they be corrected or let them be as is? > > + > > + if (opt_branch) { > > + if (opt_default) > > + die(_("--branch and --default do not make sense")); > > This error message should be qualified, perhaps with something like "do > not make sense together". Done! > The same arguments for the above apply to this case too. Actually, the > only place where they both really differ is in the call to > config_set_in_gitmodules_file_gently(). Can you do all of your argument > checks together? Something like > > if (!!new_branch == opt_default) > usage_with_options(usage, options); > > Then the call to config_set_in_gitmodules_file_gently() could look like > this: > > config_name = xstrfmt("submodule.%s.branch", path); > config_set_in_gitmodules_file_gently(config_name, new_branch ? new_branch : NULL); > free(config_name); > > and we wouldn't need the ifs at all. > Sure, I have made the changes. Regards, Shourya Shukla
On Sat, May 16, 2020 at 04:07:44PM +0530, Shourya Shukla wrote: > On 14/05 06:10, Denton Liu wrote: > > Error messages in Git are generally written without capitalising the > > first letter of the sentence. > > Corrected. BTW, many other subcommands have this problem (the error > messages as well as the options start with a caps and end with a > fullstop). Should they be corrected or let them be as is? If it's already written, it's probably fine to leave it as is. Although if you're working in an area with some messages that could be fixed, it might be good to clean them up as a preparatory patch.
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > For those who lack the context, during the conversion of the script > Shourya faced an issue where the '--branch' argument did not work as > expected. He described the issue in a private e-mail where Christian > pointed out the following (quoting his reply hoping he doesn't mind): > >> On Tue, May 12, 2020 at 5:55 PM Christian Couder > <christian.couder@gmail.com> wrote: >>> >>> In your commit (in submodule.sh line 781) there is: >>> >>> `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix >>> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} >>> ${branch:+--branch} ${default:+--default} -- "$@"` >> >> Actually the issue might come from the above. I think it should use > ${branch:+--branch $branch} instead of ${branch:+--branch}. >> >> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html > > That's why Shourya mentions the '$branch' as extra. Of course, that's > how it is supposed to be in the first place :) Perhaps all of that should be removed from the log message and instead go after the three-dash line then. Thanks.
On 17-05-2020 20:36, Junio C Hamano wrote: > Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > >> For those who lack the context, during the conversion of the script >> Shourya faced an issue where the '--branch' argument did not work as >> expected. He described the issue in a private e-mail where Christian >> pointed out the following (quoting his reply hoping he doesn't mind): >> >>> On Tue, May 12, 2020 at 5:55 PM Christian Couder >> <christian.couder@gmail.com> wrote: >>>> >>>> In your commit (in submodule.sh line 781) there is: >>>> >>>> `git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix >>>> "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} >>>> ${branch:+--branch} ${default:+--default} -- "$@"` >>> >>> Actually the issue might come from the above. I think it should use >> ${branch:+--branch $branch} instead of ${branch:+--branch}. >>> >>> See: https://www.tldp.org/LDP/abs/html/parameter-substitution.html >> >> That's why Shourya mentions the '$branch' as extra. Of course, that's >> how it is supposed to be in the first place :) > > Perhaps all of that should be removed from the log message and > instead go after the three-dash line then. > I believe it's already after the three-dash line. The log message reads: > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. > > Mentored-by: Christian Couder<chriscool@tuxfamily.org> > Mentored-by: Kaartic Sivaraam<kaartic.sivaraam@gmail.com> > Signed-off-by: Shourya Shukla<shouryashukla.oo@gmail.com> It might use more explanation, maybe.
On 2020-05-14 01:47:36+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > diff --git a/git-submodule.sh b/git-submodule.sh > index 39ebdf25b5..2438ef576e 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -719,7 +719,6 @@ cmd_update() > # $@ = requested path > # > cmd_set_branch() { > - unset_branch=false > branch= > > while test $# -ne 0 > @@ -729,7 +728,7 @@ cmd_set_branch() { > # we don't do anything with this but we need to accept it > ;; > -d|--default) > - unset_branch=true > + default=1 Hi Shourya, Thanks for your hard work, I skimmed over the current code, it seems like this: default=1 is new. If my understanding is correct, please reset its value in the beginning of this function, to avoid: - a side effect from an assignment in get_submodule_config (if there's such a side effect, I'm NOT really sure). - effect of an environment variable, c.f. https://lore.kernel.org/git/20200402084251.85840-1-zhiyou.jx@alibaba-inc.com/
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f50745a03f..5a8815b76e 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2284,6 +2284,63 @@ static int module_set_url(int argc, const char **argv, const char *prefix) return 0; } +static int module_set_branch(int argc, const char **argv, const char *prefix) +{ + int quiet = 0, opt_branch = 0, opt_default = 0; + const char *newbranch; + const char *path; + char *config_name; + + struct option options[] = { + OPT__QUIET(&quiet, N_("Suppress output for setting default tracking branch of a submodule")), + OPT_BOOL(0, "default", &opt_default, N_("Set the default tracking branch to master")), + OPT_BOOL(0, "branch", &opt_branch, N_("Set the default tracking branch to the one specified")), + OPT_END() + }; + const char *const usage[] = { + N_("git submodule--helper set-branch [--quiet] (-d|--default) <path>"), + N_("git submodule--helper set-branch [--quiet] (-b|--branch) <branch> <path>"), + NULL + }; + + argc = parse_options(argc, argv, prefix, options, usage, 0); + + if ((!opt_branch && !opt_default)) + die(_("At least one of --branch and --default required")); + + if (opt_branch) { + if (opt_default) + die(_("--branch and --default do not make sense")); + + newbranch = argv[0]; + path = argv[1]; + + if (argc != 2 || !(newbranch = argv[0]) || !(path = argv[1])) + usage_with_options(usage, options); + + config_name = xstrfmt("submodule.%s.branch", path); + config_set_in_gitmodules_file_gently(config_name, newbranch); + + printf(_("Default tracking branch set to '%s' successfully\n"), newbranch); + free(config_name); + } + + if (opt_default) { + path = argv[0]; + + if (argc != 1 || !(path = argv[0])) + usage_with_options(usage, options); + + config_name = xstrfmt("submodule.%s.branch", path); + config_set_in_gitmodules_file_gently(config_name, NULL); + + printf(_("Default tracking branch set to 'master' successfully\n")); + free(config_name); + } + + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2315,6 +2372,7 @@ static struct cmd_struct commands[] = { {"check-name", check_name, 0}, {"config", module_config, 0}, {"set-url", module_set_url, 0}, + {"set-branch", module_set_branch, 0}, }; int cmd_submodule__helper(int argc, const char **argv, const char *prefix) diff --git a/git-submodule.sh b/git-submodule.sh index 39ebdf25b5..2438ef576e 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -719,7 +719,6 @@ cmd_update() # $@ = requested path # cmd_set_branch() { - unset_branch=false branch= while test $# -ne 0 @@ -729,7 +728,7 @@ cmd_set_branch() { # we don't do anything with this but we need to accept it ;; -d|--default) - unset_branch=true + default=1 ;; -b|--branch) case "$2" in '') usage ;; esac @@ -750,33 +749,7 @@ cmd_set_branch() { shift done - if test $# -ne 1 - 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 - - test -n "$branch"; has_branch=$? - test "$unset_branch" = true; has_unset_branch=$? - - if test $((!$has_branch != !$has_unset_branch)) -eq 0 - then - usage - fi - - if test $has_branch -eq 0 - then - git submodule--helper config submodule."$name".branch "$branch" - else - git submodule--helper config --unset submodule."$name".branch - fi + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" } #
Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- Here is another conversion, this time it is 'set-branch'. It passes all the tests in t7419. I am aware that there are some repetitve parts in the conversion as well as variables which can be named better. I would love everyone's suggestion on this and how this can be made better. The extra '$branch' on line 752 was because of Christian's help after reference from TLDP's Parameter Subsitution documentation: https://www.tldp.org/LDP/abs/html/parameter-substitution.html Similarly, I had to change a coouple of other lines in the shell version so as to make it compatible with the C version. Thank you so much Christian and Kaartic for the mentoring, this wouldn't have been possible otherwise :) builtin/submodule--helper.c | 58 +++++++++++++++++++++++++++++++++++++ git-submodule.sh | 31 ++------------------ 2 files changed, 60 insertions(+), 29 deletions(-)