[PATCH] config.txt: add completion for include, includeIf

[PATCH] config.txt: add completion for include, includeIf

[Manuel Boni via GitGitGadget]

From: Manuel Boni <ziosombrero@gmail.com>

Git config's tab completion does not yet know about the "include"
and "includeIf" sections, nor the related "path" variable.

Add tab completion support for the aforementioned items,
along with two new tests, based on the existing ones,
specifically for this completion. Variable completion only works
for "include" for now.

Credit for the ideas behind this patch goes to
Ævar Arnfjörð Bjarmason.

Signed-off-by: Manuel Boni <ziosombrero@gmail.com>
Based-on-patch-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
    config.txt: add completion for include, includeIf

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1285%2Fziosombrero%2Fcomp-config-include-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1285/ziosombrero/comp-config-include-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1285

 Documentation/config/includeif.txt |  6 ++++++
 t/t9902-completion.sh              | 13 +++++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 Documentation/config/includeif.txt

diff --git a/Documentation/config/includeif.txt b/Documentation/config/includeif.txt
new file mode 100644
index 00000000000..18248cf462e
--- /dev/null
+++ b/Documentation/config/includeif.txt
@@ -0,0 +1,6 @@
+include.path::
+
+includeIf.<condition>.path::
+	Special variables to include other configuration files. See
+	the "CONFIGURATION FILE" section in the main
+	linkgit:git-config[1] documentation.
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 31526e6b641..43de868b800 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2485,6 +2485,13 @@ test_expect_success 'git config - section' '
 	EOF
 '
 
+test_expect_success 'git config - section include, includeIf' '
+	test_completion "git config inclu" <<-\EOF
+	include.Z
+	includeIf.Z
+	EOF
+'
+
 test_expect_success 'git config - variable name' '
 	test_completion "git config log.d" <<-\EOF
 	log.date Z
@@ -2493,6 +2500,12 @@ test_expect_success 'git config - variable name' '
 	EOF
 '
 
+test_expect_success 'git config - variable name include' '
+	test_completion "git config include.p" <<-\EOF
+	include.path Z
+	EOF
+'
+
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF
 	false Z

base-commit: 30cc8d0f147546d4dd77bf497f4dec51e7265bd8
-- 
gitgitgadget

Re: [PATCH] config.txt: add completion for include, includeIf

[Jeff King]

On Wed, Jul 13, 2022 at 07:02:16PM +0000, Manuel Boni via GitGitGadget wrote:

> diff --git a/Documentation/config/includeif.txt b/Documentation/config/includeif.txt
> new file mode 100644
> index 00000000000..18248cf462e
> --- /dev/null
> +++ b/Documentation/config/includeif.txt
> @@ -0,0 +1,6 @@
> +include.path::
> +
> +includeIf.<condition>.path::
> +	Special variables to include other configuration files. See
> +	the "CONFIGURATION FILE" section in the main
> +	linkgit:git-config[1] documentation.

Heh, the subject says "add completion" but the meat of this patch is
really "add documentation". I know your ulterior motive is the former,
but we should note this affects the latter just as much.

That said, I think it's an improvement there, too. It helps not only the
completion, but also humans looking in the variable list. And it does
the right thing by pointing to the existing documentation, rather than
trying to come up with a new description. So I like the direction. I
have one fix and one suggestion, though.

First the suggestion: rather than just point them to the "CONFIGURATION
FILE" section, there are subsections for "Includes" and "Conditional
Includes". Maybe worth pointing to those specific sections?

And the fix: this won't change the documentation unless you also mention
the new config/includeif.txt file from the main config.txt, like:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e376d547ce..5b5b976569 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -445,6 +445,8 @@ include::config/i18n.txt[]
 
 include::config/imap.txt[]
 
+include::config/includeif.txt[]
+
 include::config/index.txt[]
 
 include::config/init.txt[]

Without that, I don't think the completion will find it either (at least
your tests did not seem to pass for me until I added it).

-Peff

Re: [PATCH] config.txt: add completion for include, includeIf

[Junio C Hamano]

"Manuel Boni via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Manuel Boni <ziosombrero@gmail.com>
>
> Git config's tab completion does not yet know about the "include"
> and "includeIf" sections, nor the related "path" variable.
>
> Add tab completion support for the aforementioned items,
> along with two new tests, based on the existing ones,
> specifically for this completion. Variable completion only works
> for "include" for now.

You may have started this work for tab completion, but I think you
should describe it as adding missing documentation (this comment
extends to the title of the patch, too).  The work to teach "tab
completion" about the variables "git config" can take has done long
time ago, and the work by this patch to add description for
<include> and <includeIf> is used as an input to that previous work,
which results in "git config inc<TAB>" to be command line completed.

