[PATCH 0/4] Add more tests of cvsimport

The test suite for "git cvsimport" is pretty limited, and I would like
to improve the situation.  This patch series contains the first of
what I hope will eventually be several additions to the "git
cvsimport" test suite.

I am the maintainer of cvs2svn/cvs2git.  Most of the new tests will
probably use fragments from the cvs2svn test suite.  I should admit
that part of my motivation for adding tests to the "git cvsimport"
test suite is to document its weaknesses, which do not seem to be
especially well known.

Patch 1 splits out some code into a library usable by multiple
CVS-related tests.

Patch 2 changes the library to add the -f option when invoking cvs (to
make it ignore the user's ~/.cvsrc file).

Patch 3 adds a new test to t9600, namely to compare the entire module
as checked out by CVS vs. git.

Patch 4 adds a new test script t9601 that tests "git cvsimport"'s
handling of CVS vendor branches.  One of these tests fails due to an
actual bug.

These ideas in the patches are logically independent of each other,
but each patch assumes that the previous patches have been applied.

I would like to point out a few things about these patches that seem a
little bit unprecedented in the git test suite.  If other approaches
would be preferred, please let me know.

The first is that I would like to introduce a library that can be used
by the "git cvsimport" tests in the t96xx series, simply to avoid code
duplication.  I put this library in t/t96xx/cvs-lib.sh, to hopefully
make its role clear.  The library has to be sourced from the main test
directory.  (It sources test-lib.sh indirectly.)

The second is that the new test script uses a small CVS repository
that is part of the test suite (i.e., the *,v files are committed
directly into the git source tree).  This is different than the
approach of t9600, which creates its own test CVS repository using CVS
commands.  The reasons for this are:

- t9600 wants to test incremental import, so it *has to* create the
  repository dynamically.  That is not the case for t9601, which only
  tests a one-shot import.

- The repository for t9601 is derived from one that already exists as
  part of the cvs2svn test suite.  Reverse-engineering it into CVS
  commands would be extra work.

- The code to create CVS repositories via CVS commands is not very
  illuminating, and runs slowly, as CVS throttles commits to 1 per
  second (to ensure unique timestamps).

- Future tests may require even more complicated CVS repositories that
  are even more cumbersome to create, so it's good to set a precedent
  now :-)

Finally, the *,v files comprising the CVS repository have blank
trailing lines, triggering a warning from "git diff --check".  I don't
think that CVS strictly requires the blank lines, but they are always
generated by CVS, so I left them in.  But if the "git diff --check"
warnings are considered a serious problem, the blank lines could
probably be removed.

Cheers,
Michael

For now the "library" just includes code (moved from
t/t9600-cvsimport.sh) that checks whether the prerequisites for "git
cvsimport" are installed.

Signed-off-by: Michael Haggerty <redacted>
---
 t/t9600-cvsimport.sh |   29 +----------------------------
 t/t96xx/cvs-lib.sh   |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 28 deletions(-)
 create mode 100644 t/t96xx/cvs-lib.sh

diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index d2379e7..b4b9896 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh

@@ -1,37 +1,10 @@
 #!/bin/sh
 
 test_description='git cvsimport basic tests'
-. ./test-lib.sh
+. ./t96xx/cvs-lib.sh
 
 CVSROOT=$(pwd)/cvsroot
 export CVSROOT
-unset CVS_SERVER
-# for clean cvsps cache
-HOME=$(pwd)
-export HOME
-
-if ! type cvs >/dev/null 2>&1
-then
-	say 'skipping cvsimport tests, cvs not found'
-	test_done
-	exit
-fi
-
-cvsps_version=`cvsps -h 2>&1 | sed -ne 's/cvsps version //p'`
-case "$cvsps_version" in
-2.1 | 2.2*)
-	;;
-'')
-	say 'skipping cvsimport tests, cvsps not found'
-	test_done
-	exit
-	;;
-*)
-	say 'skipping cvsimport tests, unsupported cvsps version'
-	test_done
-	exit
-	;;
-esac
 
 test_expect_success 'setup cvsroot' 'cvs init'
 

diff --git a/t/t96xx/cvs-lib.sh b/t/t96xx/cvs-lib.sh
new file mode 100644
index 0000000..bfc1c12
--- /dev/null
+++ b/t/t96xx/cvs-lib.sh

@@ -0,0 +1,31 @@
+#!/bin/sh
+
+. ./test-lib.sh
+
+unset CVS_SERVER
+# for clean cvsps cache
+HOME=$(pwd)
+export HOME
+
+if ! type cvs >/dev/null 2>&1
+then
+	say 'skipping cvsimport tests, cvs not found'
+	test_done
+	exit
+fi
+
+cvsps_version=`cvsps -h 2>&1 | sed -ne 's/cvsps version //p'`
+case "$cvsps_version" in
+2.1 | 2.2*)
+	;;
+'')
+	say 'skipping cvsimport tests, cvsps not found'
+	test_done
+	exit
+	;;
+*)
+	say 'skipping cvsimport tests, unsupported cvsps version'
+	test_done
+	exit
+	;;
+esac

-- 
1.6.1.3

Test added for completeness (it passes).

Signed-off-by: Michael Haggerty <redacted>
---
 t/t9600-cvsimport.sh |    2 ++
 t/t96xx/cvs-lib.sh   |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 66393ae..dad9d49 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh

@@ -121,4 +121,6 @@ test_expect_success 'import from a CVS working tree' '
 
 '
 
+test_expect_success 'test entire HEAD' 'test_cmp_branch_tree master'
+
 test_done

diff --git a/t/t96xx/cvs-lib.sh b/t/t96xx/cvs-lib.sh
index 6738901..0136b36 100644
--- a/t/t96xx/cvs-lib.sh
+++ b/t/t96xx/cvs-lib.sh

