{"thread":{"id":"63837","subject":"`git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","startedAt":"2025-07-24T09:59:56Z","lastAt":"2025-07-28T06:05:32Z","messageCount":10,"participants":["Han Jiang","Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"522649","messageId":"CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com","threadId":"63837","inReplyTo":null,"subject":"`git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Han Jiang","fromEmail":"jhcarl0814@gmail.com","sentAt":"2025-07-24T09:59:45Z","receivedAt":"2025-07-24T09:59:56Z","isPatch":false,"sender":{"key":"jhcarl0814@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5171262?v=4"},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\nmkdir --parents -- './server' './client';\ngit -C './server' init --bare './repo.git'\nbranch_default_name=\"$(git -C './server/repo.git' branch\n--show-current)\"; echo \"$branch_default_name\"\ngit --git-dir='./server/repo.git' --work-tree='.' commit\n--message=\"$((++number))\" --allow-empty\ngit -C './client' init './repo'\ncd './client/repo'\ngit remote add -m \"$branch_default_name\" server 'file://'\"$(realpath\n'../../server/repo.git')\"\ngit remote --verbose\ngit config list --local --show-scope --show-origin\ngit symbolic-ref 'refs/remotes/server/HEAD'\ngit remote rename server server2\ngit config list --local --show-scope --show-origin\ngit symbolic-ref 'refs/remotes/server2/HEAD'\ngit symbolic-ref 'refs/remotes/server/HEAD'\n\nWhat did you expect to happen? (Expected behavior)\n\n`git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n\"refs/remotes/server/master\";\n`git symbolic-ref 'refs/remotes/server2/HEAD'` outputs\n\"refs/remotes/server2/master\".\n\nWhat happened instead? (Actual behavior)\n\n`git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n\"refs/remotes/server/master\";\n`git symbolic-ref 'refs/remotes/server2/HEAD'` outputs \"fatal: ref\nrefs/remotes/server2/HEAD is not a symbolic ref\".\n`git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n\"refs/remotes/server/master\".\n\nWhat's different between what you expected and what actually happened?\n\nAnything else you want to add:\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.50.1.windows.1\ncpu: x86_64\nbuilt from commit: 4d32d83913170b86f9753fca10e75cdb2223d1cc\nsizeof-long: 4\nsizeof-size_t: 8\nshell-path: D:/git-sdk-64/usr/bin/sh\nfeature: fsmonitor--daemon\nlibcurl: 8.14.1\nOpenSSL: OpenSSL 3.2.4 11 Feb 2025\nzlib: 1.3.1\nSHA-1: SHA1_DC\nSHA-256: SHA256_BLK\nuname: Windows 10.0 26100\ncompiler info: gnuc: 15.1\nlibc info: no libc information available\n$SHELL (typically, interactive shell): C:\\Program Files\\Git\\usr\\bin\\bash.exe\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"522653","messageId":"20250724104536.GA1316505@coredump.intra.peff.net","threadId":"63837","inReplyTo":"CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-24T10:45:36Z","receivedAt":"2025-07-24T10:45:40Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 24, 2025 at 09:59:45PM +1200, Han Jiang wrote:\n\n> What did you expect to happen? (Expected behavior)\n> \n> `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> \"refs/remotes/server/master\";\n> `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs\n> \"refs/remotes/server2/master\".\n> \n> What happened instead? (Actual behavior)\n> \n> `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> \"refs/remotes/server/master\";\n> `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs \"fatal: ref\n> refs/remotes/server2/HEAD is not a symbolic ref\".\n> `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> \"refs/remotes/server/master\".\n\nThanks for the report. I can reproduce the issue easily here. Probably a\nsimpler reproduction is just:\n\n  git init\n  git remote add -m whatever server1 /does/not/need/to/exist\n  git remote rename server1 server2\n  git symbolic-ref refs/remotes/server2/HEAD\n\nThe problem is that the branch-renaming code in git-remote is not\nprepared to handle symrefs that don't resolve. This seems to make it\nwork:\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5dd6cbbaee..478ea3a80c 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -630,7 +630,9 @@ static int read_remote_branches(const char *refname, const char *referent UNUSED\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 refname,\n+\t\t\t\t\t\t RESOLVE_REF_READING |\n+\t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n \t\t\t\t\t\t NULL, &flag);\n \t\tif (symref && (flag & REF_ISSYMREF)) {\n \t\t\titem->util = xstrdup(symref);\n@@ -835,8 +837,8 @@ static int mv(int argc, const char **argv, const char *prefix,\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+\trefs_for_each_rawref(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\nThat is, we need two fixes:\n\n  1. When iterating over the refs, we need to cover _all_ refs, not just\n     those that fully resolve (there's a related bug here: we'll\n     silently ignore an actual broken or corrupt ref, whereas I think\n     the right thing would probably be to try copying it and then\n     complain loudly if we don't have the object).\n\n  2. When resolving each one, we shouldn't recurse. We're doing a\n     shallow copy, not a deep one.\n\nReading this code, though, I can't help but think that the recent \"git\nrefs migrate\" command had to deal with all of these problems. I wonder\nif we could reuse its code. +cc pks for wisdom.\n\n-Peff\n\nPS I think there's a related bug. If we have a real upstream repo and\n   try to \"git fetch\" with HEAD pointing to the unborn state, it will be\n   overwritten! I think this is the remote.*.followRemoteHEAD logic\n   being overly zealous in \"create\" mode. It should probably leave an\n   existing value alone, even if it points to an unborn branch.\n"},{"id":"522655","messageId":"aIIf7S5iPspktxdw@pks.im","threadId":"63837","inReplyTo":"20250724104536.GA1316505@coredump.intra.peff.net","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-24T11:58:37Z","receivedAt":"2025-07-24T11:58:44Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 24, 2025 at 06:45:36AM -0400, Jeff King wrote:\n> On Thu, Jul 24, 2025 at 09:59:45PM +1200, Han Jiang wrote:\n> \n> > What did you expect to happen? (Expected behavior)\n> > \n> > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > \"refs/remotes/server/master\";\n> > `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs\n> > \"refs/remotes/server2/master\".\n> > \n> > What happened instead? (Actual behavior)\n> > \n> > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > \"refs/remotes/server/master\";\n> > `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs \"fatal: ref\n> > refs/remotes/server2/HEAD is not a symbolic ref\".\n> > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > \"refs/remotes/server/master\".\n> \n> Thanks for the report. I can reproduce the issue easily here. Probably a\n> simpler reproduction is just:\n> \n>   git init\n>   git remote add -m whatever server1 /does/not/need/to/exist\n>   git remote rename server1 server2\n>   git symbolic-ref refs/remotes/server2/HEAD\n> \n> The problem is that the branch-renaming code in git-remote is not\n> prepared to handle symrefs that don't resolve. This seems to make it\n> work:\n> \n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 5dd6cbbaee..478ea3a80c 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -630,7 +630,9 @@ static int read_remote_branches(const char *refname, const char *referent UNUSED\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 refname,\n> +\t\t\t\t\t\t RESOLVE_REF_READING |\n> +\t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n>  \t\t\t\t\t\t NULL, &flag);\n>  \t\tif (symref && (flag & REF_ISSYMREF)) {\n>  \t\t\titem->util = xstrdup(symref);\n> @@ -835,8 +837,8 @@ static int mv(int argc, const char **argv, const char *prefix,\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> +\trefs_for_each_rawref(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> \n> That is, we need two fixes:\n> \n>   1. When iterating over the refs, we need to cover _all_ refs, not just\n>      those that fully resolve (there's a related bug here: we'll\n>      silently ignore an actual broken or corrupt ref, whereas I think\n>      the right thing would probably be to try copying it and then\n>      complain loudly if we don't have the object).\n> \n>   2. When resolving each one, we shouldn't recurse. We're doing a\n>      shallow copy, not a deep one.\n> \n> Reading this code, though, I can't help but think that the recent \"git\n> refs migrate\" command had to deal with all of these problems. I wonder\n> if we could reuse its code. +cc pks for wisdom.\n\nI'm not sure whether we can easily reuse the code -- the use case is\nquite different, as the migration works across two totally independent\nrefdbs. So all refs are recreated 1:1, without any renaming involved.\n\nBut it certainly seems to me like this whole logic could use quite some\nlove:\n\n  - We create N+M*2 separate ref transactions, where N is the number of\n    direct remote refs we need to migrate and M is the number of\n    symbolic refs. This is bad with the \"reftable\" backend, but given\n    that the N transactions are all renames that have to delete the old\n    ref it's even quadratic in the worst case for the \"files\" backend\n    because we may have to rewrite the packed-refs file for each such\n    transaction.\n\n  - It is way too brittle, as the update isn't even pretending to be\n    atomic. We first delete everything, and then we recreate it. So if\n    any of these updates fails we'll be left in an in-between state.\n\n  - We shouldn't have to even call `refs_resolve_ref_unsafe()` at all,\n    as the `read_remote_branches()` nowadays gets the referent as\n    parameter.\n\nTo demonstrate:\n\n     $ git init --ref-format=files repo\n    Initialized empty Git repository in /tmp/repo/.git/\n     $ cd repo/\n     /tmp/repo:HEAD $ git commit --allow-empty -m initial\n    [main (root-commit) 00c2622] x\n     $ git remote add origin /dev/null\n     /tmp/repo:main $ for i in $(seq 100000); do printf \"create refs/remotes/origin/branch-%d HEAD\\n\" $i; done | git update-ref --stdin\n     /tmp/repo:main $ git pack-refs --all\n     /tmp/repo:main $ time git remote rename origin renamed\n    Renaming remote references:   0% (2216/100000)\n\nI stopped after a minute -- this will take hours to complete.\n\nSo I think we should adapt this logic to use a single transaction.\nThere's one catch, as refs_rename_ref()` also migrates any reflogs that\nexist. But with the recent infra that Karthik has added we can now also\nmigrate reflogs, so that's all doable.\n\nPatrick\n"},{"id":"522659","messageId":"aIIvHxR8wXLTCgMW@pks.im","threadId":"63837","inReplyTo":"aIIf7S5iPspktxdw@pks.im","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-24T13:03:27Z","receivedAt":"2025-07-24T13:03:34Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 24, 2025 at 01:58:37PM +0200, Patrick Steinhardt wrote:\n> On Thu, Jul 24, 2025 at 06:45:36AM -0400, Jeff King wrote:\n> > On Thu, Jul 24, 2025 at 09:59:45PM +1200, Han Jiang wrote:\n> > \n> > > What did you expect to happen? (Expected behavior)\n> > > \n> > > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > > \"refs/remotes/server/master\";\n> > > `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs\n> > > \"refs/remotes/server2/master\".\n> > > \n> > > What happened instead? (Actual behavior)\n> > > \n> > > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > > \"refs/remotes/server/master\";\n> > > `git symbolic-ref 'refs/remotes/server2/HEAD'` outputs \"fatal: ref\n> > > refs/remotes/server2/HEAD is not a symbolic ref\".\n> > > `git symbolic-ref 'refs/remotes/server/HEAD'` outputs\n> > > \"refs/remotes/server/master\".\n> > \n> > Thanks for the report. I can reproduce the issue easily here. Probably a\n> > simpler reproduction is just:\n> > \n> >   git init\n> >   git remote add -m whatever server1 /does/not/need/to/exist\n> >   git remote rename server1 server2\n> >   git symbolic-ref refs/remotes/server2/HEAD\n> > \n> > The problem is that the branch-renaming code in git-remote is not\n> > prepared to handle symrefs that don't resolve. This seems to make it\n> > work:\n> > \n> > diff --git a/builtin/remote.c b/builtin/remote.c\n> > index 5dd6cbbaee..478ea3a80c 100644\n> > --- a/builtin/remote.c\n> > +++ b/builtin/remote.c\n> > @@ -630,7 +630,9 @@ static int read_remote_branches(const char *refname, const char *referent UNUSED\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 refname,\n> > +\t\t\t\t\t\t RESOLVE_REF_READING |\n> > +\t\t\t\t\t\t RESOLVE_REF_NO_RECURSE,\n> >  \t\t\t\t\t\t NULL, &flag);\n> >  \t\tif (symref && (flag & REF_ISSYMREF)) {\n> >  \t\t\titem->util = xstrdup(symref);\n> > @@ -835,8 +837,8 @@ static int mv(int argc, const char **argv, const char *prefix,\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> > +\trefs_for_each_rawref(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> > \n> > That is, we need two fixes:\n> > \n> >   1. When iterating over the refs, we need to cover _all_ refs, not just\n> >      those that fully resolve (there's a related bug here: we'll\n> >      silently ignore an actual broken or corrupt ref, whereas I think\n> >      the right thing would probably be to try copying it and then\n> >      complain loudly if we don't have the object).\n> > \n> >   2. When resolving each one, we shouldn't recurse. We're doing a\n> >      shallow copy, not a deep one.\n> > \n> > Reading this code, though, I can't help but think that the recent \"git\n> > refs migrate\" command had to deal with all of these problems. I wonder\n> > if we could reuse its code. +cc pks for wisdom.\n> \n> I'm not sure whether we can easily reuse the code -- the use case is\n> quite different, as the migration works across two totally independent\n> refdbs. So all refs are recreated 1:1, without any renaming involved.\n> \n> But it certainly seems to me like this whole logic could use quite some\n> love:\n> \n>   - We create N+M*2 separate ref transactions, where N is the number of\n>     direct remote refs we need to migrate and M is the number of\n>     symbolic refs. This is bad with the \"reftable\" backend, but given\n>     that the N transactions are all renames that have to delete the old\n>     ref it's even quadratic in the worst case for the \"files\" backend\n>     because we may have to rewrite the packed-refs file for each such\n>     transaction.\n> \n>   - It is way too brittle, as the update isn't even pretending to be\n>     atomic. We first delete everything, and then we recreate it. So if\n>     any of these updates fails we'll be left in an in-between state.\n> \n>   - We shouldn't have to even call `refs_resolve_ref_unsafe()` at all,\n>     as the `read_remote_branches()` nowadays gets the referent as\n>     parameter.\n> \n> To demonstrate:\n> \n>      $ git init --ref-format=files repo\n>     Initialized empty Git repository in /tmp/repo/.git/\n>      $ cd repo/\n>      /tmp/repo:HEAD $ git commit --allow-empty -m initial\n>     [main (root-commit) 00c2622] x\n>      $ git remote add origin /dev/null\n>      /tmp/repo:main $ for i in $(seq 100000); do printf \"create refs/remotes/origin/branch-%d HEAD\\n\" $i; done | git update-ref --stdin\n>      /tmp/repo:main $ git pack-refs --all\n>      /tmp/repo:main $ time git remote rename origin renamed\n>     Renaming remote references:   0% (2216/100000)\n> \n> I stopped after a minute -- this will take hours to complete.\n> \n> So I think we should adapt this logic to use a single transaction.\n> There's one catch, as refs_rename_ref()` also migrates any reflogs that\n> exist. But with the recent infra that Karthik has added we can now also\n> migrate reflogs, so that's all doable.\n> \n> Patrick\n\nI've quickly hacked something together now, see the work-in-progress\npatch below. The patch does not yet handle reflogs, but that isn't too\nhard to implement.\n\nAnd these changes indeed speed up things by quite a lot: instead of\nhours it now takes 7 seconds :) I'll polish this patch series and will\nlikely send it in tomorrow.\n\nPatrick\n\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 5dd6cbbaeed..072a70e6b45 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -612,36 +612,53 @@ 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 strbuf *err;\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 int queue_one_rename(const char *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_refname = STRBUF_INIT, new_referent = STRBUF_INIT;\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+\tint error;\n+\n+\tstrbuf_addf(&new_refname, \"refs/remotes/%s/\", rename->old_name);\n+\tif (!starts_with(refname, new_refname.buf)) {\n+\t\terror = 0;\n+\t\tgoto out;\n \t}\n-\tstrbuf_release(&buf);\n \n-\treturn 0;\n+\tif (flags & REF_ISSYMREF) {\n+\t\tstrbuf_addstr(&new_referent, referent);\n+\t\tstrbuf_splice(&new_referent, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n+\t\t\t      rename->new_name, strlen(rename->new_name));\n+\t\toid = NULL;\n+\t}\n+\n+\terror = ref_transaction_delete(rename->transaction, refname,\n+\t\t\t\t       oid, referent, REF_NO_DEREF, NULL, rename->err);\n+\tif (error < 0)\n+\t\tgoto out;\n+\n+\tstrbuf_reset(&new_refname);\n+\tstrbuf_addstr(&new_refname, refname);\n+\tstrbuf_splice(&new_refname, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n+\t\t      rename->new_name, strlen(rename->new_name));\n+\n+\terror = ref_transaction_update(rename->transaction, new_refname.buf, oid,\n+\t\t\t\t       null_oid(the_hash_algo), (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+out:\n+\tstrbuf_release(&new_refname);\n+\tstrbuf_release(&new_referent);\n+\treturn error;\n }\n \n static int migrate_file(struct remote *remote)\n@@ -740,12 +757,10 @@ static int mv(int argc, const char **argv, const char *prefix,\n \t\tOPT_END()\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 strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT,\n+\t\told_remote_context = STRBUF_INIT, err = STRBUF_INIT;\n \tstruct rename_info rename;\n-\tint i, refs_renamed_nr = 0, refspec_updated = 0;\n-\tstruct progress *progress = NULL;\n+\tint i, refspec_updated = 0;\n \tint result = 0;\n \n \targc = parse_options(argc, argv, prefix, options,\n@@ -756,8 +771,8 @@ 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+\trename.err = &err;\n \n \toldremote = remote_get(rename.old_name);\n \tif (!remote_is_configured(oldremote, 1)) {\n@@ -831,80 +846,28 @@ static int mv(int argc, const char **argv, const char *prefix,\n \tif (!refspec_updated)\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 (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 (i = 0; i < remote_branches.nr; i++) {\n-\t\tstruct string_list_item *item = remote_branches.items + i;\n+\trename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n+\t\t\t\t\t\t\t 0, &err);\n+\tif (!rename.transaction)\n+\t\tgoto out;\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      queue_one_rename, &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.transaction, &err);\n+\tif (result < 0)\n+\t\tdie(_(\"committing renamed references failed: %s\"), rename.err->buf);\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 \n"},{"id":"522697","messageId":"xmqqbjp9h1sk.fsf@gitster.g","threadId":"63837","inReplyTo":"aIIvHxR8wXLTCgMW@pks.im","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-24T18:38:35Z","receivedAt":"2025-07-24T18:38:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> I've quickly hacked something together now, see the work-in-progress\n> patch below. The patch does not yet handle reflogs, but that isn't too\n> hard to implement.\n>\n> And these changes indeed speed up things by quite a lot: instead of\n> hours it now takes 7 seconds :) I'll polish this patch series and will\n> likely send it in tomorrow.\n\nNice.  Not just the \"oops, we shouldn't assume symrefs always point\nat an existing ref\" fix, performance fix comes at the same time ;-)\n\n> diff --git a/builtin/remote.c b/builtin/remote.c\n> index 5dd6cbbaeed..072a70e6b45 100644\n> --- a/builtin/remote.c\n> +++ b/builtin/remote.c\n> @@ -612,36 +612,53 @@ 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 strbuf *err;\n>  };\n\nOK, as a place to hook the transaction into, rename_info is a\nconvenient place, as it already is passed around throughout the\nrelevant code paths.\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 int queue_one_rename(const char *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_refname = STRBUF_INIT, new_referent = STRBUF_INIT;\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> +\tint error;\n> +\n> +\tstrbuf_addf(&new_refname, \"refs/remotes/%s/\", rename->old_name);\n> +\tif (!starts_with(refname, new_refname.buf)) {\n> +\t\terror = 0;\n> +\t\tgoto out;\n>  \t}\n> -\tstrbuf_release(&buf);\n>  \n> -\treturn 0;\n> +\tif (flags & REF_ISSYMREF) {\n> +\t\tstrbuf_addstr(&new_referent, referent);\n> +\t\tstrbuf_splice(&new_referent, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> +\t\t\t      rename->new_name, strlen(rename->new_name));\n> +\t\toid = NULL;\n> +\t}\n> +\n> +\terror = ref_transaction_delete(rename->transaction, refname,\n> +\t\t\t\t       oid, referent, REF_NO_DEREF, NULL, rename->err);\n\nRemove old ...\n\n> +\tif (error < 0)\n> +\t\tgoto out;\n> +\n> +\tstrbuf_reset(&new_refname);\n> +\tstrbuf_addstr(&new_refname, refname);\n> +\tstrbuf_splice(&new_refname, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> +\t\t      rename->new_name, strlen(rename->new_name));\n> +\n> +\terror = ref_transaction_update(rename->transaction, new_refname.buf, oid,\n> +\t\t\t\t       null_oid(the_hash_algo), (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\n... and create new.  Would we be hit with the same \"while renaming A\nto A/B, there is a D/F conflict which the ref transaction does not\nhandle by itself\" issue we saw recently here?\n\n> +\trename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),\n> +\t\t\t\t\t\t\t 0, &err);\n> +\tif (!rename.transaction)\n> +\t\tgoto out;\n>  \n> +\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n> +\t\t\t\t      queue_one_rename, &rename);\n> +\tif (result < 0)\n> +\t\tdie(_(\"renaming references failed: %s\"), rename.err->buf);\n>  \n> +\tresult = ref_transaction_commit(rename.transaction, &err);\n> +\tif (result < 0)\n> +\t\tdie(_(\"committing renamed references failed: %s\"), rename.err->buf);\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\nVery nice.\n\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"},{"id":"522732","messageId":"aIM5t3NMk9N0570v@pks.im","threadId":"63837","inReplyTo":"xmqqbjp9h1sk.fsf@gitster.g","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-25T08:00:55Z","receivedAt":"2025-07-25T08:01:07Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jul 24, 2025 at 11:38:35AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > +\tif (flags & REF_ISSYMREF) {\n> > +\t\tstrbuf_addstr(&new_referent, referent);\n> > +\t\tstrbuf_splice(&new_referent, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> > +\t\t\t      rename->new_name, strlen(rename->new_name));\n> > +\t\toid = NULL;\n> > +\t}\n> > +\n> > +\terror = ref_transaction_delete(rename->transaction, refname,\n> > +\t\t\t\t       oid, referent, REF_NO_DEREF, NULL, rename->err);\n> \n> Remove old ...\n> \n> > +\tif (error < 0)\n> > +\t\tgoto out;\n> > +\n> > +\tstrbuf_reset(&new_refname);\n> > +\tstrbuf_addstr(&new_refname, refname);\n> > +\tstrbuf_splice(&new_refname, strlen(\"refs/remotes/\"), strlen(rename->old_name),\n> > +\t\t      rename->new_name, strlen(rename->new_name));\n> > +\n> > +\terror = ref_transaction_update(rename->transaction, new_refname.buf, oid,\n> > +\t\t\t\t       null_oid(the_hash_algo), (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> \n> ... and create new.  Would we be hit with the same \"while renaming A\n> to A/B, there is a D/F conflict which the ref transaction does not\n> handle by itself\" issue we saw recently here?\n\nYes, we do indeed. I've now split this up into two transactions that get\npopulated concurrently.\n\nPatrick\n"},{"id":"522738","messageId":"20250725110243.GA3014187@coredump.intra.peff.net","threadId":"63837","inReplyTo":"aIIvHxR8wXLTCgMW@pks.im","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-25T11:02:43Z","receivedAt":"2025-07-25T11:02:46Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 24, 2025 at 03:03:27PM +0200, Patrick Steinhardt wrote:\n\n> I've quickly hacked something together now, see the work-in-progress\n> patch below. The patch does not yet handle reflogs, but that isn't too\n> hard to implement.\n> \n> And these changes indeed speed up things by quite a lot: instead of\n> hours it now takes 7 seconds :) I'll polish this patch series and will\n> likely send it in tomorrow.\n\nCool. I agree with all of the pain points you outlined, and the general\ndirection. There was one other sub-optimal thing I noticed, which was...\n\n> -\trefs_for_each_ref(get_main_ref_store(the_repository),\n> -\t\t\t  read_remote_branches, &rename);\n> [...]\n> +\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n> +\t\t\t\t      queue_one_rename, &rename);\n\nBoth before and after your patch, we're iterating over _all_ refs and\nskipping ones that aren't in \"refs/remotes/<remote>/\". If we just ask to\niterate over that subset of refs, then we save the effort of iterating\nover the others that we don't care about.\n\nBut:\n\n  1. We have refs_for_each_ref_in() and refs_for_each_rawref(), but no\n     refs_for_each_rawref_in(). Feels like it should be easy to add it,\n     though.\n\n  2. It's an obvious small optimization, but it doesn't help us in a\n     big-O way. Iterating the refs is obviously O(n), and in the worst\n     case rewriting the packed-refs file is likewise O(n). So I wouldn't\n     expect to see the dramatic improvements you found by removing the\n     quadratic bits. But I'd bet it's still measurable in a repo with a\n     lot of refs (and maybe with reftables it actually would be bigger,\n     since the goal there is to amortize the rewrites).\n\n-Peff\n"},{"id":"522741","messageId":"20250725111308.GC3014187@coredump.intra.peff.net","threadId":"63837","inReplyTo":"20250725110243.GA3014187@coredump.intra.peff.net","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-07-25T11:13:08Z","receivedAt":"2025-07-25T11:13:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 25, 2025 at 07:02:43AM -0400, Jeff King wrote:\n\n> On Thu, Jul 24, 2025 at 03:03:27PM +0200, Patrick Steinhardt wrote:\n> \n> > I've quickly hacked something together now, see the work-in-progress\n> > patch below. The patch does not yet handle reflogs, but that isn't too\n> > hard to implement.\n> > \n> > And these changes indeed speed up things by quite a lot: instead of\n> > hours it now takes 7 seconds :) I'll polish this patch series and will\n> > likely send it in tomorrow.\n> \n> Cool. I agree with all of the pain points you outlined, and the general\n> direction. There was one other sub-optimal thing I noticed, which was...\n\nOh, and I meant to say: I am very happy if you want to pick up this bug\nand fix it. In the original I mentioned also that the new\nremote.*.followRemoteHEAD=create logic was kicking in for an unborn\nbranch. And I've verified that this is the case and am working on a fix.\nBut I think the two are orthogonal and we can fix them independently.\n\n-Peff\n"},{"id":"522843","messageId":"aIcTIFbKud0k2ioB@pks.im","threadId":"63837","inReplyTo":"20250725110243.GA3014187@coredump.intra.peff.net","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T06:05:20Z","receivedAt":"2025-07-28T06:05:27Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jul 25, 2025 at 07:02:43AM -0400, Jeff King wrote:\n> On Thu, Jul 24, 2025 at 03:03:27PM +0200, Patrick Steinhardt wrote:\n> \n> > I've quickly hacked something together now, see the work-in-progress\n> > patch below. The patch does not yet handle reflogs, but that isn't too\n> > hard to implement.\n> > \n> > And these changes indeed speed up things by quite a lot: instead of\n> > hours it now takes 7 seconds :) I'll polish this patch series and will\n> > likely send it in tomorrow.\n> \n> Cool. I agree with all of the pain points you outlined, and the general\n> direction. There was one other sub-optimal thing I noticed, which was...\n> \n> > -\trefs_for_each_ref(get_main_ref_store(the_repository),\n> > -\t\t\t  read_remote_branches, &rename);\n> > [...]\n> > +\tresult = refs_for_each_rawref(get_main_ref_store(the_repository),\n> > +\t\t\t\t      queue_one_rename, &rename);\n> \n> Both before and after your patch, we're iterating over _all_ refs and\n> skipping ones that aren't in \"refs/remotes/<remote>/\". If we just ask to\n> iterate over that subset of refs, then we save the effort of iterating\n> over the others that we don't care about.\n> \n> But:\n> \n>   1. We have refs_for_each_ref_in() and refs_for_each_rawref(), but no\n>      refs_for_each_rawref_in(). Feels like it should be easy to add it,\n>      though.\n> \n>   2. It's an obvious small optimization, but it doesn't help us in a\n>      big-O way. Iterating the refs is obviously O(n), and in the worst\n>      case rewriting the packed-refs file is likewise O(n). So I wouldn't\n>      expect to see the dramatic improvements you found by removing the\n>      quadratic bits. But I'd bet it's still measurable in a repo with a\n>      lot of refs (and maybe with reftables it actually would be bigger,\n>      since the goal there is to amortize the rewrites).\n\nYeah, I was wondering whether to re-do this part while at it. I\ninitially decided to not do so, but I guess that was just me being lazy.\n\nPatrick\n"},{"id":"522844","messageId":"aIcTJilNAGvfroV-@pks.im","threadId":"63837","inReplyTo":"20250725111308.GC3014187@coredump.intra.peff.net","subject":"Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-07-28T06:05:26Z","receivedAt":"2025-07-28T06:05:32Z","isPatch":false,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Jul 25, 2025 at 07:13:08AM -0400, Jeff King wrote:\n> On Fri, Jul 25, 2025 at 07:02:43AM -0400, Jeff King wrote:\n> \n> > On Thu, Jul 24, 2025 at 03:03:27PM +0200, Patrick Steinhardt wrote:\n> > \n> > > I've quickly hacked something together now, see the work-in-progress\n> > > patch below. The patch does not yet handle reflogs, but that isn't too\n> > > hard to implement.\n> > > \n> > > And these changes indeed speed up things by quite a lot: instead of\n> > > hours it now takes 7 seconds :) I'll polish this patch series and will\n> > > likely send it in tomorrow.\n> > \n> > Cool. I agree with all of the pain points you outlined, and the general\n> > direction. There was one other sub-optimal thing I noticed, which was...\n> \n> Oh, and I meant to say: I am very happy if you want to pick up this bug\n> and fix it. In the original I mentioned also that the new\n> remote.*.followRemoteHEAD=create logic was kicking in for an unborn\n> branch. And I've verified that this is the case and am working on a fix.\n> But I think the two are orthogonal and we can fix them independently.\n\nI actually missed this one, so I'm happy to leave it to you. Thanks!\n\nPatrick\n"}]}