Re: [PATCH 1/2] handle color.ui at a central place

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> On Sat, Jan 24, 2009 at 10:36:24AM -0800, Junio C Hamano wrote:
> ...
>> You did not find the breakage in format-patch either to begin with; so
>> your not finding does not give us much confidence that there is no other
>> breakage, does it?
>> 
>> Grumble...
>
> Sadly, this is an area that is not covered very well in the tests
> (partially, I think, because it is "just" output which we tend to
> neglect, and partially because the isatty() stuff is hard to test with
> our harness). So I don't think it's _entirely_ Markus' fault.

Oh, don't get me wrong.  I am not interested in finding whose fault it
was.  I was just stating the fact that one person not finding a breakage
does not mean much as an assurance.

It is actually not trivial to test this breakage in our test suite.
Before committing 9383af1 (Revert previous two commits, 2009-01-23), I
spent about 20 minutes trying to come up with a test to expose the
breakage in an acceptable way.  A test that assumes that it is run with a
controlling terminal is relatively easy to write, but I couldn't come up
with a test that would have triggered even when the tests were run without
a tty (for gory details, see git_config_colorbool() and how stdout_is_tty
is used).

Here is the "relatively easy" but an unacceptable one.

diff --git c/t/t4014-format-patch.sh w/t/t4014-format-patch.sh
index 9d99dc2..609946a 100755
--- c/t/t4014-format-patch.sh
+++ w/t/t4014-format-patch.sh
@@ -255,4 +255,10 @@ test_expect_success 'format-patch respects -U' '
 
 '
 
+test_expect_success 'format-patch is colorless even with color.ui = auto' '
+	git config color.ui auto &&
+	TERM=ansi git format-patch -1 >/dev/tty &&
+	grep "^+5$" 0001-foo.patch
+'
+
 test_done

Points that makes the above patch unacceptable are:

 (1) It hardcodes 0001-foo.patch.  You could try doing these:

     (1.1) patchname=$( ... git format-patch -1) && grep ... <"$patchname"
     (1.2) git format-patch -1 --stdout >patchfile && grep ... <patchfile

     but they won't work, because "color.ui = auto" will not color unless
     the standard output is a tty, and TERM is better than "dumb".

 (2) It would not trigger if /dev/tty cannot be opened for writing.