Re: `git remote rename` does not work when `refs/remotes/server/HEAD` is unborn (when right after `git remote add -m`)

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2025, 18:38 UTC
Message-ID: <xmqqbjp9h1sk.fsf@gitster.g>
In-Reply-To: <aIIvHxR8wXLTCgMW@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 7 quoted lines

> I've quickly hacked something together now, see the work-in-progress
> patch below. The patch does not yet handle reflogs, but that isn't too
> hard to implement.
>
> And these changes indeed speed up things by quite a lot: instead of
> hours it now takes 7 seconds :) I'll polish this patch series and will
> likely send it in tomorrow.

Nice. Not just the "oops, we shouldn't assume symrefs always point at an existing ref" fix, performance fix comes at the same time ;-)

Show 13 quoted lines

> diff --git a/builtin/remote.c b/builtin/remote.c
> index 5dd6cbbaeed..072a70e6b45 100644
> --- a/builtin/remote.c
> +++ b/builtin/remote.c
> @@ -612,36 +612,53 @@ 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 strbuf *err;
>  };

OK, as a place to hook the transaction into, rename_info is a convenient place, as it already is passed around throughout the relevant code paths.

Show 45 quoted lines

> -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 int queue_one_rename(const char *refname, const char *referent,
> +			    const struct object_id *oid,
> +			    int flags, void *cb_data)
>  {
> +	struct strbuf new_refname = STRBUF_INIT, new_referent = STRBUF_INIT;
>  	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;
> -		}
> +	int error;
> +
> +	strbuf_addf(&new_refname, "refs/remotes/%s/", rename->old_name);
> +	if (!starts_with(refname, new_refname.buf)) {
> +		error = 0;
> +		goto out;
>  	}
> -	strbuf_release(&buf);
>  
> -	return 0;
> +	if (flags & REF_ISSYMREF) {
> +		strbuf_addstr(&new_referent, referent);
> +		strbuf_splice(&new_referent, strlen("refs/remotes/"), strlen(rename->old_name),
> +			      rename->new_name, strlen(rename->new_name));
> +		oid = NULL;
> +	}
> +
> +	error = ref_transaction_delete(rename->transaction, refname,
> +				       oid, referent, REF_NO_DEREF, NULL, rename->err);

Remove old ...

Show 12 quoted lines

> +	if (error < 0)
> +		goto out;
> +
> +	strbuf_reset(&new_refname);
> +	strbuf_addstr(&new_refname, refname);
> +	strbuf_splice(&new_refname, strlen("refs/remotes/"), strlen(rename->old_name),
> +		      rename->new_name, strlen(rename->new_name));
> +
> +	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);

... and create new. Would we be hit with the same "while renaming A to A/B, there is a D/F conflict which the ref transaction does not handle by itself" issue we saw recently here?

Show 19 quoted lines

> +	rename.transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> +							 0, &err);
> +	if (!rename.transaction)
> +		goto out;
>  
> +	result = refs_for_each_rawref(get_main_ref_store(the_repository),
> +				      queue_one_rename, &rename);
> +	if (result < 0)
> +		die(_("renaming references failed: %s"), rename.err->buf);
>  
> +	result = ref_transaction_commit(rename.transaction, &err);
> +	if (result < 0)
> +		die(_("committing renamed references failed: %s"), rename.err->buf);
>  
>  	handle_push_default(rename.old_name, rename.new_name);
>  
>  out:
> -	string_list_clear(&remote_branches, 1);
> +	ref_transaction_free(rename.transaction);

Very nice.

Show 8 quoted lines

>  	strbuf_release(&old_remote_context);
>  	strbuf_release(&buf);
>  	strbuf_release(&buf2);
> -	strbuf_release(&buf3);
> +	strbuf_release(&err);
>  	return result;
>  }
>