threads / patch / 10989

patchgit-cvsserver runs hooks/post-receive

Subject: [PATCH] git-cvsserver runs hooks/post-receive

## tl;dr

11 messages between Nov 23, 2007 and Dec 1, 2007. Diffs are folded; open one to read it.

replies: 10people: 4as markdown or json

Michael Witten· Nov 23, 2007, 09:12 UTC · lore
git-cvsserver just did the following:
    (1) run hooks/update
    (2) commit if hooks/update passed
This commit simply adds:
    (3) run hooks/post-receive

Also, there are a few grammar cleanups and consistency improvements.

Signed-off-by: Michael Witten <mfwitten@mit.edu>
---
 git-cvsserver.perl |   27 ++++++++++++++++++++-------
 1 files changed, 20 insertions(+), 7 deletions(-)
Show changes to git-cvsserver.perl +20 −7
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 0d55fec..8d4d735 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1211,13 +1211,13 @@ sub req_ci
 
     chdir $tmpdir;
 
-    # populate the temporary index based
+    # populate the temporary index
     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 $?");
+    $log->info("Created index '$file_index' for head $state->{module} - exit status $?");
 
     my @committedfiles = ();
     my %oldmeta;
@@ -1237,7 +1237,7 @@ sub req_ci
 
         my ( $filepart, $dirpart ) = filenamesplit($filename);
 
