Re: [PATCH v2 2/5] completion: complete 'submodule.*' config variables

[Philippe Blain <levraiphilippeblain@gmail.com>]

Hi Patrick,

Le 2024-02-08 à 02:42, Patrick Steinhardt a écrit :
> On Mon, Jan 29, 2024 at 01:27:58PM +0000, Philippe Blain via GitGitGadget wrote:
>> From: Philippe Blain <levraiphilippeblain@gmail.com>
>>
>> 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.
>>
>> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
>> ---
>>  contrib/completion/git-completion.bash | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> index 159a4fd8add..8af9bc3f4e1 100644
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -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
>> +		;;
> 
> Hm, it feels quite awkward that we have to manually massage the
> gitmodules config like this. But the closest tool I could find is
> `git submodule status`, which would also end up describing commits in
> each of the submodules and thus do needless work. And second, it prints
> submodule paths and not submodule names, so it surfaces the wrong info
> in the first place.
> 
> Ideally, we would create such a tool that makes the information more
> accessible to us. But that certainly seems out of scope of this patch
> series.
> 
> In any case though it would be nice to add some tests for these new
> completions.

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.