@@ -32,3 +32,28 @@ case "$cvsps_version" in
 	exit
 	;;
 esac
+
+test_cvs_co () {
+	# Usage: test_cvs_co BRANCH_NAME
+	if [ "$1" = "master" ]
+	then
+		$CVS co -P -d module-cvs-"$1" -A module
+	else
+		$CVS co -P -d module-cvs-"$1" -b "$1" module
+	fi
+}
+
+test_git_co_branch () {
+	# Usage: test_git_co BRANCH_NAME
+	(cd module-git && git checkout "$1")
+}
+
+test_cmp_branch_tree () {
+	# Usage: test_cmp_branch_tree BRANCH_NAME
+	# Check BRANCH_NAME out of CVS and git and make sure that all
+	# of the files and directories are identical.
+
+	test_cvs_co "$1" &&
+	test_git_co_branch "$1" &&
+	diff -r -x .git -x CVS module-cvs-"$1" module-git
+}

-- 
1.6.1.3

CVS's handling of vendor branches is tricky; add some tests to check
whether revisions added via "cvs imports" then imported to git via
"git cvsimport" are reflected correctly on master.

One of these tests fail and is therefore marked "test_expect_failure".
Cvsimport doesn't realize that subsequent changes on a vendor branch
affect master as long as the vendor branch is the default branch.

The test CVS repository used for these tests is derived from cvs2svn's
test suite.

Signed-off-by: Michael Haggerty <redacted>
---
 t/t9601-cvsimport-vendor-branch.sh                 |   86 ++++++++++++++++++++
 t/t9601/cvsroot/CVSROOT/.gitignore                 |    1 +
 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/t96xx/cvs-lib.sh                                 |    6 ++
 9 files changed, 417 insertions(+), 0 deletions(-)
 create mode 100755 t/t9601-cvsimport-vendor-branch.sh
 create mode 100644 t/t9601/cvsroot/CVSROOT/.gitignore
 create mode 100644 t/t9601/cvsroot/module/added-imported.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-anonymously.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-modified-imported.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-modified.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-once.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-twice.txt,v

diff --git a/t/t9601-cvsimport-vendor-branch.sh b/t/t9601-cvsimport-vendor-branch.sh
new file mode 100755
index 0000000..179898b
--- /dev/null
+++ b/t/t9601-cvsimport-vendor-branch.sh

@@ -0,0 +1,86 @@
+#!/bin/sh
+
+# Description of the files in the repository:
+#
+#    imported-once.txt:
+#
+#       Imported once.  1.1 and 1.1.1.1 should be identical.
+#
+#    imported-twice.txt:
+#
+#       Imported twice.  HEAD should reflect the contents of the
+#       second import (i.e., have the same contents as 1.1.1.2).
+#
+#    imported-modified.txt:
+#
+#       Imported, then modified on HEAD.  HEAD should reflect the
+#       modification.
+#
+#    imported-modified-imported.txt:
+#
+#       Imported, then modified on HEAD, then imported again.
+#
+#    added-imported.txt,v:
+#
+#       Added with 'cvs add' to create 1.1, then imported with
+#       completely different contents to create 1.1.1.1, therefore the
+#       vendor branch was never the default branch.
+#
+#    imported-anonymously.txt:
+#
+#       Like imported-twice.txt, but with a vendor branch whose branch
+#       tag has been removed.
+
+test_description='git cvsimport handling of vendor branches'
+. ./t96xx/cvs-lib.sh
+
+CVSROOT="$TEST_DIRECTORY"/t9601/cvsroot
+export CVSROOT
+
+test_expect_success 'import a module with a vendor branch' '
+
+	git cvsimport -C module-git module
+
+'
+
+test_expect_success 'check HEAD out of cvs repository' 'test_cvs_co master'
+
+test_expect_success 'check master out of git repository' 'test_git_co_branch master'
+
+test_expect_success 'check a file that was imported once' '
+
+	test_cmp_branch_file master imported-once.txt
+
+'
+
+test_expect_failure 'check a file that was imported twice' '
+
+	test_cmp_branch_file master imported-twice.txt
+
+'
+
+test_expect_success 'check a file that was imported then modified on HEAD' '
+
+	test_cmp_branch_file master imported-modified.txt
+
+'
+
+test_expect_success 'check a file that was imported, modified, then imported again' '
+
+	test_cmp_branch_file master imported-modified-imported.txt
+
+'
+
+test_expect_success 'check a file that was added to HEAD then imported' '
+
+	test_cmp_branch_file master added-imported.txt
+
+'
+
+test_expect_success 'a vendor branch whose tag has been removed' '
+
+	test_cmp_branch_file master imported-anonymously.txt
+
+'
+
+test_done

diff --git a/t/t9601/cvsroot/CVSROOT/.gitignore b/t/t9601/cvsroot/CVSROOT/.gitignore
new file mode 100644
index 0000000..c375d5b
--- /dev/null
+++ b/t/t9601/cvsroot/CVSROOT/.gitignore

@@ -0,0 +1 @@
+history

diff --git a/t/t9601/cvsroot/module/added-imported.txt,v b/t/t9601/cvsroot/module/added-imported.txt,v
new file mode 100644
index 0000000..5f83072
--- /dev/null
+++ b/t/t9601/cvsroot/module/added-imported.txt,v

@@ -0,0 +1,44 @@
+head	1.1;
+access;
+symbols
+	vtag-4:1.1.1.1
+	vbranchA:1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.1
+date	2004.02.09.15.43.15;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.16;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.1
+log
+@Add a file to the working copy.
+@
+text
+@Adding this file, before importing it with different contents.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-4).
+@
+text
+@d1 1
+a1 1
+This is vtag-4 (on vbranchA) of added-then-imported.txt.
+@
+

