On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> wrote: > > On 02-06-2020 22:05, Shourya Shukla wrote: > > Convert submodule subcommand 'set-branch' to a builtin and call it via > > 'git-submodule.sh'. > > > > Mentored-by: Christian Couder <chriscool@xxxxxxxxxxxxx> > > Mentored-by: Kaartic Sivaraam <kaartic.sivaraam@xxxxxxxxx> > > Helped-by: Denton Liu <liu.denton@xxxxxxxxx> > > Helped-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx> > > Helped-by: Đoàn Trần Công Danh <congdanhqx@xxxxxxxxx> > > Signed-off-by: Shourya Shukla <shouryashukla.oo@xxxxxxxxx> > > --- > > Here is the v5 of the subcommand. Thank you Danh for the feedback! I > > apologise for not replying on time. I have taken into account Danh's > > suggestions on the `quiet` option as well as done the fixup Dscho > > suggested (fixed by Junio here: > > https://github.com/gitster/git/commit/77ba62f66ff8e3de54d81c240542edb42a2711c7) > > > > builtin/submodule--helper.c | 44 +++++++++++++++++++++++++++++++++++++ > > git-submodule.sh | 32 +++------------------------ > > 2 files changed, 47 insertions(+), 29 deletions(-) > > > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > > index f50745a03f..a974e17571 100644 > > --- a/builtin/submodule--helper.c > > +++ b/builtin/submodule--helper.c > > @@ -2284,6 +2284,49 @@ 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 opt_default = 0, ret; > > + const char *opt_branch = NULL; > > + const char *path; > > + char *config_name; > > + > > + /* > > + * We accept the `quiet` option for uniformity across subcommands, > > + * though there is nothing to make less verbose in this subcommand. > > + */ > > + struct option options[] = { > > + OPT_NOOP_NOARG('q', "quiet"), > > + OPT_BOOL('d', "default", &opt_default, > > + N_("set the default tracking branch to master")), > > + OPT_STRING('b', "branch", &opt_branch, N_("branch"), > > + N_("set the default tracking branch")), > > + OPT_END() > > + }; > > + const char *const usage[] = { > > + N_("git submodule--helper set-branch [-q|--quiet] (-d|--default) <path>"), > > + N_("git submodule--helper set-branch [-q|--quiet] (-b|--branch) <branch> <path>"), > > + NULL > > + }; > > I'm having second thoughts about my suggestion[1] to include > the short option for '--quiet' in the usage. This is the only > usage in submodule--helper that mentions that '-q' is a short > hand for '--quiet'. That seems inconsistent. I see two ways but > I'm not sure which one of these would be better: > > A. Dropping the mention of '-q' in this usage thus making it consistent > with the other usages printed by submodule--helper. > > B. Fixing other usages of submodule--helper to mention that '-q' is > shorthand for quiet. This has the benefit of properly advertising > the shorthand. > > C. Just ignore this? The `git submodule` documentation has: -q:: --quiet:: Only print error messages. even though the Synopsis is: 'git submodule' [--quiet] [--cached] 'git submodule' [--quiet] add [<options>] [--] <repository> [<path>] 'git submodule' [--quiet] status [--cached] [--recursive] [--] [<path>...] ... So I prefer B, and maybe updating the synopsis, as I think most Git commands have '-q' meaning '--quiet'. [...] > So, according to this, I think the usage should be ... > > git submodule--helper set-branch [-q | --quiet] [-d | --default] <path> > > ... and ... > > git submodule--helper set-branch [-q|--quiet] [-b | --branch]<branch> <path> > > ... respectively. I don't agree. I think `git submodule--helper set-branch ...` requires either "-d | --default" or "-b | --branch", while for example: git submodule--helper set-branch [-q | --quiet] [-d | --default] <path> would mean that "git submodule--helper set-branch my/path" is valid.
