From: Junio C Hamano <gitster@pobox.com>
To: Toon Claes <toon@iotcl.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] bundle: remove unneeded code
Date: Tue, 10 Dec 2024 11:11:06 +0900 [thread overview]
Message-ID: <xmqqzfl4l22t.fsf@gitster.g> (raw)
In-Reply-To: <20241209-fix-bundle-create-race-v1-1-e6513bdcbf8a@iotcl.com> (Toon Claes's message of "Mon, 09 Dec 2024 11:41:29 +0100")
Toon Claes <toon@iotcl.com> writes:
> The changes in commit c06793a4ed (allow git-bundle to create bottomless
> bundle, 2007-08-08) ensure annotated tags are properly preserved when
> creating a bundle using a revision range operation.
>
> At the time the range notation would peel the ends to their
> corresponding commit, meaning ref v2.0 would point to the v2.0^0 commit.
> So the above workaround was introduced. This code looks up the ref
> before it's written to the bundle, and if the ref doesn't point to the
> object we expect (for tags this would be a tag object), we skip the ref
> from the bundle. Instead, when the ref is a tag that's the positive end
> of the range (e.g. v2.0 from the range "v1.0..v2.0"), then that ref is
> written to the bundle instead.
>
> Later, in 895c5ba3c1 (revision: do not peel tags used in range notation,
> 2013-09-19), the behavior of parsing ranges was changed and the problem
> was fixed at the cause. But the workaround in bundle.c was not reverted.
>
> Now it seems this workaround can cause a race condition. git-bundle(1)
> uses setup_revisions() to parse the input into `struct rev_info`. Later,
> in write_bundle_refs(), it uses this info to write refs to the bundle.
> As mentioned at this point each ref is looked up again and checked
> whether it points to the object we expect. If not, the ref is not
> written to the bundle. But, when creating a bundle in a heavy traffic
> repository (a repo with many references, and frequent ref updates) it's
> possible a branch ref was updated between setup_revisions() and
> write_bundle_refs() and thus the extra check causes the ref to be
> skipped.
>
> The workaround was originally added to deal with tags, but the code path
> also gets hit by non-tag refs, causing this race condition. Because it's
> no longer needed, remove it and fix the possible race condition.
It is always a pleasure to read a patch based on the idea to
directly target a nicely analyzed "root cause".
Thanks.
prev parent reply other threads:[~2024-12-10 2:11 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-12-09 10:41 [PATCH] bundle: remove unneeded code Toon Claes
2024-12-09 13:09 ` karthik nayak
2024-12-10 0:15 ` Junio C Hamano
2024-12-10 2:11 ` Junio C Hamano [this message]
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=xmqqzfl4l22t.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=toon@iotcl.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).