From: Jeff King <peff@peff.net>
Date: Sat, 02 Aug 2025 10:45:33 GMT
Subject: Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed
Message-ID: <20250802104533.GA1180347@coredump.intra.peff.net>
In-Reply-To: <20250731-pks-remote-rename-improvements-v2-5-dda6f083674d@pks.im>

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