[PATCH v1] git-clone.txt: add the --recursive option

[PATCH v1] git-clone.txt: add the --recursive option

[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

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Eric Sunshine]

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.

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Alban Gruin]

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

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Eric Sunshine]

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.

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Alban Gruin]

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

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Junio C Hamano]

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.

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Eric Sunshine]

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.

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Junio C Hamano]

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.

Re* [PATCH v1] git-clone.txt: add the --recursive option

[Junio C Hamano]

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

Re: [PATCH v1] git-clone.txt: add the --recursive option

[Junio C Hamano]

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.