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

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Karthik

On 25/02/2025 09:29, 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 especially when using the reftable backend, where
> batching multiple reference updates into a single transaction is more
> efficient than performing them sequentially.
> 
> Introduce partial transaction support with a new flag,
> 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows
> individual reference updates that would typically cause the entire
> transaction to fail due to non-system-related errors to be marked as
> rejected while permitting other updates to proceed. Non-system-related
> errors include issues caused by user-provided input values, whereas
> system-related errors, such as I/O failures or memory issues, continue
> to result in a full transaction failure. This approach enhances
> flexibility while preserving transactional integrity where necessary.
> 
> The implementation introduces several key components:
> 
>    - Add 'rejection_err' field to struct `ref_update` to track failed
>      updates with failure reason.
> 
>    - Modify reference backends (files, packed, reftable) to handle
>      partial transactions by using `ref_transaction_set_rejected()`
>      instead of failing the entire transaction when
>      `REF_TRANSACTION_ALLOW_PARTIAL` is set.
> 
>    - Add `ref_transaction_for_each_rejected_update()` to let callers
>      examine which updates were rejected and why.

I think this is a much better design. I wonder if we want to signal to 
the caller of ref_transaction_commit() that there were ignored errors 
rather than forcing them to call ref_transaction_for_each_rejected() to 
find that out. Another possibility would be to call the callback from 
ref_transaction_commit() but that would mean changing the signature of 
ref_transaction_begin() to take the callback and user data when 
REF_TRANSACTION_ALLOW_PARTIAL is passed.

Best Wishes

Phillip

