Re: [PATCH v2 0/3] pack-objects: fix and simplify --filter handling

From: Jeff King <peff@peff.net>
Date: Nov 22, 2022, 19:02 UTC
Message-ID: <Y30c4YYGb5bOaZae@coredump.intra.peff.net>
In-Reply-To: <d19c6cb4-611f-afea-8a14-5e58d7509113@web.de>

On Sun, Nov 20, 2022 at 11:03:35AM +0100, René Scharfe wrote:

Show 16 quoted lines

> Fix a regression that prevents using multiple --filter options, simplify
> the option parsing code and avoid relying on undefined behavior in it.
> 
> Patch 3 conflicts with cc/filtered-repack in seen, but not semantically.
> 
> Changes since v1:
> - Added patch 1 to fix an issue with existing tests.
> - Separate patch 2 for new tests.
> - Test using blob size filters, only, which is a bit simpler.
> - Test both combinations to also catch not just the current
>   last-one-wins regression, but also a possible future first-one-wins
>   issue.
> - Actually revert 5cb28270a1 (pack-objects: lazily set up
>   "struct rev_info", don't leak, 2022-03-28) instead of having a
>   minimal fix and then adding some kind of middle ground by using a
>   separate struct list_objects_filter_options.

Thanks, this looks good to me. The new test is a little funny in that it's somewhat-nonsensical input, but I find it unlikely that we'd ever add code to treat two instances of the same filter type specially. Compared to something that produces two distinct effects (like a blob and tree-depth filter combined), it also requires the "test in both directions" trick to cover everything.

But I do like that it's easier to reason about what the output _should_ be. So I don't feel too strongly about it either way.

-Peff