{"thread":{"id":"28543","subject":"[PATCH] transport: do not allow to push over git:// protocol","startedAt":"2011-10-01T01:26:55Z","lastAt":"2012-01-08T21:07:39Z","messageCount":117,"participants":["Nguyễn Thái Ngọc Duy","Ilari Liusvaara","Nguyen Thai Ngoc Duy","Jonathan Nieder","Jeff King","Johannes Sixt","Jakub Narebski","René Scharfe","Junio C Hamano","Sitaram Chamarty","Clemens Buchacher","Brian Gernhardt","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"176615","messageId":"1317432415-9459-1-git-send-email-pclouds@gmail.com","threadId":"28543","inReplyTo":null,"subject":"[PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-01T01:26:55Z","receivedAt":"2011-10-01T01:26:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This protocol has never been designed for pushing. Attempts to push\nover git:// usually result in\n\n fatal: The remote end hung up unexpectedly\n\nThat message does not really point out the reason. With this patch, we get\n\n error: this protocol does not support pushing\n error: failed to push some refs to 'git://some-host.com/my/repo'\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n I wanted to advise using remote.*.pushurl too, more friendly. But then I\n had to detect if url comes from command line or config, and I gave up.\n\n transport.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/transport.c b/transport.c\nindex fa279d5..b109145 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -933,7 +933,8 @@ struct transport *transport_get(struct remote *remote, const char *url)\n \t\tret->set_option = NULL;\n \t\tret->get_refs_list = get_refs_via_connect;\n \t\tret->fetch = fetch_refs_via_pack;\n-\t\tret->push_refs = git_transport_push;\n+\t\tif (prefixcmp(url, \"git://\"))\n+\t\t\tret->push_refs = git_transport_push;\n \t\tret->connect = connect_git;\n \t\tret->disconnect = disconnect_git;\n \t\tret->smart_options = &(data->options);\n@@ -1075,6 +1076,8 @@ int transport_push(struct transport *transport,\n \n \t\treturn ret;\n \t}\n+\telse\n+\t\treturn error(\"this protocol does not support pushing\");\n \treturn 1;\n }\n \n-- \n1.7.3.1.256.g2539c.dirty\n"},{"id":"176620","messageId":"20111001022544.GA31036@LK-Perkele-VI.localdomain","threadId":"28543","inReplyTo":"1317432415-9459-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2011-10-01T02:25:44Z","receivedAt":"2011-10-01T02:25:44Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyễn Thái Ngọc Duy wrote:\n> This protocol has never been designed for pushing. Attempts to push\n> over git:// usually result in\n> \n> fatal: The remote end hung up unexpectedly\n> \n> That message does not really point out the reason. With this patch, we get\n> \n> error: this protocol does not support pushing\n> error: failed to push some refs to 'git://some-host.com/my/repo'\n\nWhat about sticking code to return an error to git daemon instead of this?\n\nHere's what happens if I try to push to one of repos on this computer\nover git://:\n\n$ git push git://localhost/foobar\nfatal: remote error: W access for foobar DENIED to anonymous\n\nSo send-pack can deal with ERR packet (and yes, that error message\nis really from Gitolite).\n\nAside: git archive seemingly can't deal with ERR packets. And worse\nyet, it doesn't even print what it received, resulting this:\n\n$ git archive --remote=git://localhost/foobar HEAD\nfatal: git archive: protocol error\n\n\n-Ilari \n"},{"id":"176621","messageId":"CACsJy8DVBVpDMgT7e1Mx70eUOznhifQHWR+zwg-=qPQMkzNRQA@mail.gmail.com","threadId":"28543","inReplyTo":"20111001022544.GA31036@LK-Perkele-VI.localdomain","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-01T04:27:01Z","receivedAt":"2011-10-01T04:27:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/10/1 Ilari Liusvaara :\n> What about sticking code to return an error to git daemon instead of this?\n>\n> Here's what happens if I try to push to one of repos on this computer\n> over git://:\n>\n> $ git push git://localhost/foobar\n> fatal: remote error: W access for foobar DENIED to anonymous\n>\n> So send-pack can deal with ERR packet (and yes, that error message\n> is really from Gitolite).\n\nI'm dealing with git.gnome.org and not sure what's the server behind.\nI had a look at git-daemon and it does allow push, but disabled by\ndefault. So yes, maybe updating git-daemon is better.\n\n> Aside: git archive seemingly can't deal with ERR packets. And worse\n> yet, it doesn't even print what it received, resulting this:\n>\n> $ git archive --remote=git://localhost/foobar HEAD\n> fatal: git archive: protocol error\n\nYes, builtin/archive.c seems only recognize either ACK or NACK.\npack-protocol.txt does not mention about ERR either, which seems to be\nintroduced in a807328 (connect.c: add a way for git-daemon to pass an\nerror back to client).\n-- \nDuy\n"},{"id":"176622","messageId":"20111001052910.GA6502@elie","threadId":"28543","inReplyTo":"20111001022544.GA31036@LK-Perkele-VI.localdomain","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-01T05:29:10Z","receivedAt":"2011-10-01T05:29:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ilari Liusvaara wrote:\n\n> What about sticking code to return an error to git daemon instead of this?\n\nThe code has even been written:\nhttp://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573\n\nTesting and other improvements would be very welcome.\n"},{"id":"176691","messageId":"CACsJy8Du2f=SZUnV_y6A_x3Uc1o__5EYbhveQ5fu5ivXZ-0adg@mail.gmail.com","threadId":"28543","inReplyTo":"20111002223805.0bd6678b@zappedws","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-02T21:11:01Z","receivedAt":"2011-10-02T21:11:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/10/3 Alexey Shumkin :\n>> This protocol has never been designed for pushing. Attempts to push\n>> over git:// usually result in\n>>\n>>   fatal: The remote end hung up unexpectedly\n>\n> hmmm.. So, how does my project work? ))\n>\n> $ git remote -v\n> origin git://drcis/d/Data/GitRepos/projects/billing+beeline.git (fetch)\n> origin git://drcis/d/Data/GitRepos/projects/billing+beeline.git (push)\n>\n>\n> $git daemon --help\n> ...\n> SERVICES\n> ..\n>       receive-pack\n>           This serves git send-pack clients, allowing anonymous push.\n>           It is disabled by default\n> ...\n>\n> It does not correspond your words...\n>\n> What do I miss?\n\n.. what I said in a later mail, my patch is wrong :)\n-- \nDuy\n"},{"id":"176716","messageId":"20111003074250.GB9455@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1317432415-9459-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-03T07:42:51Z","receivedAt":"2011-10-03T07:42:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:\n\n> This protocol has never been designed for pushing. Attempts to push\n> over git:// usually result in\n> \n> fatal: The remote end hung up unexpectedly\n> \n> That message does not really point out the reason. With this patch, we get\n> \n> error: this protocol does not support pushing\n> error: failed to push some refs to 'git://some-host.com/my/repo'\n\nI thought pushing over git:// _is_ supported. It's just that most\nservers don't have it turned on, for the obvious lack-of-authentication\nreasons.\n\nSee 4b3b1e1 (git-push through git protocol, 2007-01-21), and the\ndiscussion here:\n\n http://thread.gmane.org/gmane.comp.version-control.git/37325\n\nYour patch shuts it off at the client level, so even with it turned on\nfor the server, the client can never get to it.\n\nI still think push-over-git:// is a bit insane, and especially now with\nsmart-http, you'd be crazy to run it. And in that sense, I wouldn't mind\nseeing it deprecated. But just shutting it off without a deprecation\nperiod seems unnecessarily harsh.\n\nThe real problem here seems to be that instead of communicating \"no, we\ndon't support that\", git-daemon just hangs up. It would be a much nicer\nfix if we could change that. I'm not sure it's possible, though. There's\nnot much room in the beginning of the room to make that communication in\na way that's backwards compatible.\n\n-Peff\n"},{"id":"176721","messageId":"4E8975E7.2040804@viscovery.net","threadId":"28543","inReplyTo":"20111003074250.GB9455@sigill.intra.peff.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2011-10-03T08:44:23Z","receivedAt":"2011-10-03T08:44:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 10/3/2011 9:42, schrieb Jeff King:\n> I still think push-over-git:// is a bit insane, and especially now with\n> smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind\n> seeing it deprecated.\n\nYou must be kidding ;) It is so much easier to type\n\n git daemon --export-all --enable=receive-pack\n\nfor a one-shot, temporary git connection compared to setting up a\nsmart-http, ssh, or even a rsh server.\n\n-- Hannes\n"},{"id":"176723","messageId":"CACsJy8C7RXec_Zq8SAdyW2mYh44GBQsJ7sdwR8nHBcwVieV5mg@mail.gmail.com","threadId":"28543","inReplyTo":"20111001052910.GA6502@elie","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T09:12:44Z","receivedAt":"2011-10-03T09:12:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/10/1 Jonathan Nieder :\n> Ilari Liusvaara wrote:\n>\n>> What about sticking code to return an error to git daemon instead of this?\n>\n> The code has even been written:\n> http://thread.gmane.org/gmane.comp.version-control.git/145456/focus=145573\n>\n> Testing and other improvements would be very welcome.\n\nTests aside, are there any problems with the patch? I don't see any\nfollowup discussions. Personally I don't see much value in adding the\ndescription though.\n-- \nDuy\n"},{"id":"176724","messageId":"20111003093912.GA16078@sigill.intra.peff.net","threadId":"28543","inReplyTo":"4E8975E7.2040804@viscovery.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-03T09:39:12Z","receivedAt":"2011-10-03T09:39:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2011 at 10:44:23AM +0200, Johannes Sixt wrote:\n\n> Am 10/3/2011 9:42, schrieb Jeff King:\n> > I still think push-over-git:// is a bit insane, and especially now with\n> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind\n> > seeing it deprecated.\n> \n> You must be kidding ;) It is so much easier to type\n> \n> git daemon --export-all --enable=receive-pack\n> \n> for a one-shot, temporary git connection compared to setting up a\n> smart-http, ssh, or even a rsh server.\n\nAh, yeah, I didn't think about one-shot invocations like that (I think\nthe original motivation was somebody actually running it all the time).\n\nSo yeah, that makes it even worse for the client to start refusing this\nwithout even contacting the server. I forgot that we added the \"ERR\"\nresponse way back in a807328 (connect.c: add a way for git-daemon to\npass an error back to client, 2008-11-01).\n\nGitHub uses it to make nice messages:\n\n $ git push origin\n fatal: remote error:\n You can't push to git://github.com/gitster/git.git\n Use git@github.com:gitster/git.git\n\nWe should maybe do something like the patch below:\n\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..c1fa55f 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -255,6 +255,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tloginfo(\"Request %s for '%s'\", service->name, dir);\n \n \tif (!enabled && !service->overridable) {\n+\t\tpacket_write(1, \"ERR %s: service not enabled\", service->name);\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n \t\treturn -1;\n@@ -288,6 +289,8 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\t\tenabled = service_enabled;\n \t}\n \tif (!enabled) {\n+\t\tpacket_write(1, \"ERR %s: service not enabled for '%s'\",\n+\t\t service->name, path);\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n\nbut:\n\n 1. There is some information leakage there. In particular, one can\n tell the difference now between \"repo does not exist\" and\n \"receive-pack is not turned on\". Personally, I think the tradeoff\n to have actual error messages is worth it. HTTP has had real error\n codes for decades, and I don't think anybody is too up-in-arms that\n I can probe which pages are 404, and which are 401.\n\n 2. It probably makes sense to have a more human-friendly error\n message.\n\n 3. It may be worth adding error messages for lots of other conditions\n (e.g., no such repo). Assuming we accept the information leakage\n for (1).\n\n-Peff\n"},{"id":"176725","messageId":"CACsJy8B7Z-fT+ED=4F-Ug-bhvCagSxr0X6vZqn5PGRfB7KnUTA@mail.gmail.com","threadId":"28543","inReplyTo":"20111003093912.GA16078@sigill.intra.peff.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T09:44:22Z","receivedAt":"2011-10-03T09:44:22Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/10/3 Jeff King :\n> So yeah, that makes it even worse for the client to start refusing this\n> without even contacting the server. I forgot that we added the \"ERR\"\n> response way back in a807328 (connect.c: add a way for git-daemon to\n> pass an error back to client, 2008-11-01).\n>\n> GitHub uses it to make nice messages:\n>\n>  $ git push origin\n>  fatal: remote error:\n>    You can't push to git://github.com/gitster/git.git\n>    Use git@github.com:gitster/git.git\n>\n> We should maybe do something like the patch below:\n\nJonathan also mentions another patch\n\nhttp://article.gmane.org/gmane.comp.version-control.git/182536\n\n> but:\n>\n>  1. There is some information leakage there. In particular, one can\n>     tell the difference now between \"repo does not exist\" and\n>     \"receive-pack is not turned on\". Personally, I think the tradeoff\n>     to have actual error messages is worth it. HTTP has had real error\n>     codes for decades, and I don't think anybody is too up-in-arms that\n>     I can probe which pages are 404, and which are 401.\n\nTo me, just \": access denied\" is enough. Not particularly\nfriendly but should be a good enough clue.\n-- \nDuy\n"},{"id":"176726","messageId":"20111003094730.GA21610@sigill.intra.peff.net","threadId":"28543","inReplyTo":"CACsJy8B7Z-fT+ED=4F-Ug-bhvCagSxr0X6vZqn5PGRfB7KnUTA@mail.gmail.com","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-03T09:47:30Z","receivedAt":"2011-10-03T09:47:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2011 at 08:44:22PM +1100, Nguyen Thai Ngoc Duy wrote:\n\n> > GitHub uses it to make nice messages:\n> >\n> >  $ git push origin\n> >  fatal: remote error:\n> >    You can't push to git://github.com/gitster/git.git\n> >    Use git@github.com:gitster/git.git\n> >\n> > We should maybe do something like the patch below:\n> \n> Jonathan also mentions another patch\n> \n> http://article.gmane.org/gmane.comp.version-control.git/182536\n\nYeah, I was just reading that. Sorry, I should have read the rest of the\nthread more carefully. :)\n\n> >  1. There is some information leakage there. In particular, one can\n> >     tell the difference now between \"repo does not exist\" and\n> >     \"receive-pack is not turned on\". Personally, I think the tradeoff\n> >     to have actual error messages is worth it. HTTP has had real error\n> >     codes for decades, and I don't think anybody is too up-in-arms that\n> >     I can probe which pages are 404, and which are 401.\n> \n> To me, just \": access denied\" is enough. Not particularly\n> friendly but should be a good enough clue.\n\nYeah, maybe. Certainly it's better than \"the remote end hung up\nunexpectedly\".\n\nHowever, the leakage is still there. You would get \"the remote hung up\"\nfor no-such-repo, and \"access denied\" for this. Or were you just\nproposing that _all_ errors give \"access denied\". Certainly it's better\nthan just hanging up, too, and there is no leakage there.\n\nIt might be nice to default to that, and let sites easily enable\nfriendlier messages, though.\n\n-Peff\n"},{"id":"176727","messageId":"m3d3eeo17l.fsf@localhost.localdomain","threadId":"28543","inReplyTo":"4E8975E7.2040804@viscovery.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-03T09:49:15Z","receivedAt":"2011-10-03T09:49:15Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johannes Sixt writes:\n> Am 10/3/2011 9:42, schrieb Jeff King:\n> > I still think push-over-git:// is a bit insane, and especially now with\n> > smart-http, you'd be crazy to run it. And in that sense, I wouldn't mind\n> > seeing it deprecated.\n> \n> You must be kidding ;) It is so much easier to type\n> \n> git daemon --export-all --enable=receive-pack\n> \n> for a one-shot, temporary git connection compared to setting up a\n> smart-http, ssh, or even a rsh server.\n\nI wonder if that is the case... but 48% responders of \"Git User's\nSurvey 2011\" (3424 out of 7100 responders who answered queston \n\"23) How do you publish/propagate your changes?\") answered that they\nuse push via git protocol.\n\nSee https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL\n\n-- \nJakub Narębski\n"},{"id":"176728","messageId":"CACsJy8Br7hvvNM-e0Qir2R-5vH3uq8b5aPH-JFpUwddo-NG9iQ@mail.gmail.com","threadId":"28543","inReplyTo":"20111003094730.GA21610@sigill.intra.peff.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T09:52:23Z","receivedAt":"2011-10-03T09:52:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 3, 2011 at 8:47 PM, Jeff King wrote:\n>> To me, just \": access denied\" is enough. Not particularly\n>> friendly but should be a good enough clue.\n>\n> Yeah, maybe. Certainly it's better than \"the remote end hung up\n> unexpectedly\".\n>\n> However, the leakage is still there. You would get \"the remote hung up\"\n> for no-such-repo, and \"access denied\" for this. Or were you just\n> proposing that _all_ errors give \"access denied\". Certainly it's better\n> than just hanging up, too, and there is no leakage there.\n\nAll of them. At least it's good to know my request has reached (and\nrejected by) the server, not dropped on the floor by some random\nfirewall along the line.\n\n> It might be nice to default to that, and let sites easily enable\n> friendlier messages, though.\n\nI'm thinking of passing \"verbose\" option back to server to get more\nhelpful messages, the option would be turned off by default. It's up\nto admin to decide (would be actually helpful during deployment test,\nfor example). Or is it possible already?\n-- \nDuy\n"},{"id":"176730","messageId":"20111003100245.GC16078@sigill.intra.peff.net","threadId":"28543","inReplyTo":"m3d3eeo17l.fsf@localhost.localdomain","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-03T10:02:45Z","receivedAt":"2011-10-03T10:02:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2011 at 02:49:15AM -0700, Jakub Narebski wrote:\n\n> I wonder if that is the case... but 48% responders of \"Git User's\n> Survey 2011\" (3424 out of 7100 responders who answered queston \n> \"23) How do you publish/propagate your changes?\") answered that they\n> use push via git protocol.\n> \n> See https://www.survs.com/results/Q5CA9SKQ/P7DE07F0PL\n\nI refuse to believe that 48% of people are using git:// to push. Surely\nthey are interpreting that response to overlap with \"git over ssh\" and\n\"git over http\".\n\n-Peff\n"},{"id":"176734","messageId":"20111003110159.GA13064@LK-Perkele-VI.localdomain","threadId":"28543","inReplyTo":"20111003074250.GB9455@sigill.intra.peff.net","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2011-10-03T11:01:59Z","receivedAt":"2011-10-03T11:01:59Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:\n> On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:\n> \n> The real problem here seems to be that instead of communicating \"no, we\n> don't support that\", git-daemon just hangs up. It would be a much nicer\n> fix if we could change that. I'm not sure it's possible, though. There's\n> not much room in the beginning of the room to make that communication in\n> a way that's backwards compatible.\n\nOh, sure it is possible (except for remote snapshot):\n\n$ /usr/bin/git fetch git://localhost/foobar\nfatal: remote error: R access for foobar DENIED to anonymous\n$ /usr/bin/git push git://localhost/foobar\nfatal: remote error: W access for foobar DENIED to anonymous\n$ /usr/bin/git archive --remote=git://localhost/foobar HEAD\nfatal: git archive: protocol error\n$ /usr/bin/git --version\ngit version 1.7.6.3\n\nSupported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over\n2.5 years ago). Oh, and even before that, but with slightly more\nugly error message.\n\nOh, and adding interpretation of ERR packets to git archive is easy\n(and I even happen to have git:// server that can send those to\ntest against):\n\n$ git archive --remote=git://localhost/foobar HEAD\nfatal: remote error: R access for foobar DENIED to anonymous\n\n(I also tested that remote snapshotting of repository that should be\nreadable succeeds, it does).\n\n--- >8 ----\nFrom ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001\nFrom: Ilari Liusvaara \nDate: Mon, 3 Oct 2011 13:55:37 +0300\nSubject: [PATCH] Support ERR in remote archive like in fetch/push\n\nMake ERR as first packet of remote snapshot reply work like it does in\nfetch/push. Lets servers decline remote snapshot with message the same\nway as declining fetch/push with a message.\n\nSigned-off-by: Ilari Liusvaara \n---\n builtin/archive.c | 2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/archive.c b/builtin/archive.c\nindex 883c009..931956d 100644\n--- a/builtin/archive.c\n+++ b/builtin/archive.c\n@@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,\n \tif (strcmp(buf, \"ACK\")) {\n \t\tif (len > 5 && !prefixcmp(buf, \"NACK \"))\n \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n+\t\tif (len > 4 && !prefixcmp(buf, \"ERR \"))\n+\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n \t\tdie(_(\"git archive: protocol error\"));\n \t}\n \n-- \n1.7.7.3.g2791de.dirty\n\n\n-Ilari\n"},{"id":"176735","messageId":"20111003111331.GA12707@elie","threadId":"28543","inReplyTo":"CACsJy8B7Z-fT+ED=4F-Ug-bhvCagSxr0X6vZqn5PGRfB7KnUTA@mail.gmail.com","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-03T11:13:31Z","receivedAt":"2011-10-03T11:13:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> To me, just \": access denied\" is enough. Not particularly\n> friendly but should be a good enough clue.\n\nYes, I think you're right. It also has the benefit of being easily\nparsable, so some day the client might learn to give a friendly\nmessage in the operator's chosen language.\n"},{"id":"176736","messageId":"20111003112649.GA12874@elie","threadId":"28543","inReplyTo":"20111003110159.GA13064@LK-Perkele-VI.localdomain","subject":"[PATCH] Support ERR in remote archive like in fetch/push","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-03T11:26:50Z","receivedAt":"2011-10-03T11:26:50Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ilari Liusvaara wrote:\n\n> Oh, and adding interpretation of ERR packets to git archive is easy\n> (and I even happen to have git:// server that can send those to\n> test against):\n>\n> $ git archive --remote=git://localhost/foobar HEAD\n> fatal: remote error: R access for foobar DENIED to anonymous\n>\n> (I also tested that remote snapshotting of repository that should be\n> readable succeeds, it does).\n\nSounds like a good idea to me. Let's see what René thinks; also\nchanging the subject line to attract other reviewers.\n\n> --- >8 ----\n> From: Ilari Liusvaara \n> Date: Mon, 3 Oct 2011 13:55:37 +0300\n> Subject: [PATCH] Support ERR in remote archive like in fetch/push\n> \n> Make ERR as first packet of remote snapshot reply work like it does in\n> fetch/push. Lets servers decline remote snapshot with message the same\n> way as declining fetch/push with a message.\n> \n> Signed-off-by: Ilari Liusvaara \n> ---\n> builtin/archive.c | 2 ++\n> 1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> diff --git a/builtin/archive.c b/builtin/archive.c\n> index 883c009..931956d 100644\n> --- a/builtin/archive.c\n> +++ b/builtin/archive.c\n> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,\n> \tif (strcmp(buf, \"ACK\")) {\n> \t\tif (len > 5 && !prefixcmp(buf, \"NACK \"))\n> \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n> +\t\tif (len > 4 && !prefixcmp(buf, \"ERR \"))\n> +\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n> \t\tdie(_(\"git archive: protocol error\"));\n> \t}\n> \n> -- \n> 1.7.7.3.g2791de.dirty\n> \n"},{"id":"176737","messageId":"4E89A04A.1060907@lsrfire.ath.cx","threadId":"28543","inReplyTo":"20111003112649.GA12874@elie","subject":"Re: [PATCH] Support ERR in remote archive like in fetch/push","fromName":"René Scharfe","fromEmail":"rene.scharfe@lsrfire.ath.cx","sentAt":"2011-10-03T11:45:14Z","receivedAt":"2011-10-03T11:45:14Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.10.2011 13:26, schrieb Jonathan Nieder:\n> Ilari Liusvaara wrote:\n> \n>> Oh, and adding interpretation of ERR packets to git archive is easy\n>> (and I even happen to have git:// server that can send those to\n>> test against):\n>>\n>> $ git archive --remote=git://localhost/foobar HEAD\n>> fatal: remote error: R access for foobar DENIED to anonymous\n>>\n>> (I also tested that remote snapshotting of repository that should be\n>> readable succeeds, it does).\n> \n> Sounds like a good idea to me. Let's see what René thinks; also\n> changing the subject line to attract other reviewers.\n\nLooks good to me, but I'm not too familiar with the remote protocol.\n\n>> --- >8 ----\n>> From: Ilari Liusvaara \n>> Date: Mon, 3 Oct 2011 13:55:37 +0300\n>> Subject: [PATCH] Support ERR in remote archive like in fetch/push\n>>\n>> Make ERR as first packet of remote snapshot reply work like it does in\n>> fetch/push. Lets servers decline remote snapshot with message the same\n>> way as declining fetch/push with a message.\n>>\n>> Signed-off-by: Ilari Liusvaara \n>> ---\n>> builtin/archive.c | 2 ++\n>> 1 files changed, 2 insertions(+), 0 deletions(-)\n>>\n>> diff --git a/builtin/archive.c b/builtin/archive.c\n>> index 883c009..931956d 100644\n>> --- a/builtin/archive.c\n>> +++ b/builtin/archive.c\n>> @@ -61,6 +61,8 @@ static int run_remote_archiver(int argc, const char **argv,\n>> \tif (strcmp(buf, \"ACK\")) {\n>> \t\tif (len > 5 && !prefixcmp(buf, \"NACK \"))\n>> \t\t\tdie(_(\"git archive: NACK %s\"), buf + 5);\n>> +\t\tif (len > 4 && !prefixcmp(buf, \"ERR \"))\n>> +\t\t\tdie(_(\"remote error: %s\"), buf + 4);\n>> \t\tdie(_(\"git archive: protocol error\"));\n>> \t}\n>> \n>> -- \n>> 1.7.7.3.g2791de.dirty\n>>\n"},{"id":"176761","messageId":"20111003181338.GA13392@duynguyen-vnpc","threadId":"28543","inReplyTo":"20111003110159.GA13064@LK-Perkele-VI.localdomain","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T18:13:38Z","receivedAt":"2011-10-03T18:13:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Oct 03, 2011 at 02:01:59PM +0300, Ilari Liusvaara wrote:\n> On Mon, Oct 03, 2011 at 03:42:51AM -0400, Jeff King wrote:\n> > On Sat, Oct 01, 2011 at 11:26:55AM +1000, Nguyen Thai Ngoc Duy wrote:\n> > \n> > The real problem here seems to be that instead of communicating \"no, we\n> > don't support that\", git-daemon just hangs up. It would be a much nicer\n> > fix if we could change that. I'm not sure it's possible, though. There's\n> > not much room in the beginning of the room to make that communication in\n> > a way that's backwards compatible.\n> \n> Oh, sure it is possible (except for remote snapshot):\n> \n> $ /usr/bin/git fetch git://localhost/foobar\n> fatal: remote error: R access for foobar DENIED to anonymous\n> $ /usr/bin/git push git://localhost/foobar\n> fatal: remote error: W access for foobar DENIED to anonymous\n> $ /usr/bin/git archive --remote=git://localhost/foobar HEAD\n> fatal: git archive: protocol error\n> $ /usr/bin/git --version\n> git version 1.7.6.3\n> \n> Supported for fetch and push since 1.6.1-rc1 (And 1.6.1 was over\n> 2.5 years ago). Oh, and even before that, but with slightly more\n> ugly error message.\n> \n> Oh, and adding interpretation of ERR packets to git archive is easy\n> (and I even happen to have git:// server that can send those to\n> test against):\n> \n> $ git archive --remote=git://localhost/foobar HEAD\n> fatal: remote error: R access for foobar DENIED to anonymous\n> \n> (I also tested that remote snapshotting of repository that should be\n> readable succeeds, it does).\n> \n> --- >8 ----\n> From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001\n> From: Ilari Liusvaara \n> Date: Mon, 3 Oct 2011 13:55:37 +0300\n> Subject: [PATCH] Support ERR in remote archive like in fetch/push\n> \n> Make ERR as first packet of remote snapshot reply work like it does in\n> fetch/push. Lets servers decline remote snapshot with message the same\n> way as declining fetch/push with a message.\n> \n> Signed-off-by: Ilari Liusvaara \n\nYeah, maybe with this patch also?\n\n-- 8< --\nSubject: [PATCH] pack-protocol: document \"ERR\" line\n\nSince a807328 (connect.c: add a way for git-daemon to pass an error\nback to client), git client recognizes \"ERR\" line and prints a\nfriendly message to user if an error happens at server side.\n\nDocument this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/technical/pack-protocol.txt | 7 +++++++\n 1 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/Documentation/technical/pack-protocol.txt b/Documentation/technical/pack-protocol.txt\nindex a7004c6..546980c 100644\n--- a/Documentation/technical/pack-protocol.txt\n+++ b/Documentation/technical/pack-protocol.txt\n@@ -60,6 +60,13 @@ process on the server side over the Git protocol is this:\n \"0039git-upload-pack /schacon/gitbook.git\\0host=example.com\\0\" |\n nc -v example.com 9418\n \n+If the server refuses the request for some reasons, it could abort\n+gracefully with an error message.\n+\n+----\n+ error-line = PKT-LINE(\"ERR\" SP explanation-text)\n+----\n+\n \n SSH Transport\n -------------\n-- \n-- 8< --\n"},{"id":"176779","messageId":"1317670109-16919-1-git-send-email-pclouds@gmail.com","threadId":"28543","inReplyTo":"20111003111331.GA12707@elie","subject":"[PATCH] daemon: print \"access denied\" if a service does not work","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T19:28:29Z","receivedAt":"2011-10-03T19:28:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Note that if a service fails, then \"access denied\" is printed too.\n Not sure if it's a good thing, the service in question may have\n responded to user already. On the other hand, this catches faults\n from start_command() in run_service_command().\n\n daemon.c | 5 ++++-\n 1 files changed, 4 insertions(+), 1 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..6552ca7 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -562,7 +562,10 @@ static int execute(void)\n \t\t\t * Note: The directory here is probably context sensitive,\n \t\t\t * and might depend on the actual service being performed.\n \t\t\t */\n-\t\t\treturn run_service(line + namelen + 5, s);\n+\t\t\tif (!run_service(line + namelen + 5, s))\n+\t\t\t\treturn 0;\n+\t\t\tpacket_write(1, \"ERR %s: access denied\", line + namelen + 5);\n+\t\t\treturn -1;\n \t\t}\n \t}\n \n-- \n1.7.3.1.256.g2539c.dirty\n"},{"id":"176785","messageId":"20111003195444.GB18153@elie","threadId":"28543","inReplyTo":"1317670109-16919-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] daemon: print \"access denied\" if a service does not work","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-03T19:54:44Z","receivedAt":"2011-10-03T19:54:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyễn Thái Ngọc Duy wrote:\n\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -562,7 +562,10 @@ static int execute(void)\n> \t\t\t * Note: The directory here is probably context sensitive,\n> \t\t\t * and might depend on the actual service being performed.\n> \t\t\t */\n> -\t\t\treturn run_service(line + namelen + 5, s);\n> +\t\t\tif (!run_service(line + namelen + 5, s))\n> +\t\t\t\treturn 0;\n> +\t\t\tpacket_write(1, \"ERR %s: access denied\", line + namelen + 5);\n> +\t\t\treturn -1;\n> \t\t}\n\nAt first I liked the simplification relative to the patch I sent.\nThis means the error message is shown when\n\n 1. the service is not enabled at all\n 2. path not allowed (for example because it doesn't exist, because\n of permission problems, or because it is blacklisted)\n 3. the repository is not exported\n 4. the service is not enabled for $path\n 5. the service command exited with nonzero status\n\nUnfortunately I think that last case (#5) would be confusing and would\nbreak protocol, especially when the command dies at an inconvenient\nmoment. Better for the service command to send an appropriate error\nindicator and to just hang up when it fails to do so.\n"},{"id":"176787","messageId":"7vsjn9etm3.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"1317670109-16919-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] daemon: print \"access denied\" if a service does not work","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-03T19:57:08Z","receivedAt":"2011-10-03T19:57:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> Note that if a service fails, then \"access denied\" is printed too.\n\nAfter run_service() returns with a failure, it is simply irresponsible to\nappend an \"ERR\" packet to the output channel without knowing what the\nstate of that channel is (e.g. it might be that the service wrote only\nhalf a pkt_line it wanted to write, and you may be appending to it).\n\nI think the earlier patch from Peff makes the division of responsibility\nclearer. If the daemon's dispatch code notices it does not want to run it,\nit is the daemon's job to report it. Otherwise the service can and should\nreport what it does, and after it starts running, the channel belongs to\nthe service.\n"},{"id":"176789","messageId":"7vobxxes7s.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111003181338.GA13392@duynguyen-vnpc","subject":"Re: [PATCH] transport: do not allow to push over git:// protocol","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-03T20:27:19Z","receivedAt":"2011-10-03T20:27:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n>> From ce3a402e4fa72cf603f92801d6f021ff89d3ac35 Mon Sep 17 00:00:00 2001\n>> From: Ilari Liusvaara \n>> Date: Mon, 3 Oct 2011 13:55:37 +0300\n>> Subject: [PATCH] Support ERR in remote archive like in fetch/push\n>> \n>> Make ERR as first packet of remote snapshot reply work like it does in\n>> fetch/push. Lets servers decline remote snapshot with message the same\n>> way as declining fetch/push with a message.\n>> \n>> Signed-off-by: Ilari Liusvaara \n>\n> Yeah, maybe with this patch also?\n>\n> -- 8< --\n> Subject: [PATCH] pack-protocol: document \"ERR\" line\n>\n> Since a807328 (connect.c: add a way for git-daemon to pass an error\n> back to client), git client recognizes \"ERR\" line and prints a\n> friendly message to user if an error happens at server side.\n>\n> Document this.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n\nMmakes sense; thanks, both of you.\n"},{"id":"176792","messageId":"1317678909-19383-1-git-send-email-pclouds@gmail.com","threadId":"28543","inReplyTo":"7vsjn9etm3.fsf@alter.siamese.dyndns.org","subject":"[PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-03T21:55:09Z","receivedAt":"2011-10-03T21:55:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The message is chosen to avoid leaking information, yet let users know\nthat they are deliberately not allowed to use the service, not a fault\nin service configuration or the service itself.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n OK let's try again. I don't send ERR when faults happen in\n service->fn() (eventually run_service_command) because\n\n - if it's start_command(), it's likely due to service configuration\n fault (wrong --exec-path..)\n\n - if it's finish_command(), the service may have run and sent\n something back to users. We may break the protocol by sending ERR\n\n daemon.c | 12 ++++++++----\n 1 files changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..f0cae24 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!enabled && !service->overridable) {\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\tgoto failed;\n \t}\n \n \tif (!(path = path_ok(dir)))\n-\t\treturn -1;\n+\t\tgoto failed;\n \n \t/*\n \t * Security on the cheap.\n@@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n \t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\tgoto failed;\n \t}\n \n \tif (service->overridable) {\n@@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\tgoto failed;\n \t}\n \n \t/*\n@@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)\n \tsignal(SIGTERM, SIG_IGN);\n \n \treturn service->fn();\n+\n+failed:\n+\tpacket_write(1, \"ERR %s: access denied\", dir);\n+\treturn -1;\n }\n \n static void copy_to_log(int fd)\n-- \n1.7.3.1.256.g2539c.dirty\n"},{"id":"176795","messageId":"7vaa9hemzw.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"1317678909-19383-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-03T22:20:03Z","receivedAt":"2011-10-03T22:20:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> The message is chosen to avoid leaking information, yet let users know\n> that they are deliberately not allowed to use the service, not a fault\n> in service configuration or the service itself.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> OK let's try again. I don't send ERR when faults happen in\n> service->fn() (eventually run_service_command) because\n>\n> - if it's start_command(), it's likely due to service configuration\n> fault (wrong --exec-path..)\n>\n> - if it's finish_command(), the service may have run and sent\n> something back to users. We may break the protocol by sending ERR\n>\n> daemon.c | 12 ++++++++----\n> 1 files changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/daemon.c b/daemon.c\n> index 4c8346d..f0cae24 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -257,11 +257,11 @@ static int run_service(char *dir, struct daemon_service *service)\n> \tif (!enabled && !service->overridable) {\n> \t\tlogerror(\"'%s': service not enabled.\", service->name);\n> \t\terrno = EACCES;\n> -\t\treturn -1;\n> +\t\tgoto failed;\n> \t}\n> \n> \tif (!(path = path_ok(dir)))\n> -\t\treturn -1;\n> +\t\tgoto failed;\n> \n> \t/*\n> \t * Security on the cheap.\n> @@ -277,7 +277,7 @@ static int run_service(char *dir, struct daemon_service *service)\n> \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n> \t\tlogerror(\"'%s': repository not exported.\", path);\n> \t\terrno = EACCES;\n> -\t\treturn -1;\n> +\t\tgoto failed;\n> \t}\n> \n> \tif (service->overridable) {\n> @@ -291,7 +291,7 @@ static int run_service(char *dir, struct daemon_service *service)\n> \t\tlogerror(\"'%s': service not enabled for '%s'\",\n> \t\t\t service->name, path);\n> \t\terrno = EACCES;\n> -\t\treturn -1;\n> +\t\tgoto failed;\n> \t}\n> \n> \t/*\n> @@ -301,6 +301,10 @@ static int run_service(char *dir, struct daemon_service *service)\n> \tsignal(SIGTERM, SIG_IGN);\n> \n> \treturn service->fn();\n> +\n> +failed:\n> +\tpacket_write(1, \"ERR %s: access denied\", dir);\n> +\treturn -1;\n> }\n\nThis looks better.\n\nI think telling \"dir\" back to the user is probably safe (it is not\naffected by what path_ok() does).\n\nI briefly wondered if we also want to say which service failed, but \"dir\"\nis much more likely to be typoed and deserves to be parroted back to help\nthe user realize mistakes, while the service name is not something the\nuser usually types, so the balance the patch strikes is probably the\noptimal.\n\nThanks.\n"},{"id":"177480","messageId":"20111012200916.GA1502@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1317678909-19383-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-12T20:09:16Z","receivedAt":"2011-10-12T20:09:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:\n\n> The message is chosen to avoid leaking information, yet let users know\n> that they are deliberately not allowed to use the service, not a fault\n> in service configuration or the service itself.\n\nI do think this is an improvement, but I wonder if the verbosity should\nbe configurable. Then open sites like kernel.org could be friendlier to\ntheir users. Something like this instead:\n\n---\n daemon.c | 21 +++++++++++++++++----\n 1 files changed, 17 insertions(+), 4 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..ec88fd0 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -20,6 +20,7 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n+static int informative_errors;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n+static int daemon_error(const char *dir, const char *msg)\n+{\n+\tif (!informative_errors)\n+\t\tmsg = \"access denied\";\n+\tpacket_write(1, \"ERR %s: %s\", dir, msg);\n+\treturn -1;\n+}\n+\n static int run_service(char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!enabled && !service->overridable) {\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \tif (!(path = path_ok(dir)))\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"no such repository\");\n \n \t/*\n \t * Security on the cheap.\n@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n \t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"repository not exported\");\n \t}\n \n \tif (service->overridable) {\n@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \t/*\n@@ -1167,6 +1176,10 @@ int main(int argc, char **argv)\n \t\t\tmake_service_overridable(arg + 18, 0);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n+\t\t\tinformative_errors = 1;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n-- \n1.7.7.rc2.21.gb9948\n"},{"id":"177505","messageId":"20111013021404.GA21045@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"20111012200916.GA1502@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-13T02:14:04Z","receivedAt":"2011-10-13T02:14:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Andreas[*])\nJeff King wrote:\n> On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:\n\n>> The message is chosen to avoid leaking information, yet let users know\n>> that they are deliberately not allowed to use the service, not a fault\n>> in service configuration or the service itself.\n>\n> I do think this is an improvement, but I wonder if the verbosity should\n> be configurable. Then open sites like kernel.org could be friendlier to\n> their users. Something like this instead:\n\nFWIW the more verbose version you suggest also sounds fine to me. A\nperson trying to find the names of local users by checking for\nrepositories with names like \"/home/user\" would always receive the\nerror \"no such repository\", whether that user exists or not and\nwhether the actual error encountered was ENOENT, EACCES, lack of git\nmetadata, or the path running afoul of a whitelist or blacklist.\n\nEither Duy's patch or this patch sounds very good to me. Thanks to\nboth of you for working on it.\n\n[*] context:\nhttp://thread.gmane.org/gmane.comp.version-control.git/182529/focus=183409\n"},{"id":"177509","messageId":"20111013044544.GA27890@duynguyen-vnpc.dek-tpc.internal","threadId":"28543","inReplyTo":"20111012200916.GA1502@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-13T04:45:44Z","receivedAt":"2011-10-13T04:45:44Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:\n> On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:\n> \n> > The message is chosen to avoid leaking information, yet let users know\n> > that they are deliberately not allowed to use the service, not a fault\n> > in service configuration or the service itself.\n> \n> I do think this is an improvement, but I wonder if the verbosity should\n> be configurable. Then open sites like kernel.org could be friendlier to\n> their users. Something like this instead:\n\nHow about allow users to select which messages they want to print? We\ncan even go further, allowing users to specify the messages themselves..\n\nI don't know. I'm not a real server admin so maybe I'm just too\nparanoid. Any admins care to speak up?\n\nOn the other hand, grouping all messages at one place may be easier to\naudit, even if we don't allow customization.\n\nAnyway, two cents on top of your patch..\n\n-- 8< --\ndiff --git a/daemon.c b/daemon.c\nindex ec88fd0..a846ef1 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -17,10 +17,25 @@\n #define initgroups(x, y) (0) /* nothing */\n #endif\n \n+/* Must match messages[] order below */\n+#define MSG_SERVICE_NOT_ENABLED 0\n+#define MSG_NO_SUCH_REPOSITORY 1\n+#define MSG_REPOSITORY_NOT_EXPORTED 2\n+\n+static struct daemon_message\n+{\n+\tconst char *message;\n+\tconst char *config;\n+\tint enabled;\n+} messages[] = {\n+\t{ \"service not enabled\", \"message.serviceNotEnabled\" },\n+\t{ \"no such repository\", \"message.noSuchRepository\" },\n+\t{ \"repository not exported\", \"message.repositoryNotExported\" },\n+};\n+\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n-static int informative_errors;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -238,20 +253,31 @@ static int service_enabled;\n \n static int git_daemon_config(const char *var, const char *value, void *cb)\n {\n+\tint i;\n+\n \tif (!prefixcmp(var, \"daemon.\") &&\n \t !strcmp(var + 7, service_looking_at->config_name)) {\n \t\tservice_enabled = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n+\tfor (i = 0; i < ARRAY_SIZE(messages); i++)\n+\t\tif (!strcmp(var, messages[i].config)) {\n+\t\t\tmessages[i].enabled = git_config_bool(var, value);\n+\t\t\treturn 0;\n+\t\t}\n+\n \t/* we are not interested in parsing any other configuration here */\n \treturn 0;\n }\n \n-static int daemon_error(const char *dir, const char *msg)\n+static int daemon_error(const char *dir, int msg_id)\n {\n-\tif (!informative_errors)\n+\tconst char *msg;\n+\tif (!messages[msg_id].enabled)\n \t\tmsg = \"access denied\";\n+\telse\n+\t\tmsg = messages[msg_id].message;\n \tpacket_write(1, \"ERR %s: %s\", dir, msg);\n \treturn -1;\n }\n@@ -266,11 +292,11 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!enabled && !service->overridable) {\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n-\t\treturn daemon_error(dir, \"service not enabled\");\n+\t\treturn daemon_error(dir, MSG_SERVICE_NOT_ENABLED);\n \t}\n \n \tif (!(path = path_ok(dir)))\n-\t\treturn daemon_error(dir, \"no such repository\");\n+\t\treturn daemon_error(dir, MSG_NO_SUCH_REPOSITORY);\n \n \t/*\n \t * Security on the cheap.\n@@ -286,7 +312,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n \t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n-\t\treturn daemon_error(dir, \"repository not exported\");\n+\t\treturn daemon_error(dir, MSG_REPOSITORY_NOT_EXPORTED);\n \t}\n \n \tif (service->overridable) {\n@@ -300,7 +326,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n-\t\treturn daemon_error(dir, \"service not enabled\");\n+\t\treturn daemon_error(dir, MSG_SERVICE_NOT_ENABLED);\n \t}\n \n \t/*\n@@ -1177,7 +1203,9 @@ int main(int argc, char **argv)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n-\t\t\tinformative_errors = 1;\n+\t\t\tint i;\n+\t\t\tfor (i = 0; i < ARRAY_SIZE(messages); i++)\n+\t\t\t\tmessages[i].enabled = 1;\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n-- 8< --\n"},{"id":"177511","messageId":"20111013055924.GA24019@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"20111013044544.GA27890@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-13T05:59:24Z","receivedAt":"2011-10-13T05:59:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> How about allow users to select which messages they want to print? We\n> can even go further, allowing users to specify the messages themselves..\n[...]\n> +\t{ \"service not enabled\", \"message.serviceNotEnabled\" },\n> +\t{ \"no such repository\", \"message.noSuchRepository\" },\n> +\t{ \"repository not exported\", \"message.repositoryNotExported\" },\n\nI administer a private server that is only accessible as \"localhost\".\n:) This much customization would leave me confused about what the\nright choices are and what the choices mean (even if I were to make\nthe server public and start having security worries).\n\nWhat is the intended use --- translation? The idealist in me thinks\nthat should be taken care of on the client side, if at all. (This\nway, we would not be preventing especially friendly clients from\noffering pertinent detailed advice for each error condition.\nAlternatively, maybe some day the protocol will want to provide a way\nfor clients to indicate a preferred language and message verbosity.)\n"},{"id":"177514","messageId":"CACsJy8C6o_-SM4oCM6o5-VDXFy5PBXsE0oL_uhYH1_Zk9h06QQ@mail.gmail.com","threadId":"28543","inReplyTo":"20111013055924.GA24019@elie.hsd1.il.comcast.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-13T06:56:45Z","receivedAt":"2011-10-13T06:56:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 13, 2011 at 4:59 PM, Jonathan Nieder wrote:\n> Nguyen Thai Ngoc Duy wrote:\n>\n>> How about allow users to select which messages they want to print? We\n>> can even go further, allowing users to specify the messages themselves..\n> [...]\n>> +     { \"service not enabled\", \"message.serviceNotEnabled\" },\n>> +     { \"no such repository\", \"message.noSuchRepository\" },\n>> +     { \"repository not exported\", \"message.repositoryNotExported\" },\n>\n> I administer a private server that is only accessible as \"localhost\".\n> :)  This much customization would leave me confused about what the\n> right choices are and what the choices mean (even if I were to make\n> the server public and start having security worries).\n\n--informative-errors is your friend. All errors are enabled.\n\n> What is the intended use --- translation?  The idealist in me thinks\n> that should be taken care of on the client side, if at all.  (This\n> way, we would not be preventing especially friendly clients from\n> offering pertinent detailed advice for each error condition.\n> Alternatively, maybe some day the protocol will want to provide a way\n> for clients to indicate a preferred language and message verbosity.)\n\nTranslation could be fun to do, but it's more about how much admins\nwant to reveal. For example, I may only want to show \"service not\nenabled\" and \"no such repository\", not the last one, which simply\nbecomes \"access denied\".\n\nAgain I'm not real admin and this may be just bogus.\n-- \nDuy\n"},{"id":"177515","messageId":"CACsJy8Dc_=kLtK28VVsOJgwgqAgsmkXrr0s3jR4fno_N-WiqUw@mail.gmail.com","threadId":"28543","inReplyTo":"CACsJy8C6o_-SM4oCM6o5-VDXFy5PBXsE0oL_uhYH1_Zk9h06QQ@mail.gmail.com","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-13T07:02:13Z","receivedAt":"2011-10-13T07:02:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Oct 13, 2011 at 5:56 PM, Nguyen Thai Ngoc Duy wrote:\n> Translation could be fun to do\n\nBy translation, I don't mean inter-language translation. More like\npersonification. Instead of \"service not enabled\" you may want\n\"service is off, you want to attack me or what?\"\n-- \nDuy\n"},{"id":"177575","messageId":"20111013182816.GA17573@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111013044544.GA27890@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-13T18:28:16Z","receivedAt":"2011-10-13T18:28:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 13, 2011 at 03:45:44PM +1100, Nguyen Thai Ngoc Duy wrote:\n\n> On Wed, Oct 12, 2011 at 04:09:16PM -0400, Jeff King wrote:\n> > On Tue, Oct 04, 2011 at 08:55:09AM +1100, Nguyen Thai Ngoc Duy wrote:\n> > \n> > > The message is chosen to avoid leaking information, yet let users know\n> > > that they are deliberately not allowed to use the service, not a fault\n> > > in service configuration or the service itself.\n> > \n> > I do think this is an improvement, but I wonder if the verbosity should\n> > be configurable. Then open sites like kernel.org could be friendlier to\n> > their users. Something like this instead:\n> \n> How about allow users to select which messages they want to print? We\n> can even go further, allowing users to specify the messages themselves..\n\nI thought about that, but it just seemed like it was making things way\nmore complex than it needed to be. GitHub does do this kind of\ncustomization, but we also have a custom layer that intercepts git://\nconnections, anyway, so we added the relevant code there.\n\nI don't know if medium-sized sites (i.e., ones that aren't so big they\nare running custom proxies on the frontend) would care about adding\ncustom messages here or not.\n\n> I don't know. I'm not a real server admin so maybe I'm just too\n> paranoid. Any admins care to speak up?\n\nI doubt anybody would care that much about turning individual messages\non and off. I think the real value is in being able to say \"don't push\nby git://. The right way to push to this site is...\".\n\nBut your patch kind of falls short of what people would want to do for\ntwo reasons:\n\n 1. The message isn't dynamic at all. So I can't say:\n\n You tried to push to git://host.tld/foo.git. The right way to do\n that is:\n\n git push https://host.tld/foo.git\n\n That's what the GitHub message does if you try to push over git://;\n it gives you a new remote name that will actually work, customized\n to the repo you wanted to push to.\n\n 2. Tweaking just the message for anything but \"service not enabled\"\n isn't all that useful. What do you say about \"no such repository\"\n in a simple message, even with placeholders?\n\n If you _really_ want to get fancy, a server could do a fuzzy\n search on the available repos and say \"did you mean...?\".\n But now we are talking about hooking arbitrary code into the\n message.\n\nSo if we want to do anything, I would think it would be a hook. Except\nthat we may or may not have a repo, so it would not be a hook in\n$GIT_DIR/hooks, but rather some script to be run passed on the command\nline, like:\n\n git daemon --informative-errors=/path/to/hook\n\n-Peff\n"},{"id":"177607","messageId":"7vvcrs181e.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111013182816.GA17573@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-14T05:01:01Z","receivedAt":"2011-10-14T05:01:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So if we want to do anything, I would think it would be a hook. Except\n> that we may or may not have a repo, so it would not be a hook in\n> $GIT_DIR/hooks, but rather some script to be run passed on the command\n> line, like:\n>\n> git daemon --informative-errors=/path/to/hook\n\nI don't think it is necessarily good to have such a variation across\nhosting sites. Your \"something like this\" patch looked like it was giving\na reasonable level of detail, IMO.\n"},{"id":"177642","messageId":"20111014131041.GC7808@sigill.intra.peff.net","threadId":"28543","inReplyTo":"7vvcrs181e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T13:10:41Z","receivedAt":"2011-10-14T13:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 13, 2011 at 10:01:01PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > So if we want to do anything, I would think it would be a hook. Except\n> > that we may or may not have a repo, so it would not be a hook in\n> > $GIT_DIR/hooks, but rather some script to be run passed on the command\n> > line, like:\n> >\n> > git daemon --informative-errors=/path/to/hook\n> \n> I don't think it is necessarily good to have such a variation across\n> hosting sites. Your \"something like this\" patch looked like it was giving\n> a reasonable level of detail, IMO.\n\nYeah. With arbitrary messages, the client has no way of programatically\ndeciphering which message is which, so localization becomes impossible.\nHTTP solved this by having a response code _and_ still allowing content\nfor custom pages. That kind of works, though most of the time I find\nthings like custom 404 pages to just be junk.\n\nLet's start with my original patch (which I'll clean and repost). And if\nsomebody really wants to push towards customized messages, that is easy\nenough to do on top later.\n\n-Peff\n"},{"id":"177669","messageId":"20111014192326.GA7713@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111014131041.GC7808@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T19:23:26Z","receivedAt":"2011-10-14T19:23:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 09:10:41AM -0400, Jeff King wrote:\n\n> Let's start with my original patch (which I'll clean and repost). And if\n> somebody really wants to push towards customized messages, that is easy\n> enough to do on top later.\n\nHere it is, with a few minor tweaks:\n\n 1. git-daemon respects --no-informative-errors, too\n\n 2. the new flag is documented\n\n 3. I switched the format from:\n\n fatal: remote: /path/in/your/git/url: access denied\n\n to:\n\n fatal: remote: access denied: /path/in/your/git/url\n\n I find the latter easier to read, and it would be easier for a\n client to recognize.\n\n 4. there's now a commit message\n\n-- >8 --\nSubject: [PATCH] daemon: give friendlier error messages to clients\n\nWhen the git-daemon is asked about an inaccessible\nrepository, it simply hangs up the connection without saying\nanything further. This makes it hard to distinguish between\na repository we cannot access (e.g., due to typo), and a\nservice or network outage.\n\nInstead, let's print an \"ERR\" line, which git clients\nunderstand since v1.6.1 (2008-12-24).\n\nBecause there is a risk of leaking information about\nnon-exported repositories, by default all errors simply say\n\"access denied\". Open sites can pass a flag to turn on more\nspecific messages.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-daemon.txt | 8 ++++++++\n daemon.c | 25 +++++++++++++++++++++----\n 2 files changed, 29 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 69a1e4a..ac57c6d 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -161,6 +161,14 @@ the facility of inet daemon to achieve the same before spawning\n \trepository configuration. By default, all the services\n \tare overridable.\n \n+--informative-errors::\n+\tReturn more verbose errors to the client, differentiating\n+\tconditions like \"no such repository\" from \"repository not\n+\texported\". This is more convenient for clients, but may leak\n+\tinformation about the existence of unexported repositories.\n+\tWithout this option, all errors report \"access denied\" to the\n+\tclient.\n+\n ::\n \tA directory to add to the whitelist of allowed directories. Unless\n \t--strict-paths is specified this will also include subdirectories\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..e5869ec 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -20,6 +20,7 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n+static int informative_errors;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n+static int daemon_error(const char *dir, const char *msg)\n+{\n+\tif (!informative_errors)\n+\t\tmsg = \"access denied\";\n+\tpacket_write(1, \"ERR %s: %s\", msg, dir);\n+\treturn -1;\n+}\n+\n static int run_service(char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!enabled && !service->overridable) {\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \tif (!(path = path_ok(dir)))\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"no such repository\");\n \n \t/*\n \t * Security on the cheap.\n@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n \t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"repository not exported\");\n \t}\n \n \tif (service->overridable) {\n@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \t/*\n@@ -1167,6 +1176,14 @@ int main(int argc, char **argv)\n \t\t\tmake_service_overridable(arg + 18, 0);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n+\t\t\tinformative_errors = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\telse if (!prefixcmp(arg, \"--no-informative-errors\")) {\n+\t\t\tinformative_errors = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177674","messageId":"20111014192741.GA13029@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111014192326.GA7713@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T19:27:41Z","receivedAt":"2011-10-14T19:27:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 03:23:26PM -0400, Jeff King wrote:\n\n> Subject: [PATCH] daemon: give friendlier error messages to clients\n> \n> When the git-daemon is asked about an inaccessible\n> repository, it simply hangs up the connection without saying\n> anything further. This makes it hard to distinguish between\n> a repository we cannot access (e.g., due to typo), and a\n> service or network outage.\n> \n> Instead, let's print an \"ERR\" line, which git clients\n> understand since v1.6.1 (2008-12-24).\n> \n> Because there is a risk of leaking information about\n> non-exported repositories, by default all errors simply say\n> \"access denied\". Open sites can pass a flag to turn on more\n> specific messages.\n\nI'm tempted to suggest this on top:\n\n-- >8 --\nSubject: [PATCH] daemon: turn on informative errors by default\n\nThese are only a problem if you have a bunch of inaccessible\nrepositories served from the same root as your regular\nexported repositories, and you are sensitive about people\nlearning about the existence of those repositories.\n\nGit is foremost an open system, and our defaults should\nreflect that.\n\nSigned-off-by: Jeff King \n---\nBut since it is a potential security issue, it does seem kind of mean to\nclosed sites to just flip the switch on them.\n\n Documentation/git-daemon.txt | 6 +++---\n daemon.c | 2 +-\n 2 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex ac57c6d..2b17175 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -161,12 +161,12 @@ the facility of inet daemon to achieve the same before spawning\n \trepository configuration. By default, all the services\n \tare overridable.\n \n---informative-errors::\n-\tReturn more verbose errors to the client, differentiating\n+--no-informative-errors::\n+\tBy default, we return verbose errors to the client, differentiating\n \tconditions like \"no such repository\" from \"repository not\n \texported\". This is more convenient for clients, but may leak\n \tinformation about the existence of unexported repositories.\n-\tWithout this option, all errors report \"access denied\" to the\n+\tWith this option, all errors report \"access denied\" to the\n \tclient.\n \n ::\ndiff --git a/daemon.c b/daemon.c\nindex e5869ec..ba41a40 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -20,7 +20,7 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n-static int informative_errors;\n+static int informative_errors = 1;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177675","messageId":"7v7h47z5i0.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111014192741.GA13029@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-14T20:24:07Z","receivedAt":"2011-10-14T20:24:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Subject: [PATCH] daemon: turn on informative errors by default\n>\n> These are only a problem if you have a bunch of inaccessible\n> repositories served from the same root as your regular\n> exported repositories, and you are sensitive about people\n> learning about the existence of those repositories.\n>\n> Git is foremost an open system, and our defaults should\n> reflect that.\n>\n> Signed-off-by: Jeff King \n\nI think the logic in the last paragraph is flawed.\n\nThere is a difference between Git being an open system, and installations\nand users of Git being primarily people who work on open projects.\n\nEven though personally I wish there weren't.\n\n> But since it is a potential security issue, it does seem kind of mean to\n> closed sites to just flip the switch on them.\n\nIt would have been a better split to have the 1/2 patch to support both\ninformative and uninformative errors, with the default to say \"access\ndenied\", and 2/2 to flip the default to be more open.\n\nWill queue as-is, though.\n\n> Documentation/git-daemon.txt | 6 +++---\n> daemon.c | 2 +-\n> 2 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> index ac57c6d..2b17175 100644\n> --- a/Documentation/git-daemon.txt\n> +++ b/Documentation/git-daemon.txt\n> @@ -161,12 +161,12 @@ the facility of inet daemon to achieve the same before spawning\n> \trepository configuration. By default, all the services\n> \tare overridable.\n> \n> ---informative-errors::\n> -\tReturn more verbose errors to the client, differentiating\n> +--no-informative-errors::\n> +\tBy default, we return verbose errors to the client, differentiating\n> \tconditions like \"no such repository\" from \"repository not\n> \texported\". This is more convenient for clients, but may leak\n> \tinformation about the existence of unexported repositories.\n> -\tWithout this option, all errors report \"access denied\" to the\n> +\tWith this option, all errors report \"access denied\" to the\n> \tclient.\n"},{"id":"177676","messageId":"20111014203438.GA15643@sigill.intra.peff.net","threadId":"28543","inReplyTo":"7v7h47z5i0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T20:34:38Z","receivedAt":"2011-10-14T20:34:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 01:24:07PM -0700, Junio C Hamano wrote:\n\n> > Git is foremost an open system, and our defaults should\n> > reflect that.\n> [...]\n> \n> I think the logic in the last paragraph is flawed.\n> \n> There is a difference between Git being an open system, and installations\n> and users of Git being primarily people who work on open projects.\n> \n> Even though personally I wish there weren't.\n\nI think it is not the logic that is flawed, but the communication. What\nI meant was that git was originally designed to support open projects\n(like the kernel), and they are our primary target.\n\nIngo said something similar here:\n\n http://article.gmane.org/gmane.linux.kernel/1202320\n\nStill, primary target and primary user are not necessarily the same\nthing. And a minor convenience for one audience that introduces a\nsecurity problem for another audience may not be a good tradeoff, no\nmatter who the audiences are.\n\nI didn't really expect you to take my second patch. We tend to be a bit\nmore conservative than that around here.\n\n> > But since it is a potential security issue, it does seem kind of mean to\n> > closed sites to just flip the switch on them.\n> \n> It would have been a better split to have the 1/2 patch to support both\n> informative and uninformative errors, with the default to say \"access\n> denied\", and 2/2 to flip the default to be more open.\n\nIsn't that what I did? It was what I meant to do, anyway...\n\nOr did you mean the options would have been better worded as:\n\n --errors={terse,informative}\n\nor something similar?\n\n-Peff\n"},{"id":"177677","messageId":"7vpqhzxpsc.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111014203438.GA15643@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-14T20:48:51Z","receivedAt":"2011-10-14T20:48:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> It would have been a better split to have the 1/2 patch to support both\n>> informative and uninformative errors, with the default to say \"access\n>> denied\", and 2/2 to flip the default to be more open.\n>\n> Isn't that what I did? It was what I meant to do, anyway...\n>\n> Or did you mean the options would have been better worded as:\n>\n> --errors={terse,informative}\n>\n> or something similar?\n\nNothing that elaborate.\n\nSupporting --no-* variant even when the default is already no will allow\npeople to prepare their daemon invocation command line beforehand to ensure\nthat they won't be affected to a more lenient default that may or may not\ncome in the future. That's all.\n"},{"id":"177680","messageId":"20111014210251.GD16371@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"20111014192326.GA7713@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-14T21:02:51Z","receivedAt":"2011-10-14T21:02:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> When the git-daemon is asked about an inaccessible\n> repository, it simply hangs up the connection without saying\n> anything further. This makes it hard to distinguish between\n> a repository we cannot access (e.g., due to typo), and a\n> service or network outage.\n\n*nod*\n\n> Instead, let's print an \"ERR\" line, which git clients\n> understand since v1.6.1 (2008-12-24).\n\nJust to be clear, \"git archive --remote\" does not understand ERR lines\nin the 'master' branch (though 908aaceb makes it understand them in\n'next'). But I consider even distinguishing\n\n a. fatal: git archive: protocol error\n b. fatal: git archive: expected ACK/NAK, got EOF\n\n[(a) is how an ERR response is reported, and (b) a remote hangup] to\nbe progress, so it's not so important. :)\n\n> Because there is a risk of leaking information about\n> non-exported repositories, by default all errors simply say\n> \"access denied\". Open sites can pass a flag to turn on more\n> specific messages.\n\nI'm not sure what an \"open site\" is. :) But having this flag for\nsites to declare whether they consider whether a repository exists to\nbe privileged information seems reasonable to me.\n\nNote that this really would be privileged information in some\nnot-too-weird cases. For example, if many users have a repository at\n~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to\naccess\n\n\t/home/alice/.git\n\t/home/alice/.config/.git\n\t/home/alice/src/linux/.git\n\t/home/bob/.git\n\t...\n\nin turn to find a valid username, as reconnaisance for a later\nattack not involving git.\n\nLuckily, this can be avoided with\n\n\tgit daemon --user-path=public_git --base-path=/pub/git\n\nwhich only allows access to subdirectories of /pub/git and\npublic_git in home directories. With\n\n\tgit daemon --base-path=/pub/git\n\nthere is still no problem, since access to home directories is not\nallowed at all in that case. I suppose the documentation should\nmention that --informative-errors is best not used unless --base-path\nis also in use. (Almost everyone is already using --base-path, so\nthis isn't a very serious problem.)\n\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -20,6 +20,7 @@\n[...]\n> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)\n> \t\t\tmake_service_overridable(arg + 18, 0);\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n> +\t\t\tinformative_errors = 1;\n> +\t\t\tcontinue;\n> +\t\t}\n> +\t\telse if (!prefixcmp(arg, \"--no-informative-errors\")) {\n> +\t\t\tinformative_errors = 0;\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tif (!strcmp(arg, \"--\")) {\n\nMicronit: uncuddled \"else\". The style of the surrounding code is to\njust not include the \"else\" at all and rely on \"continue\" to\nshort-circuit things.\n\nAnyway, except for the documentation nits mentioned above (and Junio's\nnit, too),\n\nReviewed-by: Jonathan Nieder \n\nThanks a lot for this.\n"},{"id":"177681","messageId":"20111014210506.GA16226@sigill.intra.peff.net","threadId":"28543","inReplyTo":"7vpqhzxpsc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T21:05:06Z","receivedAt":"2011-10-14T21:05:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 01:48:51PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> It would have been a better split to have the 1/2 patch to support both\n> >> informative and uninformative errors, with the default to say \"access\n> >> denied\", and 2/2 to flip the default to be more open.\n> >\n> > Isn't that what I did? It was what I meant to do, anyway...\n> >\n> > Or did you mean the options would have been better worded as:\n> >\n> > --errors={terse,informative}\n> >\n> > or something similar?\n> \n> Nothing that elaborate.\n> \n> Supporting --no-* variant even when the default is already no will allow\n> people to prepare their daemon invocation command line beforehand to ensure\n> that they won't be affected to a more lenient default that may or may not\n> come in the future. That's all.\n\nOh. Then look again at 1/2. It supports both forms; I just didn't bother\nadvertising the --no form in the manpage, since it was the default.\n\n-Peff\n"},{"id":"177682","messageId":"20111014210646.GE16371@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"20111014210506.GA16226@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-14T21:06:46Z","receivedAt":"2011-10-14T21:06:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Oh. Then look again at 1/2. It supports both forms; I just didn't bother\n> advertising the --no form in the manpage, since it was the default.\n\nYes, and that was the bug Junio mentioned. If we are considering 2/2\nthen admins will need to know about the --no form before we roll out\nthe change in default. :)\n"},{"id":"177683","messageId":"20111014211244.GA16429@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111014210251.GD16371@elie.hsd1.il.comcast.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T21:12:44Z","receivedAt":"2011-10-14T21:12:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 14, 2011 at 04:02:51PM -0500, Jonathan Nieder wrote:\n\n> > Instead, let's print an \"ERR\" line, which git clients\n> > understand since v1.6.1 (2008-12-24).\n> \n> Just to be clear, \"git archive --remote\" does not understand ERR lines\n> in the 'master' branch (though 908aaceb makes it understand them in\n> 'next'). But I consider even distinguishing\n> \n> a. fatal: git archive: protocol error\n> b. fatal: git archive: expected ACK/NAK, got EOF\n> \n> [(a) is how an ERR response is reported, and (b) a remote hangup] to\n> be progress, so it's not so important. :)\n\nThanks, I forgot to mention that. It's not as nice as the push/fetch\ncase, but I agree it's a step forward.\n\n> > Because there is a risk of leaking information about\n> > non-exported repositories, by default all errors simply say\n> > \"access denied\". Open sites can pass a flag to turn on more\n> > specific messages.\n> \n> I'm not sure what an \"open site\" is. :) But having this flag for\n> sites to declare whether they consider whether a repository exists to\n> be privileged information seems reasonable to me.\n\nI meant sites which are just serving a bunch of public repos, like\nkernel.org.\n\n> Note that this really would be privileged information in some\n> not-too-weird cases. For example, if many users have a repository at\n> ~/.git, ~/.config/.git, or ~/src/linux/.git, then someone might try to\n> access\n> \n> \t/home/alice/.git\n> \t/home/alice/.config/.git\n> \t/home/alice/src/linux/.git\n> \t/home/bob/.git\n> \t...\n> \n> in turn to find a valid username, as reconnaisance for a later\n> attack not involving git.\n\nI sort of assume everybody serves a specific directory hierarchy, but\nmaybe that is not the case. I don't run git-daemon myself, so I am\nprobably guilty of generalizing how other people use it.\n\nAnyway, I think the issue is sufficiently nuanced that we should keep\nthe default to the conservative \"access denied\" (i.e., throw away my\nsecond patch for now).\n\n> > --- a/daemon.c\n> > +++ b/daemon.c\n> > @@ -20,6 +20,7 @@\n> [...]\n> > @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)\n> > \t\t\tmake_service_overridable(arg + 18, 0);\n> > \t\t\tcontinue;\n> > \t\t}\n> > +\t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n> > +\t\t\tinformative_errors = 1;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > +\t\telse if (!prefixcmp(arg, \"--no-informative-errors\")) {\n> > +\t\t\tinformative_errors = 0;\n> > +\t\t\tcontinue;\n> > +\t\t}\n> > \t\tif (!strcmp(arg, \"--\")) {\n> \n> Micronit: uncuddled \"else\". The style of the surrounding code is to\n> just not include the \"else\" at all and rely on \"continue\" to\n> short-circuit things.\n\nOops. Yes, it should just drop the else to match the surrounding code.\n\n-Peff\n"},{"id":"177684","messageId":"20111014211921.GB16429@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111014211244.GA16429@sigill.intra.peff.net","subject":"[PATCHv3] daemon: give friendlier error messages to clients","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-14T21:19:21Z","receivedAt":"2011-10-14T21:19:21Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When the git-daemon is asked about an inaccessible\nrepository, it simply hangs up the connection without saying\nanything further. This makes it hard to distinguish between\na repository we cannot access (e.g., due to typo), and a\nservice or network outage.\n\nInstead, let's print an \"ERR\" line, which git clients\nunderstand since v1.6.1 (2008-12-24).\n\nBecause there is a risk of leaking information about\nnon-exported repositories, by default all errors simply say\n\"access denied\". Sites which don't have hidden repositories,\nor don't care, can pass a flag to turn on more specific\nmessages.\n\nSigned-off-by: Jeff King \n---\nMinor tweaks to the documentation and code style to make Jonathan happy.\n:)\n\nNote: I labeled this \"v3\", as it is the third one posted, but the prior\nones were not labeled with versions at all.\n\n Documentation/git-daemon.txt | 10 ++++++++++\n daemon.c | 25 +++++++++++++++++++++----\n 2 files changed, 31 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\nindex 69a1e4a..31b28fc 100644\n--- a/Documentation/git-daemon.txt\n+++ b/Documentation/git-daemon.txt\n@@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning\n \trepository configuration. By default, all the services\n \tare overridable.\n \n+--informative-errors::\n+--no-informative-errors::\n+\tWhen informative errors are turned on, git-daemon will report\n+\tmore verbose errors to the client, differentiating conditions\n+\tlike \"no such repository\" from \"repository not exported\". This\n+\tis more convenient for clients, but may leak information about\n+\tthe existence of unexported repositories. When informative\n+\terrors are not enabled, all errors report \"access denied\" to the\n+\tclient. The default is --no-informative-errors.\n+\n ::\n \tA directory to add to the whitelist of allowed directories. Unless\n \t--strict-paths is specified this will also include subdirectories\ndiff --git a/daemon.c b/daemon.c\nindex 4c8346d..6f111af 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -20,6 +20,7 @@\n static int log_syslog;\n static int verbose;\n static int reuseaddr;\n+static int informative_errors;\n \n static const char daemon_usage[] =\n \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n@@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n \treturn 0;\n }\n \n+static int daemon_error(const char *dir, const char *msg)\n+{\n+\tif (!informative_errors)\n+\t\tmsg = \"access denied\";\n+\tpacket_write(1, \"ERR %s: %s\", msg, dir);\n+\treturn -1;\n+}\n+\n static int run_service(char *dir, struct daemon_service *service)\n {\n \tconst char *path;\n@@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!enabled && !service->overridable) {\n \t\tlogerror(\"'%s': service not enabled.\", service->name);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \tif (!(path = path_ok(dir)))\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"no such repository\");\n \n \t/*\n \t * Security on the cheap.\n@@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \tif (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n \t\tlogerror(\"'%s': repository not exported.\", path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"repository not exported\");\n \t}\n \n \tif (service->overridable) {\n@@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\tlogerror(\"'%s': service not enabled for '%s'\",\n \t\t\t service->name, path);\n \t\terrno = EACCES;\n-\t\treturn -1;\n+\t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n \t/*\n@@ -1167,6 +1176,14 @@ int main(int argc, char **argv)\n \t\t\tmake_service_overridable(arg + 18, 0);\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--informative-errors\")) {\n+\t\t\tinformative_errors = 1;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!prefixcmp(arg, \"--no-informative-errors\")) {\n+\t\t\tinformative_errors = 0;\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!strcmp(arg, \"--\")) {\n \t\t\tok_paths = &argv[i+1];\n \t\t\tbreak;\n-- \n1.7.6.4.37.g43b58b\n"},{"id":"177685","messageId":"20111014212026.GF16371@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"20111014192741.GA13029@sigill.intra.peff.net","subject":"Re: [PATCH] daemon: return \"access denied\" if a service is not allowed","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-14T21:20:26Z","receivedAt":"2011-10-14T21:20:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> I'm tempted to suggest this on top:\n>\n> -- >8 --\n> Subject: [PATCH] daemon: turn on informative errors by default\n\nVery good idea, as long as we are cautious about making sure admins\nknow about the change before it comes (for example using release\nnotes).\n\n[...]\n> Git is foremost an open system, and our defaults should\n> reflect that.\n\nI think this is a lousy justification. :)\n\nSure, certain prominent users of git (like kernel.org) would probably\nnot want to set --no-informative-errors. And you and I might prefer\nthat _nobody_ set --no-informative-errors. But this does not mean the\ndesign of Git has anything to do with that, and I am afraid of the\ndirection it would take us in if we start pretending it does.\n\nThe git daemon is primarily a functional, secure, admin-friendly and\nclient-friendly program whose defaults should make admins happy where\npossible. Luckily all that is consistent with --informative-errors.\nIn most use cases (i.e., not weird military security kinds of things),\nalthough --informative-errors without --base-path could have negative\nsecurity impact as Andreas has explained before, --informative-errors\nwith --base-path is harmless as far as I can tell.\n\nI am tempted to propose making --base-path mandatory when there is not\nat least one path_ok argument, so in the unusual case, people would\nhave to explicitly say they want to serve repositories rooted at /.\n\nJust my two cents,\nJonathan\n"},{"id":"177687","messageId":"7vhb3bxmuh.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111014211921.GB16429@sigill.intra.peff.net","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-14T21:52:22Z","receivedAt":"2011-10-14T21:52:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both. I'll drop the previous one, and rebuild the integration\nbranches by queuign this version instead.\n"},{"id":"177702","messageId":"CAMK1S_g0aKUa=+ndAm7rqeoPAobjVb6oJ1Z4DqSeNrdauXNH3w@mail.gmail.com","threadId":"28543","inReplyTo":"20111014211921.GB16429@sigill.intra.peff.net","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-10-14T23:39:33Z","receivedAt":"2011-10-14T23:39:33Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Sat, Oct 15, 2011 at 2:49 AM, Jeff King wrote:\n> When the git-daemon is asked about an inaccessible\n> repository, it simply hangs up the connection without saying\n> anything further. This makes it hard to distinguish between\n> a repository we cannot access (e.g., due to typo), and a\n> service or network outage.\n>\n> Instead, let's print an \"ERR\" line, which git clients\n> understand since v1.6.1 (2008-12-24).\n>\n> Because there is a risk of leaking information about\n> non-exported repositories, by default all errors simply say\n> \"access denied\". Sites which don't have hidden repositories,\n\nI suggest that even the \"secure\" version of the message say something\nlike \"access denied or repository not exported\". You're still not\nleaking anything, but it reduces confusion to the user, who otherwise\nmay not realise it *could be* the latter.\n\nregards\n\nsitaram\n\n> or don't care, can pass a flag to turn on more specific\n> messages.\n>\n> Signed-off-by: Jeff King \n> ---\n> Minor tweaks to the documentation and code style to make Jonathan happy.\n> :)\n>\n> Note: I labeled this \"v3\", as it is the third one posted, but the prior\n> ones were not labeled with versions at all.\n>\n>  Documentation/git-daemon.txt |   10 ++++++++++\n>  daemon.c                     |   25 +++++++++++++++++++++----\n>  2 files changed, 31 insertions(+), 4 deletions(-)\n>\n> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt\n> index 69a1e4a..31b28fc 100644\n> --- a/Documentation/git-daemon.txt\n> +++ b/Documentation/git-daemon.txt\n> @@ -161,6 +161,16 @@ the facility of inet daemon to achieve the same before spawning\n>        repository configuration.  By default, all the services\n>        are overridable.\n>\n> +--informative-errors::\n> +--no-informative-errors::\n> +       When informative errors are turned on, git-daemon will report\n> +       more verbose errors to the client, differentiating conditions\n> +       like \"no such repository\" from \"repository not exported\". This\n> +       is more convenient for clients, but may leak information about\n> +       the existence of unexported repositories.  When informative\n> +       errors are not enabled, all errors report \"access denied\" to the\n> +       client. The default is --no-informative-errors.\n> +\n>  ::\n>        A directory to add to the whitelist of allowed directories. Unless\n>        --strict-paths is specified this will also include subdirectories\n> diff --git a/daemon.c b/daemon.c\n> index 4c8346d..6f111af 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -20,6 +20,7 @@\n>  static int log_syslog;\n>  static int verbose;\n>  static int reuseaddr;\n> +static int informative_errors;\n>\n>  static const char daemon_usage[] =\n>  \"git daemon [--verbose] [--syslog] [--export-all]\\n\"\n> @@ -247,6 +248,14 @@ static int git_daemon_config(const char *var, const char *value, void *cb)\n>        return 0;\n>  }\n>\n> +static int daemon_error(const char *dir, const char *msg)\n> +{\n> +       if (!informative_errors)\n> +               msg = \"access denied\";\n> +       packet_write(1, \"ERR %s: %s\", msg, dir);\n> +       return -1;\n> +}\n> +\n>  static int run_service(char *dir, struct daemon_service *service)\n>  {\n>        const char *path;\n> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)\n>        if (!enabled && !service->overridable) {\n>                logerror(\"'%s': service not enabled.\", service->name);\n>                errno = EACCES;\n> -               return -1;\n> +               return daemon_error(dir, \"service not enabled\");\n>        }\n>\n>        if (!(path = path_ok(dir)))\n> -               return -1;\n> +               return daemon_error(dir, \"no such repository\");\n>\n>        /*\n>         * Security on the cheap.\n> @@ -277,7 +286,7 @@ static int run_service(char *dir, struct daemon_service *service)\n>        if (!export_all_trees && access(\"git-daemon-export-ok\", F_OK)) {\n>                logerror(\"'%s': repository not exported.\", path);\n>                errno = EACCES;\n> -               return -1;\n> +               return daemon_error(dir, \"repository not exported\");\n>        }\n>\n>        if (service->overridable) {\n> @@ -291,7 +300,7 @@ static int run_service(char *dir, struct daemon_service *service)\n>                logerror(\"'%s': service not enabled for '%s'\",\n>                         service->name, path);\n>                errno = EACCES;\n> -               return -1;\n> +               return daemon_error(dir, \"service not enabled\");\n>        }\n>\n>        /*\n> @@ -1167,6 +1176,14 @@ int main(int argc, char **argv)\n>                        make_service_overridable(arg + 18, 0);\n>                        continue;\n>                }\n> +               if (!prefixcmp(arg, \"--informative-errors\")) {\n> +                       informative_errors = 1;\n> +                       continue;\n> +               }\n> +               if (!prefixcmp(arg, \"--no-informative-errors\")) {\n> +                       informative_errors = 0;\n> +                       continue;\n> +               }\n>                if (!strcmp(arg, \"--\")) {\n>                        ok_paths = &argv[i+1];\n>                        break;\n> --\n> 1.7.6.4.37.g43b58b\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n\n\n\n-- \nSitaram\n"},{"id":"177703","messageId":"CACsJy8CB4uBZkM8xPH6N+HPR-KX+KCCvfT4RrBV+gzTy=zz8og@mail.gmail.com","threadId":"28543","inReplyTo":"20111014211921.GB16429@sigill.intra.peff.net","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-10-15T00:51:15Z","receivedAt":"2011-10-15T00:51:15Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Oct 15, 2011 at 8:19 AM, Jeff King wrote:\n> @@ -257,11 +266,11 @@ static int run_service(char *dir, struct daemon_service *service)\n>        if (!enabled && !service->overridable) {\n>                logerror(\"'%s': service not enabled.\", service->name);\n>                errno = EACCES;\n> -               return -1;\n> +               return daemon_error(dir, \"service not enabled\");\n>        }\n\nNit picking. In this case the service is disabled entirely regardless\ndir and it uses the same message with the case where service is\ndisabled per repo later on. Maybe we could reword it a little bit to\ndifferentiate the two cases? Say the first case \"service disabled\",\nand the second one \"service not enabled\"?\n-- \nDuy\n"},{"id":"177714","messageId":"7vk486x0hq.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"CAMK1S_g0aKUa=+ndAm7rqeoPAobjVb6oJ1Z4DqSeNrdauXNH3w@mail.gmail.com","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-15T05:55:13Z","receivedAt":"2011-10-15T05:55:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sitaram Chamarty writes:\n\n>> Because there is a risk of leaking information about\n>> non-exported repositories, by default all errors simply say\n>> \"access denied\". Sites which don't have hidden repositories,\n>\n> I suggest that even the \"secure\" version of the message say something\n> like \"access denied or repository not exported\". You're still not\n> leaking anything, but it reduces confusion to the user, who otherwise\n> may not realise it *could be* the latter.\n\nI kind of like the suggestion, but I am afraid that \"access denied,\nrepository nonexistent or not exported\" can soon easily get long enough to\nbe unmanageable.\n"},{"id":"177715","messageId":"CAMK1S_gkB49qhnt8U=3G3UPnjo2vzFx5mL4cOM1Ubu68ySJrDA@mail.gmail.com","threadId":"28543","inReplyTo":"7vk486x0hq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-10-15T07:09:30Z","receivedAt":"2011-10-15T07:09:30Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano wrote:\n> Sitaram Chamarty writes:\n>\n>>> Because there is a risk of leaking information about\n>>> non-exported repositories, by default all errors simply say\n>>> \"access denied\". Sites which don't have hidden repositories,\n>>\n>> I suggest that even the \"secure\" version of the message say something\n>> like \"access denied or repository not exported\".  You're still not\n>> leaking anything, but it reduces confusion to the user, who otherwise\n>> may not realise it *could be* the latter.\n>\n> I kind of like the suggestion, but I am afraid that \"access denied,\n> repository nonexistent or not exported\" can soon easily get long enough to\n> be unmanageable.\n\nWhen someone who *does* have access makes a typo, \"access denied\"\nmakes it harder to realise it, because it subtly implies the repo\n*does* exist and it's an ACL issue. I've seen lots of frustrating\nback-and-forth between admin and user before someone eventually\nnoticed the typo.\n\n\"Access denied or no such repo\" is much better. (The \"not exported\"\nnuance is not relevant in this context; you can safely ignore it.)\n\nregards\n\n-- \nSitaram\n"},{"id":"177716","messageId":"m3r52e7js7.fsf@localhost.localdomain","threadId":"28543","inReplyTo":"CAMK1S_gkB49qhnt8U=3G3UPnjo2vzFx5mL4cOM1Ubu68ySJrDA@mail.gmail.com","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2011-10-15T08:16:43Z","receivedAt":"2011-10-15T08:16:43Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Sitaram Chamarty writes:\n> On Sat, Oct 15, 2011 at 11:25 AM, Junio C Hamano wrote:\n>> Sitaram Chamarty writes:\n>>\n>>>> Because there is a risk of leaking information about\n>>>> non-exported repositories, by default all errors simply say\n>>>> \"access denied\". Sites which don't have hidden repositories,\n>>>\n>>> I suggest that even the \"secure\" version of the message say something\n>>> like \"access denied or repository not exported\".  You're still not\n>>> leaking anything, but it reduces confusion to the user, who otherwise\n>>> may not realise it *could be* the latter.\n>>\n>> I kind of like the suggestion, but I am afraid that \"access denied,\n>> repository nonexistent or not exported\" can soon easily get long enough to\n>> be unmanageable.\n> \n> When someone who *does* have access makes a typo, \"access denied\"\n> makes it harder to realise it, because it subtly implies the repo\n> *does* exist and it's an ACL issue. I've seen lots of frustrating\n> back-and-forth between admin and user before someone eventually\n> noticed the typo.\n> \n> \"Access denied or no such repo\" is much better. (The \"not exported\"\n> nuance is not relevant in this context; you can safely ignore it.)\n\nTo join this bike-shedding:\n\n \"Access denied or repository not available\"\n\nor just\n\n \"Repository not available\"\n\n-- \nJakub Narębski\n"},{"id":"177717","messageId":"20111015082647.GA7302@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"m3r52e7js7.fsf@localhost.localdomain","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-15T08:26:47Z","receivedAt":"2011-10-15T08:26:47Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jakub Narebski wrote:\n> Sitaram Chamarty writes:\n\n>> \"Access denied or no such repo\" is much better. (The \"not exported\"\n>> nuance is not relevant in this context; you can safely ignore it.)\n>\n> To join this bike-shedding:\n>\n> \"Access denied or repository not available\"\n>\n> or just\n>\n> \"Repository not available\"\n\nIf such details about the message matter, then I have to say that\nSitaram's \"access denied or no such repository\" is as close to perfect\nas I can imagine. The admin who is eventually forwarded this message\nwill be reminded to check two things:\n\n - that access is not denied, neither globally, using a whitelist,\n using filesystem permissions, nor by leaving out\n git-daemon-export-ok, and\n\n - that such a repo exists at all, and there was not a typo in the\n address.\n"},{"id":"177746","messageId":"7vzkh1vrdq.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111015082647.GA7302@elie.hsd1.il.comcast.net","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-15T20:13:11Z","receivedAt":"2011-10-15T20:13:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> ... The admin who is eventually forwarded this message\n> will be reminded ...\n\nThe admin has access to logs that record the real cause anyway, no?\nI thought this was all about how the error is given to the end user.\n"},{"id":"177749","messageId":"20111015221711.GA17470@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"7vzkh1vrdq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-10-15T22:17:11Z","receivedAt":"2011-10-15T22:17:11Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> The admin has access to logs that record the real cause anyway, no?\n\nYes, you're right. If this is a good admin then she will look at the\nlogs, preventing the back-and-forth Sitaram described.\n\nThough that doesn't really change anything fundamental. It seems nice\nto remind the end user to check for typos, too.\n"},{"id":"177754","messageId":"CAMK1S_iHCUrKc24kYqfUnmgEQ8yeAHiQ2StQAcPRknJ8-CvyFw@mail.gmail.com","threadId":"28543","inReplyTo":"20111015221711.GA17470@elie.hsd1.il.comcast.net","subject":"Re: [PATCHv3] daemon: give friendlier error messages to clients","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2011-10-16T01:51:31Z","receivedAt":"2011-10-16T01:51:31Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Sun, Oct 16, 2011 at 3:47 AM, Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n>\n>> The admin has access to logs that record the real cause anyway, no?\n>\n> Yes, you're right.  If this is a good admin then she will look at the\n> logs, preventing the back-and-forth Sitaram described.\n\nActually, even if it's a good admin, you're adding to her load needlessly.\n\n> Though that doesn't really change anything fundamental.  It seems nice\n> to remind the end user to check for typos, too.\n\nYup.\n\nDId I mention \"been there, done that\" in my earlier email? I'm not\nbike-shedding -- there *is* an impact on productivity in terms of how\npeople troubleshoot when they run across a problem, and I really *do*\nfeel strongly about this in principle (even though I don't use\ngit-daemon myself so it doesn't bother me how you decide in this\n*specific* case)\n\nI'll shut up now... :-)\n\n-- \nSitaram\n"},{"id":"177805","messageId":"1318803076-4229-1-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"20111014211921.GB16429@sigill.intra.peff.net","subject":"[PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-16T22:11:15Z","receivedAt":"2011-10-16T22:11:15Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The semantics of the git daemon tests are similar to the http\ntransport tests. In fact, they are only a slightly modified copy\nof t5550, plus the newly added remote error tests.\n\nAll daemon tests will be skipped unless the environment variable\nGIT_TEST_DAEMON is set.\n\nSigned-off-by: Clemens Buchacher \n---\n\nThis patch is based on jk/daemon-msgs.\n\n t/lib-daemon.sh | 52 +++++++++++++++++\n t/t5570-git-daemon.sh | 148 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 200 insertions(+), 0 deletions(-)\n create mode 100644 t/lib-daemon.sh\n create mode 100755 t/t5570-git-daemon.sh\n\ndiff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\nnew file mode 100644\nindex 0000000..30a89ea\n--- /dev/null\n+++ b/t/lib-daemon.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+if test -z \"$GIT_TEST_DAEMON\"\n+then\n+\tskip_all=\"Daemon testing disabled (define GIT_TEST_DAEMON to enable)\"\n+\ttest_done\n+fi\n+\n+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n+\n+DAEMON_PID=\n+DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n+DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT\n+\n+start_daemon() {\n+\tif test -n \"$DAEMON_PID\"\n+\tthen\n+\t\terror \"start_daemon already called\"\n+\tfi\n+\n+\tmkdir -p \"$DAEMON_DOCUMENT_ROOT_PATH\"\n+\n+\ttrap 'code=$?; stop_daemon; (exit $code); die' EXIT\n+\n+\tsay >&3 \"Starting git daemon ...\"\n+\tgit daemon --listen=127.0.0.1 --port=\"$LIB_DAEMON_PORT\" \\\n+\t\t--reuseaddr --verbose \\\n+\t\t--base-path=\"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t\"$@\" \"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t>&3 2>&4 &\n+\tDAEMON_PID=$!\n+}\n+\n+stop_daemon() {\n+\tif test -z \"$DAEMON_PID\"\n+\tthen\n+\t\treturn\n+\tfi\n+\n+\ttrap 'die' EXIT\n+\n+\t# kill git-daemon child of git\n+\tsay >&3 \"Stopping git daemon ...\"\n+\tpkill -P \"$DAEMON_PID\"\n+\twait \"$DAEMON_PID\"\n+\tret=$?\n+\tif test $ret -ne 143\n+\tthen\n+\t\terror \"git daemon exited with status: $ret\"\n+\tfi\n+\tDAEMON_PID=\n+}\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nnew file mode 100755\nindex 0000000..aa5771a\n--- /dev/null\n+++ b/t/t5570-git-daemon.sh\n@@ -0,0 +1,148 @@\n+#!/bin/sh\n+\n+test_description='test fetching over git protocol'\n+. ./test-lib.sh\n+\n+. \"$TEST_DIRECTORY\"/lib-daemon.sh\n+start_daemon\n+\n+test_expect_success 'setup repository' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one\n+'\n+\n+test_expect_success 'create git-accessible bare repository' '\n+\tmkdir \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t git --bare init &&\n+\t : >git-daemon-export-ok\n+\t) &&\n+\tgit remote add public \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'clone git repository' '\n+\tgit clone $DAEMON_URL/repo.git clone &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_success 'fetch changes via git protocol' '\n+\techo content >>file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\t(cd clone && git pull) &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_failure 'remote detects correct HEAD' '\n+\tgit push public master:other &&\n+\t(cd clone &&\n+\t git remote set-head -d origin &&\n+\t git remote set-head -a origin &&\n+\t git symbolic-ref refs/remotes/origin/HEAD > output &&\n+\t echo refs/remotes/origin/master > expect &&\n+\t test_cmp expect output\n+\t)\n+'\n+\n+test_expect_success 'prepare pack objects' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack &&\n+\t git --bare prune-packed\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n+test_remote_error()\n+{\n+\tdo_export=YesPlease\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase $1 in\n+\t\t-x)\n+\t\t\tshift\n+\t\t\tchmod -X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t\t\t;;\n+\t\t-n)\n+\t\t\tshift\n+\t\t\tdo_export=\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\tesac\n+\tdone\n+\n+\tif test $# -ne 3\n+\tthen\n+\t\terror \"invalid number of arguments\"\n+\tfi\n+\n+\tcmd=$1\n+\trepo=$2\n+\tmsg=$3\n+\n+\tif test -x \"$DAEMON_DOCUMENT_ROOT_PATH/$repo\"\n+\tthen\n+\t\tif test -n \"$do_export\"\n+\t\tthen\n+\t\t\t: >\"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\telse\n+\t\t\trm -f \"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\tfi\n+\tfi\n+\n+\ttest_must_fail git \"$cmd\" \"$DAEMON_URL/$repo\" 2>output &&\n+\techo \"fatal: remote error: $msg: /$repo\" >expect &&\n+\ttest_cmp expect output\n+\tret=$?\n+\tchmod +X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t(exit $ret)\n+}\n+\n+msg=\"access denied or repository not exported\"\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git '$msg'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git '$msg'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git '$msg'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git '$msg'\"\n+\n+stop_daemon\n+start_daemon --informative-errors\n+\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n+\n+stop_daemon\n+test_done\n-- \n1.7.7\n"},{"id":"177806","messageId":"1318803076-4229-2-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1318803076-4229-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/2] daemon: report permission denied error to clients","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-16T22:11:16Z","receivedAt":"2011-10-16T22:11:16Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If passed an inaccessible url, git daemon returns the\nfollowing error:\n\n $ git clone git://host/repo\n fatal: remote error: no such repository: /repo\n\nIn case of a permission denied error, return the following\ninstead:\n\n fatal: remote error: permission denied: /repo\n\nSigned-off-by: Clemens Buchacher \n---\n daemon.c | 32 +++++++++++++++++++++-----------\n path.c | 31 +++++++++++++++++++++----------\n t/t5570-git-daemon.sh | 2 +-\n 3 files changed, 43 insertions(+), 22 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 72fb53a..1442b5b 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -109,7 +109,7 @@ static void NORETURN daemon_die(const char *err, va_list params)\n \texit(1);\n }\n \n-static char *path_ok(char *directory)\n+static int path_ok(char *directory, const char **return_path)\n {\n \tstatic char rpath[PATH_MAX];\n \tstatic char interp_path[PATH_MAX];\n@@ -120,13 +120,13 @@ static char *path_ok(char *directory)\n \n \tif (daemon_avoid_alias(dir)) {\n \t\tlogerror(\"'%s': aliased\", dir);\n-\t\treturn NULL;\n+\t\treturn -1;\n \t}\n \n \tif (*dir == '~') {\n \t\tif (!user_path) {\n \t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n-\t\t\treturn NULL;\n+\t\t\treturn EACCES;\n \t\t}\n \t\tif (*user_path) {\n \t\t\t/* Got either \"~alice\" or \"~alice/foo\";\n@@ -158,7 +158,7 @@ static char *path_ok(char *directory)\n \t\tif (*dir != '/') {\n \t\t\t/* Allow only absolute */\n \t\t\tlogerror(\"'%s': Non-absolute path denied (interpolated-path active)\", dir);\n-\t\t\treturn NULL;\n+\t\t\treturn EACCES;\n \t\t}\n \n \t\tstrbuf_expand(&expanded_path, interpolated_path,\n@@ -173,7 +173,7 @@ static char *path_ok(char *directory)\n \t\tif (*dir != '/') {\n \t\t\t/* Allow only absolute */\n \t\t\tlogerror(\"'%s': Non-absolute path denied (base-path active)\", dir);\n-\t\t\treturn NULL;\n+\t\t\treturn EACCES;\n \t\t}\n \t\tsnprintf(rpath, PATH_MAX, \"%s%s\", base_path, dir);\n \t\tdir = rpath;\n@@ -190,10 +190,14 @@ static char *path_ok(char *directory)\n \t}\n \n \tif (!path) {\n+\t\tint ret = -1;\n+\t\tif (errno == EACCES)\n+\t\t ret = EACCES;\n \t\tlogerror(\"'%s' does not appear to be a git repository\", dir);\n-\t\treturn NULL;\n+\t\treturn ret;\n \t}\n \n+\t*return_path = path;\n \tif ( ok_paths && *ok_paths ) {\n \t\tchar **pp;\n \t\tint pathlen = strlen(path);\n@@ -211,17 +215,17 @@ static char *path_ok(char *directory)\n \t\t\t !memcmp(*pp, path, len) &&\n \t\t\t (path[len] == '\\0' ||\n \t\t\t (!strict_paths && path[len] == '/')))\n-\t\t\t\treturn path;\n+\t\t\t\treturn 0;\n \t\t}\n \t}\n \telse {\n \t\t/* be backwards compatible */\n \t\tif (!strict_paths)\n-\t\t\treturn path;\n+\t\t\treturn 0;\n \t}\n \n \tlogerror(\"'%s': not in whitelist\", path);\n-\treturn NULL;\t\t/* Fallthrough. Deny by default */\n+\treturn EACCES;\t\t/* Fallthrough. Deny by default */\n }\n \n typedef int (*daemon_service_fn)(void);\n@@ -258,6 +262,7 @@ static int daemon_error(const char *dir, const char *msg)\n \n static int run_service(char *dir, struct daemon_service *service)\n {\n+\tint err;\n \tconst char *path;\n \tint enabled = service->enabled;\n \n@@ -269,8 +274,13 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n-\tif (!(path = path_ok(dir)))\n-\t\treturn daemon_error(dir, \"no such repository\");\n+\terr = path_ok(dir, &path);\n+\tif (err) {\n+\t\tif (err == EACCES)\n+\t\t\treturn daemon_error(dir, \"permission denied\");\n+\t\telse\n+\t\t\treturn daemon_error(dir, \"no such repository\");\n+\t}\n \n \t/*\n \t * Security on the cheap.\ndiff --git a/path.c b/path.c\nindex 6f3f5d5..227d8d7 100644\n--- a/path.c\n+++ b/path.c\n@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)\n \tstatic char used_path[PATH_MAX];\n \tstatic char validated_path[PATH_MAX];\n \n+\terrno = 0;\n \tif (!path)\n \t\treturn NULL;\n \n@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)\n \t\t\tpath[len-1] = 0;\n \t\t\tlen--;\n \t\t}\n-\t\tif (PATH_MAX <= len)\n+\t\tif (PATH_MAX <= len) {\n+\t\t\terrno = ENAMETOOLONG;\n \t\t\treturn NULL;\n+\t\t}\n \t\tif (path[0] == '~') {\n \t\t\tchar *newpath = expand_user_path(path);\n \t\t\tif (!newpath || (PATH_MAX - 10 < strlen(newpath))) {\n \t\t\t\tfree(newpath);\n+\t\t\t\terrno = 0;\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\t/*\n@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)\n \t\t\tstrcpy(validated_path, path);\n \t\t\tpath = used_path;\n \t\t}\n-\t\telse if (PATH_MAX - 10 < len)\n+\t\telse if (PATH_MAX - 10 < len) {\n+\t\t\terrno = ENAMETOOLONG;\n \t\t\treturn NULL;\n-\t\telse {\n+\t\t} else {\n \t\t\tpath = strcpy(used_path, path);\n \t\t\tstrcpy(validated_path, path);\n \t\t}\n@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)\n \t\t\tif (!access(path, F_OK)) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n \t\t\t\tbreak;\n+\t\t\t} else if (errno == EACCES) {\n+\t\t\t\treturn NULL;\n \t\t\t}\n \t\t}\n-\t\tif (!suffix[i] || chdir(path))\n+\t\tif (!suffix[i])\n+\t\t\treturn NULL;\n+\t\tif (chdir(path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n \telse if (chdir(path))\n \t\treturn NULL;\n \n-\tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n-\t validate_headref(\"HEAD\") == 0) {\n-\t\tset_git_dir(\".\");\n-\t\tcheck_repository_format();\n-\t\treturn path;\n+\tif (access(\"objects\", X_OK) || access(\"refs\", X_OK))\n+\t\treturn NULL;\n+\tif (validate_headref(\"HEAD\")) {\n+\t\terrno = 0;\n+\t\treturn NULL;\n \t}\n \n-\treturn NULL;\n+\tset_git_dir(\".\");\n+\tcheck_repository_format();\n+\treturn path;\n }\n \n int set_shared_perm(const char *path, int mode)\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex aa5771a..e6482eb 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -141,7 +141,7 @@ start_daemon --informative-errors\n \n test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n-test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'permission denied'\"\n test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n \n stop_daemon\n-- \n1.7.7\n"},{"id":"177817","messageId":"20111017020103.GA18536@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1318803076-4229-1-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-17T02:01:03Z","receivedAt":"2011-10-17T02:01:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2011 at 12:11:15AM +0200, Clemens Buchacher wrote:\n\n> The semantics of the git daemon tests are similar to the http\n> transport tests. In fact, they are only a slightly modified copy\n> of t5550, plus the newly added remote error tests.\n> \n> All daemon tests will be skipped unless the environment variable\n> GIT_TEST_DAEMON is set.\n\nThanks, it's nice to have some tests. Overall, some of the tests feel a\nlittle silly, because the results should be exactly the same as fetching\nor pushing a local repository (so the \"set-head\" thing, for example,\nreally has little to do with git-daemon). At the same time, maybe it's a\ngood thing to re-confirm that the results really are the same. :)\n\n> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\n> new file mode 100644\n> index 0000000..30a89ea\n> --- /dev/null\n> +++ b/t/lib-daemon.sh\n> @@ -0,0 +1,52 @@\n> +#!/bin/sh\n> +\n> +if test -z \"$GIT_TEST_DAEMON\"\n> +then\n> +\tskip_all=\"Daemon testing disabled (define GIT_TEST_DAEMON to enable)\"\n> +\ttest_done\n> +fi\n> +\n> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n\nI assume you picked this arbitrarily to be LIB_HTTPD_PORT+10. It's fine\nto have a default, but note that each of the httpd tests actually\ndefaults the port to their test number, so they can be run in parallel.\n\nSo that would be:\n\n> --- /dev/null\n> +++ b/t/t5570-git-daemon.sh\n> @@ -0,0 +1,148 @@\n> +#!/bin/sh\n> +\n> +test_description='test fetching over git protocol'\n> +. ./test-lib.sh\n> +\n> +. \"$TEST_DIRECTORY\"/lib-daemon.sh\n> +start_daemon\n\nLIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}\n\nhere.\n"},{"id":"177818","messageId":"20111017020912.GB18536@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1318803076-4229-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-17T02:09:12Z","receivedAt":"2011-10-17T02:09:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:\n\n> If passed an inaccessible url, git daemon returns the\n> following error:\n> \n> $ git clone git://host/repo\n> fatal: remote error: no such repository: /repo\n> \n> In case of a permission denied error, return the following\n> instead:\n> \n> fatal: remote error: permission denied: /repo\n> \n> Signed-off-by: Clemens Buchacher \n> ---\n\nI like the intent. This actually does leak a little more information\nthan the existing --informative-errors, as before you couldn't tell the\ndifference between \"not found\" and \"not exported\". But I think the\nspirit of --informative-errors is to let that information leak, and this\nis a good change.\n\n> -static char *path_ok(char *directory)\n> +static int path_ok(char *directory, const char **return_path)\n> {\n> \tstatic char rpath[PATH_MAX];\n> \tstatic char interp_path[PATH_MAX];\n> @@ -120,13 +120,13 @@ static char *path_ok(char *directory)\n> \n> \tif (daemon_avoid_alias(dir)) {\n> \t\tlogerror(\"'%s': aliased\", dir);\n> -\t\treturn NULL;\n> +\t\treturn -1;\n> \t}\n> \n> \tif (*dir == '~') {\n> \t\tif (!user_path) {\n> \t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n> -\t\t\treturn NULL;\n> +\t\t\treturn EACCES;\n\nThe new calling conventions for this function seem a little weird. I\nwould expect either \"return negative, and set errno\" for usual library\ncode, or possibly \"return negative error value\". But \"return -1, or a\npositive error code\" seems unusual to me.\n\nOne of:\n\n errno = EACCESS;\n return -1;\n\nor\n\n return -EACCESS;\n\nwould be more idiomatic, I think.\n\n-Peff\n"},{"id":"177880","messageId":"20111017194821.GA29479@ecki","threadId":"28543","inReplyTo":"20111017020912.GB18536@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-17T19:48:21Z","receivedAt":"2011-10-17T19:48:21Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Oct 16, 2011 at 10:09:12PM -0400, Jeff King wrote:\n> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:\n> \n> > If passed an inaccessible url, git daemon returns the\n> > following error:\n> > \n> > $ git clone git://host/repo\n> > fatal: remote error: no such repository: /repo\n> > \n> > In case of a permission denied error, return the following\n> > instead:\n> > \n> > fatal: remote error: permission denied: /repo\n> > \n> > Signed-off-by: Clemens Buchacher \n> > ---\n> \n> I like the intent. This actually does leak a little more information\n> than the existing --informative-errors, as before you couldn't tell the\n> difference between \"not found\" and \"not exported\".\n\nI think you mean that before, you couldn't tell the difference\nbetween \"not found\" and \"permission denied\".\n\n> > -static char *path_ok(char *directory)\n> > +static int path_ok(char *directory, const char **return_path)\n> > {\n> > \tstatic char rpath[PATH_MAX];\n> > \tstatic char interp_path[PATH_MAX];\n> > @@ -120,13 +120,13 @@ static char *path_ok(char *directory)\n> > \n> > \tif (daemon_avoid_alias(dir)) {\n> > \t\tlogerror(\"'%s': aliased\", dir);\n> > -\t\treturn NULL;\n> > +\t\treturn -1;\n> > \t}\n> > \n> > \tif (*dir == '~') {\n> > \t\tif (!user_path) {\n> > \t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n> > -\t\t\treturn NULL;\n> > +\t\t\treturn EACCES;\n> \n> The new calling conventions for this function seem a little weird. I\n> would expect either \"return negative, and set errno\" for usual library\n> code, or possibly \"return negative error value\". But \"return -1, or a\n> positive error code\" seems unusual to me.\n\nYes indeed, will fix.\n\nClemens\n"},{"id":"177881","messageId":"20111017195154.GA23242@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111017194821.GA29479@ecki","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-17T19:51:54Z","receivedAt":"2011-10-17T19:51:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2011 at 09:48:21PM +0200, Clemens Buchacher wrote:\n\n> > I like the intent. This actually does leak a little more information\n> > than the existing --informative-errors, as before you couldn't tell the\n> > difference between \"not found\" and \"not exported\".\n> \n> I think you mean that before, you couldn't tell the difference\n> between \"not found\" and \"permission denied\".\n\nAh, right. Sorry, I was thinking path_ok handled the export-ok flag, but\nI already handled it in my patch to run_service. So it is leaking a\nlittle more, but even less than I indicated. And at any rate, I think it\nis consistent with what --informative-errors is meant to do, so it's a\ngood change.\n\n> > The new calling conventions for this function seem a little weird. I\n> > would expect either \"return negative, and set errno\" for usual library\n> > code, or possibly \"return negative error value\". But \"return -1, or a\n> > positive error code\" seems unusual to me.\n> \n> Yes indeed, will fix.\n\nThanks.\n\n-Peff\n"},{"id":"177882","messageId":"20111017195547.GB29479@ecki","threadId":"28543","inReplyTo":"20111017020103.GA18536@sigill.intra.peff.net","subject":"[PATCH] use test number as port number","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-17T19:55:47Z","receivedAt":"2011-10-17T19:55:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Test 5550 was apparently using the default port number by mistake.\n\nSigned-off-by: Clemens Buchacher \n---\n\nOn Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:\n> \n> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}\n\nThanks, I missed that.\n\nClemens\n\n t/t5550-http-fetch.sh | 2 +-\n t/t5570-git-daemon.sh | 1 +\n 2 files changed, 2 insertions(+), 1 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex a1883ca..8a77750 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -8,8 +8,8 @@ if test -n \"$NO_CURL\"; then\n \ttest_done\n fi\n \n-. \"$TEST_DIRECTORY\"/lib-httpd.sh\n LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}\n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n test_expect_success 'setup repository' '\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex e6482eb..a92d996 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -3,6 +3,7 @@\n test_description='test fetching over git protocol'\n . ./test-lib.sh\n \n+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}\n . \"$TEST_DIRECTORY\"/lib-daemon.sh\n start_daemon\n \n-- \n1.7.7\n"},{"id":"177883","messageId":"20111017195850.GC29479@ecki","threadId":"28543","inReplyTo":"1318803076-4229-2-git-send-email-drizzd@aon.at","subject":"[PATCH v2 2/2] daemon: report permission denied error to clients","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-17T19:58:51Z","receivedAt":"2011-10-17T19:58:51Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If passed an inaccessible url, git daemon returns the\nfollowing error:\n\n $ git clone git://host/repo\n fatal: remote error: no such repository: /repo\n\nIn case of a permission denied error, return the following\ninstead:\n\n fatal: remote error: permission denied: /repo\n\nSigned-off-by: Clemens Buchacher \n---\n\nCompared to v1 of this patch, the calling convention of path_ok are\nback to what they were previously. Now the only change is that it\nsets errno.\n\n daemon.c | 15 +++++++++++++--\n path.c | 31 +++++++++++++++++++++----------\n t/t5570-git-daemon.sh | 2 +-\n 3 files changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 72fb53a..2f7f84e 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -120,12 +120,14 @@ static char *path_ok(char *directory)\n \n \tif (daemon_avoid_alias(dir)) {\n \t\tlogerror(\"'%s': aliased\", dir);\n+\t\terrno = 0;\n \t\treturn NULL;\n \t}\n \n \tif (*dir == '~') {\n \t\tif (!user_path) {\n \t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n+\t\t\terrno = EACCES;\n \t\t\treturn NULL;\n \t\t}\n \t\tif (*user_path) {\n@@ -158,6 +160,7 @@ static char *path_ok(char *directory)\n \t\tif (*dir != '/') {\n \t\t\t/* Allow only absolute */\n \t\t\tlogerror(\"'%s': Non-absolute path denied (interpolated-path active)\", dir);\n+\t\t\terrno = EACCES;\n \t\t\treturn NULL;\n \t\t}\n \n@@ -173,6 +176,7 @@ static char *path_ok(char *directory)\n \t\tif (*dir != '/') {\n \t\t\t/* Allow only absolute */\n \t\t\tlogerror(\"'%s': Non-absolute path denied (base-path active)\", dir);\n+\t\t\terrno = EACCES;\n \t\t\treturn NULL;\n \t\t}\n \t\tsnprintf(rpath, PATH_MAX, \"%s%s\", base_path, dir);\n@@ -190,7 +194,9 @@ static char *path_ok(char *directory)\n \t}\n \n \tif (!path) {\n+\t\tint err = errno;\n \t\tlogerror(\"'%s' does not appear to be a git repository\", dir);\n+\t\terrno = err;\n \t\treturn NULL;\n \t}\n \n@@ -221,6 +227,7 @@ static char *path_ok(char *directory)\n \t}\n \n \tlogerror(\"'%s': not in whitelist\", path);\n+\terrno = EACCES;\n \treturn NULL;\t\t/* Fallthrough. Deny by default */\n }\n \n@@ -269,8 +276,12 @@ static int run_service(char *dir, struct daemon_service *service)\n \t\treturn daemon_error(dir, \"service not enabled\");\n \t}\n \n-\tif (!(path = path_ok(dir)))\n-\t\treturn daemon_error(dir, \"no such repository\");\n+\tif (!(path = path_ok(dir))) {\n+\t\tif (errno == EACCES)\n+\t\t\treturn daemon_error(dir, \"permission denied\");\n+\t\telse\n+\t\t\treturn daemon_error(dir, \"no such repository\");\n+\t}\n \n \t/*\n \t * Security on the cheap.\ndiff --git a/path.c b/path.c\nindex 6f3f5d5..227d8d7 100644\n--- a/path.c\n+++ b/path.c\n@@ -288,6 +288,7 @@ char *enter_repo(char *path, int strict)\n \tstatic char used_path[PATH_MAX];\n \tstatic char validated_path[PATH_MAX];\n \n+\terrno = 0;\n \tif (!path)\n \t\treturn NULL;\n \n@@ -301,12 +302,15 @@ char *enter_repo(char *path, int strict)\n \t\t\tpath[len-1] = 0;\n \t\t\tlen--;\n \t\t}\n-\t\tif (PATH_MAX <= len)\n+\t\tif (PATH_MAX <= len) {\n+\t\t\terrno = ENAMETOOLONG;\n \t\t\treturn NULL;\n+\t\t}\n \t\tif (path[0] == '~') {\n \t\t\tchar *newpath = expand_user_path(path);\n \t\t\tif (!newpath || (PATH_MAX - 10 < strlen(newpath))) {\n \t\t\t\tfree(newpath);\n+\t\t\t\terrno = 0;\n \t\t\t\treturn NULL;\n \t\t\t}\n \t\t\t/*\n@@ -319,9 +323,10 @@ char *enter_repo(char *path, int strict)\n \t\t\tstrcpy(validated_path, path);\n \t\t\tpath = used_path;\n \t\t}\n-\t\telse if (PATH_MAX - 10 < len)\n+\t\telse if (PATH_MAX - 10 < len) {\n+\t\t\terrno = ENAMETOOLONG;\n \t\t\treturn NULL;\n-\t\telse {\n+\t\t} else {\n \t\t\tpath = strcpy(used_path, path);\n \t\t\tstrcpy(validated_path, path);\n \t\t}\n@@ -331,23 +336,29 @@ char *enter_repo(char *path, int strict)\n \t\t\tif (!access(path, F_OK)) {\n \t\t\t\tstrcat(validated_path, suffix[i]);\n \t\t\t\tbreak;\n+\t\t\t} else if (errno == EACCES) {\n+\t\t\t\treturn NULL;\n \t\t\t}\n \t\t}\n-\t\tif (!suffix[i] || chdir(path))\n+\t\tif (!suffix[i])\n+\t\t\treturn NULL;\n+\t\tif (chdir(path))\n \t\t\treturn NULL;\n \t\tpath = validated_path;\n \t}\n \telse if (chdir(path))\n \t\treturn NULL;\n \n-\tif (access(\"objects\", X_OK) == 0 && access(\"refs\", X_OK) == 0 &&\n-\t validate_headref(\"HEAD\") == 0) {\n-\t\tset_git_dir(\".\");\n-\t\tcheck_repository_format();\n-\t\treturn path;\n+\tif (access(\"objects\", X_OK) || access(\"refs\", X_OK))\n+\t\treturn NULL;\n+\tif (validate_headref(\"HEAD\")) {\n+\t\terrno = 0;\n+\t\treturn NULL;\n \t}\n \n-\treturn NULL;\n+\tset_git_dir(\".\");\n+\tcheck_repository_format();\n+\treturn path;\n }\n \n int set_shared_perm(const char *path, int mode)\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex aa5771a..e6482eb 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -141,7 +141,7 @@ start_daemon --informative-errors\n \n test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n-test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'permission denied'\"\n test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n \n stop_daemon\n-- \n1.7.7\n"},{"id":"177884","messageId":"20111017200528.GA19054@ecki","threadId":"28543","inReplyTo":"20111017020103.GA18536@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-17T20:05:28Z","receivedAt":"2011-10-17T20:05:28Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:\n> \n> Thanks, it's nice to have some tests. Overall, some of the tests feel a\n> little silly, because the results should be exactly the same as fetching\n> or pushing a local repository (so the \"set-head\" thing, for example,\n> really has little to do with git-daemon).\n\nHmm, yes. Actually, I thought I had found a bug with the failure of\n\"set-head -a\". But now I see that in t5505 this treated like a\nfeature.\n\nWould it be difficult to support this over the git protocol? Maybe\nI will have a look.\n\nClemens\n"},{"id":"177885","messageId":"20111017200809.GA23964@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20111017200528.GA19054@ecki","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-10-17T20:08:09Z","receivedAt":"2011-10-17T20:08:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 17, 2011 at 10:05:28PM +0200, Clemens Buchacher wrote:\n\n> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:\n> > \n> > Thanks, it's nice to have some tests. Overall, some of the tests feel a\n> > little silly, because the results should be exactly the same as fetching\n> > or pushing a local repository (so the \"set-head\" thing, for example,\n> > really has little to do with git-daemon).\n> \n> Hmm, yes. Actually, I thought I had found a bug with the failure of\n> \"set-head -a\". But now I see that in t5505 this treated like a\n> feature.\n\nIt's not a feature, exactly. It's just documenting that we fail in the\nface of ambiguous HEADs. Arguably, the test should be switched to use\ntext_expect_failure to document that we would prefer it the other way,\nbut it doesn't work now.\n\n> Would it be difficult to support this over the git protocol? Maybe\n> I will have a look.\n\nIt needs a protocol extension to communicate symbolic ref destinations.\nThe topic has come up a few times, and I think Junio even had patches at\none point.\n\n-Peff\n"},{"id":"177889","messageId":"7vobxfl4kj.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111017195547.GB29479@ecki","subject":"Re: [PATCH] use test number as port number","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T20:57:00Z","receivedAt":"2011-10-17T20:57:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Test 5550 was apparently using the default port number by mistake.\n>\n> Signed-off-by: Clemens Buchacher \n> ---\n>\n> On Sun, Oct 16, 2011 at 10:01:03PM -0400, Jeff King wrote:\n>> \n>> LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}\n>\n> Thanks, I missed that.\n>\n> Clemens\n>\n> t/t5550-http-fetch.sh | 2 +-\n> t/t5570-git-daemon.sh | 1 +\n> 2 files changed, 2 insertions(+), 1 deletions(-)\n>\n> diff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\n> index a1883ca..8a77750 100755\n> --- a/t/t5550-http-fetch.sh\n> +++ b/t/t5550-http-fetch.sh\n> @@ -8,8 +8,8 @@ if test -n \"$NO_CURL\"; then\n> \ttest_done\n> fi\n> \n> -. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5550'}\n> +. \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n\nGood eyes. This is the only one in the 55xx series that gets the order\nwrong.\n\nI'll drop the patch to 5570 for now as that should be done in the change\nthat is still not in 'next' that adds 5570.\n\nI've fixed and queued the previous one as aa0b028 (daemon: add tests,\n2011-10-17); does that look good enough?\n\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> index e6482eb..a92d996 100755\n> --- a/t/t5570-git-daemon.sh\n> +++ b/t/t5570-git-daemon.sh\n> @@ -3,6 +3,7 @@\n> test_description='test fetching over git protocol'\n> . ./test-lib.sh\n> \n> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'5570'}\n> . \"$TEST_DIRECTORY\"/lib-daemon.sh\n> start_daemon\n"},{"id":"177890","messageId":"7vhb37l4ag.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111017020912.GB18536@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T21:03:03Z","receivedAt":"2011-10-17T21:03:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Oct 17, 2011 at 12:11:16AM +0200, Clemens Buchacher wrote:\n>\n> I like the intent. This actually does leak a little more information\n> than the existing --informative-errors, as before you couldn't tell the\n> difference between \"not found\" and \"not exported\".\n\nI personally think this is going a bit too far, even for \"informative\"\noption, by allowing to fish for possible list of usernames. It would make\nit a tougher sell to later default to \"informative\", I am afraid.\n\nSuppose you are setting up your own repository (either on your own box or\non a box with a separate administrator), and youare wondering why your\nattempted access failed. You know /pub/repo/sito.git exists (you created\nit after all) and you get \"no such repository: /repo/sito.git\" when you\nran:\n\n $ git clone git://host/repo/sito.git/\n\nIf you have another repository in /pub/repo/ that does already work, and\nif you know /pub/repo/sito.git/ is fine locally (e.g. you can see local\ncommand like \"git log\" works fine there), then even if you see \"not found\"\nyou would know to compare what the difference between these two are.\n\nIf there is no other repositories in /pub/repo/ or you are setting up many\nrepositories on this same box for the first time, wouldn't it be plausible\nthat you _are_ the administrator of the box and have access to the daemon\nlog to diagnose the problem more easily anyway?\n\nI can see how this is \"leaking a little more information\", but I am not\nconvinced that leak is helping legit users more than helping unwanted\nsnoopers.\n\n> The new calling conventions for this function seem a little weird. I\n> would expect either \"return negative, and set errno\" for usual library\n> code, or possibly \"return negative error value\". But \"return -1, or a\n> positive error code\" seems unusual to me.\n>\n> One of:\n>\n> errno = EACCESS;\n> return -1;\n>\n> or\n>\n> return -EACCESS;\n>\n> would be more idiomatic, I think.\n\nYes, the former would probably be easier to handle.\n"},{"id":"177967","messageId":"20111018200959.GA2072@ecki","threadId":"28543","inReplyTo":"7vobxfl4kj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] use test number as port number","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-18T20:09:59Z","receivedAt":"2011-10-18T20:09:59Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Oct 17, 2011 at 01:57:00PM -0700, Junio C Hamano wrote:\n> \n> I've fixed and queued the previous one as aa0b028 (daemon: add tests,\n> 2011-10-17); does that look good enough?\n\nYep, perfect. Thanks.\n"},{"id":"177968","messageId":"20111018204101.GB2072@ecki","threadId":"28543","inReplyTo":"7vhb37l4ag.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-18T20:41:01Z","receivedAt":"2011-10-18T20:41:01Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Oct 17, 2011 at 02:03:03PM -0700, Junio C Hamano wrote:\n> \n> I personally think this is going a bit too far, even for \"informative\"\n> option, by allowing to fish for possible list of usernames. It would make\n> it a tougher sell to later default to \"informative\", I am afraid.\n\nI guess if permission is denied for access over git://, then nobody\ncan use the repository. So it's clearly a server side issue.\n\nThis change probably makes more sense for local access and over\nssh. I already have a similar patch brewing for that.\n\nClemens\n"},{"id":"177986","messageId":"ec721333-9b89-40ab-9550-851350507914@email.android.com","threadId":"28543","inReplyTo":"20111018204101.GB2072@ecki","subject":"Re: [PATCH 2/2] daemon: report permission denied error to clients","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2011-10-19T06:33:53Z","receivedAt":"2011-10-19T06:33:53Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Clemens Buchacher wrote:\n>\n> I guess if permission is denied for access over git://, then nobody\n> can use the repository. So it's clearly a server side issue.\n> \n> This change probably makes more sense for local access and over\n> ssh. I already have a similar patch brewing for that.\n\nAs far as security is concerned, we have to treat ssh the same as git://, unless the user has permission to execute arbitrary commands and not just git-upload-pack. But I can think of no way to figure that out on the server side.\n"},{"id":"178143","messageId":"7vaa8u87vm.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20111017195850.GC29479@ecki","subject":"Re: [PATCH v2 2/2] daemon: report permission denied error to clients","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-21T19:25:17Z","receivedAt":"2011-10-21T19:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> diff --git a/daemon.c b/daemon.c\n> index 72fb53a..2f7f84e 100644\n> --- a/daemon.c\n> +++ b/daemon.c\n> @@ -120,12 +120,14 @@ static char *path_ok(char *directory)\n> \n> \tif (daemon_avoid_alias(dir)) {\n> \t\tlogerror(\"'%s': aliased\", dir);\n> +\t\terrno = 0;\n> \t\treturn NULL;\n> \t}\n> \n> \tif (*dir == '~') {\n> \t\tif (!user_path) {\n> \t\t\tlogerror(\"'%s': User-path not allowed\", dir);\n> +\t\t\terrno = EACCES;\n> \t\t\treturn NULL;\n> \t\t}\n\nIsn't the first one inconsistent from all the others?\n\nA request cames \"../some/path\" and it is not allowed by a daemon policy\nand it gets errno==0 which is turned into \"no such repo\" later, while\nanother request to \"~drizzed/another/path\" is also rejected by a daemon\npolicy and gets errno==EACCESS which is turned into \"permission denied\".\n\nIndeed everything else says EACCESS in this patch, except for the check\ndone by enter_repo() which can additionally say ENAMETOOLONG (which would\nnot be very useful in practice) or whatever error coming from failure to\ngo there with chdir(), which is not likely to be EACCESS because it has\nalready been checked with a separate access() that is done before the\nactual chdir() call.\n\n> +\tif (!(path = path_ok(dir))) {\n> +\t\tif (errno == EACCES)\n> +\t\t\treturn daemon_error(dir, \"permission denied\");\n> +\t\telse\n> +\t\t\treturn daemon_error(dir, \"no such repository\");\n> +\t}\n\nIf errno is set to EACCESS in cases (1) we are not even going to tell you\nif a repository exists there or not--you are not authorized to know and\n(2) there is a repository but you do not have authorization to access it,\nthen this \"leaking a bit more information\" part is acceptable for site\nwith \"--informative-errors\", I would think. A repository that is invalid\nfrom the daemon's point of view (e.g. validate_headref(\"HEAD\") fails\nbecause it points at an object that does not exist) but that the owner\nintended to make it valid by correcting such mistakes would be reported as\n\"no such repository\" with such a logic, so I am not sure if the distinction\nbetween these two cases really matters in practice, though.\n"},{"id":"181837","messageId":"20120102092508.GA10977@elie.hsd1.il.comcast.net","threadId":"28543","inReplyTo":"1318803076-4229-1-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-01-02T09:25:08Z","receivedAt":"2012-01-02T09:25:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: Erik, Ilari, Duy)\nHi,\n\nClemens Buchacher wrote:\n\n> [Subject: daemon: add tests]\n\nCan't believe I missed this. That seems like a worthy cause ---\ncan someone remind me why this is dropped, or if there are any\ntweaks I can help with to get it picked up again?\n\nPatch left unsnipped for convenience of people cc-ed.\n\nJonathan\n\n> The semantics of the git daemon tests are similar to the http\n> transport tests. In fact, they are only a slightly modified copy\n> of t5550, plus the newly added remote error tests.\n>\n> All daemon tests will be skipped unless the environment variable\n> GIT_TEST_DAEMON is set.\n> \n> Signed-off-by: Clemens Buchacher \n> ---\n> \n> This patch is based on jk/daemon-msgs.\n> \n> t/lib-daemon.sh | 52 +++++++++++++++++\n> t/t5570-git-daemon.sh | 148 +++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 200 insertions(+), 0 deletions(-)\n> create mode 100644 t/lib-daemon.sh\n> create mode 100755 t/t5570-git-daemon.sh\n> \n> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\n> new file mode 100644\n> index 0000000..30a89ea\n> --- /dev/null\n> +++ b/t/lib-daemon.sh\n> @@ -0,0 +1,52 @@\n> +#!/bin/sh\n> +\n> +if test -z \"$GIT_TEST_DAEMON\"\n> +then\n> +\tskip_all=\"Daemon testing disabled (define GIT_TEST_DAEMON to enable)\"\n> +\ttest_done\n> +fi\n> +\n> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n> +\n> +DAEMON_PID=\n> +DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n> +DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT\n> +\n> +start_daemon() {\n> +\tif test -n \"$DAEMON_PID\"\n> +\tthen\n> +\t\terror \"start_daemon already called\"\n> +\tfi\n> +\n> +\tmkdir -p \"$DAEMON_DOCUMENT_ROOT_PATH\"\n> +\n> +\ttrap 'code=$?; stop_daemon; (exit $code); die' EXIT\n> +\n> +\tsay >&3 \"Starting git daemon ...\"\n> +\tgit daemon --listen=127.0.0.1 --port=\"$LIB_DAEMON_PORT\" \\\n> +\t\t--reuseaddr --verbose \\\n> +\t\t--base-path=\"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n> +\t\t\"$@\" \"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n> +\t\t>&3 2>&4 &\n> +\tDAEMON_PID=$!\n> +}\n> +\n> +stop_daemon() {\n> +\tif test -z \"$DAEMON_PID\"\n> +\tthen\n> +\t\treturn\n> +\tfi\n> +\n> +\ttrap 'die' EXIT\n> +\n> +\t# kill git-daemon child of git\n> +\tsay >&3 \"Stopping git daemon ...\"\n> +\tpkill -P \"$DAEMON_PID\"\n> +\twait \"$DAEMON_PID\"\n> +\tret=$?\n> +\tif test $ret -ne 143\n> +\tthen\n> +\t\terror \"git daemon exited with status: $ret\"\n> +\tfi\n> +\tDAEMON_PID=\n> +}\n> diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\n> new file mode 100755\n> index 0000000..aa5771a\n> --- /dev/null\n> +++ b/t/t5570-git-daemon.sh\n> @@ -0,0 +1,148 @@\n> +#!/bin/sh\n> +\n> +test_description='test fetching over git protocol'\n> +. ./test-lib.sh\n> +\n> +. \"$TEST_DIRECTORY\"/lib-daemon.sh\n> +start_daemon\n> +\n> +test_expect_success 'setup repository' '\n> +\techo content >file &&\n> +\tgit add file &&\n> +\tgit commit -m one\n> +'\n> +\n> +test_expect_success 'create git-accessible bare repository' '\n> +\tmkdir \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\t git --bare init &&\n> +\t : >git-daemon-export-ok\n> +\t) &&\n> +\tgit remote add public \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n> +\tgit push public master:master\n> +'\n> +\n> +test_expect_success 'clone git repository' '\n> +\tgit clone $DAEMON_URL/repo.git clone &&\n> +\ttest_cmp file clone/file\n> +'\n> +\n> +test_expect_success 'fetch changes via git protocol' '\n> +\techo content >>file &&\n> +\tgit commit -a -m two &&\n> +\tgit push public &&\n> +\t(cd clone && git pull) &&\n> +\ttest_cmp file clone/file\n> +'\n> +\n> +test_expect_failure 'remote detects correct HEAD' '\n> +\tgit push public master:other &&\n> +\t(cd clone &&\n> +\t git remote set-head -d origin &&\n> +\t git remote set-head -a origin &&\n> +\t git symbolic-ref refs/remotes/origin/HEAD > output &&\n> +\t echo refs/remotes/origin/master > expect &&\n> +\t test_cmp expect output\n> +\t)\n> +'\n> +\n> +test_expect_success 'prepare pack objects' '\n> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n> +\t git --bare repack &&\n> +\t git --bare prune-packed\n> +\t)\n> +'\n> +\n> +test_expect_success 'fetch notices corrupt pack' '\n> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n> +\t p=`ls objects/pack/pack-*.pack` &&\n> +\t chmod u+w $p &&\n> +\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n> +\t) &&\n> +\tmkdir repo_bad1.git &&\n> +\t(cd repo_bad1.git &&\n> +\t git --bare init &&\n> +\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&\n> +\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n> +\t)\n> +'\n> +\n> +test_expect_success 'fetch notices corrupt idx' '\n> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n> +\t p=`ls objects/pack/pack-*.idx` &&\n> +\t chmod u+w $p &&\n> +\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n> +\t) &&\n> +\tmkdir repo_bad2.git &&\n> +\t(cd repo_bad2.git &&\n> +\t git --bare init &&\n> +\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&\n> +\t test 0 = `ls objects/pack | wc -l`\n> +\t)\n> +'\n> +\n> +test_remote_error()\n> +{\n> +\tdo_export=YesPlease\n> +\twhile test $# -gt 0\n> +\tdo\n> +\t\tcase $1 in\n> +\t\t-x)\n> +\t\t\tshift\n> +\t\t\tchmod -X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n> +\t\t\t;;\n> +\t\t-n)\n> +\t\t\tshift\n> +\t\t\tdo_export=\n> +\t\t\t;;\n> +\t\t*)\n> +\t\t\tbreak\n> +\t\tesac\n> +\tdone\n> +\n> +\tif test $# -ne 3\n> +\tthen\n> +\t\terror \"invalid number of arguments\"\n> +\tfi\n> +\n> +\tcmd=$1\n> +\trepo=$2\n> +\tmsg=$3\n> +\n> +\tif test -x \"$DAEMON_DOCUMENT_ROOT_PATH/$repo\"\n> +\tthen\n> +\t\tif test -n \"$do_export\"\n> +\t\tthen\n> +\t\t\t: >\"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n> +\t\telse\n> +\t\t\trm -f \"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n> +\t\tfi\n> +\tfi\n> +\n> +\ttest_must_fail git \"$cmd\" \"$DAEMON_URL/$repo\" 2>output &&\n> +\techo \"fatal: remote error: $msg: /$repo\" >expect &&\n> +\ttest_cmp expect output\n> +\tret=$?\n> +\tchmod +X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n> +\t(exit $ret)\n> +}\n> +\n> +msg=\"access denied or repository not exported\"\n> +test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git '$msg'\"\n> +test_expect_success 'push disabled' \"test_remote_error push repo.git '$msg'\"\n> +test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git '$msg'\"\n> +test_expect_success 'not exported' \"test_remote_error -n fetch repo.git '$msg'\"\n> +\n> +stop_daemon\n> +start_daemon --informative-errors\n> +\n> +test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n> +test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n> +test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n> +test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n> +\n> +stop_daemon\n> +test_done\n> -- \n> 1.7.7\n> \n> \n"},{"id":"181841","messageId":"20120102194711.GA25296@ecki.lan","threadId":"28543","inReplyTo":"20120102092508.GA10977@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-02T19:47:11Z","receivedAt":"2012-01-02T19:47:11Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Jan 02, 2012 at 03:25:08AM -0600, Jonathan Nieder wrote:\n> \n> > [Subject: daemon: add tests]\n> \n> Can't believe I missed this. That seems like a worthy cause ---\n> can someone remind me why this is dropped, or if there are any\n> tweaks I can help with to get it picked up again?\n\nWe were discussing some open issues with patch 2/2, which was based\non the tests. I later abandoned the idea for that patch. But the\ntests should be ok by themselves.\n\nClemens\n"},{"id":"181875","messageId":"20120103191855.GD20926@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120102194711.GA25296@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-03T19:18:55Z","receivedAt":"2012-01-03T19:18:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 02, 2012 at 08:47:11PM +0100, Clemens Buchacher wrote:\n\n> On Mon, Jan 02, 2012 at 03:25:08AM -0600, Jonathan Nieder wrote:\n> > \n> > > [Subject: daemon: add tests]\n> > \n> > Can't believe I missed this. That seems like a worthy cause ---\n> > can someone remind me why this is dropped, or if there are any\n> > tweaks I can help with to get it picked up again?\n> \n> We were discussing some open issues with patch 2/2, which was based\n> on the tests. I later abandoned the idea for that patch. But the\n> tests should be ok by themselves.\n\nYes, I'd like to see them included, even without the second patch. We\ncurrently have zero tests for git-daemon, so even just verifying that\nit starts and can let people fetch is an improvement.\n\n-Peff\n"},{"id":"181877","messageId":"7v8vlovavj.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20120102092508.GA10977@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-03T19:34:08Z","receivedAt":"2012-01-03T19:34:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> (+cc: Erik, Ilari, Duy)\n> Hi,\n>\n> Clemens Buchacher wrote:\n>\n>> [Subject: daemon: add tests]\n>\n> Can't believe I missed this. That seems like a worthy cause ---\n> can someone remind me why this is dropped, or if there are any\n> tweaks I can help with to get it picked up again?\n\nThanks for your interest in this.\n\n>> diff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\n>> new file mode 100644\n>> index 0000000..30a89ea\n>> --- /dev/null\n>> +++ b/t/lib-daemon.sh\n>> @@ -0,0 +1,52 @@\n>> +#!/bin/sh\n>> +\n>> +if test -z \"$GIT_TEST_DAEMON\"\n>> +then\n>> +\tskip_all=\"Daemon testing disabled (define GIT_TEST_DAEMON to enable)\"\n>> +\ttest_done\n>> +fi\n>> +\n>> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n\nIn lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is\nalways overridden by the users and the convention there is to use the test\nnumbers (cf. \"git grep LIB_HTTPD_PORT t/\"), which should be followed here\nas well.\n\nI am not very keen on the \"lib-daemon.sh\", GIT_TEST_DAEMON, etc. naming to\npretend as if \"git daemon\" will forever be the only daemon we will ever\nship, by the way. We might one day want to add an inotify daemon, a\ndaemon for the git-pubsub protocol or somesuch.\n\n>> +DAEMON_PID=\n>> +DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n>> +DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT\n>> +\n>> +start_daemon() {\n>> +\tif test -n \"$DAEMON_PID\"\n>> +\tthen\n>> +\t\terror \"start_daemon already called\"\n>> +\tfi\n>> +\n>> +\tmkdir -p \"$DAEMON_DOCUMENT_ROOT_PATH\"\n>> +\n>> +\ttrap 'code=$?; stop_daemon; (exit $code); die' EXIT\n>> +\n>> +\tsay >&3 \"Starting git daemon ...\"\n>> +\tgit daemon --listen=127.0.0.1 --port=\"$LIB_DAEMON_PORT\" \\\n>> +\t\t--reuseaddr --verbose \\\n>> +\t\t--base-path=\"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n>> +\t\t\"$@\" \"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n>> +\t\t>&3 2>&4 &\n>> +\tDAEMON_PID=$!\n>> +}\n>> +\n>> +stop_daemon() {\n>> +\tif test -z \"$DAEMON_PID\"\n>> +\tthen\n>> +\t\treturn\n>> +\tfi\n>> +\n>> +\ttrap 'die' EXIT\n>> +\n>> +\t# kill git-daemon child of git\n>> +\tsay >&3 \"Stopping git daemon ...\"\n>> +\tpkill -P \"$DAEMON_PID\"\n\nHow portable is this one (I usually do not trust use of pkill anywhere)?\n\n>> +\twait \"$DAEMON_PID\"\n>> +\tret=$?\n\t# Please comment what 143 is on this line.\n>> +\tif test $ret -ne 143\n>> +\tthen\n>> +\t\terror \"git daemon exited with status: $ret\"\n>> +\tfi\n>> +\tDAEMON_PID=\n>> +}\n>> ...\n>> +test_expect_success 'prepare pack objects' '\n>> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n>> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n>> +\t git --bare repack &&\n\nAs the later tests assume there will be only one pack, don't you want at\nleast \"-a\" and possibly \"-a -d\" here?\n\n>> +\t git --bare prune-packed\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'fetch notices corrupt pack' '\n>> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n>> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n>> +\t p=`ls objects/pack/pack-*.pack` &&\n>> +\t chmod u+w $p &&\n>> +\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n>> +\t) &&\n>> +\tmkdir repo_bad1.git &&\n>> +\t(cd repo_bad1.git &&\n>> +\t git --bare init &&\n>> +\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&\n>> +\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n>> +\t)\n>> +'\n>> +\n>> +test_expect_success 'fetch notices corrupt idx' '\n>> +\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n>> +\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n>> +\t p=`ls objects/pack/pack-*.idx` &&\n>> +\t chmod u+w $p &&\n>> +\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n>> +\t) &&\n>> +\tmkdir repo_bad2.git &&\n>> +\t(cd repo_bad2.git &&\n>> +\t git --bare init &&\n>> +\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&\n>> +\t test 0 = `ls objects/pack | wc -l`\n>> +\t)\n>> +'\n>> +\n>> +test_remote_error()\n>> +{\n>> +\tdo_export=YesPlease\n>> +\twhile test $# -gt 0\n>> +\tdo\n>> +\t\tcase $1 in\n>> +\t\t-x)\n>> +\t\t\tshift\n>> +\t\t\tchmod -X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n\nI find the use of cap X here dubious; it makes your intention unclear.\n\nAre you interested in the current status of 'x' bits on that directory, or\nare you more interested in dropping the executable/searchable bits from\nthe directory no matter what its current status is (rhetorical: I fully\nexpect that the answer is the latter)? The same comment applies to the use\nof \"chmod +X\" at the end of this helper function.\n"},{"id":"181922","messageId":"1325692539-26748-1-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"7v8vlovavj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:33Z","receivedAt":"2012-01-04T15:55:33Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Thanks for your review. Please find fixes in reply to this email. In\norder to better show individual changes I have not squashed them into\none commit. For upstream, you will probably want to squash patches 3-6\ninto patch 2. Patch 2 is the same as the one once queued as part of\ncb/daemon-permission-errors.\n\n[PATCH 1/6] t5550: repack everything into one file\n[PATCH 2/6] daemon: add tests\n[PATCH 3/6] avoid use of pkill\n[PATCH 4/6] explain expected exit code\n[PATCH 5/6] t5570: everything into one file\n[PATCH 6/6] chmod: use lower-case x\n\nOn Tue, Jan 03, 2012 at 11:34:08AM -0800, Junio C Hamano wrote:\n> >> +\n> >> +LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n> \n> In lib-httpd.sh, LIB_HTTPD_PORT is defined in a similar way, but that is\n> always overridden by the users and the convention there is to use the test\n> numbers (cf. \"git grep LIB_HTTPD_PORT t/\"), which should be followed here\n> as well.\n\nThis you already fixed in the version previously queued and is contained\nin [PATCH 2/6] daemon: add tests.\n\n> I am not very keen on the \"lib-daemon.sh\", GIT_TEST_DAEMON, etc. naming to\n> pretend as if \"git daemon\" will forever be the only daemon we will ever\n> ship, by the way. We might one day want to add an inotify daemon, a\n> daemon for the git-pubsub protocol or somesuch.\n\nAre you saying that the name \"daemon\" is too general, and it should\ninstead be \"lib-git-daemon.sh\" and GIT_TEST_GIT_DAEMON? Or do you\nmean that it is not general enough and it should be called\nlib-networking.sh and \"GIT_TEST_NETWORKING\"?\n\nEither way, I have no preference here. Feel free to change any way you\nlike.\n\n> >> +\t# kill git-daemon child of git\n> >> +\tsay >&3 \"Stopping git daemon ...\"\n> >> +\tpkill -P \"$DAEMON_PID\"\n> \n> How portable is this one (I usually do not trust use of pkill anywhere)?\n\nI read that it is supposed to be more portable than skill or killall.\nBut I have no way to research this. I have implemented a workaround\nusing only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n\n> >> + wait \"$DAEMON_PID\"\n> >> + ret=$?\n> # Please comment what 143 is on this line.\n> >> + if test $ret -ne 143\n\nFixed in [PATCH 4/6] explain expected exit code.\n\n> >> +\t git --bare repack &&\n> \n> As the later tests assume there will be only one pack, don't you want at\n> least \"-a\" and possibly \"-a -d\" here?\n\nFixed in\n\n [PATCH 1/6] t5550: repack everything into one file,\n [PATCH 5/6] t5570: repack everything into one file.\n\n> I find the use of cap X here dubious; it makes your intention unclear.\n> \n> Are you interested in the current status of 'x' bits on that directory, or\n> are you more interested in dropping the executable/searchable bits from\n> the directory no matter what its current status is (rhetorical: I fully\n> expect that the answer is the latter)?\n\nFor directories, upper-case X does not have that meaning. The status is\nalways overwritten, irrespective of the current status. I wanted to\nemphasize the fact that I am changing 'searchable' bits. But since that\ndoes not seem to have the desired effect, I changed it to lower-case in\n[PATCH 6/6] chmod: use lower-case x.\n\nClemens\n"},{"id":"181925","messageId":"1325692539-26748-2-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/6] t5550: repack everything into one file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:34Z","receivedAt":"2012-01-04T15:55:34Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Subsequently we assume that there is only one pack. Currently this is\ntrue only by accident. Pass '-a -d' to repack in order to guarantee that\nassumption to hold true.\n\nThe prune-packed command is now redundant since repack -d already calls\nit.\n---\n t/t5550-http-fetch.sh | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5550-http-fetch.sh b/t/t5550-http-fetch.sh\nindex 311a33c..7926ab3 100755\n--- a/t/t5550-http-fetch.sh\n+++ b/t/t5550-http-fetch.sh\n@@ -118,8 +118,7 @@ test_expect_success 'http remote detects correct HEAD' '\n test_expect_success 'fetch packed objects' '\n \tcp -R \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo.git \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n \t(cd \"$HTTPD_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\t git --bare repack &&\n-\t git --bare prune-packed\n+\t git --bare repack -a -d\n \t) &&\n \tgit clone $HTTPD_URL/dumb/repo_pack.git\n '\n-- \n1.7.8\n"},{"id":"181927","messageId":"1325692539-26748-3-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/6] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:35Z","receivedAt":"2012-01-04T15:55:35Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The semantics of the git daemon tests are similar to the http\ntransport tests. In fact, they are only a slightly modified copy\nof t5550, plus the newly added remote error tests.\n\nAll daemon tests will be skipped unless the environment variable\nGIT_TEST_DAEMON is set.\n\nSigned-off-by: Clemens Buchacher \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/lib-daemon.sh | 52 +++++++++++++++++\n t/t5570-git-daemon.sh | 149 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 201 insertions(+), 0 deletions(-)\n create mode 100644 t/lib-daemon.sh\n create mode 100755 t/t5570-git-daemon.sh\n\ndiff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\nnew file mode 100644\nindex 0000000..30a89ea\n--- /dev/null\n+++ b/t/lib-daemon.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+if test -z \"$GIT_TEST_DAEMON\"\n+then\n+\tskip_all=\"Daemon testing disabled (define GIT_TEST_DAEMON to enable)\"\n+\ttest_done\n+fi\n+\n+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-'8121'}\n+\n+DAEMON_PID=\n+DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n+DAEMON_URL=git://127.0.0.1:$LIB_DAEMON_PORT\n+\n+start_daemon() {\n+\tif test -n \"$DAEMON_PID\"\n+\tthen\n+\t\terror \"start_daemon already called\"\n+\tfi\n+\n+\tmkdir -p \"$DAEMON_DOCUMENT_ROOT_PATH\"\n+\n+\ttrap 'code=$?; stop_daemon; (exit $code); die' EXIT\n+\n+\tsay >&3 \"Starting git daemon ...\"\n+\tgit daemon --listen=127.0.0.1 --port=\"$LIB_DAEMON_PORT\" \\\n+\t\t--reuseaddr --verbose \\\n+\t\t--base-path=\"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t\"$@\" \"$DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t>&3 2>&4 &\n+\tDAEMON_PID=$!\n+}\n+\n+stop_daemon() {\n+\tif test -z \"$DAEMON_PID\"\n+\tthen\n+\t\treturn\n+\tfi\n+\n+\ttrap 'die' EXIT\n+\n+\t# kill git-daemon child of git\n+\tsay >&3 \"Stopping git daemon ...\"\n+\tpkill -P \"$DAEMON_PID\"\n+\twait \"$DAEMON_PID\"\n+\tret=$?\n+\tif test $ret -ne 143\n+\tthen\n+\t\terror \"git daemon exited with status: $ret\"\n+\tfi\n+\tDAEMON_PID=\n+}\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nnew file mode 100755\nindex 0000000..a7d666c\n--- /dev/null\n+++ b/t/t5570-git-daemon.sh\n@@ -0,0 +1,149 @@\n+#!/bin/sh\n+\n+test_description='test fetching over git protocol'\n+. ./test-lib.sh\n+\n+LIB_DAEMON_PORT=${LIB_DAEMON_PORT-5570}\n+. \"$TEST_DIRECTORY\"/lib-daemon.sh\n+start_daemon\n+\n+test_expect_success 'setup repository' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one\n+'\n+\n+test_expect_success 'create git-accessible bare repository' '\n+\tmkdir \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t git --bare init &&\n+\t : >git-daemon-export-ok\n+\t) &&\n+\tgit remote add public \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'clone git repository' '\n+\tgit clone $DAEMON_URL/repo.git clone &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_success 'fetch changes via git protocol' '\n+\techo content >>file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\t(cd clone && git pull) &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_failure 'remote detects correct HEAD' '\n+\tgit push public master:other &&\n+\t(cd clone &&\n+\t git remote set-head -d origin &&\n+\t git remote set-head -a origin &&\n+\t git symbolic-ref refs/remotes/origin/HEAD > output &&\n+\t echo refs/remotes/origin/master > expect &&\n+\t test_cmp expect output\n+\t)\n+'\n+\n+test_expect_success 'prepare pack objects' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack &&\n+\t git --bare prune-packed\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad1.git &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch $DAEMON_URL/repo_bad2.git &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n+test_remote_error()\n+{\n+\tdo_export=YesPlease\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase $1 in\n+\t\t-x)\n+\t\t\tshift\n+\t\t\tchmod -X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t\t\t;;\n+\t\t-n)\n+\t\t\tshift\n+\t\t\tdo_export=\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\tesac\n+\tdone\n+\n+\tif test $# -ne 3\n+\tthen\n+\t\terror \"invalid number of arguments\"\n+\tfi\n+\n+\tcmd=$1\n+\trepo=$2\n+\tmsg=$3\n+\n+\tif test -x \"$DAEMON_DOCUMENT_ROOT_PATH/$repo\"\n+\tthen\n+\t\tif test -n \"$do_export\"\n+\t\tthen\n+\t\t\t: >\"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\telse\n+\t\t\trm -f \"$DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\tfi\n+\tfi\n+\n+\ttest_must_fail git \"$cmd\" \"$DAEMON_URL/$repo\" 2>output &&\n+\techo \"fatal: remote error: $msg: /$repo\" >expect &&\n+\ttest_cmp expect output\n+\tret=$?\n+\tchmod +X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t(exit $ret)\n+}\n+\n+msg=\"access denied or repository not exported\"\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git '$msg'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git '$msg'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git '$msg'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git '$msg'\"\n+\n+stop_daemon\n+start_daemon --informative-errors\n+\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n+\n+stop_daemon\n+test_done\n-- \n1.7.8\n"},{"id":"181928","messageId":"1325692539-26748-4-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 3/6] avoid use of pkill","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:36Z","receivedAt":"2012-01-04T15:55:36Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"---\n t/lib-daemon.sh | 20 +++++++++++++++++++-\n 1 files changed, 19 insertions(+), 1 deletions(-)\n\ndiff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\nindex 30a89ea..5edced5 100644\n--- a/t/lib-daemon.sh\n+++ b/t/lib-daemon.sh\n@@ -31,6 +31,24 @@ start_daemon() {\n \tDAEMON_PID=$!\n }\n \n+kill_children() {\n+\tparent=$1\n+\n+\tps -A -o ppid,pid |\n+\t(\n+\t\t# skip header\n+\t\tread\n+\t\twhile read ppid pid\n+\t\tdo\n+\t\t\tif test x\"$ppid\" = x\"$parent\"\n+\t\t\tthen\n+\t\t\t\techo \"$pid\"\n+\t\t\tfi\n+\t\tdone\n+\t) |\n+\txargs kill\n+}\n+\n stop_daemon() {\n \tif test -z \"$DAEMON_PID\"\n \tthen\n@@ -41,7 +59,7 @@ stop_daemon() {\n \n \t# kill git-daemon child of git\n \tsay >&3 \"Stopping git daemon ...\"\n-\tpkill -P \"$DAEMON_PID\"\n+\tkill_children \"$DAEMON_PID\"\n \twait \"$DAEMON_PID\"\n \tret=$?\n \tif test $ret -ne 143\n-- \n1.7.8\n"},{"id":"181923","messageId":"1325692539-26748-5-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 4/6] explain expected exit code","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:37Z","receivedAt":"2012-01-04T15:55:37Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"---\n t/lib-daemon.sh | 4 ++++\n 1 files changed, 4 insertions(+), 0 deletions(-)\n\ndiff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\nindex 5edced5..4701124 100644\n--- a/t/lib-daemon.sh\n+++ b/t/lib-daemon.sh\n@@ -62,6 +62,10 @@ stop_daemon() {\n \tkill_children \"$DAEMON_PID\"\n \twait \"$DAEMON_PID\"\n \tret=$?\n+\t#\n+\t# We signal TERM=15 to the child and expect the parent to\n+\t# exit with 143 = 128+15.\n+\t#\n \tif test $ret -ne 143\n \tthen\n \t\terror \"git daemon exited with status: $ret\"\n-- \n1.7.8\n"},{"id":"181924","messageId":"1325692539-26748-6-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 5/6] t5570: repack everything into one file","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:38Z","receivedAt":"2012-01-04T15:55:38Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Subsequently we assume that there is only one pack. Currently this is\ntrue only by accident. Pass '-a -d' to repack in order to guarantee that\nassumption to hold true.\n\nThe prune-packed command is now redundant since repack -d already calls\nit.\n---\n t/t5570-git-daemon.sh | 3 +--\n 1 files changed, 1 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex a7d666c..f2b374b 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -50,8 +50,7 @@ test_expect_failure 'remote detects correct HEAD' '\n test_expect_success 'prepare pack objects' '\n \tcp -R \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n \t(cd \"$DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n-\t git --bare repack &&\n-\t git --bare prune-packed\n+\t git --bare repack -a -d\n \t)\n '\n \n-- \n1.7.8\n"},{"id":"181926","messageId":"1325692539-26748-7-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"[PATCH 6/6] chmod: use lower-case x","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T15:55:39Z","receivedAt":"2012-01-04T15:55:39Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"---\n t/t5570-git-daemon.sh | 4 ++--\n 1 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nindex f2b374b..d9667f9 100755\n--- a/t/t5570-git-daemon.sh\n+++ b/t/t5570-git-daemon.sh\n@@ -92,7 +92,7 @@ test_remote_error()\n \t\tcase $1 in\n \t\t-x)\n \t\t\tshift\n-\t\t\tchmod -X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t\t\tchmod -x \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n \t\t\t;;\n \t\t-n)\n \t\t\tshift\n@@ -126,7 +126,7 @@ test_remote_error()\n \techo \"fatal: remote error: $msg: /$repo\" >expect &&\n \ttest_cmp expect output\n \tret=$?\n-\tchmod +X \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\tchmod +x \"$DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n \t(exit $ret)\n }\n \n-- \n1.7.8\n"},{"id":"181933","messageId":"7vy5tnpcuw.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-04T18:00:07Z","receivedAt":"2012-01-04T18:00:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> Are you saying that the name \"daemon\" is too general, and it should\n> instead be \"lib-git-daemon.sh\" and GIT_TEST_GIT_DAEMON? Or do you\n> mean that it is not general enough and it should be called\n> lib-networking.sh and \"GIT_TEST_NETWORKING\"?\n\nThe former. \"daemon\" is too general and letting \"git daemon\" squat on that\nname makes it harder for other people to build daemons for new git\nservices and write tests for them.\n\n> Either way, I have no preference here. Feel free to change any way you\n> like.\n\nNo thanks.\n\n>> >> +\t# kill git-daemon child of git\n>> >> +\tsay >&3 \"Stopping git daemon ...\"\n>> >> +\tpkill -P \"$DAEMON_PID\"\n>> \n>> How portable is this one (I usually do not trust use of pkill anywhere)?\n>\n> I read that it is supposed to be more portable than skill or killall.\n> But I have no way to research this. I have implemented a workaround\n> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n\nYuck, that patch looks even uglier X-<.\n\nDo you really need to kill the children but not the daemon?\n"},{"id":"181934","messageId":"7vty4bpcm1.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"1325692539-26748-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/6] t5550: repack everything into one file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-04T18:05:26Z","receivedAt":"2012-01-04T18:05:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks; I assume this is also signed off?\n"},{"id":"181939","messageId":"7vehvfp6ok.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"7vy5tnpcuw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-04T20:13:31Z","receivedAt":"2012-01-04T20:13:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Clemens Buchacher writes:\n> ...\n>>> >> +\t# kill git-daemon child of git\n>>> >> +\tsay >&3 \"Stopping git daemon ...\"\n>>> >> +\tpkill -P \"$DAEMON_PID\"\n>>> \n>>> How portable is this one (I usually do not trust use of pkill anywhere)?\n>>\n>> I read that it is supposed to be more portable than skill or killall.\n>> But I have no way to research this. I have implemented a workaround\n>> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n>\n> Yuck, that patch looks even uglier X-<.\n>\n> Do you really need to kill the children but not the daemon?\n\nTo reduce round-trip cost, here is what I'll queue for now.\n\n-- >8 --\nFrom: Clemens Buchacher \nDate: Wed, 4 Jan 2012 16:55:35 +0100\nSubject: [PATCH] daemon: add tests\n\nThe semantics of the git daemon tests are similar to the http transport\ntests. In fact, they are only a slightly modified copy of t5550, plus the\nnewly added remote error tests.\n\nAll git-daemon tests will be skipped unless the environment variable\nGIT_TEST_GIT_DAEMON is set.\n\nSigned-off-by: Clemens Buchacher \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/lib-git-daemon.sh | 56 ++++++++++++++++++\n t/t5570-git-daemon.sh | 148 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 204 insertions(+), 0 deletions(-)\n create mode 100644 t/lib-git-daemon.sh\n create mode 100755 t/t5570-git-daemon.sh\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nnew file mode 100644\nindex 0000000..c0ff9e2\n--- /dev/null\n+++ b/t/lib-git-daemon.sh\n@@ -0,0 +1,56 @@\n+#!/bin/sh\n+\n+if test -z \"$GIT_TEST_GIT_DAEMON\"\n+then\n+\tskip_all=\"git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)\"\n+\ttest_done\n+fi\n+\n+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}\n+\n+GIT_DAEMON_PID=\n+GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT\n+\n+start_git_daemon() {\n+\tif test -n \"$GIT_DAEMON_PID\"\n+\tthen\n+\t\terror \"start_git_daemon already called\"\n+\tfi\n+\n+\tmkdir -p \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"\n+\n+\ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n+\n+\tsay >&3 \"Starting git daemon ...\"\n+\tgit daemon --listen=127.0.0.1 --port=\"$LIB_GIT_DAEMON_PORT\" \\\n+\t\t--reuseaddr --verbose \\\n+\t\t--base-path=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t>&3 2>&4 &\n+\tGIT_DAEMON_PID=$!\n+}\n+\n+stop_git_daemon() {\n+\tif test -z \"$GIT_DAEMON_PID\"\n+\tthen\n+\t\treturn\n+\tfi\n+\n+\ttrap 'die' EXIT\n+\n+\t# kill git-daemon child of git\n+\tsay >&3 \"Stopping git daemon ...\"\n+\tpkill -P \"$GIT_DAEMON_PID\"\n+\twait \"$GIT_DAEMON_PID\"\n+\tret=$?\n+\t#\n+\t# We signal TERM=15 to the child and expect the parent to\n+\t# exit with 143 = 128+15.\n+\t#\n+\tif test $ret -ne 143\n+\tthen\n+\t\terror \"git daemon exited with status: $ret\"\n+\tfi\n+\tGIT_DAEMON_PID=\n+}\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nnew file mode 100755\nindex 0000000..7cbc999\n--- /dev/null\n+++ b/t/t5570-git-daemon.sh\n@@ -0,0 +1,148 @@\n+#!/bin/sh\n+\n+test_description='test fetching over git protocol'\n+. ./test-lib.sh\n+\n+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}\n+. \"$TEST_DIRECTORY\"/lib-git-daemon.sh\n+start_git_daemon\n+\n+test_expect_success 'setup repository' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one\n+'\n+\n+test_expect_success 'create git-accessible bare repository' '\n+\tmkdir \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t git --bare init &&\n+\t : >git-daemon-export-ok\n+\t) &&\n+\tgit remote add public \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'clone git repository' '\n+\tgit clone \"$GIT_DAEMON_URL/repo.git\" clone &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_success 'fetch changes via git protocol' '\n+\techo content >>file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\t(cd clone && git pull) &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_failure 'remote detects correct HEAD' '\n+\tgit push public master:other &&\n+\t(cd clone &&\n+\t git remote set-head -d origin &&\n+\t git remote set-head -a origin &&\n+\t git symbolic-ref refs/remotes/origin/HEAD > output &&\n+\t echo refs/remotes/origin/master > expect &&\n+\t test_cmp expect output\n+\t)\n+'\n+\n+test_expect_success 'prepare pack objects' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack -a -d\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch \"$GIT_DAEMON_URL/repo_bad1.git\" &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch \"$GIT_DAEMON_URL/repo_bad2.git\" &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n+test_remote_error()\n+{\n+\tdo_export=YesPlease\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase $1 in\n+\t\t-x)\n+\t\t\tshift\n+\t\t\tchmod -x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t\t\t;;\n+\t\t-n)\n+\t\t\tshift\n+\t\t\tdo_export=\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\tesac\n+\tdone\n+\n+\tif test $# -ne 3\n+\tthen\n+\t\terror \"invalid number of arguments\"\n+\tfi\n+\n+\tcmd=$1\n+\trepo=$2\n+\tmsg=$3\n+\n+\tif test -x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo\"\n+\tthen\n+\t\tif test -n \"$do_export\"\n+\t\tthen\n+\t\t\t: >\"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\telse\n+\t\t\trm -f \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\tfi\n+\tfi\n+\n+\ttest_must_fail git \"$cmd\" \"$GIT_DAEMON_URL/$repo\" 2>output &&\n+\techo \"fatal: remote error: $msg: /$repo\" >expect &&\n+\ttest_cmp expect output\n+\tret=$?\n+\tchmod +x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t(exit $ret)\n+}\n+\n+msg=\"access denied or repository not exported\"\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git '$msg'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git '$msg'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git '$msg'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git '$msg'\"\n+\n+stop_git_daemon\n+start_git_daemon --informative-errors\n+\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n+\n+stop_git_daemon\n+test_done\n-- \n1.7.8.2.340.gd18f0f\n"},{"id":"181940","messageId":"20120104204017.GC27567@ecki.lan","threadId":"28543","inReplyTo":"7vy5tnpcuw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-04T20:40:17Z","receivedAt":"2012-01-04T20:40:17Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 04, 2012 at 10:00:07AM -0800, Junio C Hamano wrote:\n>\n> >> >> +\t# kill git-daemon child of git\n> >> >> +\tsay >&3 \"Stopping git daemon ...\"\n> >> >> +\tpkill -P \"$DAEMON_PID\"\n> >> \n> >> How portable is this one (I usually do not trust use of pkill anywhere)?\n> >\n> > I read that it is supposed to be more portable than skill or killall.\n> > But I have no way to research this. I have implemented a workaround\n> > using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n> \n> Yuck, that patch looks even uglier X-<.\n> \n> Do you really need to kill the children but not the daemon?\n\nIf I kill just the parent \"git daemon\" command, then the actual\ngit-daemon (started by run_command) will be left behind.\n"},{"id":"181946","messageId":"7vaa63p11t.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20120104204017.GC27567@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-04T22:15:10Z","receivedAt":"2012-01-04T22:15:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Jan 04, 2012 at 10:00:07AM -0800, Junio C Hamano wrote:\n>>\n>> >> >> +\t# kill git-daemon child of git\n>> >> >> +\tsay >&3 \"Stopping git daemon ...\"\n>> >> >> +\tpkill -P \"$DAEMON_PID\"\n>> >> \n>> >> How portable is this one (I usually do not trust use of pkill anywhere)?\n>> >\n>> > I read that it is supposed to be more portable than skill or killall.\n>> > But I have no way to research this. I have implemented a workaround\n>> > using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n>> \n>> Yuck, that patch looks even uglier X-<.\n>> \n>> Do you really need to kill the children but not the daemon?\n>\n> If I kill just the parent \"git daemon\" command, then the actual\n> git-daemon (started by run_command) will be left behind.\n\nSounds like we would be better off with a new \"--foreground\" option other\ndaemon-ish projects seem to have?\n"},{"id":"181948","messageId":"20120104222649.GA14727@sigill.intra.peff.net","threadId":"28543","inReplyTo":"7vaa63p11t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-04T22:26:49Z","receivedAt":"2012-01-04T22:26:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 04, 2012 at 02:15:10PM -0800, Junio C Hamano wrote:\n\n> >> Do you really need to kill the children but not the daemon?\n> >\n> > If I kill just the parent \"git daemon\" command, then the actual\n> > git-daemon (started by run_command) will be left behind.\n> \n> Sounds like we would be better off with a new \"--foreground\" option other\n> daemon-ish projects seem to have?\n\nIsn't that usually for \"don't background yourself\"? AFAIK, git-daemon\nstays in the foreground and only forks to handle each connection. So\ncan't we just kill the main process, and any running cruft will\neventually die as connections are closed?\n\nOr is the problem the git wrapper itself, which doesn't kill its\nsubprocess when it dies (which IMHO is a bug which we might want to\nfix)? In that case, couldn't we just use --pid-file to save the actual\ndaemon pid, and then kill using that?\n\nAs a side note, it looks like we just start the daemon with \"git daemon\n&\". Doesn't that create a race condition with the tests which\nimmediately try to access it (i.e., the first test may run before the\ndaemon actually opens the socket)?\n\n-Peff\n"},{"id":"181953","messageId":"20120105000713.GA24220@ecki.lan","threadId":"28543","inReplyTo":"20120104222649.GA14727@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-05T00:07:13Z","receivedAt":"2012-01-05T00:07:13Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:\n> \n> Or is the problem the git wrapper itself, which doesn't kill its\n> subprocess when it dies (which IMHO is a bug which we might want to\n> fix)? In that case, couldn't we just use --pid-file to save the actual\n> daemon pid, and then kill using that?\n\nOr like this. Doesn't work with multiple children. I have yet to\ncheck if we have those anywhere.\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..0c105e6 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -65,6 +65,22 @@ static int execv_shell_cmd(const char **argv)\n #ifndef WIN32\n static int child_err = 2;\n static int child_notifier = -1;\n+static struct child_process *current_cmd;\n+\n+static void kill_current_cmd(int signo)\n+{\n+\tsignal(signo, SIG_DFL);\n+\n+\tif (current_cmd) {\n+\t\tif (current_cmd->pid) {\n+\t\t\t/* forward signal to the child process */\n+\t\t\tkill(current_cmd->pid, signo);\n+\t\t} else {\n+\t\t\t/* trigger the default signal handler */\n+\t\t\traise(signo);\n+\t\t}\n+\t}\n+}\n \n static void notify_parent(void)\n {\n@@ -201,6 +217,9 @@ fail_pipe:\n \tif (pipe(notify_pipe))\n \t\tnotify_pipe[0] = notify_pipe[1] = -1;\n \n+\tcurrent_cmd = cmd;\n+\tsignal(SIGTERM, kill_current_cmd);\n+\n \tcmd->pid = fork();\n \tif (!cmd->pid) {\n \t\t/*\ndiff --git a/t/lib-daemon.sh b/t/lib-daemon.sh\nindex b2ffd54..c1c41ee 100644\n--- a/t/lib-daemon.sh\n+++ b/t/lib-daemon.sh\n@@ -41,8 +41,8 @@ stop_daemon() {\n \n \t# kill git-daemon child of git\n \tsay >&3 \"Stopping git daemon ...\"\n-\tpkill -P \"$DAEMON_PID\"\n-\twait \"$DAEMON_PID\"\n+\tkill \"$DAEMON_PID\"\n+\twait \"$DAEMON_PID\" >&3 2>&4\n \tret=$?\n \t#\n \t# We signal TERM=15 to the child and expect the parent to\n\n> As a side note, it looks like we just start the daemon with \"git daemon\n> &\". Doesn't that create a race condition with the tests which\n> immediately try to access it (i.e., the first test may run before the\n> daemon actually opens the socket)?\n\nThat's correct. How would I fix that? Try connecting and sleep in a\nloop until ready or timeout? Will look into that.\n"},{"id":"181955","messageId":"7vk457ngi0.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20120105000713.GA24220@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-05T00:24:23Z","receivedAt":"2012-01-05T00:24:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:\n>> \n>> Or is the problem the git wrapper itself, which doesn't kill its\n>> subprocess when it dies (which IMHO is a bug which we might want to\n>> fix)? In that case, couldn't we just use --pid-file to save the actual\n>> daemon pid, and then kill using that?\n>\n> Or like this. Doesn't work with multiple children. I have yet to\n> check if we have those anywhere.\n\nHmm, don't we have them in the same process group or something, though?\nCan't we kill them as a whole?\n"},{"id":"181957","messageId":"20120105003841.GA25285@ecki.lan","threadId":"28543","inReplyTo":"7vk457ngi0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-05T00:38:41Z","receivedAt":"2012-01-05T00:38:41Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 04, 2012 at 04:24:23PM -0800, Junio C Hamano wrote:\n> Clemens Buchacher writes:\n> \n> > On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:\n> >> \n> >> Or is the problem the git wrapper itself, which doesn't kill its\n> >> subprocess when it dies (which IMHO is a bug which we might want to\n> >> fix)? In that case, couldn't we just use --pid-file to save the actual\n> >> daemon pid, and then kill using that?\n> >\n> > Or like this. Doesn't work with multiple children. I have yet to\n> > check if we have those anywhere.\n> \n> Hmm, don't we have them in the same process group or something, though?\n> Can't we kill them as a whole?\n\nI tried that, but it seems that the test script itself is in the\nsame process group.\n"},{"id":"181958","messageId":"m3vcoqevjm.fsf@localhost.localdomain","threadId":"28543","inReplyTo":"20120104222649.GA14727@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-05T02:24:16Z","receivedAt":"2012-01-05T02:24:16Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Jeff King writes:\n\n> As a side note, it looks like we just start the daemon with \"git daemon\n> &\". Doesn't that create a race condition with the tests which\n> immediately try to access it (i.e., the first test may run before the\n> daemon actually opens the socket)?\n\nHmmm... perhaps the trick that git-instaweb does for \"plackup\" web\nserver would be of use here, waiting for socket to be ready?\n\n-- \nJakub Narebski\n"},{"id":"181959","messageId":"20120105025154.GA7326@sigill.intra.peff.net","threadId":"28543","inReplyTo":"m3vcoqevjm.fsf@localhost.localdomain","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-05T02:51:54Z","receivedAt":"2012-01-05T02:51:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 04, 2012 at 06:24:16PM -0800, Jakub Narebski wrote:\n\n> Jeff King writes:\n> \n> > As a side note, it looks like we just start the daemon with \"git daemon\n> > &\". Doesn't that create a race condition with the tests which\n> > immediately try to access it (i.e., the first test may run before the\n> > daemon actually opens the socket)?\n> \n> Hmmm... perhaps the trick that git-instaweb does for \"plackup\" web\n> server would be of use here, waiting for socket to be ready?\n\nIt looks like it busy loops, which is kind of ugly.\n\nThe credential-cache helper has a similar problem. It wants to kick off\na daemon if one is not already running, and then connect to it. So the\ndaemon does:\n\n printf(\"ok\\n\");\n fclose(stdout);\n\nwhen it has set up the socket, and the client does:\n\n r = read_in_full(daemon.out, buf, sizeof(buf));\n if (r < 0)\n die_errno(\"unable to read result code from cache daemon\");\n if (r != 3 || memcmp(buf, \"ok\\n\", 3))\n die(\"cache daemon did not start: %.*s\", r, buf);\n /* now we can connect over the socket */\n\nWe could probably add a \"--notify-when-ready\" option to git-daemon to\ndo something similar.\n\n-Peff\n"},{"id":"181960","messageId":"20120105025559.GB7326@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120105000713.GA24220@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-05T02:55:59Z","receivedAt":"2012-01-05T02:55:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2012 at 01:07:13AM +0100, Clemens Buchacher wrote:\n\n> On Wed, Jan 04, 2012 at 05:26:49PM -0500, Jeff King wrote:\n> > \n> > Or is the problem the git wrapper itself, which doesn't kill its\n> > subprocess when it dies (which IMHO is a bug which we might want to\n> > fix)? In that case, couldn't we just use --pid-file to save the actual\n> > daemon pid, and then kill using that?\n> \n> Or like this. Doesn't work with multiple children. I have yet to\n> check if we have those anywhere.\n\nIt so happens that I have just the patch you need. I've been meaning to\ngo over it again and submit it:\n\n run-command: optionally kill children on exit\n https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989\n\nThe original use case was to help with this:\n\n https://github.com/peff/git/commit/79bf3f232f89c3e2f5284a3b7b71a667be8825d1\n\n> > As a side note, it looks like we just start the daemon with \"git daemon\n> > &\". Doesn't that create a race condition with the tests which\n> > immediately try to access it (i.e., the first test may run before the\n> > daemon actually opens the socket)?\n> \n> That's correct. How would I fix that? Try connecting and sleep in a\n> loop until ready or timeout? Will look into that.\n\nYour choices are basically busy-waiting, or convincing the daemon to\nsend a signal when it's ready to serve. I like the latter, but it does\nmean adding a small amount of code to git-daemon.\n\n-Peff\n"},{"id":"181983","messageId":"20120105160612.GA27251@ecki.lan","threadId":"28543","inReplyTo":"20120105025559.GB7326@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-05T16:06:15Z","receivedAt":"2012-01-05T16:06:15Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Wed, Jan 04, 2012 at 09:55:59PM -0500, Jeff King wrote:\n> \n> It so happens that I have just the patch you need. I've been meaning to\n> go over it again and submit it:\n> \n> run-command: optionally kill children on exit\n> https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989\n\nThanks, looks great. But if I add this on top (to enable this for\n\"git daemon\"), then t0001 kills my entire X session. Not sure yet\nwhat's going.\n\ndiff --git a/run-command.c b/run-command.c\nindex aeb9c6e..53218df 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -497,6 +497,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,\n cmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n cmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n cmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n+ cmd->clean_on_exit = 1;\n }\n \n int run_command_v_opt(const char **argv, int opt)\n"},{"id":"182006","messageId":"C9D7A58F-853D-4DAB-B27D-78D60B8EE152@silverinsanity.com","threadId":"28543","inReplyTo":"1325692539-26748-1-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Brian Gernhardt","fromEmail":"benji@silverinsanity.com","sentAt":"2012-01-06T06:17:16Z","receivedAt":"2012-01-06T06:17:16Z","isPatch":true,"sender":{"key":"benji@silverinsanity.com","avatar":"https://gravatar.com/avatar/e06c101dbc25c68114d859b4a9ec7cf8a2c52fd2b0270ef0eac0e2e63ff22311?d=mp&s=160"},"body":"\nOn Jan 4, 2012, at 10:55 AM, Clemens Buchacher wrote:\n\n> On Tue, Jan 03, 2012 at 11:34:08AM -0800, Junio C Hamano wrote:\n>>> +\t# kill git-daemon child of git\n>>> +\tsay >&3 \"Stopping git daemon ...\"\n>>> +\tpkill -P \"$DAEMON_PID\"\n>> \n>> How portable is this one (I usually do not trust use of pkill anywhere)?\n> \n> I read that it is supposed to be more portable than skill or killall.\n> But I have no way to research this. I have implemented a workaround\n> using only 'ps' and 'kill' in [PATCH 3/6] avoid use of pkill.\n\nAs a data point: pkill and skill do not exist on OS X 10.7, but killall does.\n\n~~ Brian Gernhardt\n"},{"id":"182027","messageId":"20120106155204.GA17355@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120105160612.GA27251@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T15:52:04Z","receivedAt":"2012-01-06T15:52:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 05, 2012 at 05:06:15PM +0100, Clemens Buchacher wrote:\n\n> On Wed, Jan 04, 2012 at 09:55:59PM -0500, Jeff King wrote:\n> > \n> > It so happens that I have just the patch you need. I've been meaning to\n> > go over it again and submit it:\n> > \n> > run-command: optionally kill children on exit\n> > https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989\n> \n> Thanks, looks great. But if I add this on top (to enable this for\n> \"git daemon\"), then t0001 kills my entire X session. Not sure yet\n> what's going.\n\nYikes. Thanks for noticing.\n\nWhat happens is we have a failure case in start_command, set pid to -1,\nand then fall through to the end of the function. So we end up marking\n\"-1\" for cleanup, which attempts to kill all processes.\n\nI never noticed it because it can only happen when fork() fails, or when\na child process signals an exec failure (which happens all the time with\naliases, but could not be triggered until your patch).\n\nThe fix is to move the recording of the PID up to a spot where we are\ncertain that it's a real PID. Fixup patch is below, and I'll push a new\nversion out to my github repo.\n\n-Peff\n\ndiff --git a/run-command.c b/run-command.c\nindex aeb9c6e..614b722 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -353,6 +353,8 @@ fail_pipe:\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n \t\t\tstrerror(failed_errno = errno));\n+\telse if (cmd->clean_on_exit)\n+\t\tmark_child_for_cleanup(cmd->pid);\n \n \t/*\n \t * Wait for child's execvp. If the execvp succeeds (or if fork()\n@@ -374,8 +376,6 @@ fail_pipe:\n \t}\n \tclose(notify_pipe[0]);\n \n-\tif (cmd->clean_on_exit)\n-\t\tmark_child_for_cleanup(cmd->pid);\n }\n #else\n {\n"},{"id":"182038","messageId":"20120106194800.GA9301@ecki.lan","threadId":"28543","inReplyTo":"20120106155204.GA17355@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-06T19:48:00Z","receivedAt":"2012-01-06T19:48:00Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Jan 06, 2012 at 10:52:04AM -0500, Jeff King wrote:\n> > > \n> > > run-command: optionally kill children on exit\n> > > https://github.com/peff/git/commit/5523d7ebf2a0386c9c61d7bfbc21375041df4989\n> > \n> > Thanks, looks great. But if I add this on top (to enable this for\n> > \"git daemon\"), then t0001 kills my entire X session. Not sure yet\n> > what's going.\n> \n> The fix is to move the recording of the PID up to a spot where we are\n> certain that it's a real PID. Fixup patch is below, and I'll push a new\n> version out to my github repo.\n\nI have rebased Junio's cb/git-daemon-tests onto your\njk/child-cleanup and replaced the call to pkill with a regular kill\ncommand.\n\nOn top of that, I have added two commits to fix the discussed race\ncondition. I also verified that the race condition actually happens\nby adding an artificial delay in the daemon (this change is\nobviously not included).\n\nI pushed the new cb/git-daemon-tests to\nhttps://github.com/drizzd/git . If you have no objections I will\npost the entire series including your run-command and send-pack\npatches to the list.\n\nClemens\n"},{"id":"182044","messageId":"20120106223215.GA13106@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120106194800.GA9301@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-06T22:32:15Z","receivedAt":"2012-01-06T22:32:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:\n\n> I have rebased Junio's cb/git-daemon-tests onto your\n> jk/child-cleanup and replaced the call to pkill with a regular kill\n> command.\n\nLooks pretty good from my cursory examination. I think you should fill\nout the rationale for \"kill dashed externals on exit\" a bit. My\nreasoning is that whether a git command is an internal or external\nprocess is purely an implementation detail, and killing the git wrapper\nshould behave identically in both cases.\n\n> On top of that, I have added two commits to fix the discussed race\n> condition. I also verified that the race condition actually happens\n> by adding an artificial delay in the daemon (this change is\n> obviously not included).\n\nLooks reasonable to me.\n\n> I pushed the new cb/git-daemon-tests to\n> https://github.com/drizzd/git . If you have no objections I will\n> post the entire series including your run-command and send-pack\n> patches to the list.\n\nNo objections here. Thanks for moving this forward.\n\n-Peff\n"},{"id":"182048","messageId":"7vipkoih0e.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20120106194800.GA9301@ecki.lan","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-06T22:49:05Z","receivedAt":"2012-01-06T22:49:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Clemens Buchacher writes:\n\n> I have rebased Junio's cb/git-daemon-tests onto your\n> jk/child-cleanup and replaced the call to pkill with a regular kill\n> command.\n>\n> On top of that, I have added two commits to fix the discussed race\n> condition. I also verified that the race condition actually happens\n> by adding an artificial delay in the daemon (this change is\n> obviously not included).\n>\n> I pushed the new cb/git-daemon-tests to\n> https://github.com/drizzd/git . If you have no objections I will\n> post the entire series including your run-command and send-pack\n> patches to the list.\n\nLooked fine except that some patches seem to lack enough justification\n(justification in Peff's reply was good enough).\n\nI actually was thinking that the previous round was good enough (perhaps\ndropping the \"pkill\" bit altogether and replacing it with \"kill\" on the\ndaemon process itself, if OSX folks complain loudly), so it is in \"next\"\nalready, but it seems that the best course of action would be to drop it\nand queue your re-roll afresh, aiming for the next cycle.\n\nThanks.\n"},{"id":"182050","messageId":"201201070035.52581.jnareb@gmail.com","threadId":"28543","inReplyTo":"20120105025154.GA7326@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-01-06T23:35:50Z","receivedAt":"2012-01-06T23:35:50Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 5 Jan 2012, Jeff King wrote:\n> On Wed, Jan 04, 2012 at 06:24:16PM -0800, Jakub Narebski wrote:\n> \n> > Jeff King writes:\n> > \n> > > As a side note, it looks like we just start the daemon with \"git daemon\n> > > &\". Doesn't that create a race condition with the tests which\n> > > immediately try to access it (i.e., the first test may run before the\n> > > daemon actually opens the socket)?\n> > \n> > Hmmm... perhaps the trick that git-instaweb does for \"plackup\" web\n> > server would be of use here, waiting for socket to be ready?\n> \n> It looks like it busy loops, which is kind of ugly.\n\nWell, as far as I know you can wait for data on socket or pipe, but\nyou can't wait for socket to be created.\n\nAnyway this busy-wait is not too busy, and it is better than just\nadding 'sleep 1' in testsuite.\n\n> The credential-cache helper has a similar problem. It wants to kick off\n> a daemon if one is not already running, and then connect to it. So the\n> daemon does:\n> \n> printf(\"ok\\n\");\n> fclose(stdout);\n> \n> when it has set up the socket, and the client does:\n> \n> r = read_in_full(daemon.out, buf, sizeof(buf));\n> if (r < 0)\n> die_errno(\"unable to read result code from cache daemon\");\n> if (r != 3 || memcmp(buf, \"ok\\n\", 3))\n> die(\"cache daemon did not start: %.*s\", r, buf);\n> /* now we can connect over the socket */\n> \n> We could probably add a \"--notify-when-ready\" option to git-daemon to\n> do something similar.\n\nWhat would git-daemon do what it is ready? Write to socket, raise signal,\nprint to STDOUT / STDERR?\n\nBTW. I wonder if it would be worth it to add something a la systemd\ntrick creating sockets first to git-daemon. Adding systemd support\ndoesn't make sense for daemon that is to be run from inetd / xinetd,\nI guess.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"182062","messageId":"1325936567-3136-1-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"7vipkoih0e.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:42Z","receivedAt":"2012-01-07T11:42:42Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Jan 06, 2012 at 02:49:05PM -0800, Junio C Hamano wrote:\n> \n> but it seems that the best course of action would be to drop it\n> and queue your re-roll afresh, aiming for the next cycle.\n\nHere's the re-rolled series, also available as cb/git-daemon-tests based\non current master at https://github.com/drizzd/git .\n\n[PATCH 1/5] run-command: optionally kill children on exit\n[PATCH 2/5] run-command: kill children on exit by default\n[PATCH 3/5] git-daemon: add tests\n[PATCH 4/5] git-daemon: produce output when ready\n[PATCH 5/5] git-daemon tests: wait until daemon is ready\n\nOn Fri, Jan 06, 2012 at 05:32:15PM -0500, Jeff King wrote:\n> On Fri, Jan 06, 2012 at 08:48:00PM +0100, Clemens Buchacher wrote:\n> \n> > I have rebased Junio's cb/git-daemon-tests onto your\n> > jk/child-cleanup and replaced the call to pkill with a regular kill\n> > command.\n> \n> Looks pretty good from my cursory examination. I think you should fill\n> out the rationale for \"kill dashed externals on exit\" a bit. My\n> reasoning is that whether a git command is an internal or external\n> process is purely an implementation detail, and killing the git wrapper\n> should behave identically in both cases.\n\nThe previous version of this patch only changed the behavior for users\nof run_command_v_opt, but not for those who filled out the child_process\nstructure by themselves. I could have manually enabled all of those, but\nthat felt unnatural. Instead, I have now reversed the meaning of\nclean_on_exit to stay_alive_on_exit in [PATCH 2/5] run-command: kill\nchildren on exit by default. Cleanup is on by default and callers of\nrun_command must disable it if children should stay alive.\n"},{"id":"182064","messageId":"1325936567-3136-2-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325936567-3136-1-git-send-email-drizzd@aon.at","subject":"[PATCH 1/5] run-command: optionally kill children on exit","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:43Z","receivedAt":"2012-01-07T11:42:43Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"From: Jeff King \n\nWhen we spawn a helper process, it should generally be done\nand finish_command called before we exit. However, if we\nexit abnormally due to an early return or a signal, the\nhelper may continue to run in our absence.\n\nIn the best case, this may simply be wasted CPU cycles or a\nfew stray messages on a terminal. But it could also mean a\nprocess that the user thought was aborted continues to run\nto completion (e.g., a push's pack-objects helper will\ncomplete the push, even though you killed the push process).\n\nThis patch provides infrastructure for run-command to keep\ntrack of PIDs to be killed, and clean them on signal\nreception or input, just as we do with tempfiles. PIDs can\nbe added in two ways:\n\n 1. If NO_PTHREADS is defined, async helper processes are\n automatically marked. By definition this code must be\n ready to die when the parent dies, since it may be\n implemented as a thread of the parent process.\n\n 2. If the run-command caller specifies the \"clean_on_exit\"\n option. This is not the default, as there are cases\n where it is OK for the child to outlive us (e.g., when\n spawning a pager).\n\nPIDs are cleared from the kill-list automatically during\nwait_or_whine, which is called from finish_command and\nfinish_async.\n\nSigned-off-by: Clemens Buchacher \n---\n\nNot sure if I can sign off without your sign-off. Should I have\nreplaced this with Acked-by?\n\n run-command.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n run-command.h | 1 +\n 2 files changed, 69 insertions(+), 0 deletions(-)\n\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..0204aaf 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -1,8 +1,66 @@\n #include \"cache.h\"\n #include \"run-command.h\"\n #include \"exec_cmd.h\"\n+#include \"sigchain.h\"\n #include \"argv-array.h\"\n \n+struct child_to_clean {\n+\tpid_t pid;\n+\tstruct child_to_clean *next;\n+};\n+static struct child_to_clean *children_to_clean;\n+static int installed_child_cleanup_handler;\n+\n+static void cleanup_children(int sig)\n+{\n+\twhile (children_to_clean) {\n+\t\tstruct child_to_clean *p = children_to_clean;\n+\t\tchildren_to_clean = p->next;\n+\t\tkill(p->pid, sig);\n+\t\tfree(p);\n+\t}\n+}\n+\n+static void cleanup_children_on_signal(int sig)\n+{\n+\tcleanup_children(sig);\n+\tsigchain_pop(sig);\n+\traise(sig);\n+}\n+\n+static void cleanup_children_on_exit(void)\n+{\n+\tcleanup_children(SIGTERM);\n+}\n+\n+static void mark_child_for_cleanup(pid_t pid)\n+{\n+\tstruct child_to_clean *p = xmalloc(sizeof(*p));\n+\tp->pid = pid;\n+\tp->next = children_to_clean;\n+\tchildren_to_clean = p;\n+\n+\tif (!installed_child_cleanup_handler) {\n+\t\tatexit(cleanup_children_on_exit);\n+\t\tsigchain_push_common(cleanup_children_on_signal);\n+\t\tinstalled_child_cleanup_handler = 1;\n+\t}\n+}\n+\n+static void clear_child_for_cleanup(pid_t pid)\n+{\n+\tstruct child_to_clean **last, *p;\n+\n+\tlast = &children_to_clean;\n+\tfor (p = children_to_clean; p; p = p->next) {\n+\t\tif (p->pid == pid) {\n+\t\t\t*last = p->next;\n+\t\t\tfree(p);\n+\t\t\treturn;\n+\t\t}\n+\t}\n+}\n+\n static inline void close_pair(int fd[2])\n {\n \tclose(fd[0]);\n@@ -130,6 +188,9 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)\n \t} else {\n \t\terror(\"waitpid is confused (%s)\", argv0);\n \t}\n+\n+\tclear_child_for_cleanup(pid);\n+\n \terrno = failed_errno;\n \treturn code;\n }\n@@ -292,6 +353,8 @@ fail_pipe:\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n \t\t\tstrerror(failed_errno = errno));\n+\telse if (cmd->clean_on_exit)\n+\t\tmark_child_for_cleanup(cmd->pid);\n \n \t/*\n \t * Wait for child's execvp. If the execvp succeeds (or if fork()\n@@ -312,6 +375,7 @@ fail_pipe:\n \t\tcmd->pid = -1;\n \t}\n \tclose(notify_pipe[0]);\n+\n }\n #else\n {\n@@ -356,6 +420,8 @@ fail_pipe:\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n \t\terror(\"cannot spawn %s: %s\", cmd->argv[0], strerror(errno));\n+\tif (cmd->clean_on_exit && cmd->pid >= 0)\n+\t\tmark_child_for_cleanup(cmd->pid);\n \n \tif (cmd->env)\n \t\tfree_environ(env);\n@@ -540,6 +606,8 @@ int start_async(struct async *async)\n \t\texit(!!async->proc(proc_in, proc_out, async->data));\n \t}\n \n+\tmark_child_for_cleanup(async->pid);\n+\n \tif (need_in)\n \t\tclose(fdin[0]);\n \telse if (async->in)\ndiff --git a/run-command.h b/run-command.h\nindex 56491b9..2a69466 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -38,6 +38,7 @@ struct child_process {\n \tunsigned silent_exec_failure:1;\n \tunsigned stdout_to_stderr:1;\n \tunsigned use_shell:1;\n+\tunsigned clean_on_exit:1;\n \tvoid (*preexec_cb)(void);\n };\n \n-- \n1.7.8\n"},{"id":"182060","messageId":"1325936567-3136-3-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325936567-3136-1-git-send-email-drizzd@aon.at","subject":"[PATCH 2/5] run-command: kill children on exit by default","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:44Z","receivedAt":"2012-01-07T11:42:44Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"It feels natural for a user to view git commands as monolithic\ncommands with a single thread of execution. If the parent git\ncommand dies, it should therefore clean up its child processes as\nwell. So enable the cleanup mechanism by default.\n\nFor dashed externals, this means that killing the git wrapper will\nkill the command itself, just like what would happen in case of an\ninternal command. A notable exception is the credentials cache\ndaemon, which must stay alive after the store command has\ncompleted.\n\nSigned-off-by: Clemens Buchacher \n---\n\nI considered squashing this into the previous commit. But it's a fairly\nsmall change and may help with bisecting in case of problems.\n\n credential-cache.c | 1 +\n run-command.c | 4 ++--\n run-command.h | 2 +-\n 3 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex dc98372..15e7236 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -48,6 +48,7 @@ static void spawn_daemon(const char *socket)\n \tdaemon.argv = argv;\n \tdaemon.no_stdin = 1;\n \tdaemon.out = -1;\n+\tdaemon.stay_alive_on_exit = 1;\n \n \tif (start_command(&daemon))\n \t\tdie_errno(\"unable to start cache daemon\");\ndiff --git a/run-command.c b/run-command.c\nindex 0204aaf..fe07b20 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -353,7 +353,7 @@ fail_pipe:\n \tif (cmd->pid < 0)\n \t\terror(\"cannot fork() for %s: %s\", cmd->argv[0],\n \t\t\tstrerror(failed_errno = errno));\n-\telse if (cmd->clean_on_exit)\n+\telse if (!cmd->stay_alive_on_exit)\n \t\tmark_child_for_cleanup(cmd->pid);\n \n \t/*\n@@ -420,7 +420,7 @@ fail_pipe:\n \tfailed_errno = errno;\n \tif (cmd->pid < 0 && (!cmd->silent_exec_failure || errno != ENOENT))\n \t\terror(\"cannot spawn %s: %s\", cmd->argv[0], strerror(errno));\n-\tif (cmd->clean_on_exit && cmd->pid >= 0)\n+\tif (!cmd->stay_alive_on_exit && cmd->pid >= 0)\n \t\tmark_child_for_cleanup(cmd->pid);\n \n \tif (cmd->env)\ndiff --git a/run-command.h b/run-command.h\nindex 2a69466..69dbea1 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -38,7 +38,7 @@ struct child_process {\n \tunsigned silent_exec_failure:1;\n \tunsigned stdout_to_stderr:1;\n \tunsigned use_shell:1;\n-\tunsigned clean_on_exit:1;\n+\tunsigned stay_alive_on_exit:1;\n \tvoid (*preexec_cb)(void);\n };\n \n-- \n1.7.8\n"},{"id":"182065","messageId":"1325936567-3136-4-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325936567-3136-1-git-send-email-drizzd@aon.at","subject":"[PATCH 3/5] git-daemon: add tests","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:45Z","receivedAt":"2012-01-07T11:42:45Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"The semantics of the git daemon tests are similar to the http transport\ntests. In fact, they are only a slightly modified copy of t5550, plus the\nnewly added remote error tests.\n\nAll git-daemon tests will be skipped unless the environment variable\nGIT_TEST_GIT_DAEMON is set.\n\nSigned-off-by: Clemens Buchacher \nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/lib-git-daemon.sh | 53 +++++++++++++++++\n t/t5570-git-daemon.sh | 148 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 201 insertions(+), 0 deletions(-)\n create mode 100644 t/lib-git-daemon.sh\n create mode 100755 t/t5570-git-daemon.sh\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nnew file mode 100644\nindex 0000000..5e81a25\n--- /dev/null\n+++ b/t/lib-git-daemon.sh\n@@ -0,0 +1,53 @@\n+#!/bin/sh\n+\n+if test -z \"$GIT_TEST_GIT_DAEMON\"\n+then\n+\tskip_all=\"git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to enable)\"\n+\ttest_done\n+fi\n+\n+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-'8121'}\n+\n+GIT_DAEMON_PID=\n+GIT_DAEMON_DOCUMENT_ROOT_PATH=\"$PWD\"/repo\n+GIT_DAEMON_URL=git://127.0.0.1:$LIB_GIT_DAEMON_PORT\n+\n+start_git_daemon() {\n+\tif test -n \"$GIT_DAEMON_PID\"\n+\tthen\n+\t\terror \"start_git_daemon already called\"\n+\tfi\n+\n+\tmkdir -p \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"\n+\n+\ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n+\n+\tsay >&3 \"Starting git daemon ...\"\n+\tgit daemon --listen=127.0.0.1 --port=\"$LIB_GIT_DAEMON_PORT\" \\\n+\t\t--reuseaddr --verbose \\\n+\t\t--base-path=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n+\t\t>&3 2>&4 &\n+\tGIT_DAEMON_PID=$!\n+}\n+\n+stop_git_daemon() {\n+\tif test -z \"$GIT_DAEMON_PID\"\n+\tthen\n+\t\treturn\n+\tfi\n+\n+\ttrap 'die' EXIT\n+\n+\t# kill git-daemon child of git\n+\tsay >&3 \"Stopping git daemon ...\"\n+\tkill \"$GIT_DAEMON_PID\"\n+\twait \"$GIT_DAEMON_PID\" >&3 2>&4\n+\tret=$?\n+\t# expect exit with status 143 = 128+15 for signal TERM=15\n+\tif test $ret -ne 143\n+\tthen\n+\t\terror \"git daemon exited with status: $ret\"\n+\tfi\n+\tGIT_DAEMON_PID=\n+}\ndiff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh\nnew file mode 100755\nindex 0000000..7cbc999\n--- /dev/null\n+++ b/t/t5570-git-daemon.sh\n@@ -0,0 +1,148 @@\n+#!/bin/sh\n+\n+test_description='test fetching over git protocol'\n+. ./test-lib.sh\n+\n+LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-5570}\n+. \"$TEST_DIRECTORY\"/lib-git-daemon.sh\n+start_git_daemon\n+\n+test_expect_success 'setup repository' '\n+\techo content >file &&\n+\tgit add file &&\n+\tgit commit -m one\n+'\n+\n+test_expect_success 'create git-accessible bare repository' '\n+\tmkdir \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\t git --bare init &&\n+\t : >git-daemon-export-ok\n+\t) &&\n+\tgit remote add public \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\" &&\n+\tgit push public master:master\n+'\n+\n+test_expect_success 'clone git repository' '\n+\tgit clone \"$GIT_DAEMON_URL/repo.git\" clone &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_success 'fetch changes via git protocol' '\n+\techo content >>file &&\n+\tgit commit -a -m two &&\n+\tgit push public &&\n+\t(cd clone && git pull) &&\n+\ttest_cmp file clone/file\n+'\n+\n+test_expect_failure 'remote detects correct HEAD' '\n+\tgit push public master:other &&\n+\t(cd clone &&\n+\t git remote set-head -d origin &&\n+\t git remote set-head -a origin &&\n+\t git symbolic-ref refs/remotes/origin/HEAD > output &&\n+\t echo refs/remotes/origin/master > expect &&\n+\t test_cmp expect output\n+\t)\n+'\n+\n+test_expect_success 'prepare pack objects' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git &&\n+\t git --bare repack -a -d\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt pack' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad1.git &&\n+\t p=`ls objects/pack/pack-*.pack` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad1.git &&\n+\t(cd repo_bad1.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch \"$GIT_DAEMON_URL/repo_bad1.git\" &&\n+\t test 0 = `ls objects/pack/pack-*.pack | wc -l`\n+\t)\n+'\n+\n+test_expect_success 'fetch notices corrupt idx' '\n+\tcp -R \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_pack.git \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t(cd \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\"/repo_bad2.git &&\n+\t p=`ls objects/pack/pack-*.idx` &&\n+\t chmod u+w $p &&\n+\t printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc\n+\t) &&\n+\tmkdir repo_bad2.git &&\n+\t(cd repo_bad2.git &&\n+\t git --bare init &&\n+\t test_must_fail git --bare fetch \"$GIT_DAEMON_URL/repo_bad2.git\" &&\n+\t test 0 = `ls objects/pack | wc -l`\n+\t)\n+'\n+\n+test_remote_error()\n+{\n+\tdo_export=YesPlease\n+\twhile test $# -gt 0\n+\tdo\n+\t\tcase $1 in\n+\t\t-x)\n+\t\t\tshift\n+\t\t\tchmod -x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t\t\t;;\n+\t\t-n)\n+\t\t\tshift\n+\t\t\tdo_export=\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\tesac\n+\tdone\n+\n+\tif test $# -ne 3\n+\tthen\n+\t\terror \"invalid number of arguments\"\n+\tfi\n+\n+\tcmd=$1\n+\trepo=$2\n+\tmsg=$3\n+\n+\tif test -x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo\"\n+\tthen\n+\t\tif test -n \"$do_export\"\n+\t\tthen\n+\t\t\t: >\"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\telse\n+\t\t\trm -f \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/$repo/git-daemon-export-ok\"\n+\t\tfi\n+\tfi\n+\n+\ttest_must_fail git \"$cmd\" \"$GIT_DAEMON_URL/$repo\" 2>output &&\n+\techo \"fatal: remote error: $msg: /$repo\" >expect &&\n+\ttest_cmp expect output\n+\tret=$?\n+\tchmod +x \"$GIT_DAEMON_DOCUMENT_ROOT_PATH/repo.git\"\n+\t(exit $ret)\n+}\n+\n+msg=\"access denied or repository not exported\"\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git '$msg'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git '$msg'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git '$msg'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git '$msg'\"\n+\n+stop_git_daemon\n+start_git_daemon --informative-errors\n+\n+test_expect_success 'clone non-existent' \"test_remote_error clone nowhere.git 'no such repository'\"\n+test_expect_success 'push disabled' \"test_remote_error push repo.git 'service not enabled'\"\n+test_expect_success 'read access denied' \"test_remote_error -x fetch repo.git 'no such repository'\"\n+test_expect_success 'not exported' \"test_remote_error -n fetch repo.git 'repository not exported'\"\n+\n+stop_git_daemon\n+test_done\n-- \n1.7.8\n"},{"id":"182061","messageId":"1325936567-3136-5-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325936567-3136-1-git-send-email-drizzd@aon.at","subject":"[PATCH 4/5] git-daemon: produce output when ready","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:46Z","receivedAt":"2012-01-07T11:42:46Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"If a client tries to connect after git-daemon starts, but before it\nopens a listening socket, the connection will fail. Output \"[PID]\nReady to rumble]\" after opening the socket successfully in order to\ninform the user that the daemon is now ready to receive\nconnections.\n\nSigned-off-by: Clemens Buchacher \n---\n daemon.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/daemon.c b/daemon.c\nindex 15ce918..ab21e66 100644\n--- a/daemon.c\n+++ b/daemon.c\n@@ -1086,6 +1086,8 @@ static int serve(struct string_list *listen_addr, int listen_port,\n \n \tdrop_privileges(cred);\n \n+\tloginfo(\"Ready to rumble\");\n+\n \treturn service_loop(&socklist);\n }\n \n@@ -1270,10 +1272,8 @@ int main(int argc, char **argv)\n \tif (inetd_mode || serve_mode)\n \t\treturn execute();\n \n-\tif (detach) {\n+\tif (detach)\n \t\tdaemonize();\n-\t\tloginfo(\"Ready to rumble\");\n-\t}\n \telse\n \t\tsanitize_stdfds();\n \n-- \n1.7.8\n"},{"id":"182063","messageId":"1325936567-3136-6-git-send-email-drizzd@aon.at","threadId":"28543","inReplyTo":"1325936567-3136-1-git-send-email-drizzd@aon.at","subject":"[PATCH 5/5] git-daemon tests: wait until daemon is ready","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:42:47Z","receivedAt":"2012-01-07T11:42:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"In start_daemon, git-daemon is started as a background process. In\ntheory, the tests may try to connect before the daemon had a chance\nto open a listening socket. Avoid this race condition by waiting\nfor it to output \"Ready to rumble\". Any other output is considered\nan error and the test is aborted.\n\nShould git-daemon produce no output at all, lib-git-daemon would\nblock forever. This could be fixed by introducing a timeout. On\nthe other hand, we have no timeout for other git commands which\ncould suffer from the same problem. Since such a mechanism adds\nsome complexity, I have decided against it.\n\nSigned-off-by: Clemens Buchacher \n---\n t/lib-git-daemon.sh | 18 +++++++++++++++++-\n 1 files changed, 17 insertions(+), 1 deletions(-)\n\ndiff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh\nindex 5e81a25..ef2d01f 100644\n--- a/t/lib-git-daemon.sh\n+++ b/t/lib-git-daemon.sh\n@@ -23,12 +23,27 @@ start_git_daemon() {\n \ttrap 'code=$?; stop_git_daemon; (exit $code); die' EXIT\n \n \tsay >&3 \"Starting git daemon ...\"\n+\tmkfifo git_daemon_output\n \tgit daemon --listen=127.0.0.1 --port=\"$LIB_GIT_DAEMON_PORT\" \\\n \t\t--reuseaddr --verbose \\\n \t\t--base-path=\"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n \t\t\"$@\" \"$GIT_DAEMON_DOCUMENT_ROOT_PATH\" \\\n-\t\t>&3 2>&4 &\n+\t\t>&3 2>git_daemon_output &\n \tGIT_DAEMON_PID=$!\n+\t{\n+\t\tread line\n+\t\techo >&4 \"$line\"\n+\t\tcat >&4 &\n+\n+\t\t# Check expected output\n+\t\tif test x\"$(expr \"$line\" : \"\\[[0-9]*\\] \\(.*\\)\")\" != x\"Ready to rumble\"\n+\t\tthen\n+\t\t\tkill \"$GIT_DAEMON_PID\"\n+\t\t\twait \"$GIT_DAEMON_PID\"\n+\t\t\ttrap 'die' EXIT\n+\t\t\terror \"git daemon failed to start\"\n+\t\tfi\n+\t} > \n> > We could probably add a \"--notify-when-ready\" option to git-daemon to\n> > do something similar.\n> \n> What would git-daemon do what it is ready? Write to socket, raise signal,\n> print to STDOUT / STDERR?\n\nPlease have a look at my \"git-daemon: produce output when ready\" patch.\nAfter opening the socket, git-daemon --verbose writes \"Ready to rumble\"\nto stderr.\n"},{"id":"182067","messageId":"20120107115434.GA8568@ecki.lan","threadId":"28543","inReplyTo":"20120106223215.GA13106@sigill.intra.peff.net","subject":"[PATCH] credentials: unable to connect to cache daemon","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-07T11:54:36Z","receivedAt":"2012-01-07T11:54:36Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Error out if we just spawned the daemon and yet we cannot connect.\n\nAnd always release the string buffer.\n\nSigned-off-by: Clemens Buchacher \n---\n\nHi Jeff,\n\nI wrote this while debugging why t0301-credential-cache.sh failed after\nI enabled cleanup_children by default. This error condition turned out\nnot to be the problem, and this patch would not have helped in debugging\nthis case. But I think it makes sense anyways.\n\n credential-cache.c | 7 +++----\n 1 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/credential-cache.c b/credential-cache.c\nindex dc98372..8f25c06 100644\n--- a/credential-cache.c\n+++ b/credential-cache.c\n@@ -71,11 +71,10 @@ static void do_cache(const char *socket, const char *action, int timeout,\n \t\t\tdie_errno(\"unable to relay credential\");\n \t}\n \n-\tif (!send_request(socket, &buf))\n-\t\treturn;\n-\tif (flags & FLAG_SPAWN) {\n+\tif (send_request(socket, &buf) < 0 && flags & FLAG_SPAWN) {\n \t\tspawn_daemon(socket);\n-\t\tsend_request(socket, &buf);\n+\t\tif (send_request(socket, &buf) < 0)\n+\t\t\tdie_errno(\"unable to connect to cache daemon\");\n \t}\n \tstrbuf_release(&buf);\n }\n-- \n1.7.8\n"},{"id":"182068","messageId":"CABPQNSb57LA6dYJvT7xF_vFfBFqKhCMbrQYp49_Ko1WmbUnYPw@mail.gmail.com","threadId":"28543","inReplyTo":"1325936567-3136-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/5] run-command: optionally kill children on exit","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-01-07T12:45:03Z","receivedAt":"2012-01-07T12:45:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Sat, Jan 7, 2012 at 12:42 PM, Clemens Buchacher wrote:\n> +static void cleanup_children(int sig)\n> +{\n> +       while (children_to_clean) {\n> +               struct child_to_clean *p = children_to_clean;\n> +               children_to_clean = p->next;\n> +               kill(p->pid, sig);\n> +               free(p);\n> +       }\n> +}\n> +\n> +static void cleanup_children_on_signal(int sig)\n> +{\n> +       cleanup_children(sig);\n> +       sigchain_pop(sig);\n> +       raise(sig);\n> +}\n> +\n\nOur Windows implementation of kill (mingw_kill in compat/mingw.c) only\nsupports SIGKILL, so propagating other signals to child-processes will\nfail with EINVAL. That being said, Windows' support for signals is\nseverely limited, but I'm not entirely sure which ones can be\ngenerated in this case.\n\n> @@ -312,6 +375,7 @@ fail_pipe:\n>                cmd->pid = -1;\n>        }\n>        close(notify_pipe[0]);\n> +\n>  }\n>  #else\n>  {\n\nThis hunk is probably unintentional...\n"},{"id":"182071","messageId":"20120107144157.GA2461@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1325936567-3136-2-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 1/5] run-command: optionally kill children on exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-07T14:41:57Z","receivedAt":"2012-01-07T14:41:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 07, 2012 at 12:42:43PM +0100, Clemens Buchacher wrote:\n\n> Signed-off-by: Clemens Buchacher \n> ---\n> \n> Not sure if I can sign off without your sign-off. Should I have\n> replaced this with Acked-by?\n\nSorry, I usually sign-off when I sent to the list. But:\n\nSigned-off-by: Jeff King \n\nfor this and the other patch in this series.\n\nAs for whether you can sign-off, I think it is OK in this case. You are\nbasically signing off on the \"Certificate of Origin\" found in\nSubmittingPatches. I think you are covered under (b), which is that to\nthe best of your knowledge it is based on open source work (i.e., even\nthough I didn't sign off explicitly, it is pretty obvious that this is\nmeant to be open source). But it's nicer to be explicit.\n\n-Peff\n"},{"id":"182073","messageId":"20120107145004.GB2461@sigill.intra.peff.net","threadId":"28543","inReplyTo":"1325936567-3136-3-git-send-email-drizzd@aon.at","subject":"Re: [PATCH 2/5] run-command: kill children on exit by default","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-07T14:50:04Z","receivedAt":"2012-01-07T14:50:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:\n\n> It feels natural for a user to view git commands as monolithic\n> commands with a single thread of execution. If the parent git\n> command dies, it should therefore clean up its child processes as\n> well. So enable the cleanup mechanism by default.\n\nI'm not sure this is a good idea. run_command is used in ~70 places in\ngit, and I'm sure at least one of them is going to be unhappy (I see you\nfound one in credential-cache, but how many others are there). I'd\nrather be conservative and leave the default the same, and then switch\nover callsites that make sense.\n\n-Peff\n\nPS I thought this would certainly break the pager, since it should\n outlast us after we finish producing output. But I think at one point\n I switched the pager invocation so that the git wrapper lives and\n waits until the pager dies.\n"},{"id":"182074","messageId":"20120107145538.GC2461@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120107115434.GA8568@ecki.lan","subject":"Re: [PATCH] credentials: unable to connect to cache daemon","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-07T14:55:38Z","receivedAt":"2012-01-07T14:55:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 07, 2012 at 12:54:36PM +0100, Clemens Buchacher wrote:\n\n> Error out if we just spawned the daemon and yet we cannot connect.\n\nActually it was intentional not to produce an error. The cache helper is\njust a cache, so I consider it \"best effort\", and if it cannot cache a\npassword, it's not the end of the world. Git should continue, anyway.\n\nThat being said, it's probably nicer to be informative in this case than\nnot, since it is a configuration error the user probably would like to\nfix. And since the rewrite of the credential helper API, it's OK for\nhelpers to return a failing exit code; git will just ignore it and keep\ngoing.\n\nSo I think this is a reasonable thing to do.\n\nAcked-by: Jeff King \n\n> And always release the string buffer.\n\nOops, thanks.\n\n-Peff\n"},{"id":"182088","messageId":"7v4nw6hfpy.fsf@alter.siamese.dyndns.org","threadId":"28543","inReplyTo":"20120107145004.GB2461@sigill.intra.peff.net","subject":"Re: [PATCH 2/5] run-command: kill children on exit by default","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-08T06:26:49Z","receivedAt":"2012-01-08T06:26:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Jan 07, 2012 at 12:42:44PM +0100, Clemens Buchacher wrote:\n>\n>> It feels natural for a user to view git commands as monolithic\n>> commands with a single thread of execution. If the parent git\n>> command dies, it should therefore clean up its child processes as\n>> well. So enable the cleanup mechanism by default.\n>\n> I'm not sure this is a good idea. run_command is used in ~70 places in\n> git, and I'm sure at least one of them is going to be unhappy (I see you\n> found one in credential-cache, but how many others are there). I'd\n> rather be conservative and leave the default the same, and then switch\n> over callsites that make sense.\n\nYeah, I agree 100% with that reasoning. I seem to recall that was how this\ncommit was done in what I privately reviewed after Clemens announced his\ngithub branch?\n"},{"id":"182126","messageId":"20120108204109.GA3394@ecki.lan","threadId":"28543","inReplyTo":"7v4nw6hfpy.fsf@alter.siamese.dyndns.org","subject":"[PATCH 2/5 v2] dashed externals: kill children on exit","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-08T20:41:09Z","receivedAt":"2012-01-08T20:41:09Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Several git commands are so-called dashed externals, that is commands\nexecuted as a child process of the git wrapper command. If the git\nwrapper is killed by a signal, the child process will continue to run.\nThis is different from internal commands, which always die with the git\nwrapper command.\n\nEnable the recently introduced cleanup mechanism for child processes in\norder to make dashed externals act more in line with internal commands.\n\nSigned-off-by: Clemens Buchacher \n---\n\nOn Sat, Jan 07, 2012 at 10:26:49PM -0800, Junio C Hamano wrote:\n> \n> Yeah, I agree 100% with that reasoning. I seem to recall that was how this\n> commit was done in what I privately reviewed after Clemens announced his\n> github branch?\n\nWhat I had previously enabled child cleanup for all callers of\nrun_command_v_opt. There is quite a few of those as well, most of them\nnot related to dashed externals at all.\n\nSo I reworked that patch a bit to enable cleanup only for dashed\nexternals. This is a replacement for \"[PATCH 2/5] run-command: kill\nchildren on exit by default\".\n\nI also re-added Jeff's \"send-pack: kill pack-objects helper on signal or\nexit\" and I dropped the extraneous newline that Erik spotted in\n\"[PATCH 1/5] run-command: optionally kill children on exit\".\n\nI have pushed the reworked series to cb/git-daemon-tests on my github\nrepo.\n\n git.c | 2 +-\n run-command.c | 1 +\n run-command.h | 1 +\n 3 files changed, 3 insertions(+), 1 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex fb9029c..3805616 100644\n--- a/git.c\n+++ b/git.c\n@@ -495,7 +495,7 @@ static void execv_dashed_external(const char **argv)\n \t * if we fail because the command is not found, it is\n \t * OK to return. Otherwise, we just pass along the status code.\n \t */\n-\tstatus = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE);\n+\tstatus = run_command_v_opt(argv, RUN_SILENT_EXEC_FAILURE | RUN_CLEAN_ON_EXIT);\n \tif (status >= 0 || errno != ENOENT)\n \t\texit(status);\n \ndiff --git a/run-command.c b/run-command.c\nindex fff9073..90bfd8c 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -496,6 +496,7 @@ static void prepare_run_command_v_opt(struct child_process *cmd,\n \tcmd->stdout_to_stderr = opt & RUN_COMMAND_STDOUT_TO_STDERR ? 1 : 0;\n \tcmd->silent_exec_failure = opt & RUN_SILENT_EXEC_FAILURE ? 1 : 0;\n \tcmd->use_shell = opt & RUN_USING_SHELL ? 1 : 0;\n+\tcmd->clean_on_exit = opt & RUN_CLEAN_ON_EXIT ? 1 : 0;\n }\n \n int run_command_v_opt(const char **argv, int opt)\ndiff --git a/run-command.h b/run-command.h\nindex 2a69466..44f7d2b 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -53,6 +53,7 @@ extern int run_hook(const char *index_file, const char *name, ...);\n #define RUN_COMMAND_STDOUT_TO_STDERR 4\n #define RUN_SILENT_EXEC_FAILURE 8\n #define RUN_USING_SHELL 16\n+#define RUN_CLEAN_ON_EXIT 32\n int run_command_v_opt(const char **argv, int opt);\n \n /*\n-- \n1.7.8\n"},{"id":"182127","messageId":"20120108205657.GB3394@ecki.lan","threadId":"28543","inReplyTo":"CABPQNSb57LA6dYJvT7xF_vFfBFqKhCMbrQYp49_Ko1WmbUnYPw@mail.gmail.com","subject":"Re: [PATCH 1/5] run-command: optionally kill children on exit","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-01-08T20:56:58Z","receivedAt":"2012-01-08T20:56:58Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sat, Jan 07, 2012 at 01:45:03PM +0100, Erik Faye-Lund wrote:\n> \n> Our Windows implementation of kill (mingw_kill in compat/mingw.c) only\n> supports SIGKILL, so propagating other signals to child-processes will\n> fail with EINVAL. That being said, Windows' support for signals is\n> severely limited, but I'm not entirely sure which ones can be\n> generated in this case.\n\nOn Linux at least, SIGKILL is not a viable alternative for SIGTERM,\nsince it does not give the child process to do any cleanup of its own\n(such as signaling its own children, for example).\n\nIn any case, due this whole experience, and recently another one with\noverzealous virus scanners, I have added a \"get rid of dashed externals\"\nwork item to my TODO list.\n"},{"id":"182128","messageId":"20120108210739.GA18311@sigill.intra.peff.net","threadId":"28543","inReplyTo":"20120108204109.GA3394@ecki.lan","subject":"Re: [PATCH 2/5 v2] dashed externals: kill children on exit","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-08T21:07:39Z","receivedAt":"2012-01-08T21:07:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 08, 2012 at 09:41:09PM +0100, Clemens Buchacher wrote:\n\n> What I had previously enabled child cleanup for all callers of\n> run_command_v_opt. There is quite a few of those as well, most of them\n> not related to dashed externals at all.\n> \n> So I reworked that patch a bit to enable cleanup only for dashed\n> externals. This is a replacement for \"[PATCH 2/5] run-command: kill\n> children on exit by default\".\n\nThanks, this looks much closer to what I was expecting.\n\n-Peff\n"}]}