* [PATCH/RFC] Pro-Git thanks, Control-flow bug report
@ 2011-07-25 12:50 Steffen Daode Nurpmeso
2011-07-25 16:25 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Steffen Daode Nurpmeso @ 2011-07-25 12:50 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report
2011-07-25 12:50 [PATCH/RFC] Pro-Git thanks, Control-flow bug report Steffen Daode Nurpmeso
@ 2011-07-25 16:25 ` Jeff King
2011-07-25 19:39 ` Steffen Daode Nurpmeso
0 siblings, 1 reply; 4+ messages in thread
From: Jeff King @ 2011-07-25 16:25 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report
2011-07-25 16:25 ` Jeff King
@ 2011-07-25 19:39 ` Steffen Daode Nurpmeso
2011-07-25 19:49 ` Jeff King
0 siblings, 1 reply; 4+ messages in thread
From: Steffen Daode Nurpmeso @ 2011-07-25 19:39 UTC (permalink / raw)
To: git; +Cc: Jeff King
@ Jeff King <peff@peff.net> wrote (2011-07-25 18:25+0200):
> 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
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH/RFC] Pro-Git thanks, Control-flow bug report
2011-07-25 19:39 ` Steffen Daode Nurpmeso
@ 2011-07-25 19:49 ` Jeff King
0 siblings, 0 replies; 4+ messages in thread
From: Jeff King @ 2011-07-25 19:49 UTC (permalink / raw)
To: git
On Mon, Jul 25, 2011 at 09:39:41PM +0200, Steffen Daode Nurpmeso wrote:
> @ Jeff King <peff@peff.net> wrote (2011-07-25 18:25+0200):
> > 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
>
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-25 19:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-25 12:50 [PATCH/RFC] Pro-Git thanks, Control-flow bug report Steffen Daode Nurpmeso
2011-07-25 16:25 ` Jeff King
2011-07-25 19:39 ` Steffen Daode Nurpmeso
2011-07-25 19:49 ` Jeff King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.