[PATCH] test: completion: tests for __gitcomp regression

[PATCH] test: completion: tests for __gitcomp regression

[Felipe Contreras]

There's a regression in the completion since the introduction of
__gitcomp.

Go to any directory that doesn't contain a git repository, like /tmp.
Then type the following:

  git checkout --<tab>

You will see nothing. That's because
`git checkout --git-completion-helper` fails when you run it outside a
git repository.

You might change to a directory that has a git repository, but it's too
late, because the empty options have been cached.

It's unclear how many commands are affected, but this patch attempts to
at least detect some already in the testing framework.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9902-completion.sh | 33 ++++++++++++++++++++++++++-------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 43cf313a1c..1319c542e1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -122,6 +122,15 @@ test_gitcomp_nl ()
 	test_cmp expected out
 }
 
+offgit ()
+{
+	GIT_CEILING_DIRECTORIES="$ROOT" &&
+	export GIT_CEILING_DIRECTORIES &&
+	test_when_finished "ROOT='$ROOT'; cd '$TRASH_DIRECTORY'; unset GIT_CEILING_DIRECTORIES" &&
+	ROOT="$ROOT"/non-repo &&
+	cd "$ROOT"
+}
+
 invalid_variable_name='${foo.bar}'
 
 actual="$TRASH_DIRECTORY/actual"
@@ -358,10 +367,8 @@ test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symli
 '
 
 test_expect_success '__git_find_repo_path - not a git repository' '
+	offgit &&
 	(
-		cd non-repo &&
-		GIT_CEILING_DIRECTORIES="$ROOT" &&
-		export GIT_CEILING_DIRECTORIES &&
 		test_must_fail __git_find_repo_path &&
 		printf "$__git_repo_path" >"$actual"
 	) &&
@@ -1388,6 +1395,7 @@ test_expect_success '__git_pretty_aliases' '
 '
 
 test_expect_success 'basic' '
+	offgit &&
 	run_completion "git " &&
 	# built-in
 	grep -q "^add \$" out &&
@@ -1401,6 +1409,7 @@ test_expect_success 'basic' '
 '
 
 test_expect_success 'double dash "git" itself' '
+	offgit &&
 	test_completion "git --" <<-\EOF
 	--paginate Z
 	--no-pager Z
@@ -1419,7 +1428,8 @@ test_expect_success 'double dash "git" itself' '
 	EOF
 '
 
-test_expect_success 'double dash "git checkout"' '
+test_expect_failure 'double dash "git checkout"' '
+	offgit &&
 	test_completion "git checkout --" <<-\EOF
 	--quiet Z
 	--detach Z
@@ -1442,6 +1452,7 @@ test_expect_success 'double dash "git checkout"' '
 '
 
 test_expect_success 'general options' '
+	offgit &&
 	test_completion "git --ver" "--version " &&
 	test_completion "git --hel" "--help " &&
 	test_completion "git --exe" <<-\EOF &&
@@ -1460,6 +1471,7 @@ test_expect_success 'general options' '
 '
 
 test_expect_success 'general options plus command' '
+	offgit &&
 	test_completion "git --version check" "checkout " &&
 	test_completion "git --paginate check" "checkout " &&
 	test_completion "git --git-dir=foo check" "checkout " &&
@@ -1480,11 +1492,13 @@ test_expect_success 'general options plus command' '
 '
 
 test_expect_success 'git --help completion' '
+	offgit &&
 	test_completion "git --help ad" "add " &&
 	test_completion "git --help core" "core-tutorial "
 '
 
-test_expect_success 'completion.commands removes multiple commands' '
+test_expect_failure 'completion.commands removes multiple commands' '
+	offgit &&
 	test_config completion.commands "-cherry -mergetool" &&
 	git --list-cmds=list-mainporcelain,list-complete,config >out &&
 	! grep -E "^(cherry|mergetool)$" out
@@ -1547,7 +1561,8 @@ test_expect_success 'complete tree filename with metacharacters' '
 	EOF
 '
 
-test_expect_success PERL 'send-email' '
+test_expect_failure PERL 'send-email' '
+	offgit &&
 	test_completion "git send-email --cov" "--cover-letter " &&
 	test_completion "git send-email ma" "master "
 '
@@ -1649,6 +1664,7 @@ test_expect_success 'completion used <cmd> completion for alias: !f() { : git <c
 '
 
 test_expect_success 'completion without explicit _git_xxx function' '
+	offgit &&
 	test_completion "git version --" <<-\EOF
 	--build-options Z
 	--no-build-options Z
