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

[Patrick Steinhardt <ps@pks.im>]

On Thu, Sep 10, 2026 at 07:55:28AM -0700, Ariel Keselman wrote:
> 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.
> 
> Test deleting a root ref with packed-refs.lock held, and check that a
> transaction deleting both a root ref and a packed branch still fails
> without changing either ref.

Nit: this last paragraph doesn't really add any value, as it's trivially
visible from the patch that we add tests.

> Signed-off-by: Ariel Keselman <skariel@gmail.com>
> ---
> Thanks for the review, Patrick.
> 
> Changes since v1:
> - Keep the packed-ref deletion comment focused on its original purpose.
> - Use one existing root ref and include a packed-refs file in the test.
> - Drop the timeout overrides and redundant individual-ref cases.
> - Add a transaction deleting a root ref and a packed branch together;
>   check that a held packed-ref lock causes failure and preserves both refs.
> 
> AI assistance was used to generate the patch, tests, and commit message,
> including this revision.
> 
> The root-ref deletion regression fails without the fix. With the fix,
> 123 test scripts / 4078 tests pass, along with 249 unit tests and t0600
> with SHA-256 (one platform skip in t0600).

Huh? I hope that _all_ tests pass with this, not only 4078, and I would
assume that you verified that this is the case at least on your machine.

> diff --git a/t/t0600-reffiles-backend.sh b/t/t0600-reffiles-backend.sh
> index 74bfa2e9ba..65ca19e84b 100755
> --- a/t/t0600-reffiles-backend.sh
> +++ b/t/t0600-reffiles-backend.sh
> @@ -519,4 +519,47 @@ test_expect_success 'symref transaction supports false symlink config' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'deleting a root ref does not lock packed-refs' '
> +	test_when_finished "rm -rf root-ref" &&
> +	git init root-ref &&
> +	(
> +		cd root-ref &&
> +		test_commit initial &&
> +		git pack-refs --all &&
> +		cp .git/packed-refs expect &&
> +		git update-ref AUTO_MERGE HEAD &&

For added benefit we could even execute git-pack-refs(1) after having
created AUTO_MERGE and then execute `test_path_is_file` for it just to
prove that it really doesn't get packed. But other than that the tests
look good to me.

> +		: >.git/packed-refs.lock &&
> +		git update-ref --no-deref -d AUTO_MERGE &&
> +		test_path_is_missing .git/AUTO_MERGE &&
> +		test_path_is_file .git/packed-refs.lock &&
> +		test_cmp expect .git/packed-refs
> +	)
> +'
> +
> +test_expect_success 'deleting root and packed refs in one transaction requires packed-refs lock' '
> +	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 &&
> +		git update-ref AUTO_MERGE HEAD &&
> +		git rev-parse AUTO_MERGE refs/heads/packed-branch >expect &&

We could strengthen this a bit by listing the state of all refs:

    git refs list --include-root-refs >expect

> +		cat >stdin <<-EOF &&
> +		start
> +		delete AUTO_MERGE
> +		delete refs/heads/packed-branch
> +		prepare
> +		commit

We can drop start/prepare/commit here, those are optional. We can also
drop the extra file and just write the data into git-update-ref(1)
directly via the heredoc.

> +		EOF
> +		: >.git/packed-refs.lock &&
> +		test_must_fail git update-ref --no-deref --stdin <stdin 2>err &&
> +		test_grep "Unable to create .*packed-refs.lock" err &&
> +		git rev-parse AUTO_MERGE refs/heads/packed-branch >actual &&
> +		test_cmp expect actual
> +	)
> +'

Thanks!

Patrick