completion: remove hardcoded config variable names

This series removes hardcoded config variable names in the __git_complete_config_variable_name function, partly by adding a new mode to 'git help'. It also adds completion for 'submodule.*' config variables, which were previously missing.

I think it makes sense to do that in the same series since it's closely related, and splitting it would result in textual conflicts between both series if one does not build on top of the other, but I'm open to other suggestions.

Thanks,

Philippe.

Philippe Blain (5):
completion: add space after config variable names also in Bash 3
completion: complete 'submodule.*' config variables
completion: add and use
__git_compute_first_level_config_vars_for_section
builtin/help: add --config-all-for-completion
completion: add an use
__git_compute_second_level_config_vars_for_section

 builtin/help.c                         |  7 ++
 contrib/completion/git-completion.bash | 90 +++++++++++++-------------
 t/t9902-completion.sh                  | 21 ++++++
 3 files changed, 74 insertions(+), 44 deletions(-)

base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1660%2Fphil-blain%2Fcompletion-submodule-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1660/phil-blain/completion-submodule-config-v1
Pull-Request: https://github.com/git/git/pull/1660

gitgitgadget

From: Philippe Blain <[email protected]>

In be6444d1ca (completion: bash: add correct suffix in variables, 2021-08-16), __git_complete_config_variable_name was changed to use "${sfx- }" instead of "$sfx" as the fourth argument of _gitcomp_nl and _gitcomp_nl_append, such that this argument evaluates to a space if sfx is unset. This was to ensure that e.g.

git config branch.autoSetupMe[TAB]

correctly completes to 'branch.autoSetupMerge ' with the trailing space. This commits notes that the fix only works in Bash 4 because in Bash 3 the 'local sfx' construct at the beginning of __git_complete_config_variable_name creates an empty string.

Make the fix also work for Bash 3 by using the "unset or null' parameter expansion syntax ("${sfx:- }"), such that the parameter is also expanded to a space if it is set but null, as is the behaviour of 'local sfx' in Bash 3.

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

index 6662db221df..159a4fd8add 100644
@@ -2750,7 +2750,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2784,7 +2784,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__git_compute_all_commands
-		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	remote.*.*)

@@ -2800,7 +2800,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

gitgitgadget

From: Philippe Blain <[email protected]>

In the Bash completion script, function __git_complete_config_variable_name completes config variables and has special logic to deal with config variables involving user-defined names, like branch.<name>.* and remote.<name>.*.

This special logic is missing for submodule-related config variables. Add the appropriate branches to the case statement, making use of the in-tree '.gitmodules' to list relevant submodules.

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

index 159a4fd8add..8af9bc3f4e1 100644
@@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
+	submodule.*.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_##*.}"
+		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
+		return
+		;;
+	submodule.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_#*.}"
+		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
+		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		return
+		;;
 	url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"

gitgitgadget

From: Philippe Blain <[email protected]>

The function __git_complete_config_variable_name in the Bash completion script hardcodes several config variable names. These variables are those in config section where user-defined names can appear, such as "branch.<name>". These sections are treated first by the case statement, and the two last "catch all" cases are used for other sections, making use of the __git_compute_config_vars and __git_compute_config_sections function, which omit listing any variables containing wildcards or placeholders. Having hardcoded config variables introduces the risk of the completion code becoming out of sync with the actual config variables accepted by Git.

To avoid these hardcoded config variables, introduce a new function, __git_compute_first_level_config_vars_for_section, making use of the existing __git_config_vars variable. This function takes as argument a config section name and computes the matching "first level" config variables for that section, i.e. those _not_ containing any placeholder, like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this function and the variables it defines in the 'branch.*', 'remote.*' and 'submodule.*' switches of the case statement instead of hardcoding the corresponding config variables. Note that we use indirect expansion instead of associative arrays because those are not supported in Bash 3, on which macOS is stuck for licensing reasons.

Add a test to make sure the new function works correctly by verfying it lists all 'submodule' config variables. This has the downside that this test must be updated when new 'submodule' configuration are added, but this should be a small burden since it happens infrequently.

---
 contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
 t/t9902-completion.sh                  | 11 +++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

index 8af9bc3f4e1..2934ceb7637 100644
@@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_compute_first_level_config_vars_for_section ()
+{
+	section="$1"
+	__git_compute_config_vars
+	local this_section="__git_first_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
 	branch.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	submodule.*.*)

@@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
-		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

index 35eb534fdda..f28d8f531b7 100755
@@ -2583,6 +2583,17 @@ test_expect_success 'git config - variable name include' '
 	EOF
 '
 
+test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
+	test_completion "git config submodule." <<-\EOF
+	submodule.active Z
+	submodule.alternateErrorStrategy Z
+	submodule.alternateLocation Z
+	submodule.fetchJobs Z
+	submodule.propagateBranches Z
+	submodule.recurse Z
+	EOF
+'
+
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF
 	false Z

gitgitgadget

From: Philippe Blain <[email protected]>

There is currently no machine-friendly way to show _all_ configuration variables from the command line. 'git help --config' does show them all, but it also sets up the pager. 'git help --config-for-completion' omits some variables (those containing wildcards, for example) and 'git help --config-section-for-completion' shows only top-level section names.

In a following commit we will want to have access to a list of all configuration variables from the Bash completion script. As such, add a new mode for the command, HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, triggered by the new option '--config-all-for-completion'. In this mode, show all variables, just as HELP_ACTION_CONFIG, but do not set up the pager.

---
 builtin/help.c | 7 +++++++
 1 file changed, 7 insertions(+)

index dc1fbe2b986..dacaeb10bf4 100644
@@ -50,6 +50,7 @@ static enum help_action {
 	HELP_ACTION_DEVELOPER_INTERFACES,
 	HELP_ACTION_CONFIG_FOR_COMPLETION,
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+	HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
 } cmd_mode;
 
 static const char *html_path;

