Re: [PATCH] Use git-update-ref to update a ref during commit in git-cvsserver

[Junio C Hamano <junkio@xxxxxxx>]

Andy Parkins <andyparkins@xxxxxxxxx> writes:

> As promised...
>
>  git-cvsserver.perl |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index b4ef6bc..54d943a 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -1216,9 +1216,9 @@ sub req_ci
>      }
>  
>      close LOCKFILE;
> -    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
> -    unlink($reffile);
> -    rename($lockfile, $reffile);
> +    my $reffile = "refs/heads/$state->{module}";
> +	`git-update-ref -m "git-cvsserver commit" $reffile $commithash $parenthash`;
> +	unlink($lockfile);
>      chdir "/";
>  
>      print "ok\n";

Using its own lockfile to update ref by hand while running
update-ref alongside it feels _very_ wrong.  How about this one
instead?

-- >8 --
[PATCH] Make 'cvs ci' lockless

This makes "ci" codepath lockless by following the usual
"remember the tip, do your thing, then compare and swap at the
end" update pattern using update-ref.  Incidentally, by updating
the code that reads where the tip of the head is to use
show-ref, it makes it safe to use in a repository whose refs are
pack-pruned.

I noticed that other parts of the program are not yet pack-refs
safe, but tried to keep the changes to the minimum.

Now I rarely use git-cvsserver myself, so I may be completely
breaking the check-in codepath.  Buyers beware...

---

 git-cvsserver.perl |   41 ++++++++++++++++-------------------------
 1 files changed, 16 insertions(+), 25 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 9371788..471621b 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1031,36 +1031,35 @@ sub req_ci
         exit;
     }
 
-    my $lockfile = "$state->{CVSROOT}/refs/heads/$state->{module}.lock";
-    unless ( sysopen(LOCKFILE,$lockfile,O_EXCL|O_CREAT|O_WRONLY) )
-    {
-        $log->warn("lockfile '$lockfile' already exists, please try again");
-        print "error 1 Lock file '$lockfile' already exists, please try again\n";
-        exit;
-    }
-
     # Grab a handle to the SQLite db and do any necessary updates
     my $updater = GITCVS::updater->new($state->{CVSROOT}, $state->{module}, $log);
     $updater->update();
 
     my $tmpdir = tempdir ( DIR => $TEMP_DIR );
     my ( undef, $file_index ) = tempfile ( DIR => $TEMP_DIR, OPEN => 0 );
-    $log->info("Lock successful, basing commit on '$tmpdir', index file is '$file_index'");
+    $log->info("Lockless commit start, basing commit on '$tmpdir', index file is '$file_index'");
 
     $ENV{GIT_DIR} = $state->{CVSROOT} . "/";
     $ENV{GIT_INDEX_FILE} = $file_index;
 
+    # Remember where the head was at the beginning.
+    my $parenthash = `git show-ref -s refs/heads/$state->{module}`;
+    chomp $parenthash;
+    if ($parenthash !~ /^[0-9a-f]{40}$/) {
+	    print "error 1 pserver cannot find the current HEAD of module";
+	    exit;
+    }
+
     chdir $tmpdir;
 
     # populate the temporary index based
-    system("git-read-tree", $state->{module});
+    system("git-read-tree", $parenthash);
     unless ($? == 0)
     {
 	die "Error running git-read-tree $state->{module} $file_index $!";
     }
     $log->info("Created index '$file_index' with for head $state->{module} - exit status $?");
 
-
     my @committedfiles = ();
 
     # foreach file specified on the command line ...
@@ -1095,8 +1094,6 @@ sub req_ci
         {
             # fail everything if an up to date check fails
             print "error 1 Up to date check failed for $filename\n";
-            close LOCKFILE;
-            unlink($lockfile);
             chdir "/";
             exit;
         }
@@ -1139,16 +1136,12 @@ sub req_ci
     {
         print "E No files to commit\n";
         print "ok\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         return;
     }
 
     my $treehash = `git-write-tree`;
-    my $parenthash = `cat $ENV{GIT_DIR}refs/heads/$state->{module}`;
     chomp $treehash;
-    chomp $parenthash;
 
     $log->debug("Treehash : $treehash, Parenthash : $parenthash");
 
@@ -1165,13 +1158,16 @@ sub req_ci
     {
         $log->warn("Commit failed (Invalid commit hash)");
         print "error 1 Commit failed (unknown reason)\n";
-        close LOCKFILE;
-        unlink($lockfile);
         chdir "/";
         exit;
     }
 
-    print LOCKFILE $commithash;
+    if (system(qw(git update-ref -m), "cvsserver ci",
+	       "refs/heads/$state->{module}", $commithash, $parenthash)) {
+	    $log->warn("update-ref for $state->{module} failed.");
+	    print "error 1 Cannot commit -- update first\n";
+	    exit;
+    }
 
     $updater->update();
 
@@ -1200,12 +1196,7 @@ sub req_ci
         }
     }
 
-    close LOCKFILE;
-    my $reffile = "$ENV{GIT_DIR}refs/heads/$state->{module}";
-    unlink($reffile);
-    rename($lockfile, $reffile);
     chdir "/";
-
     print "ok\n";
 }
 

-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html