{"thread":{"id":"63455","subject":"[PATCH 0/3] fetch/receive: use batched reference updates","startedAt":"2025-05-14T09:03:58Z","lastAt":"2025-05-18T11:30:26Z","messageCount":30,"participants":["Karthik Nayak","Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"518019","messageId":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com","threadId":"63455","inReplyTo":null,"subject":"[PATCH 0/3] fetch/receive: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-14T09:03:46Z","receivedAt":"2025-05-14T09:03:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The git-fetch(1) and git-receive-pack(1) commands update references as\npart of the flow. Each reference update is treated as a single entity\nand a transaction is created for each update.\n\nThis can be really slow, specifically in reference backends where there\nare optimizations which ensure a single transaction with 'N' reference\nupdate perform much faster than 'N' individual transactions. Also having\n'N' individual transactions has buildup and teardown costs. These costs\nadd up in repositories with a large number of references.\n\nAlso specifically in the reftable backend, 'N' individual transactions\nwould also trigger auto-compaction for every transaction.\n\nThe reasoning for using individual transactions is because we want to\nallow partial updates of references in these commands. Using a single\ntransaction would be an all-or-nothing scenario.\n\nRecently we introduced an in-between solution called batched reference\nupdates in 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08). This allows us to batch a set of reference updates, where\nindividual updates can pass/fail without affecting the batch.\n\nThis patch series, modifies both 'git-fetch(1)' and\n'git-receive-pack(1)' to use this mechanism. With this, we see a\nsignificant performance boost:\n\n+---------------------+---------------+------------------+\n| | files backend | reftable backend |\n+---------------------+---------------+------------------+\n| git-fetch(1) | 1.25x | 22x |\n| git-receive-pack(1) | 1.21x | 18x |\n+---------------------+---------------+------------------+\n\nThe first and third patch handle the changes for 'git-fetch(1)' and\n'git-receive-pack(1)' respectively. The second patch fixes a small\nmemory leak I encountered while working on this series.\n\nThis is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of\nhttps://github.com/j6t/gitk, 2025-05-09). There were no conflicts \nobserved with next or seen.\n\n---\n builtin/fetch.c | 119 +++++++++++++++++++++------------------\n builtin/receive-pack.c | 88 +++++++++++++++++++++++------\n send-pack.c | 7 +++\n t/t1416-ref-transaction-hooks.sh | 2 -\n t/t5408-send-pack-stdin.sh | 14 ++++-\n 5 files changed, 155 insertions(+), 75 deletions(-)\n\nKarthik Nayak (3):\n fetch: use batched reference updates\n send-pack: fix memory leak around duplicate refs\n receive-pack: use batched reference updates\n\n\n\nbase-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0\nchange-id: 20250423-501-update-git-fetch-1-to-use-partial-transactions-7fb66339fe79\n\nThanks\n- Karthik\n\n"},{"id":"518020","messageId":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-1-7c65f46493d4@gmail.com","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com","subject":"[PATCH 1/3] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-14T09:03:47Z","receivedAt":"2025-05-14T09:03:58Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference updates performed as a part of 'git-fetch(1)', take place\none at a time. For each reference update, a new transaction is created\nand committed. This is necessary to ensure we can allow individual\nupdates to fail without failing the entire command. The command also\nsupports an '--atomic' mode, which uses a single transaction to update\nall of the references. But this mode has an all-or-nothing approach,\nwhere if a single update fails, all updates would fail.\n\nIn 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08), we introduced a new mechanism to batch reference updates.\nUnder the hood, this uses a single transaction to perform a batch of\nreference updates, while allowing only individual updates to fail.\nUtilize this newly introduced batch update mechanism in 'git-fetch(1)'.\nThis provides a significant bump in performance, especially when dealing\nwith repositories with large number of references.\n\nAdding support for batched updates is simply modifying the flow to also\ncreate a batch update transaction in the non-atomic flow.\n\nWith the reftable backend there is a 22x performance improvement, when\nperforming 'git-fetch(1)' with 10000 refs:\n\n Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n Time (mean ± σ): 3.403 s ± 0.775 s [User: 1.875 s, System: 1.417 s]\n Range (min … max): 2.454 s … 4.529 s 10 runs\n\n Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 154.3 ms ± 17.6 ms [User: 102.5 ms, System: 56.1 ms]\n Range (min … max): 145.2 ms … 220.5 ms 18 runs\n\n Summary\n fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n 22.06 ± 5.62 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n\nIn similar conditions, the files backend sees a 1.25x performance\nimprovement:\n\n Benchmark 1: fetch: many refs (refformat = files, refcount = 10000, revision = master)\n Time (mean ± σ): 605.5 ms ± 9.4 ms [User: 117.8 ms, System: 483.3 ms]\n Range (min … max): 595.6 ms … 621.5 ms 10 runs\n\n Benchmark 2: fetch: many refs (refformat = files, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 485.8 ms ± 4.3 ms [User: 91.1 ms, System: 396.7 ms]\n Range (min … max): 477.6 ms … 494.3 ms 10 runs\n\n Summary\n fetch: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n 1.25 ± 0.02 times faster than fetch: many refs (refformat = files, refcount = 10000, revision = master)\n\nWith this we'll either be using a regular transaction or a batch update\ntransaction. This helps cleanup some code which is no longer needed as\nwe'll now always have some type of 'ref_transaction' object being\npropagated.\n\nOne big change is that earlier, each individual update would propagate a\nfailure. Whereas now, the `ref_transaction_for_each_rejected_update`\nfunction is called at the end of the flow to capture the exit status for\n'git-fetch(1)' and also to print F/D conflict errors. This does change\nthe order of the errors being printed, but the behavior stays the same.\n\nSince transaction errors are now explicitly defined as part of\n76e760b999 (refs: introduce enum-based transaction error types,\n2025-04-08), utilize them and get rid of custom errors defined within\n'builtin/fetch.c'.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 119 +++++++++++++++++++++++++++++++-------------------------\n 1 file changed, 65 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5279997c96..1558f6d1e8 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -641,9 +641,6 @@ static struct ref *get_ref_map(struct remote *remote,\n \treturn ref_map;\n }\n \n-#define STORE_REF_ERROR_OTHER 1\n-#define STORE_REF_ERROR_DF_CONFLICT 2\n-\n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tstruct ref_transaction *transaction,\n@@ -651,7 +648,6 @@ static int s_update_ref(const char *action,\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -661,43 +657,10 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\t/*\n-\t * If no transaction was passed to us, we manage the transaction\n-\t * ourselves. Otherwise, we trust the caller to handle the transaction\n-\t * lifecycle.\n-\t */\n-\tif (!transaction) {\n-\t\ttransaction = our_transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t\t\t\t 0, &err);\n-\t\tif (!transaction) {\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n-\n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n \t\t\t\t check_old ? &ref->old_oid : NULL,\n \t\t\t\t NULL, NULL, 0, msg, &err);\n-\tif (ret) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n-\t}\n \n-\tif (our_transaction) {\n-\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n-\t\tcase 0:\n-\t\t\tbreak;\n-\t\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n-\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n-\t\t\tgoto out;\n-\t\tdefault:\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n-\n-out:\n-\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n@@ -1139,7 +1102,6 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\n \n static int store_updated_refs(struct display_state *display_state,\n-\t\t\t const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head,\n@@ -1277,11 +1239,6 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t}\n \t}\n \n-\tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n-\t\terror(_(\"some local refs could not be updated; try running\\n\"\n-\t\t \" 'git remote prune %s' to remove any old, conflicting \"\n-\t\t \"branches\"), remote_name);\n-\n \tif (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {\n \t\tif (!config->show_forced_updates) {\n \t\t\twarning(_(warn_show_forced_updates));\n@@ -1366,9 +1323,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(display_state, transport->remote->name,\n-\t\t\t\t connectivity_checked, transaction, ref_map,\n-\t\t\t\t fetch_head, config);\n+\tret = store_updated_refs(display_state, connectivity_checked,\n+\t\t\t\t transaction, ref_map, fetch_head, config);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1688,6 +1644,32 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n \treturn result;\n }\n \n+struct ref_rejection_data {\n+\tint *retcode;\n+\tint conflict_msg_shown;\n+\tconst char *remote_name;\n+};\n+\n+static void ref_transaction_rejection_handler(const char *refname UNUSED,\n+\t\t\t\t\t const struct object_id *old_oid UNUSED,\n+\t\t\t\t\t const struct object_id *new_oid UNUSED,\n+\t\t\t\t\t const char *old_target UNUSED,\n+\t\t\t\t\t const char *new_target UNUSED,\n+\t\t\t\t\t enum ref_transaction_error err,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n+\n+\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n+\t\terror(_(\"some local refs could not be updated; try running\\n\"\n+\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n+\t\t\t\"branches\"), data->remote_name);\n+\t\tdata->conflict_msg_shown = 1;\n+\t}\n+\n+\t*data->retcode = 1;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1808,6 +1790,20 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n+\t/*\n+\t * If not atomic, we can still use batched updates, which would be much\n+\t * more performent. We don't initiate the transaction before pruning,\n+\t * since pruning must be an independent step, to avoid F/D conflicts.\n+\t */\n+\tif (!transaction) {\n+\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\tif (!transaction) {\n+\t\t\tretcode = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n \t\tretcode = 1;\n@@ -1839,16 +1835,31 @@ static int do_fetch(struct transport *transport,\n \t\tfree_refs(tags_ref_map);\n \t}\n \n-\tif (transaction) {\n-\t\tif (retcode)\n-\t\t\tgoto cleanup;\n+\tif (retcode)\n+\t\tgoto cleanup;\n+\n+\tretcode = ref_transaction_commit(transaction, &err);\n+\tif (retcode) {\n+\t\t/*\n+\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t * aborting an already closed transaction.\n+\t\t */\n+\t\tref_transaction_free(transaction);\n+\t\ttransaction = NULL;\n+\t\tgoto cleanup;\n+\t}\n \n-\t\tretcode = ref_transaction_commit(transaction, &err);\n+\tif (!atomic_fetch) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.retcode = &retcode,\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = transport->remote->name,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n \t\tif (retcode) {\n-\t\t\t/*\n-\t\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t\t * aborting an already closed transaction.\n-\t\t\t */\n \t\t\tref_transaction_free(transaction);\n \t\t\ttransaction = NULL;\n \t\t\tgoto cleanup;\n\n-- \n2.49.0\n\n"},{"id":"518021","messageId":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-3-7c65f46493d4@gmail.com","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com","subject":"[PATCH 3/3] receive-pack: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-14T09:03:49Z","receivedAt":"2025-05-14T09:04:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference updates performed as a part of 'git-receive-pack(1)', take\nplace one at a time. For each reference update, a new transaction is\ncreated and committed. This is necessary to ensure we can allow\nindividual updates to fail without failing the entire command. The\ncommand also supports an 'atomic' mode, which uses a single transaction\nto update all of the references. But this mode has an all-or-nothing\napproach, where if a single update fails, all updates would fail.\n\nIn 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08), we introduced a new mechanism to batch reference updates.\nUnder the hood, this uses a single transaction to perform a batch of\nreference updates, while allowing only individual updates to fail.\nUtilize this newly introduced batch update mechanism in\n'git-receive-pack(1)'. This provides a significant bump in performance,\nespecially when dealing with repositories with large number of\nreferences.\n\nWith the reftable backend there is a 18x performance improvement, when\nperforming receive-pack with 10000 refs:\n\n Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n Time (mean ± σ): 4.276 s ± 0.078 s [User: 0.796 s, System: 3.318 s]\n Range (min … max): 4.185 s … 4.430 s 10 runs\n\n Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 235.4 ms ± 6.9 ms [User: 75.4 ms, System: 157.3 ms]\n Range (min … max): 228.5 ms … 254.2 ms 11 runs\n\n Summary\n receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n 18.16 ± 0.63 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n\nIn similar conditions, the files backend sees a 1.21x performance\nimprovement:\n\n Benchmark 1: receive: many refs (refformat = files, refcount = 10000, revision = master)\n Time (mean ± σ): 1.121 s ± 0.021 s [User: 0.128 s, System: 0.975 s]\n Range (min … max): 1.097 s … 1.156 s 10 runs\n\n Benchmark 2: receive: many refs (refformat = files, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 927.9 ms ± 22.6 ms [User: 99.0 ms, System: 815.2 ms]\n Range (min … max): 903.1 ms … 978.0 ms 10 runs\n\n Summary\n receive: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n 1.21 ± 0.04 times faster than receive: many refs (refformat = files, refcount = 10000, revision = master)\n\nAs using batched updates requires the error handling to be moved to the\nend of the flow, create and use a 'struct strset' to track the failed\nrefs and attribute the correct errors to them.\n\nThis change also uncovers an issue when a client provides multiple\nupdates to the same reference. For example:\n\n $ git send-pack remote.git A:foo B:foo\n Enumerating objects: 3, done.\n Counting objects: 100% (3/3), done.\n Delta compression using up to 20 threads\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 226 bytes | 226.00 KiB/s, done.\n Total 3 (delta 1), reused 0 (delta 0), pack-reused 0 (from 0)\n remote: error: cannot lock ref 'refs/heads/foo': reference already exists\n To remote.git\n ! [remote rejected] A -> foo (failed to update ref)\n ! [remote failure] B -> foo (remote failed to report status)\n\nAs you can see, the remote runs into an error because it cannot lock the\ntarget reference for the second update. Furthermore, the remote complains\nthat the first update has been rejected whereas the second update didn't\nreceive any status update because we failed to lock it. Reading this status\nmessage alone a user would probably expect that `foo` has not been updated\nat all. But that's not the case: while we claim that the ref wasn't updated,\nit surprisingly points to `A` now.\n\nOne could argue that this is merely an error in how we report the result of\nthis push. But ultimately, the user's request itself is already broken and\ndoesn't make any sense in the first place and cannot ever lead to a sensible\noutcome that honors the full request.\n\nThe conversion to batched transactions fixes the issue because we now try to\nqueue both updates in the same transaction. As such, the transaction itself\nwill notice this conflict and refuse the update altogether before we commit\nany of the values.\n\nNote that this requires changes to a couple of tests in t5408 that happened\nto exercise this behaviour. Given that the generated output is misleading\nand given that the user request cannot ever be fully honored this really\nfeels more like a bug than properly designed behaviour. As such, changing\nthe behaviour feels like the right thing to do.\n\nSince now reference updates are batched, the 'reference-transaction'\nhook will be invoked with all updates together. Currently git will 'die'\nwhen the hook returns with a non-zero exit status in the 'prepared'\nstage. For 'git-receive-pack(1)', this allowed users to reject an\nindividual reference update, git would have applied previous updates but\nimmediately abort further execution. This is definitely an incorrect\nusage of this hook, since the right place to do this would be the\n'update' hook. This patch retains the latter behavior, but\n'reference-transaction' hook now changes to a all-or-nothing behavior\nwhen a non-zero exit status is returned in the 'prepared' stage, since\nbatch updates use a transaction under the hood. This explains the change\nin 't1416'.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 88 ++++++++++++++++++++++++++++++++--------\n t/t1416-ref-transaction-hooks.sh | 2 -\n t/t5408-send-pack-stdin.sh | 12 +++---\n 3 files changed, 79 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex be314879e8..b4fceb3837 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,\n \tBUG_if_bug(\"connectivity check skipped???\");\n }\n \n+static void ref_transaction_rejection_handler(const char *refname,\n+\t\t\t\t\t const struct object_id *old_oid UNUSED,\n+\t\t\t\t\t const struct object_id *new_oid UNUSED,\n+\t\t\t\t\t const char *old_target UNUSED,\n+\t\t\t\t\t const char *new_target UNUSED,\n+\t\t\t\t\t enum ref_transaction_error err,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tstruct strmap *failed_refs = (struct strmap *)cb_data;\n+\tconst char *reason = \"\";\n+\n+\tswitch (err) {\n+\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n+\t\treason = \"refname conflict\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n+\t\treason = \"reference already exists\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n+\t\treason = \"reference does not exist\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n+\t\treason = \"incorrect old value provided\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n+\t\treason = \"invalid new value provided\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_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+\tstrmap_put(failed_refs, refname, xstrdup(reason));\n+}\n+\n static void execute_commands_non_atomic(struct command *commands,\n \t\t\t\t\tstruct shallow_info *si)\n {\n \tstruct command *cmd;\n \tstruct strbuf err = STRBUF_INIT;\n+\tconst char *reported_error = \"\";\n+\tstruct strmap failed_refs = STRMAP_INIT;\n+\n+\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\tif (!transaction) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\tstrbuf_reset(&err);\n+\t\treported_error = \"transaction failed to start\";\n+\t\tgoto failure;\n+\t}\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n \t\t\tcontinue;\n \n-\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t\t 0, &err);\n-\t\tif (!transaction) {\n-\t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_reset(&err);\n-\t\t\tcmd->error_string = \"transaction failed to start\";\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tcmd->error_string = update(cmd, si);\n+\t}\n \n-\t\tif (!cmd->error_string\n-\t\t && ref_transaction_commit(transaction, &err)) {\n-\t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_reset(&err);\n-\t\t\tcmd->error_string = \"failed to update ref\";\n-\t\t}\n-\t\tref_transaction_free(transaction);\n+\tif (ref_transaction_commit(transaction, &err)) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\treported_error = \"failed to update refs\";\n+\t\tgoto failure;\n \t}\n+\n+\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t &failed_refs);\n+\n+\tif (strmap_empty(&failed_refs))\n+\t\tgoto cleanup;\n+\n+failure:\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (strmap_empty(&failed_refs))\n+\t\t\tcmd->error_string = reported_error;\n+\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n+\t}\n+\n+cleanup:\n+\tref_transaction_free(transaction);\n+\tstrmap_clear(&failed_refs, 1);\n \tstrbuf_release(&err);\n }\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 8c777f7cf8..d91dd3a3b5 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -120,8 +120,6 @@ test_expect_success 'interleaving hook calls succeed' '\n \n \tcat >expect <<-EOF &&\n \t\thooks/update refs/tags/PRE $ZERO_OID $PRE_OID\n-\t\thooks/reference-transaction prepared\n-\t\thooks/reference-transaction committed\n \t\thooks/update refs/tags/POST $ZERO_OID $POST_OID\n \t\thooks/reference-transaction prepared\n \t\thooks/reference-transaction committed\ndiff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh\nindex 45fb20179b..76fb8bbc68 100755\n--- a/t/t5408-send-pack-stdin.sh\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -69,21 +69,23 @@ test_expect_success 'stdin mixed with cmdline' '\n \n test_expect_success 'cmdline refs written in order' '\n \tclear_remote &&\n-\ttest_must_fail git send-pack remote.git A:foo B:foo &&\n-\tverify_push A foo\n+\ttest_must_fail git send-pack remote.git A:foo B:foo 2>err &&\n+\ttest_grep \"multiple updates for ref ${SQ}refs/heads/foo${SQ} not allowed\" err &&\n+\ttest_must_fail git --git-dir=remote.git rev-parse foo\n '\n \n test_expect_success 'cmdline refs with multiple duplicates' '\n \tclear_remote &&\n-\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo &&\n-\tverify_push A foo\n+\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo 2>err &&\n+\ttest_grep \"multiple updates for ref ${SQ}refs/heads/foo${SQ} not allowed\" err &&\n+\ttest_must_fail git --git-dir=remote.git rev-parse foo\n '\n \n test_expect_success '--stdin refs come after cmdline' '\n \tclear_remote &&\n \techo A:foo >input &&\n \ttest_must_fail git send-pack remote.git --stdin B:foo remote_status` field. While the command allows duplicate ref\ninputs, the list of doesn't accommodate this behavior as a particular\nrefname is linked to a single `struct ref*` element. So if the user\ninputs a reference twice like:\n\n git send-pack remote.git A:foo B:foo\n\nwhere the user is trying to update the same reference 'foo' twice and\nthe reference fails to be updated, we first fill `ref->remote_status`\nwith error message for the input 'A:foo' then we override the same field\nwith the error message for 'B:foo'. This override happens without first\nfree'ing the previous value. Fix this leak.\n\nThe current tests already incorporate the above example, but in the test\n'A:foo' succeeds while 'B:foo' fails, meaning that the memory leak isn't\ntriggered. Add a new test with multiple duplicates.\n\nSigned-off-by: Karthik Nayak \n---\n send-pack.c | 7 +++++++\n t/t5408-send-pack-stdin.sh | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 5005689cb5..4cd41a64ce 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -260,6 +260,13 @@ static int receive_status(struct repository *r,\n \t\t\t\trefname);\n \t\t\tcontinue;\n \t\t}\n+\n+\t\t/*\n+\t\t * Clients sending duplicate refs can cause the same value\n+\t\t * to be overridden, causing a memory leak.\n+\t\t */\n+\t\tfree(hint->remote_status);\n+\n \t\tif (!strcmp(head, \"ng\")) {\n \t\t\thint->status = REF_STATUS_REMOTE_REJECT;\n \t\t\tif (p)\ndiff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh\nindex 526a675045..45fb20179b 100755\n--- a/t/t5408-send-pack-stdin.sh\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -73,6 +73,12 @@ test_expect_success 'cmdline refs written in order' '\n \tverify_push A foo\n '\n \n+test_expect_success 'cmdline refs with multiple duplicates' '\n+\tclear_remote &&\n+\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo &&\n+\tverify_push A foo\n+'\n+\n test_expect_success '--stdin refs come after cmdline' '\n \tclear_remote &&\n \techo A:foo >input &&\n\n-- \n2.49.0\n\n"},{"id":"518025","messageId":"aCSNDbUX-MMJZj5S@pks.im","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-3-7c65f46493d4@gmail.com","subject":"Re: [PATCH 3/3] receive-pack: use batched reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-14T12:31:09Z","receivedAt":"2025-05-14T12:31:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 14, 2025 at 11:03:49AM +0200, Karthik Nayak wrote:\n[snip]\n> With the reftable backend there is a 18x performance improvement, when\n> performing receive-pack with 10000 refs:\n> \n> Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n> Time (mean ± σ): 4.276 s ± 0.078 s [User: 0.796 s, System: 3.318 s]\n> Range (min … max): 4.185 s … 4.430 s 10 runs\n> \n> Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n> Time (mean ± σ): 235.4 ms ± 6.9 ms [User: 75.4 ms, System: 157.3 ms]\n> Range (min … max): 228.5 ms … 254.2 ms 11 runs\n> \n> Summary\n> receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n> 18.16 ± 0.63 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n> \n> In similar conditions, the files backend sees a 1.21x performance\n> improvement:\n> \n> Benchmark 1: receive: many refs (refformat = files, refcount = 10000, revision = master)\n> Time (mean ± σ): 1.121 s ± 0.021 s [User: 0.128 s, System: 0.975 s]\n> Range (min … max): 1.097 s … 1.156 s 10 runs\n> \n> Benchmark 2: receive: many refs (refformat = files, refcount = 10000, revision = HEAD)\n> Time (mean ± σ): 927.9 ms ± 22.6 ms [User: 99.0 ms, System: 815.2 ms]\n> Range (min … max): 903.1 ms … 978.0 ms 10 runs\n> \n> Summary\n> receive: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n> 1.21 ± 0.04 times faster than receive: many refs (refformat = files, refcount = 10000, revision = master)\n\nWe see almost the same speedups as we saw in git-fetch(1), and the\nreason why we see them is basically the same.\n\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index be314879e8..b4fceb3837 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,\n> \tBUG_if_bug(\"connectivity check skipped???\");\n> }\n> \n> +static void ref_transaction_rejection_handler(const char *refname,\n> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n> +\t\t\t\t\t const char *old_target UNUSED,\n> +\t\t\t\t\t const char *new_target UNUSED,\n> +\t\t\t\t\t enum ref_transaction_error err,\n> +\t\t\t\t\t void *cb_data)\n> +{\n> +\tstruct strmap *failed_refs = (struct strmap *)cb_data;\n\nThis cast is unnecessary.\n\n> +\tconst char *reason = \"\";\n> +\n> +\tswitch (err) {\n> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n> +\t\treason = \"refname conflict\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n> +\t\treason = \"reference already exists\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n> +\t\treason = \"reference does not exist\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n> +\t\treason = \"incorrect old value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n> +\t\treason = \"invalid new value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_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> +\tstrmap_put(failed_refs, refname, xstrdup(reason));\n> +}\n\nI'd have expected something like this for git-fetch(1), as well, so that\nwe don't silently swallow failed ref updates. Would it make sense to\nmaybe provide an array of reasons by enum so that we can reuse those\nmessages?\n\n> static void execute_commands_non_atomic(struct command *commands,\n> \t\t\t\t\tstruct shallow_info *si)\n> {\n> \tstruct command *cmd;\n> \tstruct strbuf err = STRBUF_INIT;\n> +\tconst char *reported_error = \"\";\n> +\tstruct strmap failed_refs = STRMAP_INIT;\n> +\n> +\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n> +\tif (!transaction) {\n> +\t\trp_error(\"%s\", err.buf);\n> +\t\tstrbuf_reset(&err);\n> +\t\treported_error = \"transaction failed to start\";\n> +\t\tgoto failure;\n> +\t}\n\nOkay. We now create a single transaction with failures being allowed.\n\n> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n> \t\t\tcontinue;\n> \n> -\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\t\t 0, &err);\n> -\t\tif (!transaction) {\n> -\t\t\trp_error(\"%s\", err.buf);\n> -\t\t\tstrbuf_reset(&err);\n> -\t\t\tcmd->error_string = \"transaction failed to start\";\n> -\t\t\tcontinue;\n> -\t\t}\n> -\n> \t\tcmd->error_string = update(cmd, si);\n> +\t}\n\nSo here we only need to queue each update.\n\n> -\t\tif (!cmd->error_string\n> -\t\t && ref_transaction_commit(transaction, &err)) {\n> -\t\t\trp_error(\"%s\", err.buf);\n> -\t\t\tstrbuf_reset(&err);\n> -\t\t\tcmd->error_string = \"failed to update ref\";\n> -\t\t}\n> -\t\tref_transaction_free(transaction);\n> +\tif (ref_transaction_commit(transaction, &err)) {\n> +\t\trp_error(\"%s\", err.buf);\n> +\t\treported_error = \"failed to update refs\";\n> +\t\tgoto failure;\n> \t}\n> +\n> +\tref_transaction_for_each_rejected_update(transaction,\n> +\t\t\t\t\t\t ref_transaction_rejection_handler,\n> +\t\t\t\t\t\t &failed_refs);\n> +\n> +\tif (strmap_empty(&failed_refs))\n> +\t\tgoto cleanup;\n> +\n> +failure:\n> +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> +\t\tif (strmap_empty(&failed_refs))\n> +\t\t\tcmd->error_string = reported_error;\n\nThe reported error may have one of there values now:\n\n - The empty string. Is it correct to set the error string to that\n value? Shouldn't it rather be a `NULL` pointer?\n\n - \"transaction failed to start\". It makes sense to update every\n command accordingly, as we wouldn't have updated any of them.\n\n - \"failed to update refs\", in case the commit failed. Does the commit\n fail only in cases where we couldn't update _any_ reference, or does\n it also retrun an error when only one of the updates failed? If the\n latter, we shouldn't update all the others to \"failed\", should we?\n\nIn any case, it feels weird that we use `strmap_empty()` to check for\nthis condition. I'd have expected that we rather check for\n`reported_error` to be a non-empty string directly to figure out whether\nthe transaction itself has failed as a whole. Like that, we'd know that\nwe only ever do this if we hit a `goto failure` branch.\n\nPatrick\n"},{"id":"518026","messageId":"aCSNFMeh3WMav_Rn@pks.im","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-1-7c65f46493d4@gmail.com","subject":"Re: [PATCH 1/3] fetch: use batched reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-14T12:31:16Z","receivedAt":"2025-05-14T12:31:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, May 14, 2025 at 11:03:47AM +0200, Karthik Nayak wrote:\n> The reference updates performed as a part of 'git-fetch(1)', take place\n\ns/,//\n\n> one at a time. For each reference update, a new transaction is created\n> and committed. This is necessary to ensure we can allow individual\n> updates to fail without failing the entire command. The command also\n> supports an '--atomic' mode, which uses a single transaction to update\n> all of the references. But this mode has an all-or-nothing approach,\n> where if a single update fails, all updates would fail.\n> \n> In 23fc8e4f61 (refs: implement batch reference update support,\n> 2025-04-08), we introduced a new mechanism to batch reference updates.\n> Under the hood, this uses a single transaction to perform a batch of\n> reference updates, while allowing only individual updates to fail.\n> Utilize this newly introduced batch update mechanism in 'git-fetch(1)'.\n> This provides a significant bump in performance, especially when dealing\n> with repositories with large number of references.\n> \n> Adding support for batched updates is simply modifying the flow to also\n> create a batch update transaction in the non-atomic flow.\n> \n> With the reftable backend there is a 22x performance improvement, when\n> performing 'git-fetch(1)' with 10000 refs:\n> \n> Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n> Time (mean ± σ): 3.403 s ± 0.775 s [User: 1.875 s, System: 1.417 s]\n> Range (min … max): 2.454 s … 4.529 s 10 runs\n> \n> Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n> Time (mean ± σ): 154.3 ms ± 17.6 ms [User: 102.5 ms, System: 56.1 ms]\n> Range (min … max): 145.2 ms … 220.5 ms 18 runs\n> \n> Summary\n> fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n> 22.06 ± 5.62 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n\nNice. The speedup is larger than I have originally anticipated, but I\ncertainly won't complain about that :) For a good part, the speedup\nshould result from us not having to do 10000 auto-compactions anymore\nfor each of the updates, as well as not having to write 10000 new\ntables. Instead, we only write a single new table and perform compaction\na single time, only.\n\n> In similar conditions, the files backend sees a 1.25x performance\n> improvement:\n> \n> Benchmark 1: fetch: many refs (refformat = files, refcount = 10000, revision = master)\n> Time (mean ± σ): 605.5 ms ± 9.4 ms [User: 117.8 ms, System: 483.3 ms]\n> Range (min … max): 595.6 ms … 621.5 ms 10 runs\n> \n> Benchmark 2: fetch: many refs (refformat = files, refcount = 10000, revision = HEAD)\n> Time (mean ± σ): 485.8 ms ± 4.3 ms [User: 91.1 ms, System: 396.7 ms]\n> Range (min … max): 477.6 ms … 494.3 ms 10 runs\n> \n> Summary\n> fetch: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n> 1.25 ± 0.02 times faster than fetch: many refs (refformat = files, refcount = 10000, revision = master)\n\nYup, this makes sense, as well. We lose a bunch of overhead by creating\nseparate transactions for each of the updates, but the slow path is\nstill that we have to create 10000 files for each of the references. So\nit's expected to see a small performance improvement, but nothing game\nchanging.\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 5279997c96..1558f6d1e8 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1688,6 +1644,32 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n> \treturn result;\n> }\n> \n> +struct ref_rejection_data {\n> +\tint *retcode;\n> +\tint conflict_msg_shown;\n> +\tconst char *remote_name;\n> +};\n> +\n> +static void ref_transaction_rejection_handler(const char *refname UNUSED,\n> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n> +\t\t\t\t\t const char *old_target UNUSED,\n> +\t\t\t\t\t const char *new_target UNUSED,\n> +\t\t\t\t\t enum ref_transaction_error err,\n> +\t\t\t\t\t void *cb_data)\n> +{\n> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n> +\n> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n> +\t\t\t\"branches\"), data->remote_name);\n> +\t\tdata->conflict_msg_shown = 1;\n> +\t}\n> +\n> +\t*data->retcode = 1;\n> +}\n> +\n> static int do_fetch(struct transport *transport,\n> \t\t struct refspec *rs,\n> \t\t const struct fetch_config *config)\n\nOkay, so we now handle errors over here. Is the handled error the only\nerror that we may see, or do we also accept other errors like D/F now?\nIf the latter, wouldn't it mean that we don't print any error messages\nfor those other failures at all? That might be quite confusing.\n\n> @@ -1808,6 +1790,20 @@ static int do_fetch(struct transport *transport,\n> \t\t\tretcode = 1;\n> \t}\n> \n> +\t/*\n> +\t * If not atomic, we can still use batched updates, which would be much\n> +\t * more performent. We don't initiate the transaction before pruning,\n\ns/performent/performant/\n\n> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n> +\t */\n> +\tif (!transaction) {\n> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n> +\t\tif (!transaction) {\n> +\t\t\tretcode = -1;\n> +\t\t\tgoto cleanup;\n> +\t\t}\n> +\t}\n> +\n> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n> \t\t\t\t &fetch_head, config)) {\n> \t\tretcode = 1;\n\nDon't transactions handle D/F conflicts for us? Isn't that the sole\nreason why for example `refs_verify_refname_available()` accepts an\n\"extras\" parameter that is supposed to contain refs that are about to be\ndeleted?\n\nPatrick\n"},{"id":"518058","messageId":"xmqqmsbf9jri.fsf@gitster.g","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-1-7c65f46493d4@gmail.com","subject":"Re: [PATCH 1/3] fetch: use batched reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T17:36:49Z","receivedAt":"2025-05-14T17:36:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> With this we'll either be using a regular transaction or a batch update\n> transaction. This helps cleanup some code which is no longer needed as\n> we'll now always have some type of 'ref_transaction' object being\n> propagated.\n\nGreat. From the above description, I imagined that the change\ninvolved would be removal of all \"update one by one\" code paths, and\naddition of a new line to set one bit in the transaction object that\nsays \"this is not the usual all-or-none transaction but is a\nbest-effort batch\", but it does not seem to lose as many lines as I\nhoped ;-)\n\nBut still, conceptually this change should simplify the things quite\na bit.\n"},{"id":"518060","messageId":"xmqqikm39jbp.fsf@gitster.g","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-2-7c65f46493d4@gmail.com","subject":"Re: [PATCH 2/3] send-pack: fix memory leak around duplicate refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T17:46:18Z","receivedAt":"2025-05-14T17:46:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> The 'git-send-pack(1)' allows users to push objects to a remote\n> repository and explicitly list the references to be pushed. The status\n> of each reference pushed is captured into a list mapped by refname.\n>\n> If a reference fails to be updated, its error message is captured in the\n> `ref->remote_status` field. While the command allows duplicate ref\n> inputs, the list of doesn't accommodate this behavior as a particular\n\n-ECANNOTPARSE around \"the list of doesn't accommodate\".\n\n> refname is linked to a single `struct ref*` element. So if the user\n> inputs a reference twice like:\n>\n> git send-pack remote.git A:foo B:foo\n>\n> where the user is trying to update the same reference 'foo' twice and\n> the reference fails to be updated, we first fill `ref->remote_status`\n> with error message for the input 'A:foo' then we override the same field\n> with the error message for 'B:foo'. This override happens without first\n> free'ing the previous value. Fix this leak.\n\nOK. A natural question is what happens when A successfully updates\nand B fails, or A fails but B successfully updates (failing both is\nmuch less interesting). What should happen in such cases is unclear\nbut my gut feeling is that the last-one wins (which you implemented)\nis probably just as OK as the first-one gets retained (which might\nbe less work at runtime), and perhaps keeping-the-more-severe-one\nmight be more useful than either of these two, but I didn't think\nit through.\n\nTHanks.\n\n"},{"id":"518065","messageId":"xmqqecwr9fvm.fsf@gitster.g","threadId":"63455","inReplyTo":"aCSNDbUX-MMJZj5S@pks.im","subject":"Re: [PATCH 3/3] receive-pack: use batched reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-14T19:00:45Z","receivedAt":"2025-05-14T19:00:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n>> +\tswitch (err) {\n>> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n>> +\t\treason = \"refname conflict\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n>> +\t\treason = \"reference already exists\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n>> +\t\treason = \"reference does not exist\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n>> +\t\treason = \"incorrect old value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n>> +\t\treason = \"invalid new value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_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>> +\tstrmap_put(failed_refs, refname, xstrdup(reason));\n>> +}\n>\n> I'd have expected something like this for git-fetch(1), as well, so that\n> we don't silently swallow failed ref updates. Would it make sense to\n> maybe provide an array of reasons by enum so that we can reuse those\n> messages?\n\nExcellent. It does not have to be an array, but a helper function\nthat takes an enum and returns a \"const char *\".\n"},{"id":"518098","messageId":"CAOLa=ZRbEqs6X1KJj-CikCANX-BC8r4RqbwoT06qmtF=x+7hQA@mail.gmail.com","threadId":"63455","inReplyTo":"aCSNFMeh3WMav_Rn@pks.im","subject":"Re: [PATCH 1/3] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T11:13:32Z","receivedAt":"2025-05-15T11:13:33Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n> Yup, this makes sense, as well. We lose a bunch of overhead by creating\n> separate transactions for each of the updates, but the slow path is\n> still that we have to create 10000 files for each of the references. So\n> it's expected to see a small performance improvement, but nothing game\n> changing.\n>\n\nExactly, but it is still a good bump in speed. Which is always welcome\n\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 5279997c96..1558f6d1e8 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1688,6 +1644,32 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n>> \treturn result;\n>> }\n>>\n>> +struct ref_rejection_data {\n>> +\tint *retcode;\n>> +\tint conflict_msg_shown;\n>> +\tconst char *remote_name;\n>> +};\n>> +\n>> +static void ref_transaction_rejection_handler(const char *refname UNUSED,\n>> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n>> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n>> +\t\t\t\t\t const char *old_target UNUSED,\n>> +\t\t\t\t\t const char *new_target UNUSED,\n>> +\t\t\t\t\t enum ref_transaction_error err,\n>> +\t\t\t\t\t void *cb_data)\n>> +{\n>> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n>> +\n>> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n>> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n>> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n>> +\t\t\t\"branches\"), data->remote_name);\n>> +\t\tdata->conflict_msg_shown = 1;\n>> +\t}\n>> +\n>> +\t*data->retcode = 1;\n>> +}\n>> +\n>> static int do_fetch(struct transport *transport,\n>> \t\t struct refspec *rs,\n>> \t\t const struct fetch_config *config)\n>\n> Okay, so we now handle errors over here. Is the handled error the only\n> error that we may see, or do we also accept other errors like D/F now?\n> If the latter, wouldn't it mean that we don't print any error messages\n> for those other failures at all? That might be quite confusing.\n>\n\nI was mostly trying to replicate the current behavior, which is\n- For F/D conflicts print this error message.\n- For any other error, simply propagate the error code.\n\nI do think there is merit in changing this, and since you're pointing\nit out too, I think we should make this change. I've modified this to\nnow print a better message for other errors.\n\n>> @@ -1808,6 +1790,20 @@ static int do_fetch(struct transport *transport,\n>> \t\t\tretcode = 1;\n>> \t}\n>>\n>> +\t/*\n>> +\t * If not atomic, we can still use batched updates, which would be much\n>> +\t * more performent. We don't initiate the transaction before pruning,\n>\n> s/performent/performant/\n>\n\nAh! Thanks.\n\n>> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n>> +\t */\n>> +\tif (!transaction) {\n>> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> +\t\tif (!transaction) {\n>> +\t\t\tretcode = -1;\n>> +\t\t\tgoto cleanup;\n>> +\t\t}\n>> +\t}\n>> +\n>> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n>> \t\t\t\t &fetch_head, config)) {\n>> \t\tretcode = 1;\n>\n> Don't transactions handle D/F conflicts for us? Isn't that the sole\n> reason why for example `refs_verify_refname_available()` accepts an\n> \"extras\" parameter that is supposed to contain refs that are about to be\n> deleted?\n>\n\nMy understanding was a little different, from the documentation for the\nfunction:\n\n If extras is non-NULL, it is a list of additional refnames with which\n refname is not allowed to conflict.\n\nThis is to capture additional conflicts. We want a way to avoid said\nconflicts. That said, there is a 'skip' parameter which does exactly\nwhat you're saying. But the transaction logic doesn't incorporate this\nentirely. Specifically in the files backend, where we create a lock in\nthe filesystem, this would cause a conflict, consider the following:\n\n ❯ eza --tree .git/refs/remotes/\n .git/refs/remotes\n └── origin\n ├── dir\n │ └── file.lock\n ├── dir.lock\n └── HEAD\n\nThis is from the test 'branchname D/F conflict resolved by --prune', the\ntest prunes the existing reference 'refs/remotes/origin/dir/file' while\nadding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and\nread the reference, but this causes an issue since\n'refs/remotes/origin/dir' exists as a directory already.\n\nI would say this is logically solvable if we start treating conflict\nresolution within updates as a first class problem. But perhaps that's\nsomething of a patch series in itself and better solved outside of this?\n\n> Patrick\n"},{"id":"518099","messageId":"CAOLa=ZRA7ax5buMO-3MewzH7ZH7jddDYAVw8abrEkGYDvd6Vvg@mail.gmail.com","threadId":"63455","inReplyTo":"xmqqikm39jbp.fsf@gitster.g","subject":"Re: [PATCH 2/3] send-pack: fix memory leak around duplicate refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T11:23:20Z","receivedAt":"2025-05-15T11:23:22Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n> Karthik Nayak writes:\n>\n>> The 'git-send-pack(1)' allows users to push objects to a remote\n>> repository and explicitly list the references to be pushed. The status\n>> of each reference pushed is captured into a list mapped by refname.\n>>\n>> If a reference fails to be updated, its error message is captured in the\n>> `ref->remote_status` field. While the command allows duplicate ref\n>> inputs, the list of doesn't accommodate this behavior as a particular\n>\n> -ECANNOTPARSE around \"the list of doesn't accommodate\".\n>\n\nIndeed, a s/of// should make this readable.\n\n>> refname is linked to a single `struct ref*` element. So if the user\n>> inputs a reference twice like:\n>>\n>> git send-pack remote.git A:foo B:foo\n>>\n>> where the user is trying to update the same reference 'foo' twice and\n>> the reference fails to be updated, we first fill `ref->remote_status`\n>> with error message for the input 'A:foo' then we override the same field\n>> with the error message for 'B:foo'. This override happens without first\n>> free'ing the previous value. Fix this leak.\n>\n> OK. A natural question is what happens when A successfully updates\n> and B fails, or A fails but B successfully updates (failing both is\n> much less interesting). What should happen in such cases is unclear\n> but my gut feeling is that the last-one wins (which you implemented)\n> is probably just as OK as the first-one gets retained (which might\n> be less work at runtime), and perhaps keeping-the-more-severe-one\n> might be more useful than either of these two, but I didn't think\n> it through.\n>\n\nI think the whole thing reeks a little bit. We shouldn't allow such\nduplicates in the first place. This is something we tackle in the next\npatch, where any such duplicates fails all updates. This is a lot more\ndeterministic and also the error reporting to the user is inline with\nwhat is actually done.\n\nAs of this patch, if there are duplicates, one success one failure,\nmeans that the ref is updated once. But we report to the user that both\nthe refs failed to be updated as:\n\n $ git send-pack remote.git A:foo B:foo\n Enumerating objects: 3, done.\n Counting objects: 100% (3/3), done.\n Delta compression using up to 20 threads\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 226 bytes | 226.00 KiB/s, done.\n Total 3 (delta 1), reused 0 (delta 0), pack-reused 0 (from 0)\n remote: error: cannot lock ref 'refs/heads/foo': reference already exists\n To remote.git\n ! [remote rejected] A -> foo (failed to update ref)\n ! [remote failure] B -> foo (remote failed to report status)\n\nThis is totally wrong, since we actually do update one of the refs.\n\n> THanks.\n"},{"id":"518100","messageId":"CAOLa=ZSDvAqiebkHfSV_Z6dNiBzrPU-inxMnFP7-Kzs0VDXOTg@mail.gmail.com","threadId":"63455","inReplyTo":"aCSNDbUX-MMJZj5S@pks.im","subject":"Re: [PATCH 3/3] receive-pack: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T11:30:57Z","receivedAt":"2025-05-15T11:30:59Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n[snip]\n\n>> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n>> index be314879e8..b4fceb3837 100644\n>> --- a/builtin/receive-pack.c\n>> +++ b/builtin/receive-pack.c\n>> @@ -1843,35 +1843,91 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,\n>> \tBUG_if_bug(\"connectivity check skipped???\");\n>> }\n>>\n>> +static void ref_transaction_rejection_handler(const char *refname,\n>> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n>> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n>> +\t\t\t\t\t const char *old_target UNUSED,\n>> +\t\t\t\t\t const char *new_target UNUSED,\n>> +\t\t\t\t\t enum ref_transaction_error err,\n>> +\t\t\t\t\t void *cb_data)\n>> +{\n>> +\tstruct strmap *failed_refs = (struct strmap *)cb_data;\n>\n> This cast is unnecessary.\n>\n\nYes, will remove.\n\n>> +\tconst char *reason = \"\";\n>> +\n>> +\tswitch (err) {\n>> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n>> +\t\treason = \"refname conflict\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n>> +\t\treason = \"reference already exists\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n>> +\t\treason = \"reference does not exist\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n>> +\t\treason = \"incorrect old value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n>> +\t\treason = \"invalid new value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_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>> +\tstrmap_put(failed_refs, refname, xstrdup(reason));\n>> +}\n>\n> I'd have expected something like this for git-fetch(1), as well, so that\n> we don't silently swallow failed ref updates. Would it make sense to\n> maybe provide an array of reasons by enum so that we can reuse those\n> messages?\n>\n\nI think you've convinced me of this, As Junio also mentions, I've now\nadded a function to provide a mapping from 'enum -> char *'.\n\n>> static void execute_commands_non_atomic(struct command *commands,\n>> \t\t\t\t\tstruct shallow_info *si)\n>> {\n>> \tstruct command *cmd;\n>> \tstruct strbuf err = STRBUF_INIT;\n>> +\tconst char *reported_error = \"\";\n>> +\tstruct strmap failed_refs = STRMAP_INIT;\n>> +\n>> +\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> +\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> +\tif (!transaction) {\n>> +\t\trp_error(\"%s\", err.buf);\n>> +\t\tstrbuf_reset(&err);\n>> +\t\treported_error = \"transaction failed to start\";\n>> +\t\tgoto failure;\n>> +\t}\n>\n> Okay. We now create a single transaction with failures being allowed.\n>\n>> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n>> \t\t\tcontinue;\n>>\n>> -\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> -\t\t\t\t\t\t\t 0, &err);\n>> -\t\tif (!transaction) {\n>> -\t\t\trp_error(\"%s\", err.buf);\n>> -\t\t\tstrbuf_reset(&err);\n>> -\t\t\tcmd->error_string = \"transaction failed to start\";\n>> -\t\t\tcontinue;\n>> -\t\t}\n>> -\n>> \t\tcmd->error_string = update(cmd, si);\n>> +\t}\n>\n> So here we only need to queue each update.\n>\n>> -\t\tif (!cmd->error_string\n>> -\t\t && ref_transaction_commit(transaction, &err)) {\n>> -\t\t\trp_error(\"%s\", err.buf);\n>> -\t\t\tstrbuf_reset(&err);\n>> -\t\t\tcmd->error_string = \"failed to update ref\";\n>> -\t\t}\n>> -\t\tref_transaction_free(transaction);\n>> +\tif (ref_transaction_commit(transaction, &err)) {\n>> +\t\trp_error(\"%s\", err.buf);\n>> +\t\treported_error = \"failed to update refs\";\n>> +\t\tgoto failure;\n>> \t}\n>> +\n>> +\tref_transaction_for_each_rejected_update(transaction,\n>> +\t\t\t\t\t\t ref_transaction_rejection_handler,\n>> +\t\t\t\t\t\t &failed_refs);\n>> +\n>> +\tif (strmap_empty(&failed_refs))\n>> +\t\tgoto cleanup;\n>> +\n>> +failure:\n>> +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> +\t\tif (strmap_empty(&failed_refs))\n>> +\t\t\tcmd->error_string = reported_error;\n>\n> The reported error may have one of there values now:\n>\n> - The empty string. Is it correct to set the error string to that\n> value? Shouldn't it rather be a `NULL` pointer?\n>\n> - \"transaction failed to start\". It makes sense to update every\n> command accordingly, as we wouldn't have updated any of them.\n>\n> - \"failed to update refs\", in case the commit failed. Does the commit\n> fail only in cases where we couldn't update _any_ reference, or does\n> it also retrun an error when only one of the updates failed? If the\n> latter, we shouldn't update all the others to \"failed\", should we?\n>\n> In any case, it feels weird that we use `strmap_empty()` to check for\n> this condition. I'd have expected that we rather check for\n> `reported_error` to be a non-empty string directly to figure out whether\n> the transaction itself has failed as a whole. Like that, we'd know that\n> we only ever do this if we hit a `goto failure` branch.\n>\n\nThis is silly mistake on my side. This is exactly what I wanted to do,\nbut instead of:\n\n const char *reported_error = NULL;\n\nI did:\n\n const char *reported_error;\n\nWhich obviously raised:\n\n In function ‘execute_commands_non_atomic’,\n inlined from ‘execute_commands’ at ../builtin/receive-pack.c:2059:3,\n inlined from ‘cmd_receive_pack’ at ../builtin/receive-pack.c:2636:3:\n ../builtin/receive-pack.c:1899:43: warning: ‘reported_error’ may be\nused uninitialized [-Wmaybe-uninitialized]\n 1899 | cmd->error_string = reported_error;\n | ~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~\n ../builtin/receive-pack.c: In function ‘cmd_receive_pack’:\n ../builtin/receive-pack.c:1864:21: note: ‘reported_error’ was declared here\n 1864 | const char *reported_error;\n | ^~~~~~~~~~~~~~\n [31/31] Linking target git-upload-archive\n\nprompting me to do this. Let me fix this. Thanks for pointing it out :)\n\n> Patrick\n"},{"id":"518101","messageId":"aCXQcSsc4p6u9ljZ@pks.im","threadId":"63455","inReplyTo":"CAOLa=ZRbEqs6X1KJj-CikCANX-BC8r4RqbwoT06qmtF=x+7hQA@mail.gmail.com","subject":"Re: [PATCH 1/3] fetch: use batched reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-15T11:30:57Z","receivedAt":"2025-05-15T11:31:02Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> >> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n> >> +\t */\n> >> +\tif (!transaction) {\n> >> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> >> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n> >> +\t\tif (!transaction) {\n> >> +\t\t\tretcode = -1;\n> >> +\t\t\tgoto cleanup;\n> >> +\t\t}\n> >> +\t}\n> >> +\n> >> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n> >> \t\t\t\t &fetch_head, config)) {\n> >> \t\tretcode = 1;\n> >\n> > Don't transactions handle D/F conflicts for us? Isn't that the sole\n> > reason why for example `refs_verify_refname_available()` accepts an\n> > \"extras\" parameter that is supposed to contain refs that are about to be\n> > deleted?\n> >\n> \n> My understanding was a little different, from the documentation for the\n> function:\n> \n> If extras is non-NULL, it is a list of additional refnames with which\n> refname is not allowed to conflict.\n> \n> This is to capture additional conflicts. We want a way to avoid said\n> conflicts. That said, there is a 'skip' parameter which does exactly\n> what you're saying.\n\nOh, right, my mistake -- that's what I actually meant.\n\n> But the transaction logic doesn't incorporate this\n> entirely. Specifically in the files backend, where we create a lock in\n> the filesystem, this would cause a conflict, consider the following:\n> \n> ❯ eza --tree .git/refs/remotes/\n> .git/refs/remotes\n> └── origin\n> ├── dir\n> │ └── file.lock\n> ├── dir.lock\n> └── HEAD\n> \n> This is from the test 'branchname D/F conflict resolved by --prune', the\n> test prunes the existing reference 'refs/remotes/origin/dir/file' while\n> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and\n> read the reference, but this causes an issue since\n> 'refs/remotes/origin/dir' exists as a directory already.\n> \n> I would say this is logically solvable if we start treating conflict\n> resolution within updates as a first class problem. But perhaps that's\n> something of a patch series in itself and better solved outside of this?\n\nFair enough, makes sense. Might be worth it to add a TODO comment there\nthen?\n\nPatrick\n"},{"id":"518103","messageId":"CAOLa=ZRbrAwk_+P3Y1dXxpTw7Yuxbge6O53SoD-Y6KXsNW+7UA@mail.gmail.com","threadId":"63455","inReplyTo":"aCXQcSsc4p6u9ljZ@pks.im","subject":"Re: [PATCH 1/3] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T11:36:35Z","receivedAt":"2025-05-15T11:36:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>> >> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n>> >> +\t */\n>> >> +\tif (!transaction) {\n>> >> +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n>> >> +\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n>> >> +\t\tif (!transaction) {\n>> >> +\t\t\tretcode = -1;\n>> >> +\t\t\tgoto cleanup;\n>> >> +\t\t}\n>> >> +\t}\n>> >> +\n>> >> \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n>> >> \t\t\t\t &fetch_head, config)) {\n>> >> \t\tretcode = 1;\n>> >\n>> > Don't transactions handle D/F conflicts for us? Isn't that the sole\n>> > reason why for example `refs_verify_refname_available()` accepts an\n>> > \"extras\" parameter that is supposed to contain refs that are about to be\n>> > deleted?\n>> >\n>>\n>> My understanding was a little different, from the documentation for the\n>> function:\n>>\n>> If extras is non-NULL, it is a list of additional refnames with which\n>> refname is not allowed to conflict.\n>>\n>> This is to capture additional conflicts. We want a way to avoid said\n>> conflicts. That said, there is a 'skip' parameter which does exactly\n>> what you're saying.\n>\n> Oh, right, my mistake -- that's what I actually meant.\n>\n>> But the transaction logic doesn't incorporate this\n>> entirely. Specifically in the files backend, where we create a lock in\n>> the filesystem, this would cause a conflict, consider the following:\n>>\n>> ❯ eza --tree .git/refs/remotes/\n>> .git/refs/remotes\n>> └── origin\n>> ├── dir\n>> │ └── file.lock\n>> ├── dir.lock\n>> └── HEAD\n>>\n>> This is from the test 'branchname D/F conflict resolved by --prune', the\n>> test prunes the existing reference 'refs/remotes/origin/dir/file' while\n>> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and\n>> read the reference, but this causes an issue since\n>> 'refs/remotes/origin/dir' exists as a directory already.\n>>\n>> I would say this is logically solvable if we start treating conflict\n>> resolution within updates as a first class problem. But perhaps that's\n>> something of a patch series in itself and better solved outside of this?\n>\n> Fair enough, makes sense. Might be worth it to add a TODO comment there\n> then?\n>\n\nYeah totally agree, will add in a TODO here.\n\n> Patrick\n"},{"id":"518140","messageId":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com","threadId":"63455","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com","subject":"[PATCH v2 0/4] fetch/receive: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T14:07:24Z","receivedAt":"2025-05-15T14:07:30Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The git-fetch(1) and git-receive-pack(1) commands update references as\npart of the flow. Each reference update is treated as a single entity\nand a transaction is created for each update.\n\nThis can be really slow, specifically in reference backends where there\nare optimizations which ensure a single transaction with 'N' reference\nupdate perform much faster than 'N' individual transactions. Also having\n'N' individual transactions has buildup and teardown costs. These costs\nadd up in repositories with a large number of references.\n\nAlso specifically in the reftable backend, 'N' individual transactions\nwould also trigger auto-compaction for every transaction.\n\nThe reasoning for using individual transactions is because we want to\nallow partial updates of references in these commands. Using a single\ntransaction would be an all-or-nothing scenario.\n\nRecently we introduced an in-between solution called batched reference\nupdates in 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08). This allows us to batch a set of reference updates, where\nindividual updates can pass/fail without affecting the batch.\n\nThis patch series, modifies both 'git-fetch(1)' and\n'git-receive-pack(1)' to use this mechanism. With this, we see a\nsignificant performance boost:\n\n+---------------------+---------------+------------------+\n| | files backend | reftable backend |\n+---------------------+---------------+------------------+\n| git-fetch(1) | 1.25x | 22x |\n| git-receive-pack(1) | 1.21x | 18x |\n+---------------------+---------------+------------------+\n\nThe first and third patch handle the changes for 'git-fetch(1)' and\n'git-receive-pack(1)' respectively. The second patch fixes a small\nmemory leak I encountered while working on this series.\n\nThis is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of\nhttps://github.com/j6t/gitk, 2025-05-09). There were no conflicts\nobserved with next or seen.\n\nJunio, since the release window for 2.50 is closing in. I would prefer\nwe mark this for 2.51, so perhaps when/if we merge it to 'next', we let\nit bake there till the next release window opens. While all the tests\npass, any bugs here would be high impact and it would be nice to catch\nit before it hits a release.\n\n---\nChanges in v2:\n- Added a new commit to introduce a mapping from 'ref_transaction_error'\n to a human readable string. We now use this to map the errors\n observed.\n- Add a TODO regarding handling pruning and creation of refs in the same\n transaction in 'git-fetch(1)'.\n- Cleanup commit message typos.\n- Fix logic in 'builtin/receive-pack.c' around error reporting to be\n cleaner. Thanks Patrick for the suggestion.\n- Link to v1: https://lore.kernel.org/r/20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com\n\n---\n builtin/fetch.c | 128 ++++++++++++++++++++++-----------------\n builtin/receive-pack.c | 64 +++++++++++++++-----\n builtin/update-ref.c | 26 +-------\n refs.c | 30 +++++++++\n refs.h | 5 ++\n send-pack.c | 7 +++\n t/t1416-ref-transaction-hooks.sh | 2 -\n t/t5408-send-pack-stdin.sh | 14 ++++-\n 8 files changed, 177 insertions(+), 99 deletions(-)\n\nKarthik Nayak (4):\n refs: add function to translate errors to strings\n fetch: use batched reference updates\n send-pack: fix memory leak around duplicate refs\n receive-pack: use batched reference updates\n\nRange-diff versus v1:\n\n-: ---------- > 1: 8c2b3d08d0 refs: add function to translate errors to strings\n1: b6a257eeff ! 2: 1683b19f10 fetch: use batched reference updates\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n -\t\tret = STORE_REF_ERROR_OTHER;\n -\t\tgoto out;\n -\t}\n - \n +-\n -\tif (our_transaction) {\n -\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n -\t\tcase 0:\n @@ builtin/fetch.c: static int s_update_ref(const char *action,\n -\t\t\tgoto out;\n -\t\t}\n -\t}\n --\n + \n -out:\n -\tref_transaction_free(our_transaction);\n \tif (ret)\n @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot\n +\tconst char *remote_name;\n +};\n +\n -+static void ref_transaction_rejection_handler(const char *refname UNUSED,\n ++static void ref_transaction_rejection_handler(const char *refname,\n +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n +\t\t\t\t\t const char *old_target UNUSED,\n @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot\n +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n +\t\t\t\"branches\"), data->remote_name);\n +\t\tdata->conflict_msg_shown = 1;\n ++\t} else {\n ++\t\tchar *reason = ref_transaction_error_msg(err);\n ++\n ++\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n ++\t\tfree(reason);\n +\t}\n +\n +\t*data->retcode = 1;\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n \n +\t/*\n +\t * If not atomic, we can still use batched updates, which would be much\n -+\t * more performent. We don't initiate the transaction before pruning,\n ++\t * more performant. We don't initiate the transaction before pruning,\n +\t * since pruning must be an independent step, to avoid F/D conflicts.\n ++\t *\n ++\t * TODO: if reference transactions gain logical conflict resolution, we\n ++\t * can delete and create refs (with F/D conflicts) in the same transaction\n ++\t * and this can be moved about the 'prune_refs()' block.\n +\t */\n +\tif (!transaction) {\n +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n -\t\t\tgoto cleanup;\n +\tif (retcode)\n +\t\tgoto cleanup;\n -+\n + \n +-\t\tretcode = ref_transaction_commit(transaction, &err);\n +\tretcode = ref_transaction_commit(transaction, &err);\n +\tif (retcode) {\n +\t\t/*\n @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n +\t\ttransaction = NULL;\n +\t\tgoto cleanup;\n +\t}\n - \n --\t\tretcode = ref_transaction_commit(transaction, &err);\n ++\n +\tif (!atomic_fetch) {\n +\t\tstruct ref_rejection_data data = {\n +\t\t\t.retcode = &retcode,\n2: 1697d15527 ! 3: 85f68863d8 send-pack: fix memory leak around duplicate refs\n @@ Commit message\n \n If a reference fails to be updated, its error message is captured in the\n `ref->remote_status` field. While the command allows duplicate ref\n - inputs, the list of doesn't accommodate this behavior as a particular\n + inputs, the list doesn't accommodate this behavior as a particular\n refname is linked to a single `struct ref*` element. So if the user\n inputs a reference twice like:\n \n3: 0579a66c98 ! 4: 1cd2324045 receive-pack: use batched reference updates\n @@ Commit message\n batch updates use a transaction under the hood. This explains the change\n in 't1416'.\n \n + Helped-by: Patrick Steinhardt \n Signed-off-by: Karthik Nayak \n \n ## builtin/receive-pack.c ##\n @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\n +\t\t\t\t\t enum ref_transaction_error err,\n +\t\t\t\t\t void *cb_data)\n +{\n -+\tstruct strmap *failed_refs = (struct strmap *)cb_data;\n -+\tconst char *reason = \"\";\n ++\tstruct strmap *failed_refs = cb_data;\n +\n -+\tswitch (err) {\n -+\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n -+\t\treason = \"refname conflict\";\n -+\t\tbreak;\n -+\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n -+\t\treason = \"reference already exists\";\n -+\t\tbreak;\n -+\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n -+\t\treason = \"reference does not exist\";\n -+\t\tbreak;\n -+\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n -+\t\treason = \"incorrect old value provided\";\n -+\t\tbreak;\n -+\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n -+\t\treason = \"invalid new value provided\";\n -+\t\tbreak;\n -+\tcase REF_TRANSACTION_ERROR_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 -+\tstrmap_put(failed_refs, refname, xstrdup(reason));\n ++\tstrmap_put(failed_refs, refname, ref_transaction_error_msg(err));\n +}\n +\n static void execute_commands_non_atomic(struct command *commands,\n @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\n {\n \tstruct command *cmd;\n \tstruct strbuf err = STRBUF_INIT;\n -+\tconst char *reported_error = \"\";\n ++\tconst char *reported_error = NULL;\n +\tstruct strmap failed_refs = STRMAP_INIT;\n +\n +\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\n +\t\trp_error(\"%s\", err.buf);\n +\t\treported_error = \"failed to update refs\";\n +\t\tgoto failure;\n - \t}\n ++\t}\n +\n +\tref_transaction_for_each_rejected_update(transaction,\n +\t\t\t\t\t\t ref_transaction_rejection_handler,\n @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\n +\n +failure:\n +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n -+\t\tif (strmap_empty(&failed_refs))\n ++\t\tif (reported_error)\n +\t\t\tcmd->error_string = reported_error;\n +\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n +\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n -+\t}\n + \t}\n +\n +cleanup:\n +\tref_transaction_free(transaction);\n\n\nbase-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0\nchange-id: 20250423-501-update-git-fetch-1-to-use-partial-transactions-7fb66339fe79\n\nThanks\n- Karthik\n\n"},{"id":"518141","messageId":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-1-80cbaaa55d2e@gmail.com","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com","subject":"[PATCH v2 1/4] refs: add function to translate errors to strings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T14:07:25Z","receivedAt":"2025-05-15T14:07:31Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The commit 76e760b999 (refs: introduce enum-based transaction error\ntypes, 2025-04-08) introduced enum-based transaction error types. The\nrefs transaction logic was also modified to propagate these errors. For\nclients of the ref transaction system, it would be beneficial to provide\nhuman readable messages for these errors.\n\nThere is already an existing mapping in 'builtin/update-ref.c', move it\nto 'refs.c' as `ref_transaction_error_msg()` and use the same within the\n'builtin/update-ref.c'.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/update-ref.c | 26 ++------------------------\n refs.c | 30 ++++++++++++++++++++++++++++++\n refs.h | 5 +++++\n 3 files changed, 37 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2b1e336ba1..09b99143bf 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -575,30 +575,7 @@ static void print_rejected_refs(const char *refname,\n \t\t\t\tvoid *cb_data UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *reason = \"\";\n-\n-\tswitch (err) {\n-\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n-\t\treason = \"refname conflict\";\n-\t\tbreak;\n-\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n-\t\treason = \"reference already exists\";\n-\t\tbreak;\n-\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n-\t\treason = \"reference does not exist\";\n-\t\tbreak;\n-\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n-\t\treason = \"incorrect old value provided\";\n-\t\tbreak;\n-\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n-\t\treason = \"invalid new value provided\";\n-\t\tbreak;\n-\tcase REF_TRANSACTION_ERROR_EXPECTED_SYMREF:\n-\t\treason = \"expected symref but found regular ref\";\n-\t\tbreak;\n-\tdefault:\n-\t\treason = \"unkown failure\";\n-\t}\n+\tchar *reason = ref_transaction_error_msg(err);\n \n \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n \t\t new_oid ? oid_to_hex(new_oid) : new_target,\n@@ -606,6 +583,7 @@ static void print_rejected_refs(const char *refname,\n \t\t reason);\n \n \tfwrite(sb.buf, sb.len, 1, stdout);\n+\tfree(reason);\n \tstrbuf_release(&sb);\n }\n \ndiff --git a/refs.c b/refs.c\nindex 6559db3789..4548fd8fcf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3314,3 +3314,33 @@ int ref_update_expects_existing_old_ref(struct ref_update *update)\n \treturn (update->flags & REF_HAVE_OLD) &&\n \t\t(!is_null_oid(&update->old_oid) || update->old_target);\n }\n+\n+char *ref_transaction_error_msg(enum ref_transaction_error err)\n+{\n+\tconst char *reason = \"\";\n+\n+\tswitch (err) {\n+\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n+\t\treason = \"refname conflict\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n+\t\treason = \"reference already exists\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n+\t\treason = \"reference does not exist\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n+\t\treason = \"incorrect old value provided\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n+\t\treason = \"invalid new value provided\";\n+\t\tbreak;\n+\tcase REF_TRANSACTION_ERROR_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+\treturn xstrdup(reason);\n+}\ndiff --git a/refs.h b/refs.h\nindex 46a6008e07..a0b2e3c43d 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -907,6 +907,11 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio\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+ * Translate errors to human readable error messages.\n+ */\n+char *ref_transaction_error_msg(enum ref_transaction_error err);\n+\n /*\n * Free `*transaction` and all associated data.\n */\n\n-- \n2.49.0\n\n"},{"id":"518142","messageId":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-2-80cbaaa55d2e@gmail.com","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com","subject":"[PATCH v2 2/4] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T14:07:26Z","receivedAt":"2025-05-15T14:07:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference updates performed as a part of 'git-fetch(1)', take place\none at a time. For each reference update, a new transaction is created\nand committed. This is necessary to ensure we can allow individual\nupdates to fail without failing the entire command. The command also\nsupports an '--atomic' mode, which uses a single transaction to update\nall of the references. But this mode has an all-or-nothing approach,\nwhere if a single update fails, all updates would fail.\n\nIn 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08), we introduced a new mechanism to batch reference updates.\nUnder the hood, this uses a single transaction to perform a batch of\nreference updates, while allowing only individual updates to fail.\nUtilize this newly introduced batch update mechanism in 'git-fetch(1)'.\nThis provides a significant bump in performance, especially when dealing\nwith repositories with large number of references.\n\nAdding support for batched updates is simply modifying the flow to also\ncreate a batch update transaction in the non-atomic flow.\n\nWith the reftable backend there is a 22x performance improvement, when\nperforming 'git-fetch(1)' with 10000 refs:\n\n Benchmark 1: fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n Time (mean ± σ): 3.403 s ± 0.775 s [User: 1.875 s, System: 1.417 s]\n Range (min … max): 2.454 s … 4.529 s 10 runs\n\n Benchmark 2: fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 154.3 ms ± 17.6 ms [User: 102.5 ms, System: 56.1 ms]\n Range (min … max): 145.2 ms … 220.5 ms 18 runs\n\n Summary\n fetch: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n 22.06 ± 5.62 times faster than fetch: many refs (refformat = reftable, refcount = 10000, revision = master)\n\nIn similar conditions, the files backend sees a 1.25x performance\nimprovement:\n\n Benchmark 1: fetch: many refs (refformat = files, refcount = 10000, revision = master)\n Time (mean ± σ): 605.5 ms ± 9.4 ms [User: 117.8 ms, System: 483.3 ms]\n Range (min … max): 595.6 ms … 621.5 ms 10 runs\n\n Benchmark 2: fetch: many refs (refformat = files, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 485.8 ms ± 4.3 ms [User: 91.1 ms, System: 396.7 ms]\n Range (min … max): 477.6 ms … 494.3 ms 10 runs\n\n Summary\n fetch: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n 1.25 ± 0.02 times faster than fetch: many refs (refformat = files, refcount = 10000, revision = master)\n\nWith this we'll either be using a regular transaction or a batch update\ntransaction. This helps cleanup some code which is no longer needed as\nwe'll now always have some type of 'ref_transaction' object being\npropagated.\n\nOne big change is that earlier, each individual update would propagate a\nfailure. Whereas now, the `ref_transaction_for_each_rejected_update`\nfunction is called at the end of the flow to capture the exit status for\n'git-fetch(1)' and also to print F/D conflict errors. This does change\nthe order of the errors being printed, but the behavior stays the same.\n\nSince transaction errors are now explicitly defined as part of\n76e760b999 (refs: introduce enum-based transaction error types,\n2025-04-08), utilize them and get rid of custom errors defined within\n'builtin/fetch.c'.\n\nSigned-off-by: Karthik Nayak \n---\n builtin/fetch.c | 128 ++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 74 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5279997c96..15eac2b1c2 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -641,9 +641,6 @@ static struct ref *get_ref_map(struct remote *remote,\n \treturn ref_map;\n }\n \n-#define STORE_REF_ERROR_OTHER 1\n-#define STORE_REF_ERROR_DF_CONFLICT 2\n-\n static int s_update_ref(const char *action,\n \t\t\tstruct ref *ref,\n \t\t\tstruct ref_transaction *transaction,\n@@ -651,7 +648,6 @@ static int s_update_ref(const char *action,\n {\n \tchar *msg;\n \tchar *rla = getenv(\"GIT_REFLOG_ACTION\");\n-\tstruct ref_transaction *our_transaction = NULL;\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret;\n \n@@ -661,43 +657,10 @@ static int s_update_ref(const char *action,\n \t\trla = default_rla.buf;\n \tmsg = xstrfmt(\"%s: %s\", rla, action);\n \n-\t/*\n-\t * If no transaction was passed to us, we manage the transaction\n-\t * ourselves. Otherwise, we trust the caller to handle the transaction\n-\t * lifecycle.\n-\t */\n-\tif (!transaction) {\n-\t\ttransaction = our_transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t\t\t\t 0, &err);\n-\t\tif (!transaction) {\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n-\n \tret = ref_transaction_update(transaction, ref->name, &ref->new_oid,\n \t\t\t\t check_old ? &ref->old_oid : NULL,\n \t\t\t\t NULL, NULL, 0, msg, &err);\n-\tif (ret) {\n-\t\tret = STORE_REF_ERROR_OTHER;\n-\t\tgoto out;\n-\t}\n-\n-\tif (our_transaction) {\n-\t\tswitch (ref_transaction_commit(our_transaction, &err)) {\n-\t\tcase 0:\n-\t\t\tbreak;\n-\t\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n-\t\t\tret = STORE_REF_ERROR_DF_CONFLICT;\n-\t\t\tgoto out;\n-\t\tdefault:\n-\t\t\tret = STORE_REF_ERROR_OTHER;\n-\t\t\tgoto out;\n-\t\t}\n-\t}\n \n-out:\n-\tref_transaction_free(our_transaction);\n \tif (ret)\n \t\terror(\"%s\", err.buf);\n \tstrbuf_release(&err);\n@@ -1139,7 +1102,6 @@ N_(\"it took %.2f seconds to check forced updates; you can use\\n\"\n \"to avoid this check\\n\");\n \n static int store_updated_refs(struct display_state *display_state,\n-\t\t\t const char *remote_name,\n \t\t\t int connectivity_checked,\n \t\t\t struct ref_transaction *transaction, struct ref *ref_map,\n \t\t\t struct fetch_head *fetch_head,\n@@ -1277,11 +1239,6 @@ static int store_updated_refs(struct display_state *display_state,\n \t\t}\n \t}\n \n-\tif (rc & STORE_REF_ERROR_DF_CONFLICT)\n-\t\terror(_(\"some local refs could not be updated; try running\\n\"\n-\t\t \" 'git remote prune %s' to remove any old, conflicting \"\n-\t\t \"branches\"), remote_name);\n-\n \tif (advice_enabled(ADVICE_FETCH_SHOW_FORCED_UPDATES)) {\n \t\tif (!config->show_forced_updates) {\n \t\t\twarning(_(warn_show_forced_updates));\n@@ -1366,9 +1323,8 @@ static int fetch_and_consume_refs(struct display_state *display_state,\n \t}\n \n \ttrace2_region_enter(\"fetch\", \"consume_refs\", the_repository);\n-\tret = store_updated_refs(display_state, transport->remote->name,\n-\t\t\t\t connectivity_checked, transaction, ref_map,\n-\t\t\t\t fetch_head, config);\n+\tret = store_updated_refs(display_state, connectivity_checked,\n+\t\t\t\t transaction, ref_map, fetch_head, config);\n \ttrace2_region_leave(\"fetch\", \"consume_refs\", the_repository);\n \n out:\n@@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n \treturn result;\n }\n \n+struct ref_rejection_data {\n+\tint *retcode;\n+\tint conflict_msg_shown;\n+\tconst char *remote_name;\n+};\n+\n+static void ref_transaction_rejection_handler(const char *refname,\n+\t\t\t\t\t const struct object_id *old_oid UNUSED,\n+\t\t\t\t\t const struct object_id *new_oid UNUSED,\n+\t\t\t\t\t const char *old_target UNUSED,\n+\t\t\t\t\t const char *new_target UNUSED,\n+\t\t\t\t\t enum ref_transaction_error err,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n+\n+\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n+\t\terror(_(\"some local refs could not be updated; try running\\n\"\n+\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n+\t\t\t\"branches\"), data->remote_name);\n+\t\tdata->conflict_msg_shown = 1;\n+\t} else {\n+\t\tchar *reason = ref_transaction_error_msg(err);\n+\n+\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n+\t\tfree(reason);\n+\t}\n+\n+\t*data->retcode = 1;\n+}\n+\n static int do_fetch(struct transport *transport,\n \t\t struct refspec *rs,\n \t\t const struct fetch_config *config)\n@@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,\n \t\t\tretcode = 1;\n \t}\n \n+\t/*\n+\t * If not atomic, we can still use batched updates, which would be much\n+\t * more performant. We don't initiate the transaction before pruning,\n+\t * since pruning must be an independent step, to avoid F/D conflicts.\n+\t *\n+\t * TODO: if reference transactions gain logical conflict resolution, we\n+\t * can delete and create refs (with F/D conflicts) in the same transaction\n+\t * and this can be moved about the 'prune_refs()' block.\n+\t */\n+\tif (!transaction) {\n+\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\t\tif (!transaction) {\n+\t\t\tretcode = -1;\n+\t\t\tgoto cleanup;\n+\t\t}\n+\t}\n+\n \tif (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,\n \t\t\t\t &fetch_head, config)) {\n \t\tretcode = 1;\n@@ -1839,16 +1844,31 @@ static int do_fetch(struct transport *transport,\n \t\tfree_refs(tags_ref_map);\n \t}\n \n-\tif (transaction) {\n-\t\tif (retcode)\n-\t\t\tgoto cleanup;\n+\tif (retcode)\n+\t\tgoto cleanup;\n \n-\t\tretcode = ref_transaction_commit(transaction, &err);\n+\tretcode = ref_transaction_commit(transaction, &err);\n+\tif (retcode) {\n+\t\t/*\n+\t\t * Explicitly handle transaction cleanup to avoid\n+\t\t * aborting an already closed transaction.\n+\t\t */\n+\t\tref_transaction_free(transaction);\n+\t\ttransaction = NULL;\n+\t\tgoto cleanup;\n+\t}\n+\n+\tif (!atomic_fetch) {\n+\t\tstruct ref_rejection_data data = {\n+\t\t\t.retcode = &retcode,\n+\t\t\t.conflict_msg_shown = 0,\n+\t\t\t.remote_name = transport->remote->name,\n+\t\t};\n+\n+\t\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t\t &data);\n \t\tif (retcode) {\n-\t\t\t/*\n-\t\t\t * Explicitly handle transaction cleanup to avoid\n-\t\t\t * aborting an already closed transaction.\n-\t\t\t */\n \t\t\tref_transaction_free(transaction);\n \t\t\ttransaction = NULL;\n \t\t\tgoto cleanup;\n\n-- \n2.49.0\n\n"},{"id":"518143","messageId":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-3-80cbaaa55d2e@gmail.com","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com","subject":"[PATCH v2 3/4] send-pack: fix memory leak around duplicate refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T14:07:27Z","receivedAt":"2025-05-15T14:07:32Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The 'git-send-pack(1)' allows users to push objects to a remote\nrepository and explicitly list the references to be pushed. The status\nof each reference pushed is captured into a list mapped by refname.\n\nIf a reference fails to be updated, its error message is captured in the\n`ref->remote_status` field. While the command allows duplicate ref\ninputs, the list doesn't accommodate this behavior as a particular\nrefname is linked to a single `struct ref*` element. So if the user\ninputs a reference twice like:\n\n git send-pack remote.git A:foo B:foo\n\nwhere the user is trying to update the same reference 'foo' twice and\nthe reference fails to be updated, we first fill `ref->remote_status`\nwith error message for the input 'A:foo' then we override the same field\nwith the error message for 'B:foo'. This override happens without first\nfree'ing the previous value. Fix this leak.\n\nThe current tests already incorporate the above example, but in the test\n'A:foo' succeeds while 'B:foo' fails, meaning that the memory leak isn't\ntriggered. Add a new test with multiple duplicates.\n\nSigned-off-by: Karthik Nayak \n---\n send-pack.c | 7 +++++++\n t/t5408-send-pack-stdin.sh | 6 ++++++\n 2 files changed, 13 insertions(+)\n\ndiff --git a/send-pack.c b/send-pack.c\nindex 5005689cb5..4cd41a64ce 100644\n--- a/send-pack.c\n+++ b/send-pack.c\n@@ -260,6 +260,13 @@ static int receive_status(struct repository *r,\n \t\t\t\trefname);\n \t\t\tcontinue;\n \t\t}\n+\n+\t\t/*\n+\t\t * Clients sending duplicate refs can cause the same value\n+\t\t * to be overridden, causing a memory leak.\n+\t\t */\n+\t\tfree(hint->remote_status);\n+\n \t\tif (!strcmp(head, \"ng\")) {\n \t\t\thint->status = REF_STATUS_REMOTE_REJECT;\n \t\t\tif (p)\ndiff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh\nindex 526a675045..45fb20179b 100755\n--- a/t/t5408-send-pack-stdin.sh\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -73,6 +73,12 @@ test_expect_success 'cmdline refs written in order' '\n \tverify_push A foo\n '\n \n+test_expect_success 'cmdline refs with multiple duplicates' '\n+\tclear_remote &&\n+\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo &&\n+\tverify_push A foo\n+'\n+\n test_expect_success '--stdin refs come after cmdline' '\n \tclear_remote &&\n \techo A:foo >input &&\n\n-- \n2.49.0\n\n"},{"id":"518144","messageId":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-4-80cbaaa55d2e@gmail.com","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com","subject":"[PATCH v2 4/4] receive-pack: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-15T14:07:28Z","receivedAt":"2025-05-15T14:07:34Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"The reference updates performed as a part of 'git-receive-pack(1)', take\nplace one at a time. For each reference update, a new transaction is\ncreated and committed. This is necessary to ensure we can allow\nindividual updates to fail without failing the entire command. The\ncommand also supports an 'atomic' mode, which uses a single transaction\nto update all of the references. But this mode has an all-or-nothing\napproach, where if a single update fails, all updates would fail.\n\nIn 23fc8e4f61 (refs: implement batch reference update support,\n2025-04-08), we introduced a new mechanism to batch reference updates.\nUnder the hood, this uses a single transaction to perform a batch of\nreference updates, while allowing only individual updates to fail.\nUtilize this newly introduced batch update mechanism in\n'git-receive-pack(1)'. This provides a significant bump in performance,\nespecially when dealing with repositories with large number of\nreferences.\n\nWith the reftable backend there is a 18x performance improvement, when\nperforming receive-pack with 10000 refs:\n\n Benchmark 1: receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n Time (mean ± σ): 4.276 s ± 0.078 s [User: 0.796 s, System: 3.318 s]\n Range (min … max): 4.185 s … 4.430 s 10 runs\n\n Benchmark 2: receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 235.4 ms ± 6.9 ms [User: 75.4 ms, System: 157.3 ms]\n Range (min … max): 228.5 ms … 254.2 ms 11 runs\n\n Summary\n receive: many refs (refformat = reftable, refcount = 10000, revision = HEAD) ran\n 18.16 ± 0.63 times faster than receive: many refs (refformat = reftable, refcount = 10000, revision = master)\n\nIn similar conditions, the files backend sees a 1.21x performance\nimprovement:\n\n Benchmark 1: receive: many refs (refformat = files, refcount = 10000, revision = master)\n Time (mean ± σ): 1.121 s ± 0.021 s [User: 0.128 s, System: 0.975 s]\n Range (min … max): 1.097 s … 1.156 s 10 runs\n\n Benchmark 2: receive: many refs (refformat = files, refcount = 10000, revision = HEAD)\n Time (mean ± σ): 927.9 ms ± 22.6 ms [User: 99.0 ms, System: 815.2 ms]\n Range (min … max): 903.1 ms … 978.0 ms 10 runs\n\n Summary\n receive: many refs (refformat = files, refcount = 10000, revision = HEAD) ran\n 1.21 ± 0.04 times faster than receive: many refs (refformat = files, refcount = 10000, revision = master)\n\nAs using batched updates requires the error handling to be moved to the\nend of the flow, create and use a 'struct strset' to track the failed\nrefs and attribute the correct errors to them.\n\nThis change also uncovers an issue when a client provides multiple\nupdates to the same reference. For example:\n\n $ git send-pack remote.git A:foo B:foo\n Enumerating objects: 3, done.\n Counting objects: 100% (3/3), done.\n Delta compression using up to 20 threads\n Compressing objects: 100% (2/2), done.\n Writing objects: 100% (3/3), 226 bytes | 226.00 KiB/s, done.\n Total 3 (delta 1), reused 0 (delta 0), pack-reused 0 (from 0)\n remote: error: cannot lock ref 'refs/heads/foo': reference already exists\n To remote.git\n ! [remote rejected] A -> foo (failed to update ref)\n ! [remote failure] B -> foo (remote failed to report status)\n\nAs you can see, the remote runs into an error because it cannot lock the\ntarget reference for the second update. Furthermore, the remote complains\nthat the first update has been rejected whereas the second update didn't\nreceive any status update because we failed to lock it. Reading this status\nmessage alone a user would probably expect that `foo` has not been updated\nat all. But that's not the case: while we claim that the ref wasn't updated,\nit surprisingly points to `A` now.\n\nOne could argue that this is merely an error in how we report the result of\nthis push. But ultimately, the user's request itself is already broken and\ndoesn't make any sense in the first place and cannot ever lead to a sensible\noutcome that honors the full request.\n\nThe conversion to batched transactions fixes the issue because we now try to\nqueue both updates in the same transaction. As such, the transaction itself\nwill notice this conflict and refuse the update altogether before we commit\nany of the values.\n\nNote that this requires changes to a couple of tests in t5408 that happened\nto exercise this behaviour. Given that the generated output is misleading\nand given that the user request cannot ever be fully honored this really\nfeels more like a bug than properly designed behaviour. As such, changing\nthe behaviour feels like the right thing to do.\n\nSince now reference updates are batched, the 'reference-transaction'\nhook will be invoked with all updates together. Currently git will 'die'\nwhen the hook returns with a non-zero exit status in the 'prepared'\nstage. For 'git-receive-pack(1)', this allowed users to reject an\nindividual reference update, git would have applied previous updates but\nimmediately abort further execution. This is definitely an incorrect\nusage of this hook, since the right place to do this would be the\n'update' hook. This patch retains the latter behavior, but\n'reference-transaction' hook now changes to a all-or-nothing behavior\nwhen a non-zero exit status is returned in the 'prepared' stage, since\nbatch updates use a transaction under the hood. This explains the change\nin 't1416'.\n\nHelped-by: Patrick Steinhardt \nSigned-off-by: Karthik Nayak \n---\n builtin/receive-pack.c | 64 ++++++++++++++++++++++++++++++----------\n t/t1416-ref-transaction-hooks.sh | 2 --\n t/t5408-send-pack-stdin.sh | 12 ++++----\n 3 files changed, 55 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex be314879e8..4c47d6d444 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1843,35 +1843,67 @@ static void BUG_if_skipped_connectivity_check(struct command *commands,\n \tBUG_if_bug(\"connectivity check skipped???\");\n }\n \n+static void ref_transaction_rejection_handler(const char *refname,\n+\t\t\t\t\t const struct object_id *old_oid UNUSED,\n+\t\t\t\t\t const struct object_id *new_oid UNUSED,\n+\t\t\t\t\t const char *old_target UNUSED,\n+\t\t\t\t\t const char *new_target UNUSED,\n+\t\t\t\t\t enum ref_transaction_error err,\n+\t\t\t\t\t void *cb_data)\n+{\n+\tstruct strmap *failed_refs = cb_data;\n+\n+\tstrmap_put(failed_refs, refname, ref_transaction_error_msg(err));\n+}\n+\n static void execute_commands_non_atomic(struct command *commands,\n \t\t\t\t\tstruct shallow_info *si)\n {\n \tstruct command *cmd;\n \tstruct strbuf err = STRBUF_INIT;\n+\tconst char *reported_error = NULL;\n+\tstruct strmap failed_refs = STRMAP_INIT;\n+\n+\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t REF_TRANSACTION_ALLOW_FAILURE, &err);\n+\tif (!transaction) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\tstrbuf_reset(&err);\n+\t\treported_error = \"transaction failed to start\";\n+\t\tgoto failure;\n+\t}\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tif (!should_process_cmd(cmd) || cmd->run_proc_receive)\n \t\t\tcontinue;\n \n-\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t\t 0, &err);\n-\t\tif (!transaction) {\n-\t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_reset(&err);\n-\t\t\tcmd->error_string = \"transaction failed to start\";\n-\t\t\tcontinue;\n-\t\t}\n-\n \t\tcmd->error_string = update(cmd, si);\n+\t}\n \n-\t\tif (!cmd->error_string\n-\t\t && ref_transaction_commit(transaction, &err)) {\n-\t\t\trp_error(\"%s\", err.buf);\n-\t\t\tstrbuf_reset(&err);\n-\t\t\tcmd->error_string = \"failed to update ref\";\n-\t\t}\n-\t\tref_transaction_free(transaction);\n+\tif (ref_transaction_commit(transaction, &err)) {\n+\t\trp_error(\"%s\", err.buf);\n+\t\treported_error = \"failed to update refs\";\n+\t\tgoto failure;\n+\t}\n+\n+\tref_transaction_for_each_rejected_update(transaction,\n+\t\t\t\t\t\t ref_transaction_rejection_handler,\n+\t\t\t\t\t\t &failed_refs);\n+\n+\tif (strmap_empty(&failed_refs))\n+\t\tgoto cleanup;\n+\n+failure:\n+\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tif (reported_error)\n+\t\t\tcmd->error_string = reported_error;\n+\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n+\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n \t}\n+\n+cleanup:\n+\tref_transaction_free(transaction);\n+\tstrmap_clear(&failed_refs, 1);\n \tstrbuf_release(&err);\n }\n \ndiff --git a/t/t1416-ref-transaction-hooks.sh b/t/t1416-ref-transaction-hooks.sh\nindex 8c777f7cf8..d91dd3a3b5 100755\n--- a/t/t1416-ref-transaction-hooks.sh\n+++ b/t/t1416-ref-transaction-hooks.sh\n@@ -120,8 +120,6 @@ test_expect_success 'interleaving hook calls succeed' '\n \n \tcat >expect <<-EOF &&\n \t\thooks/update refs/tags/PRE $ZERO_OID $PRE_OID\n-\t\thooks/reference-transaction prepared\n-\t\thooks/reference-transaction committed\n \t\thooks/update refs/tags/POST $ZERO_OID $POST_OID\n \t\thooks/reference-transaction prepared\n \t\thooks/reference-transaction committed\ndiff --git a/t/t5408-send-pack-stdin.sh b/t/t5408-send-pack-stdin.sh\nindex 45fb20179b..76fb8bbc68 100755\n--- a/t/t5408-send-pack-stdin.sh\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -69,21 +69,23 @@ test_expect_success 'stdin mixed with cmdline' '\n \n test_expect_success 'cmdline refs written in order' '\n \tclear_remote &&\n-\ttest_must_fail git send-pack remote.git A:foo B:foo &&\n-\tverify_push A foo\n+\ttest_must_fail git send-pack remote.git A:foo B:foo 2>err &&\n+\ttest_grep \"multiple updates for ref ${SQ}refs/heads/foo${SQ} not allowed\" err &&\n+\ttest_must_fail git --git-dir=remote.git rev-parse foo\n '\n \n test_expect_success 'cmdline refs with multiple duplicates' '\n \tclear_remote &&\n-\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo &&\n-\tverify_push A foo\n+\ttest_must_fail git send-pack remote.git A:foo B:foo C:foo 2>err &&\n+\ttest_grep \"multiple updates for ref ${SQ}refs/heads/foo${SQ} not allowed\" err &&\n+\ttest_must_fail git --git-dir=remote.git rev-parse foo\n '\n \n test_expect_success '--stdin refs come after cmdline' '\n \tclear_remote &&\n \techo A:foo >input &&\n \ttest_must_fail git send-pack remote.git --stdin B:foo +failure:\n> +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> +\t\tif (reported_error)\n> +\t\t\tcmd->error_string = reported_error;\n> +\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n> +\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n> \t}\n\nCoverity complains about this code, claiming that strmap_get() could\nreturn NULL. At first glance, I thought it was being totally stupid,\nsince we just called strmap_contains() above. But it's only being a\nlittle bit stupid: even if the entry exists, it is still possible for it\nto contain NULL.\n\nHowever, that won't ever be the case, since it is always fed from the\nset of ref transaction reasons.\n\nStill, I wondered if:\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bd0fb729ff..fe001bbfe8 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1897,10 +1897,11 @@ static void execute_commands_non_atomic(struct command *commands,\n \n failure:\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tconst char *reason;\n \t\tif (reported_error)\n \t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n+\t\telse if ((reason = strmap_get(&failed_refs, cmd->ref_name)))\n+\t\t\tcmd->error_string = xstrdup(reason);\n \t}\n \n cleanup:\n\nmight be simpler, and skip the extra lookup? I dunno, it is probably\ngetting into bikeshedding, and it is not like this is the only Coverity\nfalse positive we have seen. So feel free to ignore. ;)\n\n-Peff\n"},{"id":"518163","messageId":"20250515190909.GA3320028@coredump.intra.peff.net","threadId":"63455","inReplyTo":"20250515185535.GA3309052@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] receive-pack: use batched reference updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:09:09Z","receivedAt":"2025-05-15T19:09:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 02:55:36PM -0400, Jeff King wrote:\n\n> On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:\n> \n> > +failure:\n> > +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n> > +\t\tif (reported_error)\n> > +\t\t\tcmd->error_string = reported_error;\n> > +\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n> > +\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n> > \t}\n\nBTW, one other funny thing about this code: we duplicate the strings\nthat we assign to cmd->error_string. But no other call path does so. So\nnow we have allocated memory, but nobody can ever free() it because\noften it is not allocated!\n\nI'm surprised LSan does not report a leak here, so I might be missing\nsomething.\n\nIt looks like there is some magic in the struct here to handle this\nlike:\n\n cmd->error_string = cmd->error_string_owned = xstrdup(...);\n\nwhich would solve it. But I wonder: do these need to be allocated at\nall? We are pulling out strings owned by the strmap, which will free\nthem. So as-is, yes, we need to make our own copies.\n\nBut does the strmap need to own them in the first place? It is taking\nthe values from ref_transaction_error_msg(). That returns an allocated\nstring, but it doesn't need to. It could just return the string literals\ndirectly, which are valid for the life of the program. That would save\nother callers from having to call free(), though it does mean we have to\ncast away the constness when putting them into the strmap (since it\naccepts a non-const void pointer). Something like:\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4a3c46eca7..cf0bcf1ad0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1665,10 +1665,9 @@ static void ref_transaction_rejection_handler(const char *refname,\n \t\t\t\"branches\"), data->remote_name);\n \t\tdata->conflict_msg_shown = 1;\n \t} else {\n-\t\tchar *reason = ref_transaction_error_msg(err);\n+\t\tconst char *reason = ref_transaction_error_msg(err);\n \n \t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n-\t\tfree(reason);\n \t}\n \n \t*data->retcode = 1;\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex bd0fb729ff..4cef2ddd3d 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1855,7 +1855,7 @@ static void ref_transaction_rejection_handler(const char *refname,\n {\n \tstruct strmap *failed_refs = cb_data;\n \n-\tstrmap_put(failed_refs, refname, ref_transaction_error_msg(err));\n+\tstrmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));\n }\n \n static void execute_commands_non_atomic(struct command *commands,\n@@ -1897,15 +1897,16 @@ static void execute_commands_non_atomic(struct command *commands,\n \n failure:\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\tconst char *reason;\n \t\tif (reported_error)\n \t\t\tcmd->error_string = reported_error;\n-\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n-\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n+\t\telse if ((reason = strmap_get(&failed_refs, cmd->ref_name)))\n+\t\t\tcmd->error_string = reason;\n \t}\n \n cleanup:\n \tref_transaction_free(transaction);\n-\tstrmap_clear(&failed_refs, 1);\n+\tstrmap_clear(&failed_refs, 0);\n \tstrbuf_release(&err);\n }\n \ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 09b99143bf..1e6131e04a 100644\n--- a/builtin/update-ref.c\n+++ b/builtin/update-ref.c\n@@ -575,15 +575,14 @@ static void print_rejected_refs(const char *refname,\n \t\t\t\tvoid *cb_data UNUSED)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n-\tchar *reason = ref_transaction_error_msg(err);\n+\tconst char *reason = ref_transaction_error_msg(err);\n \n \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n \t\t new_oid ? oid_to_hex(new_oid) : new_target,\n \t\t old_oid ? oid_to_hex(old_oid) : old_target,\n \t\t reason);\n \n \tfwrite(sb.buf, sb.len, 1, stdout);\n-\tfree(reason);\n \tstrbuf_release(&sb);\n }\n \ndiff --git a/refs.c b/refs.c\nindex 351ed52deb..953f83bb52 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3315,7 +3315,7 @@ int ref_update_expects_existing_old_ref(struct ref_update *update)\n \t\t(!is_null_oid(&update->old_oid) || update->old_target);\n }\n \n-char *ref_transaction_error_msg(enum ref_transaction_error err)\n+const char *ref_transaction_error_msg(enum ref_transaction_error err)\n {\n \tconst char *reason = \"\";\n \n@@ -3342,5 +3342,5 @@ char *ref_transaction_error_msg(enum ref_transaction_error err)\n \t\treason = \"unkown failure\";\n \t}\n \n-\treturn xstrdup(reason);\n+\treturn reason;\n }\ndiff --git a/refs.h b/refs.h\nindex a0b2e3c43d..2d58af3d88 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -910,7 +910,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio\n /*\n * Translate errors to human readable error messages.\n */\n-char *ref_transaction_error_msg(enum ref_transaction_error err);\n+const char *ref_transaction_error_msg(enum ref_transaction_error err);\n \n /*\n * Free `*transaction` and all associated data.\n\n-Peff\n"},{"id":"518164","messageId":"20250515191153.GB3309052@coredump.intra.peff.net","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-1-80cbaaa55d2e@gmail.com","subject":"Re: [PATCH v2 1/4] refs: add function to translate errors to strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-15T19:11:53Z","receivedAt":"2025-05-15T19:11:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 15, 2025 at 04:07:25PM +0200, Karthik Nayak wrote:\n\n> +char *ref_transaction_error_msg(enum ref_transaction_error err)\n> +{\n> +\tconst char *reason = \"\";\n> +\n> +\tswitch (err) {\n> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n> +\t\treason = \"refname conflict\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n> +\t\treason = \"reference already exists\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n> +\t\treason = \"reference does not exist\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n> +\t\treason = \"incorrect old value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n> +\t\treason = \"invalid new value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_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> +\treturn xstrdup(reason);\n> +}\n\nThe assignment of \"\" is dead code, I think? We will always assign\n\"unknown failure\" as a last resort. Not a big deal, but just something I\nnoticed while reading this related to what's going on in patch 4.\n\nAlso, s/unkown/unknown/, but that is present in the pre-image. I hope we\ndon't need to retain it for bug-for-bug plumbing compatibility. :)\n\n(I guess the dead store of \"\" was present in the original, too, for that\nmatter).\n\n-Peff\n\nPS Sorry for all the nit-picky comments. I was just going down the\n Coverity rabbit hole and didn't really review the rest of the series.\n But I wanted to say that the numbers you are seeing are very cool!\n"},{"id":"518183","messageId":"xmqqbjrtzkm1.fsf@gitster.g","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-1-80cbaaa55d2e@gmail.com","subject":"Re: [PATCH v2 1/4] refs: add function to translate errors to strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-15T20:26:14Z","receivedAt":"2025-05-15T20:26:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak writes:\n\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 2b1e336ba1..09b99143bf 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -575,30 +575,7 @@ static void print_rejected_refs(const char *refname,\n> \t\t\t\tvoid *cb_data UNUSED)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> -\tconst char *reason = \"\";\n> -\n> -\tswitch (err) {\n> -\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n> -\t\treason = \"refname conflict\";\n> -\t\tbreak;\n> -\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n> -\t\treason = \"reference already exists\";\n> -\t\tbreak;\n> -\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n> -\t\treason = \"reference does not exist\";\n> -\t\tbreak;\n> -\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n> -\t\treason = \"incorrect old value provided\";\n> -\t\tbreak;\n> -\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n> -\t\treason = \"invalid new value provided\";\n> -\t\tbreak;\n> -\tcase REF_TRANSACTION_ERROR_EXPECTED_SYMREF:\n> -\t\treason = \"expected symref but found regular ref\";\n> -\t\tbreak;\n> -\tdefault:\n> -\t\treason = \"unkown failure\";\n> -\t}\n> +\tchar *reason = ref_transaction_error_msg(err);\n> \n> \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n> \t\t new_oid ? oid_to_hex(new_oid) : new_target,\n> @@ -606,6 +583,7 @@ static void print_rejected_refs(const char *refname,\n> \t\t reason);\n> \n> \tfwrite(sb.buf, sb.len, 1, stdout);\n> +\tfree(reason);\n> \tstrbuf_release(&sb);\n> }\n\nWhy free()? Goes and reads on...\n\n> +char *ref_transaction_error_msg(enum ref_transaction_error err)\n> +{\n> +\tconst char *reason = \"\";\n> +\n> +\tswitch (err) {\n> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n> +\t\treason = \"refname conflict\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n> +\t\treason = \"reference already exists\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n> +\t\treason = \"reference does not exist\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n> +\t\treason = \"incorrect old value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n> +\t\treason = \"invalid new value provided\";\n> +\t\tbreak;\n> +\tcase REF_TRANSACTION_ERROR_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> +\treturn xstrdup(reason);\n> +}\n\nWhy can't this return \"const char *\", without xstrdup()?\n"},{"id":"518212","messageId":"aCbP1SxncSVw2fCa@pks.im","threadId":"63455","inReplyTo":"20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-2-80cbaaa55d2e@gmail.com","subject":"Re: [PATCH v2 2/4] fetch: use batched reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-16T05:40:37Z","receivedAt":"2025-05-16T05:40:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 5279997c96..15eac2b1c2 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n> \treturn result;\n> }\n> \n> +struct ref_rejection_data {\n> +\tint *retcode;\n> +\tint conflict_msg_shown;\n> +\tconst char *remote_name;\n> +};\n> +\n> +static void ref_transaction_rejection_handler(const char *refname,\n> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n> +\t\t\t\t\t const char *old_target UNUSED,\n> +\t\t\t\t\t const char *new_target UNUSED,\n> +\t\t\t\t\t enum ref_transaction_error err,\n> +\t\t\t\t\t void *cb_data)\n> +{\n> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n\nNit: unnecessary cast.\n\n> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n> +\t\t\t\"branches\"), data->remote_name);\n> +\t\tdata->conflict_msg_shown = 1;\n> +\t} else {\n> +\t\tchar *reason = ref_transaction_error_msg(err);\n> +\n> +\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n> +\t\tfree(reason);\n> +\t}\n> +\n> +\t*data->retcode = 1;\n> +}\n\nOkay, we stopped ignoring generic errors now and will print them. What\nI'm still unclear about: which exact errors do we accept now that\n`REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we\nprobably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?\n\nThis makes me wonder a bit about the current layout of how we handle\nthese errors. If the rejection handler was invoked while preparing the\ntransaction for each reference as we go instead of afterwards we could\ndecide on-the-fly whether a specific error should be ignored or not.\nThat might lead to a design that is both more flexible and more obvious\nat the same time because error handling is now handled explicitly by the\ncallsite that wants to ignore some errors.\n\nLast but not least, I think that it would also allow us to decide ahead\nof time whether we want to commit. Right now we basically say \"just\ncommit it, whatever happens\". But if I'm not mistaken, all the errors\nthat we care about and that callers may want to ignore are already\ndetected at prepare time. So if we already bubbled up relevant info\nwhile calling `ref_transaction_prepare()` the caller may then decide to\nnot commit at all based on some criteria.\n\nSorry, I should've probably proposed this when you introducued this\nmechanism. But sometimes you only see things like that as we gain more\nusers.\n\n> @@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,\n> \t\t\tretcode = 1;\n> \t}\n> \n> +\t/*\n> +\t * If not atomic, we can still use batched updates, which would be much\n> +\t * more performant. We don't initiate the transaction before pruning,\n> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n> +\t *\n> +\t * TODO: if reference transactions gain logical conflict resolution, we\n> +\t * can delete and create refs (with F/D conflicts) in the same transaction\n> +\t * and this can be moved about the 'prune_refs()' block.\n\ns/about/above/?\n\nPatrick\n"},{"id":"518220","messageId":"CAOLa=ZQHR5Xt3yP4FM4gAbFVRM-dXVPpQHS0tObeoz45T89X7w@mail.gmail.com","threadId":"63455","inReplyTo":"20250515191153.GB3309052@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/4] refs: add function to translate errors to strings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-16T09:11:05Z","receivedAt":"2025-05-16T09:11:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 15, 2025 at 04:07:25PM +0200, Karthik Nayak wrote:\n>\n>> +char *ref_transaction_error_msg(enum ref_transaction_error err)\n>> +{\n>> +\tconst char *reason = \"\";\n>> +\n>> +\tswitch (err) {\n>> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n>> +\t\treason = \"refname conflict\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n>> +\t\treason = \"reference already exists\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n>> +\t\treason = \"reference does not exist\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n>> +\t\treason = \"incorrect old value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n>> +\t\treason = \"invalid new value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_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>> +\treturn xstrdup(reason);\n>> +}\n>\n> The assignment of \"\" is dead code, I think? We will always assign\n> \"unknown failure\" as a last resort. Not a big deal, but just something I\n> noticed while reading this related to what's going on in patch 4.\n>\n\nYeah, I mostly moved the code without much thought into it. This can be\ncleaned up. Specifically because we can avoid all the memory allocation\nand directly return the string as a 'const char *' as Junio mentioned.\n\n> Also, s/unkown/unknown/, but that is present in the pre-image. I hope we\n> don't need to retain it for bug-for-bug plumbing compatibility. :)\n>\n\nThanks, will change.\n\n> (I guess the dead store of \"\" was present in the original, too, for that\n> matter).\n>\n> -Peff\n>\n> PS Sorry for all the nit-picky comments. I was just going down the\n> Coverity rabbit hole and didn't really review the rest of the series.\n> But I wanted to say that the numbers you are seeing are very cool!\n\nI think all these points have good value. I'm happy to address them :)\n"},{"id":"518221","messageId":"CAOLa=ZTwchMMg81JnHagQ10rpRw6pR5+JQUWLUcaxusGFBL8PA@mail.gmail.com","threadId":"63455","inReplyTo":"xmqqbjrtzkm1.fsf@gitster.g","subject":"Re: [PATCH v2 1/4] refs: add function to translate errors to strings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-16T09:12:36Z","receivedAt":"2025-05-16T09:12:39Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano writes:\n\n[snip]\n\n>> +char *ref_transaction_error_msg(enum ref_transaction_error err)\n>> +{\n>> +\tconst char *reason = \"\";\n>> +\n>> +\tswitch (err) {\n>> +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n>> +\t\treason = \"refname conflict\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n>> +\t\treason = \"reference already exists\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n>> +\t\treason = \"reference does not exist\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n>> +\t\treason = \"incorrect old value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n>> +\t\treason = \"invalid new value provided\";\n>> +\t\tbreak;\n>> +\tcase REF_TRANSACTION_ERROR_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>> +\treturn xstrdup(reason);\n>> +}\n>\n> Why can't this return \"const char *\", without xstrdup()?\n\nYeah I think that is much better, string literals are anyways not on the\nstack and can be passed around. Will change, this would make it much\nsimpler.\n"},{"id":"518227","messageId":"CAOLa=ZR+3RPDHucjEVx8s64nrVGjzNTu9gX6Nw5vwQGg8PtpUw@mail.gmail.com","threadId":"63455","inReplyTo":"aCbP1SxncSVw2fCa@pks.im","subject":"Re: [PATCH v2 2/4] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-16T09:53:22Z","receivedAt":"2025-05-16T09:53:35Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 5279997c96..15eac2b1c2 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n>> \treturn result;\n>> }\n>>\n>> +struct ref_rejection_data {\n>> +\tint *retcode;\n>> +\tint conflict_msg_shown;\n>> +\tconst char *remote_name;\n>> +};\n>> +\n>> +static void ref_transaction_rejection_handler(const char *refname,\n>> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n>> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n>> +\t\t\t\t\t const char *old_target UNUSED,\n>> +\t\t\t\t\t const char *new_target UNUSED,\n>> +\t\t\t\t\t enum ref_transaction_error err,\n>> +\t\t\t\t\t void *cb_data)\n>> +{\n>> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n>\n> Nit: unnecessary cast.\n>\n>> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n>> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n>> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n>> +\t\t\t\"branches\"), data->remote_name);\n>> +\t\tdata->conflict_msg_shown = 1;\n>> +\t} else {\n>> +\t\tchar *reason = ref_transaction_error_msg(err);\n>> +\n>> +\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n>> +\t\tfree(reason);\n>> +\t}\n>> +\n>> +\t*data->retcode = 1;\n>> +}\n>\n> Okay, we stopped ignoring generic errors now and will print them. What\n> I'm still unclear about: which exact errors do we accept now that\n> `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we\n> probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?\n\nThe current mechanism in `ref_transaction_maybe_set_rejected()` doesn't\nhandle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice\n(more of a requirement of what this error represents), where\n`REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an\nindividual reference level. It includes:\n\n - System errors such as I/O errors\n - Duplicates present\n\nBoth of these represent issues which are bigger than a single ref\nupdate, so we have to propagate these errors up.\n\n>\n> This makes me wonder a bit about the current layout of how we handle\n> these errors. If the rejection handler was invoked while preparing the\n> transaction for each reference as we go instead of afterwards we could\n> decide on-the-fly whether a specific error should be ignored or not.\n> That might lead to a design that is both more flexible and more obvious\n> at the same time because error handling is now handled explicitly by the\n> callsite that wants to ignore some errors.\n>\n\nI did ponder on this while I was building the batched transaction\nmechanism. I decided to take it iteratively. We can, for instance,\nmodify `ref_transaction_maybe_set_rejected()` to work with a callback\nfunction which would allow the users to accept/reject errors.\n\nHowever, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC`\nerrors still cannot be overlooked, these errors will abort the entire\ntransaction.\n\nThat said, I'm not trying to avoid going down that route. I do agree\nwith the flexibility it does provide. Once we hit such a usecase, we\nshould make that change.\n\nFor 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?\n\n> Last but not least, I think that it would also allow us to decide ahead\n> of time whether we want to commit. Right now we basically say \"just\n> commit it, whatever happens\". But if I'm not mistaken, all the errors\n> that we care about and that callers may want to ignore are already\n> detected at prepare time. So if we already bubbled up relevant info\n> while calling `ref_transaction_prepare()` the caller may then decide to\n> not commit at all based on some criteria.\n>\n\nIndeed, that is correct. I can confirm that even now all the calls to\n`ref_transaction_maybe_set_rejected()` are made in the prepare phase, so\nwe could already do this, since `transaction->rejections` is already\npopulated at this stage.\n\n> Sorry, I should've probably proposed this when you introducued this\n> mechanism. But sometimes you only see things like that as we gain more\n> users.\n>\n\nYou don't have to apologize. Such discussions are very important and you\nshouldn't hesitate to bring up such points.\n\n>> @@ -1808,6 +1795,24 @@ static int do_fetch(struct transport *transport,\n>> \t\t\tretcode = 1;\n>> \t}\n>>\n>> +\t/*\n>> +\t * If not atomic, we can still use batched updates, which would be much\n>> +\t * more performant. We don't initiate the transaction before pruning,\n>> +\t * since pruning must be an independent step, to avoid F/D conflicts.\n>> +\t *\n>> +\t * TODO: if reference transactions gain logical conflict resolution, we\n>> +\t * can delete and create refs (with F/D conflicts) in the same transaction\n>> +\t * and this can be moved about the 'prune_refs()' block.\n>\n> s/about/above/?\n>\n\nIndeed!\n\n> Patrick\n"},{"id":"518228","messageId":"aCcM0QK6QBdWO2jj@pks.im","threadId":"63455","inReplyTo":"CAOLa=ZR+3RPDHucjEVx8s64nrVGjzNTu9gX6Nw5vwQGg8PtpUw@mail.gmail.com","subject":"Re: [PATCH v2 2/4] fetch: use batched reference updates","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-05-16T10:00:49Z","receivedAt":"2025-05-16T10:00:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, May 16, 2025 at 02:53:22AM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:\n> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> >> index 5279997c96..15eac2b1c2 100644\n> >> --- a/builtin/fetch.c\n> >> +++ b/builtin/fetch.c\n> >> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n> >> \treturn result;\n> >> }\n> >>\n> >> +struct ref_rejection_data {\n> >> +\tint *retcode;\n> >> +\tint conflict_msg_shown;\n> >> +\tconst char *remote_name;\n> >> +};\n> >> +\n> >> +static void ref_transaction_rejection_handler(const char *refname,\n> >> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n> >> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n> >> +\t\t\t\t\t const char *old_target UNUSED,\n> >> +\t\t\t\t\t const char *new_target UNUSED,\n> >> +\t\t\t\t\t enum ref_transaction_error err,\n> >> +\t\t\t\t\t void *cb_data)\n> >> +{\n> >> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n> >\n> > Nit: unnecessary cast.\n> >\n> >> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n> >> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n> >> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n> >> +\t\t\t\"branches\"), data->remote_name);\n> >> +\t\tdata->conflict_msg_shown = 1;\n> >> +\t} else {\n> >> +\t\tchar *reason = ref_transaction_error_msg(err);\n> >> +\n> >> +\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n> >> +\t\tfree(reason);\n> >> +\t}\n> >> +\n> >> +\t*data->retcode = 1;\n> >> +}\n> >\n> > Okay, we stopped ignoring generic errors now and will print them. What\n> > I'm still unclear about: which exact errors do we accept now that\n> > `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we\n> > probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?\n> \n> The current mechanism in `ref_transaction_maybe_set_rejected()` doesn't\n> handle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice\n> (more of a requirement of what this error represents), where\n> `REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an\n> individual reference level. It includes:\n> \n> - System errors such as I/O errors\n> - Duplicates present\n> \n> Both of these represent issues which are bigger than a single ref\n> update, so we have to propagate these errors up.\n\nThe second case is also why the behaviour changes now, right? If we were\nable to handle duplicates via the same mechanism then it would become\npossible to retain current behaviour for git-receive-pack(1)?\n\nNot that I'm proposing this -- I very much think that the current\nbehaviour in git-receive-pack(1) is a bug that should be fixed. Mostly\ntrying to understand.\n\n> > This makes me wonder a bit about the current layout of how we handle\n> > these errors. If the rejection handler was invoked while preparing the\n> > transaction for each reference as we go instead of afterwards we could\n> > decide on-the-fly whether a specific error should be ignored or not.\n> > That might lead to a design that is both more flexible and more obvious\n> > at the same time because error handling is now handled explicitly by the\n> > callsite that wants to ignore some errors.\n> >\n> \n> I did ponder on this while I was building the batched transaction\n> mechanism. I decided to take it iteratively. We can, for instance,\n> modify `ref_transaction_maybe_set_rejected()` to work with a callback\n> function which would allow the users to accept/reject errors.\n> \n> However, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC`\n> errors still cannot be overlooked, these errors will abort the entire\n> transaction.\n\nOkay, good.\n\n> That said, I'm not trying to avoid going down that route. I do agree\n> with the flexibility it does provide. Once we hit such a usecase, we\n> should make that change.\n> \n> For 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?\n\nNo, I don't right now. I just want to avoid that we have to eventually\nrefactor all of this to support an alternative API. But agreed, there\nisn't really much of a reason why we wouldn't be able to introduce such\na mechanism retroactively while keeping existing callers intact.\n\nSo let's stick with what we have and keep this in the back of our minds\nif we ever need such a mechanism going forward.\n\n> > Last but not least, I think that it would also allow us to decide ahead\n> > of time whether we want to commit. Right now we basically say \"just\n> > commit it, whatever happens\". But if I'm not mistaken, all the errors\n> > that we care about and that callers may want to ignore are already\n> > detected at prepare time. So if we already bubbled up relevant info\n> > while calling `ref_transaction_prepare()` the caller may then decide to\n> > not commit at all based on some criteria.\n> >\n> \n> Indeed, that is correct. I can confirm that even now all the calls to\n> `ref_transaction_maybe_set_rejected()` are made in the prepare phase, so\n> we could already do this, since `transaction->rejections` is already\n> populated at this stage.\n\nGood. After all, we shouldn't have to perform checks in the \"commit\"\nphase. Things are locked, things have been checked, so it should\nbasically be a mere \"let's move everything into place now\". Which of\ncourse can still fail, but the only valid reason should be system\nfailures.\n\nPatrick\n"},{"id":"518305","messageId":"CAOLa=ZSwzWm=43daWfu7aAb7q9gnseT=bifh0DUS3Cr02taCRQ@mail.gmail.com","threadId":"63455","inReplyTo":"20250515190909.GA3320028@coredump.intra.peff.net","subject":"Re: [PATCH v2 4/4] receive-pack: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-16T19:49:35Z","receivedAt":"2025-05-16T19:49:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 15, 2025 at 02:55:36PM -0400, Jeff King wrote:\n>\n>> On Thu, May 15, 2025 at 04:07:28PM +0200, Karthik Nayak wrote:\n>>\n>> > +failure:\n>> > +\tfor (cmd = commands; cmd; cmd = cmd->next) {\n>> > +\t\tif (reported_error)\n>> > +\t\t\tcmd->error_string = reported_error;\n>> > +\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n>> > +\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n>> > \t}\n>\n> BTW, one other funny thing about this code: we duplicate the strings\n> that we assign to cmd->error_string. But no other call path does so. So\n> now we have allocated memory, but nobody can ever free() it because\n> often it is not allocated!\n>\n> I'm surprised LSan does not report a leak here, so I might be missing\n> something.\n>\n\nYeah, indeed that is a leak and surprising that this wasn't caught by\nLSan.\n\n> It looks like there is some magic in the struct here to handle this\n> like:\n>\n> cmd->error_string = cmd->error_string_owned = xstrdup(...);\n>\n> which would solve it. But I wonder: do these need to be allocated at\n> all? We are pulling out strings owned by the strmap, which will free\n> them. So as-is, yes, we need to make our own copies.\n>\n> But does the strmap need to own them in the first place? It is taking\n> the values from ref_transaction_error_msg(). That returns an allocated\n> string, but it doesn't need to. It could just return the string literals\n> directly, which are valid for the life of the program. That would save\n> other callers from having to call free(), though it does mean we have to\n> cast away the constness when putting them into the strmap (since it\n> accepts a non-const void pointer). Something like:\n>\n\nThanks Jeff, this does make a lot of sense, I think with Junio's\nsuggestion in the first patch, making `ref_transaction_error_msg()`\nreturn a 'const char *' solves this issue too, we no longer will hold\nallocated strings here and as such no longer have to worry about\nallocation/freeing of memory. Much cleaner.\n\nIn the end it looks similar to you patch below!\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 4a3c46eca7..cf0bcf1ad0 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1665,10 +1665,9 @@ static void ref_transaction_rejection_handler(const char *refname,\n> \t\t\t\"branches\"), data->remote_name);\n> \t\tdata->conflict_msg_shown = 1;\n> \t} else {\n> -\t\tchar *reason = ref_transaction_error_msg(err);\n> +\t\tconst char *reason = ref_transaction_error_msg(err);\n>\n> \t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n> -\t\tfree(reason);\n> \t}\n>\n> \t*data->retcode = 1;\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index bd0fb729ff..4cef2ddd3d 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1855,7 +1855,7 @@ static void ref_transaction_rejection_handler(const char *refname,\n> {\n> \tstruct strmap *failed_refs = cb_data;\n>\n> -\tstrmap_put(failed_refs, refname, ref_transaction_error_msg(err));\n> +\tstrmap_put(failed_refs, refname, (char *)ref_transaction_error_msg(err));\n> }\n>\n> static void execute_commands_non_atomic(struct command *commands,\n> @@ -1897,15 +1897,16 @@ static void execute_commands_non_atomic(struct command *commands,\n>\n> failure:\n> \tfor (cmd = commands; cmd; cmd = cmd->next) {\n> +\t\tconst char *reason;\n> \t\tif (reported_error)\n> \t\t\tcmd->error_string = reported_error;\n> -\t\telse if (strmap_contains(&failed_refs, cmd->ref_name))\n> -\t\t\tcmd->error_string = xstrdup(strmap_get(&failed_refs, cmd->ref_name));\n> +\t\telse if ((reason = strmap_get(&failed_refs, cmd->ref_name)))\n> +\t\t\tcmd->error_string = reason;\n> \t}\n>\n> cleanup:\n> \tref_transaction_free(transaction);\n> -\tstrmap_clear(&failed_refs, 1);\n> +\tstrmap_clear(&failed_refs, 0);\n> \tstrbuf_release(&err);\n> }\n>\n> diff --git a/builtin/update-ref.c b/builtin/update-ref.c\n> index 09b99143bf..1e6131e04a 100644\n> --- a/builtin/update-ref.c\n> +++ b/builtin/update-ref.c\n> @@ -575,15 +575,14 @@ static void print_rejected_refs(const char *refname,\n> \t\t\t\tvoid *cb_data UNUSED)\n> {\n> \tstruct strbuf sb = STRBUF_INIT;\n> -\tchar *reason = ref_transaction_error_msg(err);\n> +\tconst char *reason = ref_transaction_error_msg(err);\n>\n> \tstrbuf_addf(&sb, \"rejected %s %s %s %s\\n\", refname,\n> \t\t new_oid ? oid_to_hex(new_oid) : new_target,\n> \t\t old_oid ? oid_to_hex(old_oid) : old_target,\n> \t\t reason);\n>\n> \tfwrite(sb.buf, sb.len, 1, stdout);\n> -\tfree(reason);\n> \tstrbuf_release(&sb);\n> }\n>\n> diff --git a/refs.c b/refs.c\n> index 351ed52deb..953f83bb52 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3315,7 +3315,7 @@ int ref_update_expects_existing_old_ref(struct ref_update *update)\n> \t\t(!is_null_oid(&update->old_oid) || update->old_target);\n> }\n>\n> -char *ref_transaction_error_msg(enum ref_transaction_error err)\n> +const char *ref_transaction_error_msg(enum ref_transaction_error err)\n> {\n> \tconst char *reason = \"\";\n>\n> @@ -3342,5 +3342,5 @@ char *ref_transaction_error_msg(enum ref_transaction_error err)\n> \t\treason = \"unkown failure\";\n> \t}\n>\n> -\treturn xstrdup(reason);\n> +\treturn reason;\n> }\n> diff --git a/refs.h b/refs.h\n> index a0b2e3c43d..2d58af3d88 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -910,7 +910,7 @@ void ref_transaction_for_each_rejected_update(struct ref_transaction *transactio\n> /*\n> * Translate errors to human readable error messages.\n> */\n> -char *ref_transaction_error_msg(enum ref_transaction_error err);\n> +const char *ref_transaction_error_msg(enum ref_transaction_error err);\n>\n> /*\n> * Free `*transaction` and all associated data.\n>\n> -Peff\n"},{"id":"518362","messageId":"CAOLa=ZT4LnyG33=atULuFxoCa2V5rO70aCmMQYsxzhieQDp5iA@mail.gmail.com","threadId":"63455","inReplyTo":"aCcM0QK6QBdWO2jj@pks.im","subject":"Re: [PATCH v2 2/4] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-18T11:30:24Z","receivedAt":"2025-05-18T11:30:26Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Fri, May 16, 2025 at 02:53:22AM -0700, Karthik Nayak wrote:\n>> Patrick Steinhardt writes:\n>>\n>> > On Thu, May 15, 2025 at 04:07:26PM +0200, Karthik Nayak wrote:\n>> >> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> >> index 5279997c96..15eac2b1c2 100644\n>> >> --- a/builtin/fetch.c\n>> >> +++ b/builtin/fetch.c\n>> >> @@ -1688,6 +1644,37 @@ static int set_head(const struct ref *remote_refs, struct remote *remote)\n>> >> \treturn result;\n>> >> }\n>> >>\n>> >> +struct ref_rejection_data {\n>> >> +\tint *retcode;\n>> >> +\tint conflict_msg_shown;\n>> >> +\tconst char *remote_name;\n>> >> +};\n>> >> +\n>> >> +static void ref_transaction_rejection_handler(const char *refname,\n>> >> +\t\t\t\t\t const struct object_id *old_oid UNUSED,\n>> >> +\t\t\t\t\t const struct object_id *new_oid UNUSED,\n>> >> +\t\t\t\t\t const char *old_target UNUSED,\n>> >> +\t\t\t\t\t const char *new_target UNUSED,\n>> >> +\t\t\t\t\t enum ref_transaction_error err,\n>> >> +\t\t\t\t\t void *cb_data)\n>> >> +{\n>> >> +\tstruct ref_rejection_data *data = (struct ref_rejection_data *)cb_data;\n>> >\n>> > Nit: unnecessary cast.\n>> >\n>> >> +\tif (err == REF_TRANSACTION_ERROR_NAME_CONFLICT && !data->conflict_msg_shown) {\n>> >> +\t\terror(_(\"some local refs could not be updated; try running\\n\"\n>> >> +\t\t\t\" 'git remote prune %s' to remove any old, conflicting \"\n>> >> +\t\t\t\"branches\"), data->remote_name);\n>> >> +\t\tdata->conflict_msg_shown = 1;\n>> >> +\t} else {\n>> >> +\t\tchar *reason = ref_transaction_error_msg(err);\n>> >> +\n>> >> +\t\terror(_(\"fetching ref %s failed: %s\"), refname, reason);\n>> >> +\t\tfree(reason);\n>> >> +\t}\n>> >> +\n>> >> +\t*data->retcode = 1;\n>> >> +}\n>> >\n>> > Okay, we stopped ignoring generic errors now and will print them. What\n>> > I'm still unclear about: which exact errors do we accept now that\n>> > `REF_TRANSACTION_ALLOW_FAILURE` is specified? Most of the error codes we\n>> > probably want to accept, but what about `REF_TRANSACTION_ERROR_GENERIC`?\n>>\n>> The current mechanism in `ref_transaction_maybe_set_rejected()` doesn't\n>> handle `REF_TRANSACTION_ERROR_GENERIC` errors. This was a design choice\n>> (more of a requirement of what this error represents), where\n>> `REF_TRANSACTION_ERROR_GENERIC` errors cannot be resolved on an\n>> individual reference level. It includes:\n>>\n>> - System errors such as I/O errors\n>> - Duplicates present\n>>\n>> Both of these represent issues which are bigger than a single ref\n>> update, so we have to propagate these errors up.\n>\n> The second case is also why the behaviour changes now, right? If we were\n> able to handle duplicates via the same mechanism then it would become\n> possible to retain current behaviour for git-receive-pack(1)?\n>\n\nYeah indeed, but if we want to allow users supporting conflict\nresolution of duplicates, we'll also have to think about how that would\nlook. Our discussion till now was around allowing a callback for each\nreference update with the associated error.\n\nWith duplicates, we'd also want to provide the context of which N\nupdates are duplicated.\n\n> Not that I'm proposing this -- I very much think that the current\n> behaviour in git-receive-pack(1) is a bug that should be fixed. Mostly\n> trying to understand.\n>\n\nYeah I agree with you on this!\n\n>> > This makes me wonder a bit about the current layout of how we handle\n>> > these errors. If the rejection handler was invoked while preparing the\n>> > transaction for each reference as we go instead of afterwards we could\n>> > decide on-the-fly whether a specific error should be ignored or not.\n>> > That might lead to a design that is both more flexible and more obvious\n>> > at the same time because error handling is now handled explicitly by the\n>> > callsite that wants to ignore some errors.\n>> >\n>>\n>> I did ponder on this while I was building the batched transaction\n>> mechanism. I decided to take it iteratively. We can, for instance,\n>> modify `ref_transaction_maybe_set_rejected()` to work with a callback\n>> function which would allow the users to accept/reject errors.\n>>\n>> However, even if we go down that route, `REF_TRANSACTION_ERROR_GENERIC`\n>> errors still cannot be overlooked, these errors will abort the entire\n>> transaction.\n>\n> Okay, good.\n>\n>> That said, I'm not trying to avoid going down that route. I do agree\n>> with the flexibility it does provide. Once we hit such a usecase, we\n>> should make that change.\n>>\n>> For 'git-fetch(1)' and 'git-recieve-pack(1)', do you see a usecase?\n>\n> No, I don't right now. I just want to avoid that we have to eventually\n> refactor all of this to support an alternative API. But agreed, there\n> isn't really much of a reason why we wouldn't be able to introduce such\n> a mechanism retroactively while keeping existing callers intact.\n>\n> So let's stick with what we have and keep this in the back of our minds\n> if we ever need such a mechanism going forward.\n>\n\nI think this makes sense!\n\n>> > Last but not least, I think that it would also allow us to decide ahead\n>> > of time whether we want to commit. Right now we basically say \"just\n>> > commit it, whatever happens\". But if I'm not mistaken, all the errors\n>> > that we care about and that callers may want to ignore are already\n>> > detected at prepare time. So if we already bubbled up relevant info\n>> > while calling `ref_transaction_prepare()` the caller may then decide to\n>> > not commit at all based on some criteria.\n>> >\n>>\n>> Indeed, that is correct. I can confirm that even now all the calls to\n>> `ref_transaction_maybe_set_rejected()` are made in the prepare phase, so\n>> we could already do this, since `transaction->rejections` is already\n>> populated at this stage.\n>\n> Good. After all, we shouldn't have to perform checks in the \"commit\"\n> phase. Things are locked, things have been checked, so it should\n> basically be a mere \"let's move everything into place now\". Which of\n> course can still fail, but the only valid reason should be system\n> failures.\n>\n> Patrick\n>\n\nExactly, totally agreed.\n\nThanks for the discussion Patrick!\n"}]}