Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`

From: Martin Ågren <martin.agren@gmail.com>
Date: Dec 19, 2018, 21:42 UTC
Message-ID: <CAN0heSpfksXsKZrFNSaO_qitAB6gbF7QePQkuN37kAerSe2xWA@mail.gmail.com>
In-Reply-To: <20181219152735.GA14802@sigill.intra.peff.net>

On Wed, 19 Dec 2018 at 16:27, Jeff King <peff@peff.net> wrote:

Show 14 quoted lines

>
> 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]

Show 5 quoted lines

> 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