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

From: Jeff King <peff@peff.net>
Date: Jul 14, 2022, 21:13 UTC
Message-ID: <YtCG/xVfWKsiSXUp@coredump.intra.peff.net>
In-Reply-To: <d21115d5-8bae-c120-453f-9dbb600d0431@gmail.com>

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

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

Show 7 quoted lines

> > 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.

Show 16 quoted lines

> > 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.