Re: [PATCH] index-pack: downgrade twice-resolved REF_DELTA to die()

[Taylor Blau <me@ttaylorr.com>]

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@peff.net>
> ---
> 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@ttaylorr.com>

Thanks,
Taylor