diff --git a/t/t9601/cvsroot/module/imported-anonymously.txt,v b/t/t9601/cvsroot/module/imported-anonymously.txt,v
new file mode 100644
index 0000000..55e1b0c
--- /dev/null
+++ b/t/t9601/cvsroot/module/imported-anonymously.txt,v

@@ -0,0 +1,42 @@
+head	1.1;
+branch	1.1.1;
+access;
+symbols
+	vtag-1:1.1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.1
+log
+@Initial revision
+@
+text
+@This is vtag-1 (on vbranchA) of imported-anonymously.txt.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-1).
+@
+text
+@@
+
+

diff --git a/t/t9601/cvsroot/module/imported-modified-imported.txt,v b/t/t9601/cvsroot/module/imported-modified-imported.txt,v
new file mode 100644
index 0000000..e5830ae
--- /dev/null
+++ b/t/t9601/cvsroot/module/imported-modified-imported.txt,v

@@ -0,0 +1,76 @@
+head	1.2;
+access;
+symbols
+	vtag-2:1.1.1.2
+	vtag-1:1.1.1.1
+	vbranchA:1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2004.02.09.15.43.14;	author kfogel;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	1.1.1.2;
+
+1.1.1.2
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@First regular commit, to imported-modified-imported.txt, on HEAD.
+@
+text
+@This is a modification of imported-modified-imported.txt on HEAD.
+It should supersede the version from the vendor branch.
+@
+
+
+1.1
+log
+@Initial revision
+@
+text
+@d1 2
+a2 1
+This is vtag-1 (on vbranchA) of imported-modified-imported.txt.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-1).
+@
+text
+@@
+
+
+1.1.1.2
+log
+@Import (vbranchA, vtag-2).
+@
+text
+@d1 1
+a1 1
+This is vtag-2 (on vbranchA) of imported-modified-imported.txt.
+@
+
+

diff --git a/t/t9601/cvsroot/module/imported-modified.txt,v b/t/t9601/cvsroot/module/imported-modified.txt,v
new file mode 100644
index 0000000..bbcfe44
--- /dev/null
+++ b/t/t9601/cvsroot/module/imported-modified.txt,v

@@ -0,0 +1,59 @@
+head	1.2;
+access;
+symbols
+	vtag-1:1.1.1.1
+	vbranchA:1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2004.02.09.15.43.14;	author kfogel;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Commit on HEAD.
+@
+text
+@This is a modification of imported-modified.txt on HEAD.
+It should supersede the version from the vendor branch.
+@
+
+
+1.1
+log
+@Initial revision
+@
+text
+@d1 2
+a2 1
+This is vtag-1 (on vbranchA) of imported-modified.txt.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-1).
+@
+text
+@@
+
+

diff --git a/t/t9601/cvsroot/module/imported-once.txt,v b/t/t9601/cvsroot/module/imported-once.txt,v
new file mode 100644
index 0000000..c5dd82b
--- /dev/null
+++ b/t/t9601/cvsroot/module/imported-once.txt,v

@@ -0,0 +1,43 @@
+head	1.1;
+branch	1.1.1;
+access;
+symbols
+	vtag-1:1.1.1.1
+	vbranchA:1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.1
+log
+@Initial revision
+@
+text
+@This is vtag-1 (on vbranchA) of imported-once.txt.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-1).
+@
+text
+@@
+
+

diff --git a/t/t9601/cvsroot/module/imported-twice.txt,v b/t/t9601/cvsroot/module/imported-twice.txt,v
new file mode 100644
index 0000000..d1f3f1b
--- /dev/null
+++ b/t/t9601/cvsroot/module/imported-twice.txt,v

@@ -0,0 +1,60 @@
+head	1.1;
+branch	1.1.1;
+access;
+symbols
+	vtag-2:1.1.1.2
+	vtag-1:1.1.1.1
+	vbranchA:1.1.1;
+locks; strict;
+comment	@# @;
+
+
+1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches
+	1.1.1.1;
+next	;
+
+1.1.1.1
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	1.1.1.2;
+
+1.1.1.2
+date	2004.02.09.15.43.13;	author kfogel;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.1
+log
+@Initial revision
+@
+text
+@This is vtag-1 (on vbranchA) of imported-twice.txt.
+@
+
+
+1.1.1.1
+log
+@Import (vbranchA, vtag-1).
+@
+text
+@@
+
+
+1.1.1.2
+log
+@Import (vbranchA, vtag-2).
+@
+text
+@d1 1
+a1 1
+This is vtag-2 (on vbranchA) of imported-twice.txt.
+@
+
+

diff --git a/t/t96xx/cvs-lib.sh b/t/t96xx/cvs-lib.sh
index 0136b36..785d8d6 100644
--- a/t/t96xx/cvs-lib.sh
+++ b/t/t96xx/cvs-lib.sh

@@ -48,6 +48,12 @@ test_git_co_branch () {
 	(cd module-git && git checkout "$1")
 }
 
