git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDS

From
Junio C Hamano <gitster@pobox.com>
Date
Jan 26, 2022, 22:34 UTC
Message-ID
<xmqqk0emp1m8.fsf@gitster.g>
In-Reply-To
<patch-1.1-5f18305ca08-20220125T124757Z-avarab@gmail.com>
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 8 quoted lines
> 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.
Show 5 quoted lines
> 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.

Show 11 quoted lines
> --- 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.

Show 13 quoted lines
>  #   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.

Show 5 quoted lines
> +				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.

Show 11 quoted lines
> 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.
Previous: Ævar Arnfjörð BjarmasonNext: Ævar Arnfjörð Bjarmason
Message 10 of 17 in “Some sub-commands can't be completed by TAB key.”
  1. Hongyi ZhaoJan 22, 2022
  2. Johannes SixtJan 22, 2022
  3. Hongyi ZhaoJan 23, 2022
  4. Philippe BlainJan 23, 2022
  5. Junio C HamanoJan 23, 2022
  6. Hongyi ZhaoJan 24, 2022
  7. João Victor BonfimJan 24, 2022
  8. Junio C HamanoJan 25, 2022
  9. completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDSÆvar Arnfjörð Bjarmason, Jan 25, 2022
  10. Junio C HamanoJan 26, 2022
  11. 0/2 completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDSÆvar Arnfjörð Bjarmason, Feb 2, 2022
  12. 1/2 completion tests: re-source git-completion.bash in a subshellÆvar Arnfjörð Bjarmason, Feb 2, 2022
  13. 2/2 completion: add a GIT_COMPLETION_SHOW_ALL_COMMANDSÆvar Arnfjörð Bjarmason, Feb 2, 2022
  14. SZEDER GáborFeb 6, 2022
  15. Junio C HamanoFeb 6, 2022
  16. SZEDER GáborFeb 6, 2022
  17. Junio C HamanoFeb 7, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.