> Credit for the ideas behind this patch goes to
> Ævar Arnfjörð Bjarmason.
>
> Signed-off-by: Manuel Boni <ziosombrero@gmail.com>
> Based-on-patch-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>

The order of these two is backwards, no?  Ævar may have written
something that inspire you to arrive at this version, which you
signed off before sending it out.

> +include.path::
> +
> +includeIf.<condition>.path::

Losing the extra blank line will make it clearer, if your intention
is that the next lines apply to both of the above two.

> +	Special variables to include other configuration files. See
> +	the "CONFIGURATION FILE" section in the main
> +	linkgit:git-config[1] documentation.

Thanks.

Re: [PATCH] config.txt: add completion for include, includeIf

[Jeff King]

[+cc git@vger; please keep the discussion on-list]

On Thu, Jul 14, 2022 at 09:37:55AM +0200, Manuel Boni wrote:

> > First the suggestion: rather than just point them to the "CONFIGURATION
> > FILE" section, there are subsections for "Includes" and "Conditional
> > Includes". Maybe worth pointing to those specific sections?
> 
> Is there specific syntax that I should use to directly point to those
> sections? Or should I simply add something such as "Refer to the 'Includes'
> and 'Conditional Includes' sections"?

Sadly, no, I don't think we can hyperlink to sections easily with
asciidoc. I think we will reliably create an "_includes" id for the
subsection in the HTML output, but no portable way to create a link.
Doing <a href="git-config.html#_includes"> works for HTML, but you'd
want to omit that from the man output (and need to structure the text to
make sense without the hyperlink).

That's roughly what our linkgit: macro does, but I don't think we have
an equivalent for subsections themselves.

So for now, yeah, I think something like:

  See the 'Includes' and 'Conditional Includes' headings in the
  "CONFIGURATION FILE" section of the main linkgit:git-config[1]
  documentation.

or something similar.

> > diff --git a/Documentation/config.txt b/Documentation/config.txt
> > index e376d547ce..5b5b976569 100644
> > --- a/Documentation/config.txt
> > +++ b/Documentation/config.txt
> > @@ -445,6 +445,8 @@ include::config/i18n.txt[]
> >   include::config/imap.txt[]
> > +include::config/includeif.txt[]
> > +
> >   include::config/index.txt[]
> >   include::config/init.txt[]
> > 
> > Without that, I don't think the completion will find it either (at least
> > your tests did not seem to pass for me until I added it).
> 
> Strange, the tests worked fine for me even without that line, but I will add
> it anyway.

