Replace git-cvsimport with a rewrite that fixes major bugs.

The combination of git-cvsimport and cvsps had serious problems. Among these were:

(1) Analysis of branchy repos was buggy in multiple ways in both programs, leading to incorrect repo translations.

(2) Even after a correct branch analysis, extra (redundant) fileops would often be generated on the new-branch side.

(3) Inability to report more than one tag pointing to the same revision.

(4) Failure in certain cases of clock-skew reported by the t9603 test.

(5) Failure to use commitids for changeset ordering in cases were this would have prevented clock skew from causing incorrect grouping.

Problems 2-5 and portions of problem 1 have been solved by a major rewrite of cvsps (the 3.x release series); it now emits a git fast-import stream. Also, the buggy attempt at ancestry-branch tracking previously invoked by -A has been replaced with a simpler and better topo analysis. cvsps is now about 20% smaller than formerly.

All this changed cvsps's interface enough to require a complete rewrite of git-cvsimport (hence this patch). In the process the code size of the wrapper script dropped by about x3 and it can now support alternate conversion engines; the first new engine is cvs2git, with parsecvs expected to follow shortly if that code proves salvageable.

This patch also removes Michael Haggerty's git-cvsimport tests from the git tree. These are actually conversion-engine tests and have been merged into a larger cvsps test suite, which I intend to spin out into a general CVS-lifting test that can also be applied to utilities such as cvs2git and parsecvs.

The following known bug has not been fixed: "If any files were ever "cvs import"ed more than once (e.g., import of more than one vendor release) the HEAD contains the wrong content." However, cvsps now emits a warning in this case. There is also one pathological tagging case that was successful the former t9602 test that now fails (with a warning).

I plan to address these problems. This patch at least gets the cvsps-3.x/git-cvsimport combination to a state that is not too broken to ship - that is, in all failure cases known to me it now emits useful warnings rather than silently botching the import.

---
 Documentation/git-cvsimport.txt                    |  303 +++--
 Makefile                                           |    2 +-
 git-cvsimport.perl                                 | 1177 --------------------
 git-cvsimport.py                                   |  342 ++++++
 t/t9601-cvsimport-vendor-branch.sh                 |   85 --
 t/t9601/cvsroot/.gitattributes                     |    1 -
 t/t9601/cvsroot/CVSROOT/.gitignore                 |    2 -
 t/t9601/cvsroot/module/added-imported.txt,v        |   44 -
 t/t9601/cvsroot/module/imported-anonymously.txt,v  |   42 -
 .../module/imported-modified-imported.txt,v        |   76 --
 t/t9601/cvsroot/module/imported-modified.txt,v     |   59 -
 t/t9601/cvsroot/module/imported-once.txt,v         |   43 -
 t/t9601/cvsroot/module/imported-twice.txt,v        |   60 -
 t/t9602-cvsimport-branches-tags.sh                 |   78 --
 t/t9602/README                                     |   62 --
 t/t9602/cvsroot/.gitattributes                     |    1 -
 t/t9602/cvsroot/CVSROOT/.gitignore                 |    2 -
 t/t9602/cvsroot/module/default,v                   |  102 --
 t/t9602/cvsroot/module/sub1/default,v              |  102 --
 t/t9602/cvsroot/module/sub1/subsubA/default,v      |  101 --
 t/t9602/cvsroot/module/sub1/subsubB/default,v      |  107 --
 .../module/sub2/Attic/branch_B_MIXED_only,v        |   59 -
 t/t9602/cvsroot/module/sub2/default,v              |  102 --
 t/t9602/cvsroot/module/sub2/subsubA/default,v      |  102 --
 t/t9602/cvsroot/module/sub3/default,v              |  102 --
 t/t9603-cvsimport-patchsets.sh                     |   39 -
 t/t9603/cvsroot/.gitattributes                     |    1 -
 t/t9603/cvsroot/CVSROOT/.gitignore                 |    2 -
 t/t9603/cvsroot/module/a,v                         |   74 --
 t/t9603/cvsroot/module/b,v                         |   90 --
 30 files changed, 492 insertions(+), 2870 deletions(-)
 delete mode 100755 git-cvsimport.perl
 create mode 100755 git-cvsimport.py
 delete mode 100755 t/t9601-cvsimport-vendor-branch.sh
 delete mode 100644 t/t9601/cvsroot/.gitattributes
 delete mode 100644 t/t9601/cvsroot/CVSROOT/.gitignore
 delete mode 100644 t/t9601/cvsroot/module/added-imported.txt,v
 delete mode 100644 t/t9601/cvsroot/module/imported-anonymously.txt,v
 delete mode 100644 t/t9601/cvsroot/module/imported-modified-imported.txt,v
 delete mode 100644 t/t9601/cvsroot/module/imported-modified.txt,v
 delete mode 100644 t/t9601/cvsroot/module/imported-once.txt,v
 delete mode 100644 t/t9601/cvsroot/module/imported-twice.txt,v
 delete mode 100755 t/t9602-cvsimport-branches-tags.sh
 delete mode 100644 t/t9602/README
 delete mode 100644 t/t9602/cvsroot/.gitattributes
 delete mode 100644 t/t9602/cvsroot/CVSROOT/.gitignore
 delete mode 100644 t/t9602/cvsroot/module/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub1/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub1/subsubA/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub1/subsubB/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub2/Attic/branch_B_MIXED_only,v
 delete mode 100644 t/t9602/cvsroot/module/sub2/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub2/subsubA/default,v
 delete mode 100644 t/t9602/cvsroot/module/sub3/default,v
 delete mode 100755 t/t9603-cvsimport-patchsets.sh
 delete mode 100644 t/t9603/cvsroot/.gitattributes
 delete mode 100644 t/t9603/cvsroot/CVSROOT/.gitignore
 delete mode 100644 t/t9603/cvsroot/module/a,v
 delete mode 100644 t/t9603/cvsroot/module/b,v

