{"thread":{"id":"56111","subject":"[PATCH] parse-options: don't complete option aliases by default","startedAt":"2021-07-15T12:58:50Z","lastAt":"2021-07-16T01:55:51Z","messageCount":9,"participants":["Philippe Blain via GitGitGadget","Ævar Arnfjörð Bjarmason","Felipe Contreras","Junio C Hamano","Philippe Blain"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"430196","messageId":"pull.996.git.1626353925051.gitgitgadget@gmail.com","threadId":"56111","inReplyTo":null,"subject":"[PATCH] parse-options: don't complete option aliases by default","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-15T12:58:44Z","receivedAt":"2021-07-15T12:58:50Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit\n\"ambiguous option\" for aliases, 2019-04-29), 'git clone\n--git-completion-helper', which is used by the Bash completion script to\nlist options accepted by clone (via '__gitcomp_builtin'), lists both\n'--recurse-submodules' and its alias '--recursive', which was not the\ncase before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and\noptions with this flag are skipped by 'parse-options.c::show_gitcomp',\nwhich implements 'git <cmd> --git-completion-helper'.\n\nAt the point where 'show_gitcomp' is called in 'parse_options_step',\n'preprocess_options' was already called in 'parse_options', so any\naliases are now copies of the original options with a modified help text\nindicating they are aliases.\n\nHelpfully, since 64cc539fd2 (parse-options: don't leak alias help\nmessages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag\nset, so check that flag early in 'show_gitcomp' and do not print them,\nunless the user explicitely requested that *all* completion be shown (by\nsetting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage\nthe use of '--recurse-submodules' over '--recursive', we'd better just\nsuggest the former.\n\nThe only other options alias is 'log' and friends' '--mailmap', which is\nan alias for '--use-mailmap', but the Bash completion helpers for these\ncommands do not use '__gitcomp_builtin', and thus are unnaffected by\nthis change.\n\nTest the new behaviour in t9902-completion.sh. As a side effect, this\nalso tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was\nnot tested before. Note that since '__gitcomp_builtin' caches the\noptions it shows, we need to re-source the completion script to clear\nthat cache for the second test.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    parse-options: don't complete option aliases by default\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/996\n\n parse-options.c       |  2 +-\n t/t9902-completion.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e6f56768ca5..2abff136a17 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\tif (!opts->long_name)\n \t\t\tcontinue;\n \t\tif (!show_all &&\n-\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))\n+\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))\n \t\t\tcontinue;\n \n \t\tswitch (opts->type) {\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex cb057ef1613..11573936d58 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '\n \tverbose test -z \"$__gitcomp_builtin_notes_edit\"\n '\n \n+test_expect_success 'option aliases are not shown by default' '\n+\ttest_completion \"git clone --recurs\" \"--recurse-submodules \"\n+'\n+\n+test_expect_success 'option aliases are shown with GIT_COMPLETION_SHOW_ALL' '\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tGIT_COMPLETION_SHOW_ALL=1 && export GIT_COMPLETION_SHOW_ALL &&\n+\ttest_completion \"git clone --recurs\" <<-\\EOF\n+\t--recurse-submodules Z\n+\t--recursive Z\n+\tEOF\n+'\n+\n test_expect_success '__git_complete' '\n \tunset -f __git_wrap__git_main &&\n \n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\n-- \ngitgitgadget\n"},{"id":"430233","messageId":"8735sfzgkg.fsf@evledraar.gmail.com","threadId":"56111","inReplyTo":"pull.996.git.1626353925051.gitgitgadget@gmail.com","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-15T16:16:26Z","receivedAt":"2021-07-15T16:29:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jul 15 2021, Philippe Blain via GitGitGadget wrote:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit\n> \"ambiguous option\" for aliases, 2019-04-29), 'git clone\n> --git-completion-helper', which is used by the Bash completion script to\n> list options accepted by clone (via '__gitcomp_builtin'), lists both\n> '--recurse-submodules' and its alias '--recursive', which was not the\n> case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and\n> options with this flag are skipped by 'parse-options.c::show_gitcomp',\n> which implements 'git <cmd> --git-completion-helper'.\n>\n> At the point where 'show_gitcomp' is called in 'parse_options_step',\n> 'preprocess_options' was already called in 'parse_options', so any\n> aliases are now copies of the original options with a modified help text\n> indicating they are aliases.\n>\n> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help\n> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag\n> set, so check that flag early in 'show_gitcomp' and do not print them,\n> unless the user explicitely requested that *all* completion be shown (by\n> setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage\n> the use of '--recurse-submodules' over '--recursive', we'd better just\n> suggest the former.\n>\n> The only other options alias is 'log' and friends' '--mailmap', which is\n> an alias for '--use-mailmap', but the Bash completion helpers for these\n> commands do not use '__gitcomp_builtin', and thus are unnaffected by\n> this change.\n>\n> Test the new behaviour in t9902-completion.sh. As a side effect, this\n> also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was\n> not tested before. Note that since '__gitcomp_builtin' caches the\n> options it shows, we need to re-source the completion script to clear\n> that cache for the second test.\n>\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>     parse-options: don't complete option aliases by default\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/996\n>\n>  parse-options.c       |  2 +-\n>  t/t9902-completion.sh | 13 +++++++++++++\n>  2 files changed, 14 insertions(+), 1 deletion(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index e6f56768ca5..2abff136a17 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)\n>  \t\tif (!opts->long_name)\n>  \t\t\tcontinue;\n>  \t\tif (!show_all &&\n> -\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))\n> +\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))\n>  \t\t\tcontinue;\n>  \n>  \t\tswitch (opts->type) {\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index cb057ef1613..11573936d58 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '\n>  \tverbose test -z \"$__gitcomp_builtin_notes_edit\"\n>  '\n>  \n> +test_expect_success 'option aliases are not shown by default' '\n> +\ttest_completion \"git clone --recurs\" \"--recurse-submodules \"\n> +'\n\nI'm a bit biased here since I like --recursive better, but let's not\ndrag that whole argument up again. I don't find the argument in\nbb62e0a99fc (clone: teach --recurse-submodules to optionally take a\npathspec, 2017-03-17) convincing enough to have moved such a prominent\nuse-case to a longer option name.\n\nBut, water under the bridge. Aside from that:\n\nFor me a Google search for \"git clone --recursive\" is ~40k results, but\n\"git clone --recurse-submodules\". The former links to various\ndiscussions/docs/stackoverflow answers, often --recurse-submodules isn't\nmentioned at all or as an aside.\n\nI think it's unfortunate that we:\n\n 1. Conflate whether something shows up in completion v.s. the\n    help. Given its prominence and that \"git clone -h\" is ~50 lines why\n    not note --recursive there.\n\n 2. Don't have the completion aware of these aliases, i.e. \"git clone\n    --rec<TAB>\" before your chance offers a completion of both, that sucks,\n    we should fully complete the non-alias instead.\n\n 3. Making it PARSE_OPT_HIDDEN \"solves\" #2 at the cost of hiding it in\n    \"git help -h\", and now this won't work, but did before:\n\n        git clone --recursi<TAB>\n\n    I.e. even if we didn't want to do #2 *and* wanted to hide it in the\n    usage output surely completing an unmbigous prefix is better, even\n    if it's a hidden option?\n\nPer-se none of this is a blocker or \"we must fix this first\" for this\nparticular change, we have this in many existing cases.\n\nI daresay there's no other alias that's in as wide a use in the wild, so\nwe should think about this one particularly carefully though.\n\nIt's not fully clear from your commit message which of 1-3 you're aiming\nfor, I think it's more of the \"discourage its use\". \n\nSure, fair enough, but PARSE_OPT_HIDDEN is not a 1=1 mapping to that,\nand can often lead to more user confusion.\n\nE.g. the user has used --recursive for years, but can't even find it in\n-h (I also think it's a mistake to have entirely removed it from the\ndocs, even if one agrees with its \"deprecation\" I'd say we should keep\nsome \"used to be called --recursive\" note there).\n\n"},{"id":"430240","messageId":"60f06a8a8e611_4b68208a9@natae.notmuch","threadId":"56111","inReplyTo":"pull.996.git.1626353925051.gitgitgadget@gmail.com","subject":"RE: [PATCH] parse-options: don't complete option aliases by default","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T17:04:10Z","receivedAt":"2021-07-15T17:04:14Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Philippe Blain via GitGitGadget wrote:\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n> \n> Since 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit\n> \"ambiguous option\" for aliases, 2019-04-29), 'git clone\n> --git-completion-helper', which is used by the Bash completion script to\n> list options accepted by clone (via '__gitcomp_builtin'), lists both\n> '--recurse-submodules' and its alias '--recursive', which was not the\n> case before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and\n> options with this flag are skipped by 'parse-options.c::show_gitcomp',\n> which implements 'git <cmd> --git-completion-helper'.\n> \n> At the point where 'show_gitcomp' is called in 'parse_options_step',\n> 'preprocess_options' was already called in 'parse_options', so any\n> aliases are now copies of the original options with a modified help text\n> indicating they are aliases.\n> \n> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help\n> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag\n> set, so check that flag early in 'show_gitcomp' and do not print them,\n\nMakes sense but I don't think what the patch is doing should be buried\nhere. I think a separate paragraph explaining what you are trying to do\nwill be better.\n\n> unless the user explicitely requested that *all* completion be shown (by\n> setting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage\n> the use of '--recurse-submodules' over '--recursive', we'd better just\n> suggest the former.\n> \n> The only other options alias is 'log' and friends' '--mailmap', which is\n> an alias for '--use-mailmap', but the Bash completion helpers for these\n> commands do not use '__gitcomp_builtin', and thus are unnaffected by\n> this change.\n> \n> Test the new behaviour in t9902-completion.sh. As a side effect, this\n> also tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was\n> not tested before. Note that since '__gitcomp_builtin' caches the\n> options it shows, we need to re-source the completion script to clear\n> that cache for the second test.\n\nI agree this is better, and the patch itself looks obviously correct.\n\nReviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n\n-- \nFelipe Contreras\n"},{"id":"430243","messageId":"60f06d62868db_4b6820816@natae.notmuch","threadId":"56111","inReplyTo":"8735sfzgkg.fsf@evledraar.gmail.com","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-15T17:16:18Z","receivedAt":"2021-07-15T17:16:22Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n> On Thu, Jul 15 2021, Philippe Blain via GitGitGadget wrote:\n\n> I'm a bit biased here since I like --recursive better, but let's not\n> drag that whole argument up again. I don't find the argument in\n> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a\n> pathspec, 2017-03-17) convincing enough to have moved such a prominent\n> use-case to a longer option name.\n\nI agree.\n\n> But, water under the bridge. Aside from that:\n> \n> For me a Google search for \"git clone --recursive\" is ~40k results, but\n> \"git clone --recurse-submodules\". The former links to various\n> discussions/docs/stackoverflow answers, often --recurse-submodules isn't\n> mentioned at all or as an aside.\n\nIt would be nice if facts could be used as evidence of a UI mistake, but\nalas in my experience that has never been the case.\n\n> I think it's unfortunate that we:\n> \n>  1. Conflate whether something shows up in completion v.s. the\n>     help. Given its prominence and that \"git clone -h\" is ~50 lines why\n>     not note --recursive there.\n\nAgreed.\n\n>  2. Don't have the completion aware of these aliases, i.e. \"git clone\n>     --rec<TAB>\" before your chance offers a completion of both, that sucks,\n>     we should fully complete the non-alias instead.\n\nYes, that's what would happen with the patch.\n\n>  3. Making it PARSE_OPT_HIDDEN \"solves\" #2 at the cost of hiding it in\n>     \"git help -h\", and now this won't work, but did before:\n> \n>         git clone --recursi<TAB>\n> \n>     I.e. even if we didn't want to do #2 *and* wanted to hide it in the\n>     usage output surely completing an unmbigous prefix is better, even\n>     if it's a hidden option?\n\nThis is something that could be done in zsh, but not in bash (at least\nnot easily).\n\n> E.g. the user has used --recursive for years, but can't even find it in\n> -h (I also think it's a mistake to have entirely removed it from the\n> docs, even if one agrees with its \"deprecation\" I'd say we should keep\n> some \"used to be called --recursive\" note there).\n\nBut that is not a problem of this patch. If users can't find --recursive\nand complain about it, then that's actually a good thing, because now\nthe facts about --recursive vs. --recurse-submodules are not needed\nanymore, and we could just fix the interface.\n\n-- \nFelipe Contreras"},{"id":"430259","messageId":"xmqqy2a7s8vh.fsf@gitster.g","threadId":"56111","inReplyTo":"8735sfzgkg.fsf@evledraar.gmail.com","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-15T18:57:22Z","receivedAt":"2021-07-15T18:58:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> I'm a bit biased here since I like --recursive better, but let's not\n> drag that whole argument up again. I don't find the argument in\n> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a\n> pathspec, 2017-03-17) convincing enough to have moved such a prominent\n> use-case to a longer option name.\n\nI do agree that it is a separate topic to pick one or the other as\nthe primary.  I think this change means that the users no longer\nneed to stop after hitting a tab:\n\n    clone --recurs<TAB>\n\nand instead get to \"clone --recurse-submodules\" right away, which is\na positigve change.  Of course, if the separate topic swaps which is\nthe canonical and which is the alias, the same user would get the\nnew canonical one right away from the same state.\n\nSo it does look like a good change to me.\n"},{"id":"430287","messageId":"c8322c37-38da-bde7-9361-adb68834b7d1@gmail.com","threadId":"56111","inReplyTo":"60f06a8a8e611_4b68208a9@natae.notmuch","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-07-16T01:28:00Z","receivedAt":"2021-07-16T01:28:14Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Felipe,\n\nLe 2021-07-15 à 13:04, Felipe Contreras a écrit :\n> Philippe Blain via GitGitGadget wrote:\n>> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>>\n>>\n>> Helpfully, since 64cc539fd2 (parse-options: don't leak alias help\n>> messages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag\n>> set, so check that flag early in 'show_gitcomp' and do not print them,\n> \n> Makes sense but I don't think what the patch is doing should be buried\n> here. I think a separate paragraph explaining what you are trying to do\n> will be better.\n\nIndeed. Will clarify.\n\n\n> \n> I agree this is better, and the patch itself looks obviously correct.\n> \n> Reviewed-by: Felipe Contreras <felipe.contreras@gmail.com>\n> \n\nThanks!\n\nPhilippe.\n"},{"id":"430288","messageId":"f85969ca-48e0-b2f1-6a42-7e4cc41a3639@gmail.com","threadId":"56111","inReplyTo":"xmqqy2a7s8vh.fsf@gitster.g","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-07-16T01:28:16Z","receivedAt":"2021-07-16T01:28:24Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Junio,\n\nLe 2021-07-15 à 14:57, Junio C Hamano a écrit :\n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n>> I'm a bit biased here since I like --recursive better, but let's not\n>> drag that whole argument up again. I don't find the argument in\n>> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a\n>> pathspec, 2017-03-17) convincing enough to have moved such a prominent\n>> use-case to a longer option name.\n> \n> I do agree that it is a separate topic to pick one or the other as\n> the primary.  I think this change means that the users no longer\n> need to stop after hitting a tab:\n> \n>      clone --recurs<TAB>\n> \n> and instead get to \"clone --recurse-submodules\" right away, which is\n> a positigve change.  \n\nThis is indeed the goal, I'll update the commit message to make that\nclearer.\n\nOf course, if the separate topic swaps which is\n> the canonical and which is the alias, the same user would get the\n> new canonical one right away from the same state.\n> \n> So it does look like a good change to me.\n> \n\nThanks,\nPhilippe.\n"},{"id":"430289","messageId":"1f41f94a-ebe1-69f0-7631-864cbbd1e1a0@gmail.com","threadId":"56111","inReplyTo":"8735sfzgkg.fsf@evledraar.gmail.com","subject":"Re: [PATCH] parse-options: don't complete option aliases by default","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2021-07-16T01:31:45Z","receivedAt":"2021-07-16T01:31:50Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Ævar,\n\nLe 2021-07-15 à 12:16, Ævar Arnfjörð Bjarmason a écrit :\n> \n> \n> I'm a bit biased here since I like --recursive better, but let's not\n> drag that whole argument up again. I don't find the argument in\n> bb62e0a99fc (clone: teach --recurse-submodules to optionally take a\n> pathspec, 2017-03-17) convincing enough to have moved such a prominent\n> use-case to a longer option name.\n> \n> But, water under the bridge. Aside from that:\n> \n> For me a Google search for \"git clone --recursive\" is ~40k results, but\n> \"git clone --recurse-submodules\". The former links to various\n> discussions/docs/stackoverflow answers, often --recurse-submodules isn't\n> mentioned at all or as an aside.\n> \n> I think it's unfortunate that we:\n> \n>   1. Conflate whether something shows up in completion v.s. the\n>      help. Given its prominence and that \"git clone -h\" is ~50 lines why\n>      not note --recursive there.\n\nAs far as I understand (and tested), '--recursive' was listed in the short help but not the completion\nbefore bb62e0a99f (clone: teach --recurse-submodules to optionally take a pathspec,\n2017-03-17). At the time the completion did not use '--git-completion-helper'\nand only '--recurse-submodules' was listed (since 5f072e0017 (completion: add option\n'--recurse-submodules' to 'git clone', 2016-07-27)).\n\nStarting from bb62e0a99f, it was not listed in the short help (because of PARSE_OPT_HIDDEN)\nnor the completion (because it was still not listed).\n\nThen starting from 5c387428f1 (parse-options: don't emit \"ambiguous option\"\nfor aliases, 2019-04-29), it started being listed in the short help AND\nthe completion again; in the short help because of the new code in usage_with_options_internal\nand in the completion because of the way preprocess_options is implemented,\nand at that time '--git-completion-helper' was used for '_git_clone'.\n\nAfter my patch, it would still be listed in the short help, as \"alias of --recurse-submodules\",\nbut would not be listed by the completion (unless GIT_COMPLETION_SHOW_ALL is set).\n\n> \n>   2. Don't have the completion aware of these aliases, i.e. \"git clone\n>      --rec<TAB>\" before your chance offers a completion of both, that sucks,\n>      we should fully complete the non-alias instead.\n\nIndeed. That's my main motivation.\n\n> \n>   3. Making it PARSE_OPT_HIDDEN \"solves\" #2 at the cost of hiding it in\n>      \"git help -h\", and now this won't work, but did before:\n> \n>          git clone --recursi<TAB>\n> \n>      I.e. even if we didn't want to do #2 *and* wanted to hide it in the\n>      usage output surely completing an unmbigous prefix is better, even\n>      if it's a hidden option?\n\nI agree that with my patch 'git clone --recursi<TAB>' does not complete to '--recursive'\n(unless you've set GIT_COMPLETION_SHOW_ALL). But I'm not sure it's that big of\na deal (after all if you typed this far you only have two letters left :P)\nBut '--rec' will be sufficient to complete to '--recurse-submoduele', so\nit's even less letters to type.\n\nBut this has nothing to do with PARSE_OPT_HIDDEN ('--recurse-submodules' is\nnot hidden and aliases are not hidden either). So I'm not sure what you mean...\n\n> \n> Per-se none of this is a blocker or \"we must fix this first\" for this\n> particular change, we have this in many existing cases.\n> \n> I daresay there's no other alias that's in as wide a use in the wild, so\n> we should think about this one particularly carefully though.\n> \n> It's not fully clear from your commit message which of 1-3 you're aiming\n> for, I think it's more of the \"discourage its use\".\n\nAs I wrote it's mainly #2, I'll update the commit message to be clearer.\n\n> \n> Sure, fair enough, but PARSE_OPT_HIDDEN is not a 1=1 mapping to that,\n> and can often lead to more user confusion.\n> \n> E.g. the user has used --recursive for years, but can't even find it in\n> -h (I also think it's a mistake to have entirely removed it from the\n> docs, even if one agrees with its \"deprecation\" I'd say we should keep\n> some \"used to be called --recursive\" note there).\n> \n\nYeah, maybe it should have stayed in the docs. Again, my patch does not remove\nit from the short help.\n\nThanks for you comments!:)\n\nPhilippe.\n"},{"id":"430291","messageId":"pull.996.v2.git.1626400544014.gitgitgadget@gmail.com","threadId":"56111","inReplyTo":"pull.996.git.1626353925051.gitgitgadget@gmail.com","subject":"[PATCH v2] parse-options: don't complete option aliases by default","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-16T01:55:43Z","receivedAt":"2021-07-16T01:55:51Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince 'OPT_ALIAS' was created in 5c387428f1 (parse-options: don't emit\n\"ambiguous option\" for aliases, 2019-04-29), 'git clone\n--git-completion-helper', which is used by the Bash completion script to\nlist options accepted by clone (via '__gitcomp_builtin'), lists both\n'--recurse-submodules' and its alias '--recursive', which was not the\ncase before since '--recursive' had the PARSE_OPT_HIDDEN flag set, and\noptions with this flag are skipped by 'parse-options.c::show_gitcomp',\nwhich implements 'git <cmd> --git-completion-helper'.\n\nThis means that typing 'git clone --recurs<TAB>' will yield both\n'--recurse-submodules' and '--recursive', which is not ideal since both\ndo the same thing, and so the completion should directly complete the\ncanonical option.\n\nAt the point where 'show_gitcomp' is called in 'parse_options_step',\n'preprocess_options' was already called in 'parse_options', so any\naliases are now copies of the original options with a modified help text\nindicating they are aliases.\n\nHelpfully, since 64cc539fd2 (parse-options: don't leak alias help\nmessages, 2021-03-21) these copies have the PARSE_OPT_FROM_ALIAS flag\nset, so check that flag early in 'show_gitcomp' and do not print them,\nunless the user explicitely requested that *all* completion be shown (by\nsetting 'GIT_COMPLETION_SHOW_ALL'). After all, if we want to encourage\nthe use of '--recurse-submodules' over '--recursive', we'd better just\nsuggest the former.\n\nThe only other options alias is 'log' and friends' '--mailmap', which is\nan alias for '--use-mailmap', but the Bash completion helpers for these\ncommands do not use '__gitcomp_builtin', and thus are unnaffected by\nthis change.\n\nTest the new behaviour in t9902-completion.sh. As a side effect, this\nalso tests the correct behaviour of GIT_COMPLETION_SHOW_ALL, which was\nnot tested before. Note that since '__gitcomp_builtin' caches the\noptions it shows, we need to re-source the completion script to clear\nthat cache for the second test.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n    parse-options: don't complete option aliases by default\n    \n    Changes since v1:\n    \n     * Added a paragraph to the commit message to clarify that the goal of\n       this patch is to have the completion show only the canonical option\n       and not its alias.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-996%2Fphil-blain%2Fclone-recurse-completion-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-996/phil-blain/clone-recurse-completion-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/996\n\nRange-diff vs v1:\n\n 1:  091a59323b7 ! 1:  9ae43e17e0c parse-options: don't complete option aliases by default\n     @@ Commit message\n          options with this flag are skipped by 'parse-options.c::show_gitcomp',\n          which implements 'git <cmd> --git-completion-helper'.\n      \n     +    This means that typing 'git clone --recurs<TAB>' will yield both\n     +    '--recurse-submodules' and '--recursive', which is not ideal since both\n     +    do the same thing, and so the completion should directly complete the\n     +    canonical option.\n     +\n          At the point where 'show_gitcomp' is called in 'parse_options_step',\n          'preprocess_options' was already called in 'parse_options', so any\n          aliases are now copies of the original options with a modified help text\n\n\n parse-options.c       |  2 +-\n t/t9902-completion.sh | 13 +++++++++++++\n 2 files changed, 14 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex e6f56768ca5..2abff136a17 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -585,7 +585,7 @@ static int show_gitcomp(const struct option *opts, int show_all)\n \t\tif (!opts->long_name)\n \t\t\tcontinue;\n \t\tif (!show_all &&\n-\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE)))\n+\t\t\t(opts->flags & (PARSE_OPT_HIDDEN | PARSE_OPT_NOCOMPLETE | PARSE_OPT_FROM_ALIAS)))\n \t\t\tcontinue;\n \n \t\tswitch (opts->type) {\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex cb057ef1613..11573936d58 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2404,6 +2404,19 @@ test_expect_success 'sourcing the completion script clears cached --options' '\n \tverbose test -z \"$__gitcomp_builtin_notes_edit\"\n '\n \n+test_expect_success 'option aliases are not shown by default' '\n+\ttest_completion \"git clone --recurs\" \"--recurse-submodules \"\n+'\n+\n+test_expect_success 'option aliases are shown with GIT_COMPLETION_SHOW_ALL' '\n+\t. \"$GIT_BUILD_DIR/contrib/completion/git-completion.bash\" &&\n+\tGIT_COMPLETION_SHOW_ALL=1 && export GIT_COMPLETION_SHOW_ALL &&\n+\ttest_completion \"git clone --recurs\" <<-\\EOF\n+\t--recurse-submodules Z\n+\t--recursive Z\n+\tEOF\n+'\n+\n test_expect_success '__git_complete' '\n \tunset -f __git_wrap__git_main &&\n \n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\n-- \ngitgitgadget\n"}]}