{"thread":{"id":"60513","subject":"[PATCH] completion: add and use the __git_get_config_subsection helper function","startedAt":"2023-11-13T22:26:04Z","lastAt":"2023-11-28T12:40:59Z","messageCount":3,"participants":["SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"484821","messageId":"20231113222528.62771-1-szeder.dev@gmail.com","threadId":"60513","inReplyTo":null,"subject":"[PATCH] completion: add and use the __git_get_config_subsection helper function","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2023-11-13T22:25:27Z","receivedAt":"2023-11-13T22:26:04Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Our Bash completion script recently learned to complete configured\ntrailer key aliases for 'git config --trailer=<TAB>', but the helper\nfunction extracting the key alias from 'git config's output, i.e. the\nsubsection from 'trailer.*.key', ended up more complex than necessary,\nwith considerable overhead from executing four external processes in a\npipe.\n\nReplace those commands in the pipe with a simple shell loop using only\na pair of parameter expansions and a builtin 'echo', which is easier\nto understand and should perform better (I assume that users don't\nhave that many subsections in any particular section to make the\nprocessing with an external process (let alone four) worth it).\n\nAnd while at it, let's extract this loop into a generic\n__git_get_config_subsections() helper function, as it might be useful\nelsewhere in the future as well (at the moment it isn't, AFAICT).\n\nSigned-off-by: SZEDER Gábor <szeder.dev@gmail.com>\n---\n contrib/completion/git-completion.bash | 14 +++++++++++++-\n t/t9902-completion.sh                  | 13 +++++++++++++\n 2 files changed, 26 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex 13a39ebd2e..34bbb66f85 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -1148,6 +1148,18 @@ __git_get_config_variables ()\n \tdone\n }\n \n+# Lists all subsections in the given section which contain the given\n+# config variable, with the section and variable names removed.\n+__git_get_config_subsections ()\n+{\n+\tlocal section=\"$1\" var=\"$2\" i IFS=$'\\n'\n+\tfor i in $(__git config --name-only --get-regexp \"^$section\\..*\\.$var$\"); do\n+\t\ti=${i#$section.}\n+\t\ti=${i%.$var}\n+\t\techo \"$i\"\n+\tdone\n+}\n+\n __git_pretty_aliases ()\n {\n \t__git_get_config_variables \"pretty\"\n@@ -1681,7 +1693,7 @@ __git_untracked_file_modes=\"all no normal\"\n \n __git_trailer_tokens ()\n {\n-\t__git config --name-only --get-regexp '^trailer\\..*\\.key$' | cut -d. -f 2- | rev | cut -d. -f2- | rev\n+\t__git_get_config_subsections trailer key\n }\n \n _git_commit ()\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex a7c3b4eb63..11ed83d0ed 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -2130,6 +2130,19 @@ test_expect_success '__git_get_config_variables' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '__git_get_config_subsections' '\n+\tcat >expect <<-\\EOF &&\n+\tsubsection-1\n+\tSubSection-2\n+\tsub.section.3\n+\tEOF\n+\ttest_config interesting.subsection-1.name good &&\n+\ttest_config Interesting.SubSection-2.Name good &&\n+\ttest_config interesting.sub.section.3.name good &&\n+\t__git_get_config_subsections interesting name >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '__git_pretty_aliases' '\n \tcat >expect <<-EOF &&\n \tauthor\n-- \n2.43.0.rc1.528.g8f9d60d041\n\n"},{"id":"484834","messageId":"xmqqzfzh16qp.fsf@gitster.g","threadId":"60513","inReplyTo":"20231113222528.62771-1-szeder.dev@gmail.com","subject":"Re: [PATCH] completion: add and use the __git_get_config_subsection helper function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-11-14T01:08:46Z","receivedAt":"2023-11-14T01:08:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> +# Lists all subsections in the given section which contain the given\n> +# config variable, with the section and variable names removed.\n> +__git_get_config_subsections ()\n> +{\n> +\tlocal section=\"$1\" var=\"$2\" i IFS=$'\\n'\n> +\tfor i in $(__git config --name-only --get-regexp \"^$section\\..*\\.$var$\"); do\n> +\t\ti=${i#$section.}\n> +\t\ti=${i%.$var}\n\nAs this script is allowed bash-isms, I wondered if we can use\na single pattern substitution instead of two remove pre/suffix\npattern substitution, but I guess it would not work, and the above\nis perfectly readable.\n\n> +\t\techo \"$i\"\n\nAs the subsection is designed to contain unbounded set of end-user\ncontrolled names, we probably should do\n\n\t\tprintf \"%s\\n\" \"$i\"\n\ninstead to protect us from interesting names (e.g. ones that begin\nwith a dash).\n\n> +\tdone\n> +}\n\nInteresting to see that we do not need to bother deduplicating the\noutput from here.\n\n> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> index a7c3b4eb63..11ed83d0ed 100755\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -2130,6 +2130,19 @@ test_expect_success '__git_get_config_variables' '\n>  \ttest_cmp expect actual\n>  '\n>  \n> +test_expect_success '__git_get_config_subsections' '\n> +\tcat >expect <<-\\EOF &&\n> +\tsubsection-1\n> +\tSubSection-2\n> +\tsub.section.3\n> +\tEOF\n> +\ttest_config interesting.subsection-1.name good &&\n> +\ttest_config Interesting.SubSection-2.Name good &&\n> +\ttest_config interesting.sub.section.3.name good &&\n> +\t__git_get_config_subsections interesting name >actual &&\n> +\ttest_cmp expect actual\n> +'\n\nGood to see an uppercase character is used here ;-).\n\nThanks.\n"},{"id":"485192","messageId":"20231128124054.GA1964@szeder.dev","threadId":"60513","inReplyTo":"xmqqzfzh16qp.fsf@gitster.g","subject":"Re: [PATCH] completion: add and use the __git_get_config_subsection helper function","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2023-11-28T12:40:54Z","receivedAt":"2023-11-28T12:40:59Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Nov 14, 2023 at 10:08:46AM +0900, Junio C Hamano wrote:\n> SZEDER Gábor <szeder.dev@gmail.com> writes:\n> \n> > +# Lists all subsections in the given section which contain the given\n> > +# config variable, with the section and variable names removed.\n> > +__git_get_config_subsections ()\n> > +{\n> > +\tlocal section=\"$1\" var=\"$2\" i IFS=$'\\n'\n> > +\tfor i in $(__git config --name-only --get-regexp \"^$section\\..*\\.$var$\"); do\n> > +\t\ti=${i#$section.}\n> > +\t\ti=${i%.$var}\n> \n> As this script is allowed bash-isms, I wondered if we can use\n> a single pattern substitution instead of two remove pre/suffix\n> pattern substitution, but I guess it would not work, and the above\n> is perfectly readable.\n\nYeah, I don't think it's possible to remove the prefix and suffix with\nBash builtins in a single operation.\n\n> > +\t\techo \"$i\"\n> \n> As the subsection is designed to contain unbounded set of end-user\n> controlled names, we probably should do\n> \n> \t\tprintf \"%s\\n\" \"$i\"\n> \n> instead to protect us from interesting names (e.g. ones that begin\n> with a dash).\n\nIndeed, will do.\n\n> > +\tdone\n> > +}\n> \n> Interesting to see that we do not need to bother deduplicating the\n> output from here.\n\nBash will sort and deduplicate the completion words anyway, so we\ndon't have to.  Sometimes we do deduplicate them, though, but either\nto make testing easier or for performance reasons; in this case\nneither of them applies.\n\n> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\n> > index a7c3b4eb63..11ed83d0ed 100755\n> > --- a/t/t9902-completion.sh\n> > +++ b/t/t9902-completion.sh\n> > @@ -2130,6 +2130,19 @@ test_expect_success '__git_get_config_variables' '\n> >  \ttest_cmp expect actual\n> >  '\n> >  \n> > +test_expect_success '__git_get_config_subsections' '\n> > +\tcat >expect <<-\\EOF &&\n> > +\tsubsection-1\n> > +\tSubSection-2\n> > +\tsub.section.3\n> > +\tEOF\n> > +\ttest_config interesting.subsection-1.name good &&\n> > +\ttest_config Interesting.SubSection-2.Name good &&\n> > +\ttest_config interesting.sub.section.3.name good &&\n> > +\t__git_get_config_subsections interesting name >actual &&\n> > +\ttest_cmp expect actual\n> > +'\n> \n> Good to see an uppercase character is used here ;-).\n\nThat's just for good measure, but not really necessasry here, as that\nprimarily tests that 'git config' lists the section and variable names\n(but not the subsection!) normalized to lowercase, no matter what\nCaMeLCase the user might have written them.\n\n"}]}