[PATCH] cvsimport: add test illustrating a bug in cvsps

[PATCH] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Some cvs repositories may have time deviations in their recorded commits. This
is a test for one of such cases. These kind of repositories can happen if the
system time of cvs clients is not fully synchronised.

Consider the following sequence of events:

 * client A commits file a r1.1
 * client A commits file a r1.2, b r1.1
 * client B commits file b r1.2 using the same timestamp as a r1.1

This can be resolved but due to cvsps grouping its patchsets solely based
on the timestamp and never breaking these groups it outputs the wrong
order which then leads to a broken import.

I hit this bug when importing from a real repository which was originally
converted from another rcs based scm. Other import tools can handle this
correctly, e.g. parsecvs.
---

As this is my first real patch, please let me know if I added anything
to the wrong place. There is actually another bug depending on timings
which I haven't written a testcase for yet. It is not such a showstopper
but cvsps skips revisions from a file if they have the same commit
message and fit into the same time window.

cheers Heiko

 t/t9603-cvsimport-time.sh      |   60 ++++++++++++++++++++++++++++++++++++++++
 t/t9603/cvsroot/time/a,v       |   41 +++++++++++++++++++++++++++
 t/t9603/cvsroot/time/b,v       |   41 +++++++++++++++++++++++++++
 3 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100755 t/t9603-cvsimport-time.sh
 create mode 100644 t/t9603/cvsroot/CVSROOT/.empty
 create mode 100644 t/t9603/cvsroot/time/a,v
 create mode 100644 t/t9603/cvsroot/time/b,v

diff --git a/t/t9603-cvsimport-time.sh b/t/t9603-cvsimport-time.sh
new file mode 100755
index 0000000..d6de242
--- /dev/null
+++ b/t/t9603-cvsimport-time.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+
+test_description='git cvsimport basic tests'
+. ./test-lib.sh
+
+CVSROOT="$TEST_DIRECTORY"/t9603/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
+
+# Structure of the test cvs repository
+#
+# Message   File:Content         Commit Time
+# Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
+# Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
+# Rev 3               b: 1.2     2009-02-21 19:11:43 +0100
+#
+# As you can see the commit of Rev 3 has the same time as
+# Rev 1 this leads to a broken import because of a cvsps
+# bug.
+
+test_expect_success 'import with criss cross times on revisions' '
+
+    git cvsimport -p"-x" -C import time && 
+    cd import &&
+        git log --pretty=format:%s > ../actual &&
+        echo "" >> ../actual &&
+    cd .. &&
+    echo "Rev 3
+Rev 2
+Rev 1" > expect &&
+    test_cmp actual expect 
+'
+
+test_done
diff --git a/t/t9603/cvsroot/CVSROOT/.empty b/t/t9603/cvsroot/CVSROOT/.empty
new file mode 100644
index 0000000..e69de29
diff --git a/t/t9603/cvsroot/time/a,v b/t/t9603/cvsroot/time/a,v
new file mode 100644
index 0000000..66a96aa
--- /dev/null
+++ b/t/t9603/cvsroot/time/a,v
@@ -0,0 +1,41 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.14;	author hvoigt;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.43;	author hvoigt;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 2
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 1
+@
+text
+@d1 1
+a1 1
+1.1
+@
+
diff --git a/t/t9603/cvsroot/time/b,v b/t/t9603/cvsroot/time/b,v
new file mode 100644
index 0000000..b5da856
--- /dev/null
+++ b/t/t9603/cvsroot/time/b,v
@@ -0,0 +1,41 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.43;	author hvoigt;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.14;	author hvoigt;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 3
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 2
+@
+text
+@d1 1
+a1 1
+1.1
+@
+
-- 
1.6.1.2.390.gba743

Re: [PATCH] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Heiko Voigt schrieb:
> +    echo "Rev 3
> +Rev 2
> +Rev 1" > expect &&
> +    test_cmp actual expect 

Just realized that I have overseen some whitespace issues in this patch.
I will resend a cleaned version after I got some feedback and also add
my Signed-off line.

bye Heiko 

Re: [PATCH] cvsimport: add test illustrating a bug in cvsps

[Michael Haggerty]

Heiko Voigt wrote:
> Some cvs repositories may have time deviations in their recorded commits. This
> is a test for one of such cases. [...]

Based on the numbering of your test, it looks like you've seen my
proposed patch [1].  If my patch is accepted, it would make sense for
yours to use the lib-cvs.sh library that I started.  But I guess that
can be changed if and when my patch is accepted.

To share the most test code, it would help to choose the same CVS module
name for each test when possible.  I imitated t9600 and named the CVS
modules in my tests "module", and for now the library assumes that name.
 Unless there is a special reason to use the module name "time", you
might think of renaming it, and similarly renaming your conversion
directory "module-git".

> +test_expect_success 'import with criss cross times on revisions' '

Since this test is known to fail, I think it should be
"test_expect_failure".

> diff --git a/t/t9603/cvsroot/CVSROOT/.empty b/t/t9603/cvsroot/CVSROOT/.empty
> new file mode 100644
> index 0000000..e69de29

I suggest adding a .gitignore file instead of .empty to keep the CVSROOT
directory around, and in it to list the filenames "history" and
"val-tags" (which are created by some cvs commands).  This will preserve
a clean "git status" even after running the test.

> diff --git a/t/t9603/cvsroot/time/a,v b/t/t9603/cvsroot/time/a,v
> new file mode 100644
> index 0000000..66a96aa
> --- /dev/null
> +++ b/t/t9603/cvsroot/time/a,v
> [...]

To avoid complaints from "git diff --check" about trailing whitespace in
the *,v files, you can add a file .gitattributes in your cvsroot
directory, containing the line "* -whitespace".  There is also trailing
whitespace in t9603-cvsimport-time.sh that can simply be deleted.

Michael

[1] http://www.spinics.net/lists/git/msg95366.html

[PATCH v2 0/1] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Hallo,

here is the second version of my patch with one more test for cvsimport.
This mail is just for documentation what has been fixed since the first
version.

Michael Haggerty schrieb:
> Heiko Voigt wrote:
>> Some cvs repositories may have time deviations in their recorded commits. This
>> is a test for one of such cases. [...]
> 
> Based on the numbering of your test, it looks like you've seen my
> proposed patch [1].  If my patch is accepted, it would make sense for
> yours to use the lib-cvs.sh library that I started.  But I guess that
> can be changed if and when my patch is accepted.

Indeed, I saw your patches which actually let me start to work on tests
of the problems I have found in cvsimport. I have already switched to
your library because I did not like to copy that code in the first
place. So this patch is now dependent on your series.

> To share the most test code, it would help to choose the same CVS module
> name for each test when possible.  I imitated t9600 and named the CVS
> modules in my tests "module", and for now the library assumes that name.
>  Unless there is a special reason to use the module name "time", you
> might think of renaming it, and similarly renaming your conversion
> directory "module-git".

Done.

> 
>> +test_expect_success 'import with criss cross times on revisions' '
> 
> Since this test is known to fail, I think it should be
> "test_expect_failure".

