From: Patrick Steinhardt <ps@pks•im>
To: Karthik Nayak <karthik.188@gmail•com>
Cc: git@vger•kernel.org, toon@iotcl•com
Subject: Re: [PATCH 1/3] fetch: use batched reference updates
Date: Thu, 15 May 2025 13:30:57 +0200 [thread overview]
Message-ID: <aCXQcSsc4p6u9ljZ@pks.im> (raw)
In-Reply-To: <CAOLa=ZRbEqs6X1KJj-CikCANX-BC8r4RqbwoT06qmtF=x+7hQA@mail.gmail.com>
On Thu, May 15, 2025 at 11:13:32AM +0000, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks•im> writes:
> >> + * since pruning must be an independent step, to avoid F/D conflicts.
> >> + */
> >> + if (!transaction) {
> >> + transaction = ref_store_transaction_begin(get_main_ref_store(the_repository),
> >> + REF_TRANSACTION_ALLOW_FAILURE, &err);
> >> + if (!transaction) {
> >> + retcode = -1;
> >> + goto cleanup;
> >> + }
> >> + }
> >> +
> >> if (fetch_and_consume_refs(&display_state, transport, transaction, ref_map,
> >> &fetch_head, config)) {
> >> retcode = 1;
> >
> > Don't transactions handle D/F conflicts for us? Isn't that the sole
> > reason why for example `refs_verify_refname_available()` accepts an
> > "extras" parameter that is supposed to contain refs that are about to be
> > deleted?
> >
>
> My understanding was a little different, from the documentation for the
> function:
>
> If extras is non-NULL, it is a list of additional refnames with which
> refname is not allowed to conflict.
>
> This is to capture additional conflicts. We want a way to avoid said
> conflicts. That said, there is a 'skip' parameter which does exactly
> what you're saying.
Oh, right, my mistake -- that's what I actually meant.
> But the transaction logic doesn't incorporate this
> entirely. Specifically in the files backend, where we create a lock in
> the filesystem, this would cause a conflict, consider the following:
>
> ❯ eza --tree .git/refs/remotes/
> .git/refs/remotes
> └── origin
> ├── dir
> │ └── file.lock
> ├── dir.lock
> └── HEAD
>
> This is from the test 'branchname D/F conflict resolved by --prune', the
> test prunes the existing reference 'refs/remotes/origin/dir/file' while
> adding 'refs/remotes/origin/dir'. In 'lock_raw_ref()' we lock both and
> read the reference, but this causes an issue since
> 'refs/remotes/origin/dir' exists as a directory already.
>
> I would say this is logically solvable if we start treating conflict
> resolution within updates as a first class problem. But perhaps that's
> something of a patch series in itself and better solved outside of this?
Fair enough, makes sense. Might be worth it to add a TODO comment there
then?
Patrick
next prev parent reply other threads:[~2025-05-15 11:31 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-05-14 9:03 [PATCH 0/3] fetch/receive: use batched reference updates Karthik Nayak
2025-05-14 9:03 ` [PATCH 1/3] fetch: " Karthik Nayak
2025-05-14 12:31 ` Patrick Steinhardt
2025-05-15 11:13 ` Karthik Nayak
2025-05-15 11:30 ` Patrick Steinhardt [this message]
2025-05-15 11:36 ` Karthik Nayak
2025-05-14 17:36 ` Junio C Hamano
2025-05-14 9:03 ` [PATCH 2/3] send-pack: fix memory leak around duplicate refs Karthik Nayak
2025-05-14 17:46 ` Junio C Hamano
2025-05-15 11:23 ` Karthik Nayak
2025-05-14 9:03 ` [PATCH 3/3] receive-pack: use batched reference updates Karthik Nayak
2025-05-14 12:31 ` Patrick Steinhardt
2025-05-14 19:00 ` Junio C Hamano
2025-05-15 11:30 ` Karthik Nayak
2025-05-15 14:07 ` [PATCH v2 0/4] fetch/receive: " Karthik Nayak
2025-05-15 14:07 ` [PATCH v2 1/4] refs: add function to translate errors to strings Karthik Nayak
2025-05-15 19:11 ` Jeff King
2025-05-16 9:11 ` Karthik Nayak
2025-05-15 20:26 ` Junio C Hamano
2025-05-16 9:12 ` Karthik Nayak
2025-05-15 14:07 ` [PATCH v2 2/4] fetch: use batched reference updates Karthik Nayak
2025-05-16 5:40 ` Patrick Steinhardt
2025-05-16 9:53 ` Karthik Nayak
2025-05-16 10:00 ` Patrick Steinhardt
2025-05-18 11:30 ` Karthik Nayak
2025-05-15 14:07 ` [PATCH v2 3/4] send-pack: fix memory leak around duplicate refs Karthik Nayak
2025-05-15 14:07 ` [PATCH v2 4/4] receive-pack: use batched reference updates Karthik Nayak
2025-05-15 18:55 ` Jeff King
2025-05-15 19:09 ` Jeff King
2025-05-16 19:49 ` Karthik Nayak
2025-05-19 9:58 ` [PATCH v3 0/4] fetch/receive: " Karthik Nayak
2025-05-19 9:58 ` [PATCH v3 1/4] refs: add function to translate errors to strings Karthik Nayak
2025-05-19 9:58 ` [PATCH v3 2/4] fetch: use batched reference updates Karthik Nayak
2025-05-19 9:58 ` [PATCH v3 3/4] send-pack: fix memory leak around duplicate refs Karthik Nayak
2025-05-19 9:58 ` [PATCH v3 4/4] receive-pack: use batched reference updates Karthik Nayak
2025-05-19 18:14 ` [PATCH v3 0/4] fetch/receive: " Junio C Hamano
2025-05-20 9:05 ` Karthik Nayak
2025-05-21 13:14 ` Junio C Hamano
2025-05-22 6:00 ` Jeff King
2025-05-22 8:50 ` Karthik Nayak
2025-05-22 15:31 ` Jeff King
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=aCXQcSsc4p6u9ljZ@pks.im \
--to=ps@pks$(echo .)im \
--cc=git@vger$(echo .)kernel.org \
--cc=karthik.188@gmail$(echo .)com \
--cc=toon@iotcl$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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