Re: [PATCH 2/5] refs.c: write updates to packed refs when a transaction has more than one ref

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Jul 30, 2014, 19:19 UTC
Message-ID: <CAL=YDWn60wSnY3cKBDgZYdCnNDzyF9VyJ72CbBcHSi7kM2uY2A@mail.gmail.com>
In-Reply-To: <xmqq4mxz3m8q.fsf@gitster.dls.corp.google.com>

On Tue, Jul 29, 2014 at 2:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 36 quoted lines

> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> +     /*
>> +      * Always copy loose refs that are to be deleted to the packed refs.
>> +      * If we are updating multiple refs then copy all non symref refs
>> +      * to the packed refs too.
>> +      */
>>       for (i = 0; i < n; i++) {
>>               struct ref_update *update = updates[i];
>>               unsigned char sha1[20];
>> +             int flag;
>>
>>               if (update->update_type != UPDATE_SHA1)
>>                       continue;
>> -             if (!is_null_sha1(update->new_sha1))
>> +             if (num_updates < 2 && !is_null_sha1(update->new_sha1))
>>                       continue;
>>               if (get_packed_ref(update->refname))
>>                       continue;
>>               if (!resolve_ref_unsafe(update->refname, sha1,
>> -                                     RESOLVE_REF_READING, NULL))
>> +                                     RESOLVE_REF_READING, &flag))
>> +                     continue;
>> +             if (flag & REF_ISSYMREF)
>>                       continue;
>
> This skipping of symref didn't use to be here.
>
> Is this an enhancement needed to implement the new "feature"
> (i.e. use packed refs to represent multi-update as an atomic
> operation)?  It looks to me more like an unrelated "oops, forgot
> that we cannot use packed refs to store symrefs" fix-up, unless no
> caller passed symref in updates[] in the original code, and now we
> have callers that do.
>
> Puzzled...

It was mainly as an enhancement. Prior to this patch we were only using the packed-refs trick for the delete+rename operation part of a rename_ref() call. And for that case we already explicitly test for and disallow symbolic refs already in rename_ref().

I added this just for extra safety for "now that we possibly delete multiple refs in one transaction, should I special case/disallow the packed refs trick for symrefs?" Looking at it again I don't think we need this special casing and it only makes the code more complex and confusing. I will review the use of this a little and run some tests and if all looks sane I will remove this ISSYMREF special casing.

Thank! ronnie sahlberg