[GSoC,v5] submodule: port subcommand 'set-branch' from shell to C
diff mbox series

Message ID 20200602163523.7131-1-shouryashukla.oo@gmail.com
State New
Headers show
Series
  • [GSoC,v5] submodule: port subcommand 'set-branch' from shell to C
Related show

Commit Message

Shourya Shukla June 2, 2020, 4:35 p.m. UTC
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(-)

Comments

Junio C Hamano June 2, 2020, 5:58 p.m. UTC | #1
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>
> 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>"),


I notice that we gained back -d and -b shorthands that was
advertised but not implemented the previous rounds.  It is a bit
curious that we are adding these short-hands that nobody uses,
though.  

Will queue.  Thanks.
Kaartic Sivaraam June 2, 2020, 7:01 p.m. UTC | #2
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?

I also noticed one other thing. A quote from
Documentation/CodingGuidelines regarding the usage for reference:

>  Optional parts are enclosed in square brackets:
>    [<extra>]
>    (Zero or one <extra>.)
> 
>    --exec-path[=<path>]
>    (Option with an optional argument.  Note that the "=" is inside the
>    brackets.)
> 
>    [<patch>...]
>    (Zero or more of <patch>.  Note that the dots are inside, not
>    outside the brackets.)
> 
>  Multiple alternatives are indicated with vertical bars:
>    [-q | --quiet]
>    [--utf8 | --no-utf8]
> 
>  Parentheses are used for grouping:
>    [(<rev> | <range>)...]
>    (Any number of either <rev> or <range>.  Parens are needed to make
>    it clear that "..." pertains to both <rev> and <range>.)
> 
>    [(-p <parent>)...]
>    (Any number of option -p, each with one <parent> argument.)
> 
>    git remote set-head <name> (-a | -d | <branch>)
>    (One and only one of "-a", "-d" or "<branch>" _must_ (no square
>    brackets) be provided.)

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.

> +		NULL
> +	};

---
Footnotes:

[1]: 
https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384
Kaartic Sivaraam June 2, 2020, 7:10 p.m. UTC | #3
On Wed, Jun 3, 2020 at 12:31 AM Kaartic Sivaraam
<kaartic.sivaraam@gmail.com> wrote:
>
> I also noticed one other thing. A quote from
> Documentation/CodingGuidelines regarding the usage for reference:
>
> >  Optional parts are enclosed in square brackets:
> >    [<extra>]
> >    (Zero or one <extra>.)
> >
> >    --exec-path[=<path>]
> >    (Option with an optional argument.  Note that the "=" is inside the
> >    brackets.)
> >
> >    [<patch>...]
> >    (Zero or more of <patch>.  Note that the dots are inside, not
> >    outside the brackets.)
> >
> >  Multiple alternatives are indicated with vertical bars:
> >    [-q | --quiet]
> >    [--utf8 | --no-utf8]
> >
> >  Parentheses are used for grouping:
> >    [(<rev> | <range>)...]
> >    (Any number of either <rev> or <range>.  Parens are needed to make
> >    it clear that "..." pertains to both <rev> and <range>.)
> >
> >    [(-p <parent>)...]
> >    (Any number of option -p, each with one <parent> argument.)
> >
> >    git remote set-head <name> (-a | -d | <branch>)
> >    (One and only one of "-a", "-d" or "<branch>" _must_ (no square
> >    brackets) be provided.)
>
> 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>
>

Apologies, my mail client messed a little with the formatting.
This should actually be:

    git submodule--helper set-branch [-q | --quiet] [-b | --branch]
<branch> <path>

> ... respectively.
>
> > +             NULL
> > +     };
>
> ---
> Footnotes:
>
> [1]:
> https://github.com/periperidip/git/commit/9a8918bf0688c583740b3dddafdba82f47972442#r39606384
>
Christian Couder June 2, 2020, 7:45 p.m. UTC | #4
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.
Đoàn Trần Công Danh June 3, 2020, 12:12 a.m. UTC | #5
On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:
> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
> 
> > +	 * 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>"),
> 
> 
> I notice that we gained back -d and -b shorthands that was
> advertised but not implemented the previous rounds.  It is a bit
> curious that we are adding these short-hands that nobody uses,
> though.  

I think a day will come, when all git-submodule functionalities will
run by calling git-submodule--helper.

In that day, we will use current git-submodule--helper as the new
git-submodule.

To me, it'll be less noise to just gs/--helper// from this file and use
it as the new git-submodule, instead of changing the OPT_* all over
places.

Or is that a complain for missing some tests?
Junio C Hamano June 3, 2020, 8:02 p.m. UTC | #6
Đoàn Trần Công Danh  <congdanhqx@gmail.com> writes:

