{"thread":{"id":"63480","subject":"[PATCH v3 1/4] refs: add function to translate errors to strings","startedAt":"2025-05-19T09:58:17Z","lastAt":"2025-05-22T15:31:38Z","messageCount":11,"participants":["Karthik Nayak","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":4},"messages":[{"id":"518400","messageId":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","threadId":"63480","inReplyTo":"20250514-501-update-git-fetch-1-to-use-partial-transactions-v1-0-7c65f46493d4@gmail.com","subject":"[PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-19T09:58:05Z","receivedAt":"2025-05-19T09:58:17Z","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 v3:\n- Modify `ref_transaction_error_msg()` to no longer provided an\n  allocated string, but rather a 'const char *' string literal, this\n  cleans up unnecessary allocation/free calls.\n- Remove some unnecessary type casts.\n- Fix some typos.\n- Link to v2: https://lore.kernel.org/r/20250515-501-update-git-fetch-1-to-use-partial-transactions-v2-0-80cbaaa55d2e@gmail.com\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                  | 127 ++++++++++++++++++++++-----------------\n builtin/receive-pack.c           |  64 +++++++++++++++-----\n builtin/update-ref.c             |  25 +-------\n refs.c                           |  20 ++++++\n refs.h                           |   5 ++\n send-pack.c                      |   7 +++\n t/t1416-ref-transaction-hooks.sh |   2 -\n t/t5408-send-pack-stdin.sh       |  15 ++++-\n 8 files changed, 166 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 v2:\n\n1:  5196124be4 ! 1:  5bd1ffe4d2 refs: add function to translate errors to strings\n    @@ Commit message\n         to 'refs.c' as `ref_transaction_error_msg()` and use the same within the\n         'builtin/update-ref.c'.\n     \n    +    Helped-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n      ## builtin/update-ref.c ##\n    @@ builtin/update-ref.c: static void print_rejected_refs(const char *refname,\n     -\tdefault:\n     -\t\treason = \"unkown failure\";\n     -\t}\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    -@@ builtin/update-ref.c: 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    - \n     \n      ## refs.c ##\n     @@ refs.c: int ref_update_expects_existing_old_ref(struct ref_update *update)\n    @@ refs.c: 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     +\tswitch (err) {\n     +\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n    -+\t\treason = \"refname conflict\";\n    -+\t\tbreak;\n    ++\t\treturn \"refname conflict\";\n     +\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n    -+\t\treason = \"reference already exists\";\n    -+\t\tbreak;\n    ++\t\treturn \"reference already exists\";\n     +\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n    -+\t\treason = \"reference does not exist\";\n    -+\t\tbreak;\n    ++\t\treturn \"reference does not exist\";\n     +\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n    -+\t\treason = \"incorrect old value provided\";\n    -+\t\tbreak;\n    ++\t\treturn \"incorrect old value provided\";\n     +\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n    -+\t\treason = \"invalid new value provided\";\n    -+\t\tbreak;\n    ++\t\treturn \"invalid new value provided\";\n     +\tcase REF_TRANSACTION_ERROR_EXPECTED_SYMREF:\n    -+\t\treason = \"expected symref but found regular ref\";\n    -+\t\tbreak;\n    ++\t\treturn \"expected symref but found regular ref\";\n     +\tdefault:\n    -+\t\treason = \"unkown failure\";\n    ++\t\treturn \"unknown failure\";\n     +\t}\n    -+\n    -+\treturn xstrdup(reason);\n     +}\n     \n      ## refs.h ##\n    @@ refs.h: void ref_transaction_for_each_rejected_update(struct ref_transaction *tr\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.\n2:  c2014b959f ! 2:  dadabf1918 fetch: use batched reference updates\n    @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot\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    ++\tstruct ref_rejection_data *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    @@ builtin/fetch.c: static int set_head(const struct ref *remote_refs, struct remot\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    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\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 * and this can be moved above the 'prune_refs()' block.\n     +\t */\n     +\tif (!transaction) {\n     +\t\ttransaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n3:  43f0dd3c31 = 3:  4006905807 send-pack: fix memory leak around duplicate refs\n4:  a976afa025 ! 4:  0549b78b48 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: Jeff King <peff@peff.net>\n         Helped-by: Patrick Steinhardt <ps@pks.im>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n    @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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    @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\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    @@ t/t5408-send-pack-stdin.sh: test_expect_success 'stdin mixed with cmdline' '\n      \techo A:foo >input &&\n      \ttest_must_fail git send-pack remote.git --stdin B:foo <input &&\n     -\tverify_push B foo\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\n\nbase-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0\nchange-id: 20250423-501-update-git-fetch-1-to-use-partial-transactions-7fb66339fe79\n\nThanks\n- Karthik\n\n"},{"id":"518399","messageId":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-1-6cdfd4f769b9@gmail.com","threadId":"63480","inReplyTo":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","subject":"[PATCH v3 1/4] refs: add function to translate errors to strings","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-19T09:58:06Z","receivedAt":"2025-05-19T09:58:18Z","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\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/update-ref.c | 25 +------------------------\n refs.c               | 20 ++++++++++++++++++++\n refs.h               |  5 +++++\n 3 files changed, 26 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/update-ref.c b/builtin/update-ref.c\nindex 2b1e336ba1..1e6131e04a 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+\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,\ndiff --git a/refs.c b/refs.c\nindex 6559db3789..f21778c75f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3314,3 +3314,23 @@ 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+const char *ref_transaction_error_msg(enum ref_transaction_error err)\n+{\n+\tswitch (err) {\n+\tcase REF_TRANSACTION_ERROR_NAME_CONFLICT:\n+\t\treturn \"refname conflict\";\n+\tcase REF_TRANSACTION_ERROR_CREATE_EXISTS:\n+\t\treturn \"reference already exists\";\n+\tcase REF_TRANSACTION_ERROR_NONEXISTENT_REF:\n+\t\treturn \"reference does not exist\";\n+\tcase REF_TRANSACTION_ERROR_INCORRECT_OLD_VALUE:\n+\t\treturn \"incorrect old value provided\";\n+\tcase REF_TRANSACTION_ERROR_INVALID_NEW_VALUE:\n+\t\treturn \"invalid new value provided\";\n+\tcase REF_TRANSACTION_ERROR_EXPECTED_SYMREF:\n+\t\treturn \"expected symref but found regular ref\";\n+\tdefault:\n+\t\treturn \"unknown failure\";\n+\t}\n+}\ndiff --git a/refs.h b/refs.h\nindex 46a6008e07..2d58af3d88 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+const 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":"518401","messageId":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-2-6cdfd4f769b9@gmail.com","threadId":"63480","inReplyTo":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","subject":"[PATCH v3 2/4] fetch: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-19T09:58:07Z","receivedAt":"2025-05-19T09:58:19Z","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 <karthik.188@gmail.com>\n---\n builtin/fetch.c | 127 ++++++++++++++++++++++++++++++++------------------------\n 1 file changed, 73 insertions(+), 54 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 5279997c96..f200194f77 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,36 @@ 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 = 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\tconst char *reason = ref_transaction_error_msg(err);\n+\n+\t\terror(_(\"fetching ref %s failed: %s\"), refname, 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 +1794,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 above 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 +1843,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":"518403","messageId":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-3-6cdfd4f769b9@gmail.com","threadId":"63480","inReplyTo":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","subject":"[PATCH v3 3/4] send-pack: fix memory leak around duplicate refs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-19T09:58:08Z","receivedAt":"2025-05-19T09:58:19Z","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 <karthik.188@gmail.com>\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":"518402","messageId":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-4-6cdfd4f769b9@gmail.com","threadId":"63480","inReplyTo":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","subject":"[PATCH v3 4/4] receive-pack: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-19T09:58:09Z","receivedAt":"2025-05-19T09:58:20Z","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: Jeff King <peff@peff.net>\nHelped-by: Patrick Steinhardt <ps@pks.im>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n builtin/receive-pack.c           | 64 ++++++++++++++++++++++++++++++----------\n t/t1416-ref-transaction-hooks.sh |  2 --\n t/t5408-send-pack-stdin.sh       | 13 ++++----\n 3 files changed, 56 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex be314879e8..2a302c836f 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, (char *)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 = strmap_get(&failed_refs, cmd->ref_name);\n \t}\n+\n+cleanup:\n+\tref_transaction_free(transaction);\n+\tstrmap_clear(&failed_refs, 0);\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..ec339761c2 100755\n--- a/t/t5408-send-pack-stdin.sh\n+++ b/t/t5408-send-pack-stdin.sh\n@@ -69,21 +69,24 @@ 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 <input &&\n-\tverify_push B foo\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 'refspecs and --mirror do not mix (cmdline)' '\n\n-- \n2.49.0\n\n"},{"id":"518443","messageId":"xmqqy0usmprh.fsf@gitster.g","threadId":"63480","inReplyTo":"20250519-501-update-git-fetch-1-to-use-partial-transactions-v3-0-6cdfd4f769b9@gmail.com","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-19T18:14:42Z","receivedAt":"2025-05-19T18:14:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> The git-fetch(1) and git-receive-pack(1) commands update references as\n> part of the flow. Each reference update is treated as a single entity\n> and a transaction is created for each update.\n>\n> This can be really slow, specifically in reference backends where there\n> are optimizations which ensure a single transaction with 'N' reference\n> update perform much faster than 'N' individual transactions. Also having\n> 'N' individual transactions has buildup and teardown costs. These costs\n> add up in repositories with a large number of references.\n>\n> Also specifically in the reftable backend, 'N' individual transactions\n> would also trigger auto-compaction for every transaction.\n>\n> The reasoning for using individual transactions is because we want to\n> allow partial updates of references in these commands. Using a single\n> transaction would be an all-or-nothing scenario.\n>\n> Recently we introduced an in-between solution called batched reference\n> updates in 23fc8e4f61 (refs: implement batch reference update support,\n> 2025-04-08). This allows us to batch a set of reference updates, where\n> individual updates can pass/fail without affecting the batch.\n>\n> This patch series, modifies both 'git-fetch(1)' and\n> 'git-receive-pack(1)' to use this mechanism. With this, we see a\n> significant 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\nVery nice.\n\n> The first and third patch handle the changes for 'git-fetch(1)' and\n> 'git-receive-pack(1)' respectively. The second patch fixes a small\n> memory leak I encountered while working on this series.\n>\n> This is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of\n> https://github.com/j6t/gitk, 2025-05-09). There were no conflicts\n> observed with next or seen.\n>\n> Junio, since the release window for 2.50 is closing in. I would prefer\n> we mark this for 2.51, so perhaps when/if we merge it to 'next', we let\n> it bake there till the next release window opens. While all the tests\n> pass, any bugs here would be high impact and it would be nice to catch\n> it before it hits a release.\n\nI've read the difference since the last iteration, \"git diff @{1}\",\nand everything looked sensible.\n\nNot an issue with this series at all, but one thing I wondered is if\nit makes sense to change the type of strmap_get/strmap_put to deal\nwith \"const void *\".  That way, it would not be necessary to cast\naway the constness like so:\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\nwithout harming the other side, namely\n\n>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n\nthis piece of code.\n\nIt may not make sense, and even if it did, of course, it is totally\noutside of this series.\n\nThanks.\n"},{"id":"518470","messageId":"CAOLa=ZTyE9myZqy8UHe=YeZ-diy0Q1UDTBTB3d1XGC09XvshAQ@mail.gmail.com","threadId":"63480","inReplyTo":"xmqqy0usmprh.fsf@gitster.g","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-20T09:05:09Z","receivedAt":"2025-05-20T09:05:21Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> The git-fetch(1) and git-receive-pack(1) commands update references as\n>> part of the flow. Each reference update is treated as a single entity\n>> and a transaction is created for each update.\n>>\n>> This can be really slow, specifically in reference backends where there\n>> are optimizations which ensure a single transaction with 'N' reference\n>> update perform much faster than 'N' individual transactions. Also having\n>> 'N' individual transactions has buildup and teardown costs. These costs\n>> add up in repositories with a large number of references.\n>>\n>> Also specifically in the reftable backend, 'N' individual transactions\n>> would also trigger auto-compaction for every transaction.\n>>\n>> The reasoning for using individual transactions is because we want to\n>> allow partial updates of references in these commands. Using a single\n>> transaction would be an all-or-nothing scenario.\n>>\n>> Recently we introduced an in-between solution called batched reference\n>> updates in 23fc8e4f61 (refs: implement batch reference update support,\n>> 2025-04-08). This allows us to batch a set of reference updates, where\n>> individual updates can pass/fail without affecting the batch.\n>>\n>> This patch series, modifies both 'git-fetch(1)' and\n>> 'git-receive-pack(1)' to use this mechanism. With this, we see a\n>> significant 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>\n> Very nice.\n>\n>> The first and third patch handle the changes for 'git-fetch(1)' and\n>> 'git-receive-pack(1)' respectively. The second patch fixes a small\n>> memory leak I encountered while working on this series.\n>>\n>> This is based on top of master: 7a1d2bd0a5 (Merge branch 'master' of\n>> https://github.com/j6t/gitk, 2025-05-09). There were no conflicts\n>> observed with next or seen.\n>>\n>> Junio, since the release window for 2.50 is closing in. I would prefer\n>> we mark this for 2.51, so perhaps when/if we merge it to 'next', we let\n>> it bake there till the next release window opens. While all the tests\n>> pass, any bugs here would be high impact and it would be nice to catch\n>> it before it hits a release.\n>\n> I've read the difference since the last iteration, \"git diff @{1}\",\n> and everything looked sensible.\n>\n\nThanks for the review!\n\n> Not an issue with this series at all, but one thing I wondered is if\n> it makes sense to change the type of strmap_get/strmap_put to deal\n> with \"const void *\".  That way, it would not be necessary to cast\n> away the constness like so:\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> without harming the other side, namely\n>\n>>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n>\n> this piece of code.\n>\n> It may not make sense, and even if it did, of course, it is totally\n> outside of this series.\n>\n> Thanks.\n\nIt definitely does, The only other typecast I did find for `strmap_put`\nwas within 'strmap.h'. Nevertheless, I think it makes sense to make that\nchange. strmap shouldn't modify the data provided. Perhaps #leftoverbits.\n\nThanks\n"},{"id":"518565","messageId":"xmqqmsb6ds2b.fsf@gitster.g","threadId":"63480","inReplyTo":"CAOLa=ZTyE9myZqy8UHe=YeZ-diy0Q1UDTBTB3d1XGC09XvshAQ@mail.gmail.com","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-05-21T13:14:20Z","receivedAt":"2025-05-21T13:14:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> Not an issue with this series at all, but one thing I wondered is if\n>> it makes sense to change the type of strmap_get/strmap_put to deal\n>> with \"const void *\".  That way, it would not be necessary to cast\n>> away the constness like so:\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>> without harming the other side, namely\n>>\n>>>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n>>\n>> this piece of code.\n>>\n>> It may not make sense, and even if it did, of course, it is totally\n>> outside of this series.\n>>\n>> Thanks.\n>\n> It definitely does, The only other typecast I did find for `strmap_put`\n> was within 'strmap.h'. Nevertheless, I think it makes sense to make that\n> change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.\n\nOK.  Thanks.\n"},{"id":"518644","messageId":"20250522060044.GB1135327@coredump.intra.peff.net","threadId":"63480","inReplyTo":"CAOLa=ZTyE9myZqy8UHe=YeZ-diy0Q1UDTBTB3d1XGC09XvshAQ@mail.gmail.com","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T06:00:44Z","receivedAt":"2025-05-22T06:00:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:\n\n> > Not an issue with this series at all, but one thing I wondered is if\n> > it makes sense to change the type of strmap_get/strmap_put to deal\n> > with \"const void *\".  That way, it would not be necessary to cast\n> > away the constness like so:\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> > without harming the other side, namely\n> >\n> >>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n> >\n> > this piece of code.\n> >\n> > It may not make sense, and even if it did, of course, it is totally\n> > outside of this series.\n> >\n> > Thanks.\n> \n> It definitely does, The only other typecast I did find for `strmap_put`\n> was within 'strmap.h'. Nevertheless, I think it makes sense to make that\n> change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.\n\nI'm not sure that is a good idea. Even though strmap does not touch the\nvoid data pointer itself, it is accessible to the callers, and we do not\nknow if they stored const data or not, or how they plan to access it.\n\nIf we store a \"const void *\" pointer and returned that via strmap_get(),\nthen there will be callers who need to cast away the constness.\n\nIf we store and return a \"void *\" pointer as we do now, but accept a\nconst pointer via strmap_put(), then we're casting away potentially\nimportant const-ness without the caller even seeing it. I think it's\nsafer for the client to do the cast explicitly (since they are the ones\nwho know how they plan to use it).\n\nI don't think we can really represent what we want in C's type system.\nBut if we wanted a safe(r) interface that didn't involve type-casting,\nwe might be able to do something like:\n\n  - the strmap stores an extra flag for \"the data pointer is const\",\n    which can be set by strmap_init(). (It is tempting to replace\n    strmap_clear()'s free_entries parameter by checking this flag, but\n    the two are not always going to be exactly the same).\n\n  - introduce strmap_get_const() and strmap_put_const() which give and\n    receive const pointers\n\n  - in the const functions, BUG() if the \"pointers are const\" flag is\n    not set\n\nBut it feels gross, and it only gives runtime checking, not\ncompile-time. What we really want are generics that can carry the type\nannotation for a particular instantiated map. That is probably pretty\neasy in most modern languages, but not really in C without horrible\nmacros. :)\n\n-Peff\n"},{"id":"518650","messageId":"CAOLa=ZQhaPXS1y5zkRv2CQ_S5=8+XRg_A1snXHVYXz3uvVgZSg@mail.gmail.com","threadId":"63480","inReplyTo":"20250522060044.GB1135327@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-05-22T08:50:18Z","receivedAt":"2025-05-22T08:50:19Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, May 20, 2025 at 02:05:09AM -0700, Karthik Nayak wrote:\n>\n>> > Not an issue with this series at all, but one thing I wondered is if\n>> > it makes sense to change the type of strmap_get/strmap_put to deal\n>> > with \"const void *\".  That way, it would not be necessary to cast\n>> > away the constness like so:\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>> > without harming the other side, namely\n>> >\n>> >>     @@ builtin/receive-pack.c: static void BUG_if_skipped_connectivity_check(struct com\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\t\tcmd->error_string = strmap_get(&failed_refs, cmd->ref_name);\n>> >\n>> > this piece of code.\n>> >\n>> > It may not make sense, and even if it did, of course, it is totally\n>> > outside of this series.\n>> >\n>> > Thanks.\n>>\n>> It definitely does, The only other typecast I did find for `strmap_put`\n>> was within 'strmap.h'. Nevertheless, I think it makes sense to make that\n>> change. strmap shouldn't modify the data provided. Perhaps #leftoverbits.\n>\n> I'm not sure that is a good idea. Even though strmap does not touch the\n> void data pointer itself, it is accessible to the callers, and we do not\n> know if they stored const data or not, or how they plan to access it.\n>\n> If we store a \"const void *\" pointer and returned that via strmap_get(),\n> then there will be callers who need to cast away the constness.\n>\n> If we store and return a \"void *\" pointer as we do now, but accept a\n> const pointer via strmap_put(), then we're casting away potentially\n> important const-ness without the caller even seeing it. I think it's\n> safer for the client to do the cast explicitly (since they are the ones\n> who know how they plan to use it).\n>\n\nBut isn't that the case now anyways? We always lose the const-ness since\nwe only accept a 'void *'. But by only changing 'strmap_put()' to accept\na 'const void *', but storing and returning a 'void *'. We simply modify\nthe current construct to also say that any data received is not\nmodified. But I do see your point, we'll have to cast there anyways and\nmight as well do it on the client side.\n\n\n> I don't think we can really represent what we want in C's type system.\n> But if we wanted a safe(r) interface that didn't involve type-casting,\n> we might be able to do something like:\n>\n>   - the strmap stores an extra flag for \"the data pointer is const\",\n>     which can be set by strmap_init(). (It is tempting to replace\n>     strmap_clear()'s free_entries parameter by checking this flag, but\n>     the two are not always going to be exactly the same).\n>\n>   - introduce strmap_get_const() and strmap_put_const() which give and\n>     receive const pointers\n>\n>   - in the const functions, BUG() if the \"pointers are const\" flag is\n>     not set\n>\n> But it feels gross, and it only gives runtime checking, not\n> compile-time. What we really want are generics that can carry the type\n> annotation for a particular instantiated map. That is probably pretty\n> easy in most modern languages, but not really in C without horrible\n> macros. :)\n>\n> -Peff\n\nAgreed, and then there is additional load to ensure users will use\n'strmap_put()' and 'strmap_put_const()' as required and simply not cast\naway.\n\nOverall I agree with what you're saying. Thanks for spelling it out.\n\nKarthik\n"},{"id":"518661","messageId":"20250522153136.GA1150962@coredump.intra.peff.net","threadId":"63480","inReplyTo":"CAOLa=ZQhaPXS1y5zkRv2CQ_S5=8+XRg_A1snXHVYXz3uvVgZSg@mail.gmail.com","subject":"Re: [PATCH v3 0/4] fetch/receive: use batched reference updates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-05-22T15:31:36Z","receivedAt":"2025-05-22T15:31:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 22, 2025 at 01:50:18AM -0700, Karthik Nayak wrote:\n\n> > If we store and return a \"void *\" pointer as we do now, but accept a\n> > const pointer via strmap_put(), then we're casting away potentially\n> > important const-ness without the caller even seeing it. I think it's\n> > safer for the client to do the cast explicitly (since they are the ones\n> > who know how they plan to use it).\n> >\n> \n> But isn't that the case now anyways? We always lose the const-ness since\n> we only accept a 'void *'. But by only changing 'strmap_put()' to accept\n> a 'const void *', but storing and returning a 'void *'. We simply modify\n> the current construct to also say that any data received is not\n> modified. But I do see your point, we'll have to cast there anyways and\n> might as well do it on the client side.\n\nRight, there is no winning here in the type system. But by pushing the\ncast onto the calling side, it is more visible and closer to the code\nthat knows how the result will be used.\n\n> > I don't think we can really represent what we want in C's type system.\n> > But if we wanted a safe(r) interface that didn't involve type-casting,\n> > we might be able to do something like:\n> [...]\n> Agreed, and then there is additional load to ensure users will use\n> 'strmap_put()' and 'strmap_put_const()' as required and simply not cast\n> away.\n\nThat part I'm not too worried about, since casts are easy-ish to catch\nand flag in reviews. But I'm not sure if it's materially making the\nworld a better place to have the extra complexity.\n\n-Peff\n"}]}