Re: [PATCH v3 0/8] repack: support repacking into a geometric sequence

> On Wed, Feb 17, 2021 at 10:14:11PM -0500, Taylor Blau wrote:
>
> > Here is another updated version of mine and Peff's series to add a new 'git
> > repack --geometric' mode which supports repacking a repository into a geometric
> > progression of packs by object count.
>
> Thanks. This version looks pretty good to me. I have a few inline
> comments below. Mostly just observations, but there a couple tiny nits
> that I think may justify one more re-roll.

> > [snip documentation]
>
> We don't have a real procedure for marking something as "off limits" for
> users. IMHO omitting it from the documentation and putting an explicit
> note in the commit message is probably enough. It would be perhaps
> stronger to mark it explicitly as "do not touch" in the documentation,
> but then we are polluting the documentation. :)

> I also wondered if we could piggy-back on the sorting of packed_git,
> which is already in reverse chronological order. But here our primary
> structure is the string-list, so we lose that order.
>
> I'm not sure if your sort function is going the right way, though. It
> does:
>
> >     ++static int pack_mtime_cmp(const void *_a, const void *_b)
> >     ++{
> >     ++        struct packed_git *a = ((const struct string_list_item*)_a)->util;
> >     ++        struct packed_git *b = ((const struct string_list_item*)_b)->util;
> >     ++
> >     ++        if (a->mtime < b->mtime)
> >     ++                return -1;
> >     ++        else if (b->mtime < a->mtime)
> >     ++                return 1;
> >     ++        else
> >     ++                return 0;
> >     ++}
> >     ++
>
> Does that give us the packs in increasing chronological order, but then
> decreasing chronological order within the packs themselves?

> > 17:  b5081c01b5 !  5:  181c104a03 p5303: measure time to repack with keep
> >     @@ Metadata
> >       ## Commit message ##
> >          p5303: measure time to repack with keep
> >
> >     -    This is the same as the regular repack test, except that we mark the
> >     -    single base pack as "kept" and use --assume-kept-packs-closed. The
> >     -    theory is that this should be faster than the normal repack, because
> >     -    we'll have fewer objects to traverse and process.
> >     +    Add two new tests to measure repack performance. Both test split the
>
> s/test split/tests split/, I think.

> >     +    Likewise, the scaling is pretty extreme on --stdin-packs:
> >     +
> >     +      5303.7: repack with --stdin-packs (1)       0.01(0.01+0.00)
> >     +      5303.13: repack with --stdin-packs (50)     3.53(12.07+0.24)
> >     +      5303.19: repack with --stdin-packs (1000)   195.83(371.82+8.10)
> >
> >          That's because the code paths around handling .keep files are known to
> >          scale badly; they look in every single pack file to find each object.
>
> Your "that's because" is a little confusing to me. It certainly applies
> to the repack vs repack-with-kept comparisons for a given number of
> packs. But the scaling on the three --stdin-packs tests is high because
> each subsequent test is being asked to do a lot more work. But they're
> still cheaper than the matching "repack" case with a given number of
> packs. Just not _as_ cheap as they would be if the kept code weren't so
> slow.
>
> Would it make sense to reorder those two paragraphs?

> >     ++Loose objects are implicitly included in this "roll-up", without respect
> >     ++to their reachability. This is subject to change in the future. This
> >     ++option (implying a drastically different repack mode) is not guarenteed
> >     ++to work with all other combinations of option to `git repack`).
>
> Likewise, this is a big improvement. But should it make it clear that
> touching loose objects requires --unpacked? I.e., something like:
>
>   When `--unpacked` is specified, loose objects are included in this
>   "roll-up" without respect to their reachability...
>
> Also, s/guarenteed/guaranteed/.