Re: [PATCH] completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDS

[Junio C Hamano <gitster@pobox.com>]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Add a GIT_COMPLETION_SHOW_ALL_COMMANDS=1 configuration setting to go
> with the existing GIT_COMPLETION_SHOW_ALL=1 added in
> c099f579b98 (completion: add GIT_COMPLETION_SHOW_ALL env var,
> 2020-08-19).
>
> This will include plumbing commands such as "cat-file" in "git <TAB>"
> and "git c<TAB>" completion. Without/with this I have 134 and 243
> completion with git <TAB>, respectively.

OK.  This makes sense in the sense that more choice is better.

> It was already possible to do this by tweaking
> GIT_COMPLETION_SHOW_ALL_COMMANDS from the outside, that testing
> variable was added in 84a97131065 (completion: let git provide the
> completable command list, 2018-05-20). Doing this before loading
> git-completion.bash worked:

Perhaps there is a typo that ruined whole the paragraph.  We are
adding that variable with this patch, so by definition, it did not
exist before, which means we cannot "tweak" it because it did not
exist.

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -49,6 +49,11 @@
>  #     and git-switch completion (e.g., completing "foo" when "origin/foo"
>  #     exists).
>  #
> +#   GIT_COMPLETION_SHOW_ALL_COMMANDS
> +#
> +#     When set to "1" suggest all commands, including plumbing commands
> +#     which are hidden by default (e.g. "cat-file" on "git ca<TAB>").
> +#

Usually we frown upon inserting a new thing to the middle of a list
of things that has no inherent order.  In this case, I think this is
OK, as the existing "all" (below) is about completing options, while
the new one is about completing subcommands, and the latter is at a
higher conceptual level than the former.

>  #   GIT_COMPLETION_SHOW_ALL
>  #
>  #     When set to "1" suggest all options, including options which are
> @@ -3455,7 +3460,13 @@ __git_main ()
>  			then
>  				__gitcomp "$GIT_TESTING_PORCELAIN_COMMAND_LIST"
>  			else
> -				__gitcomp "$(__git --list-cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config)"
> +				local list_cmds=list-mainporcelain,others,nohelpers,alias,list-complete,config
> +
> +				if test "${GIT_COMPLETION_SHOW_ALL_COMMANDS-}" = "1"
> +				then
> +					list_cmds=builtins,$list_cmds

It is sad that there is no "plumbing" class (assuming the goal is
"we by default exclude plumbing, so add that to the list"), or just
"everything under the sun" class.  If there were a plumbing command
that is not implemented as a built-in, adding buitlins to list_cmds
will not show the command, will it?  Also, because nohelpers is not
removed from list_cmds, whatever command that were removed from
exclude_helpers_from_list() will be hidden.

It looks as though help.c needs a new list_all_cmds() that can be
called from git.c::list_cmds() when "all" is asked for, and dumps
everything from command_list[] plus whatever load_command_list()
loads.

> +				fi
> +				__gitcomp "$(__git --list-cmds=$list_cmds)"
>  			fi
>  			;;
>  		esac

Having said all that, assuming that including "builtins" is a good
enough approximation (which I do not have no opinion on), the
implementation looks good to me.

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 98c62806328..e3ea6a41b00 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2432,6 +2432,33 @@ test_expect_success 'option aliases are shown with GIT_COMPLETION_SHOW_ALL' '
>  	EOF
>  '
>  
> +test_expect_success 'plumbing commands are excluded without GIT_COMPLETION_SHOW_ALL_COMMANDS' '
> +	. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" &&
> +	sane_unset GIT_TESTING_PORCELAIN_COMMAND_LIST &&

As we've done dot-sourcing of the file at the beginning of the
script already, dot-sourcing the same thing again would only
overwrite what was done before, without clearing the deck.  Which
may not hurt for the purpose of _this_ test _right_ _now_q.

But as this is not done inside a subshell, whetever we dot-source
here will persist til the end of the script.  Which may be more
problematic as it will affect the tests that come (and new tests
that will be added) after this point.

The same comment applies to the other new test added immediately
after this one.

Other than that, looks sensible to me.

Thanks.