All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] parse-options: don't complete option aliases by default
@ 2021-07-15 12:58 Philippe Blain via GitGitGadget
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2021-07-15 12:58 UTC (permalink / raw)
  To: git
  Cc: Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Philippe Blain, Philippe Blain

From: Philippe Blain <levraiphilippeblain@gmail.com>

Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit
"ambiguous option" for aliases, 2019-04-29), 'git clone
--git-completion-helper', which is used by the Bash completion script to
list options accepted by clone (via '__gitcomp_builtin'), lists both
'--recurse-submodules' and its alias '--recursive', which was not the
case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and
options with this flag are skipped by 'parse-options.c::show_gitcomp',
which implements 'git <cmd> --git-completion-helper'.

At the point where 'show_gitcomp' is called in 'parse_options_step',
'preprocess_options' was already called in 'parse_options', so any
aliases are now copies of the original options with a modified help text
indicating they are aliases.

Helpfully, since 64cc539fd2 (parse-options: don't leak alias help
messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag
set, so check that flag early in 'show_gitcomp' and do not print them,
unless the user explicitely requested that *all* completion be shown (by
setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage
the use of '--recurse-submodules' over '--recursive', we'd better just
suggest the former.

The only other options alias is 'log' and friends' '--mailmap', which is
an alias for '--use-mailmap', but the Bash completion helpers for these
commands do not use '__gitcomp_builtin', and thus are unnaffected by
this change.

Test the new behaviour in t9902-completion.sh. As a side effect, this
also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was
not tested before. Note that since '__gitcomp_builtin' caches the
options it shows, we need to re-source the completion script to clear
that cache for the second test.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
    parse-options: don't complete option aliases by default

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/996

 parse-options.c       |  2 +-
 t/t9902-completion.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index e6f56768ca5..2abff136a17 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)
 		if (!opts->long_name)
 			continue;
 		if (!show_all &&
-			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))
+			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))
 			continue;
 
 		switch (opts->type) {
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cb057ef1613..11573936d58 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '
 	verbose test -z "$__gitcomp_builtin_notes_edit"
 '
 
+test_expect_success 'option aliases are not shown by default' '
+	test_completion "git clone --recurs" "--recurse-submodules "
+'
+
+test_expect_success 'option aliases are shown with GIT_COMPLETION_SHOW_ALL' '
+	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
+	GIT_COMPLETION_SHOW_ALL=1 && export GIT_COMPLETION_SHOW_ALL &&
+	test_completion "git clone --recurs" <<-\EOF
+	--recurse-submodules Z
+	--recursive Z
+	EOF
+'
+
 test_expect_success '__git_complete' '
 	unset -f __git_wrap__git_main &&
 

base-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 12:58 [PATCH] parse-options: don't complete option aliases by default Philippe Blain via GitGitGadget
@ 2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
  2021-07-15 17:16   ` Felipe Contreras
                     ` (2 more replies)
  2021-07-15 17:04 ` Felipe Contreras
  2021-07-16  1:55 ` [PATCH v2] " Philippe Blain via GitGitGadget
  2 siblings, 3 replies; 9+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-07-15 16:16 UTC (permalink / raw)
  To: Philippe Blain via GitGitGadget
  Cc: git, Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Philippe Blain


On Thu, Jul 15 2021, Philippe Blain via GitGitGadget wrote:

> From: Philippe Blain <levraiphilippeblain@gmail.com>
>
> Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit
> "ambiguous option" for aliases, 2019-04-29), 'git clone
> --git-completion-helper', which is used by the Bash completion script to
> list options accepted by clone (via '__gitcomp_builtin'), lists both
> '--recurse-submodules' and its alias '--recursive', which was not the
> case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and
> options with this flag are skipped by 'parse-options.c::show_gitcomp',
> which implements 'git <cmd> --git-completion-helper'.
>
> At the point where 'show_gitcomp' is called in 'parse_options_step',
> 'preprocess_options' was already called in 'parse_options', so any
> aliases are now copies of the original options with a modified help text
> indicating they are aliases.
>
> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help
> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag
> set, so check that flag early in 'show_gitcomp' and do not print them,
> unless the user explicitely requested that *all* completion be shown (by
> setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage
> the use of '--recurse-submodules' over '--recursive', we'd better just
> suggest the former.
>
> The only other options alias is 'log' and friends' '--mailmap', which is
> an alias for '--use-mailmap', but the Bash completion helpers for these
> commands do not use '__gitcomp_builtin', and thus are unnaffected by
> this change.
>
> Test the new behaviour in t9902-completion.sh. As a side effect, this
> also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was
> not tested before. Note that since '__gitcomp_builtin' caches the
> options it shows, we need to re-source the completion script to clear
> that cache for the second test.
>
> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
> ---
>     parse-options: don't complete option aliases by default
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/996
>
>  parse-options.c       |  2 +-
>  t/t9902-completion.sh | 13 +++++++++++++
>  2 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index e6f56768ca5..2abff136a17 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)
>  		if (!opts->long_name)
>  			continue;
>  		if (!show_all &&
> -			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))
> +			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))
>  			continue;
>  
>  		switch (opts->type) {
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index cb057ef1613..11573936d58 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '
>  	verbose test -z "$__gitcomp_builtin_notes_edit"
>  '
>  
> +test_expect_success 'option aliases are not shown by default' '
> +	test_completion "git clone --recurs" "--recurse-submodules "
> +'

I'm a bit biased here since I like --recursive better, but let's not
drag that whole argument up again. I don't find the argument in
bb62e0a99fc (clone: teach --recurse-submodules to optionally take a
pathspec, 2017-03-17) convincing enough to have moved such a prominent
use-case to a longer option name.

But, water under the bridge. Aside from that:

For me a Google search for "git clone --recursive" is ~40k results, but
"git clone --recurse-submodules". The former links to various
discussions/docs/stackoverflow answers, often --recurse-submodules isn't
mentioned at all or as an aside.

I think it's unfortunate that we:

 1. Conflate whether something shows up in completion v.s. the
    help. Given its prominence and that "git clone -h" is ~50 lines why
    not note --recursive there.

 2. Don't have the completion aware of these aliases, i.e. "git clone
    --rec<TAB>" before your chance offers a completion of both, that sucks,
    we should fully complete the non-alias instead.

 3. Making it PARSE_OPT_HIDDEN "solves" #2 at the cost of hiding it in
    "git help -h", and now this won't work, but did before:

        git clone --recursi<TAB>

    I.e. even if we didn't want to do #2 *and* wanted to hide it in the
    usage output surely completing an unmbigous prefix is better, even
    if it's a hidden option?

Per-se none of this is a blocker or "we must fix this first" for this
particular change, we have this in many existing cases.

I daresay there's no other alias that's in as wide a use in the wild, so
we should think about this one particularly carefully though.

It's not fully clear from your commit message which of 1-3 you're aiming
for, I think it's more of the "discourage its use". 

Sure, fair enough, but PARSE_OPT_HIDDEN is not a 1=1 mapping to that,
and can often lead to more user confusion.

E.g. the user has used --recursive for years, but can't even find it in
-h (I also think it's a mistake to have entirely removed it from the
docs, even if one agrees with its "deprecation" I'd say we should keep
some "used to be called --recursive" note there).


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 12:58 [PATCH] parse-options: don't complete option aliases by default Philippe Blain via GitGitGadget
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
@ 2021-07-15 17:04 ` Felipe Contreras
  2021-07-16  1:28   ` Philippe Blain
  2021-07-16  1:55 ` [PATCH v2] " Philippe Blain via GitGitGadget
  2 siblings, 1 reply; 9+ messages in thread
From: Felipe Contreras @ 2021-07-15 17:04 UTC (permalink / raw)
  To: Philippe Blain via GitGitGadget, git
  Cc: Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Philippe Blain, Philippe Blain

Philippe Blain via GitGitGadget wrote:
> From: Philippe Blain <levraiphilippeblain@gmail.com>
> 
> Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit
> "ambiguous option" for aliases, 2019-04-29), 'git clone
> --git-completion-helper', which is used by the Bash completion script to
> list options accepted by clone (via '__gitcomp_builtin'), lists both
> '--recurse-submodules' and its alias '--recursive', which was not the
> case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and
> options with this flag are skipped by 'parse-options.c::show_gitcomp',
> which implements 'git <cmd> --git-completion-helper'.
> 
> At the point where 'show_gitcomp' is called in 'parse_options_step',
> 'preprocess_options' was already called in 'parse_options', so any
> aliases are now copies of the original options with a modified help text
> indicating they are aliases.
> 
> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help
> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag
> set, so check that flag early in 'show_gitcomp' and do not print them,

Makes sense but I don't think what the patch is doing should be buried
here. I think a separate paragraph explaining what you are trying to do
will be better.

> unless the user explicitely requested that *all* completion be shown (by
> setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage
> the use of '--recurse-submodules' over '--recursive', we'd better just
> suggest the former.
> 
> The only other options alias is 'log' and friends' '--mailmap', which is
> an alias for '--use-mailmap', but the Bash completion helpers for these
> commands do not use '__gitcomp_builtin', and thus are unnaffected by
> this change.
> 
> Test the new behaviour in t9902-completion.sh. As a side effect, this
> also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was
> not tested before. Note that since '__gitcomp_builtin' caches the
> options it shows, we need to re-source the completion script to clear
> that cache for the second test.

I agree this is better, and the patch itself looks obviously correct.

Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
@ 2021-07-15 17:16   ` Felipe Contreras
  2021-07-15 18:57   ` Junio C Hamano
  2021-07-16  1:31   ` Philippe Blain
  2 siblings, 0 replies; 9+ messages in thread
From: Felipe Contreras @ 2021-07-15 17:16 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Philippe Blain via GitGitGadget
  Cc: git, Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Philippe Blain

Ævar Arnfjörð Bjarmason wrote:
> On Thu, Jul 15 2021, Philippe Blain via GitGitGadget wrote:

> I'm a bit biased here since I like --recursive better, but let's not
> drag that whole argument up again. I don't find the argument in
> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a
> pathspec, 2017-03-17) convincing enough to have moved such a prominent
> use-case to a longer option name.

