patch, 7 partsmidx: ignore cruft pack with `repack`, `expire`

This series fixes a pair of bugs that were originally pointed out by Gregory Szorc in [1].

Namely, that both `git multi-pack-index repack` and `git multi-pack-index expire` can cause us to "absorb" the cruft pack, distributing its objects into a new pack, and removing its metadata.

This is worth avoiding, since even though it doesn't result in object corruption, this bug removes semi-important metadata contained in the .mtimes file, which controls how fast objects leave the repository during a pruning GC.

This series teaches both sub-commands to avoid any cruft pack(s), preserving their metadata.

Thanks in advance for your review.

[1]: https://lore.kernel.org/git/CAKQoGanPBec6wRO6uWrETaoJXdszpjRWytXaJwx6jw0mrrj-gQ@mail.gmail.com/

Taylor Blau (7):
  Documentation/git-multi-pack-index.txt: fix typo
  Documentation/git-multi-pack-index.txt: clarify expire behavior
  midx.c: prevent `expire` from removing the cruft pack
  midx.c: avoid cruft packs with `repack --batch-size=0`
  midx.c: replace `xcalloc()` with `CALLOC_ARRAY()`
  midx.c: remove unnecessary loop condition
  midx.c: avoid cruft packs with non-zero `repack --batch-size`

 Documentation/git-multi-pack-index.txt |  7 +-
 midx.c                                 | 12 +++-
 t/t5319-multi-pack-index.sh            | 94 ++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 6 deletions(-)

Remove the extra space character between "tracked" and "by", which dates back to when this paragraph was originally written in cff9711616 (multi-pack-index: prepare for 'expire' subcommand, 2019-06-10).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-multi-pack-index.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The `expire` sub-command of `git multi-pack-index` will never expire `.keep` packs, regardless of whether or not any of their objects were selected in the MIDX.

This has always been the case since 19575c7c8e (multi-pack-index: implement 'expire' subcommand, 2019-06-10), which came after cff9711616 (multi-pack-index: prepare for 'expire' subcommand, 2019-06-10), when this documentation was originally written.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-multi-pack-index.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

The `expire` sub-command unlinks any packs that are (a) contained in the MIDX, but (b) have no objects referenced by the MIDX.

This sub-command ignores `.keep` packs, which remain on-disk even if they have no objects referenced by the MIDX. Cruft packs, however, aren't given the same treatment: if none of the objects contained in the cruft pack are selected from the cruft pack by the MIDX, then the cruft pack is eligible to be expired.

This is less than desireable, since the cruft pack has important metadata about the individual object mtimes, which is useful to determine how quickly an object should age out of the repository when pruning.

Ordinarily, we wouldn't expect the contents of a cruft pack to duplicated across non-cruft packs (and we'd expect to see the MIDX select all cruft objects from other sources even less often). But nonetheless, it is still possible to trick the `expire` sub-command into removing the `.mtimes` file in this circumstance.

Teach the `expire` sub-command to ignore cruft packs in the same manner as it does `.keep` packs, in order to keep their metadata around, even when they are unreferenced by the MIDX.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/git-multi-pack-index.txt |  4 ++--
 midx.c                                 |  2 +-
 t/t5319-multi-pack-index.sh            | 30 ++++++++++++++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

On 9/19/2022 9:55 PM, Taylor Blau wrote:

It is important to note that this can only happen if all objects in
the pack have duplicates in other pack-files.
 

s/desireable/desirable/ (according to my spell-checker)

I was initially unconvinced that this scenario was super-critical to keeping the .mtimes file, but I was able to think of cases where objects are duplicated out of the cruft pack due to de-thinning or otherwise inefficiently packing unreachable objects into these other pack-files.

Thanks, -Stolee

The `repack` sub-command of the `git multi-pack-index` builtin creates a new pack aggregating smaller packs contained in the MIDX up to some given `--batch-size`.

When `--batch-size=0`, this instructs the MIDX builtin to repack everything contained in the MIDX into a single pack.

In similar spirit as a previous commit, it is undesirable to repack the contents of a cruft pack in this step. Teach `repack` to ignore any cruft pack(s) when `--batch-size=0` for the same reason(s).

(The case of a non-zero `--batch-size` will be handled in a subsequent commit).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c                      |  2 ++
 t/t5319-multi-pack-index.sh | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+)

Replace a direct invocation of Git's `xcalloc()` wrapper with the `CALLOC_ARRAY()` macro instead.

The latter is preferred since it is more conventional in Git's codebase, but also because it automatically picks the correct value for the record size.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

The fill_included_packs_batch() routine is responsible for aggregating objects in packs with a non-zero value for the `--batch-size` option of the `git multi-pack-index repack` sub-command.

Since this routine is explicitly called only when `--batch-size` is non-zero, there is no point in checking that this is the case in our loop condition.

Remove the unnecessary part of this condition to avoid confusion.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Apply similar treatment with respect to cruft packs as in a few commits ago to `repack` with a non-zero `--batch-size`.

Since the case of a non-zero `--batch-size` is handled separately (in `fill_included_packs_batch()` instead of `fill_included_packs_all()`), a separate fix must be applied for this case.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c                      |  2 ++
 t/t5319-multi-pack-index.sh | 41 +++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

On Tue, Sep 20, 2022 at 4:59 AM Taylor Blau <me@ttaylorr.com> wrote:

Maybe:

s/to avoid any cruft pack(s)/to avoid removing any cruft pack(s)/

or:

s/to avoid any cruft pack(s)/to avoid absorbing any cruft pack(s)/

On 9/19/2022 9:55 PM, Taylor Blau wrote:

I gave these patches a careful review. They all look correct and are closing possible gaps in this mixed mode of repacking with different strategies.

I do think the 'expire' situation is very rare, but it's best to be safe here.

Overall, the end result is that users could set up their background maintenance as follows:

 1. maintenance.incremental-repack.schedule=daily to do an
    incremental repack every night.

 2. maintenance.gc.schedule=weekly to do a GC once a week.

 3. With gc.cruftPacks=true and a gc.pruneExpire value, the cruft
    packs will be written on that weekly job, only expiring old
    enough objects.

Before this series, that weekly GC creating cruft packs ran the (very likely) risk of those cruft packs being rewritten with the rest of the new object data, losing the .mtimes data.

(While the proposed schedule above is now possible, I don't recommend it as a default. Not only will it delete objects automatically, but the GC task is very expensive for very large repositories and the incremental-repack task was explicitly created to avoid that huge cost in those cases.)

Thanks for this update! -Stolee