Adapted. I was not aware that this function marked a known bug. Sounded
to me like "if this test does succeed its an error".

> 
>> diff --git a/t/t9603/cvsroot/CVSROOT/.empty b/t/t9603/cvsroot/CVSROOT/.empty
>> new file mode 100644
>> index 0000000..e69de29
> 
> I suggest adding a .gitignore file instead of .empty to keep the CVSROOT
> directory around, and in it to list the filenames "history" and
> "val-tags" (which are created by some cvs commands).  This will preserve
> a clean "git status" even after running the test.

Done.

>> diff --git a/t/t9603/cvsroot/time/a,v b/t/t9603/cvsroot/time/a,v
>> new file mode 100644
>> index 0000000..66a96aa
>> --- /dev/null
>> +++ b/t/t9603/cvsroot/time/a,v
>> [...]
> 
> To avoid complaints from "git diff --check" about trailing whitespace in
> the *,v files, you can add a file .gitattributes in your cvsroot
> directory, containing the line "* -whitespace".  There is also trailing
> whitespace in t9603-cvsimport-time.sh that can simply be deleted.

That would have been another possibility. I actually deleted the
whitespace in the ,v files which seems to cause no problems. 

I share Michael's opinion that fixing these issues in cvsps are probably
not worth the effort. It did fix a few but ultimately gave up when
finding this one. Knowing that there are other tools available that do
the job more robust I would vote to switch to a new import tool if that
is possible. Also the users could be pointed to a more robust one in the
documentation like cvs2svn or parsecvs (which I found quite useful
because tag exactness was not that important)

cheers Heiko

[PATCH v2 1/1] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Some cvs repositories may have time deviations in their recorded commits. This
is a test for one of such cases. These kind of repositories can happen if the
system time of cvs clients is not fully synchronised.

Consider the following sequence of events:

 * client A commits file a r1.1
 * client A commits file a r1.2, b r1.1
 * client B commits file b r1.2 using the same timestamp as a r1.1

This can be resolved but due to cvsps grouping its patchsets solely based
on the timestamp and never breaking these groups it outputs the wrong
order which then leads to a broken import.

I hit this bug when importing from a real repository which was originally
converted from another rcs based scm. Other import tools can handle this
correctly, e.g. parsecvs.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 t/t9603-cvsimport-patchsets.sh     |   33 +++++++++++++++++++++++++++++
 t/t9603/cvsroot/CVSROOT/.gitignore |    2 +
 t/t9603/cvsroot/module/a,v         |   40 ++++++++++++++++++++++++++++++++++++
 t/t9603/cvsroot/module/b,v         |   40 ++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100755 t/t9603-cvsimport-patchsets.sh
 create mode 100644 t/t9603/cvsroot/CVSROOT/.gitignore
 create mode 100644 t/t9603/cvsroot/module/a,v
 create mode 100644 t/t9603/cvsroot/module/b,v

diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
new file mode 100755
index 0000000..15a971f
--- /dev/null
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+# Structure of the test cvs repository
+#
+# Message   File:Content         Commit Time
+# Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
+# Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
+# Rev 3               b: 1.2     2009-02-21 19:11:43 +0100
+#
+# As you can see the commit of Rev 3 has the same time as
+# Rev 1 this leads to a broken import because of a cvsps
+# bug.
+
+test_description='git cvsimport testing for correct patchset estimation'
+. ./lib-cvs.sh
+
+CVSROOT="$TEST_DIRECTORY"/t9603/cvsroot
+export CVSROOT
+
+test_expect_failure 'import with criss cross times on revisions' '
+
+    git cvsimport -p"-x" -C module-git module &&
+    cd module-git &&
+        git log --pretty=format:%s > ../actual &&
+        echo "" >> ../actual &&
+    cd .. &&
+    echo "Rev 3
+Rev 2
+Rev 1" > expect &&
+    test_cmp actual expect
+'
+
+test_done
diff --git a/t/t9603/cvsroot/CVSROOT/.gitignore b/t/t9603/cvsroot/CVSROOT/.gitignore
new file mode 100644
index 0000000..3bb9b34
--- /dev/null
+++ b/t/t9603/cvsroot/CVSROOT/.gitignore
@@ -0,0 +1,2 @@
+history
+val-tags
diff --git a/t/t9603/cvsroot/module/a,v b/t/t9603/cvsroot/module/a,v
new file mode 100644
index 0000000..e86adfc
--- /dev/null
+++ b/t/t9603/cvsroot/module/a,v
@@ -0,0 +1,40 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.14;	author tester;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.43;	author tester;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 2
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 1
+@
+text
+@d1 1
+a1 1
+1.1
+@
diff --git a/t/t9603/cvsroot/module/b,v b/t/t9603/cvsroot/module/b,v
new file mode 100644
index 0000000..ab3089f
--- /dev/null
+++ b/t/t9603/cvsroot/module/b,v
@@ -0,0 +1,40 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.43;	author tester;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.14;	author tester;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 3
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 2
+@
+text
+@d1 1
+a1 1
+1.1
+@
-- 
1.6.1.2.390.gba743

[CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised

[Heiko Voigt]

This fixes the following kind of cvsps issues:

 Structure of the test cvs repository

 Message   File:Content         Commit Time
 Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
 Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
 Rev 3               b: 1.2     2009-02-21 19:11:43 +0100

 As you can see the commit of Rev 3 has the same time as
 Rev 1 this was leading to a broken estimation of patchset
 order.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
After precisely formulating my problem with cvsps I was curious if this
issue actually could be fixed. Additionally I don't want my test to be
disabled ;). This patch seems to fix it. Is Yann still maintaining the
patches for cvsps or should I forward this patch to someone else ?

Calculating with time_t like I did is probably not valid on all systems
if you know of a nicer, more portable solution please let me know.

 cvsps.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/cvsps.c b/cvsps.c
index 81c6e21..d5c30d4 100644
--- a/cvsps.c
+++ b/cvsps.c
@@ -259,6 +259,32 @@ int main(int argc, char *argv[])
     exit(0);
 }
 
+void detect_and_repair_time_skew(const char *last_date, char *date, int n,
+                                 const char *filename)
+{
+
+    time_t smaller;
+    time_t bigger;
+    struct tm *ts;
+
+    /* if last_date does not exist do nothing */
+    if (last_date[0] == '\0')
+        return;
+
+    /* important: because rlog is showing revisions backwards last_date should
+     * always be bigger than date */
+    convert_date(&bigger, last_date);
+    convert_date(&smaller, date);
+
+    if (difftime(bigger, smaller) <= 0) {
+        debug(DEBUG_APPMSG1, "broken revision date: %s -> %s file: %s, repairing.\n",
+              date, last_date, filename);
+        smaller = bigger - 1;
+        ts = gmtime(&smaller);
+        strftime(date, n, "%Y-%m-%d %H:%M:%S", ts);
+    }
+}
+
 static void load_from_cvs()
 {
     FILE * cvsfp;
@@ -267,6 +293,7 @@ static void load_from_cvs()
     CvsFile * file = NULL;
     PatchSetMember * psm = NULL;
     char datebuff[20];
+    char last_datebuff[20];
     char authbuff[AUTH_STR_MAX];
     int logbufflen = LOG_STR_MAX + 1;
     char * logbuff = malloc(logbufflen);
@@ -334,6 +361,8 @@ static void load_from_cvs()
 	exit(1);
     }
 
