Web lists-archives.com

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




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