| Message ID | 838aabf2858b73361be8e8579bc80826e1cfd4c3.1706534882.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: remove hardcoded config variable names | expand |
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? Patrick > 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 : > 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? Good eyes, I'll fix that in v3. >> + __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. 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. >> __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? 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.
Philippe Blain <levraiphilippeblain@gmail.com> writes: >>> + __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}')" >>> +} 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? > 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. 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 : > Philippe Blain <levraiphilippeblain@gmail.com> writes: > >>>> + __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}')" >>>> +} > > 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? No, unfortunately neither "$this_section" nor "${!this_section}" work, so we must use the long name. > >> 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. > > 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. Indeed. Here I'm caching Git config variables, not any user-defined names, so these are stable. Thanks, Philippe.
Philippe Blain <levraiphilippeblain@gmail.com> writes: >> 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? > > No, unfortunately neither "$this_section" nor "${!this_section}" > work, so we must use the long name. 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
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" + __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.*.*) 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 +' + test_expect_success 'git config - value' ' test_completion "git config color.pager " <<-\EOF false Z