I agree.

> But, water under the bridge. Aside from that:
> 
> For me a Google search for "git clone --recursive" is ~40k results, but
> "git clone --recurse-submodules". The former links to various
> discussions/docs/stackoverflow answers, often --recurse-submodules isn't
> mentioned at all or as an aside.

It would be nice if facts could be used as evidence of a UI mistake, but
alas in my experience that has never been the case.

> I think it's unfortunate that we:
> 
>  1. Conflate whether something shows up in completion v.s. the
>     help. Given its prominence and that "git clone -h" is ~50 lines why
>     not note --recursive there.

Agreed.

>  2. Don't have the completion aware of these aliases, i.e. "git clone
>     --rec<TAB>" before your chance offers a completion of both, that sucks,
>     we should fully complete the non-alias instead.

Yes, that's what would happen with the patch.

>  3. Making it PARSE_OPT_HIDDEN "solves" #2 at the cost of hiding it in
>     "git help -h", and now this won't work, but did before:
> 
>         git clone --recursi<TAB>
> 
>     I.e. even if we didn't want to do #2 *and* wanted to hide it in the
>     usage output surely completing an unmbigous prefix is better, even
>     if it's a hidden option?

This is something that could be done in zsh, but not in bash (at least
not easily).

> E.g. the user has used --recursive for years, but can't even find it in
> -h (I also think it's a mistake to have entirely removed it from the
> docs, even if one agrees with its "deprecation" I'd say we should keep
> some "used to be called --recursive" note there).

