Re: [PATCH] Add warning about known issues to documentation of cvsimport

Jeff King [off-list ref] writes:

Going back to the original discussion, it looks like it is a workaround
for docbook-xsl 1.69.0:

  http://article.gmane.org/gmane.comp.version-control.git/32957

Assuming that is correct, I think the sane choices are:

  1. drop the workaround, as that version of docbook-xsl is now several
     years old

     or

  2. turn the workaround off by default, but add a knob to turn it on
     (DOCBOOK_XSL_1690?)

Having it on by default and turning it off with a knob seems silly,
since most versions don't need it. Debian stable is shipping 1.73 these
days, which looks fine without 7ef0435. Are there other platforms still
shipping 1.69.0? Is it too old for us to care?

I am very tempted to say 1. but we seem to have a track record of trying
to be nice to people.  How involved would 2 be compared to 1?

With this change, the "spurious .sp" suppression XSLT code is
disabled by default. It can be enabled by defining
DOCBOOK_SUPPRESS_SP.

The "spurious .sp" XSLT fragment was used to work around a bug
first released in docbook-xsl 1.69.1. Modern versions of
docbook-xsl are negatively affected by the code (some empty lines
are omitted from manpage output; see
<http://article.gmane.org/gmane.comp.version-control.git/115302>).

The key revisions in the docbook SVN repo seem to be 5144 (before
docbook-xsl 1.69.1) and 6359 (before docbook-xsl 1.71.1).

Testing done with asciidoc 8.3.1 and docbook-xsl 1.74.0.

Signed-off-by: Chris Johnsen <redacted>

---

Here is a proof-of-concept. It is on top of next (it requires the
previous XSLT/asciidoc cleanup).

I went with a "feature knob" instead of a "version knob" since my
research in the docbook SVN repo indicates that multiple versions
are affected. Maybe the name could be better. Also I am not at
all sure that my research into past docbook-xsl releases is 100%
accurate. Anyone motivated enough to install old versions of
docbook-xsl and test with them?

The message that Peff cites
(<http://article.gmane.org/gmane.comp.version-control.git/32957>)
seems to indicate that the "spurious .sp" problem was injected
_between_ 1.69.0 and 1.69.1. So, I did some research in the
docbook SVN repo.

I grepped for ".sp" and "simpara" in
<http://docbook.svn.sourceforge.net/viewvc/docbook/trunk/xsl/manpages/block.xsl?view=log>
to find likely interesting spots (sure, not thorough, but I hoped
to get lucky). Then I slogged through the "tags" directory to
find out when in the revision stream docbook-xsl releases seemed
to have been cut.

Here are some of the "interesting" revision numbers:

5119  1.69.0
5144          .sp instead of blank line in mixed blocks
5152  1.69.1
5755          newline before .sp in verbatims (not simpara)
5985  1.70.0
6003  1.70.1
6166          suppress .sp inside {author,person}blurb
6279  1.71.0
6359          newline before .sp in simpara
6373  1.71.1
6552  1.72.0
...
7398  1.73.2
      no tags for 1.74.*?
7782          move .sp to before, not after simpara text
7844          suppress .sp inside callout
?     1.74.0  {relnotes include descriptions of 7782 and 7844}

Before I got tired of digging through the SVN history, it seemed
to me that the problematic ".sp" was introduced at 5144 and
resolved at 6359. The code in Git's XSLT is very similar to that
of revision 6359, lines 81-91 (with a double newline instead of a
properly positioned ".sp" command).

So, it seems that the "spurious .sp" problem that Git's simpara
template "fixes" is not present in docbook-xsl 1.69.0, but is
present in 1.69.1, 1.70.0, 1.70.1, and 1.71.0. The "spurious .sp"
might not be present in 1.69.0 and earlier, but I would guess
that there are still line spacing issues there.

Should more of this background info be in the commit message?
Less?
---
 Documentation/Makefile                |    7 ++++++-
 Documentation/manpage-base.xsl        |   13 -------------
 Documentation/manpage-suppress-sp.xsl |   21 +++++++++++++++++++++
 3 files changed, 27 insertions(+), 14 deletions(-)
 create mode 100644 Documentation/manpage-suppress-sp.xsl

diff --git a/Documentation/Makefile b/Documentation/Makefile
index dae3174..dba97dc 100644
--- a/Documentation/Makefile
+++ b/Documentation/Makefile

@@ -69,7 +69,9 @@ endif
 #
 # For docbook-xsl ...
 #	-1.68.1,	set ASCIIDOC_NO_ROFF? (based on changelog from 1.73.0)
-#	1.69.0-1.71.1,	no extra settings are needed?
+#	1.69.0,		no extra settings are needed?
+#	1.69.1-1.71.0,	set DOCBOOK_SUPPRESS_SP?
+#	1.71.1,		no extra settings are needed?
 #	1.72.0,		set DOCBOOK_XSL_172.
 #	1.73.0-,	set ASCIIDOC_NO_ROFF
 #

@@ -97,6 +99,9 @@ endif
 ifdef MAN_BOLD_LITERAL
 XMLTO_EXTRA += -m manpage-bold-literal.xsl
 endif
+ifdef DOCBOOK_SUPPRESS_SP
+XMLTO_EXTRA += -m manpage-suppress-sp.xsl
+endif
 
 #
 # Please note that there is a minor bug in asciidoc.

diff --git a/Documentation/manpage-base.xsl b/Documentation/manpage-base.xsl
index 16e2e40..a264fa6 100644
--- a/Documentation/manpage-base.xsl
+++ b/Documentation/manpage-base.xsl

@@ -32,17 +32,4 @@
 	<xsl:text>br&#10;</xsl:text>
 </xsl:template>
 
-<!-- attempt to work around spurious .sp at the tail of the line
-     that docbook stylesheets seem to add -->
-<xsl:template match="simpara">
-  <xsl:variable name="content">
-    <xsl:apply-templates/>
-  </xsl:variable>
-  <xsl:value-of select="normalize-space($content)"/>
-  <xsl:if test="not(ancestor::authorblurb) and
-                not(ancestor::personblurb)">
-    <xsl:text>&#10;&#10;</xsl:text>
-  </xsl:if>
-</xsl:template>
-
 </xsl:stylesheet>

diff --git a/Documentation/manpage-suppress-sp.xsl b/Documentation/manpage-suppress-sp.xsl
new file mode 100644
index 0000000..a63c763
--- /dev/null
+++ b/Documentation/manpage-suppress-sp.xsl

@@ -0,0 +1,21 @@
+<!-- manpage-suppress-sp.xsl:
+     special settings for manpages rendered from asciidoc+docbook
+     handles erroneous, inline .sp in manpage output of some
+     versions of docbook-xsl -->
+<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform"
+		version="1.0">
+
+<!-- attempt to work around spurious .sp at the tail of the line
+     that some versions of docbook stylesheets seem to add -->
+<xsl:template match="simpara">
+  <xsl:variable name="content">
+    <xsl:apply-templates/>
+  </xsl:variable>
+  <xsl:value-of select="normalize-space($content)"/>
+  <xsl:if test="not(ancestor::authorblurb) and
+                not(ancestor::personblurb)">
+    <xsl:text>&#10;&#10;</xsl:text>
+  </xsl:if>
+</xsl:template>
+
+</xsl:stylesheet>

-- 
1.6.2.1.556.g581a3

On Wed, Apr 01, 2009 at 03:50:34AM -0500, Chris Johnsen wrote:

With this change, the "spurious .sp" suppression XSLT code is
disabled by default. It can be enabled by defining
DOCBOOK_SUPPRESS_SP.
[...]
Signed-off-by: Chris Johnsen <redacted>

Acked-by: Jeff King <redacted>

This looks good to me. Thank you for being so thorough in both the
research and implementation, especially when I was being so lazy. :)

I went with a "feature knob" instead of a "version knob" since my
research in the docbook SVN repo indicates that multiple versions
are affected. Maybe the name could be better. Also I am not at
all sure that my research into past docbook-xsl releases is 100%
accurate. Anyone motivated enough to install old versions of
docbook-xsl and test with them?

I think the "feature knob" makes sense. I don't know that it is worth
extensive testing with old releases. You have a pretty good guess about
which versions are affected, and people who experience the problem can
turn the knob. Your Makefile comments make it easy for them find the
knob once they see the breakage.

It is probably worth mentioning in the release notes to give a heads-up,
though.

Something like:

-- >8 --
Subject: mention docbook knob in the release notes

People with ancient docbook-xsl will see the return of the "spurious
.sp" unless this knob is turned, so let's inform them.

---

diff --git a/Documentation/RelNotes-1.6.3.txt b/Documentation/RelNotes-1.6.3.txt
index f0a2e41..0c8a14e 100644
--- a/Documentation/RelNotes-1.6.3.txt
+++ b/Documentation/RelNotes-1.6.3.txt

@@ -107,6 +107,11 @@ Updates since v1.6.2
 * Makefile learned 'coverage' option to run the test suites with
   coverage tracking enabled.
 
+* Building the manpages with docbook-xsl between 1.69.1 and 1.71.1 now
+  requires setting DOCBOOK_SUPPRESS_SP to work around a docbook-xsl bug.
+  This workaround used to be enabled by default, but causes problems
+  with newer versions of docbook-xsl.
+
 Fixes since v1.6.2
 ------------------
 

On Wed, Apr 01, 2009 at 03:50:34AM -0500, Chris Johnsen wrote:

The key revisions in the docbook SVN repo seem to be 5144 (before
docbook-xsl 1.69.1) and 6359 (before docbook-xsl 1.71.1).

Testing done with asciidoc 8.3.1 and docbook-xsl 1.74.0.

In the course of your SVN research, did you find the fixes between
1.73.1 and 1.74.3 that fixed the spacing issue? If so, I wonder if it's
worth backporting that fix to DOCBOOK_FIX_LIST_SPACING.

-Peff

On 2009 Apr 1, at 05:34, Jeff King wrote:

On Wed, Apr 01, 2009 at 03:50:34AM -0500, Chris Johnsen wrote:

quoted

The key revisions in the docbook SVN repo seem to be 5144 (before
docbook-xsl 1.69.1) and 6359 (before docbook-xsl 1.71.1).

Testing done with asciidoc 8.3.1 and docbook-xsl 1.74.0.

In the course of your SVN research, did you find the fixes between
1.73.1 and 1.74.3 that fixed the spacing issue? If so, I wonder if  
it's
worth backporting that fix to DOCBOOK_FIX_LIST_SPACING.

I guess you are referring to an issue different from the one created  
by using the "spurious .sp" simpara template, but I am not familiar  
with another one. If not, then I am confused. The new patch to avoid  
using the "spurious .sp" template fixes the list spacing in pu's git- 
cvsimport.1 when I generate it here (using docbook-xsl 1.74.0). For  
example, the extra blank line after "Problems related to timestamps:"  
goes away and a new blank line is inserted before "Problems related  
to branches:".

My poking around in the docbook SVN repo was largely limited to the  
manpages/block.xsl file since that is where the normal simpara  
template lives. If this other issue is list specific, it seems likely  
that fixes would be in manpages/lists.xsl. It looks like there have  
only been around ten commits to that lists.xsl since 1.73.1, but none  
of them jumped out at me as likely culprits unless the spacing you  
mean is indentation or "bullet"-to-text spacing (though my brain is  
tired right now).

-- 
Chris

On Wed, Apr 01, 2009 at 07:19:08AM -0500, Chris Johnsen wrote:

quoted

In the course of your SVN research, did you find the fixes between
1.73.1 and 1.74.3 that fixed the spacing issue? If so, I wonder if it's
worth backporting that fix to DOCBOOK_FIX_LIST_SPACING.

I guess you are referring to an issue different from the one created by 
using the "spurious .sp" simpara template, but I am not familiar with 
another one. If not, then I am confused. The new patch to avoid using the 
"spurious .sp" template fixes the list spacing in pu's git-cvsimport.1 
when I generate it here (using docbook-xsl 1.74.0). For example, the extra 
blank line after "Problems related to timestamps:" goes away and a new 
blank line is inserted before "Problems related to branches:".

Sorry, I should have been more clear (it seems we have enough docbook
problems to cause confusion in referring to them :) ).  What I meant is:

  The original issue which caused me to investigate this, namely the
  extra blank line before a list and the missing blank line after the
  list, is present in 1.73 but not in 1.74 (I tested only with 1.74.3,
  but your statement above leads me to believe it is fixed in 1.74.0).

  Is it worth including a fix in our docbook templates to make it look
  right for people on 1.73?

