Re: [PATCH 0/3] repack: handle --keep-pack, --max-pack-size for geometric repacks

From: Victoria Dye <vdye@github.com>
Date: May 20, 2022, 19:46 UTC
Message-ID: <5517f0f8-871c-a155-7d8c-d0d6086bb7d2@github.com>
In-Reply-To: <cover.1653073280.git.me@ttaylorr.com>

Taylor Blau wrote:

Show 12 quoted lines

> This series fixes two issues that Victoria and I noticed while working on an
> unrelated issue yesterday.
> 
>   - The first patch comes from Victoria's earlier submission[1], and addresses
>     an issue where packs specified as kept via the `--keep-pack` option could
>     potentially be removed (without rewriting their objects) during a
>     `--geometric` repack.
> 
>     The first patch is Victoria's alone, with some minor fixups applied from my
>     review in [2]. It's included in this series since it's related, and avoids
>     any conflicts when merging.
>

I'm happy with the fixes you applied here and don't have anything else I'd like to add this patch.

Show 9 quoted lines

>   - The latter two patches are mine, and address an issue where specifying a
>     `--max-pack-size` value during a `--geometric` repack could result in object
>     loss because of a false positive in our "did we write a pack with this
>     name?" check (which can occur when the list of packs we wrote isn't sorted).
> 
>     The first of these two patches demonstrates the issue (done in a separate
>     patch, since the scenario is quite involved), and the second patch fixes the
>     bug.
>

I was worried about the robustness of the test, but some deeper diving revealed that it should produce consistent results. Otherwise, the fix itself is a straightforward (albeit hard to find in the first place). These two patches look good to me!

Show 16 quoted lines

> Thanks in advance for your review.
> 
> [1]: https://lore.kernel.org/git/pull.1235.git.1653064572170.gitgitgadget@gmail.com/
> [2]: https://lore.kernel.org/git/YofJLv8+x5J7yPmf@nand.local/
> 
> Taylor Blau (2):
>   t7703: demonstrate object corruption with pack.packSizeLimit
>   builtin/repack.c: ensure that `names` is sorted
> 
> Victoria Dye (1):
>   repack: respect --keep-pack with geometric repack
> 
>  builtin/repack.c            | 49 ++++++++++++++++++------
>  t/t7703-repack-geometric.sh | 75 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 112 insertions(+), 12 deletions(-)
>