@@ -86,6 +87,8 @@ static struct option builtin_help_options[] = {
 		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
 	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
 		    HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+	OPT_CMDMODE_F(0, "config-all-for-completion", &cmd_mode, "",
+		    HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, PARSE_OPT_HIDDEN),
 
 	OPT_END(),
 };

@@ -670,6 +673,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		opt_mode_usage(argc, "--config-for-completion", help_format);
 		list_config_help(SHOW_CONFIG_VARS);
 		return 0;
+	case HELP_ACTION_CONFIG_ALL_FOR_COMPLETION:
+		opt_mode_usage(argc, "--config-all-for-completion", help_format);
+		list_config_help(SHOW_CONFIG_HUMAN);
+		return 0;
 	case HELP_ACTION_USER_INTERFACES:
 		opt_mode_usage(argc, "--user-interfaces", help_format);
 		list_user_interfaces_help();

gitgitgadget

From: Philippe Blain <[email protected]>

In a previous commit we removed some hardcoded config variable names from function __git_complete_config_variable_name in the completion script by introducing a new function, __git_compute_first_level_config_vars_for_section.

The remaining hardcoded config variables are "second level" configuration variables, meaning 'branch.<name>.upstream', 'remote.<name>.url', etc. where <name> is a user-defined name.

Making use of the new --config-all-for-completion flag to 'git help' introduced in the previous commit, add a new function, __git_compute_second_level_config_vars_for_section. This function takes as argument a config section name and computes the corresponding second-level config variables, i.e. those that contain a '<' which indicates the start of a placeholder. Note that as in __git_compute_first_level_config_vars_for_section added previsouly, we use indirect expansion instead of associative arrays to stay compatible with Bash 3 on which macOS is stuck for licensing reasons.

Use this new function and the variables it defines in __git_complete_config_variable_name to remove hardcoded config variables, and add a test to verify the new function. Use a single 'case' for all sections with second-level variables names, since the code for each of them is now exactly the same.

---
 contrib/completion/git-completion.bash | 71 ++++++++------------------
 t/t9902-completion.sh                  | 10 ++++
 2 files changed, 31 insertions(+), 50 deletions(-)

index 2934ceb7637..0e8fd63bfdb 100644
@@ -2596,6 +2596,13 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_config_vars_all=
+__git_compute_config_vars_all ()
+{
+	test -n "$__git_config_vars_all" ||
+	__git_config_vars_all="$(git help --config-all-for-completion)"
+}
+
 __git_compute_first_level_config_vars_for_section ()
 {
 	section="$1"

@@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
 	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
 }
 
+__git_compute_second_level_config_vars_for_section ()
+{
+	section="$1"
+	__git_compute_config_vars_all
+	local this_section="__git_second_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
 	done
 
 	case "$cur_" in
-	branch.*.*)
+	branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"
-		__gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
+		local section="${pfx%.*.}"
+		__git_compute_second_level_config_vars_for_section "${section}"
+		local this_section="__git_second_level_config_vars_for_section_${section}"
+		__gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
 		return
 		;;
 	branch.*)

@@ -2765,33 +2784,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	guitool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			argPrompt cmd confirm needsFile noConsole noRescan
-			prompt revPrompt revUnmerged title
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	difftool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	man.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	mergetool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path trustExitCode" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	pager.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2799,15 +2791,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	remote.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			url proxy fetch push mirror skipDefaultUpdate
-			receivepack uploadpack tagOpt pushurl
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2818,12 +2801,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	submodule.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2834,12 +2811,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	url.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	*.*)
 		__git_compute_config_vars
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"

index f28d8f531b7..24ff786b273 100755
@@ -2593,6 +2593,16 @@ test_expect_success 'git config - variable name - __git_compute_first_level_conf
 	submodule.recurse Z
 	EOF
 '
+test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
+	test_completion "git config branch.main." <<-\EOF
+	branch.main.description Z
+	branch.main.remote Z
+	branch.main.pushRemote Z
+	branch.main.merge Z
+	branch.main.mergeOptions Z
+	branch.main.rebase Z
+	EOF
+'
 
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF

gitgitgadget

Changes since v1:

* Corrected my email in PATCH 2/5 (sorry for the noise)

v1: This series removes hardcoded config variable names in the __git_complete_config_variable_name function, partly by adding a new mode to 'git help'. It also adds completion for 'submodule.*' config variables, which were previously missing.

I think it makes sense to do that in the same series since it's closely related, and splitting it would result in textual conflicts between both series if one does not build on top of the other, but I'm open to other suggestions.

Thanks,

Philippe.

Philippe Blain (5):
completion: add space after config variable names also in Bash 3
completion: complete 'submodule.*' config variables
completion: add and use
__git_compute_first_level_config_vars_for_section
builtin/help: add --config-all-for-completion
completion: add an use
__git_compute_second_level_config_vars_for_section

 builtin/help.c                         |  7 ++
 contrib/completion/git-completion.bash | 90 +++++++++++++-------------
 t/t9902-completion.sh                  | 21 ++++++
 3 files changed, 74 insertions(+), 44 deletions(-)

base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1660%2Fphil-blain%2Fcompletion-submodule-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1660/phil-blain/completion-submodule-config-v2
Pull-Request: https://github.com/git/git/pull/1660

Range-diff vs v1:

 1:  837d92a6c27 = 1:  837d92a6c27 completion: add space after config variable names also in Bash 3
 2:  2dd3085f8d8 ! 2:  426374ff9b3 completion: complete 'submodule.*' config variables

@@
## Metadata ##
-Author: Philippe Blain <[email protected]> +Author: Philippe Blain <[email protected]>

## Commit message ##
completion: complete 'submodule.*' config variables

 3:  dd9395bda32 = 3:  838aabf2858 completion: add and use __git_compute_first_level_config_vars_for_section
 4:  3e83f21eb4e = 4:  d442a039b27 builtin/help: add --config-all-for-completion
 5:  b41844cd86e = 5:  a2e792c911e completion: add an use __git_compute_second_level_config_vars_for_section

