Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`

From: Martin Ågren <martin.agren@gmail.com>
Date: Jan 22, 2019, 13:34 UTC
Message-ID: <CAN0heSq0Nb-WdhDFpdwgjUMrkJNbviAtietn=B5nJg-rDgcR_g@mail.gmail.com>
In-Reply-To: <20190122070725.GA28555@sigill.intra.peff.net>

On Tue, 22 Jan 2019 at 08:07, Jeff King <peff@peff.net> wrote:

Show 10 quoted lines

>
> On Thu, Jan 17, 2019 at 07:31:14AM +0100, Martin Ågren wrote:
>
> > Something like the below on top of this series (then rebased). (The last
> > hunk below is a revert of this patch.)
>
> Yes, that's exactly what I had in mind. Usually our clear() functions
> put the struct back into some default state from which it can be used
> gain. But the state after clear() here (without the patch below) is
> something that nobody is ever expected to look at.

Show 7 quoted lines

> > So in particular, why doesn't `clear...()` and the error path in
> > `read_...()` impose sane, usable defaults? My first concern is that it
> > means we need to make a stronger promise, which might then be hard to
> > back away from, if we want to. Maybe we'll never want to...
>
> I'm not too worried about that personally. I think the more likely
> problem is that the API is misunderstood and misused. ;)

Heh. Agreed. :-)

> Now if your next question is: "does any caller misuse this as more than
> looking at the repo format", I don't know the answer for sure. That
> would be worth poking at (or perhaps having just poked yourself, you
> might have an idea already).

Not really. I've stumbled around a little, but I'll need to do that some more.

Show 5 quoted lines

> For the record, I can live with it either way. There are so many funky
> little setup corner cases in the code already, and we don't even really
> have a real-world case to dissect at this point. So the right thing may
> also just be to finish this patch series as quickly as possible and move
> on to something more useful. :)

I rebased the "something like this?" into this series yesterday and I think the end result is better, but also that the way there is clearer, mostly because this patch is then gone. I wanted to double-check it tonight and submit it. I'll do that tonight.

Thank you for your comments. They're really helpful.

Martin