| Message ID | 20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | refs: introduce support for partial reference transactions | expand |
On Tue, Feb 25, 2025 at 10:29:09AM +0100, Karthik Nayak wrote: > diff --git a/refs.c b/refs.c > index f989a46a5a..243c09c368 100644 > --- a/refs.c > +++ b/refs.c > @@ -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; This kind of proves my point that `TRANSACTION_OK` is pointless and leads to a mixture of using and not using the enum :) > 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 && Hm. If the error values were defined as a bitfield we could refactor this to not be a flag, but have a `transaction->accepted_rejections` instead that allows the caller to ask for only a subset of rejections to be accepted. I'm not quite sure whether it is a good idea, but the logic to handle all of that could be self-contained in `ref_transaction_set_rejected()`: if it returned an error code itself, it would swallow any errors in case the transaction allows a given error, and bubble up the error again in case the error is not allowed. The function could use a rename in that case though, e.g. `ref_transaction_maybe_set_rejected()`. Patrick
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); >
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);
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. 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(-)