From: Maciej Ciemborowicz Date: Mon, 21 Sep 2026 20:01:12 GMT Subject: Re: [PATCH v2 0/3] refs: report old OIDs for batched deletions Message-ID: In-Reply-To: Karthik Nayak 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 wrote: > > Maciej Ciemborowicz 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!