On Fri, May 16, 2014 at 3:01 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote:
> On Fri, May 16, 2014 at 5:35 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> Ronnie Sahlberg <sahlberg@xxxxxxxxxx> 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@xxxxxxxxxx>
>>> ---
>>> 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').
Done.
>
>>> + /* 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.
Done. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html