[PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[Martin Ågren]

We recently regressed our rendering of "literal" elements in our
manpages, i.e, stuff we have placed within `backticks` in order to
render as monospace. In particular, we lost the bold rendering of such
literal text.

The culprit is f6461b82b9 ("Documentation: fix build with Asciidoctor 2",
2019-09-15), where we switched from DocBook 4.5 to DocBook 5 with
Asciidoctor. As part of the switch, we started using the namespaced
DocBook XSLT stylesheets rather than the non-namespaced ones. (See
f6461b82b9 for more details on why we changed to the namespaced ones.)

The bold literals are implemented as an XSLT snippet <xsl:template
match="literal">...</xsl:template>. Now that we use namespaces, this
doesn't pick up our literals like it used to.

Add an exact copy of the template where we match for "d:literal" instead
of just "literal", after defining the d namespace. ("d" is what
http://docbook.sourceforge.net/release/xsl-ns/current/manpages/docbook.xsl
uses.) Note that we need to keep the non-namespaced version for
AsciiDoc.

This boldness was introduced by 5121a6d993 ("Documentation: option to
render literal text as bold for manpages", 2009-03-27) and made the
default in 5945717009 ("Documentation: bold literals in man",
2016-05-31).

One reason this was not caught in review is that our doc-diff tool diffs
without any boldness, i.e., it "only" compares text.

This has been optically tested with AsciiDoc 8.6.10, Asciidoctor 1.5.5
and Asciidoctor 2.0.10. I've also verified that doc-diff produces the
empty output in all three cases, as expected.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 I've kept thinking lately that "wow, are we behind on marking up stuff
 to be monospaced/boldened"...

 I'm pretty sure about the background here, but I'm not at all sure 
 that this is the prettiest or correctest fix.
 
 Not sure if this problem is bad enough (and the fix good enough) for
 this to go into 2.24, but I offer this anyway.

 There are more manpage-*.xsl -- manpage-suppress-sp.xsl looks like it
 would have the exact same problem. But before diving in too deep, I'd
 rather submit this one to see if it's in the right direction at all.

 Martin

 Documentation/manpage-bold-literal.xsl | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/manpage-bold-literal.xsl b/Documentation/manpage-bold-literal.xsl
index 608eb5df62..172388d6cf 100644
--- a/Documentation/manpage-bold-literal.xsl
+++ b/Documentation/manpage-bold-literal.xsl
@@ -1,11 +1,20 @@
 <!-- manpage-bold-literal.xsl:
      special formatting for manpages rendered from asciidoc+docbook -->
 <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
+		xmlns:d="http://docbook.org/ns/docbook"
 		version="1.0">
 
 <!-- render literal text as bold (instead of plain or monospace);
      this makes literal text easier to distinguish in manpages
      viewed on a tty -->
+<xsl:template match="d:literal">
+	<xsl:value-of select="$git.docbook.backslash"/>
+	<xsl:text>fB</xsl:text>
+	<xsl:apply-templates/>
+	<xsl:value-of select="$git.docbook.backslash"/>
+	<xsl:text>fR</xsl:text>
+</xsl:template>
+
 <xsl:template match="literal">
 	<xsl:value-of select="$git.docbook.backslash"/>
 	<xsl:text>fB</xsl:text>
-- 
2.24.0.rc2

Re: [PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[Jeff King]

On Wed, Oct 30, 2019 at 09:41:04PM +0100, Martin Ågren wrote:

> We recently regressed our rendering of "literal" elements in our
> manpages, i.e, stuff we have placed within `backticks` in order to
> render as monospace. In particular, we lost the bold rendering of such
> literal text.

This is just when rendering with asciidoctor, right? AFAICT the bolding
is still fine in pages built with asciidoc.

> The culprit is f6461b82b9 ("Documentation: fix build with Asciidoctor 2",
> 2019-09-15), where we switched from DocBook 4.5 to DocBook 5 with
> Asciidoctor. As part of the switch, we started using the namespaced
> DocBook XSLT stylesheets rather than the non-namespaced ones. (See
> f6461b82b9 for more details on why we changed to the namespaced ones.)
> 
> The bold literals are implemented as an XSLT snippet <xsl:template
> match="literal">...</xsl:template>. Now that we use namespaces, this
> doesn't pick up our literals like it used to.
>
> Add an exact copy of the template where we match for "d:literal" instead
> of just "literal", after defining the d namespace. ("d" is what
> http://docbook.sourceforge.net/release/xsl-ns/current/manpages/docbook.xsl
> uses.) Note that we need to keep the non-namespaced version for
> AsciiDoc.

Both the explanation and the solution make sense. We'd eventually be
able to drop this duplicate xsl if we go asciidoctor-only.

I have more general thoughts below, but the patch itself looks good to
me (and IMHO is worth taking for 2.24).

> This boldness was introduced by 5121a6d993 ("Documentation: option to
> render literal text as bold for manpages", 2009-03-27) and made the
> default in 5945717009 ("Documentation: bold literals in man",
> 2016-05-31).

I somehow missed that we turned on this bolding by default. I can
finally delete MAN_BOLD_LITERAL from my config.mak. ;)

> One reason this was not caught in review is that our doc-diff tool diffs
> without any boldness, i.e., it "only" compares text.

I don't think this was intentional, but just a consequence of
redirecting man's stdout to a non-terminal. Doing:

  MAN_KEEP_FORMATTING=1 ./doc-diff --asciidoctor HEAD^ HEAD

on your patch shows the improvement, though note that the diffed version
is kind of ugly. It looks like the bolding is done with ^H characters,
and it interacts in a funny way with our diff coloring, as well as with
diff-highlight if you use it. Piping the above through "less" looks
decent, but it gives me pause on whether we should be setting that
variable inside the script.

One other annoyance is that the directory names we use as a key for
caching results from run to run don't know about MAN_KEEP_FORMATTING. So
you may need "-f".

Speaking of annoyances, is it just me, or does the rendering stage of
doc-diff not actually proceed in parallel? Doing this seems to help, but
I'm not sure why:

diff --git a/Documentation/doc-diff b/Documentation/doc-diff
index 88a9b20168..1694300e50 100755
--- a/Documentation/doc-diff
+++ b/Documentation/doc-diff
@@ -127,7 +127,7 @@ generate_render_makefile () {
 	while read src
 	do
 		dst=$2/${src#$1/}
-		printf 'all:: %s\n' "$dst"
+		printf 'all: %s\n' "$dst"
 		printf '%s: %s\n' "$dst" "$src"
 		printf '\t@echo >&2 "  RENDER $(notdir $@)" && \\\n'
 		printf '\tmkdir -p $(dir $@) && \\\n'

> This has been optically tested with AsciiDoc 8.6.10, Asciidoctor 1.5.5
> and Asciidoctor 2.0.10. I've also verified that doc-diff produces the
> empty output in all three cases, as expected.

I like the phrase "optically tested". :)

I also confirmed with the MAN_KEEP_FORMATTING trick above that "doc-diff
--asciidoctor" fixes the problem as advertised, and "--asciidoc" has no
change at all.

>  I'm pretty sure about the background here, but I'm not at all sure 
>  that this is the prettiest or correctest fix.
>  
>  Not sure if this problem is bad enough (and the fix good enough) for
>  this to go into 2.24, but I offer this anyway.

It will only be noticed by people building with asciidoctor. But it _is_
a regression for them in 2.24, and the patch seems pretty safe. So I
think it's probably worth doing, but if it doesn't happen until the next
maint release, I don't think it's the end of the world.

>  There are more manpage-*.xsl -- manpage-suppress-sp.xsl looks like it
>  would have the exact same problem. But before diving in too deep, I'd
>  rather submit this one to see if it's in the right direction at all.

It looks like a lot of them don't actually match on the namespaced
tagnames, and so are OK. Some of them require special options to enable,
so we wouldn't necessarily notice problems via doc-diff.

From my brief look, I think suppress-sp is the only one that needs
attention. I kind of wonder if we can just drop it. According to the
Makefile comment, it's needed only for docbook 1.69.1-1.71.0. But 1.71.1
came out in 2006. Surely even RHEL7 or whatever ancient system people
use is past that, right? :)

Or alternatively, as I've argued elsewhere, we could simply be a little
more aggressive about deprecating old doc build tools. According to the
Makefile, no extra settings are needed with docbook >1.73.0. That came
out in 2007. I'd be willing to just call that the cutoff point, and
anybody without it can install the pre-formatted pages.

-Peff

Re: [PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[Martin Ågren]

On Wed, 30 Oct 2019 at 22:24, Jeff King <peff@peff.net> wrote:
>
> On Wed, Oct 30, 2019 at 09:41:04PM +0100, Martin Ågren wrote:
>
> > We recently regressed our rendering of "literal" elements in our
> > manpages, i.e, stuff we have placed within `backticks` in order to
> > render as monospace. In particular, we lost the bold rendering of such
> > literal text.
>
> This is just when rendering with asciidoctor, right? AFAICT the bolding
> is still fine in pages built with asciidoc.

Right. Sorry for being unclear. Fixed in v2.

> > One reason this was not caught in review is that our doc-diff tool diffs
> > without any boldness, i.e., it "only" compares text.
>
> I don't think this was intentional, but just a consequence of
> redirecting man's stdout to a non-terminal. Doing:
>
>   MAN_KEEP_FORMATTING=1 ./doc-diff --asciidoctor HEAD^ HEAD
>
> on your patch shows the improvement, though note that the diffed version
> is kind of ugly. It looks like the bolding is done with ^H characters,
> and it interacts in a funny way with our diff coloring, as well as with
> diff-highlight if you use it. Piping the above through "less" looks
> decent, but it gives me pause on whether we should be setting that
> variable inside the script.

Very interesting! Thanks for this trick.

> Speaking of annoyances, is it just me, or does the rendering stage of
> doc-diff not actually proceed in parallel? Doing this seems to help, but
> I'm not sure why:
>
> diff --git a/Documentation/doc-diff b/Documentation/doc-diff
> index 88a9b20168..1694300e50 100755
> --- a/Documentation/doc-diff
> +++ b/Documentation/doc-diff
> @@ -127,7 +127,7 @@ generate_render_makefile () {
>         while read src
>         do
>                 dst=$2/${src#$1/}
> -               printf 'all:: %s\n' "$dst"
> +               printf 'all: %s\n' "$dst"
>                 printf '%s: %s\n' "$dst" "$src"
>                 printf '\t@echo >&2 "  RENDER $(notdir $@)" && \\\n'
>                 printf '\tmkdir -p $(dir $@) && \\\n'
>

Hm, didn't look into this. Will try to find the time.

> I also confirmed with the MAN_KEEP_FORMATTING trick above that "doc-diff
> --asciidoctor" fixes the problem as advertised, and "--asciidoc" has no
> change at all.

Thanks!

> >  There are more manpage-*.xsl -- manpage-suppress-sp.xsl looks like it
> >  would have the exact same problem. But before diving in too deep, I'd
> >  rather submit this one to see if it's in the right direction at all.
>
> It looks like a lot of them don't actually match on the namespaced
> tagnames, and so are OK. Some of them require special options to enable,
> so we wouldn't necessarily notice problems via doc-diff.
>
> From my brief look, I think suppress-sp is the only one that needs
> attention. I kind of wonder if we can just drop it. According to the
> Makefile comment, it's needed only for docbook 1.69.1-1.71.0. But 1.71.1
> came out in 2006. Surely even RHEL7 or whatever ancient system people
> use is past that, right? :)

I also only had a brief look and realized I wouldn't know how to test
with such a broken version, so I didn't feel comfortable mucking around
with that file.

> Or alternatively, as I've argued elsewhere, we could simply be a little
> more aggressive about deprecating old doc build tools. According to the
> Makefile, no extra settings are needed with docbook >1.73.0. That came
> out in 2007. I'd be willing to just call that the cutoff point, and
> anybody without it can install the pre-formatted pages.

Yeah, that makes sense.

Martin

Re: [PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 1685 bytes --]

On 2019-10-30 at 20:41:04, Martin Ågren wrote:
>  I've kept thinking lately that "wow, are we behind on marking up stuff
>  to be monospaced/boldened"...
> 
>  I'm pretty sure about the background here, but I'm not at all sure 
>  that this is the prettiest or correctest fix.

This fix is correct.  Thanks for sending a patch for this, and sorry
about regressing our bolding behavior.

> diff --git a/Documentation/manpage-bold-literal.xsl b/Documentation/manpage-bold-literal.xsl
> index 608eb5df62..172388d6cf 100644
> --- a/Documentation/manpage-bold-literal.xsl
> +++ b/Documentation/manpage-bold-literal.xsl
> @@ -1,11 +1,20 @@
>  <!-- manpage-bold-literal.xsl:
>       special formatting for manpages rendered from asciidoc+docbook -->
>  <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
> +		xmlns:d="http://docbook.org/ns/docbook"
>  		version="1.0">
>  
>  <!-- render literal text as bold (instead of plain or monospace);
>       this makes literal text easier to distinguish in manpages
>       viewed on a tty -->
> +<xsl:template match="d:literal">
> +	<xsl:value-of select="$git.docbook.backslash"/>
> +	<xsl:text>fB</xsl:text>
> +	<xsl:apply-templates/>
> +	<xsl:value-of select="$git.docbook.backslash"/>
> +	<xsl:text>fR</xsl:text>
> +</xsl:template>
> +
>  <xsl:template match="literal">

If you want to avoid duplication, you can write the existing template as
follows:

  <xsl:template match="literal|d:literal">

That will match both literal and d:literal tags.  You still need the
namespace declaration you added, of course.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[Martin Ågren]

On Thu, 31 Oct 2019 at 03:32, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2019-10-30 at 20:41:04, Martin Ågren wrote:
> >  I'm pretty sure about the background here, but I'm not at all sure
> >  that this is the prettiest or correctest fix.
>
> This fix is correct.  Thanks for sending a patch for this, and sorry
> about regressing our bolding behavior.

Thanks! And no worries. I should have spotted this sooner.

> If you want to avoid duplication, you can write the existing template as
> follows:
>
>   <xsl:template match="literal|d:literal">
>
> That will match both literal and d:literal tags.  You still need the
> namespace declaration you added, of course.

Thanks. I use this for v2.

Martin

Re: [PATCH] manpage-bold-literal.xsl: provide namespaced template for "d:literal"

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

>> If you want to avoid duplication, you can write the existing template as
>> follows:
>>
>>   <xsl:template match="literal|d:literal">
>>
>> That will match both literal and d:literal tags.  You still need the
>> namespace declaration you added, of course.
>
> Thanks. I use this for v2.

Thanks, both.

[PATCH v2] manpage-bold-literal.xsl: match for namespaced "d:literal" in template

[Martin Ågren]

We recently regressed our rendering with Asciidoctor of "literal"
elements in our manpages, i.e, stuff we have placed within `backticks`
in order to render as monospace. In particular, we lost the bold
rendering of such literal text.

The culprit is f6461b82b9 ("Documentation: fix build with Asciidoctor 2",
2019-09-15), where we switched from DocBook 4.5 to DocBook 5 with
Asciidoctor. As part of the switch, we started using the namespaced
DocBook XSLT stylesheets rather than the non-namespaced ones. (See
f6461b82b9 for more details on why we changed to the namespaced ones.)

The bold literals are implemented as an XSLT snippet <xsl:template
match="literal">...</xsl:template>. Now that we use namespaces, this
doesn't pick up our literals like it used to.

Match for "d:literal" in addition to just "literal", after defining the
d namespace. ("d" is what
http://docbook.sourceforge.net/release/xsl-ns/current/manpages/docbook.xsl
uses.) Note that we need to keep matching without the namespace for
AsciiDoc.

This boldness was introduced by 5121a6d993 ("Documentation: option to
render literal text as bold for manpages", 2009-03-27) and made the
default in 5945717009 ("Documentation: bold literals in man",
2016-05-31).

One reason this was not caught in review is that our doc-diff tool diffs
without any boldness, i.e., it "only" compares text. As pointed out by
Peff in review of this patch, one can use `MAN_KEEP_FORMATTING=1
./doc-diff <...>`

This has been optically tested with AsciiDoc 8.6.10, Asciidoctor 1.5.5
and Asciidoctor 2.0.10. I've also verified that doc-diff produces the
empty output for all three programs, as expected, and that with the
MAN_KEEP_FORMATTING trick, AsciiDoc yields no diff, whereas with
Asciidoctor, we get bold literals, just like we want.

Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
Range-diff against v1:
1:  fb5372c3cf ! 1:  a22ed4896c manpage-bold-literal.xsl: provide namespaced template for "d:literal"
    @@ Metadata
     Author: Martin Ågren <martin.agren@gmail.com>
     
      ## Commit message ##
    -    manpage-bold-literal.xsl: provide namespaced template for "d:literal"
    +    manpage-bold-literal.xsl: match for namespaced "d:literal" in template
     
    -    We recently regressed our rendering of "literal" elements in our
    -    manpages, i.e, stuff we have placed within `backticks` in order to
    -    render as monospace. In particular, we lost the bold rendering of such
    -    literal text.
    +    We recently regressed our rendering with Asciidoctor of "literal"
    +    elements in our manpages, i.e, stuff we have placed within `backticks`
    +    in order to render as monospace. In particular, we lost the bold
    +    rendering of such literal text.
     
         The culprit is f6461b82b9 ("Documentation: fix build with Asciidoctor 2",
         2019-09-15), where we switched from DocBook 4.5 to DocBook 5 with
    @@ Commit message
         match="literal">...</xsl:template>. Now that we use namespaces, this
         doesn't pick up our literals like it used to.
     
    -    Add an exact copy of the template where we match for "d:literal" instead
    -    of just "literal", after defining the d namespace. ("d" is what
    +    Match for "d:literal" in addition to just "literal", after defining the
    +    d namespace. ("d" is what
         http://docbook.sourceforge.net/release/xsl-ns/current/manpages/docbook.xsl
    -    uses.) Note that we need to keep the non-namespaced version for
    +    uses.) Note that we need to keep matching without the namespace for
         AsciiDoc.
     
         This boldness was introduced by 5121a6d993 ("Documentation: option to
    @@ Commit message
         2016-05-31).
     
         One reason this was not caught in review is that our doc-diff tool diffs
    -    without any boldness, i.e., it "only" compares text.
    +    without any boldness, i.e., it "only" compares text. As pointed out by
    +    Peff in review of this patch, one can use `MAN_KEEP_FORMATTING=1
    +    ./doc-diff <...>`
     
         This has been optically tested with AsciiDoc 8.6.10, Asciidoctor 1.5.5
         and Asciidoctor 2.0.10. I've also verified that doc-diff produces the
    -    empty output for all three programs, as expected.
    +    empty output for all three programs, as expected, and that with the
    +    MAN_KEEP_FORMATTING trick, AsciiDoc yields no diff, whereas with
    +    Asciidoctor, we get bold literals, just like we want.
     
         Signed-off-by: Martin Ågren <martin.agren@gmail.com>
     
    @@ Documentation/manpage-bold-literal.xsl
      <!-- render literal text as bold (instead of plain or monospace);
           this makes literal text easier to distinguish in manpages
           viewed on a tty -->
    -+<xsl:template match="d:literal">
    -+	<xsl:value-of select="$git.docbook.backslash"/>
    -+	<xsl:text>fB</xsl:text>
    -+	<xsl:apply-templates/>
    -+	<xsl:value-of select="$git.docbook.backslash"/>
    -+	<xsl:text>fR</xsl:text>
    -+</xsl:template>
    -+
    - <xsl:template match="literal">
    +-<xsl:template match="literal">
    ++<xsl:template match="literal|d:literal">
      	<xsl:value-of select="$git.docbook.backslash"/>
      	<xsl:text>fB</xsl:text>
    + 	<xsl:apply-templates/>

 Documentation/manpage-bold-literal.xsl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/manpage-bold-literal.xsl b/Documentation/manpage-bold-literal.xsl
index 608eb5df62..94d6c1b545 100644
--- a/Documentation/manpage-bold-literal.xsl
+++ b/Documentation/manpage-bold-literal.xsl
@@ -1,12 +1,13 @@
 <!-- manpage-bold-literal.xsl:
      special formatting for manpages rendered from asciidoc+docbook -->
 <xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
+		xmlns:d="http://docbook.org/ns/docbook"
 		version="1.0">
 
 <!-- render literal text as bold (instead of plain or monospace);
      this makes literal text easier to distinguish in manpages
      viewed on a tty -->
-<xsl:template match="literal">
+<xsl:template match="literal|d:literal">
 	<xsl:value-of select="$git.docbook.backslash"/>
 	<xsl:text>fB</xsl:text>
 	<xsl:apply-templates/>
-- 
2.24.0.rc2