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

Message

René Scharfe Nov. 20, 2022, 10:03 a.m. UTC

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.

  t5317: stop losing return codes of git ls-files
  t5317: demonstrate failure to handle multiple --filter options
  Revert "pack-objects: lazily set up "struct rev_info", don't leak"

 builtin/pack-objects.c                 | 31 ++-------
 list-objects-filter-options.c          |  4 --
 list-objects-filter-options.h          | 24 +------
 t/t5317-pack-objects-filter-objects.sh | 90 +++++++++++++++++++-------
 4 files changed, 75 insertions(+), 74 deletions(-)

Range-Diff gegen v1:
1:  2df1185b5f < -:  ---------- pack-objects: fix handling of multiple --filter options
2:  b8e951fb4f < -:  ---------- pack-object: simplify --filter handling
3:  1e4ae7d9f1 < -:  ---------- list-objects-filter: remove OPT_PARSE_LIST_OBJECTS_FILTER_INIT()
-:  ---------- > 1:  43af432f5c t5317: stop losing return codes of git ls-files
-:  ---------- > 2:  a77b85e71d t5317: demonstrate failure to handle multiple --filter options
-:  ---------- > 3:  63ccb81357 Revert "pack-objects: lazily set up "struct rev_info", don't leak"
--
2.38.1

Comments

Jeff King Nov. 22, 2022, 7:02 p.m. UTC | #1

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

> 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