Re: [PATCH v2 0/3] refs: report old OIDs for batched deletions

From: Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com>
Date: Sep 21, 2026, 20:01 UTC
Message-ID: <CACQ=SRG0q6Ezre3Z2bv6JJw07KXnUn2SDxNLTt0FqbwEdcbOqw@mail.gmail.com>
In-Reply-To: <CAOLa=ZR4V45R0zST_gxb3FMSWCwbi2MFN=5sCzhTAQfuZrRH7g@mail.gmail.com>

Karthik Nayak <karthik.188@gmail.com> writes:

> This does break user behavior though, previously we would never fail on
> pruning refs, but now we would and in a all-or-nothing manner. So
> perhaps a better way would be to use the REF_TRANSACTION_ALLOW_FAILURE?

That makes sense. I will use it.

> Here refnames is built, but below it is only used for the non-atomic
> flow. Perhaps, we should move this into the else block?

Yes, I will move construction of the refname and OID arrays into the non-atomic branch.

> Nit: the formatting seems off.

Will fix.

> Curiously, was an LLM used to create these patches? If so, please do
> read our policy in 'Documentation/SubmittingPatches' regarding AI usage.

Yes, I use an AI coding agent. Thank you for pointing me to the policy. I have now read it. I reviewed the resulting changes and tests, understand the implementation, so I take responsibility for the version I submit.

> Seems like the other branch condition also does the same, we can extract
> this out?

Will do.

> We do this in the previous block? We don't need a new iterator here.

Right. I will append the refname and corresponding OID in the same loop.

> test_create_repo is considered deprecated, let's use git init
> directly. While we're at it, we should also cleanup the directories we
> create here.

Will do it.

> Why do we need to pack-refs?

It is not required to reproduce this problem. I will remove it.

> All the commands above also run in the 'prune' directory, can we put all
> of them in this subshell?

Yes, I will move the repository operations into the subshell where possible.

Thanks for the review. I hope tomorow I will prepare v3.

Cheers,
- Maciej Ciemborowicz

On Mon, Sep 21, 2026 at 3:57 PM Karthik Nayak <karthik.188@gmail.com> wrote:

Show 42 quoted lines

>
> Maciej Ciemborowicz <maciej.ciemborowicz@gmail.com> writes:
>
> > The reference-transaction hook receives zero as both the old and new OID
> > when branch, tag, fetch, and remote delete refs through refs_delete_refs().
> > Those callers already know the values that they selected for deletion.
> >
> > Teach refs_delete_refs() to accept aligned old OIDs and pass them into the
> > transaction. Besides making the hook records useful, this makes the selected
> > callers reject concurrent changes instead of deleting values that they did
> > not inspect. For branch and tag, this restores the compare-and-delete
> > behavior that existed before 8198907795 converted them to batched deletion.
> > For pruning, it prevents a stale scan from deleting a ref updated by another
> > process.
> >
> > The values are already available at every updated call site, so the series
> > adds no ref reads and retains batched performance.
>
> I still have some concerns about backward comparability here, since we
> go from a delete all without any checks to a all-or-nothing situation,
> which can be an issue with commands like `git fetch --prune`.
>
> Left some comments on the individual patches.
>
> >
> > Changes since v1:
> >
> >  * Document the conditional deletion behavior and its race protection.
> >  * Add tests that update refs from the hook's preparing phase and verify that
> >    branch deletion and remote pruning preserve the concurrent update.
> >  * Avoid printing deletion status when a non-atomic prune fails.
> >  * Use a local string_list_item in refs_delete_refs(), as suggested by
> >    Karthik.
> >
> > Based on maint at e9019fcafe (Git 2.55).
>
> Might be worthwhile to rebase on top of master. Seems like there are
> conflicts with d38352cd43 (A few more fixes before -rc2, 2026-09-17).
>
> [snip]
>
> Thanks!