gitgitgadget

From: Philippe Blain <[email protected]>

In be6444d1ca (completion: bash: add correct suffix in variables, 2021-08-16), __git_complete_config_variable_name was changed to use "${sfx- }" instead of "$sfx" as the fourth argument of _gitcomp_nl and _gitcomp_nl_append, such that this argument evaluates to a space if sfx is unset. This was to ensure that e.g.

git config branch.autoSetupMe[TAB]

correctly completes to 'branch.autoSetupMerge ' with the trailing space. This commits notes that the fix only works in Bash 4 because in Bash 3 the 'local sfx' construct at the beginning of __git_complete_config_variable_name creates an empty string.

Make the fix also work for Bash 3 by using the "unset or null' parameter expansion syntax ("${sfx:- }"), such that the parameter is also expanded to a space if it is set but null, as is the behaviour of 'local sfx' in Bash 3.

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

index 6662db221df..159a4fd8add 100644
@@ -2750,7 +2750,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2784,7 +2784,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__git_compute_all_commands
-		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	remote.*.*)

@@ -2800,7 +2800,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

gitgitgadget

From: Philippe Blain <[email protected]>

In the Bash completion script, function __git_complete_config_variable_name completes config variables and has special logic to deal with config variables involving user-defined names, like branch.<name>.* and remote.<name>.*.

This special logic is missing for submodule-related config variables. Add the appropriate branches to the case statement, making use of the in-tree '.gitmodules' to list relevant submodules.

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

index 159a4fd8add..8af9bc3f4e1 100644
@@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
+	submodule.*.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_##*.}"
+		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
+		return
+		;;
+	submodule.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_#*.}"
+		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
+		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		return
+		;;
 	url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"

gitgitgadget

Hi Patrick,

Le 2024-02-08 à 02:42, Patrick Steinhardt a écrit :

OK, I end up testing them in 3/5 via the __git_compute_first_level_config_vars_for_section function I'm adding. But it's true I could add the test directly
in 2/5, if it makes more sense.

Thanks for your review !

Philippe.

From: Philippe Blain <[email protected]>

The function __git_complete_config_variable_name in the Bash completion script hardcodes several config variable names. These variables are those in config section where user-defined names can appear, such as "branch.<name>". These sections are treated first by the case statement, and the two last "catch all" cases are used for other sections, making use of the __git_compute_config_vars and __git_compute_config_sections function, which omit listing any variables containing wildcards or placeholders. Having hardcoded config variables introduces the risk of the completion code becoming out of sync with the actual config variables accepted by Git.

To avoid these hardcoded config variables, introduce a new function, __git_compute_first_level_config_vars_for_section, making use of the existing __git_config_vars variable. This function takes as argument a config section name and computes the matching "first level" config variables for that section, i.e. those _not_ containing any placeholder, like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this function and the variables it defines in the 'branch.*', 'remote.*' and 'submodule.*' switches of the case statement instead of hardcoding the corresponding config variables. Note that we use indirect expansion instead of associative arrays because those are not supported in Bash 3, on which macOS is stuck for licensing reasons.

Add a test to make sure the new function works correctly by verfying it lists all 'submodule' config variables. This has the downside that this test must be updated when new 'submodule' configuration are added, but this should be a small burden since it happens infrequently.

---
 contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
 t/t9902-completion.sh                  | 11 +++++++++++
 2 files changed, 32 insertions(+), 3 deletions(-)

index 8af9bc3f4e1..2934ceb7637 100644
@@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_compute_first_level_config_vars_for_section ()
+{
+	section="$1"
+	__git_compute_config_vars
+	local this_section="__git_first_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
 	branch.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	submodule.*.*)

@@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
-		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

index 35eb534fdda..f28d8f531b7 100755
@@ -2583,6 +2583,17 @@ test_expect_success 'git config - variable name include' '
 	EOF
 '
 
+test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
+	test_completion "git config submodule." <<-\EOF
+	submodule.active Z
+	submodule.alternateErrorStrategy Z
+	submodule.alternateLocation Z
+	submodule.fetchJobs Z
+	submodule.propagateBranches Z
+	submodule.recurse Z
+	EOF
+'
+
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF
 	false Z

gitgitgadget

Le 2024-02-08 à 02:42, Patrick Steinhardt a écrit :

Good eyes, I'll fix that in v3.

Yes, I used this pattern because it was already used for other variables in the script, __git_config_vars and __git_config_sections are some examples,

    git grep 'test -n .* ||' contrib/completion/git-completion.bash

finds also others. I think the idea is to cache these lists to avoid computing them everytime they are needed (probably most useful on Windows where process creation is longer). I'll mention that in the commit message.

Yes that would be more complete indeed, but it would then make more sense to add that test in 2/5 since __git_compute_first_level_config_vars_for_section is not involved in determining submodule names.

I'll make that change, thanks.

Philippe.

A silly question (primarily because I do not much use the indirect reference construct ${!name}). Does the assignment with printf need to spell out the long variable name with "_${section}"? Can it be

    printf -v "$this_section" ...

instead, as we already have the short-hand for it?

Yup, as long as the contents of the list stays stable (e.g., list of Git subcommands, list of options a Git subcommand takes, list of configuration variable names that do not have end-user customization part, etc.), it is a viable optimization technique. The available <slot> for color.branch.<slot> and color.diff.<slot> do not change (unless you talk about new version of Git adding support for more slots) and is a good idea to cache. remote.<name>.url takes its <name> component out of an unbound set of end-user controlled names, so unless we somehow have a method to invalidate cached values, the list can go stale as remotes are added and removed.

Thanks.

Hi Junio,

Le 2024-02-10 à 12:15, Junio C Hamano a écrit :

No, unfortunately neither "$this_section" nor "${!this_section}" work, so we must use the long name.

Indeed. Here I'm caching Git config variables, not any user-defined names, so these are stable.

