Re: [PATCH 10/15] refs.c: allow multiple reflog updates during a single transaction

From: Ronnie Sahlberg <sahlberg@google.com>
Date: Oct 28, 2014, 19:59 UTC
Message-ID: <CAL=YDWkjBf_Ch=ww6oQDp=KLnDLSe=LkjLLrDJ6Rv-k2zM7gmA@mail.gmail.com>
In-Reply-To: <xmqq4muusjkc.fsf@gitster.dls.corp.google.com>

On Thu, Oct 23, 2014 at 11:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 12 quoted lines

> Ronnie Sahlberg <sahlberg@google.com> writes:
>
>> @@ -3531,7 +3537,7 @@ struct ref_update {
>>       enum transaction_update_type update_type;
>>       unsigned char new_sha1[20];
>>       unsigned char old_sha1[20];
>> -     int flags; /* REF_NODEREF? */
>> +     int flags; /* REF_NODEREF? or private flags */
>
> Not a very informative comment, I'd have to say.  How are users of
> this API expected to avoid stepping on each others' and API
> implementation's toes?

That is an old and bitrotted comment.
I changed it to point to the canonical definition of these flags instead :
  int flags;  /* The flags to transaction_update_ref[log] are defined
                  * in refs.h
                  */

Show 10 quoted lines

>
>> @@ -3539,8 +3545,9 @@ struct ref_update {
>>
>>       /* used by reflog updates */
>>       int reflog_fd;
>> -     struct lock_file reflog_lock;
>> +     struct lock_file *reflog_lock;
>
> What is this change about?
>

We have one update entry for each line we want to write to the reflog. So for the first update to a reflog we allocate a lock_file structure. For any subsequent reflog entry to the same ref we just let the pointer point to the previous structure we already allocated.

I.e. a way to have only one lock_file structure and share it across multiple struct ref_update structures.

> Does the lifetime rule for "struct lock_file" described in
> Documentation/technical/api-lockfile.txt, namely, "once you call
> hold_lock_file_* family on it, you cannot free it yourself", have
> any implication on this?

Nope.

Show 9 quoted lines

>
>> +     if (!(update->flags & UPDATE_REFLOG_NOLOCK))
>> +             update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
>> +
>
> Hmph, does this mean that the caller needs to keep track of the refs
> it ever touched inside a single transaction, call this without nolock
> on the first invocation on a particular ref and with nolock on the
> subsequent invocation?

Nope. This is not visible to the caller and is managed transparently inside the transaction code.

>
> Or is the "caller" just implementation detail of the API and higher level
> callers do not have to care?

The latter,    the higher level do not have to care.

regards ronnie sahlberg