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

> 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?

>> +     /* 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;