Thanks,

Philippe.

Hmph, this does not match my experiment, though. What am I doing wrong?

bash$ vname=foo
bash$ foo=bar
bash$ set | grep foo
foo=bar
vname=foo
bash$ printf -v "$vname" "%d" 1234
bash$ set | grep foo
foo=1234
vname=foo
bash$ echo $BASH_VERSION
5.2.21(1)-release

From: Philippe Blain <[email protected]>

There is currently no machine-friendly way to show _all_ configuration variables from the command line. 'git help --config' does show them all, but it also sets up the pager. 'git help --config-for-completion' omits some variables (those containing wildcards, for example) and 'git help --config-section-for-completion' shows only top-level section names.

In a following commit we will want to have access to a list of all configuration variables from the Bash completion script. As such, add a new mode for the command, HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, triggered by the new option '--config-all-for-completion'. In this mode, show all variables, just as HELP_ACTION_CONFIG, but do not set up the pager.

---
 builtin/help.c | 7 +++++++
 1 file changed, 7 insertions(+)

index dc1fbe2b986..dacaeb10bf4 100644
@@ -50,6 +50,7 @@ static enum help_action {
 	HELP_ACTION_DEVELOPER_INTERFACES,
 	HELP_ACTION_CONFIG_FOR_COMPLETION,
 	HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION,
+	HELP_ACTION_CONFIG_ALL_FOR_COMPLETION,
 } cmd_mode;
 
 static const char *html_path;

@@ -86,6 +87,8 @@ static struct option builtin_help_options[] = {
 		    HELP_ACTION_CONFIG_FOR_COMPLETION, PARSE_OPT_HIDDEN),
 	OPT_CMDMODE_F(0, "config-sections-for-completion", &cmd_mode, "",
 		    HELP_ACTION_CONFIG_SECTIONS_FOR_COMPLETION, PARSE_OPT_HIDDEN),
+	OPT_CMDMODE_F(0, "config-all-for-completion", &cmd_mode, "",
+		    HELP_ACTION_CONFIG_ALL_FOR_COMPLETION, PARSE_OPT_HIDDEN),
 
 	OPT_END(),
 };

@@ -670,6 +673,10 @@ int cmd_help(int argc, const char **argv, const char *prefix)
 		opt_mode_usage(argc, "--config-for-completion", help_format);
 		list_config_help(SHOW_CONFIG_VARS);
 		return 0;
+	case HELP_ACTION_CONFIG_ALL_FOR_COMPLETION:
+		opt_mode_usage(argc, "--config-all-for-completion", help_format);
+		list_config_help(SHOW_CONFIG_HUMAN);
+		return 0;
 	case HELP_ACTION_USER_INTERFACES:
 		opt_mode_usage(argc, "--user-interfaces", help_format);
 		list_user_interfaces_help();

gitgitgadget

You can invoke git --no-pager help --config so that Git does not set up the pager. Is there a reason why we can't use that?

Le 2024-02-08 à 02:42, Patrick Steinhardt a écrit :

I'm glad to say there is no reason we can't use that! I just did not think of it and dived straight into the C code. I'll use that in v3 and just drop this patch from the series.

Thanks!

Philippe.

From: Philippe Blain <[email protected]>

In a previous commit we removed some hardcoded config variable names from function __git_complete_config_variable_name in the completion script by introducing a new function, __git_compute_first_level_config_vars_for_section.

The remaining hardcoded config variables are "second level" configuration variables, meaning 'branch.<name>.upstream', 'remote.<name>.url', etc. where <name> is a user-defined name.

Making use of the new --config-all-for-completion flag to 'git help' introduced in the previous commit, add a new function, __git_compute_second_level_config_vars_for_section. This function takes as argument a config section name and computes the corresponding second-level config variables, i.e. those that contain a '<' which indicates the start of a placeholder. Note that as in __git_compute_first_level_config_vars_for_section added previsouly, we use indirect expansion instead of associative arrays to stay compatible with Bash 3 on which macOS is stuck for licensing reasons.

Use this new function and the variables it defines in __git_complete_config_variable_name to remove hardcoded config variables, and add a test to verify the new function. Use a single 'case' for all sections with second-level variables names, since the code for each of them is now exactly the same.

---
 contrib/completion/git-completion.bash | 71 ++++++++------------------
 t/t9902-completion.sh                  | 10 ++++
 2 files changed, 31 insertions(+), 50 deletions(-)

index 2934ceb7637..0e8fd63bfdb 100644
@@ -2596,6 +2596,13 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_config_vars_all=
+__git_compute_config_vars_all ()
+{
+	test -n "$__git_config_vars_all" ||
+	__git_config_vars_all="$(git help --config-all-for-completion)"
+}
+
 __git_compute_first_level_config_vars_for_section ()
 {
 	section="$1"

@@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
 	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
 }
 
+__git_compute_second_level_config_vars_for_section ()
+{
+	section="$1"
+	__git_compute_config_vars_all
+	local this_section="__git_second_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
 	done
 
 	case "$cur_" in
-	branch.*.*)
+	branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"
-		__gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
+		local section="${pfx%.*.}"
+		__git_compute_second_level_config_vars_for_section "${section}"
+		local this_section="__git_second_level_config_vars_for_section_${section}"
+		__gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
 		return
 		;;
 	branch.*)

@@ -2765,33 +2784,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	guitool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			argPrompt cmd confirm needsFile noConsole noRescan
-			prompt revPrompt revUnmerged title
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	difftool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	man.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	mergetool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path trustExitCode" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	pager.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2799,15 +2791,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	remote.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			url proxy fetch push mirror skipDefaultUpdate
-			receivepack uploadpack tagOpt pushurl
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2818,12 +2801,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	submodule.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2834,12 +2811,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	url.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	*.*)
 		__git_compute_config_vars
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"

