On 06/23/2017 09:58 PM, Jeff King wrote: > 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? That's a good idea. While I'm at it, I'll distinguish between unterminated lines and lines that are invalid for other reasons, because the error message should differ for these cases. > I assume this isn't something you saw in the wild, but just a deficiency > you noticed while reading the code. Correct, I've never seen this problem in the wild. Though, since Git would have covered up the problem while it existed and obliterated it at the next `pack-refs`, it's the kind of thing that would be easy to overlook and hard to prove afterwards. > 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). That's an interesting consideration. I suppose we could plan in some way to extend the `packed-refs` format in a backwards-transparent fashion, if there is ever an extension that old versions of git would be free to ignore, while leaving the format strict enough that genuine corruption would be unlikely to be overlooked. For example, we could reserve one or more special characters which, when they appear at the beginning of a line, mean that the line should be ignored. But such an extension would only be practical if other Git implementations are similarly lax. But libgit2 is currently strict about rejecting unrecognized lines, so such an extension couldn't be backwards-compatible with old versions of libgit2. So it doesn't seem very useful. Michael
