v2, 3 partsmidx: use `--stdin-packs` to implement `repack`

Here's a few patches that replace the existing "feed each OID one-by-one" approach to implement the `repack` sub-command of the `multi-pack-index` builtin with one that uses `pack-objects`'s `--stdin-packs` option.

There is an additional patch at the beginning of this series in order to extract the mtime-sorted list of packs to rollup from their home in `fill_included_packs_batch()`. There's also one more on the end to unify the `include_pack` array into the `repack_info` struct(s) themselves.

The second patch is the substantive one. Thanks for all of the review so far! :-)

Taylor Blau (3):
  midx.c: compute pack_info array outside of fill_included_packs_batch()
  midx.c: use `pack-objects --stdin-packs` when repacking
  midx.c: unify `include_pack` array into `pack_info` struct

 midx.c | 76 +++++++++++++++++++++++++++++-----------------------------
 1 file changed, 38 insertions(+), 38 deletions(-)

Range-diff against v1:
-:  ---------- > 1:  a501776f6e midx.c: compute pack_info array outside of fill_included_packs_batch()
1:  7e987eb9d7 ! 2:  4218d9e08a midx.c: use `pack-objects --stdin-packs` when repacking
    @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat
      	struct strbuf base_name = STRBUF_INIT;
     +	struct strbuf scratch = STRBUF_INIT;
      	struct multi_pack_index *m = lookup_multi_pack_index(r, object_dir);
    + 	struct repack_info *pack_info = NULL;
      
    - 	/*
     @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t batch_size,
      	repo_config_get_bool(r, "repack.usedeltabaseoffset", &delta_base_offset);
      	repo_config_get_bool(r, "repack.usedeltaislands", &use_delta_islands);
    @@ midx.c: int midx_repack(struct repository *r, const char *object_dir, size_t bat
     -		struct object_id oid;
     -		uint32_t pack_int_id = nth_midxed_pack_int_id(m, i);
     +	for (i = 0; i < m->num_packs; i++) {
    -+		strbuf_reset(&scratch);
    ++		struct repack_info *info = &pack_info[i];
      
     -		if (!include_pack[pack_int_id])
     -			continue;
    -+		strbuf_addstr(&scratch, m->pack_names[i]);
    -+		strbuf_strip_suffix(&scratch, ".idx");
    -+		strbuf_addstr(&scratch, ".pack");
    ++		strbuf_reset(&scratch);
      
     -		nth_midxed_object_oid(&oid, m, i);
     -		fprintf(cmd_in, "%s\n", oid_to_hex(&oid));
    -+		fprintf(cmd_in, "%s%s\n", include_pack[i] ? "" : "^", scratch.buf);
    ++		strbuf_addstr(&scratch, m->pack_names[info->pack_int_id]);
    ++		strbuf_strip_suffix(&scratch, ".idx");
    ++		strbuf_addstr(&scratch, ".pack");
    ++
    ++		fprintf(cmd_in, "%s%s\n", include_pack[info->pack_int_id] ? "" : "^", scratch.buf);
      	}
      	fclose(cmd_in);
     +	strbuf_release(&scratch);
-:  ---------- > 3:  81e9ccc323 midx.c: unify `include_pack` array into `pack_info` struct

The fill_included_packs_batch() function needs to compute a list of packs which are included in the MIDX, and then sort that list according to the pack mtimes.

Since packs are listed in the `m->pack_names` array in SHA-1 order, we have to reconstruct their mtimes by reading the `pack_mtime` member of the struct `packed_git` in order to determine the value by which to sort.

This list is only needed by the fill_included_packs_batch(), but the subsequent commit will want to have this list accessible in this function's caller, `midx_repack()`.

Prepare for that by teaching `midx_repack()` to compute, pass in, and then free that list so it is available throughout the body of `midx_repack()`.

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

The `git multi-pack-index repack` command aggregates all objects in certain packs contained in the MIDX, largely dictated by the value of its `--batch-size` parameter.

To create the new pack, the MIDX machinery spawns a `pack-objects` sub-process, which it feeds the object IDs in each of the pack(s) that it wants to aggregate.

This implementation, which dates back to ce1e4a105b (midx: implement midx_repack(), 2019-06-10), predates the `--stdin-packs` mode in `pack-objects`. This mode (which was introduced a couple of years later in 339bce27f4 (builtin/pack-objects.c: add '--stdin-packs' option, 2021-02-22)) allows `pack-objects` callers to specify the contents of the output pack by indicating which packs to aggregate.

This patch replaces the pre-`--stdin-packs` invocation (where each object is given to `pack-objects` one by one) with the more modern `--stdin-packs` option.

This allows us to avoid some CPU cycles serializing and deserializing every object ID in all of the packs we're aggregating. It also avoids us having to send a potentially large amount of data down to `pack-objects`.

But more importantly, it generates slightly higher quality (read: more tightly compressed) packs, because of the reachability traversal that `--stdin-packs` does after the fact in order to gather namehash values which seed the delta selection process.

In practice, this seems to add a slight amount of overhead (on the order of a few seconds for git.git broken up into ~100 packs), in exchange for a modest reduction (on the order of ~3.5%) in the resulting pack size.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

On 9/20/2022 4:40 PM, Taylor Blau wrote:

...

Outside of how the object set is provided (a list of objects or a list of packs) it seems this is equivalent. The only difference is that we are writing a line for _every_ pack in the multi-pack-index, but we preface the line with "^" to say "not this pack".

Do you know if there is any reason to do this explicitly? Does this change the set of objects in any way (perhaps by not including duplicates that are tracked in those other packs)?

Personally, I would have kept the "if (...) continue;" pattern, so maybe you had a concrete idea why this approach is better.

Thanks, -Stolee

On Fri, Sep 23, 2022 at 02:23:30PM -0400, Derrick Stolee wrote:

Yes. The "^" lines become excluded packs from the perspective of the follow-on reachability traversal to discover the namehashes. So as soon as we hit an object contained in one of the packs marked as excluded, we'll halt the traversal along that direction, since we know that we're not going to pick up any objects in those packs.

So you could omit them, and you'd get the same resulting pack, but it would take longer to generate since we wouldn't be stopping the follow-on traversal as early as possible.

Thanks, Taylor

On 9/23/2022 2:28 PM, Taylor Blau wrote:

Excellent. Thanks for explaining that!

-Stolee

The MIDX repack code uses a separate byte-array to track which packs are "included" in the rollup. Now that an array of `repack_info` structs is available for the lifetime of the `midx_repack()` function, we can instead use a new bit in that struct's record instead of maintaining a separate array.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 midx.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

On Tue, Sep 20, 2022 at 04:40:14PM -0400, Taylor Blau wrote:

The first and third make sense to me, and it looks like a nice cleanup.

The middle one looks fine, modulo all of our earlier discussion about the extra traversal being fine. I think it would be good t get a review from Stolee as the person who invented "git multi-index-pack repack" and would understand its intended use. Presumably this is all weird "I have a giant Windows repository" packing strategy. :)

-Peff

Taylor Blau <me@ttaylorr.com> writes:

One question. How is this series expected to interact with the 7-patch series about ignoring cruft pack while "midx repack" etc.?

I guess it is not so urgent during the -rc period, so I'll stop at asking that question without reading further, at least for now.

Thanks.

On Wed, Sep 21, 2022 at 12:09:31PM -0700, Junio C Hamano wrote:

The first round merges cleanly, but the second round will have a slight conflict. I'll send you a new version of this series based on whatever is in `master` following the release.

Thanks, Taylor