Re: [PATCH] show-branch: fix crash with long ref name

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 16, 2017, 12:40 UTC
Message-ID: <CAP8UFD20=zVy=1Tr4D1Rxf+a4yR_X2zmexNCTNKV5RSF9ueYrg@mail.gmail.com>
In-Reply-To: <20170215214052.5py4pxkcz4g2bmtk@sigill.intra.peff.net>

On Wed, Feb 15, 2017 at 10:40 PM, Jeff King <peff@peff.net> wrote:

Show 15 quoted lines

> On Tue, Feb 14, 2017 at 10:29:46PM +0100, Christian Couder wrote:
>
>> > I notice Christian's patch added a few tests. I don't know if we'd want
>> > to squash them in (I didn't mean to override his patch at all; I was
>> > about to send mine out when I noticed his, and I wondered if we wanted
>> > to combine the two efforts).
>>
>> I think it would be nice to have at least one test. Feel free to
>> squash mine if you want.
>
> I started to add some tests, but I had second thoughts. It _is_ nice
> to show off the fix, but as far as regressions go, this specific case is
> unlikely to come up again. What would be more valuable, I think, is a
> test script which set up a very long refname (not just 150 bytes or
> whatever) and ran it through a series of git commands.

I agree that a test script running through a series of command with long refnames would be great.

But I think the refname should not necesarily be too long. As I wrote in the commit message of my patch, if the ref name had been much longer the crash would not have happened because the ref could not have been created in the first place.

So the best would be to run through a series of commands with a refname ranging from let's say 80 chars to 300 chars.

That would have a chance to catch crashes due to legacy code using for example things like `char stuff[128]` or `char stuff[256]`.

Implementing those tests could have started with something like the test case I sent, but as it would in the end be about many different commands, one can see it as part of a different topic.

> But then you run into all sorts of portability annoyances with pathname
> restrictions (you can hack around creation by writing the refname
> directly into packed-refs, but most manipulations will want to take the
> .lock in the filesystem).

Yeah, but if a crash doesn't happen because we die() as the ref is too long for the file system, we could detect that and make the test succeed.

> So I dunno. It seems like being thorough is a
> lot of hassle for not much gain. Being not-thorough is easy, but is
> mostly a token that is unlikely to find any real bugs.

Yeah, if we really care, it might be better to start using a fuzzer or a property based testing tool instead of bothering with these kind of tests by ourselves, which is also a different topic.

> So I punted, at least for now.

Ok, no problem.