[PATCH v2 0/13] a rabbit hole of update-server-info (and midx!) fixes
- From
Jeff King <peff@peff.net>
- Date
- Apr 5, 2019, 18:03 UTC
- Message-ID
- <20190405180306.GA21113@sigill.intra.peff.net>
- In-Reply-To
- <20190404232104.GA27770@sigill.intra.peff.net>
On Thu, Apr 04, 2019 at 07:21:04PM -0400, Jeff King wrote:
Show 7 quoted lines
> This patch series started with patch 12: I just wanted to drop the > unused "force" parameter from update_info_refs(). > [...] > And here we are. I present them here in reverse rabbit-hole order (which > is also roughly important fixes first, and minor cleanups last). The > individual chunks are mostly independent, but the server-info cleanups > rely on the shared pack_basename() helper added as part of the midx fix.
Here's a v2 which addresses the feedback on the last round (thank you both). In particular:
- the rabbit hole goes one deeper, as fixing the midx bug reveals that
the revindex code just assumes that the pack .idx has been loaded.
Patch 2 corrects this, which addresses most of the test failures
with GIT_TEST_MULTI_PACK_INDEX. - There was one other failure, which is that t5570 corrupted a .idx
and expected us to notice (which no longer happens when the midx is
able to provide the answer). I've tweaked the test to address this. - I took René's suggestion of making midx_contains_pack() friendlier
by handling either a .pack or a .idx argument. I had to tweak the
suggested code just a little, and I added comments, since it took me
a little staring to make sure everything was correct. I split that out from the basename fix, so that's now patches 5 and
6. It also lets us drop the patch to factor out pack_name_to_idx().Other than that, it's substantially the same as v1. I did move the public declaration of pack_basename() to the patch where it is added, rather than doing it later, which shows up in the range diff below.
[01/13]: packfile.h: drop extern from function declarations [02/13]: pack-revindex: open index if necessary [03/13]: t5319: fix bogus cat-file argument [04/13]: t5319: drop useless --buffer from cat-file [05/13]: midx: check both pack and index names for containment [06/13]: packfile: fix pack basename computation [07/13]: http: simplify parsing of remote objects/info/packs [08/13]: server-info: fix blind pointer arithmetic [09/13]: server-info: simplify cleanup in parse_pack_def() [10/13]: server-info: use strbuf to read old info/packs file [11/13]: server-info: drop nr_alloc struct member [12/13]: server-info: drop objdirlen pointer arithmetic [13/13]: update_info_refs(): drop unused force parameter
http.c | 35 ++++++--------- midx.c | 36 +++++++++++++++- midx.h | 2 +- pack-bitmap.c | 3 +- pack-revindex.c | 13 ++++-- pack-revindex.h | 2 +- packfile.c | 18 ++++++-- packfile.h | 86 ++++++++++++++++++++----------------- server-info.c | 57 +++++++++++------------- t/t5319-multi-pack-index.sh | 29 ++++++++++--- t/t5570-git-daemon.sh | 1 + 11 files changed, 172 insertions(+), 110 deletions(-)
10: bead52a5dc = 1: 829e666ec9 packfile.h: drop extern from function declarations
-: ---------- > 2: d46c0eafd0 pack-revindex: open index if necessary
1: 3d1978ccaf = 3: 5b965de360 t5319: fix bogus cat-file argument
2: ce74312004 = 4: 32ef03e059 t5319: drop useless --buffer from cat-file
3: ea637c9cf1 < -: ---------- packfile: factor out .pack to .idx name conversion
-: ---------- > 5: d87a2aa2bf midx: check both pack and index names for containment
4: 4723e1b952 ! 6: c2bed91a3f packfile: check midx coverage with .idx rather than .pack
@@ -1,35 +1,40 @@
Author: Jeff King <peff@peff.net>
- packfile: check midx coverage with .idx rather than .pack
+ packfile: fix pack basename computation
- When we have a .midx that covers many packfiles, we try to avoid opening
- the .idx for those packfiles. However, there are a few problems with the
- filename comparison we use:
+ 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 ask midx_contains_pack() about the .pack name, not the .idx name.
- But it compares to the latter.
+ 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.
- - we compute the basename of the pack using strrchr() to find the
- final slash. But that leaves an extra "/" at the start of our
- string; we need to advance past it.
-
- That also raises the question of what to do when the name does not
- have a slash 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.
+
diff --git a/packfile.c b/packfile.c
--- a/packfile.c
+++ b/packfile.c
@@
#endif
}
-+static const char *pack_basename(struct packed_git *p)
++const char *pack_basename(struct packed_git *p)
+{
+ const char *ret = strrchr(p->pack_name, '/');
+ if (ret)
@@ -43,25 +48,31 @@
* Do not call this directly as this leaks p->pack_fd on error return;
* call open_packed_git() instead.
@@
- ssize_t read_result;
- const unsigned hashsz = the_hash_algo->rawsz;
-- if (!p->index_data) {
-+ if (!p->index_data && the_repository->objects->multi_pack_index) {
+ if (!p->index_data) {
struct multi_pack_index *m;
- const char *pack_name = strrchr(p->pack_name, '/');
-+ char *idx_name = pack_name_to_idx(pack_basename(p));
++ 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))
-+ if (midx_contains_pack(m, idx_name))
- break;
- }
-+ free(idx_name);
+
+ diff --git a/packfile.h b/packfile.h
+ --- a/packfile.h
+ +++ b/packfile.h
+@@
+ */
+ char *sha1_pack_index_name(const unsigned char *sha1);
- if (!m && open_pack_index(p))
- return error("packfile %s index unavailable", p->pack_name);
++/*
++ * Return the basename of the packfile, omitting any containing directory
++ * (e.g., "pack-1234abcd[...].pack").
++ */
++const char *pack_basename(struct packed_git *p);
++
+ struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
+
+ typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
--- a/t/t5319-multi-pack-index.sh
@@ -87,3 +98,15 @@
test_expect_success 'add more objects' '
for i in $(test_seq 6 10)
do
+
+ diff --git a/t/t5570-git-daemon.sh b/t/t5570-git-daemon.sh
+ --- a/t/t5570-git-daemon.sh
+ +++ b/t/t5570-git-daemon.sh
+@@
+ test_expect_success 'fetch notices corrupt idx' '
+ cp -R "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_pack.git "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
+ (cd "$GIT_DAEMON_DOCUMENT_ROOT_PATH"/repo_bad2.git &&
++ rm -f objects/pack/multi-pack-index &&
+ p=$(ls objects/pack/pack-*.idx) &&
+ chmod u+w $p &&
+ printf %0256d 0 | dd of=$p bs=256 count=1 seek=1 conv=notrunc
5: 0bf3fb7758 = 7: 15a7d0c690 http: simplify parsing of remote objects/info/packs
6: 3f711f74f1 = 8: aee572065c server-info: fix blind pointer arithmetic
7: 5a77637699 = 9: a9d226960a server-info: simplify cleanup in parse_pack_def()
8: cbc7b6fa71 = 10: 388e7bf2be server-info: use strbuf to read old info/packs file
9: 9601aa240c = 11: 76ca72f4bf server-info: drop nr_alloc struct member
11: 23ff6ba3dc ! 12: 8753c537a7 server-info: drop objdirlen pointer arithmetic
@@ -18,36 +18,6 @@
future-proofs us, and should hopefully be more obviously safe to
somebody reading the code.
- diff --git a/packfile.c b/packfile.c
- --- a/packfile.c
- +++ b/packfile.c
-@@
- #endif
- }
-
--static const char *pack_basename(struct packed_git *p)
-+const char *pack_basename(struct packed_git *p)
- {
- const char *ret = strrchr(p->pack_name, '/');
- if (ret)
-
- diff --git a/packfile.h b/packfile.h
- --- a/packfile.h
- +++ b/packfile.h
-@@
- */
- char *sha1_pack_index_name(const unsigned char *sha1);
-
-+/*
-+ * Return the basename of the packfile, omitting any containing directory
-+ * (e.g., "pack-1234abcd[...].pack").
-+ */
-+const char *pack_basename(struct packed_git *p);
-+
- struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
-
- typedef void each_file_in_pack_dir_fn(const char *full_path, size_t full_path_len,
-
diff --git a/server-info.c b/server-info.c
--- a/server-info.c
+++ b/server-info.c
12: 3f9cff4360 = 13: 96c5a26a99 update_info_refs(): drop unused force parameter