But that is not a problem of this patch. If users can't find --recursive
and complain about it, then that's actually a good thing, because now
the facts about --recursive vs. --recurse-submodules are not needed
anymore, and we could just fix the interface.

-- 
Felipe Contreras

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
  2021-07-15 17:16   ` Felipe Contreras
@ 2021-07-15 18:57   ` Junio C Hamano
  2021-07-16  1:28     ` Philippe Blain
  2021-07-16  1:31   ` Philippe Blain
  2 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2021-07-15 18:57 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Philippe Blain via GitGitGadget, git,
	Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Philippe Blain

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> I'm a bit biased here since I like --recursive better, but let's not
> drag that whole argument up again. I don't find the argument in
> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a
> pathspec, 2017-03-17) convincing enough to have moved such a prominent
> use-case to a longer option name.

I do agree that it is a separate topic to pick one or the other as
the primary.  I think this change means that the users no longer
need to stop after hitting a tab:

    clone --recurs<TAB>

and instead get to "clone --recurse-submodules" right away, which is
a positigve change.  Of course, if the separate topic swaps which is
the canonical and which is the alias, the same user would get the
new canonical one right away from the same state.

So it does look like a good change to me.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 17:04 ` Felipe Contreras
@ 2021-07-16  1:28   ` Philippe Blain
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain @ 2021-07-16  1:28 UTC (permalink / raw)
  To: Felipe Contreras, Philippe Blain via GitGitGadget, git
  Cc: Nguyễn Thái Ngọc Duy, Brandon Williams,
	Ryan Zoeller, SZEDER Gábor