index 98d9881..92da652 100644
@@ -9,75 +9,78 @@ git-cvsimport - Salvage your data out of another SCM people love to hate
 SYNOPSIS
 --------
 [verse]
-'git cvsimport' [-o <branch-for-HEAD>] [-h] [-v] [-d <CVSROOT>]
-	      [-A <author-conv-file>] [-p <options-for-cvsps>] [-P <file>]
-	      [-C <git_repository>] [-z <fuzz>] [-i] [-k] [-u] [-s <subst>]
-	      [-a] [-m] [-M <regex>] [-S <regex>] [-L <commitlimit>]
-	      [-r <remote>] [-R] [<CVS_module>]
-
+'git cvsimport' [-A <author-conv-file>] [-b] [-C <git_repository>] 
+              [-d <CVSROOT>] [-h] [-i] [-k] [-p <options-for-engine>] 
+	      [-P <cvsps-output-file>] [-r <remote>] [-R] [-s <subst>] 
+	      [-S <regex>] [-u] [-v] [-z <fuzz>] [<CVS_module>]
 
 DESCRIPTION
 -----------
-Imports a CVS repository into git. It will either create a new
+Imports a CVS repository into git. This tool will either create a new
 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
-to work with; after that, you need to 'git merge' incremental imports, or
-any CVS branches, yourself.  It is advisable to specify a named remote via
--r to separate and protect the incoming branches.
-
-If you intend to set up a shared public repository that all developers can
-read/write, or if you want to use linkgit:git-cvsserver[1], then you
-probably want to make a bare clone of the imported repository,
-and use the clone as the shared repository.
-See linkgit:gitcvs-migration[7].
+*WARNING:* The CVS model of version control lends itself to all manner
+of perversities; not all sequences of CVS operations can be translated
+into an import stream, and importing is not guaranteed to produce a
+perfectly accurate representation of CVS history. Please see the
+section on <<issues,engine-specific issues>> for further reference.
 
+git cvsimport will do well at translating CVS repositories with a
+linear or close-to-linear revision history, no merges, and
+well-disciplined tagging practices.  More complex cases will require
+human judgment amplified by a repository-editing tool such as
+http://www.catb.org/~esr/reposurgeon[reposurgeon].
 
 OPTIONS
 -------
