{"thread":{"id":"36466","subject":"[PATCH] Revert \"Stop starting pager recursively\"","startedAt":"2014-04-21T20:46:22Z","lastAt":"2014-04-28T23:39:15Z","messageCount":13,"participants":["Jörn Engel","Matthieu Moy","Jeff King","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"239241","messageId":"20140421204622.GA9532@logfs.org","threadId":"36466","inReplyTo":null,"subject":"[PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jörn Engel","fromEmail":"joern@logfs.org","sentAt":"2014-04-21T20:46:22Z","receivedAt":"2014-04-21T20:46:22Z","isPatch":true,"sender":{"key":"joern@logfs.org","avatar":null},"body":"This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.\n\nCaused a usability regression for me and foul language for my coworkers.\n\nIn particular, I commonly do a \"git log\", with results going through\nless, find a potentially interesting commit and execute \"git show\n<commit>\" from less.  This used to show the patch in less and no longer\ndoes.\n\nManually typing \"git show <commit> | less\" works, but is annoying,\nwasting time and usually only done after seeing the mess from the simple\ninvocation.\n---\n pager.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pager.c b/pager.c\nindex 0cc75a8eee32..53670a63a7ae 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -64,7 +64,7 @@ void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n \n-\tif (!pager || pager_in_use())\n+\tif (!pager)\n \t\treturn;\n \n \t/*\n-- \n1.9.1\n"},{"id":"239687","messageId":"20140425182928.GA29904@logfs.org","threadId":"36466","inReplyTo":"20140421204622.GA9532@logfs.org","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jörn Engel","fromEmail":"joern@logfs.org","sentAt":"2014-04-25T18:29:29Z","receivedAt":"2014-04-25T18:29:29Z","isPatch":true,"sender":{"key":"joern@logfs.org","avatar":null},"body":"On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:\n> \n> This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.\n> \n> Caused a usability regression for me and foul language for my coworkers.\n\nPing.\n\nJörn\n\n--\nPeople really ought to be forced to read their code aloud over the phone.\nThat would rapidly improve the choice of identifiers.\n-- Al Viro\n"},{"id":"239690","messageId":"vpqoazpdz1r.fsf@anie.imag.fr","threadId":"36466","inReplyTo":"20140425182928.GA29904@logfs.org","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-25T18:49:52Z","receivedAt":"2014-04-25T18:49:52Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jörn Engel <joern@logfs.org> writes:\n\n> On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:\n>> \n>> This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.\n>> \n>> Caused a usability regression for me and foul language for my coworkers.\n>\n> Ping.\n\nHow do you solve the problem that the commit you revert was solving? The\ncommit you propose to revert says in its message:\n\n    git-column can be used as a pager for other git commands, something\n    like this:\n    \n        GIT_PAGER=\"git -p column --mode='dense color'\" git -p branch\n    \n    The problem with this is that \"git -p column\" also has $GIT_PAGER set so\n    the pager runs itself again as another pager. The end result is an\n    infinite loop of forking.\n\nThere's probably a solution, but you can't ignore the problem (or\nsomeone else will later try to solve the infinite loop and revert your\ncommit, and so on ...).\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"239708","messageId":"20140425201048.GB29904@logfs.org","threadId":"36466","inReplyTo":"vpqoazpdz1r.fsf@anie.imag.fr","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jörn Engel","fromEmail":"joern@logfs.org","sentAt":"2014-04-25T20:10:49Z","receivedAt":"2014-04-25T20:10:49Z","isPatch":true,"sender":{"key":"joern@logfs.org","avatar":null},"body":"On Fri, 25 April 2014 20:49:52 +0200, Matthieu Moy wrote:\n> Jörn Engel <joern@logfs.org> writes:\n> > On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:\n> >> \n> >> This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.\n> >> \n> >> Caused a usability regression for me and foul language for my coworkers.\n> >\n> > Ping.\n> \n> How do you solve the problem that the commit you revert was solving? The\n> commit you propose to revert says in its message:\n> \n>     git-column can be used as a pager for other git commands, something\n>     like this:\n>     \n>         GIT_PAGER=\"git -p column --mode='dense color'\" git -p branch\n>     \n>     The problem with this is that \"git -p column\" also has $GIT_PAGER set so\n>     the pager runs itself again as another pager. The end result is an\n>     infinite loop of forking.\n> \n> There's probably a solution, but you can't ignore the problem (or\n> someone else will later try to solve the infinite loop and revert your\n> commit, and so on ...).\n\nDisclaimer: I never looked at git internals before this regression\nforced me to and am likely talking out of my arse.\n\nOne approach is \"don't do that then\".  Someone explicitly changed the\ngit pager to be git, which itself takes the git pager, etc.  That is\nasking for infinite recursion and the original problem was that git\ngave the user exactly what they asked for.\n\nA second option is to add a --pager (or rather --no-pager) option to\nthe command line and allow the user to specify\n    GIT_PAGER=\"git --no-pager -p column --mode='dense color'\" git -p branch\n\nA third option is to try to be smart and give the user what he wants,\nnot what he asked for.  If the pager happens to be git, unset\n$GIT_PAGER, $PAGER and somehow disable core.pager.  Yeah, that will\nturn nasty rather quickly.\n\nA fourth option is to set an environment variable for the pager\nprocess itself.  Disable paging similar to the original patch, but\nmake it conditional on we_are_the_pager(), not pager_in_use().\n\nMy preference is option four, but see disclaimer above.\n\nJörn\n\n--\nI've never met a human being who would want to read 17,000 pages of\ndocumentation, and if there was, I'd kill him to get him out of the\ngene pool.\n-- Joseph Costello\n"},{"id":"239744","messageId":"20140426071358.GA7558@sigill.intra.peff.net","threadId":"36466","inReplyTo":"20140425201048.GB29904@logfs.org","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-26T07:13:58Z","receivedAt":"2014-04-26T07:13:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Duy, whose patch this is]\n\nOn Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote:\n\n> A second option is to add a --pager (or rather --no-pager) option to\n> the command line and allow the user to specify\n>     GIT_PAGER=\"git --no-pager -p column --mode='dense color'\" git -p branch\n\nI think we have \"--no-pager\" already. But the \"-p\" is turning _on_ the\npager, so you could also just omit it. IOW, I really don't understand\nwhy the original command was not simply:\n\n  GIT_PAGER=\"git column --mode='dense color'\" git -p branch\n\nThe whole infinite loop that the original commit solved is caused by\nspecifying the \"-p\". So it sounds like the right solution is \"don't do\nthat\". Am I missing something useful that the \"-p\" does?\n\nI wonder if perhaps the intent was that the user might have set\n\"pager.column\", in which case the use of the pager is implied. I still\nthink that the right solution is to use \"--no-pager\" explicitly then. If\nthe user is invoking git inside GIT_PAGER, it is up to them to save\nthemselves from infinite recursion.\n\n-Peff\n"},{"id":"239761","messageId":"7vtx9ghuhg.fsf@alter.siamese.dyndns.org","threadId":"36466","inReplyTo":"20140426071358.GA7558@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-26T17:27:07Z","receivedAt":"2014-04-26T17:27:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [+cc Duy, whose patch this is]\n>\n> On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote:\n>\n>> A second option is to add a --pager (or rather --no-pager) option to\n>> the command line and allow the user to specify\n>>     GIT_PAGER=\"git --no-pager -p column --mode='dense color'\" git -p branch\n>\n> I think we have \"--no-pager\" already. But the \"-p\" is turning _on_ the\n> pager, so you could also just omit it. IOW, I really don't understand\n> why the original command was not simply:\n>\n>   GIT_PAGER=\"git column --mode='dense color'\" git -p branch\n>\n> The whole infinite loop that the original commit solved is caused by\n> specifying the \"-p\". So it sounds like the right solution is \"don't do\n> that\". Am I missing something useful that the \"-p\" does?\n\nMy reading of this is that Duy wanted to let \"-p\" kick in to pass\nthe columnized output, which could be more than a page long, thru\nthe usual \"less\" or whatever pager.\n"},{"id":"239764","messageId":"20140426173549.GB21493@sigill.intra.peff.net","threadId":"36466","inReplyTo":"7vtx9ghuhg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-26T17:35:49Z","receivedAt":"2014-04-26T17:35:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Duy for real this time]\n\nOn Sat, Apr 26, 2014 at 10:27:07AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > [+cc Duy, whose patch this is]\n> >\n> > On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote:\n> >\n> >> A second option is to add a --pager (or rather --no-pager) option to\n> >> the command line and allow the user to specify\n> >>     GIT_PAGER=\"git --no-pager -p column --mode='dense color'\" git -p branch\n> >\n> > I think we have \"--no-pager\" already. But the \"-p\" is turning _on_ the\n> > pager, so you could also just omit it. IOW, I really don't understand\n> > why the original command was not simply:\n> >\n> >   GIT_PAGER=\"git column --mode='dense color'\" git -p branch\n> >\n> > The whole infinite loop that the original commit solved is caused by\n> > specifying the \"-p\". So it sounds like the right solution is \"don't do\n> > that\". Am I missing something useful that the \"-p\" does?\n> \n> My reading of this is that Duy wanted to let \"-p\" kick in to pass\n> the columnized output, which could be more than a page long, thru\n> the usual \"less\" or whatever pager.\n\nI thought that at first, too. But it doesn't work, because his patch\nactually _suppresses_ the pager. For example, something like:\n\n  git config pager.column less\n  git config pager.branch \"git column --mode=dense\"\n  git branch\n\nactively works without the original patch (and after Jörn's revert), but\nnot with current git. However, you would not use \"-p\" there in any case,\nbecause it kicks in early and only respects $GIT_PAGER.\n\nAnd I would also say that you are much better off there actually\nspecifying your whole pipeline as a unit:\n\n  git config pager.branch \"git column --mode=dense | less\"\n\nI do a similar thing where I have PAGER=less, but set pager.log to\n\"diff-highlight | less\". I tried at one point to have it chain\nautomatically, but the setup is just too complicated (as we're seeing\nhere), and it's not _that_ big a deal to explicitly pipe to the next\npager in the sequence.\n\n-Peff\n"},{"id":"239786","messageId":"CACsJy8ANE+2n6fd-mF5pp+p2sXy_uRzJ2c+O=AiySyre+6KLtg@mail.gmail.com","threadId":"36466","inReplyTo":"20140426071358.GA7558@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-04-27T02:12:39Z","receivedAt":"2014-04-27T02:12:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 26, 2014 at 2:13 PM, Jeff King <peff@peff.net> wrote:\n> [+cc Duy, whose patch this is]\n>\n> On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote:\n>\n>> A second option is to add a --pager (or rather --no-pager) option to\n>> the command line and allow the user to specify\n>>     GIT_PAGER=\"git --no-pager -p column --mode='dense color'\" git -p branch\n>\n> I think we have \"--no-pager\" already. But the \"-p\" is turning _on_ the\n> pager, so you could also just omit it. IOW, I really don't understand\n> why the original command was not simply:\n>\n>   GIT_PAGER=\"git column --mode='dense color'\" git -p branch\n>\n> The whole infinite loop that the original commit solved is caused by\n> specifying the \"-p\". So it sounds like the right solution is \"don't do\n> that\". Am I missing something useful that the \"-p\" does?\n\nThe intent of the commit was \"that is a stupid thing to do, but it's\nnot so obvious from the first glance, do not freeze my system for my\nmistake\". But if it stops an actual use case, then I agree it should\nbe reverted.\n\n> I wonder if perhaps the intent was that the user might have set\n> \"pager.column\", in which case the use of the pager is implied. I still\n> think that the right solution is to use \"--no-pager\" explicitly then. If\n> the user is invoking git inside GIT_PAGER, it is up to them to save\n> themselves from infinite recursion.\n--\nDuy\n"},{"id":"239791","messageId":"20140427075640.GB19976@sigill.intra.peff.net","threadId":"36466","inReplyTo":"CACsJy8ANE+2n6fd-mF5pp+p2sXy_uRzJ2c+O=AiySyre+6KLtg@mail.gmail.com","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-04-27T07:56:40Z","receivedAt":"2014-04-27T07:56:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:\n\n> The intent of the commit was \"that is a stupid thing to do, but it's\n> not so obvious from the first glance, do not freeze my system for my\n> mistake\". But if it stops an actual use case, then I agree it should\n> be reverted.\n\nThanks for the explanation. I think we should just go with Jörn's patch\nas-is, then.\n\n-Peff\n"},{"id":"239800","messageId":"985432164.425366.1398611186431.JavaMail.zimbra@imag.fr","threadId":"36466","inReplyTo":"20140427075640.GB19976@sigill.intra.peff.net","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2014-04-27T15:06:26Z","receivedAt":"2014-04-27T15:06:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"----- Original Message -----\n> On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:\n> \n> > The intent of the commit was \"that is a stupid thing to do, but it's\n> > not so obvious from the first glance, do not freeze my system for my\n> > mistake\". But if it stops an actual use case, then I agree it should\n> > be reverted.\n> \n> Thanks for the explanation. I think we should just go with Jörn's patch\n> as-is, then.\n\nAgreed. At best, the commit message could be improved to explain the\nsituation, but the patch itself is OK.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"239997","messageId":"20140428183244.GA6358@logfs.org","threadId":"36466","inReplyTo":"xmqqk3a9s7fm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jörn Engel","fromEmail":"joern@logfs.org","sentAt":"2014-04-28T18:32:44Z","receivedAt":"2014-04-28T18:32:44Z","isPatch":true,"sender":{"key":"joern@logfs.org","avatar":null},"body":"On Mon, 28 April 2014 10:14:05 -0700, Junio C Hamano wrote:\n> Matthieu Moy <matthieu.moy@grenoble-inp.fr> writes:\n> \n> > ----- Original Message -----\n> >> On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:\n> >> \n> >> > The intent of the commit was \"that is a stupid thing to do, but it's\n> >> > not so obvious from the first glance, do not freeze my system for my\n> >> > mistake\". But if it stops an actual use case, then I agree it should\n> >> > be reverted.\n> >> \n> >> Thanks for the explanation. I think we should just go with Jörn's patch\n> >> as-is, then.\n> >\n> > Agreed. At best, the commit message could be improved to explain the\n> > situation, but the patch itself is OK.\n> \n> True and I agree.\n> \n> The patch needs sign-off, though (I am looking at $gmane/246644).\n\nSigned-off-by: Joern Engel <joern@logfs.org>\n\nOr do you want me to resend with sob?\n\nJörn\n\n--\nTough times don't last, but tough people do.\n-- Nigerian proverb\n"},{"id":"240086","messageId":"xmqqsioxm4xv.fsf@gitster.dls.corp.google.com","threadId":"36466","inReplyTo":"20140428183244.GA6358@logfs.org","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-28T23:04:28Z","receivedAt":"2014-04-28T23:04:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jörn Engel <joern@logfs.org> writes:\n\n> On Mon, 28 April 2014 10:14:05 -0700, Junio C Hamano wrote:\n>> Matthieu Moy <matthieu.moy@grenoble-inp.fr> writes:\n>> \n>> > ----- Original Message -----\n>> >> On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:\n>> >> \n>> >> > The intent of the commit was \"that is a stupid thing to do, but it's\n>> >> > not so obvious from the first glance, do not freeze my system for my\n>> >> > mistake\". But if it stops an actual use case, then I agree it should\n>> >> > be reverted.\n>> >> \n>> >> Thanks for the explanation. I think we should just go with Jörn's patch\n>> >> as-is, then.\n>> >\n>> > Agreed. At best, the commit message could be improved to explain the\n>> > situation, but the patch itself is OK.\n>> \n>> True and I agree.\n>> \n>> The patch needs sign-off, though (I am looking at $gmane/246644).\n>\n> Signed-off-by: Joern Engel <joern@logfs.org>\n>\n> Or do you want me to resend with sob?\n\nJust the Sign-off is trivial enough that even this brainless\npatch-monkey^Wpanda should be able to handle.  The part \"The log\nmessage could be improved\" is something you may be better equipped\nto, though.\n\nI'll tentatively queue this; if you or anybody in the thread can\nproofread it to typofix it or give me a better phrasing, that would\nbe very much appreciated.\n\nThanks.\n\n-- >8 --\nFrom: Jörn Engel <joern@logfs.org>\nDate: Mon, 21 Apr 2014 16:46:22 -0400\nSubject: [PATCH] pager: do allow spawning pager recursively\n\nThis reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84, which\ntried to allow\n\n    GIT_PAGER=\"git -p column --mode='dense color'\" git -p branch\n\nand still wanted to avoid \"git -p column\" to invoke itself.  However,\nthis falls into \"don't do that -p then\" category.\n\nIn particular, inside \"git log\", with results going through less, a\npotentially interesting commit may be found and from there inside\n\"less\", the user may want to execute \"git show <commit>\".  Before\nthe commit being reverted, this used to show the patch in less but\nit no longer does.\n\nSigned-off-by: Jörn Engel <joern@logfs.org>\nReviewed-by: Jeff King <peff@peff.net>\nReviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr>\nAcked-by: Duy Nguyen <pclouds@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n pager.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pager.c b/pager.c\nindex 0cc75a8..53670a6 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -64,7 +64,7 @@ void setup_pager(void)\n {\n \tconst char *pager = git_pager(isatty(1));\n \n-\tif (!pager || pager_in_use())\n+\tif (!pager)\n \t\treturn;\n \n \t/*\n-- \n2.0.0-rc1-219-gf8dda7a\n"},{"id":"240092","messageId":"20140428233915.GB6358@logfs.org","threadId":"36466","inReplyTo":"xmqqsioxm4xv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] Revert \"Stop starting pager recursively\"","fromName":"Jörn Engel","fromEmail":"joern@logfs.org","sentAt":"2014-04-28T23:39:15Z","receivedAt":"2014-04-28T23:39:15Z","isPatch":true,"sender":{"key":"joern@logfs.org","avatar":null},"body":"On Mon, 28 April 2014 16:04:28 -0700, Junio C Hamano wrote:\n> \n> Just the Sign-off is trivial enough that even this brainless\n> patch-monkey^Wpanda should be able to handle.  The part \"The log\n> message could be improved\" is something you may be better equipped\n> to, though.\n\nLooks good to me.  The next bamboo is on the house.\n\nJörn\n\n--\nMaintenance in other professions and of other articles is concerned with\nthe return of the item to its original state; in Software, maintenance\nis concerned with moving an item away from its original state.\n-- Les Belady\n"}]}