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

[Jeff King <peff@peff.net>]

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