patch, 10 partsgc: enable cruft packs by default

This series graduates `gc.cruftPacks` out of `feature.experimental` and enables it by default, meaning that a bare `git gc` will generate a cruft pack.

The main benefits are described in detail in the second to last patch, but a brief summary is that:

  - cruft packs avoid an issue where repositories with many unreachable
    objects that were written too recently to be pruned causes an
    explosion of loose object files

  - cruft packs allow pairs of unreachable objects with similar content
    to delta with each other since they can be represented in a single
    pack instead of as individual loose objects.

Cruft packs have been in the tree for the vast majority of the past calendar year, and have been in use at GitHub for even longer without issue.

The series is structured as follows:

  - The first two patches are cleanup.
  - The third patches fixes an oversight where the code for `git gc`'s
    `--keep-largest-pack` option would incorrectly consider cruft packs.
  - The next five patches are test preparation.
  - Then the substantive patch, to actually graduate `gc.cruftPacks` and
    enable it by default.
  - The final patch is some cleanup that can only take place towards the
    end of the series.

Cruft packs have been a success for us at GitHub, and I am really excited to get it in the hands of more users by default.

Thanks in advance for your review.

-Taylor

Taylor Blau (10):
  pack-write.c: plug a leak in stage_tmp_packfiles()
  builtin/repack.c: fix incorrect reference to '-C'
  builtin/gc.c: ignore cruft packs with `--keep-largest-pack`
  t/t5304-prune.sh: prepare for `gc --cruft` by default
  t/t9300-fast-import.sh: prepare for `gc --cruft` by default
  t/t6500-gc.sh: refactor cruft pack tests
  t/t6500-gc.sh: add additional test cases
  t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
  builtin/gc.c: make `gc.cruftPacks` enabled by default
  repository.h: drop unused `gc_cruft_packs`

 Documentation/config/feature.txt |   3 -
 Documentation/config/gc.txt      |  12 +--
 Documentation/git-gc.txt         |  12 +--
 Documentation/gitformat-pack.txt |   4 +-
 builtin/gc.c                     |   8 +-
 builtin/repack.c                 |   2 +-
 pack-write.c                     |  14 ++--
 repo-settings.c                  |   4 +-
 repository.h                     |   1 -
 t/t5304-prune.sh                 |  28 +++----
 t/t6500-gc.sh                    | 136 ++++++++++++++++---------------
 t/t6501-freshen-objects.sh       |  10 +--
 t/t9300-fast-import.sh           |  13 +--
 13 files changed, 121 insertions(+), 126 deletions(-)

The function `stage_tmp_packfiles()` generates a filename to use for staging the contents of what will become the pack's ".mtimes" file.

The name is generated in `write_mtimes_file()` and the result is returned back to `stage_tmp_packfiles()` which uses it to rename the temporary file into place via `rename_tmp_packfiles()`.

`write_mtimes_file()` returns a `const char *`, indicating that callers are not expected to free its result (similar to, e.g., `oid_to_hex()`). But callers are expected to free its result, so this return type is incorrect.

Change the function's signature to return a non-const `char *`, and free it at the end of `stage_tmp_packfiles()`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-write.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

On Mon, Apr 17, 2023 at 04:54:15PM -0400, Taylor Blau wrote:

Makes sense. I do think in the long run that it might make sense for all of these pack-write tmpfiles to tempfile.[ch] (either directly, or via register_tempfile() after creating the file). And then it would be safe to pass around the tempfile struct itself, which contains the filename, without worrying so much about ownership issues.

But that's a much bigger change that shouldn't be part of your series.

-Peff

On Tue, Apr 18, 2023 at 06:30:05AM -0400, Jeff King wrote:

Agreed on both fronts. I'll put investigating something like this on my list of things to-do.

Thanks, Taylor

When cruft packs were originally being developed, `-C` was designated as the short-form for `--cruft` (as in `git repack -C`).

This was dropped due to confusion with Git's top-level `-C` option before submitting to the list. But the reference to it in `--cruft-expiration`'s help text was never updated. Fix that dangling reference in this patch.

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

When cruft packs were implemented, we never adjusted the code for `git gc`'s `--keep-largest-pack` and `gc.bigPackThreshold` to ignore cruft packs. This option and configuration option share a common implementation, but including cruft packs is wrong in both cases:

  - Running `git gc --keep-largest-pack` in a repository where the
    largest pack is the cruft pack itself will make it impossible for
    `git gc` to prune objects, since the cruft pack itself is kept.

  - The same is true for `gc.bigPackThreshold`, if the size of the cruft
    pack exceeds the limit set by the caller.

