On 03/20/2017 07:05 PM, Jeff King wrote:
> On Mon, Mar 20, 2017 at 05:33:23PM +0100, Michael Haggerty wrote:
>
>> -/*
>> - * An each_ref_entry_fn that writes the entry to a packed-refs file.
>> - */
>> -static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)
>> -{
>> - enum peel_status peel_status = peel_entry(entry, 0);
>> -
>> - if (peel_status != PEEL_PEELED && peel_status != PEEL_NON_TAG)
>> - error("internal error: %s is not a valid packed reference!",
>> - entry->name);
>> - write_packed_entry(cb_data, entry->name, entry->u.value.oid.hash,
>> - peel_status == PEEL_PEELED ?
>> - entry->u.value.peeled.hash : NULL);
>> - return 0;
>> -}
>
> This assertion goes away. It can't be moved into write_packed_entry()
> because the peel status is only known in the caller.
>
> But here:
>
>> @@ -1376,8 +1362,18 @@ static int commit_packed_refs(struct files_ref_store
>> *refs)
>> die_errno("unable to fdopen packed-refs descriptor");
>>
>> fprintf_or_die(out, "%s", PACKED_REFS_HEADER);
>> - do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache),
>> - write_packed_entry_fn, out);
>> +
>> + iter = cache_ref_iterator_begin(packed_ref_cache->cache, NULL, 0);
>> + while ((ok = ref_iterator_advance(iter)) == ITER_OK) {
>> + struct object_id peeled;
>> + int peel_error = ref_iterator_peel(iter, &peeled);
>> +
>> + write_packed_entry(out, iter->refname, iter->oid->hash,
>> + peel_error ? NULL : peeled.hash);
>> + }
>
> Should we be checking that peel_error is only PEELED or NON_TAG?
This is a good question, and it took me a while to figure out the whole
answer. At first I deleted this check without much thought because it
was just an internal consistency check that should never trigger. But
actually the story is more complicated than that.
Tl;dr: the old code is slightly wrong and I think the new code is correct.
First the superficial answer: we can't `peel_error` in
`commit_packed_refs()` as you suggested, because `ref_iterator_peel()`
doesn't return an `enum peel_status`. It only returns 0 on OK / nonzero
for problems. A legitimate reference should never have a status
`PEEL_INVALID`. That status is meant for stale packed refs that are
hidden by more recent loose refs, which *can* legitimately point at
objects that have since been GCed. Since `ref_iterator_peel()` is
someday meant to be an exposed part of the API, I didn't want it to give
out more information than pass/fail [1]. Also, the reason for a peel
failure is not necessarily known accurately (information is lost when a
reference is packed then the packed-refs file is read).
So, when could the old error message have been emitted? It is when there
is an entry in the packed-ref cache that is not `REF_KNOWS_PEELED`, and
when the peel is attempted, the result is `PEEL_INVALID`,
`PEEL_IS_SYMREF`, or `PEEL_BROKEN`. Since a packed ref cannot be a
symref, `PEEL_IS_SYMREF` and `PEEL_BROKEN` can be ruled out. So we're
left with `PEEL_INVALID`.
How could an entry get into the packed-refs cache without
`REF_KNOWS_PEELED`? One of the following:
* It was read from a `packed-refs` file that didn't have the
`fully-peeled` attribute. In that case, we *don't want* to emit an
error, because the broken value is presumably masked by a loose version
of the same reference (which we just don't happen to be packing this
time). The old code incorrectly emits the error message in this case. It
was probably never reported as a bug because this scenario is rare.
* It was a loose reference that was just added to the packed ref cache
by `files_packed_refs()` via `pack_if_possible_fn()` in preparation for
being packed. The latter function refuses to pack a reference for which
`entry_resolves_to_object()` returns false, and otherwise calls
`peel_entry()` itself and checks the return value. So a reference added
this way should always be `REF_KNOWS_PEELED`.
Therefore, I think it is a good thing to remove this check. I'll improve
the commit message to make this story clearer.
Michael
[1] We could change this policy, for example by only documenting the
pass/fail return value externally, while distinguishing between the
types of failure when iterating internal to the module.
