Re: [GSoC][PATCH v5] submodule: port subcommand 'set-branch' from shell to C

[Christian Couder <christian.couder@gmail.com>]

On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> 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@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>
> > Helped-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> > Signed-off-by: Shourya Shukla <shouryashukla.oo@gmail.com>
> > ---
> > 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.