Eric Sunshine [off-list ref] writes:
On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano [off-list ref] wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
ACK!
Did you write a buggy caller that would have been caught or helped
with this change? You did not write the callee that is made more
defensive with this patch, so I am being curious as to where that
Ack is coming from (I wouldn't have felt curious if this were
a reviewed-by instead).
The code being made more defensive with this patch was authored by Dscho[1].
[1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)
Ah, OK. The original by Peff done long time ago didn't check three
fds separately, but just did a single check_auto_color() implicitly
only for the standard output.
Come to think of it, would want_color_fd(0, var) ever make sense?
In any case, thanks for unconfusing me.
On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote:
Eric Sunshine [off-list ref] writes:
quoted
On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano [off-list ref] wrote:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
ACK!
Did you write a buggy caller that would have been caught or helped
with this change? You did not write the callee that is made more
defensive with this patch, so I am being curious as to where that
Ack is coming from (I wouldn't have felt curious if this were
a reviewed-by instead).
The code being made more defensive with this patch was authored by Dscho[1].
[1]: 295d949cfa (color: introduce support for colorizing stderr, 2018-04-21)
Ah, OK. The original by Peff done long time ago didn't check three
fds separately, but just did a single check_auto_color() implicitly
only for the standard output.
Right. Technically Eric's check could go into the "if (...AUTO)"
conditional, since that was what was touched in 295d949cfa. But I doubt
it matters for performance (and if it did, we should probably be
coalescing all of these conditionals into a single cached int flag).
Come to think of it, would want_color_fd(0, var) ever make sense?
No, it's just there because of 0-indexing. The BUG() check could
actually be "if (fd < 1 || ...)" to cover that (it "works", but it is
nonsensical).
Or we could even use "fd - 1" as the index. But it is probably not worth
the effort.
-Peff