[PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Andy Parkins]

git-cvsserver is analogous to git-receive-pack; a checking from a cvs
client to a central server is like a git-push from a working repository.
Therefore it's nice to use the same access control (and email sending)
that a receive-pack would perform.

This patch tests for an executable update hook; if it is it is run with
the ref being updated and the old and new hashes as normal.  If the
update hook returns an error code the update is aborted and the ref is
never updated.

Signed-off-by: Andy Parkins <andyparkins@gmail.com>
---
I'm dubious whether this is the correct thing to do.  It suits me for my
circumstances and I'm arguing that git-cvsserver is more like git-receive-pack
than git-commit; but internally it looks a lot more like git-commit.

Without calling the update hook though, git-cvsserver can completely bypass any
extra access checks that git-receive-pack would have to perform.  Assuming
git-cvsserver is a central repository - I think it's correct to call the update
hook rather than the pre-commit hook.

However, it's a judgement call, so I defer to someone with better judgement
than me :-)

 git-cvsserver.perl |   15 +++++++++++++++
 1 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 9371788..b4ef6bc 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1171,6 +1171,21 @@ 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}",
+			$parenthash, $commithash );
+	if( -x $cmd[0] ) {
+		unless( system( @cmd ) == 0 )
+		{
+			$log->warn("Commit failed (update hook declined to update ref)");
+			print "error 2 Commit failed (update hook declined)\n";
+			close LOCKFILE;
+			unlink($lockfile);
+			chdir "/";
+			exit;
+		}
+	}
+
     print LOCKFILE $commithash;
 
     $updater->update();
-- 
1.5.0.rc4.364.g85b1

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Junio C Hamano]

As a principle, I am in favor of this.  Perhaps post 1.5.0 after
hearing what real cvsserver users have to say on the list.

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Nicolas Pitre]

On Tue, 13 Feb 2007, Junio C Hamano wrote:

> As a principle, I am in favor of this.  Perhaps post 1.5.0 after
> hearing what real cvsserver users have to say on the list.

Andy looks to me like a real cvsserver user though.


Nicolas

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Junio C Hamano]

Nicolas Pitre <nico@cam.org> writes:

> On Tue, 13 Feb 2007, Junio C Hamano wrote:
>
>> As a principle, I am in favor of this.  Perhaps post 1.5.0 after
>> hearing what real cvsserver users have to say on the list.
>
> Andy looks to me like a real cvsserver user though.

I know.  If he were the only real cvsserver user, then the patch
is fine as there is no risk breaking somebody else's setup.

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Andy Parkins]

On Tuesday 2007, February 13, Junio C Hamano wrote:
> As a principle, I am in favor of this.  Perhaps post 1.5.0 after
> hearing what real cvsserver users have to say on the list.

Am I not a real user then? ;-)  Cool - I /love/ being imaginary.

-- 
Dr Andrew Parkins, M Eng (Hons), AMIEE
andyparkins@gmail.com

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index 9371788..b4ef6bc 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -1171,6 +1171,21 @@ 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}",
> +			$parenthash, $commithash );
> +	if( -x $cmd[0] ) {
> +		unless( system( @cmd ) == 0 )
> +		{
> +			$log->warn("Commit failed (update hook declined to update ref)");
> +			print "error 2 Commit failed (update hook declined)\n";

Everybody else seems to say 'print "error 1 blah blah"'.  Are
you sure "error 2 message" is Kosher protocol-wise?

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Andy Parkins]

On Wednesday 2007 February 14 05:36, Junio C Hamano wrote:

> Everybody else seems to say 'print "error 1 blah blah"'.  Are
> you sure "error 2 message" is Kosher protocol-wise?

Oops.  No I'm not sure at all.  I hadn't noticed that was a protocol command; 
I'd read it as being output to stderr which appears to be just commentary to 
the CVS client.  I assume you don't want a patch just to correct that?


Andy

-- 
Dr Andy Parkins, M Eng (hons), MIEE
andyparkins@gmail.com

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Junio C Hamano]

Andy Parkins <andyparkins@gmail.com> writes:

> On Wednesday 2007 February 14 05:36, Junio C Hamano wrote:
>
>> Everybody else seems to say 'print "error 1 blah blah"'.  Are
>> you sure "error 2 message" is Kosher protocol-wise?
>
> Oops.  No I'm not sure at all.  I hadn't noticed that was a protocol command; 
> I'd read it as being output to stderr which appears to be just commentary to 
> the CVS client.  I assume you don't want a patch just to correct that?

I did not even read the protocol when I wondered what that 2 vs
1 was, and I wanted to know what was the right thing to send to
the client.  In other words, I was lazy ;-)

`error ERRNO-CODE ` ' TEXT \n'
     The command completed with an error.  ERRNO-CODE is a symbolic
     error code (e.g. `ENOENT'); if the server doesn't support this
     feature, or if it's not appropriate for this particular message,
     it just omits the errno-code (in that case there are two spaces
     after `error').  Text is an error message such as that provided by
     strerror(), or any other message the server wants to use.  The
     TEXT is like the `M' response, in the sense that it is not
     particularly intended to be machine-parsed; servers may wish to
     print an error message with `MT' responses, and then issue a
     `error' response without TEXT (although it should be noted that
     `MT' currently has no way of flagging the output as intended for
     standard error, the way that the `E' response does).

I suspect Martin and Martyn chose 1 to match EPERM (I am looking
at /usr/include/asm-generic/errno-base.h), and I think your new
error case falls into EPERM category as well.  Since your patch
is currently queued in 'pu', I can just go ahead and fix it
myself with "git commit --amend", but an Ack from down under
would certainly be appreciated ;-).

Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref

[Martin Langhoff]

Junio C Hamano wrote:
> I suspect Martin and Martyn chose 1 to match EPERM (I am looking
> at /usr/include/asm-generic/errno-base.h), and I think your new
> error case falls into EPERM category as well.  Since your patch
> is currently queued in 'pu', I can just go ahead and fix it
> myself with "git commit --amend", but an Ack from down under
> would certainly be appreciated ;-).

ACK -- we were either smart in using EPERM or, more likely, super smart
in forcing an error against a real CVS server and watching what it did.

It's better than saying "I don't remember" in any case.

(Sorry about the delay, justback from holidays)

cheers


m
-- 
-----------------------------------------------------------------------
Martin @ Catalyst .Net .NZ  Ltd, PO Box 11-053, Manners St,  Wellington
WEB: http://catalyst.net.nz/           PHYS: Level 2, 150-154 Willis St
OFFICE: +64(4)916-7224  UK: 0845 868 5733 ext 7224  MOB: +64(21)364-017
      Make things as simple as possible, but no simpler - Einstein
-----------------------------------------------------------------------