{"thread":{"id":"63865","subject":"[PATCH 0/4] builtin/remote: rework how remote refs get renamed","startedAt":"2025-07-28T13:08:58Z","lastAt":"2025-08-04T18:24:25Z","messageCount":37,"participants":["Patrick Steinhardt","Junio C Hamano","Justin Tobler","Jeff King","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"522846","messageId":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","threadId":"63865","inReplyTo":null,"subject":"[PATCH 0/4] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T13:08:44Z","receivedAt":"2025-07-28T13:08:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series is the result from the discussion at [1]. On the one\nhand this series fixes the reported bug where dangling symrefs are not\nrenamed via `git remote rename`.\n\nOn the other hand this series reworks the logic used to rename remotes\nso that we use two transactions instead of one transaction per ref. This\nfixes quadratic runtime behaviour, where renaming 10k refs takes ~4\nminutes, 100k takes hours. This results in a significant speedup with\nboth the \"files\" backend (benchmarked with a smaller number of refs to\nretain sanity):\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n Range (min … max): 204.863 s … 247.699 s 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s]\n Range (min … max): 2.011 s … 2.141 s 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~)\n\nFor the \"reftable\" backend we see a significant speedup, as well, but\nnot as extreme as with the \"files\" backend:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s]\n Range (min … max): 7.880 s … 9.556 s 10 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s]\n Range (min … max): 1.023 s … 1.410 s 10 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nBut in any case, it's one more case where the \"reftable\" backend\noutperforms the \"files\" backend.\n\nThe series is built on top of e4ef0485fd7 (The fourteenth batch,\n2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\ninvalid old object IDs when migrating reflogs, 2025-07-25) merged into\nit.\n\nI'd normally have withheld sending until that series was merged to\n\"next\", but given that I promised to send something on Friday already I\ndecided to just get it out. In any case, if that causes problems I'm\nhappy to wait a bit before this series here gets merged into \"seen\".\n\nThanks!\n\nPatrick\n\n[1]: \n\n---\nPatrick Steinhardt (4):\n refs: pass refname when invoking reflog entry callback\n refs: simplify logic when migrating reflog entries\n builtin/remote: rework how remote refs get renamed\n builtin/remote: only iterate through refs that are to be renamed\n\n builtin/fsck.c | 9 +-\n builtin/gc.c | 3 +-\n builtin/remote.c | 275 ++++++++++++++++++++++++++++++----------------\n builtin/stash.c | 6 +-\n commit.c | 3 +-\n object-name.c | 3 +-\n reflog-walk.c | 7 +-\n reflog.c | 3 +-\n reflog.h | 3 +-\n refs.c | 63 +++++------\n refs.h | 3 +\n"},{"id":"522847","messageId":"20250728-pks-remote-rename-improvements-v1-1-f654f2b5c5ae@pks.im","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"[PATCH 1/4] refs: pass refname when invoking reflog entry callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T13:08:45Z","receivedAt":"2025-07-28T13:09:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With `refs_for_each_reflog_ent()` callers can iterate through all the\nreflog entries for a given reference. The callback that is being invoked\nfor each such entry does not receive the name of the reference that we\nare currently iterating through. This isn't really a limiting factor, as\ncallers can simply pass the name via the callback data.\n\nBut this layout sometimes does make for a bit of an awkward calling\npattern. One example: when iterating through all reflogs, and for each\nreflog we iterate through all refnames, we have to do some extra book\nkeeping to track which reference name we are currently yielding reflog\nentries for.\n\nChange the signature of the callback function so that the reference name\nof the reflog gets passed through to it. Adapt callers accordingly and\nstart using the new parameter in trivial cases. The next commit will\nrefactor the reference migration logic to make use of this parameter so\nthat we can simplify its logic a bit.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fsck.c | 9 ++++-----\n builtin/gc.c | 3 ++-\n builtin/stash.c | 6 ++++--\n commit.c | 3 ++-\n object-name.c | 3 ++-\n reflog-walk.c | 7 ++++---\n reflog.c | 3 ++-\n reflog.h | 3 ++-\n refs.c | 19 +++++++++----------\n refs.h | 1 +\n refs/debug.c | 5 +++--\n refs/files-backend.c | 15 +++++++++------\n refs/reftable-backend.c | 2 +-\n remote.c | 6 ++++--\n revision.c | 3 ++-\n t/helper/test-ref-store.c | 3 ++-\n wt-status.c | 6 ++++--\n 17 files changed, 57 insertions(+), 40 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 0084cf7400b..67eb5e4fa0f 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -502,13 +502,12 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n \t}\n }\n \n-static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int fsck_handle_reflog_ent(const char *refname,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz UNUSED,\n-\t\t\t\t const char *message UNUSED, void *cb_data)\n+\t\t\t\t const char *message UNUSED, void *cb_data UNUSED)\n {\n-\tconst char *refname = cb_data;\n-\n \tif (verbose)\n \t\tfprintf_ln(stderr, _(\"Checking reflog %s->%s\"),\n \t\t\t oid_to_hex(ooid), oid_to_hex(noid));\n@@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)\n \tstrbuf_worktree_ref(cb_data, &refname, logname);\n \trefs_for_each_reflog_ent(get_main_ref_store(the_repository),\n \t\t\t\t refname.buf, fsck_handle_reflog_ent,\n-\t\t\t\t refname.buf);\n+\t\t\t\t NULL);\n \tstrbuf_release(&refname);\n \treturn 0;\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex fab8f4dd4f7..9ae87065d35 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -312,7 +312,8 @@ struct count_reflog_entries_data {\n \tsize_t limit;\n };\n \n-static int count_reflog_entries(struct object_id *old_oid, struct object_id *new_oid,\n+static int count_reflog_entries(const char *refname UNUSED,\n+\t\t\t\tstruct object_id *old_oid, struct object_id *new_oid,\n \t\t\t\tconst char *committer, timestamp_t timestamp,\n \t\t\t\tint tz, const char *msg, void *cb_data)\n {\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e2f95cc2ebc..a1ed67661e3 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -738,7 +738,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix,\n \treturn ret;\n }\n \n-static int reject_reflog_ent(struct object_id *ooid UNUSED,\n+static int reject_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid UNUSED,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED,\n@@ -2173,7 +2174,8 @@ struct stash_entry_data {\n \tsize_t count;\n };\n \n-static int collect_stash_entries(struct object_id *old_oid UNUSED,\n+static int collect_stash_entries(const char *refname UNUSED,\n+\t\t\t\t struct object_id *old_oid UNUSED,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\ndiff --git a/commit.c b/commit.c\nindex 15115125c36..7ebd05f3527 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1031,7 +1031,8 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)\n \tcommit->object.flags |= TMP_MARK;\n }\n \n-static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int collect_one_reflog_ent(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *ident UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n \t\t\t\t const char *message UNUSED, void *cbdata)\ndiff --git a/object-name.c b/object-name.c\nindex ddafe7f9b13..9ec192c3731 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1516,7 +1516,8 @@ struct grab_nth_branch_switch_cbdata {\n \tstruct strbuf *sb;\n };\n \n-static int grab_nth_branch_switch(struct object_id *ooid UNUSED,\n+static int grab_nth_branch_switch(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid UNUSED,\n \t\t\t\t struct object_id *noid UNUSED,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex c7070b13b00..4f1ce047498 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -22,9 +22,10 @@ struct complete_reflogs {\n \tint nr, alloc;\n };\n \n-static int read_one_reflog(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+static int read_one_reflog(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n+\t\t\t const char *email, timestamp_t timestamp, int tz,\n+\t\t\t const char *message, void *cb_data)\n {\n \tstruct complete_reflogs *array = cb_data;\n \tstruct reflog_info *item;\ndiff --git a/reflog.c b/reflog.c\nindex 39c205fd26e..2264b3bd605 100644\n--- a/reflog.c\n+++ b/reflog.c\n@@ -492,7 +492,8 @@ void reflog_expiry_cleanup(void *cb_data)\n \tfree_commit_list(cb->mark_list);\n }\n \n-int count_reflog_ent(struct object_id *ooid UNUSED,\n+int count_reflog_ent(const char *refname UNUSED,\n+\t\t struct object_id *ooid UNUSED,\n \t\t struct object_id *noid UNUSED,\n \t\t const char *email UNUSED,\n \t\t timestamp_t timestamp, int tz UNUSED,\ndiff --git a/reflog.h b/reflog.h\nindex 63bb56280f4..44b306c08ae 100644\n--- a/reflog.h\n+++ b/reflog.h\n@@ -63,7 +63,8 @@ void reflog_expiry_prepare(const char *refname, const struct object_id *oid,\n int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email, timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data);\n-int count_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+int count_reflog_ent(const char *refname,\n+\t\t struct object_id *ooid, struct object_id *noid,\n \t\t const char *email, timestamp_t timestamp, int tz,\n \t\t const char *message, void *cb_data);\n int should_expire_reflog_ent_verbose(struct object_id *ooid,\ndiff --git a/refs.c b/refs.c\nindex 4bd80287054..fd9a5f36b20 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1022,7 +1022,6 @@ int is_branch(const char *refname)\n }\n \n struct read_ref_at_cb {\n-\tconst char *refname;\n \ttimestamp_t at_time;\n \tint cnt;\n \tint reccnt;\n@@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,\n \t\t*cb->cutoff_cnt = cb->reccnt;\n }\n \n-static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n+static int read_ref_at_ent(const char *refname,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data)\n@@ -1072,13 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\toidcpy(cb->oid, noid);\n \t\t\tif (!oideq(&cb->ooid, noid))\n \t\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n-\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n+\t\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\t}\n \t\telse if (cb->date == cb->at_time)\n \t\t\toidcpy(cb->oid, noid);\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n-\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n+\t\t\t\trefname, show_date(cb->date, cb->tz,\n \t\t\t\t\t\t DATE_MODE(RFC2822)));\n \t\tcb->reccnt++;\n \t\toidcpy(&cb->ooid, ooid);\n@@ -1094,7 +1094,8 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n-static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,\n+static int read_ref_at_ent_oldest(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz,\n \t\t\t\t const char *message, void *cb_data)\n@@ -1117,7 +1118,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \tstruct read_ref_at_cb cb;\n \n \tmemset(&cb, 0, sizeof(cb));\n-\tcb.refname = refname;\n \tcb.at_time = at_time;\n \tcb.cnt = cnt;\n \tcb.msg = msg;\n@@ -2976,14 +2976,14 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n \n struct reflog_migration_data {\n \tuint64_t index;\n-\tconst char *refname;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\n \tstruct strbuf *errbuf;\n \tstruct strbuf *sb, *name, *mail;\n };\n \n-static int migrate_one_reflog_entry(struct object_id *old_oid,\n+static int migrate_one_reflog_entry(const char *refname,\n+\t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer,\n \t\t\t\t timestamp_t timestamp, int tz,\n@@ -3006,7 +3006,7 @@ static int migrate_one_reflog_entry(struct object_id *old_oid,\n \tstrbuf_reset(data->sb);\n \tstrbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));\n \n-\tret = ref_transaction_update_reflog(data->transaction, data->refname,\n+\tret = ref_transaction_update_reflog(data->transaction, refname,\n \t\t\t\t\t new_oid, old_oid, data->sb->buf,\n \t\t\t\t\t msg, data->index++, data->errbuf);\n \treturn ret;\n@@ -3016,7 +3016,6 @@ static int migrate_one_reflog(const char *refname, void *cb_data)\n {\n \tstruct migration_data *migration_data = cb_data;\n \tstruct reflog_migration_data data = {\n-\t\t.refname = refname,\n \t\t.old_refs = migration_data->old_refs,\n \t\t.transaction = migration_data->transaction,\n \t\t.errbuf = migration_data->errbuf,\ndiff --git a/refs.h b/refs.h\nindex 99b58d0b73c..a39f873b1fe 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -559,6 +559,7 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);\n * functions.\n */\n typedef int each_reflog_ent_fn(\n+\t\tconst char *refname,\n \t\tstruct object_id *old_oid, struct object_id *new_oid,\n \t\tconst char *committer, timestamp_t timestamp,\n \t\tint tz, const char *msg, void *cb_data);\ndiff --git a/refs/debug.c b/refs/debug.c\nindex 485e3079d7a..5e113db307a 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -276,7 +276,8 @@ struct debug_reflog {\n \tvoid *cb_data;\n };\n \n-static int debug_print_reflog_ent(struct object_id *old_oid,\n+static int debug_print_reflog_ent(const char *refname,\n+\t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer, timestamp_t timestamp,\n \t\t\t\t int tz, const char *msg, void *cb_data)\n@@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tif (new_oid)\n \t\toid_to_hex_r(n, new_oid);\n \n-\tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n+\tret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,\n \t\t dbg->cb_data);\n \ttrace_printf_key(&trace_refs,\n \t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3ebe0323d4e..24d0a8ebde0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n+static int show_one_reflog_ent(struct files_ref_store *refs,\n+\t\t\t const char *refname,\n+\t\t\t struct strbuf *sb,\n \t\t\t each_reflog_ent_fn fn, void *cb_data)\n {\n \tstruct object_id ooid, noid;\n@@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n \t\tmessage += 6;\n \telse\n \t\tmessage += 7;\n-\treturn fn(&ooid, &noid, p, timestamp, tz, message, cb_data);\n+\treturn fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);\n }\n \n static char *find_beginning_of_line(char *bob, char *scan)\n@@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n \t\t\t\tscanp = bp;\n \t\t\t\tendp = bp + 1;\n-\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tif (ret)\n \t\t\t\t\tbreak;\n@@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\t * Process it, and we can end the loop.\n \t\t\t\t */\n \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n-\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tbreak;\n \t\t\t}\n@@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \t\treturn -1;\n \n \twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n-\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \tfclose(logfp);\n \tstrbuf_release(&sb);\n \treturn ret;\n@@ -3323,7 +3325,8 @@ struct expire_reflog_cb {\n \t\t dry_run:1;\n };\n \n-static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int expire_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email, timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data)\n {\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 99fafd75ebe..25a1d516184 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,\n \n \tfull_committer = fmt_ident(log->value.update.name, log->value.update.email,\n \t\t\t\t WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);\n-\treturn fn(&old_oid, &new_oid, full_committer,\n+\treturn fn(log->refname, &old_oid, &new_oid, full_committer,\n \t\t log->value.update.time, log->value.update.tz_offset,\n \t\t log->value.update.message, cb_data);\n }\ndiff --git a/remote.c b/remote.c\nindex e965f022f12..db9eea4fa45 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2578,7 +2578,8 @@ struct check_and_collect_until_cb_data {\n };\n \n /* Get the timestamp of the latest entry. */\n-static int peek_reflog(struct object_id *o_oid UNUSED,\n+static int peek_reflog(const char *refname UNUSED,\n+\t\t struct object_id *o_oid UNUSED,\n \t\t struct object_id *n_oid UNUSED,\n \t\t const char *ident UNUSED,\n \t\t timestamp_t timestamp, int tz UNUSED,\n@@ -2589,7 +2590,8 @@ static int peek_reflog(struct object_id *o_oid UNUSED,\n \treturn 1;\n }\n \n-static int check_and_collect_until(struct object_id *o_oid UNUSED,\n+static int check_and_collect_until(const char *refname UNUSED,\n+\t\t\t\t struct object_id *o_oid UNUSED,\n \t\t\t\t struct object_id *n_oid,\n \t\t\t\t const char *ident UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz UNUSED,\ndiff --git a/revision.c b/revision.c\nindex 212ca0de276..0fc1a167a10 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1699,7 +1699,8 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)\n \t}\n }\n \n-static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int handle_one_reflog_ent(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\n \t\t\t\t int tz UNUSED,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 8d9a271845c..b2380d57ba3 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -215,7 +215,8 @@ static int cmd_for_each_reflog(struct ref_store *refs,\n \treturn refs_for_each_reflog(refs, each_reflog, NULL);\n }\n \n-static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,\n+static int each_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *old_oid, struct object_id *new_oid,\n \t\t\t const char *committer, timestamp_t timestamp,\n \t\t\t int tz, const char *msg, void *cb_data UNUSED)\n {\ndiff --git a/wt-status.c b/wt-status.c\nindex 454601afa15..71bd17b610a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -972,7 +972,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)\n \twt_longstatus_print_trailer(s);\n }\n \n-static int stash_count_refs(struct object_id *ooid UNUSED,\n+static int stash_count_refs(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid UNUSED,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n@@ -1664,7 +1665,8 @@ struct grab_1st_switch_cbdata {\n \tstruct object_id noid;\n };\n \n-static int grab_1st_switch(struct object_id *ooid UNUSED,\n+static int grab_1st_switch(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n\n-- \n2.50.1.565.gc32cd1483b.dirty\n\n"},{"id":"522848","messageId":"20250728-pks-remote-rename-improvements-v1-2-f654f2b5c5ae@pks.im","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"[PATCH 2/4] refs: simplify logic when migrating reflog entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T13:08:46Z","receivedAt":"2025-07-28T13:09:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When migrating reflog entries between two storage formats we have to do\nso via two callback-driven functions:\n\n - `migrate_one_reflog()` gets invoked via `refs_for_each_reflog()` to\n first list all available reflogs.\n\n - `migrate_one_reflog_entry()` gets invoked via\n `refs_for_each_reflog_ent()` in `migrate_one_reflog()`.\n\nBefore the preceding commit we didn't have the refname available in\n`migrate_one_reflog_entry()`, which made it necessary to have a separate\nstructure that we pass to the second callback so that we can propagate\nthe refname. Now that `refs_for_each_reflog_ent()` knows to pass the\nrefname to the callback though that indirection isn't necessary anymore.\n\nThere's one catch though: we do have an update index that is also stored\nin the entry-specific callback data. This update index is required so\nthat we can tell the ref backend in which order it should persist the\nreflog entries to disk.\n\nBut that purpose can be trivially achieved by just converting it into a\nglobal counter that is used for all reflog entries, regardless of which\nreference they are for. The ordering will remain the same as both the\nupdate index and the refname is considered when sorting the entries.\n\nMove the index into `struct migration_data` and drop the now-unused\n`struct reflog_migration_data` to simplify the code a bit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 36 ++++++++++--------------------------\n 1 file changed, 10 insertions(+), 26 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fd9a5f36b20..b820c3908bd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2942,6 +2942,7 @@ struct migration_data {\n \tstruct ref_transaction *transaction;\n \tstruct strbuf *errbuf;\n \tstruct strbuf sb, name, mail;\n+\tuint64_t index;\n };\n \n static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,\n@@ -2974,14 +2975,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n \treturn ret;\n }\n \n-struct reflog_migration_data {\n-\tuint64_t index;\n-\tstruct ref_store *old_refs;\n-\tstruct ref_transaction *transaction;\n-\tstruct strbuf *errbuf;\n-\tstruct strbuf *sb, *name, *mail;\n-};\n-\n static int migrate_one_reflog_entry(const char *refname,\n \t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n@@ -2989,7 +2982,7 @@ static int migrate_one_reflog_entry(const char *refname,\n \t\t\t\t timestamp_t timestamp, int tz,\n \t\t\t\t const char *msg, void *cb_data)\n {\n-\tstruct reflog_migration_data *data = cb_data;\n+\tstruct migration_data *data = cb_data;\n \tstruct ident_split ident;\n \tconst char *date;\n \tint ret;\n@@ -2997,17 +2990,17 @@ static int migrate_one_reflog_entry(const char *refname,\n \tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n \t\treturn -1;\n \n-\tstrbuf_reset(data->name);\n-\tstrbuf_add(data->name, ident.name_begin, ident.name_end - ident.name_begin);\n-\tstrbuf_reset(data->mail);\n-\tstrbuf_add(data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\tstrbuf_reset(&data->name);\n+\tstrbuf_add(&data->name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_reset(&data->mail);\n+\tstrbuf_add(&data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n \n \tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n-\tstrbuf_reset(data->sb);\n-\tstrbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));\n+\tstrbuf_reset(&data->sb);\n+\tstrbuf_addstr(&data->sb, fmt_ident(data->name.buf, data->mail.buf, WANT_BLANK_IDENT, date, 0));\n \n \tret = ref_transaction_update_reflog(data->transaction, refname,\n-\t\t\t\t\t new_oid, old_oid, data->sb->buf,\n+\t\t\t\t\t new_oid, old_oid, data->sb.buf,\n \t\t\t\t\t msg, data->index++, data->errbuf);\n \treturn ret;\n }\n@@ -3015,17 +3008,8 @@ static int migrate_one_reflog_entry(const char *refname,\n static int migrate_one_reflog(const char *refname, void *cb_data)\n {\n \tstruct migration_data *migration_data = cb_data;\n-\tstruct reflog_migration_data data = {\n-\t\t.old_refs = migration_data->old_refs,\n-\t\t.transaction = migration_data->transaction,\n-\t\t.errbuf = migration_data->errbuf,\n-\t\t.sb = &migration_data->sb,\n-\t\t.name = &migration_data->name,\n-\t\t.mail = &migration_data->mail,\n-\t};\n-\n \treturn refs_for_each_reflog_ent(migration_data->old_refs, refname,\n-\t\t\t\t\tmigrate_one_reflog_entry, &data);\n+\t\t\t\t\tmigrate_one_reflog_entry, migration_data);\n }\n \n static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf)\n\n-- \n2.50.1.565.gc32cd1483b.dirty\n\n"},{"id":"522849","messageId":"20250728-pks-remote-rename-improvements-v1-3-f654f2b5c5ae@pks.im","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"[PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T13:08:47Z","receivedAt":"2025-07-28T13:09:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was recently reported [1] that renaming a remote that has dangling\nsymrefs is broken. This issue can be trivially reproduced:\n\n $ git init repo\n Initialized empty Git repository in /tmp/repo/.git/\n $ cd repo/\n $ git remote add origin /dev/null\n $ git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master\n $ git remote rename origin renamed\n $ git symbolic-ref refs/remotes/origin/HEAD\n refs/remotes/origin/master\n $ git symbolic-ref refs/remotes/renamed/HEAD\n fatal: ref refs/remotes/renamed/HEAD is not a symbolic ref\n\nAs one can see, the \"HEAD\" reference did not get renamed but stays in\nthe same place. There are two issues here:\n\n - We use `refs_resolve_ref_unsafe()` to resolve references, but we\n don't pass the `RESOLVE_REF_NO_RECURSE` flag. Consequently, if the\n reference does not resolve, the function will fail and we thus\n ignore this branch.\n\n - We use `refs_for_each_ref()` to iterate through the old remote's\n references, but that function ignores broken references.\n\nBoth of these issues are easy to fix. But having a closer look at the\nlogic that renames remote references surfaces that it leaves a lot to be\ndesired overall.\n\nThe problem is that we're using O(|refs| + |symrefs| * 2) many reference\ntransactions to perform the renames. We first delete all symrefs, then\nindividually rename every direct reference and finally we recreate the\nsymrefs. On the one hand this isn't even remotely an atomic operation,\nso if we hit any error we'll already have deleted all references.\n\nBut more importantly it is also extremely inperformant. The number of\ntransactions for symrefs doesn't really bother us too much, as there\nshould generally only be a single symref anyway (\"HEAD\"). But the\nrenames are very expensive:\n\n - For the \"reftable\" backend we perform auto-compaction after every\n single rename, which does add up.\n\n - For the \"files\" backend we potentially have to rewrite the\n \"packed-refs\" file on every single rename in case they are packed.\n The consequence here is quadratic runtime performance. Renaming a\n 100k references takes hours to complete.\n\nIdeally we'd refactor the code to use a single transaction to perform\nall the reference updates atomically. But unfortunately that does not\nwork in the case where you rename a remote so that it becomes nested\ninto itself, as that can lead to conflicting reference updates.\n\nThe next-best thing is to do it in two transactions: one to delete all\nthe references, and one to recreate the references and their reflogs.\nThis signicantly speeds up the operation with the \"files\" backend. The\nfollowing benchmark renames a remote with 10000 references:\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n Range (min … max): 204.863 s … 247.699 s 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s]\n Range (min … max): 2.011 s … 2.141 s 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~)\n\nFor the \"reftable\" backend we see a significant speedup, as well, but\ngiven that we don't have quadratic runtime behaviour there it's way less\nextreme:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s]\n Range (min … max): 7.880 s … 9.556 s 10 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s]\n Range (min … max): 1.023 s … 1.410 s 10 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nWhile at it, fix the handling of symbolic/broken references by using\n`refs_for_each_rawref()`. Add tests that cover both this reported issue\nand tests that verify that nesting remotes into itself continues to work\nwith conflicting references.\n\nOne thing to note: with this change we cannot provide a proper progress\nmonitor anymore as we queue the references into the transactions as we\niterate through them. Consequently, as we don't know yet how many refs\nthere are in total, we cannot report how many percent of the operation\nis done anymore. But that's a small price to pay considering that you\nnow shouldn't need the progress monitor in most situations at all\nanymore.\n\n[1]: \n\nReported-by: Han Jiang \nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 280 ++++++++++++++++++++++++++++++++++++------------------\n t/t5505-remote.sh | 71 ++++++++++++++\n 2 files changed, 259 insertions(+), 92 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5dd6cbbaeed..b1c55909184 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -3,7 +3,9 @@\n \n #include \"builtin.h\"\n #include \"config.h\"\n+#include \"date.h\"\n #include \"gettext.h\"\n+#include \"ident.h\"\n #include \"parse-options.h\"\n #include \"path.h\"\n #include \"transport.h\"\n@@ -612,36 +614,148 @@ static int add_branch_for_removal(const char *refname,\n struct rename_info {\n \tconst char *old_name;\n \tconst char *new_name;\n-\tstruct string_list *remote_branches;\n-\tuint32_t symrefs_nr;\n+\tstruct ref_transaction *tx_create;\n+\tstruct ref_transaction *tx_delete;\n+\tstruct progress *progress;\n+\tuint32_t progress_nr;\n+\tstruct strbuf *buf1, *buf2, *buf3, *err;\n+\tstruct strbuf *new_refname;\n+\tuint64_t index;\n };\n \n-static int read_remote_branches(const char *refname, const char *referent UNUSED,\n-\t\t\t\tconst struct object_id *oid UNUSED,\n-\t\t\t\tint flags UNUSED, void *cb_data)\n+static void renamed_refname(struct rename_info *rename,\n+\t\t\t const char *refname,\n+\t\t\t struct strbuf *out)\n+{\n+\tstrbuf_reset(out);\n+\tstrbuf_addstr(out, refname);\n+\tstrbuf_splice(out, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n+\t\t rename->new_name, strlen(rename->new_name));\n+}\n+\n+static int rename_one_reflog_entry(const char *old_refname UNUSED,\n+\t\t\t\t struct object_id *old_oid,\n+\t\t\t\t struct object_id *new_oid,\n+\t\t\t\t const char *committer,\n+\t\t\t\t timestamp_t timestamp, int tz,\n+\t\t\t\t const char *msg, void *cb_data)\n {\n \tstruct rename_info *rename = cb_data;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct string_list_item *item;\n-\tint flag;\n-\tconst char *symref;\n-\n-\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old_name);\n-\tif (starts_with(refname, buf.buf)) {\n-\t\titem = string_list_append(rename->remote_branches, refname);\n-\t\tsymref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t refname, RESOLVE_REF_READING,\n-\t\t\t\t\t\t NULL, &flag);\n-\t\tif (symref && (flag & REF_ISSYMREF)) {\n-\t\t\titem->util = xstrdup(symref);\n-\t\t\trename->symrefs_nr++;\n-\t\t} else {\n-\t\t\titem->util = NULL;\n-\t\t}\n+\tstruct strbuf *identity = rename->buf1;\n+\tstruct strbuf *name = rename->buf2;\n+\tstruct strbuf *mail = rename->buf3;\n+\tstruct ident_split ident;\n+\tconst char *date;\n+\tint error;\n+\n+\tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n+\t\treturn -1;\n+\n+\tstrbuf_reset(name);\n+\tstrbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_reset(mail);\n+\tstrbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\n+\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n+\tstrbuf_reset(identity);\n+\tstrbuf_addstr(identity, fmt_ident(name->buf, mail->buf,\n+\t\t\t\t\t WANT_BLANK_IDENT, date, 0));\n+\n+\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n+\t\t\t\t\t new_oid, old_oid, identity->buf, msg,\n+\t\t\t\t\t rename->index++, rename->err);\n+\n+\treturn error;\n+}\n+\n+static int rename_one_reflog(const char *old_refname,\n+\t\t\t const struct object_id *old_oid,\n+\t\t\t struct rename_info *rename)\n+{\n+\tstruct strbuf *message = rename->buf1;\n+\tint error;\n+\n+\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n+\t\treturn 0;\n+\n+\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n+\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n+\tif (error < 0)\n+\t\treturn error;\n+\n+\t/*\n+\t * Manually write the reflog entry for the now-renamed ref. We cannot\n+\t * rely on `rename_one_ref()` to do this for us as that would screw\n+\t * over order in which reflog entries are being written.\n+\t *\n+\t * Furthermore, we only append the entry in case the reference\n+\t * resolves. Missing references shouldn't have reflogs anyway.\n+\t */\n+\tstrbuf_reset(message);\n+\tstrbuf_addf(message, \"remote: renamed %s to %s\", old_refname,\n+\t\t rename->new_refname->buf);\n+\n+\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n+\t\t\t\t\t old_oid, old_oid, git_committer_info(0),\n+\t\t\t\t\t message->buf, rename->index++, rename->err);\n+\tif (error < 0)\n+\t\treturn error;\n+\n+\treturn error;\n+}\n+\n+static int rename_one_ref(const char *old_refname, const char *referent,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int flags, void *cb_data)\n+{\n+\tstruct rename_info *rename = cb_data;\n+\tstruct strbuf *new_referent = rename->buf1;\n+\tconst char *ptr = old_refname;\n+\tint error;\n+\n+\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n+\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n+\t !skip_prefix(ptr, \"/\", &ptr)) {\n+\t\terror = 0;\n+\t\tgoto out;\n \t}\n-\tstrbuf_release(&buf);\n \n-\treturn 0;\n+\trenamed_refname(rename, old_refname, rename->new_refname);\n+\n+\tif (flags & REF_ISSYMREF) {\n+\t\t/*\n+\t\t * Stupidly enough `referent` is not pointing to the immediate\n+\t\t * target of a symref, but it's the recursively resolved value.\n+\t\t * So symrefs pointing to symrefs would be misresolved, and\n+\t\t * unborn symrefs don't have any value for the `referent` at all.\n+\t\t */\n+\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t\t NULL, NULL);\n+\t\trenamed_refname(rename, referent, new_referent);\n+\t\toid = NULL;\n+\t}\n+\n+\terror = ref_transaction_delete(rename->tx_delete, old_refname,\n+\t\t\t\t oid, referent, REF_NO_DEREF, NULL, rename->err);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\terror = ref_transaction_update(rename->tx_create, rename->new_refname->buf, oid, null_oid(the_hash_algo),\n+\t\t\t\t (flags & REF_ISSYMREF) ? new_referent->buf : NULL, NULL,\n+\t\t\t\t REF_SKIP_CREATE_REFLOG | REF_NO_DEREF | REF_SKIP_OID_VERIFICATION,\n+\t\t\t\t NULL, rename->err);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\terror = rename_one_reflog(old_refname, oid, rename);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\tdisplay_progress(rename->progress, ++rename->progress_nr);\n+\n+out:\n+\treturn error;\n }\n \n static int migrate_file(struct remote *remote)\n@@ -730,7 +844,6 @@ static void handle_push_default(const char* old_name, const char* new_name)\n \tstrbuf_release(&push_default.origin);\n }\n \n-\n static int mv(int argc, const char **argv, const char *prefix,\n \t struct repository *repo UNUSED)\n {\n@@ -741,11 +854,15 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t};\n \tstruct remote *oldremote, *newremote;\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n-\t\told_remote_context = STRBUF_INIT;\n-\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n-\tstruct rename_info rename;\n-\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n-\tstruct progress *progress = NULL;\n+\t\told_remote_context = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct rename_info rename = {\n+\t\t.buf1 = &buf,\n+\t\t.buf2 = &buf2,\n+\t\t.buf3 = &buf3,\n+\t\t.new_refname = &old_remote_context,\n+\t\t.err = &err,\n+\t};\n+\tint i, refspec_updated = 0;\n \tint result = 0;\n \n \targc = parse_options(argc, argv, prefix, options,\n@@ -756,8 +873,6 @@ static int mv(int argc, const char **argv, const char *prefix,\n \n \trename.old_name = argv[0];\n \trename.new_name = argv[1];\n-\trename.remote_branches = &remote_branches;\n-\trename.symrefs_nr = 0;\n \n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -832,79 +947,60 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\tgoto out;\n \n \t/*\n-\t * First remove symrefs, then rename the rest, finally create\n-\t * the new symrefs.\n+\t * Note that we're using two transactions to rename the references:\n+\t *\n+\t * - One transaction contains all deletions for references part of\n+\t * the old remote.\n+\t * - One transaction contains all creations of references and reflogs\n+\t * part of the new remote.\n+\t *\n+\t * This split is required to avoid conflicting ref updates when a\n+\t * remote is being nested into itself or converted into its parent\n+\t * directory.\n+\t *\n+\t * Unfortunately this means that the operation isn't atomic. But we\n+\t * cannot avoid that, unless transactions learn to handle such\n+\t * conflicts one day.\n \t */\n-\trefs_for_each_ref(get_main_ref_store(the_repository),\n-\t\t\t read_remote_branches, &rename);\n-\tif (show_progress) {\n-\t\t/*\n-\t\t * Count symrefs twice, since \"renaming\" them is done by\n-\t\t * deleting and recreating them in two separate passes.\n-\t\t */\n-\t\tprogress = start_progress(the_repository,\n-\t\t\t\t\t _(\"Renaming remote references\"),\n-\t\t\t\t\t rename.remote_branches->nr + rename.symrefs_nr);\n-\t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n-\t\tstruct strbuf referent = STRBUF_INIT;\n+\trename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t 0, &err);\n+\tif (!rename.tx_delete)\n+\t\tgoto out;\n \n-\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n-\t\t\t\t\t &referent))\n-\t\t\tcontinue;\n-\t\tif (refs_delete_ref(get_main_ref_store(the_repository), NULL, item->string, NULL, REF_NO_DEREF))\n-\t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n+\trename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t 0, &err);\n+\tif (!rename.tx_create)\n+\t\tgoto out;\n \n-\t\tstrbuf_release(&referent);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n-\t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n+\tif (show_progress)\n+\t\trename.progress = start_delayed_progress(the_repository,\n+\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n \n-\t\tif (item->util)\n-\t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n-\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addf(&buf2, \"remote: renamed %s to %s\",\n-\t\t\t\titem->string, buf.buf);\n-\t\tif (refs_rename_ref(get_main_ref_store(the_repository), item->string, buf.buf, buf2.buf))\n-\t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n-\t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n+\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n+\t\t\t\t rename_one_ref, &rename);\n+\tif (result < 0)\n+\t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n \n-\t\tif (!item->util)\n-\t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n-\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addstr(&buf2, item->util);\n-\t\tstrbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf3);\n-\t\tstrbuf_addf(&buf3, \"remote: renamed %s to %s\",\n-\t\t\t\titem->string, buf.buf);\n-\t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf))\n-\t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n-\t}\n-\tstop_progress(&progress);\n+\tresult = ref_transaction_commit(rename.tx_delete, &err);\n+\tif (result < 0)\n+\t\tdie(_(\"deleting old remote refs failed: %s\"), rename.err->buf);\n+\n+\tresult = ref_transaction_commit(rename.tx_create, &err);\n+\tif (result < 0)\n+\t\tdie(_(\"committing new remote refs failed: %s\"), rename.err->buf);\n+\n+\tstop_progress(&rename.progress);\n \n \thandle_push_default(rename.old_name, rename.new_name);\n \n out:\n-\tstring_list_clear(&remote_branches, 1);\n+\tref_transaction_free(rename.tx_create);\n+\tref_transaction_free(rename.tx_delete);\n \tstrbuf_release(&old_remote_context);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n \tstrbuf_release(&buf3);\n+\tstrbuf_release(&err);\n \treturn result;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 2701eef85e9..f4a407b022d 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1658,4 +1658,75 @@ test_expect_success 'forbid adding superset of existing remote' '\n \ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n '\n \n+test_expect_success 'rename handles unborn HEAD' '\n+\ttest_when_finished \"git remote remove unborn-renamed\" &&\n+\tgit remote add unborn url &&\n+\tgit symbolic-ref refs/remotes/unborn/HEAD refs/remotes/unborn/nonexistent &&\n+\tgit remote rename unborn unborn-renamed &&\n+\tgit symbolic-ref refs/remotes/unborn-renamed/HEAD >actual &&\n+\techo refs/remotes/unborn-renamed/nonexistent >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can nest a remote into itself' '\n+\ttest_commit parent-commit &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent || true\" &&\n+\tgit remote add parent url &&\n+\tgit update-ref refs/remotes/parent/branch $COMMIT_ID &&\n+\ttest_when_finished \"git remote remove parent/child\" &&\n+\tgit remote rename parent parent/child &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/branch\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can nest a remote into itself with a conflicting branch name' '\n+\ttest_commit parent-conflict &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent || true\" &&\n+\tgit remote add parent url &&\n+\tgit update-ref refs/remotes/parent/child $COMMIT_ID &&\n+\ttest_when_finished \"git remote remove parent/child\" &&\n+\tgit remote rename parent parent/child &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/child\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can unnest a remote' '\n+\ttest_commit parent-child-commit &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent/child || true\" &&\n+\tgit remote add parent/child url &&\n+\tgit update-ref refs/remotes/parent/child/branch $COMMIT_ID &&\n+\tgit remote rename parent/child parent &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/branch\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename moves around the reflog' '\n+\ttest_commit reflog-old &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git remote remove reflog-old || true\" &&\n+\tgit remote add reflog-old url &&\n+\tgit update-ref refs/remotes/reflog-old/branch $COMMIT_ID &&\n+\ttest-tool ref-store main for-each-reflog >actual &&\n+\ttest_grep refs/remotes/reflog-old/branch actual &&\n+\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-old/branch >reflog-entries-old &&\n+\ttest_line_count = 1 reflog-entries-old &&\n+\tgit remote rename reflog-old reflog-new &&\n+\ttest-tool ref-store main for-each-reflog >actual &&\n+\ttest_grep ! refs/remotes/reflog-old actual &&\n+\ttest_grep refs/remotes/reflog-new/branch actual &&\n+\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-new/branch >reflog-entries-new &&\n+\tcat >expect <<-EOF &&\n+\t$(cat reflog-entries-old)\n+\t$COMMIT_ID $COMMIT_ID $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1112912173 -0700\tremote: renamed refs/remotes/reflog-old/branch to refs/remotes/reflog-new/branch\n+\tEOF\n+\ttest_cmp expect reflog-entries-new\n+'\n+\n test_done\n\n-- \n2.50.1.565.gc32cd1483b.dirty\n\n"},{"id":"522850","messageId":"20250728-pks-remote-rename-improvements-v1-4-f654f2b5c5ae@pks.im","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"[PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T13:08:48Z","receivedAt":"2025-07-28T13:09:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When renaming a remote we also need to rename all references\naccordingly. But while we only need to rename references that are\ncontained in the \"refs/remotes/$OLDNAME/\" namespace, we end up using\n`refs_for_each_rawref()` that iterates through _all_ references. We know\nto exit early in the callback in case we see an irrelevant reference,\nbut ultimately this is still a waste of compute as we knowingly iterate\nthrough references that we won't ever care about.\n\nImprove this by introducing `refs_for_each_rawref_in()`, which knows to\nonly iterate through (potentially broken) references in a given prefix.\n\nThe following benchmark renames a remote with a single reference in a\nrepository that has 100k unrelated references. This shows a sizeable\nimprovement with the \"files\" backend:\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 42.6 ms ± 0.9 ms [User: 29.1 ms, System: 8.4 ms]\n Range (min … max): 40.1 ms … 43.3 ms 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 31.7 ms ± 4.0 ms [User: 19.6 ms, System: 6.9 ms]\n Range (min … max): 27.1 ms … 36.0 ms 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 1.35 ± 0.17 times faster than rename remote (refformat = files, revision = HEAD~)\n\nThe \"reftable\" backend shows roughly the same absolute improvement, but\ngiven that it's already significantly faster than the \"files\" backend\nthis translates to a much larger relative improvement:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 18.2 ms ± 0.5 ms [User: 12.7 ms, System: 3.0 ms]\n Range (min … max): 17.3 ms … 21.4 ms 110 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 8.8 ms ± 0.5 ms [User: 3.8 ms, System: 2.9 ms]\n Range (min … max): 7.5 ms … 9.9 ms 167 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 2.07 ± 0.12 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 15 +++++----------\n refs.c | 8 +++++++-\n refs.h | 2 ++\n 3 files changed, 14 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex b1c55909184..11981f732bc 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -710,16 +710,8 @@ static int rename_one_ref(const char *old_refname, const char *referent,\n {\n \tstruct rename_info *rename = cb_data;\n \tstruct strbuf *new_referent = rename->buf1;\n-\tconst char *ptr = old_refname;\n \tint error;\n \n-\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n-\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n-\t !skip_prefix(ptr, \"/\", &ptr)) {\n-\t\terror = 0;\n-\t\tgoto out;\n-\t}\n-\n \trenamed_refname(rename, old_refname, rename->new_refname);\n \n \tif (flags & REF_ISSYMREF) {\n@@ -976,8 +968,11 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\trename.progress = start_delayed_progress(the_repository,\n \t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n \n-\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n-\t\t\t\t rename_one_ref, &rename);\n+\tstrbuf_reset(&buf);\n+\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename.old_name);\n+\n+\tresult = refs_for_each_rawref_in(get_main_ref_store(the_repository), buf.buf,\n+\t\t\t\t\t rename_one_ref, &rename);\n \tif (result < 0)\n \t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n \ndiff --git a/refs.c b/refs.c\nindex b820c3908bd..861a0deb924 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1840,7 +1840,13 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(refs, \"\", NULL, fn, 0,\n+\treturn refs_for_each_rawref_in(refs, \"\", fn, cb_data);\n+}\n+\n+int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n+\t\t\t each_ref_fn fn, void *cb_data)\n+{\n+\treturn do_for_each_ref(refs, prefix, NULL, fn, 0,\n \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \ndiff --git a/refs.h b/refs.h\nindex a39f873b1fe..9decd3126e3 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -428,6 +428,8 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \n /* can be used to learn about broken ref and symref */\n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n+int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n+\t\t\t each_ref_fn fn, void *cb_data);\n \n /*\n * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.\n\n-- \n2.50.1.565.gc32cd1483b.dirty\n\n"},{"id":"522861","messageId":"xmqqzfcowcat.fsf@gitster.g","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 0/4] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T15:43:54Z","receivedAt":"2025-07-28T15:43:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On the other hand this series reworks the logic used to rename remotes\n> so that we use two transactions instead of one transaction per ref. This\n> fixes quadratic runtime behaviour, where renaming 10k refs takes ~4\n> minutes, 100k takes hours. This results in a significant speedup with\n> both the \"files\" backend (benchmarked with a smaller number of refs to\n> retain sanity):\n\nGreat. Hopefully we will teach transaction mechanism to sort out\nits D/F false-positive bug so that we do not have to risk succeesing\nthe removal half of these two transactions while failing the adding\nhalf of them soonish?\n\n> But in any case, it's one more case where the \"reftable\" backend\n> outperforms the \"files\" backend.\n\n;-).\n\n> The series is built on top of e4ef0485fd7 (The fourteenth batch,\n> 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\n> invalid old object IDs when migrating reflogs, 2025-07-25) merged into\n> it.\n>\n> I'd normally have withheld sending until that series was merged to\n> \"next\", but given that I promised to send something on Friday already I\n> decided to just get it out. In any case, if that causes problems I'm\n> happy to wait a bit before this series here gets merged into \"seen\".\n>\n> Thanks!\n\nGreat. Will queue. If the reflog-migrate-fix needs further work,\nit shouldn't be too hard to rebase this one on my end, either.\n\n"},{"id":"522862","messageId":"vo4s7oj3ypcfnjgky6zlt3zjgqlwqzorvsmac55uajugafnrde@yzb3c4kwbk77","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-1-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-07-28T15:59:48Z","receivedAt":"2025-07-28T16:05:44Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/07/28 03:08PM, Patrick Steinhardt wrote:\n> With `refs_for_each_reflog_ent()` callers can iterate through all the\n> reflog entries for a given reference. The callback that is being invoked\n> for each such entry does not receive the name of the reference that we\n> are currently iterating through. This isn't really a limiting factor, as\n> callers can simply pass the name via the callback data.\n> \n> But this layout sometimes does make for a bit of an awkward calling\n> pattern. One example: when iterating through all reflogs, and for each\n> reflog we iterate through all refnames, we have to do some extra book\n> keeping to track which reference name we are currently yielding reflog\n> entries for.\n\nMaking the reference name part of the callback signature seems\nreasonable here. For the above mentioned example, it will certainly\nsimplify things quite a bit.\n\n> Change the signature of the callback function so that the reference name\n> of the reflog gets passed through to it. Adapt callers accordingly and\n> start using the new parameter in trivial cases. The next commit will\n> refactor the reference migration logic to make use of this parameter so\n> that we can simplify its logic a bit.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/fsck.c | 9 ++++-----\n> builtin/gc.c | 3 ++-\n> builtin/stash.c | 6 ++++--\n> commit.c | 3 ++-\n> object-name.c | 3 ++-\n> reflog-walk.c | 7 ++++---\n> reflog.c | 3 ++-\n> reflog.h | 3 ++-\n> refs.c | 19 +++++++++----------\n> refs.h | 1 +\n> refs/debug.c | 5 +++--\n> refs/files-backend.c | 15 +++++++++------\n> refs/reftable-backend.c | 2 +-\n> remote.c | 6 ++++--\n> revision.c | 3 ++-\n> t/helper/test-ref-store.c | 3 ++-\n> wt-status.c | 6 ++++--\n> 17 files changed, 57 insertions(+), 40 deletions(-)\n> \n[snip]\n> diff --git a/refs.h b/refs.h\n> index 99b58d0b73c..a39f873b1fe 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -559,6 +559,7 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);\n> * functions.\n> */\n> typedef int each_reflog_ent_fn(\n> +\t\tconst char *refname,\n\nThe callback function now propagates the reference name and each\nreference backend is updated accordingly. This patch looks good.\n\n> \t\tstruct object_id *old_oid, struct object_id *new_oid,\n> \t\tconst char *committer, timestamp_t timestamp,\n> \t\tint tz, const char *msg, void *cb_data);\n> diff --git a/refs/debug.c b/refs/debug.c\n> index 485e3079d7a..5e113db307a 100644\n> --- a/refs/debug.c\n> +++ b/refs/debug.c\n> @@ -276,7 +276,8 @@ struct debug_reflog {\n> \tvoid *cb_data;\n> };\n> \n> -static int debug_print_reflog_ent(struct object_id *old_oid,\n> +static int debug_print_reflog_ent(const char *refname,\n> +\t\t\t\t struct object_id *old_oid,\n> \t\t\t\t struct object_id *new_oid,\n> \t\t\t\t const char *committer, timestamp_t timestamp,\n> \t\t\t\t int tz, const char *msg, void *cb_data)\n> @@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n> \tif (new_oid)\n> \t\toid_to_hex_r(n, new_oid);\n> \n> -\tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n> +\tret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,\n> \t\t dbg->cb_data);\n> \ttrace_printf_key(&trace_refs,\n> \t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 3ebe0323d4e..24d0a8ebde0 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,\n> \treturn ret;\n> }\n> \n> -static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n> +static int show_one_reflog_ent(struct files_ref_store *refs,\n> +\t\t\t const char *refname,\n> +\t\t\t struct strbuf *sb,\n> \t\t\t each_reflog_ent_fn fn, void *cb_data)\n> {\n> \tstruct object_id ooid, noid;\n> @@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n> \t\tmessage += 6;\n> \telse\n> \t\tmessage += 7;\n> -\treturn fn(&ooid, &noid, p, timestamp, tz, message, cb_data);\n> +\treturn fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);\n> }\n> \n> static char *find_beginning_of_line(char *bob, char *scan)\n> @@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n> \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n> \t\t\t\tscanp = bp;\n> \t\t\t\tendp = bp + 1;\n> -\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n> +\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n> \t\t\t\tstrbuf_reset(&sb);\n> \t\t\t\tif (ret)\n> \t\t\t\t\tbreak;\n> @@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n> \t\t\t\t * Process it, and we can end the loop.\n> \t\t\t\t */\n> \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n> -\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n> +\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n> \t\t\t\tstrbuf_reset(&sb);\n> \t\t\t\tbreak;\n> \t\t\t}\n> @@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n> \t\treturn -1;\n> \n> \twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n> -\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n> +\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n> \tfclose(logfp);\n> \tstrbuf_release(&sb);\n> \treturn ret;\n> @@ -3323,7 +3325,8 @@ struct expire_reflog_cb {\n> \t\t dry_run:1;\n> };\n> \n> -static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n> +static int expire_reflog_ent(const char *refname UNUSED,\n> +\t\t\t struct object_id *ooid, struct object_id *noid,\n> \t\t\t const char *email, timestamp_t timestamp, int tz,\n> \t\t\t const char *message, void *cb_data)\n> {\n> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\n> index 99fafd75ebe..25a1d516184 100644\n> --- a/refs/reftable-backend.c\n> +++ b/refs/reftable-backend.c\n> @@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,\n> \n> \tfull_committer = fmt_ident(log->value.update.name, log->value.update.email,\n> \t\t\t\t WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);\n> -\treturn fn(&old_oid, &new_oid, full_committer,\n> +\treturn fn(log->refname, &old_oid, &new_oid, full_committer,\n> \t\t log->value.update.time, log->value.update.tz_offset,\n> \t\t log->value.update.message, cb_data);\n> }\n"},{"id":"522863","messageId":"xmqqv7ncwb73.fsf@gitster.g","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-1-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T16:07:44Z","receivedAt":"2025-07-28T16:07:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> -static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,\n> +static int fsck_handle_reflog_ent(const char *refname,\n> +\t\t\t\t struct object_id *ooid, struct object_id *noid,\n> \t\t\t\t const char *email UNUSED,\n> \t\t\t\t timestamp_t timestamp, int tz UNUSED,\n> -\t\t\t\t const char *message UNUSED, void *cb_data)\n> +\t\t\t\t const char *message UNUSED, void *cb_data UNUSED)\n> {\n> -\tconst char *refname = cb_data;\n> -\n> \tif (verbose)\n> \t\tfprintf_ln(stderr, _(\"Checking reflog %s->%s\"),\n> \t\t\t oid_to_hex(ooid), oid_to_hex(noid));\n> @@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)\n> \tstrbuf_worktree_ref(cb_data, &refname, logname);\n> \trefs_for_each_reflog_ent(get_main_ref_store(the_repository),\n> \t\t\t\t refname.buf, fsck_handle_reflog_ent,\n> -\t\t\t\t refname.buf);\n> +\t\t\t\t NULL);\n> \tstrbuf_release(&refname);\n> \treturn 0;\n> }\n\nNice. There are a few callsites that passed refname as (a part of)\ncb_data, like this one ...\n\n> diff --git a/refs.c b/refs.c\n> index 4bd80287054..fd9a5f36b20 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1022,7 +1022,6 @@ int is_branch(const char *refname)\n> }\n> \n> struct read_ref_at_cb {\n> -\tconst char *refname;\n> \ttimestamp_t at_time;\n> \tint cnt;\n> \tint reccnt;\n> @@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,\n> \t\t*cb->cutoff_cnt = cb->reccnt;\n> }\n> \n> -static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n> +static int read_ref_at_ent(const char *refname,\n> +\t\t\t struct object_id *ooid, struct object_id *noid,\n> \t\t\t const char *email UNUSED,\n> \t\t\t timestamp_t timestamp, int tz,\n> \t\t\t const char *message, void *cb_data)\n> @@ -1072,13 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n> \t\t\toidcpy(cb->oid, noid);\n> \t\t\tif (!oideq(&cb->ooid, noid))\n> \t\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n> -\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n> +\t\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n> \t\t}\n\n... that makes quite sense.\n\n"},{"id":"522864","messageId":"b4mvddzjkdv3htup4htigdifp3zydrknhheza4vn3dqkecgx55@vyhj57qanllk","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-2-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 2/4] refs: simplify logic when migrating reflog entries","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-07-28T16:08:10Z","receivedAt":"2025-07-28T16:14:06Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/07/28 03:08PM, Patrick Steinhardt wrote:\n> diff --git a/refs.c b/refs.c\n> index fd9a5f36b20..b820c3908bd 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2942,6 +2942,7 @@ struct migration_data {\n> \tstruct ref_transaction *transaction;\n> \tstruct strbuf *errbuf;\n> \tstruct strbuf sb, name, mail;\n> +\tuint64_t index;\n> };\n> \n> static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,\n> @@ -2974,14 +2975,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n> \treturn ret;\n> }\n> \n> -struct reflog_migration_data {\n> -\tuint64_t index;\n> -\tstruct ref_store *old_refs;\n> -\tstruct ref_transaction *transaction;\n> -\tstruct strbuf *errbuf;\n> -\tstruct strbuf *sb, *name, *mail;\n> -};\n\nWhen I recently was looking at this in [1], I remember finding this\nrather awkward/confusing. Very happy to see it go now in favor of\nsomething much simpler :)\n\n-Justin\n\n[1]: \n"},{"id":"522865","messageId":"xmqqqzy0wakc.fsf@gitster.g","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-2-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 2/4] refs: simplify logic when migrating reflog entries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T16:21:23Z","receivedAt":"2025-07-28T16:21:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Move the index into `struct migration_data` and drop the now-unused\n> `struct reflog_migration_data` to simplify the code a bit.\n\nNice.\n\n> @@ -3015,17 +3008,8 @@ static int migrate_one_reflog_entry(const char *refname,\n> static int migrate_one_reflog(const char *refname, void *cb_data)\n> {\n> \tstruct migration_data *migration_data = cb_data;\n> -\tstruct reflog_migration_data data = {\n> -\t\t.old_refs = migration_data->old_refs,\n> -\t\t.transaction = migration_data->transaction,\n> -\t\t.errbuf = migration_data->errbuf,\n> -\t\t.sb = &migration_data->sb,\n> -\t\t.name = &migration_data->name,\n> -\t\t.mail = &migration_data->mail,\n> -\t};\n\nWe no longer make this copy, which makes sense.\n\n> \treturn refs_for_each_reflog_ent(migration_data->old_refs, refname,\n> -\t\t\t\t\tmigrate_one_reflog_entry, &data);\n> +\t\t\t\t\tmigrate_one_reflog_entry, migration_data);\n> }\n"},{"id":"522868","messageId":"xmqqbjp4w7uu.fsf@gitster.g","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-3-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T17:19:53Z","receivedAt":"2025-07-28T17:19:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> But more importantly it is also extremely inperformant. The number of\n\nIs \"inperformant\" a real word? \"it performs extremely poorly\"?\n\n> +static void renamed_refname(struct rename_info *rename,\n> +\t\t\t const char *refname,\n> +\t\t\t struct strbuf *out)\n> +{\n> +\tstrbuf_reset(out);\n> +\tstrbuf_addstr(out, refname);\n> +\tstrbuf_splice(out, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> +\t\t rename->new_name, strlen(rename->new_name));\n> +}\n> +\n\nThe function name felt somewhat iffy (sounded as if you are letting\na third-party know that you have renamed a ref), but I cannot come\nup with a better alternative X-<.\n\n> +static int rename_one_reflog_entry(const char *old_refname UNUSED,\n> +\t\t\t\t struct object_id *old_oid,\n> +\t\t\t\t struct object_id *new_oid,\n> +\t\t\t\t const char *committer,\n> +\t\t\t\t timestamp_t timestamp, int tz,\n> +\t\t\t\t const char *msg, void *cb_data)\n> {\n> \tstruct rename_info *rename = cb_data;\n\nUsing a name of a system call for an unrelated variable, even if a\nlocal one in a function scope, makes me nauseous. Not a new problem\nintroduced by this change, though.\n\n> +\tstruct strbuf *identity = rename->buf1;\n> +\tstruct strbuf *name = rename->buf2;\n> +\tstruct strbuf *mail = rename->buf3;\n> +\tstruct ident_split ident;\n> +\tconst char *date;\n> +\tint error;\n> +\n> +\tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n> +\t\treturn -1;\n> +\n> +\tstrbuf_reset(name);\n> +\tstrbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);\n> +\tstrbuf_reset(mail);\n> +\tstrbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n> +\n> +\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n> +\tstrbuf_reset(identity);\n> +\tstrbuf_addstr(identity, fmt_ident(name->buf, mail->buf,\n> +\t\t\t\t\t WANT_BLANK_IDENT, date, 0));\n\nIt is somewhat unfortunate that we need to do all of the above only\nso that we can recreate the full ident with the given committer with\na timestamp that is given separately. This probably cannot be helped,\nthough. The backend may not be keeping this information as a single\nstring anyway.\n\n> +static int rename_one_reflog(const char *old_refname,\n> +\t\t\t const struct object_id *old_oid,\n> +\t\t\t struct rename_info *rename)\n> +{\n> +\tstruct strbuf *message = rename->buf1;\n\nAs these temporary strbuf's passed around as part of the rename_info\nstructure are never released or recreated during the run, this is\nsafe, but feels dirty, because we saw rename_one_reflog_entry() uses\nthis exact one for totally different purpose. Perhaps it would make\nit easier to follow if you left \"message\" uninitialized here, before\nrefs_for_each_reflog_ent() returns. And then ...\n\n> +\tint error;\n> +\n> +\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n> +\t\treturn 0;\n> +\n> +\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n> +\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n> +\tif (error < 0)\n> +\t\treturn error;\n> +\n> +\t/*\n> +\t * Manually write the reflog entry for the now-renamed ref. We cannot\n> +\t * rely on `rename_one_ref()` to do this for us as that would screw\n> +\t * over order in which reflog entries are being written.\n> +\t *\n> +\t * Furthermore, we only append the entry in case the reference\n> +\t * resolves. Missing references shouldn't have reflogs anyway.\n> +\t */\n\n... give the \"message\" synonym to rename->buf1 here.\n\n> +\tstrbuf_reset(message);\n> +\tstrbuf_addf(message, \"remote: renamed %s to %s\", old_refname,\n> +\t\t rename->new_refname->buf);\n> +\n> +\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n> +\t\t\t\t\t old_oid, old_oid, git_committer_info(0),\n> +\t\t\t\t\t message->buf, rename->index++, rename->err);\n> +\tif (error < 0)\n> +\t\treturn error;\n> +\n> +\treturn error;\n> +}\n\n> +static int rename_one_ref(const char *old_refname, const char *referent,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t int flags, void *cb_data)\n> +{\n> +\tstruct rename_info *rename = cb_data;\n> +\tstruct strbuf *new_referent = rename->buf1;\n> +\tconst char *ptr = old_refname;\n> +\tint error;\n> +\n> +\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n> +\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n> +\t !skip_prefix(ptr, \"/\", &ptr)) {\n> +\t\terror = 0;\n> +\t\tgoto out;\n> \t}\n> -\tstrbuf_release(&buf);\n> \n> -\treturn 0;\n> +\trenamed_refname(rename, old_refname, rename->new_refname);\n> +\n> +\tif (flags & REF_ISSYMREF) {\n> +\t\t/*\n> +\t\t * Stupidly enough `referent` is not pointing to the immediate\n> +\t\t * target of a symref, but it's the recursively resolved value.\n> +\t\t * So symrefs pointing to symrefs would be misresolved, and\n> +\t\t * unborn symrefs don't have any value for the `referent` at all.\n> +\t\t */\n> +\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n> +\t\t\t\t\t\t NULL, NULL);\n> +\t\trenamed_refname(rename, referent, new_referent);\n> +\t\toid = NULL;\n\nYuck, but this cannot be helped, I guess X-<.\n\n> +\tstruct rename_info rename = {\n> +\t\t.buf1 = &buf,\n> +\t\t.buf2 = &buf2,\n> +\t\t.buf3 = &buf3,\n\nThese can be embedded in the struct, not left as three separate\nstrbuf instances whose addresses are known to this struct, no? We'd\nneed to do strbuf_release() on them at the end anyway, so it would\nnot be a huge deal, though.\n\n> \tstrbuf_release(&buf);\n> \tstrbuf_release(&buf2);\n> \tstrbuf_release(&buf3);\n> +\tstrbuf_release(&err);\n> \treturn result;\n> }\n"},{"id":"522869","messageId":"xmqq7bzsw6r4.fsf@gitster.g","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-4-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T17:43:43Z","receivedAt":"2025-07-28T17:43:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> When renaming a remote we also need to rename all references\n> accordingly. But while we only need to rename references that are\n> contained in the \"refs/remotes/$OLDNAME/\" namespace, we end up using\n> `refs_for_each_rawref()` that iterates through _all_ references. We know\n> to exit early in the callback in case we see an irrelevant reference,\n> but ultimately this is still a waste of compute as we knowingly iterate\n> through references that we won't ever care about.\n\nVery true. Even if we guarantee that callbacks are fed references in\nlexicographic order, that would only allow us to stop iterationg\nwhen we see an irrelevant one\", and still would force us to skip\nover all the irrelevant ones until we see the first one in scope.\n\nIt makes perfect sense to give us a way to say \"iterate only in this\nsubhierarchy\" like this step does.\n\nNice.\n"},{"id":"522873","messageId":"j75h4trgwn5dvvxy6cqjv6pbh5ernehvnoi5itqr3icogebegp@ldbsvfwtxoeo","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-3-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-07-28T18:47:13Z","receivedAt":"2025-07-28T18:53:09Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 25/07/28 03:08PM, Patrick Steinhardt wrote:\n> It was recently reported [1] that renaming a remote that has dangling\n> symrefs is broken. This issue can be trivially reproduced:\n> \n> $ git init repo\n> Initialized empty Git repository in /tmp/repo/.git/\n> $ cd repo/\n> $ git remote add origin /dev/null\n> $ git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master\n> $ git remote rename origin renamed\n> $ git symbolic-ref refs/remotes/origin/HEAD\n> refs/remotes/origin/master\n> $ git symbolic-ref refs/remotes/renamed/HEAD\n> fatal: ref refs/remotes/renamed/HEAD is not a symbolic ref\n> \n> As one can see, the \"HEAD\" reference did not get renamed but stays in\n> the same place. There are two issues here:\n> \n> - We use `refs_resolve_ref_unsafe()` to resolve references, but we\n> don't pass the `RESOLVE_REF_NO_RECURSE` flag. Consequently, if the\n> reference does not resolve, the function will fail and we thus\n> ignore this branch.\n> \n> - We use `refs_for_each_ref()` to iterate through the old remote's\n> references, but that function ignores broken references.\n> \n> Both of these issues are easy to fix. But having a closer look at the\n> logic that renames remote references surfaces that it leaves a lot to be\n> desired overall.\n> \n> The problem is that we're using O(|refs| + |symrefs| * 2) many reference\n> transactions to perform the renames. We first delete all symrefs, then\n> individually rename every direct reference and finally we recreate the\n> symrefs. On the one hand this isn't even remotely an atomic operation,\n> so if we hit any error we'll already have deleted all references.\n> \n> But more importantly it is also extremely inperformant. The number of\n\ns/inperformant/inefficient/\n\n> transactions for symrefs doesn't really bother us too much, as there\n> should generally only be a single symref anyway (\"HEAD\"). But the\n> renames are very expensive:\n> \n> - For the \"reftable\" backend we perform auto-compaction after every\n> single rename, which does add up.\n> \n> - For the \"files\" backend we potentially have to rewrite the\n> \"packed-refs\" file on every single rename in case they are packed.\n> The consequence here is quadratic runtime performance. Renaming a\n> 100k references takes hours to complete.\n> \n> Ideally we'd refactor the code to use a single transaction to perform\n> all the reference updates atomically. But unfortunately that does not\n> work in the case where you rename a remote so that it becomes nested\n> into itself, as that can lead to conflicting reference updates.\n\nOk so we use two transactions to avoid D/F conflicts which could be\ncaused by something like: `git remote rename foo foo/bar`.\n\n> The next-best thing is to do it in two transactions: one to delete all\n> the references, and one to recreate the references and their reflogs.\n> This signicantly speeds up the operation with the \"files\" backend. The\n> following benchmark renames a remote with 10000 references:\n\nWe still have the problem of not being fully atomic here since the first\ntransaction could commit and delete the references, but the second\ntransaction fail. While unfortunate, this still leaves us better off\nsince there are fewer transactions being created.\n\n> \n> Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n> Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n> Range (min … max): 204.863 s … 247.699 s 10 runs\n> \n> Benchmark 2: rename remote (refformat = files, revision = HEAD)\n> Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s]\n> Range (min … max): 2.011 s … 2.141 s 10 runs\n> \n> Summary\n> rename remote (refformat = files, revision = HEAD) ran\n> 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~)\n> \n> For the \"reftable\" backend we see a significant speedup, as well, but\n> given that we don't have quadratic runtime behaviour there it's way less\n> extreme:\n> \n> Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n> Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s]\n> Range (min … max): 7.880 s … 9.556 s 10 runs\n> \n> Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n> Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s]\n> Range (min … max): 1.023 s … 1.410 s 10 runs\n> \n> Summary\n> rename remote (refformat = reftable, revision = HEAD) ran\n> 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nNice speedup :)\n\n> While at it, fix the handling of symbolic/broken references by using\n> `refs_for_each_rawref()`. Add tests that cover both this reported issue\n> and tests that verify that nesting remotes into itself continues to work\n> with conflicting references.\n> \n> One thing to note: with this change we cannot provide a proper progress\n> monitor anymore as we queue the references into the transactions as we\n> iterate through them. Consequently, as we don't know yet how many refs\n> there are in total, we cannot report how many percent of the operation\n> is done anymore. But that's a small price to pay considering that you\n> now shouldn't need the progress monitor in most situations at all\n> anymore.\n> \n> [1]: \n> \n> Reported-by: Han Jiang \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/remote.c | 280 ++++++++++++++++++++++++++++++++++++------------------\n> t/t5505-remote.sh | 71 ++++++++++++++\n> 2 files changed, 259 insertions(+), 92 deletions(-)\n> \n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 5dd6cbbaeed..b1c55909184 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -3,7 +3,9 @@\n> \n> #include \"builtin.h\"\n> #include \"config.h\"\n> +#include \"date.h\"\n> #include \"gettext.h\"\n> +#include \"ident.h\"\n> #include \"parse-options.h\"\n> #include \"path.h\"\n> #include \"transport.h\"\n> @@ -612,36 +614,148 @@ static int add_branch_for_removal(const char *refname,\n> struct rename_info {\n> \tconst char *old_name;\n> \tconst char *new_name;\n> -\tstruct string_list *remote_branches;\n> -\tuint32_t symrefs_nr;\n> +\tstruct ref_transaction *tx_create;\n> +\tstruct ref_transaction *tx_delete;\n> +\tstruct progress *progress;\n> +\tuint32_t progress_nr;\n> +\tstruct strbuf *buf1, *buf2, *buf3, *err;\n\nMy understanding is that these strbuf are stored here as an optimization\nso they can be reused. It looks like, in the context of the associated\ncallback though, `buf2` and `buf3` and only used for `name` and `mail`\nrespectively. I wonder if we could be better off calling them that to be\nmore explicit.\n\n> +\tstruct strbuf *new_refname;\n> +\tuint64_t index;\n> };\n> \n> -static int read_remote_branches(const char *refname, const char *referent UNUSED,\n> -\t\t\t\tconst struct object_id *oid UNUSED,\n> -\t\t\t\tint flags UNUSED, void *cb_data)\n> +static void renamed_refname(struct rename_info *rename,\n> +\t\t\t const char *refname,\n> +\t\t\t struct strbuf *out)\n> +{\n> +\tstrbuf_reset(out);\n> +\tstrbuf_addstr(out, refname);\n> +\tstrbuf_splice(out, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> +\t\t rename->new_name, strlen(rename->new_name));\n> +}\n> +\n> +static int rename_one_reflog_entry(const char *old_refname UNUSED,\n> +\t\t\t\t struct object_id *old_oid,\n> +\t\t\t\t struct object_id *new_oid,\n> +\t\t\t\t const char *committer,\n> +\t\t\t\t timestamp_t timestamp, int tz,\n> +\t\t\t\t const char *msg, void *cb_data)\n> {\n> \tstruct rename_info *rename = cb_data;\n> -\tstruct strbuf buf = STRBUF_INIT;\n> -\tstruct string_list_item *item;\n> -\tint flag;\n> -\tconst char *symref;\n> -\n> -\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old_name);\n> -\tif (starts_with(refname, buf.buf)) {\n> -\t\titem = string_list_append(rename->remote_branches, refname);\n> -\t\tsymref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n> -\t\t\t\t\t\t refname, RESOLVE_REF_READING,\n> -\t\t\t\t\t\t NULL, &flag);\n> -\t\tif (symref && (flag & REF_ISSYMREF)) {\n> -\t\t\titem->util = xstrdup(symref);\n> -\t\t\trename->symrefs_nr++;\n> -\t\t} else {\n> -\t\t\titem->util = NULL;\n> -\t\t}\n> +\tstruct strbuf *identity = rename->buf1;\n> +\tstruct strbuf *name = rename->buf2;\n> +\tstruct strbuf *mail = rename->buf3;\n> +\tstruct ident_split ident;\n> +\tconst char *date;\n> +\tint error;\n> +\n> +\tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n> +\t\treturn -1;\n> +\n> +\tstrbuf_reset(name);\n> +\tstrbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);\n> +\tstrbuf_reset(mail);\n> +\tstrbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n> +\n> +\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n> +\tstrbuf_reset(identity);\n> +\tstrbuf_addstr(identity, fmt_ident(name->buf, mail->buf,\n> +\t\t\t\t\t WANT_BLANK_IDENT, date, 0));\n> +\n> +\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n> +\t\t\t\t\t new_oid, old_oid, identity->buf, msg,\n> +\t\t\t\t\t rename->index++, rename->err);\n> +\n> +\treturn error;\n> +}\n> +\n> +static int rename_one_reflog(const char *old_refname,\n> +\t\t\t const struct object_id *old_oid,\n> +\t\t\t struct rename_info *rename)\n> +{\n> +\tstruct strbuf *message = rename->buf1;\n> +\tint error;\n> +\n> +\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n> +\t\treturn 0;\n> +\n> +\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n> +\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n> +\tif (error < 0)\n> +\t\treturn error;\n> +\n> +\t/*\n> +\t * Manually write the reflog entry for the now-renamed ref. We cannot\n> +\t * rely on `rename_one_ref()` to do this for us as that would screw\n> +\t * over order in which reflog entries are being written.\n> +\t *\n> +\t * Furthermore, we only append the entry in case the reference\n> +\t * resolves. Missing references shouldn't have reflogs anyway.\n> +\t */\n> +\tstrbuf_reset(message);\n> +\tstrbuf_addf(message, \"remote: renamed %s to %s\", old_refname,\n> +\t\t rename->new_refname->buf);\n> +\n> +\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n> +\t\t\t\t\t old_oid, old_oid, git_committer_info(0),\n> +\t\t\t\t\t message->buf, rename->index++, rename->err);\n> +\tif (error < 0)\n> +\t\treturn error;\n> +\n> +\treturn error;\n> +}\n> +\n> +static int rename_one_ref(const char *old_refname, const char *referent,\n> +\t\t\t const struct object_id *oid,\n> +\t\t\t int flags, void *cb_data)\n> +{\n> +\tstruct rename_info *rename = cb_data;\n> +\tstruct strbuf *new_referent = rename->buf1;\n> +\tconst char *ptr = old_refname;\n> +\tint error;\n> +\n> +\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n> +\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n> +\t !skip_prefix(ptr, \"/\", &ptr)) {\n> +\t\terror = 0;\n> +\t\tgoto out;\n> \t}\n> -\tstrbuf_release(&buf);\n> \n> -\treturn 0;\n> +\trenamed_refname(rename, old_refname, rename->new_refname);\n> +\n> +\tif (flags & REF_ISSYMREF) {\n> +\t\t/*\n> +\t\t * Stupidly enough `referent` is not pointing to the immediate\n> +\t\t * target of a symref, but it's the recursively resolved value.\n> +\t\t * So symrefs pointing to symrefs would be misresolved, and\n> +\t\t * unborn symrefs don't have any value for the `referent` at all.\n> +\t\t */\n> +\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n> +\t\t\t\t\t\t NULL, NULL);\n> +\t\trenamed_refname(rename, referent, new_referent);\n> +\t\toid = NULL;\n> +\t}\n> +\n> +\terror = ref_transaction_delete(rename->tx_delete, old_refname,\n> +\t\t\t\t oid, referent, REF_NO_DEREF, NULL, rename->err);\n> +\tif (error < 0)\n> +\t\tgoto out;\n> +\n> +\terror = ref_transaction_update(rename->tx_create, rename->new_refname->buf, oid, null_oid(the_hash_algo),\n> +\t\t\t\t (flags & REF_ISSYMREF) ? new_referent->buf : NULL, NULL,\n> +\t\t\t\t REF_SKIP_CREATE_REFLOG | REF_NO_DEREF | REF_SKIP_OID_VERIFICATION,\n> +\t\t\t\t NULL, rename->err);\n> +\tif (error < 0)\n> +\t\tgoto out;\n> +\n> +\terror = rename_one_reflog(old_refname, oid, rename);\n> +\tif (error < 0)\n> +\t\tgoto out;\n> +\n> +\tdisplay_progress(rename->progress, ++rename->progress_nr);\n> +\n> +out:\n> +\treturn error;\n> }\n> \n> static int migrate_file(struct remote *remote)\n> @@ -730,7 +844,6 @@ static void handle_push_default(const char* old_name, const char* new_name)\n> \tstrbuf_release(&push_default.origin);\n> }\n> \n> -\n> static int mv(int argc, const char **argv, const char *prefix,\n> \t struct repository *repo UNUSED)\n> {\n> @@ -741,11 +854,15 @@ static int mv(int argc, const char **argv, const char *prefix,\n> \t};\n> \tstruct remote *oldremote, *newremote;\n> \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n> -\t\told_remote_context = STRBUF_INIT;\n> -\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n> -\tstruct rename_info rename;\n> -\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n> -\tstruct progress *progress = NULL;\n> +\t\told_remote_context = STRBUF_INIT, err = STRBUF_INIT;\n> +\tstruct rename_info rename = {\n> +\t\t.buf1 = &buf,\n> +\t\t.buf2 = &buf2,\n> +\t\t.buf3 = &buf3,\n> +\t\t.new_refname = &old_remote_context,\n> +\t\t.err = &err,\n> +\t};\n> +\tint i, refspec_updated = 0;\n> \tint result = 0;\n> \n> \targc = parse_options(argc, argv, prefix, options,\n> @@ -756,8 +873,6 @@ static int mv(int argc, const char **argv, const char *prefix,\n> \n> \trename.old_name = argv[0];\n> \trename.new_name = argv[1];\n> -\trename.remote_branches = &remote_branches;\n> -\trename.symrefs_nr = 0;\n> \n> \toldremote = remote_get(rename.old_name);\n> \tif (!remote_is_configured(oldremote, 1)) {\n> @@ -832,79 +947,60 @@ static int mv(int argc, const char **argv, const char *prefix,\n> \t\tgoto out;\n> \n> \t/*\n> -\t * First remove symrefs, then rename the rest, finally create\n> -\t * the new symrefs.\n> +\t * Note that we're using two transactions to rename the references:\n> +\t *\n> +\t * - One transaction contains all deletions for references part of\n> +\t * the old remote.\n> +\t * - One transaction contains all creations of references and reflogs\n> +\t * part of the new remote.\n> +\t *\n> +\t * This split is required to avoid conflicting ref updates when a\n> +\t * remote is being nested into itself or converted into its parent\n> +\t * directory.\n> +\t *\n> +\t * Unfortunately this means that the operation isn't atomic. But we\n> +\t * cannot avoid that, unless transactions learn to handle such\n> +\t * conflicts one day.\n> \t */\n\nWe could detect if the rename operation would result in a D/F conflict\nupfront and special case it by using two transactions. If we know there\nisn't a D/F conflict, I think a single transaction would be sufficient.\n\nThat being said, it might be best to keep it simple for now and leave it\nas-is.\n\n> -\trefs_for_each_ref(get_main_ref_store(the_repository),\n> -\t\t\t read_remote_branches, &rename);\n> -\tif (show_progress) {\n> -\t\t/*\n> -\t\t * Count symrefs twice, since \"renaming\" them is done by\n> -\t\t * deleting and recreating them in two separate passes.\n> -\t\t */\n> -\t\tprogress = start_progress(the_repository,\n> -\t\t\t\t\t _(\"Renaming remote references\"),\n> -\t\t\t\t\t rename.remote_branches->nr + rename.symrefs_nr);\n> -\t}\n> -\tfor (i = 0; i < remote_branches.nr; i++) {\n> -\t\tstruct string_list_item *item = remote_branches.items + i;\n> -\t\tstruct strbuf referent = STRBUF_INIT;\n> +\trename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t 0, &err);\n> +\tif (!rename.tx_delete)\n> +\t\tgoto out;\n> \n> -\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n> -\t\t\t\t\t &referent))\n> -\t\t\tcontinue;\n> -\t\tif (refs_delete_ref(get_main_ref_store(the_repository), NULL, item->string, NULL, REF_NO_DEREF))\n> -\t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n> +\trename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t 0, &err);\n> +\tif (!rename.tx_create)\n> +\t\tgoto out;\n> \n> -\t\tstrbuf_release(&referent);\n> -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n> -\t}\n> -\tfor (i = 0; i < remote_branches.nr; i++) {\n> -\t\tstruct string_list_item *item = remote_branches.items + i;\n> +\tif (show_progress)\n> +\t\trename.progress = start_delayed_progress(the_repository,\n> +\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n> \n> -\t\tif (item->util)\n> -\t\t\tcontinue;\n> -\t\tstrbuf_reset(&buf);\n> -\t\tstrbuf_addstr(&buf, item->string);\n> -\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n> -\t\t\t\trename.new_name, strlen(rename.new_name));\n> -\t\tstrbuf_reset(&buf2);\n> -\t\tstrbuf_addf(&buf2, \"remote: renamed %s to %s\",\n> -\t\t\t\titem->string, buf.buf);\n> -\t\tif (refs_rename_ref(get_main_ref_store(the_repository), item->string, buf.buf, buf2.buf))\n> -\t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n> -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n> -\t}\n> -\tfor (i = 0; i < remote_branches.nr; i++) {\n> -\t\tstruct string_list_item *item = remote_branches.items + i;\n> +\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n> +\t\t\t\t rename_one_ref, &rename);\n> +\tif (result < 0)\n> +\t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n> \n> -\t\tif (!item->util)\n> -\t\t\tcontinue;\n> -\t\tstrbuf_reset(&buf);\n> -\t\tstrbuf_addstr(&buf, item->string);\n> -\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n> -\t\t\t\trename.new_name, strlen(rename.new_name));\n> -\t\tstrbuf_reset(&buf2);\n> -\t\tstrbuf_addstr(&buf2, item->util);\n> -\t\tstrbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n> -\t\t\t\trename.new_name, strlen(rename.new_name));\n> -\t\tstrbuf_reset(&buf3);\n> -\t\tstrbuf_addf(&buf3, \"remote: renamed %s to %s\",\n> -\t\t\t\titem->string, buf.buf);\n> -\t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf))\n> -\t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n> -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n> -\t}\n> -\tstop_progress(&progress);\n> +\tresult = ref_transaction_commit(rename.tx_delete, &err);\n> +\tif (result < 0)\n> +\t\tdie(_(\"deleting old remote refs failed: %s\"), rename.err->buf);\n> +\n> +\tresult = ref_transaction_commit(rename.tx_create, &err);\n> +\tif (result < 0)\n> +\t\tdie(_(\"committing new remote refs failed: %s\"), rename.err->buf);\n> +\n> +\tstop_progress(&rename.progress);\n> \n> \thandle_push_default(rename.old_name, rename.new_name);\n> \n> out:\n> -\tstring_list_clear(&remote_branches, 1);\n> +\tref_transaction_free(rename.tx_create);\n> +\tref_transaction_free(rename.tx_delete);\n> \tstrbuf_release(&old_remote_context);\n> \tstrbuf_release(&buf);\n> \tstrbuf_release(&buf2);\n> \tstrbuf_release(&buf3);\n> +\tstrbuf_release(&err);\n> \treturn result;\n> }\n> \n> diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\n> index 2701eef85e9..f4a407b022d 100755\n> --- a/t/t5505-remote.sh\n> +++ b/t/t5505-remote.sh\n> @@ -1658,4 +1658,75 @@ test_expect_success 'forbid adding superset of existing remote' '\n> \ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n> '\n> \n> +test_expect_success 'rename handles unborn HEAD' '\n> +\ttest_when_finished \"git remote remove unborn-renamed\" &&\n> +\tgit remote add unborn url &&\n> +\tgit symbolic-ref refs/remotes/unborn/HEAD refs/remotes/unborn/nonexistent &&\n> +\tgit remote rename unborn unborn-renamed &&\n> +\tgit symbolic-ref refs/remotes/unborn-renamed/HEAD >actual &&\n> +\techo refs/remotes/unborn-renamed/nonexistent >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'rename can nest a remote into itself' '\n> +\ttest_commit parent-commit &&\n> +\tCOMMIT_ID=$(git rev-parse HEAD) &&\n> +\ttest_when_finished \"git remote remove parent || true\" &&\n> +\tgit remote add parent url &&\n> +\tgit update-ref refs/remotes/parent/branch $COMMIT_ID &&\n> +\ttest_when_finished \"git remote remove parent/child\" &&\n> +\tgit remote rename parent parent/child &&\n> +\tgit for-each-ref refs/remotes/ >actual &&\n> +\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/branch\\n\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'rename can nest a remote into itself with a conflicting branch name' '\n> +\ttest_commit parent-conflict &&\n> +\tCOMMIT_ID=$(git rev-parse HEAD) &&\n> +\ttest_when_finished \"git remote remove parent || true\" &&\n> +\tgit remote add parent url &&\n> +\tgit update-ref refs/remotes/parent/child $COMMIT_ID &&\n> +\ttest_when_finished \"git remote remove parent/child\" &&\n> +\tgit remote rename parent parent/child &&\n> +\tgit for-each-ref refs/remotes/ >actual &&\n> +\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/child\\n\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'rename can unnest a remote' '\n> +\ttest_commit parent-child-commit &&\n> +\tCOMMIT_ID=$(git rev-parse HEAD) &&\n> +\ttest_when_finished \"git remote remove parent/child || true\" &&\n> +\tgit remote add parent/child url &&\n> +\tgit update-ref refs/remotes/parent/child/branch $COMMIT_ID &&\n> +\tgit remote rename parent/child parent &&\n> +\tgit for-each-ref refs/remotes/ >actual &&\n> +\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/branch\\n\" >expected &&\n> +\ttest_cmp expected actual\n> +'\n> +\n> +test_expect_success 'rename moves around the reflog' '\n> +\ttest_commit reflog-old &&\n> +\tCOMMIT_ID=$(git rev-parse HEAD) &&\n> +\ttest_config core.logAllRefUpdates true &&\n> +\ttest_when_finished \"git remote remove reflog-old || true\" &&\n> +\tgit remote add reflog-old url &&\n> +\tgit update-ref refs/remotes/reflog-old/branch $COMMIT_ID &&\n> +\ttest-tool ref-store main for-each-reflog >actual &&\n> +\ttest_grep refs/remotes/reflog-old/branch actual &&\n> +\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-old/branch >reflog-entries-old &&\n> +\ttest_line_count = 1 reflog-entries-old &&\n> +\tgit remote rename reflog-old reflog-new &&\n> +\ttest-tool ref-store main for-each-reflog >actual &&\n> +\ttest_grep ! refs/remotes/reflog-old actual &&\n> +\ttest_grep refs/remotes/reflog-new/branch actual &&\n> +\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-new/branch >reflog-entries-new &&\n> +\tcat >expect <<-EOF &&\n> +\t$(cat reflog-entries-old)\n> +\t$COMMIT_ID $COMMIT_ID $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1112912173 -0700\tremote: renamed refs/remotes/reflog-old/branch to refs/remotes/reflog-new/branch\n> +\tEOF\n> +\ttest_cmp expect reflog-entries-new\n> +'\n> +\n> test_done\n> \n> -- \n> 2.50.1.565.gc32cd1483b.dirty\n> \n> \n"},{"id":"522874","messageId":"xmqqjz3suosc.fsf@gitster.g","threadId":"63865","inReplyTo":"j75h4trgwn5dvvxy6cqjv6pbh5ernehvnoi5itqr3icogebegp@ldbsvfwtxoeo","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T18:57:07Z","receivedAt":"2025-07-28T18:57:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Justin Tobler writes:\n\n>> +\t * This split is required to avoid conflicting ref updates when a\n>> +\t * remote is being nested into itself or converted into its parent\n>> +\t * directory.\n>> +\t *\n>> +\t * Unfortunately this means that the operation isn't atomic. But we\n>> +\t * cannot avoid that, unless transactions learn to handle such\n>> +\t * conflicts one day.\n>> \t */\n>\n> We could detect if the rename operation would result in a D/F conflict\n> upfront and special case it by using two transactions. If we know there\n> isn't a D/F conflict, I think a single transaction would be sufficient.\n\nThe right solution should be at the implementation of the\ntransactions, not the application that uses the transaction\nmechanism, no? So I would think the above workaround is actually\ncounter-productive.\n\n> That being said, it might be best to keep it simple for now and leave it\n> as-is.\n\nYes, we do not have to update the transaction layer to fix that D/F\nthing in this same series.\n\nThanks.\n"},{"id":"522932","messageId":"20250729081658.GA1842689@coredump.intra.peff.net","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-3-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-29T08:16:58Z","receivedAt":"2025-07-29T08:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 28, 2025 at 03:08:47PM +0200, Patrick Steinhardt wrote:\n\n> The next-best thing is to do it in two transactions: one to delete all\n> the references, and one to recreate the references and their reflogs.\n> This signicantly speeds up the operation with the \"files\" backend. The\n> following benchmark renames a remote with 10000 references:\n\nHmm. I was surprised to see so much reflog code here. It looks like\nyou're replaying the old reflog entry by entry. But the old code was\nleaning on refs_rename_ref() to do the individual renames, which just\nasks the backend to handle that for us (so e.g., the files backend just\ncopies/moves the log files).\n\nSo it feels like ideally we'd be able to create a transaction element\nfor renaming, and then the backends could similarly do what makes sense\nfor them (and we wouldn't need a bunch of reflog code here).\n\nI guess that does not work with the two delete/create transactions you\nend up with here, though. And you need those to worry about D/F\nconflicts. But then...how did the original handle D/F conflicts? It kind\nof looks like it didn't, as it is doing a mass ref-by-ref rename in the\nmiddle.\n\nIf the refs code learned how to order things to handle the D/F conflicts\nwithin a transaction, then we could do a single transaction. And it\ncould learn about rename primitives.\n\nI dunno. I think that would be nicer, but it's probably not worth\nholding up this topic. Your perf numbers are very nice. I guess the\npossible flip-side is that the existing code could be faster when\nrenaming a single ref (so no quadratic behavior) with a pathological\nreflog (so moving the file is faster than re-writing all of those logs).\n\nHmm, yeah. Something like this:\n\n\tcat >setup <<-\\EOF\n\t#!/bin/sh\n\n\trm -rf repo\n\tgit init repo\n\tcd repo\n\n\tgit init server\n\tgit -C server commit --allow-empty -m foo\n\n\tgit remote add origin server\n\tgit fetch\n\n\t# make the reflog gigantic\n\tperl -i -ne 'for my $i (1..10**5) { print }' .git/logs/refs/remotes/origin/main\n\tEOF\n\n\thyperfine -p ./setup -L v old,new './git.{v} -C repo remote rename origin foo'\n\nresults in:\n \n Benchmark 1: ./git.old -C repo remote rename origin foo\n Time (mean ± σ): 5.5 ms ± 1.1 ms [User: 1.5 ms, System: 1.3 ms]\n Range (min … max): 3.6 ms … 9.7 ms 58 runs\n \n Benchmark 2: ./git.new -C repo remote rename origin foo\n Time (mean ± σ): 476.3 ms ± 9.8 ms [User: 203.6 ms, System: 268.0 ms]\n Range (min … max): 467.8 ms … 498.7 ms 10 runs\n \n Summary\n ./git.old -C repo remote rename origin foo ran\n 86.43 ± 16.61 times faster than ./git.new -C repo remote rename origin foo\n\nIt's hard to bring myself to care, though. This is a stupidly\npathological reflog, and the absolute time change is peanuts compared to\nthe per-ref cost you're fixing here.\n\n-Peff\n"},{"id":"522934","messageId":"aIiJpUC5cn9oeMv9@pks.im","threadId":"63865","inReplyTo":"xmqqjz3suosc.fsf@gitster.g","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-29T08:43:17Z","receivedAt":"2025-07-29T08:43:29Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 28, 2025 at 11:57:07AM -0700, Junio C Hamano wrote:\n> Justin Tobler writes:\n> \n> >> +\t * This split is required to avoid conflicting ref updates when a\n> >> +\t * remote is being nested into itself or converted into its parent\n> >> +\t * directory.\n> >> +\t *\n> >> +\t * Unfortunately this means that the operation isn't atomic. But we\n> >> +\t * cannot avoid that, unless transactions learn to handle such\n> >> +\t * conflicts one day.\n> >> \t */\n> >\n> > We could detect if the rename operation would result in a D/F conflict\n> > upfront and special case it by using two transactions. If we know there\n> > isn't a D/F conflict, I think a single transaction would be sufficient.\n> \n> The right solution should be at the implementation of the\n> transactions, not the application that uses the transaction\n> mechanism, no? So I would think the above workaround is actually\n> counter-productive.\n> \n> > That being said, it might be best to keep it simple for now and leave it\n> > as-is.\n> \n> Yes, we do not have to update the transaction layer to fix that D/F\n> thing in this same series.\n\nIdeally, yes, and with the \"reftable\" backend this is a trivial\naddition. But as ever so often, the problem is with the \"files\" backend:\nif we have \"refs/heads/parent\" and create \"refs/heads/parent/child\" we\ncannot create the lockfile for the latter ref.\n\nThere probably are ways to solve this, but we would have to add new\nlocking semantics to the \"files\" backend to achieve it. And I am afraid\nthat such new semantics might break existing implementations of the\n\"files\" backend that are outside of Git. So I'm not really sure whether\nthis is even a possible route to go down, unfortunately.\n\nSo with that I don't think Justin's proposal is unreasonable. Some\noptions:\n\n - We can check whether the old and new remote names are prefixes of\n one another. If so, we create two transactions, otherwise we do an\n atomic commit.\n\n - We can outright refuse to do a nesting/unnesting rename and just\n always use a single transaction where there cannot be any conflicts.\n\nThe latter is of course a backwards incompatible change. It feels quite\ntempting though, as hierarchical remotes are just so esoteric. And then\nrenaming such hierarchical remotes with nesting/unnesting semantics is\neven more unlikely to ever happen.\n\nA slightly less intrusive way could be to opportunistically rename refs\nin a single transaction, regardless of whether refs are nested or not.\nAnd if we do notice any conflict we abort the whole command and give the\nuser guidance to please do `git remote rename parent/child unrelated &&\ngit remote rename unrelated parent`. Which is overall a bit of a stupid\nlimitation, but maybe it's good enough given that it is so supremely\nunlikely that anyone will ever hit the issue?\n\nPatrick\n"},{"id":"522935","messageId":"aIiJsdPrGrhZYis5@pks.im","threadId":"63865","inReplyTo":"xmqqbjp4w7uu.fsf@gitster.g","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-29T08:43:29Z","receivedAt":"2025-07-29T08:43:34Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jul 28, 2025 at 10:19:53AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > But more importantly it is also extremely inperformant. The number of\n> \n> Is \"inperformant\" a real word? \"it performs extremely poorly\"?\n\nWell, in my head it is :) But it doesn't seem to exist anywhere else, so\nI'll reword this.\n\n> > +static void renamed_refname(struct rename_info *rename,\n> > +\t\t\t const char *refname,\n> > +\t\t\t struct strbuf *out)\n> > +{\n> > +\tstrbuf_reset(out);\n> > +\tstrbuf_addstr(out, refname);\n> > +\tstrbuf_splice(out, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> > +\t\t rename->new_name, strlen(rename->new_name));\n> > +}\n> > +\n> \n> The function name felt somewhat iffy (sounded as if you are letting\n> a third-party know that you have renamed a ref), but I cannot come\n> up with a better alternative X-<.\n\nWe could name it `compute_renamed_refname()` to make it a bit more\nverb-y.\n\n> > +static int rename_one_reflog_entry(const char *old_refname UNUSED,\n> > +\t\t\t\t struct object_id *old_oid,\n> > +\t\t\t\t struct object_id *new_oid,\n> > +\t\t\t\t const char *committer,\n> > +\t\t\t\t timestamp_t timestamp, int tz,\n> > +\t\t\t\t const char *msg, void *cb_data)\n> > {\n> > \tstruct rename_info *rename = cb_data;\n> \n> Using a name of a system call for an unrelated variable, even if a\n> local one in a function scope, makes me nauseous. Not a new problem\n> introduced by this change, though.\n\nYeah, I don't love it, either.\n\n> > +static int rename_one_reflog(const char *old_refname,\n> > +\t\t\t const struct object_id *old_oid,\n> > +\t\t\t struct rename_info *rename)\n> > +{\n> > +\tstruct strbuf *message = rename->buf1;\n> \n> As these temporary strbuf's passed around as part of the rename_info\n> structure are never released or recreated during the run, this is\n> safe, but feels dirty, because we saw rename_one_reflog_entry() uses\n> this exact one for totally different purpose. Perhaps it would make\n> it easier to follow if you left \"message\" uninitialized here, before\n> refs_for_each_reflog_ent() returns. And then ...\n> \n> > +\tint error;\n> > +\n> > +\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n> > +\t\treturn 0;\n> > +\n> > +\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n> > +\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n> > +\tif (error < 0)\n> > +\t\treturn error;\n> > +\n> > +\t/*\n> > +\t * Manually write the reflog entry for the now-renamed ref. We cannot\n> > +\t * rely on `rename_one_ref()` to do this for us as that would screw\n> > +\t * over order in which reflog entries are being written.\n> > +\t *\n> > +\t * Furthermore, we only append the entry in case the reference\n> > +\t * resolves. Missing references shouldn't have reflogs anyway.\n> > +\t */\n> \n> ... give the \"message\" synonym to rename->buf1 here.\n\nI was quite on the edge whether these buffers are really worth it in the\nfirst place as an optimization -- I mostly adopted it from the migration\ncode. But I've benchmarked it now and couldn't really make out much of a\ndifference at all. So let's just drop all of this buffer reusing infra.\n\n> > +static int rename_one_ref(const char *old_refname, const char *referent,\n> > +\t\t\t const struct object_id *oid,\n> > +\t\t\t int flags, void *cb_data)\n> > +{\n> > +\tstruct rename_info *rename = cb_data;\n> > +\tstruct strbuf *new_referent = rename->buf1;\n> > +\tconst char *ptr = old_refname;\n> > +\tint error;\n> > +\n> > +\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n> > +\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n> > +\t !skip_prefix(ptr, \"/\", &ptr)) {\n> > +\t\terror = 0;\n> > +\t\tgoto out;\n> > \t}\n> > -\tstrbuf_release(&buf);\n> > \n> > -\treturn 0;\n> > +\trenamed_refname(rename, old_refname, rename->new_refname);\n> > +\n> > +\tif (flags & REF_ISSYMREF) {\n> > +\t\t/*\n> > +\t\t * Stupidly enough `referent` is not pointing to the immediate\n> > +\t\t * target of a symref, but it's the recursively resolved value.\n> > +\t\t * So symrefs pointing to symrefs would be misresolved, and\n> > +\t\t * unborn symrefs don't have any value for the `referent` at all.\n> > +\t\t */\n> > +\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n> > +\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n> > +\t\t\t\t\t\t NULL, NULL);\n> > +\t\trenamed_refname(rename, referent, new_referent);\n> > +\t\toid = NULL;\n> \n> Yuck, but this cannot be helped, I guess X-<.\n\nI dunno. I feel like this is something we should eventually fix.\nCurrently this is misleading and basically useless.\n\n#leftoverbits\n\nPatrick\n"},{"id":"522950","messageId":"aIi9mAoAb1v9igQ6@pks.im","threadId":"63865","inReplyTo":"20250729081658.GA1842689@coredump.intra.peff.net","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-29T12:24:56Z","receivedAt":"2025-07-29T12:25:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 29, 2025 at 04:16:58AM -0400, Jeff King wrote:\n> On Mon, Jul 28, 2025 at 03:08:47PM +0200, Patrick Steinhardt wrote:\n> \n> > The next-best thing is to do it in two transactions: one to delete all\n> > the references, and one to recreate the references and their reflogs.\n> > This signicantly speeds up the operation with the \"files\" backend. The\n> > following benchmark renames a remote with 10000 references:\n> \n> Hmm. I was surprised to see so much reflog code here. It looks like\n> you're replaying the old reflog entry by entry. But the old code was\n> leaning on refs_rename_ref() to do the individual renames, which just\n> asks the backend to handle that for us (so e.g., the files backend just\n> copies/moves the log files).\n> \n> So it feels like ideally we'd be able to create a transaction element\n> for renaming, and then the backends could similarly do what makes sense\n> for them (and we wouldn't need a bunch of reflog code here).\n\nI think overall the ref transaction code is in for a bit of a redesign.\nThe current one-size-fits-all `struct ref_update` doesn't make a lot of\nsense anymore. The requirements have shifted because of the reftable\nbackend, where we want to redesign interfaces so that we batch updates\ntogether to the best extent possible.\n\nAnd this patch series here demonstrates that not only the \"reftable\"\nbackend benefits.\n\nIn any case, the current infrastructure is extremely brittle and every\nchange one does is a bit like threading a needle. More likely than not\nyou missed this one edge case where changing a flag for one of the\nupdates has a consequence in a completely unrelated place.\n\nI think we should start splitting this up more and introduce different\nupdate types:\n\n - Forced updates where we don't very the old state.\n\n - Raceless updates where we do.\n\n - Reflog updates that only write a message as well as an old/new\n state.\n\nIf we had such an infrastructure it would also be feasible to introduce\nmore types, like deletes or renames.\n\n> I guess that does not work with the two delete/create transactions you\n> end up with here, though. And you need those to worry about D/F\n> conflicts. But then...how did the original handle D/F conflicts? It kind\n> of looks like it didn't, as it is doing a mass ref-by-ref rename in the\n> middle.\n> \n> If the refs code learned how to order things to handle the D/F conflicts\n> within a transaction, then we could do a single transaction. And it\n> could learn about rename primitives.\n\nTrue. But as I already mentioned to Junio I don't really know how to\nbackfill D/F conflict handling in the \"files\" backend. The problem is\nthat with preexisting \"refs/heads/parent\" and \"refs/heads/parent/child\"\nyou cannot create the latter \".lock\" file. Sure, we can hack our way\naround that in some manner. But is that backwards compatible if another\nGit client were to operate in the same repository? I dunno.\n\n> I dunno. I think that would be nicer, but it's probably not worth\n> holding up this topic. Your perf numbers are very nice. I guess the\n> possible flip-side is that the existing code could be faster when\n> renaming a single ref (so no quadratic behavior) with a pathological\n> reflog (so moving the file is faster than re-writing all of those logs).\n> \n> Hmm, yeah. Something like this:\n> \n> \tcat >setup <<-\\EOF\n> \t#!/bin/sh\n> \n> \trm -rf repo\n> \tgit init repo\n> \tcd repo\n> \n> \tgit init server\n> \tgit -C server commit --allow-empty -m foo\n> \n> \tgit remote add origin server\n> \tgit fetch\n> \n> \t# make the reflog gigantic\n> \tperl -i -ne 'for my $i (1..10**5) { print }' .git/logs/refs/remotes/origin/main\n> \tEOF\n> \n> \thyperfine -p ./setup -L v old,new './git.{v} -C repo remote rename origin foo'\n> \n> results in:\n> \n> Benchmark 1: ./git.old -C repo remote rename origin foo\n> Time (mean ± σ): 5.5 ms ± 1.1 ms [User: 1.5 ms, System: 1.3 ms]\n> Range (min … max): 3.6 ms … 9.7 ms 58 runs\n> \n> Benchmark 2: ./git.new -C repo remote rename origin foo\n> Time (mean ± σ): 476.3 ms ± 9.8 ms [User: 203.6 ms, System: 268.0 ms]\n> Range (min … max): 467.8 ms … 498.7 ms 10 runs\n> \n> Summary\n> ./git.old -C repo remote rename origin foo ran\n> 86.43 ± 16.61 times faster than ./git.new -C repo remote rename origin foo\n> \n> It's hard to bring myself to care, though. This is a stupidly\n> pathological reflog, and the absolute time change is peanuts compared to\n> the per-ref cost you're fixing here.\n\nFor the \"files\" backend performance is worse, for the \"reftable\" backend\nI'd expect that this might even be faster. Mostly because there is no\nway to trivially rename a reflog -- we basically do the same on a rename\nas we are doing with this patch series now.\n\nOverall I don't care too much about this edge case. By default we never\nwrite reflogs for remote references anyway, and I doubt that you'll ever\nend up with a remote reflog that has thousands of entries. So I'd rather\nmake the general case fast even if the esoteric case becomes slower.\n\nBut ideally we're able to lift such limitations in the future if we were\nto do the above rework.\n\nPatrick\n"},{"id":"522985","messageId":"CAOLa=ZR10eOikwFyDwMDSyX2qQM-knCKhCKB4YtLD=Pdoz=vTA@mail.gmail.com","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-1-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-07-29T20:30:49Z","receivedAt":"2025-07-29T20:30:52Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> With `refs_for_each_reflog_ent()` callers can iterate through all the\n> reflog entries for a given reference. The callback that is being invoked\n> for each such entry does not receive the name of the reference that we\n> are currently iterating through. This isn't really a limiting factor, as\n> callers can simply pass the name via the callback data.\n>\n> But this layout sometimes does make for a bit of an awkward calling\n> pattern. One example: when iterating through all reflogs, and for each\n> reflog we iterate through all refnames, we have to do some extra book\n> keeping to track which reference name we are currently yielding reflog\n> entries for.\n>\n> Change the signature of the callback function so that the reference name\n> of the reflog gets passed through to it. Adapt callers accordingly and\n> start using the new parameter in trivial cases. The next commit will\n> refactor the reference migration logic to make use of this parameter so\n> that we can simplify its logic a bit.\n>\n\nI remember hitting this issue with the migration code in 'refs.c', so I\nthink this is a good improvement. The changes themselves look good.\n\nNit: Changes suggested by clang-format in case you re-roll:\n\ndiff --git a/refs.c b/refs.c\nindex fd9a5f36b2..6ed0cd6ddc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1078,8 +1078,7 @@ static int read_ref_at_ent(const char *refname,\n \t\t\toidcpy(cb->oid, noid);\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n-\t\t\t\trefname, show_date(cb->date, cb->tz,\n-\t\t\t\t\t\t DATE_MODE(RFC2822)));\n+\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\tcb->reccnt++;\n \t\toidcpy(&cb->ooid, ooid);\n \t\toidcpy(&cb->noid, noid);\ndiff --git a/refs.h b/refs.h\nindex a39f873b1f..5b0efaf752 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -559,10 +559,10 @@ int refs_delete_reflog(struct ref_store *refs,\nconst char *refname);\n * functions.\n */\n typedef int each_reflog_ent_fn(\n-\t\tconst char *refname,\n-\t\tstruct object_id *old_oid, struct object_id *new_oid,\n-\t\tconst char *committer, timestamp_t timestamp,\n-\t\tint tz, const char *msg, void *cb_data);\n+\tconst char *refname,\n+\tstruct object_id *old_oid, struct object_id *new_oid,\n+\tconst char *committer, timestamp_t timestamp,\n+\tint tz, const char *msg, void *cb_data);\n\n /* Iterate over reflog entries in the log for `refname`. */\n"},{"id":"522999","messageId":"CAOLa=ZR=pQ58GSKh-M1fg5Ym5U6THmmvgTSNGUGCmdhhuVHFCg@mail.gmail.com","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-4-f654f2b5c5ae@pks.im","subject":"Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2025-07-30T07:53:13Z","receivedAt":"2025-07-30T07:53:15Z","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/refs.c b/refs.c\n> index b820c3908bd..861a0deb924 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -1840,7 +1840,13 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n>\n> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n> {\n> -\treturn do_for_each_ref(refs, \"\", NULL, fn, 0,\n> +\treturn refs_for_each_rawref_in(refs, \"\", fn, cb_data);\n> +}\n> +\n> +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n> +\t\t\t each_ref_fn fn, void *cb_data)\n> +{\n> +\treturn do_for_each_ref(refs, prefix, NULL, fn, 0,\n> \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n> }\n>\n> diff --git a/refs.h b/refs.h\n> index a39f873b1fe..9decd3126e3 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -428,6 +428,8 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n>\n> /* can be used to learn about broken ref and symref */\n> int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n> +\t\t\t each_ref_fn fn, void *cb_data);\n>\n> /*\n> * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.\n>\n> --\n> 2.50.1.565.gc32cd1483b.dirty\n\nNit: we do expose the reference iterators now with\n'kn/for-each-ref-skip' (merged to next). We could directly use the\niterator instead of introducting a specific function like this.\n\n- Karthik\n"},{"id":"523081","messageId":"aIspLhS-hVIHQ473@pks.im","threadId":"63865","inReplyTo":"CAOLa=ZR10eOikwFyDwMDSyX2qQM-knCKhCKB4YtLD=Pdoz=vTA@mail.gmail.com","subject":"Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T08:28:30Z","receivedAt":"2025-07-31T08:28:38Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jul 29, 2025 at 01:30:49PM -0700, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> > With `refs_for_each_reflog_ent()` callers can iterate through all the\n> > reflog entries for a given reference. The callback that is being invoked\n> > for each such entry does not receive the name of the reference that we\n> > are currently iterating through. This isn't really a limiting factor, as\n> > callers can simply pass the name via the callback data.\n> >\n> > But this layout sometimes does make for a bit of an awkward calling\n> > pattern. One example: when iterating through all reflogs, and for each\n> > reflog we iterate through all refnames, we have to do some extra book\n> > keeping to track which reference name we are currently yielding reflog\n> > entries for.\n> >\n> > Change the signature of the callback function so that the reference name\n> > of the reflog gets passed through to it. Adapt callers accordingly and\n> > start using the new parameter in trivial cases. The next commit will\n> > refactor the reference migration logic to make use of this parameter so\n> > that we can simplify its logic a bit.\n> >\n> \n> I remember hitting this issue with the migration code in 'refs.c', so I\n> think this is a good improvement. The changes themselves look good.\n> \n> Nit: Changes suggested by clang-format in case you re-roll:\n\nOkay, I've rolled in both suggestions. I'll not send a new version only\nto address these reformats though.\n\nPatrick\n"},{"id":"523082","messageId":"aIspNyUCbf0_dQbh@pks.im","threadId":"63865","inReplyTo":"CAOLa=ZR=pQ58GSKh-M1fg5Ym5U6THmmvgTSNGUGCmdhhuVHFCg@mail.gmail.com","subject":"Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T08:28:39Z","receivedAt":"2025-07-31T08:28:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jul 30, 2025 at 09:53:13AM +0200, Karthik Nayak wrote:\n> Patrick Steinhardt writes:\n> \n> [snip]\n> \n> > diff --git a/refs.c b/refs.c\n> > index b820c3908bd..861a0deb924 100644\n> > --- a/refs.c\n> > +++ b/refs.c\n> > @@ -1840,7 +1840,13 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n> >\n> > int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n> > {\n> > -\treturn do_for_each_ref(refs, \"\", NULL, fn, 0,\n> > +\treturn refs_for_each_rawref_in(refs, \"\", fn, cb_data);\n> > +}\n> > +\n> > +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n> > +\t\t\t each_ref_fn fn, void *cb_data)\n> > +{\n> > +\treturn do_for_each_ref(refs, prefix, NULL, fn, 0,\n> > \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n> > }\n> >\n> > diff --git a/refs.h b/refs.h\n> > index a39f873b1fe..9decd3126e3 100644\n> > --- a/refs.h\n> > +++ b/refs.h\n> > @@ -428,6 +428,8 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n> >\n> > /* can be used to learn about broken ref and symref */\n> > int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n> > +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n> > +\t\t\t each_ref_fn fn, void *cb_data);\n> >\n> > /*\n> > * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.\n> >\n> > --\n> > 2.50.1.565.gc32cd1483b.dirty\n> \n> Nit: we do expose the reference iterators now with\n> 'kn/for-each-ref-skip' (merged to next). We could directly use the\n> iterator instead of introducting a specific function like this.\n\nI'll leave this as-is for now. The additional wrapper isn't all that\nbad, and I'd rather want to avoid adding another dependency to this\npatch series.\n\nPatrick\n"},{"id":"523097","messageId":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im","subject":"[PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:48Z","receivedAt":"2025-07-31T14:57:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series is the result from the discussion at [1]. On the one\nhand this series fixes the reported bug where dangling symrefs are not\nrenamed via `git remote rename`.\n\nOn the other hand this series reworks the logic used to rename remotes\nso that we use two transactions instead of one transaction per ref. This\nfixes quadratic runtime behaviour, where renaming 10k refs takes ~4\nminutes, 100k takes hours. This results in a significant speedup with\nboth the \"files\" backend (benchmarked with a smaller number of refs to\nretain sanity):\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n Range (min … max): 204.863 s … 247.699 s 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s]\n Range (min … max): 2.011 s … 2.141 s 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~)\n\nFor the \"reftable\" backend we see a significant speedup, as well, but\nnot as extreme as with the \"files\" backend:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s]\n Range (min … max): 7.880 s … 9.556 s 10 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s]\n Range (min … max): 1.023 s … 1.410 s 10 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nBut in any case, it's one more case where the \"reftable\" backend\noutperforms the \"files\" backend.\n\nThe series is built on top of e4ef0485fd7 (The fourteenth batch,\n2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\ninvalid old object IDs when migrating reflogs, 2025-07-25) merged into\nit.\n\nI'd normally have withheld sending until that series was merged to\n\"next\", but given that I promised to send something on Friday already I\ndecided to just get it out. In any case, if that causes problems I'm\nhappy to wait a bit before this series here gets merged into \"seen\".\n\nChanges in v2:\n - Insert another commit to fix sign-comparison warnings.\n - Some code formatting fixes.\n - Drop the infrastructure to reuse buffers. It didn't really help much\n with performance anyway.\n - I'm now using a single transaction to rename references so that we\n have proper atomicity. Conflicts are detected early before any\n changes are performed in the repository and an advice is printed\n indicating what has happened.\n - Link to v1: https://lore.kernel.org/r/20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im\n\nThanks!\n\nPatrick\n\n[1]: \n\n---\nPatrick Steinhardt (6):\n refs: pass refname when invoking reflog entry callback\n refs: simplify logic when migrating reflog entries\n builtin/remote: fix sign comparison warnings\n builtin/remote: determine whether refs need renaming early on\n builtin/remote: rework how remote refs get renamed\n builtin/remote: only iterate through refs that are to be renamed\n\n builtin/fsck.c | 9 +-\n builtin/gc.c | 3 +-\n builtin/remote.c | 345 +++++++++++++++++++++++++++++-----------------\n builtin/stash.c | 6 +-\n commit.c | 3 +-\n object-name.c | 3 +-\n reflog-walk.c | 7 +-\n reflog.c | 3 +-\n reflog.h | 3 +-\n refs.c | 64 ++++-----\n refs.h | 13 +-\n refs/debug.c | 5 +-\n refs/files-backend.c | 15 +-\n refs/reftable-backend.c | 2 +-\n remote.c | 6 +-\n revision.c | 3 +-\n t/helper/test-ref-store.c | 3 +-\n t/t5505-remote.sh | 73 ++++++++++\n wt-status.c | 6 +-\n 19 files changed, 372 insertions(+), 200 deletions(-)\n\nRange-diff versus v1:\n\n1: ca658850cf ! 1: 67b46a7afe refs: pass refname when invoking reflog entry callback\n @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n -\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n -+\t\t\t\trefname, show_date(cb->date, cb->tz,\n - \t\t\t\t\t\t DATE_MODE(RFC2822)));\n +-\t\t\t\t\t\t DATE_MODE(RFC2822)));\n ++\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\tcb->reccnt++;\n \t\toidcpy(&cb->ooid, ooid);\n + \t\toidcpy(&cb->noid, noid);\n @@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n @@ refs.c: static int migrate_one_reflog(const char *refname, void *cb_data)\n \n ## refs.h ##\n @@ refs.h: int refs_delete_reflog(struct ref_store *refs, const char *refname);\n + * The cb_data is a caller-supplied pointer given to the iterator\n * functions.\n */\n - typedef int each_reflog_ent_fn(\n -+\t\tconst char *refname,\n - \t\tstruct object_id *old_oid, struct object_id *new_oid,\n - \t\tconst char *committer, timestamp_t timestamp,\n - \t\tint tz, const char *msg, void *cb_data);\n +-typedef int each_reflog_ent_fn(\n +-\t\tstruct object_id *old_oid, struct object_id *new_oid,\n +-\t\tconst char *committer, timestamp_t timestamp,\n +-\t\tint tz, const char *msg, void *cb_data);\n ++typedef int each_reflog_ent_fn(const char *refname,\n ++\t\t\t struct object_id *old_oid,\n ++\t\t\t struct object_id *new_oid,\n ++\t\t\t const char *committer,\n ++\t\t\t timestamp_t timestamp,\n ++\t\t\t int tz, const char *msg,\n ++\t\t\t void *cb_data);\n + \n + /* Iterate over reflog entries in the log for `refname`. */\n + \n \n ## refs/debug.c ##\n @@ refs/debug.c: struct debug_reflog {\n2: b119cdfd48 = 2: 239f7acd1d refs: simplify logic when migrating reflog entries\n-: ---------- > 3: e421091721 builtin/remote: fix sign comparison warnings\n-: ---------- > 4: 0d88ed6365 builtin/remote: determine whether refs need renaming early on\n3: 8d2678f27c ! 5: 8aed1ce7f5 builtin/remote: rework how remote refs get renamed\n @@ Commit message\n symrefs. On the one hand this isn't even remotely an atomic operation,\n so if we hit any error we'll already have deleted all references.\n \n - But more importantly it is also extremely inperformant. The number of\n + But more importantly it is also extremely inefficient. The number of\n transactions for symrefs doesn't really bother us too much, as there\n should generally only be a single symref anyway (\"HEAD\"). But the\n renames are very expensive:\n @@ Commit message\n The consequence here is quadratic runtime performance. Renaming a\n 100k references takes hours to complete.\n \n - Ideally we'd refactor the code to use a single transaction to perform\n - all the reference updates atomically. But unfortunately that does not\n - work in the case where you rename a remote so that it becomes nested\n - into itself, as that can lead to conflicting reference updates.\n -\n - The next-best thing is to do it in two transactions: one to delete all\n - the references, and one to recreate the references and their reflogs.\n - This signicantly speeds up the operation with the \"files\" backend. The\n - following benchmark renames a remote with 10000 references:\n + Refactor the code to use a single transaction to perform all the\n + reference updates atomically, which speeds up the transaction quite\n + significantly:\n \n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n @@ Commit message\n rename remote (refformat = reftable, revision = HEAD) ran\n 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n \n + There is one issue though with using atomic transactions: when nesting a\n + remote into itself it can happen that renamed references conflict with\n + the old referencse. For example, when we have a reference\n + \"refs/remotes/origin/foo\" and we rename \"origin\" to \"origin/foo\", then\n + we'll end up with an F/D conflict when we try to create the renamed\n + reference \"refs/remotes/origin/foo/foo\".\n +\n + This situation is overall quite unlikely to happen: people tend to not\n + use nested remotes, and if they do they must at the same time also have\n + a conflicting refname. But the end result would be that the old remote\n + references stay intact whereas all the other parts of the repository\n + have been adjusted for the new remote name.\n +\n + Address this by queueing and preparing the reference update before we\n + touch any other part of the repository. Like this we can make sure that\n + the reference update will go through before rewriting the configuration.\n + Otherwise, if the transaction fails to prepare we can gracefully abort\n + the whole operation without any changes having been performed in the\n + repository yet. Furthermore, we can detect the conflict and print some\n + helpful advice for how the user can resolve this situation. So overall,\n + the tradeoff is that:\n +\n + - Reference transactions are now all-or-nothing. This is a significant\n + improvement over the previous state where we may have ended up with\n + partially-renamed references.\n +\n + - Rewriting references is now significantly faster.\n +\n + - We only rewrite the configuration in case we know that all\n + references can be updated.\n +\n + - But we may refuse to rename a remote in case references conflict.\n +\n + Overall this seems like an acceptable tradeoff.\n +\n While at it, fix the handling of symbolic/broken references by using\n `refs_for_each_rawref()`. Add tests that cover both this reported issue\n - and tests that verify that nesting remotes into itself continues to work\n - with conflicting references.\n + and tests that exercise nesting of remotes.\n \n One thing to note: with this change we cannot provide a proper progress\n monitor anymore as we queue the references into the transactions as we\n @@ Commit message\n \n ## builtin/remote.c ##\n @@\n + #define USE_THE_REPOSITORY_VARIABLE\n \n #include \"builtin.h\"\n ++#include \"advice.h\"\n #include \"config.h\"\n +#include \"date.h\"\n #include \"gettext.h\"\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n \tconst char *new_name;\n -\tstruct string_list *remote_branches;\n -\tuint32_t symrefs_nr;\n -+\tstruct ref_transaction *tx_create;\n -+\tstruct ref_transaction *tx_delete;\n ++\tstruct ref_transaction *transaction;\n +\tstruct progress *progress;\n ++\tstruct strbuf *err;\n +\tuint32_t progress_nr;\n -+\tstruct strbuf *buf1, *buf2, *buf3, *err;\n -+\tstruct strbuf *new_refname;\n +\tuint64_t index;\n };\n \n -static int read_remote_branches(const char *refname, const char *referent UNUSED,\n -\t\t\t\tconst struct object_id *oid UNUSED,\n -\t\t\t\tint flags UNUSED, void *cb_data)\n -+static void renamed_refname(struct rename_info *rename,\n -+\t\t\t const char *refname,\n -+\t\t\t struct strbuf *out)\n ++static void compute_renamed_ref(struct rename_info *rename,\n ++\t\t\t\tconst char *refname,\n ++\t\t\t\tstruct strbuf *out)\n +{\n +\tstrbuf_reset(out);\n +\tstrbuf_addstr(out, refname);\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t\t rename->new_name, strlen(rename->new_name));\n +}\n +\n -+static int rename_one_reflog_entry(const char *old_refname UNUSED,\n ++static int rename_one_reflog_entry(const char *old_refname,\n +\t\t\t\t struct object_id *old_oid,\n +\t\t\t\t struct object_id *new_oid,\n +\t\t\t\t const char *committer,\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n -\t\t} else {\n -\t\t\titem->util = NULL;\n -\t\t}\n -+\tstruct strbuf *identity = rename->buf1;\n -+\tstruct strbuf *name = rename->buf2;\n -+\tstruct strbuf *mail = rename->buf3;\n ++\tstruct strbuf new_refname = STRBUF_INIT;\n ++\tstruct strbuf identity = STRBUF_INIT;\n ++\tstruct strbuf name = STRBUF_INIT;\n ++\tstruct strbuf mail = STRBUF_INIT;\n +\tstruct ident_split ident;\n +\tconst char *date;\n +\tint error;\n +\n -+\tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n -+\t\treturn -1;\n ++\tcompute_renamed_ref(rename, old_refname, &new_refname);\n +\n -+\tstrbuf_reset(name);\n -+\tstrbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);\n -+\tstrbuf_reset(mail);\n -+\tstrbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n ++\tif (split_ident_line(&ident, committer, strlen(committer)) < 0) {\n ++\t\terror = -1;\n ++\t\tgoto out;\n + \t}\n +-\tstrbuf_release(&buf);\n + \n +-\treturn 0;\n ++\tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n ++\tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n +\n +\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n -+\tstrbuf_reset(identity);\n -+\tstrbuf_addstr(identity, fmt_ident(name->buf, mail->buf,\n ++\tstrbuf_addstr(&identity, fmt_ident(name.buf, mail.buf,\n +\t\t\t\t\t WANT_BLANK_IDENT, date, 0));\n +\n -+\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n -+\t\t\t\t\t new_oid, old_oid, identity->buf, msg,\n ++\terror = ref_transaction_update_reflog(rename->transaction, new_refname.buf,\n ++\t\t\t\t\t new_oid, old_oid, identity.buf, msg,\n +\t\t\t\t\t rename->index++, rename->err);\n +\n ++out:\n ++\tstrbuf_release(&new_refname);\n ++\tstrbuf_release(&identity);\n ++\tstrbuf_release(&name);\n ++\tstrbuf_release(&mail);\n +\treturn error;\n +}\n +\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t\t\t const struct object_id *old_oid,\n +\t\t\t struct rename_info *rename)\n +{\n -+\tstruct strbuf *message = rename->buf1;\n ++\tstruct strbuf new_refname = STRBUF_INIT;\n ++\tstruct strbuf message = STRBUF_INIT;\n +\tint error;\n +\n +\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n +\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n +\tif (error < 0)\n -+\t\treturn error;\n ++\t\tgoto out;\n ++\n ++\tcompute_renamed_ref(rename, old_refname, &new_refname);\n +\n +\t/*\n +\t * Manually write the reflog entry for the now-renamed ref. We cannot\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t * Furthermore, we only append the entry in case the reference\n +\t * resolves. Missing references shouldn't have reflogs anyway.\n +\t */\n -+\tstrbuf_reset(message);\n -+\tstrbuf_addf(message, \"remote: renamed %s to %s\", old_refname,\n -+\t\t rename->new_refname->buf);\n ++\tstrbuf_addf(&message, \"remote: renamed %s to %s\", old_refname,\n ++\t\t new_refname.buf);\n +\n -+\terror = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,\n ++\terror = ref_transaction_update_reflog(rename->transaction, new_refname.buf,\n +\t\t\t\t\t old_oid, old_oid, git_committer_info(0),\n -+\t\t\t\t\t message->buf, rename->index++, rename->err);\n ++\t\t\t\t\t message.buf, rename->index++, rename->err);\n +\tif (error < 0)\n +\t\treturn error;\n +\n ++out:\n ++\tstrbuf_release(&new_refname);\n ++\tstrbuf_release(&message);\n +\treturn error;\n +}\n +\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t\t\t const struct object_id *oid,\n +\t\t\t int flags, void *cb_data)\n +{\n ++\tstruct strbuf new_referent = STRBUF_INIT;\n ++\tstruct strbuf new_refname = STRBUF_INIT;\n +\tstruct rename_info *rename = cb_data;\n -+\tstruct strbuf *new_referent = rename->buf1;\n +\tconst char *ptr = old_refname;\n +\tint error;\n +\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t !skip_prefix(ptr, \"/\", &ptr)) {\n +\t\terror = 0;\n +\t\tgoto out;\n - \t}\n --\tstrbuf_release(&buf);\n - \n --\treturn 0;\n -+\trenamed_refname(rename, old_refname, rename->new_refname);\n ++\t}\n ++\n ++\tcompute_renamed_ref(rename, old_refname, &new_refname);\n +\n +\tif (flags & REF_ISSYMREF) {\n +\t\t/*\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n +\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n +\t\t\t\t\t\t NULL, NULL);\n -+\t\trenamed_refname(rename, referent, new_referent);\n ++\t\tcompute_renamed_ref(rename, referent, &new_referent);\n +\t\toid = NULL;\n +\t}\n +\n -+\terror = ref_transaction_delete(rename->tx_delete, old_refname,\n ++\terror = ref_transaction_delete(rename->transaction, old_refname,\n +\t\t\t\t oid, referent, REF_NO_DEREF, NULL, rename->err);\n +\tif (error < 0)\n +\t\tgoto out;\n +\n -+\terror = ref_transaction_update(rename->tx_create, rename->new_refname->buf, oid, null_oid(the_hash_algo),\n -+\t\t\t\t (flags & REF_ISSYMREF) ? new_referent->buf : NULL, NULL,\n ++\terror = ref_transaction_update(rename->transaction, new_refname.buf, oid, null_oid(the_hash_algo),\n ++\t\t\t\t (flags & REF_ISSYMREF) ? new_referent.buf : NULL, NULL,\n +\t\t\t\t REF_SKIP_CREATE_REFLOG | REF_NO_DEREF | REF_SKIP_OID_VERIFICATION,\n +\t\t\t\t NULL, rename->err);\n +\tif (error < 0)\n @@ builtin/remote.c: static int add_branch_for_removal(const char *refname,\n +\tdisplay_progress(rename->progress, ++rename->progress_nr);\n +\n +out:\n ++\tstrbuf_release(&new_referent);\n ++\tstrbuf_release(&new_refname);\n +\treturn error;\n }\n \n @@ builtin/remote.c: static void handle_push_default(const char* old_name, const ch\n \tstrbuf_release(&push_default.origin);\n }\n \n --\n ++static const char conflicting_remote_refs_advice[] = N_(\n ++\t\"The remote you are trying to rename has conflicting references in the\\n\"\n ++\t\"new target refspec. This is most likely caused by you trying to nest\\n\"\n ++\t\"a remote into itself, e.g. by renaming 'parent' into 'parent/child'\\n\"\n ++\t\"or by unnesting a remote, e.g. the other way round.\\n\"\n ++\t\"\\n\"\n ++\t\"If that is the case, you can address this by first renaming the\\n\"\n ++\t\"remote to a different name.\\n\");\n + \n static int mv(int argc, const char **argv, const char *prefix,\n \t struct repository *repo UNUSED)\n - {\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n \t};\n \tstruct remote *oldremote, *newremote;\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n -\t\told_remote_context = STRBUF_INIT;\n -\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n -\tstruct rename_info rename;\n --\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n +-\tint refs_renamed_nr = 0, refspecs_need_update = 0;\n -\tstruct progress *progress = NULL;\n +\t\told_remote_context = STRBUF_INIT, err = STRBUF_INIT;\n +\tstruct rename_info rename = {\n -+\t\t.buf1 = &buf,\n -+\t\t.buf2 = &buf2,\n -+\t\t.buf3 = &buf3,\n -+\t\t.new_refname = &old_remote_context,\n +\t\t.err = &err,\n +\t};\n -+\tint i, refspec_updated = 0;\n ++\tint refspecs_need_update = 0;\n \tint result = 0;\n \n \targc = parse_options(argc, argv, prefix, options,\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n - \t\tgoto out;\n + \t\trefspecs_need_update = !!strstr(oldremote->fetch.items[i].raw,\n + \t\t\t\t\t\told_remote_context.buf);\n \n - \t/*\n ++\tif (refspecs_need_update) {\n ++\t\trename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n ++\t\t\t\t\t\t\t 0, &err);\n ++\t\tif (!rename.transaction)\n ++\t\t\tgoto out;\n ++\n ++\t\tif (show_progress)\n ++\t\t\trename.progress = start_delayed_progress(the_repository,\n ++\t\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n ++\n ++\t\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n ++\t\t\t\trename_one_ref, &rename);\n ++\t\tif (result < 0)\n ++\t\t\tdie(_(\"queueing remote ref renames failed: %s\"), rename.err->buf);\n ++\n ++\t\tresult = ref_transaction_prepare(rename.transaction, &err);\n ++\t\tif (result < 0) {\n ++\t\t\terror(\"renaming remote references failed: %s\", err.buf);\n ++\t\t\tif (result == REF_TRANSACTION_ERROR_NAME_CONFLICT)\n ++\t\t\t\tadvise(conflicting_remote_refs_advice);\n ++\t\t\tdie(NULL);\n ++\t\t}\n ++\t}\n ++\n + \tif (oldremote->fetch.nr) {\n + \t\tstrbuf_reset(&buf);\n + \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n +@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n + \t\t}\n + \t}\n + \n +-\tif (!refspecs_need_update)\n +-\t\tgoto out;\n +-\n +-\t/*\n -\t * First remove symrefs, then rename the rest, finally create\n -\t * the new symrefs.\n -+\t * Note that we're using two transactions to rename the references:\n -+\t *\n -+\t * - One transaction contains all deletions for references part of\n -+\t * the old remote.\n -+\t * - One transaction contains all creations of references and reflogs\n -+\t * part of the new remote.\n -+\t *\n -+\t * This split is required to avoid conflicting ref updates when a\n -+\t * remote is being nested into itself or converted into its parent\n -+\t * directory.\n -+\t *\n -+\t * Unfortunately this means that the operation isn't atomic. But we\n -+\t * cannot avoid that, unless transactions learn to handle such\n -+\t * conflicts one day.\n - \t */\n +-\t */\n -\trefs_for_each_ref(get_main_ref_store(the_repository),\n -\t\t\t read_remote_branches, &rename);\n -\tif (show_progress) {\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n -\t\t\t\t\t _(\"Renaming remote references\"),\n -\t\t\t\t\t rename.remote_branches->nr + rename.symrefs_nr);\n -\t}\n --\tfor (i = 0; i < remote_branches.nr; i++) {\n +-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n -\t\tstruct string_list_item *item = remote_branches.items + i;\n -\t\tstruct strbuf referent = STRBUF_INIT;\n -+\trename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),\n -+\t\t\t\t\t\t 0, &err);\n -+\tif (!rename.tx_delete)\n -+\t\tgoto out;\n - \n +-\n -\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n -\t\t\t\t\t &referent))\n -\t\t\tcontinue;\n -\t\tif (refs_delete_ref(get_main_ref_store(the_repository), NULL, item->string, NULL, REF_NO_DEREF))\n -\t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n -+\trename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),\n -+\t\t\t\t\t\t 0, &err);\n -+\tif (!rename.tx_create)\n -+\t\tgoto out;\n - \n +-\n -\t\tstrbuf_release(&referent);\n -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n -\t}\n --\tfor (i = 0; i < remote_branches.nr; i++) {\n +-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n -\t\tstruct string_list_item *item = remote_branches.items + i;\n -+\tif (show_progress)\n -+\t\trename.progress = start_delayed_progress(the_repository,\n -+\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n ++\tif (refspecs_need_update) {\n ++\t\tresult = ref_transaction_commit(rename.transaction, &err);\n ++\t\tif (result < 0)\n ++\t\t\tdie(_(\"renaming remote refs failed: %s\"), rename.err->buf);\n \n -\t\tif (item->util)\n -\t\t\tcontinue;\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n -\t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n -\t}\n --\tfor (i = 0; i < remote_branches.nr; i++) {\n +-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n -\t\tstruct string_list_item *item = remote_branches.items + i;\n -+\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n -+\t\t\t\t rename_one_ref, &rename);\n -+\tif (result < 0)\n -+\t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n ++\t\tstop_progress(&rename.progress);\n \n -\t\tif (!item->util)\n -\t\t\tcontinue;\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n -\t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf))\n -\t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n -\t\tdisplay_progress(progress, ++refs_renamed_nr);\n --\t}\n ++\t\thandle_push_default(rename.old_name, rename.new_name);\n + \t}\n -\tstop_progress(&progress);\n -+\tresult = ref_transaction_commit(rename.tx_delete, &err);\n -+\tif (result < 0)\n -+\t\tdie(_(\"deleting old remote refs failed: %s\"), rename.err->buf);\n -+\n -+\tresult = ref_transaction_commit(rename.tx_create, &err);\n -+\tif (result < 0)\n -+\t\tdie(_(\"committing new remote refs failed: %s\"), rename.err->buf);\n -+\n -+\tstop_progress(&rename.progress);\n - \n - \thandle_push_default(rename.old_name, rename.new_name);\n +-\n +-\thandle_push_default(rename.old_name, rename.new_name);\n \n out:\n -\tstring_list_clear(&remote_branches, 1);\n -+\tref_transaction_free(rename.tx_create);\n -+\tref_transaction_free(rename.tx_delete);\n ++\tref_transaction_free(rename.transaction);\n \tstrbuf_release(&old_remote_context);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n @@ t/t5505-remote.sh: test_expect_success 'forbid adding superset of existing remot\n +\tgit remote add parent url &&\n +\tgit update-ref refs/remotes/parent/child $COMMIT_ID &&\n +\ttest_when_finished \"git remote remove parent/child\" &&\n -+\tgit remote rename parent parent/child &&\n ++\ttest_must_fail git remote rename parent parent/child 2>err &&\n ++\ttest_grep \"renaming remote references failed\" err &&\n ++\ttest_grep \"The remote you are trying to rename has conflicting references\" err &&\n +\tgit for-each-ref refs/remotes/ >actual &&\n -+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/child\\n\" >expected &&\n ++\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child\\n\" >expected &&\n +\ttest_cmp expected actual\n +'\n +\n4: 6a68671b8f ! 6: 2fc020e25f builtin/remote: only iterate through refs that are to be renamed\n @@ Commit message\n but ultimately this is still a waste of compute as we knowingly iterate\n through references that we won't ever care about.\n \n - Improve this by introducing `refs_for_each_rawref_in()`, which knows to\n - only iterate through (potentially broken) references in a given prefix.\n + Improve this by using `refs_for_each_rawref_in()`, which knows to only\n + iterate through (potentially broken) references in a given prefix.\n \n The following benchmark renames a remote with a single reference in a\n repository that has 100k unrelated references. This shows a sizeable\n @@ Commit message\n \n ## builtin/remote.c ##\n @@ builtin/remote.c: static int rename_one_ref(const char *old_refname, const char *referent,\n - {\n + \tstruct strbuf new_referent = STRBUF_INIT;\n + \tstruct strbuf new_refname = STRBUF_INIT;\n \tstruct rename_info *rename = cb_data;\n - \tstruct strbuf *new_referent = rename->buf1;\n -\tconst char *ptr = old_refname;\n \tint error;\n \n @@ builtin/remote.c: static int rename_one_ref(const char *old_refname, const char\n -\t\tgoto out;\n -\t}\n -\n - \trenamed_refname(rename, old_refname, rename->new_refname);\n + \tcompute_renamed_ref(rename, old_refname, &new_refname);\n \n \tif (flags & REF_ISSYMREF) {\n @@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,\n - \t\trename.progress = start_delayed_progress(the_repository,\n - \t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n + \t\t\trename.progress = start_delayed_progress(the_repository,\n + \t\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n \n --\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n --\t\t\t\t rename_one_ref, &rename);\n -+\tstrbuf_reset(&buf);\n -+\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename.old_name);\n +-\t\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n ++\t\tstrbuf_reset(&buf);\n ++\t\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename.old_name);\n +\n -+\tresult = refs_for_each_rawref_in(get_main_ref_store(the_repository), buf.buf,\n -+\t\t\t\t\t rename_one_ref, &rename);\n - \tif (result < 0)\n - \t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n - \n ++\t\tresult = refs_for_each_rawref_in(get_main_ref_store(the_repository), buf.buf,\n + \t\t\t\trename_one_ref, &rename);\n + \t\tif (result < 0)\n + \t\t\tdie(_(\"queueing remote ref renames failed: %s\"), rename.err->buf);\n \n ## refs.c ##\n @@ refs.c: int refs_for_each_namespaced_ref(struct ref_store *refs,\n\n---\nbase-commit: 49e86c58e5687de1089b97288ec5d9582ee6ebd6\nchange-id: 20250725-pks-remote-rename-improvements-85ce262146e0\n\n"},{"id":"523098","messageId":"20250731-pks-remote-rename-improvements-v2-1-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 1/6] refs: pass refname when invoking reflog entry callback","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:49Z","receivedAt":"2025-07-31T14:57:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"With `refs_for_each_reflog_ent()` callers can iterate through all the\nreflog entries for a given reference. The callback that is being invoked\nfor each such entry does not receive the name of the reference that we\nare currently iterating through. This isn't really a limiting factor, as\ncallers can simply pass the name via the callback data.\n\nBut this layout sometimes does make for a bit of an awkward calling\npattern. One example: when iterating through all reflogs, and for each\nreflog we iterate through all refnames, we have to do some extra book\nkeeping to track which reference name we are currently yielding reflog\nentries for.\n\nChange the signature of the callback function so that the reference name\nof the reflog gets passed through to it. Adapt callers accordingly and\nstart using the new parameter in trivial cases. The next commit will\nrefactor the reference migration logic to make use of this parameter so\nthat we can simplify its logic a bit.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fsck.c | 9 ++++-----\n builtin/gc.c | 3 ++-\n builtin/stash.c | 6 ++++--\n commit.c | 3 ++-\n object-name.c | 3 ++-\n reflog-walk.c | 7 ++++---\n reflog.c | 3 ++-\n reflog.h | 3 ++-\n refs.c | 20 +++++++++-----------\n refs.h | 11 +++++++----\n refs/debug.c | 5 +++--\n refs/files-backend.c | 15 +++++++++------\n refs/reftable-backend.c | 2 +-\n remote.c | 6 ++++--\n revision.c | 3 ++-\n t/helper/test-ref-store.c | 3 ++-\n wt-status.c | 6 ++++--\n 17 files changed, 63 insertions(+), 45 deletions(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 0084cf7400b..67eb5e4fa0f 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -502,13 +502,12 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,\n \t}\n }\n \n-static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int fsck_handle_reflog_ent(const char *refname,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz UNUSED,\n-\t\t\t\t const char *message UNUSED, void *cb_data)\n+\t\t\t\t const char *message UNUSED, void *cb_data UNUSED)\n {\n-\tconst char *refname = cb_data;\n-\n \tif (verbose)\n \t\tfprintf_ln(stderr, _(\"Checking reflog %s->%s\"),\n \t\t\t oid_to_hex(ooid), oid_to_hex(noid));\n@@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)\n \tstrbuf_worktree_ref(cb_data, &refname, logname);\n \trefs_for_each_reflog_ent(get_main_ref_store(the_repository),\n \t\t\t\t refname.buf, fsck_handle_reflog_ent,\n-\t\t\t\t refname.buf);\n+\t\t\t\t NULL);\n \tstrbuf_release(&refname);\n \treturn 0;\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex fab8f4dd4f7..9ae87065d35 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -312,7 +312,8 @@ struct count_reflog_entries_data {\n \tsize_t limit;\n };\n \n-static int count_reflog_entries(struct object_id *old_oid, struct object_id *new_oid,\n+static int count_reflog_entries(const char *refname UNUSED,\n+\t\t\t\tstruct object_id *old_oid, struct object_id *new_oid,\n \t\t\t\tconst char *committer, timestamp_t timestamp,\n \t\t\t\tint tz, const char *msg, void *cb_data)\n {\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex e2f95cc2ebc..a1ed67661e3 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -738,7 +738,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix,\n \treturn ret;\n }\n \n-static int reject_reflog_ent(struct object_id *ooid UNUSED,\n+static int reject_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid UNUSED,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED,\n@@ -2173,7 +2174,8 @@ struct stash_entry_data {\n \tsize_t count;\n };\n \n-static int collect_stash_entries(struct object_id *old_oid UNUSED,\n+static int collect_stash_entries(const char *refname UNUSED,\n+\t\t\t\t struct object_id *old_oid UNUSED,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\ndiff --git a/commit.c b/commit.c\nindex 15115125c36..7ebd05f3527 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1031,7 +1031,8 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)\n \tcommit->object.flags |= TMP_MARK;\n }\n \n-static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int collect_one_reflog_ent(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *ident UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n \t\t\t\t const char *message UNUSED, void *cbdata)\ndiff --git a/object-name.c b/object-name.c\nindex ddafe7f9b13..9ec192c3731 100644\n--- a/object-name.c\n+++ b/object-name.c\n@@ -1516,7 +1516,8 @@ struct grab_nth_branch_switch_cbdata {\n \tstruct strbuf *sb;\n };\n \n-static int grab_nth_branch_switch(struct object_id *ooid UNUSED,\n+static int grab_nth_branch_switch(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid UNUSED,\n \t\t\t\t struct object_id *noid UNUSED,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex c7070b13b00..4f1ce047498 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -22,9 +22,10 @@ struct complete_reflogs {\n \tint nr, alloc;\n };\n \n-static int read_one_reflog(struct object_id *ooid, struct object_id *noid,\n-\t\tconst char *email, timestamp_t timestamp, int tz,\n-\t\tconst char *message, void *cb_data)\n+static int read_one_reflog(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n+\t\t\t const char *email, timestamp_t timestamp, int tz,\n+\t\t\t const char *message, void *cb_data)\n {\n \tstruct complete_reflogs *array = cb_data;\n \tstruct reflog_info *item;\ndiff --git a/reflog.c b/reflog.c\nindex 39c205fd26e..2264b3bd605 100644\n--- a/reflog.c\n+++ b/reflog.c\n@@ -492,7 +492,8 @@ void reflog_expiry_cleanup(void *cb_data)\n \tfree_commit_list(cb->mark_list);\n }\n \n-int count_reflog_ent(struct object_id *ooid UNUSED,\n+int count_reflog_ent(const char *refname UNUSED,\n+\t\t struct object_id *ooid UNUSED,\n \t\t struct object_id *noid UNUSED,\n \t\t const char *email UNUSED,\n \t\t timestamp_t timestamp, int tz UNUSED,\ndiff --git a/reflog.h b/reflog.h\nindex 63bb56280f4..44b306c08ae 100644\n--- a/reflog.h\n+++ b/reflog.h\n@@ -63,7 +63,8 @@ void reflog_expiry_prepare(const char *refname, const struct object_id *oid,\n int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email, timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data);\n-int count_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+int count_reflog_ent(const char *refname,\n+\t\t struct object_id *ooid, struct object_id *noid,\n \t\t const char *email, timestamp_t timestamp, int tz,\n \t\t const char *message, void *cb_data);\n int should_expire_reflog_ent_verbose(struct object_id *ooid,\ndiff --git a/refs.c b/refs.c\nindex 4bd80287054..6ed0cd6ddca 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1022,7 +1022,6 @@ int is_branch(const char *refname)\n }\n \n struct read_ref_at_cb {\n-\tconst char *refname;\n \ttimestamp_t at_time;\n \tint cnt;\n \tint reccnt;\n@@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,\n \t\t*cb->cutoff_cnt = cb->reccnt;\n }\n \n-static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n+static int read_ref_at_ent(const char *refname,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data)\n@@ -1072,14 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \t\t\toidcpy(cb->oid, noid);\n \t\t\tif (!oideq(&cb->ooid, noid))\n \t\t\t\twarning(_(\"log for ref %s has gap after %s\"),\n-\t\t\t\t\tcb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n+\t\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\t}\n \t\telse if (cb->date == cb->at_time)\n \t\t\toidcpy(cb->oid, noid);\n \t\telse if (!oideq(noid, cb->oid))\n \t\t\twarning(_(\"log for ref %s unexpectedly ended on %s\"),\n-\t\t\t\tcb->refname, show_date(cb->date, cb->tz,\n-\t\t\t\t\t\t DATE_MODE(RFC2822)));\n+\t\t\t\trefname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));\n \t\tcb->reccnt++;\n \t\toidcpy(&cb->ooid, ooid);\n \t\toidcpy(&cb->noid, noid);\n@@ -1094,7 +1093,8 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,\n \treturn 0;\n }\n \n-static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,\n+static int read_ref_at_ent_oldest(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz,\n \t\t\t\t const char *message, void *cb_data)\n@@ -1117,7 +1117,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,\n \tstruct read_ref_at_cb cb;\n \n \tmemset(&cb, 0, sizeof(cb));\n-\tcb.refname = refname;\n \tcb.at_time = at_time;\n \tcb.cnt = cnt;\n \tcb.msg = msg;\n@@ -2976,14 +2975,14 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n \n struct reflog_migration_data {\n \tuint64_t index;\n-\tconst char *refname;\n \tstruct ref_store *old_refs;\n \tstruct ref_transaction *transaction;\n \tstruct strbuf *errbuf;\n \tstruct strbuf *sb, *name, *mail;\n };\n \n-static int migrate_one_reflog_entry(struct object_id *old_oid,\n+static int migrate_one_reflog_entry(const char *refname,\n+\t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer,\n \t\t\t\t timestamp_t timestamp, int tz,\n@@ -3006,7 +3005,7 @@ static int migrate_one_reflog_entry(struct object_id *old_oid,\n \tstrbuf_reset(data->sb);\n \tstrbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));\n \n-\tret = ref_transaction_update_reflog(data->transaction, data->refname,\n+\tret = ref_transaction_update_reflog(data->transaction, refname,\n \t\t\t\t\t new_oid, old_oid, data->sb->buf,\n \t\t\t\t\t msg, data->index++, data->errbuf);\n \treturn ret;\n@@ -3016,7 +3015,6 @@ static int migrate_one_reflog(const char *refname, void *cb_data)\n {\n \tstruct migration_data *migration_data = cb_data;\n \tstruct reflog_migration_data data = {\n-\t\t.refname = refname,\n \t\t.old_refs = migration_data->old_refs,\n \t\t.transaction = migration_data->transaction,\n \t\t.errbuf = migration_data->errbuf,\ndiff --git a/refs.h b/refs.h\nindex 99b58d0b73c..0bf50ce25cc 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -558,10 +558,13 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);\n * The cb_data is a caller-supplied pointer given to the iterator\n * functions.\n */\n-typedef int each_reflog_ent_fn(\n-\t\tstruct object_id *old_oid, struct object_id *new_oid,\n-\t\tconst char *committer, timestamp_t timestamp,\n-\t\tint tz, const char *msg, void *cb_data);\n+typedef int each_reflog_ent_fn(const char *refname,\n+\t\t\t struct object_id *old_oid,\n+\t\t\t struct object_id *new_oid,\n+\t\t\t const char *committer,\n+\t\t\t timestamp_t timestamp,\n+\t\t\t int tz, const char *msg,\n+\t\t\t void *cb_data);\n \n /* Iterate over reflog entries in the log for `refname`. */\n \ndiff --git a/refs/debug.c b/refs/debug.c\nindex 485e3079d7a..5e113db307a 100644\n--- a/refs/debug.c\n+++ b/refs/debug.c\n@@ -276,7 +276,8 @@ struct debug_reflog {\n \tvoid *cb_data;\n };\n \n-static int debug_print_reflog_ent(struct object_id *old_oid,\n+static int debug_print_reflog_ent(const char *refname,\n+\t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n \t\t\t\t const char *committer, timestamp_t timestamp,\n \t\t\t\t int tz, const char *msg, void *cb_data)\n@@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,\n \tif (new_oid)\n \t\toid_to_hex_r(n, new_oid);\n \n-\tret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,\n+\tret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,\n \t\t dbg->cb_data);\n \ttrace_printf_key(&trace_refs,\n \t\t\t \"reflog_ent %s (ret %d): %s -> %s, %s %ld \\\"%.*s\\\"\\n\",\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3ebe0323d4e..24d0a8ebde0 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,\n \treturn ret;\n }\n \n-static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n+static int show_one_reflog_ent(struct files_ref_store *refs,\n+\t\t\t const char *refname,\n+\t\t\t struct strbuf *sb,\n \t\t\t each_reflog_ent_fn fn, void *cb_data)\n {\n \tstruct object_id ooid, noid;\n@@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,\n \t\tmessage += 6;\n \telse\n \t\tmessage += 7;\n-\treturn fn(&ooid, &noid, p, timestamp, tz, message, cb_data);\n+\treturn fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);\n }\n \n static char *find_beginning_of_line(char *bob, char *scan)\n@@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\tstrbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));\n \t\t\t\tscanp = bp;\n \t\t\t\tendp = bp + 1;\n-\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tif (ret)\n \t\t\t\t\tbreak;\n@@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,\n \t\t\t\t * Process it, and we can end the loop.\n \t\t\t\t */\n \t\t\t\tstrbuf_splice(&sb, 0, 0, buf, endp - buf);\n-\t\t\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\t\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \t\t\t\tstrbuf_reset(&sb);\n \t\t\t\tbreak;\n \t\t\t}\n@@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,\n \t\treturn -1;\n \n \twhile (!ret && !strbuf_getwholeline(&sb, logfp, '\\n'))\n-\t\tret = show_one_reflog_ent(refs, &sb, fn, cb_data);\n+\t\tret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);\n \tfclose(logfp);\n \tstrbuf_release(&sb);\n \treturn ret;\n@@ -3323,7 +3325,8 @@ struct expire_reflog_cb {\n \t\t dry_run:1;\n };\n \n-static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int expire_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t const char *email, timestamp_t timestamp, int tz,\n \t\t\t const char *message, void *cb_data)\n {\ndiff --git a/refs/reftable-backend.c b/refs/reftable-backend.c\nindex 99fafd75ebe..25a1d516184 100644\n--- a/refs/reftable-backend.c\n+++ b/refs/reftable-backend.c\n@@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,\n \n \tfull_committer = fmt_ident(log->value.update.name, log->value.update.email,\n \t\t\t\t WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);\n-\treturn fn(&old_oid, &new_oid, full_committer,\n+\treturn fn(log->refname, &old_oid, &new_oid, full_committer,\n \t\t log->value.update.time, log->value.update.tz_offset,\n \t\t log->value.update.message, cb_data);\n }\ndiff --git a/remote.c b/remote.c\nindex e965f022f12..db9eea4fa45 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -2578,7 +2578,8 @@ struct check_and_collect_until_cb_data {\n };\n \n /* Get the timestamp of the latest entry. */\n-static int peek_reflog(struct object_id *o_oid UNUSED,\n+static int peek_reflog(const char *refname UNUSED,\n+\t\t struct object_id *o_oid UNUSED,\n \t\t struct object_id *n_oid UNUSED,\n \t\t const char *ident UNUSED,\n \t\t timestamp_t timestamp, int tz UNUSED,\n@@ -2589,7 +2590,8 @@ static int peek_reflog(struct object_id *o_oid UNUSED,\n \treturn 1;\n }\n \n-static int check_and_collect_until(struct object_id *o_oid UNUSED,\n+static int check_and_collect_until(const char *refname UNUSED,\n+\t\t\t\t struct object_id *o_oid UNUSED,\n \t\t\t\t struct object_id *n_oid,\n \t\t\t\t const char *ident UNUSED,\n \t\t\t\t timestamp_t timestamp, int tz UNUSED,\ndiff --git a/revision.c b/revision.c\nindex 212ca0de276..0fc1a167a10 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1699,7 +1699,8 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)\n \t}\n }\n \n-static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,\n+static int handle_one_reflog_ent(const char *refname UNUSED,\n+\t\t\t\t struct object_id *ooid, struct object_id *noid,\n \t\t\t\t const char *email UNUSED,\n \t\t\t\t timestamp_t timestamp UNUSED,\n \t\t\t\t int tz UNUSED,\ndiff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c\nindex 8d9a271845c..b2380d57ba3 100644\n--- a/t/helper/test-ref-store.c\n+++ b/t/helper/test-ref-store.c\n@@ -215,7 +215,8 @@ static int cmd_for_each_reflog(struct ref_store *refs,\n \treturn refs_for_each_reflog(refs, each_reflog, NULL);\n }\n \n-static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,\n+static int each_reflog_ent(const char *refname UNUSED,\n+\t\t\t struct object_id *old_oid, struct object_id *new_oid,\n \t\t\t const char *committer, timestamp_t timestamp,\n \t\t\t int tz, const char *msg, void *cb_data UNUSED)\n {\ndiff --git a/wt-status.c b/wt-status.c\nindex 454601afa15..71bd17b610a 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -972,7 +972,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)\n \twt_longstatus_print_trailer(s);\n }\n \n-static int stash_count_refs(struct object_id *ooid UNUSED,\n+static int stash_count_refs(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid UNUSED,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n@@ -1664,7 +1665,8 @@ struct grab_1st_switch_cbdata {\n \tstruct object_id noid;\n };\n \n-static int grab_1st_switch(struct object_id *ooid UNUSED,\n+static int grab_1st_switch(const char *refname UNUSED,\n+\t\t\t struct object_id *ooid UNUSED,\n \t\t\t struct object_id *noid,\n \t\t\t const char *email UNUSED,\n \t\t\t timestamp_t timestamp UNUSED, int tz UNUSED,\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523099","messageId":"20250731-pks-remote-rename-improvements-v2-2-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 2/6] refs: simplify logic when migrating reflog entries","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:50Z","receivedAt":"2025-07-31T14:57:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When migrating reflog entries between two storage formats we have to do\nso via two callback-driven functions:\n\n - `migrate_one_reflog()` gets invoked via `refs_for_each_reflog()` to\n first list all available reflogs.\n\n - `migrate_one_reflog_entry()` gets invoked via\n `refs_for_each_reflog_ent()` in `migrate_one_reflog()`.\n\nBefore the preceding commit we didn't have the refname available in\n`migrate_one_reflog_entry()`, which made it necessary to have a separate\nstructure that we pass to the second callback so that we can propagate\nthe refname. Now that `refs_for_each_reflog_ent()` knows to pass the\nrefname to the callback though that indirection isn't necessary anymore.\n\nThere's one catch though: we do have an update index that is also stored\nin the entry-specific callback data. This update index is required so\nthat we can tell the ref backend in which order it should persist the\nreflog entries to disk.\n\nBut that purpose can be trivially achieved by just converting it into a\nglobal counter that is used for all reflog entries, regardless of which\nreference they are for. The ordering will remain the same as both the\nupdate index and the refname is considered when sorting the entries.\n\nMove the index into `struct migration_data` and drop the now-unused\n`struct reflog_migration_data` to simplify the code a bit.\n\nSigned-off-by: Patrick Steinhardt \n---\n refs.c | 36 ++++++++++--------------------------\n 1 file changed, 10 insertions(+), 26 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6ed0cd6ddc..04c9ace793 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2941,6 +2941,7 @@ struct migration_data {\n \tstruct ref_transaction *transaction;\n \tstruct strbuf *errbuf;\n \tstruct strbuf sb, name, mail;\n+\tuint64_t index;\n };\n \n static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,\n@@ -2973,14 +2974,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con\n \treturn ret;\n }\n \n-struct reflog_migration_data {\n-\tuint64_t index;\n-\tstruct ref_store *old_refs;\n-\tstruct ref_transaction *transaction;\n-\tstruct strbuf *errbuf;\n-\tstruct strbuf *sb, *name, *mail;\n-};\n-\n static int migrate_one_reflog_entry(const char *refname,\n \t\t\t\t struct object_id *old_oid,\n \t\t\t\t struct object_id *new_oid,\n@@ -2988,7 +2981,7 @@ static int migrate_one_reflog_entry(const char *refname,\n \t\t\t\t timestamp_t timestamp, int tz,\n \t\t\t\t const char *msg, void *cb_data)\n {\n-\tstruct reflog_migration_data *data = cb_data;\n+\tstruct migration_data *data = cb_data;\n \tstruct ident_split ident;\n \tconst char *date;\n \tint ret;\n@@ -2996,17 +2989,17 @@ static int migrate_one_reflog_entry(const char *refname,\n \tif (split_ident_line(&ident, committer, strlen(committer)) < 0)\n \t\treturn -1;\n \n-\tstrbuf_reset(data->name);\n-\tstrbuf_add(data->name, ident.name_begin, ident.name_end - ident.name_begin);\n-\tstrbuf_reset(data->mail);\n-\tstrbuf_add(data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\tstrbuf_reset(&data->name);\n+\tstrbuf_add(&data->name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_reset(&data->mail);\n+\tstrbuf_add(&data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n \n \tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n-\tstrbuf_reset(data->sb);\n-\tstrbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));\n+\tstrbuf_reset(&data->sb);\n+\tstrbuf_addstr(&data->sb, fmt_ident(data->name.buf, data->mail.buf, WANT_BLANK_IDENT, date, 0));\n \n \tret = ref_transaction_update_reflog(data->transaction, refname,\n-\t\t\t\t\t new_oid, old_oid, data->sb->buf,\n+\t\t\t\t\t new_oid, old_oid, data->sb.buf,\n \t\t\t\t\t msg, data->index++, data->errbuf);\n \treturn ret;\n }\n@@ -3014,17 +3007,8 @@ static int migrate_one_reflog_entry(const char *refname,\n static int migrate_one_reflog(const char *refname, void *cb_data)\n {\n \tstruct migration_data *migration_data = cb_data;\n-\tstruct reflog_migration_data data = {\n-\t\t.old_refs = migration_data->old_refs,\n-\t\t.transaction = migration_data->transaction,\n-\t\t.errbuf = migration_data->errbuf,\n-\t\t.sb = &migration_data->sb,\n-\t\t.name = &migration_data->name,\n-\t\t.mail = &migration_data->mail,\n-\t};\n-\n \treturn refs_for_each_reflog_ent(migration_data->old_refs, refname,\n-\t\t\t\t\tmigrate_one_reflog_entry, &data);\n+\t\t\t\t\tmigrate_one_reflog_entry, migration_data);\n }\n \n static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf)\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523100","messageId":"20250731-pks-remote-rename-improvements-v2-3-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 3/6] builtin/remote: fix sign comparison warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:51Z","receivedAt":"2025-07-31T14:57:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix -Wsign-comparison warnings. All of the warnings we have are about\nmismatches in signedness for loop counters. These are trivially fixable\nby using the correct integer type.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 54 +++++++++++++++++++++++-------------------------------\n 1 file changed, 23 insertions(+), 31 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5dd6cbbaee..f63c5eb888 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1,5 +1,4 @@\n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"config.h\"\n@@ -182,7 +181,6 @@ static int add(int argc, const char **argv, const char *prefix,\n \tstruct remote *remote;\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT;\n \tconst char *name, *url;\n-\tint i;\n \tint result = 0;\n \n \tstruct option options[] = {\n@@ -233,7 +231,7 @@ static int add(int argc, const char **argv, const char *prefix,\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", name);\n \t\tif (track.nr == 0)\n \t\t\tstring_list_append(&track, \"*\");\n-\t\tfor (i = 0; i < track.nr; i++) {\n+\t\tfor (size_t i = 0; i < track.nr; i++) {\n \t\t\tadd_branch(buf.buf, track.items[i].string,\n \t\t\t\t name, mirror, &buf2);\n \t\t}\n@@ -647,18 +645,17 @@ static int read_remote_branches(const char *refname, const char *referent UNUSED\n static int migrate_file(struct remote *remote)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint i;\n \n \tstrbuf_addf(&buf, \"remote.%s.url\", remote->name);\n-\tfor (i = 0; i < remote->url.nr; i++)\n+\tfor (size_t i = 0; i < remote->url.nr; i++)\n \t\tgit_config_set_multivar(buf.buf, remote->url.v[i], \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.push\", remote->name);\n-\tfor (i = 0; i < remote->push.nr; i++)\n+\tfor (int i = 0; i < remote->push.nr; i++)\n \t\tgit_config_set_multivar(buf.buf, remote->push.items[i].raw, \"^$\", 0);\n \tstrbuf_reset(&buf);\n \tstrbuf_addf(&buf, \"remote.%s.fetch\", remote->name);\n-\tfor (i = 0; i < remote->fetch.nr; i++)\n+\tfor (int i = 0; i < remote->fetch.nr; i++)\n \t\tgit_config_set_multivar(buf.buf, remote->fetch.items[i].raw, \"^$\", 0);\n #ifndef WITH_BREAKING_CHANGES\n \tif (remote->origin == REMOTE_REMOTES)\n@@ -744,7 +741,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n-\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n+\tint refs_renamed_nr = 0, refspec_updated = 0;\n \tstruct progress *progress = NULL;\n \tint result = 0;\n \n@@ -790,7 +787,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n \t\tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old_name);\n-\t\tfor (i = 0; i < oldremote->fetch.nr; i++) {\n+\t\tfor (int i = 0; i < oldremote->fetch.nr; i++) {\n \t\t\tchar *ptr;\n \n \t\t\tstrbuf_reset(&buf2);\n@@ -813,7 +810,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t}\n \n \tread_branches();\n-\tfor (i = 0; i < branch_list.nr; i++) {\n+\tfor (size_t i = 0; i < branch_list.nr; i++) {\n \t\tstruct string_list_item *item = branch_list.items + i;\n \t\tstruct branch_info *info = item->util;\n \t\tif (info->remote_name && !strcmp(info->remote_name, rename.old_name)) {\n@@ -846,7 +843,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t\t\t\t _(\"Renaming remote references\"),\n \t\t\t\t\t rename.remote_branches->nr + rename.symrefs_nr);\n \t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n+\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \t\tstruct strbuf referent = STRBUF_INIT;\n \n@@ -859,7 +856,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\tstrbuf_release(&referent);\n \t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n+\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \n \t\tif (item->util)\n@@ -875,7 +872,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n \t\tdisplay_progress(progress, ++refs_renamed_nr);\n \t}\n-\tfor (i = 0; i < remote_branches.nr; i++) {\n+\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n \t\tstruct string_list_item *item = remote_branches.items + i;\n \n \t\tif (!item->util)\n@@ -920,7 +917,7 @@ static int rm(int argc, const char **argv, const char *prefix,\n \tstruct string_list branches = STRING_LIST_INIT_DUP;\n \tstruct string_list skipped = STRING_LIST_INIT_DUP;\n \tstruct branches_for_remote cb_data;\n-\tint i, result;\n+\tint result;\n \n \tmemset(&cb_data, 0, sizeof(cb_data));\n \tcb_data.branches = &branches;\n@@ -942,7 +939,7 @@ static int rm(int argc, const char **argv, const char *prefix,\n \tfor_each_remote(add_known_remote, &known_remotes);\n \n \tread_branches();\n-\tfor (i = 0; i < branch_list.nr; i++) {\n+\tfor (size_t i = 0; i < branch_list.nr; i++) {\n \t\tstruct string_list_item *item = branch_list.items + i;\n \t\tstruct branch_info *info = item->util;\n \t\tif (info->remote_name && !strcmp(info->remote_name, remote->name)) {\n@@ -988,7 +985,7 @@ static int rm(int argc, const char **argv, const char *prefix,\n \t\t\t \"Note: Some branches outside the refs/remotes/ hierarchy were not removed;\\n\"\n \t\t\t \"to delete them, use:\",\n \t\t\t skipped.nr));\n-\t\tfor (i = 0; i < skipped.nr; i++)\n+\t\tfor (size_t i = 0; i < skipped.nr; i++)\n \t\t\tfprintf(stderr, \" git branch -d %s\\n\",\n \t\t\t\tskipped.items[i].string);\n \t}\n@@ -1166,7 +1163,6 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data)\n \tstruct branch_info *branch_info = item->util;\n \tstruct string_list *merge = &branch_info->merge;\n \tint width = show_info->width + 4;\n-\tint i;\n \n \tif (branch_info->rebase >= REBASE_TRUE && branch_info->merge.nr > 1) {\n \t\terror(_(\"invalid branch.%s.merge; cannot rebase onto > 1 branch\"),\n@@ -1192,7 +1188,7 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data)\n \t} else {\n \t\tprintf_ln(_(\"merges with remote %s\"), merge->items[0].string);\n \t}\n-\tfor (i = 1; i < merge->nr; i++)\n+\tfor (size_t i = 1; i < merge->nr; i++)\n \t\tprintf(_(\"%-*s and with remote %s\\n\"), width, \"\",\n \t\t merge->items[i].string);\n \n@@ -1277,7 +1273,6 @@ static int get_one_entry(struct remote *remote, void *priv)\n \tstruct string_list *list = priv;\n \tstruct strbuf remote_info_buf = STRBUF_INIT;\n \tstruct strvec *url;\n-\tint i;\n \n \tif (remote->url.nr > 0) {\n \t\tstruct strbuf promisor_config = STRBUF_INIT;\n@@ -1294,8 +1289,7 @@ static int get_one_entry(struct remote *remote, void *priv)\n \t} else\n \t\tstring_list_append(list, remote->name)->util = NULL;\n \turl = push_url_of_remote(remote);\n-\tfor (i = 0; i < url->nr; i++)\n-\t{\n+\tfor (size_t i = 0; i < url->nr; i++) {\n \t\tstrbuf_addf(&remote_info_buf, \"%s (push)\", url->v[i]);\n \t\tstring_list_append(list, remote->name)->util =\n \t\t\t\tstrbuf_detach(&remote_info_buf, NULL);\n@@ -1312,10 +1306,8 @@ static int show_all(void)\n \tresult = for_each_remote(get_one_entry, &list);\n \n \tif (!result) {\n-\t\tint i;\n-\n \t\tstring_list_sort(&list);\n-\t\tfor (i = 0; i < list.nr; i++) {\n+\t\tfor (size_t i = 0; i < list.nr; i++) {\n \t\t\tstruct string_list_item *item = list.items + i;\n \t\t\tif (verbose)\n \t\t\t\tprintf(\"%s\\t%s\\n\", item->string,\n@@ -1352,7 +1344,7 @@ static int show(int argc, const char **argv, const char *prefix,\n \t\tquery_flag = (GET_REF_STATES | GET_HEAD_NAMES | GET_PUSH_REF_STATES);\n \n \tfor (; argc; argc--, argv++) {\n-\t\tint i;\n+\t\tsize_t i;\n \t\tstruct strvec *url;\n \n \t\tget_remote_ref_states(*argv, &info.states, query_flag);\n@@ -1458,7 +1450,7 @@ static void report_set_head_auto(const char *remote, const char *head_name,\n static int set_head(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n-\tint i, opt_a = 0, opt_d = 0, result = 0, was_detached;\n+\tint opt_a = 0, opt_d = 0, result = 0, was_detached;\n \tstruct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,\n \t\tb_local_head = STRBUF_INIT;\n \tchar *head_name = NULL;\n@@ -1489,7 +1481,7 @@ static int set_head(int argc, const char **argv, const char *prefix,\n \t\telse if (states.heads.nr > 1) {\n \t\t\tresult |= error(_(\"Multiple remote HEAD branches. \"\n \t\t\t\t\t \"Please choose one explicitly with:\"));\n-\t\t\tfor (i = 0; i < states.heads.nr; i++)\n+\t\t\tfor (size_t i = 0; i < states.heads.nr; i++)\n \t\t\t\tfprintf(stderr, \" git remote set-head %s %s\\n\",\n \t\t\t\t\targv[0], states.heads.items[i].string);\n \t\t} else\n@@ -1714,7 +1706,7 @@ static int set_branches(int argc, const char **argv, const char *prefix,\n static int get_url(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n-\tint i, push_mode = 0, all_mode = 0;\n+\tint push_mode = 0, all_mode = 0;\n \tconst char *remotename = NULL;\n \tstruct remote *remote;\n \tstruct strvec *url;\n@@ -1742,7 +1734,7 @@ static int get_url(int argc, const char **argv, const char *prefix,\n \turl = push_mode ? push_url_of_remote(remote) : &remote->url;\n \n \tif (all_mode) {\n-\t\tfor (i = 0; i < url->nr; i++)\n+\t\tfor (size_t i = 0; i < url->nr; i++)\n \t\t\tprintf_ln(\"%s\", url->v[i]);\n \t} else {\n \t\tprintf_ln(\"%s\", url->v[0]);\n@@ -1754,7 +1746,7 @@ static int get_url(int argc, const char **argv, const char *prefix,\n static int set_url(int argc, const char **argv, const char *prefix,\n \t\t struct repository *repo UNUSED)\n {\n-\tint i, push_mode = 0, add_mode = 0, delete_mode = 0;\n+\tint push_mode = 0, add_mode = 0, delete_mode = 0;\n \tint matches = 0, negative_matches = 0;\n \tconst char *remotename = NULL;\n \tconst char *newurl = NULL;\n@@ -1818,7 +1810,7 @@ static int set_url(int argc, const char **argv, const char *prefix,\n \tif (regcomp(&old_regex, oldurl, REG_EXTENDED))\n \t\tdie(_(\"Invalid old URL pattern: %s\"), oldurl);\n \n-\tfor (i = 0; i < urlset->nr; i++)\n+\tfor (size_t i = 0; i < urlset->nr; i++)\n \t\tif (!regexec(&old_regex, urlset->v[i], 0, NULL, 0))\n \t\t\tmatches++;\n \t\telse\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523101","messageId":"20250731-pks-remote-rename-improvements-v2-4-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 4/6] builtin/remote: determine whether refs need renaming early on","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:52Z","receivedAt":"2025-07-31T14:57:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When renaming a remote we may have to also rename remote refs in case\nthe refspec changes. Pull out this computation into a separate loop.\nWhile that seems nonsensical right now, it'll help us in a subsequent\ncommit where we will prepare the reference transaction before we rewrite\nthe configuration.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 12 ++++++++----\n 1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex f63c5eb888..34ddcaf5f6 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -741,7 +741,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\told_remote_context = STRBUF_INIT;\n \tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n \tstruct rename_info rename;\n-\tint refs_renamed_nr = 0, refspec_updated = 0;\n+\tint refs_renamed_nr = 0, refspecs_need_update = 0;\n \tstruct progress *progress = NULL;\n \tint result = 0;\n \n@@ -782,11 +782,16 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\tgoto out;\n \t}\n \n+\tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old_name);\n+\n+\tfor (int i = 0; i < oldremote->fetch.nr && !refspecs_need_update; i++)\n+\t\trefspecs_need_update = !!strstr(oldremote->fetch.items[i].raw,\n+\t\t\t\t\t\told_remote_context.buf);\n+\n \tif (oldremote->fetch.nr) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n \t\tgit_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE);\n-\t\tstrbuf_addf(&old_remote_context, \":refs/remotes/%s/\", rename.old_name);\n \t\tfor (int i = 0; i < oldremote->fetch.nr; i++) {\n \t\t\tchar *ptr;\n \n@@ -794,7 +799,6 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t\tstrbuf_addstr(&buf2, oldremote->fetch.items[i].raw);\n \t\t\tptr = strstr(buf2.buf, old_remote_context.buf);\n \t\t\tif (ptr) {\n-\t\t\t\trefspec_updated = 1;\n \t\t\t\tstrbuf_splice(&buf2,\n \t\t\t\t\t ptr-buf2.buf + strlen(\":refs/remotes/\"),\n \t\t\t\t\t strlen(rename.old_name), rename.new_name,\n@@ -825,7 +829,7 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\tif (!refspec_updated)\n+\tif (!refspecs_need_update)\n \t\tgoto out;\n \n \t/*\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523102","messageId":"20250731-pks-remote-rename-improvements-v2-5-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 5/6] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:53Z","receivedAt":"2025-07-31T14:57:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"It was recently reported [1] that renaming a remote that has dangling\nsymrefs is broken. This issue can be trivially reproduced:\n\n $ git init repo\n Initialized empty Git repository in /tmp/repo/.git/\n $ cd repo/\n $ git remote add origin /dev/null\n $ git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master\n $ git remote rename origin renamed\n $ git symbolic-ref refs/remotes/origin/HEAD\n refs/remotes/origin/master\n $ git symbolic-ref refs/remotes/renamed/HEAD\n fatal: ref refs/remotes/renamed/HEAD is not a symbolic ref\n\nAs one can see, the \"HEAD\" reference did not get renamed but stays in\nthe same place. There are two issues here:\n\n - We use `refs_resolve_ref_unsafe()` to resolve references, but we\n don't pass the `RESOLVE_REF_NO_RECURSE` flag. Consequently, if the\n reference does not resolve, the function will fail and we thus\n ignore this branch.\n\n - We use `refs_for_each_ref()` to iterate through the old remote's\n references, but that function ignores broken references.\n\nBoth of these issues are easy to fix. But having a closer look at the\nlogic that renames remote references surfaces that it leaves a lot to be\ndesired overall.\n\nThe problem is that we're using O(|refs| + |symrefs| * 2) many reference\ntransactions to perform the renames. We first delete all symrefs, then\nindividually rename every direct reference and finally we recreate the\nsymrefs. On the one hand this isn't even remotely an atomic operation,\nso if we hit any error we'll already have deleted all references.\n\nBut more importantly it is also extremely inefficient. The number of\ntransactions for symrefs doesn't really bother us too much, as there\nshould generally only be a single symref anyway (\"HEAD\"). But the\nrenames are very expensive:\n\n - For the \"reftable\" backend we perform auto-compaction after every\n single rename, which does add up.\n\n - For the \"files\" backend we potentially have to rewrite the\n \"packed-refs\" file on every single rename in case they are packed.\n The consequence here is quadratic runtime performance. Renaming a\n 100k references takes hours to complete.\n\nRefactor the code to use a single transaction to perform all the\nreference updates atomically, which speeds up the transaction quite\nsignificantly:\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s]\n Range (min … max): 204.863 s … 247.699 s 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s]\n Range (min … max): 2.011 s … 2.141 s 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~)\n\nFor the \"reftable\" backend we see a significant speedup, as well, but\ngiven that we don't have quadratic runtime behaviour there it's way less\nextreme:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s]\n Range (min … max): 7.880 s … 9.556 s 10 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s]\n Range (min … max): 1.023 s … 1.410 s 10 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nThere is one issue though with using atomic transactions: when nesting a\nremote into itself it can happen that renamed references conflict with\nthe old referencse. For example, when we have a reference\n\"refs/remotes/origin/foo\" and we rename \"origin\" to \"origin/foo\", then\nwe'll end up with an F/D conflict when we try to create the renamed\nreference \"refs/remotes/origin/foo/foo\".\n\nThis situation is overall quite unlikely to happen: people tend to not\nuse nested remotes, and if they do they must at the same time also have\na conflicting refname. But the end result would be that the old remote\nreferences stay intact whereas all the other parts of the repository\nhave been adjusted for the new remote name.\n\nAddress this by queueing and preparing the reference update before we\ntouch any other part of the repository. Like this we can make sure that\nthe reference update will go through before rewriting the configuration.\nOtherwise, if the transaction fails to prepare we can gracefully abort\nthe whole operation without any changes having been performed in the\nrepository yet. Furthermore, we can detect the conflict and print some\nhelpful advice for how the user can resolve this situation. So overall,\nthe tradeoff is that:\n\n - Reference transactions are now all-or-nothing. This is a significant\n improvement over the previous state where we may have ended up with\n partially-renamed references.\n\n - Rewriting references is now significantly faster.\n\n - We only rewrite the configuration in case we know that all\n references can be updated.\n\n - But we may refuse to rename a remote in case references conflict.\n\nOverall this seems like an acceptable tradeoff.\n\nWhile at it, fix the handling of symbolic/broken references by using\n`refs_for_each_rawref()`. Add tests that cover both this reported issue\nand tests that exercise nesting of remotes.\n\nOne thing to note: with this change we cannot provide a proper progress\nmonitor anymore as we queue the references into the transactions as we\niterate through them. Consequently, as we don't know yet how many refs\nthere are in total, we cannot report how many percent of the operation\nis done anymore. But that's a small price to pay considering that you\nnow shouldn't need the progress monitor in most situations at all\nanymore.\n\n[1]: \n\nReported-by: Han Jiang \nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 296 ++++++++++++++++++++++++++++++++++++------------------\n t/t5505-remote.sh | 73 ++++++++++++++\n 2 files changed, 270 insertions(+), 99 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 34ddcaf5f6..db481f39bc 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -1,8 +1,11 @@\n #define USE_THE_REPOSITORY_VARIABLE\n \n #include \"builtin.h\"\n+#include \"advice.h\"\n #include \"config.h\"\n+#include \"date.h\"\n #include \"gettext.h\"\n+#include \"ident.h\"\n #include \"parse-options.h\"\n #include \"path.h\"\n #include \"transport.h\"\n@@ -610,36 +613,161 @@ static int add_branch_for_removal(const char *refname,\n struct rename_info {\n \tconst char *old_name;\n \tconst char *new_name;\n-\tstruct string_list *remote_branches;\n-\tuint32_t symrefs_nr;\n+\tstruct ref_transaction *transaction;\n+\tstruct progress *progress;\n+\tstruct strbuf *err;\n+\tuint32_t progress_nr;\n+\tuint64_t index;\n };\n \n-static int read_remote_branches(const char *refname, const char *referent UNUSED,\n-\t\t\t\tconst struct object_id *oid UNUSED,\n-\t\t\t\tint flags UNUSED, void *cb_data)\n+static void compute_renamed_ref(struct rename_info *rename,\n+\t\t\t\tconst char *refname,\n+\t\t\t\tstruct strbuf *out)\n+{\n+\tstrbuf_reset(out);\n+\tstrbuf_addstr(out, refname);\n+\tstrbuf_splice(out, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n+\t\t rename->new_name, strlen(rename->new_name));\n+}\n+\n+static int rename_one_reflog_entry(const char *old_refname,\n+\t\t\t\t struct object_id *old_oid,\n+\t\t\t\t struct object_id *new_oid,\n+\t\t\t\t const char *committer,\n+\t\t\t\t timestamp_t timestamp, int tz,\n+\t\t\t\t const char *msg, void *cb_data)\n {\n \tstruct rename_info *rename = cb_data;\n-\tstruct strbuf buf = STRBUF_INIT;\n-\tstruct string_list_item *item;\n-\tint flag;\n-\tconst char *symref;\n-\n-\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old_name);\n-\tif (starts_with(refname, buf.buf)) {\n-\t\titem = string_list_append(rename->remote_branches, refname);\n-\t\tsymref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n-\t\t\t\t\t\t refname, RESOLVE_REF_READING,\n-\t\t\t\t\t\t NULL, &flag);\n-\t\tif (symref && (flag & REF_ISSYMREF)) {\n-\t\t\titem->util = xstrdup(symref);\n-\t\t\trename->symrefs_nr++;\n-\t\t} else {\n-\t\t\titem->util = NULL;\n-\t\t}\n+\tstruct strbuf new_refname = STRBUF_INIT;\n+\tstruct strbuf identity = STRBUF_INIT;\n+\tstruct strbuf name = STRBUF_INIT;\n+\tstruct strbuf mail = STRBUF_INIT;\n+\tstruct ident_split ident;\n+\tconst char *date;\n+\tint error;\n+\n+\tcompute_renamed_ref(rename, old_refname, &new_refname);\n+\n+\tif (split_ident_line(&ident, committer, strlen(committer)) < 0) {\n+\t\terror = -1;\n+\t\tgoto out;\n \t}\n-\tstrbuf_release(&buf);\n \n-\treturn 0;\n+\tstrbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin);\n+\tstrbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin);\n+\n+\tdate = show_date(timestamp, tz, DATE_MODE(NORMAL));\n+\tstrbuf_addstr(&identity, fmt_ident(name.buf, mail.buf,\n+\t\t\t\t\t WANT_BLANK_IDENT, date, 0));\n+\n+\terror = ref_transaction_update_reflog(rename->transaction, new_refname.buf,\n+\t\t\t\t\t new_oid, old_oid, identity.buf, msg,\n+\t\t\t\t\t rename->index++, rename->err);\n+\n+out:\n+\tstrbuf_release(&new_refname);\n+\tstrbuf_release(&identity);\n+\tstrbuf_release(&name);\n+\tstrbuf_release(&mail);\n+\treturn error;\n+}\n+\n+static int rename_one_reflog(const char *old_refname,\n+\t\t\t const struct object_id *old_oid,\n+\t\t\t struct rename_info *rename)\n+{\n+\tstruct strbuf new_refname = STRBUF_INIT;\n+\tstruct strbuf message = STRBUF_INIT;\n+\tint error;\n+\n+\tif (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname))\n+\t\treturn 0;\n+\n+\terror = refs_for_each_reflog_ent(get_main_ref_store(the_repository),\n+\t\t\t\t\t old_refname, rename_one_reflog_entry, rename);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\tcompute_renamed_ref(rename, old_refname, &new_refname);\n+\n+\t/*\n+\t * Manually write the reflog entry for the now-renamed ref. We cannot\n+\t * rely on `rename_one_ref()` to do this for us as that would screw\n+\t * over order in which reflog entries are being written.\n+\t *\n+\t * Furthermore, we only append the entry in case the reference\n+\t * resolves. Missing references shouldn't have reflogs anyway.\n+\t */\n+\tstrbuf_addf(&message, \"remote: renamed %s to %s\", old_refname,\n+\t\t new_refname.buf);\n+\n+\terror = ref_transaction_update_reflog(rename->transaction, new_refname.buf,\n+\t\t\t\t\t old_oid, old_oid, git_committer_info(0),\n+\t\t\t\t\t message.buf, rename->index++, rename->err);\n+\tif (error < 0)\n+\t\treturn error;\n+\n+out:\n+\tstrbuf_release(&new_refname);\n+\tstrbuf_release(&message);\n+\treturn error;\n+}\n+\n+static int rename_one_ref(const char *old_refname, const char *referent,\n+\t\t\t const struct object_id *oid,\n+\t\t\t int flags, void *cb_data)\n+{\n+\tstruct strbuf new_referent = STRBUF_INIT;\n+\tstruct strbuf new_refname = STRBUF_INIT;\n+\tstruct rename_info *rename = cb_data;\n+\tconst char *ptr = old_refname;\n+\tint error;\n+\n+\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n+\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n+\t !skip_prefix(ptr, \"/\", &ptr)) {\n+\t\terror = 0;\n+\t\tgoto out;\n+\t}\n+\n+\tcompute_renamed_ref(rename, old_refname, &new_refname);\n+\n+\tif (flags & REF_ISSYMREF) {\n+\t\t/*\n+\t\t * Stupidly enough `referent` is not pointing to the immediate\n+\t\t * target of a symref, but it's the recursively resolved value.\n+\t\t * So symrefs pointing to symrefs would be misresolved, and\n+\t\t * unborn symrefs don't have any value for the `referent` at all.\n+\t\t */\n+\t\treferent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t old_refname, RESOLVE_REF_NO_RECURSE,\n+\t\t\t\t\t\t NULL, NULL);\n+\t\tcompute_renamed_ref(rename, referent, &new_referent);\n+\t\toid = NULL;\n+\t}\n+\n+\terror = ref_transaction_delete(rename->transaction, old_refname,\n+\t\t\t\t oid, referent, REF_NO_DEREF, NULL, rename->err);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\terror = ref_transaction_update(rename->transaction, new_refname.buf, oid, null_oid(the_hash_algo),\n+\t\t\t\t (flags & REF_ISSYMREF) ? new_referent.buf : NULL, NULL,\n+\t\t\t\t REF_SKIP_CREATE_REFLOG | REF_NO_DEREF | REF_SKIP_OID_VERIFICATION,\n+\t\t\t\t NULL, rename->err);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\terror = rename_one_reflog(old_refname, oid, rename);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\tdisplay_progress(rename->progress, ++rename->progress_nr);\n+\n+out:\n+\tstrbuf_release(&new_referent);\n+\tstrbuf_release(&new_refname);\n+\treturn error;\n }\n \n static int migrate_file(struct remote *remote)\n@@ -727,6 +855,14 @@ static void handle_push_default(const char* old_name, const char* new_name)\n \tstrbuf_release(&push_default.origin);\n }\n \n+static const char conflicting_remote_refs_advice[] = N_(\n+\t\"The remote you are trying to rename has conflicting references in the\\n\"\n+\t\"new target refspec. This is most likely caused by you trying to nest\\n\"\n+\t\"a remote into itself, e.g. by renaming 'parent' into 'parent/child'\\n\"\n+\t\"or by unnesting a remote, e.g. the other way round.\\n\"\n+\t\"\\n\"\n+\t\"If that is the case, you can address this by first renaming the\\n\"\n+\t\"remote to a different name.\\n\");\n \n static int mv(int argc, const char **argv, const char *prefix,\n \t struct repository *repo UNUSED)\n@@ -738,11 +874,11 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t};\n \tstruct remote *oldremote, *newremote;\n \tstruct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT,\n-\t\told_remote_context = STRBUF_INIT;\n-\tstruct string_list remote_branches = STRING_LIST_INIT_DUP;\n-\tstruct rename_info rename;\n-\tint refs_renamed_nr = 0, refspecs_need_update = 0;\n-\tstruct progress *progress = NULL;\n+\t\told_remote_context = STRBUF_INIT, err = STRBUF_INIT;\n+\tstruct rename_info rename = {\n+\t\t.err = &err,\n+\t};\n+\tint refspecs_need_update = 0;\n \tint result = 0;\n \n \targc = parse_options(argc, argv, prefix, options,\n@@ -753,8 +889,6 @@ static int mv(int argc, const char **argv, const char *prefix,\n \n \trename.old_name = argv[0];\n \trename.new_name = argv[1];\n-\trename.remote_branches = &remote_branches;\n-\trename.symrefs_nr = 0;\n \n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -788,6 +922,30 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\trefspecs_need_update = !!strstr(oldremote->fetch.items[i].raw,\n \t\t\t\t\t\told_remote_context.buf);\n \n+\tif (refspecs_need_update) {\n+\t\trename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t 0, &err);\n+\t\tif (!rename.transaction)\n+\t\t\tgoto out;\n+\n+\t\tif (show_progress)\n+\t\t\trename.progress = start_delayed_progress(the_repository,\n+\t\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n+\n+\t\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n+\t\t\t\trename_one_ref, &rename);\n+\t\tif (result < 0)\n+\t\t\tdie(_(\"queueing remote ref renames failed: %s\"), rename.err->buf);\n+\n+\t\tresult = ref_transaction_prepare(rename.transaction, &err);\n+\t\tif (result < 0) {\n+\t\t\terror(\"renaming remote references failed: %s\", err.buf);\n+\t\t\tif (result == REF_TRANSACTION_ERROR_NAME_CONFLICT)\n+\t\t\t\tadvise(conflicting_remote_refs_advice);\n+\t\t\tdie(NULL);\n+\t\t}\n+\t}\n+\n \tif (oldremote->fetch.nr) {\n \t\tstrbuf_reset(&buf);\n \t\tstrbuf_addf(&buf, \"remote.%s.fetch\", rename.new_name);\n@@ -829,83 +987,23 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t}\n \t}\n \n-\tif (!refspecs_need_update)\n-\t\tgoto out;\n-\n-\t/*\n-\t * First remove symrefs, then rename the rest, finally create\n-\t * the new symrefs.\n-\t */\n-\trefs_for_each_ref(get_main_ref_store(the_repository),\n-\t\t\t read_remote_branches, &rename);\n-\tif (show_progress) {\n-\t\t/*\n-\t\t * Count symrefs twice, since \"renaming\" them is done by\n-\t\t * deleting and recreating them in two separate passes.\n-\t\t */\n-\t\tprogress = start_progress(the_repository,\n-\t\t\t\t\t _(\"Renaming remote references\"),\n-\t\t\t\t\t rename.remote_branches->nr + rename.symrefs_nr);\n-\t}\n-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n-\t\tstruct strbuf referent = STRBUF_INIT;\n-\n-\t\tif (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string,\n-\t\t\t\t\t &referent))\n-\t\t\tcontinue;\n-\t\tif (refs_delete_ref(get_main_ref_store(the_repository), NULL, item->string, NULL, REF_NO_DEREF))\n-\t\t\tdie(_(\"deleting '%s' failed\"), item->string);\n-\n-\t\tstrbuf_release(&referent);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n-\t}\n-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n+\tif (refspecs_need_update) {\n+\t\tresult = ref_transaction_commit(rename.transaction, &err);\n+\t\tif (result < 0)\n+\t\t\tdie(_(\"renaming remote refs failed: %s\"), rename.err->buf);\n \n-\t\tif (item->util)\n-\t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n-\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addf(&buf2, \"remote: renamed %s to %s\",\n-\t\t\t\titem->string, buf.buf);\n-\t\tif (refs_rename_ref(get_main_ref_store(the_repository), item->string, buf.buf, buf2.buf))\n-\t\t\tdie(_(\"renaming '%s' failed\"), item->string);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n-\t}\n-\tfor (size_t i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n+\t\tstop_progress(&rename.progress);\n \n-\t\tif (!item->util)\n-\t\t\tcontinue;\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, item->string);\n-\t\tstrbuf_splice(&buf, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf2);\n-\t\tstrbuf_addstr(&buf2, item->util);\n-\t\tstrbuf_splice(&buf2, strlen(\"refs/remotes/\"), strlen(rename.old_name),\n-\t\t\t\trename.new_name, strlen(rename.new_name));\n-\t\tstrbuf_reset(&buf3);\n-\t\tstrbuf_addf(&buf3, \"remote: renamed %s to %s\",\n-\t\t\t\titem->string, buf.buf);\n-\t\tif (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf))\n-\t\t\tdie(_(\"creating '%s' failed\"), buf.buf);\n-\t\tdisplay_progress(progress, ++refs_renamed_nr);\n+\t\thandle_push_default(rename.old_name, rename.new_name);\n \t}\n-\tstop_progress(&progress);\n-\n-\thandle_push_default(rename.old_name, rename.new_name);\n \n out:\n-\tstring_list_clear(&remote_branches, 1);\n+\tref_transaction_free(rename.transaction);\n \tstrbuf_release(&old_remote_context);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&buf2);\n \tstrbuf_release(&buf3);\n+\tstrbuf_release(&err);\n \treturn result;\n }\n \ndiff --git a/t/t5505-remote.sh b/t/t5505-remote.sh\nindex 2701eef85e..e592c0bcde 100755\n--- a/t/t5505-remote.sh\n+++ b/t/t5505-remote.sh\n@@ -1658,4 +1658,77 @@ test_expect_success 'forbid adding superset of existing remote' '\n \ttest_grep \".outer. is a superset of existing remote .outer/inner.\" err\n '\n \n+test_expect_success 'rename handles unborn HEAD' '\n+\ttest_when_finished \"git remote remove unborn-renamed\" &&\n+\tgit remote add unborn url &&\n+\tgit symbolic-ref refs/remotes/unborn/HEAD refs/remotes/unborn/nonexistent &&\n+\tgit remote rename unborn unborn-renamed &&\n+\tgit symbolic-ref refs/remotes/unborn-renamed/HEAD >actual &&\n+\techo refs/remotes/unborn-renamed/nonexistent >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can nest a remote into itself' '\n+\ttest_commit parent-commit &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent || true\" &&\n+\tgit remote add parent url &&\n+\tgit update-ref refs/remotes/parent/branch $COMMIT_ID &&\n+\ttest_when_finished \"git remote remove parent/child\" &&\n+\tgit remote rename parent parent/child &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child/branch\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can nest a remote into itself with a conflicting branch name' '\n+\ttest_commit parent-conflict &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent || true\" &&\n+\tgit remote add parent url &&\n+\tgit update-ref refs/remotes/parent/child $COMMIT_ID &&\n+\ttest_when_finished \"git remote remove parent/child\" &&\n+\ttest_must_fail git remote rename parent parent/child 2>err &&\n+\ttest_grep \"renaming remote references failed\" err &&\n+\ttest_grep \"The remote you are trying to rename has conflicting references\" err &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/child\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename can unnest a remote' '\n+\ttest_commit parent-child-commit &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_when_finished \"git remote remove parent/child || true\" &&\n+\tgit remote add parent/child url &&\n+\tgit update-ref refs/remotes/parent/child/branch $COMMIT_ID &&\n+\tgit remote rename parent/child parent &&\n+\tgit for-each-ref refs/remotes/ >actual &&\n+\tprintf \"$COMMIT_ID commit\\trefs/remotes/parent/branch\\n\" >expected &&\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'rename moves around the reflog' '\n+\ttest_commit reflog-old &&\n+\tCOMMIT_ID=$(git rev-parse HEAD) &&\n+\ttest_config core.logAllRefUpdates true &&\n+\ttest_when_finished \"git remote remove reflog-old || true\" &&\n+\tgit remote add reflog-old url &&\n+\tgit update-ref refs/remotes/reflog-old/branch $COMMIT_ID &&\n+\ttest-tool ref-store main for-each-reflog >actual &&\n+\ttest_grep refs/remotes/reflog-old/branch actual &&\n+\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-old/branch >reflog-entries-old &&\n+\ttest_line_count = 1 reflog-entries-old &&\n+\tgit remote rename reflog-old reflog-new &&\n+\ttest-tool ref-store main for-each-reflog >actual &&\n+\ttest_grep ! refs/remotes/reflog-old actual &&\n+\ttest_grep refs/remotes/reflog-new/branch actual &&\n+\ttest-tool ref-store main for-each-reflog-ent refs/remotes/reflog-new/branch >reflog-entries-new &&\n+\tcat >expect <<-EOF &&\n+\t$(cat reflog-entries-old)\n+\t$COMMIT_ID $COMMIT_ID $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1112912173 -0700\tremote: renamed refs/remotes/reflog-old/branch to refs/remotes/reflog-new/branch\n+\tEOF\n+\ttest_cmp expect reflog-entries-new\n+'\n+\n test_done\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523103","messageId":"20250731-pks-remote-rename-improvements-v2-6-dda6f083674d@pks.im","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"[PATCH v2 6/6] builtin/remote: only iterate through refs that are to be renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-31T14:56:54Z","receivedAt":"2025-07-31T14:57:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When renaming a remote we also need to rename all references\naccordingly. But while we only need to rename references that are\ncontained in the \"refs/remotes/$OLDNAME/\" namespace, we end up using\n`refs_for_each_rawref()` that iterates through _all_ references. We know\nto exit early in the callback in case we see an irrelevant reference,\nbut ultimately this is still a waste of compute as we knowingly iterate\nthrough references that we won't ever care about.\n\nImprove this by using `refs_for_each_rawref_in()`, which knows to only\niterate through (potentially broken) references in a given prefix.\n\nThe following benchmark renames a remote with a single reference in a\nrepository that has 100k unrelated references. This shows a sizeable\nimprovement with the \"files\" backend:\n\n Benchmark 1: rename remote (refformat = files, revision = HEAD~)\n Time (mean ± σ): 42.6 ms ± 0.9 ms [User: 29.1 ms, System: 8.4 ms]\n Range (min … max): 40.1 ms … 43.3 ms 10 runs\n\n Benchmark 2: rename remote (refformat = files, revision = HEAD)\n Time (mean ± σ): 31.7 ms ± 4.0 ms [User: 19.6 ms, System: 6.9 ms]\n Range (min … max): 27.1 ms … 36.0 ms 10 runs\n\n Summary\n rename remote (refformat = files, revision = HEAD) ran\n 1.35 ± 0.17 times faster than rename remote (refformat = files, revision = HEAD~)\n\nThe \"reftable\" backend shows roughly the same absolute improvement, but\ngiven that it's already significantly faster than the \"files\" backend\nthis translates to a much larger relative improvement:\n\n Benchmark 1: rename remote (refformat = reftable, revision = HEAD~)\n Time (mean ± σ): 18.2 ms ± 0.5 ms [User: 12.7 ms, System: 3.0 ms]\n Range (min … max): 17.3 ms … 21.4 ms 110 runs\n\n Benchmark 2: rename remote (refformat = reftable, revision = HEAD)\n Time (mean ± σ): 8.8 ms ± 0.5 ms [User: 3.8 ms, System: 2.9 ms]\n Range (min … max): 7.5 ms … 9.9 ms 167 runs\n\n Summary\n rename remote (refformat = reftable, revision = HEAD) ran\n 2.07 ± 0.12 times faster than rename remote (refformat = reftable, revision = HEAD~)\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/remote.c | 13 ++++---------\n refs.c | 8 +++++++-\n refs.h | 2 ++\n 3 files changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex db481f39bc..60e67f1b74 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -720,16 +720,8 @@ static int rename_one_ref(const char *old_refname, const char *referent,\n \tstruct strbuf new_referent = STRBUF_INIT;\n \tstruct strbuf new_refname = STRBUF_INIT;\n \tstruct rename_info *rename = cb_data;\n-\tconst char *ptr = old_refname;\n \tint error;\n \n-\tif (!skip_prefix(ptr, \"refs/remotes/\", &ptr) ||\n-\t !skip_prefix(ptr, rename->old_name, &ptr) ||\n-\t !skip_prefix(ptr, \"/\", &ptr)) {\n-\t\terror = 0;\n-\t\tgoto out;\n-\t}\n-\n \tcompute_renamed_ref(rename, old_refname, &new_refname);\n \n \tif (flags & REF_ISSYMREF) {\n@@ -932,7 +924,10 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\t\trename.progress = start_delayed_progress(the_repository,\n \t\t\t\t\t\t\t\t _(\"Renaming remote references\"), 0);\n \n-\t\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n+\t\tstrbuf_reset(&buf);\n+\t\tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename.old_name);\n+\n+\t\tresult = refs_for_each_rawref_in(get_main_ref_store(the_repository), buf.buf,\n \t\t\t\trename_one_ref, &rename);\n \t\tif (result < 0)\n \t\t\tdie(_(\"queueing remote ref renames failed: %s\"), rename.err->buf);\ndiff --git a/refs.c b/refs.c\nindex 04c9ace793..7e2f02dddf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1839,7 +1839,13 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data)\n {\n-\treturn do_for_each_ref(refs, \"\", NULL, fn, 0,\n+\treturn refs_for_each_rawref_in(refs, \"\", fn, cb_data);\n+}\n+\n+int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n+\t\t\t each_ref_fn fn, void *cb_data)\n+{\n+\treturn do_for_each_ref(refs, prefix, NULL, fn, 0,\n \t\t\t DO_FOR_EACH_INCLUDE_BROKEN, cb_data);\n }\n \ndiff --git a/refs.h b/refs.h\nindex 0bf50ce25c..19fb1d924a 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -428,6 +428,8 @@ int refs_for_each_namespaced_ref(struct ref_store *refs,\n \n /* can be used to learn about broken ref and symref */\n int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data);\n+int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix,\n+\t\t\t each_ref_fn fn, void *cb_data);\n \n /*\n * Iterates over all refs including root refs, i.e. pseudorefs and HEAD.\n\n-- \n2.50.1.619.g074bbf1d35.dirty\n\n"},{"id":"523116","messageId":"xmqqcy9gqiht.fsf@gitster.g","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-0-dda6f083674d@pks.im","subject":"Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-31T19:15:42Z","receivedAt":"2025-07-31T19:15:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The series is built on top of e4ef0485fd7 (The fourteenth batch,\n> 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\n> invalid old object IDs when migrating reflogs, 2025-07-25) merged into\n> it.\n\nI'll use the newer iteration of the other topic that ends at\nf0fde561 (refs: fix invalid old object IDs when migrating reflogs,\n2025-07-29) instead; that was what was used in the version in 'seen'.\n\n> I'd normally have withheld sending until that series was merged to\n> \"next\", but given that I promised to send something on Friday already I\n> decided to just get it out. In any case, if that causes problems I'm\n> happy to wait a bit before this series here gets merged into \"seen\".\n\nThanks, will try to include this in the batch of this evening.\n"},{"id":"523166","messageId":"aIxJrvqLvEl8qM7V@pks.im","threadId":"63865","inReplyTo":"xmqqcy9gqiht.fsf@gitster.g","subject":"Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-01T04:59:26Z","receivedAt":"2025-08-01T04:59:33Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > The series is built on top of e4ef0485fd7 (The fourteenth batch,\n> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\n> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into\n> > it.\n> \n> I'll use the newer iteration of the other topic that ends at\n> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,\n> 2025-07-29) instead; that was what was used in the version in 'seen'.\n\nOkay, makes sense. I'll adapt my local base to match then.\n\n> > I'd normally have withheld sending until that series was merged to\n> > \"next\", but given that I promised to send something on Friday already I\n> > decided to just get it out. In any case, if that causes problems I'm\n> > happy to wait a bit before this series here gets merged into \"seen\".\n> \n> Thanks, will try to include this in the batch of this evening.\n\nThanks!\n\nPatrick\n"},{"id":"523225","messageId":"xmqqpldfm1ps.fsf@gitster.g","threadId":"63865","inReplyTo":"aIxJrvqLvEl8qM7V@pks.im","subject":"Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-01T16:43:59Z","receivedAt":"2025-08-01T16:44:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:\n>> Patrick Steinhardt writes:\n>> \n>> > The series is built on top of e4ef0485fd7 (The fourteenth batch,\n>> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\n>> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into\n>> > it.\n>> \n>> I'll use the newer iteration of the other topic that ends at\n>> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,\n>> 2025-07-29) instead; that was what was used in the version in 'seen'.\n>\n> Okay, makes sense. I'll adapt my local base to match then.\n\nCurious. You had sent v3 but based your other topic on v2 and\nexpected the result will merge well?\n\n>> > I'd normally have withheld sending until that series was merged to\n>> > \"next\", but given that I promised to send something on Friday already I\n>> > decided to just get it out. In any case, if that causes problems I'm\n>> > happy to wait a bit before this series here gets merged into \"seen\".\n>> \n>> Thanks, will try to include this in the batch of this evening.\n\nWhich has happened.\nThanks.\n"},{"id":"523268","messageId":"20250802104533.GA1180347@coredump.intra.peff.net","threadId":"63865","inReplyTo":"20250731-pks-remote-rename-improvements-v2-5-dda6f083674d@pks.im","subject":"Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T10:45:33Z","receivedAt":"2025-08-02T10:45:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 31, 2025 at 04:56:53PM +0200, Patrick Steinhardt wrote:\n\n> There is one issue though with using atomic transactions: when nesting a\n> remote into itself it can happen that renamed references conflict with\n> the old referencse. For example, when we have a reference\n> \"refs/remotes/origin/foo\" and we rename \"origin\" to \"origin/foo\", then\n> we'll end up with an F/D conflict when we try to create the renamed\n> reference \"refs/remotes/origin/foo/foo\".\n\nI think that was true even in the old code. E.g., if I do:\n\n git init server\n git -C server commit --allow-empty -m foo\n git -C server branch a\n git -C server branch b\n\n git init\n git remote add foo/b server\n git fetch\n git remote rename foo/b foo\n\nthen I get (before your patches):\n\n error: 'refs/remotes/foo/b/main' exists; cannot create 'refs/remotes/foo/b'\n fatal: renaming 'refs/remotes/foo/b/b' failed\n\nWorse, we moved \"a\" but not \"b\" (nor \"main\"/\"master\", which are\nimportant because they are what's blocking the rename of \"b\"). So we are\nleft with a broken half-moved state.\n\nAfter your patches we get a nicer hint message, and of course we retain\nthe unbroken state from prior to the rename. So IMHO it is strictly\nbetter.\n\n-Peff\n"},{"id":"523269","messageId":"20250802104816.GB1180347@coredump.intra.peff.net","threadId":"63865","inReplyTo":"aIi9mAoAb1v9igQ6@pks.im","subject":"Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T10:48:16Z","receivedAt":"2025-08-02T10:48:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 29, 2025 at 02:24:56PM +0200, Patrick Steinhardt wrote:\n\n> > It's hard to bring myself to care, though. This is a stupidly\n> > pathological reflog, and the absolute time change is peanuts compared to\n> > the per-ref cost you're fixing here.\n> \n> For the \"files\" backend performance is worse, for the \"reftable\" backend\n> I'd expect that this might even be faster. Mostly because there is no\n> way to trivially rename a reflog -- we basically do the same on a rename\n> as we are doing with this patch series now.\n> \n> Overall I don't care too much about this edge case. By default we never\n> write reflogs for remote references anyway, and I doubt that you'll ever\n> end up with a remote reflog that has thousands of entries. So I'd rather\n> make the general case fast even if the esoteric case becomes slower.\n> \n> But ideally we're able to lift such limitations in the future if we were\n> to do the above rework.\n\nOK. It looks like you did end up with a single transaction in your\nre-roll. So in theory _if_ we had a \"rename\" transaction entry the\nbackends could be smarter here. But I agree with you that touching the\ncore of the ref transaction code is tricky and liable to cause\nregressions. Given the numbers I produced earlier, I'm fine with leaving\nit for later (or maybe never) and just copying the reflog entries as you\ndo in your series.\n\n-Peff\n"},{"id":"523380","messageId":"aJBYdBqmPtfS-ocR@pks.im","threadId":"63865","inReplyTo":"xmqqpldfm1ps.fsf@gitster.g","subject":"Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T06:51:32Z","receivedAt":"2025-08-04T06:51:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Aug 01, 2025 at 09:43:59AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:\n> >> Patrick Steinhardt writes:\n> >> \n> >> > The series is built on top of e4ef0485fd7 (The fourteenth batch,\n> >> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix\n> >> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into\n> >> > it.\n> >> \n> >> I'll use the newer iteration of the other topic that ends at\n> >> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,\n> >> 2025-07-29) instead; that was what was used in the version in 'seen'.\n> >\n> > Okay, makes sense. I'll adapt my local base to match then.\n> \n> Curious. You had sent v3 but based your other topic on v2 and\n> expected the result will merge well?\n\nWell, the only reason why this series depends on the migration fixes for\nreflogs is that the renamed reflogs for a remote would have invalid old\nobject IDs without it. We basically hit the bug that this other series\naims to fix.\n\nThere shouldn't be any textual conflicts between these two series.\n\nPatrick\n"},{"id":"523381","messageId":"aJBZL9B4foXZW-ix@pks.im","threadId":"63865","inReplyTo":"20250802104533.GA1180347@coredump.intra.peff.net","subject":"Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-08-04T06:54:39Z","receivedAt":"2025-08-04T06:54:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sat, Aug 02, 2025 at 06:45:33AM -0400, Jeff King wrote:\n> On Thu, Jul 31, 2025 at 04:56:53PM +0200, Patrick Steinhardt wrote:\n> \n> > There is one issue though with using atomic transactions: when nesting a\n> > remote into itself it can happen that renamed references conflict with\n> > the old referencse. For example, when we have a reference\n> > \"refs/remotes/origin/foo\" and we rename \"origin\" to \"origin/foo\", then\n> > we'll end up with an F/D conflict when we try to create the renamed\n> > reference \"refs/remotes/origin/foo/foo\".\n> \n> I think that was true even in the old code. E.g., if I do:\n> \n> git init server\n> git -C server commit --allow-empty -m foo\n> git -C server branch a\n> git -C server branch b\n> \n> git init\n> git remote add foo/b server\n> git fetch\n> git remote rename foo/b foo\n> \n> then I get (before your patches):\n> \n> error: 'refs/remotes/foo/b/main' exists; cannot create 'refs/remotes/foo/b'\n> fatal: renaming 'refs/remotes/foo/b/b' failed\n\nThe test case I have added for this conflicting case only fails with the\npostimage of this series. So we hit some _new_ edge cases now that\ndidn't exist before, but I think that's acceptable.\n\n> Worse, we moved \"a\" but not \"b\" (nor \"main\"/\"master\", which are\n> important because they are what's blocking the rename of \"b\"). So we are\n> left with a broken half-moved state.\n\nYes, this half-applied state is quite awful.\n\n> After your patches we get a nicer hint message, and of course we retain\n> the unbroken state from prior to the rename. So IMHO it is strictly\n> better.\n\nPatrick\n"},{"id":"523472","messageId":"xmqqa54f54ix.fsf@gitster.g","threadId":"63865","inReplyTo":"aJBYdBqmPtfS-ocR@pks.im","subject":"Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-04T18:24:22Z","receivedAt":"2025-08-04T18:24:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> There shouldn't be any textual conflicts between these two series.\n\nWhat I meant was this. This series is built on top of an older\niteration of the other series. The other series however has a newer\niteration. We would eventually want to both topics in the system,\nso as an early preview, both would be merged to 'seen'.\n\nThe topic branch for the other series has patches from iteration vN+1.\n\nThe topic branch for this series is, since it is built on top of the\nmerge of the other series at iteration vN into 'master'.\n\nWe merge the former into 'seen'; we now have patches from the other\nseries at iteration vN+1. We then merge the latter into 'seen'. It\nwants to _also_ merge the patches from the other series at iteration\nvN, that duplicates vN+1 but in different ways. If there wouldn't\nbe any textual conflicts between vN and vN+1 of the other series, it\nmay resolve cleanly, but is the result sane? These two iterations\nare trying to be moral equivalents, with the difference that the\nnewer iteration is trying to be better than the older one.\n\nAnd they in practice are most likely to textually conflict with each\nother. After all they are different iteration of the same topic.\n"}]}