Jeff King <peff@xxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
