On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:
quoted
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/
From: Jörn Engel <hidden> Date: 2016-06-15 23:00:50
On Fri, 25 April 2014 20:49:52 +0200, Matthieu Moy wrote:
Jörn Engel [off-list ref] writes:
quoted
On Mon, 21 April 2014 16:46:22 -0400, Jörn Engel wrote:
quoted
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
From: Jeff King <hidden> Date: 2016-06-15 23:00:50
[+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
On Sat, Apr 26, 2014 at 2:13 PM, Jeff King [off-list ref] wrote:
[+cc Duy, whose patch this is]
On Fri, Apr 25, 2014 at 04:10:49PM -0400, Jörn Engel wrote:
quoted
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.
From: Jeff King <hidden> Date: 2016-06-15 23:00:51
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
On Sun, Apr 27, 2014 at 09:12:39AM +0700, Duy Nguyen wrote:
quoted
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/