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

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

[Heiko Voigt]

The described issues are compiled from the tests by Michael Haggerty and me.
Because it is not apparent that these can be fixed anytime soon at least warn
unwary users not to rely on the inbuilt cvsimport to much.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Documentation/git-cvsimport.txt |   34 ++++++++++++++++++++++++++++++++++
 1 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index b7a8c10..3123725 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -24,6 +24,9 @@ repository, or incrementally import into an existing one.
 Splitting the CVS log into patch sets is done by 'cvsps'.
 At least version 2.1 is required.
 
+*WARNING:* for certain situations the import leads to incorrect results.
+Please see the section <<issues,ISSUES>> for further reference.
+
 You should *never* do any work of your own on the branches that are
 created by 'git-cvsimport'.  By default initial import will create and populate a
 "master" branch from the CVS repository's main branch which you're free
@@ -164,6 +167,37 @@ If '-v' is specified, the script reports what it is doing.
 Otherwise, success is indicated the Unix way, i.e. by simply exiting with
 a zero exit status.
 
+[[issues]]
+ISSUES
+------
+Problems related to timestamps:
+
+ * If timestamps of commits in the cvs repository are not stable enough
+   to be used for ordering commits
+ * If any files were ever "cvs import"ed more than once (e.g., import of
+   more than one vendor release)
+ * If the timestamp order of different files cross the revision order
+   within the commit matching time window
+
+Problems related to branches:
+
+ * Branches on which no commits have been made are not imported
+ * All files from the branching point are added to a branch even if
+   never added in cvs
+ * files added to the source branch *after* a daughter branch was
+   created: If previously no commit was made on the daugther branch they
+   will erroneously be added to the daughter branch in git
+
+Problems related to tags:
+
+* Multiple tags on the same revision are not imported
+
+If you suspect that any of these issues may apply to the repository you
+want to import consider using these alternative tools which proved to be
+more stable in practise:
+
+* cvs2git (part of cvs2svn), `http://cvs2svn.tigris.org`
+* parsecvs, `http://cgit.freedesktop.org/~keithp/parsecvs`
 
 Author
 ------
-- 
1.6.1.2.390.gba743

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

[Ferry Huberts (Pelagic)]

Heiko Voigt wrote:
> The described issues are compiled from the tests by Michael Haggerty and me.
> Because it is not apparent that these can be fixed anytime soon at least warn
> unwary users not to rely on the inbuilt cvsimport to much.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
> ---
>  Documentation/git-cvsimport.txt |   34 ++++++++++++++++++++++++++++++++++
>  1 files changed, 34 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index b7a8c10..3123725 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -24,6 +24,9 @@ repository, or incrementally import into an existing one.
>  Splitting the CVS log into patch sets is done by 'cvsps'.
>  At least version 2.1 is required.
>  
> +*WARNING:* for certain situations the import leads to incorrect results.
> +Please see the section <<issues,ISSUES>> for further reference.
> +
>  You should *never* do any work of your own on the branches that are
>  created by 'git-cvsimport'.  By default initial import will create and populate a
>  "master" branch from the CVS repository's main branch which you're free
> @@ -164,6 +167,37 @@ If '-v' is specified, the script reports what it is doing.
>  Otherwise, success is indicated the Unix way, i.e. by simply exiting with
>  a zero exit status.
>  
> +[[issues]]
> +ISSUES
> +------
> +Problems related to timestamps:
> +
> + * If timestamps of commits in the cvs repository are not stable enough
> +   to be used for ordering commits
> + * If any files were ever "cvs import"ed more than once (e.g., import of
> +   more than one vendor release)
> + * If the timestamp order of different files cross the revision order
> +   within the commit matching time window
> +
> +Problems related to branches:
> +
> + * Branches on which no commits have been made are not imported
> + * All files from the branching point are added to a branch even if
> +   never added in cvs
> + * files added to the source branch *after* a daughter branch was
> +   created: If previously no commit was made on the daugther branch they
> +   will erroneously be added to the daughter branch in git
> +
> +Problems related to tags:
> +
> +* Multiple tags on the same revision are not imported
> +
> +If you suspect that any of these issues may apply to the repository you
> +want to import consider using these alternative tools which proved to be
> +more stable in practise:
> +
> +* cvs2git (part of cvs2svn), `http://cvs2svn.tigris.org`
> +* parsecvs, `http://cgit.freedesktop.org/~keithp/parsecvs`
>  
>  Author
>  ------
maybe you can also add remarks about autocrlf and safecrlf?
both need to be off

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