--v::
-	Verbosity: let 'cvsimport' report what it is doing.
-
--d <CVSROOT>::
-	The root of the CVS archive. May be local (a simple path) or remote;
-	currently, only the :local:, :ext: and :pserver: access methods
-	are supported. If not given, 'git cvsimport' will try to read it
-	from `CVS/Root`. If no such file exists, it checks for the
-	`CVSROOT` environment variable.
+-A <author-conv-file>::
+	CVS by default uses the Unix username when writing its
+	commit logs. Using this option and an author-conv-file
+	maps the name recorded in CVS to author name, e-mail and
+	optional timezone:
++
+---------
+	exon=Andreas Ericsson <[email protected]> +0200
+	spawn=Simon Pawn <[email protected]> -0500
+---------
++
+'git cvsimport' will make it appear as those authors had
+their GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL set properly
+all along.
++
+For convenience, this data is saved to `$GIT_DIR/cvs-authors`
+each time the '-A' option is provided and read from that same
+file each time 'git cvsimport' is run.
++
+It is not recommended to use this feature if you intend to
+export changes back to CVS again later with
+'git cvsexportcommit'.
 
-<CVS_module>::
-	The CVS module you want to import. Relative to <CVSROOT>.
-	If not given, 'git cvsimport' tries to read it from
-	`CVS/Repository`.
+-b::
+        Create a bare repo. If you intend to set up a shared public
+	repository that all developers can read/write, or if you want
+	to use linkgit:git-cvsserver[1], then you probably want to
+	make a bare clone of the imported repository using this
+	option. See linkgit:gitcvs-migration[7].
 
 -C <target-dir>::
         The git repository to import to.  If the directory doesn't
         exist, it will be created.  Default is the current directory.
 
--r <remote>::
-	The git remote to import this CVS repository into.
-	Moves all CVS branches into remotes/<remote>/<branch>
-	akin to the way 'git clone' uses 'origin' by default.
+-d <CVSROOT>::
+	The root of the CVS archive. It is only necessary to specify
+	this option if you are running from somewhere other than a 
+	CVS checkout directory; the value is passed to the conversion
+	engine to be interpreted.
 
--o <branch-for-HEAD>::
-	When no remote is specified (via -r) the 'HEAD' branch
-	from CVS is imported to the 'origin' branch within the git
-	repository, as 'HEAD' already has a special meaning for git.
-	When a remote is specified the 'HEAD' branch is named
-	remotes/<remote>/master mirroring 'git clone' behaviour.
-	Use this option if you want to import into a different
-	branch.
-+
-Use '-o master' for continuing an import that was initially done by
-the old cvs2git tool.
+-e <engine>::
+	Splitting the CVS log into patch sets is done by an engine program,
+	which must emit a git fast-import stream to standard output.
+	This option changes the engine used; when given, it must be the
+	first option on the command line.
+
+-h::
+	Print a short usage message and exit.
 
 -i::
 	Import-only: don't perform a checkout after importing.  This option

@@ -89,75 +92,19 @@ the old cvs2git tool.
 	to avoid noisy changesets. Highly recommended, but off by default
 	to preserve compatibility with early imported trees.
 
--u::
-	Convert underscores in tag and branch names to dots.
-
--s <subst>::
-	Substitute the character "/" in branch names with <subst>
-
--p <options-for-cvsps>::
-	Additional options for cvsps.
-	The options '-u' and '-A' are implicit and should not be used here.
-+
-If you need to pass multiple options, separate them with a comma.
-
--z <fuzz>::
-	Pass the timestamp fuzz factor to cvsps, in seconds. If unset,
-	cvsps defaults to 300s.
-
 -P <cvsps-output-file>::
-	Instead of calling cvsps, read the provided cvsps output file. Useful
-	for debugging or when cvsps is being handled outside cvsimport.
-
--m::
-	Attempt to detect merges based on the commit message. This option
-	will enable default regexes that try to capture the source
-	branch name from the commit message.
-
--M <regex>::
-	Attempt to detect merges based on the commit message with a custom
-	regex. It can be used with '-m' to enable the default regexes
-	as well. You must escape forward slashes.
-+
-The regex must capture the source branch name in $1.
-+
-This option can be used several times to provide several detection regexes.
-
--S <regex>::
-	Skip paths matching the regex.
+	Instead of calling a conversion engine, read the provided
+	import-stream file. Useful for debugging or when the first
+	stage of conversion is being handled outside cvsimport.
 
