Re: [PATCH v2] refs: run copy and rename through transactions

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 5, 2026, 06:03 UTC
Message-ID: <asM9m-_ZoX_5UQ-I@pks.im>
In-Reply-To: <CACQ=SRGSEsbNz3v3obd3JUOs2MrROnvuHkx1Dm51seCcv+12Cw@mail.gmail.com>

On Fri, Oct 02, 2026 at 04:16:08PM +0200, Maciej Ciemborowicz wrote:

Show 13 quoted lines

> On Fri, Oct 2, 2026 at 12:56 PM Patrick Steinhardt <ps@pks.im> wrote:
> > why can't we make this whole mechanism completely agnostic of the
> > backend and implement this via pure transactions?
> 
> The part I was trying to preserve is the existing reflog semantics. A
> normal ref transaction can express the logical ref updates. In example
> deleting the old ref and creating/updating the destination. But branch
> rename/copy also moves or copies the existing reflog history. For the
> files backend that currently involves filesystem-level reflog
> rename/copy and D/F handling, while reftable represents the same
> operation differently. So my assumption was that the logical ref
> updates could go through the generic transaction API, while the
> reflog-history operation would remain backend-specific.

Yes, the reflog semantics should of course stay the same. But nowadays, this would also be achievable with only backend-agnostic logic as the reference transactions have learned to write many reflog entries for a single reference. This was added back when we introduced the migration logic to convert between two different backends.

Now there's potentially two caveats:

  - I don't think we have a way to delete many old reflog entries yet.

  - There may be a significant impact on performance.

The question thus is whether we can avoid or fix those caveats somehow and thus arrive at a more future-proof mechanism.

Show 9 quoted lines

> > Is this new behaviour? Is this retaining old behaviour?
> 
> The source/destination revalidation is new validation required by
> introducing the preparing hook before the backend locks are taken. The
> hook can itself change one of the refs. Without revalidation, the hook
> payload could describe one state while the rename/copy later operates
> on another state. The intention is therefore to reject an operation
> when the state observed by the preparing hook is no longer the state
> being committed.

I don't feel like that's sensible. The "preparing" hook is explicitly run before we perform locking and is documented as such. So it is fully expected that the on-disk state may still change between executing this and the "prepared" phase. It is the responsibility of the hook author to handle such cases, we shouldn't do this ourselves as we're now starting to assume semantics of the hook itself.

Show 23 quoted lines

> > Sorry, but I'm going to stop reading here. This is not in a state that
> > is reviewable and has way too much stuff that is obviously generated by
> > an AI without much thought being put into it by the author. I don't want
> > to invest my time into a topic where the author has obviously not spent
> > their time thinking about it, either.
> 
> I'm really sorry to hear that. Yes, the patches I prepared were
> AI-assisted, but I do feel that I understand what I am doing. I would
> appreciate some understanding, though, as I do not work with C on a
> daily basis. The bug report and my attempt to fix it came from the
> fact that I am working on a Ruby gem for per-branch and per-worktree
> containerization. That is why I had to write git-hooks-ext, which is
> how I ended up running into this bug in the first place.
> 
> I am not insisting that my patch should be merged. I simply thought
> that submitting a patch might help get the bug fixed faster, and
> getting the bug fixed is what I care about most. Karthik Nayak offered
> to help fix it, so perhaps it would be better for someone who works
> with C on a daily basis to take it over.
> 
> I can, of course, also prepare a v3, split it into more commits, and
> explain my reasoning more clearly. But I cannot guarantee that it will
> meet your standards, simply because I am not yet familiar with them.

I'd suggest to iterate then. In the current version this patch is not in a shape that is ready for review. The patch needs to be split up, and there are a lot of gaps in the commit message. Taken together that gives the signal that you don't really understand what you are doing.

That doesn't mean that you cannot fix that with another iteration though. But I'd suggest to take your time prepping the next iteration to read through the code, understand the concepts and doubt what AI spits out.

Thanks!

Patrick