My poking around in the docbook SVN repo was largely limited to the  
manpages/block.xsl file since that is where the normal simpara template 
lives. If this other issue is list specific, it seems likely that fixes 
would be in manpages/lists.xsl. It looks like there have only been around 
ten commits to that lists.xsl since 1.73.1, but none of them jumped out at 
me as likely culprits unless the spacing you mean is indentation or 
"bullet"-to-text spacing (though my brain is tired right now).

Hmm. I think part of the fix is actually in param.xsl, which contains:

    <!-- * squeeze multiple .sp instances into a single .sp-->
    <substitution oldstring=".sp&#10;.sp" newstring=".sp"/>

in 1.74, but not 1.73.

I am torn on whether it makes sense to try backporting this. Debian
stable, at least, will be on 1.73 for quite a long time. On the other
hand, the problem is relatively minor (it is ugly, but you can still
read the text) and I'm not sure we want to get into pulling random fixes
from upstream docbook-xsl; it could turn into a huge time sink.

-Peff

Jeff King [off-list ref] writes:

I think the "feature knob" makes sense. I don't know that it is worth
extensive testing with old releases. You have a pretty good guess about
which versions are affected, and people who experience the problem can
turn the knob. Your Makefile comments make it easy for them find the
knob once they see the breakage.

It is probably worth mentioning in the release notes to give a heads-up,
though.

Thanks.