| Message ID | cover.1653073280.git.me@ttaylorr.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | repack: handle --keep-pack, --max-pack-size for geometric repacks | expand |
Taylor Blau wrote: > 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. > - 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! > 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(-) >
On 5/20/2022 3:46 PM, Victoria Dye wrote: > Taylor Blau wrote: >> 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. > >> - 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! > >> Thanks in advance for your review. I'm chiming in to say that I also read these patches and think they are good. Couldn't find a way to improve them. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> 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! >> >>> Thanks in advance for your review. > > I'm chiming in to say that I also read these patches and think they > are good. Couldn't find a way to improve them. Thanks, all. Queued.
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. - 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. 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(-)