* [PATCH] cvsimport: add test illustrating a bug in cvsps @ 2009-02-23 18:49 Heiko Voigt 2009-02-23 20:35 ` Heiko Voigt 2009-02-24 5:00 ` Michael Haggerty 0 siblings, 2 replies; 21+ messages in thread From: Heiko Voigt @ 2009-02-23 18:49 UTC (permalink / raw) To: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] cvsimport: add test illustrating a bug in cvsps 2009-02-23 18:49 [PATCH] cvsimport: add test illustrating a bug in cvsps Heiko Voigt @ 2009-02-23 20:35 ` Heiko Voigt 2009-02-24 5:00 ` Michael Haggerty 1 sibling, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-02-23 20:35 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] cvsimport: add test illustrating a bug in cvsps 2009-02-23 18:49 [PATCH] cvsimport: add test illustrating a bug in cvsps Heiko Voigt 2009-02-23 20:35 ` Heiko Voigt @ 2009-02-24 5:00 ` Michael Haggerty 2009-03-02 17:59 ` [PATCH v2 0/1] " Heiko Voigt 2009-03-02 17:59 ` [PATCH v2 1/1] " Heiko Voigt 1 sibling, 2 replies; 21+ messages in thread From: Michael Haggerty @ 2009-02-24 5:00 UTC (permalink / raw) To: Heiko Voigt; +Cc: git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 0/1] cvsimport: add test illustrating a bug in cvsps 2009-02-24 5:00 ` Michael Haggerty @ 2009-03-02 17:59 ` Heiko Voigt 2009-03-02 17:59 ` [PATCH v2 1/1] " Heiko Voigt 1 sibling, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-02 17:59 UTC (permalink / raw) To: Michael Haggerty; +Cc: git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/1] cvsimport: add test illustrating a bug in cvsps 2009-02-24 5:00 ` Michael Haggerty 2009-03-02 17:59 ` [PATCH v2 0/1] " Heiko Voigt @ 2009-03-02 17:59 ` Heiko Voigt 2009-03-09 11:26 ` [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt 1 sibling, 1 reply; 21+ messages in thread From: Heiko Voigt @ 2009-03-02 17:59 UTC (permalink / raw) To: Michael Haggerty, Junio C Hamano; +Cc: git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised 2009-03-02 17:59 ` [PATCH v2 1/1] " Heiko Voigt @ 2009-03-09 11:26 ` Heiko Voigt 2009-03-09 15:02 ` Michael Haggerty 0 siblings, 1 reply; 21+ messages in thread From: Heiko Voigt @ 2009-03-09 11:26 UTC (permalink / raw) To: Michael Haggerty, Junio C Hamano, ydirson; +Cc: git 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); } ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised 2009-03-09 11:26 ` [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt @ 2009-03-09 15:02 ` Michael Haggerty 2009-03-18 17:33 ` [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps Heiko Voigt ` (3 more replies) 0 siblings, 4 replies; 21+ messages in thread From: Michael Haggerty @ 2009-03-09 15:02 UTC (permalink / raw) To: Heiko Voigt; +Cc: Junio C Hamano, ydirson, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-09 15:02 ` Michael Haggerty @ 2009-03-18 17:33 ` Heiko Voigt 2009-03-18 18:22 ` Junio C Hamano 2009-03-18 17:33 ` [PATCH v3 1/2] " Heiko Voigt ` (2 subsequent siblings) 3 siblings, 1 reply; 21+ messages in thread From: Heiko Voigt @ 2009-03-18 17:33 UTC (permalink / raw) To: Michael Haggerty; +Cc: Junio C Hamano, ydirson, git 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-18 17:33 ` [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps Heiko Voigt @ 2009-03-18 18:22 ` Junio C Hamano 2009-03-19 10:41 ` Michael J Gruber 2009-03-23 17:47 ` Heiko Voigt 0 siblings, 2 replies; 21+ messages in thread From: Junio C Hamano @ 2009-03-18 18:22 UTC (permalink / raw) To: Heiko Voigt; +Cc: Michael Haggerty, ydirson, git 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-18 18:22 ` Junio C Hamano @ 2009-03-19 10:41 ` Michael J Gruber 2009-03-19 11:00 ` Johannes Schindelin 2009-03-21 5:41 ` Michael Haggerty 2009-03-23 17:47 ` Heiko Voigt 1 sibling, 2 replies; 21+ messages in thread From: Michael J Gruber @ 2009-03-19 10:41 UTC (permalink / raw) To: Junio C Hamano; +Cc: Heiko Voigt, Michael Haggerty, ydirson, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-19 10:41 ` Michael J Gruber @ 2009-03-19 11:00 ` Johannes Schindelin 2009-03-19 11:22 ` Michael J Gruber 2009-03-21 5:41 ` Michael Haggerty 1 sibling, 1 reply; 21+ messages in thread From: Johannes Schindelin @ 2009-03-19 11:00 UTC (permalink / raw) To: Michael J Gruber Cc: Junio C Hamano, Heiko Voigt, Michael Haggerty, ydirson, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-19 11:00 ` Johannes Schindelin @ 2009-03-19 11:22 ` Michael J Gruber 0 siblings, 0 replies; 21+ messages in thread From: Michael J Gruber @ 2009-03-19 11:22 UTC (permalink / raw) To: Johannes Schindelin Cc: Junio C Hamano, Heiko Voigt, Michael Haggerty, ydirson, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-19 10:41 ` Michael J Gruber 2009-03-19 11:00 ` Johannes Schindelin @ 2009-03-21 5:41 ` Michael Haggerty 2009-03-23 18:11 ` started a cvsps testsuite Was: " Heiko Voigt 1 sibling, 1 reply; 21+ messages in thread From: Michael Haggerty @ 2009-03-21 5:41 UTC (permalink / raw) To: Michael J Gruber; +Cc: Junio C Hamano, Heiko Voigt, ydirson, git [-- 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 --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-21 5:41 ` Michael Haggerty @ 2009-03-23 18:11 ` Heiko Voigt 2009-03-23 19:06 ` Martin Langhoff 2009-03-24 4:50 ` Michael Haggerty 0 siblings, 2 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-23 18:11 UTC (permalink / raw) To: Michael Haggerty Cc: Michael J Gruber, Junio C Hamano, Heiko Voigt, ydirson, git, CVSps 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-23 18:11 ` started a cvsps testsuite Was: " Heiko Voigt @ 2009-03-23 19:06 ` Martin Langhoff 2009-03-24 4:50 ` Michael Haggerty 1 sibling, 0 replies; 21+ messages in thread From: Martin Langhoff @ 2009-03-23 19:06 UTC (permalink / raw) To: Heiko Voigt Cc: Michael Haggerty, Michael J Gruber, Junio C Hamano, Heiko Voigt, ydirson, git, CVSps 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-23 18:11 ` started a cvsps testsuite Was: " Heiko Voigt 2009-03-23 19:06 ` Martin Langhoff @ 2009-03-24 4:50 ` Michael Haggerty 2009-04-06 19:01 ` Heiko Voigt 1 sibling, 1 reply; 21+ messages in thread From: Michael Haggerty @ 2009-03-24 4:50 UTC (permalink / raw) To: Heiko Voigt Cc: Michael J Gruber, Junio C Hamano, Heiko Voigt, ydirson, git, CVSps 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: started a cvsps testsuite Was: Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-24 4:50 ` Michael Haggerty @ 2009-04-06 19:01 ` Heiko Voigt 0 siblings, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-04-06 19:01 UTC (permalink / raw) To: Michael Haggerty Cc: Michael J Gruber, Junio C Hamano, Heiko Voigt, ydirson, git, CVSps 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps 2009-03-18 18:22 ` Junio C Hamano 2009-03-19 10:41 ` Michael J Gruber @ 2009-03-23 17:47 ` Heiko Voigt 1 sibling, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-23 17:47 UTC (permalink / raw) To: Junio C Hamano; +Cc: Heiko Voigt, Michael Haggerty, ydirson, git 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3 1/2] cvsimport: add test illustrating a bug in cvsps 2009-03-09 15:02 ` Michael Haggerty 2009-03-18 17:33 ` [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps Heiko Voigt @ 2009-03-18 17:33 ` Heiko Voigt 2009-03-18 17:33 ` [PATCH v3 2/2] cvsimport: extend testcase about patchset order to contain branches Heiko Voigt 2009-03-18 17:34 ` [CVSPS PATCH v2] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt 3 siblings, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-18 17:33 UTC (permalink / raw) To: Michael Haggerty; +Cc: Junio C Hamano, ydirson, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v3 2/2] cvsimport: extend testcase about patchset order to contain branches 2009-03-09 15:02 ` Michael Haggerty 2009-03-18 17:33 ` [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps Heiko Voigt 2009-03-18 17:33 ` [PATCH v3 1/2] " Heiko Voigt @ 2009-03-18 17:33 ` Heiko Voigt 2009-03-18 17:34 ` [CVSPS PATCH v2] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt 3 siblings, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-18 17:33 UTC (permalink / raw) To: Michael Haggerty; +Cc: Junio C Hamano, ydirson, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [CVSPS PATCH v2] fix: correct rev order in case commiters clocks were not syncronised 2009-03-09 15:02 ` Michael Haggerty ` (2 preceding siblings ...) 2009-03-18 17:33 ` [PATCH v3 2/2] cvsimport: extend testcase about patchset order to contain branches Heiko Voigt @ 2009-03-18 17:34 ` Heiko Voigt 3 siblings, 0 replies; 21+ messages in thread From: Heiko Voigt @ 2009-03-18 17:34 UTC (permalink / raw) To: Michael Haggerty; +Cc: Junio C Hamano, ydirson, git 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2009-04-06 19:04 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-02-23 18:49 [PATCH] cvsimport: add test illustrating a bug in cvsps Heiko Voigt 2009-02-23 20:35 ` Heiko Voigt 2009-02-24 5:00 ` Michael Haggerty 2009-03-02 17:59 ` [PATCH v2 0/1] " Heiko Voigt 2009-03-02 17:59 ` [PATCH v2 1/1] " Heiko Voigt 2009-03-09 11:26 ` [CVSPS PATCH] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt 2009-03-09 15:02 ` Michael Haggerty 2009-03-18 17:33 ` [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps Heiko Voigt 2009-03-18 18:22 ` Junio C Hamano 2009-03-19 10:41 ` Michael J Gruber 2009-03-19 11:00 ` Johannes Schindelin 2009-03-19 11:22 ` Michael J Gruber 2009-03-21 5:41 ` Michael Haggerty 2009-03-23 18:11 ` started a cvsps testsuite Was: " Heiko Voigt 2009-03-23 19:06 ` Martin Langhoff 2009-03-24 4:50 ` Michael Haggerty 2009-04-06 19:01 ` Heiko Voigt 2009-03-23 17:47 ` Heiko Voigt 2009-03-18 17:33 ` [PATCH v3 1/2] " Heiko Voigt 2009-03-18 17:33 ` [PATCH v3 2/2] cvsimport: extend testcase about patchset order to contain branches Heiko Voigt 2009-03-18 17:34 ` [CVSPS PATCH v2] fix: correct rev order in case commiters clocks were not syncronised Heiko Voigt
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).