[Heiko Voigt]

On Mon, Mar 23, 2009 at 09:33:55PM +0100, Ferry Huberts (Pelagic) wrote:
> maybe you can also add remarks about autocrlf and safecrlf?
> both need to be off

>From my experience thats not necessarily true. You can use
autocrlf=input to repair broken revisions were crlf's have been
mistakenly committed into the repository. And if I remember correctly
safecrlf helps if you want to make sure that no information gets lost.

So when importing from a nice correct cvs repository you would expect
safecrlf to not stop your import. And I suspect there are actually cvs
users that were very careful with their lineendings who would use it.

cheers Heiko

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

[Ferry Huberts (Pelagic)]

Heiko Voigt wrote:
> On Mon, Mar 23, 2009 at 09:33:55PM +0100, Ferry Huberts (Pelagic) wrote:
>> maybe you can also add remarks about autocrlf and safecrlf?
>> both need to be off
> 
> From my experience thats not necessarily true. You can use
> autocrlf=input to repair broken revisions were crlf's have been
> mistakenly committed into the repository. And if I remember correctly
> safecrlf helps if you want to make sure that no information gets lost.
> 
> So when importing from a nice correct cvs repository you would expect
> safecrlf to not stop your import. And I suspect there are actually cvs
> users that were very careful with their lineendings who would use it.
> 
> cheers Heiko
If you look at this thread:
http://thread.gmane.org/gmane.comp.version-control.git/110152/focus=110358
you'll see why I said it. I did some testing to prove my statement.

Ferry

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

[Heiko Voigt]

On Tue, Mar 31, 2009 at 07:36:16AM +0200, Ferry Huberts (Pelagic) wrote:
> Heiko Voigt wrote:
> > On Mon, Mar 23, 2009 at 09:33:55PM +0100, Ferry Huberts (Pelagic) wrote:
> >> maybe you can also add remarks about autocrlf and safecrlf?
> >> both need to be off
> > 
> > From my experience thats not necessarily true. You can use
> > autocrlf=input to repair broken revisions were crlf's have been
> > mistakenly committed into the repository. And if I remember correctly
> > safecrlf helps if you want to make sure that no information gets lost.
> > 
> > So when importing from a nice correct cvs repository you would expect
> > safecrlf to not stop your import. And I suspect there are actually cvs
> > users that were very careful with their lineendings who would use it.
> > 
> > cheers Heiko
> If you look at this thread:
> http://thread.gmane.org/gmane.comp.version-control.git/110152/focus=110358
> you'll see why I said it. I did some testing to prove my statement.

Well, from that thread I see my statement supported. It is not true that
they *need* to be off. Maybe a statement that certain crlf settings are
exclusive would be good, but I agree that should go into the config
documentation.

The main point I see here is that the User may not be aware that such a
conversion is applied so something like this could help.

cheers Heiko

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index e1fd047..d4e7fd4 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -40,6 +40,11 @@ probably want to make a bare clone of the imported repository
 and use the clone as the shared repository.
 See linkgit:gitcvs-migration[7].
 
+Note: All revisions are imported using the index so settings of
+core.autocrlf and core.safecrlf are applied. This way you can change or
+safety check the import. If you do not want this make sure these options
+are both set to false.
+
 
 OPTIONS
 -------