Hi Felipe,

Le 2021-07-15 à 13:04, Felipe Contreras a écrit :
> Philippe Blain via GitGitGadget wrote:
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>>
>> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help
>> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag
>> set, so check that flag early in 'show_gitcomp' and do not print them,
> 
> Makes sense but I don't think what the patch is doing should be buried
> here. I think a separate paragraph explaining what you are trying to do
> will be better.

Indeed. Will clarify.


> 
> I agree this is better, and the patch itself looks obviously correct.
> 
> Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>
> 

Thanks!

Philippe.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 18:57   ` Junio C Hamano
@ 2021-07-16  1:28     ` Philippe Blain
  0 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain @ 2021-07-16  1:28 UTC (permalink / raw)
  To: Junio C Hamano, Ævar Arnfjörð Bjarmason
  Cc: Philippe Blain via GitGitGadget, git,
	Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor

Hi Junio,

Le 2021-07-15 à 14:57, Junio C Hamano a écrit :
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>> I'm a bit biased here since I like --recursive better, but let's not
>> drag that whole argument up again. I don't find the argument in
>> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a
>> pathspec, 2017-03-17) convincing enough to have moved such a prominent
>> use-case to a longer option name.
> 
> I do agree that it is a separate topic to pick one or the other as
> the primary.  I think this change means that the users no longer
> need to stop after hitting a tab:
> 
>      clone --recurs<TAB>
> 
> and instead get to "clone --recurse-submodules" right away, which is
> a positigve change.  

This is indeed the goal, I'll update the commit message to make that
clearer.

Of course, if the separate topic swaps which is
> the canonical and which is the alias, the same user would get the
> new canonical one right away from the same state.
> 
> So it does look like a good change to me.
> 

Thanks,
Philippe.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] parse-options: don't complete option aliases by default
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
  2021-07-15 17:16   ` Felipe Contreras
  2021-07-15 18:57   ` Junio C Hamano