Ah, sorry, I was holding it wrong. I hadn't run "make" to build
"config-list.h" when I was testing before. The generate-configlist.sh
script just globs Documentation/config/*.txt, so it works as soon as you
run "make"[1].

So in that sense your commit message _is_ accurate, because it doesn't
affect the documentation at all. But IMHO it is worth including the new
entries there.

-Peff

[1] There's an interesting subtlety there. The Makefile rule for
    config-list.h depends on config/*.txt, too. So it picks up the new
    file correctly, but it fails to notice if you then switch away from
    your commit back to vanilla "master". It thinks config-list.h
    doesn't need to be rebuilt! Probably not worth worrying about too
    much as we don't often remove files, and certainly not something
    that needs to be dealt with for your change.

[PATCH v2] config.txt: document include, includeIf

[Manuel Boni via GitGitGadget]

From: Manuel Boni <ziosombrero@gmail.com>

Git config's tab completion does not yet know about the "include"
and "includeIf" sections, nor the related "path" variable.

Add a description for these two sections in
'Documentation/config/includeif.txt', which points to git-config's
documentation, specifically the "Includes" and "Conditional Includes"
subsections.

As a side effect, tab completion can successfully complete the
'include', 'includeIf', and 'include.add' expressions.
This effect is tested by two new ad-hoc tests.
Variable completion only works for "include" for now.

Credit for the ideas behind this patch goes to
Ævar Arnfjörð Bjarmason.

Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Manuel Boni <ziosombrero@gmail.com>
---
    config.txt: add completion for include, includeIf
    
    CC: Ævar Arnfjörð Bjarmason avarab@gmail.com cc: Jeff King peff@peff.net
    
    Update 2022-07-16: I addressed the suggestions by Jeff King and Junio C.
    Hamano by integrating their fixes and by rewording the commit message,
    so that emphasis is put on the documentation enhancement and mentioning
    the improved tab completion as a beneficial side effect.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1285%2Fziosombrero%2Fcomp-config-include-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1285/ziosombrero/comp-config-include-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1285

Range-diff vs v1:

 1:  79d877bcd6c ! 1:  38b1860020f config.txt: add completion for include, includeIf
     @@ Metadata
      Author: Manuel Boni <ziosombrero@gmail.com>
      
       ## Commit message ##
     -    config.txt: add completion for include, includeIf
     +    config.txt: document include, includeIf
      
          Git config's tab completion does not yet know about the "include"
          and "includeIf" sections, nor the related "path" variable.
      
     -    Add tab completion support for the aforementioned items,
     -    along with two new tests, based on the existing ones,
     -    specifically for this completion. Variable completion only works
     -    for "include" for now.
     +    Add a description for these two sections in
     +    'Documentation/config/includeif.txt', which points to git-config's
     +    documentation, specifically the "Includes" and "Conditional Includes"
     +    subsections.
     +
     +    As a side effect, tab completion can successfully complete the
     +    'include', 'includeIf', and 'include.add' expressions.
     +    This effect is tested by two new ad-hoc tests.
     +    Variable completion only works for "include" for now.
      
          Credit for the ideas behind this patch goes to
          Ævar Arnfjörð Bjarmason.
      
     +    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
          Signed-off-by: Manuel Boni <ziosombrero@gmail.com>
     -    Based-on-patch-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     +
     + ## Documentation/config.txt ##
     +@@ Documentation/config.txt: include::config/i18n.txt[]
     + 
     + include::config/imap.txt[]
     + 
     ++include::config/includeif.txt[]
     ++
     + include::config/index.txt[]
     + 
     + include::config/init.txt[]
      
       ## Documentation/config/includeif.txt (new) ##
      @@
      +include.path::
     -+
      +includeIf.<condition>.path::
      +	Special variables to include other configuration files. See
      +	the "CONFIGURATION FILE" section in the main
     -+	linkgit:git-config[1] documentation.
     ++	linkgit:git-config[1] documentation,
     ++	specifically the "Includes" and "Conditional Includes" subsections.
     ++	
      
       ## t/t9902-completion.sh ##
      @@ t/t9902-completion.sh: test_expect_success 'git config - section' '


 Documentation/config.txt           |  2 ++
 Documentation/config/includeif.txt |  7 +++++++
 t/t9902-completion.sh              | 13 +++++++++++++
 3 files changed, 22 insertions(+)
 create mode 100644 Documentation/config/includeif.txt

diff --git a/Documentation/config.txt b/Documentation/config.txt
index e376d547ce0..5b5b9765699 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -445,6 +445,8 @@ include::config/i18n.txt[]
 
 include::config/imap.txt[]
 
+include::config/includeif.txt[]
+
 include::config/index.txt[]
 
 include::config/init.txt[]
diff --git a/Documentation/config/includeif.txt b/Documentation/config/includeif.txt
new file mode 100644
index 00000000000..28b6a52d9f5
--- /dev/null
+++ b/Documentation/config/includeif.txt
@@ -0,0 +1,7 @@
+include.path::
+includeIf.<condition>.path::
+	Special variables to include other configuration files. See
+	the "CONFIGURATION FILE" section in the main
+	linkgit:git-config[1] documentation,
+	specifically the "Includes" and "Conditional Includes" subsections.
+	
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 31526e6b641..43de868b800 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2485,6 +2485,13 @@ test_expect_success 'git config - section' '
 	EOF
 '
 
+test_expect_success 'git config - section include, includeIf' '
+	test_completion "git config inclu" <<-\EOF
+	include.Z
+	includeIf.Z
+	EOF
+'
+
 test_expect_success 'git config - variable name' '
 	test_completion "git config log.d" <<-\EOF
 	log.date Z
@@ -2493,6 +2500,12 @@ test_expect_success 'git config - variable name' '
 	EOF
 '
 
+test_expect_success 'git config - variable name include' '
+	test_completion "git config include.p" <<-\EOF
+	include.path Z
+	EOF
+'
+
 test_expect_success 'git config - value' '
 	test_completion "git config color.pager " <<-\EOF
 	false Z

base-commit: 9dd64cb4d310986dd7b8ca7fff92f9b61e0bd21a
-- 
gitgitgadget

Re: [PATCH v2] config.txt: document include, includeIf

[Jeff King]

On Sat, Jul 16, 2022 at 08:13:43PM +0000, Manuel Boni via GitGitGadget wrote:

>     Update 2022-07-16: I addressed the suggestions by Jeff King and Junio C.
>     Hamano by integrating their fixes and by rewording the commit message,
>     so that emphasis is put on the documentation enhancement and mentioning
>     the improved tab completion as a beneficial side effect.

Thanks. This version looks OK to me.

-Peff