midx: fix `BUG()` when getting preferred pack without a reverse index

The function `midx_preferred_pack()` returns the preferred pack for a
given multi-pack index. To compute the preferred pack we:

  1. Take the first position indexed by the MIDX in pseudo-pack order.

  2. Convert this pseudo-pack position into the MIDX position.

  3. We then look up the pack that corresponds to this MIDX position.

This reliably returns the preferred pack given that all of its contained
objects will be up front in pseudo-pack order.

The second step that turns the pseudo-pack order into MIDX order
requires the reverse index though, which may not exist for example when
the MIDX does not have a bitmap. And in that case one may easily hit a
bug:

    BUG: ../pack-revindex.c:491: pack_pos_to_midx: reverse index not yet loaded

In theory, `midx_preferred_pack()` already knows to handle the case
where no reverse index exists, as it calls `load_midx_revindex()` before
calling into `midx_preferred_pack()`. But we only check for negative
return values there, even though the function returns a positive error
code in case the reverse index does not exist.

Fix the issue by testing for a non-zero return value instead, same as
all the other callers of this function already do. While at it, document
the return value of `load_midx_revindex()`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/midx.c b/midx.c
index 1d6269f957e7819fb62963cde191360422b29229..79c890cf1b948eeec18532096de14f7113588c13 100644 (file)
--- a/midx.c
+++ b/midx.c
@@ -688,7 +688,7 @@ int midx_preferred_pack(struct multi_pack_index *m, uint32_t *pack_int_id)
 {
        if (m->preferred_pack_idx == -1) {
                uint32_t midx_pos;
-               if (load_midx_revindex(m) < 0) {
+               if (load_midx_revindex(m)) {
                        m->preferred_pack_idx = -2;
                        return -1;
                }

diff --git a/pack-revindex.h b/pack-revindex.h
index 422c2487ae32d8a4500f4a151ca673364fed96a0..004289209191d0990ed2353b83338a1df05ace0c 100644 (file)
--- a/pack-revindex.h
+++ b/pack-revindex.h
@@ -72,7 +72,8 @@ int verify_pack_revindex(struct packed_git *p);
  * multi-pack index by mmap-ing it and assigning pointers in the
  * multi_pack_index to point at it.
  *
- * A negative number is returned on error.
+ * A negative number is returned on error. A positive number is returned in
+ * case the multi-pack-index does not have a reverse index.
  */
 int load_midx_revindex(struct multi_pack_index *m);
 

diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 93f319a4b29fbb3d3899a1d1f3914dd7766dd672..9492a9737b5f8ec2e9319834f04775aa98ad20c7 100755 (executable)
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -350,7 +350,20 @@ test_expect_success 'preferred pack from existing MIDX without bitmaps' '
                # the new MIDX
                git multi-pack-index write --preferred-pack=pack-$pack.pack
        )
+'
 
+test_expect_success 'preferred pack cannot be determined without bitmap' '
+       test_when_finished "rm -fr preferred-can-be-queried" &&
+       git init preferred-can-be-queried &&
+       (
+               cd preferred-can-be-queried &&
+               test_commit initial &&
+               git repack -Adl --write-midx --no-write-bitmap-index &&
+               test_must_fail test-tool read-midx --preferred-pack .git/objects 2>err &&
+               test_grep "could not determine MIDX preferred pack" err &&
+               git repack -Adl --write-midx --write-bitmap-index &&
+               test-tool read-midx --preferred-pack .git/objects
+       )
 '
 
 test_expect_success 'verify multi-pack-index success' '