Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 13, 2025, 15:46 UTC
Message-ID: <xmqqldpvably.fsf@gitster.g>
In-Reply-To: <20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-2-ebf53edb9795@gmail.com>

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

Show 12 quoted lines

> In 9d2962a7c4 (receive-pack: use batched reference updates, 2025-05-19)
> we updated the 'git-receive-pack(1)' command to use batched reference
> updates. One edge case which was missed during this implementation was
> when a user pushes multiple branches such as:
>
>   delete refs/heads/branch/conflict
>   create refs/heads/branch
>
> Before using batched updates, the references would be applied
> sequentially and hence no conflicts would arise. With batched updates,
> while the first update applies, the second fails due to D/F conflict. A
> similar issue was present in 'git-fetch(1)' and was fixed by separating

Do you have a reference to such an earlier fix to "git fetch"? If so, let's add it here.

> out reference pruning into a separate transaction. Apply a similar
> mechanism for 'git-receive-pack(1)' and separate out reference deletions
> into its own batch.

The implication of this is that the earlier "delete" half of the operation can succeed and be committed but the "create" half can fail, leaving the resulting repository without the reference the user wanted to have.

For now, this "two transactions" may suffice as a workaround but do you think it is a viable solution for longer term? As long as we claim that the reference updates are transactional, my answer is no. We'd need to fix it at a lower layer within a single transaction.

It is outside the topic of this patch series but we can at least leave a NEEDSWORK comment that this is merely a workaround and we'll have to fix the later? I see a in-code comment that says "To mitigate this" to hint the nature of the two phase solution, but we may want an explicit note that says that "we know this is broken even though it is less broken than it used to be".

Show 5 quoted lines

> This means 'git-receive-pack(1)' will now use up to two transactions,
> whereas before using batched updates it would use _at least_ two
> transactions. So using batched updates is still the better option.
>
> Add a test to validate this behavior.

I wonder if we can write a test against a remote that accepts deletions but fails the actions in the second phase as a test_expect_failure documentation?

Other than that, very well described. I know it is hard to describe a patch that knowingly does a workaround instead of doing the right thing for the sake of simplicity, and the proposed log message did a very good job at it.

Will queue.  Thanks.