| Message ID | 20200523163929.7040-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] submodule: port subcommand 'set-branch' from shell to C | expand |
Hi Shourya, I believe you missed Danh's v3 comments[1]. I'm mentioning them inline with some additional comments. On 23-05-2020 22:09, Shourya Shukla wrote: > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index f50745a03f..7e844e8971 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -2284,6 +2284,50 @@ 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) > +{ > + /* > + * We accept the `quiet` option for uniformity across subcommands, > + * though there is nothing to make less verbose in this subcommand. > + */ > + int quiet = 0, opt_default = 0, ret; > + const char *opt_branch = NULL; > + const char *path; > + char *config_name; > + > + struct option options[] = { > + OPT__QUIET(&quiet, > + N_("suppress output for setting default tracking branch")), As '--quiet' in 'set-branch' is a no-op and is being accepted only for uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of OPT__QUIET for specifying it, as suggested by Danh. Also, the description "suppress output for setting default tracking branch" doesn't seem to be valid anymore as we don't print anything when set-branch succeeds. > + OPT_BOOL(0, "default", &opt_default, > + N_("set the default tracking branch to master")), > + OPT_STRING(0, "branch", &opt_branch, N_("branch"), > + N_("set the default tracking branch")), > + 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 > + }; > + I also agree with the Danh here that '--quiet' could be removed from usage. There's no point in mentioning '--quiet' in the usage when it has no effect. > diff --git a/git-submodule.sh b/git-submodule.sh > index 39ebdf25b5..8c56191f77 100755 > --- a/git-submodule.sh > +++ b/git-submodule.sh > @@ -750,33 +750,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} -- "$@" > } > Danh questioned whether '$branch' needs to be quoted here. I too think it needs to be quoted unless I'm missing something. --- Footnotes: [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e Thanks, Sivaraam
Hi Kaartic, On 2020-05-24 00:19:38+0530, Kaartic Sivaraam <kaartic.sivaraam@gmail.com> wrote: > I believe you missed Danh's v3 comments[1]. I'm mentioning them inline with > some additional comments. Thanks for checking this. > On 23-05-2020 22:09, Shourya Shukla wrote: > > > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > > index f50745a03f..7e844e8971 100644 > > --- a/builtin/submodule--helper.c > > +++ b/builtin/submodule--helper.c > > @@ -2284,6 +2284,50 @@ 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) > > +{ > > + /* > > + * We accept the `quiet` option for uniformity across subcommands, > > + * though there is nothing to make less verbose in this subcommand. > > + */ > > + int quiet = 0, opt_default = 0, ret; > > + const char *opt_branch = NULL; > > + const char *path; > > + char *config_name; > > + > > + struct option options[] = { > > + OPT__QUIET(&quiet, > > + N_("suppress output for setting default tracking branch")), > > As '--quiet' in 'set-branch' is a no-op and is being accepted only for > uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of > OPT__QUIET for specifying it, as suggested by Danh. Yay, I still think it's better to use OPT_NOOP_NOARG, (and with shortopt q, which I forgot in previous reply.) OPT_NOOP_NOARG('q', "quiet") > Also, the description "suppress output for setting default tracking branch" > doesn't seem to be valid anymore as we don't print anything when set-branch > succeeds. OPT_NOOP_NOARG will take care of description itself. Even if we choose to not use OPT_NOOP_NOARG, a better description should be provided. > > + OPT_BOOL(0, "default", &opt_default, > > + N_("set the default tracking branch to master")), > > + OPT_STRING(0, "branch", &opt_branch, N_("branch"), > > + N_("set the default tracking branch")), > > + 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 > > + }; > > + > > I also agree with the Danh here that '--quiet' could be removed from usage. > There's no point in mentioning '--quiet' in the usage when it has no effect. > > > diff --git a/git-submodule.sh b/git-submodule.sh > > index 39ebdf25b5..8c56191f77 100755 > > --- a/git-submodule.sh > > +++ b/git-submodule.sh > > @@ -750,33 +750,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} -- "$@" > > } > > > > Danh questioned whether '$branch' needs to be quoted here. I too think it > needs to be quoted unless I'm missing something. > > > --- > Footnotes: > [1]: https://lore.kernel.org/git/nycvar.QRO.7.76.6.2005230012090.56@tvgsbejvaqbjf.bet/T/#maf26182b084087ed08a2a72d3da2ee2026b1618e For the better record, I think it's better to use a permenent link, just in case lore.kernel.org go into the dust someday, people can still have a reference if they have an archive. https://lore.kernel.org/git/20200521230453.GB2042@danh.dev/
On 24/05 12:19, Kaartic Sivaraam wrote: > As '--quiet' in 'set-branch' is a no-op and is being accepted only for > uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of > OPT__QUIET for specifying it, as suggested by Danh. > > Also, the description "suppress output for setting default tracking branch" > doesn't seem to be valid anymore as we don't print anything when set-branch > succeeds. I think it will all boil down to the consistency of all the subcommands. Changing this would require making changes in various places: the C code (obviously), the shell script (not only the cmd_set_branch() function but the part for accepting user input as well) and the Documentation (I might have maybe missed a couple of other changes to list here too). Its not that I don't want to do this, but it would add unnecessary changes don't you think? I would love it if others could weigh in their opinions too about this. > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" > Danh questioned whether '$branch' needs to be quoted here. I too think it > needs to be quoted unless I'm missing something. We want to do this because $branch is an argument right?
On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > On 24/05 12:19, Kaartic Sivaraam wrote: > > As '--quiet' in 'set-branch' is a no-op and is being accepted only for > > uniformity, I think it makes sense to use OPT_NOOP_NOARG instead of > > OPT__QUIET for specifying it, as suggested by Danh. > > > > Also, the description "suppress output for setting default tracking branch" > > doesn't seem to be valid anymore as we don't print anything when set-branch > > succeeds. > > I think it will all boil down to the consistency of all the subcommands. > Changing this would require making changes in various places: the C code > (obviously), the shell script (not only the cmd_set_branch() function > but the part for accepting user input as well) and the Documentation (I > might have maybe missed a couple of other changes to list here too). Its I don't think this is a valid argument. Using OPT_NOOP_NOARG doesn't require any change in shell script since the binary still accepts -q|--quiet. The documentation of --quiet is still valid (since it doesn't print anything regardless) The only necessary change in in that C code. > not that I don't want to do this, but it would add unnecessary changes > don't you think? I would love it if others could weigh in their opinions > too about this. > > > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" > > > Danh questioned whether '$branch' needs to be quoted here. I too think it > > needs to be quoted unless I'm missing something. > > We want to do this because $branch is an argument right? We want to do this because we don't want to whitespace-split "$branch" Let's say, for some reason, this command was run: git submodule set-branch --branch "a-branch --branch another" a-submodule This version will run: git submodule--helper --branch a-branch --branch another a-submodule Which will success if there's a branch "another" in the "a-submodule". While that command should fail because we don't accept refname with space.
On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > > > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" > > > > > Danh questioned whether '$branch' needs to be quoted here. I too think it > > > needs to be quoted unless I'm missing something. > > > > We want to do this because $branch is an argument right? > > We want to do this because we don't want to whitespace-split "$branch" > > Let's say, for some reason, this command was run: > > git submodule set-branch --branch "a-branch --branch another" a-submodule Anyway, after typing this. I'm thinking a bit, then re-read gitcli(7), I think git-submodule is quite broken regarding to Git's guidelines: -----------8<---------- Here are the rules regarding the "flags" that you should follow when you are scripting Git: * it's preferred to use the non-dashed form of Git commands, which means that you should prefer `git foo` to `git-foo`. * splitting short options to separate words (prefer `git foo -a -b` to `git foo -ab`, the latter may not even work). * when a command-line option takes an argument, use the 'stuck' form. In other words, write `git foo -oArg` instead of `git foo -o Arg` for short options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg` for long options. An option that takes optional option-argument must be written in the 'stuck' form. ------------>8-------------- Current Git, with and without this change, this command will fail: git submodule set-branch --branch=a-branch a-submodule Thus, a script conformed with gitcli(7) will fail. (And our git-submodule(1) doesn't conform with gitcli(7), FWIW). After this change, those commands will success: git submodule--helper set-branch --branch a-branch a-submodule git submodule set-branch --branch "a-branch --branch=another" a-submodule (The second one was written for demonstration purpose only, I don't expect it will success) This isn't related to this change, and git-submodule(1) will be rewritten in C in the very near future. Just want to make sure it's awared. > > This version will run: > > git submodule--helper --branch a-branch --branch another a-submodule > > Which will success if there's a branch "another" in the "a-submodule". > While that command should fail because we don't accept refname with > space.
On 2020-05-28 21:01:42+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > On 2020-05-28 19:21:47+0700, Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote: > > On 2020-05-27 22:43:58+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > > > > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" > > > > > > > Danh questioned whether '$branch' needs to be quoted here. I too think it > > > > needs to be quoted unless I'm missing something. > > > > > > We want to do this because $branch is an argument right? > > > > We want to do this because we don't want to whitespace-split "$branch" > > > > Let's say, for some reason, this command was run: > > > > git submodule set-branch --branch "a-branch --branch another" a-submodule > > Anyway, after typing this. > I'm thinking a bit, then re-read gitcli(7), > I think git-submodule is quite broken regarding to Git's guidelines: > > -----------8<---------- > > Here are the rules regarding the "flags" that you should follow when you are > scripting Git: > > * it's preferred to use the non-dashed form of Git commands, which means that > you should prefer `git foo` to `git-foo`. > > * splitting short options to separate words (prefer `git foo -a -b` > to `git foo -ab`, the latter may not even work). > > * when a command-line option takes an argument, use the 'stuck' form. In > other words, write `git foo -oArg` instead of `git foo -o Arg` for short > options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg` > for long options. An option that takes optional option-argument must be > written in the 'stuck' form. > ------------>8-------------- > > Current Git, with and without this change, this command will fail: > > git submodule set-branch --branch=a-branch a-submodule > > Thus, a script conformed with gitcli(7) will fail. > (And our git-submodule(1) doesn't conform with gitcli(7), FWIW). > > After this change, those commands will success: > > git submodule--helper set-branch --branch a-branch a-submodule This should be read: git submodule--helper set-branch --branch=a-branch a-submodule > git submodule set-branch --branch "a-branch --branch=another" a-submodule > > (The second one was written for demonstration purpose only, > I don't expect it will success) > > This isn't related to this change, and git-submodule(1) will be > rewritten in C in the very near future. > Just want to make sure it's awared. > > > > > This version will run: > > > > git submodule--helper --branch a-branch --branch another a-submodule > > > > Which will success if there's a branch "another" in the "a-submodule". > > While that command should fail because we don't accept refname with > > space. It's me being noisy again. I'm still puzzled by this idea (and I drank too much coffee, today). I think the day of conversion of submodule from shell to C finish, we can use current git-submodule--helper as the new git-submodule. With that idea, I think why don't we passed all arguments from git submodule set-branch into git-submodule--helper. (Yes, the idea is wrong because the usage output will have git submodule--helper as $0) I tried that idea and run the test. To my surprise the test failed :(. Turn out git-submodule--helper set-branch doesn't do its advertised job, git-submodule--helper set-branch doesn't understand short options -d and -b We'll need this fixup regardless of the agreement on my other concerns. ----------8<--------- diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 305c9abb3b..64636161a7 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2291,9 +2291,9 @@ static int module_set_branch(int argc, const char **argv, const char *prefix) struct option options[] = { OPT__QUIET(&quiet, N_("suppress output for setting default tracking branch")), - OPT_BOOL(0, "default", &opt_default, + OPT_BOOL('d', "default", &opt_default, N_("set the default tracking branch to master")), - OPT_STRING(0, "branch", &opt_branch, N_("branch"), + OPT_STRING('b', "branch", &opt_branch, N_("branch"), N_("set the default tracking branch")), OPT_END() }; ------8<-----------
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f50745a03f..7e844e8971 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2284,6 +2284,50 @@ 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) +{ + /* + * We accept the `quiet` option for uniformity across subcommands, + * though there is nothing to make less verbose in this subcommand. + */ + int quiet = 0, opt_default = 0, ret; + const char *opt_branch = NULL; + const char *path; + char *config_name; + + struct option options[] = { + OPT__QUIET(&quiet, + N_("suppress output for setting default tracking branch")), + OPT_BOOL(0, "default", &opt_default, + N_("set the default tracking branch to master")), + OPT_STRING(0, "branch", &opt_branch, N_("branch"), + N_("set the default tracking branch")), + 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(_("--branch or --default required")); + + if (opt_branch && opt_default) + die(_("--branch and --default are mutually exclusive")); + + if (argc != 1 || !(path = argv[0])) + usage_with_options(usage, options); + + config_name = xstrfmt("submodule.%s.branch", path); + ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); + + free(config_name); + return ret; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2315,6 +2359,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..8c56191f77 100755 --- a/git-submodule.sh +++ b/git-submodule.sh @@ -719,7 +719,7 @@ cmd_update() # $@ = requested path # cmd_set_branch() { - unset_branch=false + default= branch= while test $# -ne 0 @@ -729,7 +729,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 +750,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 and call it via 'git-submodule.sh'. Mentored-by: Christian Couder <chriscool@tuxfamily.org> Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@gmail.com> Helped-by: Denton Liu <liu.denton@gmail.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- Thank you for the review Junio and Johannes. I have made the requested changes. builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++ git-submodule.sh | 32 +++----------------------- 2 files changed, 48 insertions(+), 29 deletions(-)