--a::
-	Import all commits, including recent ones. cvsimport by default
-	skips commits that have a timestamp less than 10 minutes ago.
-
--L <limit>::
-	Limit the number of commits imported. Workaround for cases where
-	cvsimport leaks memory.
-
--A <author-conv-file>::
-	CVS by default uses the Unix username when writing its
-	commit logs. Using this option and an author-conv-file
-	maps the name recorded in CVS to author name, e-mail and
-	optional timezone:
-+
----------
-	exon=Andreas Ericsson <[email protected]>
-	spawn=Simon Pawn <[email protected]> America/Chicago
+-r <remote>::
+	The git remote to import this CVS repository into.
+	Moves all CVS branches into remotes/<remote>/<branch>
+	akin to the way 'git clone' uses 'origin' by default.
 
----------
-+
-'git cvsimport' will make it appear as those authors had
-their GIT_AUTHOR_NAME and GIT_AUTHOR_EMAIL set properly
-all along.  If a timezone is specified, GIT_AUTHOR_DATE will
-have the corresponding offset applied.
-+
-For convenience, this data is saved to `$GIT_DIR/cvs-authors`
-each time the '-A' option is provided and read from that same
-file each time 'git cvsimport' is run.
-+
-It is not recommended to use this feature if you intend to
-export changes back to CVS again later with
-'git cvsexportcommit'.
+-p <options-for-engine>::
+	Additional options for the engine. If you need to pass
+	multiple options, separate them with a comma.
 
 -R::
 	Generate a `$GIT_DIR/cvs-revisions` file containing a mapping from CVS

@@ -174,50 +121,98 @@ doing incremental imports.
 +
 This option may be useful if you have CVS revision numbers stored in commit
 messages, bug-tracking systems, email archives, and the like.
++
+-s <subst>::
+	Substitute the character "/" in branch names with <subst>
 
--h::
-	Print a short usage message and exit.
-
-OUTPUT
-------
-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.
+-S <regex>::
+	Skip paths matching the regex.
 
-[[issues]]
-ISSUES
-------
-Problems related to timestamps:
+-u::
+	Convert underscores in tag and branch names to dots.
 
- * If timestamps of commits in the CVS repository are not stable enough
-   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 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.
+-v::
+	Verbosity: let 'cvsimport' report what it is doing.
 
-Problems related to branches:
+-z <fuzz>::
+	Pass the timestamp fuzz factor, in seconds. If unset, this has 
+	an engine-dependent default - usually 300s.
 
- * 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.
- * 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.
+<CVS_module>::
+	The CVS module you want to import. Relative to <CVSROOT>.  It
+	is only necessary to specify this option if you are running
+	from somewhere other than a CVS checkout directory; the value
+	is passed to the conversion engine to be interpreted.
 
-Problems related to tags:
+OUTPUT
+------
+If '-v' is specified, the program reports what it is doing.
 
-* Multiple tags on the same revision are not imported.
+Otherwise, success is indicated the Unix way, i.e. by simply exiting with
+a zero exit status.
 
-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 practice:
+[[compatibility]]
+COMPATIBILITY
+-------------
+In 2012 two serious bugs dating back to 2006 in cvsps were exposed.  The
+ancestry-branch tracking formerly enabled by -A did not work, and
+branch detection was generally buggy; translations of branchy repos
+could be mangled.  While the --fast-export mode in 3.x releases of
+cvsps solved the problem, it required an emergency rewrite of
+git-cvsimport.  Some compatibility with older versions was unavoidably
+lost
+
+The -a, -o, -m, -M, and -L options in older versions of this tool have
+been removed (in effect, -a is always on; you can negate it with
+suitably crafted -d arguments).  Certain older versions could take a
+named timezone (like "America/Chicago") in an author-map file rather
+than just a [+-]hhmm offset; this version doesn't do that, but the
+capability may be restored in a future release.
 