+test_cmp_branch_file () {
+	# Usage: test_cmp_branch_file BRANCH_NAME PATH
+	# The branch must already be checked out of CVS and git.
+	test_cmp module-cvs-"$1"/"$2" module-git/"$2"
+}
+
 test_cmp_branch_tree () {
 	# Usage: test_cmp_branch_tree BRANCH_NAME
 	# Check BRANCH_NAME out of CVS and git and make sure that all

-- 
1.6.1.3

A user's ~/.cvsrc file can change the basic behavior of CVS commands.
Therefore we should ignore it in order to ensure consistent results
from the test suite.

Signed-off-by: Michael Haggerty <redacted>
---
 t/t9600-cvsimport.sh |   16 ++++++++--------
 t/t96xx/cvs-lib.sh   |    3 +++
 2 files changed, 11 insertions(+), 8 deletions(-)

diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index b4b9896..66393ae 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh

@@ -6,12 +6,12 @@ test_description='git cvsimport basic tests'
 CVSROOT=$(pwd)/cvsroot
 export CVSROOT
 
-test_expect_success 'setup cvsroot' 'cvs init'
+test_expect_success 'setup cvsroot' '$CVS init'
 
 test_expect_success 'setup a cvs module' '
 
 	mkdir "$CVSROOT/module" &&
-	cvs co -d module-cvs module &&
+	$CVS co -d module-cvs module &&
 	cd module-cvs &&
 	cat <<EOF >o_fortuna &&
 O Fortuna

@@ -30,13 +30,13 @@ egestatem,
 potestatem
 dissolvit ut glaciem.
 EOF
-	cvs add o_fortuna &&
+	$CVS add o_fortuna &&
 	cat <<EOF >message &&
 add "O Fortuna" lyrics
 
 These public domain lyrics make an excellent sample text.
 EOF
-	cvs commit -F message &&
+	$CVS commit -F message &&
 	cd ..
 '
 

@@ -74,7 +74,7 @@ translate to English
 
 My Latin is terrible.
 EOF
-	cvs commit -F message &&
+	$CVS commit -F message &&
 	cd ..
 '
 

@@ -92,8 +92,8 @@ test_expect_success 'update cvs module' '
 
 	cd module-cvs &&
 		echo 1 >tick &&
-		cvs add tick &&
-		cvs commit -m 1
+		$CVS add tick &&
+		$CVS commit -m 1
 	cd ..
 
 '

@@ -111,7 +111,7 @@ test_expect_success 'cvsimport.module config works' '
 
 test_expect_success 'import from a CVS working tree' '
 
-	cvs co -d import-from-wt module &&
+	$CVS co -d import-from-wt module &&
 	cd import-from-wt &&
 		git cvsimport -a -z0 &&
 		echo 1 >expect &&

diff --git a/t/t96xx/cvs-lib.sh b/t/t96xx/cvs-lib.sh
index bfc1c12..6738901 100644
--- a/t/t96xx/cvs-lib.sh
+++ b/t/t96xx/cvs-lib.sh

@@ -14,6 +14,9 @@ then
 	exit
 fi
 
+CVS="cvs -f"
+export CVS
+
 cvsps_version=`cvsps -h 2>&1 | sed -ne 's/cvsps version //p'`
 case "$cvsps_version" in
 2.1 | 2.2*)

-- 
1.6.1.3

On Fri, Feb 20, 2009 at 06:18:09AM +0100, Michael Haggerty wrote:

The test suite for "git cvsimport" is pretty limited, and I would like
to improve the situation.  This patch series contains the first of
what I hope will eventually be several additions to the "git
cvsimport" test suite.

Great. I agree the test suite is terrible for cvsimport; what little is
there was added only after a regression where it was totally broken. ;)

I am the maintainer of cvs2svn/cvs2git.  Most of the new tests will
probably use fragments from the cvs2svn test suite.  I should admit
that part of my motivation for adding tests to the "git cvsimport"
test suite is to document its weaknesses, which do not seem to be
especially well known.

I don't think it is a problem to document cvsimport's weakness. It is
clear from list traffic that it has shortcomings, and IMHO documenting
them clearly and rigorously with test cases is the first step to fixing
them (or admitting that people should just use something else ;) ).

The only downside I see is that it bloats git's test suite a bit (and
cvs tests are often slow to run). We can always make them optional,
I suppose.

I do wonder, though, whether it would be simpler to make a "cvs import
test suite" that could pluggably test cvs2svn, git-cvsimport, or other
converters. Then you could test each on the exact same set of test
repos. And abstracting "OK, now make a repository from this cvsroot"
wouldn't be that hard for each command (I wouldn't think, but obviously
I haven't tried it :) ).

Patch 1 splits out some code into a library usable by multiple
CVS-related tests.

That is definitely a good first step, though the usual naming convention
is t/lib-cvs.sh. See t/lib-{git-svn,httpd,rebase}.sh, for example.

Patch 2 changes the library to add the -f option when invoking cvs (to
make it ignore the user's ~/.cvsrc file).

The code in t9600 (which gets moved to lib-cvs in your patch 1) sets
HOME explicitly. So is this really a problem?

Patch 3 adds a new test to t9600, namely to compare the entire module
as checked out by CVS vs. git.

Sounds reasonable.

Patch 4 adds a new test script t9601 that tests "git cvsimport"'s
handling of CVS vendor branches.  One of these tests fails due to an
actual bug.

Cool. Are you volunteering to fix git-cvsimport, too? :)

The second is that the new test script uses a small CVS repository
that is part of the test suite (i.e., the *,v files are committed
directly into the git source tree).  This is different than the
approach of t9600, which creates its own test CVS repository using CVS
commands.  The reasons for this are:

I think that's fine. There are other places in the test suite where
things that are a pain to produce are just included as content (e.g.,
see some of the SVN tests in the 9100 series).

And I think all of the reasons you gave are compelling.

Finally, the *,v files comprising the CVS repository have blank
trailing lines, triggering a warning from "git diff --check".  I don't
think that CVS strictly requires the blank lines, but they are always
generated by CVS, so I left them in.  But if the "git diff --check"
warnings are considered a serious problem, the blank lines could
probably be removed.

It's best to leave them in, I think, to create as realistic a test as
possible. But you should mark the paths as "we don't care about
whitespace" using gitattributes. I.e.,:

diff --git a/t/t9601/.gitattributes b/t/t9601/.gitattributes
new file mode 100644
index 0000000..562b12e
--- /dev/null
+++ b/t/t9601/.gitattributes

@@ -0,0 +1 @@
+* -whitespace

-Peff

Jeff King [off-list ref] writes:

