patchtest: completion: tests for __gitcomp regression

> "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
> 

>
> 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.

>
> 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.

> > 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).

>
> 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.

> > 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.

>
> 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?

>  	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

> "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