@ 2021-07-16  1:31   ` Philippe Blain
  2 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain @ 2021-07-16  1:31 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Philippe Blain via GitGitGadget
  Cc: git, Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor

Hi Ævar,

Le 2021-07-15 à 12:16, Ævar Arnfjörð Bjarmason a écrit :
> 
> 
> I'm a bit biased here since I like --recursive better, but let's not
> drag that whole argument up again. I don't find the argument in
> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a
> pathspec, 2017-03-17) convincing enough to have moved such a prominent
> use-case to a longer option name.
> 
> But, water under the bridge. Aside from that:
> 
> For me a Google search for "git clone --recursive" is ~40k results, but
> "git clone --recurse-submodules". The former links to various
> discussions/docs/stackoverflow answers, often --recurse-submodules isn't
> mentioned at all or as an aside.
> 
> I think it's unfortunate that we:
> 
>   1. Conflate whether something shows up in completion v.s. the
>      help. Given its prominence and that "git clone -h" is ~50 lines why
>      not note --recursive there.

As far as I understand (and tested), '--recursive' was listed in the short help but not the completion
before bb62e0a99f (clone: teach --recurse-submodules to optionally take a pathspec,
2017-03-17). At the time the completion did not use '--git-completion-helper'
and only '--recurse-submodules' was listed (since 5f072e0017 (completion: add option
'--recurse-submodules' to 'git clone', 2016-07-27)).

Starting from bb62e0a99f, it was not listed in the short help (because of PARSE_OPT_HIDDEN)
nor the completion (because it was still not listed).

Then starting from 5c387428f1 (parse-options: don't emit "ambiguous option"
for aliases, 2019-04-29), it started being listed in the short help AND
the completion again; in the short help because of the new code in usage_with_options_internal
and in the completion because of the way preprocess_options is implemented,
and at that time '--git-completion-helper' was used for '_git_clone'.

After my patch, it would still be listed in the short help, as "alias of --recurse-submodules",
but would not be listed by the completion (unless GIT_COMPLETION_SHOW_ALL is set).

> 
>   2. Don't have the completion aware of these aliases, i.e. "git clone
>      --rec<TAB>" before your chance offers a completion of both, that sucks,
>      we should fully complete the non-alias instead.

Indeed. That's my main motivation.

> 
>   3. Making it PARSE_OPT_HIDDEN "solves" #2 at the cost of hiding it in
>      "git help -h", and now this won't work, but did before:
> 
>          git clone --recursi<TAB>
> 
>      I.e. even if we didn't want to do #2 *and* wanted to hide it in the
>      usage output surely completing an unmbigous prefix is better, even
>      if it's a hidden option?

I agree that with my patch 'git clone --recursi<TAB>' does not complete to '--recursive'
(unless you've set GIT_COMPLETION_SHOW_ALL). But I'm not sure it's that big of
a deal (after all if you typed this far you only have two letters left :P)
But '--rec' will be sufficient to complete to '--recurse-submoduele', so
it's even less letters to type.

But this has nothing to do with PARSE_OPT_HIDDEN ('--recurse-submodules' is
not hidden and aliases are not hidden either). So I'm not sure what you mean...

> 
> Per-se none of this is a blocker or "we must fix this first" for this
> particular change, we have this in many existing cases.
> 
> I daresay there's no other alias that's in as wide a use in the wild, so
> we should think about this one particularly carefully though.
> 
> It's not fully clear from your commit message which of 1-3 you're aiming
> for, I think it's more of the "discourage its use".

As I wrote it's mainly #2, I'll update the commit message to be clearer.

> 
> Sure, fair enough, but PARSE_OPT_HIDDEN is not a 1=1 mapping to that,
> and can often lead to more user confusion.
> 
> E.g. the user has used --recursive for years, but can't even find it in
> -h (I also think it's a mistake to have entirely removed it from the
> docs, even if one agrees with its "deprecation" I'd say we should keep
> some "used to be called --recursive" note there).
> 

Yeah, maybe it should have stayed in the docs. Again, my patch does not remove
it from the short help.

Thanks for you comments!:)

Philippe.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2] parse-options: don't complete option aliases by default
  2021-07-15 12:58 [PATCH] parse-options: don't complete option aliases by default Philippe Blain via GitGitGadget
  2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
  2021-07-15 17:04 ` Felipe Contreras
