{"thread":{"id":"6802","subject":"[PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","startedAt":"2007-02-13T15:12:45Z","lastAt":"2007-02-19T20:59:28Z","messageCount":9,"participants":["Andy Parkins","Junio C Hamano","Nicolas Pitre","Martin Langhoff"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"34413","messageId":"200702131512.45412.andyparkins@gmail.com","threadId":"6802","inReplyTo":null,"subject":"[PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-13T15:12:45Z","receivedAt":"2007-02-13T15:12:45Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"git-cvsserver is analogous to git-receive-pack; a checking from a cvs\nclient to a central server is like a git-push from a working repository.\nTherefore it's nice to use the same access control (and email sending)\nthat a receive-pack would perform.\n\nThis patch tests for an executable update hook; if it is it is run with\nthe ref being updated and the old and new hashes as normal. If the\nupdate hook returns an error code the update is aborted and the ref is\nnever updated.\n\nSigned-off-by: Andy Parkins \n---\nI'm dubious whether this is the correct thing to do. It suits me for my\ncircumstances and I'm arguing that git-cvsserver is more like git-receive-pack\nthan git-commit; but internally it looks a lot more like git-commit.\n\nWithout calling the update hook though, git-cvsserver can completely bypass any\nextra access checks that git-receive-pack would have to perform. Assuming\ngit-cvsserver is a central repository - I think it's correct to call the update\nhook rather than the pre-commit hook.\n\nHowever, it's a judgement call, so I defer to someone with better judgement\nthan me :-)\n\n git-cvsserver.perl | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/git-cvsserver.perl b/git-cvsserver.perl\nindex 9371788..b4ef6bc 100755\n--- a/git-cvsserver.perl\n+++ b/git-cvsserver.perl\n@@ -1171,6 +1171,21 @@ sub req_ci\n exit;\n }\n \n+\t# Check that this is allowed, just as we would with a receive-pack\n+\tmy @cmd = ( $ENV{GIT_DIR}.'hooks/update', \"refs/heads/$state->{module}\",\n+\t\t\t$parenthash, $commithash );\n+\tif( -x $cmd[0] ) {\n+\t\tunless( system( @cmd ) == 0 )\n+\t\t{\n+\t\t\t$log->warn(\"Commit failed (update hook declined to update ref)\");\n+\t\t\tprint \"error 2 Commit failed (update hook declined)\\n\";\n+\t\t\tclose LOCKFILE;\n+\t\t\tunlink($lockfile);\n+\t\t\tchdir \"/\";\n+\t\t\texit;\n+\t\t}\n+\t}\n+\n print LOCKFILE $commithash;\n \n $updater->update();\n-- \n1.5.0.rc4.364.g85b1\n"},{"id":"34433","messageId":"7v7iumymvq.fsf@assigned-by-dhcp.cox.net","threadId":"6802","inReplyTo":"200702131512.45412.andyparkins@gmail.com","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-13T17:43:05Z","receivedAt":"2007-02-13T17:43:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"As a principle, I am in favor of this. Perhaps post 1.5.0 after\nhearing what real cvsserver users have to say on the list.\n"},{"id":"34437","messageId":"Pine.LNX.4.64.0702131253010.1757@xanadu.home","threadId":"6802","inReplyTo":"7v7iumymvq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-02-13T17:54:39Z","receivedAt":"2007-02-13T17:54:39Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 13 Feb 2007, Junio C Hamano wrote:\n\n> As a principle, I am in favor of this. Perhaps post 1.5.0 after\n> hearing what real cvsserver users have to say on the list.\n\nAndy looks to me like a real cvsserver user though.\n\n\nNicolas\n"},{"id":"34447","messageId":"7vtzxqvsle.fsf@assigned-by-dhcp.cox.net","threadId":"6802","inReplyTo":"Pine.LNX.4.64.0702131253010.1757@xanadu.home","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-13T18:07:57Z","receivedAt":"2007-02-13T18:07:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre writes:\n\n> On Tue, 13 Feb 2007, Junio C Hamano wrote:\n>\n>> As a principle, I am in favor of this. Perhaps post 1.5.0 after\n>> hearing what real cvsserver users have to say on the list.\n>\n> Andy looks to me like a real cvsserver user though.\n\nI know. If he were the only real cvsserver user, then the patch\nis fine as there is no risk breaking somebody else's setup.\n"},{"id":"34453","messageId":"200702131814.41512.andyparkins@gmail.com","threadId":"6802","inReplyTo":"7v7iumymvq.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-13T18:14:39Z","receivedAt":"2007-02-13T18:14:39Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Tuesday 2007, February 13, Junio C Hamano wrote:\n> As a principle, I am in favor of this. Perhaps post 1.5.0 after\n> hearing what real cvsserver users have to say on the list.\n\nAm I not a real user then? ;-) Cool - I /love/ being imaginary.\n\n-- \nDr Andrew Parkins, M Eng (Hons), AMIEE\nandyparkins@gmail.com\n"},{"id":"34500","messageId":"7v4pppuwpv.fsf@assigned-by-dhcp.cox.net","threadId":"6802","inReplyTo":"200702131512.45412.andyparkins@gmail.com","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-14T05:36:28Z","receivedAt":"2007-02-14T05:36:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> diff --git a/git-cvsserver.perl b/git-cvsserver.perl\n> index 9371788..b4ef6bc 100755\n> --- a/git-cvsserver.perl\n> +++ b/git-cvsserver.perl\n> @@ -1171,6 +1171,21 @@ sub req_ci\n> exit;\n> }\n> \n> +\t# Check that this is allowed, just as we would with a receive-pack\n> +\tmy @cmd = ( $ENV{GIT_DIR}.'hooks/update', \"refs/heads/$state->{module}\",\n> +\t\t\t$parenthash, $commithash );\n> +\tif( -x $cmd[0] ) {\n> +\t\tunless( system( @cmd ) == 0 )\n> +\t\t{\n> +\t\t\t$log->warn(\"Commit failed (update hook declined to update ref)\");\n> +\t\t\tprint \"error 2 Commit failed (update hook declined)\\n\";\n\nEverybody else seems to say 'print \"error 1 blah blah\"'. Are\nyou sure \"error 2 message\" is Kosher protocol-wise?\n"},{"id":"34507","messageId":"200702140913.55506.andyparkins@gmail.com","threadId":"6802","inReplyTo":"7v4pppuwpv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Andy Parkins","fromEmail":"andyparkins@gmail.com","sentAt":"2007-02-14T09:13:53Z","receivedAt":"2007-02-14T09:13:53Z","isPatch":true,"sender":{"key":"andyparkins@gmail.com","avatar":null},"body":"On Wednesday 2007 February 14 05:36, Junio C Hamano wrote:\n\n> Everybody else seems to say 'print \"error 1 blah blah\"'. Are\n> you sure \"error 2 message\" is Kosher protocol-wise?\n\nOops. No I'm not sure at all. I hadn't noticed that was a protocol command; \nI'd read it as being output to stderr which appears to be just commentary to \nthe CVS client. I assume you don't want a patch just to correct that?\n\n\nAndy\n\n-- \nDr Andy Parkins, M Eng (hons), MIEE\nandyparkins@gmail.com\n"},{"id":"34508","messageId":"7vfy99t761.fsf@assigned-by-dhcp.cox.net","threadId":"6802","inReplyTo":"200702140913.55506.andyparkins@gmail.com","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-02-14T09:33:42Z","receivedAt":"2007-02-14T09:33:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andy Parkins writes:\n\n> On Wednesday 2007 February 14 05:36, Junio C Hamano wrote:\n>\n>> Everybody else seems to say 'print \"error 1 blah blah\"'. Are\n>> you sure \"error 2 message\" is Kosher protocol-wise?\n>\n> Oops. No I'm not sure at all. I hadn't noticed that was a protocol command; \n> I'd read it as being output to stderr which appears to be just commentary to \n> the CVS client. I assume you don't want a patch just to correct that?\n\nI did not even read the protocol when I wondered what that 2 vs\n1 was, and I wanted to know what was the right thing to send to\nthe client. In other words, I was lazy ;-)\n\n`error ERRNO-CODE ` ' TEXT \\n'\n The command completed with an error. ERRNO-CODE is a symbolic\n error code (e.g. `ENOENT'); if the server doesn't support this\n feature, or if it's not appropriate for this particular message,\n it just omits the errno-code (in that case there are two spaces\n after `error'). Text is an error message such as that provided by\n strerror(), or any other message the server wants to use. The\n TEXT is like the `M' response, in the sense that it is not\n particularly intended to be machine-parsed; servers may wish to\n print an error message with `MT' responses, and then issue a\n `error' response without TEXT (although it should be noted that\n `MT' currently has no way of flagging the output as intended for\n standard error, the way that the `E' response does).\n\nI suspect Martin and Martyn chose 1 to match EPERM (I am looking\nat /usr/include/asm-generic/errno-base.h), and I think your new\nerror case falls into EPERM category as well. Since your patch\nis currently queued in 'pu', I can just go ahead and fix it\nmyself with \"git commit --amend\", but an Ack from down under\nwould certainly be appreciated ;-).\n"},{"id":"35032","messageId":"45DA0FB0.8030403@catalyst.net.nz","threadId":"6802","inReplyTo":"7vfy99t761.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH/RFC] Have git-cvsserver call hooks/update before really altering the ref","fromName":"Martin Langhoff","fromEmail":"martin@catalyst.net.nz","sentAt":"2007-02-19T20:59:28Z","receivedAt":"2007-02-19T20:59:28Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n> I suspect Martin and Martyn chose 1 to match EPERM (I am looking\n> at /usr/include/asm-generic/errno-base.h), and I think your new\n> error case falls into EPERM category as well. Since your patch\n> is currently queued in 'pu', I can just go ahead and fix it\n> myself with \"git commit --amend\", but an Ack from down under\n> would certainly be appreciated ;-).\n\nACK -- we were either smart in using EPERM or, more likely, super smart\nin forcing an error against a real CVS server and watching what it did.\n\nIt's better than saying \"I don't remember\" in any case.\n\n(Sorry about the delay, justback from holidays)\n\ncheers\n\n\nm\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington\nWEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St\nOFFICE: +64(4)916-7224 UK: 0845 868 5733 ext 7224 MOB: +64(21)364-017\n Make things as simple as possible, but no simpler - Einstein\n-----------------------------------------------------------------------\n"}]}