Ignore cruft packs in the common implementation for both of these options, and add a pair of tests to prevent any future regressions here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/gc.txt | 10 ++++-----
 Documentation/git-gc.txt    |  7 +++---
 builtin/gc.c                |  2 +-
 t/t6500-gc.sh               | 43 +++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 9 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Makes sense. We want to keep the largest pack that is actually in use, and we want to consolidate other non-cruft packs into one.

This is not as cut-and-dried clear as the previous one. "This pack is so large that it is not worth rewriting it only to expunge a handful of objects that are no longer reachable from it" is the main motivation to use this configuration, but doesn't some part of the same reasoning apply equally to a large cruft pack? But let's assume that the configuration is totally irrelevant to cruft packs and read on.

"except the largest pack" -> "except the largest, non-cruft pack"

On Mon, Apr 17, 2023 at 03:54:35PM -0700, Junio C Hamano wrote:

This is an inherent design trade-off. I imagine that callers who want to avoid rewriting their (large) cruft packs would prefer to generate a new cruft pack on top with just the recently accumulated unreachable objects.

That kind of works, except if you need to prune objects that are packed in an earlier cruft pack. If you have `gc.bigPackThreshold`, there is no way to do this if you need to expire objects that are in cruft packs above that threshold.

A user may find themselves frustrated when trying to `git gc --prune` some sensitive object(s) from their repository doesn't appear to work, only to discover that `gc.bigPackThreshold` is set somewhere in their configuration.

Writing (largely) the same cruft pack to expunge a few objects isn't ideal, but it is better than the status quo. And if you have so many unreachable objects that this is a concern, it is probably time to prune anyway.

It is possible that in the future we could support writing multiple cruft packs (we already handle the presence of multiple cruft packs fine, just don't expose an easy way for the user to write >1 of them). And at that point we would be able to relax this patch a bit and allow `gc.bigPackThreshold` to cover cruft packs, too. But in the meantime, the benefit of avoiding loose object explosions outweighs the possible drawbacks here, IMHO.

Indeed, good eyes.

Thanks, Taylor

On Mon, Apr 17, 2023 at 07:03:08PM -0400, Taylor Blau wrote:

Yeah, what your patch does makes sense to me as a default behavior. In a pre-cruft-pack world, those objects would all be left alone by gc.bigPackThreshol (because they're loose), and the essence of cruft-packs is creating a parallel universe where those ejected-to-loose objects just happen to be stored in a more efficient format.

I wondered if that interface might be an option to say "hey, I have a gigantic cruft file I want to carry forward, please leave it alone".

But if you have a giant cruft pack that is making your "git gc" too slow, it will eventually age out on its own. And if you're impatient, then "git gc --prune=now" is probably the right tool.

And If you really did want to keep rolling it forward for some reason, then I'd think marking it with ".keep" would be the best thing (and maybe even dropping the mtimes file? I'm not sure a how a kept-cruft pack does or should behave).

-Peff

On 4/18/2023 6:39 AM, Jeff King wrote:

I agree with the prior discussion that gc.bigPackThreshold is currently misbehaving and stopping it from caring about cruft packs is the best way to fix that behavior in this series.

Generally, it's probably a good idea to (later) create a separate knob for "don't rewrite the objects in a 'big' cruft pack unless you need to". For situations where cruft objects are being collected and not regularly pruned, this helps avoid repacking all unreachable objects into a giant single pack, even though only a small number of objects were discovered unreachable this time.

The important times where we'd want to consider a 'big' cruft pack for rewrite are:

 1. Some objects in the cruft pack are being pruned.
 2. Some objects in the cruft pack need updated mtimes.

However, in the typical case that we are adding new cruft objects and not changing the mtimes of existing unreachable objects, we could create a sensible limit on the size of a cruft pack to be rewritten during normal maintenance.

My personal preference would be something between 2GB and 10GB, which seems like a decent balance between "size of cruft pack" and "number of cruft packs" for most repositories. Since none of the objects are reachable, we don't really care about them having good deltas for things like fetches and clones. The benefit of reducing the time spent in 'git repack --cruft' outweighs the slight disk space savings by having a single cruft pack, in my opinion.

Thanks, -Stolee

Many of the tests in t5304 run `git gc`, and rely on its behavior that unreachable-but-recent objects are written out loose. This is sensible, since t5304 deals specifically with this kind of pruning.

If left unattended, however, this test would break when the default behavior of a bare "git gc" is adjusted to generate a cruft pack by default.

Ensure that these tests continue to work as-is (and continue to provide coverage of loose object pruning) by passing `--no-cruft` explicitly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5304-prune.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

In a similar fashion as the previous commit, adjust the fast-import tests to prepare for "git gc" generating a cruft pack by default.

This adjustment is slightly different, however. Instead of relying on us writing out the objects loose, and then calling `git prune` to remove them, t9300 needs to be prepared to drop objects that would be moved into cruft packs.

To do this, we can combine the `git gc` invocation with `git prune` into one `git gc --prune`, which handles pruning both loose objects, and objects that would otherwise be written to a cruft pack.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t9300-fast-import.sh | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

On Mon, Apr 17, 2023 at 04:54:27PM -0400, Taylor Blau wrote:

Good. This is more efficient anyway. It probably does not matter for our tiny test repository, but it is always good for our tests to model the best option when possible. :)

