Re: [PATCH] doc: substitute ETC_GIT(CONFIG|ATTRIBUTES) in generated docs

From: Todd Zullinger <tmz@pobox.com>
Date: Jun 27, 2018, 15:03 UTC
Message-ID: <20180627150352.GJ20217@zaya.teonanacatl.net>
In-Reply-To: <20180627141430.GA13904@sigill.intra.peff.net>

Jeff King wrote:

Show 7 quoted lines

> On Wed, Jun 27, 2018 at 12:56:37AM -0400, Todd Zullinger wrote:
> 
>> Replace `$(prefix)/etc/gitconfig` and `$(prefix)/etc/gitattributes` in
>> generated documentation with the paths chosen when building.  Readers of
>> the documentation should not need to know how `$(prefix)` was defined.
> 
> Yes, I was just complaining about this yesterday.

So what you're saying is that if I had procrastinated a little, you may have written such a patch for me? :)

Show 14 quoted lines

>                                                   Besides readers not
> having any clue what $(prefix) means here, $(prefix)/etc is not even
> correct for builds with prefix=/usr.
>
> So I like the overall direction here, but it leaves me with one
> question: what happens for documentation outside of customized builds?
> 
> Specifically, I'm thinking of:
> 
>   1. The pre-built documentation that Junio builds for
>      quick-install-doc. This _could_ be customized during the "quick"
>      step, but it's probably not worth the effort. However, we'd want
>      some kind of generic fill-in then, and hopefully not
>      "/home/jch/etc" or something.

If building docs separately for such a use, the values can be overridden by setting ETC_GITCONFIG and ETC_GITATTRIBUTES (or prefix or sysconfig). To keep the same values as we currently use, for example:

    make -C Documentation V=1 \
            ETC_GITCONFIG='$$(prefix)/etc/gitconfig' \
            ETC_GITATTRIBUTES='$$(prefix)/etc/gitattributes'

I don't know if that's sufficient for folks who build documentation to share with a general audience or not.

It might be enough if the default values are relatively sane and consistent. That would be a slight improvement over the current situation still.

Show 5 quoted lines

>   2. The manpages on git-scm.com, which are built with asciidoctor. I
>      think we'd want the same generic value there. Ideally it would be
>      embedded in the asciidoc source as "if this attribute isn't
>      defined, then use this", but it's not the end of the world to
>      require a patch to the site to handle this.

We have that for the DEFAULT_(EDITOR|PAGER). I didn't know if we'd want that here, but maybe it's worth the effort if it helps when building docs destined for the website and such. It might make the plain text files a bit less readable though.

The EDITOR/PAGER bits are in git-var.txt, like this:

    GIT_EDITOR::
        Text editor for use by Git commands.  The value is meant to be
        interpreted by the shell when it is used.  Examples: `~/bin/vi`,
        `$SOME_ENVIRONMENT_VARIABLE`, `"C:\Program Files\Vim\gvim.exe"
        --nofork`.  The order of preference is the `$GIT_EDITOR`
        environment variable, then `core.editor` configuration, then
        `$VISUAL`, then `$EDITOR`, and then the default chosen at compile
        time, which is usually 'vi'.
    ifdef::git-default-editor[]
        The build you are using chose '{git-default-editor}' as the default.
    endif::git-default-editor[]

The ifdef would be a little different to set the var if it was not set, of course.

I think if we ensure that ETC_GITCONFIG / ETC_GITATTRIBUTES are set sanely by default (and easily overridden) then we can probably avoid the need to handle it within the asciidoc file though. (There's more on that though below.)

>      (Related, there's a build target in the local Makefile for using
>      asciidoctor; does it need updated, too?)

I didn't test asciidoctor specficially, but it also respects the ASCIIDOC_EXTRA parameters, so I think it will work just as well. I'll try to confirm that later today.

Show 34 quoted lines

>> diff --git a/Documentation/Makefile b/Documentation/Makefile
>> index d079d7c73a..75af671798 100644
>> --- a/Documentation/Makefile
>> +++ b/Documentation/Makefile
>> @@ -95,6 +95,7 @@ DOC_MAN7 = $(patsubst %.txt,%.7,$(MAN7_TXT))
>>  
>>  prefix ?= $(HOME)
>>  bindir ?= $(prefix)/bin
>> +sysconfdir ?= $(prefix)/etc
>>  htmldir ?= $(prefix)/share/doc/git-doc
>>  infodir ?= $(prefix)/share/info
>>  pdfdir ?= $(prefix)/share/doc/git-doc
>> @@ -205,6 +206,18 @@ DEFAULT_EDITOR_SQ = $(subst ','\'',$(DEFAULT_EDITOR))
>>  ASCIIDOC_EXTRA += -a 'git-default-editor=$(DEFAULT_EDITOR_SQ)'
>>  endif
>>  
>> +ifndef ETC_GITCONFIG
>> +ETC_GITCONFIG = $(sysconfdir)/gitconfig
>> +endif
>> +ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
>> +ASCIIDOC_EXTRA += -a 'etc-gitconfig=$(ETC_GITCONFIG_SQ)'
>> +
>> +ifndef ETC_GITATTRIBUTES
>> +ETC_GITATTRIBUTES = $(sysconfdir)/gitattributes
>> +endif
>> +ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
>> +ASCIIDOC_EXTRA += -a 'etc-gitattributes=$(ETC_GITATTRIBUTES_SQ)'
>> +
> 
> It's a shame we have to repeat this logic from the Makefile, though I
> guess we already do so for prefix, bindir, etc, so far.
> 
> Should we factor the path logic from the top-level Makefile into an
> include that can be used from either?

Yeah, maybe. I didn't like the duplication either, but as you noticed, we do it already for many of the other variables. I suspect we could put these defaults into config.mak.uname which Documentation/Makefile includes and then you could run 'make -C Documentation' in a fresh clone or tarball and get docs built with the defaults set for each platform.

Show 15 quoted lines

>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 1cc18a828c..ed903b60bd 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -5,7 +5,7 @@ The Git configuration file contains a number of variables that affect
>>  the Git commands' behavior. The `.git/config` file in each repository
>>  is used to store the configuration for that repository, and
>>  `$HOME/.gitconfig` is used to store a per-user configuration as
>> -fallback values for the `.git/config` file. The file `/etc/gitconfig`
>> +fallback values for the `.git/config` file. The file +{etc-gitconfig}+
> 
> I think the formatting tweak you've done here is the right thing.
> There's no way to expand within literal backticks (since that's the
> point). So we only care about the monospacing effect, which ++ should
> give.

Thanks. It took me longer to decide that I couldn't find a clean way to do it without using ++ than anything else. :)

-- 
Todd
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
No one ever went broke underestimating the taste of the American
public.
    -- H. L. Mencken