{"thread":{"id":"51259","subject":"[PATCH] test: completion: tests for __gitcomp regression","startedAt":"2019-06-07T02:24:49Z","lastAt":"2019-06-16T01:32:51Z","messageCount":12,"participants":["Felipe Contreras","Nguyễn Thái Ngọc Duy","SZEDER Gábor","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"376808","messageId":"20190607022443.5706-1-felipe.contreras@gmail.com","threadId":"51259","inReplyTo":null,"subject":"[PATCH] test: completion: tests for __gitcomp regression","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-07T02:24:43Z","receivedAt":"2019-06-07T02:24:49Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"There's a regression in the completion since the introduction of\n__gitcomp.\n\nGo to any directory that doesn't contain a git repository, like /tmp.\nThen type the following:\n\n git checkout --\n\nYou will see nothing. That's because\n`git checkout --git-completion-helper` fails when you run it outside a\ngit repository.\n\nYou might change to a directory that has a git repository, but it's too\nlate, because the empty options have been cached.\n\nIt's unclear how many commands are affected, but this patch attempts to\nat least detect some already in the testing framework.\n\nSigned-off-by: Felipe Contreras \n---\n t/t9902-completion.sh | 33 ++++++++++++++++++++++++++-------\n 1 file changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 43cf313a1c..1319c542e1 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -122,6 +122,15 @@ test_gitcomp_nl ()\n \ttest_cmp expected out\n }\n \n+offgit ()\n+{\n+\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n+\texport GIT_CEILING_DIRECTORIES &&\n+\ttest_when_finished \"ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES\" &&\n+\tROOT=\"$ROOT\"/non-repo &&\n+\tcd \"$ROOT\"\n+}\n+\n invalid_variable_name='${foo.bar}'\n \n actual=\"$TRASH_DIRECTORY/actual\"\n@@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli\n '\n \n test_expect_success '__git_find_repo_path - not a git repository' '\n+\toffgit &&\n \t(\n-\t\tcd non-repo &&\n-\t\tGIT_CEILING_DIRECTORIES=\"$ROOT\" &&\n-\t\texport GIT_CEILING_DIRECTORIES &&\n \t\ttest_must_fail __git_find_repo_path &&\n \t\tprintf \"$__git_repo_path\" >\"$actual\"\n \t) &&\n@@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' '\n '\n \n test_expect_success 'basic' '\n+\toffgit &&\n \trun_completion \"git \" &&\n \t# built-in\n \tgrep -q \"^add \\$\" out &&\n@@ -1401,6 +1409,7 @@ test_expect_success 'basic' '\n '\n \n test_expect_success 'double dash \"git\" itself' '\n+\toffgit &&\n \ttest_completion \"git --\" <<-\\EOF\n \t--paginate Z\n \t--no-pager Z\n@@ -1419,7 +1428,8 @@ test_expect_success 'double dash \"git\" itself' '\n \tEOF\n '\n \n-test_expect_success 'double dash \"git checkout\"' '\n+test_expect_failure 'double dash \"git checkout\"' '\n+\toffgit &&\n \ttest_completion \"git checkout --\" <<-\\EOF\n \t--quiet Z\n \t--detach Z\n@@ -1442,6 +1452,7 @@ test_expect_success 'double dash \"git checkout\"' '\n '\n \n test_expect_success 'general options' '\n+\toffgit &&\n \ttest_completion \"git --ver\" \"--version \" &&\n \ttest_completion \"git --hel\" \"--help \" &&\n \ttest_completion \"git --exe\" <<-\\EOF &&\n@@ -1460,6 +1471,7 @@ test_expect_success 'general options' '\n '\n \n test_expect_success 'general options plus command' '\n+\toffgit &&\n \ttest_completion \"git --version check\" \"checkout \" &&\n \ttest_completion \"git --paginate check\" \"checkout \" &&\n \ttest_completion \"git --git-dir=foo check\" \"checkout \" &&\n@@ -1480,11 +1492,13 @@ test_expect_success 'general options plus command' '\n '\n \n test_expect_success 'git --help completion' '\n+\toffgit &&\n \ttest_completion \"git --help ad\" \"add \" &&\n \ttest_completion \"git --help core\" \"core-tutorial \"\n '\n \n-test_expect_success 'completion.commands removes multiple commands' '\n+test_expect_failure 'completion.commands removes multiple commands' '\n+\toffgit &&\n \ttest_config completion.commands \"-cherry -mergetool\" &&\n \tgit --list-cmds=list-mainporcelain,list-complete,config >out &&\n \t! grep -E \"^(cherry|mergetool)$\" out\n@@ -1547,7 +1561,8 @@ test_expect_success 'complete tree filename with metacharacters' '\n \tEOF\n '\n \n-test_expect_success PERL 'send-email' '\n+test_expect_failure PERL 'send-email' '\n+\toffgit &&\n \ttest_completion \"git send-email --cov\" \"--cover-letter \" &&\n \ttest_completion \"git send-email ma\" \"master \"\n '\n@@ -1649,6 +1664,7 @@ test_expect_success 'completion used completion for alias: !f() { : git --git-completion-helper\" could fail if the command checks for\na repo before parse_options(). If the result is cached, later on when\nthe user moves to a worktree with repo, tab completion will still fail.\n\nAvoid this by detecting errors and not cache the completion output. We\ncan try again and hopefully succeed next time (e.g. when a repo is\nfound).\n\nOf course if --git-completion-helper fails permanently because of other\nreasons (*), this will slow down completion. But I don't see any better\noption to handle that case.\n\n(*) one of those cases is if __gitcomp_builtin is called on a command\n that does not support --git-completion-helper. And we do have a\n generic call\n\n __git_complete_common \"$command\"\n\n but this case is protected with __git_support_parseopt_helper so we're\n good.\n\nReported-by: Felipe Contreras \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n contrib/completion/git-completion.bash | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9f71bcde96..a515de8501 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -398,13 +398,15 @@ __gitcomp_builtin ()\n \teval \"options=\\$$var\"\n \n \tif [ -z \"$options\" ]; then\n+\t\tlocal nocache=\n \t\t# leading and trailing spaces are significant to make\n \t\t# option removal work correctly.\n-\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n+\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \" || nocache=t\n+\n \t\tfor i in $excl; do\n \t\t\toptions=\"${options/ $i / }\"\n \t\tdone\n-\t\teval \"$var=\\\"$options\\\"\"\n+\t\ttest -n \"$nocache\" || eval \"$var=\\\"$options\\\"\"\n \tfi\n \n \t__gitcomp \"$options\"\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"376815","messageId":"20190607100252.GA24208@szeder.dev","threadId":"51259","inReplyTo":"20190607093034.816-1-pclouds@gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-07T10:02:52Z","receivedAt":"2019-06-07T10:03:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> \"git --git-completion-helper\" could fail if the command checks for\n> a repo before parse_options(). If the result is cached, later on when\n> the user moves to a worktree with repo, tab completion will still fail.\n> \n> Avoid this by detecting errors and not cache the completion output. We\n> can try again and hopefully succeed next time (e.g. when a repo is\n> found).\n> \n> Of course if --git-completion-helper fails permanently because of other\n> reasons (*), this will slow down completion. But I don't see any better\n> option to handle that case.\n\nI think a permanently failing 'git cmd --git-completion-helper'\nshouldn't really happen, unless there is a bug in the completion\nscript or the git installation or similar exceptional situation. And\nthen that issue should be fixed, but I don't think we should worry\nabout an extra subshell and git process in those situations.\n\n> (*) one of those cases is if __gitcomp_builtin is called on a command\n> that does not support --git-completion-helper. And we do have a\n> generic call\n> \n> __git_complete_common \"$command\"\n> \n> but this case is protected with __git_support_parseopt_helper so we're\n> good.\n> \n> Reported-by: Felipe Contreras \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> contrib/completion/git-completion.bash | 6 ++++--\n> 1 file changed, 4 insertions(+), 2 deletions(-)\n> \n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 9f71bcde96..a515de8501 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -398,13 +398,15 @@ __gitcomp_builtin ()\n> \teval \"options=\\$$var\"\n> \n> \tif [ -z \"$options\" ]; then\n> +\t\tlocal nocache=\n> \t\t# leading and trailing spaces are significant to make\n> \t\t# option removal work correctly.\n> -\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n> +\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \" || nocache=t\n\nWhy not just 'return' here?\n\nIf we return, then it won't list any options, and the shell should fall\nback to regular file completion. With this patch it will still list\nextra options, if there are any, and I don't really see the point in\ndoing that.\n\n\n> +\n> \t\tfor i in $excl; do\n> \t\t\toptions=\"${options/ $i / }\"\n> \t\tdone\n> -\t\teval \"$var=\\\"$options\\\"\"\n> +\t\ttest -n \"$nocache\" || eval \"$var=\\\"$options\\\"\"\n> \tfi\n> \n> \t__gitcomp \"$options\"\n> -- \n> 2.22.0.rc0.322.g2b0371e29a\n> \n"},{"id":"376823","messageId":"xmqqzhmtpd44.fsf@gitster-ct.c.googlers.com","threadId":"51259","inReplyTo":"20190607093034.816-1-pclouds@gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-07T16:35:39Z","receivedAt":"2019-06-07T16:35:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> \tif [ -z \"$options\" ]; then\n> +\t\tlocal nocache=\n> \t\t# leading and trailing spaces are significant to make\n> \t\t# option removal work correctly.\n> -\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n> +\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \" || nocache=t\n> +\n> \t\tfor i in $excl; do\n> \t\t\toptions=\"${options/ $i / }\"\n> \t\tdone\n\nIs there a point in doing this loop if we are not going to eval\nafter all? IOW...\n\n> -\t\teval \"$var=\\\"$options\\\"\"\n> +\t\ttest -n \"$nocache\" || eval \"$var=\\\"$options\\\"\"\n> \tfi\n\n... I am wondering why it is not more like this\n\n\tif options=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n\tthen\n\t\tfor i in $excl\n\t\tdo\n\t\t\toptions=...\n\t\tdone\n\t\teval \"$var=...\"\n\tfi\n\nor just return from the function instead of introducing and setting\na new variable, as only remaining thing the function does is to call\n__gitcomp with $options, but we know that we are giving up on\ncompleting this round.\n\n"},{"id":"376826","messageId":"CAMP44s3EsNzBtt_pG8HVp_RTMTTArk7Twhty4_tzf2iiZ7TKUQ@mail.gmail.com","threadId":"51259","inReplyTo":"20190607100252.GA24208@szeder.dev","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-07T17:33:12Z","receivedAt":"2019-06-07T17:33:26Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor wrote:\n>\n> On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> > \"git --git-completion-helper\" could fail if the command checks for\n> > a repo before parse_options(). If the result is cached, later on when\n> > the user moves to a worktree with repo, tab completion will still fail.\n> >\n> > Avoid this by detecting errors and not cache the completion output. We\n> > can try again and hopefully succeed next time (e.g. when a repo is\n> > found).\n> >\n> > Of course if --git-completion-helper fails permanently because of other\n> > reasons (*), this will slow down completion. But I don't see any better\n> > option to handle that case.\n>\n> I think a permanently failing 'git cmd --git-completion-helper'\n> shouldn't really happen, unless there is a bug in the completion\n> script or the git installation or similar exceptional situation. And\n> then that issue should be fixed, but I don't think we should worry\n> about an extra subshell and git process in those situations.\n\nIndeed. In think there's only sane option to make this work in all\nsituation; a reorganization.\n\nSomething like this should work:\n\nstruct command checkout_command = {\n.name = \"checkout\",\n.function = cmd_checkout,\n.run_options = RUN_SETUP | NEED_WORK_TREE,\n.help = N_(\"Switch branches or restore working tree files\"),\n.options = {\nOPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n...\n},\n}\n\nThis way we could run parse_options_show_gitcomp() from git.c and not\nworry about whatever cmd_checkout() needs.\n\nThis has the added advantage that it gathers information about this\ncommand that is stray in multiple sources (git.c, command-list.h), and\nit makes builtin.h cleaner too.\n\nPlus, we could rework the way -h works too.\n\n-- \nFelipe Contreras\n"},{"id":"377050","messageId":"CACsJy8CGtZMn0aa7QRj7Dz-jZS2K2fviUJ_Lii48FRY4E84i5Q@mail.gmail.com","threadId":"51259","inReplyTo":"CAMP44s3EsNzBtt_pG8HVp_RTMTTArk7Twhty4_tzf2iiZ7TKUQ@mail.gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-12T08:52:19Z","receivedAt":"2019-06-12T08:52:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras\n wrote:\n>\n> On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor wrote:\n> >\n> > On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:\n> > > \"git --git-completion-helper\" could fail if the command checks for\n> > > a repo before parse_options(). If the result is cached, later on when\n> > > the user moves to a worktree with repo, tab completion will still fail.\n> > >\n> > > Avoid this by detecting errors and not cache the completion output. We\n> > > can try again and hopefully succeed next time (e.g. when a repo is\n> > > found).\n> > >\n> > > Of course if --git-completion-helper fails permanently because of other\n> > > reasons (*), this will slow down completion. But I don't see any better\n> > > option to handle that case.\n> >\n> > I think a permanently failing 'git cmd --git-completion-helper'\n> > shouldn't really happen, unless there is a bug in the completion\n> > script or the git installation or similar exceptional situation. And\n> > then that issue should be fixed, but I don't think we should worry\n> > about an extra subshell and git process in those situations.\n>\n> Indeed. In think there's only sane option to make this work in all\n> situation; a reorganization.\n>\n> Something like this should work:\n>\n> struct command checkout_command = {\n> .name = \"checkout\",\n> .function = cmd_checkout,\n> .run_options = RUN_SETUP | NEED_WORK_TREE,\n> .help = N_(\"Switch branches or restore working tree files\"),\n> .options = {\n> OPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n> ...\n> },\n> }\n>\n> This way we could run parse_options_show_gitcomp() from git.c and not\n> worry about whatever cmd_checkout() needs.\n\nThis only works for a few commands. Those with subcommands already\nhave struct option[] array scattered in different places. And some new\nones also have struct option array dynamically created.\n\nIt's not impossible to do. But I feel there's a lot of reorganizing\nfor little gain. Maybe when we pass 'struct repository *' to all\ncommands, which means we hit all commmands at once anyway, we can\nreconsider this (and having config parser in a more declarative form\nlike cmd option parser).\n\n> This has the added advantage that it gathers information about this\n> command that is stray in multiple sources (git.c, command-list.h), and\n> it makes builtin.h cleaner too.\n>\n> Plus, we could rework the way -h works too.\n-- \nDuy\n"},{"id":"377051","messageId":"20190612085606.12144-1-pclouds@gmail.com","threadId":"51259","inReplyTo":"20190607093034.816-1-pclouds@gmail.com","subject":"[PATCH v2] completion: do not cache if --git-completion-helper fails","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-12T08:56:06Z","receivedAt":"2019-06-12T08:56:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"git --git-completion-helper\" could fail if the command checks for\na repo before parse_options(). If the result is cached, later on when\nthe user moves to a worktree with repo, tab completion will still fail.\n\nAvoid this by detecting errors and not cache the completion output. We\ncan try again and hopefully succeed next time (e.g. when a repo is\nfound).\n\nOf course if --git-completion-helper fails permanently because of other\nreasons (*), this will slow down completion. But I don't see any better\noption to handle that case.\n\n(*) one of those cases is if __gitcomp_builtin is called on a command\n that does not support --git-completion-helper. And we do have a\n generic call\n\n __git_complete_common \"$command\"\n\n but this case is protected with __git_support_parseopt_helper so we're\n good.\n\nReported-by: Felipe Contreras \nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n v2 simplifies the code. $nocache in v1 was overkill.\n\n contrib/completion/git-completion.bash | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 9f71bcde96..8c6b610a24 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -400,7 +400,8 @@ __gitcomp_builtin ()\n \tif [ -z \"$options\" ]; then\n \t\t# leading and trailing spaces are significant to make\n \t\t# option removal work correctly.\n-\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n+\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \" || return\n+\n \t\tfor i in $excl; do\n \t\t\toptions=\"${options/ $i / }\"\n \t\tdone\n-- \n2.22.0.rc0.322.g2b0371e29a\n\n"},{"id":"377092","messageId":"xmqqa7emk8oj.fsf@gitster-ct.c.googlers.com","threadId":"51259","inReplyTo":"20190612085606.12144-1-pclouds@gmail.com","subject":"Re: [PATCH v2] completion: do not cache if --git-completion-helper fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-12T17:36:12Z","receivedAt":"2019-06-12T17:36:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> \"git --git-completion-helper\" could fail if the command checks for\n> a repo before parse_options(). If the result is cached, later on when\n> the user moves to a worktree with repo, tab completion will still fail.\n>\n> Avoid this by detecting errors and not cache the completion output. We\n> can try again and hopefully succeed next time (e.g. when a repo is\n> found).\n>\n> Of course if --git-completion-helper fails permanently because of other\n> reasons (*), this will slow down completion. But I don't see any better\n> option to handle that case.\n>\n> (*) one of those cases is if __gitcomp_builtin is called on a command\n> that does not support --git-completion-helper. And we do have a\n> generic call\n>\n> __git_complete_common \"$command\"\n>\n> but this case is protected with __git_support_parseopt_helper so we're\n> good.\n>\n> Reported-by: Felipe Contreras \n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> v2 simplifies the code. $nocache in v1 was overkill.\n\nNicely done. Thanks, all. Will queue.\n\n\n>\n> contrib/completion/git-completion.bash | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index 9f71bcde96..8c6b610a24 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -400,7 +400,8 @@ __gitcomp_builtin ()\n> \tif [ -z \"$options\" ]; then\n> \t\t# leading and trailing spaces are significant to make\n> \t\t# option removal work correctly.\n> -\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \"\n> +\t\toptions=\" $incl $(__git ${cmd/_/ } --git-completion-helper) \" || return\n> +\n> \t\tfor i in $excl; do\n> \t\t\toptions=\"${options/ $i / }\"\n> \t\tdone\n"},{"id":"377207","messageId":"CAMP44s2ewOJNYSqQ+8EiBJ8ik7rWMj9yaxWwzYk9nUO+A1gwcg@mail.gmail.com","threadId":"51259","inReplyTo":"CACsJy8CGtZMn0aa7QRj7Dz-jZS2K2fviUJ_Lii48FRY4E84i5Q@mail.gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-14T00:30:33Z","receivedAt":"2019-06-14T00:30:46Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen wrote:\n>\n> On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras\n> wrote:\n\n> > Something like this should work:\n> >\n> > struct command checkout_command = {\n> > .name = \"checkout\",\n> > .function = cmd_checkout,\n> > .run_options = RUN_SETUP | NEED_WORK_TREE,\n> > .help = N_(\"Switch branches or restore working tree files\"),\n> > .options = {\n> > OPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n> > ...\n> > },\n> > }\n> >\n> > This way we could run parse_options_show_gitcomp() from git.c and not\n> > worry about whatever cmd_checkout() needs.\n>\n> This only works for a few commands. Those with subcommands already\n> have struct option[] array scattered in different places. And some new\n> ones also have struct option array dynamically created.\n>\n> It's not impossible to do. But I feel there's a lot of reorganizing\n> for little gain. Maybe when we pass 'struct repository *' to all\n> commands, which means we hit all commmands at once anyway, we can\n> reconsider this (and having config parser in a more declarative form\n> like cmd option parser).\n\nWell yes, there is little *functional* gain at the moment, but this\n(or some version of this) must be done eventually.\n\nFor the moment we still have an issue, but I see there's already a\nhack present for '-h', maybe we can re-utilize it. Something like\nthis:\n\n--- a/git.c\n+++ b/git.c\n@@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int\nargc, const char **argv)\n\n prefix = NULL;\n help = argc == 2 && !strcmp(argv[1], \"-h\");\n+ if (!help)\n+ help = argc == 2 && !strcmp(argv[1], \"--git-completion-helper\");\n if (!help) {\n if (p->option & RUN_SETUP)\n prefix = setup_git_directory();\n\nOne way or the other, shouldn't my tests be merged? The issue is still\nthere, and it's nice to have tests for that.\n\n-- \nFelipe Contreras\n"},{"id":"377208","messageId":"CACsJy8DThHXksARSM32fQ_8fdYqpQYnR5SQmC0i1oVZVc58fPw@mail.gmail.com","threadId":"51259","inReplyTo":"CAMP44s2ewOJNYSqQ+8EiBJ8ik7rWMj9yaxWwzYk9nUO+A1gwcg@mail.gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-14T02:52:56Z","receivedAt":"2019-06-14T02:53:25Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras\n wrote:\n>\n> On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen wrote:\n> >\n> > On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras\n> > wrote:\n>\n> > > Something like this should work:\n> > >\n> > > struct command checkout_command = {\n> > > .name = \"checkout\",\n> > > .function = cmd_checkout,\n> > > .run_options = RUN_SETUP | NEED_WORK_TREE,\n> > > .help = N_(\"Switch branches or restore working tree files\"),\n> > > .options = {\n> > > OPT__QUIET(&opts.quiet, N_(\"suppress progress reporting\")),\n> > > ...\n> > > },\n> > > }\n> > >\n> > > This way we could run parse_options_show_gitcomp() from git.c and not\n> > > worry about whatever cmd_checkout() needs.\n> >\n> > This only works for a few commands. Those with subcommands already\n> > have struct option[] array scattered in different places. And some new\n> > ones also have struct option array dynamically created.\n> >\n> > It's not impossible to do. But I feel there's a lot of reorganizing\n> > for little gain. Maybe when we pass 'struct repository *' to all\n> > commands, which means we hit all commmands at once anyway, we can\n> > reconsider this (and having config parser in a more declarative form\n> > like cmd option parser).\n>\n> Well yes, there is little *functional* gain at the moment, but this\n> (or some version of this) must be done eventually.\n>\n> For the moment we still have an issue, but I see there's already a\n> hack present for '-h', maybe we can re-utilize it. Something like\n> this:\n>\n> --- a/git.c\n> +++ b/git.c\n> @@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int\n> argc, const char **argv)\n>\n> prefix = NULL;\n> help = argc == 2 && !strcmp(argv[1], \"-h\");\n> + if (!help)\n> + help = argc == 2 && !strcmp(argv[1], \"--git-completion-helper\");\n> if (!help) {\n> if (p->option & RUN_SETUP)\n> prefix = setup_git_directory();\n>\n> One way or the other, shouldn't my tests be merged? The issue is still\n> there, and it's nice to have tests for that.\n\nIs there any good reason to complete options when they are not going\nto work anyway (e.g. like checkout which needs $GIT_DIR)? Besides \"it\nused to work before --git-completion-helper\" which I don't consider a\ngood reason given the maintenance tradeoff of --git-completion-helper.\n-- \nDuy\n"},{"id":"377214","messageId":"CAMP44s1RP8ERVX7EPt3oJyd8FP+4OjrGnWnaavnHuORoc+BGxw@mail.gmail.com","threadId":"51259","inReplyTo":"CACsJy8DThHXksARSM32fQ_8fdYqpQYnR5SQmC0i1oVZVc58fPw@mail.gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2019-06-14T06:06:48Z","receivedAt":"2019-06-14T06:07:01Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen wrote:\n>\n> On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras\n> wrote:\n\n> > One way or the other, shouldn't my tests be merged? The issue is still\n> > there, and it's nice to have tests for that.\n>\n> Is there any good reason to complete options when they are not going\n> to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides \"it\n> used to work before --git-completion-helper\" which I don't consider a\n> good reason given the maintenance tradeoff of --git-completion-helper.\n\nNo, there is no good reason that I can think of, except checking the\narguments, which is precisely how I found out the issue; not something\nI usually do. But a newcomer might not know what commands don't work\noutside a git directory.\n\nBut more importantly; is there a good enough reason not to? I seem to\nrecall to be annoyed by the fact that 'git command -h' failed on some\ncommand with a fatal error. Similarly, I don't see any good reason why\n'git help clone' should ever fail.\n\nThese are not dealbreakers by any means, just some kind of weird\ncorner cases. Such things never happen in in Mercurial BTW.\n\nAnd of course --git-completion-helper is the way to go, I recall I\nwanted to implement such a thing myself, this has the potential to\nincrease the power of the zsh completion incredibly, although not yet.\n\nBut that doesn't mean it's perfect and there are no regressions; there\nare. I just think they should be documented in the testing framework.\nThey are not big enough to warrant going back from\n--git-completion-helper though.\n\nThe only real issue I think has not been raised is that the completion\nscripts are in \"contrib\", they are not considered part of the main\ndeliverables. So it's conceivable that somebody running Git v1.6 tries\nthe completion scripts of v1.20, and everything breaks. I'm still not\nexactly sure what should be the way to solve this conundrum, but\nagain, I don't think going back from --git-completion-helper is a good\nmove. I don't think I suggested that.\n\nCheers.\n\n-- \nFelipe Contreras\n"},{"id":"377309","messageId":"CACsJy8AkKEf=EvFTcw2QvsQOfaHawWLas-K0FRq-BBLeHuLFbQ@mail.gmail.com","threadId":"51259","inReplyTo":"CAMP44s1RP8ERVX7EPt3oJyd8FP+4OjrGnWnaavnHuORoc+BGxw@mail.gmail.com","subject":"Re: [PATCH] completion: do not cache if --git-completion-helper fails","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-06-16T01:32:22Z","receivedAt":"2019-06-16T01:32:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jun 14, 2019 at 1:07 PM Felipe Contreras\n wrote:\n>\n> On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen wrote:\n> >\n> > On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras\n> > wrote:\n>\n> > > One way or the other, shouldn't my tests be merged? The issue is still\n> > > there, and it's nice to have tests for that.\n> >\n> > Is there any good reason to complete options when they are not going\n> > to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides \"it\n> > used to work before --git-completion-helper\" which I don't consider a\n> > good reason given the maintenance tradeoff of --git-completion-helper.\n>\n> No, there is no good reason that I can think of, except checking the\n> arguments, which is precisely how I found out the issue; not something\n> I usually do. But a newcomer might not know what commands don't work\n> outside a git directory.\n>\n> But more importantly; is there a good enough reason not to?\n\nFor me, yes. No extra work for me.\n\n> I seem to\n> recall to be annoyed by the fact that 'git command -h' failed on some\n> command with a fatal error. Similarly, I don't see any good reason why\n> 'git help clone' should ever fail.\n\nYou could give the '-h' hack a try. It probably works for some, but I\ndon't think it also works for things like 'git worktree add\n--git-completion-helper'.\n-- \nDuy\n"}]}