The patch itself looks good to me.

-Peff

On Tue, Apr 18, 2023 at 06:43:31AM -0400, Jeff King wrote:

I spent more time than I'd willingly admit second-guessing myself wondering if there was something I'm missing why we'd want to call `git gc` and `git prune` separately.

As best as I can tell, this pattern started all the way back in 03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which happened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c: deprecate --prune, it now really has no effect, 2008-05-09).

After `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful again by accepting an optional parameter, 2009-02-14), we got a handful of new uses in this script via 4cedb78cb5 (fast-import: add input format tests, 2011-08-11), which could have been `git gc --prune`, but weren't (likely taking after 03db4525d3).

I don't know if any of that detail is interesting enough to rise to the level of needing to be in the patch itself, those were just my notes I took while wandering through this part of the suite.

Thanks!

-Taylor

In 12253ab6d0 (gc: add tests for --cruft and friends, 2022-10-26), we added a handful of tests to t6500 to ensure that `git gc` respected the value of `--cruft` and `gc.cruftPacks`.

Then, in c695592850 (config: let feature.experimental imply gc.cruftPacks=true, 2022-10-26), another set of similar tests was added to ensure that `feature.experimental` correctly implied enabling cruft pack generation (or not).

These tests are similar and could be consolidated. Do so in this patch to prepare for expanding the set of command-line invocations that enable or disable writing cruft packs. This makes it possible to easily test more combinations of arguments without being overly repetitive.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 126 ++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 82 deletions(-)

In the last commit, we refactored some of the tests in t6500 to make clearer when cruft packs will and won't be generated by `git gc`.

Add the remaining cases not covered by the previous patch into this one, which enumerates all possible combinations of arguments that will produce (or not produce) a cruft pack.

This prepares us for the following commit, which will change the default of `gc.cruftPacks` by ensuring that we understand which invocations do and do not change as a result.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 4 ++++
 1 file changed, 4 insertions(+)

On Mon, Apr 17, 2023 at 04:54:33PM -0400, Taylor Blau wrote:

I think "the following commit" is really patch 9. Patch 8 adjusts t6501 (and likewise says "like the previous commits"). It would probably make more sense to put patch 8 before patches 6 and 7.

Probably not worth a re-roll on its own, but if you're doing one anyway, it should be easy to do.

-Peff

On Tue, Apr 18, 2023 at 06:48:13AM -0400, Jeff King wrote:

I'm glad you said something, because shortly after I sent this series I was annoyed with myself for not organizing the preparatory patches in order of their test number.

I reordered them like so:

    c02f80e1e2 t/t5304-prune.sh: prepare for `gc --cruft` by default
    2e0fb1382b t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
    c916e6d356 t/t6500-gc.sh: refactor cruft pack tests
    f9f7b2137b t/t6500-gc.sh: add additional test cases
    5a74520b35 t/t9300-fast-import.sh: prepare for `gc --cruft` by default

Which feels much better to me.

Agreed on both. I'm rerolling a few tiny things anyways, so I'll squash this in, too.

Thanks, Taylor

In a similar spirit as previous commits, prepare for `gc --cruft` becoming the default by ensuring that the tests in t6501 explicitly cover the case of freshening loose objects not using cruft packs.

We could run this test twice, once with `--cruft` and once with `--no-cruft`, but doing so is unnecessary, since the object rescuing and freshening behavior is already extensively tested via t5329.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6501-freshen-objects.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

On Mon, Apr 17, 2023 at 04:54:36PM -0400, Taylor Blau wrote:

That's doubtless true for the general case of freshening (after all, that's the point of cruft packs). I do wonder about these "broken links" cases:

The idea is that we don't complain when repacking unreachable-but-broken segments of history. Which could perhaps behave differently for objects that are going into a cruft pack versus being turned loose. So maybe it's worth covering for the --cruft case, too. I dunno.

Certainly your patch is not making the test coverage worse, but it might be highlighting an existing blind-spot (and one that will become the default behavior in the next patch).

-Peff

On Tue, Apr 18, 2023 at 06:56:22AM -0400, Jeff King wrote:

I think we already have coverage of those cases in t5329, specifically in the tests:

  - cruft packs tolerate missing trees (expire $expire)
  - cruft packs tolerate missing blobs (expire $expire)

which are tested for both the pruning and non-pruning implementations (by setting $expire to "2.weeks.ago", and "never", respectively).

Thanks, Taylor

On Tue, Apr 18, 2023 at 03:50:33PM -0400, Taylor Blau wrote:

Ah, perfect. I only gave a quick look over t5329, and didn't see those. So yeah, I think it is fine to just have this whole script run in no-cruft mode.

-Peff

Back in 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via loose, 2022-05-20), `git gc` learned the `--cruft` option and `gc.cruftPacks` configuration to opt-in to writing cruft packs when collecting or pruning unreachable objects.

Cruft packs were introduced with the merge in a50036da1a (Merge branch 'tb/cruft-packs', 2022-06-03). They address the problem of "loose object explosions", where Git will write out many individual loose objects when there is a large number of unreachable objects that have not yet aged past `--prune=<date>`.

Instead of keeping track of those unreachable yet recent objects via their loose object file's mtime, cruft packs collect all unreachable objects into a single pack with a corresponding `*.mtimes` file that acts as a table to store the mtimes of all unreachable objects. This prevents the need to store unreachable objects as loose as they age out of the repository, and avoids the problem of loose object explosions.

Beyond avoiding loose object explosions, cruft packs also act as a more efficient mechanism to store unreachable objects as they age out of a repository. This is because pairs of similar unreachable objects serve as delta bases for one another.

In 5b92477f89, the feature was introduced as experimental. Since then, GitHub has been running these patches in every repository generating hundreds of millions of cruft packs along the way. The feature is battle-tested, and avoids many pathological cases such as above. Users who either run `git gc` manually, or via `git maintenance` can benefit from having cruft packs.

As such, enable cruft pack generation to take place by default (by making `gc.cruftPacks` have the default of "true" rather than "false).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/feature.txt | 3 ---
 Documentation/config/gc.txt      | 2 +-
 Documentation/git-gc.txt         | 5 +++--
 Documentation/gitformat-pack.txt | 4 ++--
 builtin/gc.c                     | 6 +-----
 t/t6500-gc.sh                    | 9 +++------
 6 files changed, 10 insertions(+), 19 deletions(-)

On Mon, Apr 17, 2023 at 04:54:39PM -0400, Taylor Blau wrote:

Oh good. I was a little sad to see the increase in the size of this loop in the earlier patches, so now reducing the number of combinations is a welcome change.

The set you have here looks fine, though isn't "gc --cruft" redundant with "gc" now?

Likewise here, "gc --no-cruft" would have been redundant with "gc" before this patch, but we did not even bother with it (so no need to remove it here!).

The rest of the patch looks good, and I am quite on board with the overall goal. It's been a long time coming. :)

-Peff

On Tue, Apr 18, 2023 at 07:00:49AM -0400, Jeff King wrote:

Sorry for the roller-coaster of emotions ;-).

Yeah, these are redundant. I figured that it might be good to test all cases, but I think this matrix-style of testing only gets you so far, and can often come out wasteful.

"gc --cruft" isn't testing anything meaningful, so let's drop it.

Yeah, this one we definitely want to keep. We probably *could* have dropped it in the previous patch and brought it back here, but I think that doing so would have been unnecessary churn for reviewers reading this series.

Thanks, I agree ;-).

Thanks, Taylor

As of the previous commit, all callers that need to read the value of `gc.cruftPacks` do so outside without using the `repo_settings` struct, making its `gc_cruft_packs` unused. Drop it accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 repo-settings.c | 4 +---
 repository.h    | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

On Mon, Apr 17, 2023 at 04:54:43PM -0400, Taylor Blau wrote:

Nice cleanup (I had to peek forward to understand the mystery repo_settings hunk in the previous patch; it is a little weird to me that this "settings" struct exists independent of the config, but that is not your doing).

-Peff

On Mon, Apr 17, 2023 at 04:54:11PM -0400, Taylor Blau wrote:

Thanks, this was a pleasant read. Besides Junio's small fixup in the docs, everything I pointed out was minor enough that it wouldn't be a big deal to go in as-is.

Me too. :)

-Peff

On Tue, Apr 18, 2023 at 07:04:24AM -0400, Jeff King wrote:

Thanks, both, for the review. Most (all?) of the clean-up was all pretty minor, but I would have rerolled here regardless. New version incoming just as soon as CI says it's all good.

Thanks, Taylor

Here is a very tiny reroll of my series to graduate `gc.cruftPacks` out of `feature.experimental` and enables it by default.

A complete summary of the topic is available in the original cover letter[1], and the changes since last time are limited to test clean-up, patch reorganization, and some touch-ups on the patch messages themselves.

As always, a range-diff is below for convenience.

Thanks for all of the review thus far, and in advance for any review on this round, too.

[1]: https://lore.kernel.org/git/cover.1681764848.git.me@ttaylorr.com/

Taylor Blau (10):
  pack-write.c: plug a leak in stage_tmp_packfiles()
  builtin/repack.c: fix incorrect reference to '-C'
  builtin/gc.c: ignore cruft packs with `--keep-largest-pack`
  t/t5304-prune.sh: prepare for `gc --cruft` by default
  t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
  t/t6500-gc.sh: refactor cruft pack tests
  t/t6500-gc.sh: add additional test cases
  t/t9300-fast-import.sh: prepare for `gc --cruft` by default
  builtin/gc.c: make `gc.cruftPacks` enabled by default
  repository.h: drop unused `gc_cruft_packs`

 Documentation/config/feature.txt |   3 -
 Documentation/config/gc.txt      |  12 +--
 Documentation/git-gc.txt         |  12 +--
 Documentation/gitformat-pack.txt |   4 +-
 builtin/gc.c                     |   8 +-
 builtin/repack.c                 |   2 +-
 pack-write.c                     |  14 ++--
 repo-settings.c                  |   4 +-
 repository.h                     |   1 -
 t/t5304-prune.sh                 |  28 +++----
 t/t6500-gc.sh                    | 135 ++++++++++++++++---------------
 t/t6501-freshen-objects.sh       |  10 +--
 t/t9300-fast-import.sh           |  13 +--
 13 files changed, 120 insertions(+), 126 deletions(-)

Range-diff against v1:
 1:  65ac7ed9b8 =  1:  c477b754e7 pack-write.c: plug a leak in stage_tmp_packfiles()
 2:  fbc8d15032 =  2:  52fb61fa9c builtin/repack.c: fix incorrect reference to '-C'
 3:  796df920ad !  3:  63b9ee8e2e builtin/gc.c: ignore cruft packs with `--keep-largest-pack`
    @@ Commit message
           - The same is true for `gc.bigPackThreshold`, if the size of the cruft
             pack exceeds the limit set by the caller.
     
    -    Ignore cruft packs in the common implementation for both of these
    -    options, and add a pair of tests to prevent any future regressions here.
    +    In the future, it is possible that `gc.bigPackThreshold` could be used
    +    to write a separate cruft pack containing any new unreachable objects
    +    that entered the repository since the last time a cruft pack was
    +    written.
    +
    +    There are some complexities to doing so, mainly around handling
    +    pruning objects that are in an existing cruft pack that is above the
    +    threshold (which would either need to be rewritten, or else delay
    +    pruning). Rewriting a substantially similar cruft pack isn't ideal, but
    +    it is significantly better than the status-quo.
    +
    +    If users have large cruft packs that they don't want to rewrite, they
    +    can mark them as `*.keep` packs. But in general, if a repository has a
    +    cruft pack that is so large it is slowing down GC's, it should probably
    +    be pruned anyway.
    +
    +    In the meantime, ignore cruft packs in the common implementation for
    +    both of these options, and add a pair of tests to prevent any future
    +    regressions here.
     
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
    @@ Documentation/git-gc.txt: be performed as well.
     -	All packs except the largest pack and those marked with a
     -	`.keep` files are consolidated into a single pack. When this
     -	option is used, `gc.bigPackThreshold` is ignored.
    -+	All packs except the largest pack, any packs marked with a
    -+	`.keep` file, and any cruft pack(s) are consolidated into a
    -+	single pack. When this option is used, `gc.bigPackThreshold` is
    -+	ignored.
    ++	All packs except the largest non-cruft pack, any packs marked
    ++	with a `.keep` file, and any cruft pack(s) are consolidated into
    ++	a single pack. When this option is used, `gc.bigPackThreshold`
    ++	is ignored.
      
      AGGRESSIVE
      ----------
 4:  44006da959 =  4:  905eeb9027 t/t5304-prune.sh: prepare for `gc --cruft` by default
 8:  4ccc525c39 !  5:  fa6eafb1fe t/t6501-freshen-objects.sh: prepare for `gc --cruft` by default
    @@ Commit message
         cover the case of freshening loose objects not using cruft packs.
     
         We could run this test twice, once with `--cruft` and once with
    -    `--no-cruft`, but doing so is unnecessary, since the object rescuing and
    -    freshening behavior is already extensively tested via t5329.
    +    `--no-cruft`, but doing so is unnecessary, since we already test object
    +    rescuing, freshening, and dealing with corrupt parts of the unreachable
    +    object graph extensively via t5329.
     
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
 6:  56a965e517 =  6:  e6270d75fa t/t6500-gc.sh: refactor cruft pack tests
 7:  6957e54f51 !  7:  9db3fa9e36 t/t6500-gc.sh: add additional test cases
    @@ Commit message
         which enumerates all possible combinations of arguments that will
         produce (or not produce) a cruft pack.
     
    -    This prepares us for the following commit, which will change the default
    +    This prepares us for a future commit which will change the default value
         of `gc.cruftPacks` by ensuring that we understand which invocations do
         and do not change as a result.
     
    @@ t/t6500-gc.sh: do
      done
      
      for argv in \
    -+	"gc --no-cruft" \
    ++	"gc" \
     +	"-c gc.cruftPacks=false gc" \
     +	"-c gc.cruftPacks=true gc --no-cruft" \
      	"-c feature.expiremental=true -c gc.cruftPacks=false gc" \
 5:  1b07eb83fe !  8:  894cf176ea t/t9300-fast-import.sh: prepare for `gc --cruft` by default
    @@ Metadata
      ## Commit message ##
         t/t9300-fast-import.sh: prepare for `gc --cruft` by default
     
    -    In a similar fashion as the previous commit, adjust the fast-import
    -    tests to prepare for "git gc" generating a cruft pack by default.
    +    In a similar fashion as previous commits, adjust the fast-import tests
    +    to prepare for "git gc" generating a cruft pack by default.
     
         This adjustment is slightly different, however. Instead of relying on us
         writing out the objects loose, and then calling `git prune` to remove
    @@ Commit message
         one `git gc --prune`, which handles pruning both loose objects, and
         objects that would otherwise be written to a cruft pack.
     
    +    Likely this pattern of "git gc && git prune" started all the way back in
    +    03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which
    +    happened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c:
    +    deprecate --prune, it now really has no effect, 2008-05-09).
    +
    +    After `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful
    +    again by accepting an optional parameter, 2009-02-14), this script got a
    +    handful of new "git gc && git prune" instances via via 4cedb78cb5
    +    (fast-import: add input format tests, 2011-08-11). These could have been
    +    `git gc --prune`, but weren't (likely taking after 03db4525d3).
    +
         Signed-off-by: Taylor Blau <me@ttaylorr.com>
     
      ## t/t9300-fast-import.sh ##
 9:  bfda40a21d !  9:  b6784ddfe2 builtin/gc.c: make `gc.cruftPacks` enabled by default
    @@ t/t6500-gc.sh: assert_no_cruft_packs () {
      }
      
      for argv in \
    +-	"gc --cruft" \
     +	"gc" \
    - 	"gc --cruft" \
      	"-c gc.cruftPacks=true gc" \
     -	"-c gc.cruftPacks=false gc --cruft" \
     -	"-c feature.experimental=true gc" \
    @@ t/t6500-gc.sh: assert_no_cruft_packs () {
      do
      	test_expect_success "git $argv generates a cruft pack" '
      		test_when_finished "rm -fr repo" &&
    -@@ t/t6500-gc.sh: done
    +@@ t/t6500-gc.sh: do
    + done
    + 
      for argv in \
    - 	"gc --no-cruft" \
    +-	"gc" \
    ++	"gc --no-cruft" \
      	"-c gc.cruftPacks=false gc" \
     -	"-c gc.cruftPacks=true gc --no-cruft" \
     -	"-c feature.expiremental=true -c gc.cruftPacks=false gc" \
10:  fa15125454 = 10:  c67ee7c2ff repository.h: drop unused `gc_cruft_packs`

The function `stage_tmp_packfiles()` generates a filename to use for staging the contents of what will become the pack's ".mtimes" file.

The name is generated in `write_mtimes_file()` and the result is returned back to `stage_tmp_packfiles()` which uses it to rename the temporary file into place via `rename_tmp_packfiles()`.

`write_mtimes_file()` returns a `const char *`, indicating that callers are not expected to free its result (similar to, e.g., `oid_to_hex()`). But callers are expected to free its result, so this return type is incorrect.

Change the function's signature to return a non-const `char *`, and free it at the end of `stage_tmp_packfiles()`.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 pack-write.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Indeed the string that holds the name of the file returned by write_mtimes_file() is leaking. Does the same logic apply to the returned filename from write_rev_file() and stored in rev_tmp_name that is not freed in stage_tmp_packfiles() in another topic?

On Wed, Apr 19, 2023 at 03:00:48PM -0700, Junio C Hamano wrote:

They are similar, but unfortunately different.

Here, our temporary name is generated by `write_mtimes_file()` which *always* comes up with a new name from scratch. So we know that it should always be free()'d at the end of `stage_tmp_packfiles()`.

But in the case of `write_rev_file()`, it only *sometimes* generates a new name from scratch. The first parameter can be NULL, in which case `write_rev_file()` will generate a new name. Or it can be non-NULL, in which case that name will be used instead.

So in that topic, it's less clear on what the ultimate right path forward is. But in this topic, changing `mtimes_tmp_name` and the return type of `write_mtimes_file()` to be a non-const `char *` (and free()ing it, of course ;-)) is the right thing to do.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

I hoped "fortunately different and there is no leak", but it seems what you said below is a bit different X-<.

IOW, it is a mess.

But I think the topic to introduce that variable is being rerolled and has no interaction with the codepath we are discussing here, so we are still in good shape wrt this series ;-)