index f28d8f531b7..24ff786b273 100755
@@ -2593,6 +2593,16 @@ test_expect_success 'git config - variable name - __git_compute_first_level_conf
 	submodule.recurse Z
 	EOF
 '
+test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
+	test_completion "git config branch.main." <<-\EOF
+	branch.main.description Z
+	branch.main.remote Z
+	branch.main.pushRemote Z
+	branch.main.merge Z
+	branch.main.mergeOptions Z
+	branch.main.rebase Z
+	EOF
+'
 
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF

gitgitgadget

[snip]

Le 2024-02-08 à 02:42, Patrick Steinhardt a écrit :

Thanks, fixed.

Fixed.

Thank you again for your review Patrick, much appreciated.

Philippe.

Neither rounds of this series unfortunately got any review. Comments from anybody interested in helping to improve completion scripts?

Thanks.

Well, I've spent some time with Bash completion recently, so let me give it a go. I was trying to avoid the dense Bash logic and thus shied away from reviewing it. But the end result of having less hardcoded values is quite nice indeed.

Patrick

Changes since v2:

* Moved the addition of the tests to 2/4, and tweaked 3/4 and 4/4 so they simply adjust the test names
* Added a test for user-defined submodule names, as suggested by Patrick
* Added more details in the commit message of 3/4 around the use of global variables as caches
* Slightly improved commit message wording and fixed typos
* Added 'local' where suggested
* Dropped 4/5 which modified 'git help', since it's not needed (thanks Patrick!)

Changes since v1:

* Corrected my email in PATCH 2/5 (sorry for the noise)

v1: This series removes hardcoded config variable names in the __git_complete_config_variable_name function, partly by adding a new mode to 'git help'. It also adds completion for 'submodule.*' config variables, which were previously missing.

I think it makes sense to do that in the same series since it's closely related, and splitting it would result in textual conflicts between both series if one does not build on top of the other, but I'm open to other suggestions.

Thanks,

Philippe.

Philippe Blain (4):
completion: add space after config variable names also in Bash 3
completion: complete 'submodule.*' config variables
completion: add and use
__git_compute_first_level_config_vars_for_section
completion: add and use
__git_compute_second_level_config_vars_for_section

 contrib/completion/git-completion.bash | 90 +++++++++++++-------------
 t/t9902-completion.sh                  | 29 +++++++++
 2 files changed, 75 insertions(+), 44 deletions(-)

base-commit: b50a608ba20348cb3dfc16a696816d51780e3f0f
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1660%2Fphil-blain%2Fcompletion-submodule-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1660/phil-blain/completion-submodule-config-v3
Pull-Request: https://github.com/git/git/pull/1660

Range-diff vs v2:

 1:  837d92a6c27 = 1:  837d92a6c27 completion: add space after config variable names also in Bash 3
 2:  426374ff9b3 ! 2:  6b75582ee35 completion: complete 'submodule.*' config variables

@@ Commit message
Add the appropriate branches to the case statement, making use of the in-tree '.gitmodules' to list relevant submodules.

+ Add corresponding tests in t9902-completion.sh, making sure we complete
+ both first level submodule config variables as well as second level
+ variables involving submodule names.
+

## contrib/completion/git-completion.bash ##
@@ contrib/completion/git-completion.bash: __git_complete_config_variable_name ()
url.*.*)
local pfx="${cur_%.*}."
cur_="${cur_##*.}"
+
+ ## t/t9902-completion.sh ##
+@@ t/t9902-completion.sh: test_expect_success 'git config - variable name include' '
+ EOF
+ '
+
++test_expect_success 'setup for git config submodule tests' '
++ test_create_repo sub &&
++ test_commit -C sub initial &&
++ git submodule add ./sub
++'
++
++test_expect_success 'git config - variable name - submodule' '
++ test_completion "git config submodule." <<-\EOF
++ submodule.active Z
++ submodule.alternateErrorStrategy Z
++ submodule.alternateLocation Z
++ submodule.fetchJobs Z
++ submodule.propagateBranches Z
++ submodule.recurse Z
++ submodule.sub.Z
++ EOF
++'
++
++test_expect_success 'git config - variable name - submodule names' '
++ test_completion "git config submodule.sub." <<-\EOF
++ submodule.sub.url Z
++ submodule.sub.update Z
++ submodule.sub.branch Z
++ submodule.sub.fetchRecurseSubmodules Z
++ submodule.sub.ignore Z
++ submodule.sub.active Z
++ EOF
++'
++
+ test_expect_success 'git config - value' '
+ test_completion "git config color.pager " <<-\EOF
+ false Z
3: 838aabf2858 ! 3: fb210325394 completion: add and use __git_compute_first_level_config_vars_for_section
@@ Commit message

The function __git_complete_config_variable_name in the Bash completion script hardcodes several config variable names. These variables are
- those in config section where user-defined names can appear, such as
+ those in config sections where user-defined names can appear, such as
"branch.<name>". These sections are treated first by the case statement, and the two last "catch all" cases are used for other sections, making use of the __git_compute_config_vars and __git_compute_config_sections
@@ Commit message
like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this function and the variables it defines in the 'branch.*', 'remote.*' and 'submodule.*' switches of the case statement instead of hardcoding the
- corresponding config variables. Note that we use indirect expansion
- instead of associative arrays because those are not supported in Bash 3,
- on which macOS is stuck for licensing reasons.
+ corresponding config variables. Note that we use indirect expansion to
+ create a variable for each section, instead of using a single
+ associative array indexed by section names, because associative arrays
+ are not supported in Bash 3, on which macOS is stuck for licensing
+ reasons.

- Add a test to make sure the new function works correctly by verfying it
- lists all 'submodule' config variables. This has the downside that this
- test must be updated when new 'submodule' configuration are added, but
- this should be a small burden since it happens infrequently.
+ Use the existing pattern in the completion script of using global
+ variables to cache the list of config variables for each section. The
+ rationale for such caching is explained in eaa4e6ee2a (Speed up bash
+ completion loading, 2009-11-17), and the current approach to using and
+ defining them via 'test -n' is explained in cf0ff02a38 (completion: work
+ around zsh option propagation bug, 2012-02-02).
+
+ Adjust the name of one of the tests added in the previous commit,
+ reflecting that it now also tests the new function.