+    /* initialize the last_datebuff with value indicating invalid date */
+    last_datebuff[0]='\0';
     for (;;)
     {
 	char * tst;
@@ -474,8 +503,14 @@ static void load_from_cvs()
 	    {
 		if (psm)
 		{
+		    detect_and_repair_time_skew(last_datebuff, datebuff, 20, psm->file->filename);
 		    PatchSet * ps = get_patch_set(datebuff, logbuff, authbuff, psm->post_rev->branch, psm);
 		    patch_set_add_member(ps, psm);
+
+		    /* remember last revision */
+		    strncpy(last_datebuff, datebuff, 20);
+		    /* just to be sure */
+		    last_datebuff[19] = '\0';
 		}
 
 		logbuff[0] = 0;
@@ -487,8 +522,13 @@ static void load_from_cvs()
 	    {
 		if (psm)
 		{
+		    detect_and_repair_time_skew(last_datebuff, datebuff, 20, psm->file->filename);
 		    PatchSet * ps = get_patch_set(datebuff, logbuff, authbuff, psm->post_rev->branch, psm);
 		    patch_set_add_member(ps, psm);
+
+		    /* just finished the last revision of this file, set last_datebuff to invalid */
+		    last_datebuff[0]='\0';
+
 		    assign_pre_revision(psm, NULL);
 		}
 

Re: [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised

[Michael Haggerty]

Heiko Voigt wrote:
> This fixes the following kind of cvsps issues:
> 
>  Structure of the test cvs repository
> 
>  Message   File:Content         Commit Time
>  Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
>  Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
>  Rev 3               b: 1.2     2009-02-21 19:11:43 +0100
> 
>  As you can see the commit of Rev 3 has the same time as
>  Rev 1 this was leading to a broken estimation of patchset
>  order.
> 
> Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>

I am not familiar with the cvsps code, so I will just make some comments
about things that it is not obvious from your patch that you have
considered.  These things all caused problems in pre-2.0 versions of
cvs2svn:

1. It is not clear from the patch in what order the revisions are being
processed.  If they are being processed in the order that they appear in
the RCS file, then you have to consider branches:

   * The date adjustment should only occur along chains of revisions
that are "causally related" -- that is, adjacent revisions on trunk, or
adjacent revisions on a branch, or the first revision on a branch
relative to the revision from which the branch sprouted.  It is not
always the case that revisions that are adjacent in the RCS file are
causally related.

   * The revisions along trunk appear in RCS files in reverse
chronological order; e.g., 1.3, 1.2, 1.1 (this seems to be the case you
handle).  But the revisions along a branch appear in chronological
order; e.g., 1.3.2.1, 1.3.2.2, 1.3.2.3.  Do you handle both cases
correctly?  (A unit test involving revisions on branches would be helpful.)

2. One form of clock skew that is common in CVS repositories is that
some computer's CMOS battery went dead and the clock reverted to 1970
after every reboot.  Given that you adjust revisions' times only towards
the past, then such a glitch would force the times of all earlier
revisions to be pushed back to 1970.  (Since you unconditionally
subtract one second from each commit timestamp, this could also
conceivably cause an underflow to 2038, but this is admittedly rather
unlikely.)  This is a hard problem to solve generally.  But if you want
to handle this problem more robustly, I suggest that you always adjust
times towards the future, as incorrect clock times in the far future
seem to be less common in practice.

Of course these clock skew corrections, if only applied to one file at a
time, can easily cause changesets to be broken up if the time deltas
exceed five minutes.

3. When cvsps collects individual file revisions into changesets (within
the 5 minute window), a single "consensus" commit time has to be chosen
from all of the single-file commits.  Depending on how cvsps does this,
it could be that the consensus commit times for two commits involving
revisions within a single file are put back out of order (undoing your
timestamp fixup).  It would be nice to verify that this does not result
in out-of-order commits.

Michael

[PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

This is an updated version of the first patch and an addition to ensure
correct handling of branches in fixes. 

The commit message of the first patch did not describe the problem
correctly. 

The second patch adds branches to the testcase so a possible bugfix in
cvsps can be made sure that it retains a correct ordering on branches. I
would like to keep the commits seperate for a better illustration of the
purpose of this test.

There also a new bugfix for cvsps following this patch series.

[PATCH v3 1/2] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Some cvs repositories may have time deviations in their recorded commits.
This is a test for one of such cases. These kind of repositories can happen
if the system time of cvs clients is not fully synchronised.

Consider the following sequence of events:

 * client A commits file a r1.1
 * client A commits file a r1.2, b r1.1
 * client B commits file b r1.2 using the same timestamp as a r1.1

This can be resolved but due to cvsps ordering its patchsets solely based
on the timestamp. It only takes revision odering into account if there
is no difference in the timestamp.

I hit this bug when importing from a real repository which was originally
converted from another rcs based scm. Other import tools can handle this
correctly, e.g. parsecvs.

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

 t/t9603-cvsimport-patchsets.sh     |   33 +++++++++++++++++++++++++++++
 t/t9603/cvsroot/CVSROOT/.gitignore |    2 +
 t/t9603/cvsroot/module/a,v         |   40 ++++++++++++++++++++++++++++++++++++
 t/t9603/cvsroot/module/b,v         |   40 ++++++++++++++++++++++++++++++++++++
 4 files changed, 115 insertions(+), 0 deletions(-)
 create mode 100755 t/t9603-cvsimport-patchsets.sh
 create mode 100644 t/t9603/cvsroot/CVSROOT/.gitignore
 create mode 100644 t/t9603/cvsroot/module/a,v
 create mode 100644 t/t9603/cvsroot/module/b,v

diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
new file mode 100755
index 0000000..15a971f
--- /dev/null
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -0,0 +1,33 @@
+#!/bin/sh
+
+# Structure of the test cvs repository
+#
+# Message   File:Content         Commit Time
+# Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
+# Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
+# Rev 3               b: 1.2     2009-02-21 19:11:43 +0100
+#
+# As you can see the commit of Rev 3 has the same time as
+# Rev 1 this leads to a broken import because of a cvsps
+# bug.
+
+test_description='git cvsimport testing for correct patchset estimation'
+. ./lib-cvs.sh
+
+CVSROOT="$TEST_DIRECTORY"/t9603/cvsroot
+export CVSROOT
+
+test_expect_failure 'import with criss cross times on revisions' '
+
+    git cvsimport -p"-x" -C module-git module &&
+    cd module-git &&
+        git log --pretty=format:%s > ../actual &&
+        echo "" >> ../actual &&
+    cd .. &&
+    echo "Rev 3
+Rev 2
+Rev 1" > expect &&
+    test_cmp actual expect
+'
+
+test_done
diff --git a/t/t9603/cvsroot/CVSROOT/.gitignore b/t/t9603/cvsroot/CVSROOT/.gitignore
new file mode 100644
index 0000000..3bb9b34
--- /dev/null
+++ b/t/t9603/cvsroot/CVSROOT/.gitignore
@@ -0,0 +1,2 @@
+history
+val-tags
diff --git a/t/t9603/cvsroot/module/a,v b/t/t9603/cvsroot/module/a,v
new file mode 100644
index 0000000..e86adfc
--- /dev/null
+++ b/t/t9603/cvsroot/module/a,v
@@ -0,0 +1,40 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.14;	author tester;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.43;	author tester;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 2
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 1
+@
+text
+@d1 1
+a1 1
+1.1
+@
diff --git a/t/t9603/cvsroot/module/b,v b/t/t9603/cvsroot/module/b,v
new file mode 100644
index 0000000..ab3089f
--- /dev/null
+++ b/t/t9603/cvsroot/module/b,v
@@ -0,0 +1,40 @@
+head	1.2;
+access;
+symbols;
+locks; strict;
+comment	@# @;
+
+
+1.2
+date	2009.02.21.18.11.43;	author tester;	state Exp;
+branches;
+next	1.1;
+
+1.1
+date	2009.02.21.18.11.14;	author tester;	state Exp;
+branches;
+next	;
+
+
+desc
+@@
+
+
+1.2
+log
+@Rev 3
+@
+text
+@1.2
+@
+
+
+1.1
+log
+@Rev 2
+@
+text
+@d1 1
+a1 1
+1.1
+@
-- 
1.6.1.2.390.gba743

[PATCH v3 2/2] cvsimport: extend testcase about patchset order to contain branches

[Heiko Voigt]

This makes sure that timestamps and ordering on branches is not influenced
by a fix for cvsps.

The test extension does not deal which patchset correction on branches it
only verifes that branches are basically handled as before.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
 t/t9603-cvsimport-patchsets.sh |   19 +++++++++----
 t/t9603/cvsroot/.gitattributes |    1 +
 t/t9603/cvsroot/module/a,v     |   38 ++++++++++++++++++++++++-
 t/t9603/cvsroot/module/b,v     |   58 +++++++++++++++++++++++++++++++++++++---
 4 files changed, 104 insertions(+), 12 deletions(-)
 create mode 100644 t/t9603/cvsroot/.gitattributes

diff --git a/t/t9603-cvsimport-patchsets.sh b/t/t9603-cvsimport-patchsets.sh
index 15a971f..2511b69 100755
--- a/t/t9603-cvsimport-patchsets.sh
+++ b/t/t9603-cvsimport-patchsets.sh
@@ -17,17 +17,24 @@ test_description='git cvsimport testing for correct patchset estimation'
 CVSROOT="$TEST_DIRECTORY"/t9603/cvsroot
 export CVSROOT
 
-test_expect_failure 'import with criss cross times on revisions' '
+test_expect_success 'import with criss cross times on revisions' '
 
     git cvsimport -p"-x" -C module-git module &&
     cd module-git &&
-        git log --pretty=format:%s > ../actual &&
-        echo "" >> ../actual &&
+        git log --pretty=format:%s > ../actual-master &&
+        git log A~2..A --pretty="format:%s %ad" -- > ../actual-A &&
+        echo "" >> ../actual-master &&
+        echo "" >> ../actual-A &&
     cd .. &&
-    echo "Rev 3
+    echo "Rev 4
+Rev 3
 Rev 2
-Rev 1" > expect &&
-    test_cmp actual expect
+Rev 1" > expect-master &&
+    test_cmp actual-master expect-master &&
+
+    echo "Rev 5 Branch A Wed Mar 11 19:09:10 2009 +0000
+Rev 4 Branch A Wed Mar 11 19:03:52 2009 +0000" > expect-A &&
+    test_cmp actual-A expect-A
 '
 
 test_done
diff --git a/t/t9603/cvsroot/.gitattributes b/t/t9603/cvsroot/.gitattributes
new file mode 100644
index 0000000..562b12e
--- /dev/null
+++ b/t/t9603/cvsroot/.gitattributes
@@ -0,0 +1 @@
+* -whitespace
diff --git a/t/t9603/cvsroot/module/a,v b/t/t9603/cvsroot/module/a,v
index e86adfc..ba8fd5a 100644
--- a/t/t9603/cvsroot/module/a,v
+++ b/t/t9603/cvsroot/module/a,v
@@ -1,13 +1,15 @@
 head	1.2;
 access;
-symbols;
+symbols
+	A:1.2.0.2;
 locks; strict;
 comment	@# @;
 
 
 1.2
 date	2009.02.21.18.11.14;	author tester;	state Exp;
-branches;
+branches
+	1.2.2.1;
 next	1.1;
 
 1.1
@@ -15,6 +17,16 @@ date	2009.02.21.18.11.43;	author tester;	state Exp;
 branches;
 next	;
 
+1.2.2.1
+date	2009.03.11.19.03.52;	author tester;	state Exp;
+branches;
+next	1.2.2.2;
+
+1.2.2.2
+date	2009.03.11.19.09.10;	author tester;	state Exp;
+branches;
+next	;
+
 
 desc
 @@
@@ -29,6 +41,28 @@ text
 @
 
 
+1.2.2.1
+log
+@Rev 4 Branch A
+@
+text
+@d1 1
+a1 1
+1.2.2.1
+@
+
+
+1.2.2.2
+log
+@Rev 5 Branch A
+@
+text
+@d1 1
+a1 1
+1.2.2.2
+@
+
+
 1.1
 log
 @Rev 1
diff --git a/t/t9603/cvsroot/module/b,v b/t/t9603/cvsroot/module/b,v
index ab3089f..d268855 100644
--- a/t/t9603/cvsroot/module/b,v
+++ b/t/t9603/cvsroot/module/b,v
@@ -1,13 +1,20 @@
-head	1.2;
+head	1.3;
 access;
-symbols;
+symbols
+	A:1.2.0.2;
 locks; strict;
 comment	@# @;
 
 
+1.3
+date	2009.03.11.19.05.08;	author tester;	state Exp;
+branches;
+next	1.2;
+
 1.2
 date	2009.02.21.18.11.43;	author tester;	state Exp;
-branches;
+branches
+	1.2.2.1;
 next	1.1;
 
 1.1
@@ -15,17 +22,60 @@ date	2009.02.21.18.11.14;	author tester;	state Exp;
 branches;
 next	;
 
+1.2.2.1
+date	2009.03.11.19.03.52;	author tester;	state Exp;
+branches;
+next	1.2.2.2;
+
+1.2.2.2
+date	2009.03.11.19.09.10;	author tester;	state Exp;
+branches;
+next	;
+
 
 desc
 @@
 
 
+1.3
+log
+@Rev 4
+@
+text
+@1.3
+@
+
+
 1.2
 log
 @Rev 3
 @
 text
-@1.2
+@d1 1
+a1 1
+1.2
+@
+
+
+1.2.2.1
+log
+@Rev 4 Branch A
+@
+text
+@d1 1
+a1 1
+1.2.2.1
+@
+
+
+1.2.2.2
+log
+@Rev 5 Branch A
+@
+text
+@d1 1
+a1 1
+1.2
 @
 
 
-- 
1.6.1.2.390.gba743

[CVSPS PATCH v2] fix: correct rev order in case commiters clocks were not syncronised

[Heiko Voigt]

This fixes the following kind of cvs issues:

 Structure of the test cvs repository

 Message   File:Content         Commit Time
 Rev 1     a: 1.1               2009-02-21 19:11:43 +0100
 Rev 2     a: 1.2    b: 1.1     2009-02-21 19:11:14 +0100
 Rev 3               b: 1.2     2009-02-21 19:11:43 +0100

 As you can see the commit of Rev 3 has the same time as
 Rev 1 this was leading to a broken estimation of patchset
 order.

The correction only applies to the main development line and specifically
ignores branches. The reason behind this is that a complete implementation
covering the correction of branches needs much more work.

I do not consider this patch a very "clean" bugfix. It is somewhat hacky
but may help someone who otherwise would not be able to use cvsps. It
additionally can help to find broken revisions and manually correct the
input files.

Signed-off-by: Heiko Voigt <hvoigt@hvoigt.net>
---
I am currently looking into a more clean solution which does not fidle
with the timestamps but only corrects the ordering of patchsets. To
implement this cvsps needs to be extended to use a topological sort
algorithm instead of a simple merge sort. Maybe Michael can give me some
directions how this was solved in cvs2svn.

My current idea is about using a modified selection sort. Sorting in
chronological order. It should always select the minimum date as the
next patchset as long as it does not conflict with the minimum last
chosen revision of all affected files. If it conflicts another pass will
select the patchset with the minimum revision for that file. This means
sorting time will be O(n^2), another drawback is the space required to
remember the last revision of every file and its branches.

Assuming that this can be implemented in my limited time I will attempt
to do so.

Here are a few explanations about this updated patch:

Michael Haggerty schrieb:
> 
> I am not familiar with the cvsps code, so I will just make some comments
> about things that it is not obvious from your patch that you have
> considered.  These things all caused problems in pre-2.0 versions of
> cvs2svn:
> 
> 1. It is not clear from the patch in what order the revisions are being
> processed.  If they are being processed in the order that they appear in
> the RCS file, then you have to consider branches:
> 
>    * The date adjustment should only occur along chains of revisions
> that are "causally related" -- that is, adjacent revisions on trunk, or
> adjacent revisions on a branch, or the first revision on a branch
> relative to the revision from which the branch sprouted.  It is not
> always the case that revisions that are adjacent in the RCS file are
> causally related.
> 
>    * The revisions along trunk appear in RCS files in reverse
> chronological order; e.g., 1.3, 1.2, 1.1 (this seems to be the case you
> handle).  But the revisions along a branch appear in chronological
> order; e.g., 1.3.2.1, 1.3.2.2, 1.3.2.3.  Do you handle both cases
> correctly?  (A unit test involving revisions on branches would be helpful.)

The input which is parsed by cvsps comes from the cvs log command. There
everything appears in reverse chronological order. Branches always come
at the end of the log so they are not timely ordered among the main
development line.

I adressed this issue by only applying the correction to the mainline.
This is not complete but solves the most practical special case.

A complete implementation needs much more work because in addition to
the last revision of the main line the last revision of every branch
would need to be remembered.

> 2. One form of clock skew that is common in CVS repositories is that
> some computer's CMOS battery went dead and the clock reverted to 1970
> after every reboot.  Given that you adjust revisions' times only towards
> the past, then such a glitch would force the times of all earlier
> revisions to be pushed back to 1970.  (Since you unconditionally
> subtract one second from each commit timestamp, this could also
> conceivably cause an underflow to 2038, but this is admittedly rather
> unlikely.)  This is a hard problem to solve generally.  But if you want
> to handle this problem more robustly, I suggest that you always adjust
> times towards the future, as incorrect clock times in the far future
> seem to be less common in practice.
> 
> Of course these clock skew corrections, if only applied to one file at a
> time, can easily cause changesets to be broken up if the time deltas
> exceed five minutes.

I addressed this by checking for time_t underflow before correction.
cvps will exit with an error message in case it happens. Due to the
reverse chronological order of the input I am only able to see one step
into the future but not into the past. Thats why I can only correct into
the past.

> 
> 3. When cvsps collects individual file revisions into changesets (within
> the 5 minute window), a single "consensus" commit time has to be chosen
> from all of the single-file commits.  Depending on how cvsps does this,
> it could be that the consensus commit times for two commits involving
> revisions within a single file are put back out of order (undoing your
> timestamp fixup).  It would be nice to verify that this does not result
> in out-of-order commits.

On file revisions cvsps corrects using this patch it will probably
happen that one change is broken into multiple patchsets. Because cvsps
issues warnings for every revision time it is changing the user can 
manually repair these rcs files.

cvsps keeps the minimum time of any member as the "consensus" time. So
the time correction backwards in time can result in out of order
commits. So it is not ensured that all commits compile. But it should be
ensured that all revisions of single files appear in the correct order.

As stated in the commit message this is not a "clean" solution but can
help to clean the imported repository and get correct revision ordering
on the main development line.

 cvsps.c |   53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/cvsps.c b/cvsps.c
index 81c6e21..1e72969 100644
--- a/cvsps.c
+++ b/cvsps.c
@@ -259,6 +259,45 @@ int main(int argc, char *argv[])
     exit(0);
 }
 
+void detect_and_repair_time_skew(const char *last_date, char *date, int n,
+                                 PatchSetMember *psm)
+{
+
+    time_t smaller;
+    time_t bigger;
+    struct tm *ts;
+    char *rev_end;
+
+    /* if last_date does not exist do nothing */
+    if (last_date[0] == '\0')
+        return;
+
+    /* TODO: repairing of branch times, skipping them for the moment */
+    /* check whether rev is of the form /1.[0-9]+/ */
+    if (psm->post_rev->rev[0] != '1' || psm->post_rev->rev[1] != '.')
+        return;
+    strtol(psm->post_rev->rev+2, &rev_end, 10);
+    if (*rev_end != '\0')
+        return;
+
+    /* important: because rlog is showing revisions backwards last_date should
+     * always be bigger than date */
+    convert_date(&bigger, last_date);
+    convert_date(&smaller, date);
+
+    if (difftime(bigger, smaller) <= 0) {
+        debug(DEBUG_APPMSG1, "broken revision date: %s -> %s file: %s, repairing.\n",
+              date, last_date, psm->file->filename);
+        if (!(bigger > 0)) {
+            debug(DEBUG_APPERROR, "timestamp underflow, exiting ... ");
+            exit(1);
+        }
+        smaller = bigger - 1;
+        ts = gmtime(&smaller);
+        strftime(date, n, "%Y-%m-%d %H:%M:%S", ts);
+    }
+}
+
 static void load_from_cvs()
 {
     FILE * cvsfp;
@@ -267,6 +306,7 @@ static void load_from_cvs()
     CvsFile * file = NULL;
     PatchSetMember * psm = NULL;
     char datebuff[20];
+    char last_datebuff[20];
     char authbuff[AUTH_STR_MAX];
     int logbufflen = LOG_STR_MAX + 1;
     char * logbuff = malloc(logbufflen);
@@ -334,6 +374,8 @@ static void load_from_cvs()
 	exit(1);
     }
 
+    /* initialize the last_datebuff with value indicating invalid date */
+    last_datebuff[0]='\0';
     for (;;)
     {
 	char * tst;
@@ -474,8 +516,14 @@ static void load_from_cvs()
 	    {
 		if (psm)
 		{
+		    detect_and_repair_time_skew(last_datebuff, datebuff, 20, psm);
 		    PatchSet * ps = get_patch_set(datebuff, logbuff, authbuff, psm->post_rev->branch, psm);
 		    patch_set_add_member(ps, psm);
+
+		    /* remember last revision */
+		    strncpy(last_datebuff, datebuff, 20);
+		    /* just to be sure */
+		    last_datebuff[19] = '\0';
 		}
 
 		logbuff[0] = 0;
@@ -487,8 +535,13 @@ static void load_from_cvs()
 	    {
 		if (psm)
 		{
+		    detect_and_repair_time_skew(last_datebuff, datebuff, 20, psm);
 		    PatchSet * ps = get_patch_set(datebuff, logbuff, authbuff, psm->post_rev->branch, psm);
 		    patch_set_add_member(ps, psm);
+
+		    /* just finished the last revision of this file, set last_datebuff to invalid */
+		    last_datebuff[0]='\0';
+
 		    assign_pre_revision(psm, NULL);
 		}
 
-- 
1.6.1.2.390.gba743

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Junio C Hamano]

Heiko Voigt <git-list@hvoigt.net> writes:

> This is an updated version of the first patch and an addition to ensure
> correct handling of branches in fixes. 

I've already merged the first one to 'next' so this needs to be turned
into an incremental update if we were to continue building on top in the
git project.

I however have a bigger issue with this, perhaps because I do not have the
feeling that I fully understand where these patches are going.

Your approach seems to me to:

 - add tests to git test suite that expose issues the current cvsimport
   that runs on an unpatched cvsps has;

 - diagnose and fix

   - the issues in cvsimport, if the problem is because cvsimport is
     mishandling correct output from cvsps; or

   - the issues in cvsps (and adjust cvsimport to patched cvsps if
     necessary), if the problem is because output from cvsps is incorrect.

That all feels sane, and having the tests to verify the end result would
help the people who collaborate on these tasks.

But how much of the actual fix will be made to cvsps, and how much to
cvsimport?  If the majority of the changes are to happen on cvsps (which
is not unexpected, given that many people who tried and wrote various cvs
importers put blame on the shortcomings of its output), I am afraid that
it would not help majority of git users until the fixes to cvsps that come
out of this effort hit their distros for me to keep these tests in the
git.git repository.  I do not build and install custom cvsps (because I
haven't had to work with complex history in CVS that your improvements to
cvsps are need to deal with correctly), and I suspect many others are in
the same boat.  In addition, if your tests are in the git.git repository,
they need to say test_expect_success for people with patched cvsps and
test_expect_failure for people without, and because I suspect that the
majority of git developers do not run bleeding edge cvsps, it does not do
anything but slowing down the test suite.

It feels as if you are scratching my feet through my shoes while I still
am wearing them.  I wonder if it would be more direct and simpler approach
to add tests to cvsps and handle these improvements as part of the cvsps
maintenance/development effort, not as part of cvsimport fixes, at least
initially.

I think it is great that you started actively working on identifying and
fixing issues with cvsps, that many others have gave up and gone to
different avenues, and I certainly do not mind keeping the new tests in
'pu' for wider exposure, in order to make it easier for other people who
use cvsimport and want to collaborate with you improving it through
improving cvsps.

But I am starting to think that it was a mistake on my part to have merged
the initial set of tests to 'next'.

Thoughts?

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 18.03.2009 19:22:
> Heiko Voigt <git-list@hvoigt.net> writes:
> 
>> This is an updated version of the first patch and an addition to ensure
>> correct handling of branches in fixes. 
> 
> I've already merged the first one to 'next' so this needs to be turned
> into an incremental update if we were to continue building on top in the
> git project.
> 
> I however have a bigger issue with this, perhaps because I do not have the
> feeling that I fully understand where these patches are going.
> 
> Your approach seems to me to:
> 
>  - add tests to git test suite that expose issues the current cvsimport
>    that runs on an unpatched cvsps has;
> 
>  - diagnose and fix
> 
>    - the issues in cvsimport, if the problem is because cvsimport is
>      mishandling correct output from cvsps; or
> 
>    - the issues in cvsps (and adjust cvsimport to patched cvsps if
>      necessary), if the problem is because output from cvsps is incorrect.
> 
> That all feels sane, and having the tests to verify the end result would
> help the people who collaborate on these tasks.
> 
> But how much of the actual fix will be made to cvsps, and how much to
> cvsimport?  If the majority of the changes are to happen on cvsps (which
> is not unexpected, given that many people who tried and wrote various cvs
> importers put blame on the shortcomings of its output), I am afraid that
> it would not help majority of git users until the fixes to cvsps that come
> out of this effort hit their distros for me to keep these tests in the
> git.git repository.  I do not build and install custom cvsps (because I
> haven't had to work with complex history in CVS that your improvements to
> cvsps are need to deal with correctly), and I suspect many others are in
> the same boat.  In addition, if your tests are in the git.git repository,
> they need to say test_expect_success for people with patched cvsps and
> test_expect_failure for people without, and because I suspect that the
> majority of git developers do not run bleeding edge cvsps, it does not do
> anything but slowing down the test suite.
> 
> It feels as if you are scratching my feet through my shoes while I still
> am wearing them.  I wonder if it would be more direct and simpler approach
> to add tests to cvsps and handle these improvements as part of the cvsps
> maintenance/development effort, not as part of cvsimport fixes, at least
> initially.
> 
> I think it is great that you started actively working on identifying and
> fixing issues with cvsps, that many others have gave up and gone to
> different avenues, and I certainly do not mind keeping the new tests in
> 'pu' for wider exposure, in order to make it easier for other people who
> use cvsimport and want to collaborate with you improving it through
> improving cvsps.
> 
> But I am starting to think that it was a mistake on my part to have merged
> the initial set of tests to 'next'.
> 
> Thoughts?

I second this, especially now that we've even got cvsps patches coming
to the git list. cvsps development should procede wherever all cvsps
developers can participate. It's not a central component of git.

In fact, the way git cvsimport uses cvsps may even mask some of cvsps's
behaviour. So, exposing cvsps bugs really only makes sense in a pure
cvsps test suite.

If git cvsimport is supposed to work around cvsps shortcomings then that
is supposed to be tested for in the git suite.

Right now this really seems to be going in the direction of: Make
everyone feel uneasy about working with git on cvs repos at all (aka
guerilla git), because git cvsimport still seems to be "recommended" or
"the standard", we have all this talk about how bad it is, and no clear
alternative.

I wish we had something saying: "If your cvs repo fulfills these
conditions you can use cvsimport. If not watch out for the following
(cvsps related) caveats. If you don't need two-way sync you're better of
with..."

Unfortunately, I'm not cvs(ps)-competent enough to spell out those
recommendations in detail.

Michael

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Johannes Schindelin]

Hi,

On Thu, 19 Mar 2009, Michael J Gruber wrote:

> cvsps development should procede wherever all cvsps developers can 
> participate. It's not a central component of git.

You are forgetting a central fact to this discussion: cvsps is not 
actively developed anymore.

Virtually all the "recent" patches going into cvsps (IIRC all patches 
between the last two releases) were coming directly from the Git 
community.

Ciao,
Dscho

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Michael J Gruber]

Johannes Schindelin venit, vidit, dixit 19.03.2009 12:00:
> Hi,
> 
> On Thu, 19 Mar 2009, Michael J Gruber wrote:
> 
>> cvsps development should procede wherever all cvsps developers can 
>> participate. It's not a central component of git.
> 
> You are forgetting a central fact to this discussion: cvsps is not 
> actively developed anymore.
> 
> Virtually all the "recent" patches going into cvsps (IIRC all patches 
> between the last two releases) were coming directly from the Git 
> community.

Well, then I guess "wherever all cvsps developers can participate" is de
facto here...

Still it's not in the git codebase. git cvsimport may expose cvsps short
comings, but testing cvsps in git's test suite does not seem right.

Michael

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Michael Haggerty]

[-- Attachment #1: Type: text/plain, Size: 2628 bytes --]

Michael J Gruber wrote:
> [...]
> Right now this really seems to be going in the direction of: Make
> everyone feel uneasy about working with git on cvs repos at all (aka
> guerilla git), because git cvsimport still seems to be "recommended" or
> "the standard", we have all this talk about how bad it is, and no clear
> alternative.
> 
> I wish we had something saying: "If your cvs repo fulfills these
> conditions you can use cvsimport. If not watch out for the following
> (cvsps related) caveats. If you don't need two-way sync you're better of
> with..."

Good idea.  Let me try to summarize the situations I have found that can
result in cvsps/cvsimport giving objectively incorrect output.  I am
only including the most blatant problems that can cause the content
checked out of git to differ from that checked out of CVS.  I have
attached some scripts that illustrate some of the problems that I
haven't yet submitted to the test suite.


Problems known to occur in the conversion of CVS HEAD:

- If any files were ever "cvs import"ed more than once (e.g., import of
more than one vendor release) but not modified locally, then the HEAD
version is incorrect (t9601-cvsimport-vendor-branch.sh).

- If the CVS timestamps on subsequent revisions are out of order (e.g.,
because of clock skew) then file commits can be out of order in git
(e.g. b.txt<1.2> overwritten by b.txt<1.1>).  This is demonstrated by
t9603-cvsimport-time.sh.

- If two files ever had what look like cross-cross commits within
cvsps's 5-minute time window (e.g.,

a.txt<1.1> Log message 1
a.txt<1.2> Log message 2

b.txt<1.1> Log message 2
b.txt<1.2> Log message 1

) then the commits of one file will be out of order in git (e.g.
b.txt<1.2> overwritten by b.txt<1.1>).  I have a test for this that I
haven't yet submitted.


Problems known to occur in the conversion of CVS branches:

Any of the above problems can occur, plus:

- A branch does not appear in the git repository if no commits were made
on the branch (t01.sh)

- All files from the branching point are added to a branch, including
those that were never added to the branch in CVS.  (CVS allows part of a
source tree to be added to a branch/tag.) (t06.sh)

- A file that was added to the source branch *after* a daughter branch
was created but before the first commit on the branch will incorrectly
be added to the daughter branch in git. (t04.sh)


Problems known to occur in the conversion of CVS tags:

Any of the above problems can affect the content of the tag, plus:

- If multiple tags mark the same revision, only one of them is included
in the conversion. (t03.sh)


Michael

[-- Attachment #2: t01.sh --]
[-- Type: application/x-sh, Size: 414 bytes --]

[-- Attachment #3: t06.sh --]
[-- Type: application/x-sh, Size: 678 bytes --]

[-- Attachment #4: t04.sh --]
[-- Type: application/x-sh, Size: 700 bytes --]

[-- Attachment #5: t03.sh --]
[-- Type: application/x-sh, Size: 448 bytes --]

Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

On Wed, Mar 18, 2009 at 11:22:43AM -0700, Junio C Hamano was talking about:
> Heiko Voigt <git-list@hvoigt.net> writes:
> 
> > This is an updated version of the first patch and an addition to ensure
> > correct handling of branches in fixes. 
> 
> I've already merged the first one to 'next' so this needs to be turned
> into an incremental update if we were to continue building on top in the
> git project.
> 
> I however have a bigger issue with this, perhaps because I do not have the
> feeling that I fully understand where these patches are going.

To explain my intentions a little more: I recently imported a huge
repository with ~11 years of history from a RCS based system. 

The final step into git land was through CVS. Here I really struggled to
find the right tool and it costed me quite some time just to find a
robust tool for the job (and to figure out why the "standard" tool
failed). 

So my main goal is to save some time and hassle for everyone else going
this route. There are really nicer things to spend time on than
importing and repairing RCS files.

I think many people when starting with git get the feeling: Oh look at
this SHA1 based database and stuff these people have written a really
neat robust system. Then you look at getting your history (from CVS)
into it and it becomes a nightmare. The command git cvsimport just does
not fit with the rest of the git tools quality.

> 
> Your approach seems to me to:
> 
>  - add tests to git test suite that expose issues the current cvsimport
>    that runs on an unpatched cvsps has;
> 
>  - diagnose and fix
> 
>    - the issues in cvsimport, if the problem is because cvsimport is
>      mishandling correct output from cvsps; or
> 
>    - the issues in cvsps (and adjust cvsimport to patched cvsps if
>      necessary), if the problem is because output from cvsps is incorrect.
> 
> That all feels sane, and having the tests to verify the end result would
> help the people who collaborate on these tasks.
> 
> But how much of the actual fix will be made to cvsps, and how much to
> cvsimport? 

I can not answer this question at the moment. One thing would be fixing
cvsps, which just for one test (mine) seems like a lot of work. I
haven't even looked into the other issues.

After writing my last email it came to my mind that it could be a
simpler approach to take parsecvs (only because its already written in C
otherwise probably cvs2svn/cvs2git) and change the interface so it
matches the one of git cvsimport and integrate it with git.

To let this happen the question is how important various features of the
current cvsimport are:

  * incremental import

  * keyword substitution

  * ...

> If the majority of the changes are to happen on cvsps (which
> is not unexpected, given that many people who tried and wrote various cvs
> importers put blame on the shortcomings of its output), I am afraid that
> it would not help majority of git users until the fixes to cvsps that come
> out of this effort hit their distros for me to keep these tests in the
> git.git repository.  I do not build and install custom cvsps (because I
> haven't had to work with complex history in CVS that your improvements to
> cvsps are need to deal with correctly), and I suspect many others are in
> the same boat.  In addition, if your tests are in the git.git repository,
> they need to say test_expect_success for people with patched cvsps and
> test_expect_failure for people without, and because I suspect that the
> majority of git developers do not run bleeding edge cvsps, it does not do
> anything but slowing down the test suite.
> 
> It feels as if you are scratching my feet through my shoes while I still
> am wearing them.  I wonder if it would be more direct and simpler approach
> to add tests to cvsps and handle these improvements as part of the cvsps
> maintenance/development effort, not as part of cvsimport fixes, at least
> initially.
> 
> I think it is great that you started actively working on identifying and
> fixing issues with cvsps, that many others have gave up and gone to
> different avenues, and I certainly do not mind keeping the new tests in
> 'pu' for wider exposure, in order to make it easier for other people who
> use cvsimport and want to collaborate with you improving it through
> improving cvsps.
> 
> But I am starting to think that it was a mistake on my part to have merged
> the initial set of tests to 'next'.
> 
> Thoughts?

In the long run I think at least a basic test for the current issues
should be in git.  Otherwise if cvsimport gets fixed you do not have a
way of making sure all tools (cvsps) in their right versions are
installed. Even with another non cvsps importer this property needs to
be ensured to handle non trivial repositories.
 
The only people who actually need to know about issues in cvsimport are
the ones who are trying to get away from CVS. So its probably best to
disable the "advanced" tests and have an environment variable e.g.:
"ALL_CVSIMPORT_TESTS" for enabling them.

cheers Heiko

started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

Hallo,

I just started a very simple but hopefully effective testsuite to
collect all the issues we found with cvsps. Because I haven't heard
anything from Yann who where the last one to collect some patches I
forked his master and started from there. 

    http://repo.or.cz/w/cvsps-hv.git

The testsuite is on the pu branch so I can first get some feedback
before I will merge it to master.

Maybe Michael can add his tests as well. You should just need to
construct the expected cvsps output. I choose a similar naming
to the one from git. The suite is very simple:

for each test tXXXX-name should be a correspondingly named CVS module in
t/cvsroot and a directory t/tXXXX-name. In this directory a file named
'expect' contains the expected output of cvsps. If the test is known to
fail also an empty file: 'known_to_fail' is added to the test directory.
Thats it.

Hopefully we can collect some more specifics about the situations where
cvsps fails and most importantly the amount of coding needed for fixing.
From there on it should be easier to decide which route to follow for
a robust inbuild git cvsimport.  

cheers Heiko

Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Martin Langhoff]

On Mon, Mar 23, 2009 at 7:11 PM, Heiko Voigt <hvoigt@hvoigt.net> wrote:
> I just started a very simple but hopefully effective testsuite to
> collect all the issues we found with cvsps.

Excellent effort. One thing I found when I was debugging (and trying
to understand) corner cases in cvs imports was the test suite for the
cvs2svn importer. It had a number of tiny cvs repositories that were
broken in various ways.

The test cvs repos, straight from the svn repo for cvs2svn are in
http://cvs2svn.tigris.org/svn/cvs2svn/trunk/test-data/ - access with
user:guest and an empty password.

...
> Hopefully we can collect some more specifics about the situations where
> cvsps fails and most importantly the amount of coding needed for fixing.
> From there on it should be easier to decide which route to follow for
> a robust inbuild git cvsimport.

Some cvs repos are ambiguous. Others are "impossible", fruits of
direct file manipulation. cvs2svn has a fairly robust (and
inordinately slow!) approach, but you cannot rerun it to get
incremental imports.

It is certainly possible... and bloody hard!

cheers,


m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Michael Haggerty]

Heiko Voigt wrote:
> I just started a very simple but hopefully effective testsuite to 
> collect all the issues we found with cvsps. [...]
> 
> Maybe Michael can add his tests as well. You should just need to 
> construct the expected cvsps output.

Please note that the cvsps output format itself is not rich enough to
represent all of the information needed, so it is not trivial to
construct the "expected" output.  The following is from an email that I
sent to the Mercurial mailing list [1]:

> I spent some time looking into cvsps, and I don't think that its
> output format, let alone the output itself, is suitable for
> industrial-strength conversion from CVS.  For example, as far as I
> can tell from my brief investigation:
> 
> * log messages are not unambiguously separable from the metadata
> 
> * it seems to only allow a single tag per changeset
> 
> * it seems to only apply tags to single changesets, whereas in CVS
>   files can be added to a tag at different times.
> ...
> 
> * it seems to completely ignore a branch tag if no commits were
>   ever made on that branch.
> 
> * it seems to ignore the effect that a second import to a vendor
>   branch has on the main branch.

You might also be interested to know that there has been some work in
the Mercurial project on a Python-based "cvsps".  I believe they use
their own version for their cvsimport extension.  I don't know how
different their cvsps is from the original.

Michael

[1] http://selenic.com/pipermail/mercurial-devel/2008-February/004975.html

Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

[Heiko Voigt]

On Tue, Mar 24, 2009 at 05:50:03AM +0100, Michael Haggerty wrote:
> Heiko Voigt wrote:
> > I just started a very simple but hopefully effective testsuite to 
> > collect all the issues we found with cvsps. [...]
> > 
> > Maybe Michael can add his tests as well. You should just need to 
> > construct the expected cvsps output.
> 
> Please note that the cvsps output format itself is not rich enough to
> represent all of the information needed, so it is not trivial to
> construct the "expected" output.  

But its possible ;) I have extracted some tests from the scripts you
send and pushed them into the repo. The output format definitely needs
extension. I already included that in the expected output. Maybe you
want to have a look. Tell me what you think.

> The following is from an email that I
> sent to the Mercurial mailing list [1]:
> 
> > I spent some time looking into cvsps, and I don't think that its
> > output format, let alone the output itself, is suitable for
> > industrial-strength conversion from CVS.  For example, as far as I
> > can tell from my brief investigation:
> > 
> > * log messages are not unambiguously separable from the metadata
> > 

Thats one issue which could be addressed by escaping the sequences which
lead to the ambiguity.

All issues described below could be addressed by the examples I included in
the tests.

> > * it seems to only allow a single tag per changeset
> > 
> > * it seems to only apply tags to single changesets, whereas in CVS
> >   files can be added to a tag at different times.
> > ...
> > 
> > * it seems to completely ignore a branch tag if no commits were
> >   ever made on that branch.
> > 
> > * it seems to ignore the effect that a second import to a vendor
> >   branch has on the main branch.
> 
> You might also be interested to know that there has been some work in
> the Mercurial project on a Python-based "cvsps".  I believe they use
> their own version for their cvsimport extension.  I don't know how
> different their cvsps is from the original.

Thats indeed interesting I have to try it once the testsuite is ready.
Maybe we can then just replace the original.

cheers Heiko