Re: [PATCH 18/20] commit_packed_refs(): use reference iteration
- From
Michael Haggerty <mhagger@alum.mit.edu>
- Date
- Mar 22, 2017, 08:42 UTC
- Message-ID
- <b49b4ae0-bcef-ce1d-62d5-f76a11e84766@alum.mit.edu>
- In-Reply-To
- <20170320180532.vxzra6tpf3t7qjks@sigill.intra.peff.net>
On 03/20/2017 07:05 PM, Jeff King wrote:
Show 40 quoted lines
> 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.