Thanks.

When cruft packs were originally being developed, `-C` was designated as the short-form for `--cruft` (as in `git repack -C`).

This was dropped due to confusion with Git's top-level `-C` option before submitting to the list. But the reference to it in `--cruft-expiration`'s help text was never updated. Fix that dangling reference in this patch.

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

When cruft packs were implemented, we never adjusted the code for `git gc`'s `--keep-largest-pack` and `gc.bigPackThreshold` to ignore cruft packs. This option and configuration option share a common implementation, but including cruft packs is wrong in both cases:

  - Running `git gc --keep-largest-pack` in a repository where the
    largest pack is the cruft pack itself will make it impossible for
    `git gc` to prune objects, since the cruft pack itself is kept.

  - The same is true for `gc.bigPackThreshold`, if the size of the cruft
    pack exceeds the limit set by the caller.

In the future, it is possible that `gc.bigPackThreshold` could be used to write a separate cruft pack containing any new unreachable objects that entered the repository since the last time a cruft pack was written.

There are some complexities to doing so, mainly around handling pruning objects that are in an existing cruft pack that is above the threshold (which would either need to be rewritten, or else delay pruning). Rewriting a substantially similar cruft pack isn't ideal, but it is significantly better than the status-quo.

If users have large cruft packs that they don't want to rewrite, they can mark them as `*.keep` packs. But in general, if a repository has a cruft pack that is so large it is slowing down GC's, it should probably be pruned anyway.

