| Message ID | 426374ff9b3820512f73ef094f9533e6a1ea5cad.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: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. Patrick > url.*.*) > local pfx="${cur_%.*}." > cur_="${cur_##*.}" > -- > gitgitgadget > >
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.
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 + ;; url.*.*) local pfx="${cur_%.*}." cur_="${cur_##*.}"