From: Junio C Hamano <gitster@pobox.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: git@vger.kernel.org, dstolee@microsoft.com, peff@peff.net
Subject: Re: [PATCH 1/2] packfile.c: protect against disappearing indexes
Date: Wed, 25 Nov 2020 13:15:07 -0800 [thread overview]
Message-ID: <xmqq7dq9dtpg.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <cee25cc1ca5bc3371e32099980f28b623c1349d5.1606324509.git.me@ttaylorr.com> (Taylor Blau's message of "Wed, 25 Nov 2020 12:17:28 -0500")
Taylor Blau <me@ttaylorr.com> writes:
> In 17c35c8969 (packfile: skip loading index if in multi-pack-index,
> 2018-07-12) we stopped loading the .idx file for packs that are
> contained within a multi-pack index.
>
> This saves us the effort of loading an .idx and doing some lightweight
> validity checks by way of 'packfile.c:load_idx()', but introduces a race
> between processes that need to load the index (e.g., to generate a
> reverse index) and processes that can delete the index.
Ah, OK. And demonstration using %(objectsize:disk) makes perfect
sense given the above explanation.
> These two together effectively revert 17c35c8969, and avoid the race
> explained above.
>
> Co-authored-by: Jeff King <peff@peff.net>
> Signed-off-by: Taylor Blau <me@ttaylorr.com>
> ---
> packfile.c | 19 ++-----------------
> t/t5319-multi-pack-index.sh | 24 ++++++++++++++++++++++--
> 2 files changed, 24 insertions(+), 19 deletions(-)
Makes sense. Thanks.
> diff --git a/packfile.c b/packfile.c
> index 0929ebe4fc..8d7f37a5f6 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -514,19 +514,8 @@ static int open_packed_git_1(struct packed_git *p)
> ssize_t read_result;
> const unsigned hashsz = the_hash_algo->rawsz;
>
> - if (!p->index_data) {
> - struct multi_pack_index *m;
> - const char *pack_name = pack_basename(p);
> -
> - for (m = the_repository->objects->multi_pack_index;
> - m; m = m->next) {
> - if (midx_contains_pack(m, pack_name))
> - break;
> - }
> -
> - if (!m && open_pack_index(p))
> - return error("packfile %s index unavailable", p->pack_name);
> - }
> + if (open_pack_index(p))
> + return error("packfile %s index unavailable", p->pack_name);
>
> if (!pack_max_fds) {
> unsigned int max_fds = get_max_fd_limit();
> @@ -567,10 +556,6 @@ static int open_packed_git_1(struct packed_git *p)
> " supported (try upgrading GIT to a newer version)",
> p->pack_name, ntohl(hdr.hdr_version));
>
> - /* Skip index checking if in multi-pack-index */
> - if (!p->index_data)
> - return 0;
> -
> /* Verify the pack matches its index. */
> if (p->num_objects != ntohl(hdr.hdr_entries))
> return error("packfile %s claims to have %"PRIu32" objects"
> diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
> index ace469c95c..d4607daec1 100755
> --- a/t/t5319-multi-pack-index.sh
> +++ b/t/t5319-multi-pack-index.sh
> @@ -138,7 +138,7 @@ test_expect_success 'write midx with one v2 pack' '
>
> compare_results_with_midx "one v2 pack"
>
> -test_expect_success 'corrupt idx not opened' '
> +test_expect_success 'corrupt idx reports errors' '
> idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&
> mv $objdir/pack/$idx backup-$idx &&
> test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&
> @@ -149,7 +149,7 @@ test_expect_success 'corrupt idx not opened' '
> test_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&
>
> git -c core.multiPackIndex=true rev-list --objects --all 2>err &&
> - test_must_be_empty err
> + grep "index unavailable" err
> '
>
> test_expect_success 'add more objects' '
> @@ -755,4 +755,24 @@ test_expect_success 'repack --batch-size=<large> repacks everything' '
> )
> '
>
> +test_expect_success 'load reverse index when missing .idx' '
> + git init repo &&
> + test_when_finished "rm -fr repo" &&
> + (
> + cd repo &&
> +
> + git config core.multiPackIndex true &&
> +
> + test_commit base &&
> + git repack -ad &&
> + git multi-pack-index write &&
> +
> + git rev-parse HEAD >tip &&
> + idx=$(ls .git/objects/pack/pack-*.idx) &&
> +
> + mv $idx $idx.bak &&
> + git cat-file --batch-check="%(objectsize:disk)" <tip
> + )
> +'
> +
> test_done
next prev parent reply other threads:[~2020-11-25 21:27 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-25 17:17 [PATCH 0/2] midx: prevent against racily disappearing packs Taylor Blau
2020-11-25 17:17 ` [PATCH 1/2] packfile.c: protect against disappearing indexes Taylor Blau
2020-11-25 21:15 ` Junio C Hamano [this message]
2020-11-25 17:17 ` [PATCH 2/2] midx.c: protect against disappearing packs Taylor Blau
2020-11-25 21:22 ` Junio C Hamano
2020-11-25 21:13 ` [PATCH 0/2] midx: prevent against racily " Junio C Hamano
2020-11-26 0:48 ` Jeff King
2020-11-26 1:04 ` Taylor Blau
2020-11-26 1:27 ` 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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq7dq9dtpg.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=dstolee@microsoft.com \
--cc=git@vger.kernel.org \
--cc=me@ttaylorr.com \
--cc=peff@peff.net \
/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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).