> On 2020-06-02 10:58:45-0700, Junio C Hamano <gitster@pobox.com> wrote:
>> Shourya Shukla <shouryashukla.oo@gmail.com> writes:
>> 
>> > +	 * 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>"),
>> 
>> 
>> I notice that we gained back -d and -b shorthands that was
>> advertised but not implemented the previous rounds.  It is a bit
>> curious that we are adding these short-hands that nobody uses,
>> though.  
>
> I think a day will come, when all git-submodule functionalities will
> run by calling git-submodule--helper.

I'd expect that when that day with no scripted parts of "git
submodule" remains comes, the main entry point functions in
builtin/submodule--helper.c (like module_list(), update_clone(),
module_set_branch(), etc.) will become helper functions that live in
submodule-lib.c and would be called from builtin/submodule.c.  And
the conversion would rip out calls to parse_options() in each of
these functions that would migrate to submodule-lib.c

    Side note: instead of adding submodule-lib.c, you could add them
    directly to submodule.c if they are small enough.  I am however
    modeling after how the "diff" family was converted to C; the
    diff-lib.c layer is "library-ish helpers that get pre-parsed
    command line arguments and performs a single unit of work" that
    utilizes service routines at the lower layer that are in diff.c
    and submodule-lib.c and submodule.c will be in a similar kind of
    relationship.

> In that day, we will use current git-submodule--helper as the new
> git-submodule.

No, I do not think so.  Most of the option parsers would be redone
in builtin/submodule.c; only some that can be used as-is may migrate
as a whole to builtin/submodule.c and its parse_options() stuff
reused, but most of what is in submodule--helper would have to lose
their parse_options() calls, as nobody would be using module_list()
when there is no scripted "git submodule" exists, for example.

> Or is that a complain for missing some tests?

No, it was "do the minimum necessary for an implementation detail,
as we'll discard that part later anyway".
Shourya Shukla June 4, 2020, 7:09 a.m. UTC | #7
On 02/06 09:45, Christian Couder wrote:
> >
> > 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'.

Yep, (B) sounds good! Junio in one of the previous mails stated that
this thing will need to be done for all subcommands when the shell
script is to be demolished i.e., `git submodule` becomes a builtin
completely. Actually, vrious usages might need to be fixed, for many
subcommands because almost none of them have any info about the
shorthand usages of options in their `options` array.
Shourya Shukla June 4, 2020, 7:17 a.m. UTC | #8
On 03/06 01:02, Junio C Hamano wrote:
> I'd expect that when that day with no scripted parts of "git
> submodule" remains comes, the main entry point functions in
> builtin/submodule--helper.c (like module_list(), update_clone(),
> module_set_branch(), etc.) will become helper functions that live in
> submodule-lib.c and would be called from builtin/submodule.c.  And
> the conversion would rip out calls to parse_options() in each of
> these functions that would migrate to submodule-lib.c
> 
>     Side note: instead of adding submodule-lib.c, you could add them
>     directly to submodule.c if they are small enough.  I am however
>     modeling after how the "diff" family was converted to C; the
>     diff-lib.c layer is "library-ish helpers that get pre-parsed
>     command line arguments and performs a single unit of work" that
>     utilizes service routines at the lower layer that are in diff.c
>     and submodule-lib.c and submodule.c will be in a similar kind of
>     relationship.

There does exist a `submodule.c` outside of `builtin/` which has various
helper functions. Will that require renaming to `submodule-lib.c`? BTW
`set-branch` is a subcommand of `git submodule` so do we have to put it
into `submodule-lib.c` if there were to be one?
What is the motivation behind modelling it on the diff-family?
Christian Couder June 4, 2020, 7:49 a.m. UTC | #9
On Thu, Jun 4, 2020 at 9:17 AM Shourya Shukla
<shouryashukla.oo@gmail.com> wrote:
>
> On 03/06 01:02, Junio C Hamano wrote:
> > I'd expect that when that day with no scripted parts of "git
> > submodule" remains comes, the main entry point functions in
> > builtin/submodule--helper.c (like module_list(), update_clone(),
> > module_set_branch(), etc.) will become helper functions that live in
> > submodule-lib.c and would be called from builtin/submodule.c.  And
> > the conversion would rip out calls to parse_options() in each of
> > these functions that would migrate to submodule-lib.c
> >
> >     Side note: instead of adding submodule-lib.c, you could add them
> >     directly to submodule.c if they are small enough.  I am however
> >     modeling after how the "diff" family was converted to C; the
> >     diff-lib.c layer is "library-ish helpers that get pre-parsed
> >     command line arguments and performs a single unit of work" that
> >     utilizes service routines at the lower layer that are in diff.c
> >     and submodule-lib.c and submodule.c will be in a similar kind of
> >     relationship.
>
> There does exist a `submodule.c` outside of `builtin/` which has various
> helper functions. Will that require renaming to `submodule-lib.c`?

