Re: [PATCH 3/4] builtin/remote: rework how remote refs get renamed

[Patrick Steinhardt <ps@pks.im>]

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