* [PATCH 1/4] refs: pass refname when invoking reflog entry callback
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
@ 2025-07-28 13:08 ` Patrick Steinhardt
2025-07-28 15:59 ` Justin Tobler
` (2 more replies)
2025-07-28 13:08 ` [PATCH 2/4] refs: simplify logic when migrating reflog entries Patrick Steinhardt
` (4 subsequent siblings)
5 siblings, 3 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-28 13:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang
With `refs_for_each_reflog_ent()` callers can iterate through all the
reflog entries for a given reference. The callback that is being invoked
for each such entry does not receive the name of the reference that we
are currently iterating through. This isn't really a limiting factor, as
callers can simply pass the name via the callback data.
But this layout sometimes does make for a bit of an awkward calling
pattern. One example: when iterating through all reflogs, and for each
reflog we iterate through all refnames, we have to do some extra book
keeping to track which reference name we are currently yielding reflog
entries for.
Change the signature of the callback function so that the reference name
of the reflog gets passed through to it. Adapt callers accordingly and
start using the new parameter in trivial cases. The next commit will
refactor the reference migration logic to make use of this parameter so
that we can simplify its logic a bit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fsck.c | 9 ++++-----
builtin/gc.c | 3 ++-
builtin/stash.c | 6 ++++--
commit.c | 3 ++-
object-name.c | 3 ++-
reflog-walk.c | 7 ++++---
reflog.c | 3 ++-
reflog.h | 3 ++-
refs.c | 19 +++++++++----------
refs.h | 1 +
refs/debug.c | 5 +++--
refs/files-backend.c | 15 +++++++++------
refs/reftable-backend.c | 2 +-
remote.c | 6 ++++--
revision.c | 3 ++-
t/helper/test-ref-store.c | 3 ++-
wt-status.c | 6 ++++--
17 files changed, 57 insertions(+), 40 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0084cf7400b..67eb5e4fa0f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -502,13 +502,12 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,
}
}
-static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int fsck_handle_reflog_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz UNUSED,
- const char *message UNUSED, void *cb_data)
+ const char *message UNUSED, void *cb_data UNUSED)
{
- const char *refname = cb_data;
-
if (verbose)
fprintf_ln(stderr, _("Checking reflog %s->%s"),
oid_to_hex(ooid), oid_to_hex(noid));
@@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)
strbuf_worktree_ref(cb_data, &refname, logname);
refs_for_each_reflog_ent(get_main_ref_store(the_repository),
refname.buf, fsck_handle_reflog_ent,
- refname.buf);
+ NULL);
strbuf_release(&refname);
return 0;
}
diff --git a/builtin/gc.c b/builtin/gc.c
index fab8f4dd4f7..9ae87065d35 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -312,7 +312,8 @@ struct count_reflog_entries_data {
size_t limit;
};
-static int count_reflog_entries(struct object_id *old_oid, struct object_id *new_oid,
+static int count_reflog_entries(const char *refname UNUSED,
+ struct object_id *old_oid, struct object_id *new_oid,
const char *committer, timestamp_t timestamp,
int tz, const char *msg, void *cb_data)
{
diff --git a/builtin/stash.c b/builtin/stash.c
index e2f95cc2ebc..a1ed67661e3 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -738,7 +738,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix,
return ret;
}
-static int reject_reflog_ent(struct object_id *ooid UNUSED,
+static int reject_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
@@ -2173,7 +2174,8 @@ struct stash_entry_data {
size_t count;
};
-static int collect_stash_entries(struct object_id *old_oid UNUSED,
+static int collect_stash_entries(const char *refname UNUSED,
+ struct object_id *old_oid UNUSED,
struct object_id *new_oid,
const char *committer UNUSED,
timestamp_t timestamp UNUSED,
diff --git a/commit.c b/commit.c
index 15115125c36..7ebd05f3527 100644
--- a/commit.c
+++ b/commit.c
@@ -1031,7 +1031,8 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)
commit->object.flags |= TMP_MARK;
}
-static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int collect_one_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *ident UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
const char *message UNUSED, void *cbdata)
diff --git a/object-name.c b/object-name.c
index ddafe7f9b13..9ec192c3731 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1516,7 +1516,8 @@ struct grab_nth_branch_switch_cbdata {
struct strbuf *sb;
};
-static int grab_nth_branch_switch(struct object_id *ooid UNUSED,
+static int grab_nth_branch_switch(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
diff --git a/reflog-walk.c b/reflog-walk.c
index c7070b13b00..4f1ce047498 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -22,9 +22,10 @@ struct complete_reflogs {
int nr, alloc;
};
-static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
- const char *email, timestamp_t timestamp, int tz,
- const char *message, void *cb_data)
+static int read_one_reflog(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
+ const char *email, timestamp_t timestamp, int tz,
+ const char *message, void *cb_data)
{
struct complete_reflogs *array = cb_data;
struct reflog_info *item;
diff --git a/reflog.c b/reflog.c
index 39c205fd26e..2264b3bd605 100644
--- a/reflog.c
+++ b/reflog.c
@@ -492,7 +492,8 @@ void reflog_expiry_cleanup(void *cb_data)
free_commit_list(cb->mark_list);
}
-int count_reflog_ent(struct object_id *ooid UNUSED,
+int count_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp, int tz UNUSED,
diff --git a/reflog.h b/reflog.h
index 63bb56280f4..44b306c08ae 100644
--- a/reflog.h
+++ b/reflog.h
@@ -63,7 +63,8 @@ void reflog_expiry_prepare(const char *refname, const struct object_id *oid,
int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data);
-int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
+int count_reflog_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data);
int should_expire_reflog_ent_verbose(struct object_id *ooid,
diff --git a/refs.c b/refs.c
index 4bd80287054..fd9a5f36b20 100644
--- a/refs.c
+++ b/refs.c
@@ -1022,7 +1022,6 @@ int is_branch(const char *refname)
}
struct read_ref_at_cb {
- const char *refname;
timestamp_t at_time;
int cnt;
int reccnt;
@@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,
*cb->cutoff_cnt = cb->reccnt;
}
-static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
+static int read_ref_at_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz,
const char *message, void *cb_data)
@@ -1072,13 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
oidcpy(cb->oid, noid);
if (!oideq(&cb->ooid, noid))
warning(_("log for ref %s has gap after %s"),
- cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
+ refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
}
else if (cb->date == cb->at_time)
oidcpy(cb->oid, noid);
else if (!oideq(noid, cb->oid))
warning(_("log for ref %s unexpectedly ended on %s"),
- cb->refname, show_date(cb->date, cb->tz,
+ refname, show_date(cb->date, cb->tz,
DATE_MODE(RFC2822)));
cb->reccnt++;
oidcpy(&cb->ooid, ooid);
@@ -1094,7 +1094,8 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
return 0;
}
-static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
+static int read_ref_at_ent_oldest(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz,
const char *message, void *cb_data)
@@ -1117,7 +1118,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,
struct read_ref_at_cb cb;
memset(&cb, 0, sizeof(cb));
- cb.refname = refname;
cb.at_time = at_time;
cb.cnt = cnt;
cb.msg = msg;
@@ -2976,14 +2976,14 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
struct reflog_migration_data {
uint64_t index;
- const char *refname;
struct ref_store *old_refs;
struct ref_transaction *transaction;
struct strbuf *errbuf;
struct strbuf *sb, *name, *mail;
};
-static int migrate_one_reflog_entry(struct object_id *old_oid,
+static int migrate_one_reflog_entry(const char *refname,
+ struct object_id *old_oid,
struct object_id *new_oid,
const char *committer,
timestamp_t timestamp, int tz,
@@ -3006,7 +3006,7 @@ static int migrate_one_reflog_entry(struct object_id *old_oid,
strbuf_reset(data->sb);
strbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));
- ret = ref_transaction_update_reflog(data->transaction, data->refname,
+ ret = ref_transaction_update_reflog(data->transaction, refname,
new_oid, old_oid, data->sb->buf,
msg, data->index++, data->errbuf);
return ret;
@@ -3016,7 +3016,6 @@ static int migrate_one_reflog(const char *refname, void *cb_data)
{
struct migration_data *migration_data = cb_data;
struct reflog_migration_data data = {
- .refname = refname,
.old_refs = migration_data->old_refs,
.transaction = migration_data->transaction,
.errbuf = migration_data->errbuf,
diff --git a/refs.h b/refs.h
index 99b58d0b73c..a39f873b1fe 100644
--- a/refs.h
+++ b/refs.h
@@ -559,6 +559,7 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);
* functions.
*/
typedef int each_reflog_ent_fn(
+ const char *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);
diff --git a/refs/debug.c b/refs/debug.c
index 485e3079d7a..5e113db307a 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -276,7 +276,8 @@ struct debug_reflog {
void *cb_data;
};
-static int debug_print_reflog_ent(struct object_id *old_oid,
+static int debug_print_reflog_ent(const char *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)
@@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
if (new_oid)
oid_to_hex_r(n, new_oid);
- ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+ ret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,
dbg->cb_data);
trace_printf_key(&trace_refs,
"reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3ebe0323d4e..24d0a8ebde0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,
return ret;
}
-static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
+static int show_one_reflog_ent(struct files_ref_store *refs,
+ const char *refname,
+ struct strbuf *sb,
each_reflog_ent_fn fn, void *cb_data)
{
struct object_id ooid, noid;
@@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
message += 6;
else
message += 7;
- return fn(&ooid, &noid, p, timestamp, tz, message, cb_data);
+ return fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);
}
static char *find_beginning_of_line(char *bob, char *scan)
@@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
scanp = bp;
endp = bp + 1;
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
strbuf_reset(&sb);
if (ret)
break;
@@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
* Process it, and we can end the loop.
*/
strbuf_splice(&sb, 0, 0, buf, endp - buf);
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
strbuf_reset(&sb);
break;
}
@@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
return -1;
while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
fclose(logfp);
strbuf_release(&sb);
return ret;
@@ -3323,7 +3325,8 @@ struct expire_reflog_cb {
dry_run:1;
};
-static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int expire_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data)
{
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 99fafd75ebe..25a1d516184 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,
full_committer = fmt_ident(log->value.update.name, log->value.update.email,
WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
- return fn(&old_oid, &new_oid, full_committer,
+ return fn(log->refname, &old_oid, &new_oid, full_committer,
log->value.update.time, log->value.update.tz_offset,
log->value.update.message, cb_data);
}
diff --git a/remote.c b/remote.c
index e965f022f12..db9eea4fa45 100644
--- a/remote.c
+++ b/remote.c
@@ -2578,7 +2578,8 @@ struct check_and_collect_until_cb_data {
};
/* Get the timestamp of the latest entry. */
-static int peek_reflog(struct object_id *o_oid UNUSED,
+static int peek_reflog(const char *refname UNUSED,
+ struct object_id *o_oid UNUSED,
struct object_id *n_oid UNUSED,
const char *ident UNUSED,
timestamp_t timestamp, int tz UNUSED,
@@ -2589,7 +2590,8 @@ static int peek_reflog(struct object_id *o_oid UNUSED,
return 1;
}
-static int check_and_collect_until(struct object_id *o_oid UNUSED,
+static int check_and_collect_until(const char *refname UNUSED,
+ struct object_id *o_oid UNUSED,
struct object_id *n_oid,
const char *ident UNUSED,
timestamp_t timestamp, int tz UNUSED,
diff --git a/revision.c b/revision.c
index 212ca0de276..0fc1a167a10 100644
--- a/revision.c
+++ b/revision.c
@@ -1699,7 +1699,8 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)
}
}
-static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int handle_one_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
int tz UNUSED,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 8d9a271845c..b2380d57ba3 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -215,7 +215,8 @@ static int cmd_for_each_reflog(struct ref_store *refs,
return refs_for_each_reflog(refs, each_reflog, NULL);
}
-static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,
+static int each_reflog_ent(const char *refname UNUSED,
+ struct object_id *old_oid, struct object_id *new_oid,
const char *committer, timestamp_t timestamp,
int tz, const char *msg, void *cb_data UNUSED)
{
diff --git a/wt-status.c b/wt-status.c
index 454601afa15..71bd17b610a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -972,7 +972,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)
wt_longstatus_print_trailer(s);
}
-static int stash_count_refs(struct object_id *ooid UNUSED,
+static int stash_count_refs(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
@@ -1664,7 +1665,8 @@ struct grab_1st_switch_cbdata {
struct object_id noid;
};
-static int grab_1st_switch(struct object_id *ooid UNUSED,
+static int grab_1st_switch(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
--
2.50.1.565.gc32cd1483b.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback
2025-07-28 13:08 ` [PATCH 1/4] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
@ 2025-07-28 15:59 ` Justin Tobler
2025-07-28 16:07 ` Junio C Hamano
2025-07-29 20:30 ` Karthik Nayak
2 siblings, 0 replies; 37+ messages in thread
From: Justin Tobler @ 2025-07-28 15:59 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Jeff King, Han Jiang
On 25/07/28 03:08PM, Patrick Steinhardt wrote:
> With `refs_for_each_reflog_ent()` callers can iterate through all the
> reflog entries for a given reference. The callback that is being invoked
> for each such entry does not receive the name of the reference that we
> are currently iterating through. This isn't really a limiting factor, as
> callers can simply pass the name via the callback data.
>
> But this layout sometimes does make for a bit of an awkward calling
> pattern. One example: when iterating through all reflogs, and for each
> reflog we iterate through all refnames, we have to do some extra book
> keeping to track which reference name we are currently yielding reflog
> entries for.
Making the reference name part of the callback signature seems
reasonable here. For the above mentioned example, it will certainly
simplify things quite a bit.
> Change the signature of the callback function so that the reference name
> of the reflog gets passed through to it. Adapt callers accordingly and
> start using the new parameter in trivial cases. The next commit will
> refactor the reference migration logic to make use of this parameter so
> that we can simplify its logic a bit.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/fsck.c | 9 ++++-----
> builtin/gc.c | 3 ++-
> builtin/stash.c | 6 ++++--
> commit.c | 3 ++-
> object-name.c | 3 ++-
> reflog-walk.c | 7 ++++---
> reflog.c | 3 ++-
> reflog.h | 3 ++-
> refs.c | 19 +++++++++----------
> refs.h | 1 +
> refs/debug.c | 5 +++--
> refs/files-backend.c | 15 +++++++++------
> refs/reftable-backend.c | 2 +-
> remote.c | 6 ++++--
> revision.c | 3 ++-
> t/helper/test-ref-store.c | 3 ++-
> wt-status.c | 6 ++++--
> 17 files changed, 57 insertions(+), 40 deletions(-)
>
[snip]
> diff --git a/refs.h b/refs.h
> index 99b58d0b73c..a39f873b1fe 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -559,6 +559,7 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);
> * functions.
> */
> typedef int each_reflog_ent_fn(
> + const char *refname,
The callback function now propagates the reference name and each
reference backend is updated accordingly. This patch looks good.
> struct object_id *old_oid, struct object_id *new_oid,
> const char *committer, timestamp_t timestamp,
> int tz, const char *msg, void *cb_data);
> diff --git a/refs/debug.c b/refs/debug.c
> index 485e3079d7a..5e113db307a 100644
> --- a/refs/debug.c
> +++ b/refs/debug.c
> @@ -276,7 +276,8 @@ struct debug_reflog {
> void *cb_data;
> };
>
> -static int debug_print_reflog_ent(struct object_id *old_oid,
> +static int debug_print_reflog_ent(const char *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)
> @@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
> if (new_oid)
> oid_to_hex_r(n, new_oid);
>
> - ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
> + ret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,
> dbg->cb_data);
> trace_printf_key(&trace_refs,
> "reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 3ebe0323d4e..24d0a8ebde0 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,
> return ret;
> }
>
> -static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
> +static int show_one_reflog_ent(struct files_ref_store *refs,
> + const char *refname,
> + struct strbuf *sb,
> each_reflog_ent_fn fn, void *cb_data)
> {
> struct object_id ooid, noid;
> @@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
> message += 6;
> else
> message += 7;
> - return fn(&ooid, &noid, p, timestamp, tz, message, cb_data);
> + return fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);
> }
>
> static char *find_beginning_of_line(char *bob, char *scan)
> @@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
> strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
> scanp = bp;
> endp = bp + 1;
> - ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
> + ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
> strbuf_reset(&sb);
> if (ret)
> break;
> @@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
> * Process it, and we can end the loop.
> */
> strbuf_splice(&sb, 0, 0, buf, endp - buf);
> - ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
> + ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
> strbuf_reset(&sb);
> break;
> }
> @@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
> return -1;
>
> while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
> - ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
> + ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
> fclose(logfp);
> strbuf_release(&sb);
> return ret;
> @@ -3323,7 +3325,8 @@ struct expire_reflog_cb {
> dry_run:1;
> };
>
> -static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
> +static int expire_reflog_ent(const char *refname UNUSED,
> + struct object_id *ooid, struct object_id *noid,
> const char *email, timestamp_t timestamp, int tz,
> const char *message, void *cb_data)
> {
> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 99fafd75ebe..25a1d516184 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,
>
> full_committer = fmt_ident(log->value.update.name, log->value.update.email,
> WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
> - return fn(&old_oid, &new_oid, full_committer,
> + return fn(log->refname, &old_oid, &new_oid, full_committer,
> log->value.update.time, log->value.update.tz_offset,
> log->value.update.message, cb_data);
> }
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback
2025-07-28 13:08 ` [PATCH 1/4] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
2025-07-28 15:59 ` Justin Tobler
@ 2025-07-28 16:07 ` Junio C Hamano
2025-07-29 20:30 ` Karthik Nayak
2 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 16:07 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Han Jiang
Patrick Steinhardt <ps@pks.im> writes:
> -static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,
> +static int fsck_handle_reflog_ent(const char *refname,
> + struct object_id *ooid, struct object_id *noid,
> const char *email UNUSED,
> timestamp_t timestamp, int tz UNUSED,
> - const char *message UNUSED, void *cb_data)
> + const char *message UNUSED, void *cb_data UNUSED)
> {
> - const char *refname = cb_data;
> -
> if (verbose)
> fprintf_ln(stderr, _("Checking reflog %s->%s"),
> oid_to_hex(ooid), oid_to_hex(noid));
> @@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)
> strbuf_worktree_ref(cb_data, &refname, logname);
> refs_for_each_reflog_ent(get_main_ref_store(the_repository),
> refname.buf, fsck_handle_reflog_ent,
> - refname.buf);
> + NULL);
> strbuf_release(&refname);
> return 0;
> }
Nice. There are a few callsites that passed refname as (a part of)
cb_data, like this one ...
> diff --git a/refs.c b/refs.c
> index 4bd80287054..fd9a5f36b20 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1022,7 +1022,6 @@ int is_branch(const char *refname)
> }
>
> struct read_ref_at_cb {
> - const char *refname;
> timestamp_t at_time;
> int cnt;
> int reccnt;
> @@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,
> *cb->cutoff_cnt = cb->reccnt;
> }
>
> -static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
> +static int read_ref_at_ent(const char *refname,
> + struct object_id *ooid, struct object_id *noid,
> const char *email UNUSED,
> timestamp_t timestamp, int tz,
> const char *message, void *cb_data)
> @@ -1072,13 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
> oidcpy(cb->oid, noid);
> if (!oideq(&cb->ooid, noid))
> warning(_("log for ref %s has gap after %s"),
> - cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
> + refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
> }
... that makes quite sense.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback
2025-07-28 13:08 ` [PATCH 1/4] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
2025-07-28 15:59 ` Justin Tobler
2025-07-28 16:07 ` Junio C Hamano
@ 2025-07-29 20:30 ` Karthik Nayak
2025-07-31 8:28 ` Patrick Steinhardt
2 siblings, 1 reply; 37+ messages in thread
From: Karthik Nayak @ 2025-07-29 20:30 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Jeff King, Han Jiang
[-- Attachment #1: Type: text/plain, Size: 2375 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
> With `refs_for_each_reflog_ent()` callers can iterate through all the
> reflog entries for a given reference. The callback that is being invoked
> for each such entry does not receive the name of the reference that we
> are currently iterating through. This isn't really a limiting factor, as
> callers can simply pass the name via the callback data.
>
> But this layout sometimes does make for a bit of an awkward calling
> pattern. One example: when iterating through all reflogs, and for each
> reflog we iterate through all refnames, we have to do some extra book
> keeping to track which reference name we are currently yielding reflog
> entries for.
>
> Change the signature of the callback function so that the reference name
> of the reflog gets passed through to it. Adapt callers accordingly and
> start using the new parameter in trivial cases. The next commit will
> refactor the reference migration logic to make use of this parameter so
> that we can simplify its logic a bit.
>
I remember hitting this issue with the migration code in 'refs.c', so I
think this is a good improvement. The changes themselves look good.
Nit: Changes suggested by clang-format in case you re-roll:
diff --git a/refs.c b/refs.c
index fd9a5f36b2..6ed0cd6ddc 100644
--- a/refs.c
+++ b/refs.c
@@ -1078,8 +1078,7 @@ static int read_ref_at_ent(const char *refname,
oidcpy(cb->oid, noid);
else if (!oideq(noid, cb->oid))
warning(_("log for ref %s unexpectedly ended on %s"),
- refname, show_date(cb->date, cb->tz,
- DATE_MODE(RFC2822)));
+ refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
cb->reccnt++;
oidcpy(&cb->ooid, ooid);
oidcpy(&cb->noid, noid);
diff --git a/refs.h b/refs.h
index a39f873b1f..5b0efaf752 100644
--- a/refs.h
+++ b/refs.h
@@ -559,10 +559,10 @@ int refs_delete_reflog(struct ref_store *refs,
const char *refname);
* functions.
*/
typedef int each_reflog_ent_fn(
- const char *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);
+ const char *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);
/* Iterate over reflog entries in the log for `refname`. */
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 1/4] refs: pass refname when invoking reflog entry callback
2025-07-29 20:30 ` Karthik Nayak
@ 2025-07-31 8:28 ` Patrick Steinhardt
0 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 8:28 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Junio C Hamano, Jeff King, Han Jiang
On Tue, Jul 29, 2025 at 01:30:49PM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > With `refs_for_each_reflog_ent()` callers can iterate through all the
> > reflog entries for a given reference. The callback that is being invoked
> > for each such entry does not receive the name of the reference that we
> > are currently iterating through. This isn't really a limiting factor, as
> > callers can simply pass the name via the callback data.
> >
> > But this layout sometimes does make for a bit of an awkward calling
> > pattern. One example: when iterating through all reflogs, and for each
> > reflog we iterate through all refnames, we have to do some extra book
> > keeping to track which reference name we are currently yielding reflog
> > entries for.
> >
> > Change the signature of the callback function so that the reference name
> > of the reflog gets passed through to it. Adapt callers accordingly and
> > start using the new parameter in trivial cases. The next commit will
> > refactor the reference migration logic to make use of this parameter so
> > that we can simplify its logic a bit.
> >
>
> I remember hitting this issue with the migration code in 'refs.c', so I
> think this is a good improvement. The changes themselves look good.
>
> Nit: Changes suggested by clang-format in case you re-roll:
Okay, I've rolled in both suggestions. I'll not send a new version only
to address these reformats though.
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 2/4] refs: simplify logic when migrating reflog entries
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
2025-07-28 13:08 ` [PATCH 1/4] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
@ 2025-07-28 13:08 ` Patrick Steinhardt
2025-07-28 16:08 ` Justin Tobler
2025-07-28 16:21 ` Junio C Hamano
2025-07-28 13:08 ` [PATCH 3/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
` (3 subsequent siblings)
5 siblings, 2 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-28 13:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang
When migrating reflog entries between two storage formats we have to do
so via two callback-driven functions:
- `migrate_one_reflog()` gets invoked via `refs_for_each_reflog()` to
first list all available reflogs.
- `migrate_one_reflog_entry()` gets invoked via
`refs_for_each_reflog_ent()` in `migrate_one_reflog()`.
Before the preceding commit we didn't have the refname available in
`migrate_one_reflog_entry()`, which made it necessary to have a separate
structure that we pass to the second callback so that we can propagate
the refname. Now that `refs_for_each_reflog_ent()` knows to pass the
refname to the callback though that indirection isn't necessary anymore.
There's one catch though: we do have an update index that is also stored
in the entry-specific callback data. This update index is required so
that we can tell the ref backend in which order it should persist the
reflog entries to disk.
But that purpose can be trivially achieved by just converting it into a
global counter that is used for all reflog entries, regardless of which
reference they are for. The ordering will remain the same as both the
update index and the refname is considered when sorting the entries.
Move the index into `struct migration_data` and drop the now-unused
`struct reflog_migration_data` to simplify the code a bit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 36 ++++++++++--------------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index fd9a5f36b20..b820c3908bd 100644
--- a/refs.c
+++ b/refs.c
@@ -2942,6 +2942,7 @@ struct migration_data {
struct ref_transaction *transaction;
struct strbuf *errbuf;
struct strbuf sb, name, mail;
+ uint64_t index;
};
static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,
@@ -2974,14 +2975,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
return ret;
}
-struct reflog_migration_data {
- uint64_t index;
- struct ref_store *old_refs;
- struct ref_transaction *transaction;
- struct strbuf *errbuf;
- struct strbuf *sb, *name, *mail;
-};
-
static int migrate_one_reflog_entry(const char *refname,
struct object_id *old_oid,
struct object_id *new_oid,
@@ -2989,7 +2982,7 @@ static int migrate_one_reflog_entry(const char *refname,
timestamp_t timestamp, int tz,
const char *msg, void *cb_data)
{
- struct reflog_migration_data *data = cb_data;
+ struct migration_data *data = cb_data;
struct ident_split ident;
const char *date;
int ret;
@@ -2997,17 +2990,17 @@ static int migrate_one_reflog_entry(const char *refname,
if (split_ident_line(&ident, committer, strlen(committer)) < 0)
return -1;
- strbuf_reset(data->name);
- strbuf_add(data->name, ident.name_begin, ident.name_end - ident.name_begin);
- strbuf_reset(data->mail);
- strbuf_add(data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
+ strbuf_reset(&data->name);
+ strbuf_add(&data->name, ident.name_begin, ident.name_end - ident.name_begin);
+ strbuf_reset(&data->mail);
+ strbuf_add(&data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
date = show_date(timestamp, tz, DATE_MODE(NORMAL));
- strbuf_reset(data->sb);
- strbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));
+ strbuf_reset(&data->sb);
+ strbuf_addstr(&data->sb, fmt_ident(data->name.buf, data->mail.buf, WANT_BLANK_IDENT, date, 0));
ret = ref_transaction_update_reflog(data->transaction, refname,
- new_oid, old_oid, data->sb->buf,
+ new_oid, old_oid, data->sb.buf,
msg, data->index++, data->errbuf);
return ret;
}
@@ -3015,17 +3008,8 @@ static int migrate_one_reflog_entry(const char *refname,
static int migrate_one_reflog(const char *refname, void *cb_data)
{
struct migration_data *migration_data = cb_data;
- struct reflog_migration_data data = {
- .old_refs = migration_data->old_refs,
- .transaction = migration_data->transaction,
- .errbuf = migration_data->errbuf,
- .sb = &migration_data->sb,
- .name = &migration_data->name,
- .mail = &migration_data->mail,
- };
-
return refs_for_each_reflog_ent(migration_data->old_refs, refname,
- migrate_one_reflog_entry, &data);
+ migrate_one_reflog_entry, migration_data);
}
static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf)
--
2.50.1.565.gc32cd1483b.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 2/4] refs: simplify logic when migrating reflog entries
2025-07-28 13:08 ` [PATCH 2/4] refs: simplify logic when migrating reflog entries Patrick Steinhardt
@ 2025-07-28 16:08 ` Justin Tobler
2025-07-28 16:21 ` Junio C Hamano
1 sibling, 0 replies; 37+ messages in thread
From: Justin Tobler @ 2025-07-28 16:08 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Jeff King, Han Jiang
On 25/07/28 03:08PM, Patrick Steinhardt wrote:
> diff --git a/refs.c b/refs.c
> index fd9a5f36b20..b820c3908bd 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -2942,6 +2942,7 @@ struct migration_data {
> struct ref_transaction *transaction;
> struct strbuf *errbuf;
> struct strbuf sb, name, mail;
> + uint64_t index;
> };
>
> static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,
> @@ -2974,14 +2975,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
> return ret;
> }
>
> -struct reflog_migration_data {
> - uint64_t index;
> - struct ref_store *old_refs;
> - struct ref_transaction *transaction;
> - struct strbuf *errbuf;
> - struct strbuf *sb, *name, *mail;
> -};
When I recently was looking at this in [1], I remember finding this
rather awkward/confusing. Very happy to see it go now in favor of
something much simpler :)
-Justin
[1]: <tg72v5vgu56b6akawy7sfapi2qtrmy7q3uruhersy4dtzkpvju@wamlylndp3xv>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 2/4] refs: simplify logic when migrating reflog entries
2025-07-28 13:08 ` [PATCH 2/4] refs: simplify logic when migrating reflog entries Patrick Steinhardt
2025-07-28 16:08 ` Justin Tobler
@ 2025-07-28 16:21 ` Junio C Hamano
1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 16:21 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Han Jiang
Patrick Steinhardt <ps@pks.im> writes:
> Move the index into `struct migration_data` and drop the now-unused
> `struct reflog_migration_data` to simplify the code a bit.
Nice.
> @@ -3015,17 +3008,8 @@ static int migrate_one_reflog_entry(const char *refname,
> static int migrate_one_reflog(const char *refname, void *cb_data)
> {
> struct migration_data *migration_data = cb_data;
> - struct reflog_migration_data data = {
> - .old_refs = migration_data->old_refs,
> - .transaction = migration_data->transaction,
> - .errbuf = migration_data->errbuf,
> - .sb = &migration_data->sb,
> - .name = &migration_data->name,
> - .mail = &migration_data->mail,
> - };
We no longer make this copy, which makes sense.
> return refs_for_each_reflog_ent(migration_data->old_refs, refname,
> - migrate_one_reflog_entry, &data);
> + migrate_one_reflog_entry, migration_data);
> }
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
2025-07-28 13:08 ` [PATCH 1/4] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
2025-07-28 13:08 ` [PATCH 2/4] refs: simplify logic when migrating reflog entries Patrick Steinhardt
@ 2025-07-28 13:08 ` Patrick Steinhardt
2025-07-28 17:19 ` Junio C Hamano
` (2 more replies)
2025-07-28 13:08 ` [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
` (2 subsequent siblings)
5 siblings, 3 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-28 13:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang
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 inperformant. 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.
Ideally we'd refactor the code to use a single transaction to perform
all the reference updates atomically. But unfortunately that does not
work in the case where you rename a remote so that it becomes nested
into itself, as that can lead to conflicting reference updates.
The next-best thing is to do it in two transactions: one to delete all
the references, and one to recreate the references and their reflogs.
This signicantly speeds up the operation with the "files" backend. The
following benchmark renames a remote with 10000 references:
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~)
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 verify that nesting remotes into itself continues to work
with conflicting references.
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]: <CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com>
Reported-by: Han Jiang <jhcarl0814@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/remote.c | 280 ++++++++++++++++++++++++++++++++++++------------------
t/t5505-remote.sh | 71 ++++++++++++++
2 files changed, 259 insertions(+), 92 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index 5dd6cbbaeed..b1c55909184 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -3,7 +3,9 @@
#include "builtin.h"
#include "config.h"
+#include "date.h"
#include "gettext.h"
+#include "ident.h"
#include "parse-options.h"
#include "path.h"
#include "transport.h"
@@ -612,36 +614,148 @@ 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 *tx_create;
+ struct ref_transaction *tx_delete;
+ struct progress *progress;
+ uint32_t progress_nr;
+ struct strbuf *buf1, *buf2, *buf3, *err;
+ struct strbuf *new_refname;
+ 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 renamed_refname(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 UNUSED,
+ 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 *identity = rename->buf1;
+ struct strbuf *name = rename->buf2;
+ struct strbuf *mail = rename->buf3;
+ struct ident_split ident;
+ const char *date;
+ int error;
+
+ if (split_ident_line(&ident, committer, strlen(committer)) < 0)
+ return -1;
+
+ strbuf_reset(name);
+ strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);
+ strbuf_reset(mail);
+ strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
+
+ date = show_date(timestamp, tz, DATE_MODE(NORMAL));
+ strbuf_reset(identity);
+ strbuf_addstr(identity, fmt_ident(name->buf, mail->buf,
+ WANT_BLANK_IDENT, date, 0));
+
+ error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
+ new_oid, old_oid, identity->buf, msg,
+ rename->index++, rename->err);
+
+ return error;
+}
+
+static int rename_one_reflog(const char *old_refname,
+ const struct object_id *old_oid,
+ struct rename_info *rename)
+{
+ struct strbuf *message = rename->buf1;
+ 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)
+ return error;
+
+ /*
+ * 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_reset(message);
+ strbuf_addf(message, "remote: renamed %s to %s", old_refname,
+ rename->new_refname->buf);
+
+ error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
+ old_oid, old_oid, git_committer_info(0),
+ message->buf, rename->index++, rename->err);
+ if (error < 0)
+ return error;
+
+ 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 rename_info *rename = cb_data;
+ struct strbuf *new_referent = rename->buf1;
+ 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;
}
- strbuf_release(&buf);
- return 0;
+ renamed_refname(rename, old_refname, rename->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);
+ renamed_refname(rename, referent, new_referent);
+ oid = NULL;
+ }
+
+ error = ref_transaction_delete(rename->tx_delete, old_refname,
+ oid, referent, REF_NO_DEREF, NULL, rename->err);
+ if (error < 0)
+ goto out;
+
+ error = ref_transaction_update(rename->tx_create, rename->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:
+ return error;
}
static int migrate_file(struct remote *remote)
@@ -730,7 +844,6 @@ static void handle_push_default(const char* old_name, const char* new_name)
strbuf_release(&push_default.origin);
}
-
static int mv(int argc, const char **argv, const char *prefix,
struct repository *repo UNUSED)
{
@@ -741,11 +854,15 @@ 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 i, refs_renamed_nr = 0, refspec_updated = 0;
- struct progress *progress = NULL;
+ old_remote_context = STRBUF_INIT, err = STRBUF_INIT;
+ struct rename_info rename = {
+ .buf1 = &buf,
+ .buf2 = &buf2,
+ .buf3 = &buf3,
+ .new_refname = &old_remote_context,
+ .err = &err,
+ };
+ int i, refspec_updated = 0;
int result = 0;
argc = parse_options(argc, argv, prefix, options,
@@ -756,8 +873,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)) {
@@ -832,79 +947,60 @@ static int mv(int argc, const char **argv, const char *prefix,
goto out;
/*
- * First remove symrefs, then rename the rest, finally create
- * the new symrefs.
+ * Note that we're using two transactions to rename the references:
+ *
+ * - One transaction contains all deletions for references part of
+ * the old remote.
+ * - One transaction contains all creations of references and reflogs
+ * part of the new remote.
+ *
+ * This split is required to avoid conflicting ref updates when a
+ * remote is being nested into itself or converted into its parent
+ * directory.
+ *
+ * Unfortunately this means that the operation isn't atomic. But we
+ * cannot avoid that, unless transactions learn to handle such
+ * conflicts one day.
*/
- 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 (i = 0; i < remote_branches.nr; i++) {
- struct string_list_item *item = remote_branches.items + i;
- struct strbuf referent = STRBUF_INIT;
+ rename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),
+ 0, &err);
+ if (!rename.tx_delete)
+ goto out;
- 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);
+ rename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),
+ 0, &err);
+ if (!rename.tx_create)
+ goto out;
- strbuf_release(&referent);
- display_progress(progress, ++refs_renamed_nr);
- }
- for (i = 0; i < remote_branches.nr; i++) {
- struct string_list_item *item = remote_branches.items + i;
+ if (show_progress)
+ rename.progress = start_delayed_progress(the_repository,
+ _("Renaming remote references"), 0);
- 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 (i = 0; i < remote_branches.nr; i++) {
- struct string_list_item *item = remote_branches.items + i;
+ result = refs_for_each_rawref(get_main_ref_store(the_repository),
+ rename_one_ref, &rename);
+ if (result < 0)
+ die(_("renaming references 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_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);
- }
- stop_progress(&progress);
+ result = ref_transaction_commit(rename.tx_delete, &err);
+ if (result < 0)
+ die(_("deleting old remote refs failed: %s"), rename.err->buf);
+
+ result = ref_transaction_commit(rename.tx_create, &err);
+ if (result < 0)
+ die(_("committing new remote refs failed: %s"), rename.err->buf);
+
+ stop_progress(&rename.progress);
handle_push_default(rename.old_name, rename.new_name);
out:
- string_list_clear(&remote_branches, 1);
+ ref_transaction_free(rename.tx_create);
+ ref_transaction_free(rename.tx_delete);
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 2701eef85e9..f4a407b022d 100755
--- a/t/t5505-remote.sh
+++ b/t/t5505-remote.sh
@@ -1658,4 +1658,75 @@ 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" &&
+ git remote rename parent parent/child &&
+ git for-each-ref refs/remotes/ >actual &&
+ printf "$COMMIT_ID commit\trefs/remotes/parent/child/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
--
2.50.1.565.gc32cd1483b.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 ` [PATCH 3/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
@ 2025-07-28 17:19 ` Junio C Hamano
2025-07-29 8:43 ` Patrick Steinhardt
2025-07-28 18:47 ` Justin Tobler
2025-07-29 8:16 ` Jeff King
2 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 17:19 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Han Jiang
Patrick Steinhardt <ps@pks.im> writes:
> But more importantly it is also extremely inperformant. The number of
Is "inperformant" a real word? "it performs extremely poorly"?
> +static void renamed_refname(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));
> +}
> +
The function name felt somewhat iffy (sounded as if you are letting
a third-party know that you have renamed a ref), but I cannot come
up with a better alternative X-<.
> +static int rename_one_reflog_entry(const char *old_refname UNUSED,
> + 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;
Using a name of a system call for an unrelated variable, even if a
local one in a function scope, makes me nauseous. Not a new problem
introduced by this change, though.
> + struct strbuf *identity = rename->buf1;
> + struct strbuf *name = rename->buf2;
> + struct strbuf *mail = rename->buf3;
> + struct ident_split ident;
> + const char *date;
> + int error;
> +
> + if (split_ident_line(&ident, committer, strlen(committer)) < 0)
> + return -1;
> +
> + strbuf_reset(name);
> + strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);
> + strbuf_reset(mail);
> + strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
> +
> + date = show_date(timestamp, tz, DATE_MODE(NORMAL));
> + strbuf_reset(identity);
> + strbuf_addstr(identity, fmt_ident(name->buf, mail->buf,
> + WANT_BLANK_IDENT, date, 0));
It is somewhat unfortunate that we need to do all of the above only
so that we can recreate the full ident with the given committer with
a timestamp that is given separately. This probably cannot be helped,
though. The backend may not be keeping this information as a single
string anyway.
> +static int rename_one_reflog(const char *old_refname,
> + const struct object_id *old_oid,
> + struct rename_info *rename)
> +{
> + struct strbuf *message = rename->buf1;
As these temporary strbuf's passed around as part of the rename_info
structure are never released or recreated during the run, this is
safe, but feels dirty, because we saw rename_one_reflog_entry() uses
this exact one for totally different purpose. Perhaps it would make
it easier to follow if you left "message" uninitialized here, before
refs_for_each_reflog_ent() returns. And then ...
> + 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)
> + return error;
> +
> + /*
> + * 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.
> + */
... give the "message" synonym to rename->buf1 here.
> + strbuf_reset(message);
> + strbuf_addf(message, "remote: renamed %s to %s", old_refname,
> + rename->new_refname->buf);
> +
> + error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
> + old_oid, old_oid, git_committer_info(0),
> + message->buf, rename->index++, rename->err);
> + if (error < 0)
> + return error;
> +
> + 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 rename_info *rename = cb_data;
> + struct strbuf *new_referent = rename->buf1;
> + 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;
> }
> - strbuf_release(&buf);
>
> - return 0;
> + renamed_refname(rename, old_refname, rename->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);
> + renamed_refname(rename, referent, new_referent);
> + oid = NULL;
Yuck, but this cannot be helped, I guess X-<.
> + struct rename_info rename = {
> + .buf1 = &buf,
> + .buf2 = &buf2,
> + .buf3 = &buf3,
These can be embedded in the struct, not left as three separate
strbuf instances whose addresses are known to this struct, no? We'd
need to do strbuf_release() on them at the end anyway, so it would
not be a huge deal, though.
> strbuf_release(&buf);
> strbuf_release(&buf2);
> strbuf_release(&buf3);
> + strbuf_release(&err);
> return result;
> }
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 17:19 ` Junio C Hamano
@ 2025-07-29 8:43 ` Patrick Steinhardt
0 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-29 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Han Jiang
On Mon, Jul 28, 2025 at 10:19:53AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > But more importantly it is also extremely inperformant. The number of
>
> Is "inperformant" a real word? "it performs extremely poorly"?
Well, in my head it is :) But it doesn't seem to exist anywhere else, so
I'll reword this.
> > +static void renamed_refname(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));
> > +}
> > +
>
> The function name felt somewhat iffy (sounded as if you are letting
> a third-party know that you have renamed a ref), but I cannot come
> up with a better alternative X-<.
We could name it `compute_renamed_refname()` to make it a bit more
verb-y.
> > +static int rename_one_reflog_entry(const char *old_refname UNUSED,
> > + 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;
>
> Using a name of a system call for an unrelated variable, even if a
> local one in a function scope, makes me nauseous. Not a new problem
> introduced by this change, though.
Yeah, I don't love it, either.
> > +static int rename_one_reflog(const char *old_refname,
> > + const struct object_id *old_oid,
> > + struct rename_info *rename)
> > +{
> > + struct strbuf *message = rename->buf1;
>
> As these temporary strbuf's passed around as part of the rename_info
> structure are never released or recreated during the run, this is
> safe, but feels dirty, because we saw rename_one_reflog_entry() uses
> this exact one for totally different purpose. Perhaps it would make
> it easier to follow if you left "message" uninitialized here, before
> refs_for_each_reflog_ent() returns. And then ...
>
> > + 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)
> > + return error;
> > +
> > + /*
> > + * 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.
> > + */
>
> ... give the "message" synonym to rename->buf1 here.
I was quite on the edge whether these buffers are really worth it in the
first place as an optimization -- I mostly adopted it from the migration
code. But I've benchmarked it now and couldn't really make out much of a
difference at all. So let's just drop all of this buffer reusing infra.
> > +static int rename_one_ref(const char *old_refname, const char *referent,
> > + const struct object_id *oid,
> > + int flags, void *cb_data)
> > +{
> > + struct rename_info *rename = cb_data;
> > + struct strbuf *new_referent = rename->buf1;
> > + 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;
> > }
> > - strbuf_release(&buf);
> >
> > - return 0;
> > + renamed_refname(rename, old_refname, rename->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);
> > + renamed_refname(rename, referent, new_referent);
> > + oid = NULL;
>
> Yuck, but this cannot be helped, I guess X-<.
I dunno. I feel like this is something we should eventually fix.
Currently this is misleading and basically useless.
#leftoverbits
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 ` [PATCH 3/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
2025-07-28 17:19 ` Junio C Hamano
@ 2025-07-28 18:47 ` Justin Tobler
2025-07-28 18:57 ` Junio C Hamano
2025-07-29 8:16 ` Jeff King
2 siblings, 1 reply; 37+ messages in thread
From: Justin Tobler @ 2025-07-28 18:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Jeff King, Han Jiang
On 25/07/28 03:08PM, Patrick Steinhardt wrote:
> 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 inperformant. The number of
s/inperformant/inefficient/
> 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.
>
> Ideally we'd refactor the code to use a single transaction to perform
> all the reference updates atomically. But unfortunately that does not
> work in the case where you rename a remote so that it becomes nested
> into itself, as that can lead to conflicting reference updates.
Ok so we use two transactions to avoid D/F conflicts which could be
caused by something like: `git remote rename foo foo/bar`.
> The next-best thing is to do it in two transactions: one to delete all
> the references, and one to recreate the references and their reflogs.
> This signicantly speeds up the operation with the "files" backend. The
> following benchmark renames a remote with 10000 references:
We still have the problem of not being fully atomic here since the first
transaction could commit and delete the references, but the second
transaction fail. While unfortunate, this still leaves us better off
since there are fewer transactions being created.
>
> 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~)
Nice speedup :)
> 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 verify that nesting remotes into itself continues to work
> with conflicting references.
>
> 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]: <CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com>
>
> Reported-by: Han Jiang <jhcarl0814@gmail.com>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
> builtin/remote.c | 280 ++++++++++++++++++++++++++++++++++++------------------
> t/t5505-remote.sh | 71 ++++++++++++++
> 2 files changed, 259 insertions(+), 92 deletions(-)
>
> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5dd6cbbaeed..b1c55909184 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -3,7 +3,9 @@
>
> #include "builtin.h"
> #include "config.h"
> +#include "date.h"
> #include "gettext.h"
> +#include "ident.h"
> #include "parse-options.h"
> #include "path.h"
> #include "transport.h"
> @@ -612,36 +614,148 @@ 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 *tx_create;
> + struct ref_transaction *tx_delete;
> + struct progress *progress;
> + uint32_t progress_nr;
> + struct strbuf *buf1, *buf2, *buf3, *err;
My understanding is that these strbuf are stored here as an optimization
so they can be reused. It looks like, in the context of the associated
callback though, `buf2` and `buf3` and only used for `name` and `mail`
respectively. I wonder if we could be better off calling them that to be
more explicit.
> + struct strbuf *new_refname;
> + 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 renamed_refname(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 UNUSED,
> + 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 *identity = rename->buf1;
> + struct strbuf *name = rename->buf2;
> + struct strbuf *mail = rename->buf3;
> + struct ident_split ident;
> + const char *date;
> + int error;
> +
> + if (split_ident_line(&ident, committer, strlen(committer)) < 0)
> + return -1;
> +
> + strbuf_reset(name);
> + strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);
> + strbuf_reset(mail);
> + strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
> +
> + date = show_date(timestamp, tz, DATE_MODE(NORMAL));
> + strbuf_reset(identity);
> + strbuf_addstr(identity, fmt_ident(name->buf, mail->buf,
> + WANT_BLANK_IDENT, date, 0));
> +
> + error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
> + new_oid, old_oid, identity->buf, msg,
> + rename->index++, rename->err);
> +
> + return error;
> +}
> +
> +static int rename_one_reflog(const char *old_refname,
> + const struct object_id *old_oid,
> + struct rename_info *rename)
> +{
> + struct strbuf *message = rename->buf1;
> + 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)
> + return error;
> +
> + /*
> + * 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_reset(message);
> + strbuf_addf(message, "remote: renamed %s to %s", old_refname,
> + rename->new_refname->buf);
> +
> + error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
> + old_oid, old_oid, git_committer_info(0),
> + message->buf, rename->index++, rename->err);
> + if (error < 0)
> + return error;
> +
> + 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 rename_info *rename = cb_data;
> + struct strbuf *new_referent = rename->buf1;
> + 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;
> }
> - strbuf_release(&buf);
>
> - return 0;
> + renamed_refname(rename, old_refname, rename->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);
> + renamed_refname(rename, referent, new_referent);
> + oid = NULL;
> + }
> +
> + error = ref_transaction_delete(rename->tx_delete, old_refname,
> + oid, referent, REF_NO_DEREF, NULL, rename->err);
> + if (error < 0)
> + goto out;
> +
> + error = ref_transaction_update(rename->tx_create, rename->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:
> + return error;
> }
>
> static int migrate_file(struct remote *remote)
> @@ -730,7 +844,6 @@ static void handle_push_default(const char* old_name, const char* new_name)
> strbuf_release(&push_default.origin);
> }
>
> -
> static int mv(int argc, const char **argv, const char *prefix,
> struct repository *repo UNUSED)
> {
> @@ -741,11 +854,15 @@ 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 i, refs_renamed_nr = 0, refspec_updated = 0;
> - struct progress *progress = NULL;
> + old_remote_context = STRBUF_INIT, err = STRBUF_INIT;
> + struct rename_info rename = {
> + .buf1 = &buf,
> + .buf2 = &buf2,
> + .buf3 = &buf3,
> + .new_refname = &old_remote_context,
> + .err = &err,
> + };
> + int i, refspec_updated = 0;
> int result = 0;
>
> argc = parse_options(argc, argv, prefix, options,
> @@ -756,8 +873,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)) {
> @@ -832,79 +947,60 @@ static int mv(int argc, const char **argv, const char *prefix,
> goto out;
>
> /*
> - * First remove symrefs, then rename the rest, finally create
> - * the new symrefs.
> + * Note that we're using two transactions to rename the references:
> + *
> + * - One transaction contains all deletions for references part of
> + * the old remote.
> + * - One transaction contains all creations of references and reflogs
> + * part of the new remote.
> + *
> + * This split is required to avoid conflicting ref updates when a
> + * remote is being nested into itself or converted into its parent
> + * directory.
> + *
> + * Unfortunately this means that the operation isn't atomic. But we
> + * cannot avoid that, unless transactions learn to handle such
> + * conflicts one day.
> */
We could detect if the rename operation would result in a D/F conflict
upfront and special case it by using two transactions. If we know there
isn't a D/F conflict, I think a single transaction would be sufficient.
That being said, it might be best to keep it simple for now and leave it
as-is.
> - 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 (i = 0; i < remote_branches.nr; i++) {
> - struct string_list_item *item = remote_branches.items + i;
> - struct strbuf referent = STRBUF_INIT;
> + rename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),
> + 0, &err);
> + if (!rename.tx_delete)
> + goto out;
>
> - 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);
> + rename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),
> + 0, &err);
> + if (!rename.tx_create)
> + goto out;
>
> - strbuf_release(&referent);
> - display_progress(progress, ++refs_renamed_nr);
> - }
> - for (i = 0; i < remote_branches.nr; i++) {
> - struct string_list_item *item = remote_branches.items + i;
> + if (show_progress)
> + rename.progress = start_delayed_progress(the_repository,
> + _("Renaming remote references"), 0);
>
> - 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 (i = 0; i < remote_branches.nr; i++) {
> - struct string_list_item *item = remote_branches.items + i;
> + result = refs_for_each_rawref(get_main_ref_store(the_repository),
> + rename_one_ref, &rename);
> + if (result < 0)
> + die(_("renaming references 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_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);
> - }
> - stop_progress(&progress);
> + result = ref_transaction_commit(rename.tx_delete, &err);
> + if (result < 0)
> + die(_("deleting old remote refs failed: %s"), rename.err->buf);
> +
> + result = ref_transaction_commit(rename.tx_create, &err);
> + if (result < 0)
> + die(_("committing new remote refs failed: %s"), rename.err->buf);
> +
> + stop_progress(&rename.progress);
>
> handle_push_default(rename.old_name, rename.new_name);
>
> out:
> - string_list_clear(&remote_branches, 1);
> + ref_transaction_free(rename.tx_create);
> + ref_transaction_free(rename.tx_delete);
> 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 2701eef85e9..f4a407b022d 100755
> --- a/t/t5505-remote.sh
> +++ b/t/t5505-remote.sh
> @@ -1658,4 +1658,75 @@ 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" &&
> + git remote rename parent parent/child &&
> + git for-each-ref refs/remotes/ >actual &&
> + printf "$COMMIT_ID commit\trefs/remotes/parent/child/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
>
> --
> 2.50.1.565.gc32cd1483b.dirty
>
>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 18:47 ` Justin Tobler
@ 2025-07-28 18:57 ` Junio C Hamano
2025-07-29 8:43 ` Patrick Steinhardt
0 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 18:57 UTC (permalink / raw)
To: Justin Tobler; +Cc: Patrick Steinhardt, git, Jeff King, Han Jiang
Justin Tobler <jltobler@gmail.com> writes:
>> + * This split is required to avoid conflicting ref updates when a
>> + * remote is being nested into itself or converted into its parent
>> + * directory.
>> + *
>> + * Unfortunately this means that the operation isn't atomic. But we
>> + * cannot avoid that, unless transactions learn to handle such
>> + * conflicts one day.
>> */
>
> We could detect if the rename operation would result in a D/F conflict
> upfront and special case it by using two transactions. If we know there
> isn't a D/F conflict, I think a single transaction would be sufficient.
The right solution should be at the implementation of the
transactions, not the application that uses the transaction
mechanism, no? So I would think the above workaround is actually
counter-productive.
> That being said, it might be best to keep it simple for now and leave it
> as-is.
Yes, we do not have to update the transaction layer to fix that D/F
thing in this same series.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 18:57 ` Junio C Hamano
@ 2025-07-29 8:43 ` Patrick Steinhardt
0 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-29 8:43 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Justin Tobler, git, Jeff King, Han Jiang
On Mon, Jul 28, 2025 at 11:57:07AM -0700, Junio C Hamano wrote:
> Justin Tobler <jltobler@gmail.com> writes:
>
> >> + * This split is required to avoid conflicting ref updates when a
> >> + * remote is being nested into itself or converted into its parent
> >> + * directory.
> >> + *
> >> + * Unfortunately this means that the operation isn't atomic. But we
> >> + * cannot avoid that, unless transactions learn to handle such
> >> + * conflicts one day.
> >> */
> >
> > We could detect if the rename operation would result in a D/F conflict
> > upfront and special case it by using two transactions. If we know there
> > isn't a D/F conflict, I think a single transaction would be sufficient.
>
> The right solution should be at the implementation of the
> transactions, not the application that uses the transaction
> mechanism, no? So I would think the above workaround is actually
> counter-productive.
>
> > That being said, it might be best to keep it simple for now and leave it
> > as-is.
>
> Yes, we do not have to update the transaction layer to fix that D/F
> thing in this same series.
Ideally, yes, and with the "reftable" backend this is a trivial
addition. But as ever so often, the problem is with the "files" backend:
if we have "refs/heads/parent" and create "refs/heads/parent/child" we
cannot create the lockfile for the latter ref.
There probably are ways to solve this, but we would have to add new
locking semantics to the "files" backend to achieve it. And I am afraid
that such new semantics might break existing implementations of the
"files" backend that are outside of Git. So I'm not really sure whether
this is even a possible route to go down, unfortunately.
So with that I don't think Justin's proposal is unreasonable. Some
options:
- We can check whether the old and new remote names are prefixes of
one another. If so, we create two transactions, otherwise we do an
atomic commit.
- We can outright refuse to do a nesting/unnesting rename and just
always use a single transaction where there cannot be any conflicts.
The latter is of course a backwards incompatible change. It feels quite
tempting though, as hierarchical remotes are just so esoteric. And then
renaming such hierarchical remotes with nesting/unnesting semantics is
even more unlikely to ever happen.
A slightly less intrusive way could be to opportunistically rename refs
in a single transaction, regardless of whether refs are nested or not.
And if we do notice any conflict we abort the whole command and give the
user guidance to please do `git remote rename parent/child unrelated &&
git remote rename unrelated parent`. Which is overall a bit of a stupid
limitation, but maybe it's good enough given that it is so supremely
unlikely that anyone will ever hit the issue?
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 ` [PATCH 3/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
2025-07-28 17:19 ` Junio C Hamano
2025-07-28 18:47 ` Justin Tobler
@ 2025-07-29 8:16 ` Jeff King
2025-07-29 12:24 ` Patrick Steinhardt
2 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2025-07-29 8:16 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han Jiang
On Mon, Jul 28, 2025 at 03:08:47PM +0200, Patrick Steinhardt wrote:
> The next-best thing is to do it in two transactions: one to delete all
> the references, and one to recreate the references and their reflogs.
> This signicantly speeds up the operation with the "files" backend. The
> following benchmark renames a remote with 10000 references:
Hmm. I was surprised to see so much reflog code here. It looks like
you're replaying the old reflog entry by entry. But the old code was
leaning on refs_rename_ref() to do the individual renames, which just
asks the backend to handle that for us (so e.g., the files backend just
copies/moves the log files).
So it feels like ideally we'd be able to create a transaction element
for renaming, and then the backends could similarly do what makes sense
for them (and we wouldn't need a bunch of reflog code here).
I guess that does not work with the two delete/create transactions you
end up with here, though. And you need those to worry about D/F
conflicts. But then...how did the original handle D/F conflicts? It kind
of looks like it didn't, as it is doing a mass ref-by-ref rename in the
middle.
If the refs code learned how to order things to handle the D/F conflicts
within a transaction, then we could do a single transaction. And it
could learn about rename primitives.
I dunno. I think that would be nicer, but it's probably not worth
holding up this topic. Your perf numbers are very nice. I guess the
possible flip-side is that the existing code could be faster when
renaming a single ref (so no quadratic behavior) with a pathological
reflog (so moving the file is faster than re-writing all of those logs).
Hmm, yeah. Something like this:
cat >setup <<-\EOF
#!/bin/sh
rm -rf repo
git init repo
cd repo
git init server
git -C server commit --allow-empty -m foo
git remote add origin server
git fetch
# make the reflog gigantic
perl -i -ne 'for my $i (1..10**5) { print }' .git/logs/refs/remotes/origin/main
EOF
hyperfine -p ./setup -L v old,new './git.{v} -C repo remote rename origin foo'
results in:
Benchmark 1: ./git.old -C repo remote rename origin foo
Time (mean ± σ): 5.5 ms ± 1.1 ms [User: 1.5 ms, System: 1.3 ms]
Range (min … max): 3.6 ms … 9.7 ms 58 runs
Benchmark 2: ./git.new -C repo remote rename origin foo
Time (mean ± σ): 476.3 ms ± 9.8 ms [User: 203.6 ms, System: 268.0 ms]
Range (min … max): 467.8 ms … 498.7 ms 10 runs
Summary
./git.old -C repo remote rename origin foo ran
86.43 ± 16.61 times faster than ./git.new -C repo remote rename origin foo
It's hard to bring myself to care, though. This is a stupidly
pathological reflog, and the absolute time change is peanuts compared to
the per-ref cost you're fixing here.
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-29 8:16 ` Jeff King
@ 2025-07-29 12:24 ` Patrick Steinhardt
2025-08-02 10:48 ` Jeff King
0 siblings, 1 reply; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-29 12:24 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Han Jiang
On Tue, Jul 29, 2025 at 04:16:58AM -0400, Jeff King wrote:
> On Mon, Jul 28, 2025 at 03:08:47PM +0200, Patrick Steinhardt wrote:
>
> > The next-best thing is to do it in two transactions: one to delete all
> > the references, and one to recreate the references and their reflogs.
> > This signicantly speeds up the operation with the "files" backend. The
> > following benchmark renames a remote with 10000 references:
>
> Hmm. I was surprised to see so much reflog code here. It looks like
> you're replaying the old reflog entry by entry. But the old code was
> leaning on refs_rename_ref() to do the individual renames, which just
> asks the backend to handle that for us (so e.g., the files backend just
> copies/moves the log files).
>
> So it feels like ideally we'd be able to create a transaction element
> for renaming, and then the backends could similarly do what makes sense
> for them (and we wouldn't need a bunch of reflog code here).
I think overall the ref transaction code is in for a bit of a redesign.
The current one-size-fits-all `struct ref_update` doesn't make a lot of
sense anymore. The requirements have shifted because of the reftable
backend, where we want to redesign interfaces so that we batch updates
together to the best extent possible.
And this patch series here demonstrates that not only the "reftable"
backend benefits.
In any case, the current infrastructure is extremely brittle and every
change one does is a bit like threading a needle. More likely than not
you missed this one edge case where changing a flag for one of the
updates has a consequence in a completely unrelated place.
I think we should start splitting this up more and introduce different
update types:
- Forced updates where we don't very the old state.
- Raceless updates where we do.
- Reflog updates that only write a message as well as an old/new
state.
If we had such an infrastructure it would also be feasible to introduce
more types, like deletes or renames.
> I guess that does not work with the two delete/create transactions you
> end up with here, though. And you need those to worry about D/F
> conflicts. But then...how did the original handle D/F conflicts? It kind
> of looks like it didn't, as it is doing a mass ref-by-ref rename in the
> middle.
>
> If the refs code learned how to order things to handle the D/F conflicts
> within a transaction, then we could do a single transaction. And it
> could learn about rename primitives.
True. But as I already mentioned to Junio I don't really know how to
backfill D/F conflict handling in the "files" backend. The problem is
that with preexisting "refs/heads/parent" and "refs/heads/parent/child"
you cannot create the latter ".lock" file. Sure, we can hack our way
around that in some manner. But is that backwards compatible if another
Git client were to operate in the same repository? I dunno.
> I dunno. I think that would be nicer, but it's probably not worth
> holding up this topic. Your perf numbers are very nice. I guess the
> possible flip-side is that the existing code could be faster when
> renaming a single ref (so no quadratic behavior) with a pathological
> reflog (so moving the file is faster than re-writing all of those logs).
>
> Hmm, yeah. Something like this:
>
> cat >setup <<-\EOF
> #!/bin/sh
>
> rm -rf repo
> git init repo
> cd repo
>
> git init server
> git -C server commit --allow-empty -m foo
>
> git remote add origin server
> git fetch
>
> # make the reflog gigantic
> perl -i -ne 'for my $i (1..10**5) { print }' .git/logs/refs/remotes/origin/main
> EOF
>
> hyperfine -p ./setup -L v old,new './git.{v} -C repo remote rename origin foo'
>
> results in:
>
> Benchmark 1: ./git.old -C repo remote rename origin foo
> Time (mean ± σ): 5.5 ms ± 1.1 ms [User: 1.5 ms, System: 1.3 ms]
> Range (min … max): 3.6 ms … 9.7 ms 58 runs
>
> Benchmark 2: ./git.new -C repo remote rename origin foo
> Time (mean ± σ): 476.3 ms ± 9.8 ms [User: 203.6 ms, System: 268.0 ms]
> Range (min … max): 467.8 ms … 498.7 ms 10 runs
>
> Summary
> ./git.old -C repo remote rename origin foo ran
> 86.43 ± 16.61 times faster than ./git.new -C repo remote rename origin foo
>
> It's hard to bring myself to care, though. This is a stupidly
> pathological reflog, and the absolute time change is peanuts compared to
> the per-ref cost you're fixing here.
For the "files" backend performance is worse, for the "reftable" backend
I'd expect that this might even be faster. Mostly because there is no
way to trivially rename a reflog -- we basically do the same on a rename
as we are doing with this patch series now.
Overall I don't care too much about this edge case. By default we never
write reflogs for remote references anyway, and I doubt that you'll ever
end up with a remote reflog that has thousands of entries. So I'd rather
make the general case fast even if the esoteric case becomes slower.
But ideally we're able to lift such limitations in the future if we were
to do the above rework.
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed
2025-07-29 12:24 ` Patrick Steinhardt
@ 2025-08-02 10:48 ` Jeff King
0 siblings, 0 replies; 37+ messages in thread
From: Jeff King @ 2025-08-02 10:48 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Han Jiang
On Tue, Jul 29, 2025 at 02:24:56PM +0200, Patrick Steinhardt wrote:
> > It's hard to bring myself to care, though. This is a stupidly
> > pathological reflog, and the absolute time change is peanuts compared to
> > the per-ref cost you're fixing here.
>
> For the "files" backend performance is worse, for the "reftable" backend
> I'd expect that this might even be faster. Mostly because there is no
> way to trivially rename a reflog -- we basically do the same on a rename
> as we are doing with this patch series now.
>
> Overall I don't care too much about this edge case. By default we never
> write reflogs for remote references anyway, and I doubt that you'll ever
> end up with a remote reflog that has thousands of entries. So I'd rather
> make the general case fast even if the esoteric case becomes slower.
>
> But ideally we're able to lift such limitations in the future if we were
> to do the above rework.
OK. It looks like you did end up with a single transaction in your
re-roll. So in theory _if_ we had a "rename" transaction entry the
backends could be smarter here. But I agree with you that touching the
core of the ref transaction code is tricky and liable to cause
regressions. Given the numbers I produced earlier, I'm fine with leaving
it for later (or maybe never) and just copying the reflog entries as you
do in your series.
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
` (2 preceding siblings ...)
2025-07-28 13:08 ` [PATCH 3/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
@ 2025-07-28 13:08 ` Patrick Steinhardt
2025-07-28 17:43 ` Junio C Hamano
2025-07-30 7:53 ` Karthik Nayak
2025-07-28 15:43 ` [PATCH 0/4] builtin/remote: rework how remote refs get renamed Junio C Hamano
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
5 siblings, 2 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-28 13:08 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang
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 introducing `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 <ps@pks.im>
---
builtin/remote.c | 15 +++++----------
refs.c | 8 +++++++-
refs.h | 2 ++
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/builtin/remote.c b/builtin/remote.c
index b1c55909184..11981f732bc 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -710,16 +710,8 @@ static int rename_one_ref(const char *old_refname, const char *referent,
{
struct rename_info *rename = cb_data;
struct strbuf *new_referent = rename->buf1;
- 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;
- }
-
renamed_refname(rename, old_refname, rename->new_refname);
if (flags & REF_ISSYMREF) {
@@ -976,8 +968,11 @@ 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),
- rename_one_ref, &rename);
+ 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(_("renaming references failed: %s"), rename.err->buf);
diff --git a/refs.c b/refs.c
index b820c3908bd..861a0deb924 100644
--- a/refs.c
+++ b/refs.c
@@ -1840,7 +1840,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 a39f873b1fe..9decd3126e3 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.
--
2.50.1.565.gc32cd1483b.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed
2025-07-28 13:08 ` [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
@ 2025-07-28 17:43 ` Junio C Hamano
2025-07-30 7:53 ` Karthik Nayak
1 sibling, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 17:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Han Jiang
Patrick Steinhardt <ps@pks.im> writes:
> 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.
Very true. Even if we guarantee that callbacks are fed references in
lexicographic order, that would only allow us to stop iterationg
when we see an irrelevant one", and still would force us to skip
over all the irrelevant ones until we see the first one in scope.
It makes perfect sense to give us a way to say "iterate only in this
subhierarchy" like this step does.
Nice.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed
2025-07-28 13:08 ` [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
2025-07-28 17:43 ` Junio C Hamano
@ 2025-07-30 7:53 ` Karthik Nayak
2025-07-31 8:28 ` Patrick Steinhardt
1 sibling, 1 reply; 37+ messages in thread
From: Karthik Nayak @ 2025-07-30 7:53 UTC (permalink / raw)
To: Patrick Steinhardt, git; +Cc: Junio C Hamano, Jeff King, Han Jiang
[-- Attachment #1: Type: text/plain, Size: 1429 bytes --]
Patrick Steinhardt <ps@pks.im> writes:
[snip]
> diff --git a/refs.c b/refs.c
> index b820c3908bd..861a0deb924 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -1840,7 +1840,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 a39f873b1fe..9decd3126e3 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.
>
> --
> 2.50.1.565.gc32cd1483b.dirty
Nit: we do expose the reference iterators now with
'kn/for-each-ref-skip' (merged to next). We could directly use the
iterator instead of introducting a specific function like this.
- Karthik
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed
2025-07-30 7:53 ` Karthik Nayak
@ 2025-07-31 8:28 ` Patrick Steinhardt
0 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 8:28 UTC (permalink / raw)
To: Karthik Nayak; +Cc: git, Junio C Hamano, Jeff King, Han Jiang
On Wed, Jul 30, 2025 at 09:53:13AM +0200, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> [snip]
>
> > diff --git a/refs.c b/refs.c
> > index b820c3908bd..861a0deb924 100644
> > --- a/refs.c
> > +++ b/refs.c
> > @@ -1840,7 +1840,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 a39f873b1fe..9decd3126e3 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.
> >
> > --
> > 2.50.1.565.gc32cd1483b.dirty
>
> Nit: we do expose the reference iterators now with
> 'kn/for-each-ref-skip' (merged to next). We could directly use the
> iterator instead of introducting a specific function like this.
I'll leave this as-is for now. The additional wrapper isn't all that
bad, and I'd rather want to avoid adding another dependency to this
patch series.
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 0/4] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
` (3 preceding siblings ...)
2025-07-28 13:08 ` [PATCH 4/4] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
@ 2025-07-28 15:43 ` Junio C Hamano
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
5 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2025-07-28 15:43 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Jeff King, Han Jiang
Patrick Steinhardt <ps@pks.im> writes:
> On the other hand this series reworks the logic used to rename remotes
> so that we use two transactions instead of one transaction per ref. This
> fixes quadratic runtime behaviour, where renaming 10k refs takes ~4
> minutes, 100k takes hours. This results in a significant speedup with
> both the "files" backend (benchmarked with a smaller number of refs to
> retain sanity):
Great. Hopefully we will teach transaction mechanism to sort out
its D/F false-positive bug so that we do not have to risk succeesing
the removal half of these two transactions while failing the adding
half of them soonish?
> But in any case, it's one more case where the "reftable" backend
> outperforms the "files" backend.
;-).
> The series is built on top of e4ef0485fd7 (The fourteenth batch,
> 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
> invalid old object IDs when migrating reflogs, 2025-07-25) merged into
> it.
>
> I'd normally have withheld sending until that series was merged to
> "next", but given that I promised to send something on Friday already I
> decided to just get it out. In any case, if that causes problems I'm
> happy to wait a bit before this series here gets merged into "seen".
>
> Thanks!
Great. Will queue. If the reflog-migrate-fix needs further work,
it shouldn't be too hard to rebase this one on my end, either.
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-07-28 13:08 [PATCH 0/4] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
` (4 preceding siblings ...)
2025-07-28 15:43 ` [PATCH 0/4] builtin/remote: rework how remote refs get renamed Junio C Hamano
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 1/6] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
` (6 more replies)
5 siblings, 7 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
Hi,
this patch series is the result from the discussion at [1]. On the one
hand this series fixes the reported bug where dangling symrefs are not
renamed via `git remote rename`.
On the other hand this series reworks the logic used to rename remotes
so that we use two transactions instead of one transaction per ref. This
fixes quadratic runtime behaviour, where renaming 10k refs takes ~4
minutes, 100k takes hours. This results in a significant speedup with
both the "files" backend (benchmarked with a smaller number of refs to
retain sanity):
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
not as extreme as with the "files" backend:
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~)
But in any case, it's one more case where the "reftable" backend
outperforms the "files" backend.
The series is built on top of e4ef0485fd7 (The fourteenth batch,
2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
invalid old object IDs when migrating reflogs, 2025-07-25) merged into
it.
I'd normally have withheld sending until that series was merged to
"next", but given that I promised to send something on Friday already I
decided to just get it out. In any case, if that causes problems I'm
happy to wait a bit before this series here gets merged into "seen".
Changes in v2:
- Insert another commit to fix sign-comparison warnings.
- Some code formatting fixes.
- Drop the infrastructure to reuse buffers. It didn't really help much
with performance anyway.
- I'm now using a single transaction to rename references so that we
have proper atomicity. Conflicts are detected early before any
changes are performed in the repository and an advice is printed
indicating what has happened.
- Link to v1: https://lore.kernel.org/r/20250728-pks-remote-rename-improvements-v1-0-f654f2b5c5ae@pks.im
Thanks!
Patrick
[1]: <CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com>
---
Patrick Steinhardt (6):
refs: pass refname when invoking reflog entry callback
refs: simplify logic when migrating reflog entries
builtin/remote: fix sign comparison warnings
builtin/remote: determine whether refs need renaming early on
builtin/remote: rework how remote refs get renamed
builtin/remote: only iterate through refs that are to be renamed
builtin/fsck.c | 9 +-
builtin/gc.c | 3 +-
builtin/remote.c | 345 +++++++++++++++++++++++++++++-----------------
builtin/stash.c | 6 +-
commit.c | 3 +-
object-name.c | 3 +-
reflog-walk.c | 7 +-
reflog.c | 3 +-
reflog.h | 3 +-
refs.c | 64 ++++-----
refs.h | 13 +-
refs/debug.c | 5 +-
refs/files-backend.c | 15 +-
refs/reftable-backend.c | 2 +-
remote.c | 6 +-
revision.c | 3 +-
t/helper/test-ref-store.c | 3 +-
t/t5505-remote.sh | 73 ++++++++++
wt-status.c | 6 +-
19 files changed, 372 insertions(+), 200 deletions(-)
Range-diff versus v1:
1: ca658850cf ! 1: 67b46a7afe refs: pass refname when invoking reflog entry callback
@@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noi
else if (!oideq(noid, cb->oid))
warning(_("log for ref %s unexpectedly ended on %s"),
- cb->refname, show_date(cb->date, cb->tz,
-+ refname, show_date(cb->date, cb->tz,
- DATE_MODE(RFC2822)));
+- DATE_MODE(RFC2822)));
++ refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
cb->reccnt++;
oidcpy(&cb->ooid, ooid);
+ oidcpy(&cb->noid, noid);
@@ refs.c: static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
return 0;
}
@@ refs.c: static int migrate_one_reflog(const char *refname, void *cb_data)
## refs.h ##
@@ refs.h: int refs_delete_reflog(struct ref_store *refs, const char *refname);
+ * The cb_data is a caller-supplied pointer given to the iterator
* functions.
*/
- typedef int each_reflog_ent_fn(
-+ const char *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);
+-typedef int each_reflog_ent_fn(
+- struct object_id *old_oid, struct object_id *new_oid,
+- const char *committer, timestamp_t timestamp,
+- int tz, const char *msg, void *cb_data);
++typedef int each_reflog_ent_fn(const char *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);
+
+ /* Iterate over reflog entries in the log for `refname`. */
+
## refs/debug.c ##
@@ refs/debug.c: struct debug_reflog {
2: b119cdfd48 = 2: 239f7acd1d refs: simplify logic when migrating reflog entries
-: ---------- > 3: e421091721 builtin/remote: fix sign comparison warnings
-: ---------- > 4: 0d88ed6365 builtin/remote: determine whether refs need renaming early on
3: 8d2678f27c ! 5: 8aed1ce7f5 builtin/remote: rework how remote refs get renamed
@@ Commit message
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 inperformant. The number of
+ 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:
@@ Commit message
The consequence here is quadratic runtime performance. Renaming a
100k references takes hours to complete.
- Ideally we'd refactor the code to use a single transaction to perform
- all the reference updates atomically. But unfortunately that does not
- work in the case where you rename a remote so that it becomes nested
- into itself, as that can lead to conflicting reference updates.
-
- The next-best thing is to do it in two transactions: one to delete all
- the references, and one to recreate the references and their reflogs.
- This signicantly speeds up the operation with the "files" backend. The
- following benchmark renames a remote with 10000 references:
+ 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]
@@ Commit message
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 verify that nesting remotes into itself continues to work
- with conflicting references.
+ 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
@@ Commit message
## builtin/remote.c ##
@@
+ #define USE_THE_REPOSITORY_VARIABLE
#include "builtin.h"
++#include "advice.h"
#include "config.h"
+#include "date.h"
#include "gettext.h"
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
const char *new_name;
- struct string_list *remote_branches;
- uint32_t symrefs_nr;
-+ struct ref_transaction *tx_create;
-+ struct ref_transaction *tx_delete;
++ struct ref_transaction *transaction;
+ struct progress *progress;
++ struct strbuf *err;
+ uint32_t progress_nr;
-+ struct strbuf *buf1, *buf2, *buf3, *err;
-+ struct strbuf *new_refname;
+ 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 renamed_refname(struct rename_info *rename,
-+ const char *refname,
-+ struct strbuf *out)
++static void compute_renamed_ref(struct rename_info *rename,
++ const char *refname,
++ struct strbuf *out)
+{
+ strbuf_reset(out);
+ strbuf_addstr(out, refname);
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ rename->new_name, strlen(rename->new_name));
+}
+
-+static int rename_one_reflog_entry(const char *old_refname UNUSED,
++static int rename_one_reflog_entry(const char *old_refname,
+ struct object_id *old_oid,
+ struct object_id *new_oid,
+ const char *committer,
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
- } else {
- item->util = NULL;
- }
-+ struct strbuf *identity = rename->buf1;
-+ struct strbuf *name = rename->buf2;
-+ struct strbuf *mail = rename->buf3;
++ 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;
+
-+ if (split_ident_line(&ident, committer, strlen(committer)) < 0)
-+ return -1;
++ compute_renamed_ref(rename, old_refname, &new_refname);
+
-+ strbuf_reset(name);
-+ strbuf_add(name, ident.name_begin, ident.name_end - ident.name_begin);
-+ strbuf_reset(mail);
-+ strbuf_add(mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
++ 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_reset(identity);
-+ strbuf_addstr(identity, fmt_ident(name->buf, mail->buf,
++ strbuf_addstr(&identity, fmt_ident(name.buf, mail.buf,
+ WANT_BLANK_IDENT, date, 0));
+
-+ error = ref_transaction_update_reflog(rename->tx_create, rename->new_refname->buf,
-+ new_oid, old_oid, identity->buf, msg,
++ 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;
+}
+
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ const struct object_id *old_oid,
+ struct rename_info *rename)
+{
-+ struct strbuf *message = rename->buf1;
++ 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))
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ error = refs_for_each_reflog_ent(get_main_ref_store(the_repository),
+ old_refname, rename_one_reflog_entry, rename);
+ if (error < 0)
-+ return error;
++ goto out;
++
++ compute_renamed_ref(rename, old_refname, &new_refname);
+
+ /*
+ * Manually write the reflog entry for the now-renamed ref. We cannot
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ * Furthermore, we only append the entry in case the reference
+ * resolves. Missing references shouldn't have reflogs anyway.
+ */
-+ strbuf_reset(message);
-+ strbuf_addf(message, "remote: renamed %s to %s", old_refname,
-+ rename->new_refname->buf);
++ strbuf_addf(&message, "remote: renamed %s to %s", old_refname,
++ new_refname.buf);
+
-+ error = ref_transaction_update_reflog(rename->tx_create, rename->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);
++ message.buf, rename->index++, rename->err);
+ if (error < 0)
+ return error;
+
++out:
++ strbuf_release(&new_refname);
++ strbuf_release(&message);
+ return error;
+}
+
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ 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;
-+ struct strbuf *new_referent = rename->buf1;
+ const char *ptr = old_refname;
+ int error;
+
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ !skip_prefix(ptr, "/", &ptr)) {
+ error = 0;
+ goto out;
- }
-- strbuf_release(&buf);
-
-- return 0;
-+ renamed_refname(rename, old_refname, rename->new_refname);
++ }
++
++ compute_renamed_ref(rename, old_refname, &new_refname);
+
+ if (flags & REF_ISSYMREF) {
+ /*
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ referent = refs_resolve_ref_unsafe(get_main_ref_store(the_repository),
+ old_refname, RESOLVE_REF_NO_RECURSE,
+ NULL, NULL);
-+ renamed_refname(rename, referent, new_referent);
++ compute_renamed_ref(rename, referent, &new_referent);
+ oid = NULL;
+ }
+
-+ error = ref_transaction_delete(rename->tx_delete, old_refname,
++ 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->tx_create, rename->new_refname->buf, oid, null_oid(the_hash_algo),
-+ (flags & REF_ISSYMREF) ? new_referent->buf : NULL, NULL,
++ 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)
@@ builtin/remote.c: static int add_branch_for_removal(const char *refname,
+ display_progress(rename->progress, ++rename->progress_nr);
+
+out:
++ strbuf_release(&new_referent);
++ strbuf_release(&new_refname);
+ return error;
}
@@ builtin/remote.c: static void handle_push_default(const char* old_name, const ch
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)
- {
@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,
};
struct remote *oldremote, *newremote;
@@ builtin/remote.c: 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, refspecs_need_update = 0;
- struct progress *progress = NULL;
+ old_remote_context = STRBUF_INIT, err = STRBUF_INIT;
+ struct rename_info rename = {
-+ .buf1 = &buf,
-+ .buf2 = &buf2,
-+ .buf3 = &buf3,
-+ .new_refname = &old_remote_context,
+ .err = &err,
+ };
-+ int i, refspec_updated = 0;
++ int refspecs_need_update = 0;
int result = 0;
argc = parse_options(argc, argv, prefix, options,
@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,
oldremote = remote_get(rename.old_name);
if (!remote_is_configured(oldremote, 1)) {
@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,
- goto out;
+ 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);
+@@ builtin/remote.c: 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.
-+ * Note that we're using two transactions to rename the references:
-+ *
-+ * - One transaction contains all deletions for references part of
-+ * the old remote.
-+ * - One transaction contains all creations of references and reflogs
-+ * part of the new remote.
-+ *
-+ * This split is required to avoid conflicting ref updates when a
-+ * remote is being nested into itself or converted into its parent
-+ * directory.
-+ *
-+ * Unfortunately this means that the operation isn't atomic. But we
-+ * cannot avoid that, unless transactions learn to handle such
-+ * conflicts one day.
- */
+- */
- refs_for_each_ref(get_main_ref_store(the_repository),
- read_remote_branches, &rename);
- if (show_progress) {
@@ builtin/remote.c: 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;
-+ rename.tx_delete = ref_store_transaction_begin(get_main_ref_store(the_repository),
-+ 0, &err);
-+ if (!rename.tx_delete)
-+ goto out;
-
+-
- 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);
-+ rename.tx_create = ref_store_transaction_begin(get_main_ref_store(the_repository),
-+ 0, &err);
-+ if (!rename.tx_create)
-+ goto out;
-
+-
- 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 (show_progress)
-+ rename.progress = start_delayed_progress(the_repository,
-+ _("Renaming remote references"), 0);
++ 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;
@@ builtin/remote.c: 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;
-+ result = refs_for_each_rawref(get_main_ref_store(the_repository),
-+ rename_one_ref, &rename);
-+ if (result < 0)
-+ die(_("renaming references failed: %s"), rename.err->buf);
++ stop_progress(&rename.progress);
- if (!item->util)
- continue;
@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,
- 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);
-+ result = ref_transaction_commit(rename.tx_delete, &err);
-+ if (result < 0)
-+ die(_("deleting old remote refs failed: %s"), rename.err->buf);
-+
-+ result = ref_transaction_commit(rename.tx_create, &err);
-+ if (result < 0)
-+ die(_("committing new remote refs failed: %s"), rename.err->buf);
-+
-+ stop_progress(&rename.progress);
-
- handle_push_default(rename.old_name, rename.new_name);
+-
+- handle_push_default(rename.old_name, rename.new_name);
out:
- string_list_clear(&remote_branches, 1);
-+ ref_transaction_free(rename.tx_create);
-+ ref_transaction_free(rename.tx_delete);
++ ref_transaction_free(rename.transaction);
strbuf_release(&old_remote_context);
strbuf_release(&buf);
strbuf_release(&buf2);
@@ t/t5505-remote.sh: test_expect_success 'forbid adding superset of existing remot
+ git remote add parent url &&
+ git update-ref refs/remotes/parent/child $COMMIT_ID &&
+ test_when_finished "git remote remove parent/child" &&
-+ git remote rename parent 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/child\n" >expected &&
++ printf "$COMMIT_ID commit\trefs/remotes/parent/child\n" >expected &&
+ test_cmp expected actual
+'
+
4: 6a68671b8f ! 6: 2fc020e25f builtin/remote: only iterate through refs that are to be renamed
@@ Commit message
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 introducing `refs_for_each_rawref_in()`, which knows to
- only iterate through (potentially broken) references in a given prefix.
+ 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
@@ Commit message
## builtin/remote.c ##
@@ builtin/remote.c: 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;
- struct strbuf *new_referent = rename->buf1;
- const char *ptr = old_refname;
int error;
@@ builtin/remote.c: static int rename_one_ref(const char *old_refname, const char
- goto out;
- }
-
- renamed_refname(rename, old_refname, rename->new_refname);
+ compute_renamed_ref(rename, old_refname, &new_refname);
if (flags & REF_ISSYMREF) {
@@ builtin/remote.c: static int mv(int argc, const char **argv, const char *prefix,
- rename.progress = start_delayed_progress(the_repository,
- _("Renaming remote references"), 0);
+ 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);
-+ strbuf_reset(&buf);
-+ strbuf_addf(&buf, "refs/remotes/%s/", rename.old_name);
+- 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(_("renaming references failed: %s"), rename.err->buf);
-
++ 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);
## refs.c ##
@@ refs.c: int refs_for_each_namespaced_ref(struct ref_store *refs,
---
base-commit: 49e86c58e5687de1089b97288ec5d9582ee6ebd6
change-id: 20250725-pks-remote-rename-improvements-85ce262146e0
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH v2 1/6] refs: pass refname when invoking reflog entry callback
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 2/6] refs: simplify logic when migrating reflog entries Patrick Steinhardt
` (5 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
With `refs_for_each_reflog_ent()` callers can iterate through all the
reflog entries for a given reference. The callback that is being invoked
for each such entry does not receive the name of the reference that we
are currently iterating through. This isn't really a limiting factor, as
callers can simply pass the name via the callback data.
But this layout sometimes does make for a bit of an awkward calling
pattern. One example: when iterating through all reflogs, and for each
reflog we iterate through all refnames, we have to do some extra book
keeping to track which reference name we are currently yielding reflog
entries for.
Change the signature of the callback function so that the reference name
of the reflog gets passed through to it. Adapt callers accordingly and
start using the new parameter in trivial cases. The next commit will
refactor the reference migration logic to make use of this parameter so
that we can simplify its logic a bit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/fsck.c | 9 ++++-----
builtin/gc.c | 3 ++-
builtin/stash.c | 6 ++++--
commit.c | 3 ++-
object-name.c | 3 ++-
reflog-walk.c | 7 ++++---
reflog.c | 3 ++-
reflog.h | 3 ++-
refs.c | 20 +++++++++-----------
refs.h | 11 +++++++----
refs/debug.c | 5 +++--
refs/files-backend.c | 15 +++++++++------
refs/reftable-backend.c | 2 +-
remote.c | 6 ++++--
revision.c | 3 ++-
t/helper/test-ref-store.c | 3 ++-
wt-status.c | 6 ++++--
17 files changed, 63 insertions(+), 45 deletions(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 0084cf7400b..67eb5e4fa0f 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -502,13 +502,12 @@ static void fsck_handle_reflog_oid(const char *refname, struct object_id *oid,
}
}
-static int fsck_handle_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int fsck_handle_reflog_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz UNUSED,
- const char *message UNUSED, void *cb_data)
+ const char *message UNUSED, void *cb_data UNUSED)
{
- const char *refname = cb_data;
-
if (verbose)
fprintf_ln(stderr, _("Checking reflog %s->%s"),
oid_to_hex(ooid), oid_to_hex(noid));
@@ -525,7 +524,7 @@ static int fsck_handle_reflog(const char *logname, void *cb_data)
strbuf_worktree_ref(cb_data, &refname, logname);
refs_for_each_reflog_ent(get_main_ref_store(the_repository),
refname.buf, fsck_handle_reflog_ent,
- refname.buf);
+ NULL);
strbuf_release(&refname);
return 0;
}
diff --git a/builtin/gc.c b/builtin/gc.c
index fab8f4dd4f7..9ae87065d35 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -312,7 +312,8 @@ struct count_reflog_entries_data {
size_t limit;
};
-static int count_reflog_entries(struct object_id *old_oid, struct object_id *new_oid,
+static int count_reflog_entries(const char *refname UNUSED,
+ struct object_id *old_oid, struct object_id *new_oid,
const char *committer, timestamp_t timestamp,
int tz, const char *msg, void *cb_data)
{
diff --git a/builtin/stash.c b/builtin/stash.c
index e2f95cc2ebc..a1ed67661e3 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -738,7 +738,8 @@ static int apply_stash(int argc, const char **argv, const char *prefix,
return ret;
}
-static int reject_reflog_ent(struct object_id *ooid UNUSED,
+static int reject_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
@@ -2173,7 +2174,8 @@ struct stash_entry_data {
size_t count;
};
-static int collect_stash_entries(struct object_id *old_oid UNUSED,
+static int collect_stash_entries(const char *refname UNUSED,
+ struct object_id *old_oid UNUSED,
struct object_id *new_oid,
const char *committer UNUSED,
timestamp_t timestamp UNUSED,
diff --git a/commit.c b/commit.c
index 15115125c36..7ebd05f3527 100644
--- a/commit.c
+++ b/commit.c
@@ -1031,7 +1031,8 @@ static void add_one_commit(struct object_id *oid, struct rev_collect *revs)
commit->object.flags |= TMP_MARK;
}
-static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int collect_one_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *ident UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
const char *message UNUSED, void *cbdata)
diff --git a/object-name.c b/object-name.c
index ddafe7f9b13..9ec192c3731 100644
--- a/object-name.c
+++ b/object-name.c
@@ -1516,7 +1516,8 @@ struct grab_nth_branch_switch_cbdata {
struct strbuf *sb;
};
-static int grab_nth_branch_switch(struct object_id *ooid UNUSED,
+static int grab_nth_branch_switch(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
diff --git a/reflog-walk.c b/reflog-walk.c
index c7070b13b00..4f1ce047498 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -22,9 +22,10 @@ struct complete_reflogs {
int nr, alloc;
};
-static int read_one_reflog(struct object_id *ooid, struct object_id *noid,
- const char *email, timestamp_t timestamp, int tz,
- const char *message, void *cb_data)
+static int read_one_reflog(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
+ const char *email, timestamp_t timestamp, int tz,
+ const char *message, void *cb_data)
{
struct complete_reflogs *array = cb_data;
struct reflog_info *item;
diff --git a/reflog.c b/reflog.c
index 39c205fd26e..2264b3bd605 100644
--- a/reflog.c
+++ b/reflog.c
@@ -492,7 +492,8 @@ void reflog_expiry_cleanup(void *cb_data)
free_commit_list(cb->mark_list);
}
-int count_reflog_ent(struct object_id *ooid UNUSED,
+int count_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp, int tz UNUSED,
diff --git a/reflog.h b/reflog.h
index 63bb56280f4..44b306c08ae 100644
--- a/reflog.h
+++ b/reflog.h
@@ -63,7 +63,8 @@ void reflog_expiry_prepare(const char *refname, const struct object_id *oid,
int should_expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data);
-int count_reflog_ent(struct object_id *ooid, struct object_id *noid,
+int count_reflog_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data);
int should_expire_reflog_ent_verbose(struct object_id *ooid,
diff --git a/refs.c b/refs.c
index 4bd80287054..6ed0cd6ddca 100644
--- a/refs.c
+++ b/refs.c
@@ -1022,7 +1022,6 @@ int is_branch(const char *refname)
}
struct read_ref_at_cb {
- const char *refname;
timestamp_t at_time;
int cnt;
int reccnt;
@@ -1052,7 +1051,8 @@ static void set_read_ref_cutoffs(struct read_ref_at_cb *cb,
*cb->cutoff_cnt = cb->reccnt;
}
-static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
+static int read_ref_at_ent(const char *refname,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz,
const char *message, void *cb_data)
@@ -1072,14 +1072,13 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
oidcpy(cb->oid, noid);
if (!oideq(&cb->ooid, noid))
warning(_("log for ref %s has gap after %s"),
- cb->refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
+ refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
}
else if (cb->date == cb->at_time)
oidcpy(cb->oid, noid);
else if (!oideq(noid, cb->oid))
warning(_("log for ref %s unexpectedly ended on %s"),
- cb->refname, show_date(cb->date, cb->tz,
- DATE_MODE(RFC2822)));
+ refname, show_date(cb->date, cb->tz, DATE_MODE(RFC2822)));
cb->reccnt++;
oidcpy(&cb->ooid, ooid);
oidcpy(&cb->noid, noid);
@@ -1094,7 +1093,8 @@ static int read_ref_at_ent(struct object_id *ooid, struct object_id *noid,
return 0;
}
-static int read_ref_at_ent_oldest(struct object_id *ooid, struct object_id *noid,
+static int read_ref_at_ent_oldest(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp, int tz,
const char *message, void *cb_data)
@@ -1117,7 +1117,6 @@ int read_ref_at(struct ref_store *refs, const char *refname,
struct read_ref_at_cb cb;
memset(&cb, 0, sizeof(cb));
- cb.refname = refname;
cb.at_time = at_time;
cb.cnt = cnt;
cb.msg = msg;
@@ -2976,14 +2975,14 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
struct reflog_migration_data {
uint64_t index;
- const char *refname;
struct ref_store *old_refs;
struct ref_transaction *transaction;
struct strbuf *errbuf;
struct strbuf *sb, *name, *mail;
};
-static int migrate_one_reflog_entry(struct object_id *old_oid,
+static int migrate_one_reflog_entry(const char *refname,
+ struct object_id *old_oid,
struct object_id *new_oid,
const char *committer,
timestamp_t timestamp, int tz,
@@ -3006,7 +3005,7 @@ static int migrate_one_reflog_entry(struct object_id *old_oid,
strbuf_reset(data->sb);
strbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));
- ret = ref_transaction_update_reflog(data->transaction, data->refname,
+ ret = ref_transaction_update_reflog(data->transaction, refname,
new_oid, old_oid, data->sb->buf,
msg, data->index++, data->errbuf);
return ret;
@@ -3016,7 +3015,6 @@ static int migrate_one_reflog(const char *refname, void *cb_data)
{
struct migration_data *migration_data = cb_data;
struct reflog_migration_data data = {
- .refname = refname,
.old_refs = migration_data->old_refs,
.transaction = migration_data->transaction,
.errbuf = migration_data->errbuf,
diff --git a/refs.h b/refs.h
index 99b58d0b73c..0bf50ce25cc 100644
--- a/refs.h
+++ b/refs.h
@@ -558,10 +558,13 @@ int refs_delete_reflog(struct ref_store *refs, const char *refname);
* The cb_data is a caller-supplied pointer given to the iterator
* functions.
*/
-typedef int each_reflog_ent_fn(
- struct object_id *old_oid, struct object_id *new_oid,
- const char *committer, timestamp_t timestamp,
- int tz, const char *msg, void *cb_data);
+typedef int each_reflog_ent_fn(const char *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);
/* Iterate over reflog entries in the log for `refname`. */
diff --git a/refs/debug.c b/refs/debug.c
index 485e3079d7a..5e113db307a 100644
--- a/refs/debug.c
+++ b/refs/debug.c
@@ -276,7 +276,8 @@ struct debug_reflog {
void *cb_data;
};
-static int debug_print_reflog_ent(struct object_id *old_oid,
+static int debug_print_reflog_ent(const char *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)
@@ -291,7 +292,7 @@ static int debug_print_reflog_ent(struct object_id *old_oid,
if (new_oid)
oid_to_hex_r(n, new_oid);
- ret = dbg->fn(old_oid, new_oid, committer, timestamp, tz, msg,
+ ret = dbg->fn(refname, old_oid, new_oid, committer, timestamp, tz, msg,
dbg->cb_data);
trace_printf_key(&trace_refs,
"reflog_ent %s (ret %d): %s -> %s, %s %ld \"%.*s\"\n",
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 3ebe0323d4e..24d0a8ebde0 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -2109,7 +2109,9 @@ static int files_delete_reflog(struct ref_store *ref_store,
return ret;
}
-static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
+static int show_one_reflog_ent(struct files_ref_store *refs,
+ const char *refname,
+ struct strbuf *sb,
each_reflog_ent_fn fn, void *cb_data)
{
struct object_id ooid, noid;
@@ -2136,7 +2138,7 @@ static int show_one_reflog_ent(struct files_ref_store *refs, struct strbuf *sb,
message += 6;
else
message += 7;
- return fn(&ooid, &noid, p, timestamp, tz, message, cb_data);
+ return fn(refname, &ooid, &noid, p, timestamp, tz, message, cb_data);
}
static char *find_beginning_of_line(char *bob, char *scan)
@@ -2220,7 +2222,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
strbuf_splice(&sb, 0, 0, bp + 1, endp - (bp + 1));
scanp = bp;
endp = bp + 1;
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
strbuf_reset(&sb);
if (ret)
break;
@@ -2232,7 +2234,7 @@ static int files_for_each_reflog_ent_reverse(struct ref_store *ref_store,
* Process it, and we can end the loop.
*/
strbuf_splice(&sb, 0, 0, buf, endp - buf);
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
strbuf_reset(&sb);
break;
}
@@ -2282,7 +2284,7 @@ static int files_for_each_reflog_ent(struct ref_store *ref_store,
return -1;
while (!ret && !strbuf_getwholeline(&sb, logfp, '\n'))
- ret = show_one_reflog_ent(refs, &sb, fn, cb_data);
+ ret = show_one_reflog_ent(refs, refname, &sb, fn, cb_data);
fclose(logfp);
strbuf_release(&sb);
return ret;
@@ -3323,7 +3325,8 @@ struct expire_reflog_cb {
dry_run:1;
};
-static int expire_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int expire_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email, timestamp_t timestamp, int tz,
const char *message, void *cb_data)
{
diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 99fafd75ebe..25a1d516184 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -2148,7 +2148,7 @@ static int yield_log_record(struct reftable_ref_store *refs,
full_committer = fmt_ident(log->value.update.name, log->value.update.email,
WANT_COMMITTER_IDENT, NULL, IDENT_NO_DATE);
- return fn(&old_oid, &new_oid, full_committer,
+ return fn(log->refname, &old_oid, &new_oid, full_committer,
log->value.update.time, log->value.update.tz_offset,
log->value.update.message, cb_data);
}
diff --git a/remote.c b/remote.c
index e965f022f12..db9eea4fa45 100644
--- a/remote.c
+++ b/remote.c
@@ -2578,7 +2578,8 @@ struct check_and_collect_until_cb_data {
};
/* Get the timestamp of the latest entry. */
-static int peek_reflog(struct object_id *o_oid UNUSED,
+static int peek_reflog(const char *refname UNUSED,
+ struct object_id *o_oid UNUSED,
struct object_id *n_oid UNUSED,
const char *ident UNUSED,
timestamp_t timestamp, int tz UNUSED,
@@ -2589,7 +2590,8 @@ static int peek_reflog(struct object_id *o_oid UNUSED,
return 1;
}
-static int check_and_collect_until(struct object_id *o_oid UNUSED,
+static int check_and_collect_until(const char *refname UNUSED,
+ struct object_id *o_oid UNUSED,
struct object_id *n_oid,
const char *ident UNUSED,
timestamp_t timestamp, int tz UNUSED,
diff --git a/revision.c b/revision.c
index 212ca0de276..0fc1a167a10 100644
--- a/revision.c
+++ b/revision.c
@@ -1699,7 +1699,8 @@ static void handle_one_reflog_commit(struct object_id *oid, void *cb_data)
}
}
-static int handle_one_reflog_ent(struct object_id *ooid, struct object_id *noid,
+static int handle_one_reflog_ent(const char *refname UNUSED,
+ struct object_id *ooid, struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp UNUSED,
int tz UNUSED,
diff --git a/t/helper/test-ref-store.c b/t/helper/test-ref-store.c
index 8d9a271845c..b2380d57ba3 100644
--- a/t/helper/test-ref-store.c
+++ b/t/helper/test-ref-store.c
@@ -215,7 +215,8 @@ static int cmd_for_each_reflog(struct ref_store *refs,
return refs_for_each_reflog(refs, each_reflog, NULL);
}
-static int each_reflog_ent(struct object_id *old_oid, struct object_id *new_oid,
+static int each_reflog_ent(const char *refname UNUSED,
+ struct object_id *old_oid, struct object_id *new_oid,
const char *committer, timestamp_t timestamp,
int tz, const char *msg, void *cb_data UNUSED)
{
diff --git a/wt-status.c b/wt-status.c
index 454601afa15..71bd17b610a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -972,7 +972,8 @@ static void wt_longstatus_print_changed(struct wt_status *s)
wt_longstatus_print_trailer(s);
}
-static int stash_count_refs(struct object_id *ooid UNUSED,
+static int stash_count_refs(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid UNUSED,
const char *email UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
@@ -1664,7 +1665,8 @@ struct grab_1st_switch_cbdata {
struct object_id noid;
};
-static int grab_1st_switch(struct object_id *ooid UNUSED,
+static int grab_1st_switch(const char *refname UNUSED,
+ struct object_id *ooid UNUSED,
struct object_id *noid,
const char *email UNUSED,
timestamp_t timestamp UNUSED, int tz UNUSED,
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 2/6] refs: simplify logic when migrating reflog entries
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 1/6] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 3/6] builtin/remote: fix sign comparison warnings Patrick Steinhardt
` (4 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
When migrating reflog entries between two storage formats we have to do
so via two callback-driven functions:
- `migrate_one_reflog()` gets invoked via `refs_for_each_reflog()` to
first list all available reflogs.
- `migrate_one_reflog_entry()` gets invoked via
`refs_for_each_reflog_ent()` in `migrate_one_reflog()`.
Before the preceding commit we didn't have the refname available in
`migrate_one_reflog_entry()`, which made it necessary to have a separate
structure that we pass to the second callback so that we can propagate
the refname. Now that `refs_for_each_reflog_ent()` knows to pass the
refname to the callback though that indirection isn't necessary anymore.
There's one catch though: we do have an update index that is also stored
in the entry-specific callback data. This update index is required so
that we can tell the ref backend in which order it should persist the
reflog entries to disk.
But that purpose can be trivially achieved by just converting it into a
global counter that is used for all reflog entries, regardless of which
reference they are for. The ordering will remain the same as both the
update index and the refname is considered when sorting the entries.
Move the index into `struct migration_data` and drop the now-unused
`struct reflog_migration_data` to simplify the code a bit.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
refs.c | 36 ++++++++++--------------------------
1 file changed, 10 insertions(+), 26 deletions(-)
diff --git a/refs.c b/refs.c
index 6ed0cd6ddc..04c9ace793 100644
--- a/refs.c
+++ b/refs.c
@@ -2941,6 +2941,7 @@ struct migration_data {
struct ref_transaction *transaction;
struct strbuf *errbuf;
struct strbuf sb, name, mail;
+ uint64_t index;
};
static int migrate_one_ref(const char *refname, const char *referent UNUSED, const struct object_id *oid,
@@ -2973,14 +2974,6 @@ static int migrate_one_ref(const char *refname, const char *referent UNUSED, con
return ret;
}
-struct reflog_migration_data {
- uint64_t index;
- struct ref_store *old_refs;
- struct ref_transaction *transaction;
- struct strbuf *errbuf;
- struct strbuf *sb, *name, *mail;
-};
-
static int migrate_one_reflog_entry(const char *refname,
struct object_id *old_oid,
struct object_id *new_oid,
@@ -2988,7 +2981,7 @@ static int migrate_one_reflog_entry(const char *refname,
timestamp_t timestamp, int tz,
const char *msg, void *cb_data)
{
- struct reflog_migration_data *data = cb_data;
+ struct migration_data *data = cb_data;
struct ident_split ident;
const char *date;
int ret;
@@ -2996,17 +2989,17 @@ static int migrate_one_reflog_entry(const char *refname,
if (split_ident_line(&ident, committer, strlen(committer)) < 0)
return -1;
- strbuf_reset(data->name);
- strbuf_add(data->name, ident.name_begin, ident.name_end - ident.name_begin);
- strbuf_reset(data->mail);
- strbuf_add(data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
+ strbuf_reset(&data->name);
+ strbuf_add(&data->name, ident.name_begin, ident.name_end - ident.name_begin);
+ strbuf_reset(&data->mail);
+ strbuf_add(&data->mail, ident.mail_begin, ident.mail_end - ident.mail_begin);
date = show_date(timestamp, tz, DATE_MODE(NORMAL));
- strbuf_reset(data->sb);
- strbuf_addstr(data->sb, fmt_ident(data->name->buf, data->mail->buf, WANT_BLANK_IDENT, date, 0));
+ strbuf_reset(&data->sb);
+ strbuf_addstr(&data->sb, fmt_ident(data->name.buf, data->mail.buf, WANT_BLANK_IDENT, date, 0));
ret = ref_transaction_update_reflog(data->transaction, refname,
- new_oid, old_oid, data->sb->buf,
+ new_oid, old_oid, data->sb.buf,
msg, data->index++, data->errbuf);
return ret;
}
@@ -3014,17 +3007,8 @@ static int migrate_one_reflog_entry(const char *refname,
static int migrate_one_reflog(const char *refname, void *cb_data)
{
struct migration_data *migration_data = cb_data;
- struct reflog_migration_data data = {
- .old_refs = migration_data->old_refs,
- .transaction = migration_data->transaction,
- .errbuf = migration_data->errbuf,
- .sb = &migration_data->sb,
- .name = &migration_data->name,
- .mail = &migration_data->mail,
- };
-
return refs_for_each_reflog_ent(migration_data->old_refs, refname,
- migrate_one_reflog_entry, &data);
+ migrate_one_reflog_entry, migration_data);
}
static int move_files(const char *from_path, const char *to_path, struct strbuf *errbuf)
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 3/6] builtin/remote: fix sign comparison warnings
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 1/6] refs: pass refname when invoking reflog entry callback Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 2/6] refs: simplify logic when migrating reflog entries Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 4/6] builtin/remote: determine whether refs need renaming early on Patrick Steinhardt
` (3 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
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 <ps@pks.im>
---
builtin/remote.c | 54 +++++++++++++++++++++++-------------------------------
1 file changed, 23 insertions(+), 31 deletions(-)
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
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 4/6] builtin/remote: determine whether refs need renaming early on
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
` (2 preceding siblings ...)
2025-07-31 14:56 ` [PATCH v2 3/6] builtin/remote: fix sign comparison warnings Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 14:56 ` [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
` (2 subsequent siblings)
6 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
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 <ps@pks.im>
---
builtin/remote.c | 12 ++++++++----
1 file changed, 8 insertions(+), 4 deletions(-)
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;
/*
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
` (3 preceding siblings ...)
2025-07-31 14:56 ` [PATCH v2 4/6] builtin/remote: determine whether refs need renaming early on Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-08-02 10:45 ` Jeff King
2025-07-31 14:56 ` [PATCH v2 6/6] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
2025-07-31 19:15 ` [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed Junio C Hamano
6 siblings, 1 reply; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
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]: <CANrWfmQWa=RJnm7d3C7ogRX6Tth2eeuGwvwrNmzS2gr+eP0OpA@mail.gmail.com>
Reported-by: Han Jiang <jhcarl0814@gmail.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
builtin/remote.c | 296 ++++++++++++++++++++++++++++++++++++------------------
t/t5505-remote.sh | 73 ++++++++++++++
2 files changed, 270 insertions(+), 99 deletions(-)
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
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed
2025-07-31 14:56 ` [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
@ 2025-08-02 10:45 ` Jeff King
2025-08-04 6:54 ` Patrick Steinhardt
0 siblings, 1 reply; 37+ messages in thread
From: Jeff King @ 2025-08-02 10:45 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Junio C Hamano, Han Jiang, Justin Tobler, Karthik Nayak
On Thu, Jul 31, 2025 at 04:56:53PM +0200, Patrick Steinhardt wrote:
> 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".
I think that was true even in the old code. E.g., if I do:
git init server
git -C server commit --allow-empty -m foo
git -C server branch a
git -C server branch b
git init
git remote add foo/b server
git fetch
git remote rename foo/b foo
then I get (before your patches):
error: 'refs/remotes/foo/b/main' exists; cannot create 'refs/remotes/foo/b'
fatal: renaming 'refs/remotes/foo/b/b' failed
Worse, we moved "a" but not "b" (nor "main"/"master", which are
important because they are what's blocking the rename of "b"). So we are
left with a broken half-moved state.
After your patches we get a nicer hint message, and of course we retain
the unbroken state from prior to the rename. So IMHO it is strictly
better.
-Peff
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed
2025-08-02 10:45 ` Jeff King
@ 2025-08-04 6:54 ` Patrick Steinhardt
0 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-08-04 6:54 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, Han Jiang, Justin Tobler, Karthik Nayak
On Sat, Aug 02, 2025 at 06:45:33AM -0400, Jeff King wrote:
> On Thu, Jul 31, 2025 at 04:56:53PM +0200, Patrick Steinhardt wrote:
>
> > 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".
>
> I think that was true even in the old code. E.g., if I do:
>
> git init server
> git -C server commit --allow-empty -m foo
> git -C server branch a
> git -C server branch b
>
> git init
> git remote add foo/b server
> git fetch
> git remote rename foo/b foo
>
> then I get (before your patches):
>
> error: 'refs/remotes/foo/b/main' exists; cannot create 'refs/remotes/foo/b'
> fatal: renaming 'refs/remotes/foo/b/b' failed
The test case I have added for this conflicting case only fails with the
postimage of this series. So we hit some _new_ edge cases now that
didn't exist before, but I think that's acceptable.
> Worse, we moved "a" but not "b" (nor "main"/"master", which are
> important because they are what's blocking the rename of "b"). So we are
> left with a broken half-moved state.
Yes, this half-applied state is quite awful.
> After your patches we get a nicer hint message, and of course we retain
> the unbroken state from prior to the rename. So IMHO it is strictly
> better.
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH v2 6/6] builtin/remote: only iterate through refs that are to be renamed
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
` (4 preceding siblings ...)
2025-07-31 14:56 ` [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed Patrick Steinhardt
@ 2025-07-31 14:56 ` Patrick Steinhardt
2025-07-31 19:15 ` [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed Junio C Hamano
6 siblings, 0 replies; 37+ messages in thread
From: Patrick Steinhardt @ 2025-07-31 14:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Han Jiang, Justin Tobler,
Karthik Nayak
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 <ps@pks.im>
---
builtin/remote.c | 13 ++++---------
refs.c | 8 +++++++-
refs.h | 2 ++
3 files changed, 13 insertions(+), 10 deletions(-)
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.
--
2.50.1.619.g074bbf1d35.dirty
^ permalink raw reply related [flat|nested] 37+ messages in thread* Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-07-31 14:56 ` [PATCH v2 0/6] " Patrick Steinhardt
` (5 preceding siblings ...)
2025-07-31 14:56 ` [PATCH v2 6/6] builtin/remote: only iterate through refs that are to be renamed Patrick Steinhardt
@ 2025-07-31 19:15 ` Junio C Hamano
2025-08-01 4:59 ` Patrick Steinhardt
6 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2025-07-31 19:15 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Han Jiang, Justin Tobler, Karthik Nayak
Patrick Steinhardt <ps@pks.im> writes:
> The series is built on top of e4ef0485fd7 (The fourteenth batch,
> 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
> invalid old object IDs when migrating reflogs, 2025-07-25) merged into
> it.
I'll use the newer iteration of the other topic that ends at
f0fde561 (refs: fix invalid old object IDs when migrating reflogs,
2025-07-29) instead; that was what was used in the version in 'seen'.
> I'd normally have withheld sending until that series was merged to
> "next", but given that I promised to send something on Friday already I
> decided to just get it out. In any case, if that causes problems I'm
> happy to wait a bit before this series here gets merged into "seen".
Thanks, will try to include this in the batch of this evening.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-07-31 19:15 ` [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed Junio C Hamano
@ 2025-08-01 4:59 ` Patrick Steinhardt
2025-08-01 16:43 ` Junio C Hamano
0 siblings, 1 reply; 37+ messages in thread
From: Patrick Steinhardt @ 2025-08-01 4:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Han Jiang, Justin Tobler, Karthik Nayak
On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > The series is built on top of e4ef0485fd7 (The fourteenth batch,
> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into
> > it.
>
> I'll use the newer iteration of the other topic that ends at
> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,
> 2025-07-29) instead; that was what was used in the version in 'seen'.
Okay, makes sense. I'll adapt my local base to match then.
> > I'd normally have withheld sending until that series was merged to
> > "next", but given that I promised to send something on Friday already I
> > decided to just get it out. In any case, if that causes problems I'm
> > happy to wait a bit before this series here gets merged into "seen".
>
> Thanks, will try to include this in the batch of this evening.
Thanks!
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-08-01 4:59 ` Patrick Steinhardt
@ 2025-08-01 16:43 ` Junio C Hamano
2025-08-04 6:51 ` Patrick Steinhardt
0 siblings, 1 reply; 37+ messages in thread
From: Junio C Hamano @ 2025-08-01 16:43 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Han Jiang, Justin Tobler, Karthik Nayak
Patrick Steinhardt <ps@pks.im> writes:
> On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:
>> Patrick Steinhardt <ps@pks.im> writes:
>>
>> > The series is built on top of e4ef0485fd7 (The fourteenth batch,
>> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
>> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into
>> > it.
>>
>> I'll use the newer iteration of the other topic that ends at
>> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,
>> 2025-07-29) instead; that was what was used in the version in 'seen'.
>
> Okay, makes sense. I'll adapt my local base to match then.
Curious. You had sent v3 but based your other topic on v2 and
expected the result will merge well?
>> > I'd normally have withheld sending until that series was merged to
>> > "next", but given that I promised to send something on Friday already I
>> > decided to just get it out. In any case, if that causes problems I'm
>> > happy to wait a bit before this series here gets merged into "seen".
>>
>> Thanks, will try to include this in the batch of this evening.
Which has happened.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-08-01 16:43 ` Junio C Hamano
@ 2025-08-04 6:51 ` Patrick Steinhardt
2025-08-04 18:24 ` Junio C Hamano
0 siblings, 1 reply; 37+ messages in thread
From: Patrick Steinhardt @ 2025-08-04 6:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Jeff King, Han Jiang, Justin Tobler, Karthik Nayak
On Fri, Aug 01, 2025 at 09:43:59AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
>
> > On Thu, Jul 31, 2025 at 12:15:42PM -0700, Junio C Hamano wrote:
> >> Patrick Steinhardt <ps@pks.im> writes:
> >>
> >> > The series is built on top of e4ef0485fd7 (The fourteenth batch,
> >> > 2025-07-24) with ps/reflog-migrate-fixes at de7cc0782a7 (refs: fix
> >> > invalid old object IDs when migrating reflogs, 2025-07-25) merged into
> >> > it.
> >>
> >> I'll use the newer iteration of the other topic that ends at
> >> f0fde561 (refs: fix invalid old object IDs when migrating reflogs,
> >> 2025-07-29) instead; that was what was used in the version in 'seen'.
> >
> > Okay, makes sense. I'll adapt my local base to match then.
>
> Curious. You had sent v3 but based your other topic on v2 and
> expected the result will merge well?
Well, the only reason why this series depends on the migration fixes for
reflogs is that the renamed reflogs for a remote would have invalid old
object IDs without it. We basically hit the bug that this other series
aims to fix.
There shouldn't be any textual conflicts between these two series.
Patrick
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH v2 0/6] builtin/remote: rework how remote refs get renamed
2025-08-04 6:51 ` Patrick Steinhardt
@ 2025-08-04 18:24 ` Junio C Hamano
0 siblings, 0 replies; 37+ messages in thread
From: Junio C Hamano @ 2025-08-04 18:24 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: git, Jeff King, Han Jiang, Justin Tobler, Karthik Nayak
Patrick Steinhardt <ps@pks.im> writes:
> There shouldn't be any textual conflicts between these two series.
What I meant was this. This series is built on top of an older
iteration of the other series. The other series however has a newer
iteration. We would eventually want to both topics in the system,
so as an early preview, both would be merged to 'seen'.
The topic branch for the other series has patches from iteration vN+1.
The topic branch for this series is, since it is built on top of the
merge of the other series at iteration vN into 'master'.
We merge the former into 'seen'; we now have patches from the other
series at iteration vN+1. We then merge the latter into 'seen'. It
wants to _also_ merge the patches from the other series at iteration
vN, that duplicates vN+1 but in different ways. If there wouldn't
be any textual conflicts between vN and vN+1 of the other series, it
may resolve cleanly, but is the result sane? These two iterations
are trying to be moral equivalents, with the difference that the
newer iteration is trying to be better than the older one.
And they in practice are most likely to textually conflict with each
other. After all they are different iteration of the same topic.
^ permalink raw reply [flat|nested] 37+ messages in thread