-        # do a checkout of the file if it part of this tree
+        # do a checkout of the file if it is part of this tree
         if ($wrev) {
             system('git-checkout-index', '-f', '-u', $filename);
             unless ($? == 0) {
@@ -1324,11 +1324,11 @@ sub req_ci
         exit;
     }
 
-	# Check that this is allowed, just as we would with a receive-pack
-	my @cmd = ( $ENV{GIT_DIR}.'hooks/update', "refs/heads/$state->{module}",
+	### Emulate git-receive-pack by running hooks/update
+	my @hook = ( $ENV{GIT_DIR}.'hooks/update', "refs/heads/$state->{module}",
 			$parenthash, $commithash );
-	if( -x $cmd[0] ) {
-		unless( system( @cmd ) == 0 )
+	if( -x $hook[0] ) {
+		unless( system( @hook ) == 0 )
 		{
 			$log->warn("Commit failed (update hook declined to update ref)");
 			print "error 1 Commit failed (update hook declined)\n";
@@ -1337,6 +1337,7 @@ sub req_ci
 		}
 	}
 
+	### Update the ref
 	if (system(qw(git update-ref -m), "cvsserver ci",
 			"refs/heads/$state->{module}", $commithash, $parenthash)) {
 		$log->warn("update-ref for $state->{module} failed.");
@@ -1344,6 +1345,18 @@ sub req_ci
 		exit;
 	}
 
+	### Emulate git-receive-pack by running hooks/post-receive
+	my $hook = $ENV{GIT_DIR}.'hooks/post-receive';
+	if( -x $hook ) {
+		open(my $pipe, "| $hook") || die "can't fork $!";
+
+		local $SIG{PIPE} = sub { die 'pipe broke' };
+
+		print $pipe "$parenthash $commithash refs/heads/$state->{module}\n";
+
+		close $pipe || die "bad pipe: $! $?";
+	}
+
     $updater->update();
 
     # foreach file specified on the command line ...

-- 
1.5.3.5.1623.g37154
Junio C Hamano· Nov 29, 2007, 00:24 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Michael Witten <mfwitten@mit.edu> writes:
Show 9 quoted lines
> git-cvsserver just did the following:
>     (1) run hooks/update
>     (2) commit if hooks/update passed
>
> This commit simply adds:
>     (3) run hooks/post-receive
>
> Also, there are a few grammar cleanups and
> consistency improvements.

I gave only a very cursory look; looks Ok to me. This makes me wonder if post-update wants to run as well.

What do other people who run git-cvsserver think?
Michael Witten· Nov 30, 2007, 04:06 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

On 28 Nov 2007, at 7:24:31 PM, Junio C Hamano wrote:
Show 14 quoted lines
> Michael Witten <mfwitten@mit.edu> writes:
>
>> git-cvsserver just did the following:
>>     (1) run hooks/update
>>     (2) commit if hooks/update passed
>>
>> This commit simply adds:
>>     (3) run hooks/post-receive
>>
>> Also, there are a few grammar cleanups and
>> consistency improvements.
>
> I gave only a very cursory look; looks Ok to me.  This makes me wonder
> if post-update wants to run as well.

Seems like post-receive is supposed to supersede post-update anyhow, so might as well leave post-update out at this point?

In any case, I haven't taken a thorough look at how git-cvsserver works, but it seems to duplicate a lot of git-receive-pack.

How about turning git-cvsserver into a true middleman, so that it constructs a 'temporary git working tree' and then does a real git-push into the final git repository.

Because git-cvsserver would be using git-send-pack to do the pushing, it could push to yet another server. How cool is that!

Johannes Schindelin· Nov 30, 2007, 12:03 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Hi,
On Thu, 29 Nov 2007, Michael Witten wrote:
Show 6 quoted lines
> In any case, I haven't taken a thorough look at how git-cvsserver works, 
> but it seems to duplicate a lot of git-receive-pack.
> 
> How about turning git-cvsserver into a true middleman, so that it 
> constructs a 'temporary git working tree' and then does a real git-push 
> into the final git repository.
That would yield a horrible performance.

Would be opposed, if a regular cvsserver user, Dscho

Michael Witten· Nov 30, 2007, 12:26 UTC · re: Johannes Schindelin · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

On 30 Nov 2007, at 7:03:15 AM, Johannes Schindelin wrote:
Show 16 quoted lines
> Hi,
>
> On Thu, 29 Nov 2007, Michael Witten wrote:
>
>> In any case, I haven't taken a thorough look at how git-cvsserver  
>> works,
>> but it seems to duplicate a lot of git-receive-pack.
>>
>> How about turning git-cvsserver into a true middleman, so that it
>> constructs a 'temporary git working tree' and then does a real git- 
>> push
>> into the final git repository.
>
> That would yield a horrible performance.
>
> Would be opposed, if a regular cvsserver user,
How come?

git-cvsserver it seems already does just that! The difference is that it puts objects in place by hand, requiring the code to mirror hook calls anyway.

I'm simply proposing that the code be reworked, so that cvs commits actually become git pushes, so that all future changes to the pushing mechanism are automatically handled.

It just makes sense as far as design; git-cvsserver should be an adaptor between the two kinds of repositories, not an infiltrator.

Johannes Schindelin· Nov 30, 2007, 13:19 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Hi,
On Fri, 30 Nov 2007, Michael Witten wrote:
Show 16 quoted lines
> On 30 Nov 2007, at 7:03:15 AM, Johannes Schindelin wrote:
> 
> > On Thu, 29 Nov 2007, Michael Witten wrote:
> > 
> > > In any case, I haven't taken a thorough look at how git-cvsserver 
> > > works, but it seems to duplicate a lot of git-receive-pack.
> > > 
> > > How about turning git-cvsserver into a true middleman, so that it 
> > > constructs a 'temporary git working tree' and then does a real 
> > > git-push into the final git repository.
> > 
> > That would yield a horrible performance.
> > 
> > Would be opposed, if a regular cvsserver user,
> 
> How come?

I remember driving git-cvsserver on a bare repository, with "ln -s /usr/bin/git-cvsserver ~cvsuser/bin/", and it was superfast.

One user even asked if I upgraded my cvs, because it was so much faster than before. I said yes.

Ciao, Dscho

Johannes Sixt· Nov 30, 2007, 13:24 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Michael Witten schrieb:
Show 21 quoted lines
> On 30 Nov 2007, at 7:03:15 AM, Johannes Schindelin wrote:
>> On Thu, 29 Nov 2007, Michael Witten wrote:
>>> How about turning git-cvsserver into a true middleman, so that it
>>> constructs a 'temporary git working tree' and then does a real git-push
>>> into the final git repository.
>>
>> That would yield a horrible performance.
>>
>> Would be opposed, if a regular cvsserver user,
> 
> How come?
> 
> git-cvsserver it seems already does just that!
> The difference is that it puts objects in place
> by hand, requiring the code to mirror hook calls
> anyway.
> 
> I'm simply proposing that the code be reworked,
> so that cvs commits actually become git pushes,
> so that all future changes to the pushing mechanism
> are automatically handled.

But in order push something, you must first have the commit in a repository. How would git-cvsserver do that? For example, by putting objects in place by hand. You gain nothing, except that it would push instead of call the hooks directly.

-- Hannes
Michael Witten· Dec 1, 2007, 00:19 UTC · re: Johannes Sixt · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

On 30 Nov 2007, at 8:24:27 AM, Johannes Sixt wrote:
Show 12 quoted lines
>> The difference is that it puts objects in place
>> by hand, requiring the code to mirror hook calls
>> anyway.
>> I'm simply proposing that the code be reworked,
>> so that cvs commits actually become git pushes,
>> so that all future changes to the pushing mechanism
>> are automatically handled.
>
> But in order push something, you must first have the commit in a  
> repository. How would git-cvsserver do that? For example, by  
> putting objects in place by hand. You gain nothing, except that it  
> would push instead of call the hooks directly.

"The difference is that it puts objects in place by hand, requiring the code to mirror hook calls anyway."

As far as I can tell, hook calls are the only thing.

However, I still had to write a patch to include post-receive; by doing everything by hand, git-cvsserver is inherently unsafe and incomplete over time.

The question is not the RESULT, but HOW YOU GET that result; the current design is---essentially---a hack; it is written in perl, after all ;-P

Junio C Hamano· Dec 1, 2007, 02:37 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Michael Witten <mfwitten@MIT.EDU> writes:
Show 5 quoted lines
>> I gave only a very cursory look; looks Ok to me.  This makes me wonder
>> if post-update wants to run as well.
>
> Seems like post-receive is supposed to supersede post-update anyhow,
> so might as well leave post-update out at this point?

I'll queue your patch, but I think it should be enhanced to support post-update for consistency. I do not think anybody said anything about deprecating let alone dropping support for post-update so far.

When we talk about deprecation and later removal of post-update hook, we should drop that additional support from cvsserver and receive-pack at the same time, but not before that.

Michael Witten· Dec 1, 2007, 09:17 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

On 30 Nov 2007, at 9:37:01 PM, Junio C Hamano wrote:
> I'll queue your patch, but I think it should be enhanced to support
> post-update for consistency.
I'll send another patch that includes support for post-update.
> I'll queue your patch,
Will the old patch already be in place?

Also, I explicitly decided to pipe input into post-receive by hand rather than relying on a system() call that someone might exploit maliciously.

Unfortunately, it turns out that open() with a pipe essentially invokes system(); the solution is to fork a child process and then to turn the child into the process with which communication is desired via a call to exec().

Because the rest of git-cvsserver.perl uses explicit system() calls, I have been wondering if I am being overly cautious.

Junio C Hamano· Dec 1, 2007, 19:38 UTC · re: Michael Witten · lore

Re: [PATCH] git-cvsserver runs hooks/post-receive

Michael Witten <mfwitten@MIT.EDU> writes:
Show 10 quoted lines
> On 30 Nov 2007, at 9:37:01 PM, Junio C Hamano wrote:
>
>> I'll queue your patch, but I think it should be enhanced to support
>> post-update for consistency.
>
> I'll send another patch that includes support for post-update.
>
>> I'll queue your patch,
>
> Will the old patch already be in place?

Although I would encourage total replacement patch for things not in 'next' yet, in this case, I think we both would prefer a separate patch that builds on top of the old patch that teaches the server that it should call post-update as well.

I think it is potentially a sensible thing to do to eventually deprecate and then remove post-update hook (as your first patch that is queued already suggests). I was just saying that 'eventually' is not now. When that time comes, if you have a separate patch to teach post-update, that single patch can be reverted independently from the old patch, which should be less error prone than yanking the support out by hand.

← back to recent threads