On Mon, Jun 19, 2017 at 03:43:15PM -0400, Jeff King wrote: > > > Is the iterator over packed-refs correctly skipping over what are > > > covered by loose refs? The entries in the packed-refs file that are > > > superseded by loose refs should be allowed to point at an already > > > expired object. > > > > Here it is in a test form for easier diagnosis. > > Thanks, I was just starting to do that myself. The problem is in > ca6b06eb7 (packed_ref_store: support iteration, 2017-05-15) and is > pretty obvious: the packed_ref iterator checks whether the entry > resolves. > > I think that _neither_ of the loose and packed iterators should be > checking this. It's only the merged result (where loose trumps packed) > that should bother checking. It looks like this is mostly already the case. files_ref_iterator combines the two and does check. The loose iteration is done by cache_ref_iterator[1], and does not. So it's just this new packed_refs iterator that is wrong, and we just need: diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 311fd014c..ad1143e64 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -416,12 +416,6 @@ static int packed_ref_iterator_advance(struct ref_iterator *ref_iterator) ref_type(iter->iter0->refname) != REF_TYPE_PER_WORKTREE) continue; - if (!(iter->flags & DO_FOR_EACH_INCLUDE_BROKEN) && - !ref_resolves_to_object(iter->iter0->refname, - iter->iter0->oid, - iter->iter0->flags)) - continue; - iter->base.refname = iter->iter0->refname; iter->base.oid = iter->iter0->oid; iter->base.flags = iter->iter0->flags; @@ -473,8 +467,6 @@ static struct ref_iterator *packed_ref_iterator_begin( struct ref_iterator *ref_iterator; unsigned int required_flags = REF_STORE_READ; - if (!(flags & DO_FOR_EACH_INCLUDE_BROKEN)) - required_flags |= REF_STORE_ODB; refs = packed_downcast(ref_store, required_flags, "ref_iterator_begin"); iter = xcalloc(1, sizeof(*iter)); At least that makes sense to me and passes the tests (including the new one). I haven't actually reviewed the patches yet. -Peff