@@ -1699,13 +1715,15 @@ do
 done
 
 test_expect_success 'sourcing the completion script clears cached commands' '
+	offgit &&
 	__git_compute_all_commands &&
 	verbose test -n "$__git_all_commands" &&
 	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
 	verbose test -z "$__git_all_commands"
 '
 
-test_expect_success 'sourcing the completion script clears cached merge strategies' '
+test_expect_failure 'sourcing the completion script clears cached merge strategies' '
+	offgit &&
 	GIT_TEST_GETTEXT_POISON= &&
 	__git_compute_merge_strategies &&
 	verbose test -n "$__git_merge_strategies" &&
@@ -1714,6 +1732,7 @@ test_expect_success 'sourcing the completion script clears cached merge strategi
 '
 
 test_expect_success 'sourcing the completion script clears cached --options' '
+	offgit &&
 	__gitcomp_builtin checkout &&
 	verbose test -n "$__gitcomp_builtin_checkout" &&
 	__gitcomp_builtin notes_edit &&
-- 
2.21.0

[PATCH] completion: do not cache if --git-completion-helper fails

[Nguyễn Thái Ngọc Duy]

"git <cmd> --git-completion-helper" could fail if the command checks for
a repo before parse_options(). If the result is cached, later on when
the user moves to a worktree with repo, tab completion will still fail.

Avoid this by detecting errors and not cache the completion output. We
can try again and hopefully succeed next time (e.g. when a repo is
found).

Of course if --git-completion-helper fails permanently because of other
reasons (*), this will slow down completion. But I don't see any better
option to handle that case.

(*) one of those cases is if __gitcomp_builtin is called on a command
  that does not support --git-completion-helper. And we do have a
  generic call

    __git_complete_common "$command"

  but this case is protected with __git_support_parseopt_helper so we're
  good.

Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 contrib/completion/git-completion.bash | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f71bcde96..a515de8501 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -398,13 +398,15 @@ __gitcomp_builtin ()
 	eval "options=\$$var"
 
 	if [ -z "$options" ]; then
+		local nocache=
 		# leading and trailing spaces are significant to make
 		# option removal work correctly.
-		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
+		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t
+
 		for i in $excl; do
 			options="${options/ $i / }"
 		done
-		eval "$var=\"$options\""
+		test -n "$nocache" || eval "$var=\"$options\""
 	fi
 
 	__gitcomp "$options"
-- 
2.22.0.rc0.322.g2b0371e29a

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[SZEDER Gábor]

On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
> "git <cmd> --git-completion-helper" could fail if the command checks for
> a repo before parse_options(). If the result is cached, later on when
> the user moves to a worktree with repo, tab completion will still fail.
> 
> Avoid this by detecting errors and not cache the completion output. We
> can try again and hopefully succeed next time (e.g. when a repo is
> found).
> 
> Of course if --git-completion-helper fails permanently because of other
> reasons (*), this will slow down completion. But I don't see any better
> option to handle that case.

I think a permanently failing 'git cmd --git-completion-helper'
shouldn't really happen, unless there is a bug in the completion
script or the git installation or similar exceptional situation.  And
then that issue should be fixed, but I don't think we should worry
about an extra subshell and git process in those situations.

> (*) one of those cases is if __gitcomp_builtin is called on a command
>   that does not support --git-completion-helper. And we do have a
>   generic call
> 
>     __git_complete_common "$command"
> 
>   but this case is protected with __git_support_parseopt_helper so we're
>   good.
> 
> Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 9f71bcde96..a515de8501 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -398,13 +398,15 @@ __gitcomp_builtin ()
>  	eval "options=\$$var"
>  
>  	if [ -z "$options" ]; then
> +		local nocache=
>  		# leading and trailing spaces are significant to make
>  		# option removal work correctly.
> -		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
> +		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t

Why not just 'return' here?

If we return, then it won't list any options, and the shell should fall
back to regular file completion.  With this patch it will still list
extra options, if there are any, and I don't really see the point in
doing that.


> +
>  		for i in $excl; do
>  			options="${options/ $i / }"
>  		done
> -		eval "$var=\"$options\""
> +		test -n "$nocache" || eval "$var=\"$options\""
>  	fi
>  
>  	__gitcomp "$options"
> -- 
> 2.22.0.rc0.322.g2b0371e29a
> 

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

>  	if [ -z "$options" ]; then
> +		local nocache=
>  		# leading and trailing spaces are significant to make
>  		# option removal work correctly.
> -		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
> +		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || nocache=t
> +
>  		for i in $excl; do
>  			options="${options/ $i / }"
>  		done

