# [PATCH 1/2] t9400-git-cvsserver-server: Wrap setup into test case 5 messages from 2008-01-26 to 2008-01-27. Participants: Steffen Prohaska, Junio C Hamano. Thread: https://gitlist.dev/t/11749 ## Steffen Prohaska, 2008-01-26 09:54 Subject: [PATCH 1/2] t9400-git-cvsserver-server: Wrap setup into test case Message-ID: <12013412461111-git-send-email-prohaska@zib.de> URL: https://gitlist.dev/e/12013412461111-git-send-email-prohaska%40zib.de ``` The prefer to have the test setup in a test case. The setup itself may fail and having it as a test case handles this situation gracefully. Signed-off-by: Steffen Prohaska --- t/t9400-git-cvsserver-server.sh | 7 ++++--- 1 files changed, 4 insertions(+), 3 deletions(-) diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 641303e..1f2749e 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -33,13 +33,14 @@ CVS_SERVER=git-cvsserver export CVSROOT CVS_SERVER rm -rf "$CVSWORK" "$SERVERDIR" -echo >empty && +test_expect_success 'setup' ' + echo >empty && git add empty && git commit -q -m "First Commit" && git clone -q --local --bare "$WORKDIR/.git" "$SERVERDIR" >/dev/null 2>&1 && GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled true && - GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" || - exit 1 + GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" +' # note that cvs doesn't accept absolute pathnames # as argument to co -d -- 1.5.4.rc4.42.gacc73 ``` ## Steffen Prohaska, 2008-01-26 09:54 Subject: [PATCH 2/2] cvsserver: Fix for histories with multiple roots Message-ID: <12013412462758-git-send-email-prohaska@zib.de> URL: https://gitlist.dev/e/12013412462758-git-send-email-prohaska%40zib.de In-Reply-To: <12013412461111-git-send-email-prohaska@zib.de> ``` Git histories may have multiple roots, which can cause git merge-base to fail and this caused git cvsserver to die. This commit teaches git cvsserver to handle a failing git merge-base gracefully, and modifies the test case to verify this. All the test cases now use a history with two roots. Signed-off-by: Steffen Prohaska --- git-cvsserver.perl | 5 +++++ t/t9400-git-cvsserver-server.sh | 10 +++++++++- 2 files changed, 14 insertions(+), 1 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index ecded3b..534b41e 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -2543,6 +2543,11 @@ sub update if ($parent eq $lastpicked) { next; } + # or it may fail to find a merge base. In this + # case we just ignore this merge. + if (system("git merge-base $lastpicked $parent >/dev/null 2>/dev/null")) { + next; + } my $base = safe_pipe_capture('git-merge-base', $lastpicked, $parent); chomp $base; diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 1f2749e..75d1ce4 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -37,6 +37,13 @@ test_expect_success 'setup' ' echo >empty && git add empty && git commit -q -m "First Commit" && + mkdir secondroot && + ( cd secondroot && + git init && + touch secondrootfile && + git add secondrootfile && + git commit -m "second root") && + git pull secondroot master && git clone -q --local --bare "$WORKDIR/.git" "$SERVERDIR" >/dev/null 2>&1 && GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled true && GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" @@ -46,7 +53,8 @@ test_expect_success 'setup' ' # as argument to co -d test_expect_success 'basic checkout' \ 'GIT_CONFIG="$git_config" cvs -Q co -d cvswork master && - test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"' + test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5 | head -n 1))" = "empty/1.1/" + test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5 | tail -n 1))" = "secondrootfile/1.1/"' #------------------------ # PSERVER AUTHENTICATION -- 1.5.4.rc4.42.gacc73 ``` ## Junio C Hamano, 2008-01-27 01:56 Subject: Re: [PATCH 2/2] cvsserver: Fix for histories with multiple roots Message-ID: <7v8x2cyqsg.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8x2cyqsg.fsf%40gitster.siamese.dyndns.org In-Reply-To: <12013412462758-git-send-email-prohaska@zib.de> ``` Steffen Prohaska writes: > Git histories may have multiple roots, which can cause > git merge-base to fail and this caused git cvsserver to die. > > This commit teaches git cvsserver to handle a failing git > merge-base gracefully, and modifies the test case to verify this. > All the test cases now use a history with two roots. > > Signed-off-by: Steffen Prohaska > --- > git-cvsserver.perl | 5 +++++ > t/t9400-git-cvsserver-server.sh | 10 +++++++++- > 2 files changed, 14 insertions(+), 1 deletions(-) > > diff --git a/git-cvsserver.perl b/git-cvsserver.perl > index ecded3b..534b41e 100755 > --- a/git-cvsserver.perl > +++ b/git-cvsserver.perl > @@ -2543,6 +2543,11 @@ sub update > if ($parent eq $lastpicked) { > next; > } > + # or it may fail to find a merge base. In this > + # case we just ignore this merge. > + if (system("git merge-base $lastpicked $parent >/dev/null 2>/dev/null")) { > + next; > + } > my $base = safe_pipe_capture('git-merge-base', > $lastpicked, $parent); > chomp $base; That is a "Yes, but..." patch. Running merge-base always twice, due to fear of uncommon case of failure, feels quite backwards. Doesn't this work equally well without running a rather expensive merge-base twice? git-cvsserver.perl | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index ecded3b..afe3d0b 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -2543,8 +2543,15 @@ sub update if ($parent eq $lastpicked) { next; } - my $base = safe_pipe_capture('git-merge-base', + my $base = eval { + safe_pipe_capture('git-merge-base', $lastpicked, $parent); + }; + # The two branches may not be related at all, + # in which case merge base simply fails to find + # any, but that's Ok. + next if ($@); + chomp $base; if ($base) { my @merged; ``` ## Steffen Prohaska, 2008-01-27 09:37 Subject: [PATCH v2] cvsserver: Fix for histories with multiple roots Message-ID: <12014266292347-git-send-email-prohaska@zib.de> URL: https://gitlist.dev/e/12014266292347-git-send-email-prohaska%40zib.de In-Reply-To: <7v8x2cyqsg.fsf@gitster.siamese.dyndns.org> ``` Junio, here is a replacement for the second patch. Different from your diff, I replaced tabs with 8 spaces because all of the surrounding code uses spaces for indentation. Steffen ---- snip --- Git histories may have multiple roots, which can cause git merge-base to fail and this caused git cvsserver to die. This commit teaches git cvsserver to handle a failing git merge-base gracefully, and modifies the test case to verify this. All the test cases now use a history with two roots. Thanks to Junio C Hamano for the implementation that avoids calling git-merge-base twice. Signed-off-by: Steffen Prohaska --- git-cvsserver.perl | 11 +++++++++-- t/t9400-git-cvsserver-server.sh | 10 +++++++++- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/git-cvsserver.perl b/git-cvsserver.perl index ecded3b..920e7de 100755 --- a/git-cvsserver.perl +++ b/git-cvsserver.perl @@ -2543,8 +2543,15 @@ sub update if ($parent eq $lastpicked) { next; } - my $base = safe_pipe_capture('git-merge-base', - $lastpicked, $parent); + my $base = eval { + safe_pipe_capture('git-merge-base', + $lastpicked, $parent); + }; + # The two branches may not be related at all, + # in which case merge base simply fails to find + # any, but that's Ok. + next if ($@); + chomp $base; if ($base) { my @merged; diff --git a/t/t9400-git-cvsserver-server.sh b/t/t9400-git-cvsserver-server.sh index 1f2749e..75d1ce4 100755 --- a/t/t9400-git-cvsserver-server.sh +++ b/t/t9400-git-cvsserver-server.sh @@ -37,6 +37,13 @@ test_expect_success 'setup' ' echo >empty && git add empty && git commit -q -m "First Commit" && + mkdir secondroot && + ( cd secondroot && + git init && + touch secondrootfile && + git add secondrootfile && + git commit -m "second root") && + git pull secondroot master && git clone -q --local --bare "$WORKDIR/.git" "$SERVERDIR" >/dev/null 2>&1 && GIT_DIR="$SERVERDIR" git config --bool gitcvs.enabled true && GIT_DIR="$SERVERDIR" git config gitcvs.logfile "$SERVERDIR/gitcvs.log" @@ -46,7 +53,8 @@ test_expect_success 'setup' ' # as argument to co -d test_expect_success 'basic checkout' \ 'GIT_CONFIG="$git_config" cvs -Q co -d cvswork master && - test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5))" = "empty/1.1/"' + test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5 | head -n 1))" = "empty/1.1/" + test "$(echo $(grep -v ^D cvswork/CVS/Entries|cut -d/ -f2,3,5 | tail -n 1))" = "secondrootfile/1.1/"' #------------------------ # PSERVER AUTHENTICATION -- 1.5.4.rc4.42.gacc73 ``` ## Steffen Prohaska, 2008-01-27 09:47 Subject: Re: [PATCH v2] cvsserver: Fix for histories with multiple roots Message-ID: <359E2AFB-50AA-470E-9FE3-C78805037AAA@zib.de> URL: https://gitlist.dev/e/359E2AFB-50AA-470E-9FE3-C78805037AAA%40zib.de In-Reply-To: <12014266292347-git-send-email-prohaska@zib.de> ``` On Jan 27, 2008, at 10:37 AM, Steffen Prohaska wrote: > Junio, > here is a replacement for the second patch. Different from your diff, > I replaced tabs with 8 spaces because all of the surrounding code > uses spaces for indentation. Ah... too late... I should have pulled first ;) Anyway; thanks for improving the first version. Steffen ```