On Mon, Jul 28, 2025 at 10:19:53AM -0700, Junio C Hamano wrote:

Patrick Steinhardt [off-list ref] writes:

quoted

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.

quoted

+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.

quoted

+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.

quoted

+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 ...

quoted

+	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.

quoted

+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