{"thread":{"id":"27913","subject":"[PATCH/RFC] Pro-Git thanks, Control-flow bug report","startedAt":"2011-07-25T12:50:38Z","lastAt":"2011-07-25T19:49:48Z","messageCount":4,"participants":["Steffen Daode Nurpmeso","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"171986","messageId":"20110725125037.GA24198@sherwood.local","threadId":"27913","inReplyTo":null,"subject":"[PATCH/RFC] Pro-Git thanks, Control-flow bug report","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-07-25T12:50:38Z","receivedAt":"2011-07-25T12:50:38Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"Hello git(1),\n\nfirst of all i have to say thanks to the guy who brought up\nPro-Git on this list a few weeks ago!\n*Thank you, man*!  I love this book, and i *adore* chapter 9!\nBeep beep actually beeping software!  Yes!!\n(I hope you do this with adorable software only ...\nSuch a beep.  Beep.)\n\nSo while exploring git(1) i recently tried out colours (it's oh\nso coloured for a two, since 2011 three colors vim(1) user -\nfascinating) and found a control flow bug:\n\n  ?0%0[steffen@sherwood git.git]$ ./git --version\n  git version 1.7.6.233.gd79bc.dirty\n  ?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false diff 2> AU; cat AU\n  git_config_colorbool(color.ui,auto,-1)\n    [pager_in_use(): spawned:0, GIT_PAGER_IN_USE:0]\n    auto_color:1\n    color acc. 2 getenv(TERM)\n  git_default_config(color.pager,false): 0\n\nSo the pager is spawned after the color config setting has been\nqueried (and the latter is never updated).\nI'm not aware of the codebase, and so i can't offer a patch,\nunfortunately.  I tried the following change in color.c first,\nbut that's not a solution for the real problem:\n\n  jauto_color:\n\tif (pager_in_use())\n\t\tstdout_is_tty = pager_use_color;\n\telse if (stdout_is_tty < 0)\n\t\tstdout_is_tty = isatty(1);\n\tif (stdout_is_tty) {\n\t\t...\n\n--Steffen\nCiao, sdaoden(*)(gmail.com)\nASCII ribbon campaign           ( ) More nuclear fission plants\n  against HTML e-mail            X    can serve more coloured\n    and proprietary attachments / \\     and sounding animations\n"},{"id":"171993","messageId":"20110725162548.GA7071@sigill.intra.peff.net","threadId":"27913","inReplyTo":"20110725125037.GA24198@sherwood.local","subject":"Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-25T16:25:49Z","receivedAt":"2011-07-25T16:25:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 25, 2011 at 02:50:38PM +0200, Steffen Daode Nurpmeso wrote:\n\n> So while exploring git(1) i recently tried out colours (it's oh\n> so coloured for a two, since 2011 three colors vim(1) user -\n> fascinating) and found a control flow bug:\n> \n>   ?0%0[steffen@sherwood git.git]$ ./git --version\n>   git version 1.7.6.233.gd79bc.dirty\n>   ?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false diff 2> AU; cat AU\n>   git_config_colorbool(color.ui,auto,-1)\n>     [pager_in_use(): spawned:0, GIT_PAGER_IN_USE:0]\n>     auto_color:1\n>     color acc. 2 getenv(TERM)\n>   git_default_config(color.pager,false): 0\n> \n> So the pager is spawned after the color config setting has been\n> queried (and the latter is never updated).\n\nHmm. What's old is new again, I guess. I posted a patch to fix this\nalmost exactly 3 years ago:\n\n  http://article.gmane.org/gmane.comp.version-control.git/90427\n\nThe patch is kind of an ugly special-case, and nobody else brought it up\nin the past 3 years, so it just got dropped. Maybe it's worth taking it\nnow.\n\n> I'm not aware of the codebase, and so i can't offer a patch,\n> unfortunately.  I tried the following change in color.c first,\n> but that's not a solution for the real problem:\n> \n>   jauto_color:\n> \tif (pager_in_use())\n> \t\tstdout_is_tty = pager_use_color;\n> \telse if (stdout_is_tty < 0)\n> \t\tstdout_is_tty = isatty(1);\n> \tif (stdout_is_tty) {\n> \t\t...\n\nYou can't fix it strictly through color.c. The problem is that diff asks\nthe color code about using colors, _then_ starts a pager. So the color\ncode doesn't have enough information at the time it is asked to make the\nright decision.\n\nA more elegant solution would be to push the query to color.c to happen\nat the time of color use, instead of during the startup sequence.\n\n-Peff\n"},{"id":"172004","messageId":"20110725193941.GA95001@sherwood.local","threadId":"27913","inReplyTo":"20110725162548.GA7071@sigill.intra.peff.net","subject":"Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report","fromName":"Steffen Daode Nurpmeso","fromEmail":"sdaoden@googlemail.com","sentAt":"2011-07-25T19:39:41Z","receivedAt":"2011-07-25T19:39:41Z","isPatch":true,"sender":{"key":"sdaoden@googlemail.com","avatar":null},"body":"@ Jeff King <peff@peff.net> wrote (2011-07-25 18:25+0200):\n> Hmm. What's old is new again, I guess. I posted a patch to fix this\n> almost exactly 3 years ago:\n> \n>   http://article.gmane.org/gmane.comp.version-control.git/90427\n\nUnfortunately your patch from then seems no longer be sufficient\n(i.e., from my point of view, say), since this is also coloured:\n\n  ?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false log\n\n>..> It's an ordering problem. [.]\n>..> However, breaking the dependency chain would require some pretty\n>..> major surgery, I think. [.]\n>..> I think the \"right\" solution would be refactoring the color stuff\n>..> to make the decision closer to the point of use. [.]\n\n> A more elegant solution would be to push the query to color.c to\n> happen at the time of color use, instead of during the startup\n> sequence.\n\nBeginners luck.  In the end git(1) will get a visual.c output\nmanager, and sideband.c rid of that TERM pollution.  :-)\n\n--Steffen\nCiao, sdaoden(*)(gmail.com)\nASCII ribbon campaign           ( ) More nuclear fission plants\n  against HTML e-mail            X    can serve more coloured\n    and proprietary attachments / \\     and sounding animations\n"},{"id":"172007","messageId":"20110725194947.GA14659@sigill.intra.peff.net","threadId":"27913","inReplyTo":"20110725193941.GA95001@sherwood.local","subject":"Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-07-25T19:49:48Z","receivedAt":"2011-07-25T19:49:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 25, 2011 at 09:39:41PM +0200, Steffen Daode Nurpmeso wrote:\n\n> @ Jeff King <peff@peff.net> wrote (2011-07-25 18:25+0200):\n> > Hmm. What's old is new again, I guess. I posted a patch to fix this\n> > almost exactly 3 years ago:\n> > \n> >   http://article.gmane.org/gmane.comp.version-control.git/90427\n> \n> Unfortunately your patch from then seems no longer be sufficient\n> (i.e., from my point of view, say), since this is also coloured:\n> \n>   ?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false log\n\nYeah, it only covers the \"diff\" case. And that is why the special-casing\nis a bad solution: you have to do it everywhere that you do late pager\nsetup. (Side note: at the time the original patch was written, log\nactually didn't have this problem; it was introduced much later by\n1fda91b). I wouldn't be surprised if there are others.\n\nI think my patch also has the problem that:\n\n  git -c color.ui=auto -c color.pager=false diff --color\n\nwill not properly override the config.\n\nSo probably the only sane solution is to push the \"do we want color\" bit\ninto a function, rather than keeping a static variable, and then call it\nat the last possible minute.\n\n-Peff\n"}]}