Re: [PATCH v2 3/5] completion: add and use __git_compute_first_level_config_vars_for_section

> On Mon, Jan 29, 2024 at 01:27:59PM +0000, Philippe Blain via GitGitGadget wrote:
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> 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.
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  contrib/completion/git-completion.bash | 24 +++++++++++++++++++++---
>>  t/t9902-completion.sh                  | 11 +++++++++++
>>  2 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 8af9bc3f4e1..2934ceb7637 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -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"
> 
> Section needs to be `local`, right?

>> +	__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}')"
>> +}
> 
> I've been wondering a bit why we store the result in a global variable.
> The value certainly isn't reused in the completion scripts here. It took
> me quite some time to realize though that it's going to end up in the
> user's shell environment even after completion finishes so that it can
> be reused the next time we invoke the completion function.
> 
> While this does feel a tad weird to me to be stateful like this across
> completion calls, we use the same pattern for `__git_config_vars` and
> `__git_config_sections`. So I guess it should be fine given that there
> is precedent.

>>  __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.*.*)
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 35eb534fdda..f28d8f531b7 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -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
>> +'
>> +
> 
> Shouldn't we verify that we know to complete both first-level config
> vars as well as the user-specified submodule names here?