Hello git(1),
first of all i have to say thanks to the guy who brought up
Pro-Git on this list a few weeks ago!
*Thank you, man*! I love this book, and i *adore* chapter 9!
Beep beep actually beeping software! Yes!!
(I hope you do this with adorable software only ...
Such a beep. Beep.)
So while exploring git(1) i recently tried out colours (it's oh
so coloured for a two, since 2011 three colors vim(1) user -
fascinating) and found a control flow bug:
?0%0[steffen@sherwood git.git]$ ./git --version
git version 1.7.6.233.gd79bc.dirty
?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false diff 2> AU; cat AU
git_config_colorbool(color.ui,auto,-1)
[pager_in_use(): spawned:0, GIT_PAGER_IN_USE:0]
auto_color:1
color acc. 2 getenv(TERM)
git_default_config(color.pager,false): 0
So the pager is spawned after the color config setting has been
queried (and the latter is never updated).
I'm not aware of the codebase, and so i can't offer a patch,
unfortunately. I tried the following change in color.c first,
but that's not a solution for the real problem:
jauto_color:
if (pager_in_use())
stdout_is_tty = pager_use_color;
else if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
if (stdout_is_tty) {
...
--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign ( ) More nuclear fission plants
against HTML e-mail X can serve more coloured
and proprietary attachments / \ and sounding animations
From: Jeff King <hidden> Date: 2016-06-15 22:51:39
On Mon, Jul 25, 2011 at 02:50:38PM +0200, Steffen Daode Nurpmeso wrote:
So while exploring git(1) i recently tried out colours (it's oh
so coloured for a two, since 2011 three colors vim(1) user -
fascinating) and found a control flow bug:
?0%0[steffen@sherwood git.git]$ ./git --version
git version 1.7.6.233.gd79bc.dirty
?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false diff 2> AU; cat AU
git_config_colorbool(color.ui,auto,-1)
[pager_in_use(): spawned:0, GIT_PAGER_IN_USE:0]
auto_color:1
color acc. 2 getenv(TERM)
git_default_config(color.pager,false): 0
So the pager is spawned after the color config setting has been
queried (and the latter is never updated).
Hmm. What's old is new again, I guess. I posted a patch to fix this
almost exactly 3 years ago:
http://article.gmane.org/gmane.comp.version-control.git/90427
The patch is kind of an ugly special-case, and nobody else brought it up
in the past 3 years, so it just got dropped. Maybe it's worth taking it
now.
I'm not aware of the codebase, and so i can't offer a patch,
unfortunately. I tried the following change in color.c first,
but that's not a solution for the real problem:
jauto_color:
if (pager_in_use())
stdout_is_tty = pager_use_color;
else if (stdout_is_tty < 0)
stdout_is_tty = isatty(1);
if (stdout_is_tty) {
...
You can't fix it strictly through color.c. The problem is that diff asks
the color code about using colors, _then_ starts a pager. So the color
code doesn't have enough information at the time it is asked to make the
right decision.
A more elegant solution would be to push the query to color.c to happen
at the time of color use, instead of during the startup sequence.
-Peff
Unfortunately your patch from then seems no longer be sufficient
(i.e., from my point of view, say), since this is also coloured:
?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false log
..> It's an ordering problem. [.]
..> However, breaking the dependency chain would require some pretty
..> major surgery, I think. [.]
..> I think the "right" solution would be refactoring the color stuff
..> to make the decision closer to the point of use. [.]
A more elegant solution would be to push the query to color.c to
happen at the time of color use, instead of during the startup
sequence.
Beginners luck. In the end git(1) will get a visual.c output
manager, and sideband.c rid of that TERM pollution. :-)
--Steffen
Ciao, sdaoden(*)(gmail.com)
ASCII ribbon campaign ( ) More nuclear fission plants
against HTML e-mail X can serve more coloured
and proprietary attachments / \ and sounding animations
Unfortunately your patch from then seems no longer be sufficient
(i.e., from my point of view, say), since this is also coloured:
?0%0[steffen@sherwood git.git]$ ./git -c color.ui=auto -c color.pager=false log
Yeah, it only covers the "diff" case. And that is why the special-casing
is a bad solution: you have to do it everywhere that you do late pager
setup. (Side note: at the time the original patch was written, log
actually didn't have this problem; it was introduced much later by
1fda91b). I wouldn't be surprised if there are others.
I think my patch also has the problem that:
git -c color.ui=auto -c color.pager=false diff --color
will not properly override the config.
So probably the only sane solution is to push the "do we want color" bit
into a function, rather than keeping a static variable, and then call it
at the last possible minute.
-Peff