{"thread":{"id":"30713","subject":"[PATCH] pager: drop \"wait for output to run less\" hack","startedAt":"2012-06-05T08:56:04Z","lastAt":"2012-06-05T23:09:44Z","messageCount":8,"participants":["Jeff King","Junio C Hamano","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"192882","messageId":"20120605085604.GA27298@sigill.intra.peff.net","threadId":"30713","inReplyTo":null,"subject":"[PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T08:56:04Z","receivedAt":"2012-06-05T08:56:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 02:56:28AM -0400, Jeff King wrote:\n\n> > +static int fd_to_close;\n> > +void close_fd_prexec_cb(void)\n> > +{\n> > +\tif(debug)\n> > +\t\tfprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n> > +\tclose(fd_to_close);\n> > +}\n> \n> Note that preexec_cb does not work at all on Windows, as it assumes a\n> forking model (rather than a spawn, which leaves no room to execute\n> arbitrary code in the child). If all you want to do is open an extra\n> pipe, then probably run-command should be extended to handle this\n> (though I have no idea how complex that would be for the Windows side of\n> things, it is at least _possible_, as opposed to preexec_cb, which will\n> never be possible).\n\nI'm really tempted to do this:\n\n-- >8 --\nCommit 35ce862 (pager: Work around window resizing bug in\n'less', 2007-01-24) causes git's pager sub-process to wait\nto receive input after forking but before exec-ing the\npager. To handle this, run-command had to grow a \"pre-exec\ncallback\" feature. Unfortunately, this feature does not work\nat all on Windows (where we do not fork), and interacts\npoorly with run-command's parent notification system. Its\nuse should be discouraged.\n\nThe bug in less was fixed in version 406, which was released\nin June 2007. It is probably safe at this point to remove\nour workaround. That lets us rip out the preexec_cb feature\nentirely.\n\nSigned-off-by: Jeff King \n---\nI checked, and even RHEL5 is on less 436. So besides people on antique\n\"I installed less from source more than 5 years ago\" systems, my only\nconcern would be that some other pager depends on this hack in a weird\nway. But I have never heard of such a thing, so...\n\n pager.c | 18 ------------------\n run-command.c | 10 ----------\n run-command.h | 1 -\n 3 files changed, 29 deletions(-)\n\ndiff --git a/pager.c b/pager.c\nindex 4dcb08d..c0b4387 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -11,21 +11,6 @@\n * something different on Windows.\n */\n \n-#ifndef WIN32\n-static void pager_preexec(void)\n-{\n-\t/*\n-\t * Work around bug in \"less\" by not starting it until we\n-\t * have real input\n-\t */\n-\tfd_set in;\n-\n-\tFD_ZERO(&in);\n-\tFD_SET(0, &in);\n-\tselect(1, &in, NULL, &in, NULL);\n-}\n-#endif\n-\n static const char *pager_argv[] = { NULL, NULL };\n static struct child_process pager_process;\n \n@@ -93,9 +78,6 @@ void setup_pager(void)\n \t\tstatic const char *env[] = { \"LESS=FRSX\", NULL };\n \t\tpager_process.env = env;\n \t}\n-#ifndef WIN32\n-\tpager_process.preexec_cb = pager_preexec;\n-#endif\n \tif (start_command(&pager_process))\n \t\treturn;\n \ndiff --git a/run-command.c b/run-command.c\nindex 606791d..dff28a7 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -394,16 +394,6 @@ fail_pipe:\n \t\t\t\t\tunsetenv(*cmd->env);\n \t\t\t}\n \t\t}\n-\t\tif (cmd->preexec_cb) {\n-\t\t\t/*\n-\t\t\t * We cannot predict what the pre-exec callback does.\n-\t\t\t * Forgo parent notification.\n-\t\t\t */\n-\t\t\tclose(child_notifier);\n-\t\t\tchild_notifier = -1;\n-\n-\t\t\tcmd->preexec_cb();\n-\t\t}\n \t\tif (cmd->git_cmd) {\n \t\t\texecv_git_cmd(cmd->argv);\n \t\t} else if (cmd->use_shell) {\ndiff --git a/run-command.h b/run-command.h\nindex 44f7d2b..850c638 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -39,7 +39,6 @@ struct child_process {\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 int start_command(struct child_process *);\n-- \n1.7.11.rc1.3.gf8b4f5c\n\nReply-To: \nIn-Reply-To: <20120605065628.GA25809@sigill.intra.peff.net>\n"},{"id":"192914","messageId":"7vwr3lpxyh.fsf@alter.siamese.dyndns.org","threadId":"30713","inReplyTo":"20120605085604.GA27298@sigill.intra.peff.net","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-05T15:42:14Z","receivedAt":"2012-06-05T15:42:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jun 05, 2012 at 02:56:28AM -0400, Jeff King wrote:\n>\n>> > +static int fd_to_close;\n>> > +void close_fd_prexec_cb(void)\n>> > +{\n>> > +\tif(debug)\n>> > +\t\tfprintf(stderr, \"close_fd_prexec_cb closing %d\\n\", fd_to_close);\n>> > +\tclose(fd_to_close);\n>> > +}\n>> \n>> Note that preexec_cb does not work at all on Windows, as it assumes a\n>> forking model (rather than a spawn, which leaves no room to execute\n>> arbitrary code in the child). If all you want to do is open an extra\n>> pipe, then probably run-command should be extended to handle this\n>> (though I have no idea how complex that would be for the Windows side of\n>> things, it is at least _possible_, as opposed to preexec_cb, which will\n>> never be possible).\n>\n> I'm really tempted to do this:\n\nWhy (I am slower than my usual slow self today, so pardon me)?\n\nAren't these already protected with \"ifndef WIN32\"?\n\n>\n> -- >8 --\n> Commit 35ce862 (pager: Work around window resizing bug in\n> 'less', 2007-01-24) causes git's pager sub-process to wait\n> to receive input after forking but before exec-ing the\n> pager. To handle this, run-command had to grow a \"pre-exec\n> callback\" feature. Unfortunately, this feature does not work\n> at all on Windows (where we do not fork), and interacts\n> poorly with run-command's parent notification system. Its\n> use should be discouraged.\n>\n> The bug in less was fixed in version 406, which was released\n> in June 2007. It is probably safe at this point to remove\n> our workaround. That lets us rip out the preexec_cb feature\n> entirely.\n\nAh, OK.\n\n>\n> Signed-off-by: Jeff King \n> ---\n> I checked, and even RHEL5 is on less 436. So besides people on antique\n> \"I installed less from source more than 5 years ago\" systems, my only\n> concern would be that some other pager depends on this hack in a weird\n> way. But I have never heard of such a thing, so...\n\nHrm...\n\n> pager.c | 18 ------------------\n> run-command.c | 10 ----------\n> run-command.h | 1 -\n> 3 files changed, 29 deletions(-)\n>\n> diff --git a/pager.c b/pager.c\n> index 4dcb08d..c0b4387 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -11,21 +11,6 @@\n> * something different on Windows.\n> */\n> \n> -#ifndef WIN32\n> -static void pager_preexec(void)\n> -{\n> -\t/*\n> -\t * Work around bug in \"less\" by not starting it until we\n> -\t * have real input\n> -\t */\n> -\tfd_set in;\n> -\n> -\tFD_ZERO(&in);\n> -\tFD_SET(0, &in);\n> -\tselect(1, &in, NULL, &in, NULL);\n> -}\n> -#endif\n> -\n> static const char *pager_argv[] = { NULL, NULL };\n> static struct child_process pager_process;\n> \n> @@ -93,9 +78,6 @@ void setup_pager(void)\n> \t\tstatic const char *env[] = { \"LESS=FRSX\", NULL };\n> \t\tpager_process.env = env;\n> \t}\n> -#ifndef WIN32\n> -\tpager_process.preexec_cb = pager_preexec;\n> -#endif\n> \tif (start_command(&pager_process))\n> \t\treturn;\n> \n> diff --git a/run-command.c b/run-command.c\n> index 606791d..dff28a7 100644\n> --- a/run-command.c\n> +++ b/run-command.c\n> @@ -394,16 +394,6 @@ fail_pipe:\n> \t\t\t\t\tunsetenv(*cmd->env);\n> \t\t\t}\n> \t\t}\n> -\t\tif (cmd->preexec_cb) {\n> -\t\t\t/*\n> -\t\t\t * We cannot predict what the pre-exec callback does.\n> -\t\t\t * Forgo parent notification.\n> -\t\t\t */\n> -\t\t\tclose(child_notifier);\n> -\t\t\tchild_notifier = -1;\n> -\n> -\t\t\tcmd->preexec_cb();\n> -\t\t}\n> \t\tif (cmd->git_cmd) {\n> \t\t\texecv_git_cmd(cmd->argv);\n> \t\t} else if (cmd->use_shell) {\n> diff --git a/run-command.h b/run-command.h\n> index 44f7d2b..850c638 100644\n> --- a/run-command.h\n> +++ b/run-command.h\n> @@ -39,7 +39,6 @@ struct child_process {\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> int start_command(struct child_process *);\n"},{"id":"192915","messageId":"20120605155007.GA20494@sigill.intra.peff.net","threadId":"30713","inReplyTo":"7vwr3lpxyh.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T15:50:07Z","receivedAt":"2012-06-05T15:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 08:42:14AM -0700, Junio C Hamano wrote:\n\n> > I'm really tempted to do this:\n> \n> Why (I am slower than my usual slow self today, so pardon me)?\n\nPurely to clean up the nasty preexec thing, which is a hack, and should\nnever be used for new code due to portability issues.\n\n> Aren't these already protected with \"ifndef WIN32\"?\n\nYes, but it means anything that uses it will not work on Windows.\n\n> > I checked, and even RHEL5 is on less 436. So besides people on antique\n> > \"I installed less from source more than 5 years ago\" systems, my only\n> > concern would be that some other pager depends on this hack in a weird\n> > way. But I have never heard of such a thing, so...\n> \n> Hrm...\n\nYeah. This is purely a cleanup thing. It's a cleanup I've wanted to do\nfor a long time (ever since adding it), but it is not hurting anyone\nas-is. We can always just reject new uses of preexec_cb in review (which\nis more or less what I'm doing here).\n\n-Peff\n"},{"id":"192916","messageId":"CABPQNSbhgan+i_Q142R8VvRdJ5T+GyYHqgEJ6KS-BBJguRu-OQ@mail.gmail.com","threadId":"30713","inReplyTo":"20120605085604.GA27298@sigill.intra.peff.net","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-06-05T15:52:24Z","receivedAt":"2012-06-05T15:52:24Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 5, 2012 at 10:56 AM, Jeff King wrote:\n> I checked, and even RHEL5 is on less 436. So besides people on antique\n> \"I installed less from source more than 5 years ago\" systems, my only\n> concern would be that some other pager depends on this hack in a weird\n> way. But I have never heard of such a thing, so...\n\nOn my RHEL5 box at work:\n$ less --version\nless 394\nCopyright (C) 1984-2005 Mark Nudelman\n"},{"id":"192917","messageId":"20120605160157.GA20582@sigill.intra.peff.net","threadId":"30713","inReplyTo":"CABPQNSbhgan+i_Q142R8VvRdJ5T+GyYHqgEJ6KS-BBJguRu-OQ@mail.gmail.com","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T16:01:57Z","receivedAt":"2012-06-05T16:01:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 05:52:24PM +0200, Erik Faye-Lund wrote:\n\n> On Tue, Jun 5, 2012 at 10:56 AM, Jeff King wrote:\n> > I checked, and even RHEL5 is on less 436. So besides people on antique\n> > \"I installed less from source more than 5 years ago\" systems, my only\n> > concern would be that some other pager depends on this hack in a weird\n> > way. But I have never heard of such a thing, so...\n> \n> On my RHEL5 box at work:\n> $ less --version\n> less 394\n> Copyright (C) 1984-2005 Mark Nudelman\n\nThen I think you are not following the bug-fix updates, as they've\nissued several updates based on 436:\n\n https://rhn.redhat.com/errata/RHBA-2010-0214.html\n\n https://rhn.redhat.com/errata/RHBA-2010-0805.html\n\n http://rhn.redhat.com/errata/RHBA-2011-1468.html\n\nLooks like 394 was shipping as recently as 2009:\n\n https://rhn.redhat.com/errata/RHBA-2009-0413.html\n\nGiven that the buggy less is apparently still in the wild, and that the\npatch is a pure cleanup, I guess we should scrap it for now. \n\n-Peff\n"},{"id":"192921","messageId":"7vlik1pvfv.fsf@alter.siamese.dyndns.org","threadId":"30713","inReplyTo":"20120605160157.GA20582@sigill.intra.peff.net","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-05T16:36:36Z","receivedAt":"2012-06-05T16:36:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Given that the buggy less is apparently still in the wild, and that the\n> patch is a pure cleanup, I guess we should scrap it for now. \n\nLet's keep it on hold for six more months and then re-evaluate the\nsituation, perhaps?\n"},{"id":"192924","messageId":"20120605164516.GB2694@sigill.intra.peff.net","threadId":"30713","inReplyTo":"7vlik1pvfv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-06-05T16:45:16Z","receivedAt":"2012-06-05T16:45:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 05, 2012 at 09:36:36AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Given that the buggy less is apparently still in the wild, and that the\n> > patch is a pure cleanup, I guess we should scrap it for now. \n> \n> Let's keep it on hold for six more months and then re-evaluate the\n> situation, perhaps?\n\nSure. I'll carry it in my local tree for now.\n\n-Peff\n"},{"id":"192959","messageId":"CABPQNSbk4h8bbhk92XAHVt5zVMCs3JLdjMf8P0wGD3wmyVEE1Q@mail.gmail.com","threadId":"30713","inReplyTo":"20120605160157.GA20582@sigill.intra.peff.net","subject":"Re: [PATCH] pager: drop \"wait for output to run less\" hack","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-06-05T23:09:44Z","receivedAt":"2012-06-05T23:09:44Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Jun 5, 2012 at 6:01 PM, Jeff King wrote:\n> On Tue, Jun 05, 2012 at 05:52:24PM +0200, Erik Faye-Lund wrote:\n>\n>> On Tue, Jun 5, 2012 at 10:56 AM, Jeff King wrote:\n>> > I checked, and even RHEL5 is on less 436. So besides people on antique\n>> > \"I installed less from source more than 5 years ago\" systems, my only\n>> > concern would be that some other pager depends on this hack in a weird\n>> > way. But I have never heard of such a thing, so...\n>>\n>> On my RHEL5 box at work:\n>> $ less --version\n>> less 394\n>> Copyright (C) 1984-2005 Mark Nudelman\n>\n> Then I think you are not following the bug-fix updates, as they've\n> issued several updates based on 436:\n>\n\nI don't know what exactly the setup is, other than that it's set up to\nmatch the RHEL5 setup of a major customer with a very conservative\nupgrade policy.\n"}]}