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

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

[Michael Witten]

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(-)

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

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

[Junio C Hamano]

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.

What do other people who run git-cvsserver think?

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

[Michael Witten]

On 28 Nov 2007, at 7:24:31 PM, Junio C Hamano wrote:

> 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!

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

[Johannes Schindelin]

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,
Dscho

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

[Michael Witten]

On 30 Nov 2007, at 7:03:15 AM, Johannes Schindelin wrote:

> 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.

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

[Johannes Schindelin]

Hi,

On Fri, 30 Nov 2007, Michael Witten wrote:

> 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

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

[Johannes Sixt]

Michael Witten schrieb:
> 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

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

[Michael Witten]

On 30 Nov 2007, at 8:24:27 AM, Johannes Sixt wrote:

>> 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

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

[Junio C Hamano]

Michael Witten <mfwitten@MIT.EDU> writes:

>> 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.

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

[Michael Witten]

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.

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

[Junio C Hamano]

Michael Witten <mfwitten@MIT.EDU> writes:

> 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.