Thanks for a review.  Everything you said makes sense to me.

Also I noticed that [3/4] uses "diff -r -x" --- does it pretty much mean
we require GNU diff to pass the test?  Can this be made more portable?

        Hi,

 I'm actually working on coming up with a patch for a bug I hit that has to to do with safecrlf=true. Maybe now I should coordinate with you?

 See this thread
 http://article.gmane.org/gmane.comp.version-control.git/110152

 Ferry

 Michael Haggerty wrote:    The test suite for "git cvsimport" is pretty limited, and I would like to improve the situation.  This patch series contains the first of what I hope will eventually be
several additions to the "git cvsimport" test suite.  I am the maintainer of cvs2svn/cvs2git.  Most of the new tests will probably use fragments from the cvs2svn test suite.  I should admit that
part of my motivation for adding tests to the "git cvsimport" test suite is to document its weaknesses, which do not seem to be especially well known.  Patch 1 splits out some code into a library
usable by multiple CVS-related tests.  Patch 2 changes the library to add the -f option when invoking cvs (to make it ignore the user's ~/.cvsrc file).  Patch 3 adds a new test to t9600, namely to
compare the entire module as checked out by CVS vs. git.  Patch 4 adds a new test script t9601 that tests "git cvsimport"'s handling of CVS vendor branches.  One of these tests fails due to an
actual bug.  These ideas in the patches are logically independent of each other, but each patch assumes that the previous patches have been applied.  I would like to point out a few things about
these patches that seem a little bit unprecedented in the git test suite.  If other approaches would be preferred, please let me know.  The first is that I would like to introduce a library that can
be used by the "git cvsimport" tests in the t96xx series, simply to avoid code duplication.  I put this library in t/t96xx/cvs-lib.sh, to hopefully make its role clear.  The library has to be
sourced from the main test directory.  (It sources test-lib.sh indirectly.)  The second is that the new test script uses a small CVS repository that is part of the test suite (i.e., the *,v files
are committed directly into the git source tree).  This is different than the approach of t9600, which creates its own test CVS repository using CVS commands.  The reasons for this are:  - t9600
wants to test incremental import, so it *has to* create the   repository dynamically.  That is not the case for t9601, which only   tests a one-shot import.  - The repository for t9601 is derived
from one that already exists as   part of the cvs2svn test suite.  Reverse-engineering it into CVS   commands would be extra work.  - The code to create CVS repositories via CVS commands is not very
  illuminating, and runs slowly, as CVS throttles commits to 1 per   second (to ensure unique timestamps).  - Future tests may require even more complicated CVS repositories that   are even more
cumbersome to create, so it's good to set a precedent   now :-)  Finally, the *,v files comprising the CVS repository have blank trailing lines, triggering a warning from "git diff --check".  I
don't think that CVS strictly requires the blank lines, but they are always generated by CVS, so I left them in.  But if the "git diff --check" warnings are considered a serious problem, the blank
lines could probably be removed.  Cheers, Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@vger.kernel.org More majordomo info at 
http://vger.kernel.org/majordomo-info.html

Jeff King wrote:

On Fri, Feb 20, 2009 at 06:18:09AM +0100, Michael Haggerty wrote:
[...]
I do wonder, though, whether it would be simpler to make a "cvs import
test suite" that could pluggably test cvs2svn, git-cvsimport, or other
converters. Then you could test each on the exact same set of test
repos. And abstracting "OK, now make a repository from this cvsroot"
wouldn't be that hard for each command (I wouldn't think, but obviously
I haven't tried it :) ).

Many tests only need to compare the contents of branches/tags checked
out of CVS with those checked out of the converted repository.  Such
tests could pretty easily be made agnostic about what conversion tool
they are testing and also, for that matter, the type of the target VCS
(git, SVN, hg, ...).  Testing incremental conversions at that level of
detail would not be much harder.

But other tests would be harder to write in a neutral fashion.  For
example, the cvs2svn test suite has tests of log messages, character-set
conversions of metadata, correct commit ordering, branching topology, etc.

In fact, one of the many things I haven't gotten around to yet is
writing a nontrivial test suite for cvs2git.  I'd like to share as much
as possible with the cvs2svn test suite, but there is a lot there that
is SVN-specific.

quoted

Patch 1 splits out some code into a library usable by multiple
CVS-related tests.

That is definitely a good first step, though the usual naming convention
is t/lib-cvs.sh. See t/lib-{git-svn,httpd,rebase}.sh, for example.

Thanks, I hadn't noticed that.  I'll change it the v2 of the patch.

quoted

Patch 2 changes the library to add the -f option when invoking cvs (to
make it ignore the user's ~/.cvsrc file).

The code in t9600 (which gets moved to lib-cvs in your patch 1) sets
HOME explicitly. So is this really a problem?

That's a good question.  I just checked, and empirically cvs uses .cvsrc
from my true home directory even if HOME is set differently.  So I think
that the -f option is indeed necessary.

quoted

Patch 4 adds a new test script t9601 that tests "git cvsimport"'s
handling of CVS vendor branches.  One of these tests fails due to an
actual bug.

Cool. Are you volunteering to fix git-cvsimport, too? :)

Not unless you call cvs2git the fixed version :-)

Cool, I didn't know about that feature.  I'll include that in v2 as well.

Thanks for the feedback!

BTW, I don't want to trash "git cvsimport".  I'm not brave enough even
to try to implement incremental conversions in cvs2git.  So the fact
that cvsimport sometimes works is already impressive :-)  I also
understand that its limitations come from those of cvsps, another
impressive but flawed tool (which in turn is being used outside of its
design limits).  But I hope to raise awareness that cvsps-based tools
are not the best choice for "one-shot" conversions, and maybe work
against people's tendency to use the "default" tool unless it obviously
blows up.

Michael