No, as Junio says that "submodule-lib.c and submodule.c will be in a
similar kind of relationship" as diff.c and diff-lib.c.

> BTW
> `set-branch` is a subcommand of `git submodule` so do we have to put it
> into `submodule-lib.c` if there were to be one?
> What is the motivation behind modelling it on the diff-family?

Maybe to separate helper functions for submodules from other submodule
functions.
Junio C Hamano June 4, 2020, 3:03 p.m. UTC | #10
Shourya Shukla <shouryashukla.oo@gmail.com> writes:

> On 03/06 01:02, Junio C Hamano wrote:
>> I'd expect that when that day with no scripted parts of "git
>> submodule" remains comes, the main entry point functions in
>> builtin/submodule--helper.c (like module_list(), update_clone(),
>> module_set_branch(), etc.) will become helper functions that live in
>> submodule-lib.c and would be called from builtin/submodule.c.  And
>> the conversion would rip out calls to parse_options() in each of
>> these functions that would migrate to submodule-lib.c
>> 
>>     Side note: instead of adding submodule-lib.c, you could add them
>>     directly to submodule.c if they are small enough.  I am however
>>     modeling after how the "diff" family was converted to C; the
>>     diff-lib.c layer is "library-ish helpers that get pre-parsed
>>     command line arguments and performs a single unit of work" that
>>     utilizes service routines at the lower layer that are in diff.c
>>     and submodule-lib.c and submodule.c will be in a similar kind of
>>     relationship.
>
> There does exist a `submodule.c` outside of `builtin/` which has various
> helper functions. Will that require renaming to `submodule-lib.c`?

No, that is different from what I wrote above.  Just like there is
the middle-layer diff-lib.c between the top-layer builtin/diff.c and
the low-level helper sets in diff.c, I envision that between the
top-layer builtin/submodule.c and the low-level helper sets in
submodule.c, there would be the middle layer submodule-lib.c.

If a single cmd_submodule_set_url() function implements the whole of
"git submoduel set-url" (by calling helper routines in submodule.c
and those currently in builtin/submodule--helper.c), I would expect
it to reside in builtin/submodule.c.
Kaartic Sivaraam June 4, 2020, 7:26 p.m. UTC | #11
On 03-06-2020 01:15, Christian Couder wrote:
> On Tue, Jun 2, 2020 at 9:01 PM Kaartic Sivaraam
> <kaartic.sivaraam@gmail.com> wrote:
>>
>> 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'.
> 

Makes sense.

> [...]
> 
>> 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.
> 

You're right. Even I thought about the same thing when I came up with
that suggestion after quoting that portion of the CodingGuidelines. But
it was also curious for me to observe that the original used parenthesis
to mention the short and long options of an argument:

> +	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've not seen such a usage before. That's what actually made me take a
look at the CodingGuidelines for this. As the CodingGuidelined doesn't
seem to be mentioning anything about this explicitly, let's see if I
could find something in the usage printed by other commands.

---
> git am -h
usage: git am [<options>] [(<mbox> | <Maildir>)...]
   or: git am [<options>] (--continue | --skip | --abort)

   <options snipped>

> git branch -h
usage: git branch [<options>] [-r | -a] [--merged | --no-merged]
   or: git branch [<options>] [-l] [-f] <branch-name> [<start-point>]
   or: git branch [<options>] [-r] (-d | -D) <branch-name>...
   or: git branch [<options>] (-m | -M) [<old-branch>] <new-branch>
   or: git branch [<options>] (-c | -C) [<old-branch>] <new-branch>
   or: git branch [<options>] [-r | -a] [--points-at]
   or: git branch [<options>] [-r | -a] [--format]

   <options snipped>

> git checkout -h
usage: git checkout [<options>] <branch>
   or: git checkout [<options>] [<branch>] -- <file>...

   <options snipped>

> git switch -h
usage: git switch [<options>] [<branch>]

   <options snipped>

---
Hmm. Looks like it's not common for us to mention both the short
and long options in the usage itself. This might be to avoid the
redundancy as the usage is usually followed by the list of options.

With this info, I think we could've just gone with the following as the
usage strings for the `set-branch` subcommand:

    git submodule--helper set-branch [<options>] -d <path>
    git submodule--helper set-branch [<options>] -b <branch> <path>

This also solves the problem with `--quiet` I mentioned earlier while
making it concise and inline with the usages printed by other commands.

All this said, I don't think it's worth a re-roll now for several
reasons.

Patch
diff mbox series

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
+	};
+
+	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 +2358,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..43eb6051d2 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} -- "$@"
 }
 
 #