In the meantime, ignore cruft packs in the common implementation for both of these options, and add a pair of tests to prevent any future regressions here.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/gc.txt | 10 ++++-----
 Documentation/git-gc.txt    |  7 +++---
 builtin/gc.c                |  2 +-
 t/t6500-gc.sh               | 43 +++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 9 deletions(-)

Many of the tests in t5304 run `git gc`, and rely on its behavior that unreachable-but-recent objects are written out loose. This is sensible, since t5304 deals specifically with this kind of pruning.

If left unattended, however, this test would break when the default behavior of a bare "git gc" is adjusted to generate a cruft pack by default.

Ensure that these tests continue to work as-is (and continue to provide coverage of loose object pruning) by passing `--no-cruft` explicitly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t5304-prune.sh | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

In a similar spirit as previous commits, prepare for `gc --cruft` becoming the default by ensuring that the tests in t6501 explicitly cover the case of freshening loose objects not using cruft packs.

We could run this test twice, once with `--cruft` and once with `--no-cruft`, but doing so is unnecessary, since we already test object rescuing, freshening, and dealing with corrupt parts of the unreachable object graph extensively via t5329.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6501-freshen-objects.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

In 12253ab6d0 (gc: add tests for --cruft and friends, 2022-10-26), we added a handful of tests to t6500 to ensure that `git gc` respected the value of `--cruft` and `gc.cruftPacks`.