[PATCH] cvsimport: Add a note about crlf options to the documentation

[Heiko Voigt]

---
This is a proper resend of the crlf note patch for Junio's import convenience

 Documentation/git-cvsimport.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index e1fd047..d4e7fd4 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -40,6 +40,11 @@ probably want to make a bare clone of the imported repository,
 and use the clone as the shared repository.
 See linkgit:gitcvs-migration[7].
 
+Note: All revisions are imported using the index so settings of
+core.autocrlf and core.safecrlf are applied. This way you can change or
+safety check the import. If you do not want this make sure these options
+are both set to false.
+
 
 OPTIONS
 -------
-- 
1.6.1.2.390.gba743

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

[Ferry Huberts (Pelagic)]

Heiko Voigt wrote:
> On Tue, Mar 31, 2009 at 07:36:16AM +0200, Ferry Huberts (Pelagic) wrote:
>> Heiko Voigt wrote:
>>> On Mon, Mar 23, 2009 at 09:33:55PM +0100, Ferry Huberts (Pelagic) wrote:
>>>> maybe you can also add remarks about autocrlf and safecrlf?
>>>> both need to be off
>>> From my experience thats not necessarily true. You can use
>>> autocrlf=input to repair broken revisions were crlf's have been
>>> mistakenly committed into the repository. And if I remember correctly
>>> safecrlf helps if you want to make sure that no information gets lost.
>>>
>>> So when importing from a nice correct cvs repository you would expect
>>> safecrlf to not stop your import. And I suspect there are actually cvs
>>> users that were very careful with their lineendings who would use it.
>>>
>>> cheers Heiko
>> If you look at this thread:
>> http://thread.gmane.org/gmane.comp.version-control.git/110152/focus=110358
>> you'll see why I said it. I did some testing to prove my statement.
> 
> Well, from that thread I see my statement supported. It is not true that
> they *need* to be off. Maybe a statement that certain crlf settings are
> exclusive would be good, but I agree that should go into the config
> documentation.
> 
> The main point I see here is that the User may not be aware that such a
> conversion is applied so something like this could help.
> 
> cheers Heiko
> 
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index e1fd047..d4e7fd4 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -40,6 +40,11 @@ probably want to make a bare clone of the imported repository
>  and use the clone as the shared repository.
>  See linkgit:gitcvs-migration[7].
>  
> +Note: All revisions are imported using the index so settings of
> +core.autocrlf and core.safecrlf are applied. This way you can change or
> +safety check the import. If you do not want this make sure these options
> +are both set to false.
> +
>  

I can agree with this. However,
I still think that this is too weak a statement and a bit too cryptic:
the import will/can actually fail when a crlf conversion is performed,
even though safecrlf is not set to true. At least that was the case I
was talking about in the thread. I don't know the current situation, I
haven't tried it since 'cause I just use it with both set to false :-)
I discussed a patch for this but never got around to implementing it
since I'm now busy with ignore functionality for EGit.

cheers.

Ferry

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

[Jeff King]

On Mon, Mar 23, 2009 at 08:53:05PM +0100, Heiko Voigt wrote:

> The described issues are compiled from the tests by Michael Haggerty and me.
> Because it is not apparent that these can be fixed anytime soon at least warn
> unwary users not to rely on the inbuilt cvsimport to much.

I think this change is good in concept.

> +[[issues]]
> +ISSUES
> +------
> +Problems related to timestamps:
> +
> + * If timestamps of commits in the cvs repository are not stable enough
> +   to be used for ordering commits
> + * If any files were ever "cvs import"ed more than once (e.g., import of
> +   more than one vendor release)
> + * If the timestamp order of different files cross the revision order
> +   within the commit matching time window

