diff mbox series

[v3,6/8] refs: implement partial reference transaction support

Message ID 20250305-245-partially-atomic-ref-updates-v3-6-0c64e3052354@gmail.com (mailing list archive)
State New
Headers show
Series refs: introduce support for partial reference transactions | expand

Commit Message

Karthik Nayak March 5, 2025, 5:39 p.m. UTC
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. System errors
referred by 'REF_TRANSACTION_ERROR_GENERIC' continue to result in the
entire transaction failing. 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.

  - Add a new struct `ref_transaction_rejections` and a field within
    `ref_transaction` to this struct to allow quick iteration over
    rejected updates.

  - 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.

This foundational change enables partial transaction support throughout
the reference subsystem. A following 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                  | 61 +++++++++++++++++++++++++++++++++++++++++++++++++
 refs.h                  | 22 ++++++++++++++++++
 refs/files-backend.c    | 12 +++++++++-
 refs/packed-backend.c   | 27 ++++++++++++++++++++--
 refs/refs-internal.h    | 25 ++++++++++++++++++++
 refs/reftable-backend.c | 12 +++++++++-
 6 files changed, 155 insertions(+), 4 deletions(-)

Comments

Jeff King March 7, 2025, 7:50 p.m. UTC | #1
On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:

> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>  					    update->refname,
>  					    oid_to_hex(&update->old_oid));
>  				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
> +
> +				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> +					strbuf_setlen(err, 0);
> +					ret = 0;
> +					continue;
> +				}
> +
>  				goto error;
>  			}
>  		}

This new code isn't reachable, since we return in the lines shown in the
diff context.

Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
I think the "goto error" was already unreachable, so possibly the error
is in an earlier patch. (I didn't look; Coverity flagged this in the
final state in 'jch').

-Peff
Jeff King March 7, 2025, 7:57 p.m. UTC | #2
On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:

> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 0132b8b06a..dd9912d637 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -1371,8 +1371,15 @@ 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 (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
> +				strbuf_setlen(err, 0);
> +				ret = 0;
> +
> +				continue;
> +			}
>  			goto done;
> +		}
>  	}
>  
>  	string_list_sort(&refnames_to_check);

Coverity complains that this "ret = 0" is a dead store. I think it's
right, because either:

  1. Our continue loops again, and we overwrite "ret" with the next call
     to prepare_single_update().

  2. We leave the loop (because this is the final entry in the
     transaction update array), and then we overwrite "ret" with the
     result of refs_verify_refnames_available().

But it may be better to leave it in place as a defensive measure against
the rest of the function changing.

-Peff
Junio C Hamano March 7, 2025, 8:46 p.m. UTC | #3
Jeff King <peff@peff.net> writes:

> On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
>
>> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>>  					    update->refname,
>>  					    oid_to_hex(&update->old_oid));
>>  				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
>> +
>> +				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
>> +					strbuf_setlen(err, 0);
>> +					ret = 0;
>> +					continue;
>> +				}
>> +
>>  				goto error;
>>  			}
>>  		}
>
> This new code isn't reachable, since we return in the lines shown in the
> diff context.
>
> Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
> I think the "goto error" was already unreachable, so possibly the error
> is in an earlier patch. (I didn't look; Coverity flagged this in the
> final state in 'jch').

Sorry about that.  It shows that I lack the bandwidth necessary to
go through fine toothed comb on all the topics I queue.  Perhaps I
should be more selective and queue only the ones I personally had
enough bandwidth to look over (or have seen clear "I looked each and
every line of this series with fine toothed comb, put reviewed-by:
me" messages sent by trusted reviewers) while ignoring others?

I dunno.

Thanks.
Junio C Hamano March 7, 2025, 8:48 p.m. UTC | #4
Junio C Hamano <gitster@pobox.com> writes:

> Sorry about that.  It shows that I lack the bandwidth necessary to
> go through fine toothed comb on all the topics I queue.  Perhaps I
> should be more selective and queue only the ones I personally had
> enough bandwidth to look over (or have seen clear "I looked each and
> every line of this series with fine toothed comb, put reviewed-by:
> me" messages sent by trusted reviewers) while ignoring others?

I forgot a third category.  I should be able to queue series by
those who have track record of being meticulous and not have made
silly mistakes without reading each and every line.
Karthik Nayak March 7, 2025, 9:02 p.m. UTC | #5
Jeff King <peff@peff.net> writes:

> On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
>
>> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>>  					    update->refname,
>>  					    oid_to_hex(&update->old_oid));
>>  				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
>> +
>> +				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
>> +					strbuf_setlen(err, 0);
>> +					ret = 0;
>> +					continue;
>> +				}
>> +
>>  				goto error;
>>  			}
>>  		}
>
> This new code isn't reachable, since we return in the lines shown in the
> diff context.
>
> Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
> I think the "goto error" was already unreachable, so possibly the error
> is in an earlier patch. (I didn't look; Coverity flagged this in the
> final state in 'jch').
>
> -Peff

It should have bee `ret = REF_TRANSACTION_ERROR_NONEXISTENT_REF` and it
should have been in the previous commit!

