# [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref 9 messages from 2007-02-13 to 2007-02-19. Participants: Andy Parkins, Junio C Hamano, Nicolas Pitre, Martin Langhoff. Thread: https://gitlist.dev/t/6802 ## Andy Parkins, 2007-02-13 15:12 Subject: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <200702131512.45412.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702131512.45412.andyparkins%40gmail.com ``` 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 --- 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 ``` ## Junio C Hamano, 2007-02-13 17:43 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <7v7iumymvq.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7iumymvq.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702131512.45412.andyparkins@gmail.com> ``` 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, 2007-02-13 17:54 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: URL: https://gitlist.dev/e/Pine.LNX.4.64.0702131253010.1757%40xanadu.home In-Reply-To: <7v7iumymvq.fsf@assigned-by-dhcp.cox.net> ``` 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, 2007-02-13 18:07 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <7vtzxqvsle.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vtzxqvsle.fsf%40assigned-by-dhcp.cox.net In-Reply-To: ``` Nicolas Pitre 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. ``` ## Andy Parkins, 2007-02-13 18:14 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <200702131814.41512.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702131814.41512.andyparkins%40gmail.com In-Reply-To: <7v7iumymvq.fsf@assigned-by-dhcp.cox.net> ``` 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, 2007-02-14 05:36 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <7v4pppuwpv.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v4pppuwpv.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702131512.45412.andyparkins@gmail.com> ``` Andy Parkins 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? ``` ## Andy Parkins, 2007-02-14 09:13 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <200702140913.55506.andyparkins@gmail.com> URL: https://gitlist.dev/e/200702140913.55506.andyparkins%40gmail.com In-Reply-To: <7v4pppuwpv.fsf@assigned-by-dhcp.cox.net> ``` 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, 2007-02-14 09:33 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <7vfy99t761.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfy99t761.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <200702140913.55506.andyparkins@gmail.com> ``` Andy Parkins 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 ;-). ``` ## Martin Langhoff, 2007-02-19 20:59 Subject: Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref Message-ID: <45DA0FB0.8030403@catalyst.net.nz> URL: https://gitlist.dev/e/45DA0FB0.8030403%40catalyst.net.nz In-Reply-To: <7vfy99t761.fsf@assigned-by-dhcp.cox.net> ``` 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 ----------------------------------------------------------------------- ```