Reading this, I kept waiting for the "then" to your "if". I think the
implication is "your import will be incorrect". But it would be nice to
say _how_, even if it's something as simple as "changes may show up in
the wrong commit, the wrong branch, be omitted" or whatever. Just give a
general idea of what can happen.

Also, this renders somewhat poorly in the manpage version. I get:

<quote>
ISSUES
       Problems related to timestamps:


       ·   If timestamps of commits in the cvs repository are not stable
           enough to be used for ordering commits

       ·   If any files were ever "cvs import"ed more than once (e.g., import
           of more than one vendor release)

       ·   If the timestamp order of different files cross the revision order
           within the commit matching time window
       Problems related to branches:


       ·   Branches on which no commits have been made are not imported
</quote>

Note the extra blank line between each heading and its list, and the
lack of a blank line between the end of the first list and the heading
of the second. Your source is very readable, so it really is just
asciidoc being silly, but I wonder if there is a way to work around
that.

-Peff

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

[Heiko Voigt]

On Mon, Mar 23, 2009 at 11:14:48PM -0400, Jeff King was talking about:
> On Mon, Mar 23, 2009 at 08:53:05PM +0100, Heiko Voigt wrote:
> 
> > The described issues are compiled from the tests by Michael Haggerty and me.
> > Because it is not apparent that these can be fixed anytime soon at least warn
> > unwary users not to rely on the inbuilt cvsimport to much.
> 
> I think this change is good in concept.
> 
> > +[[issues]]
> > +ISSUES
> > +------
> > +Problems related to timestamps:
> > +
> > + * If timestamps of commits in the cvs repository are not stable enough
> > +   to be used for ordering commits
> > + * If any files were ever "cvs import"ed more than once (e.g., import of
> > +   more than one vendor release)
> > + * If the timestamp order of different files cross the revision order
> > +   within the commit matching time window
> 
> Reading this, I kept waiting for the "then" to your "if". I think the
> implication is "your import will be incorrect". But it would be nice to
> say _how_, even if it's something as simple as "changes may show up in
> the wrong commit, the wrong branch, be omitted" or whatever. Just give a
> general idea of what can happen.

You are right, I actually wanted to update my patch but as I've seen
today my patch already made it into master. So I guess I will prepare an
update patch to address these issues.

> 
> Also, this renders somewhat poorly in the manpage version. I get:
> 
> <quote>
> ISSUES
>        Problems related to timestamps:
> 
> 
>        ·   If timestamps of commits in the cvs repository are not stable
>            enough to be used for ordering commits
> 
>        ·   If any files were ever "cvs import"ed more than once (e.g., import
>            of more than one vendor release)
> 
>        ·   If the timestamp order of different files cross the revision order
>            within the commit matching time window
>        Problems related to branches:
> 
> 
>        ·   Branches on which no commits have been made are not imported
> </quote>
> 
> Note the extra blank line between each heading and its list, and the
> lack of a blank line between the end of the first list and the heading
> of the second. Your source is very readable, so it really is just
> asciidoc being silly, but I wonder if there is a way to work around
> that.

My xmlto is not working at the moment. I will check that.

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

[Junio C Hamano]

Heiko Voigt <hvoigt@hvoigt.net> writes:

> On Mon, Mar 23, 2009 at 11:14:48PM -0400, Jeff King was talking about:
>> On Mon, Mar 23, 2009 at 08:53:05PM +0100, Heiko Voigt wrote:
>> 
>> > The described issues are compiled from the tests by Michael Haggerty and me.
>> > Because it is not apparent that these can be fixed anytime soon at least warn
>> > unwary users not to rely on the inbuilt cvsimport to much.
>> 
>> I think this change is good in concept.
>> 
>> > +[[issues]]
>> > +ISSUES
>> > +------
>> > +Problems related to timestamps:
>> > +
>> > + * If timestamps of commits in the cvs repository are not stable enough
>> > +   to be used for ordering commits
>> > + * If any files were ever "cvs import"ed more than once (e.g., import of
>> > +   more than one vendor release)
>> > + * If the timestamp order of different files cross the revision order
>> > +   within the commit matching time window
>> 
>> Reading this, I kept waiting for the "then" to your "if". I think the
>> implication is "your import will be incorrect". But it would be nice to
>> say _how_, even if it's something as simple as "changes may show up in
>> the wrong commit, the wrong branch, be omitted" or whatever. Just give a
>> general idea of what can happen.
>
> You are right, I actually wanted to update my patch but as I've seen
> today my patch already made it into master. So I guess I will prepare an
> update patch to address these issues.

