Re: [PATCH 1/2] handle color.ui at a central place
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 24, 2009, 20:26 UTC
- Message-ID
- <7vvds4movp.fsf@gitster.siamese.dyndns.org>
- In-Reply-To
- <20090124191700.GA17935@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 12 quoted lines
> 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.