@@ contrib/completion/git-completion.bash: __git_compute_config_vars ()

+__git_compute_first_level_config_vars_for_section ()
+{
-+ section="$1"
++ local section="$1"
+ __git_compute_config_vars
+ local this_section="__git_first_level_config_vars_for_section_${section}"
+ test -n "${!this_section}" ||
@@ contrib/completion/git-completion.bash: __git_complete_config_variable_name ()
url.*.*)

## t/t9902-completion.sh ##
-@@ t/t9902-completion.sh: test_expect_success 'git config - variable name include' '
- EOF
+@@ t/t9902-completion.sh: test_expect_success 'setup for git config submodule tests' '
+ git submodule add ./sub
'

-+test_expect_success 'git config - variable name - __git_compute_first_level_config_vars_for_section' '
-+ test_completion "git config submodule." <<-\EOF
-+ submodule.active Z
-+ submodule.alternateErrorStrategy Z
-+ submodule.alternateLocation Z
-+ submodule.fetchJobs Z
-+ submodule.propagateBranches Z
-+ submodule.recurse Z
-+ EOF
-+'
-+
- test_expect_success 'git config - value' '
- test_completion "git config color.pager " <<-\EOF
- false Z
+-test_expect_success 'git config - variable name - submodule' '
++test_expect_success 'git config - variable name - submodule and __git_compute_first_level_config_vars_for_section' '
+ test_completion "git config submodule." <<-\EOF
+ submodule.active Z
+ submodule.alternateErrorStrategy Z

 4:  d442a039b27 < -:  ----------- builtin/help: add --config-all-for-completion
 5:  a2e792c911e ! 4:  69fc02bb6b4 completion: add an use __git_compute_second_level_config_vars_for_section

@@ Metadata
Author: Philippe Blain <[email protected]>

## Commit message ##
- completion: add an use __git_compute_second_level_config_vars_for_section
+ completion: add and use __git_compute_second_level_config_vars_for_section

In a previous commit we removed some hardcoded config variable names from function __git_complete_config_variable_name in the completion script by
@@ Commit message
configuration variables, meaning 'branch.<name>.upstream', 'remote.<name>.url', etc. where <name> is a user-defined name.

- Making use of the new --config-all-for-completion flag to 'git help'
- introduced in the previous commit, add a new function,
- __git_compute_second_level_config_vars_for_section. This function takes
- as argument a config section name and computes the corresponding
- second-level config variables, i.e. those that contain a '<' which
- indicates the start of a placeholder. Note that as in
+ Making use of the new existing --config flag to 'git help', add a new
+ function, __git_compute_second_level_config_vars_for_section. This
+ function takes as argument a config section name and computes the
+ corresponding second-level config variables, i.e. those that contain a
+ '<' which indicates the start of a placeholder. Note that as in
__git_compute_first_level_config_vars_for_section added previsouly, we use indirect expansion instead of associative arrays to stay compatible with Bash 3 on which macOS is stuck for licensing reasons.

+ As explained in the previous commit, we use the existing pattern in the
+ completion script of using global variables to cache the list of
+ variables for each section.
+
Use this new function and the variables it defines in __git_complete_config_variable_name to remove hardcoded config variables, and add a test to verify the new function. Use a single 'case' for all sections with second-level variables names, since the code for each of them is now exactly the same.

+ Adjust the name of a test added in a previous commit to reflect that it
+ now tests the added function.
+

## contrib/completion/git-completion.bash ##
@@ contrib/completion/git-completion.bash: __git_compute_config_vars ()
+__git_compute_config_vars_all ()
+{
+ test -n "$__git_config_vars_all" ||
-+ __git_config_vars_all="$(git help --config-all-for-completion)"
++ __git_config_vars_all="$(git --no-pager help --config)"
+}
+
__git_compute_first_level_config_vars_for_section ()
{
- section="$1"
+ local section="$1"
@@ contrib/completion/git-completion.bash: __git_compute_first_level_config_vars_for_section ()
printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
}

+__git_compute_second_level_config_vars_for_section ()
+{
-+ section="$1"
++ local section="$1"
+ __git_compute_config_vars_all
+ local this_section="__git_second_level_config_vars_for_section_${section}"
+ test -n "${!this_section}" ||
@@ contrib/completion/git-completion.bash: __git_complete_config_variable_name ()
__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"

## t/t9902-completion.sh ##
-@@ t/t9902-completion.sh: test_expect_success 'git config - variable name - __git_compute_first_level_conf
- submodule.recurse Z
+@@ t/t9902-completion.sh: test_expect_success 'git config - variable name - submodule and __git_compute_fi
EOF
'
-+test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
-+ test_completion "git config branch.main." <<-\EOF
-+ branch.main.description Z
-+ branch.main.remote Z
-+ branch.main.pushRemote Z
-+ branch.main.merge Z
-+ branch.main.mergeOptions Z
-+ branch.main.rebase Z
-+ EOF
-+'

- test_expect_success 'git config - value' '
- test_completion "git config color.pager " <<-\EOF
+-test_expect_success 'git config - variable name - submodule names' ' ++test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
+ test_completion "git config submodule.sub." <<-\EOF
+ submodule.sub.url Z
+ submodule.sub.update Z

gitgitgadget

From: Philippe Blain <[email protected]>

In be6444d1ca (completion: bash: add correct suffix in variables, 2021-08-16), __git_complete_config_variable_name was changed to use "${sfx- }" instead of "$sfx" as the fourth argument of _gitcomp_nl and _gitcomp_nl_append, such that this argument evaluates to a space if sfx is unset. This was to ensure that e.g.

git config branch.autoSetupMe[TAB]

correctly completes to 'branch.autoSetupMerge ' with the trailing space. This commits notes that the fix only works in Bash 4 because in Bash 3 the 'local sfx' construct at the beginning of __git_complete_config_variable_name creates an empty string.

Make the fix also work for Bash 3 by using the "unset or null' parameter expansion syntax ("${sfx:- }"), such that the parameter is also expanded to a space if it is set but null, as is the behaviour of 'local sfx' in Bash 3.

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

index 6662db221df..159a4fd8add 100644
@@ -2750,7 +2750,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2784,7 +2784,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__git_compute_all_commands
-		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	remote.*.*)

