Re: [PATCH] refs/files: avoid packed-refs lock for root ref deletion

[Patrick Steinhardt <ps@pks.im>]

Hi,

On Wed, Sep 09, 2026 at 11:45:46PM -0700, Ariel Keselman wrote:
> Hi,
> 
> Deleting root refs in the files backend unnecessarily locks
> packed-refs, even though root refs cannot be packed. This can cause
> post-commit cleanup to report an error after a successful commit in a
> linked worktree with read-only shared metadata.
> 
> The attached patch skips that lock for root-ref deletion and adds
> regression tests. All seven new tests fail without the fix and pass
> with it; broader ref, worktree, and sequencer tests also pass.
> 
> AI assistance was used to generate the patch, tests, and commit message.

Please consult Documentation/SubmittingPatches. The expectation is that
patches will not be sent as attachments. I'd recommend using a tool like
b4 to send your patches, which handles a lot of the nuisances for you.

> From c5d12e97a78123965590553fddc0dd78af3e006e Mon Sep 17 00:00:00 2001
> From: Ariel Keselman <skariel@gmail.com>
> Date: Wed, 9 Sep 2026 22:16:50 -0700
> To: git@vger.kernel.org
> Subject: [PATCH] refs/files: avoid packed-refs lock for root ref deletion
> 
> Deleting a root ref queues a packed-ref transaction in the files
> backend, even though root refs cannot be packed. For example, holding
> .git/packed-refs.lock makes "git update-ref --no-deref -d AUTO_MERGE"
> fail, whether or not AUTO_MERGE exists.
> 
> This also affects post-commit cleanup, which deletes AUTO_MERGE after
> updating HEAD. In a linked worktree with read-only shared metadata,
> commit succeeds but cleanup reports a packed-refs.lock error. Deleting
> CHERRY_PICK_HEAD and REVERT_HEAD is affected as well.
> 
> Skip the packed transaction for root-ref deletions. Keep loose-ref
> locking and packed-ref deletion for other refs unchanged.

Makes sense indeed. Root refs are never packed, and consequently it does
not make any sense for us to try to evict them from packed-refs, either.

> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a4c7858787..41887f180f 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -2981,10 +2981,13 @@ static int files_transaction_prepare(struct ref_store *ref_store,
>  
>  		if (update->flags & REF_DELETING &&
>  		    !(update->flags & REF_LOG_ONLY) &&
> -		    !(update->flags & REF_IS_PRUNING)) {
> +		    !(update->flags & REF_IS_PRUNING) &&
> +		    !is_root_ref(update->refname)) {
>  			/*
> -			 * This reference has to be deleted from
> -			 * packed-refs if it exists there.
> +			 * Root refs cannot be packed. Do not acquire the shared
> +			 * packed-refs lock when deleting a per-worktree root ref.
> +			 * Other references have to be deleted from
> +			 * packed-refs if they exist there.
>  			 */

Nit: I feel like this comment is a bit too focussed on the root refs
now. A small, incremental change could've been:

	/*
     * This reference has to be deleted from packed-refs if it exists
     * there. Note that root refs are never packed, so we don't have to
     * deltee those from packed-refs.
	 */

>  			if (!packed_transaction) {
>  				packed_transaction = ref_store_transaction_begin(
> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index 74bfa2e9ba..b7f3287841 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -519,4 +519,50 @@ test_expect_success 'symref transaction supports false symlink config' '
>  	test_cmp expect actual
>  '
>  
> +for ref in AUTO_MERGE CHERRY_PICK_HEAD REVERT_HEAD

Isn't it a bit excessive to test for all these different root refs? I
don't see much value in doing that.

> +do
> +	for state in existing missing

Likewise, I'm not quite sure what we prove here. Should be fine to just
test with an existing root ref.

> +	do
> +		test_expect_success "deleting $state $ref does not lock packed-refs" '
> +			test_when_finished "rm -rf root-ref" &&
> +			git init root-ref &&
> +			(
> +				cd root-ref &&
> +				test_commit initial &&
> +				if test "$state" = existing
> +				then
> +					git update-ref "$ref" HEAD
> +				fi &&
> +				: >.git/packed-refs.lock &&
> +				git -c core.packedRefsTimeout=0 update-ref --no-deref -d "$ref" &&

Setting the timeout shouldn't really have any impact on the test result,
should it?

> +				test_path_is_missing ".git/$ref" &&
> +				test_path_is_file .git/packed-refs.lock
> +			)
> +		'
> +	done
> +done
> +
> +test_expect_success 'root ref deletion preserves packed refs and their locking' '
> +	test_when_finished "rm -rf root-ref" &&
> +	git init root-ref &&
> +	(
> +		cd root-ref &&
> +		test_commit initial &&
> +		git update-ref refs/heads/packed-branch HEAD &&
> +		git pack-refs --all &&
> +		test_path_is_missing .git/refs/heads/packed-branch &&
> +		cp .git/packed-refs expect &&
> +		git update-ref AUTO_MERGE HEAD &&
> +		: >.git/packed-refs.lock &&
> +		git -c core.packedRefsTimeout=0 update-ref --no-deref -d AUTO_MERGE &&
> +		test_cmp expect .git/packed-refs &&
> +		test_must_fail git -c core.packedRefsTimeout=0 update-ref -d refs/heads/packed-branch 2>err &&
> +		test_grep "Unable to create .*packed-refs.lock" err &&
> +		test_cmp expect .git/packed-refs &&
> +		rm .git/packed-refs.lock &&
> +		git update-ref -d refs/heads/packed-branch &&
> +		test_must_fail git rev-parse --verify refs/heads/packed-branch
> +	)
> +'

And this test feels like it's testing almost exactly what the other
test does. The only difference is that we have an actual packed-refs
file, but that can easily be squashed into the other test, too.

That being said, what we're missing is a test that creates a single
transaction that updates both a root ref and a non-root-ref with a
preexisting lockfile. Such a transaction should fail even though we skip
the packed transaction for the roof ref itself.

Thanks!

Patrick