Thanks for reporting!
Karthik Nayak March 7, 2025, 9:05 p.m. UTC | #6
Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
>>
>>> @@ -1456,6 +1471,13 @@ static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
>>>  					    update->refname,
>>>  					    oid_to_hex(&update->old_oid));
>>>  				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
>>> +
>>> +				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
>>> +					strbuf_setlen(err, 0);
>>> +					ret = 0;
>>> +					continue;
>>> +				}
>>> +
>>>  				goto error;
>>>  			}
>>>  		}
>>
>> This new code isn't reachable, since we return in the lines shown in the
>> diff context.
>>
>> Should it have been "ret = REF_TRANSACTION_ERROR"... in the first place?
>> I think the "goto error" was already unreachable, so possibly the error
>> is in an earlier patch. (I didn't look; Coverity flagged this in the
>> final state in 'jch').
>
> Sorry about that.  It shows that I lack the bandwidth necessary to
> go through fine toothed comb on all the topics I queue.  Perhaps I
> should be more selective and queue only the ones I personally had
> enough bandwidth to look over (or have seen clear "I looked each and
> every line of this series with fine toothed comb, put reviewed-by:
> me" messages sent by trusted reviewers) while ignoring others?
>
> I dunno.
>
> Thanks.

Apologies, I see that this was also present in the previous version.
Definitely a miss on my side. I'll see how it was missed in the tests
and add one if necessary!

Thanks!
Karthik Nayak March 7, 2025, 9:07 p.m. UTC | #7
Jeff King <peff@peff.net> writes:

> On Wed, Mar 05, 2025 at 06:39:01PM +0100, Karthik Nayak wrote:
>
>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
>> index 0132b8b06a..dd9912d637 100644
>> --- a/refs/reftable-backend.c
>> +++ b/refs/reftable-backend.c
>> @@ -1371,8 +1371,15 @@ 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 (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
>> +				strbuf_setlen(err, 0);
>> +				ret = 0;
>> +
>> +				continue;
>> +			}
>>  			goto done;
>> +		}
>>  	}
>>
>>  	string_list_sort(&refnames_to_check);
>
> Coverity complains that this "ret = 0" is a dead store. I think it's
> right, because either:
>
>   1. Our continue loops again, and we overwrite "ret" with the next call
>      to prepare_single_update().
>
>   2. We leave the loop (because this is the final entry in the
>      transaction update array), and then we overwrite "ret" with the
>      result of refs_verify_refnames_available().
>
> But it may be better to leave it in place as a defensive measure against
> the rest of the function changing.
>

Yes agreed with your analysis, and also your inference. So I'll let this stay.
Thanks for reporting!

> -Peff
diff mbox series

Patch

diff --git a/refs.c b/refs.c
index 63b8050ce2..b735510c3b 100644
--- a/refs.c
+++ b/refs.c
@@ -1176,6 +1176,10 @@  struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,
 	tr->ref_store = refs;
 	tr->flags = flags;
 	string_list_init_dup(&tr->refnames);
+
+	if (flags & REF_TRANSACTION_ALLOW_PARTIAL)
+		CALLOC_ARRAY(tr->rejections, 1);
+
 	return tr;
 }
 
@@ -1206,11 +1210,45 @@  void ref_transaction_free(struct ref_transaction *transaction)
 		free((char *)transaction->updates[i]->old_target);
 		free(transaction->updates[i]);
 	}
+
+	if (transaction->rejections)
+		free(transaction->rejections->update_indices);
+	free(transaction->rejections);
+
 	string_list_clear(&transaction->refnames, 0);
 	free(transaction->updates);
 	free(transaction);
 }
 
