From: Jeff King <peff@peff.net>
Date: Fri, 23 Jun 2017 19:58:46 GMT
Subject: Re: [PATCH v2 29/29] read_packed_refs(): die if `packed-refs` contains bogus data
Message-ID: <20170623195845.pz7rdyqocjqu5edp@sigill.intra.peff.net>
In-Reply-To: <1e08748aa5a6dc05003b08207a9a4bc344758ca2.1498200513.git.mhagger@alum.mit.edu>

On Fri, Jun 23, 2017 at 09:01:47AM +0200, Michael Haggerty wrote:

> The old code ignored any lines that it didn't understand. This is
> dangerous. Instead, `die()` if the `packed-refs` file contains any
> lines that we don't know how to handle.

This seems like a big improvement. Is it worth adding a test for a
corrupted file?

I assume this isn't something you saw in the wild, but just a deficiency
you noticed while reading the code.

I suspect this laxness may have been what allowed us to add the optional
peeled values long ago. But I think I'd rather see us be more strict and
notice corruption or nonsense rather than quietly ignoring (especially
because an operation like "git pack-refs" would then overwrite it,
dropping whatever entries we didn't understand).

-Peff