| Message ID | 20200519182654.33318-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] submodule: port subcommand 'set-branch' from shell to C | expand |
On Tue, May 19, 2020 at 2:27 PM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. You can reduce the redundancy by writing this as: Convert git-submodule subcommand 'set-branch' to a builtin and call it via 'git-submodule.sh'. > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> > --- > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > @@ -2284,6 +2284,46 @@ static int module_set_url(int argc, const char **argv, const char *prefix) > +static int module_set_branch(int argc, const char **argv, const char *prefix) > +{ > + int quiet = 0, opt_default = 0; > + char *opt_branch = NULL; This can be 'const const *', can't it? > + struct option options[] = { > + OPT__QUIET(&quiet, > + N_("suppress output for setting default tracking branch of a submodule")), This is unusually verbose for a _short_ description of the option. Other commands use simpler descriptions. Perhaps take a hint from the git-submodule man page: N("only print error messages")), However, the bigger question is: Why is the --quiet option even here? None of the code in this function ever consults the 'quiet' variable, so its presence seems pointless. Looking at the git-submodule documentation, I see that it is already documented as accepted --quiet, so it may make some sense for you to accept the option here. However, it might be a good idea either to have an in-code comment or a blurb in the commit message explaining that this C rewrite accepts the option for backward-compatibility (and for future extension), not because it is actually used presently. > + OPT_BOOL(0, "default", &opt_default, > + N_("set the default tracking branch to master")), We can make this and the next short description more precise and concise like this: N_("reset the default tracking branch to master")), > + OPT_STRING(0, "branch", &opt_branch, N_("branch"), > + N_("set the default tracking branch to the one specified")), Then: 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(_("at least one of --branch and --default required")); This wording makes no sense considering that --branch and --default are mutually exclusive. By writing "at least one of", you're saying that you can use _more than one_, which is clearly incorrect. Reword it like this: die(_("--branch or --default required")); > + if (opt_branch && opt_default) > + die(_("--branch and --default do not make sense together")); A more precise way to say this is: 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); > + config_set_in_gitmodules_file_gently(config_name, opt_branch); Tracing through the config code, I see that config_set_in_gitmodules_file_gently() removes the key if 'opt_branch' is NULL, which mirrors the behavior of the shell code this is replacing. Good.
Hello Eric! On 19/05 02:57, Eric Sunshine wrote: > On Tue, May 19, 2020 at 2:27 PM Shourya Shukla > <shouryashukla.oo@gmail.com> wrote: > > Convert submodule subcommand 'set-branch' to a builtin. Port 'set-branch' > > to 'submodule--helper.c' and call the latter via 'git-submodule.sh'. > > You can reduce the redundancy by writing this as: > > Convert git-submodule subcommand 'set-branch' to a builtin and > call it via 'git-submodule.sh'. Sure! Will do! > > + struct option options[] = { > > + OPT__QUIET(&quiet, > > + N_("suppress output for setting default tracking branch of a submodule")), > > This is unusually verbose for a _short_ description of the option. > Other commands use simpler descriptions. Perhaps take a hint from the > git-submodule man page: > > N("only print error messages")), > > However, the bigger question is: Why is the --quiet option even here? > None of the code in this function ever consults the 'quiet' variable, > so its presence seems pointless. I actually wanted to have *some* use of the `quiet` option and delivered it in the v1, but after some feedback had to scrap it. You may have a look here: https://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/ > Looking at the git-submodule documentation, I see that it is already > documented as accepted --quiet, so it may make some sense for you to > accept the option here. However, it might be a good idea either to > have an in-code comment or a blurb in the commit message explaining > that this C rewrite accepts the option for backward-compatibility (and > for future extension), not because it is actually used presently. That seems like a better idea; should I add this comment just above the `options` array? BTW, the shell version has a comment about this, see: https://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727 > > + OPT_STRING(0, "branch", &opt_branch, N_("branch"), > > + N_("set the default tracking branch to the one specified")), > > Then: > > N_("set the default tracking branch")), Seems good! > > + 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")); > > This wording makes no sense considering that --branch and --default > are mutually exclusive. By writing "at least one of", you're saying > that you can use _more than one_, which is clearly incorrect. Reword > it like this: > > die(_("--branch or --default required")); Yeah, I did not realize it until you mentioned this, will correct in the next version. > > + if (opt_branch && opt_default) > > + die(_("--branch and --default do not make sense together")); > > A more precise way to say this is: > > die(_("--branch and --default are mutually exclusive")); Will that be clear to everyone? What I mean is maybe a person from a non-mathematical background (someone doing programming as a hobby maybe) will not grasp at this at one go and will have to search it's meaning online. Isn't it fine as-is? > > + 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, opt_branch); > > Tracing through the config code, I see that > config_set_in_gitmodules_file_gently() removes the key if 'opt_branch' > is NULL, which mirrors the behavior of the shell code this is > replacing. Good. Thanks! :) Regards, Shourya Shukla
Hi Shourya, On 20-05-2020 17:45, Shourya Shukla wrote: > On 19/05 02:57, Eric Sunshine wrote: >> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla >> <shouryashukla.oo@gmail.com> wrote: >>> + if (opt_branch && opt_default) >>> + die(_("--branch and --default do not make sense together")); >> >> A more precise way to say this is: >> >> die(_("--branch and --default are mutually exclusive")); > > Will that be clear to everyone? What I mean is maybe a person from a > non-mathematical background (someone doing programming as a hobby maybe) > will not grasp at this at one go and will have to search it's meaning > online. Isn't it fine as-is? > While "mutually exclusive" might be prominently used in mathematics. I don't think it is only understandable by people with a mathematical background. Moreover, I see 183 results in 36 files for "mutually exclusive" in git.git (including translation files). So, this isn't anything new. I agree with Eric's suggestion. It makes the error message concise which is a nice side benefit.
Kaartic Sivaraam <kaartic.sivaraam@gmail.com> writes: > Hi Shourya, > > On 20-05-2020 17:45, Shourya Shukla wrote: >> On 19/05 02:57, Eric Sunshine wrote: >>> On Tue, May 19, 2020 at 2:27 PM Shourya Shukla >>> <shouryashukla.oo@gmail.com> wrote: >>>> + if (opt_branch && opt_default) >>>> + die(_("--branch and --default do not make sense together")); >>> >>> A more precise way to say this is: >>> >>> die(_("--branch and --default are mutually exclusive")); >> >> Will that be clear to everyone? What I mean is maybe a person from a >> non-mathematical background (someone doing programming as a hobby maybe) >> will not grasp at this at one go and will have to search it's meaning >> online. Isn't it fine as-is? >> > > While "mutually exclusive" might be prominently used in mathematics. I > don't think it is only understandable by people with a mathematical > background. > > Moreover, I see 183 results in 36 files for "mutually exclusive" in > git.git (including translation files). So, this isn't anything new. > > I agree with Eric's suggestion. It makes the error message concise > which is a nice side benefit. Sure. But "do not make sense together" is forcing the reader to infer the implication "... hence cannot use at the same time", so it is one step detached from what we really want them to know, while giving half an explanation (it still invites "why don't they make sense together?") so why not say that conclusion more directly? i.e. "... cannot be used together"? Either way those who need to be told more would ask "why can't I use them together?" anyway, so half an explanation would not help all that much.
On Wed, May 20, 2020 at 8:23 AM Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > On 19/05 02:57, Eric Sunshine wrote: > > On Tue, May 19, 2020 at 2:27 PM Shourya Shukla > > <shouryashukla.oo@gmail.com> wrote: > > > + OPT__QUIET(&quiet, > > > > However, the bigger question is: Why is the --quiet option even here? > > None of the code in this function ever consults the 'quiet' variable, > > so its presence seems pointless. > > I actually wanted to have *some* use of the `quiet` option and delivered > it in the v1, but after some feedback had to scrap it. You may have a > look here: > https://lore.kernel.org/git/20200513201737.55778-2-shouryashukla.oo@gmail.com/ I agree with Denton's conclusion about not introducing needless noise merely to give the --quiet option something to squelch. And, to answer your question about when and when not to print something, a good "Unix way" guideline is that "silence is golden"[1]. [1]: http://www.catb.org/esr/writings/taoup/html/ch01s06.html#id2878450 > > Looking at the git-submodule documentation, I see that it is already > > documented as accepted --quiet, so it may make some sense for you to > > accept the option here. However, it might be a good idea either to > > have an in-code comment or a blurb in the commit message explaining > > that this C rewrite accepts the option for backward-compatibility (and > > for future extension), not because it is actually used presently. > > That seems like a better idea; should I add this comment just above the > `options` array? BTW, the shell version has a comment about this, > see: > https://github.com/git/git/blob/v2.26.2/git-submodule.sh#L727 It would be a good idea to attach a comment like that to the declaration of 'quiet' in the C code (rather than placing it above the 'options' array). For instance: /* for backward compatibility but not presently used */ int quiet = 0; > > > + die(_("--branch and --default do not make sense together")); > > > > A more precise way to say this is: > > > > die(_("--branch and --default are mutually exclusive")); > > Will that be clear to everyone? What I mean is maybe a person from a > non-mathematical background (someone doing programming as a hobby maybe) > will not grasp at this at one go and will have to search it's meaning > online. Isn't it fine as-is? Others have already responded to this up-thread...
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f50745a03f..5cd7dc84c6 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -2284,6 +2284,46 @@ 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_default = 0; + char *opt_branch = NULL; + 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_STRING(0, "branch", &opt_branch, N_("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 && opt_default) + die(_("--branch and --default do not make sense together")); + + 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, opt_branch); + + free(config_name); + return 0; +} + #define SUPPORT_SUPER_PREFIX (1<<0) struct cmd_struct { @@ -2315,6 +2355,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. 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> Helped-by: Denton Liu <liu.denton@gmail.com> Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com> --- An improvement over the previous version, with a lot less clutter and redundancy. This version also covers the side-effect pointed out by Công Danh in (thanks to Kaartic for pointing it out): https://lore.kernel.org/git/20200517161151.GA30938@danh.dev/ I have refrained from using the `newbranch` variable because using only `opt_branch` simplified things even further (thanks to Christian). I think a similar improvement could be made to `set-url`, but let's leave that for 'leftoverbits' maybe? Thank you Denton, Christian and Kaartic for the reviews! :) Next step is conversion of `summary` to C (after the review of `set-branch` is done). builtin/submodule--helper.c | 41 +++++++++++++++++++++++++++++++++++++ git-submodule.sh | 32 +++-------------------------- 2 files changed, 44 insertions(+), 29 deletions(-)