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