Re: [PATCH v2 00/29] Create a reference backend for packed refs

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:

quoted

* 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