Re: [PATCH] asciidoctor-extensions: provide `<refmiscinfo/>`

[Jeff King <peff@peff.net>]

On Sun, Mar 17, 2019 at 03:47:47PM +0100, Martin Ågren wrote:

>  Cc Todd and Peff who had a brief exchange [1] a while ago. Apparently
>  Todd saw this "[FIXME: source]" on Fedora but Peff did not on Debian.
>  I'm on Ubuntu 18.04 and used to see this also on 16.04. I'm not sure
>  what might make Debian so special here.

I think it was just that my version of asciidoctor had

  https://github.com/asciidoctor/asciidoctor/pull/2636

and Todd's did not. However, mine still does not do the _right_ thing,
because we didn't pass the right attributes in to asciidoctor. It just
didn't print an ugly "FIXME". Looking at the XML, I have:

  <refentrytitle>git-add</refentrytitle>
  <manvolnum>1</manvolnum>
  <refmiscinfo class="source">&#160;</refmiscinfo>
  <refmiscinfo class="manual">&#160;</refmiscinfo>
  </refmeta>

So it's just an nbsp instead of the real content, and the "version"
field is missing entirely.

> That Asciidoctor ignores asciidoc.conf is nothing new. This is why we
> implement the `linkgit:` macro in asciidoc.conf *and* in
> asciidoctor-extensions.rb. Follow suit and provide these tags in
> asciidoctor-extensions.rb, using a "postprocessor" extension.

Yeah, that seems sensible overall. Some thoughts on your approach:

>   * Provide the `mansource` attribute to Asciidoctor. This attribute
>     looks promising until one realizes that it can only be given inside
>     the source file (the .txt file in our case), *not* on the command
>     line using `-a mansource=foobar`. I toyed with the idea of injecting
>     this attribute while feeding Asciidoctor the input on stdin, but it
>     didn't feel like it was worth the complexity in the Makefile.

It does seem like "mansource" is the way asciidoctor expects us to do
this. Why doesn't it work from the command line? Is it a bug in
asciidoctor, or is there something more subtle going on?

I think even if it is a bug and gets fixed, though, it still wouldn't
have the version field (though that seems like something we could
contribute to asciidoctor).

>   * Similar to that last idea, we could inject these lines into the
>     xml-files from the Makefile, e.g., using `sed`. This reduces
>     repetition, but seems fairly brittle. It feels wrong to impose
>     another step and another risk on the Asciidoc-processing only to
>     benefit the Asciidoctor-one.

That's more or less what your ruby code is doing on. That said, I'd just
as soon do it in ruby as with a separate sed invocation. At least then
the external build is the same.

>   * Considering the above abandoned ideas, it seems better to put any
>     complexity inside asciidoctor-extensions.rb. It is after all
>     supposed to be the "equivalent" of asciidoc.conf. I considered
>     providing a "tree processor" extension and use it to set, e.g.,
>     `mansource` mentioned above.

This seems like the least bad option, at least for now. Your code does
do a generic regex substitution. The promise of XML is that we're
supposed to be able to do structured, robust transformations of the
document. But my experience has been that the tooling is sufficiently
difficult to work with that you just end up writing a regex.

So I'm curious if you tried to use an actual XML parser (or god forbid,
XSLT) to do the transformation. But if you spent more than 5 minutes on
it and got disgusted, I wouldn't ask you to look deeper than that. :)

I doubt we'd see any other refmeta tags (and any non-tag content would
be quoted).

> Let's instead try to stay as close as possible to what asciidoc.conf
> does. We'll make it fairly obvious that we aim to inject the exact same
> three lines of `<refmiscinfo/>` that asciidoc.conf provides. The only
> somewhat tricky part is that we inject them *post*-processing so we need
> to do the variable expansion ourselves.

One thing that asciidoctor buys us that asciidoc does not is that we
might eventually move to directly generating the manpages, without the
XML / Docbook step in between. And if we do, then all of this XML
hackery is going to have to get replaced with something else. I guess we
can cross that bridge when we come to it.

> diff --git a/Documentation/asciidoctor-extensions.rb b/Documentation/asciidoctor-extensions.rb
> index 0089e0cfb8..059279dee1 100644
> --- a/Documentation/asciidoctor-extensions.rb
> +++ b/Documentation/asciidoctor-extensions.rb
> @@ -20,9 +20,25 @@ module Git
>          end
>        end
>      end
> +
> +    class DocumentPostProcessor < Asciidoctor::Extensions::Postprocessor
> +      def process document, output
> +        if document.basebackend? 'docbook'
> +          git_version = document.attributes['git_version']
> +          replacement = "" \
> +            "<refmiscinfo class=\"source\">Git</refmiscinfo>\n" \
> +            "<refmiscinfo class=\"version\">#{git_version}</refmiscinfo>\n" \
> +            "<refmiscinfo class=\"manual\">Git Manual</refmiscinfo>\n" \
> +            "<\/refmeta>"
> +          output = output.sub(/<\/refmeta>/, replacement)
> +        end
> +        output
> +      end
> +    end

The patch itself looks sane. Would we ever need to XML-quote the
contents of git_version? I guess the asciidoc.conf version doesn't
bother. Technically the user running "make" could put whatever they want
into it, but I think this is a case of "if it hurts, don't do it", and
we can ignore it.

-Peff