* [PATCH v1] git-clone.txt: add the --recursive option @ 2021-09-13 18:59 Alban Gruin 2021-09-13 19:26 ` Eric Sunshine 0 siblings, 1 reply; 10+ messages in thread From: Alban Gruin @ 2021-09-13 18:59 UTC (permalink / raw) To: git; +Cc: Alban Gruin This adds the --recursive option, an alias of --recurse-submodule, to git-clone's manual page. Signed-off-by: Alban Gruin <alban.gruin@gmail.com> --- I found this out when a friend told me he could not remember how to fetch submodules with git-clone, and when another one suggested `--recurse-submodule'. I checked the man page, and I was surprised to find out that `--recursive' is not mentionned at all. I did not modify the synopsis. So, this alias, although shorter than the "real" option, would still be somewhat hidden in the man page. Documentation/git-clone.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index 3fe3810f1c..8a578252a0 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -270,6 +270,7 @@ branch. This is useful e.g. to maintain minimal clones of the default branch of some repository for search indexing. --recurse-submodules[=<pathspec>]:: +--recursive[=<pathspec>]:: After the clone is created, initialize and clone submodules within based on the provided pathspec. If no pathspec is provided, all submodules are initialized and cloned. -- 2.30.2 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-13 18:59 [PATCH v1] git-clone.txt: add the --recursive option Alban Gruin @ 2021-09-13 19:26 ` Eric Sunshine 2021-09-13 20:42 ` Alban Gruin 2021-09-13 21:43 ` Junio C Hamano 0 siblings, 2 replies; 10+ messages in thread From: Eric Sunshine @ 2021-09-13 19:26 UTC (permalink / raw) To: Alban Gruin; +Cc: Git List On Mon, Sep 13, 2021 at 3:14 PM Alban Gruin <alban.gruin@gmail.com> wrote: > This adds the --recursive option, an alias of --recurse-submodule, to > git-clone's manual page. > > Signed-off-by: Alban Gruin <alban.gruin@gmail.com> > --- > I found this out when a friend told me he could not remember how to > fetch submodules with git-clone, and when another one suggested > `--recurse-submodule'. I checked the man page, and I was surprised to > find out that `--recursive' is not mentionned at all. > > I did not modify the synopsis. So, this alias, although shorter than > the "real" option, would still be somewhat hidden in the man page. Considering that the `--recursive` option was intentionally removed from `git-clone.txt` by bb62e0a99f (clone: teach --recurse-submodules to optionally take a pathspec, 2017-03-17), it's not clear that this change helps the situation. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-13 19:26 ` Eric Sunshine @ 2021-09-13 20:42 ` Alban Gruin 2021-09-13 21:57 ` Eric Sunshine 2021-09-13 21:43 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Alban Gruin @ 2021-09-13 20:42 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List Hi Eric, Le 13/09/2021 à 21:26, Eric Sunshine a écrit : > On Mon, Sep 13, 2021 at 3:14 PM Alban Gruin <alban.gruin@gmail.com> wrote: >> This adds the --recursive option, an alias of --recurse-submodule, to >> git-clone's manual page. >> >> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >> --- >> I found this out when a friend told me he could not remember how to >> fetch submodules with git-clone, and when another one suggested >> `--recurse-submodule'. I checked the man page, and I was surprised to >> find out that `--recursive' is not mentionned at all. >> >> I did not modify the synopsis. So, this alias, although shorter than >> the "real" option, would still be somewhat hidden in the man page. > > Considering that the `--recursive` option was intentionally removed > from `git-clone.txt` by bb62e0a99f (clone: teach --recurse-submodules > to optionally take a pathspec, 2017-03-17), it's not clear that this > change helps the situation. > The patch you mention also hides --recursive from the option array, but that was reverted with 5c387428f1 (parse-options: don't emit "ambiguous option" for aliases, 2019-04-29). The option should be re-hidden, or even removed. Alban ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-13 20:42 ` Alban Gruin @ 2021-09-13 21:57 ` Eric Sunshine 2021-09-14 10:27 ` Alban Gruin 0 siblings, 1 reply; 10+ messages in thread From: Eric Sunshine @ 2021-09-13 21:57 UTC (permalink / raw) To: Alban Gruin; +Cc: Git List On Mon, Sep 13, 2021 at 4:42 PM Alban Gruin <alban.gruin@gmail.com> wrote: > Le 13/09/2021 à 21:26, Eric Sunshine a écrit : > > On Mon, Sep 13, 2021 at 3:14 PM Alban Gruin <alban.gruin@gmail.com> wrote: > >> This adds the --recursive option, an alias of --recurse-submodule, to > >> git-clone's manual page. > > > > Considering that the `--recursive` option was intentionally removed > > from `git-clone.txt` by bb62e0a99f (clone: teach --recurse-submodules > > to optionally take a pathspec, 2017-03-17), it's not clear that this > > change helps the situation. > > The patch you mention also hides --recursive from the option array, but > that was reverted with 5c387428f1 (parse-options: don't emit "ambiguous > option" for aliases, 2019-04-29). The option should be re-hidden, or > even removed. I don't quite follow. As far as I understand both by reading 5c387428f1 and by testing, 5c387428f1 fixed tab-completion so it would _not_ show `--recursive`. Anyhow, another approach which we've used elsewhere is to mention the option in the documentation but indicate clearly that it's deprecated. That way, people who run across the option in existing scripts or old blogs can at least find out what it means. Something like: --recurse-submodules[=<pathspec>]:: After the clone is created, initialize and clone submodules within based on the provided pathspec. If no pathspec is provided, all submodules are initialized and cloned. (`--recursive` is a deprecated synonym.) I don't have an opinion as to whether or not we'd want to do that in this case. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-13 21:57 ` Eric Sunshine @ 2021-09-14 10:27 ` Alban Gruin 2021-09-14 17:46 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Alban Gruin @ 2021-09-14 10:27 UTC (permalink / raw) To: Eric Sunshine; +Cc: Git List Hi Eric, Le 13/09/2021 à 23:57, Eric Sunshine a écrit : > On Mon, Sep 13, 2021 at 4:42 PM Alban Gruin <alban.gruin@gmail.com> wrote: >> Le 13/09/2021 à 21:26, Eric Sunshine a écrit : >>> On Mon, Sep 13, 2021 at 3:14 PM Alban Gruin <alban.gruin@gmail.com> wrote: >>>> This adds the --recursive option, an alias of --recurse-submodule, to >>>> git-clone's manual page. >>> >>> Considering that the `--recursive` option was intentionally removed >>> from `git-clone.txt` by bb62e0a99f (clone: teach --recurse-submodules >>> to optionally take a pathspec, 2017-03-17), it's not clear that this >>> change helps the situation. >> >> The patch you mention also hides --recursive from the option array, but >> that was reverted with 5c387428f1 (parse-options: don't emit "ambiguous >> option" for aliases, 2019-04-29). The option should be re-hidden, or >> even removed. > > I don't quite follow. As far as I understand both by reading > 5c387428f1 and by testing, 5c387428f1 fixed tab-completion so it would > _not_ show `--recursive`. > bb62e0a99f hid --recursive from `git clone -h' with PARSE_OPT_HIDDEN, but 5c387428f1 reverted that: $ git checkout 5c387428f1~ $ make $ bin-wrappers/git clone -h ... -s, --shared setup as shared repository --recurse-submodules[=<pathspec>] initialize submodules in the clone -j, --jobs <n> number of submodules cloned in parallel ... $ git checkout 5c387428f1 $ make $ bin-wrappers/git clone -h ... --recursive[=<pathspec>] initialize submodules in the clone --recurse-submodules[=<pathspec>] initialize submodules in the clone ... The two options were then reordered by c28b036fe3 (clone: reorder --recursive/--recurse-submodules, 2020-03-16), and this is where we are today: $ git clone -h ... --recurse-submodules[=<pathspec>] initialize submodules in the clone --recursive[=<pathspec>] alias of --recurse-submodules ... Junio did mention[0] that --recursive was no longer in the manual, but not that it was once hidden from the option list. > Anyhow, another approach which we've used elsewhere is to mention the > option in the documentation but indicate clearly that it's deprecated. > That way, people who run across the option in existing scripts or old > blogs can at least find out what it means. Something like: > > --recurse-submodules[=<pathspec>]:: > After the clone is created, initialize and clone submodules > within based on the provided pathspec. If no pathspec is > provided, all submodules are initialized and cloned. > (`--recursive` is a deprecated synonym.) > > I don't have an opinion as to whether or not we'd want to do that in this case. > [0] https://lore.kernel.org/git/20200316212857.259093-3-gitster@pobox.com/ Alban ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-14 10:27 ` Alban Gruin @ 2021-09-14 17:46 ` Junio C Hamano 2021-09-14 17:53 ` Eric Sunshine 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2021-09-14 17:46 UTC (permalink / raw) To: Alban Gruin; +Cc: Eric Sunshine, Git List Alban Gruin <alban.gruin@gmail.com> writes: > Junio did mention[0] that --recursive was no longer in the manual, but > not that it was once hidden from the option list. Please allow me to summarize the discussion so far. We want our subcommands to take "--recurse-submodules=<arg>" uniformly, while accepting any unique prefix, e.g. --recurs=<arg>, as its short-hand. For "git clone", we kept "--recurse=<it>" in the options[] table as a HIDDEN entry as part of our deprecation plan. This nicely hid the deprecated "--recurse=<it>" from "git clone -h". But it backfired because "git cmd --recur=<it>" was not a "unique prefix" (as it matched both) and triggered a disambiguation error. To solve it, we introduced OPT_ALIAS() to tell the machinery that allows unique prefix that these two are the same thing. As a side effect, because the use of OPT_ALIAS() did not have HIDDEN bit, we started showing the deprecated "--recurse" in "git clone -h" output. Is that where we are? I am wondering if it is just a matter of either * removing the "recursive" alias from the options table. Because we accept unique prefix, --recurse=<arg> the user types will be taken as --recurse-submodules=<arg> anyway (until "git clone" learns another option --recurse-xyzzy=<arg>, at which time it will become ambiguous and error out, that is). or * adding the PARSE_OPT_HIDDEN bit to the OPT_ALIAS() element for the deprecated "recurse" option. and all would be fine? Between adding "--recursive" to the manual and describing it as a deprecated synonym for "--recurse-submodules", and not doing so, I do not have a strong preference. Thanks. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-14 17:46 ` Junio C Hamano @ 2021-09-14 17:53 ` Eric Sunshine 2021-09-14 18:31 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Eric Sunshine @ 2021-09-14 17:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Alban Gruin, Git List On Tue, Sep 14, 2021 at 1:46 PM Junio C Hamano <gitster@pobox.com> wrote: > I am wondering if it is just a matter of either > > * removing the "recursive" alias from the options table. Because > we accept unique prefix, --recurse=<arg> the user types will be > taken as --recurse-submodules=<arg> anyway (until "git clone" > learns another option --recurse-xyzzy=<arg>, at which time it > will become ambiguous and error out, that is). With this option, we risk breaking existing tooling which happens to use the deprecated --recursive. > or > > * adding the PARSE_OPT_HIDDEN bit to the OPT_ALIAS() element for > the deprecated "recurse" option. I was going to suggest this as a possible way forward to address Alban's most recent response to my response. The lack of PARSE_OPT_HIDDEN on OPT_ALIAS() almost seems like an oversight. > Between adding "--recursive" to the manual and describing it as a > deprecated synonym for "--recurse-submodules", and not doing so, I > do not have a strong preference. I don't have a strong preference either, especially considering how long ago --recursive was removed from the manual, however, adding it would help someone who runs across --recursive in existing tooling or old blog post and wants to know what it does. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-14 17:53 ` Eric Sunshine @ 2021-09-14 18:31 ` Junio C Hamano 2021-09-14 20:21 ` Re* " Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2021-09-14 18:31 UTC (permalink / raw) To: Eric Sunshine; +Cc: Alban Gruin, Git List Eric Sunshine <sunshine@sunshineco.com> writes: > On Tue, Sep 14, 2021 at 1:46 PM Junio C Hamano <gitster@pobox.com> wrote: >> I am wondering if it is just a matter of either >> >> * removing the "recursive" alias from the options table. Because >> we accept unique prefix, --recurse=<arg> the user types will be >> taken as --recurse-submodules=<arg> anyway (until "git clone" >> learns another option --recurse-xyzzy=<arg>, at which time it >> will become ambiguous and error out, that is). > > With this option, we risk breaking existing tooling which happens to > use the deprecated --recursive. Ahh, sorry and thanks for correcting my stupid thinko. recursive is not a prefix of recurse-submodules. >> * adding the PARSE_OPT_HIDDEN bit to the OPT_ALIAS() element for >> the deprecated "recurse" option. > > I was going to suggest this as a possible way forward to address > Alban's most recent response to my response. The lack of > PARSE_OPT_HIDDEN on OPT_ALIAS() almost seems like an oversight. You may have an alias with no intention to deprecate either, so it would make it cumbersome if OPT_ALIAS() always meant HIDDEN, just like it currently is cumbersome for an alias that is deprecated. Independently (because I do not think this helps in solving the current situation), we might want to tweak the disambiguation machinery to require HIDDEN ones to be spelled out exactly, because they are hidden for a reason---we do not want users to casually and accidentally trigger them. Of course, that is totally outside the scope of everything we discussed so far. >> Between adding "--recursive" to the manual and describing it as a >> deprecated synonym for "--recurse-submodules", and not doing so, I >> do not have a strong preference. > > I don't have a strong preference either, especially considering how > long ago --recursive was removed from the manual, however, adding it > would help someone who runs across --recursive in existing tooling or > old blog post and wants to know what it does. Makes sense. I think we list other deprecated ones for that exact reason. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re* [PATCH v1] git-clone.txt: add the --recursive option 2021-09-14 18:31 ` Junio C Hamano @ 2021-09-14 20:21 ` Junio C Hamano 0 siblings, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2021-09-14 20:21 UTC (permalink / raw) To: Git List; +Cc: Alban Gruin, Eric Sunshine Subject: parse-options: allow hidden aliases When OPT_ALIAS() was introduced to mark one option is a mere synonym for another option, we forgot to add support for a use case where an option is made an alias with an intention to deprecat and eventually remove it in the future, which usually means "git cmd -h" hides the deprecated alias while "git cmd --help-all" shows it. The "--recursive" option of "git clone" and the "--mailmap" option of "git log" use the OPT_ALIAS mechansim to mark themselves as an alias of another. The former has been deprecated but "git clone -h" still shows it. Introduce OPT_HIDDEN_ALIAS() that hides the entry from "git cmd -h" output and use it for "git clone --recursive". Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * So here is to add support for hidden aliases and application of it on "git clone". Perhaps everything except for the part that applies to "builtin/clone.c" should become [1/2] of a two-patch series, while the change to "builtin/clone.c", plus documentation updates to mention "--recursive" as a deprecated synonym, should become [2/2]. But I do not have time to go that last mile right now ;-) >>> * adding the PARSE_OPT_HIDDEN bit to the OPT_ALIAS() element for >>> the deprecated "recurse" option. >> >> I was going to suggest this as a possible way forward to address >> Alban's most recent response to my response. The lack of >> PARSE_OPT_HIDDEN on OPT_ALIAS() almost seems like an oversight. > > You may have an alias with no intention to deprecate either, so it > would make it cumbersome if OPT_ALIAS() always meant HIDDEN, just > like it currently is cumbersome for an alias that is deprecated. builtin/clone.c | 2 +- parse-options.c | 4 +++- parse-options.h | 3 +++ t/helper/test-parse-options.c | 1 + t/t0040-parse-options.sh | 13 ++++++++++++- 5 files changed, 20 insertions(+), 3 deletions(-) diff --git c/builtin/clone.c w/builtin/clone.c index 66fe66679c..6fd4b41eb3 100644 --- c/builtin/clone.c +++ w/builtin/clone.c @@ -110,7 +110,7 @@ static struct option builtin_clone_options[] = { { OPTION_CALLBACK, 0, "recurse-submodules", &option_recurse_submodules, N_("pathspec"), N_("initialize submodules in the clone"), PARSE_OPT_OPTARG, recurse_submodules_cb, (intptr_t)"." }, - OPT_ALIAS(0, "recursive", "recurse-submodules"), + OPT_HIDDEN_ALIAS(0, "recursive", "recurse-submodules"), OPT_INTEGER('j', "jobs", &max_jobs, N_("number of submodules cloned in parallel")), OPT_STRING(0, "template", &option_template, N_("template-directory"), diff --git c/parse-options.c w/parse-options.c index 2abff136a1..46af4eacdf 100644 --- c/parse-options.c +++ w/parse-options.c @@ -653,6 +653,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, int short_name; const char *long_name; const char *source; + int flags; struct strbuf help = STRBUF_INIT; int j; @@ -662,6 +663,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, short_name = newopt[i].short_name; long_name = newopt[i].long_name; source = newopt[i].value; + flags = newopt[i].flags; if (!long_name) BUG("An alias must have long option name"); @@ -680,7 +682,7 @@ static struct option *preprocess_options(struct parse_opt_ctx_t *ctx, newopt[i].short_name = short_name; newopt[i].long_name = long_name; newopt[i].help = strbuf_detach(&help, NULL); - newopt[i].flags |= PARSE_OPT_FROM_ALIAS; + newopt[i].flags |= PARSE_OPT_FROM_ALIAS | flags; break; } diff --git c/parse-options.h w/parse-options.h index a845a9d952..8ba72c7916 100644 --- c/parse-options.h +++ w/parse-options.h @@ -201,6 +201,9 @@ struct option { #define OPT_ALIAS(s, l, source_long_name) \ { OPTION_ALIAS, (s), (l), (source_long_name) } +#define OPT_HIDDEN_ALIAS(s, l, source_long_name) \ + { OPTION_ALIAS, (s), (l), (source_long_name), NULL, NULL, PARSE_OPT_HIDDEN } + /* * parse_options() will filter out the processed options and leave the * non-option arguments in argv[]. argv0 is assumed program name and diff --git c/t/helper/test-parse-options.c w/t/helper/test-parse-options.c index 2051ce57db..86c3eb1a29 100644 --- c/t/helper/test-parse-options.c +++ w/t/helper/test-parse-options.c @@ -154,6 +154,7 @@ int cmd__parse_options(int argc, const char **argv) OPT_GROUP("Alias"), OPT_STRING('A', "alias-source", &string, "string", "get a string"), OPT_ALIAS('Z', "alias-target", "alias-source"), + OPT_HIDDEN_ALIAS(0, "hidden-alias", "alias-source"), OPT_END(), }; int i; diff --git c/t/t0040-parse-options.sh w/t/t0040-parse-options.sh index ad4746d899..4d31367b07 100755 --- c/t/t0040-parse-options.sh +++ w/t/t0040-parse-options.sh @@ -7,7 +7,7 @@ test_description='our own option parser' . ./test-lib.sh -cat >expect <<\EOF +cat >help-all.in <<\EOF usage: test-tool parse-options <options> A helper function for the parse-options API. @@ -34,6 +34,7 @@ String options --string2 <str> get another string --st <st> get another string (pervert ordering) -o <str> get another string +# --obsolete no-op (backward compatibility) --list <str> add str to list Magic arguments @@ -55,10 +56,20 @@ Alias get a string -Z, --alias-target <string> alias of --alias-source +# --hidden-alias <string> +# alias of --alias-source EOF +test_expect_success 'hidden alias in test help' ' + sed -e "s/^#//" help-all.in >expect && + test_must_fail test-tool parse-options --help-all >output 2>output.err && + test_must_be_empty output.err && + test_cmp expect output +' + test_expect_success 'test help' ' + sed -e "/^#/d" help-all.in >expect && test_must_fail test-tool parse-options -h >output 2>output.err && test_must_be_empty output.err && test_cmp expect output ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v1] git-clone.txt: add the --recursive option 2021-09-13 19:26 ` Eric Sunshine 2021-09-13 20:42 ` Alban Gruin @ 2021-09-13 21:43 ` Junio C Hamano 1 sibling, 0 replies; 10+ messages in thread From: Junio C Hamano @ 2021-09-13 21:43 UTC (permalink / raw) To: Eric Sunshine; +Cc: Alban Gruin, Git List Eric Sunshine <sunshine@sunshineco.com> writes: > On Mon, Sep 13, 2021 at 3:14 PM Alban Gruin <alban.gruin@gmail.com> wrote: >> This adds the --recursive option, an alias of --recurse-submodule, to >> git-clone's manual page. >> >> Signed-off-by: Alban Gruin <alban.gruin@gmail.com> >> --- >> I found this out when a friend told me he could not remember how to >> fetch submodules with git-clone, and when another one suggested >> `--recurse-submodule'. I checked the man page, and I was surprised to >> find out that `--recursive' is not mentionned at all. >> >> I did not modify the synopsis. So, this alias, although shorter than >> the "real" option, would still be somewhat hidden in the man page. > > Considering that the `--recursive` option was intentionally removed > from `git-clone.txt` by bb62e0a99f (clone: teach --recurse-submodules > to optionally take a pathspec, 2017-03-17), it's not clear that this > change helps the situation. A logical continuation of what bb62e0a99f tried to do might be to hide the --recursive[=<pathspec>] from "git clone -h", I guess. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-14 20:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-09-13 18:59 [PATCH v1] git-clone.txt: add the --recursive option Alban Gruin 2021-09-13 19:26 ` Eric Sunshine 2021-09-13 20:42 ` Alban Gruin 2021-09-13 21:57 ` Eric Sunshine 2021-09-14 10:27 ` Alban Gruin 2021-09-14 17:46 ` Junio C Hamano 2021-09-14 17:53 ` Eric Sunshine 2021-09-14 18:31 ` Junio C Hamano 2021-09-14 20:21 ` Re* " Junio C Hamano 2021-09-13 21:43 ` Junio C Hamano
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.