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

[Junio C Hamano <gitster@pobox.com>]

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;