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
