On Wed, Aug 05, 2015 at 10:19:56AM -0700, Junio C Hamano wrote:
> >> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
