From: Phillip Wood Date: Tue, 25 Feb 2025 14:57:47 GMT Subject: Re: [PATCH v2 6/7] refs: implement partial reference transaction support Message-ID: <2755fb78-d587-4ad9-b22f-b60dfa752d7a@gmail.com> In-Reply-To: <20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@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 > --- > 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); >