| Message ID | 20200521163819.12544-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] submodule: port subcommand 'set-branch' from shell to C | expand |
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > 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 Eric. I have changed the commit message, > and the error prompts. Also, I have added a brief comment about > the `quiet` option. Sorry, I may have missed the previous rounds of discussion, but the comment adds more puzzles than it helps readers. "is currently not used" can be seen from the code, but it is totally unclear why it is not used. Is that a design decision to always keep quiet or always talkative (if so, "suppress output..." is not a good description)? Is that that this is a WIP patch that the behaviour the option aims to achieve hasn't been implemented? Is it that no existing callers pass "-q" to the scripted version, so there is no need to support it (if so, why do we even accept it in the first place)? Is it that all existing callers pass "-q" so we need to accept it, but there is nothing we need to make verbose so the variable is not passed around in the codepath?
On Thu, May 21, 2020 at 11:44:22AM -0700, Junio C Hamano wrote: > Shourya Shukla <shouryashukla.oo@gmail.com> writes: > > > 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 Eric. I have changed the commit message, > > and the error prompts. Also, I have added a brief comment about > > the `quiet` option. > > Sorry, I may have missed the previous rounds of discussion, but the > comment adds more puzzles than it helps readers. "is currently not > used" can be seen from the code, but it is totally unclear why it is > not used. Is that a design decision to always keep quiet or always > talkative (if so, "suppress output..." is not a good description)? > Is that that this is a WIP patch that the behaviour the option aims > to achieve hasn't been implemented? Is it that no existing callers > pass "-q" to the scripted version, so there is no need to support > it (if so, why do we even accept it in the first place)? Is it that > all existing callers pass "-q" so we need to accept it, but there is > nothing we need to make verbose so the variable is not passed around > in the codepath? As the original author of the shell code, I had it accept -q because, with the other subcommmands, you can pass -q either before or after the subcommand such as $ git submodule -q sync or $ git submodule sync -q and I wanted set-branch to retain that behaviour even though -q ultimately doesn't affect set-branch at all since it's already a quiet command. Perhaps as a follow-up to this patch, we could stop accepting -q in set-branch. I highly doubt that anyone is using it anyway.
Denton Liu <liu.denton@gmail.com> writes: >> Sorry, I may have missed the previous rounds of discussion, but the >> comment adds more puzzles than it helps readers. "is currently not >> used" can be seen from the code, but it is totally unclear why it is >> not used. Is that a design decision to always keep quiet or always >> talkative (if so, "suppress output..." is not a good description)? >> Is that that this is a WIP patch that the behaviour the option aims >> to achieve hasn't been implemented? Is it that no existing callers >> pass "-q" to the scripted version, so there is no need to support >> it (if so, why do we even accept it in the first place)? Is it that >> all existing callers pass "-q" so we need to accept it, but there is >> nothing we need to make verbose so the variable is not passed around >> in the codepath? > > As the original author of the shell code, I had it accept -q because, > with the other subcommmands, you can pass -q either before or after the > subcommand such as > > $ git submodule -q sync > > or > $ git submodule sync -q > > and I wanted set-branch to retain that behaviour even though -q > ultimately doesn't affect set-branch at all since it's already a quiet > command. OK, so "we accept -q for uniformity across subcommands, but there is nothing to make less verbose in this subcommand" is the answer to my question. That cannot be read from "... is currently not used"; especially with "currently", I expect that most readers would expect we would start using it in the (near) future, and some other readers would guess that something used to be talkative and we squelched it using the option but there no longer is such need because that something is now quiet by default and there is no option to make it talkative.
Hi Shourya, On 2020-05-21 22:08:19+0530, Shourya Shukla <shouryashukla.oo@gmail.com> wrote: > Thank you for the review Eric. I have changed the commit message, > and the error prompts. Also, I have added a brief comment about > the `quiet` option. > > + /* > + * The `quiet` option is present for backward compatibility > + * but is currently not used. > + */ > + int quiet = 0, opt_default = 0; > + 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")), IIUC, this option is provided to be backward compatible with old shell version, and this option doesn't affect anything. Would it make sense to hide quiet from default usage, via: OPT_NOOP_NOARG(0, "quiet") I may missed some discussion related to the decision to keep it OPT__QUIET. > + 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>"), And if above comment is applicable, remove `--quiet` from here. > + git ${wt_prefix:+-C "$wt_prefix"} ${prefix:+--super-prefix "$prefix"} submodule--helper set-branch ${GIT_QUIET:+--quiet} ${branch:+--branch $branch} ${default:+--default} -- "$@" I think we need to quote `$branch`, no? ${branch:+--branch "$branch"}
On 21/05 12:50, Junio C Hamano wrote: > OK, so "we accept -q for uniformity across subcommands, but there is > nothing to make less verbose in this subcommand" is the answer to my > question. > > That cannot be read from "... is currently not used"; especially > with "currently", I expect that most readers would expect we would > start using it in the (near) future, and some other readers would > guess that something used to be talkative and we squelched it using > the option but there no longer is such need because that something > is now quiet by default and there is no option to make it talkative. What do you think should be the most apt comment here? Also, the rest of the code is fine right?
Hi Shourya, On Thu, 21 May 2020, Shourya Shukla wrote: > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index f50745a03f..d14b9856a3 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) > +{ > + /* > + * The `quiet` option is present for backward compatibility > + * but is currently not used. > + */ > + int quiet = 0, opt_default = 0; > + 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); > + config_set_in_gitmodules_file_gently(config_name, opt_branch); What happens if this fails? E.g. when the permission is denied or disk is full? This C code would then still `return 0`, pretending that it succeeded. But the original shell script calls `git submodule--helper config [...]` which calls `module_config()`, which in turn passes through the return value of the `config_set_in_gitmodules_file_gently()` call. In other words, you need something like this: int ret; [...] ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); free(config_name); return ret; > + > + free(config_name); > + return 0; > +} > + > #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}, BTW I just noticed that the return value of these helpers is returned by the `cmd_submodule__helper()` function. That is not correct, as the convention is for Git's functions to return negative values in case of errors _except_ for `cmd_*()` functions, which need to return an exit code (valid values are between 0 and 127). So I think we'll also need this (it's unrelated to your patch, at least unrelated enough that it merits its own, separate patch): - return commands[i].fn(argc - 1, argv + 1, prefix); + return !!commands[i].fn(argc - 1, argv + 1, prefix); Ciao, Dscho > }; > > 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} -- "$@" > } > > # > -- > 2.26.2 > >
Shourya Shukla <shouryashukla.oo@gmail.com> writes: > On 21/05 12:50, Junio C Hamano wrote: >> OK, so "we accept -q for uniformity across subcommands, but there is >> nothing to make less verbose in this subcommand" is the answer to my >> question. >> >> That cannot be read from "... is currently not used"; especially >> with "currently", I expect that most readers would expect we would >> start using it in the (near) future, and some other readers would >> guess that something used to be talkative and we squelched it using >> the option but there no longer is such need because that something >> is now quiet by default and there is no option to make it talkative. > > What do you think should be the most apt comment here? "we accept -q for uniformity across subcommands, but there is nothing to make less verbose in this subcommand", perhaps? > Also, the rest of the code is fine right? I didn't spot anything bad worth pointing out when I sent the review message, but that does not necessarily mean the code is "fine" ;-) I see you have v4 sent out already, which probably has more improvements based on others' input. Thanks for working on this topic.
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> + config_set_in_gitmodules_file_gently(config_name, opt_branch); > > What happens if this fails? E.g. when the permission is denied or disk is > full? This C code would then still `return 0`, pretending that it > succeeded. But the original shell script calls `git submodule--helper > config [...]` which calls `module_config()`, which in turn passes through > the return value of the `config_set_in_gitmodules_file_gently()` call. > > In other words, you need something like this: > > int ret; > > [...] > > ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); > > free(config_name); > return ret; Making sure we check the return value of helper functions we call is a good discipline, but this is not quite enough. > So I think we'll also need this (it's unrelated to your patch, at least > unrelated enough that it merits its own, separate patch): > > - return commands[i].fn(argc - 1, argv + 1, prefix); > + return !!commands[i].fn(argc - 1, argv + 1, prefix); I checked (not all but most of the) functions in that commands[] table and they all seem to return 0 for success and positive non-zero for failure. config_set_in_gitmodules_file_gently() takes the return value of a helper function in its 'ret', gives an warning if it is negative, and returns that 'ret' literally to the caller. You suggestion allows module_set_branch() return a negative value as-is. You'd need to return !!ret from there. The "unrelated" change becomes only necessary if you do not do the !!ret in module_set_branch(); otherwise it is unneeded, I think. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > >>> + config_set_in_gitmodules_file_gently(config_name, opt_branch); >> >> What happens if this fails? E.g. when the permission is denied or disk is >> full? This C code would then still `return 0`, pretending that it >> succeeded. But the original shell script calls `git submodule--helper >> config [...]` which calls `module_config()`, which in turn passes through >> the return value of the `config_set_in_gitmodules_file_gently()` call. >> >> In other words, you need something like this: >> >> int ret; >> >> [...] >> >> ret = config_set_in_gitmodules_file_gently(config_name, opt_branch); >> >> free(config_name); >> return ret; > > Making sure we check the return value of helper functions we call is > a good discipline,... By the way, another topic by you for set-url has exactly the same issue. Its call to config_set_in_gitmodules_file_gently() can fail. So can the call to sync_submodule(), but when it fails it won't come back, so we do not have to worry about not capturing its return value ;-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index f50745a03f..d14b9856a3 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) +{ + /* + * The `quiet` option is present for backward compatibility + * but is currently not used. + */ + int quiet = 0, opt_default = 0; + 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); + 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 +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 Eric. I have changed the commit message, and the error prompts. Also, I have added a brief comment about the `quiet` option. builtin/submodule--helper.c | 45 +++++++++++++++++++++++++++++++++++++ git-submodule.sh | 32 +++----------------------- 2 files changed, 48 insertions(+), 29 deletions(-)