RFC patchPro-Git thanks, Control-flow bug report

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

On Mon, Jul 25, 2011 at 02:50:38PM +0200, Steffen Daode Nurpmeso wrote:

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.

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

@ Jeff King <peff@peff.net> wrote (2011-07-25 18:25+0200):

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

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

On Mon, Jul 25, 2011 at 09:39:41PM +0200, Steffen Daode Nurpmeso wrote:

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