| Message ID | 20200602163523.7131-1-shouryashukla.oo@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GSoC,v5] 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> > 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.
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
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 >
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.
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?
Đ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".
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.
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?
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.
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.
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.
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} -- "$@" } #
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(-)