Is there a point in doing this loop if we are not going to eval
after all?  IOW...

> -		eval "$var=\"$options\""
> +		test -n "$nocache" || eval "$var=\"$options\""
>  	fi

... I am wondering why it is not more like this

	if options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
	then
		for i in $excl
		do
			options=...
		done
		eval "$var=..."
	fi

or just return from the function instead of introducing and setting
a new variable, as only remaining thing the function does is to call
__gitcomp with $options, but we know that we are giving up on
completing this round.

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Felipe Contreras]

On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
> > "git <cmd> --git-completion-helper" could fail if the command checks for
> > a repo before parse_options(). If the result is cached, later on when
> > the user moves to a worktree with repo, tab completion will still fail.
> >
> > Avoid this by detecting errors and not cache the completion output. We
> > can try again and hopefully succeed next time (e.g. when a repo is
> > found).
> >
> > Of course if --git-completion-helper fails permanently because of other
> > reasons (*), this will slow down completion. But I don't see any better
> > option to handle that case.
>
> I think a permanently failing 'git cmd --git-completion-helper'
> shouldn't really happen, unless there is a bug in the completion
> script or the git installation or similar exceptional situation.  And
> then that issue should be fixed, but I don't think we should worry
> about an extra subshell and git process in those situations.

Indeed. In think there's only sane option to make this work in all
situation; a reorganization.

Something like this should work:

struct command checkout_command = {
.name = "checkout",
.function = cmd_checkout,
.run_options = RUN_SETUP | NEED_WORK_TREE,
.help = N_("Switch branches or restore working tree files"),
.options = {
OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
...
},
}

This way we could run parse_options_show_gitcomp() from git.c and not
worry about whatever cmd_checkout() needs.

This has the added advantage that it gathers information about this
command that is stray in multiple sources (git.c, command-list.h), and
it makes builtin.h cleaner too.

Plus, we could rework the way -h works too.

-- 
Felipe Contreras

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Duy Nguyen]

On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Fri, Jun 7, 2019 at 5:02 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Fri, Jun 07, 2019 at 04:30:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
> > > "git <cmd> --git-completion-helper" could fail if the command checks for
> > > a repo before parse_options(). If the result is cached, later on when
> > > the user moves to a worktree with repo, tab completion will still fail.
> > >
> > > Avoid this by detecting errors and not cache the completion output. We
> > > can try again and hopefully succeed next time (e.g. when a repo is
> > > found).
> > >
> > > Of course if --git-completion-helper fails permanently because of other
> > > reasons (*), this will slow down completion. But I don't see any better
> > > option to handle that case.
> >
> > I think a permanently failing 'git cmd --git-completion-helper'
> > shouldn't really happen, unless there is a bug in the completion
> > script or the git installation or similar exceptional situation.  And
> > then that issue should be fixed, but I don't think we should worry
> > about an extra subshell and git process in those situations.
>
> Indeed. In think there's only sane option to make this work in all
> situation; a reorganization.
>
> Something like this should work:
>
> struct command checkout_command = {
> .name = "checkout",
> .function = cmd_checkout,
> .run_options = RUN_SETUP | NEED_WORK_TREE,
> .help = N_("Switch branches or restore working tree files"),
> .options = {
> OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
> ...
> },
> }
>
> This way we could run parse_options_show_gitcomp() from git.c and not
> worry about whatever cmd_checkout() needs.

This only works for a few commands. Those with subcommands already
have struct option[] array scattered in different places. And some new
ones also have struct option array dynamically created.

It's not impossible to do. But I feel there's a lot of reorganizing
for little gain. Maybe when we pass 'struct repository *' to all
commands, which means we hit all commmands at once anyway, we can
reconsider this (and having config parser in a more declarative form
like cmd option parser).

> This has the added advantage that it gathers information about this
> command that is stray in multiple sources (git.c, command-list.h), and
> it makes builtin.h cleaner too.
>
> Plus, we could rework the way -h works too.
-- 
Duy

[PATCH v2] completion: do not cache if --git-completion-helper fails

[Nguyễn Thái Ngọc Duy]

"git <cmd> --git-completion-helper" could fail if the command checks for
a repo before parse_options(). If the result is cached, later on when
the user moves to a worktree with repo, tab completion will still fail.

Avoid this by detecting errors and not cache the completion output. We
can try again and hopefully succeed next time (e.g. when a repo is
found).

