Re: [PATCH 08/11] t5332: enable OFS_DELTAs via test_pack_objects_reused

From: Jeff King <peff@peff.net>
Date: Oct 11, 2024, 08:19 UTC
Message-ID: <20241011081939.GF18010@coredump.intra.peff.net>
In-Reply-To: <9d81d890402f94f4126aedef0845d615d10455bc.1728505840.git.me@ttaylorr.com>

On Wed, Oct 09, 2024 at 04:31:24PM -0400, Taylor Blau wrote:

Show 12 quoted lines

> Back when test_pack_objects_reused was introduced via commit
> 7c01878eeb (t5332-multi-pack-reuse.sh: extract pack-objects helper
> functions, 2024-02-05), we converted bare pack-objects invocations
> into one of two wrapped variants, either test_pack_objects_reused or
> test_pack_objects_reused_all.
> 
> The latter passes `--delta-base-offset`, allowing pack-objects to
> generate OFS_DELTAs in its output pack. But the former does not, for
> no good reason.
> 
> As we do not want to convert OFS_DELTAs to REF_DELTAs unnecessarily,
> let's unify these two and pass `--delta-base-offset` to both.

I think that matches what happens in the real world. I am puzzled that your BUG() instrumenting turns up some conversion cases. Why are we still converting in those cases?

> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 0fc0680b402..0f1b22b8674 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c

You need to indent or otherwise comment-out this diff. Otherwise "git am" will pick it up as the start of the actual diff, adding the bogus BUG() call to the applied patch (and dropping the rest of your commit message).

-Peff