On Thu, Aug 02, 2018 at 12:24:54PM -0700, Junio C Hamano wrote: > Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > > > On Thu, Aug 2, 2018 at 1:37 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > >> Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes: > >> > 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