-* cvs2git (part of cvs2svn), `http://cvs2svn.tigris.org`
-* parsecvs, `http://cgit.freedesktop.org/~keithp/parsecvs`
+[[issues]]
+ENGINE-SPECIFIC ISSUES
+----------------------
+The conversion engines try to warn you about repository histories they
+can't handle; see their individual manual pages to learn how to
+interpret the warnings you may receive.
+
+The default conversion engine is 'cvsps'.  If warnings you receive
+suggest that the repository translation is invalid, consider switching
+engines to 'cvs2git'.
+
+cvsps
+~~~~~
+The default conversion engine is 'cvsps'; at least version 3.3 is
+required.  The cvsps project page is at `http://www.catb.org/~esr/cvsps`.
+Things to know about this engine:
+
+* As well as working from within a CVS checkout directory,
+  'cvsps' will also adapt without requiring a root or module
+  specification when run from within a module subdirectory within a CVS
+  repository directory.  When run from within the top level of a CVS
+  repository, 'cvsps' requires only a module argument.
+
+* The -S option will interpret exclusion regular expressions using the
+  POSIX syntax of regex(7).
+
+* 'cvsps' automatically removes characters in CVS tag and branch names
+  that would be illegal in git.
+
+cvs2git
+~~~~~~~
+The cvs2git project page is at `http://cvs2svn.tigris.org`.  It is
+much slower than cvsps, and does not implement some git-cvsimport
+options (such as -d and -A), but it handles a wider range of
+pathological CVS cases.
+
+* 'cvs2git' takes a path option pointing to a repository module
+  subdirectory, defaulting to ".".
+
+* The -S option will interpret exclusion regular expressions using
+  Python syntax.
+
+* Illegal characters in branch and tag names will cause cvs2git to
+  abort with an error message.
 
 GIT
 ---

index 736ecd4..626494e 100644
@@ -464,7 +464,6 @@ SCRIPT_PERL += git-add--interactive.perl
 SCRIPT_PERL += git-difftool.perl
 SCRIPT_PERL += git-archimport.perl
 SCRIPT_PERL += git-cvsexportcommit.perl
-SCRIPT_PERL += git-cvsimport.perl
 SCRIPT_PERL += git-cvsserver.perl
 SCRIPT_PERL += git-relink.perl
 SCRIPT_PERL += git-send-email.perl

@@ -472,6 +471,7 @@ SCRIPT_PERL += git-svn.perl
 
 SCRIPT_PYTHON += git-remote-testgit.py
 SCRIPT_PYTHON += git-p4.py
+SCRIPT_PYTHON += git-cvsimport.py
 
 SCRIPTS = $(patsubst %.sh,%,$(SCRIPT_SH)) \
 	  $(patsubst %.perl,%,$(SCRIPT_PERL)) \

deleted file mode 100644
index 562b12e..0000000
@@ -1 +0,0 @@
-* -whitespace

deleted file mode 100644
index 3bb9b34..0000000
@@ -1,2 +0,0 @@
-history
-val-tags

deleted file mode 100644
index 562b12e..0000000
@@ -1 +0,0 @@
-* -whitespace

deleted file mode 100644
index 3bb9b34..0000000
@@ -1,2 +0,0 @@
-history
-val-tags

deleted file mode 100755
index 52034c8..0000000

deleted file mode 100644
index 562b12e..0000000

deleted file mode 100644
index 3bb9b34..0000000

1.8.1.rc2



-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Part of the diff is not shown here. View the raw message.

So..., is this a flag-day patch?

After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? As long as they update both cvsps and cvsimport, they can continue using the existing repository to get updates from the same upstream CVS repository without losing hisory continuity?

I would have preferred an addition of "git cvsimport-new" (or rename of the existing one to "git cvsimport-old"), with additional tests that compare the results of these two implemenations on simple CVS history that cvsimport-old did *not* screw up, to ensure that (1) people with existing set-up can choose to keep using the old one, perhaps by tweaking their process to use cvsimport-old, and (2) the updated one will give these people the identical conversion results, as long as the history they have been interacting with do not have the corner cases that trigger bugs in older cvsps.

Or am I being too conservative?

Junio C Hamano <[email protected]>:

Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken.

There was an error or typo in the branch-analysis code, dating from 2006 and possibly earlier, that meant that branch root points would almost always be attributed to parent patchsets one patchset earlier than they should have been. Shocked me when I found it - how was this missed for six years?

Because of the way the analysis is done, this fundamental bug would also cause secondary damage like file changes near the root point getting attributed to the wrong branch. In fact, this is how I first spotted the problem; my test suite exhibited this symptom.

And mind you this is on top of ancestry-branch tracking not working - two separate bugs that could interact in ways I'd really rather not think about. The bottom line is that every import of a branchy CVS repo with a pre-3.x version of cvsps is probably wrong.

The old git-cvsimport code was doing its part to screw things up, too. At least three of the bugs on its manual page are problems I couldn't reproduce using a bare cvsps instance, even the old broken version.

