Re: [PATCH 5/6] refs: implement partial reference transaction support

[Patrick Steinhardt <ps@pks.im>]

On Fri, Feb 07, 2025 at 08:34:40AM +0100, Karthik Nayak wrote:
> Git's reference transactions are all-or-nothing: either all updates
> succeed, or none do. While this atomic behavior is generally desirable,
> it can be suboptimal when using the reftable backend, where batching
> multiple reference updates into a single transaction is more efficient
> than performing them sequentially.

In fact it's even inefficient for the "files" backend. The whole
machinery around creating a new transaction, preparing it, committing it
and then cleaning up its state does bring a bunch of overhead with it.
But true, for the "reftable" backend it's way more impactful.

> diff --git a/refs.c b/refs.c
> index b420a120102b3793168598b885bba68e4f5f5f03..75dbd84acbc41658d4b8b6b5e7763c04e78d0061 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1211,6 +1212,14 @@ void ref_transaction_free(struct ref_transaction *transaction)
>  	free(transaction);
>  }
>  
> +void ref_transaction_add_rejection(struct ref_transaction *transaction,
> +				   size_t update_idx, struct strbuf *err)

"add" to me sounds like you're adding a new thingy to the transaction,
but you rather update something. How about `ref_update_set_rejected()`
or `ref_transacton_set_rejected()`?

> +{
> +	struct ref_update *update = transaction->updates[update_idx];

Do we want to `BUG()` in case `update_idx >= transaction->nr`?

> +	update->rejected = 1;
> +	strbuf_addbuf(&update->rejection_err, err);
> +}

Do we really need a string as rejection error? I'd expect that the set
of failures that lead to rejection should be rather limited, which means
that we could use an enum instead. This would unify the errors across
backends and also allows us to figure out the root cause of rejection in
other subsystems.

If we introduced an enum, we could eventually even iterate a bit on the
mechanism and rather trivially tell the backends which kind of failures
are acceptable. As an example, a conflicting ref update may for example
be ignored and not cause failure, a conflicting path name might cause
failure.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 9fc5454678340dd7c72539bfa0f15ee7eb24b1ff..99ec29164fbd30635125cc2325aab3d300cf906c 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2852,8 +2852,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  
>  		ret = lock_ref_for_update(refs, update, transaction,
>  					  head_ref, err);
> -		if (ret)
> +		if (ret) {

I wonder whether we want to accept all failures. Some failures are
certainly benign, like for example mismatching expected OIDs or a
conflict due to a preexisting ref that blocks the path. But other kinds
of failures which are unexpected might be a bit more on the dangerous
side to accept, so I think we should be careful here.

The same comment also applies to the other backends.

Patrick