Re: [PATCH] Revert Stop starting pager recursively
On Mon, 28 April 2014 10:14:05 -0700, Junio C Hamano wrote: Matthieu Moy matthieu@grenoble-inp.fr writes: - Original Message - On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote: The intent of the commit was that is a stupid thing to do, but it's not so obvious from the first glance, do not freeze my system for my mistake. But if it stops an actual use case, then I agree it should be reverted. Thanks for the explanation. I think we should just go with Jörn's patch as-is, then. Agreed. At best, the commit message could be improved to explain the situation, but the patch itself is OK. True and I agree. The patch needs sign-off, though (I am looking at $gmane/246644). Signed-off-by: Joern Engel jo...@logfs.org Or do you want me to resend with sob? Jörn -- Tough times don't last, but tough people do. -- Nigerian proverb -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
Jörn Engel jo...@logfs.org writes:
On Mon, 28 April 2014 10:14:05 -0700, Junio C Hamano wrote:
Matthieu Moy matthieu@grenoble-inp.fr writes:
- Original Message -
On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:
The intent of the commit was that is a stupid thing to do, but it's
not so obvious from the first glance, do not freeze my system for my
mistake. But if it stops an actual use case, then I agree it should
be reverted.
Thanks for the explanation. I think we should just go with Jörn's patch
as-is, then.
Agreed. At best, the commit message could be improved to explain the
situation, but the patch itself is OK.
True and I agree.
The patch needs sign-off, though (I am looking at $gmane/246644).
Signed-off-by: Joern Engel jo...@logfs.org
Or do you want me to resend with sob?
Just the Sign-off is trivial enough that even this brainless
patch-monkey^Wpanda should be able to handle. The part The log
message could be improved is something you may be better equipped
to, though.
I'll tentatively queue this; if you or anybody in the thread can
proofread it to typofix it or give me a better phrasing, that would
be very much appreciated.
Thanks.
-- 8 --
From: Jörn Engel jo...@logfs.org
Date: Mon, 21 Apr 2014 16:46:22 -0400
Subject: [PATCH] pager: do allow spawning pager recursively
This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84, which
tried to allow
GIT_PAGER=git -p column --mode='dense color' git -p branch
and still wanted to avoid git -p column to invoke itself. However,
this falls into don't do that -p then category.
In particular, inside git log, with results going through less, a
potentially interesting commit may be found and from there inside
less, the user may want to execute git show commit. Before
the commit being reverted, this used to show the patch in less but
it no longer does.
Signed-off-by: Jörn Engel jo...@logfs.org
Reviewed-by: Jeff King p...@peff.net
Reviewed-by: Matthieu Moy matthieu@imag.fr
Acked-by: Duy Nguyen pclo...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
pager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pager.c b/pager.c
index 0cc75a8..53670a6 100644
--- a/pager.c
+++ b/pager.c
@@ -64,7 +64,7 @@ void setup_pager(void)
{
const char *pager = git_pager(isatty(1));
- if (!pager || pager_in_use())
+ if (!pager)
return;
/*
--
2.0.0-rc1-219-gf8dda7a
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
On Mon, 28 April 2014 16:04:28 -0700, Junio C Hamano wrote: Just the Sign-off is trivial enough that even this brainless patch-monkey^Wpanda should be able to handle. The part The log message could be improved is something you may be better equipped to, though. Looks good to me. The next bamboo is on the house. Jörn -- Maintenance in other professions and of other articles is concerned with the return of the item to its original state; in Software, maintenance is concerned with moving an item away from its original state. -- Les Belady -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote: The intent of the commit was that is a stupid thing to do, but it's not so obvious from the first glance, do not freeze my system for my mistake. But if it stops an actual use case, then I agree it should be reverted. Thanks for the explanation. I think we should just go with Jörn's patch as-is, then. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
- Original Message - On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote: The intent of the commit was that is a stupid thing to do, but it's not so obvious from the first glance, do not freeze my system for my mistake. But if it stops an actual use case, then I agree it should be reverted. Thanks for the explanation. I think we should just go with Jörn's patch as-is, then. Agreed. At best, the commit message could be improved to explain the situation, but the patch itself is OK. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
[+cc Duy, whose patch this is] On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote: A second option is to add a --pager (or rather --no-pager) option to the command line and allow the user to specify GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch I think we have --no-pager already. But the -p is turning _on_ the pager, so you could also just omit it. IOW, I really don't understand why the original command was not simply: GIT_PAGER=git column --mode='dense color' git -p branch The whole infinite loop that the original commit solved is caused by specifying the -p. So it sounds like the right solution is don't do that. Am I missing something useful that the -p does? I wonder if perhaps the intent was that the user might have set pager.column, in which case the use of the pager is implied. I still think that the right solution is to use --no-pager explicitly then. If the user is invoking git inside GIT_PAGER, it is up to them to save themselves from infinite recursion. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
Jeff King p...@peff.net writes: [+cc Duy, whose patch this is] On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote: A second option is to add a --pager (or rather --no-pager) option to the command line and allow the user to specify GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch I think we have --no-pager already. But the -p is turning _on_ the pager, so you could also just omit it. IOW, I really don't understand why the original command was not simply: GIT_PAGER=git column --mode='dense color' git -p branch The whole infinite loop that the original commit solved is caused by specifying the -p. So it sounds like the right solution is don't do that. Am I missing something useful that the -p does? My reading of this is that Duy wanted to let -p kick in to pass the columnized output, which could be more than a page long, thru the usual less or whatever pager. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
[+cc Duy for real this time] On Sat, Apr 26, 2014 at 10:27:07AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: [+cc Duy, whose patch this is] On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote: A second option is to add a --pager (or rather --no-pager) option to the command line and allow the user to specify GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch I think we have --no-pager already. But the -p is turning _on_ the pager, so you could also just omit it. IOW, I really don't understand why the original command was not simply: GIT_PAGER=git column --mode='dense color' git -p branch The whole infinite loop that the original commit solved is caused by specifying the -p. So it sounds like the right solution is don't do that. Am I missing something useful that the -p does? My reading of this is that Duy wanted to let -p kick in to pass the columnized output, which could be more than a page long, thru the usual less or whatever pager. I thought that at first, too. But it doesn't work, because his patch actually _suppresses_ the pager. For example, something like: git config pager.column less git config pager.branch git column --mode=dense git branch actively works without the original patch (and after Jörn's revert), but not with current git. However, you would not use -p there in any case, because it kicks in early and only respects $GIT_PAGER. And I would also say that you are much better off there actually specifying your whole pipeline as a unit: git config pager.branch git column --mode=dense | less I do a similar thing where I have PAGER=less, but set pager.log to diff-highlight | less. I tried at one point to have it chain automatically, but the setup is just too complicated (as we're seeing here), and it's not _that_ big a deal to explicitly pipe to the next pager in the sequence. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
On Sat, Apr 26, 2014 at 2:13 PM, Jeff King p...@peff.net wrote: [+cc Duy, whose patch this is] On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote: A second option is to add a --pager (or rather --no-pager) option to the command line and allow the user to specify GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch I think we have --no-pager already. But the -p is turning _on_ the pager, so you could also just omit it. IOW, I really don't understand why the original command was not simply: GIT_PAGER=git column --mode='dense color' git -p branch The whole infinite loop that the original commit solved is caused by specifying the -p. So it sounds like the right solution is don't do that. Am I missing something useful that the -p does? The intent of the commit was that is a stupid thing to do, but it's not so obvious from the first glance, do not freeze my system for my mistake. But if it stops an actual use case, then I agree it should be reverted. I wonder if perhaps the intent was that the user might have set pager.column, in which case the use of the pager is implied. I still think that the right solution is to use --no-pager explicitly then. If the user is invoking git inside GIT_PAGER, it is up to them to save themselves from infinite recursion. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote: This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84. Caused a usability regression for me and foul language for my coworkers. Ping. Jörn -- People really ought to be forced to read their code aloud over the phone. That would rapidly improve the choice of identifiers. -- Al Viro -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
Jörn Engel jo...@logfs.org writes: On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote: This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84. Caused a usability regression for me and foul language for my coworkers. Ping. How do you solve the problem that the commit you revert was solving? The commit you propose to revert says in its message: git-column can be used as a pager for other git commands, something like this: GIT_PAGER=git -p column --mode='dense color' git -p branch The problem with this is that git -p column also has $GIT_PAGER set so the pager runs itself again as another pager. The end result is an infinite loop of forking. There's probably a solution, but you can't ignore the problem (or someone else will later try to solve the infinite loop and revert your commit, and so on ...). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert Stop starting pager recursively
On Fri, 25 April 2014 20:49:52 +0200, Matthieu Moy wrote: Jörn Engel jo...@logfs.org writes: On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote: This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84. Caused a usability regression for me and foul language for my coworkers. Ping. How do you solve the problem that the commit you revert was solving? The commit you propose to revert says in its message: git-column can be used as a pager for other git commands, something like this: GIT_PAGER=git -p column --mode='dense color' git -p branch The problem with this is that git -p column also has $GIT_PAGER set so the pager runs itself again as another pager. The end result is an infinite loop of forking. There's probably a solution, but you can't ignore the problem (or someone else will later try to solve the infinite loop and revert your commit, and so on ...). Disclaimer: I never looked at git internals before this regression forced me to and am likely talking out of my arse. One approach is don't do that then. Someone explicitly changed the git pager to be git, which itself takes the git pager, etc. That is asking for infinite recursion and the original problem was that git gave the user exactly what they asked for. A second option is to add a --pager (or rather --no-pager) option to the command line and allow the user to specify GIT_PAGER=git --no-pager -p column --mode='dense color' git -p branch A third option is to try to be smart and give the user what he wants, not what he asked for. If the pager happens to be git, unset $GIT_PAGER, $PAGER and somehow disable core.pager. Yeah, that will turn nasty rather quickly. A fourth option is to set an environment variable for the pager process itself. Disable paging similar to the original patch, but make it conditional on we_are_the_pager(), not pager_in_use(). My preference is option four, but see disclaimer above. Jörn -- I've never met a human being who would want to read 17,000 pages of documentation, and if there was, I'd kill him to get him out of the gene pool. -- Joseph Costello -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Revert Stop starting pager recursively
This reverts commit 88e8f908f2b0c56f9ccf8134d8ff9f689af9cc84.
Caused a usability regression for me and foul language for my coworkers.
In particular, I commonly do a git log, with results going through
less, find a potentially interesting commit and execute git show
commit from less. This used to show the patch in less and no longer
does.
Manually typing git show commit | less works, but is annoying,
wasting time and usually only done after seeing the mess from the simple
invocation.
---
pager.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/pager.c b/pager.c
index 0cc75a8eee32..53670a63a7ae 100644
--- a/pager.c
+++ b/pager.c
@@ -64,7 +64,7 @@ void setup_pager(void)
{
const char *pager = git_pager(isatty(1));
- if (!pager || pager_in_use())
+ if (!pager)
return;
/*
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