@@ -2800,7 +2800,7 @@ __git_complete_config_variable_name ()
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx- }"
+		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

gitgitgadget

From: Philippe Blain <[email protected]>

In the Bash completion script, function __git_complete_config_variable_name completes config variables and has special logic to deal with config variables involving user-defined names, like branch.<name>.* and remote.<name>.*.

This special logic is missing for submodule-related config variables. Add the appropriate branches to the case statement, making use of the in-tree '.gitmodules' to list relevant submodules.

Add corresponding tests in t9902-completion.sh, making sure we complete both first level submodule config variables as well as second level variables involving submodule names.

---
 contrib/completion/git-completion.bash | 13 ++++++++++++
 t/t9902-completion.sh                  | 29 ++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

index 159a4fd8add..8af9bc3f4e1 100644
@@ -2803,6 +2803,19 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
+	submodule.*.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_##*.}"
+		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
+		return
+		;;
+	submodule.*)
+		local pfx="${cur_%.*}."
+		cur_="${cur_#*.}"
+		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
+		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		return
+		;;
 	url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"

index 35eb534fdda..23d0e71324c 100755
@@ -2583,6 +2583,35 @@ test_expect_success 'git config - variable name include' '
 	EOF
 '
 
+test_expect_success 'setup for git config submodule tests' '
+	test_create_repo sub &&
+	test_commit -C sub initial &&
+	git submodule add ./sub
+'
+
+test_expect_success 'git config - variable name - submodule' '
+	test_completion "git config submodule." <<-\EOF
+	submodule.active Z
+	submodule.alternateErrorStrategy Z
+	submodule.alternateLocation Z
+	submodule.fetchJobs Z
+	submodule.propagateBranches Z
+	submodule.recurse Z
+	submodule.sub.Z
+	EOF
+'
+
+test_expect_success 'git config - variable name - submodule names' '
+	test_completion "git config submodule.sub." <<-\EOF
+	submodule.sub.url Z
+	submodule.sub.update Z
+	submodule.sub.branch Z
+	submodule.sub.fetchRecurseSubmodules Z
+	submodule.sub.ignore Z
+	submodule.sub.active Z
+	EOF
+'
+
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF
 	false Z

gitgitgadget

From: Philippe Blain <[email protected]>

The function __git_complete_config_variable_name in the Bash completion script hardcodes several config variable names. These variables are those in config sections where user-defined names can appear, such as "branch.<name>". These sections are treated first by the case statement, and the two last "catch all" cases are used for other sections, making use of the __git_compute_config_vars and __git_compute_config_sections function, which omit listing any variables containing wildcards or placeholders. Having hardcoded config variables introduces the risk of the completion code becoming out of sync with the actual config variables accepted by Git.

To avoid these hardcoded config variables, introduce a new function, __git_compute_first_level_config_vars_for_section, making use of the existing __git_config_vars variable. This function takes as argument a config section name and computes the matching "first level" config variables for that section, i.e. those _not_ containing any placeholder, like 'branch.autoSetupMerge, 'remote.pushDefault', etc. Use this function and the variables it defines in the 'branch.*', 'remote.*' and 'submodule.*' switches of the case statement instead of hardcoding the corresponding config variables. Note that we use indirect expansion to create a variable for each section, instead of using a single associative array indexed by section names, because associative arrays are not supported in Bash 3, on which macOS is stuck for licensing reasons.

Use the existing pattern in the completion script of using global variables to cache the list of config variables for each section. The rationale for such caching is explained in eaa4e6ee2a (Speed up bash completion loading, 2009-11-17), and the current approach to using and defining them via 'test -n' is explained in cf0ff02a38 (completion: work around zsh option propagation bug, 2012-02-02).

Adjust the name of one of the tests added in the previous commit, reflecting that it now also tests the new function.

---
 contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
 t/t9902-completion.sh                  |  2 +-
 2 files changed, 22 insertions(+), 4 deletions(-)

index 8af9bc3f4e1..57a8da7ca1a 100644
@@ -2596,6 +2596,15 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_compute_first_level_config_vars_for_section ()
+{
+	local section="$1"
+	__git_compute_config_vars
+	local this_section="__git_first_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,8 +2758,11 @@ __git_complete_config_variable_name ()
 	branch.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_direct "$(__git_heads "$pfx" "$cur_" ".")"
-		__gitcomp_nl_append $'autoSetupMerge\nautoSetupRebase\n' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	guitool.*.*)

@@ -2799,8 +2811,11 @@ __git_complete_config_variable_name ()
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git_remotes)" "$pfx" "$cur_" "."
-		__gitcomp_nl_append "pushDefault" "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	submodule.*.*)

@@ -2812,8 +2827,11 @@ __git_complete_config_variable_name ()
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"
+		local section="${pfx%.}"
 		__gitcomp_nl "$(__git config -f "$(__git rev-parse --show-toplevel)/.gitmodules" --get-regexp 'submodule.*.path' | awk -F. '{print $2}')" "$pfx" "$cur_" "."
-		__gitcomp_nl_append $'alternateErrorStrategy\nfetchJobs\nactive\nalternateLocation\nrecurse\npropagateBranches' "$pfx" "$cur_" "${sfx:- }"
+		__git_compute_first_level_config_vars_for_section "${section}"
+		local this_section="__git_first_level_config_vars_for_section_${section}"
+		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
 	url.*.*)

