Re: [PATCH v2 5/6] builtin/remote: rework how remote refs get renamed

From: Patrick Steinhardt <ps@pks.im>
Date: Aug 4, 2025, 06:54 UTC
Message-ID: <aJBZL9B4foXZW-ix@pks.im>
In-Reply-To: <20250802104533.GA1180347@coredump.intra.peff.net>

On Sat, Aug 02, 2025 at 06:45:33AM -0400, Jeff King wrote:

Show 25 quoted lines

> 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