Hi Junio,
On 7 Aug 2024, at 17:40, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index aa52d9be7c7..5ed69c23f74 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -245,9 +245,11 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
>> {
>> struct object_id oid;
>> int flag;
>> -
>> - if (!refs_resolve_ref_unsafe(&refs->base, refname, RESOLVE_REF_READING,
>
> Here, we had a nice blank line that separated the decls and the
> first statement.
>
>> - &oid, &flag)) {
>> + const char *referent = refs_resolve_ref_unsafe(&refs->base,
>> + refname,
>> + RESOLVE_REF_READING,
>> + &oid, &flag);
>> + if (!referent) {
>
> We lost it here, though.
Ah will restore the blank line.
>
>> oidclr(&oid, the_repository->hash_algo);
>> flag |= REF_ISBROKEN;
>> } else if (is_null_oid(&oid)) {
>> @@ -268,7 +270,11 @@ static void loose_fill_ref_dir_regular_file(struct files_ref_store *refs,
>> oidclr(&oid, the_repository->hash_algo);
>> flag |= REF_BAD_NAME | REF_ISBROKEN;
>> }
>> - add_entry_to_dir(dir, create_ref_entry(refname, &oid, flag));
>> +
>> + if (!(flag & REF_ISSYMREF))
>> + referent = NULL;
>
> OK, this is new in this round. The idea is that everybody else can
> rely on the invariant that the referent being NULL is equivalent to
> REF_ISSYMREF bit in flag word being off from here on.
>
>> + add_entry_to_dir(dir, create_ref_entry(refname, referent, &oid, flag));
>> }
>>
>> /*
>> @@ -886,6 +892,11 @@ static int files_ref_iterator_advance(struct ref_iterator *ref_iterator)
>> iter->base.refname = iter->iter0->refname;
>> iter->base.oid = iter->iter0->oid;
>> iter->base.flags = iter->iter0->flags;
>> + if (iter->iter0->flags & REF_ISSYMREF)
>> + iter->base.referent = iter->iter0->referent;
>> + else
>> + iter->base.referent = NULL;
>> return ITER_OK;
>> }
>
> Hmph, why not an unconditional
>
> iter->base.referent = iter->iter0->referent;
>
> instead? This code is making sure (iter->base.flags & REF_ISSYMREF)
> is directly linked to non-NULL-ness or iter->base.referent, and we
> want to make everybody take it as invariant. Shouldn't this code
> also rely on the same invariant? If iter-iter0->referent is NULL,
> iter->iter0->flag has REF_ISSYMREF bit off, and vice versa.
That's a good point. Even though this code will be used in a loop, if we just
set it every time unconditionally then we won't end up with leftover values. Wil
l adjust in the next version.