Of course if --git-completion-helper fails permanently because of other
reasons (*), this will slow down completion. But I don't see any better
option to handle that case.

(*) one of those cases is if __gitcomp_builtin is called on a command
  that does not support --git-completion-helper. And we do have a
  generic call

    __git_complete_common "$command"

  but this case is protected with __git_support_parseopt_helper so we're
  good.

Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 v2 simplifies the code. $nocache in v1 was overkill.

 contrib/completion/git-completion.bash | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f71bcde96..8c6b610a24 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -400,7 +400,8 @@ __gitcomp_builtin ()
 	if [ -z "$options" ]; then
 		# leading and trailing spaces are significant to make
 		# option removal work correctly.
-		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
+		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || return
+
 		for i in $excl; do
 			options="${options/ $i / }"
 		done
-- 
2.22.0.rc0.322.g2b0371e29a

Re: [PATCH v2] completion: do not cache if --git-completion-helper fails

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> "git <cmd> --git-completion-helper" could fail if the command checks for
> a repo before parse_options(). If the result is cached, later on when
> the user moves to a worktree with repo, tab completion will still fail.
>
> Avoid this by detecting errors and not cache the completion output. We
> can try again and hopefully succeed next time (e.g. when a repo is
> found).
>
> Of course if --git-completion-helper fails permanently because of other
> reasons (*), this will slow down completion. But I don't see any better
> option to handle that case.
>
> (*) one of those cases is if __gitcomp_builtin is called on a command
>   that does not support --git-completion-helper. And we do have a
>   generic call
>
>     __git_complete_common "$command"
>
>   but this case is protected with __git_support_parseopt_helper so we're
>   good.
>
> Reported-by: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  v2 simplifies the code. $nocache in v1 was overkill.

Nicely done.  Thanks, all.  Will queue.


>
>  contrib/completion/git-completion.bash | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 9f71bcde96..8c6b610a24 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -400,7 +400,8 @@ __gitcomp_builtin ()
>  	if [ -z "$options" ]; then
>  		# leading and trailing spaces are significant to make
>  		# option removal work correctly.
> -		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) "
> +		options=" $incl $(__git ${cmd/_/ } --git-completion-helper) " || return
> +
>  		for i in $excl; do
>  			options="${options/ $i / }"
>  		done

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Felipe Contreras]

On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > Something like this should work:
> >
> > struct command checkout_command = {
> > .name = "checkout",
> > .function = cmd_checkout,
> > .run_options = RUN_SETUP | NEED_WORK_TREE,
> > .help = N_("Switch branches or restore working tree files"),
> > .options = {
> > OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
> > ...
> > },
> > }
> >
> > This way we could run parse_options_show_gitcomp() from git.c and not
> > worry about whatever cmd_checkout() needs.
>
> This only works for a few commands. Those with subcommands already
> have struct option[] array scattered in different places. And some new
> ones also have struct option array dynamically created.
>
> It's not impossible to do. But I feel there's a lot of reorganizing
> for little gain. Maybe when we pass 'struct repository *' to all
> commands, which means we hit all commmands at once anyway, we can
> reconsider this (and having config parser in a more declarative form
> like cmd option parser).

Well yes, there is little *functional* gain at the moment, but this
(or some version of this) must be done eventually.

For the moment we still have an issue, but I see there's already a
hack present for '-h', maybe we can re-utilize it. Something like
this:

--- a/git.c
+++ b/git.c
@@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int
argc, const char **argv)

        prefix = NULL;
        help = argc == 2 && !strcmp(argv[1], "-h");
+       if (!help)
+               help = argc == 2 && !strcmp(argv[1], "--git-completion-helper");
        if (!help) {
                if (p->option & RUN_SETUP)
                        prefix = setup_git_directory();

One way or the other, shouldn't my tests be merged? The issue is still
there, and it's nice to have tests for that.

-- 
Felipe Contreras

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Duy Nguyen]