Thanks.

[PATCH] Cleanup warning about known issues in cvsimport documentation

[Heiko Voigt]

Not all statements were complete sentences.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 Documentation/git-cvsimport.txt |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index e1fd047..ba6a50b 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -173,24 +173,26 @@ ISSUES
 Problems related to timestamps:
 
  * If timestamps of commits in the cvs repository are not stable enough
-   to be used for ordering commits
+   to be used for ordering commits changes may show up in the wrong
+   order.
  * If any files were ever "cvs import"ed more than once (e.g., import of
-   more than one vendor release)
+   more than one vendor release) the HEAD will be incorrect.
  * If the timestamp order of different files cross the revision order
-   within the commit matching time window
+   within the commit matching time window the order of commits may be
+   wrong.
 
 Problems related to branches:
 
- * Branches on which no commits have been made are not imported
+ * Branches on which no commits have been made are not imported.
  * All files from the branching point are added to a branch even if
-   never added in cvs
- * files added to the source branch *after* a daughter branch was
-   created: If previously no commit was made on the daugther branch they
-   will erroneously be added to the daughter branch in git
+   never added in cvs.
+ * This applies to files added to the source branch *after* a daughter
+   branch was created: If previously no commit was made on the daugther
+   branch they will erroneously be added to the daughter branch in git.
 
 Problems related to tags:
 
-* Multiple tags on the same revision are not imported
+* Multiple tags on the same revision are not imported.
 
 If you suspect that any of these issues may apply to the repository you
 want to import consider using these alternative tools which proved to be
-- 
1.6.1.2.390.gba743

Re: [PATCH] Cleanup warning about known issues in cvsimport documentation

[Jeff King]

On Tue, Mar 31, 2009 at 06:45:03PM +0200, Heiko Voigt wrote:

> Not all statements were complete sentences.

Thanks, this is looking much better. A few minor comments:

>   * If any files were ever "cvs import"ed more than once (e.g., import of
> -   more than one vendor release)
> +   more than one vendor release) the HEAD will be incorrect.

Incorrect how? I assume "contains the wrong content".

> + * This applies to files added to the source branch *after* a daughter
> +   branch was created: If previously no commit was made on the daugther
> +   branch they will erroneously be added to the daughter branch in git.

s/If/if/, s/daugther/daughter/

Other than that, looks good to me.

-Peff

[PATCH v2] Cleanup warning about known issues in cvsimport documentation

[Heiko Voigt]

Not all statements were complete sentences.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---

I corrected the typos. This is the interdiff:

 diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
 index ba6a50b..d7bab13 100644
 --- a/Documentation/git-cvsimport.txt
 +++ b/Documentation/git-cvsimport.txt
 @@ -176,7 +176,7 @@ Problems related to timestamps:
     to be used for ordering commits changes may show up in the wrong
     order.
   * If any files were ever "cvs import"ed more than once (e.g., import of
 -   more than one vendor release) the HEAD will be incorrect.
 +   more than one vendor release) the HEAD contains the wrong content.
   * If the timestamp order of different files cross the revision order
     within the commit matching time window the order of commits may be
     wrong.
 @@ -187,7 +187,7 @@ Problems related to branches:
   * All files from the branching point are added to a branch even if
     never added in cvs.
   * This applies to files added to the source branch *after* a daughter
 -   branch was created: If previously no commit was made on the daugther
 +   branch was created: if previously no commit was made on the daughter
     branch they will erroneously be added to the daughter branch in git.
  
  Problems related to tags:


 Documentation/git-cvsimport.txt |   20 +++++++++++---------
 1 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index e1fd047..d7bab13 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -173,24 +173,26 @@ ISSUES
 Problems related to timestamps:
 
  * If timestamps of commits in the cvs repository are not stable enough
