On Fri, May 16, 2014 at 2: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.
Done.
>
>> @@ -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?
If we call transaction_update_reflog twice we will call add_update
twice since we are doing two updates.
But if the two calls are for the same ref then we only lock the reflog
for the first update and then in the second update we re-use the lock
structure from the first.
Later updates to reflog.c will show how to loop over the old reflog,
using the existing iterator, and then create a new expired reflog one
entry at a time.
>
>> + /* 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;
--
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