Re: [PATCH] fix color.pager = false with "git diff"

Jeff King [off-list ref] writes:

The color code makes a decision early on about whether to
use colors based on the config and whether we are using a
pager. For the most part, this works, because if we are
using a pager, we will start it more or less immediately.

In the case of diff, however, we delay starting the pager in
case --exit-code is being used. If this happens, then the
color code makes the wrong decision (because it doesn't
yet realize we are using a pager), and we need to correct
the decision after deciding whether to use a pager.

Signed-off-by: Jeff King <redacted>
---
Original discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/89599

I have mixed feelings on this one. It's kind of a hack. A more elegant
solution would be totally rewriting the color code to check for the
pager at first output.

In favor of this patch:

  1. It fixes a real bug.

  2. Perfect is the enemy of the good, and I don't care enough about
     this case to refactor the color code.

Hmm, with "[color] pager = false", what should

    $ git diff --color

do?

On Thu, Oct 21, 2010 at 03:53:36PM -0700, Junio C Hamano wrote:

quoted

Original discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/89599

I have mixed feelings on this one. It's kind of a hack. A more elegant
solution would be totally rewriting the color code to check for the
pager at first output.

In favor of this patch:

  1. It fixes a real bug.

  2. Perfect is the enemy of the good, and I don't care enough about
     this case to refactor the color code.

Hmm, with "[color] pager = false", what should

    $ git diff --color

do?

Ugh. It should definitely turn color on, which my patch breaks. And
there's currently no way to know at that point in the code whether the
COLOR_DIFF bit came from --color or from the config.

So we would need to start keeping that information through the
callchain, or we need to refactor the color code to push the color
decision until later (probably first use). The latter is the right way,
I think, but it's going to take some surgery.

Either way, let's scrap my patch. The bug it introduces is just as bad
as the bug it fixes.

Thanks for noticing.

-Peff