@ 2021-07-16  1:55 ` Philippe Blain via GitGitGadget
  2 siblings, 0 replies; 9+ messages in thread
From: Philippe Blain via GitGitGadget @ 2021-07-16  1:55 UTC (permalink / raw)
  To: git
  Cc: Nguyễn Thái Ngọc Duy, Brandon Williams,
	Felipe Contreras, Ryan Zoeller, SZEDER Gábor,
	Ævar Arnfjörð Bjarmason, Philippe Blain,
	Philippe Blain

From: Philippe Blain <levraiphilippeblain@gmail.com>

Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit
"ambiguous option" for aliases, 2019-04-29), 'git clone
--git-completion-helper', which is used by the Bash completion script to
list options accepted by clone (via '__gitcomp_builtin'), lists both
'--recurse-submodules' and its alias '--recursive', which was not the
case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and
options with this flag are skipped by 'parse-options.c::show_gitcomp',
which implements 'git <cmd> --git-completion-helper'.

This means that typing 'git clone --recurs<TAB>' will yield both
'--recurse-submodules' and '--recursive', which is not ideal since both
do the same thing, and so the completion should directly complete the
canonical option.

At the point where 'show_gitcomp' is called in 'parse_options_step',
'preprocess_options' was already called in 'parse_options', so any
aliases are now copies of the original options with a modified help text
indicating they are aliases.

Helpfully, since 64cc539fd2 (parse-options: don't leak alias help
messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag
set, so check that flag early in 'show_gitcomp' and do not print them,
unless the user explicitely requested that *all* completion be shown (by
setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage
the use of '--recurse-submodules' over '--recursive', we'd better just
suggest the former.

The only other options alias is 'log' and friends' '--mailmap', which is
an alias for '--use-mailmap', but the Bash completion helpers for these
commands do not use '__gitcomp_builtin', and thus are unnaffected by
this change.

Test the new behaviour in t9902-completion.sh. As a side effect, this
also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was
not tested before. Note that since '__gitcomp_builtin' caches the
options it shows, we need to re-source the completion script to clear
that cache for the second test.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
    parse-options: don't complete option aliases by default
    
    Changes since v1:
    
     * Added a paragraph to the commit message to clarify that the goal of
       this patch is to have the completion show only the canonical option
       and not its alias.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/996

Range-diff vs v1:

 1:  091a59323b7 ! 1:  9ae43e17e0c parse-options: don't complete option aliases by default
     @@ Commit message
          options with this flag are skipped by 'parse-options.c::show_gitcomp',
          which implements 'git <cmd> --git-completion-helper'.
      
     +    This means that typing 'git clone --recurs<TAB>' will yield both
     +    '--recurse-submodules' and '--recursive', which is not ideal since both
     +    do the same thing, and so the completion should directly complete the
     +    canonical option.
     +
          At the point where 'show_gitcomp' is called in 'parse_options_step',
          'preprocess_options' was already called in 'parse_options', so any
          aliases are now copies of the original options with a modified help text


 parse-options.c       |  2 +-
 t/t9902-completion.sh | 13 +++++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/parse-options.c b/parse-options.c
index e6f56768ca5..2abff136a17 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)
 		if (!opts->long_name)
 			continue;
 		if (!show_all &&
-			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))
+			(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))
 			continue;
 
 		switch (opts->type) {
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index cb057ef1613..11573936d58 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '
 	verbose test -z "$__gitcomp_builtin_notes_edit"
 '
 
+test_expect_success 'option aliases are not shown by default' '
+	test_completion "git clone --recurs" "--recurse-submodules "
+'
+
+test_expect_success 'option aliases are shown with GIT_COMPLETION_SHOW_ALL' '
+	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
+	GIT_COMPLETION_SHOW_ALL=1 && export GIT_COMPLETION_SHOW_ALL &&
+	test_completion "git clone --recurs" <<-\EOF
+	--recurse-submodules Z
+	--recursive Z
+	EOF
+'
+
 test_expect_success '__git_complete' '
 	unset -f __git_wrap__git_main &&
 

base-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-07-16  1:55 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-15 12:58 [PATCH] parse-options: don't complete option aliases by default Philippe Blain via GitGitGadget
2021-07-15 16:16 ` Ævar Arnfjörð Bjarmason
2021-07-15 17:16   ` Felipe Contreras
2021-07-15 18:57   ` Junio C Hamano
2021-07-16  1:28     ` Philippe Blain
2021-07-16  1:31   ` Philippe Blain
2021-07-15 17:04 ` Felipe Contreras
2021-07-16  1:28   ` Philippe Blain
2021-07-16  1:55 ` [PATCH v2] " Philippe Blain via GitGitGadget

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.