Junio C Hamano wrote:

Also I noticed that [3/4] uses "diff -r -x" --- does it pretty much mean
we require GNU diff to pass the test?  Can this be made more portable?

I can't think offhand of a more portable tool that could replace "diff
-r -x" here (suggestions, anyone?).  Of course one could re-implement
recursive diff in shell or perl, and I will take a stab at it if you
consider it important.  Alternatively, one could hardcode the paths that
the test scripts should test, but that would cost more per-test work
that I'd rather avoid.

Michael

With this patch, it is possible to exclude files based on basename
patterns.  Example:

	$ git diff --no-index -x Makefile -x Makefile.in a/ b/

In this example, the recursive diff between a/ and b/ will be shown
modulo changes in files named 'Makefile' or 'Makefile.in'.

Signed-off-by: Johannes Schindelin <redacted>
---

	Michael wrote:

	> I can't think offhand of a more portable tool that could replace 
	> "diff -r -x" here (suggestions, anyone?).

	Maybe something like this?

	Note: before it can be included in git.git, documentation and 
	tests have to be added; also, it might be a good idea to extend it 
	to the "non-no-index" case (maybe I can beat Peff in the number of 
	double negations one day...)

	So why only half a patch?  From time to time, I have to remember 
	that I work on Git for fun.  And this was the fun part, as far as 
	I am concerned.

 diff-no-index.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
 diff.c          |    9 +++++++++
 diff.h          |    6 ++++++
 3 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/diff-no-index.c b/diff-no-index.c
index 0a14268..0dc924a 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -16,7 +16,42 @@
 #include "builtin.h"
 #include "string-list.h"
 
-static int read_directory(const char *path, struct string_list *list)
+void add_basename_exclude(const char *exclude, struct diff_options *opts)
+{
+	if (!opts->basename_excludes) {
+		opts->basename_excludes =
+			xcalloc(sizeof(struct string_list), 1);
+		opts->basename_excludes->strdup_strings = 1;
+	}
+
+	string_list_append(exclude, opts->basename_excludes);
+}
+
+int basename_is_excluded(const char *basename, struct diff_options *options)
+{
+	int i;
+
+	if (!options->basename_excludes)
+		return 0;
+
+	for (i = 0; i < options->basename_excludes->nr; i++)
+		if (!fnmatch(options->basename_excludes->items[i].string,
+					basename, 0))
+			return 1;
+	return 0;
+}
+
+void free_basename_excludes(struct diff_options *options)
+{
+	if (!options->basename_excludes)
+		return;
+	string_list_clear(options->basename_excludes, 0);
+	free(options->basename_excludes);
+	options->basename_excludes = NULL;
+}
+
+static int read_directory(const char *path, struct string_list *list,
+	struct diff_options *options)
 {
 	DIR *dir;
 	struct dirent *e;

@@ -25,7 +60,8 @@ static int read_directory(const char *path, struct string_list *list)
 		return error("Could not open directory %s", path);
 
 	while ((e = readdir(dir)))
-		if (strcmp(".", e->d_name) && strcmp("..", e->d_name))
+		if (strcmp(".", e->d_name) && strcmp("..", e->d_name) &&
+				!basename_is_excluded(e->d_name, options))
 			string_list_insert(e->d_name, list);
 
 	closedir(dir);

@@ -63,9 +99,9 @@ static int queue_diff(struct diff_options *o,
 		struct string_list p1 = {NULL, 0, 0, 1}, p2 = {NULL, 0, 0, 1};
 		int len1 = 0, len2 = 0, i1, i2, ret = 0;
 
-		if (name1 && read_directory(name1, &p1))
+		if (name1 && read_directory(name1, &p1, o))
 			return -1;
-		if (name2 && read_directory(name2, &p2)) {
+		if (name2 && read_directory(name2, &p2, o)) {
 			string_list_clear(&p1, 0);
 			return -1;
 		}

@@ -177,10 +213,12 @@ void diff_no_index(struct rev_info *revs,
 			i++;
 			break;
 		}
-		if (!strcmp(argv[i], "--no-index"))
-			no_index = 1;
 		if (argv[i][0] != '-')
 			break;
+		if (!strcmp(argv[i], "--no-index"))
+			no_index = 1;
+		else if (!strcmp(argv[i], "-x"))
+			i++;
 	}
 
 	if (!no_index && !nongit) {

diff --git a/diff.c b/diff.c
index 55d73a1..29c0dd5 100644
--- a/diff.c
+++ b/diff.c

@@ -2653,6 +2653,14 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
 	else if (!prefixcmp(arg, "--output=")) {
 		options->file = fopen(arg + strlen("--output="), "w");
 		options->close_file = 1;
+	}
+	else if (!prefixcmp(arg, "--exclude="))
+		add_basename_exclude( arg + strlen("--exclude="), options);
+	else if (!strcmp(arg, "-x")) {
+		if (ac < 2)
+			die ("-x needs a parameter");
+		add_basename_exclude(av[1], options);
+		return 2;
 	} else
 		return 0;
 	return 1;

@@ -3306,6 +3314,7 @@ free_queue:
 	q->nr = q->alloc = 0;
 	if (options->close_file)
 		fclose(options->file);
+	free_basename_excludes(options);
 }
 
 static void diffcore_apply_filter(const char *filter)

diff --git a/diff.h b/diff.h
index 6703a4f..38f3acd 100644
--- a/diff.h
+++ b/diff.h

@@ -113,6 +113,7 @@ struct diff_options {
 	add_remove_fn_t add_remove;
 	diff_format_fn_t format_callback;
 	void *format_callback_data;
+	struct string_list *basename_excludes;
 };
 
 enum color_diff {

@@ -267,4 +268,9 @@ extern void diff_no_index(struct rev_info *, int, const char **, int, const char
 
 extern int index_differs_from(const char *def, int diff_flags);
 
+extern int basename_is_excluded(const char *path, struct diff_options *options);
+extern void add_basename_exclude(const char *exclude,
+		struct diff_options *options);
+extern void free_basename_excludes(struct diff_options *options);
+
 #endif /* DIFF_H */

-- 
1.6.2.rc1.350.g6caf6

On Fri, Feb 20, 2009 at 03:12:28PM +0100, Johannes Schindelin wrote:

	Michael wrote:

	> I can't think offhand of a more portable tool that could replace 
	> "diff -r -x" here (suggestions, anyone?).

	Maybe something like this?

Great. Using "git diff" was my first thought, too.

	Note: before it can be included in git.git, documentation and 
	tests have to be added; also, it might be a good idea to extend it 
	to the "non-no-index" case (maybe I can beat Peff in the number of 
	double negations one day...)

Maybe a config option "diff.denyNonIndexExclude = false"? *ducks*

But more seriously, how would a user expect this to interact with
.gitignore? I know gitignore is about ignoring untracked files, but I
can't help but feel the two have something in common. But maybe not. I'm
sick today and my brain is not working very well.

-Peff

On Fri, Feb 20, 2009 at 11:21:38AM +0100, Michael Haggerty wrote:

quoted

I do wonder, though, whether it would be simpler to make a "cvs import
test suite" that could pluggably test cvs2svn, git-cvsimport, or other
converters. Then you could test each on the exact same set of test
repos. And abstracting "OK, now make a repository from this cvsroot"
wouldn't be that hard for each command (I wouldn't think, but obviously
I haven't tried it :) ).

[...]
But other tests would be harder to write in a neutral fashion.  For
example, the cvs2svn test suite has tests of log messages, character-set
conversions of metadata, correct commit ordering, branching topology, etc.

OK. I haven't looked at it and you have, so I will accept your
judgement.

quoted

The code in t9600 (which gets moved to lib-cvs in your patch 1) sets
HOME explicitly. So is this really a problem?

That's a good question.  I just checked, and empirically cvs uses .cvsrc
from my true home directory even if HOME is set differently.  So I think
that the -f option is indeed necessary.

Yuck. But if that's the way it works, then I think your patch is the
only way.

quoted

Cool. Are you volunteering to fix git-cvsimport, too? :)

Not unless you call cvs2git the fixed version :-)

Heh.

design limits).  But I hope to raise awareness that cvsps-based tools
are not the best choice for "one-shot" conversions, and maybe work
against people's tendency to use the "default" tool unless it obviously
blows up.

Agreed. I have seen that advice given on the list several times, and it
seems to be working for people. So it really is about the right tool for
the job, IMHO.

-Peff

Hi,

On Fri, 20 Feb 2009, Jeff King wrote:

On Fri, Feb 20, 2009 at 03:12:28PM +0100, Johannes Schindelin wrote:

quoted

	Michael wrote:

	> I can't think offhand of a more portable tool that could replace 
	> "diff -r -x" here (suggestions, anyone?).

	Maybe something like this?

Great. Using "git diff" was my first thought, too.

Heh :-)

