{"thread":{"id":"37399","subject":"[PATCH] upload-pack: keep poll(2)'s timeout to -1","startedAt":"2014-08-22T15:19:11Z","lastAt":"2014-08-22T18:26:51Z","messageCount":8,"participants":["Edward Thomson","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"248119","messageId":"20140822151911.GA8531@debian","threadId":"37399","inReplyTo":null,"subject":"[PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2014-08-22T15:19:11Z","receivedAt":"2014-08-22T15:19:11Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of\nsetting it to -1000, since some pedantic old systems (eg HP-UX) and\nthe gnulib compat/poll will treat only -1 as the valid value for\nan infinite timeout.\n\nSigned-off-by: Edward Thomson \n---\n upload-pack.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 01de944..433211a 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -167,7 +167,9 @@ static void create_pack_file(void)\n \t\tif (!pollsize)\n \t\t\tbreak;\n \n-\t\tret = poll(pfd, pollsize, 1000 * keepalive);\n+\t\tret = poll(pfd, pollsize,\n+\t\t\tkeepalive < 0 ? -1 : 1000 * keepalive);\n+\n \t\tif (ret < 0) {\n \t\t\tif (errno != EINTR) {\n \t\t\t\terror(\"poll failed, resuming: %s\",\n-- \n1.7.10.4\n"},{"id":"248121","messageId":"20140822154445.GA19135@peff.net","threadId":"37399","inReplyTo":"20140822151911.GA8531@debian","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-22T15:44:45Z","receivedAt":"2014-08-22T15:44:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 22, 2014 at 03:19:11PM +0000, Edward Thomson wrote:\n\n> Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of\n> setting it to -1000, since some pedantic old systems (eg HP-UX) and\n> the gnulib compat/poll will treat only -1 as the valid value for\n> an infinite timeout.\n\nThat makes sense, and POSIX only specifies the behavior for -1 anyway.\nThe patch itself looks obviously correct. Thanks.\n\nSince we're now translating the keepalive value, and since there's no\nway to set it to \"0\" (nor would that really have any meaning), I guess\nwe could switch the internal \"no keepalive\" value to 0, and do:\n\n ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);\n\nwhich would let us avoid setting it to -1 in some other spots. I dunno\nif that actually makes a real difference to maintainability, though.\nEither way:\n\n Acked-by: Jeff King \n\n-Peff\n"},{"id":"248122","messageId":"xmqqr408plgj.fsf@gitster.dls.corp.google.com","threadId":"37399","inReplyTo":"20140822154445.GA19135@peff.net","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T15:56:12Z","receivedAt":"2014-08-22T15:56:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since we're now translating the keepalive value, and since there's no\n> way to set it to \"0\" (nor would that really have any meaning), I guess\n> we could switch the internal \"no keepalive\" value to 0, and do:\n>\n> ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);\n>\n> which would let us avoid setting it to -1 in some other spots. I dunno\n> if that actually makes a real difference to maintainability, though.\n\nWhere we parse and set the value of the variable, we do this:\n\n\telse if (!strcmp(\"uploadpack.keepalive\", var)) {\n\t\tkeepalive = git_config_int(var, value);\n\t\tif (!keepalive)\n\t\t\tkeepalive = -1;\n\t}\n\nThe condition may have to become \"if (keepalive <= 0)\".\n\n> Either way:\n>\n> Acked-by: Jeff King \n>\n> -Peff\n\nYeah, either way, the patch as-posted is good. Thanks.\n"},{"id":"248124","messageId":"20140822160334.GA20789@peff.net","threadId":"37399","inReplyTo":"xmqqr408plgj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-08-22T16:03:34Z","receivedAt":"2014-08-22T16:03:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 22, 2014 at 08:56:12AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Since we're now translating the keepalive value, and since there's no\n> > way to set it to \"0\" (nor would that really have any meaning), I guess\n> > we could switch the internal \"no keepalive\" value to 0, and do:\n> >\n> > ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);\n> >\n> > which would let us avoid setting it to -1 in some other spots. I dunno\n> > if that actually makes a real difference to maintainability, though.\n> \n> Where we parse and set the value of the variable, we do this:\n> \n> \telse if (!strcmp(\"uploadpack.keepalive\", var)) {\n> \t\tkeepalive = git_config_int(var, value);\n> \t\tif (!keepalive)\n> \t\t\tkeepalive = -1;\n> \t}\n> \n> The condition may have to become \"if (keepalive <= 0)\".\n\nYeah, I wasn't thinking we would get negative values from the user (we\ndon't document them at all), but we should probably do something\nsensible. Let's just leave it at Ed's patch.\n\n-Peff\n"},{"id":"248126","messageId":"20140822162711.GA8598@debian","threadId":"37399","inReplyTo":"20140822160334.GA20789@peff.net","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Edward Thomson","fromEmail":"ethomson@edwardthomson.com","sentAt":"2014-08-22T16:27:11Z","receivedAt":"2014-08-22T16:27:11Z","isPatch":true,"sender":{"key":"ethomson@edwardthomson.com","avatar":"https://avatars.githubusercontent.com/u/1130014?v=4"},"body":"On Fri, Aug 22, 2014 at 12:03:34PM -0400, Jeff King wrote:\n> \n> Yeah, I wasn't thinking we would get negative values from the user (we\n> don't document them at all), but we should probably do something\n> sensible. Let's just leave it at Ed's patch.\n\nThanks, both. Apologies for the dumb question: is there anything\nadditional that I need to do (repost with your Acked-by, for example)\nor is this adequate as-is?\n\nThanks-\n-ed\n"},{"id":"248130","messageId":"xmqq1ts8peud.fsf@gitster.dls.corp.google.com","threadId":"37399","inReplyTo":"20140822154445.GA19135@peff.net","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T18:19:06Z","receivedAt":"2014-08-22T18:19:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 22, 2014 at 03:19:11PM +0000, Edward Thomson wrote:\n>\n>> Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of\n>> setting it to -1000, since some pedantic old systems (eg HP-UX) and\n>> the gnulib compat/poll will treat only -1 as the valid value for\n>> an infinite timeout.\n>\n> That makes sense, and POSIX only specifies the behavior for -1 anyway.\n> The patch itself looks obviously correct. Thanks.\n>\n> Since we're now translating the keepalive value, and since there's no\n> way to set it to \"0\" (nor would that really have any meaning), I guess\n> we could switch the internal \"no keepalive\" value to 0, and do:\n>\n> ret = poll(pfd, pollsize, keepalive ? 1000 * keepalive : -1);\n>\n> which would let us avoid setting it to -1 in some other spots. I dunno\n> if that actually makes a real difference to maintainability, though.\n> Either way:\n>\n> Acked-by: Jeff King \n>\n> -Peff\n\nThere is 1000 * wakeup in credential-cache--daemon.c, by the way.\n"},{"id":"248131","messageId":"xmqqwqa0o05g.fsf@gitster.dls.corp.google.com","threadId":"37399","inReplyTo":"xmqq1ts8peud.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T18:21:47Z","receivedAt":"2014-08-22T18:21:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> There is 1000 * wakeup in credential-cache--daemon.c, by the way.\n\nAh, nevermind. That uses an expiration computed, not some \"we can\nchoose to block indefinitely\" configuration.\n"},{"id":"248132","messageId":"xmqqsikonzx0.fsf@gitster.dls.corp.google.com","threadId":"37399","inReplyTo":"20140822162711.GA8598@debian","subject":"Re: [PATCH] upload-pack: keep poll(2)'s timeout to -1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-22T18:26:51Z","receivedAt":"2014-08-22T18:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edward Thomson writes:\n\n> On Fri, Aug 22, 2014 at 12:03:34PM -0400, Jeff King wrote:\n>> \n>> Yeah, I wasn't thinking we would get negative values from the user (we\n>> don't document them at all), but we should probably do something\n>> sensible. Let's just leave it at Ed's patch.\n>\n> Thanks, both. Apologies for the dumb question: is there anything\n> additional that I need to do (repost with your Acked-by, for example)\n> or is this adequate as-is?\n\nI've picked it up and queued it on 'pu'. Thanks.\n\ncommit 6c71f8b0d3d39beffe050f92f33a25dc30dffca3\nAuthor: Edward Thomson \nDate: Fri Aug 22 15:19:11 2014 +0000\n\n upload-pack: keep poll(2)'s timeout to -1\n \n Keep poll's timeout at -1 when uploadpack.keepalive = 0, instead of\n setting it to -1000, since some pedantic old systems (eg HP-UX) and\n the gnulib compat/poll will treat only -1 as the valid value for\n an infinite timeout.\n \n Signed-off-by: Edward Thomson \n Acked-by: Jeff King \n Signed-off-by: Junio C Hamano \n"}]}