On Mon, Feb 03, 2020 at 09:40:55AM -0500, Jeff King wrote:
> When we're resolving a REF_DELTA, we compare-and-swap its type from
> REF_DELTA to whatever real type the base object has, as discussed in
> ab791dd138 (index-pack: fix race condition with duplicate bases,
> 2014-08-29). If the old type wasn't a REF_DELTA, we consider that a
> BUG(). But as discussed in that commit, we might see this case whenever
> we try to resolve an object twice, which may happen because we have
> multiple copies of the base object.
>
> So this isn't a bug at all, but rather a sign that the input pack is
> broken. And indeed, this case is triggered already in t5309.5 and
> t5309.6, which create packs with delta cycles and duplicate bases. But
> we never noticed because those tests are marked expect_failure.
>
> Those tests were added by b2ef3d9ebb (test index-pack on packs with
> recoverable delta cycles, 2013-08-23), which was leaving the door open
> for cases that we theoretically _could_ handle. And when we see an
> already-resolved object like this, in theory we could keep going after
> confirming that the previously resolved child->real_type matches
> base->obj->real_type. But:
>
> - enforcing the "only resolve once" rule here saves us from an
> infinite loop in other parts of the code. If we keep going, then the
> delta cycle in t5309.5 causes us to loop infinitely, as
> find_ref_delta_children() doesn't realize which objects have already
> been resolved. So there would be more changes needed to make this
> case work, and in the meantime we'd be worse off.
>
> - any pack that triggers this is broken anyway. It either has a
> duplicate base object, or it has a cycle which causes us to bring in
> a duplicate via --fix-thin. In either case, we'd end up rejecting
> the pack in write_idx_file(), which also detects duplicates.
>
> So the tests have little value in documenting what we _could_ be doing
> (and have been neglected for 6+ years). Let's switch them to confirming
> that we handle this case cleanly (and switch out the BUG() for a more
> informative die() so that we do so).
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> Noticed while running the tests in an environment that complains about
> SIGABRT deaths. Arguably the test suite should be looking for these even
> in test_expect_failure, but it would be a little convoluted to do so
> (e.g., teaching BUG() to write to a marker file, and then checking the
> file). And I think we're better off phrasing things as much as possible
> as expect_success anyway.
>
> builtin/index-pack.c | 4 +++-
> t/t5309-pack-delta-cycles.sh | 8 ++++----
> 2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 60a5591039..41a7c11c8e 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1003,7 +1003,9 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,
>
> if (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,
> base->obj->real_type))
> - BUG("child->real_type != OBJ_REF_DELTA");
> + die("REF_DELTA at offset %"PRIuMAX" already resolved (duplicate base %s?)",
> + (uintmax_t)child->idx.offset,
> + oid_to_hex(&base->obj->idx.oid));
This error message is informative, and matches what you described in the
patch message.
> resolve_delta(child, base, result);
> if (base->ref_first == base->ref_last && base->ofs_last == -1)
> diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
> index 491556dad9..6c209ad45c 100755
> --- a/t/t5309-pack-delta-cycles.sh
> +++ b/t/t5309-pack-delta-cycles.sh
> @@ -62,13 +62,13 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
> test_must_fail git index-pack --fix-thin --stdin <cycle.pack
> '
>
> -test_expect_failure 'failover to an object in another pack' '
> +test_expect_success 'failover to an object in another pack' '
> clear_packs &&
> git index-pack --stdin <ab.pack &&
> - git index-pack --stdin --fix-thin <cycle.pack
> + test_must_fail git index-pack --stdin --fix-thin <cycle.pack
> '
>
> -test_expect_failure 'failover to a duplicate object in the same pack' '
> +test_expect_success 'failover to a duplicate object in the same pack' '
> clear_packs &&
> {
> pack_header 3 &&
> @@ -77,7 +77,7 @@ test_expect_failure 'failover to a duplicate object in the same pack' '
> pack_obj $A
> } >recoverable.pack &&
> pack_trailer recoverable.pack &&
> - git index-pack --fix-thin --stdin <recoverable.pack
> + test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
> '
And these hunks all make sense, too.
> test_done
> --
> 2.25.0.541.gdfd61ebb85
This looks great, thanks :-)
Reviewed-by: Taylor Blau <me@xxxxxxxxxxxx>
Thanks,
Taylor