On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Wed, Jun 12, 2019 at 3:52 AM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Sat, Jun 8, 2019 at 12:33 AM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
>
> > > Something like this should work:
> > >
> > > struct command checkout_command = {
> > > .name = "checkout",
> > > .function = cmd_checkout,
> > > .run_options = RUN_SETUP | NEED_WORK_TREE,
> > > .help = N_("Switch branches or restore working tree files"),
> > > .options = {
> > > OPT__QUIET(&opts.quiet, N_("suppress progress reporting")),
> > > ...
> > > },
> > > }
> > >
> > > This way we could run parse_options_show_gitcomp() from git.c and not
> > > worry about whatever cmd_checkout() needs.
> >
> > This only works for a few commands. Those with subcommands already
> > have struct option[] array scattered in different places. And some new
> > ones also have struct option array dynamically created.
> >
> > It's not impossible to do. But I feel there's a lot of reorganizing
> > for little gain. Maybe when we pass 'struct repository *' to all
> > commands, which means we hit all commmands at once anyway, we can
> > reconsider this (and having config parser in a more declarative form
> > like cmd option parser).
>
> Well yes, there is little *functional* gain at the moment, but this
> (or some version of this) must be done eventually.
>
> For the moment we still have an issue, but I see there's already a
> hack present for '-h', maybe we can re-utilize it. Something like
> this:
>
> --- a/git.c
> +++ b/git.c
> @@ -408,6 +408,8 @@ static int run_builtin(struct cmd_struct *p, int
> argc, const char **argv)
>
>         prefix = NULL;
>         help = argc == 2 && !strcmp(argv[1], "-h");
> +       if (!help)
> +               help = argc == 2 && !strcmp(argv[1], "--git-completion-helper");
>         if (!help) {
>                 if (p->option & RUN_SETUP)
>                         prefix = setup_git_directory();
>
> One way or the other, shouldn't my tests be merged? The issue is still
> there, and it's nice to have tests for that.

Is there any good reason to complete options when they are not going
to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it
used to work before --git-completion-helper" which I don't consider a
good reason given the maintenance tradeoff of --git-completion-helper.
-- 
Duy

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Felipe Contreras]

On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen <pclouds@gmail.com> wrote:
>
> On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:

> > One way or the other, shouldn't my tests be merged? The issue is still
> > there, and it's nice to have tests for that.
>
> Is there any good reason to complete options when they are not going
> to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it
> used to work before --git-completion-helper" which I don't consider a
> good reason given the maintenance tradeoff of --git-completion-helper.

No, there is no good reason that I can think of, except checking the
arguments, which is precisely how I found out the issue; not something
I usually do. But a newcomer might not know what commands don't work
outside a git directory.

But more importantly; is there a good enough reason not to? I seem to
recall to be annoyed by the fact that 'git command -h' failed on some
command with a fatal error. Similarly, I don't see any good reason why
'git help clone' should ever fail.

These are not dealbreakers by any means, just some kind of weird
corner cases. Such things never happen in in Mercurial BTW.

And of course --git-completion-helper is the way to go, I recall I
wanted to implement such a thing myself, this has the potential to
increase the power of the zsh completion incredibly, although not yet.

But that doesn't mean it's perfect and there are no regressions; there
are. I just think they should be documented in the testing framework.
They are not big enough to warrant going back from
--git-completion-helper though.

The only real issue I think has not been raised is that the completion
scripts are in "contrib", they are not considered part of the main
deliverables. So it's conceivable that somebody running Git v1.6 tries
the completion scripts of v1.20, and everything breaks. I'm still not
exactly sure what should be the way to solve this conundrum, but
again, I don't think going back from --git-completion-helper is a good
move. I don't think I suggested that.

Cheers.

-- 
Felipe Contreras

Re: [PATCH] completion: do not cache if --git-completion-helper fails

[Duy Nguyen]

On Fri, Jun 14, 2019 at 1:07 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> On Thu, Jun 13, 2019 at 9:53 PM Duy Nguyen <pclouds@gmail.com> wrote:
> >
> > On Fri, Jun 14, 2019 at 7:30 AM Felipe Contreras
> > <felipe.contreras@gmail.com> wrote:
>
> > > One way or the other, shouldn't my tests be merged? The issue is still
> > > there, and it's nice to have tests for that.
> >
> > Is there any good reason to complete options when they are not going
> > to work anyway (e.g. like checkout which needs $GIT_DIR)? Besides "it
> > used to work before --git-completion-helper" which I don't consider a
> > good reason given the maintenance tradeoff of --git-completion-helper.
>
> No, there is no good reason that I can think of, except checking the
> arguments, which is precisely how I found out the issue; not something
> I usually do. But a newcomer might not know what commands don't work
> outside a git directory.
>
> But more importantly; is there a good enough reason not to?

For me, yes. No extra work for me.

> I seem to
> recall to be annoyed by the fact that 'git command -h' failed on some
> command with a fatal error. Similarly, I don't see any good reason why
> 'git help clone' should ever fail.

You could give the '-h' hack a try. It probably works for some, but I
don't think it also works for things like 'git worktree add
--git-completion-helper'.
-- 
Duy