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').
>> + /* 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.
--
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