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

> On Fri, May 16, 2014 at 5:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Ronnie Sahlberg <sahlberg@google.com> writes:
>>
>>> Allow to make multiple reflog updates to the same ref during a transaction.
>>> This means we only need to lock the reflog once, during the first update that
>>> touches the reflog, and that all further updates can just write the reflog
>>> entry since the reflog is already locked.
>>>
>>> This allows us to write code such as:
>>>
>>> t = transaction_begin()
>>> transaction_reflog_update(t, "foo", REFLOG_TRUNCATE, NULL);
>>> loop-over-somehting...
>>>    transaction_reflog_update(t, "foo", 0, <message>);
>>> transaction_commit(t)
>>
>> OK, so you are now doing not just "refs" but also "reflogs", you
>> felt that "ref_transaction()" does not cover the latter.  Is that
>> the reason for the rename in the earlier step?
>>
>> I am sort-of on the fence.
>>
>> Calling the begin "ref_transaction_begin" and then calling the new
>> function "ref_transaction_log_update" would still allow us to
>> differentiate transactions on refs and reflogs, while allowing other
>> kinds of transactions that are not related to refs at all to employ
>> a mechanism that is different from the one that is used to implement
>> the transactions on refs and reflogs you are building here.
>>
>> But I think I am OK with the generic "transaction-begin" now.
>> Having one mechanism for refs and reflogs, and then having another
>> completely different mechanism for other things, will not let us
>> coordinate between the two easily, so "allow transactions that are
>> not related to refs at all to be built on a different mechanism" may
>> not be a worthwhile goal to pursue in the first place.  Please
>> consider the question on the naming in the earlier one dropped.
>>
>>>
>>> where we first truncate the reflog and then build the new content one line at a
>>> time.
>>>
>>> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>
>>> ---
>>>  refs.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++---------
>>>  1 file changed, 49 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/refs.c b/refs.c
>>> index a3f60ad..e7ede03 100644
>>> --- a/refs.c
>>> +++ b/refs.c
>>> @@ -37,6 +37,10 @@ static inline int bad_ref_char(int ch)
>>>   *  need to lock the loose ref during the transaction.
>>>   */
>>>  #define REF_ISPACKONLY       0x0200
>>> +/** Only the first reflog update needs to lock the reflog file. Further updates
>>> + *  just use the lock taken by the first update.
>>> + */
>>
>> Style.
>>
>>> @@ -3349,8 +3355,23 @@ int transaction_update_reflog(struct ref_transaction *transaction,
>>>                             int flags)
>>>  {
>>>       struct ref_update *update;
>>> +     int i;
>>>
>>>       update = add_update(transaction, refname, UPDATE_LOG);
>>> +     update->flags = flags;
>>> +     for (i = 0; i < transaction->nr - 1; i++) {
>>> +             if (transaction->updates[i]->update_type != UPDATE_LOG)
>>> +                     continue;
>>> +             if (!strcmp(transaction->updates[i]->refname,
>>> +                         update->refname)) {
>>> +                     update->flags |= UPDATE_REFLOG_NOLOCK;
>>> +                     update->orig_update = transaction->updates[i];
>>> +                     break;
>>> +             }
>>> +     }
>>> +     if (!(update->flags & UPDATE_REFLOG_NOLOCK))
>>> +       update->reflog_lock = xcalloc(1, sizeof(struct lock_file));
>>
>> So with two calls to transaction-update-reflog, we make two calls to
>> add-update, and each holds a separate lock?  If we write two entries
>> to record two updates of the same ref, would we still want to do so?
>
> Also, indent with tabs rather than spaces (the line following the 'if').

>
>>> +     /* Rollback any reflog files that are still open */
>>> +     for (i = 0; i < n; i++) {
>>> +             struct ref_update *update = updates[i];
>>> +
>>> +             if (update->update_type != UPDATE_LOG)
>>> +                     continue;
>>> +             if (update->flags & UPDATE_REFLOG_NOLOCK)
>>> +                     continue;
>>> +             if (update->reflog_fd == -1)
>>> +                     continue;
>>> +             rollback_lock_file(update->reflog_lock);
>>> +     }
>>>       transaction->status = ret ? REF_TRANSACTION_ERROR
>>>         : REF_TRANSACTION_CLOSED;
>
> Indent with tabs.