Re: [PATCH] parse_color: fix return value for numeric color values 0-8

From: Jeff King <peff@peff.net>
Date: Jan 21, 2015, 00:03 UTC
Message-ID: <20150121000352.GA12543@peff.net>
In-Reply-To: <xmqqvbk1ypie.fsf@gitster.dls.corp.google.com>

On Tue, Jan 20, 2015 at 03:57:13PM -0800, Junio C Hamano wrote:

Show 11 quoted lines

> > -- >8 --
> > When commit 695d95d refactored the color parsing, it missed
> > a "return 0" when parsing literal numbers 0-8 (which
> > represent basic ANSI colors), leading us to report these
> > colors as an error.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> 
> Thanks; somebody should have caught this before we applied and
> merged to 'master', but the process obviously did not work well.

I am not too surprised. The use of numeric values for colors was completely undocumented, and we did not have any test coverage for it. I did not even know it existed until I started refactoring the function, and wondered what was going on (though I did try to preserve it once I found it).

So I suspect that almost nobody is using this undocumented "feature", which is why it was not caught while cooking in 'next'. The system cannot always have perfect output, but hopefully the number of people affected by a bug is proportional to the quickness with which it is caught.

-Peff

PS All that being said, I think it is a good example of why it is a good
   idea to beef up test coverage in an area before refactoring. A
   trivial test would have caught this.