Re: [PATCH 0/2] fix clone guess_dir_name regression in v2.4.8

From: Jeff King <peff@peff.net>
Date: Aug 5, 2015, 21:04 UTC
Message-ID: <20150805210454.GA21134@sigill.intra.peff.net>
In-Reply-To: <xmqq8u9p4pqb.fsf@gitster.dls.corp.google.com>

On Wed, Aug 05, 2015 at 10:19:56AM -0700, Junio C Hamano wrote:

Show 13 quoted lines

> >> I think this regression is in v2.4.8, as well. We should be able to use
> >> a running "len" instead of the "end" pointer in the earlier part, and
> >> then use strip_suffix_mem later (to strip from our already-reduced
> >> length, rather than the full NUL-terminated string). Like this:
> >
> > Looks like "git clone --bare host:foo/.git" is broken, too. I've added
> > some tests to cover the recently broken cases, as well as some obvious
> > normal cases (which the patch I sent earlier break!). And as a bonus, we
> > can easily cover Patrick's root-repo problems (so people will actually
> > run the tests, unlike the stuff in t1509. :) ).
> 
> Sorry, my fault; I should have been much less trusting while queuing
> a patch like that offending one that was meant to be a no-op.

I reviewed it, too. :-/

I actually did give some thought to that while working on the fix. Why did we miss what in retrospect was a pretty obvious bug? I saw two interesting bits:

  1. From the diff context, it looked like a perfectly reasonable
     change; the shrinking of the "end" pointer happened further up
     in the function.

     So I guess the lesson is not to trust reading just the diff, and
     to really read the whole of the modified function. But that's easy
     to say in retrospect; most of the time the bits outside the context
     aren't interesting, and we can't afford to read the whole code
     base for each patch. It's a judgement call where to stop looking at
     the surrounding context of a given change (e.g., the function, the
     callers, their callers, etc).

  2. We didn't have any test coverage in this area; when I wrote even
     basic tests, it caught the problem.

     I hate to set a rule like "if you are cleaning something up, make
     sure there is decent test coverage". Lots of trivial-looking
     patches really are trivial, and it doesn't make sense to insist the
     submitter add a new battery of tests.

So I dunno. This was definitely preventable, but that is all in retrospect. Bugs will happen, and we usually catch them while cooking. The biggest pain is that this slipped through to a release, and that may just be a measure of how few people were impacted (the cases it affected were relatively obscure).

-Peff