Then, in c695592850 (config: let feature.experimental imply gc.cruftPacks=true, 2022-10-26), another set of similar tests was added to ensure that `feature.experimental` correctly implied enabling cruft pack generation (or not).

These tests are similar and could be consolidated. Do so in this patch to prepare for expanding the set of command-line invocations that enable or disable writing cruft packs. This makes it possible to easily test more combinations of arguments without being overly repetitive.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 126 ++++++++++++++++++--------------------------------
 1 file changed, 44 insertions(+), 82 deletions(-)

In the last commit, we refactored some of the tests in t6500 to make clearer when cruft packs will and won't be generated by `git gc`.

Add the remaining cases not covered by the previous patch into this one, which enumerates all possible combinations of arguments that will produce (or not produce) a cruft pack.

This prepares us for a future commit which will change the default value of `gc.cruftPacks` by ensuring that we understand which invocations do and do not change as a result.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t6500-gc.sh | 4 ++++
 1 file changed, 4 insertions(+)

In a similar fashion as previous commits, adjust the fast-import tests to prepare for "git gc" generating a cruft pack by default.

This adjustment is slightly different, however. Instead of relying on us writing out the objects loose, and then calling `git prune` to remove them, t9300 needs to be prepared to drop objects that would be moved into cruft packs.