-   to be used for ordering commits
+   to be used for ordering commits changes may show up in the wrong
+   order.
  * If any files were ever "cvs import"ed more than once (e.g., import of
-   more than one vendor release)
+   more than one vendor release) the HEAD contains the wrong content.
  * If the timestamp order of different files cross the revision order
-   within the commit matching time window
+   within the commit matching time window the order of commits may be
+   wrong.
 
 Problems related to branches:
 
- * Branches on which no commits have been made are not imported
+ * Branches on which no commits have been made are not imported.
  * All files from the branching point are added to a branch even if
-   never added in cvs
- * files added to the source branch *after* a daughter branch was
-   created: If previously no commit was made on the daugther branch they
-   will erroneously be added to the daughter branch in git
+   never added in cvs.
+ * This applies to files added to the source branch *after* a daughter
+   branch was created: if previously no commit was made on the daughter
+   branch they will erroneously be added to the daughter branch in git.
 
 Problems related to tags:
 
-* Multiple tags on the same revision are not imported
+* Multiple tags on the same revision are not imported.
 
 If you suspect that any of these issues may apply to the repository you
 want to import consider using these alternative tools which proved to be
-- 
1.6.2.1.424.g0b27.dirty

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

[Jeff King]

On Tue, Mar 31, 2009 at 12:36:46AM +0200, Heiko Voigt wrote:

> > Note the extra blank line between each heading and its list, and the
> > lack of a blank line between the end of the first list and the heading
> > of the second. Your source is very readable, so it really is just
> > asciidoc being silly, but I wonder if there is a way to work around
> > that.
> 
> My xmlto is not working at the moment. I will check that.

I looked into it a little more; it happens all over the place, so it is
a problem somewhere in the documentation toolchain. So don't worry about
it for this particular patch.

-Peff

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

[Jeff King]

On Tue, Mar 31, 2009 at 07:28:12AM -0400, Jeff King wrote:

> On Tue, Mar 31, 2009 at 12:36:46AM +0200, Heiko Voigt wrote:
> 
> > > Note the extra blank line between each heading and its list, and the
> > > lack of a blank line between the end of the first list and the heading
> > > of the second. Your source is very readable, so it really is just
> > > asciidoc being silly, but I wonder if there is a way to work around
> > > that.
> > 
> > My xmlto is not working at the moment. I will check that.
> 
> I looked into it a little more; it happens all over the place, so it is
> a problem somewhere in the documentation toolchain. So don't worry about
> it for this particular patch.

I looked into it more and posted to the docbook-apps list.  Here's what
I found out: the problem is fixed in docbook-xsl 1.74.3. However, our
template to prevent extra .sp in manpage-base.xml prevents it.

That fix is in 7ef0435 (spurious .sp in manpages, 2006-12-13), and I get
good output by reverting it and using docbook 1.74.3.

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?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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?

[PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Chris Johnsen]

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 <chris_johnsen@pobox.com>

---

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

Re: [PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Jeff King]

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 <chris_johnsen@pobox.com>

Acked-by: Jeff King <peff@peff.net>

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

Re: [PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

Re: [PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Jeff King]

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

Re: [PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Chris Johnsen]

On 2009 Apr 1, at 05:34, Jeff King wrote:
> 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.

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

Re: [PATCH] Documentation: use "spurious .sp" XSLT if DOCBOOK_SUPPRESS_SP is set

[Jeff King]

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

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