From 376d7f1a11a52bc3f2f4ce74557536ac2195ce5f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 31 Jul 2025 16:56:51 +0200 Subject: builtin/remote: fix sign comparison warnings Fix -Wsign-comparison warnings. All of the warnings we have are about mismatches in signedness for loop counters. These are trivially fixable by using the correct integer type. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 54 +++++++++++++++++++++++------------------------------- 1 file changed, 23 insertions(+), 31 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 5dd6cbbaee..f63c5eb888 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1,5 +1,4 @@ #define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS #include "builtin.h" #include "config.h" @@ -182,7 +181,6 @@ static int add(int argc, const char **argv, const char *prefix, struct remote *remote; struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT; const char *name, *url; - int i; int result = 0; struct option options[] = { @@ -233,7 +231,7 @@ static int add(int argc, const char **argv, const char *prefix, strbuf_addf(&buf, "remote.%s.fetch", name); if (track.nr == 0) string_list_append(&track, "*"); - for (i = 0; i < track.nr; i++) { + for (size_t i = 0; i < track.nr; i++) { add_branch(buf.buf, track.items[i].string, name, mirror, &buf2); } @@ -647,18 +645,17 @@ static int read_remote_branches(const char *refname, const char *referent UNUSED static int migrate_file(struct remote *remote) { struct strbuf buf = STRBUF_INIT; - int i; strbuf_addf(&buf, "remote.%s.url", remote->name); - for (i = 0; i < remote->url.nr; i++) + for (size_t i = 0; i < remote->url.nr; i++) git_config_set_multivar(buf.buf, remote->url.v[i], "^$", 0); strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.push", remote->name); - for (i = 0; i < remote->push.nr; i++) + for (int i = 0; i < remote->push.nr; i++) git_config_set_multivar(buf.buf, remote->push.items[i].raw, "^$", 0); strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.fetch", remote->name); - for (i = 0; i < remote->fetch.nr; i++) + for (int i = 0; i < remote->fetch.nr; i++) git_config_set_multivar(buf.buf, remote->fetch.items[i].raw, "^$", 0); #ifndef WITH_BREAKING_CHANGES if (remote->origin == REMOTE_REMOTES) @@ -744,7 +741,7 @@ static int mv(int argc, const char **argv, const char *prefix, old_remote_context = STRBUF_INIT; struct string_list remote_branches = STRING_LIST_INIT_DUP; struct rename_info rename; - int i, refs_renamed_nr = 0, refspec_updated = 0; + int refs_renamed_nr = 0, refspec_updated = 0; struct progress *progress = NULL; int result = 0; @@ -790,7 +787,7 @@ static int mv(int argc, const char **argv, const char *prefix, strbuf_addf(&buf, "remote.%s.fetch", rename.new_name); git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name); - for (i = 0; i < oldremote->fetch.nr; i++) { + for (int i = 0; i < oldremote->fetch.nr; i++) { char *ptr; strbuf_reset(&buf2); @@ -813,7 +810,7 @@ static int mv(int argc, const char **argv, const char *prefix, } read_branches(); - for (i = 0; i < branch_list.nr; i++) { + for (size_t i = 0; i < branch_list.nr; i++) { struct string_list_item *item = branch_list.items + i; struct branch_info *info = item->util; if (info->remote_name && !strcmp(info->remote_name, rename.old_name)) { @@ -846,7 +843,7 @@ static int mv(int argc, const char **argv, const char *prefix, _("Renaming remote references"), rename.remote_branches->nr + rename.symrefs_nr); } - for (i = 0; i < remote_branches.nr; i++) { + for (size_t i = 0; i < remote_branches.nr; i++) { struct string_list_item *item = remote_branches.items + i; struct strbuf referent = STRBUF_INIT; @@ -859,7 +856,7 @@ static int mv(int argc, const char **argv, const char *prefix, strbuf_release(&referent); display_progress(progress, ++refs_renamed_nr); } - for (i = 0; i < remote_branches.nr; i++) { + for (size_t i = 0; i < remote_branches.nr; i++) { struct string_list_item *item = remote_branches.items + i; if (item->util) @@ -875,7 +872,7 @@ static int mv(int argc, const char **argv, const char *prefix, die(_("renaming '%s' failed"), item->string); display_progress(progress, ++refs_renamed_nr); } - for (i = 0; i < remote_branches.nr; i++) { + for (size_t i = 0; i < remote_branches.nr; i++) { struct string_list_item *item = remote_branches.items + i; if (!item->util) @@ -920,7 +917,7 @@ static int rm(int argc, const char **argv, const char *prefix, struct string_list branches = STRING_LIST_INIT_DUP; struct string_list skipped = STRING_LIST_INIT_DUP; struct branches_for_remote cb_data; - int i, result; + int result; memset(&cb_data, 0, sizeof(cb_data)); cb_data.branches = &branches; @@ -942,7 +939,7 @@ static int rm(int argc, const char **argv, const char *prefix, for_each_remote(add_known_remote, &known_remotes); read_branches(); - for (i = 0; i < branch_list.nr; i++) { + for (size_t i = 0; i < branch_list.nr; i++) { struct string_list_item *item = branch_list.items + i; struct branch_info *info = item->util; if (info->remote_name && !strcmp(info->remote_name, remote->name)) { @@ -988,7 +985,7 @@ static int rm(int argc, const char **argv, const char *prefix, "Note: Some branches outside the refs/remotes/ hierarchy were not removed;\n" "to delete them, use:", skipped.nr)); - for (i = 0; i < skipped.nr; i++) + for (size_t i = 0; i < skipped.nr; i++) fprintf(stderr, " git branch -d %s\n", skipped.items[i].string); } @@ -1166,7 +1163,6 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data) struct branch_info *branch_info = item->util; struct string_list *merge = &branch_info->merge; int width = show_info->width + 4; - int i; if (branch_info->rebase >= REBASE_TRUE && branch_info->merge.nr > 1) { error(_("invalid branch.%s.merge; cannot rebase onto > 1 branch"), @@ -1192,7 +1188,7 @@ static int show_local_info_item(struct string_list_item *item, void *cb_data) } else { printf_ln(_("merges with remote %s"), merge->items[0].string); } - for (i = 1; i < merge->nr; i++) + for (size_t i = 1; i < merge->nr; i++) printf(_("%-*s and with remote %s\n"), width, "", merge->items[i].string); @@ -1277,7 +1273,6 @@ static int get_one_entry(struct remote *remote, void *priv) struct string_list *list = priv; struct strbuf remote_info_buf = STRBUF_INIT; struct strvec *url; - int i; if (remote->url.nr > 0) { struct strbuf promisor_config = STRBUF_INIT; @@ -1294,8 +1289,7 @@ static int get_one_entry(struct remote *remote, void *priv) } else string_list_append(list, remote->name)->util = NULL; url = push_url_of_remote(remote); - for (i = 0; i < url->nr; i++) - { + for (size_t i = 0; i < url->nr; i++) { strbuf_addf(&remote_info_buf, "%s (push)", url->v[i]); string_list_append(list, remote->name)->util = strbuf_detach(&remote_info_buf, NULL); @@ -1312,10 +1306,8 @@ static int show_all(void) result = for_each_remote(get_one_entry, &list); if (!result) { - int i; - string_list_sort(&list); - for (i = 0; i < list.nr; i++) { + for (size_t i = 0; i < list.nr; i++) { struct string_list_item *item = list.items + i; if (verbose) printf("%s\t%s\n", item->string, @@ -1352,7 +1344,7 @@ static int show(int argc, const char **argv, const char *prefix, query_flag = (GET_REF_STATES | GET_HEAD_NAMES | GET_PUSH_REF_STATES); for (; argc; argc--, argv++) { - int i; + size_t i; struct strvec *url; get_remote_ref_states(*argv, &info.states, query_flag); @@ -1458,7 +1450,7 @@ static void report_set_head_auto(const char *remote, const char *head_name, static int set_head(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { - int i, opt_a = 0, opt_d = 0, result = 0, was_detached; + int opt_a = 0, opt_d = 0, result = 0, was_detached; struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, b_local_head = STRBUF_INIT; char *head_name = NULL; @@ -1489,7 +1481,7 @@ static int set_head(int argc, const char **argv, const char *prefix, else if (states.heads.nr > 1) { result |= error(_("Multiple remote HEAD branches. " "Please choose one explicitly with:")); - for (i = 0; i < states.heads.nr; i++) + for (size_t i = 0; i < states.heads.nr; i++) fprintf(stderr, " git remote set-head %s %s\n", argv[0], states.heads.items[i].string); } else @@ -1714,7 +1706,7 @@ static int set_branches(int argc, const char **argv, const char *prefix, static int get_url(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { - int i, push_mode = 0, all_mode = 0; + int push_mode = 0, all_mode = 0; const char *remotename = NULL; struct remote *remote; struct strvec *url; @@ -1742,7 +1734,7 @@ static int get_url(int argc, const char **argv, const char *prefix, url = push_mode ? push_url_of_remote(remote) : &remote->url; if (all_mode) { - for (i = 0; i < url->nr; i++) + for (size_t i = 0; i < url->nr; i++) printf_ln("%s", url->v[i]); } else { printf_ln("%s", url->v[0]); @@ -1754,7 +1746,7 @@ static int get_url(int argc, const char **argv, const char *prefix, static int set_url(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) { - int i, push_mode = 0, add_mode = 0, delete_mode = 0; + int push_mode = 0, add_mode = 0, delete_mode = 0; int matches = 0, negative_matches = 0; const char *remotename = NULL; const char *newurl = NULL; @@ -1818,7 +1810,7 @@ static int set_url(int argc, const char **argv, const char *prefix, if (regcomp(&old_regex, oldurl, REG_EXTENDED)) die(_("Invalid old URL pattern: %s"), oldurl); - for (i = 0; i < urlset->nr; i++) + for (size_t i = 0; i < urlset->nr; i++) if (!regexec(&old_regex, urlset->v[i], 0, NULL, 0)) matches++; else -- cgit v1.2.3 From 08e6a7add4678662d929718e8aa80d2505352cfd Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 31 Jul 2025 16:56:52 +0200 Subject: builtin/remote: determine whether refs need renaming early on When renaming a remote we may have to also rename remote refs in case the refspec changes. Pull out this computation into a separate loop. While that seems nonsensical right now, it'll help us in a subsequent commit where we will prepare the reference transaction before we rewrite the configuration. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index f63c5eb888..34ddcaf5f6 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -741,7 +741,7 @@ static int mv(int argc, const char **argv, const char *prefix, old_remote_context = STRBUF_INIT; struct string_list remote_branches = STRING_LIST_INIT_DUP; struct rename_info rename; - int refs_renamed_nr = 0, refspec_updated = 0; + int refs_renamed_nr = 0, refspecs_need_update = 0; struct progress *progress = NULL; int result = 0; @@ -782,11 +782,16 @@ static int mv(int argc, const char **argv, const char *prefix, goto out; } + strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name); + + for (int i = 0; i < oldremote->fetch.nr && !refspecs_need_update; i++) + refspecs_need_update = !!strstr(oldremote->fetch.items[i].raw, + old_remote_context.buf); + if (oldremote->fetch.nr) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.fetch", rename.new_name); git_config_set_multivar(buf.buf, NULL, NULL, CONFIG_FLAGS_MULTI_REPLACE); - strbuf_addf(&old_remote_context, ":refs/remotes/%s/", rename.old_name); for (int i = 0; i < oldremote->fetch.nr; i++) { char *ptr; @@ -794,7 +799,6 @@ static int mv(int argc, const char **argv, const char *prefix, strbuf_addstr(&buf2, oldremote->fetch.items[i].raw); ptr = strstr(buf2.buf, old_remote_context.buf); if (ptr) { - refspec_updated = 1; strbuf_splice(&buf2, ptr-buf2.buf + strlen(":refs/remotes/"), strlen(rename.old_name), rename.new_name, @@ -825,7 +829,7 @@ static int mv(int argc, const char **argv, const char *prefix, } } - if (!refspec_updated) + if (!refspecs_need_update) goto out; /* -- cgit v1.2.3 From 68d090a6829a46522da0d1b15099efd6d1cdb28c Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 31 Jul 2025 16:56:53 +0200 Subject: builtin/remote: rework how remote refs get renamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It was recently reported [1] that renaming a remote that has dangling symrefs is broken. This issue can be trivially reproduced: $ git init repo Initialized empty Git repository in /tmp/repo/.git/ $ cd repo/ $ git remote add origin /dev/null $ git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master $ git remote rename origin renamed $ git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/master $ git symbolic-ref refs/remotes/renamed/HEAD fatal: ref refs/remotes/renamed/HEAD is not a symbolic ref As one can see, the "HEAD" reference did not get renamed but stays in the same place. There are two issues here: - We use `refs_resolve_ref_unsafe()` to resolve references, but we don't pass the `RESOLVE_REF_NO_RECURSE` flag. Consequently, if the reference does not resolve, the function will fail and we thus ignore this branch. - We use `refs_for_each_ref()` to iterate through the old remote's references, but that function ignores broken references. Both of these issues are easy to fix. But having a closer look at the logic that renames remote references surfaces that it leaves a lot to be desired overall. The problem is that we're using O(|refs| + |symrefs| * 2) many reference transactions to perform the renames. We first delete all symrefs, then individually rename every direct reference and finally we recreate the symrefs. On the one hand this isn't even remotely an atomic operation, so if we hit any error we'll already have deleted all references. But more importantly it is also extremely inefficient. The number of transactions for symrefs doesn't really bother us too much, as there should generally only be a single symref anyway ("HEAD"). But the renames are very expensive: - For the "reftable" backend we perform auto-compaction after every single rename, which does add up. - For the "files" backend we potentially have to rewrite the "packed-refs" file on every single rename in case they are packed. The consequence here is quadratic runtime performance. Renaming a 100k references takes hours to complete. Refactor the code to use a single transaction to perform all the reference updates atomically, which speeds up the transaction quite significantly: Benchmark 1: rename remote (refformat = files, revision = HEAD~) Time (mean ± σ): 238.770 s ± 13.857 s [User: 91.473 s, System: 143.793 s] Range (min … max): 204.863 s … 247.699 s 10 runs Benchmark 2: rename remote (refformat = files, revision = HEAD) Time (mean ± σ): 2.103 s ± 0.036 s [User: 0.360 s, System: 1.313 s] Range (min … max): 2.011 s … 2.141 s 10 runs Summary rename remote (refformat = files, revision = HEAD) ran 113.53 ± 6.87 times faster than rename remote (refformat = files, revision = HEAD~) For the "reftable" backend we see a significant speedup, as well, but given that we don't have quadratic runtime behaviour there it's way less extreme: Benchmark 1: rename remote (refformat = reftable, revision = HEAD~) Time (mean ± σ): 8.604 s ± 0.539 s [User: 4.985 s, System: 2.368 s] Range (min … max): 7.880 s … 9.556 s 10 runs Benchmark 2: rename remote (refformat = reftable, revision = HEAD) Time (mean ± σ): 1.177 s ± 0.103 s [User: 0.446 s, System: 0.270 s] Range (min … max): 1.023 s … 1.410 s 10 runs Summary rename remote (refformat = reftable, revision = HEAD) ran 7.31 ± 0.79 times faster than rename remote (refformat = reftable, revision = HEAD~) There is one issue though with using atomic transactions: when nesting a remote into itself it can happen that renamed references conflict with the old referencse. For example, when we have a reference "refs/remotes/origin/foo" and we rename "origin" to "origin/foo", then we'll end up with an F/D conflict when we try to create the renamed reference "refs/remotes/origin/foo/foo". This situation is overall quite unlikely to happen: people tend to not use nested remotes, and if they do they must at the same time also have a conflicting refname. But the end result would be that the old remote references stay intact whereas all the other parts of the repository have been adjusted for the new remote name. Address this by queueing and preparing the reference update before we touch any other part of the repository. Like this we can make sure that the reference update will go through before rewriting the configuration. Otherwise, if the transaction fails to prepare we can gracefully abort the whole operation without any changes having been performed in the repository yet. Furthermore, we can detect the conflict and print some helpful advice for how the user can resolve this situation. So overall, the tradeoff is that: - Reference transactions are now all-or-nothing. This is a significant improvement over the previous state where we may have ended up with partially-renamed references. - Rewriting references is now significantly faster. - We only rewrite the configuration in case we know that all references can be updated. - But we may refuse to rename a remote in case references conflict. Overall this seems like an acceptable tradeoff. While at it, fix the handling of symbolic/broken references by using `refs_for_each_rawref()`. Add tests that cover both this reported issue and tests that exercise nesting of remotes. One thing to note: with this change we cannot provide a proper progress monitor anymore as we queue the references into the transactions as we iterate through them. Consequently, as we don't know yet how many refs there are in total, we cannot report how many percent of the operation is done anymore. But that's a small price to pay considering that you now shouldn't need the progress monitor in most situations at all anymore. [1]: Reported-by: Han Jiang Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 296 ++++++++++++++++++++++++++++++++++++------------------ t/t5505-remote.sh | 73 ++++++++++++++ 2 files changed, 270 insertions(+), 99 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index 34ddcaf5f6..db481f39bc 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -1,8 +1,11 @@ #define USE_THE_REPOSITORY_VARIABLE #include "builtin.h" +#include "advice.h" #include "config.h" +#include "date.h" #include "gettext.h" +#include "ident.h" #include "parse-options.h" #include "path.h" #include "transport.h" @@ -610,36 +613,161 @@ static int add_branch_for_removal(const char *refname, struct rename_info { const char *old_name; const char *new_name; - struct string_list *remote_branches; - uint32_t symrefs_nr; + struct ref_transaction *transaction; + struct progress *progress; + struct strbuf *err; + uint32_t progress_nr; + uint64_t index; }; -static int read_remote_branches(const char *refname, const char *referent UNUSED, - const struct object_id *oid UNUSED, - int flags UNUSED, void *cb_data) +static void compute_renamed_ref(struct rename_info *rename, + const char *refname, + struct strbuf *out) +{ + strbuf_reset(out); + strbuf_addstr(out, refname); + strbuf_splice(out, strlen("refs/remotes/"), strlen(rename->old_name), + rename->new_name, strlen(rename->new_name)); +} + +static int rename_one_reflog_entry(const char *old_refname, + struct object_id *old_oid, + struct object_id *new_oid, + const char *committer, + timestamp_t timestamp, int tz, + const char *msg, void *cb_data) { struct rename_info *rename = cb_data; - struct strbuf buf = STRBUF_INIT; - struct string_list_item *item; - int flag; - const char *symref; - - strbuf_addf(&buf, "refs/remotes/%s/", rename->old_name); - if (starts_with(refname, buf.buf)) { - item = string_list_append(rename->remote_branches, refname); - symref = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), - refname, RESOLVE_REF_READING, - NULL, &flag); - if (symref && (flag & REF_ISSYMREF)) { - item->util = xstrdup(symref); - rename->symrefs_nr++; - } else { - item->util = NULL; - } + struct strbuf new_refname = STRBUF_INIT; + struct strbuf identity = STRBUF_INIT; + struct strbuf name = STRBUF_INIT; + struct strbuf mail = STRBUF_INIT; + struct ident_split ident; + const char *date; + int error; + + compute_renamed_ref(rename, old_refname, &new_refname); + + if (split_ident_line(&ident, committer, strlen(committer)) < 0) { + error = -1; + goto out; } - strbuf_release(&buf); - return 0; + strbuf_add(&name, ident.name_begin, ident.name_end - ident.name_begin); + strbuf_add(&mail, ident.mail_begin, ident.mail_end - ident.mail_begin); + + date = show_date(timestamp, tz, DATE_MODE(NORMAL)); + strbuf_addstr(&identity, fmt_ident(name.buf, mail.buf, + WANT_BLANK_IDENT, date, 0)); + + error = ref_transaction_update_reflog(rename->transaction, new_refname.buf, + new_oid, old_oid, identity.buf, msg, + rename->index++, rename->err); + +out: + strbuf_release(&new_refname); + strbuf_release(&identity); + strbuf_release(&name); + strbuf_release(&mail); + return error; +} + +static int rename_one_reflog(const char *old_refname, + const struct object_id *old_oid, + struct rename_info *rename) +{ + struct strbuf new_refname = STRBUF_INIT; + struct strbuf message = STRBUF_INIT; + int error; + + if (!refs_reflog_exists(get_main_ref_store(the_repository), old_refname)) + return 0; + + error = refs_for_each_reflog_ent(get_main_ref_store(the_repository), + old_refname, rename_one_reflog_entry, rename); + if (error < 0) + goto out; + + compute_renamed_ref(rename, old_refname, &new_refname); + + /* + * Manually write the reflog entry for the now-renamed ref. We cannot + * rely on `rename_one_ref()` to do this for us as that would screw + * over order in which reflog entries are being written. + * + * Furthermore, we only append the entry in case the reference + * resolves. Missing references shouldn't have reflogs anyway. + */ + strbuf_addf(&message, "remote: renamed %s to %s", old_refname, + new_refname.buf); + + error = ref_transaction_update_reflog(rename->transaction, new_refname.buf, + old_oid, old_oid, git_committer_info(0), + message.buf, rename->index++, rename->err); + if (error < 0) + return error; + +out: + strbuf_release(&new_refname); + strbuf_release(&message); + return error; +} + +static int rename_one_ref(const char *old_refname, const char *referent, + const struct object_id *oid, + int flags, void *cb_data) +{ + struct strbuf new_referent = STRBUF_INIT; + struct strbuf new_refname = STRBUF_INIT; + struct rename_info *rename = cb_data; + const char *ptr = old_refname; + int error; + + if (!skip_prefix(ptr, "refs/remotes/", &ptr) || + !skip_prefix(ptr, rename->old_name, &ptr) || + !skip_prefix(ptr, "/", &ptr)) { + error = 0; + goto out; + } + + compute_renamed_ref(rename, old_refname, &new_refname); + + if (flags & REF_ISSYMREF) { + /* + * Stupidly enough `referent` is not pointing to the immediate + * target of a symref, but it's the recursively resolved value. + * So symrefs pointing to symrefs would be misresolved, and + * unborn symrefs don't have any value for the `referent` at all. + */ + referent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), + old_refname, RESOLVE_REF_NO_RECURSE, + NULL, NULL); + compute_renamed_ref(rename, referent, &new_referent); + oid = NULL; + } + + error = ref_transaction_delete(rename->transaction, old_refname, + oid, referent, REF_NO_DEREF, NULL, rename->err); + if (error < 0) + goto out; + + error = ref_transaction_update(rename->transaction, new_refname.buf, oid, null_oid(the_hash_algo), + (flags & REF_ISSYMREF) ? new_referent.buf : NULL, NULL, + REF_SKIP_CREATE_REFLOG | REF_NO_DEREF | REF_SKIP_OID_VERIFICATION, + NULL, rename->err); + if (error < 0) + goto out; + + error = rename_one_reflog(old_refname, oid, rename); + if (error < 0) + goto out; + + display_progress(rename->progress, ++rename->progress_nr); + +out: + strbuf_release(&new_referent); + strbuf_release(&new_refname); + return error; } static int migrate_file(struct remote *remote) @@ -727,6 +855,14 @@ static void handle_push_default(const char* old_name, const char* new_name) strbuf_release(&push_default.origin); } +static const char conflicting_remote_refs_advice[] = N_( + "The remote you are trying to rename has conflicting references in the\n" + "new target refspec. This is most likely caused by you trying to nest\n" + "a remote into itself, e.g. by renaming 'parent' into 'parent/child'\n" + "or by unnesting a remote, e.g. the other way round.\n" + "\n" + "If that is the case, you can address this by first renaming the\n" + "remote to a different name.\n"); static int mv(int argc, const char **argv, const char *prefix, struct repository *repo UNUSED) @@ -738,11 +874,11 @@ static int mv(int argc, const char **argv, const char *prefix, }; struct remote *oldremote, *newremote; struct strbuf buf = STRBUF_INIT, buf2 = STRBUF_INIT, buf3 = STRBUF_INIT, - old_remote_context = STRBUF_INIT; - struct string_list remote_branches = STRING_LIST_INIT_DUP; - struct rename_info rename; - int refs_renamed_nr = 0, refspecs_need_update = 0; - struct progress *progress = NULL; + old_remote_context = STRBUF_INIT, err = STRBUF_INIT; + struct rename_info rename = { + .err = &err, + }; + int refspecs_need_update = 0; int result = 0; argc = parse_options(argc, argv, prefix, options, @@ -753,8 +889,6 @@ static int mv(int argc, const char **argv, const char *prefix, rename.old_name = argv[0]; rename.new_name = argv[1]; - rename.remote_branches = &remote_branches; - rename.symrefs_nr = 0; oldremote = remote_get(rename.old_name); if (!remote_is_configured(oldremote, 1)) { @@ -788,6 +922,30 @@ static int mv(int argc, const char **argv, const char *prefix, refspecs_need_update = !!strstr(oldremote->fetch.items[i].raw, old_remote_context.buf); + if (refspecs_need_update) { + rename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository), + 0, &err); + if (!rename.transaction) + goto out; + + if (show_progress) + rename.progress = start_delayed_progress(the_repository, + _("Renaming remote references"), 0); + + result = refs_for_each_rawref(get_main_ref_store(the_repository), + rename_one_ref, &rename); + if (result < 0) + die(_("queueing remote ref renames failed: %s"), rename.err->buf); + + result = ref_transaction_prepare(rename.transaction, &err); + if (result < 0) { + error("renaming remote references failed: %s", err.buf); + if (result == REF_TRANSACTION_ERROR_NAME_CONFLICT) + advise(conflicting_remote_refs_advice); + die(NULL); + } + } + if (oldremote->fetch.nr) { strbuf_reset(&buf); strbuf_addf(&buf, "remote.%s.fetch", rename.new_name); @@ -829,83 +987,23 @@ static int mv(int argc, const char **argv, const char *prefix, } } - if (!refspecs_need_update) - goto out; - - /* - * First remove symrefs, then rename the rest, finally create - * the new symrefs. - */ - refs_for_each_ref(get_main_ref_store(the_repository), - read_remote_branches, &rename); - if (show_progress) { - /* - * Count symrefs twice, since "renaming" them is done by - * deleting and recreating them in two separate passes. - */ - progress = start_progress(the_repository, - _("Renaming remote references"), - rename.remote_branches->nr + rename.symrefs_nr); - } - for (size_t i = 0; i < remote_branches.nr; i++) { - struct string_list_item *item = remote_branches.items + i; - struct strbuf referent = STRBUF_INIT; - - if (refs_read_symbolic_ref(get_main_ref_store(the_repository), item->string, - &referent)) - continue; - if (refs_delete_ref(get_main_ref_store(the_repository), NULL, item->string, NULL, REF_NO_DEREF)) - die(_("deleting '%s' failed"), item->string); - - strbuf_release(&referent); - display_progress(progress, ++refs_renamed_nr); - } - for (size_t i = 0; i < remote_branches.nr; i++) { - struct string_list_item *item = remote_branches.items + i; + if (refspecs_need_update) { + result = ref_transaction_commit(rename.transaction, &err); + if (result < 0) + die(_("renaming remote refs failed: %s"), rename.err->buf); - if (item->util) - continue; - strbuf_reset(&buf); - strbuf_addstr(&buf, item->string); - strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old_name), - rename.new_name, strlen(rename.new_name)); - strbuf_reset(&buf2); - strbuf_addf(&buf2, "remote: renamed %s to %s", - item->string, buf.buf); - if (refs_rename_ref(get_main_ref_store(the_repository), item->string, buf.buf, buf2.buf)) - die(_("renaming '%s' failed"), item->string); - display_progress(progress, ++refs_renamed_nr); - } - for (size_t i = 0; i < remote_branches.nr; i++) { - struct string_list_item *item = remote_branches.items + i; + stop_progress(&rename.progress); - if (!item->util) - continue; - strbuf_reset(&buf); - strbuf_addstr(&buf, item->string); - strbuf_splice(&buf, strlen("refs/remotes/"), strlen(rename.old_name), - rename.new_name, strlen(rename.new_name)); - strbuf_reset(&buf2); - strbuf_addstr(&buf2, item->util); - strbuf_splice(&buf2, strlen("refs/remotes/"), strlen(rename.old_name), - rename.new_name, strlen(rename.new_name)); - strbuf_reset(&buf3); - strbuf_addf(&buf3, "remote: renamed %s to %s", - item->string, buf.buf); - if (refs_update_symref(get_main_ref_store(the_repository), buf.buf, buf2.buf, buf3.buf)) - die(_("creating '%s' failed"), buf.buf); - display_progress(progress, ++refs_renamed_nr); + handle_push_default(rename.old_name, rename.new_name); } - stop_progress(&progress); - - handle_push_default(rename.old_name, rename.new_name); out: - string_list_clear(&remote_branches, 1); + ref_transaction_free(rename.transaction); strbuf_release(&old_remote_context); strbuf_release(&buf); strbuf_release(&buf2); strbuf_release(&buf3); + strbuf_release(&err); return result; } diff --git a/t/t5505-remote.sh b/t/t5505-remote.sh index 2701eef85e..e592c0bcde 100755 --- a/t/t5505-remote.sh +++ b/t/t5505-remote.sh @@ -1658,4 +1658,77 @@ test_expect_success 'forbid adding superset of existing remote' ' test_grep ".outer. is a superset of existing remote .outer/inner." err ' +test_expect_success 'rename handles unborn HEAD' ' + test_when_finished "git remote remove unborn-renamed" && + git remote add unborn url && + git symbolic-ref refs/remotes/unborn/HEAD refs/remotes/unborn/nonexistent && + git remote rename unborn unborn-renamed && + git symbolic-ref refs/remotes/unborn-renamed/HEAD >actual && + echo refs/remotes/unborn-renamed/nonexistent >expected && + test_cmp expected actual +' + +test_expect_success 'rename can nest a remote into itself' ' + test_commit parent-commit && + COMMIT_ID=$(git rev-parse HEAD) && + test_when_finished "git remote remove parent || true" && + git remote add parent url && + git update-ref refs/remotes/parent/branch $COMMIT_ID && + test_when_finished "git remote remove parent/child" && + git remote rename parent parent/child && + git for-each-ref refs/remotes/ >actual && + printf "$COMMIT_ID commit\trefs/remotes/parent/child/branch\n" >expected && + test_cmp expected actual +' + +test_expect_success 'rename can nest a remote into itself with a conflicting branch name' ' + test_commit parent-conflict && + COMMIT_ID=$(git rev-parse HEAD) && + test_when_finished "git remote remove parent || true" && + git remote add parent url && + git update-ref refs/remotes/parent/child $COMMIT_ID && + test_when_finished "git remote remove parent/child" && + test_must_fail git remote rename parent parent/child 2>err && + test_grep "renaming remote references failed" err && + test_grep "The remote you are trying to rename has conflicting references" err && + git for-each-ref refs/remotes/ >actual && + printf "$COMMIT_ID commit\trefs/remotes/parent/child\n" >expected && + test_cmp expected actual +' + +test_expect_success 'rename can unnest a remote' ' + test_commit parent-child-commit && + COMMIT_ID=$(git rev-parse HEAD) && + test_when_finished "git remote remove parent/child || true" && + git remote add parent/child url && + git update-ref refs/remotes/parent/child/branch $COMMIT_ID && + git remote rename parent/child parent && + git for-each-ref refs/remotes/ >actual && + printf "$COMMIT_ID commit\trefs/remotes/parent/branch\n" >expected && + test_cmp expected actual +' + +test_expect_success 'rename moves around the reflog' ' + test_commit reflog-old && + COMMIT_ID=$(git rev-parse HEAD) && + test_config core.logAllRefUpdates true && + test_when_finished "git remote remove reflog-old || true" && + git remote add reflog-old url && + git update-ref refs/remotes/reflog-old/branch $COMMIT_ID && + test-tool ref-store main for-each-reflog >actual && + test_grep refs/remotes/reflog-old/branch actual && + test-tool ref-store main for-each-reflog-ent refs/remotes/reflog-old/branch >reflog-entries-old && + test_line_count = 1 reflog-entries-old && + git remote rename reflog-old reflog-new && + test-tool ref-store main for-each-reflog >actual && + test_grep ! refs/remotes/reflog-old actual && + test_grep refs/remotes/reflog-new/branch actual && + test-tool ref-store main for-each-reflog-ent refs/remotes/reflog-new/branch >reflog-entries-new && + cat >expect <<-EOF && + $(cat reflog-entries-old) + $COMMIT_ID $COMMIT_ID $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1112912173 -0700 remote: renamed refs/remotes/reflog-old/branch to refs/remotes/reflog-new/branch + EOF + test_cmp expect reflog-entries-new +' + test_done -- cgit v1.2.3 From 16c4fa26b99e6f6c24dc93575ffa884c13b1fe5f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Thu, 31 Jul 2025 16:56:54 +0200 Subject: builtin/remote: only iterate through refs that are to be renamed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When renaming a remote we also need to rename all references accordingly. But while we only need to rename references that are contained in the "refs/remotes/$OLDNAME/" namespace, we end up using `refs_for_each_rawref()` that iterates through _all_ references. We know to exit early in the callback in case we see an irrelevant reference, but ultimately this is still a waste of compute as we knowingly iterate through references that we won't ever care about. Improve this by using `refs_for_each_rawref_in()`, which knows to only iterate through (potentially broken) references in a given prefix. The following benchmark renames a remote with a single reference in a repository that has 100k unrelated references. This shows a sizeable improvement with the "files" backend: Benchmark 1: rename remote (refformat = files, revision = HEAD~) Time (mean ± σ): 42.6 ms ± 0.9 ms [User: 29.1 ms, System: 8.4 ms] Range (min … max): 40.1 ms … 43.3 ms 10 runs Benchmark 2: rename remote (refformat = files, revision = HEAD) Time (mean ± σ): 31.7 ms ± 4.0 ms [User: 19.6 ms, System: 6.9 ms] Range (min … max): 27.1 ms … 36.0 ms 10 runs Summary rename remote (refformat = files, revision = HEAD) ran 1.35 ± 0.17 times faster than rename remote (refformat = files, revision = HEAD~) The "reftable" backend shows roughly the same absolute improvement, but given that it's already significantly faster than the "files" backend this translates to a much larger relative improvement: Benchmark 1: rename remote (refformat = reftable, revision = HEAD~) Time (mean ± σ): 18.2 ms ± 0.5 ms [User: 12.7 ms, System: 3.0 ms] Range (min … max): 17.3 ms … 21.4 ms 110 runs Benchmark 2: rename remote (refformat = reftable, revision = HEAD) Time (mean ± σ): 8.8 ms ± 0.5 ms [User: 3.8 ms, System: 2.9 ms] Range (min … max): 7.5 ms … 9.9 ms 167 runs Summary rename remote (refformat = reftable, revision = HEAD) ran 2.07 ± 0.12 times faster than rename remote (refformat = reftable, revision = HEAD~) Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- builtin/remote.c | 13 ++++--------- refs.c | 8 +++++++- refs.h | 2 ++ 3 files changed, 13 insertions(+), 10 deletions(-) (limited to 'builtin/remote.c') diff --git a/builtin/remote.c b/builtin/remote.c index db481f39bc..60e67f1b74 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -720,16 +720,8 @@ static int rename_one_ref(const char *old_refname, const char *referent, struct strbuf new_referent = STRBUF_INIT; struct strbuf new_refname = STRBUF_INIT; struct rename_info *rename = cb_data; - const char *ptr = old_refname; int error; - if (!skip_prefix(ptr, "refs/remotes/", &ptr) || - !skip_prefix(ptr, rename->old_name, &ptr) || - !skip_prefix(ptr, "/", &ptr)) { - error = 0; - goto out; - } - compute_renamed_ref(rename, old_refname, &new_refname); if (flags & REF_ISSYMREF) { @@ -932,7 +924,10 @@ static int mv(int argc, const char **argv, const char *prefix, rename.progress = start_delayed_progress(the_repository, _("Renaming remote references"), 0); - result = refs_for_each_rawref(get_main_ref_store(the_repository), + strbuf_reset(&buf); + strbuf_addf(&buf, "refs/remotes/%s/", rename.old_name); + + result = refs_for_each_rawref_in(get_main_ref_store(the_repository), buf.buf, rename_one_ref, &rename); if (result < 0) die(_("queueing remote ref renames failed: %s"), rename.err->buf); diff --git a/refs.c b/refs.c index 04c9ace793..7e2f02dddf 100644 --- a/refs.c +++ b/refs.c @@ -1839,7 +1839,13 @@ int refs_for_each_namespaced_ref(struct ref_store *refs, int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data) { - return do_for_each_ref(refs, "", NULL, fn, 0, + return refs_for_each_rawref_in(refs, "", fn, cb_data); +} + +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix, + each_ref_fn fn, void *cb_data) +{ + return do_for_each_ref(refs, prefix, NULL, fn, 0, DO_FOR_EACH_INCLUDE_BROKEN, cb_data); } diff --git a/refs.h b/refs.h index 0bf50ce25c..19fb1d924a 100644 --- a/refs.h +++ b/refs.h @@ -428,6 +428,8 @@ int refs_for_each_namespaced_ref(struct ref_store *refs, /* can be used to learn about broken ref and symref */ int refs_for_each_rawref(struct ref_store *refs, each_ref_fn fn, void *cb_data); +int refs_for_each_rawref_in(struct ref_store *refs, const char *prefix, + each_ref_fn fn, void *cb_data); /* * Iterates over all refs including root refs, i.e. pseudorefs and HEAD. -- cgit v1.2.3