To do this, we can combine the `git gc` invocation with `git prune` into one `git gc --prune`, which handles pruning both loose objects, and objects that would otherwise be written to a cruft pack.

Likely this pattern of "git gc && git prune" started all the way back in 03db4525d3 (Support gitlinks in fast-import., 2008-07-19), which happened after deprecating `git gc --prune` in 9e7d501990 (builtin-gc.c: deprecate --prune, it now really has no effect, 2008-05-09).

After `--prune` was un-deprecated in 58e9d9d472 (gc: make --prune useful again by accepting an optional parameter, 2009-02-14), this script got a handful of new "git gc && git prune" instances via via 4cedb78cb5 (fast-import: add input format tests, 2011-08-11). These could have been `git gc --prune`, but weren't (likely taking after 03db4525d3).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 t/t9300-fast-import.sh | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Back in 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via loose, 2022-05-20), `git gc` learned the `--cruft` option and `gc.cruftPacks` configuration to opt-in to writing cruft packs when collecting or pruning unreachable objects.

Cruft packs were introduced with the merge in a50036da1a (Merge branch 'tb/cruft-packs', 2022-06-03). They address the problem of "loose object explosions", where Git will write out many individual loose objects when there is a large number of unreachable objects that have not yet aged past `--prune=<date>`.

