git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Karthik Nayak <karthik.188@gmail.com>
Cc: git@vger.kernel.org,  jltobler@gmail.com,  ps@pks.im,
	sunshine@sunshineco.com,
	 Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v4 2/2] receive-pack: handle reference deletions separately
Date: Fri, 13 Jun 2025 08:46:01 -0700	[thread overview]
Message-ID: <xmqqldpvably.fsf@gitster.g> (raw)
In-Reply-To: <20250613-6769-address-test-failures-in-the-next-branch-caused-by-batched-reference-updates-v4-2-ebf53edb9795@gmail.com> (Karthik Nayak's message of "Fri, 13 Jun 2025 10:10:18 +0200")

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.

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

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

  reply	other threads:[~2025-06-13 15:46 UTC|newest]

Thread overview: 40+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2025-06-02  9:57 [PATCH 0/3] refs: fix some bugs with batched-updates Karthik Nayak
2025-06-02  9:57 ` [PATCH 1/3] refs/files: skip updates with errors in batched updates Karthik Nayak
2025-06-02 12:00   ` Patrick Steinhardt
2025-06-02 12:46     ` Karthik Nayak
2025-06-02 15:06       ` Junio C Hamano
2025-06-02 17:13         ` Karthik Nayak
2025-06-02  9:57 ` [PATCH 2/3] t5516: use double quotes for tests with variables Karthik Nayak
2025-06-02 16:59   ` Eric Sunshine
2025-06-02 17:13     ` Karthik Nayak
2025-06-02  9:57 ` [PATCH 3/3] receive-pack: handle reference deletions separately Karthik Nayak
2025-06-02 11:59   ` Patrick Steinhardt
2025-06-02 12:54     ` Karthik Nayak
2025-06-02 15:20     ` Junio C Hamano
2025-06-02 15:56       ` Patrick Steinhardt
2025-06-04 11:08         ` Karthik Nayak
2025-06-05  8:19 ` [PATCH v2 0/2] refs: fix some bugs with batched-updates Karthik Nayak
2025-06-05  8:19   ` [PATCH v2 1/2] refs/files: skip updates with errors in batched updates Karthik Nayak
2025-06-05  8:19   ` [PATCH v2 2/2] receive-pack: handle reference deletions separately Karthik Nayak
2025-06-05  8:47     ` Patrick Steinhardt
2025-06-05  9:08       ` Karthik Nayak
2025-06-06  8:41 ` [PATCH v3 0/2] refs: fix some bugs with batched-updates Karthik Nayak
2025-06-06  8:41   ` [PATCH v3 1/2] refs/files: skip updates with errors in batched updates Karthik Nayak
2025-06-06  8:41   ` [PATCH v3 2/2] receive-pack: handle reference deletions separately Karthik Nayak
2025-06-12 17:03     ` Christian Couder
2025-06-12 20:40       ` Junio C Hamano
2025-06-13  7:23       ` Karthik Nayak
2025-06-13  8:10 ` [PATCH v4 0/2] refs: fix some bugs with batched-updates Karthik Nayak
2025-06-13  8:10   ` [PATCH v4 1/2] refs/files: skip updates with errors in batched updates Karthik Nayak
2025-06-13  8:10   ` [PATCH v4 2/2] receive-pack: handle reference deletions separately Karthik Nayak
2025-06-13 15:46     ` Junio C Hamano [this message]
2025-06-19  9:39       ` Karthik Nayak
2025-06-13 12:43   ` [PATCH v4 0/2] refs: fix some bugs with batched-updates Christian Couder
2025-06-13 18:57     ` Junio C Hamano
2025-06-20  7:15 ` [PATCH v5 " Karthik Nayak
2025-06-20  7:15   ` [PATCH v5 1/2] refs/files: skip updates with errors in batched updates Karthik Nayak
2025-06-20  7:15   ` [PATCH v5 2/2] receive-pack: handle reference deletions separately Karthik Nayak
2025-06-20 16:21   ` [PATCH v5 0/2] refs: fix some bugs with batched-updates Junio C Hamano
2025-06-21 11:08     ` Karthik Nayak
2025-06-22  4:23       ` Junio C Hamano
2025-06-22 14:20         ` Karthik Nayak

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqldpvably.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    --cc=jltobler@gmail.com \
    --cc=karthik.188@gmail.com \
    --cc=ps@pks.im \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).