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

Subsystems: the rest

STALE3731d

11 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:20

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:16

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

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:46:16

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".
quoted hunk
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.
quoted hunk
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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:18

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:
quoted
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.
quoted
+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".
quoted
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.
quoted
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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:18

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:21

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

From: Michael Haggerty <hidden>
Date: 2016-06-15 22:46:21

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:25

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:25

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

 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 v2] fix: correct rev order in case commiters clocks were not syncronised

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:25

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

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

From: Heiko Voigt <hidden>
Date: 2016-06-15 22:46:25

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 <redacted>
---
 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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help