Instead of keeping track of those unreachable yet recent objects via their loose object file's mtime, cruft packs collect all unreachable objects into a single pack with a corresponding `*.mtimes` file that acts as a table to store the mtimes of all unreachable objects. This prevents the need to store unreachable objects as loose as they age out of the repository, and avoids the problem of loose object explosions.

Beyond avoiding loose object explosions, cruft packs also act as a more efficient mechanism to store unreachable objects as they age out of a repository. This is because pairs of similar unreachable objects serve as delta bases for one another.

In 5b92477f89, the feature was introduced as experimental. Since then, GitHub has been running these patches in every repository generating hundreds of millions of cruft packs along the way. The feature is battle-tested, and avoids many pathological cases such as above. Users who either run `git gc` manually, or via `git maintenance` can benefit from having cruft packs.

As such, enable cruft pack generation to take place by default (by making `gc.cruftPacks` have the default of "true" rather than "false).

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 Documentation/config/feature.txt |  3 ---
 Documentation/config/gc.txt      |  2 +-
 Documentation/git-gc.txt         |  5 +++--
 Documentation/gitformat-pack.txt |  4 ++--
 builtin/gc.c                     |  6 +-----
 t/t6500-gc.sh                    | 12 ++++--------
 6 files changed, 11 insertions(+), 21 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

Being listed here is a definite sign that a feature behind a configuration variable is considered experimental. Do we have (and if not, do we want to establish) a procedure to mark and announce a feature that used to be experimental no longer is? If it is enough to mention it in the release notes, then I can take care of it, of course.

On Wed, Apr 19, 2023 at 03:22:13PM -0700, Junio C Hamano wrote:

I am not aware of such a procedure. But personally I think it would be fine to mention it in the release notes for the next release.

Thanks, Taylor

Taylor Blau <me@ttaylorr.com> writes:

OK. I updated the entry for the topic in the draft "What's cooking" report to read:

    * tb/enable-cruft-packs-by-default (2023-04-18) 10 commits
     - ...

     When "gc" needs to retain unreachable objects, packing them into
     cruft packs (instead of exploding them into loose object files) has
     been offered as a more efficient option for some time.  Now the use
     of cruft packs has been made the default and no longer considered
     an experimental feature.

     Will merge to 'next'.
     source: <cover.1681850424.git.me@ttaylorr.com>

and the per-topic description text is usually what is copied verbatim to the release notes, so even if I forget, the procedure will remember it for us ;-)

Thanks.

On Thu, Apr 20, 2023 at 10:31:00AM -0700, Junio C Hamano wrote:

Thanks, I think that summarizes/announces the change well. I always appreciate your effort into concisely summarizing the topics being queued.

;-).

Thanks, Taylor

As of the previous commit, all callers that need to read the value of `gc.cruftPacks` do so outside without using the `repo_settings` struct, making its `gc_cruft_packs` unused. Drop it accordingly.

Signed-off-by: Taylor Blau <me@ttaylorr.com>
---
 repo-settings.c | 4 +---
 repository.h    | 1 -
 2 files changed, 1 insertion(+), 4 deletions(-)

Taylor Blau <me@ttaylorr.com> writes:

This is in line with the current design of repo-settings, and it is a total tangent to this topic, but in retrospect, if we structured how "what's experimental" is determined in a more distributed way, we didn't have to have this patch. That is, instead of saying "ok we are in a repository with experimental setting turned on, and the set of features to be enabled are centrally described in this settings structure", each opt-in features (like the code that reads "gc.cruftPacks" by calling git_config_get_bool() to override its default value) can select their default/fallback values by seeing if the experimental bit is set.

In any case, this patch looks good and we have one fewer such centrally managed bit.