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

From: "Martin Ågren" <martin.agren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>,
	"brian m . carlson" <sandals@crustytoothpaste.net>
Subject: Re: [PATCH 1/3] setup: drop return value from `read_repository_format()`
Date: Wed, 19 Dec 2018 22:42:12 +0100	[thread overview]
Message-ID: <CAN0heSpfksXsKZrFNSaO_qitAB6gbF7QePQkuN37kAerSe2xWA@mail.gmail.com> (raw)
In-Reply-To: <20181219152735.GA14802@sigill.intra.peff.net>

On Wed, 19 Dec 2018 at 16:27, Jeff King <peff@peff.net> 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