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

["Martin Ågren" <martin.agren@gmail.com>]

On Tue, 22 Jan 2019 at 08:07, Jeff King <peff@peff.net> wrote:
>
> 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.

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

> 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