[PATCH 1/2] t9400-git-cvsserver-server: Wrap setup into test case

[PATCH 1/2] t9400-git-cvsserver-server: Wrap setup into test case

[Steffen Prohaska]

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 <prohaska@zib.de>
---
 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

[PATCH 2/2] cvsserver: Fix for histories with multiple roots

[Steffen Prohaska]

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 <prohaska@zib.de>
---
 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

Re: [PATCH 2/2] cvsserver: Fix for histories with multiple roots

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> 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 <prohaska@zib.de>
> ---
>  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;

[PATCH v2] cvsserver: Fix for histories with multiple roots

[Steffen Prohaska]

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 <prohaska@zib.de>
---
 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

Re: [PATCH v2] cvsserver: Fix for histories with multiple roots

[Steffen Prohaska]

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