Yes, but in that case I would strongly advise re-importing the entire CVS history, as the portion analyzed with 2.2b1 and earlier versions of cvsps will almost certainly have been somewhat garbled if it contains any branches.

I think you are being too conservative. This patch is *not* a mere feature upgrade. The branch-analysis bug I found three days ago is not a minor problem, it is a big ugly showstopper for any case beside the simplest linear histories. Only linear histories will not break.

'People with existing set-ups' should absolutely *not* 'keep using the old one'; we should yank that choice away from them and get the old cvsimport/cvsps pair out of use *as fast as possible*, because it silently mangles branchy imports.

Accordingly, giving people the idea that it's OK to use old and new versions in parallel would be an extremely bad idea. I would go so far as to call it irresponsible.

Here is what I have done to ease the transition:

If you try to use old git-cvsimport with new cvsps, new cvsps will detect this and ship a message to stderr telling you to upgrade

If you try to use new git-cvsimport with old cvsps, old cvsps will complain of an invalid argument and git-cvsimport will quit.

As for testing...cvsps now has several dozen self-tests on five different CVS repositories, including improved versions of the t960[123] tests. I will keep developing these as I work on bringing parsecvs up to snuff.

I don't think there is a lot of point in git-cvsimport having its own tests any more. If you read it I think you'll see why; it's a much thinner wrapper around the conversion engine(s) than it used to be. In particular, it no longer does its own protocol transactions to the CVS server.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Hi,

Speaking with my Debian packager hat on: the updated cvsps is not available in Debian. "git cvsimport" is, and it has users that report bugs from time to time. With this change, I would either have to take on responsibility for maintenance of the cvsps package (not going to happen) or drop "git cvsimport". That's a serious regression.

The moment someone takes care of packaging the updated cvsps, I'll stop minding, though. ;-)

I wouldn't be surprised if the situation on other OSes is similar. This is too early to require such a dependency.

Hope that helps,
Jonathan

Jonathan Nieder <[email protected]>:

How does going from "it silently damages imports" to "it fails with an error message" constitute a regression?

I'll ping the Debian QA group.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

The former is already loudly advertised in the package description and manpage, at least lets you get work done, and works fine for simple repositories with linear history.

Taking away a command that people have been using in everyday work is pretty much a textbook example of a regression, no?

Hope that helps,
Jonathan

Jonathan Nieder <[email protected]>:

Two of the three claims in this paragraph are false. The manual page does not tell you what is true, which is that old cvsps will fuck up every branch by putting the root point at the wrong place. And if you call silently and randomly damaging imports getting work done, your definitions of "work" and "done" are broken.

That would be, but we are talking about replacing total breakage with a git-cvsimport that actually works and that you invoke in pretty much the same way as the old one. Nothing is or will be taken away.

In any case, once the distros package cvsps 3.x, old cvsimport will terminate with an error return, because cvsps-3.x sees an obsolete option that git-cvsimport tries to use as a command to treminate after displaying a prompt to upgrade.

The most we can accomplish by being "conservative" is to lengthen the window during which people will falsely believe that their conversion process is working.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Give me a break.

Are you telling me that when multiple users read a manpage that states

| WARNING: for certain situations the import leads to incorrect | results. Please see the section ISSUES for further reference. [...]
| Problems related to timestamps:
[...]
| Problems related to branches:
[...]
| Problems related to tags:
[...]
| consider using these alternative tools which proved to be more | stable in practice:

and a package description that states

| The git cvsimport tool can incrementally import from a repository
| that is being actively developed and only requires remote access
| over CVS protocol. Unfortunately, in many situations the import
| leads to incorrect results. For reliable, one-shot imports, cvs2git
| from the cvs2svn package or parsecvs may be a better fit.

and decide to use the tool anyway, this is not evidence that the tool is invaluable to them, despite its shortcomings?

Perhaps the users reporting bugs didn't read the manpage and package description (despite quoting the same passages and explaining why they used the command nonetheless) or I should ignore the judgement calls they make.

Consider the following workflow:

1. Update imported project periodically using git-cvsimport
2. Hack, do code archaeology using "git log -S" and "git bisect", etc.
3. Fall back to a web browser and cvsweb to confirm conclusions.

You are telling me that it is not a regression to change the workflow to the following:

