| Message ID | 20191030204104.19603-1-martin.agren@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | manpage-bold-literal.xsl: provide namespaced template for "d:literal" | expand |
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
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.
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
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
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.
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>
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(+)