> This foundational change enables partial transaction support throughout
> the reference subsystem. The next commit will expose this capability to
> users by adding a `--allow-partial` flag to 'git-update-ref(1)',
> providing both a user-facing feature and a testable implementation.
> 
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>   refs.c                  | 31 +++++++++++++++++++++++++++++++
>   refs.h                  | 22 ++++++++++++++++++++++
>   refs/files-backend.c    | 12 +++++++++++-
>   refs/packed-backend.c   | 30 ++++++++++++++++++++++++++++--
>   refs/refs-internal.h    | 13 +++++++++++++
>   refs/reftable-backend.c | 12 +++++++++++-
>   6 files changed, 116 insertions(+), 4 deletions(-)
> 
> diff --git a/refs.c b/refs.c
> index f989a46a5a..243c09c368 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1211,6 +1211,15 @@ void ref_transaction_free(struct ref_transaction *transaction)
>   	free(transaction);
>   }
>   
> +void ref_transaction_set_rejected(struct ref_transaction *transaction,
> +				  size_t update_idx,
> +				  enum transaction_error err)
> +{
> +	if (update_idx >= transaction->nr)
> +		BUG("trying to set rejection on invalid update index");
> +	transaction->updates[update_idx]->rejection_err = err;
> +}
> +
>   struct ref_update *ref_transaction_add_update(
>   		struct ref_transaction *transaction,
>   		const char *refname, unsigned int flags,
> @@ -1236,6 +1245,7 @@ struct ref_update *ref_transaction_add_update(
>   	transaction->updates[transaction->nr++] = update;
>   
>   	update->flags = flags;
> +	update->rejection_err = TRANSACTION_OK;
>   
>   	update->new_target = xstrdup_or_null(new_target);
>   	update->old_target = xstrdup_or_null(old_target);
> @@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
>   	}
>   }
>   
> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,
> +					      ref_transaction_for_each_rejected_update_fn cb,
> +					      void *cb_data)
> +{
> +	if (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))
> +		return;
> +
> +	for (size_t i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> +
> +		if (!update->rejection_err)
> +			continue;
> +
> +		cb(update->refname,
> +		   (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,
> +		   (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,
> +		   update->old_target, update->new_target,
> +		   update->rejection_err, cb_data);
> +	}
> +}
> +
>   int refs_delete_refs(struct ref_store *refs, const char *logmsg,
>   		     struct string_list *refnames, unsigned int flags)
>   {
> diff --git a/refs.h b/refs.h
> index 8e9ead174c..e4a6a8218f 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -675,6 +675,13 @@ enum ref_transaction_flag {
>   	 * either be absent or null_oid.
>   	 */
>   	REF_TRANSACTION_FLAG_INITIAL = (1 << 0),
> +
> +	/*
> +	 * The transaction mechanism by default fails all updates if any conflict
> +	 * is detected. This flag allows transactions to partially apply updates
> +	 * while rejecting updates which do not match the expected state.
> +	 */
> +	REF_TRANSACTION_ALLOW_PARTIAL = (1 << 1),
>   };
>   
>   /*
> @@ -905,6 +912,21 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,
>   					    ref_transaction_for_each_queued_update_fn cb,
>   					    void *cb_data);
>   
> +/*
> + * Execute the given callback function for each of the reference updates which
> + * have been rejected in the given transaction.
> + */
> +typedef void ref_transaction_for_each_rejected_update_fn(const char *refname,
> +							 const struct object_id *old_oid,
> +							 const struct object_id *new_oid,
> +							 const char *old_target,
> +							 const char *new_target,
> +							 enum transaction_error err,
> +							 void *cb_data);
> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,
> +					      ref_transaction_for_each_rejected_update_fn cb,
> +					      void *cb_data);
> +
>   /*
>    * Free `*transaction` and all associated data.
>    */
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3b0adf8bb2..d0a53c9ace 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>   		ret = lock_ref_for_update(refs, update, transaction,
>   					  head_ref, &refnames_to_check,
>   					  err);
> -		if (ret)
> +		if (ret) {
> +			if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&
> +			    ret != TRANSACTION_GENERIC_ERROR) {
> +				ref_transaction_set_rejected(transaction, i, ret);
> +
> +				strbuf_setlen(err, 0);
> +				ret = TRANSACTION_OK;
> +
> +				continue;
> +			}
>   			goto cleanup;
> +		}
>   
>   		if (update->flags & REF_DELETING &&
>   		    !(update->flags & REF_LOG_ONLY) &&
> diff --git a/refs/packed-backend.c b/refs/packed-backend.c
> index 75e1ebf67d..0857204213 100644
> --- a/refs/packed-backend.c
> +++ b/refs/packed-backend.c
> @@ -1324,10 +1324,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,
>    * remain locked when it is done.
>    */
>   static enum transaction_error write_with_updates(struct packed_ref_store *refs,
> -						 struct string_list *updates,
> +						 struct ref_transaction *transaction,
>   						 struct strbuf *err)
>   {
>   	enum transaction_error ret = TRANSACTION_GENERIC_ERROR;
> +	struct string_list *updates = &transaction->refnames;
>   	struct ref_iterator *iter = NULL;
>   	size_t i;
>   	int ok;
> @@ -1408,6 +1409,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,
>   						    "reference already exists",
>   						    update->refname);
>   					ret = TRANSACTION_CREATE_EXISTS;
> +
> +					if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {
> +						ref_transaction_set_rejected(transaction, i, ret);
> +						strbuf_setlen(err, 0);
> +						ret = 0;
> +						continue;
> +					}
> +
>   					goto error;
>   				} else if (!oideq(&update->old_oid, iter->oid)) {
>   					strbuf_addf(err, "cannot update ref '%s': "
> @@ -1416,6 +1425,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,
>   						    oid_to_hex(iter->oid),
>   						    oid_to_hex(&update->old_oid));
>   					ret = TRANSACTION_INCORRECT_OLD_VALUE;
> +
> +					if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {
> +						ref_transaction_set_rejected(transaction, i, ret);
> +						strbuf_setlen(err, 0);
> +						ret = 0;
> +						continue;
> +					}
> +
>   					goto error;
>   				}
>   			}
> @@ -1453,6 +1470,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,
>   					    update->refname,
>   					    oid_to_hex(&update->old_oid));
>   				return TRANSACTION_NONEXISTENT_REF;
> +
> +				if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {
> +					ref_transaction_set_rejected(transaction, i, ret);
> +					strbuf_setlen(err, 0);
> +					ret = 0;
> +					continue;
> +				}
> +
>   				goto error;
>   			}
>   		}
> @@ -1518,6 +1543,7 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,
>   write_error:
>   	strbuf_addf(err, "error writing to %s: %s",
>   		    get_tempfile_path(refs->tempfile), strerror(errno));
> +	ret = TRANSACTION_GENERIC_ERROR;
>   
>   error:
>   	ref_iterator_free(iter);
> @@ -1676,7 +1702,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,
>   		data->own_lock = 1;
>   	}
>   
> -	ret = write_with_updates(refs, &transaction->refnames, err);
> +	ret = write_with_updates(refs, transaction, err);
>   	if (ret)
>   		goto failure;
>   
> diff --git a/refs/refs-internal.h b/refs/refs-internal.h
> index c97045fbed..7196f2d880 100644
> --- a/refs/refs-internal.h
> +++ b/refs/refs-internal.h
> @@ -123,6 +123,11 @@ struct ref_update {
>   	 */
>   	uint64_t index;
>   
> +	/*
> +	 * Used in partial transactions to mark if a given update was rejected.
> +	 */
> +	enum transaction_error rejection_err;
> +
>   	/*
>   	 * If this ref_update was split off of a symref update via
>   	 * split_symref_update(), then this member points at that
> @@ -142,6 +147,14 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,
>   		      struct object_id *oid, struct strbuf *referent,
>   		      unsigned int *type, int *failure_errno);
>   
> +/*
> + * Mark a given update as rejected with a given reason. To be used in conjuction
> + * with the `REF_TRANSACTION_ALLOW_PARTIAL` flag to allow partial transactions.
> + */
> +void ref_transaction_set_rejected(struct ref_transaction *transaction,
> +				  size_t update_idx,
> +				  enum transaction_error err);
> +
>   /*
>    * Add a ref_update with the specified properties to transaction, and
>    * return a pointer to the new object. This function does not verify
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index e1fd9c2de2..83cf8d582b 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1374,8 +1374,18 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,
>   					    transaction->updates[i],
>   					    &refnames_to_check, head_type,
>   					    &head_referent, &referent, err);
> -		if (ret)
> +		if (ret) {
> +			if (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&
> +			    ret != TRANSACTION_GENERIC_ERROR) {
> +				ref_transaction_set_rejected(transaction, i, ret);
> +
> +				strbuf_setlen(err, 0);
> +				ret = TRANSACTION_OK;
> +
> +				continue;
> +			}
>   			goto done;
> +		}
>   	}
>   
>   	string_list_sort(&refnames_to_check);
>