I have to admit that I had something pretty ugly involving find and grep 
in mind at first, then something equally appalling using GIT_WORKTREE and 
GIT_INDEX_FILE.

quoted

	Note: before it can be included in git.git, documentation and 
	tests have to be added; also, it might be a good idea to extend it 
	to the "non-no-index" case (maybe I can beat Peff in the number of 
	double negations one day...)

Maybe a config option "diff.denyNonIndexExclude = false"? *ducks*

How about diffNoIndex.denyNonIndexNonExclude = !true?  *swans*

But more seriously, how would a user expect this to interact with 
.gitignore? I know gitignore is about ignoring untracked files, but I 
can't help but feel the two have something in common. But maybe not. I'm 
sick today and my brain is not working very well.

I think that the -x option with regular (not --no-index) diff would be 
a little different.  .gitignore is for "git add" time, while "git diff" 
happily ignores .gitignore.

Besides, the -x option only works on the basenames (as I implemented it; 
no idea if GNU diff works the same way, but from Michael's patch it looks 
like it does).

BTW I just realized that I forgot to add a die() when !no_index && 
basename_excludes.

Ciao,
Dscho

Johannes Schindelin [off-list ref] writes:

On Fri, 20 Feb 2009, Jeff King wrote:

quoted

But more seriously, how would a user expect this to interact with 
.gitignore? I know gitignore is about ignoring untracked files, but I 
can't help but feel the two have something in common. But maybe not. I'm 
sick today and my brain is not working very well.

I think that the -x option with regular (not --no-index) diff would be 
a little different.  .gitignore is for "git add" time, while "git diff" 
happily ignores .gitignore.

Besides, the -x option only works on the basenames (as I implemented it; 
no idea if GNU diff works the same way, but from Michael's patch it looks 
like it does).

Info: (diff.info.gz)diff Options

`-x PATTERN'
`--exclude=PATTERN'
     When comparing directories, ignore files and subdirectories whose
     basenames match PATTERN.  *Note Comparing Directories::.

`-X FILE'
`--exclude-from=FILE'
     When comparing directories, ignore files and subdirectories whose
     basenames match any pattern contained in FILE.  *Note Comparing
     Directories::.

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Hi,

On Fri, 20 Feb 2009, Jakub Narebski wrote:

Johannes Schindelin [off-list ref] writes:

quoted

On Fri, 20 Feb 2009, Jeff King wrote:

quoted

But more seriously, how would a user expect this to interact with 
.gitignore? I know gitignore is about ignoring untracked files, but I 
can't help but feel the two have something in common. But maybe not. I'm 
sick today and my brain is not working very well.

