Add warning about known issues to documentation of cvsimport

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.

Quoted 4 lines

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.

Quoted 9 lines

+[[issues]]

Documentation/git-cvsimport.txt +8 -0

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

Quoted 2 lines

+ * 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.

On Mon, Mar 23, 2009 at 11:14:48PM -0400, Jeff King was talking about:

Quoted 17 lines · Heiko Voigt

Quoted 4 lines

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.

Quoted 9 lines

+[[issues]]

Documentation/git-cvsimport.txt +8 -0

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

Quoted 8 lines

Quoted 2 lines

+ * 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.

Not all statements were complete sentences.

Quoted 5 lines

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.

Quoted 6 lines · Heiko Voigt

Quoted 5 lines

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.

Quoted 1 line

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.

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?

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 <[email protected]>

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.

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.

Quoted 4 lines · Chris Johnsen

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.

Quoted 3 lines

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

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 <[email protected]>

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

Documentation/git-cvsimport.txt +27 -0

index b7a8c10..3123725 100644
@@ -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

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

Quoted 2 lines · Ferry Huberts (Pelagic)

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

Quoted 9 lines · Heiko Voigt

Quoted 2 lines · Ferry Huberts (Pelagic)

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.

Quoted 5 lines

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.

Quoted 9 lines · Ferry Huberts (Pelagic)

Quoted 8 lines · Heiko Voigt

Quoted 2 lines · Ferry Huberts (Pelagic)

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.

Quoted 8 lines

Quoted 5 lines

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

Documentation/git-cvsimport.txt +0 -0

index e1fd047..d4e7fd4 100644