+int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
+				       size_t update_idx,
+				       enum ref_transaction_error err)
+{
+	if (update_idx >= transaction->nr)
+		BUG("trying to set rejection on invalid update index");
+
+	if (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))
+		return 0;
+
+	if (!transaction->rejections)
+		BUG("transaction not inititalized with partial support");
+
+	/*
+	 * Don't accept generic errors, since these errors are not user
+	 * input related.
+	 */
+	if (err == REF_TRANSACTION_ERROR_GENERIC)
+		return 0;
+
+	transaction->updates[update_idx]->rejection_err = err;
+	ALLOC_GROW(transaction->rejections->update_indices,
+		   transaction->rejections->nr + 1,
+		   transaction->rejections->alloc);
+	transaction->rejections->update_indices[transaction->rejections->nr++] = update_idx;
+
+	return 1;
+}
+
 struct ref_update *ref_transaction_add_update(
 		struct ref_transaction *transaction,
 		const char *refname, unsigned int flags,
@@ -1236,6 +1274,7 @@  struct ref_update *ref_transaction_add_update(
 	transaction->updates[transaction->nr++] = update;
 
 	update->flags = flags;
+	update->rejection_err = 0;
 
 	update->new_target = xstrdup_or_null(new_target);
 	update->old_target = xstrdup_or_null(old_target);
@@ -2727,6 +2766,28 @@  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->rejections)
+		return;
+
+	for (size_t i = 0; i < transaction->rejections->nr; i++) {
+		size_t update_index = transaction->rejections->update_indices[i];
+		struct ref_update *update = transaction->updates[update_index];
+
+		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 1b9213f9ce..5e5ff9e57d 100644
--- a/refs.h
+++ b/refs.h
@@ -673,6 +673,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),
 };
 
 /*
@@ -903,6 +910,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 ref_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 1e1663f44b..c2fdee6013 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2852,8 +2852,15 @@  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 (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+				strbuf_setlen(err, 0);
+				ret = 0;
+
+				continue;
+			}
 			goto cleanup;
+		}
 
 		if (update->flags & REF_DELETING &&
 		    !(update->flags & REF_LOG_ONLY) &&
@@ -3151,6 +3158,9 @@  static int files_transaction_finish(struct ref_store *ref_store,
 		struct ref_update *update = transaction->updates[i];
 		struct ref_lock *lock = update->backend_data;
 
+		if (update->rejection_err)
+			continue;
+
 		if (update->flags & REF_NEEDS_COMMIT ||
 		    update->flags & REF_LOG_ONLY) {
 			if (parse_and_write_reflog(refs, update, lock, err)) {
diff --git a/refs/packed-backend.c b/refs/packed-backend.c
index 5458952624..bfc6135743 100644
--- a/refs/packed-backend.c
+++ b/refs/packed-backend.c
@@ -1327,10 +1327,11 @@  static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,
  * remain locked when it is done.
  */
 static enum ref_transaction_error write_with_updates(struct packed_ref_store *refs,
-						     struct string_list *updates,
+						     struct ref_transaction *transaction,
 						     struct strbuf *err)
 {
 	enum ref_transaction_error ret = REF_TRANSACTION_ERROR_GENERIC;
+	struct string_list *updates = &transaction->refnames;
 	struct ref_iterator *iter = NULL;
 	size_t i;
 	int ok;
@@ -1411,6 +1412,13 @@  static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
 						    "reference already exists",
 						    update->refname);
 					ret = REF_TRANSACTION_ERROR_CREATE_EXISTS;
+
+					if (ref_transaction_maybe_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': "
@@ -1419,6 +1427,13 @@  static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
 						    oid_to_hex(iter->oid),
 						    oid_to_hex(&update->old_oid));
 					ret = REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE;
+
+					if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+						strbuf_setlen(err, 0);
+						ret = 0;
+						continue;
+					}
+
 					goto error;
 				}
 			}
@@ -1456,6 +1471,13 @@  static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
 					    update->refname,
 					    oid_to_hex(&update->old_oid));
 				return REF_TRANSACTION_ERROR_NONEXISTENT_REF;
+
+				if (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+					strbuf_setlen(err, 0);
+					ret = 0;
+					continue;
+				}
+
 				goto error;
 			}
 		}
@@ -1521,6 +1543,7 @@  static enum ref_transaction_error write_with_updates(struct packed_ref_store *re
 write_error:
 	strbuf_addf(err, "error writing to %s: %s",
 		    get_tempfile_path(refs->tempfile), strerror(errno));
+	ret = REF_TRANSACTION_ERROR_GENERIC;
 
 error:
 	ref_iterator_free(iter);
@@ -1679,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 3f1d19abd9..c417aec217 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 ref_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,13 @@  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.
+ */
+int ref_transaction_maybe_set_rejected(struct ref_transaction *transaction,
+				       size_t update_idx,
+				       enum ref_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
@@ -183,6 +195,18 @@  enum ref_transaction_state {
 	REF_TRANSACTION_CLOSED   = 2
 };
 
+/*
+ * Data structure to hold indices of updates which were rejected, when
+ * partial transactions where enabled. While the updates themselves hold
+ * the rejection error, this structure allows a transaction to iterate
+ * only over the rejected updates.
+ */
+struct ref_transaction_rejections {
+	size_t *update_indices;
+	size_t alloc;
+	size_t nr;
+};
+
 /*
  * Data structure for holding a reference transaction, which can
  * consist of checks and updates to multiple references, carried out
@@ -195,6 +219,7 @@  struct ref_transaction {
 	size_t alloc;
 	size_t nr;
 	enum ref_transaction_state state;
+	struct ref_transaction_rejections *rejections;
 	void *backend_data;
 	unsigned int flags;
 	uint64_t max_index;
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 0132b8b06a..dd9912d637 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -1371,8 +1371,15 @@  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 (ref_transaction_maybe_set_rejected(transaction, i, ret)) {
+				strbuf_setlen(err, 0);
+				ret = 0;
+
+				continue;
+			}
 			goto done;
+		}
 	}
 
 	string_list_sort(&refnames_to_check);
@@ -1455,6 +1462,9 @@  static int write_transaction_table(struct reftable_writer *writer, void *cb_data
 		struct reftable_transaction_update *tx_update = &arg->updates[i];
 		struct ref_update *u = tx_update->update;
 
+		if (u->rejection_err)
+			continue;
+
 		/*
 		 * Write a reflog entry when updating a ref to point to
 		 * something new in either of the following cases: