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

[Todd Zullinger <tmz@pobox.com>]

Jeff King wrote:
> 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? :)

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

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

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

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