| Message ID | pull.996.git.1626353925051.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | parse-options: don't complete option aliases by default | expand |
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).
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>
Æ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.
Æ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.
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.
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.
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.
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 &&