I think that the -x option with regular (not --no-index) diff would be 
a little different.  .gitignore is for "git add" time, while "git diff" 
happily ignores .gitignore.

Besides, the -x option only works on the basenames (as I implemented it; 
no idea if GNU diff works the same way, but from Michael's patch it looks 
like it does).

Info: (diff.info.gz)diff Options

`-x PATTERN'
`--exclude=PATTERN'
     When comparing directories, ignore files and subdirectories whose
     basenames match PATTERN.  *Note Comparing Directories::.

Thanks for the clarification!

Ciao,
Dscho

Michael Haggerty wrote:

BTW, I don't want to trash "git cvsimport".  I'm not brave enough even
to try to implement incremental conversions in cvs2git.  So the fact

Michael,

If I run cvs2git several times against a live cvs repo (using the same configuration), wouldn't it perform an incremental import?
Is there anything that would make it produce different commits for the history?

I've just made a simple test here performing 2 imports (the 2nd with a dozen of new commits not in the 1st) and it seemed to work fine.

I know that it will take the same time/memory as the first import, but is there something that can break the repository or produce wrong data?

Thanks,

 - Samuel

Samuel Lucas Vaz de Mello wrote:

Michael Haggerty wrote:

quoted

BTW, I don't want to trash "git cvsimport".  I'm not brave enough even
to try to implement incremental conversions in cvs2git.  So the fact

If I run cvs2git several times against a live cvs repo (using the
same configuration), wouldn't it perform an incremental import?
Is there anything that would make it produce different commits for
the history?

I've just made a simple test here performing 2 imports (the 2nd with a
dozen of new commits not in the 1st) and it seemed to work fine.

I know that it will take the same time/memory as the first import,
but is there something that can break the repository or produce wrong
data?

Cool, I'd never thought of that.  It's certainly not by design, but as
you've discovered, the interaction of cvs2git and git *almost* combine
to give you an incremental import.

Alas, it is only "almost".  There are many things that can happen in a
CVS repository that would cause the overlapping part of the history to
disagree between runs of cvs2svn.  The nastiest are things that a VCS
shouldn't really even allow, but are common in CVS, like

- Retroactively adding a file to a branch or tag.  (This is a
much-beloved feature of CVS.)  Since CVS doesn't record the timestamp
when a symbol is added to a file, cvs2git tries (subject to the
constraints of other timestamps) to group all such changes into a single
changeset.  So the creation of the symbol would look different in runs N
vs N+1 of cvs2git--containing different files and likely with a
different timestamp.

- Renaming a file "with history" by renaming or copying the associated
*,v file in the repository.  This retroactively changes the entire
history of that file and thus of all changesets that involved changes to
that file.

- Changing the "text vs binary" or keyword expansion mode of a file.
These properties apply to all revisions of a file, and therefore also
have a retroactive effect.

But even aside from these retroactive changes, the output of cvs2git is
not deterministic in any practical sense (though I've tried to make it
deterministic given *identical* input).  The problem is that there are
so many ambiguities in a CVS history (because CVS doesn't record enough
information) that cvs2git has to use heuristics to decide what
individual file events should be grouped together as commits.  The
trickiest part is that the graph of naively inferred changesets can have
cycles in it, and cvs2git uses several heuristics to decide how to split
up changesets so as to remove the cycles.  (See our design notes [1] for
all the hairy details.)  The CVS commits made between runs N and N+1
could easily change some of the heuristics' decisions, giving different
results even for the overlapping part of the history.

To add robust support for incremental commits to cvs2git would require
run N+1 to know about the decisions made in run N, to avoid
contradicting them.

I wonder what would happen if one would treat the results of cvs2git
conversions N and N+1 as two separate repositories and merge them using
git.  In many cases the merge would probably be trivial, and most
conflicts (except retroactive file renaming!) would probably tend to be
in the recent past and therefore resolvable manually.  At least the
repository shouldn't silently become corrupted, which can happen with
other incremental conversion tools.

The final problem is that cvs2git conversions of large CVS repositories
are quite time-consuming, so using it for incremental conversions of
large repositories would be painful.  No doubt it could be speeded up
considerably, especially if conversion N+1 was privy to the results of
conversion N.

These are all challenging problems and I would welcome volunteers and be
happy to get them started.

Michael

[1] http://cvs2svn.tigris.org/svn/cvs2svn/trunk/doc/design-notes.txt

Ferry Huberts (Pelagic) wrote:

I'm actually working on coming up with a patch for a bug I hit that
has to to do with safecrlf=true. Maybe now I should coordinate with you?

I am only adding some tests of "git cvsimport"; I definitely don't plan
to become a "git cvsimport" hacker.  But we can certainly work together
on the test infrastructure if it will help you.

Michael

Michael Haggerty wrote:

Ferry Huberts (Pelagic) wrote:
  

quoted

I'm actually working on coming up with a patch for a bug I hit that
has to to do with safecrlf=true. Maybe now I should coordinate with you?
    

I am only adding some tests of "git cvsimport"; I definitely don't plan
to become a "git cvsimport" hacker.  But we can certainly work together
on the test infrastructure if it will help you.

Michael
  

I'd like to add a couple of tests to t9600 (as per Johannes' suggestion) 
to test my patch

Ferry

Michael Haggerty [off-list ref] writes:

Ferry Huberts (Pelagic) wrote:

quoted

I'm actually working on coming up with a patch for a bug I hit that
has to to do with safecrlf=true. Maybe now I should coordinate with you?

I am only adding some tests of "git cvsimport"; I definitely don't plan
to become a "git cvsimport" hacker.  But we can certainly work together
on the test infrastructure if it will help you.

Thanks, both.  I generally am not very fond of adding tests without
intention to look into fixes, but if they make outstanding bugs more
visible, they may have the effect of shaming the original authors badly
enough to step in in the effort of fixing them ;-)