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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jun 19, 2025, 09:39 UTC
Message-ID: <CAOLa=ZQUJ_RDLn9W64=0_E+ybeWjJ9oNtK036uTX1DGjQshxcw@mail.gmail.com>
In-Reply-To: <xmqqldpvably.fsf@gitster.g>

Junio C Hamano <gitster@pobox.com> writes:

Show 18 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> 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.
>

Good point, will add it in.

Show 9 quoted lines

>> 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.
>

I'm not sure which earlier you're referring to, so let me describe them all.

- Before we introduced batch updates. In this scenario, every reference
  update was independent. Meaning every update could pass/fail and we'd
  still iterate over all other references. Here, if a 'delete' reference
  passed and the 'create' failed, we'd end up in the scenario you
  mentioned.

- Post introduction of the batch updates. In this scenario, we use batch
  transactions, where all updates where batched together. Even here,
  individual updates can pass/fail and we'd still iterate over all the
  other references. The only issue was that, if there was a 'delete' &
  'create' with a D/F conflict, then the create would fail as they were
  within the same transaction. The ultimate fix for this is conflict
  resolution within a transaction.

- After this fix. In this scenario, we separate out deletions into their
  transaction, this ensures we don't have D/F conflicts within a single
  transaction. But we still iterate over all references.

You're right that now 'delete' half can pass and 'create' could fail, leaving the repository in a state the user didn't want. But this was also true before batched updates were introduced.

Show 5 quoted lines

> 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.
>

Yes definitely agreed here. Conflict resolution within a transaction is something we should definitely look into after this, wherein if there is a 'delete' and a 'create' within a single transaction they shouldn't cause D/F conflicts. Since the conflict is going to be deleted out anyways.

Show 7 quoted lines

> 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".
>

Totally valid point. I will add in something like that.

Show 10 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?
>

Well we possibly could, but could you explain the intent here? I'm not able to see why we'd need that. Mostly since we allow individual reference updates to fail when using batched updates. So the second transaction here could pass with a few individual updates failing, this would be similar to how things were before we introduced batched reference updates to 'git-receive-pack(1)'.

Show 7 quoted lines

> 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.
>

Thanks, and sorry for the late response, I was on vacation. Will send in a new version soon.

- Karthik