1. Try to use git-cvsimport.
2. Wonder where that command went.

Meanwhile Junio has already suggested a way out. Just rename the command.

Hope that helps,
Jonathan

First of all, I am at the same time a sad, nostalgic, and very happy that old cvsimport is getting replaced.

The existing cvsps/cvsimport combo work for CVS repos with simple branches, and can track those over time.

Replacement with something more solid is welcome, but until you are extremely confident of its handling of legacy setups... I would still provide the old cvsimport, perhaps in contrib.

cheers,

m
--
[email protected]
[email protected] -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff

Martin Langhoff <[email protected]>:

I am extremely confident. I built a test suite so I could be.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

I too am glad to see some work go into the cvsimport script. So just to clear things up, previously you said this:

This is the problem, and one that is easily "solved" by just keeping a copy of the old command.

Remember that for many users of these tools it doesn't matter if the history is correct or not, as long as the head checkout contains the right files and they are able to submit new changes. With this definition of "works" git-cvsimport is not that broken I think.

Cheers,
- Thomas

This is rather off-putting. Really.

I dealt with enough CVS repos to see that the branch point could be ambiguous, and that some cases were incurably ugly and ambiguous.

Off the top of my head I can recall

- Files created on a branch appear on HEAD (if the cvs client was
well behaved, in HEAD's attic, if the cvs client was buggy... )

- Files tagged with the branch at a much later time. Scenario is a
developer opening/tagging a new branch mindlessly on a partial checkout; then trying to "fix" the problem later.

My best guess is that you haven't dealt with enough ugly CVS repos. I used to have the old original X.org repos, but no more. Surely Mozilla's fugly old CVS repos are up somewhere, and may be therapeutic.

cheers,

m
--
[email protected]
[email protected] -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff

Martin Langhoff <[email protected]>:

You are quite right, but you have misintepreted the subject of my confidence. I am under no illusion that the new cvsimport/cvsps pair is a perfect solution to the CVS-lifting problem, nor even that such a solution is possible.

Thanks, but since I wrote reposurgeon in 2010 I've done more conversions of messy CVS and Subversion repositories than I can easily remember (the Subversion ones being relevant because they often have truly nasty CVS artifacts in their early history). Just off the top of my head there's been gpsd, the Network Utility Tools, Roundup, SSTK2000, the Hercules project, and robotfindskitten. And a raft of smaller projects - I sought them out as torture tests for reposurgeon.

I am therefore intimately, painfully familiar with how bad CVS repos can get. I take it as given that there are still boojums that will perplex my tools lurking out there in the unexplored jungle.

In fact, this very kind of prior experience had been a major motivation for reposurgeon. I became convinced several years back that the batchy design philosophy of conventional repo-conversion tools was flawed, not flexible enough to deal with the real-world messes out there. So I wrote reposurgeon to amplify human judgment rather than try to replace it.

An example of the batchiness mistake close to home is the -m and -M options in the old version of cvsimport. It takes human judgment looking at the whole commit DAG in gitspace to decide what merge points would best express the (as you say, sometimes ambiguous) CVS history - what's needed is a scalpel and sutures in a surgeon's hand, not a regexp hammer.

For extended discussion, see my blog post "Repositories In Translation" at http://esr.ibiblio.org/?p=3859 in which I argue that the process has much more in common with the ambiguity of literary translation than is normally understood.

No, what I am very confident about is the performance and stability of the new cvsps/cvsimport code on *the cases the old code handled* - and a fairly well-defined larger group of many more cases.

My confidence is derived from having built a test suite that incorporates and improves on the git-tree tests. I don't have to merely guess or hope that the new code works better, I can exhibit tests that demonstrate it.

Among my near-term to-do items are applying those tests to cvs2git and parsecvs. But I first need to get parsecvs working again; presently, as I've inherited it, it does not correctly create a HEAD reference in the translated git repo.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Thanks. That is a much more reassuring stance.

m
--
[email protected]
[email protected] -- Software Architect - OLPC
- ask interesting questions
- don't get distracted with shiny stuff - working code first
- http://wiki.laptop.org/go/User:Martinlanghoff

That doesn't look like being a widespread problem, or more people would have complained.

Andreas.

Andreas Schwab, [email protected]
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Would it be that not many people use branchy history in CVS, as merging there was soooo painful?

That is exactly my point. It never worked in a branchy history, and that is an indication that people who didn't complain and used the old cvsimport with branch-incapable cvsps happily would have been working with a linear history. Either nobody uses cvsimport in the daily work, in which case a flag-day is perfectly fine, or we will have many people who are forced to update to unproven version for no immediate upside because the upstream repositories they work with, or options they use cvsimport, do not trigger the multi-branch bug.

I however do understand that updating is the only sensible thing to do for them *in the longer term*, as older cvsimport and cvsps are no longer maintained, and sooner they update the better the chance the new cvsimport becomes perfect earlier.

I still am not convinced, especially without a "we make sure we do not regress in linear histories" side-by-side test in place. That sounds irresponsible.

But others may disagree, and I'd have to sleep on it.

I'd prefer to hear from somebody who is *not* defending on his newer implementation, but from somebody who is actively using cvsimport as an end user. On the end-users' side, there always is this anxiety that a radical rewrite will always introduce new bugs, even when they know the rewrite is done very competently.

Sounds sensible.

With an error message that tells the user to update cvsps, this also sounds sensible.

I see an opening for smoother transition here.

Like it or not, you cannot force distros to ship with cvsps 3.0 when we ship our 1.8.2 (or 2.0 or whatever) that includes a cvsimport that requires cvsps 3.0. The best we can do is to make it capable of working with cvsps 3.0 for a better result (when available), and working with cvsps 2.0 in a limited way as ever (linear history only, etc. etc.) when cvsps 3.0 is not available.

As your version already knows how to detect the case where cvsps is too old to operate with it, I imagine it to be straight-forward to ship the old cvsimport under obscure name, "git cvsimport--old" or something, and spawn it from your version when necessary, perhaps after issuing a warning "cvsps 3.0 not found; switching to an old and unmaintained version of cvsimport..."

That way, people who have been happily working with linear CVS histories with the old limited tool can keep using the same set-up until their distro update their cvsps, without harming people who need to work with more complex CVS histories, who can choose to update their cvsps early themselves as $HOME/bin/cvsps earlier on their $PATH.

By "cvsimport" (the current version), we are talking about a piece of software that has been used in the field for more than 5 years, still with a handful of patches to enhance it in the past two years. A flag-day "this hot-off-the-press version is infinitely better" replacement is not an option, especially when we can expect that existing users are not asking for an "inifinitely better" version (they rather prefer "stable" in the "works just as before" sense), even when the hot-off-the-press version *is* infinitely better in some use cases such as dealing with branchy histories.

Junio C Hamano <[email protected]>:

This can be done. As this may not be the last case in which it comes up, perhaps we should have an 'obsolete' directory distinct from 'contrib'.

I'll ship another patch.

		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Alright; thanks.

Don't forget to sign-off your patch ;-)

The issue is real, but I am afraid that the above is not sufficient because outdir can contain single quotes. I think other places that call out to external processes share the same issue of being careless about quoting in general.

Doesn't Python come with a standard subprocess module that lets you spawn external programs safely, similar to the way Perl's list form open(), e.g. "open($fh, "-|", 'git', @args)", works?

You mean something like this:

p1 = subprocess.Popen([backend.command()], stdout=subprocess.PIPE) subprocess.Popen(["git", "fast-import", "--quiet"] + gitopts,
cwd=outdir, stdin=p1.stdout)

Assuming gitopts is a list rather than a string. (care must be taken with backend.command() also)

... and of course a more boring "system('git', $subcmd, @args)", as well.

Yes.

I vaguely recall that the subprocess module once used to be one portability issue but that was between Python 2.3 and 2.4 or some ancient history, and it should no longer be relevant.

Python's os.system() takes exactly one argument, which must be a string, and executes it in a subshell. subprocess is indeed the way to go.

subprocess was added in Python 2.4, and the above example should work fine in any version >= 2.4. But please note that other functions have been added to the module since then, like check_call() (v2.5), check_output (v2.7), and some methods were added to the Popen object in v2.6.

Such things are documented pretty reliably in the Python library documentation [1]; when in doubt, one can view older versions of the library documentation, which are all available online [2].

Michael

[1] http://docs.python.org/2/library/
[2] http://www.python.org/doc/versions/

Michael Haggerty
[email protected]
http://softwareswirl.blogspot.com/