@matheusbsbs / matheus-bagliano / commits / fc78915674

packfile: fix pack basename computation

When we have a multi-pack-index that covers many packfiles, we try to avoid opening the .idx for those packfiles. To do that we feed the pack name to midx_contains_pack(). But that function wants to see only the basename, which we compute using strrchr() to find the final slash. But that leaves an extra "/" at the start of our string. We can fix this by incrementing the pointer. That also raises the question of what to do when the name does not have a '/' at all. This should generally not happen (we always find files in "pack/"), but it doesn't hurt to be defensive here. Let's wrap all of that up in a helper function and make it publicly available, since a later patch will need to use it, too. The tests don't notice because there's nothing about opening those .idx files that would cause us to give incorrect output. It's just a little slower. The new test checks this case by corrupting the covered .idx, and then making sure we don't complain about it. We also have to tweak t5570, which intentionally corrupts a .idx file and expects us to notice it. When run with GIT_TEST_MULTI_PACK_INDEX, this will fail since we now will (correctly) not bother opening the .idx at all. We can fix that by unconditionally dropping any midx that's there, which ensures we'll have to read the .idx. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Apr 5, 2019 at 14:06 UTC fc78915674ff1bca1726348d7c434dfa0048a5d7

4 files changed +32 -1

packfile.c

+11 -1

@@ -466,6 +466,16 @@ static unsigned int get_max_fd_limit(void)

466 #endif

467 }

468

469 +const char *pack_basename(struct packed_git *p)

470 +{

471 + const char *ret = strrchr(p->pack_name, '/');

472 + if (ret)

473 + ret = ret + 1; /* skip past slash */

474 + else

475 + ret = p->pack_name; /* we only have a base */

476 + return ret;

477 +}

478 +

479 /*

480 * Do not call this directly as this leaks p->pack_fd on error return;

481 * call open_packed_git() instead.

@@ -482,7 +492,7 @@ static int open_packed_git_1(struct packed_git *p)

492

493 if (!p->index_data) {

494 struct multi_pack_index *m;

485 - const char *pack_name = strrchr(p->pack_name, '/');

495 + const char *pack_name = pack_basename(p);

496

497 for (m = the_repository->objects->multi_pack_index;

498 m; m = m->next) {

packfile.h

@@ -31,6 +31,12 @@ char *sha1_pack_name(const unsigned char *sha1);

31 */

32 char *sha1_pack_index_name(const unsigned char *sha1);

34 +/*

35 + * Return the basename of the packfile, omitting any containing directory

36 + * (e.g., "pack-1234abcd[...].pack").

37 + */

38 +const char *pack_basename(struct packed_git *p);

39 +

40 struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);

42 typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,

t/t5319-multi-pack-index.sh

+14

@@ -117,6 +117,20 @@ test_expect_success 'write midx with one v2 pack' '

117

118 compare_results_with_midx "one v2 pack"

119

120 +test_expect_success 'corrupt idx not opened' '

121 + idx=$(test-tool read-midx $objdir | grep "\.idx\$") &&

122 + mv $objdir/pack/$idx backup-$idx &&

123 + test_when_finished "mv backup-\$idx \$objdir/pack/\$idx" &&

124 +

125 + # This is the minimum size for a sha-1 based .idx; this lets

126 + # us pass perfunctory tests, but anything that actually opens and reads

127 + # the idx file will complain.

128 + test_copy_bytes 1064 <backup-$idx >$objdir/pack/$idx &&

129 +

130 + git -c core.multiPackIndex=true rev-list --objects --all 2>err &&

131 + test_must_be_empty err

132 +'

133 +

134 test_expect_success 'add more objects' '

135 for i in $(test_seq 6 10)

136 do

t/t5570-git-daemon.sh

@@ -90,6 +90,7 @@ test_expect_success 'fetch notices corrupt pack' '

90 test_expect_success 'fetch notices corrupt idx' '

91 cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&

92 (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&

93 + rm -f objects/pack/multi-pack-index &&

94 p=$(ls objects/pack/pack-*.idx) &&

95 chmod u+w $p &&

96 printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc