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