Jeff King <peff@xxxxxxxx> writes: > On Fri, Jun 23, 2017 at 03:01:59PM -0400, Jeff King wrote: > >> On Fri, Jun 23, 2017 at 09:01:18AM +0200, Michael Haggerty wrote: >> >> > * Change patch 17 "packed_ref_store: support iteration" to always >> > iterate over the packed refs using `DO_FOR_EACH_INCLUDE_BROKEN`. >> > This switches off the check in the packed-ref iterator of whether a >> > reference is broken. This is now checked only in >> > `files_ref_iterator_advance()`, after the packed and loose >> > references have been merged together. It also saves some work. >> >> I'm curious why you prefer this solution to just removing the code >> entirely. Wouldn't it be an error to call the packed ref iterator >> without INCLUDE_BROKEN? The "entries may not be valid" thing is a >> property of the packed-refs concept itself, not a particular caller's >> view of it. > > Speculating on my own question. I guess it would prepare us for a day > when a possible ref store is to use a packed-refs _without_ loose refs. > IOW, the property is defined on packed-refs today, but a possible future > direction would be to use it by itself. But maybe I'm just making things > up. OK. In other words, it's not a packed-refs's characteristics that cruft are allowed. It's that a ref storage that is implemented as an overlay of one storage (which happens to be the loose one) on top of another (which happens to be the packed refs file) allows the latter one to have cruft if (and only if) that broken one is covered by the former one. > At any rate, I've read through the whole series and dropped a few > comments in there. Overall it looks good. If I had one complaint, it's > that the individual commits all look obviously correct but it is hard to > judge whether the bigger steps they are taking are the right thing. I > mostly have faith in you, as I know that your end goal is a good one, > and that you're very familiar with this code. But just something I > noticed while reviewing. Thanks.
