threads / rfc / 6802

RFC patchHave git-cvsserver call hooks/update before really altering the ref

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

## tl;dr

9 messages between Feb 13, 2007 and Feb 19, 2007. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Andy Parkins· Feb 13, 2007, 15:12 UTC · lore

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(-)
Show changes to git-cvsserver.perl +15 −0
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
Junio C Hamano· Feb 13, 2007, 17:43 UTC · re: Andy Parkins · lore

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

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.

Nicolas Pitre· Feb 13, 2007, 17:54 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Feb 13, 2007, 18:07 UTC · re: Nicolas Pitre · lore

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

Nicolas Pitre <nico@cam.org> writes:
Show 6 quoted lines
> 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.

Andy Parkins· Feb 13, 2007, 18:14 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Feb 14, 2007, 05:36 UTC · re: Andy Parkins · lore

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

Andy Parkins <andyparkins@gmail.com> writes:
Show 16 quoted lines
> 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?

Andy Parkins· Feb 14, 2007, 09:13 UTC · re: Junio C Hamano · lore

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

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
Junio C Hamano· Feb 14, 2007, 09:33 UTC · re: Andy Parkins · lore

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

Andy Parkins <andyparkins@gmail.com> writes:
Show 8 quoted lines
> 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 ;-).

Martin Langhoff· Feb 19, 2007, 20:59 UTC · re: Junio C Hamano · lore

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

Junio C Hamano wrote:
Show 6 quoted lines
> 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
-----------------------------------------------------------------------

← back to recent threads