{"thread":{"id":"62998","subject":"[PATCH v2 1/7] refs/files: remove redundant check in split_symref_update()","startedAt":"2025-02-25T09:29:30Z","lastAt":"2025-03-05T14:20:23Z","messageCount":20,"participants":["Karthik Nayak","Patrick Steinhardt","Phillip Wood"],"isPatch":true,"patchVersion":2,"patchTotal":7},"messages":[{"id":"512989","messageId":"20250225-245-partially-atomic-ref-updates-v2-1-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 1/7] refs/files: remove redundant check in split_symref_update()","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:04Z","receivedAt":"2025-02-25T09:29:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"In `split_symref_update()`, there were two checks for duplicate\nrefnames:\n\n - At the start, `string_list_has_string()` ensures the refname is not\n already in `affected_refnames`, preventing duplicates from being\n added.\n\n - After adding the refname, another check verifies whether the newly\n inserted item has a `util` value.\n\nThe second check is unnecessary because the first one guarantees that\n`string_list_insert()` will never encounter a preexisting entry.\n\nSince `item->util` is only used in this context, remove the assignment and\nsimplify the surrounding code.\n\nSigned-off-by: Karthik Nayak \n---\n refs/files-backend.c | 20 +++-----------------\n 1 file changed, 3 insertions(+), 17 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 4e1c50fead..6c7df30738 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2382,7 +2382,6 @@ static int split_head_update(struct ref_update *update,\n \t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n \tstruct ref_update *new_update;\n \n \tif ((update->flags & REF_LOG_ONLY) ||\n@@ -2421,8 +2420,7 @@ static int split_head_update(struct ref_update *update,\n \t */\n \tif (strcmp(new_update->refname, \"HEAD\"))\n \t\tBUG(\"%s unexpectedly not 'HEAD'\", new_update->refname);\n-\titem = string_list_insert(affected_refnames, new_update->refname);\n-\titem->util = new_update;\n+\tstring_list_insert(affected_refnames, new_update->refname);\n \n \treturn 0;\n }\n@@ -2441,7 +2439,6 @@ static int split_symref_update(struct ref_update *update,\n \t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n-\tstruct string_list_item *item;\n \tstruct ref_update *new_update;\n \tunsigned int new_flags;\n \n@@ -2496,11 +2493,7 @@ static int split_symref_update(struct ref_update *update,\n \t * be valid as long as affected_refnames is in use, and NOT\n \t * referent, which might soon be freed by our caller.\n \t */\n-\titem = string_list_insert(affected_refnames, new_update->refname);\n-\tif (item->util)\n-\t\tBUG(\"%s unexpectedly found in affected_refnames\",\n-\t\t new_update->refname);\n-\titem->util = new_update;\n+\tstring_list_insert(affected_refnames, new_update->refname);\n \n \treturn 0;\n }\n@@ -2834,7 +2827,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n-\t\tstruct string_list_item *item;\n \n \t\tif ((update->flags & REF_IS_PRUNING) &&\n \t\t !(update->flags & REF_NO_DEREF))\n@@ -2843,13 +2835,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tif (update->flags & REF_LOG_ONLY)\n \t\t\tcontinue;\n \n-\t\titem = string_list_append(&affected_refnames, update->refname);\n-\t\t/*\n-\t\t * We store a pointer to update in item->util, but at\n-\t\t * the moment we never use the value of this field\n-\t\t * except to check whether it is non-NULL.\n-\t\t */\n-\t\titem->util = update;\n+\t\tstring_list_append(&affected_refnames, update->refname);\n \t}\n \tstring_list_sort(&affected_refnames);\n \tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n\n-- \n2.47.2\n\n"},{"id":"512990","messageId":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250207-245-partially-atomic-ref-updates-v1-0-e6a3690ff23a@gmail.com","subject":"[PATCH v2 0/7] refs: introduce support for partial reference transactions","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:03Z","receivedAt":"2025-02-25T09:29:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":" Documentation/git-update-ref.adoc | 21 +-\n builtin/update-ref.c | 74 +++++-\n refs.c | 101 ++++++--\n refs.h | 78 ++++--\n refs/files-backend.c | 260 ++++++++------------\n refs/packed-backend.c | 70 +++---\n refs/refs-internal.h | 22 +-\n refs/reftable-backend.c | 496 +++++++++++++++++++-------------------\n t/t1400-update-ref.sh | 216 +++++++++++++++++\n 9 files changed, 849 insertions(+), 489 deletions(-)\n\nKarthik Nayak (7):\n refs/files: remove redundant check in split_symref_update()\n refs: move duplicate refname update check to generic layer\n refs/files: remove duplicate duplicates check\n refs/reftable: extract code from the transaction preparation\n refs: introduce enum-based transaction error types\n refs: implement partial reference transaction support\n update-ref: add --allow-partial flag for stdin mode\n\nGit's reference updates are traditionally all or nothing - when updating\nmultiple references in a transaction, either all updates succeed or none\ndo. While this behavior is generally desirable, it can be limiting in\ncertain scenarios, particularly with the reftable backend where batching\nmultiple reference updates is more efficient than performing them\nsequentially.\n\nThis series introduces support for partial reference transactions,\nallowing individual reference updates to fail while letting others\nproceed. This capability is exposed through git-update-ref's\n`--allow-partial` flag, which can be used in `--stdin` mode to batch\nupdates and handle failures gracefully.\n\nThe changes are structured to carefully build up this functionality:\n\nFirst, we clean up and consolidate the reference update checking logic.\nThis includes removing duplicate checks in the files backend and moving\nrefname tracking to the generic layer, which simplifies the codebase and\nprepares it for the new feature.\n\nWe then restructure the reftable backend's transaction preparation code,\nextracting the update validation logic into a dedicated function. This\nnot only improves code organization but sets the stage for implementing\npartial transaction support.\n\nTo ensure we only skip errors which are user-oriented, we introduce\ntyped errors for transactions with 'enum transaction_error'. We extend\nthe existing errors to include other scenarios and use this new errors\nthroughout the refs code.\n\nWith this groundwork in place, we implement the core partial transaction\nsupport in the refs subsystem. This adds the necessary infrastructure to\ntrack and report rejected updates while allowing transactions to proceed.\nAll reference backends are modified to support this behavior when enabled.\n\nFinally, we expose this functionality to users through\ngit-update-ref(1)'s `--allow-partial` flag, complete with test coverage\nand documentation. The flag is specifically limited to `--stdin` mode\nwhere batching multiple updates is most relevant.\n\nThis enhancement improves Git's flexibility in handling reference\nupdates while maintaining the safety of atomic transactions by default.\nIt's particularly valuable for tools and workflows that need to handle\nreference update failures gracefully without abandoning the entire batch\nof updates.\n\nThis series is based on top of b838bf1938 (Merge branch 'master' of\nhttps://github.com/j6t/gitk, 2025-02-20) with Patrick's series 'refs:\nbatch refname availability checks' [1] merged in.\n\n[1]: https://lore.kernel.org/all/20250217-pks-update-ref-optimization-v1-0-a2b6d87a24af@pks.im/\n\n---\nChanges in v2:\n- Introduce and use structured errors. This consolidates the errors\n and their handling between the ref backends.\n- In the previous version, we skipped over all failures. This include\n system failures such as low memory or IO problems. Let's instead, only\n skip user-oriented failures, such as invalid old OID and so on.\n- Change the rejection function name to `ref_transaction_set_rejected()`.\n- Modify the commit messages and documentation to be a little more\n verbose.\n- Link to v1: https://lore.kernel.org/r/20250207-245-partially-atomic-ref-updates-v1-0-e6a3690ff23a@gmail.com\n\nRange-diff versus v1:\n\n1: e48e562f27 ! 1: 4a1b748e7a refs/files: remove duplicate check in `split_symref_update()`\n @@ Metadata\n Author: Karthik Nayak \n \n ## Commit message ##\n - refs/files: remove duplicate check in `split_symref_update()`\n + refs/files: remove redundant check in split_symref_update()\n \n - In split_symref_update(), there were two redundant checks:\n - - At the start: checking if refname exists in `affected_refnames`.\n - - After adding refname: checking if the item added to\n - `affected_refnames` contains the util field.\n + In `split_symref_update()`, there were two checks for duplicate\n + refnames:\n \n - Remove the second check since the first one already prevents duplicate\n - refnames from being added to the transaction updates.\n + - At the start, `string_list_has_string()` ensures the refname is not\n + already in `affected_refnames`, preventing duplicates from being\n + added.\n \n - Since this is the only place that utilizes the `item->util` value, avoid\n - setting the value in the first place and cleanup code around it.\n + - After adding the refname, another check verifies whether the newly\n + inserted item has a `util` value.\n +\n + The second check is unnecessary because the first one guarantees that\n + `string_list_insert()` will never encounter a preexisting entry.\n +\n + Since `item->util` is only used in this context, remove the assignment and\n + simplify the surrounding code.\n \n Signed-off-by: Karthik Nayak \n \n2: b5efdd3149 ! 2: 1cfb4f91b5 refs: move duplicate refname update check to generic layer\n @@ Commit message\n \n - Add a `string_list` field `refnames` to `ref_transaction` to contain\n all the references in a transaction. This field is updated whenever\n - a new update is added.\n + a new update is added via `ref_transaction_add_update`, so manual\n + additions in reference backends are dropped.\n \n - Modify the backends to use this field internally as needed. The\n backends need to check if an update for refname already exists when\n splitting symrefs or adding an update for 'HEAD'.\n \n - - In the reftable backend, in `reftable_be_transaction_prepare()`,\n - move the instance of `string_list_has_string()` above\n - `ref_transaction_add_update()` to check before the reference is\n - added.\n + - In the reftable backend, within `reftable_be_transaction_prepare()`,\n + move the `string_list_has_string()` check above\n + `ref_transaction_add_update()`. Since `ref_transaction_add_update()`\n + automatically adds the refname to `transaction->refnames`,\n + performing the check after will always return true, so we perform\n + the check before adding the update.\n \n This helps reduce duplication of functionality between the backends and\n makes it easier to make changes in a more centralized manner.\n @@ refs/files-backend.c: static int split_symref_update(struct ref_update *update,\n \treturn 0;\n }\n \n -@@ refs/files-backend.c: struct files_transaction_backend_data {\n - static int lock_ref_for_update(struct files_ref_store *refs,\n - \t\t\t struct ref_update *update,\n +@@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t struct ref_transaction *transaction,\n --\t\t\t const char *head_ref,\n + \t\t\t const char *head_ref,\n + \t\t\t struct string_list *refnames_to_check,\n -\t\t\t struct string_list *affected_refnames,\n --\t\t\t struct strbuf *err)\n -+\t\t\t const char *head_ref, struct strbuf *err)\n + \t\t\t struct strbuf *err)\n {\n \tstruct strbuf referent = STRBUF_INIT;\n - \tint mustexist = ref_update_expects_existing_old_ref(update);\n @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tupdate->flags |= REF_DELETING;\n \n @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *ref\n \t\tlock->count++;\n \t} else {\n \t\tret = lock_raw_ref(refs, update->refname, mustexist,\n --\t\t\t\t affected_refnames,\n -+\t\t\t\t &transaction->refnames,\n - \t\t\t\t &lock, &referent,\n - \t\t\t\t &update->type, err);\n +-\t\t\t\t refnames_to_check, affected_refnames,\n +-\t\t\t\t &lock, &referent,\n +-\t\t\t\t &update->type, err);\n ++\t\t\t\t refnames_to_check, &transaction->refnames,\n ++\t\t\t\t &lock, &referent, &update->type, err);\n \t\tif (ret) {\n + \t\t\tchar *reason;\n + \n @@ refs/files-backend.c: static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * of processing the split-off update, so we\n \t\t\t * don't have to do it here.\n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref\n \tsize_t i;\n \tint ret = 0;\n -\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n + \tstruct string_list refnames_to_check = STRING_LIST_INIT_NODUP;\n \tchar *head_ref = NULL;\n \tint head_type;\n - \tstruct files_transaction_backend_data *backend_data;\n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref_store,\n \ttransaction->backend_data = backend_data;\n \n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref\n \n \t/*\n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref_store,\n - \t\tstruct ref_update *update = transaction->updates[i];\n \n \t\tret = lock_ref_for_update(refs, update, transaction,\n --\t\t\t\t\t head_ref, &affected_refnames, err);\n -+\t\t\t\t\t head_ref, err);\n + \t\t\t\t\t head_ref, &refnames_to_check,\n +-\t\t\t\t\t &affected_refnames, err);\n ++\t\t\t\t\t err);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n +@@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref_store,\n + \t * So instead, we accept the race for now.\n + \t */\n + \tif (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check,\n +-\t\t\t\t\t &affected_refnames, NULL, 0, err)) {\n ++\t\t\t\t\t &transaction->refnames, NULL, 0, err)) {\n + \t\tret = TRANSACTION_NAME_CONFLICT;\n + \t\tgoto cleanup;\n + \t}\n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref_store,\n \n cleanup:\n \tfree(head_ref);\n -\tstring_list_clear(&affected_refnames, 0);\n + \tstring_list_clear(&refnames_to_check, 0);\n \n \tif (ret)\n - \t\tfiles_transaction_cleanup(refs, transaction);\n -@@ refs/files-backend.c: static int files_transaction_finish_initial(struct files_ref_store *refs,\n - {\n - \tsize_t i;\n - \tint ret = 0;\n --\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n - \tstruct ref_transaction *packed_transaction = NULL;\n - \tstruct ref_transaction *loose_transaction = NULL;\n - \n @@ refs/files-backend.c: static int files_transaction_finish_initial(struct files_ref_store *refs,\n \tif (transaction->state != REF_TRANSACTION_PREPARED)\n \t\tBUG(\"commit called for transaction that is not prepared\");\n @@ refs/files-backend.c: static int files_transaction_finish_initial(struct files_r\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n \tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\n -@@ refs/files-backend.c: static int files_transaction_finish_initial(struct files_ref_store *refs,\n - \t\t\tBUG(\"initial ref transaction with old_sha1 set\");\n - \n - \t\tif (refs_verify_refname_available(&refs->base, update->refname,\n --\t\t\t\t\t\t &affected_refnames, NULL, 1, err)) {\n -+\t\t\t\t\t\t &transaction->refnames, NULL, 1, err)) {\n - \t\t\tret = TRANSACTION_NAME_CONFLICT;\n - \t\t\tgoto cleanup;\n - \t\t}\n -@@ refs/files-backend.c: static int files_transaction_finish_initial(struct files_ref_store *refs,\n - \tif (packed_transaction)\n - \t\tref_transaction_free(packed_transaction);\n - \ttransaction->state = REF_TRANSACTION_CLOSED;\n --\tstring_list_clear(&affected_refnames, 0);\n - \treturn ret;\n - }\n - \n \n ## refs/packed-backend.c ##\n @@ refs/packed-backend.c: int is_packed_transaction_needed(struct ref_store *ref_store,\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, \"ref_transaction_prepare\");\n \tstruct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;\n -\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n + \tstruct string_list refnames_to_check = STRING_LIST_INIT_NODUP;\n \tstruct reftable_transaction_data *tx_data = NULL;\n \tstruct reftable_backend *be;\n - \tstruct object_id head_oid;\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t\t transaction->updates[i], err);\n \t\tif (ret)\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n \t\t}\n \n \t\tret = reftable_backend_read_ref(be, rewritten_ref,\n -@@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n - \t\t\t * at a later point.\n - \t\t\t */\n - \t\t\tret = refs_verify_refname_available(ref_store, u->refname,\n --\t\t\t\t\t\t\t &affected_refnames, NULL,\n -+\t\t\t\t\t\t\t &transaction->refnames, NULL,\n - \t\t\t\t\t\t\t transaction->flags & REF_TRANSACTION_FLAG_INITIAL,\n - \t\t\t\t\t\t\t err);\n - \t\t\tif (ret < 0)\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\tif (!strcmp(rewritten_ref, \"HEAD\"))\n \t\t\t\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n \t\t\t}\n \t\t}\n \n +@@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n + \t}\n + \n + \tstring_list_sort(&refnames_to_check);\n +-\tret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL,\n ++\tret = refs_verify_refnames_available(ref_store, &refnames_to_check,\n ++\t\t\t\t\t &transaction->refnames, NULL,\n + \t\t\t\t\t transaction->flags & REF_TRANSACTION_FLAG_INITIAL,\n + \t\t\t\t\t err);\n + \tif (ret < 0)\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\tstrbuf_addf(err, _(\"reftable: transaction prepare: %s\"),\n \t\t\t\t reftable_error_str(ret));\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n -\tstring_list_clear(&affected_refnames, 0);\n \tstrbuf_release(&referent);\n \tstrbuf_release(&head_referent);\n - \n + \tstring_list_clear(&refnames_to_check, 0);\n3: 1f1c261afd = 3: 91e96c9048 refs/files: remove duplicate duplicates check\n4: 556fd87651 ! 4: c47a020dc5 refs/reftable: extract code from the transaction preparation\n @@ refs/reftable-backend.c: static int queue_transaction_update(struct reftable_ref\n \treturn 0;\n }\n \n -+static int prepare_single_update(struct ref_store *ref_store,\n -+\t\t\t\t struct reftable_ref_store *refs,\n ++static int prepare_single_update(struct reftable_ref_store *refs,\n +\t\t\t\t struct reftable_transaction_data *tx_data,\n +\t\t\t\t struct ref_transaction *transaction,\n +\t\t\t\t struct reftable_backend *be,\n +\t\t\t\t struct ref_update *u,\n ++\t\t\t\t struct string_list *refnames_to_check,\n +\t\t\t\t unsigned int head_type,\n +\t\t\t\t struct strbuf *head_referent,\n +\t\t\t\t struct strbuf *referent,\n @@ refs/reftable-backend.c: static int queue_transaction_update(struct reftable_ref\n +\t\t * can output a proper error message instead of failing\n +\t\t * at a later point.\n +\t\t */\n -+\t\tret = refs_verify_refname_available(ref_store, u->refname,\n -+\t\t\t\t\t\t &transaction->refnames, NULL,\n -+\t\t\t\t\t\t transaction->flags & REF_TRANSACTION_FLAG_INITIAL,\n -+\t\t\t\t\t\t err);\n -+\t\tif (ret < 0)\n -+\t\t\treturn ret;\n ++\t\tstring_list_append(refnames_to_check, u->refname);\n +\n +\t\t/*\n +\t\t * There is no need to write the reference deletion\n @@ refs/reftable-backend.c: static int queue_transaction_update(struct reftable_ref\n +\tif (ret > 0) {\n +\t\t/* The reference does not exist, but we expected it to. */\n +\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n ++\n ++\n +\t\t\t\t \"unable to resolve reference '%s'\"),\n +\t\t\t ref_update_original_update_refname(u), u->refname);\n +\t\treturn -1;\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n -\t\t * stack.\n -\t\t */\n -\t\tret = backend_for(&be, refs, u->refname, &rewritten_ref, 0);\n -+\t\tret = prepare_single_update(ref_store, refs, tx_data,\n -+\t\t\t\t\t transaction, be,\n -+\t\t\t\t\t transaction->updates[i], head_type,\n ++\t\tret = prepare_single_update(refs, tx_data, transaction, be,\n ++\t\t\t\t\t transaction->updates[i],\n ++\t\t\t\t\t &refnames_to_check, head_type,\n +\t\t\t\t\t &head_referent, &referent, err);\n \t\tif (ret)\n \t\t\tgoto done;\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n -\t\t\t * can output a proper error message instead of failing\n -\t\t\t * at a later point.\n -\t\t\t */\n --\t\t\tret = refs_verify_refname_available(ref_store, u->refname,\n --\t\t\t\t\t\t\t &transaction->refnames, NULL,\n --\t\t\t\t\t\t\t transaction->flags & REF_TRANSACTION_FLAG_INITIAL,\n --\t\t\t\t\t\t\t err);\n --\t\t\tif (ret < 0)\n --\t\t\t\tgoto done;\n +-\t\t\tstring_list_append(&refnames_to_check, u->refname);\n -\n -\t\t\t/*\n -\t\t\t * There is no need to write the reference deletion\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n -\t\t}\n \t}\n \n - \ttransaction->backend_data = tx_data;\n + \tstring_list_sort(&refnames_to_check);\n-: ---------- > 5: ff7235f2b8 refs: introduce enum-based transaction error types\n5: 14b9657c99 ! 6: 74dce2ea61 refs: implement partial reference transaction support\n @@ Commit message\n \n Git's reference transactions are all-or-nothing: either all updates\n succeed, or none do. While this atomic behavior is generally desirable,\n - it can be suboptimal when using the reftable backend, where batching\n - multiple reference updates into a single transaction is more efficient\n - than performing them sequentially.\n + it can be suboptimal especially when using the reftable backend, where\n + batching multiple reference updates into a single transaction is more\n + efficient than performing them sequentially.\n \n - Introduce partial transaction support through a new flag\n - `REF_TRANSACTION_ALLOW_PARTIAL`. When this flag is set, individual\n - reference updates that would normally fail the entire transaction are\n - instead marked as rejected while allowing other updates to proceed. This\n - provides more flexibility while maintaining transactional integrity\n - where needed.\n + Introduce partial transaction support with a new flag,\n + 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\n + individual reference updates that would typically cause the entire\n + transaction to fail due to non-system-related errors to be marked as\n + rejected while permitting other updates to proceed. Non-system-related\n + errors include issues caused by user-provided input values, whereas\n + system-related errors, such as I/O failures or memory issues, continue\n + to result in a full transaction failure. This approach enhances\n + flexibility while preserving transactional integrity where necessary.\n \n The implementation introduces several key components:\n \n - - Add 'rejected' and 'rejection_err' fields to struct `ref_update` to\n - track failed updates and their failure reasons.\n + - Add 'rejection_err' field to struct `ref_update` to track failed\n + updates with failure reason.\n \n - Modify reference backends (files, packed, reftable) to handle\n - partial transactions by using `ref_transaction_add_rejection()`\n + partial transactions by using `ref_transaction_set_rejected()`\n instead of failing the entire transaction when\n `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n \n @@ Commit message\n Signed-off-by: Karthik Nayak \n \n ## refs.c ##\n -@@ refs.c: void ref_transaction_free(struct ref_transaction *transaction)\n - \t\tfree(transaction->updates[i]->committer_info);\n - \t\tfree((char *)transaction->updates[i]->new_target);\n - \t\tfree((char *)transaction->updates[i]->old_target);\n -+\t\tstrbuf_release(&transaction->updates[i]->rejection_err);\n - \t\tfree(transaction->updates[i]);\n - \t}\n - \tstring_list_clear(&transaction->refnames, 0);\n @@ refs.c: void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n -+void ref_transaction_add_rejection(struct ref_transaction *transaction,\n -+\t\t\t\t size_t update_idx, struct strbuf *err)\n ++void ref_transaction_set_rejected(struct ref_transaction *transaction,\n ++\t\t\t\t size_t update_idx,\n ++\t\t\t\t enum transaction_error err)\n +{\n -+\tstruct ref_update *update = transaction->updates[update_idx];\n -+\tupdate->rejected = 1;\n -+\tstrbuf_addbuf(&update->rejection_err, err);\n ++\tif (update_idx >= transaction->nr)\n ++\t\tBUG(\"trying to set rejection on invalid update index\");\n ++\ttransaction->updates[update_idx]->rejection_err = err;\n +}\n +\n struct ref_update *ref_transaction_add_update(\n \t\tstruct ref_transaction *transaction,\n \t\tconst char *refname, unsigned int flags,\n @@ refs.c: struct ref_update *ref_transaction_add_update(\n + \ttransaction->updates[transaction->nr++] = update;\n \n \tupdate->flags = flags;\n ++\tupdate->rejection_err = TRANSACTION_OK;\n \n -+\tstrbuf_init(&update->rejection_err, 0);\n -+\n \tupdate->new_target = xstrdup_or_null(new_target);\n \tupdate->old_target = xstrdup_or_null(old_target);\n - \tif ((flags & REF_HAVE_NEW) && new_oid)\n @@ refs.c: void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n \t}\n }\n @@ refs.c: void ref_transaction_for_each_queued_update(struct ref_transaction *tran\n +\tfor (size_t i = 0; i < transaction->nr; i++) {\n +\t\tstruct ref_update *update = transaction->updates[i];\n +\n -+\t\tif (!update->rejected)\n ++\t\tif (!update->rejection_err)\n +\t\t\tcontinue;\n +\n +\t\tcb(update->refname,\n +\t\t (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n +\t\t (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n +\t\t update->old_target, update->new_target,\n -+\t\t &update->rejection_err, cb_data);\n ++\t\t update->rejection_err, cb_data);\n +\t}\n +}\n +\n @@ refs.h: void ref_transaction_for_each_queued_update(struct ref_transaction *tran\n +\t\t\t\t\t\t\t const struct object_id *new_oid,\n +\t\t\t\t\t\t\t const char *old_target,\n +\t\t\t\t\t\t\t const char *new_target,\n -+\t\t\t\t\t\t\t const struct strbuf *reason,\n ++\t\t\t\t\t\t\t enum transaction_error err,\n +\t\t\t\t\t\t\t void *cb_data);\n +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n +\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n @@ refs.h: void ref_transaction_for_each_queued_update(struct ref_transaction *tran\n \n ## refs/files-backend.c ##\n @@ refs/files-backend.c: static int files_transaction_prepare(struct ref_store *ref_store,\n - \n \t\tret = lock_ref_for_update(refs, update, transaction,\n - \t\t\t\t\t head_ref, err);\n + \t\t\t\t\t head_ref, &refnames_to_check,\n + \t\t\t\t\t err);\n -\t\tif (ret)\n +\t\tif (ret) {\n -+\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n -+\t\t\t\tref_transaction_add_rejection(transaction, i, err);\n ++\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n ++\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n ++\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n +\n +\t\t\t\tstrbuf_setlen(err, 0);\n -+\t\t\t\tret = 0;\n ++\t\t\t\tret = TRANSACTION_OK;\n +\n +\t\t\t\tcontinue;\n +\t\t\t}\n \t\t\tgoto cleanup;\n +\t\t}\n -+\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\n @@ refs/packed-backend.c\n @@ refs/packed-backend.c: static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,\n * remain locked when it is done.\n */\n - static int write_with_updates(struct packed_ref_store *refs,\n --\t\t\t struct string_list *updates,\n -+\t\t\t struct ref_transaction *transaction,\n - \t\t\t struct strbuf *err)\n + static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n +-\t\t\t\t\t\t struct string_list *updates,\n ++\t\t\t\t\t\t struct ref_transaction *transaction,\n + \t\t\t\t\t\t struct strbuf *err)\n {\n + \tenum transaction_error ret = TRANSACTION_GENERIC_ERROR;\n +\tstruct string_list *updates = &transaction->refnames;\n \tstruct ref_iterator *iter = NULL;\n \tsize_t i;\n \tint ok;\n -@@ refs/packed-backend.c: static int write_with_updates(struct packed_ref_store *refs,\n - \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n +@@ refs/packed-backend.c: static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t\t \"reference already exists\",\n \t\t\t\t\t\t update->refname);\n + \t\t\t\t\tret = TRANSACTION_CREATE_EXISTS;\n +\n +\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n -+\t\t\t\t\t\tref_transaction_add_rejection(transaction, i, err);\n ++\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n +\t\t\t\t\t\tstrbuf_setlen(err, 0);\n ++\t\t\t\t\t\tret = 0;\n +\t\t\t\t\t\tcontinue;\n +\t\t\t\t\t}\n +\n \t\t\t\t\tgoto error;\n \t\t\t\t} else if (!oideq(&update->old_oid, iter->oid)) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n -@@ refs/packed-backend.c: static int write_with_updates(struct packed_ref_store *refs,\n - \t\t\t\t\t\t update->refname,\n +@@ refs/packed-backend.c: static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t\t oid_to_hex(iter->oid),\n \t\t\t\t\t\t oid_to_hex(&update->old_oid));\n + \t\t\t\t\tret = TRANSACTION_INCORRECT_OLD_VALUE;\n +\n +\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n -+\t\t\t\t\t\tref_transaction_add_rejection(transaction, i, err);\n ++\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n +\t\t\t\t\t\tstrbuf_setlen(err, 0);\n ++\t\t\t\t\t\tret = 0;\n +\t\t\t\t\t\tcontinue;\n +\t\t\t\t\t}\n +\n \t\t\t\t\tgoto error;\n \t\t\t\t}\n \t\t\t}\n -@@ refs/packed-backend.c: static int write_with_updates(struct packed_ref_store *refs,\n - \t\t\t\t\t \"reference is missing but expected %s\",\n +@@ refs/packed-backend.c: static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t update->refname,\n \t\t\t\t\t oid_to_hex(&update->old_oid));\n + \t\t\t\treturn TRANSACTION_NONEXISTENT_REF;\n +\n +\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n -+\t\t\t\t\tref_transaction_add_rejection(transaction, i, err);\n ++\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n +\t\t\t\t\tstrbuf_setlen(err, 0);\n ++\t\t\t\t\tret = 0;\n +\t\t\t\t\tcontinue;\n +\t\t\t\t}\n +\n \t\t\t\tgoto error;\n \t\t\t}\n \t\t}\n +@@ refs/packed-backend.c: static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n + write_error:\n + \tstrbuf_addf(err, \"error writing to %s: %s\",\n + \t\t get_tempfile_path(refs->tempfile), strerror(errno));\n ++\tret = TRANSACTION_GENERIC_ERROR;\n + \n + error:\n + \tref_iterator_free(iter);\n @@ refs/packed-backend.c: static int packed_transaction_prepare(struct ref_store *ref_store,\n \t\tdata->own_lock = 1;\n \t}\n \n --\tif (write_with_updates(refs, &transaction->refnames, err))\n -+\tif (write_with_updates(refs, transaction, err))\n +-\tret = write_with_updates(refs, &transaction->refnames, err);\n ++\tret = write_with_updates(refs, transaction, err);\n + \tif (ret)\n \t\tgoto failure;\n \n - \ttransaction->state = REF_TRANSACTION_PREPARED;\n \n ## refs/refs-internal.h ##\n -@@\n - \n - #include \"refs.h\"\n - #include \"iterator.h\"\n -+#include \"strbuf.h\"\n - #include \"string-list.h\"\n - \n - struct fsck_options;\n @@ refs/refs-internal.h: struct ref_update {\n \t */\n - \tunsigned int index;\n + \tuint64_t index;\n \n +\t/*\n -+\t * Used in partial transactions to mark a given update as rejected,\n -+\t * with rejection reason.\n ++\t * Used in partial transactions to mark if a given update was rejected.\n +\t */\n -+\tunsigned int rejected;\n -+\tstruct strbuf rejection_err;\n ++\tenum transaction_error rejection_err;\n +\n \t/*\n \t * If this ref_update was split off of a symref update via\n @@ refs/refs-internal.h: int refs_read_raw_ref(struct ref_store *ref_store, const c\n + * Mark a given update as rejected with a given reason. To be used in conjuction\n + * with the `REF_TRANSACTION_ALLOW_PARTIAL` flag to allow partial transactions.\n + */\n -+void ref_transaction_add_rejection(struct ref_transaction *transaction,\n -+\t\t\t\t size_t update_idx, struct strbuf *err);\n ++void ref_transaction_set_rejected(struct ref_transaction *transaction,\n ++\t\t\t\t size_t update_idx,\n ++\t\t\t\t enum transaction_error err);\n +\n /*\n * Add a ref_update with the specified properties to transaction, and\n @@ refs/refs-internal.h: int refs_read_raw_ref(struct ref_store *ref_store, const c\n \n ## refs/reftable-backend.c ##\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n - \t\t\t\t\t transaction, be,\n - \t\t\t\t\t transaction->updates[i], head_type,\n + \t\t\t\t\t transaction->updates[i],\n + \t\t\t\t\t &refnames_to_check, head_type,\n \t\t\t\t\t &head_referent, &referent, err);\n -\t\tif (ret)\n -+\n +\t\tif (ret) {\n -+\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n -+\t\t\t\tref_transaction_add_rejection(transaction, i, err);\n ++\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n ++\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n ++\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n +\n +\t\t\t\tstrbuf_setlen(err, 0);\n -+\t\t\t\tret = 0;\n ++\t\t\t\tret = TRANSACTION_OK;\n +\n +\t\t\t\tcontinue;\n +\t\t\t}\n @@ refs/reftable-backend.c: static int reftable_be_transaction_prepare(struct ref_s\n +\t\t}\n \t}\n \n - \ttransaction->backend_data = tx_data;\n + \tstring_list_sort(&refnames_to_check);\n6: 8c90e4201a ! 7: d79851e041 update-ref: add --allow-partial flag for stdin mode\n @@ Commit message\n \n Signed-off-by: Karthik Nayak \n \n - ## Documentation/git-update-ref.txt ##\n -@@ Documentation/git-update-ref.txt: git-update-ref - Update the object name stored in a ref safely\n + ## Documentation/git-update-ref.adoc ##\n +@@ Documentation/git-update-ref.adoc: git-update-ref - Update the object name stored in a ref safely\n + \n SYNOPSIS\n --------\n - [verse]\n +-[verse]\n -'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z])\n -+'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z] [--allow-partial])\n ++[synopsis]\n ++git update-ref [-m ] [--no-deref] -d []\n ++\t [-m ] [--no-deref] [--create-reflog] []\n ++ [-m ] [--no-deref] --stdin [-z] [--allow-partial]\n \n DESCRIPTION\n -----------\n -@@ Documentation/git-update-ref.txt: performs all modifications together. Specify commands of the form:\n +@@ Documentation/git-update-ref.adoc: performs all modifications together. Specify commands of the form:\n With `--create-reflog`, update-ref will create a reflog for each ref\n even if one would not ordinarily be created.\n \n -+With `--allow-partial`, update-ref will process the transaction even if\n -+some of the updates fail, allowing remaining updates to be applied.\n -+Failed updates will be printed in the following format:\n ++With `--allow-partial`, update-ref continues executing the transaction even if\n ++some updates fail due to invalid or incorrect user input, applying only the\n ++successful updates. Errors resulting from user-provided input are treated as\n ++non-system-related and do not cause the entire transaction to be aborted.\n ++However, system-related errors—such as I/O failures or memory issues—will still\n ++result in a full failure. Additionally, errors like F/D conflicts are batched\n ++for performance optimization and will also cause a full failure. Any failed\n ++updates will be reported in the following format:\n +\n +\trejected SP ( | ) SP ( | ) SP LF\n +\n Quote fields containing whitespace as if they were strings in C source\n code; i.e., surrounded by double-quotes and with backslash escapes.\n Use 40 \"0\" characters or the empty string to specify a zero value. To\n -@@ Documentation/git-update-ref.txt: quoting:\n +@@ Documentation/git-update-ref.adoc: quoting:\n In this format, use 40 \"0\" to specify a zero value, and use the empty\n string to specify a missing value.\n \n @@ builtin/update-ref.c: static void parse_cmd_abort(struct ref_transaction *transa\n +\t\t\t\tconst struct object_id *new_oid,\n +\t\t\t\tconst char *old_target,\n +\t\t\t\tconst char *new_target,\n -+\t\t\t\tconst struct strbuf *reason,\n ++\t\t\t\tenum transaction_error err,\n +\t\t\t\tvoid *cb_data UNUSED)\n +{\n +\tstruct strbuf sb = STRBUF_INIT;\n +\tchar space = ' ';\n ++\tconst char *reason = \"\";\n ++\n ++\tswitch (err) {\n ++\tcase TRANSACTION_NAME_CONFLICT:\n ++\t\treason = _(\"refname conflict\");\n ++\t\tbreak;\n ++\tcase TRANSACTION_CREATE_EXISTS:\n ++\t\treason = _(\"reference already exists\");\n ++\t\tbreak;\n ++\tcase TRANSACTION_NONEXISTENT_REF:\n ++\t\treason = _(\"reference does not exist\");\n ++\t\tbreak;\n ++\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n ++\t\treason = _(\"incorrect old value provided\");\n ++\t\tbreak;\n ++\tcase TRANSACTION_INVALID_NEW_VALUE:\n ++\t\treason = _(\"invalid new value provided\");\n ++\t\tbreak;\n ++\tcase TRANSACTION_EXPECTED_SYMREF:\n ++\t\treason = _(\"expected symref but found regular ref\");\n ++\t\tbreak;\n ++\tdefault:\n ++\t\treason = _(\"unkown failure\");\n ++\t}\n +\n +\tif (!line_termination)\n +\t\tspace = line_termination;\n @@ builtin/update-ref.c: static void parse_cmd_abort(struct ref_transaction *transa\n +\tstrbuf_addf(&sb, \"rejected%c%s%c%s%c%c%s%c%s%c\", space,\n +\t\t refname, space, new_oid ? oid_to_hex(new_oid) : new_target,\n +\t\t space, space, old_oid ? oid_to_hex(old_oid) : old_target,\n -+\t\t space, reason->buf, line_termination);\n ++\t\t space, reason, line_termination);\n +\n +\tfwrite(sb.buf, sb.len, 1, stdout);\n +\tstrbuf_release(&sb);\n @@ builtin/update-ref.c: int cmd_update_ref(int argc,\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;\n -\tint create_reflog = 0;\n +\tint create_reflog = 0, allow_partial = 0;\n ++\tunsigned int flags = 0;\n +\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n @@ builtin/update-ref.c: int cmd_update_ref(int argc,\n \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n \t\tOPT_BOOL( 0 , \"create-reflog\", &create_reflog, N_(\"create a reflog\")),\n -+\t\tOPT_BOOL('0', \"allow-partial\", &allow_partial, N_(\"allow partial transactions\")),\n ++\t\tOPT_BIT('0', \"allow-partial\", &flags, N_(\"allow partial transactions\"),\n ++\t\t\tREF_TRANSACTION_ALLOW_PARTIAL),\n \t\tOPT_END(),\n \t};\n \n @@ builtin/update-ref.c: int cmd_update_ref(int argc,\n - \t}\n - \n - \tif (read_stdin) {\n -+\t\tunsigned int flags = 0;\n -+\n -+\t\tif (allow_partial)\n -+\t\t\tflags |= REF_TRANSACTION_ALLOW_PARTIAL;\n -+\n - \t\tif (delete || argc > 0)\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n @@ t/t1400-update-ref.sh: do\n +\t\t\techo $head >expect &&\n +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n +\t\t\ttest_cmp expect actual &&\n -+\t\t\ttest_grep -q \"trying to write ref ${SQ}refs/heads/ref2${SQ} with nonexistent object\" stdout\n ++\t\t\ttest_grep -q \"invalid new value provided\" stdout\n +\t\t)\n +\t'\n +\n @@ t/t1400-update-ref.sh: do\n +\t\t\techo $head >expect &&\n +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n +\t\t\ttest_cmp expect actual &&\n -+\t\t\ttest_grep -q \"trying to write non-commit object $head_tree to branch ${SQ}refs/heads/ref2${SQ}\" stdout\n ++\t\t\ttest_grep -q \"invalid new value provided\" stdout\n +\t\t)\n +\t'\n +\n @@ t/t1400-update-ref.sh: do\n +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n +\t\t\ttest_cmp expect actual &&\n +\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n -+\t\t\ttest_grep -q \"unable to resolve reference\" stdout\n ++\t\t\ttest_grep -q \"reference does not exist\" stdout\n +\t\t)\n +\t'\n +\n @@ t/t1400-update-ref.sh: do\n +\t\t\ttest_cmp expect actual &&\n +\t\t\techo $head >expect &&\n +\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n -+\t\t\ttest_grep -q \"reference is missing but expected $head\" stdout\n ++\t\t\ttest_grep -q \"reference does not exist\" stdout\n +\t\t)\n +\t'\n +\n @@ t/t1400-update-ref.sh: do\n +\t\t\techo $head >expect &&\n +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n +\t\t\ttest_cmp expect actual &&\n -+\t\t\ttest_grep -q \"expected symref with target ${SQ}refs/heads/nonexistent${SQ}: but is a regular ref\" stdout\n ++\t\t\ttest_grep -q \"expected symref but found regular ref\" stdout\n +\t\t)\n +\t'\n +\n @@ t/t1400-update-ref.sh: do\n +\t\t\techo $head >expect &&\n +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n +\t\t\ttest_cmp expect actual &&\n -+\t\t\ttest_grep -q \"${SQ}refs/heads/ref2${SQ}: is at $head but expected $old_head\" stdout\n ++\t\t\ttest_grep -q \"incorrect old value provided\" stdout\n ++\t\t)\n ++\t'\n ++\n ++\t# F/D conflicts on the files backend are resolved on an individual\n ++\t# update level since refs are stored as files. On the reftable backend\n ++\t# this check is batched to optimize for performance, so failures cannot\n ++\t# be isolated to a single update.\n ++\ttest_expect_success REFFILES \"stdin $type allow-partial refname conflict\" '\n ++\t\tgit init repo &&\n ++\t\ttest_when_finished \"rm -fr repo\" &&\n ++\t\t(\n ++\t\t\tcd repo &&\n ++\t\t\ttest_commit one &&\n ++\t\t\told_head=$(git rev-parse HEAD) &&\n ++\t\t\ttest_commit two &&\n ++\t\t\thead=$(git rev-parse HEAD) &&\n ++\t\t\tgit update-ref refs/heads/ref/foo $head &&\n ++\n ++\t\t\tformat_command $type \"update refs/heads/ref/foo\" \"$old_head\" \"$head\" >stdin &&\n ++\t\t\tformat_command $type \"update refs/heads/ref\" \"$old_head\" \"\" >>stdin &&\n ++\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n ++\t\t\techo $old_head >expect &&\n ++\t\t\tgit rev-parse refs/heads/ref/foo >actual &&\n ++\t\t\ttest_cmp expect actual &&\n ++\t\t\ttest_grep -q \"refname conflict\" stdout\n +\t\t)\n +\t'\n done\n\n\nbase-commit: 408c44885d5b61a728dfc1df462490487cb01dae\nchange-id: 20241206-245-partially-atomic-ref-updates-9fe8b080345c\n\nThanks\n- Karthik\n\n"},{"id":"512992","messageId":"20250225-245-partially-atomic-ref-updates-v2-2-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 2/7] refs: move duplicate refname update check to generic layer","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:05Z","receivedAt":"2025-02-25T09:29:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Move the tracking of refnames in `affected_refnames` from individual\nbackends into the generic layer in 'refs.c'. This centralizes the\nduplicate refname detection that was previously handled separately by\neach backend.\n\nMake some changes to accommodate this move:\n\n - Add a `string_list` field `refnames` to `ref_transaction` to contain\n all the references in a transaction. This field is updated whenever\n a new update is added via `ref_transaction_add_update`, so manual\n additions in reference backends are dropped.\n\n - Modify the backends to use this field internally as needed. The\n backends need to check if an update for refname already exists when\n splitting symrefs or adding an update for 'HEAD'.\n\n - In the reftable backend, within `reftable_be_transaction_prepare()`,\n move the `string_list_has_string()` check above\n `ref_transaction_add_update()`. Since `ref_transaction_add_update()`\n automatically adds the refname to `transaction->refnames`,\n performing the check after will always return true, so we perform\n the check before adding the update.\n\nThis helps reduce duplication of functionality between the backends and\nmakes it easier to make changes in a more centralized manner.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 17 +++++++++++++\n refs/files-backend.c | 67 +++++++++++--------------------------------------\n refs/packed-backend.c | 25 +-----------------\n refs/refs-internal.h | 2 ++\n refs/reftable-backend.c | 54 +++++++++++++--------------------------\n 5 files changed, 51 insertions(+), 114 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 54fd5ce21e..ab69746947 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1175,6 +1175,7 @@ struct ref_transaction *ref_store_transaction_begin(struct ref_store *refs,\n \tCALLOC_ARRAY(tr, 1);\n \ttr->ref_store = refs;\n \ttr->flags = flags;\n+\tstring_list_init_dup(&tr->refnames);\n \treturn tr;\n }\n \n@@ -1205,6 +1206,7 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \t\tfree((char *)transaction->updates[i]->old_target);\n \t\tfree(transaction->updates[i]);\n \t}\n+\tstring_list_clear(&transaction->refnames, 0);\n \tfree(transaction->updates);\n \tfree(transaction);\n }\n@@ -1218,6 +1220,7 @@ struct ref_update *ref_transaction_add_update(\n \t\tconst char *committer_info,\n \t\tconst char *msg)\n {\n+\tstruct string_list_item *item;\n \tstruct ref_update *update;\n \n \tif (transaction->state != REF_TRANSACTION_OPEN)\n@@ -1245,6 +1248,16 @@ struct ref_update *ref_transaction_add_update(\n \t\tupdate->msg = normalize_reflog_message(msg);\n \t}\n \n+\t/*\n+\t * This list is generally used by the backends to avoid duplicates.\n+\t * But we do support multiple log updates for a given refname within\n+\t * a single transaction.\n+\t */\n+\tif (!(update->flags & REF_LOG_ONLY)) {\n+\t\titem = string_list_append(&transaction->refnames, refname);\n+\t\titem->util = update;\n+\t}\n+\n \treturn update;\n }\n \n@@ -2405,6 +2418,10 @@ int ref_transaction_prepare(struct ref_transaction *transaction,\n \t\treturn -1;\n \t}\n \n+\tstring_list_sort(&transaction->refnames);\n+\tif (ref_update_reject_duplicates(&transaction->refnames, err))\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n+\n \tret = refs->be->transaction_prepare(refs, transaction, err);\n \tif (ret)\n \t\treturn ret;\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 6c7df30738..85ed85ad87 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2378,9 +2378,7 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n */\n static int split_head_update(struct ref_update *update,\n \t\t\t struct ref_transaction *transaction,\n-\t\t\t const char *head_ref,\n-\t\t\t struct string_list *affected_refnames,\n-\t\t\t struct strbuf *err)\n+\t\t\t const char *head_ref, struct strbuf *err)\n {\n \tstruct ref_update *new_update;\n \n@@ -2398,7 +2396,7 @@ static int split_head_update(struct ref_update *update,\n \t * transaction. This check is O(lg N) in the transaction\n \t * size, but it happens at most once per transaction.\n \t */\n-\tif (string_list_has_string(affected_refnames, \"HEAD\")) {\n+\tif (string_list_has_string(&transaction->refnames, \"HEAD\")) {\n \t\t/* An entry already existed */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for 'HEAD' (including one \"\n@@ -2420,7 +2418,6 @@ static int split_head_update(struct ref_update *update,\n \t */\n \tif (strcmp(new_update->refname, \"HEAD\"))\n \t\tBUG(\"%s unexpectedly not 'HEAD'\", new_update->refname);\n-\tstring_list_insert(affected_refnames, new_update->refname);\n \n \treturn 0;\n }\n@@ -2436,7 +2433,6 @@ static int split_head_update(struct ref_update *update,\n static int split_symref_update(struct ref_update *update,\n \t\t\t const char *referent,\n \t\t\t struct ref_transaction *transaction,\n-\t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n \tstruct ref_update *new_update;\n@@ -2448,7 +2444,7 @@ static int split_symref_update(struct ref_update *update,\n \t * size, but it happens at most once per symref in a\n \t * transaction.\n \t */\n-\tif (string_list_has_string(affected_refnames, referent)) {\n+\tif (string_list_has_string(&transaction->refnames, referent)) {\n \t\t/* An entry already exists */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for '%s' (including one \"\n@@ -2486,15 +2482,6 @@ static int split_symref_update(struct ref_update *update,\n \tupdate->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n-\t/*\n-\t * Add the referent. This insertion is O(N) in the transaction\n-\t * size, but it happens at most once per symref in a\n-\t * transaction. Make sure to add new_update->refname, which will\n-\t * be valid as long as affected_refnames is in use, and NOT\n-\t * referent, which might soon be freed by our caller.\n-\t */\n-\tstring_list_insert(affected_refnames, new_update->refname);\n-\n \treturn 0;\n }\n \n@@ -2558,7 +2545,6 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t struct ref_transaction *transaction,\n \t\t\t const char *head_ref,\n \t\t\t struct string_list *refnames_to_check,\n-\t\t\t struct string_list *affected_refnames,\n \t\t\t struct strbuf *err)\n {\n \tstruct strbuf referent = STRBUF_INIT;\n@@ -2575,8 +2561,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tupdate->flags |= REF_DELETING;\n \n \tif (head_ref) {\n-\t\tret = split_head_update(update, transaction, head_ref,\n-\t\t\t\t\taffected_refnames, err);\n+\t\tret = split_head_update(update, transaction, head_ref, err);\n \t\tif (ret)\n \t\t\tgoto out;\n \t}\n@@ -2586,9 +2571,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tlock->count++;\n \t} else {\n \t\tret = lock_raw_ref(refs, update->refname, mustexist,\n-\t\t\t\t refnames_to_check, affected_refnames,\n-\t\t\t\t &lock, &referent,\n-\t\t\t\t &update->type, err);\n+\t\t\t\t refnames_to_check, &transaction->refnames,\n+\t\t\t\t &lock, &referent, &update->type, err);\n \t\tif (ret) {\n \t\t\tchar *reason;\n \n@@ -2642,9 +2626,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * of processing the split-off update, so we\n \t\t\t * don't have to do it here.\n \t\t\t */\n-\t\t\tret = split_symref_update(update,\n-\t\t\t\t\t\t referent.buf, transaction,\n-\t\t\t\t\t\t affected_refnames, err);\n+\t\t\tret = split_symref_update(update, referent.buf,\n+\t\t\t\t\t\t transaction, err);\n \t\t\tif (ret)\n \t\t\t\tgoto out;\n \t\t}\n@@ -2799,7 +2782,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t \"ref_transaction_prepare\");\n \tsize_t i;\n \tint ret = 0;\n-\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \tstruct string_list refnames_to_check = STRING_LIST_INIT_NODUP;\n \tchar *head_ref = NULL;\n \tint head_type;\n@@ -2818,12 +2800,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \ttransaction->backend_data = backend_data;\n \n \t/*\n-\t * Fail if a refname appears more than once in the\n-\t * transaction. (If we end up splitting up any updates using\n-\t * split_symref_update() or split_head_update(), those\n-\t * functions will check that the new updates don't have the\n-\t * same refname as any existing ones.) Also fail if any of the\n-\t * updates use REF_IS_PRUNING without REF_NO_DEREF.\n+\t * Fail if any of the updates use REF_IS_PRUNING without REF_NO_DEREF.\n \t */\n \tfor (i = 0; i < transaction->nr; i++) {\n \t\tstruct ref_update *update = transaction->updates[i];\n@@ -2831,16 +2808,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tif ((update->flags & REF_IS_PRUNING) &&\n \t\t !(update->flags & REF_NO_DEREF))\n \t\t\tBUG(\"REF_IS_PRUNING set without REF_NO_DEREF\");\n-\n-\t\tif (update->flags & REF_LOG_ONLY)\n-\t\t\tcontinue;\n-\n-\t\tstring_list_append(&affected_refnames, update->refname);\n-\t}\n-\tstring_list_sort(&affected_refnames);\n-\tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n-\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto cleanup;\n \t}\n \n \t/*\n@@ -2882,7 +2849,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \n \t\tret = lock_ref_for_update(refs, update, transaction,\n \t\t\t\t\t head_ref, &refnames_to_check,\n-\t\t\t\t\t &affected_refnames, err);\n+\t\t\t\t\t err);\n \t\tif (ret)\n \t\t\tgoto cleanup;\n \n@@ -2929,7 +2896,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t * So instead, we accept the race for now.\n \t */\n \tif (refs_verify_refnames_available(refs->packed_ref_store, &refnames_to_check,\n-\t\t\t\t\t &affected_refnames, NULL, 0, err)) {\n+\t\t\t\t\t &transaction->refnames, NULL, 0, err)) {\n \t\tret = TRANSACTION_NAME_CONFLICT;\n \t\tgoto cleanup;\n \t}\n@@ -2975,7 +2942,6 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \n cleanup:\n \tfree(head_ref);\n-\tstring_list_clear(&affected_refnames, 0);\n \tstring_list_clear(&refnames_to_check, 0);\n \n \tif (ret)\n@@ -3050,13 +3016,8 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \tif (transaction->state != REF_TRANSACTION_PREPARED)\n \t\tBUG(\"commit called for transaction that is not prepared\");\n \n-\t/* Fail if a refname appears more than once in the transaction: */\n-\tfor (i = 0; i < transaction->nr; i++)\n-\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n-\t\t\tstring_list_append(&affected_refnames,\n-\t\t\t\t\t transaction->updates[i]->refname);\n-\tstring_list_sort(&affected_refnames);\n-\tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n+\tstring_list_sort(&transaction->refnames);\n+\tif (ref_update_reject_duplicates(&transaction->refnames, err)) {\n \t\tret = TRANSACTION_GENERIC_ERROR;\n \t\tgoto cleanup;\n \t}\n@@ -3074,7 +3035,7 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \t * that we are creating already exists.\n \t */\n \tif (refs_for_each_rawref(&refs->base, ref_present,\n-\t\t\t\t &affected_refnames))\n+\t\t\t\t &transaction->refnames))\n \t\tBUG(\"initial ref transaction called with existing refs\");\n \n \tpacked_transaction = ref_store_transaction_begin(refs->packed_ref_store,\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 71a38acfed..3247871574 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1619,8 +1619,6 @@ int is_packed_transaction_needed(struct ref_store *ref_store,\n struct packed_transaction_backend_data {\n \t/* True iff the transaction owns the packed-refs lock. */\n \tint own_lock;\n-\n-\tstruct string_list updates;\n };\n \n static void packed_transaction_cleanup(struct packed_ref_store *refs,\n@@ -1629,8 +1627,6 @@ static void packed_transaction_cleanup(struct packed_ref_store *refs,\n \tstruct packed_transaction_backend_data *data = transaction->backend_data;\n \n \tif (data) {\n-\t\tstring_list_clear(&data->updates, 0);\n-\n \t\tif (is_tempfile_active(refs->tempfile))\n \t\t\tdelete_tempfile(&refs->tempfile);\n \n@@ -1655,7 +1651,6 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t\t\tREF_STORE_READ | REF_STORE_WRITE | REF_STORE_ODB,\n \t\t\t\"ref_transaction_prepare\");\n \tstruct packed_transaction_backend_data *data;\n-\tsize_t i;\n \tint ret = TRANSACTION_GENERIC_ERROR;\n \n \t/*\n@@ -1668,34 +1663,16 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t */\n \n \tCALLOC_ARRAY(data, 1);\n-\tstring_list_init_nodup(&data->updates);\n \n \ttransaction->backend_data = data;\n \n-\t/*\n-\t * Stick the updates in a string list by refname so that we\n-\t * can sort them:\n-\t */\n-\tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *update = transaction->updates[i];\n-\t\tstruct string_list_item *item =\n-\t\t\tstring_list_append(&data->updates, update->refname);\n-\n-\t\t/* Store a pointer to update in item->util: */\n-\t\titem->util = update;\n-\t}\n-\tstring_list_sort(&data->updates);\n-\n-\tif (ref_update_reject_duplicates(&data->updates, err))\n-\t\tgoto failure;\n-\n \tif (!is_lock_file_locked(&refs->lock)) {\n \t\tif (packed_refs_lock(ref_store, 0, err))\n \t\t\tgoto failure;\n \t\tdata->own_lock = 1;\n \t}\n \n-\tif (write_with_updates(refs, &data->updates, err))\n+\tif (write_with_updates(refs, &transaction->refnames, err))\n \t\tgoto failure;\n \n \ttransaction->state = REF_TRANSACTION_PREPARED;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex cadd9808fd..a6e05eaecd 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -3,6 +3,7 @@\n \n #include \"refs.h\"\n #include \"iterator.h\"\n+#include \"string-list.h\"\n \n struct fsck_options;\n struct ref_transaction;\n@@ -198,6 +199,7 @@ enum ref_transaction_state {\n struct ref_transaction {\n \tstruct ref_store *ref_store;\n \tstruct ref_update **updates;\n+\tstruct string_list refnames;\n \tsize_t alloc;\n \tsize_t nr;\n \tenum ref_transaction_state state;\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 441b8c69c1..f616d9aabe 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1076,7 +1076,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \tstruct reftable_ref_store *refs =\n \t\treftable_be_downcast(ref_store, REF_STORE_WRITE|REF_STORE_MAIN, \"ref_transaction_prepare\");\n \tstruct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;\n-\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n \tstruct string_list refnames_to_check = STRING_LIST_INIT_NODUP;\n \tstruct reftable_transaction_data *tx_data = NULL;\n \tstruct reftable_backend *be;\n@@ -1101,10 +1100,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t\t transaction->updates[i], err);\n \t\tif (ret)\n \t\t\tgoto done;\n-\n-\t\tif (!(transaction->updates[i]->flags & REF_LOG_ONLY))\n-\t\t\tstring_list_append(&affected_refnames,\n-\t\t\t\t\t transaction->updates[i]->refname);\n \t}\n \n \t/*\n@@ -1116,17 +1111,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\ttx_data->args[i].updates_alloc = tx_data->args[i].updates_expected;\n \t}\n \n-\t/*\n-\t * Fail if a refname appears more than once in the transaction.\n-\t * This code is taken from the files backend and is a good candidate to\n-\t * be moved into the generic layer.\n-\t */\n-\tstring_list_sort(&affected_refnames);\n-\tif (ref_update_reject_duplicates(&affected_refnames, err)) {\n-\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto done;\n-\t}\n-\n \t/*\n \t * TODO: it's dubious whether we should reload the stack that \"HEAD\"\n \t * belongs to or not. In theory, it may happen that we only modify\n@@ -1194,14 +1178,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t !(u->flags & REF_LOG_ONLY) &&\n \t\t !(u->flags & REF_UPDATE_VIA_HEAD) &&\n \t\t !strcmp(rewritten_ref, head_referent.buf)) {\n-\t\t\tstruct ref_update *new_update;\n-\n \t\t\t/*\n \t\t\t * First make sure that HEAD is not already in the\n \t\t\t * transaction. This check is O(lg N) in the transaction\n \t\t\t * size, but it happens at most once per transaction.\n \t\t\t */\n-\t\t\tif (string_list_has_string(&affected_refnames, \"HEAD\")) {\n+\t\t\tif (string_list_has_string(&transaction->refnames, \"HEAD\")) {\n \t\t\t\t/* An entry already existed */\n \t\t\t\tstrbuf_addf(err,\n \t\t\t\t\t _(\"multiple updates for 'HEAD' (including one \"\n@@ -1211,12 +1193,11 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\tgoto done;\n \t\t\t}\n \n-\t\t\tnew_update = ref_transaction_add_update(\n-\t\t\t\t\ttransaction, \"HEAD\",\n-\t\t\t\t\tu->flags | REF_LOG_ONLY | REF_NO_DEREF,\n-\t\t\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, NULL,\n-\t\t\t\t\tu->msg);\n-\t\t\tstring_list_insert(&affected_refnames, new_update->refname);\n+\t\t\tref_transaction_add_update(\n+\t\t\t\ttransaction, \"HEAD\",\n+\t\t\t\tu->flags | REF_LOG_ONLY | REF_NO_DEREF,\n+\t\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, NULL,\n+\t\t\t\tu->msg);\n \t\t}\n \n \t\tret = reftable_backend_read_ref(be, rewritten_ref,\n@@ -1281,6 +1262,15 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\tif (!strcmp(rewritten_ref, \"HEAD\"))\n \t\t\t\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n \n+\t\t\t\tif (string_list_has_string(&transaction->refnames, referent.buf)) {\n+\t\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t\t _(\"multiple updates for '%s' (including one \"\n+\t\t\t\t\t\t \"via symref '%s') are not allowed\"),\n+\t\t\t\t\t\t referent.buf, u->refname);\n+\t\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n+\t\t\t\t\tgoto done;\n+\t\t\t\t}\n+\n \t\t\t\t/*\n \t\t\t\t * If we are updating a symref (eg. HEAD), we should also\n \t\t\t\t * update the branch that the symref points to.\n@@ -1305,16 +1295,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t */\n \t\t\t\tu->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n \t\t\t\tu->flags &= ~REF_HAVE_OLD;\n-\n-\t\t\t\tif (string_list_has_string(&affected_refnames, new_update->refname)) {\n-\t\t\t\t\tstrbuf_addf(err,\n-\t\t\t\t\t\t _(\"multiple updates for '%s' (including one \"\n-\t\t\t\t\t\t \"via symref '%s') are not allowed\"),\n-\t\t\t\t\t\t referent.buf, u->refname);\n-\t\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n-\t\t\t\t\tgoto done;\n-\t\t\t\t}\n-\t\t\t\tstring_list_insert(&affected_refnames, new_update->refname);\n \t\t\t}\n \t\t}\n \n@@ -1384,7 +1364,8 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t}\n \n \tstring_list_sort(&refnames_to_check);\n-\tret = refs_verify_refnames_available(ref_store, &refnames_to_check, &affected_refnames, NULL,\n+\tret = refs_verify_refnames_available(ref_store, &refnames_to_check,\n+\t\t\t\t\t &transaction->refnames, NULL,\n \t\t\t\t\t transaction->flags & REF_TRANSACTION_FLAG_INITIAL,\n \t\t\t\t\t err);\n \tif (ret < 0)\n@@ -1402,7 +1383,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\tstrbuf_addf(err, _(\"reftable: transaction prepare: %s\"),\n \t\t\t\t reftable_error_str(ret));\n \t}\n-\tstring_list_clear(&affected_refnames, 0);\n \tstrbuf_release(&referent);\n \tstrbuf_release(&head_referent);\n \tstring_list_clear(&refnames_to_check, 0);\n\n-- \n2.47.2\n\n"},{"id":"512991","messageId":"20250225-245-partially-atomic-ref-updates-v2-3-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 3/7] refs/files: remove duplicate duplicates check","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:06Z","receivedAt":"2025-02-25T09:29:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Within the files reference backend's transaction's 'finish' phase, a\nverification step is currently performed wherein the refnames list is\nsorted and examined for multiple updates targeting the same refname.\n\nIt has been observed that this verification is redundant, as an\nidentical check is already executed during the transaction's 'prepare'\nstage. Since the refnames list remains unmodified following the\n'prepare' stage, this secondary verification can be safely eliminated.\n\nThe duplicate check has been removed accordingly, and the\n`ref_update_reject_duplicates()` function has been marked as static, as\nits usage is now confined to 'refs.c'.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 9 +++++++--\n refs/files-backend.c | 6 ------\n refs/refs-internal.h | 8 --------\n 3 files changed, 7 insertions(+), 16 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex ab69746947..69f385f344 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2303,8 +2303,13 @@ int refs_update_symref_extended(struct ref_store *refs, const char *ref,\n \treturn ret;\n }\n \n-int ref_update_reject_duplicates(struct string_list *refnames,\n-\t\t\t\t struct strbuf *err)\n+/*\n+ * Write an error to `err` and return a nonzero value iff the same\n+ * refname appears multiple times in `refnames`. `refnames` must be\n+ * sorted on entry to this function.\n+ */\n+static int ref_update_reject_duplicates(struct string_list *refnames,\n+\t\t\t\t\tstruct strbuf *err)\n {\n \tsize_t i, n = refnames->nr;\n \ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 85ed85ad87..7c6a0b3478 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -3016,12 +3016,6 @@ static int files_transaction_finish_initial(struct files_ref_store *refs,\n \tif (transaction->state != REF_TRANSACTION_PREPARED)\n \t\tBUG(\"commit called for transaction that is not prepared\");\n \n-\tstring_list_sort(&transaction->refnames);\n-\tif (ref_update_reject_duplicates(&transaction->refnames, err)) {\n-\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\tgoto cleanup;\n-\t}\n-\n \t/*\n \t * It's really undefined to call this function in an active\n \t * repository or when there are existing references: we are\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex a6e05eaecd..4b7fc8f1ab 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -142,14 +142,6 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t struct object_id *oid, struct strbuf *referent,\n \t\t unsigned int *type, int *failure_errno);\n \n-/*\n- * Write an error to `err` and return a nonzero value iff the same\n- * refname appears multiple times in `refnames`. `refnames` must be\n- * sorted on entry to this function.\n- */\n-int ref_update_reject_duplicates(struct string_list *refnames,\n-\t\t\t\t struct strbuf *err);\n-\n /*\n * Add a ref_update with the specified properties to transaction, and\n * return a pointer to the new object. This function does not verify\n\n-- \n2.47.2\n\n"},{"id":"512993","messageId":"20250225-245-partially-atomic-ref-updates-v2-4-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 4/7] refs/reftable: extract code from the transaction preparation","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:07Z","receivedAt":"2025-02-25T09:29:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Extract the core logic for preparing individual reference updates from\n`reftable_be_transaction_prepare()` into `prepare_single_update()`. This\ndedicated function now handles all validation and preparation steps for\neach reference update in the transaction, including object ID\nverification, HEAD reference handling, and symref processing.\n\nThe refactoring consolidates all reference update validation into a\nsingle logical block, which improves code maintainability and\nreadability. More importantly, this restructuring lays the groundwork\nfor implementing partial transaction support in the reftable backend,\nwhich will be introduced in the following commit.\n\nNo functional changes are included in this commit - it is purely a code\nreorganization to support future enhancements.\n\nSigned-off-by: Karthik Nayak \n---\n refs/reftable-backend.c | 463 +++++++++++++++++++++++++-----------------------\n 1 file changed, 237 insertions(+), 226 deletions(-)\n\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex f616d9aabe..2c1e2995de 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1069,6 +1069,239 @@ static int queue_transaction_update(struct reftable_ref_store *refs,\n \treturn 0;\n }\n \n+static int prepare_single_update(struct reftable_ref_store *refs,\n+\t\t\t\t struct reftable_transaction_data *tx_data,\n+\t\t\t\t struct ref_transaction *transaction,\n+\t\t\t\t struct reftable_backend *be,\n+\t\t\t\t struct ref_update *u,\n+\t\t\t\t struct string_list *refnames_to_check,\n+\t\t\t\t unsigned int head_type,\n+\t\t\t\t struct strbuf *head_referent,\n+\t\t\t\t struct strbuf *referent,\n+\t\t\t\t struct strbuf *err)\n+{\n+\tstruct object_id current_oid = {0};\n+\tconst char *rewritten_ref;\n+\tint ret = 0;\n+\n+\t/*\n+\t * There is no need to reload the respective backends here as\n+\t * we have already reloaded them when preparing the transaction\n+\t * update. And given that the stacks have been locked there\n+\t * shouldn't have been any concurrent modifications of the\n+\t * stack.\n+\t */\n+\tret = backend_for(&be, refs, u->refname, &rewritten_ref, 0);\n+\tif (ret)\n+\t\treturn ret;\n+\n+\t/* Verify that the new object ID is valid. */\n+\tif ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&\n+\t !(u->flags & REF_SKIP_OID_VERIFICATION) &&\n+\t !(u->flags & REF_LOG_ONLY)) {\n+\t\tstruct object *o = parse_object(refs->base.repo, &u->new_oid);\n+\t\tif (!o) {\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t _(\"trying to write ref '%s' with nonexistent object %s\"),\n+\t\t\t\t u->refname, oid_to_hex(&u->new_oid));\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tif (o->type != OBJ_COMMIT && is_branch(u->refname)) {\n+\t\t\tstrbuf_addf(err, _(\"trying to write non-commit object %s to branch '%s'\"),\n+\t\t\t\t oid_to_hex(&u->new_oid), u->refname);\n+\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * When we update the reference that HEAD points to we enqueue\n+\t * a second log-only update for HEAD so that its reflog is\n+\t * updated accordingly.\n+\t */\n+\tif (head_type == REF_ISSYMREF &&\n+\t !(u->flags & REF_LOG_ONLY) &&\n+\t !(u->flags & REF_UPDATE_VIA_HEAD) &&\n+\t !strcmp(rewritten_ref, head_referent->buf)) {\n+\t\t/*\n+\t\t * First make sure that HEAD is not already in the\n+\t\t * transaction. This check is O(lg N) in the transaction\n+\t\t * size, but it happens at most once per transaction.\n+\t\t */\n+\t\tif (string_list_has_string(&transaction->refnames, \"HEAD\")) {\n+\t\t\t/* An entry already existed */\n+\t\t\tstrbuf_addf(err,\n+\t\t\t\t _(\"multiple updates for 'HEAD' (including one \"\n+\t\t\t\t \"via its referent '%s') are not allowed\"),\n+\t\t\t\t u->refname);\n+\t\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t\t}\n+\n+\t\tref_transaction_add_update(\n+\t\t\ttransaction, \"HEAD\",\n+\t\t\tu->flags | REF_LOG_ONLY | REF_NO_DEREF,\n+\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, NULL,\n+\t\t\tu->msg);\n+\t}\n+\n+\tret = reftable_backend_read_ref(be, rewritten_ref,\n+\t\t\t\t\t¤t_oid, referent, &u->type);\n+\tif (ret < 0)\n+\t\treturn ret;\n+\tif (ret > 0 && !ref_update_expects_existing_old_ref(u)) {\n+\t\t/*\n+\t\t * The reference does not exist, and we either have no\n+\t\t * old object ID or expect the reference to not exist.\n+\t\t * We can thus skip below safety checks as well as the\n+\t\t * symref splitting. But we do want to verify that\n+\t\t * there is no conflicting reference here so that we\n+\t\t * can output a proper error message instead of failing\n+\t\t * at a later point.\n+\t\t */\n+\t\tstring_list_append(refnames_to_check, u->refname);\n+\n+\t\t/*\n+\t\t * There is no need to write the reference deletion\n+\t\t * when the reference in question doesn't exist.\n+\t\t */\n+\t\tif ((u->flags & REF_HAVE_NEW) && !ref_update_has_null_new_value(u)) {\n+\t\t\tret = queue_transaction_update(refs, tx_data, u,\n+\t\t\t\t\t\t ¤t_oid, err);\n+\t\t\tif (ret)\n+\t\t\t\treturn ret;\n+\t\t}\n+\n+\t\treturn 0;\n+\t}\n+\tif (ret > 0) {\n+\t\t/* The reference does not exist, but we expected it to. */\n+\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\n+\n+\t\t\t\t \"unable to resolve reference '%s'\"),\n+\t\t\t ref_update_original_update_refname(u), u->refname);\n+\t\treturn -1;\n+\t}\n+\n+\tif (u->type & REF_ISSYMREF) {\n+\t\t/*\n+\t\t * The reftable stack is locked at this point already,\n+\t\t * so it is safe to call `refs_resolve_ref_unsafe()`\n+\t\t * here without causing races.\n+\t\t */\n+\t\tconst char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,\n+\t\t\t\t\t\t\t ¤t_oid, NULL);\n+\n+\t\tif (u->flags & REF_NO_DEREF) {\n+\t\t\tif (u->flags & REF_HAVE_OLD && !resolved) {\n+\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t\t \"error reading reference\"), u->refname);\n+\t\t\t\treturn -1;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tstruct ref_update *new_update;\n+\t\t\tint new_flags;\n+\n+\t\t\tnew_flags = u->flags;\n+\t\t\tif (!strcmp(rewritten_ref, \"HEAD\"))\n+\t\t\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n+\n+\t\t\tif (string_list_has_string(&transaction->refnames, referent->buf)) {\n+\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t _(\"multiple updates for '%s' (including one \"\n+\t\t\t\t\t \"via symref '%s') are not allowed\"),\n+\t\t\t\t\t referent->buf, u->refname);\n+\t\t\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t\t\t}\n+\n+\t\t\t/*\n+\t\t\t * If we are updating a symref (eg. HEAD), we should also\n+\t\t\t * update the branch that the symref points to.\n+\t\t\t *\n+\t\t\t * This is generic functionality, and would be better\n+\t\t\t * done in refs.c, but the current implementation is\n+\t\t\t * intertwined with the locking in files-backend.c.\n+\t\t\t */\n+\t\t\tnew_update = ref_transaction_add_update(\n+\t\t\t\ttransaction, referent->buf, new_flags,\n+\t\t\t\tu->new_target ? NULL : &u->new_oid,\n+\t\t\t\tu->old_target ? NULL : &u->old_oid,\n+\t\t\t\tu->new_target, u->old_target,\n+\t\t\t\tu->committer_info, u->msg);\n+\n+\t\t\tnew_update->parent_update = u;\n+\n+\t\t\t/*\n+\t\t\t * Change the symbolic ref update to log only. Also, it\n+\t\t\t * doesn't need to check its old OID value, as that will be\n+\t\t\t * done when new_update is processed.\n+\t\t\t */\n+\t\t\tu->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n+\t\t\tu->flags &= ~REF_HAVE_OLD;\n+\t\t}\n+\t}\n+\n+\t/*\n+\t * Verify that the old object matches our expectations. Note\n+\t * that the error messages here do not make a lot of sense in\n+\t * the context of the reftable backend as we never lock\n+\t * individual refs. But the error messages match what the files\n+\t * backend returns, which keeps our tests happy.\n+\t */\n+\tif (u->old_target) {\n+\t\tif (!(u->type & REF_ISSYMREF)) {\n+\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t \"expected symref with target '%s': \"\n+\t\t\t\t\t \"but is a regular ref\"),\n+\t\t\t\t ref_update_original_update_refname(u),\n+\t\t\t\t u->old_target);\n+\t\t\treturn -1;\n+\t\t}\n+\n+\t\tif (ref_update_check_old_target(referent->buf, u, err)) {\n+\t\t\treturn -1;\n+\t\t}\n+\t} else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) {\n+\t\tif (is_null_oid(&u->old_oid)) {\n+\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t \"reference already exists\"),\n+\t\t\t\t ref_update_original_update_refname(u));\n+\t\t\treturn TRANSACTION_CREATE_EXISTS;\n+\t\t}\n+\t\telse if (is_null_oid(¤t_oid))\n+\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t \"reference is missing but expected %s\"),\n+\t\t\t\t ref_update_original_update_refname(u),\n+\t\t\t\t oid_to_hex(&u->old_oid));\n+\t\telse\n+\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n+\t\t\t\t\t \"is at %s but expected %s\"),\n+\t\t\t\t ref_update_original_update_refname(u),\n+\t\t\t\t oid_to_hex(¤t_oid),\n+\t\t\t\t oid_to_hex(&u->old_oid));\n+\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t}\n+\n+\t/*\n+\t * If all of the following conditions are true:\n+\t *\n+\t * - We're not about to write a symref.\n+\t * - We're not about to write a log-only entry.\n+\t * - Old and new object ID are different.\n+\t *\n+\t * Then we're essentially doing a no-op update that can be\n+\t * skipped. This is not only for the sake of efficiency, but\n+\t * also skips writing unneeded reflog entries.\n+\t */\n+\tif ((u->type & REF_ISSYMREF) ||\n+\t (u->flags & REF_LOG_ONLY) ||\n+\t (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid)))\n+\t\treturn queue_transaction_update(refs, tx_data, u,\n+\t\t\t\t\t ¤t_oid, err);\n+\n+\treturn 0;\n+}\n+\n static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t\t struct strbuf *err)\n@@ -1133,234 +1366,12 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \tret = 0;\n \n \tfor (i = 0; i < transaction->nr; i++) {\n-\t\tstruct ref_update *u = transaction->updates[i];\n-\t\tstruct object_id current_oid = {0};\n-\t\tconst char *rewritten_ref;\n-\n-\t\t/*\n-\t\t * There is no need to reload the respective backends here as\n-\t\t * we have already reloaded them when preparing the transaction\n-\t\t * update. And given that the stacks have been locked there\n-\t\t * shouldn't have been any concurrent modifications of the\n-\t\t * stack.\n-\t\t */\n-\t\tret = backend_for(&be, refs, u->refname, &rewritten_ref, 0);\n+\t\tret = prepare_single_update(refs, tx_data, transaction, be,\n+\t\t\t\t\t transaction->updates[i],\n+\t\t\t\t\t &refnames_to_check, head_type,\n+\t\t\t\t\t &head_referent, &referent, err);\n \t\tif (ret)\n \t\t\tgoto done;\n-\n-\t\t/* Verify that the new object ID is valid. */\n-\t\tif ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&\n-\t\t !(u->flags & REF_SKIP_OID_VERIFICATION) &&\n-\t\t !(u->flags & REF_LOG_ONLY)) {\n-\t\t\tstruct object *o = parse_object(refs->base.repo, &u->new_oid);\n-\t\t\tif (!o) {\n-\t\t\t\tstrbuf_addf(err,\n-\t\t\t\t\t _(\"trying to write ref '%s' with nonexistent object %s\"),\n-\t\t\t\t\t u->refname, oid_to_hex(&u->new_oid));\n-\t\t\t\tret = -1;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\n-\t\t\tif (o->type != OBJ_COMMIT && is_branch(u->refname)) {\n-\t\t\t\tstrbuf_addf(err, _(\"trying to write non-commit object %s to branch '%s'\"),\n-\t\t\t\t\t oid_to_hex(&u->new_oid), u->refname);\n-\t\t\t\tret = -1;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t}\n-\n-\t\t/*\n-\t\t * When we update the reference that HEAD points to we enqueue\n-\t\t * a second log-only update for HEAD so that its reflog is\n-\t\t * updated accordingly.\n-\t\t */\n-\t\tif (head_type == REF_ISSYMREF &&\n-\t\t !(u->flags & REF_LOG_ONLY) &&\n-\t\t !(u->flags & REF_UPDATE_VIA_HEAD) &&\n-\t\t !strcmp(rewritten_ref, head_referent.buf)) {\n-\t\t\t/*\n-\t\t\t * First make sure that HEAD is not already in the\n-\t\t\t * transaction. This check is O(lg N) in the transaction\n-\t\t\t * size, but it happens at most once per transaction.\n-\t\t\t */\n-\t\t\tif (string_list_has_string(&transaction->refnames, \"HEAD\")) {\n-\t\t\t\t/* An entry already existed */\n-\t\t\t\tstrbuf_addf(err,\n-\t\t\t\t\t _(\"multiple updates for 'HEAD' (including one \"\n-\t\t\t\t\t \"via its referent '%s') are not allowed\"),\n-\t\t\t\t\t u->refname);\n-\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\n-\t\t\tref_transaction_add_update(\n-\t\t\t\ttransaction, \"HEAD\",\n-\t\t\t\tu->flags | REF_LOG_ONLY | REF_NO_DEREF,\n-\t\t\t\t&u->new_oid, &u->old_oid, NULL, NULL, NULL,\n-\t\t\t\tu->msg);\n-\t\t}\n-\n-\t\tret = reftable_backend_read_ref(be, rewritten_ref,\n-\t\t\t\t\t\t¤t_oid, &referent, &u->type);\n-\t\tif (ret < 0)\n-\t\t\tgoto done;\n-\t\tif (ret > 0 && !ref_update_expects_existing_old_ref(u)) {\n-\t\t\t/*\n-\t\t\t * The reference does not exist, and we either have no\n-\t\t\t * old object ID or expect the reference to not exist.\n-\t\t\t * We can thus skip below safety checks as well as the\n-\t\t\t * symref splitting. But we do want to verify that\n-\t\t\t * there is no conflicting reference here so that we\n-\t\t\t * can output a proper error message instead of failing\n-\t\t\t * at a later point.\n-\t\t\t */\n-\t\t\tstring_list_append(&refnames_to_check, u->refname);\n-\n-\t\t\t/*\n-\t\t\t * There is no need to write the reference deletion\n-\t\t\t * when the reference in question doesn't exist.\n-\t\t\t */\n-\t\t\t if ((u->flags & REF_HAVE_NEW) && !ref_update_has_null_new_value(u)) {\n-\t\t\t\t ret = queue_transaction_update(refs, tx_data, u,\n-\t\t\t\t\t\t\t\t¤t_oid, err);\n-\t\t\t\t if (ret)\n-\t\t\t\t\t goto done;\n-\t\t\t }\n-\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (ret > 0) {\n-\t\t\t/* The reference does not exist, but we expected it to. */\n-\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t \"unable to resolve reference '%s'\"),\n-\t\t\t\t ref_update_original_update_refname(u), u->refname);\n-\t\t\tret = -1;\n-\t\t\tgoto done;\n-\t\t}\n-\n-\t\tif (u->type & REF_ISSYMREF) {\n-\t\t\t/*\n-\t\t\t * The reftable stack is locked at this point already,\n-\t\t\t * so it is safe to call `refs_resolve_ref_unsafe()`\n-\t\t\t * here without causing races.\n-\t\t\t */\n-\t\t\tconst char *resolved = refs_resolve_ref_unsafe(&refs->base, u->refname, 0,\n-\t\t\t\t\t\t\t\t ¤t_oid, NULL);\n-\n-\t\t\tif (u->flags & REF_NO_DEREF) {\n-\t\t\t\tif (u->flags & REF_HAVE_OLD && !resolved) {\n-\t\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t\t\t \"error reading reference\"), u->refname);\n-\t\t\t\t\tret = -1;\n-\t\t\t\t\tgoto done;\n-\t\t\t\t}\n-\t\t\t} else {\n-\t\t\t\tstruct ref_update *new_update;\n-\t\t\t\tint new_flags;\n-\n-\t\t\t\tnew_flags = u->flags;\n-\t\t\t\tif (!strcmp(rewritten_ref, \"HEAD\"))\n-\t\t\t\t\tnew_flags |= REF_UPDATE_VIA_HEAD;\n-\n-\t\t\t\tif (string_list_has_string(&transaction->refnames, referent.buf)) {\n-\t\t\t\t\tstrbuf_addf(err,\n-\t\t\t\t\t\t _(\"multiple updates for '%s' (including one \"\n-\t\t\t\t\t\t \"via symref '%s') are not allowed\"),\n-\t\t\t\t\t\t referent.buf, u->refname);\n-\t\t\t\t\tret = TRANSACTION_NAME_CONFLICT;\n-\t\t\t\t\tgoto done;\n-\t\t\t\t}\n-\n-\t\t\t\t/*\n-\t\t\t\t * If we are updating a symref (eg. HEAD), we should also\n-\t\t\t\t * update the branch that the symref points to.\n-\t\t\t\t *\n-\t\t\t\t * This is generic functionality, and would be better\n-\t\t\t\t * done in refs.c, but the current implementation is\n-\t\t\t\t * intertwined with the locking in files-backend.c.\n-\t\t\t\t */\n-\t\t\t\tnew_update = ref_transaction_add_update(\n-\t\t\t\t\ttransaction, referent.buf, new_flags,\n-\t\t\t\t\tu->new_target ? NULL : &u->new_oid,\n-\t\t\t\t\tu->old_target ? NULL : &u->old_oid,\n-\t\t\t\t\tu->new_target, u->old_target,\n-\t\t\t\t\tu->committer_info, u->msg);\n-\n-\t\t\t\tnew_update->parent_update = u;\n-\n-\t\t\t\t/*\n-\t\t\t\t * Change the symbolic ref update to log only. Also, it\n-\t\t\t\t * doesn't need to check its old OID value, as that will be\n-\t\t\t\t * done when new_update is processed.\n-\t\t\t\t */\n-\t\t\t\tu->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n-\t\t\t\tu->flags &= ~REF_HAVE_OLD;\n-\t\t\t}\n-\t\t}\n-\n-\t\t/*\n-\t\t * Verify that the old object matches our expectations. Note\n-\t\t * that the error messages here do not make a lot of sense in\n-\t\t * the context of the reftable backend as we never lock\n-\t\t * individual refs. But the error messages match what the files\n-\t\t * backend returns, which keeps our tests happy.\n-\t\t */\n-\t\tif (u->old_target) {\n-\t\t\tif (!(u->type & REF_ISSYMREF)) {\n-\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t\t \"expected symref with target '%s': \"\n-\t\t\t\t\t \"but is a regular ref\"),\n-\t\t\t\t\t ref_update_original_update_refname(u),\n-\t\t\t\t\t u->old_target);\n-\t\t\t\tret = -1;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\n-\t\t\tif (ref_update_check_old_target(referent.buf, u, err)) {\n-\t\t\t\tret = -1;\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t} else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) {\n-\t\t\tret = TRANSACTION_NAME_CONFLICT;\n-\t\t\tif (is_null_oid(&u->old_oid)) {\n-\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t\t\t \"reference already exists\"),\n-\t\t\t\t\t ref_update_original_update_refname(u));\n-\t\t\t\tret = TRANSACTION_CREATE_EXISTS;\n-\t\t\t}\n-\t\t\telse if (is_null_oid(¤t_oid))\n-\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t\t\t \"reference is missing but expected %s\"),\n-\t\t\t\t\t ref_update_original_update_refname(u),\n-\t\t\t\t\t oid_to_hex(&u->old_oid));\n-\t\t\telse\n-\t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n-\t\t\t\t\t\t \"is at %s but expected %s\"),\n-\t\t\t\t\t ref_update_original_update_refname(u),\n-\t\t\t\t\t oid_to_hex(¤t_oid),\n-\t\t\t\t\t oid_to_hex(&u->old_oid));\n-\t\t\tgoto done;\n-\t\t}\n-\n-\t\t/*\n-\t\t * If all of the following conditions are true:\n-\t\t *\n-\t\t * - We're not about to write a symref.\n-\t\t * - We're not about to write a log-only entry.\n-\t\t * - Old and new object ID are different.\n-\t\t *\n-\t\t * Then we're essentially doing a no-op update that can be\n-\t\t * skipped. This is not only for the sake of efficiency, but\n-\t\t * also skips writing unneeded reflog entries.\n-\t\t */\n-\t\tif ((u->type & REF_ISSYMREF) ||\n-\t\t (u->flags & REF_LOG_ONLY) ||\n-\t\t (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid))) {\n-\t\t\tret = queue_transaction_update(refs, tx_data, u,\n-\t\t\t\t\t\t ¤t_oid, err);\n-\t\t\tif (ret)\n-\t\t\t\tgoto done;\n-\t\t}\n \t}\n \n \tstring_list_sort(&refnames_to_check);\n\n-- \n2.47.2\n\n"},{"id":"512994","messageId":"20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:09Z","receivedAt":"2025-02-25T09:29:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Git's reference transactions are all-or-nothing: either all updates\nsucceed, or none do. While this atomic behavior is generally desirable,\nit can be suboptimal especially when using the reftable backend, where\nbatching multiple reference updates into a single transaction is more\nefficient than performing them sequentially.\n\nIntroduce partial transaction support with a new flag,\n'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\nindividual reference updates that would typically cause the entire\ntransaction to fail due to non-system-related errors to be marked as\nrejected while permitting other updates to proceed. Non-system-related\nerrors include issues caused by user-provided input values, whereas\nsystem-related errors, such as I/O failures or memory issues, continue\nto result in a full transaction failure. This approach enhances\nflexibility while preserving transactional integrity where necessary.\n\nThe implementation introduces several key components:\n\n - Add 'rejection_err' field to struct `ref_update` to track failed\n updates with failure reason.\n\n - Modify reference backends (files, packed, reftable) to handle\n partial transactions by using `ref_transaction_set_rejected()`\n instead of failing the entire transaction when\n `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n\n - Add `ref_transaction_for_each_rejected_update()` to let callers\n examine which updates were rejected and why.\n\nThis foundational change enables partial transaction support throughout\nthe reference subsystem. The next commit will expose this capability to\nusers by adding a `--allow-partial` flag to 'git-update-ref(1)',\nproviding both a user-facing feature and a testable implementation.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 31 +++++++++++++++++++++++++++++++\n refs.h | 22 ++++++++++++++++++++++\n refs/files-backend.c | 12 +++++++++++-\n refs/packed-backend.c | 30 ++++++++++++++++++++++++++++--\n refs/refs-internal.h | 13 +++++++++++++\n refs/reftable-backend.c | 12 +++++++++++-\n 6 files changed, 116 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex f989a46a5a..243c09c368 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1211,6 +1211,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n \tfree(transaction);\n }\n \n+void ref_transaction_set_rejected(struct ref_transaction *transaction,\n+\t\t\t\t size_t update_idx,\n+\t\t\t\t enum transaction_error err)\n+{\n+\tif (update_idx >= transaction->nr)\n+\t\tBUG(\"trying to set rejection on invalid update index\");\n+\ttransaction->updates[update_idx]->rejection_err = err;\n+}\n+\n struct ref_update *ref_transaction_add_update(\n \t\tstruct ref_transaction *transaction,\n \t\tconst char *refname, unsigned int flags,\n@@ -1236,6 +1245,7 @@ struct ref_update *ref_transaction_add_update(\n \ttransaction->updates[transaction->nr++] = update;\n \n \tupdate->flags = flags;\n+\tupdate->rejection_err = TRANSACTION_OK;\n \n \tupdate->new_target = xstrdup_or_null(new_target);\n \tupdate->old_target = xstrdup_or_null(old_target);\n@@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n \t}\n }\n \n+void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tif (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))\n+\t\treturn;\n+\n+\tfor (size_t i = 0; i < transaction->nr; i++) {\n+\t\tstruct ref_update *update = transaction->updates[i];\n+\n+\t\tif (!update->rejection_err)\n+\t\t\tcontinue;\n+\n+\t\tcb(update->refname,\n+\t\t (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n+\t\t (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n+\t\t update->old_target, update->new_target,\n+\t\t update->rejection_err, cb_data);\n+\t}\n+}\n+\n int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n \t\t struct string_list *refnames, unsigned int flags)\n {\ndiff --git a/refs.h b/refs.h\nindex 8e9ead174c..e4a6a8218f 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -675,6 +675,13 @@ enum ref_transaction_flag {\n \t * either be absent or null_oid.\n \t */\n \tREF_TRANSACTION_FLAG_INITIAL = (1 << 0),\n+\n+\t/*\n+\t * The transaction mechanism by default fails all updates if any conflict\n+\t * is detected. This flag allows transactions to partially apply updates\n+\t * while rejecting updates which do not match the expected state.\n+\t */\n+\tREF_TRANSACTION_ALLOW_PARTIAL = (1 << 1),\n };\n \n /*\n@@ -905,6 +912,21 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n \t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n \t\t\t\t\t void *cb_data);\n \n+/*\n+ * Execute the given callback function for each of the reference updates which\n+ * have been rejected in the given transaction.\n+ */\n+typedef void ref_transaction_for_each_rejected_update_fn(const char *refname,\n+\t\t\t\t\t\t\t const struct object_id *old_oid,\n+\t\t\t\t\t\t\t const struct object_id *new_oid,\n+\t\t\t\t\t\t\t const char *old_target,\n+\t\t\t\t\t\t\t const char *new_target,\n+\t\t\t\t\t\t\t enum transaction_error err,\n+\t\t\t\t\t\t\t void *cb_data);\n+void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n+\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n+\t\t\t\t\t void *cb_data);\n+\n /*\n * Free `*transaction` and all associated data.\n */\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3b0adf8bb2..d0a53c9ace 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\tret = lock_ref_for_update(refs, update, transaction,\n \t\t\t\t\t head_ref, &refnames_to_check,\n \t\t\t\t\t err);\n-\t\tif (ret)\n+\t\tif (ret) {\n+\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n+\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n+\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n+\n+\t\t\t\tstrbuf_setlen(err, 0);\n+\t\t\t\tret = TRANSACTION_OK;\n+\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tgoto cleanup;\n+\t\t}\n \n \t\tif (update->flags & REF_DELETING &&\n \t\t !(update->flags & REF_LOG_ONLY) &&\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 75e1ebf67d..0857204213 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1324,10 +1324,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,\n * remain locked when it is done.\n */\n static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n-\t\t\t\t\t\t struct string_list *updates,\n+\t\t\t\t\t\t struct ref_transaction *transaction,\n \t\t\t\t\t\t struct strbuf *err)\n {\n \tenum transaction_error ret = TRANSACTION_GENERIC_ERROR;\n+\tstruct string_list *updates = &transaction->refnames;\n \tstruct ref_iterator *iter = NULL;\n \tsize_t i;\n \tint ok;\n@@ -1408,6 +1409,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t\t \"reference already exists\",\n \t\t\t\t\t\t update->refname);\n \t\t\t\t\tret = TRANSACTION_CREATE_EXISTS;\n+\n+\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n+\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n+\t\t\t\t\t\tstrbuf_setlen(err, 0);\n+\t\t\t\t\t\tret = 0;\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t}\n+\n \t\t\t\t\tgoto error;\n \t\t\t\t} else if (!oideq(&update->old_oid, iter->oid)) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n@@ -1416,6 +1425,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t\t oid_to_hex(iter->oid),\n \t\t\t\t\t\t oid_to_hex(&update->old_oid));\n \t\t\t\t\tret = TRANSACTION_INCORRECT_OLD_VALUE;\n+\n+\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n+\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n+\t\t\t\t\t\tstrbuf_setlen(err, 0);\n+\t\t\t\t\t\tret = 0;\n+\t\t\t\t\t\tcontinue;\n+\t\t\t\t\t}\n+\n \t\t\t\t\tgoto error;\n \t\t\t\t}\n \t\t\t}\n@@ -1453,6 +1470,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t update->refname,\n \t\t\t\t\t oid_to_hex(&update->old_oid));\n \t\t\t\treturn TRANSACTION_NONEXISTENT_REF;\n+\n+\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n+\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n+\t\t\t\t\tstrbuf_setlen(err, 0);\n+\t\t\t\t\tret = 0;\n+\t\t\t\t\tcontinue;\n+\t\t\t\t}\n+\n \t\t\t\tgoto error;\n \t\t\t}\n \t\t}\n@@ -1518,6 +1543,7 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n write_error:\n \tstrbuf_addf(err, \"error writing to %s: %s\",\n \t\t get_tempfile_path(refs->tempfile), strerror(errno));\n+\tret = TRANSACTION_GENERIC_ERROR;\n \n error:\n \tref_iterator_free(iter);\n@@ -1676,7 +1702,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t\tdata->own_lock = 1;\n \t}\n \n-\tret = write_with_updates(refs, &transaction->refnames, err);\n+\tret = write_with_updates(refs, transaction, err);\n \tif (ret)\n \t\tgoto failure;\n \ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex c97045fbed..7196f2d880 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -123,6 +123,11 @@ struct ref_update {\n \t */\n \tuint64_t index;\n \n+\t/*\n+\t * Used in partial transactions to mark if a given update was rejected.\n+\t */\n+\tenum transaction_error rejection_err;\n+\n \t/*\n \t * If this ref_update was split off of a symref update via\n \t * split_symref_update(), then this member points at that\n@@ -142,6 +147,14 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n \t\t struct object_id *oid, struct strbuf *referent,\n \t\t unsigned int *type, int *failure_errno);\n \n+/*\n+ * Mark a given update as rejected with a given reason. To be used in conjuction\n+ * with the `REF_TRANSACTION_ALLOW_PARTIAL` flag to allow partial transactions.\n+ */\n+void ref_transaction_set_rejected(struct ref_transaction *transaction,\n+\t\t\t\t size_t update_idx,\n+\t\t\t\t enum transaction_error err);\n+\n /*\n * Add a ref_update with the specified properties to transaction, and\n * return a pointer to the new object. This function does not verify\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex e1fd9c2de2..83cf8d582b 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1374,8 +1374,18 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \t\t\t\t\t transaction->updates[i],\n \t\t\t\t\t &refnames_to_check, head_type,\n \t\t\t\t\t &head_referent, &referent, err);\n-\t\tif (ret)\n+\t\tif (ret) {\n+\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n+\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n+\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n+\n+\t\t\t\tstrbuf_setlen(err, 0);\n+\t\t\t\tret = TRANSACTION_OK;\n+\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tgoto done;\n+\t\t}\n \t}\n \n \tstring_list_sort(&refnames_to_check);\n\n-- \n2.47.2\n\n"},{"id":"512995","messageId":"20250225-245-partially-atomic-ref-updates-v2-5-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 5/7] refs: introduce enum-based transaction error types","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:08Z","receivedAt":"2025-02-25T09:29:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Replace preprocessor-defined transaction errors with a strongly-typed\nenum `transaction_error`. This change:\n\n - Improves type safety and function signature clarity.\n - Makes error handling more explicit and discoverable.\n - Maintains existing error cases, while adding new error cases for\n common scenarios.\n\nThis refactoring paves the way for more comprehensive error handling\nwhich we will utilize in the upcoming commits to add partial transaction\nsupport.\n\nSigned-off-by: Karthik Nayak \n---\n refs.c | 44 +++++++------\n refs.h | 56 ++++++++++------\n refs/files-backend.c | 167 ++++++++++++++++++++++++------------------------\n refs/packed-backend.c | 21 +++---\n refs/refs-internal.h | 5 +-\n refs/reftable-backend.c | 59 +++++++++--------\n 6 files changed, 191 insertions(+), 161 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 69f385f344..f989a46a5a 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2497,18 +2497,18 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \treturn ret;\n }\n \n-int refs_verify_refnames_available(struct ref_store *refs,\n-\t\t\t\t const struct string_list *refnames,\n-\t\t\t\t const struct string_list *extras,\n-\t\t\t\t const struct string_list *skip,\n-\t\t\t\t unsigned int initial_transaction,\n-\t\t\t\t struct strbuf *err)\n+enum transaction_error refs_verify_refnames_available(struct ref_store *refs,\n+\t\t\t\t\t\t const struct string_list *refnames,\n+\t\t\t\t\t\t const struct string_list *extras,\n+\t\t\t\t\t\t const struct string_list *skip,\n+\t\t\t\t\t\t unsigned int initial_transaction,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct strbuf dirname = STRBUF_INIT;\n \tstruct strbuf referent = STRBUF_INIT;\n \tstruct ref_iterator *iter = NULL;\n \tstruct strset dirnames;\n-\tint ret = -1;\n+\tint ret = TRANSACTION_NAME_CONFLICT;\n \n \t/*\n \t * For the sake of comments in this function, suppose that\n@@ -2624,12 +2624,12 @@ int refs_verify_refnames_available(struct ref_store *refs,\n \treturn ret;\n }\n \n-int refs_verify_refname_available(struct ref_store *refs,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct string_list *extras,\n-\t\t\t\t const struct string_list *skip,\n-\t\t\t\t unsigned int initial_transaction,\n-\t\t\t\t struct strbuf *err)\n+enum transaction_error refs_verify_refname_available(struct ref_store *refs,\n+\t\t\t\t\t\t const char *refname,\n+\t\t\t\t\t\t const struct string_list *extras,\n+\t\t\t\t\t\t const struct string_list *skip,\n+\t\t\t\t\t\t unsigned int initial_transaction,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct string_list_item item = { .string = (char *) refname };\n \tstruct string_list refnames = {\n@@ -2817,26 +2817,28 @@ int ref_update_has_null_new_value(struct ref_update *update)\n \treturn !update->new_target && is_null_oid(&update->new_oid);\n }\n \n-int ref_update_check_old_target(const char *referent, struct ref_update *update,\n-\t\t\t\tstruct strbuf *err)\n+enum transaction_error ref_update_check_old_target(const char *referent,\n+\t\t\t\t\t struct ref_update *update,\n+\t\t\t\t\t struct strbuf *err)\n {\n \tif (!update->old_target)\n \t\tBUG(\"called without old_target set\");\n \n \tif (!strcmp(referent, update->old_target))\n-\t\treturn 0;\n+\t\treturn TRANSACTION_OK;\n \n-\tif (!strcmp(referent, \"\"))\n+\tif (!strcmp(referent, \"\")) {\n \t\tstrbuf_addf(err, \"verifying symref target: '%s': \"\n \t\t\t \"reference is missing but expected %s\",\n \t\t\t ref_update_original_update_refname(update),\n \t\t\t update->old_target);\n-\telse\n-\t\tstrbuf_addf(err, \"verifying symref target: '%s': \"\n-\t\t\t \"is at %s but expected %s\",\n+\t\treturn TRANSACTION_NONEXISTENT_REF;\n+\t}\n+\n+\tstrbuf_addf(err, \"verifying symref target: '%s': is at %s but expected %s\",\n \t\t\t ref_update_original_update_refname(update),\n \t\t\t referent, update->old_target);\n-\treturn -1;\n+\treturn TRANSACTION_INCORRECT_OLD_VALUE;\n }\n \n struct migration_data {\ndiff --git a/refs.h b/refs.h\nindex b14ba1f9ff..8e9ead174c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -16,6 +16,31 @@ struct worktree;\n enum ref_storage_format ref_storage_format_by_name(const char *name);\n const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format);\n \n+/*\n+ * enum transaction_error represents the following return codes:\n+ * TRANSACTION_OK: success code.\n+ * TRANSACTION_GENERIC_ERROR error_code: default error code.\n+ * TRANSACTION_NAME_CONFLICT error_code: ref name conflict like A vs A/B.\n+ * TRANSACTION_CREATE_EXISTS error_code: ref to be created already exists.\n+ * TRANSACTION_NONEXISTENT_REF error_code: ref expected but doesn't exist.\n+ * TRANSACTION_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of\n+ * reference doesn't match actual.\n+ * TRANSACTION_INVALID_NEW_VALUE error_code: provided new_oid or new_target is\n+ * invalid.\n+ * TRANSACTION_EXPECTED_SYMREF error_code: expected ref to be symref, but is a\n+ * regular ref.\n+ */\n+enum transaction_error {\n+\tTRANSACTION_OK = 0,\n+\tTRANSACTION_GENERIC_ERROR = -1,\n+\tTRANSACTION_NAME_CONFLICT = -2,\n+\tTRANSACTION_CREATE_EXISTS = -3,\n+\tTRANSACTION_NONEXISTENT_REF = -4,\n+\tTRANSACTION_INCORRECT_OLD_VALUE = -5,\n+\tTRANSACTION_INVALID_NEW_VALUE = -6,\n+\tTRANSACTION_EXPECTED_SYMREF = -7,\n+};\n+\n /*\n * Resolve a reference, recursively following symbolic references.\n *\n@@ -117,24 +142,24 @@ int refs_read_symbolic_ref(struct ref_store *ref_store, const char *refname,\n *\n * extras and skip must be sorted.\n */\n-int refs_verify_refname_available(struct ref_store *refs,\n-\t\t\t\t const char *refname,\n-\t\t\t\t const struct string_list *extras,\n-\t\t\t\t const struct string_list *skip,\n-\t\t\t\t unsigned int initial_transaction,\n-\t\t\t\t struct strbuf *err);\n+enum transaction_error refs_verify_refname_available(struct ref_store *refs,\n+\t\t\t\t\t\t const char *refname,\n+\t\t\t\t\t\t const struct string_list *extras,\n+\t\t\t\t\t\t const struct string_list *skip,\n+\t\t\t\t\t\t unsigned int initial_transaction,\n+\t\t\t\t\t\t struct strbuf *err);\n \n /*\n * Same as `refs_verify_refname_available()`, but checking for a list of\n * refnames instead of only a single item. This is more efficient in the case\n * where one needs to check multiple refnames.\n */\n-int refs_verify_refnames_available(struct ref_store *refs,\n-\t\t\t\t const struct string_list *refnames,\n-\t\t\t\t const struct string_list *extras,\n-\t\t\t\t const struct string_list *skip,\n-\t\t\t\t unsigned int initial_transaction,\n-\t\t\t\t struct strbuf *err);\n+enum transaction_error refs_verify_refnames_available(struct ref_store *refs,\n+\t\t\t\t\t\t const struct string_list *refnames,\n+\t\t\t\t\t\t const struct string_list *extras,\n+\t\t\t\t\t\t const struct string_list *skip,\n+\t\t\t\t\t\t unsigned int initial_transaction,\n+\t\t\t\t\t\t struct strbuf *err);\n \n int refs_ref_exists(struct ref_store *refs, const char *refname);\n \n@@ -830,13 +855,6 @@ int ref_transaction_verify(struct ref_transaction *transaction,\n \t\t\t unsigned int flags,\n \t\t\t struct strbuf *err);\n \n-/* Naming conflict (for example, the ref names A and A/B conflict). */\n-#define TRANSACTION_NAME_CONFLICT -1\n-/* When only creation was requested, but the ref already exists. */\n-#define TRANSACTION_CREATE_EXISTS -2\n-/* All other errors. */\n-#define TRANSACTION_GENERIC_ERROR -3\n-\n /*\n * Perform the preparatory stages of committing `transaction`. Acquire\n * any needed locks, check preconditions, etc.; basically, do as much\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 7c6a0b3478..3b0adf8bb2 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -676,19 +676,19 @@ static void unlock_ref(struct ref_lock *lock)\n * avoided, namely if we were successfully able to read the ref\n * - Generate informative error messages in the case of failure\n */\n-static int lock_raw_ref(struct files_ref_store *refs,\n-\t\t\tconst char *refname, int mustexist,\n-\t\t\tstruct string_list *refnames_to_check,\n-\t\t\tconst struct string_list *extras,\n-\t\t\tstruct ref_lock **lock_p,\n-\t\t\tstruct strbuf *referent,\n-\t\t\tunsigned int *type,\n-\t\t\tstruct strbuf *err)\n+static enum transaction_error lock_raw_ref(struct files_ref_store *refs,\n+\t\t\t\t\t const char *refname, int mustexist,\n+\t\t\t\t\t struct string_list *refnames_to_check,\n+\t\t\t\t\t const struct string_list *extras,\n+\t\t\t\t\t struct ref_lock **lock_p,\n+\t\t\t\t\t struct strbuf *referent,\n+\t\t\t\t\t unsigned int *type,\n+\t\t\t\t\t struct strbuf *err)\n {\n+\tenum transaction_error ret = TRANSACTION_GENERIC_ERROR;\n \tstruct ref_lock *lock;\n \tstruct strbuf ref_file = STRBUF_INIT;\n \tint attempts_remaining = 3;\n-\tint ret = TRANSACTION_GENERIC_ERROR;\n \tint failure_errno;\n \n \tassert(err);\n@@ -728,6 +728,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t\t\tstrbuf_reset(err);\n \t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n \t\t\t\t\t refname);\n+\t\t\t\tret = TRANSACTION_NONEXISTENT_REF;\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * The error message set by\n@@ -788,6 +789,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t\t\t/* Garden variety missing reference. */\n \t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n \t\t\t\t\t refname);\n+\t\t\t\tret = TRANSACTION_NONEXISTENT_REF;\n \t\t\t\tgoto error_return;\n \t\t\t} else {\n \t\t\t\t/*\n@@ -820,6 +822,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\t\t\t/* Garden variety missing reference. */\n \t\t\t\tstrbuf_addf(err, \"unable to resolve reference '%s'\",\n \t\t\t\t\t refname);\n+\t\t\t\tret = TRANSACTION_NONEXISTENT_REF;\n \t\t\t\tgoto error_return;\n \t\t\t} else if (remove_dir_recursively(&ref_file,\n \t\t\t\t\t\t\t REMOVE_DIR_EMPTY_ONLY)) {\n@@ -863,7 +866,7 @@ static int lock_raw_ref(struct files_ref_store *refs,\n \t\tstring_list_insert(refnames_to_check, refname);\n \t}\n \n-\tret = 0;\n+\tret = TRANSACTION_OK;\n \tgoto out;\n \n error_return:\n@@ -1517,10 +1520,10 @@ static int rename_tmp_log(struct files_ref_store *refs, const char *newrefname)\n \treturn ret;\n }\n \n-static int write_ref_to_lockfile(struct files_ref_store *refs,\n-\t\t\t\t struct ref_lock *lock,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t int skip_oid_verification, struct strbuf *err);\n+static enum transaction_error write_ref_to_lockfile(struct files_ref_store *refs,\n+\t\t\t\t\t\t struct ref_lock *lock,\n+\t\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t\t int skip_oid_verification, struct strbuf *err);\n static int commit_ref_update(struct files_ref_store *refs,\n \t\t\t struct ref_lock *lock,\n \t\t\t const struct object_id *oid, const char *logmsg,\n@@ -1926,10 +1929,11 @@ static int files_log_ref_write(struct files_ref_store *refs,\n * Write oid into the open lockfile, then close the lockfile. On\n * errors, rollback the lockfile, fill in *err and return -1.\n */\n-static int write_ref_to_lockfile(struct files_ref_store *refs,\n-\t\t\t\t struct ref_lock *lock,\n-\t\t\t\t const struct object_id *oid,\n-\t\t\t\t int skip_oid_verification, struct strbuf *err)\n+static enum transaction_error write_ref_to_lockfile(struct files_ref_store *refs,\n+\t\t\t\t\t\t struct ref_lock *lock,\n+\t\t\t\t\t\t const struct object_id *oid,\n+\t\t\t\t\t\t int skip_oid_verification,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tstatic char term = '\\n';\n \tstruct object *o;\n@@ -1943,7 +1947,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs,\n \t\t\t\t\"trying to write ref '%s' with nonexistent object %s\",\n \t\t\t\tlock->ref_name, oid_to_hex(oid));\n \t\t\tunlock_ref(lock);\n-\t\t\treturn -1;\n+\t\t\treturn TRANSACTION_INVALID_NEW_VALUE;\n \t\t}\n \t\tif (o->type != OBJ_COMMIT && is_branch(lock->ref_name)) {\n \t\t\tstrbuf_addf(\n@@ -1951,7 +1955,7 @@ static int write_ref_to_lockfile(struct files_ref_store *refs,\n \t\t\t\t\"trying to write non-commit object %s to branch '%s'\",\n \t\t\t\toid_to_hex(oid), lock->ref_name);\n \t\t\tunlock_ref(lock);\n-\t\t\treturn -1;\n+\t\t\treturn TRANSACTION_INVALID_NEW_VALUE;\n \t\t}\n \t}\n \tfd = get_lock_file_fd(&lock->lk);\n@@ -1962,9 +1966,9 @@ static int write_ref_to_lockfile(struct files_ref_store *refs,\n \t\tstrbuf_addf(err,\n \t\t\t \"couldn't write '%s'\", get_lock_file_path(&lock->lk));\n \t\tunlock_ref(lock);\n-\t\treturn -1;\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \t}\n-\treturn 0;\n+\treturn TRANSACTION_OK;\n }\n \n /*\n@@ -2376,9 +2380,10 @@ static struct ref_iterator *files_reflog_iterator_begin(struct ref_store *ref_st\n * If update is a direct update of head_ref (the reference pointed to\n * by HEAD), then add an extra REF_LOG_ONLY update for HEAD.\n */\n-static int split_head_update(struct ref_update *update,\n-\t\t\t struct ref_transaction *transaction,\n-\t\t\t const char *head_ref, struct strbuf *err)\n+static enum transaction_error split_head_update(struct ref_update *update,\n+\t\t\t\t\t\tstruct ref_transaction *transaction,\n+\t\t\t\t\t\tconst char *head_ref,\n+\t\t\t\t\t\tstruct strbuf *err)\n {\n \tstruct ref_update *new_update;\n \n@@ -2386,10 +2391,10 @@ static int split_head_update(struct ref_update *update,\n \t (update->flags & REF_SKIP_CREATE_REFLOG) ||\n \t (update->flags & REF_IS_PRUNING) ||\n \t (update->flags & REF_UPDATE_VIA_HEAD))\n-\t\treturn 0;\n+\t\treturn TRANSACTION_OK;\n \n \tif (strcmp(update->refname, head_ref))\n-\t\treturn 0;\n+\t\treturn TRANSACTION_OK;\n \n \t/*\n \t * First make sure that HEAD is not already in the\n@@ -2419,7 +2424,7 @@ static int split_head_update(struct ref_update *update,\n \tif (strcmp(new_update->refname, \"HEAD\"))\n \t\tBUG(\"%s unexpectedly not 'HEAD'\", new_update->refname);\n \n-\treturn 0;\n+\treturn TRANSACTION_OK;\n }\n \n /*\n@@ -2430,10 +2435,10 @@ static int split_head_update(struct ref_update *update,\n * Note that the new update will itself be subject to splitting when\n * the iteration gets to it.\n */\n-static int split_symref_update(struct ref_update *update,\n-\t\t\t const char *referent,\n-\t\t\t struct ref_transaction *transaction,\n-\t\t\t struct strbuf *err)\n+static enum transaction_error split_symref_update(struct ref_update *update,\n+\t\t\t\t\t\t const char *referent,\n+\t\t\t\t\t\t struct ref_transaction *transaction,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct ref_update *new_update;\n \tunsigned int new_flags;\n@@ -2482,7 +2487,7 @@ static int split_symref_update(struct ref_update *update,\n \tupdate->flags |= REF_LOG_ONLY | REF_NO_DEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n-\treturn 0;\n+\treturn TRANSACTION_OK;\n }\n \n /*\n@@ -2491,34 +2496,32 @@ static int split_symref_update(struct ref_update *update,\n * everything is OK, return 0; otherwise, write an error message to\n * err and return -1.\n */\n-static int check_old_oid(struct ref_update *update, struct object_id *oid,\n-\t\t\t struct strbuf *err)\n+static enum transaction_error check_old_oid(struct ref_update *update,\n+\t\t\t\t\t struct object_id *oid,\n+\t\t\t\t\t struct strbuf *err)\n {\n-\tint ret = TRANSACTION_GENERIC_ERROR;\n-\n \tif (!(update->flags & REF_HAVE_OLD) ||\n \t\t oideq(oid, &update->old_oid))\n-\t\treturn 0;\n+\t\treturn TRANSACTION_OK;\n \n \tif (is_null_oid(&update->old_oid)) {\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t \"reference already exists\",\n \t\t\t ref_update_original_update_refname(update));\n-\t\tret = TRANSACTION_CREATE_EXISTS;\n-\t}\n-\telse if (is_null_oid(oid))\n+\t\treturn TRANSACTION_CREATE_EXISTS;\n+\t} else if (is_null_oid(oid)) {\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t \"reference is missing but expected %s\",\n \t\t\t ref_update_original_update_refname(update),\n \t\t\t oid_to_hex(&update->old_oid));\n-\telse\n-\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n-\t\t\t \"is at %s but expected %s\",\n-\t\t\t ref_update_original_update_refname(update),\n-\t\t\t oid_to_hex(oid),\n-\t\t\t oid_to_hex(&update->old_oid));\n+\t\treturn TRANSACTION_NONEXISTENT_REF;\n+\t}\n \n-\treturn ret;\n+\tstrbuf_addf(err, \"cannot lock ref '%s': is at %s but expected %s\",\n+\t\t ref_update_original_update_refname(update), oid_to_hex(oid),\n+\t\t oid_to_hex(&update->old_oid));\n+\n+\treturn TRANSACTION_INCORRECT_OLD_VALUE;\n }\n \n struct files_transaction_backend_data {\n@@ -2540,17 +2543,17 @@ struct files_transaction_backend_data {\n * - If it is an update of head_ref, add a corresponding REF_LOG_ONLY\n * update of HEAD.\n */\n-static int lock_ref_for_update(struct files_ref_store *refs,\n-\t\t\t struct ref_update *update,\n-\t\t\t struct ref_transaction *transaction,\n-\t\t\t const char *head_ref,\n-\t\t\t struct string_list *refnames_to_check,\n-\t\t\t struct strbuf *err)\n+static enum transaction_error lock_ref_for_update(struct files_ref_store *refs,\n+\t\t\t\t\t\t struct ref_update *update,\n+\t\t\t\t\t\t struct ref_transaction *transaction,\n+\t\t\t\t\t\t const char *head_ref,\n+\t\t\t\t\t\t struct string_list *refnames_to_check,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = ref_update_expects_existing_old_ref(update);\n \tstruct files_transaction_backend_data *backend_data;\n-\tint ret = 0;\n+\tenum transaction_error ret = TRANSACTION_OK;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n@@ -2607,16 +2610,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t}\n \t\t\t}\n \n-\t\t\tif (update->old_target) {\n-\t\t\t\tif (ref_update_check_old_target(referent.buf, update, err)) {\n-\t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\t\t\tgoto out;\n-\t\t\t\t}\n-\t\t\t} else {\n+\t\t\tif (update->old_target)\n+\t\t\t\tret = ref_update_check_old_target(referent.buf, update, err);\n+\t\t\telse\n \t\t\t\tret = check_old_oid(update, &lock->old_oid, err);\n-\t\t\t\tif (ret) {\n-\t\t\t\t\tgoto out;\n-\t\t\t\t}\n+\t\t\tif (ret) {\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2644,7 +2643,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\t \"but is a regular ref\"),\n \t\t\t\t ref_update_original_update_refname(update),\n \t\t\t\t update->old_target);\n-\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_EXPECTED_SYMREF;\n \t\t\tgoto out;\n \t\t} else {\n \t\t\tret = check_old_oid(update, &lock->old_oid, err);\n@@ -2693,25 +2692,27 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * The reference already has the desired\n \t\t\t * value, so we don't need to write it.\n \t\t\t */\n-\t\t} else if (write_ref_to_lockfile(\n-\t\t\t\t refs, lock, &update->new_oid,\n-\t\t\t\t update->flags & REF_SKIP_OID_VERIFICATION,\n-\t\t\t\t err)) {\n-\t\t\tchar *write_err = strbuf_detach(err, NULL);\n-\n-\t\t\t/*\n-\t\t\t * The lock was freed upon failure of\n-\t\t\t * write_ref_to_lockfile():\n-\t\t\t */\n-\t\t\tupdate->backend_data = NULL;\n-\t\t\tstrbuf_addf(err,\n-\t\t\t\t \"cannot update ref '%s': %s\",\n-\t\t\t\t update->refname, write_err);\n-\t\t\tfree(write_err);\n-\t\t\tret = TRANSACTION_GENERIC_ERROR;\n-\t\t\tgoto out;\n \t\t} else {\n-\t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n+\t\t\tret = write_ref_to_lockfile(\n+\t\t\t\trefs, lock, &update->new_oid,\n+\t\t\t\tupdate->flags & REF_SKIP_OID_VERIFICATION,\n+\t\t\t\terr);\n+\t\t\tif (ret) {\n+\t\t\t\tchar *write_err = strbuf_detach(err, NULL);\n+\n+\t\t\t\t/*\n+\t\t\t\t * The lock was freed upon failure of\n+\t\t\t\t * write_ref_to_lockfile():\n+\t\t\t\t */\n+\t\t\t\tupdate->backend_data = NULL;\n+\t\t\t\tstrbuf_addf(err,\n+\t\t\t\t\t \"cannot update ref '%s': %s\",\n+\t\t\t\t\t update->refname, write_err);\n+\t\t\t\tfree(write_err);\n+\t\t\t\tgoto out;\n+\t\t\t} else {\n+\t\t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n+\t\t\t}\n \t\t}\n \t}\n \tif (!(update->flags & REF_NEEDS_COMMIT)) {\ndiff --git a/refs/packed-backend.c b/refs/packed-backend.c\nindex 3247871574..75e1ebf67d 100644\n--- a/refs/packed-backend.c\n+++ b/refs/packed-backend.c\n@@ -1323,10 +1323,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,\n * The packfile must be locked before calling this function and will\n * remain locked when it is done.\n */\n-static int write_with_updates(struct packed_ref_store *refs,\n-\t\t\t struct string_list *updates,\n-\t\t\t struct strbuf *err)\n+static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n+\t\t\t\t\t\t struct string_list *updates,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n+\tenum transaction_error ret = TRANSACTION_GENERIC_ERROR;\n \tstruct ref_iterator *iter = NULL;\n \tsize_t i;\n \tint ok;\n@@ -1350,7 +1351,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\tstrbuf_addf(err, \"unable to create file %s: %s\",\n \t\t\t sb.buf, strerror(errno));\n \t\tstrbuf_release(&sb);\n-\t\treturn -1;\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \t}\n \tstrbuf_release(&sb);\n \n@@ -1406,6 +1407,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n \t\t\t\t\t\t \"reference already exists\",\n \t\t\t\t\t\t update->refname);\n+\t\t\t\t\tret = TRANSACTION_CREATE_EXISTS;\n \t\t\t\t\tgoto error;\n \t\t\t\t} else if (!oideq(&update->old_oid, iter->oid)) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n@@ -1413,6 +1415,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t\t update->refname,\n \t\t\t\t\t\t oid_to_hex(iter->oid),\n \t\t\t\t\t\t oid_to_hex(&update->old_oid));\n+\t\t\t\t\tret = TRANSACTION_INCORRECT_OLD_VALUE;\n \t\t\t\t\tgoto error;\n \t\t\t\t}\n \t\t\t}\n@@ -1449,6 +1452,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\t\t\t \"reference is missing but expected %s\",\n \t\t\t\t\t update->refname,\n \t\t\t\t\t oid_to_hex(&update->old_oid));\n+\t\t\t\treturn TRANSACTION_NONEXISTENT_REF;\n \t\t\t\tgoto error;\n \t\t\t}\n \t\t}\n@@ -1506,10 +1510,10 @@ static int write_with_updates(struct packed_ref_store *refs,\n \t\t\t strerror(errno));\n \t\tstrbuf_release(&sb);\n \t\tdelete_tempfile(&refs->tempfile);\n-\t\treturn -1;\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \t}\n \n-\treturn 0;\n+\treturn TRANSACTION_OK;\n \n write_error:\n \tstrbuf_addf(err, \"error writing to %s: %s\",\n@@ -1518,7 +1522,7 @@ static int write_with_updates(struct packed_ref_store *refs,\n error:\n \tref_iterator_free(iter);\n \tdelete_tempfile(&refs->tempfile);\n-\treturn -1;\n+\treturn ret;\n }\n \n int is_packed_transaction_needed(struct ref_store *ref_store,\n@@ -1672,7 +1676,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n \t\tdata->own_lock = 1;\n \t}\n \n-\tif (write_with_updates(refs, &transaction->refnames, err))\n+\tret = write_with_updates(refs, &transaction->refnames, err);\n+\tif (ret)\n \t\tgoto failure;\n \n \ttransaction->state = REF_TRANSACTION_PREPARED;\ndiff --git a/refs/refs-internal.h b/refs/refs-internal.h\nindex 4b7fc8f1ab..c97045fbed 100644\n--- a/refs/refs-internal.h\n+++ b/refs/refs-internal.h\n@@ -769,8 +769,9 @@ int ref_update_has_null_new_value(struct ref_update *update);\n * If everything is OK, return 0; otherwise, write an error message to\n * err and return -1.\n */\n-int ref_update_check_old_target(const char *referent, struct ref_update *update,\n-\t\t\t\tstruct strbuf *err);\n+enum transaction_error ref_update_check_old_target(const char *referent,\n+\t\t\t\t\t struct ref_update *update,\n+\t\t\t\t\t struct strbuf *err);\n \n /*\n * Check if the ref must exist, this means that the old_oid or\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 2c1e2995de..e1fd9c2de2 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -1069,20 +1069,20 @@ static int queue_transaction_update(struct reftable_ref_store *refs,\n \treturn 0;\n }\n \n-static int prepare_single_update(struct reftable_ref_store *refs,\n-\t\t\t\t struct reftable_transaction_data *tx_data,\n-\t\t\t\t struct ref_transaction *transaction,\n-\t\t\t\t struct reftable_backend *be,\n-\t\t\t\t struct ref_update *u,\n-\t\t\t\t struct string_list *refnames_to_check,\n-\t\t\t\t unsigned int head_type,\n-\t\t\t\t struct strbuf *head_referent,\n-\t\t\t\t struct strbuf *referent,\n-\t\t\t\t struct strbuf *err)\n+static enum transaction_error prepare_single_update(struct reftable_ref_store *refs,\n+\t\t\t\t\t\t struct reftable_transaction_data *tx_data,\n+\t\t\t\t\t\t struct ref_transaction *transaction,\n+\t\t\t\t\t\t struct reftable_backend *be,\n+\t\t\t\t\t\t struct ref_update *u,\n+\t\t\t\t\t\t struct string_list *refnames_to_check,\n+\t\t\t\t\t\t unsigned int head_type,\n+\t\t\t\t\t\t struct strbuf *head_referent,\n+\t\t\t\t\t\t struct strbuf *referent,\n+\t\t\t\t\t\t struct strbuf *err)\n {\n+\tenum transaction_error ret = TRANSACTION_OK;\n \tstruct object_id current_oid = {0};\n \tconst char *rewritten_ref;\n-\tint ret = 0;\n \n \t/*\n \t * There is no need to reload the respective backends here as\n@@ -1093,7 +1093,7 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t */\n \tret = backend_for(&be, refs, u->refname, &rewritten_ref, 0);\n \tif (ret)\n-\t\treturn ret;\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \n \t/* Verify that the new object ID is valid. */\n \tif ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&\n@@ -1104,13 +1104,13 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t\t\tstrbuf_addf(err,\n \t\t\t\t _(\"trying to write ref '%s' with nonexistent object %s\"),\n \t\t\t\t u->refname, oid_to_hex(&u->new_oid));\n-\t\t\treturn -1;\n+\t\t\treturn TRANSACTION_INVALID_NEW_VALUE;\n \t\t}\n \n \t\tif (o->type != OBJ_COMMIT && is_branch(u->refname)) {\n \t\t\tstrbuf_addf(err, _(\"trying to write non-commit object %s to branch '%s'\"),\n \t\t\t\t oid_to_hex(&u->new_oid), u->refname);\n-\t\t\treturn -1;\n+\t\t\treturn TRANSACTION_INVALID_NEW_VALUE;\n \t\t}\n \t}\n \n@@ -1147,7 +1147,7 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \tret = reftable_backend_read_ref(be, rewritten_ref,\n \t\t\t\t\t¤t_oid, referent, &u->type);\n \tif (ret < 0)\n-\t\treturn ret;\n+\t\treturn TRANSACTION_GENERIC_ERROR;\n \tif (ret > 0 && !ref_update_expects_existing_old_ref(u)) {\n \t\t/*\n \t\t * The reference does not exist, and we either have no\n@@ -1168,7 +1168,7 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t\t\tret = queue_transaction_update(refs, tx_data, u,\n \t\t\t\t\t\t ¤t_oid, err);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n \t\t}\n \n \t\treturn 0;\n@@ -1180,7 +1180,7 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \n \t\t\t\t \"unable to resolve reference '%s'\"),\n \t\t\t ref_update_original_update_refname(u), u->refname);\n-\t\treturn -1;\n+\t\treturn TRANSACTION_NONEXISTENT_REF;\n \t}\n \n \tif (u->type & REF_ISSYMREF) {\n@@ -1196,7 +1196,7 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t\t\tif (u->flags & REF_HAVE_OLD && !resolved) {\n \t\t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\"), u->refname);\n-\t\t\t\treturn -1;\n+\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n \t\t\t}\n \t\t} else {\n \t\t\tstruct ref_update *new_update;\n@@ -1255,11 +1255,12 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t\t\t\t\t \"but is a regular ref\"),\n \t\t\t\t ref_update_original_update_refname(u),\n \t\t\t\t u->old_target);\n-\t\t\treturn -1;\n+\t\t\treturn TRANSACTION_EXPECTED_SYMREF;\n \t\t}\n \n-\t\tif (ref_update_check_old_target(referent->buf, u, err)) {\n-\t\t\treturn -1;\n+\t\tret = ref_update_check_old_target(referent->buf, u, err);\n+\t\tif (ret) {\n+\t\t\treturn ret;\n \t\t}\n \t} else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) {\n \t\tif (is_null_oid(&u->old_oid)) {\n@@ -1268,18 +1269,21 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \t\t\t\t ref_update_original_update_refname(u));\n \t\t\treturn TRANSACTION_CREATE_EXISTS;\n \t\t}\n-\t\telse if (is_null_oid(¤t_oid))\n+\t\telse if (is_null_oid(¤t_oid)) {\n \t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n \t\t\t\t\t \"reference is missing but expected %s\"),\n \t\t\t\t ref_update_original_update_refname(u),\n \t\t\t\t oid_to_hex(&u->old_oid));\n-\t\telse\n+\t\t\treturn TRANSACTION_NONEXISTENT_REF;\n+\n+\t\t} else {\n \t\t\tstrbuf_addf(err, _(\"cannot lock ref '%s': \"\n \t\t\t\t\t \"is at %s but expected %s\"),\n \t\t\t\t ref_update_original_update_refname(u),\n \t\t\t\t oid_to_hex(¤t_oid),\n \t\t\t\t oid_to_hex(&u->old_oid));\n-\t\treturn TRANSACTION_NAME_CONFLICT;\n+\t\t\treturn TRANSACTION_INCORRECT_OLD_VALUE;\n+\t\t}\n \t}\n \n \t/*\n@@ -1296,10 +1300,10 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n \tif ((u->type & REF_ISSYMREF) ||\n \t (u->flags & REF_LOG_ONLY) ||\n \t (u->flags & REF_HAVE_NEW && !oideq(¤t_oid, &u->new_oid)))\n-\t\treturn queue_transaction_update(refs, tx_data, u,\n-\t\t\t\t\t ¤t_oid, err);\n+\t\tif (queue_transaction_update(refs, tx_data, u, ¤t_oid, err))\n+\t\t\treturn TRANSACTION_GENERIC_ERROR;\n \n-\treturn 0;\n+\treturn TRANSACTION_OK;\n }\n \n static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n@@ -1386,7 +1390,6 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n \ttransaction->state = REF_TRANSACTION_PREPARED;\n \n done:\n-\tassert(ret != REFTABLE_API_ERROR);\n \tif (ret < 0) {\n \t\tfree_transaction_data(tx_data);\n \t\ttransaction->state = REF_TRANSACTION_CLOSED;\n\n-- \n2.47.2\n\n"},{"id":"512996","messageId":"20250225-245-partially-atomic-ref-updates-v2-7-cfa3236895d7@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-0-cfa3236895d7@gmail.com","subject":"[PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-02-25T09:29:10Z","receivedAt":"2025-02-25T09:29:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"When updating multiple references through stdin, Git's update-ref\ncommand normally aborts the entire transaction if any single update\nfails. While this atomic behavior prevents partial updates by default,\nthere are cases where applying successful updates while reporting\nfailures is desirable.\n\nAdd a new `--allow-partial` flag that allows the transaction to continue\neven when individual reference updates fail. This flag can only be used\nin `--stdin` mode and builds upon the partial transaction support added\nto the refs subsystem. When enabled, failed updates are reported in the\nfollowing format:\n\n rejected SP ( | ) SP ( | ) SP LF\n\nor with `-z`:\n\n rejected NUL ( | ) NUL ( | ) NUL NUL\n\nUpdate the documentation to reflect this change and also tests to cover\ndifferent scenarios where an update could be rejected.\n\nSigned-off-by: Karthik Nayak \n---\n Documentation/git-update-ref.adoc | 21 +++-\n builtin/update-ref.c | 74 +++++++++++--\n t/t1400-update-ref.sh | 216 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 302 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc\nindex 9e6935d38d..fc73f1d8aa 100644\n--- a/Documentation/git-update-ref.adoc\n+++ b/Documentation/git-update-ref.adoc\n@@ -7,8 +7,10 @@ git-update-ref - Update the object name stored in a ref safely\n \n SYNOPSIS\n --------\n-[verse]\n-'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z])\n+[synopsis]\n+git update-ref [-m ] [--no-deref] -d []\n+\t [-m ] [--no-deref] [--create-reflog] []\n+ [-m ] [--no-deref] --stdin [-z] [--allow-partial]\n \n DESCRIPTION\n -----------\n@@ -57,6 +59,17 @@ performs all modifications together. Specify commands of the form:\n With `--create-reflog`, update-ref will create a reflog for each ref\n even if one would not ordinarily be created.\n \n+With `--allow-partial`, update-ref continues executing the transaction even if\n+some updates fail due to invalid or incorrect user input, applying only the\n+successful updates. Errors resulting from user-provided input are treated as\n+non-system-related and do not cause the entire transaction to be aborted.\n+However, system-related errors—such as I/O failures or memory issues—will still\n+result in a full failure. Additionally, errors like F/D conflicts are batched\n+for performance optimization and will also cause a full failure. Any failed\n+updates will be reported in the following format:\n+\n+\trejected SP ( | ) SP ( | ) SP LF\n+\n Quote fields containing whitespace as if they were strings in C source\n code; i.e., surrounded by double-quotes and with backslash escapes.\n Use 40 \"0\" characters or the empty string to specify a zero value. To\n@@ -82,6 +95,10 @@ quoting:\n In this format, use 40 \"0\" to specify a zero value, and use the empty\n string to specify a missing value.\n \n+With `-z`, `--allow-partial` will print rejections in the following form:\n+\n+\trejected NUL ( | ) NUL ( | ) NUL NUL\n+\n In either format, values can be specified in any form that Git\n recognizes as an object name. Commands in any other format or a\n repeated produce an error. Command meanings are:\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 1d541e13ad..b03b40eacb 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -5,6 +5,7 @@\n #include \"config.h\"\n #include \"gettext.h\"\n #include \"hash.h\"\n+#include \"hex.h\"\n #include \"refs.h\"\n #include \"object-name.h\"\n #include \"parse-options.h\"\n@@ -13,7 +14,7 @@\n static const char * const git_update_ref_usage[] = {\n \tN_(\"git update-ref [] -d []\"),\n \tN_(\"git update-ref [] []\"),\n-\tN_(\"git update-ref [] --stdin [-z]\"),\n+\tN_(\"git update-ref [] --stdin [-z] [--allow-partial]\"),\n \tNULL\n };\n \n@@ -565,6 +566,54 @@ static void parse_cmd_abort(struct ref_transaction *transaction,\n \treport_ok(\"abort\");\n }\n \n+static void print_rejected_refs(const char *refname,\n+\t\t\t\tconst struct object_id *old_oid,\n+\t\t\t\tconst struct object_id *new_oid,\n+\t\t\t\tconst char *old_target,\n+\t\t\t\tconst char *new_target,\n+\t\t\t\tenum transaction_error err,\n+\t\t\t\tvoid *cb_data UNUSED)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar space = ' ';\n+\tconst char *reason = \"\";\n+\n+\tswitch (err) {\n+\tcase TRANSACTION_NAME_CONFLICT:\n+\t\treason = _(\"refname conflict\");\n+\t\tbreak;\n+\tcase TRANSACTION_CREATE_EXISTS:\n+\t\treason = _(\"reference already exists\");\n+\t\tbreak;\n+\tcase TRANSACTION_NONEXISTENT_REF:\n+\t\treason = _(\"reference does not exist\");\n+\t\tbreak;\n+\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n+\t\treason = _(\"incorrect old value provided\");\n+\t\tbreak;\n+\tcase TRANSACTION_INVALID_NEW_VALUE:\n+\t\treason = _(\"invalid new value provided\");\n+\t\tbreak;\n+\tcase TRANSACTION_EXPECTED_SYMREF:\n+\t\treason = _(\"expected symref but found regular ref\");\n+\t\tbreak;\n+\tdefault:\n+\t\treason = _(\"unkown failure\");\n+\t}\n+\n+\tif (!line_termination)\n+\t\tspace = line_termination;\n+\n+\tstrbuf_addf(&sb, \"rejected%c%s%c%s%c%c%s%c%s%c\", space,\n+\t\t refname, space, new_oid ? oid_to_hex(new_oid) : new_target,\n+\t\t space, space, old_oid ? oid_to_hex(old_oid) : old_target,\n+\t\t space, reason, line_termination);\n+\n+\tfwrite(sb.buf, sb.len, 1, stdout);\n+\tstrbuf_release(&sb);\n+\tfflush(stdout);\n+}\n+\n static void parse_cmd_commit(struct ref_transaction *transaction,\n \t\t\t const char *next, const char *end UNUSED)\n {\n@@ -573,6 +622,10 @@ static void parse_cmd_commit(struct ref_transaction *transaction,\n \t\tdie(\"commit: extra input: %s\", next);\n \tif (ref_transaction_commit(transaction, &error))\n \t\tdie(\"commit: %s\", error.buf);\n+\n+\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t print_rejected_refs, NULL);\n+\n \treport_ok(\"commit\");\n \tref_transaction_free(transaction);\n }\n@@ -609,7 +662,7 @@ static const struct parse_cmd {\n \t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n };\n \n-static void update_refs_stdin(void)\n+static void update_refs_stdin(unsigned int flags)\n {\n \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n \tenum update_refs_state state = UPDATE_REFS_OPEN;\n@@ -617,7 +670,7 @@ static void update_refs_stdin(void)\n \tint i, j;\n \n \ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t 0, &err);\n+\t\t\t\t\t\t flags, &err);\n \tif (!transaction)\n \t\tdie(\"%s\", err.buf);\n \n@@ -685,7 +738,7 @@ static void update_refs_stdin(void)\n \t\t\t */\n \t\t\tstate = cmd->state;\n \t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t\t\t 0, &err);\n+\t\t\t\t\t\t\t\t flags, &err);\n \t\t\tif (!transaction)\n \t\t\t\tdie(\"%s\", err.buf);\n \n@@ -701,6 +754,8 @@ static void update_refs_stdin(void)\n \t\t/* Commit by default if no transaction was requested. */\n \t\tif (ref_transaction_commit(transaction, &err))\n \t\t\tdie(\"%s\", err.buf);\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t print_rejected_refs, NULL);\n \t\tref_transaction_free(transaction);\n \t\tbreak;\n \tcase UPDATE_REFS_STARTED:\n@@ -726,7 +781,9 @@ int cmd_update_ref(int argc,\n \tconst char *refname, *oldval;\n \tstruct object_id oid, oldoid;\n \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;\n-\tint create_reflog = 0;\n+\tint create_reflog = 0, allow_partial = 0;\n+\tunsigned int flags = 0;\n+\n \tstruct option options[] = {\n \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n@@ -735,6 +792,8 @@ int cmd_update_ref(int argc,\n \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n \t\tOPT_BOOL( 0 , \"create-reflog\", &create_reflog, N_(\"create a reflog\")),\n+\t\tOPT_BIT('0', \"allow-partial\", &flags, N_(\"allow partial transactions\"),\n+\t\t\tREF_TRANSACTION_ALLOW_PARTIAL),\n \t\tOPT_END(),\n \t};\n \n@@ -756,9 +815,10 @@ int cmd_update_ref(int argc,\n \t\t\tusage_with_options(git_update_ref_usage, options);\n \t\tif (end_null)\n \t\t\tline_termination = '\\0';\n-\t\tupdate_refs_stdin();\n+\t\tupdate_refs_stdin(flags);\n \t\treturn 0;\n-\t}\n+\t} else if (allow_partial)\n+\t\tdie(\"--allow-partial can only be used with --stdin\");\n \n \tif (end_null)\n \t\tusage_with_options(git_update_ref_usage, options);\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 29045aad43..fb9442982e 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -2066,6 +2066,222 @@ do\n \t\tgrep \"$(git rev-parse $a) $(git rev-parse $a)\" actual\n \t'\n \n+\ttest_expect_success \"stdin $type allow-partial\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit commit &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$head\" \"$Z\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head\" \"$Z\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with invalid new_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit update-ref refs/heads/ref2 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$(test_oid 001)\" \"$head\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"invalid new value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with non-commit new_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\thead_tree=$(git rev-parse HEAD^{tree}) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit update-ref refs/heads/ref2 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head_tree\" \"$head\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"invalid new value provided\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with non-existent ref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$head\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n+\t\t\ttest_grep -q \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with dangling symref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit symbolic-ref refs/heads/ref2 refs/heads/nonexistent &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$head\" >>stdin &&\n+\t\t\tgit update-ref $type --no-deref --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n+\t\t\ttest_grep -q \"reference does not exist\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with regular ref as symref\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit update-ref refs/heads/ref2 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"symref-update refs/heads/ref2\" \"$old_head\" \"ref\" \"refs/heads/nonexistent\" >>stdin &&\n+\t\t\tgit update-ref $type --no-deref --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\techo $head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"expected symref but found regular ref\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with invalid old_oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit update-ref refs/heads/ref2 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$Z\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"reference already exists\" stdout\n+\t\t)\n+\t'\n+\n+\ttest_expect_success \"stdin $type allow-partial with incorrect old oid\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref1 $head &&\n+\t\t\tgit update-ref refs/heads/ref2 $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head\" \"$old_head\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\techo $head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"incorrect old value provided\" stdout\n+\t\t)\n+\t'\n+\n+\t# F/D conflicts on the files backend are resolved on an individual\n+\t# update level since refs are stored as files. On the reftable backend\n+\t# this check is batched to optimize for performance, so failures cannot\n+\t# be isolated to a single update.\n+\ttest_expect_success REFFILES \"stdin $type allow-partial refname conflict\" '\n+\t\tgit init repo &&\n+\t\ttest_when_finished \"rm -fr repo\" &&\n+\t\t(\n+\t\t\tcd repo &&\n+\t\t\ttest_commit one &&\n+\t\t\told_head=$(git rev-parse HEAD) &&\n+\t\t\ttest_commit two &&\n+\t\t\thead=$(git rev-parse HEAD) &&\n+\t\t\tgit update-ref refs/heads/ref/foo $head &&\n+\n+\t\t\tformat_command $type \"update refs/heads/ref/foo\" \"$old_head\" \"$head\" >stdin &&\n+\t\t\tformat_command $type \"update refs/heads/ref\" \"$old_head\" \"\" >>stdin &&\n+\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n+\t\t\techo $old_head >expect &&\n+\t\t\tgit rev-parse refs/heads/ref/foo >actual &&\n+\t\t\ttest_cmp expect actual &&\n+\t\t\ttest_grep -q \"refname conflict\" stdout\n+\t\t)\n+\t'\n done\n \n test_expect_success 'update-ref should also create reflog for HEAD' '\n\n-- \n2.47.2\n\n"},{"id":"513002","messageId":"Z72kjTRDNOM6iFkY@pks.im","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-25T11:07:57Z","receivedAt":"2025-02-25T11:08:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 25, 2025 at 10:29:09AM +0100, Karthik Nayak wrote:\n> diff --git a/refs.c b/refs.c\n> index f989a46a5a..243c09c368 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n> \t}\n> }\n> \n> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n> +\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n> +\t\t\t\t\t void *cb_data)\n> +{\n> +\tif (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))\n> +\t\treturn;\n> +\n> +\tfor (size_t i = 0; i < transaction->nr; i++) {\n> +\t\tstruct ref_update *update = transaction->updates[i];\n> +\n> +\t\tif (!update->rejection_err)\n> +\t\t\tcontinue;\n\nThis kind of proves my point that `TRANSACTION_OK` is pointless and\nleads to a mixture of using and not using the enum :)\n\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 3b0adf8bb2..d0a53c9ace 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\tret = lock_ref_for_update(refs, update, transaction,\n> \t\t\t\t\t head_ref, &refnames_to_check,\n> \t\t\t\t\t err);\n> -\t\tif (ret)\n> +\t\tif (ret) {\n> +\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n\nHm. If the error values were defined as a bitfield we could refactor\nthis to not be a flag, but have a `transaction->accepted_rejections`\ninstead that allows the caller to ask for only a subset of rejections to\nbe accepted.\n\nI'm not quite sure whether it is a good idea, but the logic to handle\nall of that could be self-contained in `ref_transaction_set_rejected()`:\nif it returned an error code itself, it would swallow any errors in case\nthe transaction allows a given error, and bubble up the error again in\ncase the error is not allowed. The function could use a rename in that\ncase though, e.g. `ref_transaction_maybe_set_rejected()`.\n\nPatrick\n"},{"id":"513003","messageId":"Z72klAEpQ0-cWCGa@pks.im","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-7-cfa3236895d7@gmail.com","subject":"Re: [PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-25T11:08:04Z","receivedAt":"2025-02-25T11:08:08Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 25, 2025 at 10:29:10AM +0100, Karthik Nayak wrote:\n> diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc\n> index 9e6935d38d..fc73f1d8aa 100644\n> --- a/Documentation/git-update-ref.adoc\n> +++ b/Documentation/git-update-ref.adoc\n> @@ -7,8 +7,10 @@ git-update-ref - Update the object name stored in a ref safely\n> \n> SYNOPSIS\n> --------\n> -[verse]\n> -'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z])\n> +[synopsis]\n> +git update-ref [-m ] [--no-deref] -d []\n> +\t [-m ] [--no-deref] [--create-reflog] []\n> + [-m ] [--no-deref] --stdin [-z] [--allow-partial]\n> \n> DESCRIPTION\n> -----------\n> @@ -57,6 +59,17 @@ performs all modifications together. Specify commands of the form:\n> With `--create-reflog`, update-ref will create a reflog for each ref\n> even if one would not ordinarily be created.\n> \n> +With `--allow-partial`, update-ref continues executing the transaction even if\n> +some updates fail due to invalid or incorrect user input, applying only the\n> +successful updates. Errors resulting from user-provided input are treated as\n> +non-system-related and do not cause the entire transaction to be aborted.\n> +However, system-related errors—such as I/O failures or memory issues—will still\n> +result in a full failure. Additionally, errors like F/D conflicts are batched\n> +for performance optimization and will also cause a full failure. Any failed\n> +updates will be reported in the following format:\n\nShouldn't it be possible to detect F/D conflicts though and not abort\nthe transaction? If we want to make use of partial transactions in the\ncontext of git-fetch(1) and/or git-receive-pack(1) we would have to\nhandle them.\n\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 1d541e13ad..b03b40eacb 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -565,6 +566,54 @@ static void parse_cmd_abort(struct ref_transaction *transaction,\n> \treport_ok(\"abort\");\n> }\n> \n> +static void print_rejected_refs(const char *refname,\n> +\t\t\t\tconst struct object_id *old_oid,\n> +\t\t\t\tconst struct object_id *new_oid,\n> +\t\t\t\tconst char *old_target,\n> +\t\t\t\tconst char *new_target,\n> +\t\t\t\tenum transaction_error err,\n> +\t\t\t\tvoid *cb_data UNUSED)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar space = ' ';\n> +\tconst char *reason = \"\";\n> +\n> +\tswitch (err) {\n> +\tcase TRANSACTION_NAME_CONFLICT:\n> +\t\treason = _(\"refname conflict\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_CREATE_EXISTS:\n> +\t\treason = _(\"reference already exists\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_NONEXISTENT_REF:\n> +\t\treason = _(\"reference does not exist\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n> +\t\treason = _(\"incorrect old value provided\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_INVALID_NEW_VALUE:\n> +\t\treason = _(\"invalid new value provided\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_EXPECTED_SYMREF:\n> +\t\treason = _(\"expected symref but found regular ref\");\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treason = _(\"unkown failure\");\n> +\t}\n\nAs git-update-ref(1) is part of plumbing we don't want to translate\nthose messages.\n\nPatrick\n"},{"id":"513004","messageId":"Z72kmFUARRSukMBe@pks.im","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-5-cfa3236895d7@gmail.com","subject":"Re: [PATCH v2 5/7] refs: introduce enum-based transaction error types","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-02-25T11:08:08Z","receivedAt":"2025-02-25T11:08:11Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Feb 25, 2025 at 10:29:08AM +0100, Karthik Nayak wrote:\n> diff --git a/refs.h b/refs.h\n> index b14ba1f9ff..8e9ead174c 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -16,6 +16,31 @@ struct worktree;\n> enum ref_storage_format ref_storage_format_by_name(const char *name);\n> const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format);\n> \n> +/*\n> + * enum transaction_error represents the following return codes:\n> + * TRANSACTION_OK: success code.\n> + * TRANSACTION_GENERIC_ERROR error_code: default error code.\n> + * TRANSACTION_NAME_CONFLICT error_code: ref name conflict like A vs A/B.\n> + * TRANSACTION_CREATE_EXISTS error_code: ref to be created already exists.\n> + * TRANSACTION_NONEXISTENT_REF error_code: ref expected but doesn't exist.\n> + * TRANSACTION_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of\n> + * reference doesn't match actual.\n> + * TRANSACTION_INVALID_NEW_VALUE error_code: provided new_oid or new_target is\n> + * invalid.\n> + * TRANSACTION_EXPECTED_SYMREF error_code: expected ref to be symref, but is a\n> + * regular ref.\n> + */\n> +enum transaction_error {\n> +\tTRANSACTION_OK = 0,\n> +\tTRANSACTION_GENERIC_ERROR = -1,\n> +\tTRANSACTION_NAME_CONFLICT = -2,\n> +\tTRANSACTION_CREATE_EXISTS = -3,\n> +\tTRANSACTION_NONEXISTENT_REF = -4,\n> +\tTRANSACTION_INCORRECT_OLD_VALUE = -5,\n> +\tTRANSACTION_INVALID_NEW_VALUE = -6,\n> +\tTRANSACTION_EXPECTED_SYMREF = -7,\n> +};\n\nNit: how about we name this `ref_transaction_error` and adapt the the\nenum values accordingly? We may eventually also introduce similar errors\nfor the object database, so it may make sense to have the errors be\nspecific. Doing both the enum and changing the name might be a bit hard\nto review, so we could also rename in a preparatory commit. Or we just\npunt on it for now and do it once it becomes necessary, that would also\nbe fine with me.\n\nI also wonder whether we really want to introduce `TRANSACTION_OK`. It's\nalways a bit of a mouthful, and in many cases one ends up with a mixture\nof `ret < 0`, `ret != TRANSACTION_OK` and `ret != 0`, which may lead to\nconfusion. Continuing to use `0` for the successful case should be fine.\n\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 3247871574..75e1ebf67d 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1672,7 +1676,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n> \t\tdata->own_lock = 1;\n> \t}\n> \n> -\tif (write_with_updates(refs, &transaction->refnames, err))\n> +\tret = write_with_updates(refs, &transaction->refnames, err);\n> +\tif (ret)\n> \t\tgoto failure;\n> \n> \ttransaction->state = REF_TRANSACTION_PREPARED;\n\nDo we also want to change the local variable declaration of `int ret` to\nuse the new type?\n\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 2c1e2995de..e1fd9c2de2 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1255,11 +1255,12 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n> \t\t\t\t\t \"but is a regular ref\"),\n> \t\t\t\t ref_update_original_update_refname(u),\n> \t\t\t\t u->old_target);\n> -\t\t\treturn -1;\n> +\t\t\treturn TRANSACTION_EXPECTED_SYMREF;\n> \t\t}\n> \n> -\t\tif (ref_update_check_old_target(referent->buf, u, err)) {\n> -\t\t\treturn -1;\n> +\t\tret = ref_update_check_old_target(referent->buf, u, err);\n> +\t\tif (ret) {\n> +\t\t\treturn ret;\n> \t\t}\n> \t} else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) {\n> \t\tif (is_null_oid(&u->old_oid)) {\n\nNit: superfluous braces that we could remove while at it.\n\nPatrick\n"},{"id":"513023","messageId":"2755fb78-d587-4ad9-b22f-b60dfa752d7a@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-6-cfa3236895d7@gmail.com","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-02-25T14:57:47Z","receivedAt":"2025-02-25T14:57:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Karthik\n\nOn 25/02/2025 09:29, Karthik Nayak wrote:\n> Git's reference transactions are all-or-nothing: either all updates\n> succeed, or none do. While this atomic behavior is generally desirable,\n> it can be suboptimal especially when using the reftable backend, where\n> batching multiple reference updates into a single transaction is more\n> efficient than performing them sequentially.\n> \n> Introduce partial transaction support with a new flag,\n> 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\n> individual reference updates that would typically cause the entire\n> transaction to fail due to non-system-related errors to be marked as\n> rejected while permitting other updates to proceed. Non-system-related\n> errors include issues caused by user-provided input values, whereas\n> system-related errors, such as I/O failures or memory issues, continue\n> to result in a full transaction failure. This approach enhances\n> flexibility while preserving transactional integrity where necessary.\n> \n> The implementation introduces several key components:\n> \n> - Add 'rejection_err' field to struct `ref_update` to track failed\n> updates with failure reason.\n> \n> - Modify reference backends (files, packed, reftable) to handle\n> partial transactions by using `ref_transaction_set_rejected()`\n> instead of failing the entire transaction when\n> `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n> \n> - Add `ref_transaction_for_each_rejected_update()` to let callers\n> examine which updates were rejected and why.\n\nI think this is a much better design. I wonder if we want to signal to \nthe caller of ref_transaction_commit() that there were ignored errors \nrather than forcing them to call ref_transaction_for_each_rejected() to \nfind that out. Another possibility would be to call the callback from \nref_transaction_commit() but that would mean changing the signature of \nref_transaction_begin() to take the callback and user data when \nREF_TRANSACTION_ALLOW_PARTIAL is passed.\n\nBest Wishes\n\nPhillip\n\n> This foundational change enables partial transaction support throughout\n> the reference subsystem. The next commit will expose this capability to\n> users by adding a `--allow-partial` flag to 'git-update-ref(1)',\n> providing both a user-facing feature and a testable implementation.\n> \n> Signed-off-by: Karthik Nayak \n> ---\n> refs.c | 31 +++++++++++++++++++++++++++++++\n> refs.h | 22 ++++++++++++++++++++++\n> refs/files-backend.c | 12 +++++++++++-\n> refs/packed-backend.c | 30 ++++++++++++++++++++++++++++--\n> refs/refs-internal.h | 13 +++++++++++++\n> refs/reftable-backend.c | 12 +++++++++++-\n> 6 files changed, 116 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index f989a46a5a..243c09c368 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1211,6 +1211,15 @@ void ref_transaction_free(struct ref_transaction *transaction)\n> \tfree(transaction);\n> }\n> \n> +void ref_transaction_set_rejected(struct ref_transaction *transaction,\n> +\t\t\t\t size_t update_idx,\n> +\t\t\t\t enum transaction_error err)\n> +{\n> +\tif (update_idx >= transaction->nr)\n> +\t\tBUG(\"trying to set rejection on invalid update index\");\n> +\ttransaction->updates[update_idx]->rejection_err = err;\n> +}\n> +\n> struct ref_update *ref_transaction_add_update(\n> \t\tstruct ref_transaction *transaction,\n> \t\tconst char *refname, unsigned int flags,\n> @@ -1236,6 +1245,7 @@ struct ref_update *ref_transaction_add_update(\n> \ttransaction->updates[transaction->nr++] = update;\n> \n> \tupdate->flags = flags;\n> +\tupdate->rejection_err = TRANSACTION_OK;\n> \n> \tupdate->new_target = xstrdup_or_null(new_target);\n> \tupdate->old_target = xstrdup_or_null(old_target);\n> @@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n> \t}\n> }\n> \n> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n> +\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n> +\t\t\t\t\t void *cb_data)\n> +{\n> +\tif (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))\n> +\t\treturn;\n> +\n> +\tfor (size_t i = 0; i < transaction->nr; i++) {\n> +\t\tstruct ref_update *update = transaction->updates[i];\n> +\n> +\t\tif (!update->rejection_err)\n> +\t\t\tcontinue;\n> +\n> +\t\tcb(update->refname,\n> +\t\t (update->flags & REF_HAVE_OLD) ? &update->old_oid : NULL,\n> +\t\t (update->flags & REF_HAVE_NEW) ? &update->new_oid : NULL,\n> +\t\t update->old_target, update->new_target,\n> +\t\t update->rejection_err, cb_data);\n> +\t}\n> +}\n> +\n> int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n> \t\t struct string_list *refnames, unsigned int flags)\n> {\n> diff --git a/refs.h b/refs.h\n> index 8e9ead174c..e4a6a8218f 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -675,6 +675,13 @@ enum ref_transaction_flag {\n> \t * either be absent or null_oid.\n> \t */\n> \tREF_TRANSACTION_FLAG_INITIAL = (1 << 0),\n> +\n> +\t/*\n> +\t * The transaction mechanism by default fails all updates if any conflict\n> +\t * is detected. This flag allows transactions to partially apply updates\n> +\t * while rejecting updates which do not match the expected state.\n> +\t */\n> +\tREF_TRANSACTION_ALLOW_PARTIAL = (1 << 1),\n> };\n> \n> /*\n> @@ -905,6 +912,21 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n> \t\t\t\t\t ref_transaction_for_each_queued_update_fn cb,\n> \t\t\t\t\t void *cb_data);\n> \n> +/*\n> + * Execute the given callback function for each of the reference updates which\n> + * have been rejected in the given transaction.\n> + */\n> +typedef void ref_transaction_for_each_rejected_update_fn(const char *refname,\n> +\t\t\t\t\t\t\t const struct object_id *old_oid,\n> +\t\t\t\t\t\t\t const struct object_id *new_oid,\n> +\t\t\t\t\t\t\t const char *old_target,\n> +\t\t\t\t\t\t\t const char *new_target,\n> +\t\t\t\t\t\t\t enum transaction_error err,\n> +\t\t\t\t\t\t\t void *cb_data);\n> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n> +\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n> +\t\t\t\t\t void *cb_data);\n> +\n> /*\n> * Free `*transaction` and all associated data.\n> */\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 3b0adf8bb2..d0a53c9ace 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\tret = lock_ref_for_update(refs, update, transaction,\n> \t\t\t\t\t head_ref, &refnames_to_check,\n> \t\t\t\t\t err);\n> -\t\tif (ret)\n> +\t\tif (ret) {\n> +\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n> +\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n> +\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n> +\n> +\t\t\t\tstrbuf_setlen(err, 0);\n> +\t\t\t\tret = TRANSACTION_OK;\n> +\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t\tgoto cleanup;\n> +\t\t}\n> \n> \t\tif (update->flags & REF_DELETING &&\n> \t\t !(update->flags & REF_LOG_ONLY) &&\n> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n> index 75e1ebf67d..0857204213 100644\n> --- a/refs/packed-backend.c\n> +++ b/refs/packed-backend.c\n> @@ -1324,10 +1324,11 @@ static int packed_ref_store_remove_on_disk(struct ref_store *ref_store,\n> * remain locked when it is done.\n> */\n> static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n> -\t\t\t\t\t\t struct string_list *updates,\n> +\t\t\t\t\t\t struct ref_transaction *transaction,\n> \t\t\t\t\t\t struct strbuf *err)\n> {\n> \tenum transaction_error ret = TRANSACTION_GENERIC_ERROR;\n> +\tstruct string_list *updates = &transaction->refnames;\n> \tstruct ref_iterator *iter = NULL;\n> \tsize_t i;\n> \tint ok;\n> @@ -1408,6 +1409,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n> \t\t\t\t\t\t \"reference already exists\",\n> \t\t\t\t\t\t update->refname);\n> \t\t\t\t\tret = TRANSACTION_CREATE_EXISTS;\n> +\n> +\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n> +\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n> +\t\t\t\t\t\tstrbuf_setlen(err, 0);\n> +\t\t\t\t\t\tret = 0;\n> +\t\t\t\t\t\tcontinue;\n> +\t\t\t\t\t}\n> +\n> \t\t\t\t\tgoto error;\n> \t\t\t\t} else if (!oideq(&update->old_oid, iter->oid)) {\n> \t\t\t\t\tstrbuf_addf(err, \"cannot update ref '%s': \"\n> @@ -1416,6 +1425,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n> \t\t\t\t\t\t oid_to_hex(iter->oid),\n> \t\t\t\t\t\t oid_to_hex(&update->old_oid));\n> \t\t\t\t\tret = TRANSACTION_INCORRECT_OLD_VALUE;\n> +\n> +\t\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n> +\t\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n> +\t\t\t\t\t\tstrbuf_setlen(err, 0);\n> +\t\t\t\t\t\tret = 0;\n> +\t\t\t\t\t\tcontinue;\n> +\t\t\t\t\t}\n> +\n> \t\t\t\t\tgoto error;\n> \t\t\t\t}\n> \t\t\t}\n> @@ -1453,6 +1470,14 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n> \t\t\t\t\t update->refname,\n> \t\t\t\t\t oid_to_hex(&update->old_oid));\n> \t\t\t\treturn TRANSACTION_NONEXISTENT_REF;\n> +\n> +\t\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL) {\n> +\t\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n> +\t\t\t\t\tstrbuf_setlen(err, 0);\n> +\t\t\t\t\tret = 0;\n> +\t\t\t\t\tcontinue;\n> +\t\t\t\t}\n> +\n> \t\t\t\tgoto error;\n> \t\t\t}\n> \t\t}\n> @@ -1518,6 +1543,7 @@ static enum transaction_error write_with_updates(struct packed_ref_store *refs,\n> write_error:\n> \tstrbuf_addf(err, \"error writing to %s: %s\",\n> \t\t get_tempfile_path(refs->tempfile), strerror(errno));\n> +\tret = TRANSACTION_GENERIC_ERROR;\n> \n> error:\n> \tref_iterator_free(iter);\n> @@ -1676,7 +1702,7 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n> \t\tdata->own_lock = 1;\n> \t}\n> \n> -\tret = write_with_updates(refs, &transaction->refnames, err);\n> +\tret = write_with_updates(refs, transaction, err);\n> \tif (ret)\n> \t\tgoto failure;\n> \n> diff --git a/refs/refs-internal.h b/refs/refs-internal.h\n> index c97045fbed..7196f2d880 100644\n> --- a/refs/refs-internal.h\n> +++ b/refs/refs-internal.h\n> @@ -123,6 +123,11 @@ struct ref_update {\n> \t */\n> \tuint64_t index;\n> \n> +\t/*\n> +\t * Used in partial transactions to mark if a given update was rejected.\n> +\t */\n> +\tenum transaction_error rejection_err;\n> +\n> \t/*\n> \t * If this ref_update was split off of a symref update via\n> \t * split_symref_update(), then this member points at that\n> @@ -142,6 +147,14 @@ int refs_read_raw_ref(struct ref_store *ref_store, const char *refname,\n> \t\t struct object_id *oid, struct strbuf *referent,\n> \t\t unsigned int *type, int *failure_errno);\n> \n> +/*\n> + * Mark a given update as rejected with a given reason. To be used in conjuction\n> + * with the `REF_TRANSACTION_ALLOW_PARTIAL` flag to allow partial transactions.\n> + */\n> +void ref_transaction_set_rejected(struct ref_transaction *transaction,\n> +\t\t\t\t size_t update_idx,\n> +\t\t\t\t enum transaction_error err);\n> +\n> /*\n> * Add a ref_update with the specified properties to transaction, and\n> * return a pointer to the new object. This function does not verify\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index e1fd9c2de2..83cf8d582b 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -1374,8 +1374,18 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t\t\t transaction->updates[i],\n> \t\t\t\t\t &refnames_to_check, head_type,\n> \t\t\t\t\t &head_referent, &referent, err);\n> -\t\tif (ret)\n> +\t\tif (ret) {\n> +\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n> +\t\t\t ret != TRANSACTION_GENERIC_ERROR) {\n> +\t\t\t\tref_transaction_set_rejected(transaction, i, ret);\n> +\n> +\t\t\t\tstrbuf_setlen(err, 0);\n> +\t\t\t\tret = TRANSACTION_OK;\n> +\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> \t\t\tgoto done;\n> +\t\t}\n> \t}\n> \n> \tstring_list_sort(&refnames_to_check);\n> \n\n"},{"id":"513024","messageId":"ceda422e-8c8e-4a1d-aaab-9a7a2fc009dd@gmail.com","threadId":"62998","inReplyTo":"20250225-245-partially-atomic-ref-updates-v2-7-cfa3236895d7@gmail.com","subject":"Re: [PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-02-25T14:59:14Z","receivedAt":"2025-02-25T14:59:17Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Karthik\n\nOn 25/02/2025 09:29, Karthik Nayak wrote:\n> When updating multiple references through stdin, Git's update-ref\n> command normally aborts the entire transaction if any single update\n> fails. While this atomic behavior prevents partial updates by default,\n> there are cases where applying successful updates while reporting\n> failures is desirable.\n> \n> Add a new `--allow-partial` flag that allows the transaction to continue\n> even when individual reference updates fail. This flag can only be used\n> in `--stdin` mode and builds upon the partial transaction support added\n> to the refs subsystem.\n\nAs '--stdin' allows a single instance of \"git update-ref\" to create more \nthan one transaction perhaps we should instead allow the caller to \nspecify which transactions they want to allow to fail by passing an \nargument to \"start\", similar to how we support \"no-deref\" with \"update\"\n\n> following format:\n> \n> rejected SP ( | ) SP ( | ) SP LF\n> \n> or with `-z`:\n> \n> rejected NUL ( | ) NUL ( | ) NUL NUL\n\nWhat's the reason for the different output with '-z'? In the list of \noptions '-z' is documented as only applying to the input stream. Looking \nat the code the existing messages generated by report_ok() are all \nprinted to stdout with a LF terminator.\n\n> +static void print_rejected_refs(const char *refname,\n> +\t\t\t\tconst struct object_id *old_oid,\n> +\t\t\t\tconst struct object_id *new_oid,\n> +\t\t\t\tconst char *old_target,\n> +\t\t\t\tconst char *new_target,\n> +\t\t\t\tenum transaction_error err,\n> +\t\t\t\tvoid *cb_data UNUSED)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tchar space = ' ';\n> +\tconst char *reason = \"\";\n> +\n> +\tswitch (err) {\n> +\tcase TRANSACTION_NAME_CONFLICT:\n> +\t\treason = _(\"refname conflict\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_CREATE_EXISTS:\n> +\t\treason = _(\"reference already exists\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_NONEXISTENT_REF:\n> +\t\treason = _(\"reference does not exist\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n> +\t\treason = _(\"incorrect old value provided\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_INVALID_NEW_VALUE:\n> +\t\treason = _(\"invalid new value provided\");\n> +\t\tbreak;\n> +\tcase TRANSACTION_EXPECTED_SYMREF:\n> +\t\treason = _(\"expected symref but found regular ref\");\n> +\t\tbreak;\n> +\tdefault:\n> +\t\treason = _(\"unkown failure\");\n> +\t}\n\nI agree with Patrick that these messages should not be translated.\n\n> +\tif (!line_termination)\n> +\t\tspace = line_termination;\n> +\n> +\tstrbuf_addf(&sb, \"rejected%c%s%c%s%c%c%s%c%s%c\", space,\n> +\t\t refname, space, new_oid ? oid_to_hex(new_oid) : new_target,\n> +\t\t space, space, old_oid ? oid_to_hex(old_oid) : old_target,\n> +\t\t space, reason, line_termination);\n> +\n> +\tfwrite(sb.buf, sb.len, 1, stdout);\n> +\tstrbuf_release(&sb);\n> +\tfflush(stdout);\n\nThere is no need to flush after each line, we'll flush all the error \nmessages when we call report_ok() in parse_cmd_commit() or when the \nprogram exits. The caller has no way to know how many error messages \nthere are to read so flushing each one individually does not help the \nreader avoid deadlocks.\n\n> +}\n> +\n> static void parse_cmd_commit(struct ref_transaction *transaction,\n> \t\t\t const char *next, const char *end UNUSED)\n> {\n> @@ -573,6 +622,10 @@ static void parse_cmd_commit(struct ref_transaction *transaction,\n> \t\tdie(\"commit: extra input: %s\", next);\n> \tif (ref_transaction_commit(transaction, &error))\n> \t\tdie(\"commit: %s\", error.buf);\n> +\n> +\tref_transaction_for_each_rejected_update(transaction,\n> +\t\t\t\t\t\t print_rejected_refs, NULL);\n> +\n> \treport_ok(\"commit\");\n\nThis is good, the caller knows to stop reading when they see \"commit: ok\"\n\n\nBest Wishes\n\nPhillip\n\n> \tref_transaction_free(transaction);\n> }\n> @@ -609,7 +662,7 @@ static const struct parse_cmd {\n> \t{ \"commit\", parse_cmd_commit, 0, UPDATE_REFS_CLOSED },\n> };\n> \n> -static void update_refs_stdin(void)\n> +static void update_refs_stdin(unsigned int flags)\n> {\n> \tstruct strbuf input = STRBUF_INIT, err = STRBUF_INIT;\n> \tenum update_refs_state state = UPDATE_REFS_OPEN;\n> @@ -617,7 +670,7 @@ static void update_refs_stdin(void)\n> \tint i, j;\n> \n> \ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\t 0, &err);\n> +\t\t\t\t\t\t flags, &err);\n> \tif (!transaction)\n> \t\tdie(\"%s\", err.buf);\n> \n> @@ -685,7 +738,7 @@ static void update_refs_stdin(void)\n> \t\t\t */\n> \t\t\tstate = cmd->state;\n> \t\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\t\t\t 0, &err);\n> +\t\t\t\t\t\t\t\t flags, &err);\n> \t\t\tif (!transaction)\n> \t\t\t\tdie(\"%s\", err.buf);\n> \n> @@ -701,6 +754,8 @@ static void update_refs_stdin(void)\n> \t\t/* Commit by default if no transaction was requested. */\n> \t\tif (ref_transaction_commit(transaction, &err))\n> \t\t\tdie(\"%s\", err.buf);\n> +\t\tref_transaction_for_each_rejected_update(transaction,\n> +\t\t\t\t\t\t print_rejected_refs, NULL);\n> \t\tref_transaction_free(transaction);\n> \t\tbreak;\n> \tcase UPDATE_REFS_STARTED:\n> @@ -726,7 +781,9 @@ int cmd_update_ref(int argc,\n> \tconst char *refname, *oldval;\n> \tstruct object_id oid, oldoid;\n> \tint delete = 0, no_deref = 0, read_stdin = 0, end_null = 0;\n> -\tint create_reflog = 0;\n> +\tint create_reflog = 0, allow_partial = 0;\n> +\tunsigned int flags = 0;\n> +\n> \tstruct option options[] = {\n> \t\tOPT_STRING( 'm', NULL, &msg, N_(\"reason\"), N_(\"reason of the update\")),\n> \t\tOPT_BOOL('d', NULL, &delete, N_(\"delete the reference\")),\n> @@ -735,6 +792,8 @@ int cmd_update_ref(int argc,\n> \t\tOPT_BOOL('z', NULL, &end_null, N_(\"stdin has NUL-terminated arguments\")),\n> \t\tOPT_BOOL( 0 , \"stdin\", &read_stdin, N_(\"read updates from stdin\")),\n> \t\tOPT_BOOL( 0 , \"create-reflog\", &create_reflog, N_(\"create a reflog\")),\n> +\t\tOPT_BIT('0', \"allow-partial\", &flags, N_(\"allow partial transactions\"),\n> +\t\t\tREF_TRANSACTION_ALLOW_PARTIAL),\n> \t\tOPT_END(),\n> \t};\n> \n> @@ -756,9 +815,10 @@ int cmd_update_ref(int argc,\n> \t\t\tusage_with_options(git_update_ref_usage, options);\n> \t\tif (end_null)\n> \t\t\tline_termination = '\\0';\n> -\t\tupdate_refs_stdin();\n> +\t\tupdate_refs_stdin(flags);\n> \t\treturn 0;\n> -\t}\n> +\t} else if (allow_partial)\n> +\t\tdie(\"--allow-partial can only be used with --stdin\");\n> \n> \tif (end_null)\n> \t\tusage_with_options(git_update_ref_usage, options);\n> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\n> index 29045aad43..fb9442982e 100755\n> --- a/t/t1400-update-ref.sh\n> +++ b/t/t1400-update-ref.sh\n> @@ -2066,6 +2066,222 @@ do\n> \t\tgrep \"$(git rev-parse $a) $(git rev-parse $a)\" actual\n> \t'\n> \n> +\ttest_expect_success \"stdin $type allow-partial\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit commit &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$head\" \"$Z\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head\" \"$Z\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with invalid new_oid\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit update-ref refs/heads/ref2 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$(test_oid 001)\" \"$head\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"invalid new value provided\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with non-commit new_oid\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\thead_tree=$(git rev-parse HEAD^{tree}) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit update-ref refs/heads/ref2 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head_tree\" \"$head\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"invalid new value provided\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with non-existent ref\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$head\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n> +\t\t\ttest_grep -q \"reference does not exist\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with dangling symref\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit symbolic-ref refs/heads/ref2 refs/heads/nonexistent &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$head\" >>stdin &&\n> +\t\t\tgit update-ref $type --no-deref --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\ttest_must_fail git rev-parse refs/heads/ref2 &&\n> +\t\t\ttest_grep -q \"reference does not exist\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with regular ref as symref\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit update-ref refs/heads/ref2 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"symref-update refs/heads/ref2\" \"$old_head\" \"ref\" \"refs/heads/nonexistent\" >>stdin &&\n> +\t\t\tgit update-ref $type --no-deref --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"expected symref but found regular ref\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with invalid old_oid\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit update-ref refs/heads/ref2 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$old_head\" \"$Z\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"reference already exists\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\ttest_expect_success \"stdin $type allow-partial with incorrect old oid\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref1 $head &&\n> +\t\t\tgit update-ref refs/heads/ref2 $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref1\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref2\" \"$head\" \"$old_head\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref1 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\techo $head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref2 >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"incorrect old value provided\" stdout\n> +\t\t)\n> +\t'\n> +\n> +\t# F/D conflicts on the files backend are resolved on an individual\n> +\t# update level since refs are stored as files. On the reftable backend\n> +\t# this check is batched to optimize for performance, so failures cannot\n> +\t# be isolated to a single update.\n> +\ttest_expect_success REFFILES \"stdin $type allow-partial refname conflict\" '\n> +\t\tgit init repo &&\n> +\t\ttest_when_finished \"rm -fr repo\" &&\n> +\t\t(\n> +\t\t\tcd repo &&\n> +\t\t\ttest_commit one &&\n> +\t\t\told_head=$(git rev-parse HEAD) &&\n> +\t\t\ttest_commit two &&\n> +\t\t\thead=$(git rev-parse HEAD) &&\n> +\t\t\tgit update-ref refs/heads/ref/foo $head &&\n> +\n> +\t\t\tformat_command $type \"update refs/heads/ref/foo\" \"$old_head\" \"$head\" >stdin &&\n> +\t\t\tformat_command $type \"update refs/heads/ref\" \"$old_head\" \"\" >>stdin &&\n> +\t\t\tgit update-ref $type --stdin --allow-partial stdout &&\n> +\t\t\techo $old_head >expect &&\n> +\t\t\tgit rev-parse refs/heads/ref/foo >actual &&\n> +\t\t\ttest_cmp expect actual &&\n> +\t\t\ttest_grep -q \"refname conflict\" stdout\n> +\t\t)\n> +\t'\n> done\n> \n> test_expect_success 'update-ref should also create reflog for HEAD' '\n> \n\n"},{"id":"513428","messageId":"CAOLa=ZRVHUspvZ9PJiNbj+rqrHSGg-EamLN3AkVX0XuJkL3OHQ@mail.gmail.com","threadId":"62998","inReplyTo":"Z72kmFUARRSukMBe@pks.im","subject":"Re: [PATCH v2 5/7] refs: introduce enum-based transaction error types","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-03T20:12:47Z","receivedAt":"2025-03-03T20:12:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Feb 25, 2025 at 10:29:08AM +0100, Karthik Nayak wrote:\n>> diff --git a/refs.h b/refs.h\n>> index b14ba1f9ff..8e9ead174c 100644\n>> --- a/refs.h\n>> +++ b/refs.h\n>> @@ -16,6 +16,31 @@ struct worktree;\n>> enum ref_storage_format ref_storage_format_by_name(const char *name);\n>> const char *ref_storage_format_to_name(enum ref_storage_format ref_storage_format);\n>>\n>> +/*\n>> + * enum transaction_error represents the following return codes:\n>> + * TRANSACTION_OK: success code.\n>> + * TRANSACTION_GENERIC_ERROR error_code: default error code.\n>> + * TRANSACTION_NAME_CONFLICT error_code: ref name conflict like A vs A/B.\n>> + * TRANSACTION_CREATE_EXISTS error_code: ref to be created already exists.\n>> + * TRANSACTION_NONEXISTENT_REF error_code: ref expected but doesn't exist.\n>> + * TRANSACTION_INCORRECT_OLD_VALUE error_code: provided old_oid or old_target of\n>> + * reference doesn't match actual.\n>> + * TRANSACTION_INVALID_NEW_VALUE error_code: provided new_oid or new_target is\n>> + * invalid.\n>> + * TRANSACTION_EXPECTED_SYMREF error_code: expected ref to be symref, but is a\n>> + * regular ref.\n>> + */\n>> +enum transaction_error {\n>> +\tTRANSACTION_OK = 0,\n>> +\tTRANSACTION_GENERIC_ERROR = -1,\n>> +\tTRANSACTION_NAME_CONFLICT = -2,\n>> +\tTRANSACTION_CREATE_EXISTS = -3,\n>> +\tTRANSACTION_NONEXISTENT_REF = -4,\n>> +\tTRANSACTION_INCORRECT_OLD_VALUE = -5,\n>> +\tTRANSACTION_INVALID_NEW_VALUE = -6,\n>> +\tTRANSACTION_EXPECTED_SYMREF = -7,\n>> +};\n>\n> Nit: how about we name this `ref_transaction_error` and adapt the the\n> enum values accordingly? We may eventually also introduce similar errors\n> for the object database, so it may make sense to have the errors be\n> specific. Doing both the enum and changing the name might be a bit hard\n> to review, so we could also rename in a preparatory commit. Or we just\n> punt on it for now and do it once it becomes necessary, that would also\n> be fine with me.\n>\n\nI'm happy to rename it, should be easier now. We can always change later\nif needed.\n\n> I also wonder whether we really want to introduce `TRANSACTION_OK`. It's\n> always a bit of a mouthful, and in many cases one ends up with a mixture\n> of `ret < 0`, `ret != TRANSACTION_OK` and `ret != 0`, which may lead to\n> confusion. Continuing to use `0` for the successful case should be fine.\n>\n\nFair enough, let me remove it!\n\n>> diff --git a/refs/packed-backend.c b/refs/packed-backend.c\n>> index 3247871574..75e1ebf67d 100644\n>> --- a/refs/packed-backend.c\n>> +++ b/refs/packed-backend.c\n>> @@ -1672,7 +1676,8 @@ static int packed_transaction_prepare(struct ref_store *ref_store,\n>> \t\tdata->own_lock = 1;\n>> \t}\n>>\n>> -\tif (write_with_updates(refs, &transaction->refnames, err))\n>> +\tret = write_with_updates(refs, &transaction->refnames, err);\n>> +\tif (ret)\n>> \t\tgoto failure;\n>>\n>> \ttransaction->state = REF_TRANSACTION_PREPARED;\n>\n> Do we also want to change the local variable declaration of `int ret` to\n> use the new type?\n>\n\nYes! Good catch.\n\n>> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n>> index 2c1e2995de..e1fd9c2de2 100644\n>> --- a/refs/reftable-backend.c\n>> +++ b/refs/reftable-backend.c\n>> @@ -1255,11 +1255,12 @@ static int prepare_single_update(struct reftable_ref_store *refs,\n>> \t\t\t\t\t \"but is a regular ref\"),\n>> \t\t\t\t ref_update_original_update_refname(u),\n>> \t\t\t\t u->old_target);\n>> -\t\t\treturn -1;\n>> +\t\t\treturn TRANSACTION_EXPECTED_SYMREF;\n>> \t\t}\n>>\n>> -\t\tif (ref_update_check_old_target(referent->buf, u, err)) {\n>> -\t\t\treturn -1;\n>> +\t\tret = ref_update_check_old_target(referent->buf, u, err);\n>> +\t\tif (ret) {\n>> +\t\t\treturn ret;\n>> \t\t}\n>> \t} else if ((u->flags & REF_HAVE_OLD) && !oideq(¤t_oid, &u->old_oid)) {\n>> \t\tif (is_null_oid(&u->old_oid)) {\n>\n> Nit: superfluous braces that we could remove while at it.\n>\n\nIndeed, will fix.\n\n> Patrick\n"},{"id":"513429","messageId":"CAOLa=ZTwng_NJvTZK2-Rz5JnchsTFCC6txakWcEsYh+41nqYmg@mail.gmail.com","threadId":"62998","inReplyTo":"Z72kjTRDNOM6iFkY@pks.im","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-03T20:17:41Z","receivedAt":"2025-03-03T20:17:43Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Feb 25, 2025 at 10:29:09AM +0100, Karthik Nayak wrote:\n>> diff --git a/refs.c b/refs.c\n>> index f989a46a5a..243c09c368 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2726,6 +2736,27 @@ void ref_transaction_for_each_queued_update(struct ref_transaction *transaction,\n>> \t}\n>> }\n>>\n>> +void ref_transaction_for_each_rejected_update(struct ref_transaction *transaction,\n>> +\t\t\t\t\t ref_transaction_for_each_rejected_update_fn cb,\n>> +\t\t\t\t\t void *cb_data)\n>> +{\n>> +\tif (!(transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL))\n>> +\t\treturn;\n>> +\n>> +\tfor (size_t i = 0; i < transaction->nr; i++) {\n>> +\t\tstruct ref_update *update = transaction->updates[i];\n>> +\n>> +\t\tif (!update->rejection_err)\n>> +\t\t\tcontinue;\n>\n> This kind of proves my point that `TRANSACTION_OK` is pointless and\n> leads to a mixture of using and not using the enum :)\n>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index 3b0adf8bb2..d0a53c9ace 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> @@ -2851,8 +2851,18 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n>> \t\tret = lock_ref_for_update(refs, update, transaction,\n>> \t\t\t\t\t head_ref, &refnames_to_check,\n>> \t\t\t\t\t err);\n>> -\t\tif (ret)\n>> +\t\tif (ret) {\n>> +\t\t\tif (transaction->flags & REF_TRANSACTION_ALLOW_PARTIAL &&\n>\n> Hm. If the error values were defined as a bitfield we could refactor\n> this to not be a flag, but have a `transaction->accepted_rejections`\n> instead that allows the caller to ask for only a subset of rejections to\n> be accepted.\n>\n\nI did consider making them bitfield and even tried playing with that\nidea. There are a bunch of side affects of doing that in other\nsubsystems which check call use the reference API and expect '< 0'\nerrors.\n\n> I'm not quite sure whether it is a good idea, but the logic to handle\n> all of that could be self-contained in `ref_transaction_set_rejected()`:\n> if it returned an error code itself, it would swallow any errors in case\n> the transaction allows a given error, and bubble up the error again in\n> case the error is not allowed. The function could use a rename in that\n> case though, e.g. `ref_transaction_maybe_set_rejected()`.\n>\n> Patrick\n\nI think this is great idea, it does cleanup a bunch of the code. I will\nadd this in the next version with the rename. Thanks!\n"},{"id":"513430","messageId":"CAOLa=ZQCWFGsaPN+J8R9eQMBJqNNyoeDLkLwCFBbBzTG_R_-sg@mail.gmail.com","threadId":"62998","inReplyTo":"2755fb78-d587-4ad9-b22f-b60dfa752d7a@gmail.com","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-03T20:21:04Z","receivedAt":"2025-03-03T20:21:06Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Phillip Wood writes:\n\nHello Phillip!\n\n> Hi Karthik\n>\n> On 25/02/2025 09:29, Karthik Nayak wrote:\n>> Git's reference transactions are all-or-nothing: either all updates\n>> succeed, or none do. While this atomic behavior is generally desirable,\n>> it can be suboptimal especially when using the reftable backend, where\n>> batching multiple reference updates into a single transaction is more\n>> efficient than performing them sequentially.\n>>\n>> Introduce partial transaction support with a new flag,\n>> 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\n>> individual reference updates that would typically cause the entire\n>> transaction to fail due to non-system-related errors to be marked as\n>> rejected while permitting other updates to proceed. Non-system-related\n>> errors include issues caused by user-provided input values, whereas\n>> system-related errors, such as I/O failures or memory issues, continue\n>> to result in a full transaction failure. This approach enhances\n>> flexibility while preserving transactional integrity where necessary.\n>>\n>> The implementation introduces several key components:\n>>\n>> - Add 'rejection_err' field to struct `ref_update` to track failed\n>> updates with failure reason.\n>>\n>> - Modify reference backends (files, packed, reftable) to handle\n>> partial transactions by using `ref_transaction_set_rejected()`\n>> instead of failing the entire transaction when\n>> `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n>>\n>> - Add `ref_transaction_for_each_rejected_update()` to let callers\n>> examine which updates were rejected and why.\n>\n> I think this is a much better design. I wonder if we want to signal to\n> the caller of ref_transaction_commit() that there were ignored errors\n> rather than forcing them to call ref_transaction_for_each_rejected() to\n> find that out. Another possibility would be to call the callback from\n> ref_transaction_commit() but that would mean changing the signature of\n> ref_transaction_begin() to take the callback and user data when\n> REF_TRANSACTION_ALLOW_PARTIAL is passed.\n>\n\nYes, I did toy around modifying `ref_transaction_*` at first, but I\nthink the current implementation is slightly better. Users of the ref\nAPI do not have to worry about complexity of partial transactions unless\nthey really need to. So in that case, for most users, the API remains\nsimple and clean, and for specific users who do want partial transaction\nsupport, they can activate it via the flag and use the iterator to\ncollect the rejections at the end.\n\n> Best Wishes\n>\n> Phillip\n>\n[snip]\n"},{"id":"513431","messageId":"CAOLa=ZQxziRioY9Ws6jVE7MikJifrVB3p9ifNMBn6cQFjuLZYw@mail.gmail.com","threadId":"62998","inReplyTo":"Z72klAEpQ0-cWCGa@pks.im","subject":"Re: [PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-03T20:22:17Z","receivedAt":"2025-03-03T20:22:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Tue, Feb 25, 2025 at 10:29:10AM +0100, Karthik Nayak wrote:\n>> diff --git a/Documentation/git-update-ref.adoc b/Documentation/git-update-ref.adoc\n>> index 9e6935d38d..fc73f1d8aa 100644\n>> --- a/Documentation/git-update-ref.adoc\n>> +++ b/Documentation/git-update-ref.adoc\n>> @@ -7,8 +7,10 @@ git-update-ref - Update the object name stored in a ref safely\n>>\n>> SYNOPSIS\n>> --------\n>> -[verse]\n>> -'git update-ref' [-m ] [--no-deref] (-d [] | [--create-reflog] [] | --stdin [-z])\n>> +[synopsis]\n>> +git update-ref [-m ] [--no-deref] -d []\n>> +\t [-m ] [--no-deref] [--create-reflog] []\n>> + [-m ] [--no-deref] --stdin [-z] [--allow-partial]\n>>\n>> DESCRIPTION\n>> -----------\n>> @@ -57,6 +59,17 @@ performs all modifications together. Specify commands of the form:\n>> With `--create-reflog`, update-ref will create a reflog for each ref\n>> even if one would not ordinarily be created.\n>>\n>> +With `--allow-partial`, update-ref continues executing the transaction even if\n>> +some updates fail due to invalid or incorrect user input, applying only the\n>> +successful updates. Errors resulting from user-provided input are treated as\n>> +non-system-related and do not cause the entire transaction to be aborted.\n>> +However, system-related errors—such as I/O failures or memory issues—will still\n>> +result in a full failure. Additionally, errors like F/D conflicts are batched\n>> +for performance optimization and will also cause a full failure. Any failed\n>> +updates will be reported in the following format:\n>\n> Shouldn't it be possible to detect F/D conflicts though and not abort\n> the transaction? If we want to make use of partial transactions in the\n> context of git-fetch(1) and/or git-receive-pack(1) we would have to\n> handle them.\n>\n\nYes, this was a miss in this version, mostly from me not thinking enough\nabout this series interacts with yours. This should be fixed in the next\nversion.\n\n>> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n>> index 1d541e13ad..b03b40eacb 100644\n>> --- a/builtin/update-ref.c\n>> +++ b/builtin/update-ref.c\n>> @@ -565,6 +566,54 @@ static void parse_cmd_abort(struct ref_transaction *transaction,\n>> \treport_ok(\"abort\");\n>> }\n>>\n>> +static void print_rejected_refs(const char *refname,\n>> +\t\t\t\tconst struct object_id *old_oid,\n>> +\t\t\t\tconst struct object_id *new_oid,\n>> +\t\t\t\tconst char *old_target,\n>> +\t\t\t\tconst char *new_target,\n>> +\t\t\t\tenum transaction_error err,\n>> +\t\t\t\tvoid *cb_data UNUSED)\n>> +{\n>> +\tstruct strbuf sb = STRBUF_INIT;\n>> +\tchar space = ' ';\n>> +\tconst char *reason = \"\";\n>> +\n>> +\tswitch (err) {\n>> +\tcase TRANSACTION_NAME_CONFLICT:\n>> +\t\treason = _(\"refname conflict\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_CREATE_EXISTS:\n>> +\t\treason = _(\"reference already exists\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_NONEXISTENT_REF:\n>> +\t\treason = _(\"reference does not exist\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n>> +\t\treason = _(\"incorrect old value provided\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_INVALID_NEW_VALUE:\n>> +\t\treason = _(\"invalid new value provided\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_EXPECTED_SYMREF:\n>> +\t\treason = _(\"expected symref but found regular ref\");\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\treason = _(\"unkown failure\");\n>> +\t}\n>\n> As git-update-ref(1) is part of plumbing we don't want to translate\n> those messages.\n>\n\nMakes sense, let me remove that.\n\n> Patrick\n>\nThanks for the review on the series!\n"},{"id":"513433","messageId":"CAOLa=ZTeDH+yWpK6KBK=yDFymyUFuDg7Qb3Ww7EnX8wgHC0_ag@mail.gmail.com","threadId":"62998","inReplyTo":"ceda422e-8c8e-4a1d-aaab-9a7a2fc009dd@gmail.com","subject":"Re: [PATCH v2 7/7] update-ref: add --allow-partial flag for stdin mode","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-03T20:34:13Z","receivedAt":"2025-03-03T20:34:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Phillip Wood writes:\n\n> Hi Karthik\n>\n> On 25/02/2025 09:29, Karthik Nayak wrote:\n>> When updating multiple references through stdin, Git's update-ref\n>> command normally aborts the entire transaction if any single update\n>> fails. While this atomic behavior prevents partial updates by default,\n>> there are cases where applying successful updates while reporting\n>> failures is desirable.\n>>\n>> Add a new `--allow-partial` flag that allows the transaction to continue\n>> even when individual reference updates fail. This flag can only be used\n>> in `--stdin` mode and builds upon the partial transaction support added\n>> to the refs subsystem.\n>\n> As '--stdin' allows a single instance of \"git update-ref\" to create more\n> than one transaction perhaps we should instead allow the caller to\n> specify which transactions they want to allow to fail by passing an\n> argument to \"start\", similar to how we support \"no-deref\" with \"update\"\n>\n\nI was considering adding a 'allow-rejection' flag similar to \"no-deref\"\nwith \"update\", but decided against it since that was an update level\nconfig and we want a transaction level configuration.\n\nBut I like your idea better. I'm not bullish on either the current\nimplementation or your suggestion, since we can extend one from the\nother. My main motivation to add this flag to update-ref(1) is to show\nviability. The goal would be to follow up with changes to `git-fetch(1)`\nand `git-receive-pack(1)` to use partial transactions.\n\n\n>> following format:\n>>\n>> rejected SP ( | ) SP ( | ) SP LF\n>>\n>> or with `-z`:\n>>\n>> rejected NUL ( | ) NUL ( | ) NUL NUL\n>\n> What's the reason for the different output with '-z'? In the list of\n> options '-z' is documented as only applying to the input stream. Looking\n> at the code the existing messages generated by report_ok() are all\n> printed to stdout with a LF terminator.\n>\n\nThat's a great point, there was no real reason and I think we can drop\nthis.\n\n>> +static void print_rejected_refs(const char *refname,\n>> +\t\t\t\tconst struct object_id *old_oid,\n>> +\t\t\t\tconst struct object_id *new_oid,\n>> +\t\t\t\tconst char *old_target,\n>> +\t\t\t\tconst char *new_target,\n>> +\t\t\t\tenum transaction_error err,\n>> +\t\t\t\tvoid *cb_data UNUSED)\n>> +{\n>> +\tstruct strbuf sb = STRBUF_INIT;\n>> +\tchar space = ' ';\n>> +\tconst char *reason = \"\";\n>> +\n>> +\tswitch (err) {\n>> +\tcase TRANSACTION_NAME_CONFLICT:\n>> +\t\treason = _(\"refname conflict\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_CREATE_EXISTS:\n>> +\t\treason = _(\"reference already exists\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_NONEXISTENT_REF:\n>> +\t\treason = _(\"reference does not exist\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_INCORRECT_OLD_VALUE:\n>> +\t\treason = _(\"incorrect old value provided\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_INVALID_NEW_VALUE:\n>> +\t\treason = _(\"invalid new value provided\");\n>> +\t\tbreak;\n>> +\tcase TRANSACTION_EXPECTED_SYMREF:\n>> +\t\treason = _(\"expected symref but found regular ref\");\n>> +\t\tbreak;\n>> +\tdefault:\n>> +\t\treason = _(\"unkown failure\");\n>> +\t}\n>\n> I agree with Patrick that these messages should not be translated.\n>\n>> +\tif (!line_termination)\n>> +\t\tspace = line_termination;\n>> +\n>> +\tstrbuf_addf(&sb, \"rejected%c%s%c%s%c%c%s%c%s%c\", space,\n>> +\t\t refname, space, new_oid ? oid_to_hex(new_oid) : new_target,\n>> +\t\t space, space, old_oid ? oid_to_hex(old_oid) : old_target,\n>> +\t\t space, reason, line_termination);\n>> +\n>> +\tfwrite(sb.buf, sb.len, 1, stdout);\n>> +\tstrbuf_release(&sb);\n>> +\tfflush(stdout);\n>\n> There is no need to flush after each line, we'll flush all the error\n> messages when we call report_ok() in parse_cmd_commit() or when the\n> program exits. The caller has no way to know how many error messages\n> there are to read so flushing each one individually does not help the\n> reader avoid deadlocks.\n>\n\nThat does make sense, I'll remove the flush here!\n\n>> +}\n>> +\n>> static void parse_cmd_commit(struct ref_transaction *transaction,\n>> \t\t\t const char *next, const char *end UNUSED)\n>> {\n>> @@ -573,6 +622,10 @@ static void parse_cmd_commit(struct ref_transaction *transaction,\n>> \t\tdie(\"commit: extra input: %s\", next);\n>> \tif (ref_transaction_commit(transaction, &error))\n>> \t\tdie(\"commit: %s\", error.buf);\n>> +\n>> +\tref_transaction_for_each_rejected_update(transaction,\n>> +\t\t\t\t\t\t print_rejected_refs, NULL);\n>> +\n>> \treport_ok(\"commit\");\n>\n> This is good, the caller knows to stop reading when they see \"commit: ok\"\n>\n>\n> Best Wishes\n>\n> Phillip\n\nThanks for the review on the series.\n"},{"id":"513519","messageId":"33e6e65e-c68b-4b88-a66b-53b808745a24@gmail.com","threadId":"62998","inReplyTo":"CAOLa=ZQCWFGsaPN+J8R9eQMBJqNNyoeDLkLwCFBbBzTG_R_-sg@mail.gmail.com","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2025-03-04T10:31:23Z","receivedAt":"2025-03-04T10:31:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Karthik\n\nOn 03/03/2025 20:21, Karthik Nayak wrote:\n> Phillip Wood writes:\n>> On 25/02/2025 09:29, Karthik Nayak wrote:\n>>> Git's reference transactions are all-or-nothing: either all updates\n>>> succeed, or none do. While this atomic behavior is generally desirable,\n>>> it can be suboptimal especially when using the reftable backend, where\n>>> batching multiple reference updates into a single transaction is more\n>>> efficient than performing them sequentially.\n>>>\n>>> Introduce partial transaction support with a new flag,\n>>> 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\n>>> individual reference updates that would typically cause the entire\n>>> transaction to fail due to non-system-related errors to be marked as\n>>> rejected while permitting other updates to proceed. Non-system-related\n>>> errors include issues caused by user-provided input values, whereas\n>>> system-related errors, such as I/O failures or memory issues, continue\n>>> to result in a full transaction failure. This approach enhances\n>>> flexibility while preserving transactional integrity where necessary.\n>>>\n>>> The implementation introduces several key components:\n>>>\n>>> - Add 'rejection_err' field to struct `ref_update` to track failed\n>>> updates with failure reason.\n>>>\n>>> - Modify reference backends (files, packed, reftable) to handle\n>>> partial transactions by using `ref_transaction_set_rejected()`\n>>> instead of failing the entire transaction when\n>>> `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n>>>\n>>> - Add `ref_transaction_for_each_rejected_update()` to let callers\n>>> examine which updates were rejected and why.\n>>\n>> I think this is a much better design. I wonder if we want to signal to\n>> the caller of ref_transaction_commit() that there were ignored errors\n>> rather than forcing them to call ref_transaction_for_each_rejected() to\n>> find that out. Another possibility would be to call the callback from\n>> ref_transaction_commit() but that would mean changing the signature of\n>> ref_transaction_begin() to take the callback and user data when\n>> REF_TRANSACTION_ALLOW_PARTIAL is passed.\n>>\n> \n> Yes, I did toy around modifying `ref_transaction_*` at first, but I\n> think the current implementation is slightly better. Users of the ref\n> API do not have to worry about complexity of partial transactions unless\n> they really need to. So in that case, for most users, the API remains\n> simple and clean, and for specific users who do want partial transaction\n> support, they can activate it via the flag and use the iterator to\n> collect the rejections at the end.\n\nThat makes sense. I have a slight concern that iterating through the \nerrors is O(number of ref updates) rather than O(number of errors). If \nwe expect most updates to succeed that it is a shame to have to check \nthem all just to see there were no errors. Maybe we could be store the \nerrors in a separate list of (update-index, error) pairs to avoid that.\n\nBest Wishes\n\nPhillip\n\n"},{"id":"513601","messageId":"CAOLa=ZQ-_yoPHAy5f_Z=OpPXKfiy_m2yuPqa8ekk+pKZGryafw@mail.gmail.com","threadId":"62998","inReplyTo":"33e6e65e-c68b-4b88-a66b-53b808745a24@gmail.com","subject":"Re: [PATCH v2 6/7] refs: implement partial reference transaction support","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-03-05T14:20:21Z","receivedAt":"2025-03-05T14:20:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Phillip Wood writes:\n\n> Hi Karthik\n>\n> On 03/03/2025 20:21, Karthik Nayak wrote:\n>> Phillip Wood writes:\n>>> On 25/02/2025 09:29, Karthik Nayak wrote:\n>>>> Git's reference transactions are all-or-nothing: either all updates\n>>>> succeed, or none do. While this atomic behavior is generally desirable,\n>>>> it can be suboptimal especially when using the reftable backend, where\n>>>> batching multiple reference updates into a single transaction is more\n>>>> efficient than performing them sequentially.\n>>>>\n>>>> Introduce partial transaction support with a new flag,\n>>>> 'REF_TRANSACTION_ALLOW_PARTIAL'. When enabled, this flag allows\n>>>> individual reference updates that would typically cause the entire\n>>>> transaction to fail due to non-system-related errors to be marked as\n>>>> rejected while permitting other updates to proceed. Non-system-related\n>>>> errors include issues caused by user-provided input values, whereas\n>>>> system-related errors, such as I/O failures or memory issues, continue\n>>>> to result in a full transaction failure. This approach enhances\n>>>> flexibility while preserving transactional integrity where necessary.\n>>>>\n>>>> The implementation introduces several key components:\n>>>>\n>>>> - Add 'rejection_err' field to struct `ref_update` to track failed\n>>>> updates with failure reason.\n>>>>\n>>>> - Modify reference backends (files, packed, reftable) to handle\n>>>> partial transactions by using `ref_transaction_set_rejected()`\n>>>> instead of failing the entire transaction when\n>>>> `REF_TRANSACTION_ALLOW_PARTIAL` is set.\n>>>>\n>>>> - Add `ref_transaction_for_each_rejected_update()` to let callers\n>>>> examine which updates were rejected and why.\n>>>\n>>> I think this is a much better design. I wonder if we want to signal to\n>>> the caller of ref_transaction_commit() that there were ignored errors\n>>> rather than forcing them to call ref_transaction_for_each_rejected() to\n>>> find that out. Another possibility would be to call the callback from\n>>> ref_transaction_commit() but that would mean changing the signature of\n>>> ref_transaction_begin() to take the callback and user data when\n>>> REF_TRANSACTION_ALLOW_PARTIAL is passed.\n>>>\n>>\n>> Yes, I did toy around modifying `ref_transaction_*` at first, but I\n>> think the current implementation is slightly better. Users of the ref\n>> API do not have to worry about complexity of partial transactions unless\n>> they really need to. So in that case, for most users, the API remains\n>> simple and clean, and for specific users who do want partial transaction\n>> support, they can activate it via the flag and use the iterator to\n>> collect the rejections at the end.\n>\n> That makes sense. I have a slight concern that iterating through the\n> errors is O(number of ref updates) rather than O(number of errors). If\n> we expect most updates to succeed that it is a shame to have to check\n> them all just to see there were no errors. Maybe we could be store the\n> errors in a separate list of (update-index, error) pairs to avoid that.\n>\n\nThat is a good point, let me add in a array inside 'ref_transaction'\nwhich will track rejected updates.\n\n> Best Wishes\n>\n> Phillip\n\nThanks!\n"}]}