On Wed, 19 Dec 2018 at 16:27, Jeff King <peff@xxxxxxxx> wrote: > > On Tue, Dec 18, 2018 at 08:25:26AM +0100, Martin Ågren wrote: > > > No-one looks at the return value, so we might as well drop it. It's > > still available as `format->version`. > > Hmm. If we have to pick one, I'd say that just returning a sane exit > value would be the more conventional thing to do. But looking at the > callers, many of them want to just pass the struct on to the verify > function. > > That said, there is a long-standing curiosity here that we may want to > consider: read_repository_format() does not distinguish between these > three cases: [snip several valuable insights] > I dunno. This is one of those dark corners of the code where we appear > to do the wrong thing, but nobody seems to have noticed or cared much, > and changing it runs the risk of breaking some obscure cases. I'm not > sure if we should bite the bullet and try to address that, or just back > away slowly and pretend we never looked at it. ;) That was my reaction when I first dug into this. :-/ It's only now that I've been brave enough to even try to dig through the first layer. > FWIW, the patch itself seems fine, and obviously doesn't make anything > worse on its own. The question is just whether we want to do more > cleanup here. Well, if we do want to make more cleanups around here, one of the more obvious ideas is to actually use the return value. If such a cleanup is going to start with a (moral) revert of this patch, then we're probably better off just dropping this patch. Thanks for your thoughtful response Martin