index 23d0e71324c..8600b9e0dd9 100755
@@ -2589,7 +2589,7 @@ test_expect_success 'setup for git config submodule tests' '
 	git submodule add ./sub
 '
 
-test_expect_success 'git config - variable name - submodule' '
+test_expect_success 'git config - variable name - submodule and __git_compute_first_level_config_vars_for_section' '
 	test_completion "git config submodule." <<-\EOF
 	submodule.active Z
 	submodule.alternateErrorStrategy Z

gitgitgadget

From: Philippe Blain <[email protected]>

In a previous commit we removed some hardcoded config variable names from function __git_complete_config_variable_name in the completion script by introducing a new function, __git_compute_first_level_config_vars_for_section.

The remaining hardcoded config variables are "second level" configuration variables, meaning 'branch.<name>.upstream', 'remote.<name>.url', etc. where <name> is a user-defined name.

Making use of the new existing --config flag to 'git help', add a new function, __git_compute_second_level_config_vars_for_section. This function takes as argument a config section name and computes the corresponding second-level config variables, i.e. those that contain a '<' which indicates the start of a placeholder. Note that as in __git_compute_first_level_config_vars_for_section added previsouly, we use indirect expansion instead of associative arrays to stay compatible with Bash 3 on which macOS is stuck for licensing reasons.

As explained in the previous commit, we use the existing pattern in the completion script of using global variables to cache the list of variables for each section.

Use this new function and the variables it defines in __git_complete_config_variable_name to remove hardcoded config variables, and add a test to verify the new function. Use a single 'case' for all sections with second-level variables names, since the code for each of them is now exactly the same.

Adjust the name of a test added in a previous commit to reflect that it now tests the added function.

---
 contrib/completion/git-completion.bash | 71 ++++++++------------------
 t/t9902-completion.sh                  |  2 +-
 2 files changed, 22 insertions(+), 51 deletions(-)

index 57a8da7ca1a..87678a5bb36 100644
@@ -2596,6 +2596,13 @@ __git_compute_config_vars ()
 	__git_config_vars="$(git help --config-for-completion)"
 }
 
+__git_config_vars_all=
+__git_compute_config_vars_all ()
+{
+	test -n "$__git_config_vars_all" ||
+	__git_config_vars_all="$(git --no-pager help --config)"
+}
+
 __git_compute_first_level_config_vars_for_section ()
 {
 	local section="$1"

@@ -2605,6 +2612,15 @@ __git_compute_first_level_config_vars_for_section ()
 	printf -v "__git_first_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars" | grep -E "^${section}\.[a-z]" | awk -F. '{print $2}')"
 }
 
+__git_compute_second_level_config_vars_for_section ()
+{
+	local section="$1"
+	__git_compute_config_vars_all
+	local this_section="__git_second_level_config_vars_for_section_${section}"
+	test -n "${!this_section}" ||
+	printf -v "__git_second_level_config_vars_for_section_${section}" %s "$(echo "$__git_config_vars_all" | grep -E "^${section}\.<" | awk -F. '{print $3}')"
+}
+
 __git_config_sections=
 __git_compute_config_sections ()
 {

@@ -2749,10 +2765,13 @@ __git_complete_config_variable_name ()
 	done
 
 	case "$cur_" in
-	branch.*.*)
+	branch.*.*|guitool.*.*|difftool.*.*|man.*.*|mergetool.*.*|remote.*.*|submodule.*.*|url.*.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_##*.}"
-		__gitcomp "remote pushRemote merge mergeOptions rebase" "$pfx" "$cur_" "$sfx"
+		local section="${pfx%.*.}"
+		__git_compute_second_level_config_vars_for_section "${section}"
+		local this_section="__git_second_level_config_vars_for_section_${section}"
+		__gitcomp "${!this_section}" "$pfx" "$cur_" "$sfx"
 		return
 		;;
 	branch.*)

@@ -2765,33 +2784,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	guitool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			argPrompt cmd confirm needsFile noConsole noRescan
-			prompt revPrompt revUnmerged title
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	difftool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	man.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path" "$pfx" "$cur_" "$sfx"
-		return
-		;;
-	mergetool.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "cmd path trustExitCode" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	pager.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2799,15 +2791,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl "$__git_all_commands" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	remote.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "
-			url proxy fetch push mirror skipDefaultUpdate
-			receivepack uploadpack tagOpt pushurl
-			" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	remote.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2818,12 +2801,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	submodule.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "url update branch fetchRecurseSubmodules ignore active" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	submodule.*)
 		local pfx="${cur_%.*}."
 		cur_="${cur_#*.}"

@@ -2834,12 +2811,6 @@ __git_complete_config_variable_name ()
 		__gitcomp_nl_append "${!this_section}" "$pfx" "$cur_" "${sfx:- }"
 		return
 		;;
-	url.*.*)
-		local pfx="${cur_%.*}."
-		cur_="${cur_##*.}"
-		__gitcomp "insteadOf pushInsteadOf" "$pfx" "$cur_" "$sfx"
-		return
-		;;
 	*.*)
 		__git_compute_config_vars
 		__gitcomp "$__git_config_vars" "" "$cur_" "$sfx"

index 8600b9e0dd9..64031a9eff8 100755
@@ -2601,7 +2601,7 @@ test_expect_success 'git config - variable name - submodule and __git_compute_fi
 	EOF
 '
 
-test_expect_success 'git config - variable name - submodule names' '
+test_expect_success 'git config - variable name - __git_compute_second_level_config_vars_for_section' '
 	test_completion "git config submodule.sub." <<-\EOF
 	submodule.sub.url Z
 	submodule.sub.update Z

gitgitgadget

I ain't got anything else to add to this patch series. Thanks!

Patrick

Thanks, both. Let's mark it for 'next' so that it can be one of the topics to graduate first after the current cycle.