* [PATCH 0/2] repack: implement `--cruft-max-size`
@ 2023-09-07 21:51 Taylor Blau
2023-09-07 21:52 ` [PATCH 1/2] t7700: split cruft-related tests to t7704 Taylor Blau
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: Taylor Blau @ 2023-09-07 21:51 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan
(These patches should be applied on top of a merge with
tb/repack-existing-packs-cleanup, and tb/multi-cruft-pack).
This series attempts to give users some more robust tools for managing
repositories with a large number of unreachable objects by storing them
in separate cruft packs, via a new option `--cruft-max-size`, like so:
$ git.compile repack -d --cruft --max-pack-size=10M
[...]
Enumerating cruft objects: 617483, done.
Counting objects: 100% (83791/83791), done.
Delta compression using up to 20 threads
Compressing objects: 100% (59696/59696), done.
Writing objects: 100% (83791/83791), done.
Total 83791 (delta 19251), reused 82502 (delta 19148), pack-reused 0
$ ls -la .git/objects/pack/pack-*.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 179144 Sep 7 17:46 .git/objects/pack/pack-1a95260d26f2897abfd2d54f1d58f535acb81d23.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 452 Sep 7 17:46 .git/objects/pack/pack-5fde8701ae0f2e5553f1fa33de05faf12f94c07f.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 155720 Sep 7 17:46 .git/objects/pack/pack-91f9e66921e0ebe1b5e35d34842551468cecdc28.mtimes
-r--r--r-- 1 ttaylorr ttaylorr 56 Sep 7 17:46 .git/objects/pack/pack-95fe626743207b177b45f32b60fdc313e525ea60.mtimes
The details are explained in the second patch, but the gist is that we
will combine cruft packs up until they reach a certain threshold (as
specified by `--cruft-max-size`) and then begin a new "generation" of
cruft packs. That younger generation will grow up until it reaches the
configured threshold, at which point it will become "frozen" and then
any new unreachable objects will be written into a new generation of
cruft packs.
The goal of this series is to reduce I/O churn in repositories that
either (a) have a large number of unreachable objects, (b) rarely prune
them, or (c) both.
Instead of having to rewrite a cruft pack containing every unreachable
object in the repository, we only have to rewrite a cruft pack up until
it reaches the given threshold, at which point it is effectively kept
(i.e., it behaves as if the cruft pack had a ".keep" file tied to it,
provided that the threshold is held constant).
Thanks in advance for your review!
Taylor Blau (2):
t7700: split cruft-related tests to t7704
builtin/repack.c: implement support for `--cruft-max-size`
Documentation/config/gc.txt | 6 +
Documentation/git-gc.txt | 7 +
Documentation/git-repack.txt | 9 +
builtin/gc.c | 8 +
builtin/repack.c | 133 +++++++++++--
t/t6500-gc.sh | 27 +++
t/t7700-repack.sh | 121 -----------
t/t7704-repack-cruft.sh | 375 +++++++++++++++++++++++++++++++++++
8 files changed, 553 insertions(+), 133 deletions(-)
create mode 100755 t/t7704-repack-cruft.sh
--
2.42.0.138.g7e4e42e1aa
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/2] t7700: split cruft-related tests to t7704 2023-09-07 21:51 [PATCH 0/2] repack: implement `--cruft-max-size` Taylor Blau @ 2023-09-07 21:52 ` Taylor Blau 2023-09-08 0:01 ` Eric Sunshine 2023-09-07 21:52 ` [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau 2 siblings, 1 reply; 21+ messages in thread From: Taylor Blau @ 2023-09-07 21:52 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan --- t/t7700-repack.sh | 121 ------------------------------------- t/t7704-repack-cruft.sh | 130 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 121 deletions(-) create mode 100755 t/t7704-repack-cruft.sh diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 27b66807cd..55ee3eb8ae 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -633,125 +633,4 @@ test_expect_success '-n overrides repack.updateServerInfo=true' ' test_server_info_missing ' -test_expect_success '--expire-to stores pruned objects (now)' ' - git init expire-to-now && - ( - cd expire-to-now && - - git branch -M main && - - test_commit base && - - git checkout -b cruft && - test_commit --no-tag cruft && - - git rev-list --objects --no-object-names main..cruft >moved.raw && - sort moved.raw >moved.want && - - git rev-list --all --objects --no-object-names >expect.raw && - sort expect.raw >expect && - - git checkout main && - git branch -D cruft && - git reflog expire --all --expire=all && - - git init --bare expired.git && - git repack -d \ - --cruft --cruft-expiration="now" \ - --expire-to="expired.git/objects/pack/pack" && - - expired="$(ls expired.git/objects/pack/pack-*.idx)" && - test_path_is_file "${expired%.idx}.mtimes" && - - # Since the `--cruft-expiration` is "now", the effective - # behavior is to move _all_ unreachable objects out to - # the location in `--expire-to`. - git show-index <$expired >expired.raw && - cut -d" " -f2 expired.raw | sort >expired.objects && - git rev-list --all --objects --no-object-names \ - >remaining.objects && - - # ...in other words, the combined contents of this - # repository and expired.git should be the same as the - # set of objects we started with. - cat expired.objects remaining.objects | sort >actual && - test_cmp expect actual && - - # The "moved" objects (i.e., those in expired.git) - # should be the same as the cruft objects which were - # expired in the previous step. - test_cmp moved.want expired.objects - ) -' - -test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' - git init expire-to-5.minutes.ago && - ( - cd expire-to-5.minutes.ago && - - git branch -M main && - - test_commit base && - - # Create two classes of unreachable objects, one which - # is older than 5 minutes (stale), and another which is - # newer (recent). - for kind in stale recent - do - git checkout -b $kind main && - test_commit --no-tag $kind || return 1 - done && - - git rev-list --objects --no-object-names main..stale >in && - stale="$(git pack-objects $objdir/pack/pack <in)" && - mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" && - - # expect holds the set of objects we expect to find in - # this repository after repacking - git rev-list --objects --no-object-names recent >expect.raw && - sort expect.raw >expect && - - # moved.want holds the set of objects we expect to find - # in expired.git - git rev-list --objects --no-object-names main..stale >out && - sort out >moved.want && - - git checkout main && - git branch -D stale recent && - git reflog expire --all --expire=all && - git prune-packed && - - git init --bare expired.git && - git repack -d \ - --cruft --cruft-expiration=5.minutes.ago \ - --expire-to="expired.git/objects/pack/pack" && - - # Some of the remaining objects in this repository are - # unreachable, so use `cat-file --batch-all-objects` - # instead of `rev-list` to get their names - git cat-file --batch-all-objects --batch-check="%(objectname)" \ - >remaining.objects && - sort remaining.objects >actual && - test_cmp expect actual && - - ( - cd expired.git && - - expired="$(ls objects/pack/pack-*.mtimes)" && - test-tool pack-mtimes $(basename $expired) >out && - cut -d" " -f1 out | sort >../moved.got && - - # Ensure that there are as many objects with the - # expected mtime as were moved to expired.git. - # - # In other words, ensure that the recorded - # mtimes of any moved objects was written - # correctly. - grep " $mtime$" out >matching && - test_line_count = $(wc -l <../moved.want) matching - ) && - test_cmp moved.want moved.got - ) -' - test_done diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh new file mode 100755 index 0000000000..d91fcf1af1 --- /dev/null +++ b/t/t7704-repack-cruft.sh @@ -0,0 +1,130 @@ +#!/bin/sh + +test_description='git repack works correctly' + +. ./test-lib.sh + +objdir=.git/objects + +test_expect_success '--expire-to stores pruned objects (now)' ' + git init expire-to-now && + ( + cd expire-to-now && + + git branch -M main && + + test_commit base && + + git checkout -b cruft && + test_commit --no-tag cruft && + + git rev-list --objects --no-object-names main..cruft >moved.raw && + sort moved.raw >moved.want && + + git rev-list --all --objects --no-object-names >expect.raw && + sort expect.raw >expect && + + git checkout main && + git branch -D cruft && + git reflog expire --all --expire=all && + + git init --bare expired.git && + git repack -d \ + --cruft --cruft-expiration="now" \ + --expire-to="expired.git/objects/pack/pack" && + + expired="$(ls expired.git/objects/pack/pack-*.idx)" && + test_path_is_file "${expired%.idx}.mtimes" && + + # Since the `--cruft-expiration` is "now", the effective + # behavior is to move _all_ unreachable objects out to + # the location in `--expire-to`. + git show-index <$expired >expired.raw && + cut -d" " -f2 expired.raw | sort >expired.objects && + git rev-list --all --objects --no-object-names \ + >remaining.objects && + + # ...in other words, the combined contents of this + # repository and expired.git should be the same as the + # set of objects we started with. + cat expired.objects remaining.objects | sort >actual && + test_cmp expect actual && + + # The "moved" objects (i.e., those in expired.git) + # should be the same as the cruft objects which were + # expired in the previous step. + test_cmp moved.want expired.objects + ) +' + +test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' + git init expire-to-5.minutes.ago && + ( + cd expire-to-5.minutes.ago && + + git branch -M main && + + test_commit base && + + # Create two classes of unreachable objects, one which + # is older than 5 minutes (stale), and another which is + # newer (recent). + for kind in stale recent + do + git checkout -b $kind main && + test_commit --no-tag $kind || return 1 + done && + + git rev-list --objects --no-object-names main..stale >in && + stale="$(git pack-objects $objdir/pack/pack <in)" && + mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" && + + # expect holds the set of objects we expect to find in + # this repository after repacking + git rev-list --objects --no-object-names recent >expect.raw && + sort expect.raw >expect && + + # moved.want holds the set of objects we expect to find + # in expired.git + git rev-list --objects --no-object-names main..stale >out && + sort out >moved.want && + + git checkout main && + git branch -D stale recent && + git reflog expire --all --expire=all && + git prune-packed && + + git init --bare expired.git && + git repack -d \ + --cruft --cruft-expiration=5.minutes.ago \ + --expire-to="expired.git/objects/pack/pack" && + + # Some of the remaining objects in this repository are + # unreachable, so use `cat-file --batch-all-objects` + # instead of `rev-list` to get their names + git cat-file --batch-all-objects --batch-check="%(objectname)" \ + >remaining.objects && + sort remaining.objects >actual && + test_cmp expect actual && + + ( + cd expired.git && + + expired="$(ls objects/pack/pack-*.mtimes)" && + test-tool pack-mtimes $(basename $expired) >out && + cut -d" " -f1 out | sort >../moved.got && + + # Ensure that there are as many objects with the + # expected mtime as were moved to expired.git. + # + # In other words, ensure that the recorded + # mtimes of any moved objects was written + # correctly. + grep " $mtime$" out >matching && + test_line_count = $(wc -l <../moved.want) matching + ) && + test_cmp moved.want moved.got + ) +' + +test_done -- 2.42.0.138.g7e4e42e1aa ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] t7700: split cruft-related tests to t7704 2023-09-07 21:52 ` [PATCH 1/2] t7700: split cruft-related tests to t7704 Taylor Blau @ 2023-09-08 0:01 ` Eric Sunshine 0 siblings, 0 replies; 21+ messages in thread From: Eric Sunshine @ 2023-09-08 0:01 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Jonathan Tan On Thu, Sep 7, 2023 at 5:52 PM Taylor Blau <me@ttaylorr.com> wrote: > t7700: split cruft-related tests to t7704 > --- Missing sign-off. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` 2023-09-07 21:51 [PATCH 0/2] repack: implement `--cruft-max-size` Taylor Blau 2023-09-07 21:52 ` [PATCH 1/2] t7700: split cruft-related tests to t7704 Taylor Blau @ 2023-09-07 21:52 ` Taylor Blau 2023-09-07 23:42 ` Junio C Hamano 2023-09-08 11:21 ` Patrick Steinhardt 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau 2 siblings, 2 replies; 21+ messages in thread From: Taylor Blau @ 2023-09-07 21:52 UTC (permalink / raw) To: git; +Cc: Junio C Hamano, Jeff King, Jonathan Tan Cruft packs are an alternative mechanism for storing a collection of unreachable objects whose mtimes are recent enough to avoid being pruned out of the repository. When cruft packs were first introduced back in b757353676 (builtin/pack-objects.c: --cruft without expiration, 2022-05-20) and a7d493833f (builtin/pack-objects.c: --cruft with expiration, 2022-05-20), the recommended workflow consisted of: - Repacking periodically, either by packing anything loose in the repository (via `git repack -d`) or producing a geometric sequence of packs (via `git repack --geometric=<d> -d`). - Every so often, splitting the repository into two packs, one cruft to store the unreachable objects, and another non-cruft pack to store the reachable objects. Repositories may (out of band with the above) choose periodically to prune out some unreachable objects which have aged out of the grace period by generating a pack with `--cruft-expiration=<approxidate>`. This allowed repositories to maintain relatively few packs on average, and quarantine unreachable objects together in a cruft pack, avoiding the pitfalls of holding unreachable objects as loose while they age out (for more, see some of the details in 3d89a8c118 (Documentation/technical: add cruft-packs.txt, 2022-05-20)). This all works, but can be costly from an I/O-perspective when a repository has either (a) many unreachable objects, (b) prunes objects relatively infrequently/never, or (c) both. Since there is at most one cruft pack in the above scheme, each time we update the cruft pack it must be rewritten from scratch. Because much of the pack is reused, this is a relatively inexpensive operation from a CPU-perspective, but is very costly in terms of I/O since we end up rewriting basically the same pack (plus any new unreachable objects that have entered the repository since the last time a cruft pack was generated). At the time, we decided against implementing more robust support for multiple cruft packs. This patch implements that support which we were lacking. Introduce a new option `--cruft-max-size` which allows repositories to accumulate cruft packs up to a given size, after which point a new generation of cruft packs can accumulate until it reaches the maximum size, and so on. To generate a new cruft pack, the process works like so: - Sort a list of any existing cruft packs in ascending order of pack size. - Starting from the beginning of the list, group cruft packs together while the accumulated size is smaller than the maximum specified pack size. - Combine the objects in these cruft packs together into a new cruft pack, along with any other unreachable objects which have since entered the repository. This limits the I/O churn up to a quadratic function of the value specified by the `--cruft-max-size` option, instead of behaving quadratically in the number of total unreachable objects. When pruning unreachable objects, we bypass the new paths which combine small cruft packs together, and instead start from scratch, passing in the appropriate `--max-pack-size` down to `pack-objects`, putting it in charge of keeping the resulting set of cruft packs sized correctly. This may seem like further I/O churn, but in practice it isn't so bad. We could prune old cruft packs for whom all or most objects are removed, and then generate a new cruft pack with just the remaining set of objects. But this additional complexity buys us relatively little, because most objects end up being pruned anyway, so the I/O churn is well contained. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/gc.txt | 6 + Documentation/git-gc.txt | 7 + Documentation/git-repack.txt | 9 ++ builtin/gc.c | 8 ++ builtin/repack.c | 133 +++++++++++++++++-- t/t6500-gc.sh | 27 ++++ t/t7704-repack-cruft.sh | 245 +++++++++++++++++++++++++++++++++++ 7 files changed, 423 insertions(+), 12 deletions(-) diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt index ca47eb2008..795fcfbb69 100644 --- a/Documentation/config/gc.txt +++ b/Documentation/config/gc.txt @@ -86,6 +86,12 @@ gc.cruftPacks:: linkgit:git-repack[1]) instead of as loose objects. The default is `true`. +gc.cruftMaxSize:: + Limit the size of new cruft packs when repacking. When + specified in addition to `--cruft-max-size`, the command line + option takes priority. See the `--cruft-max-size` option of + linkgit:git-repack[1]. + gc.pruneExpire:: When 'git gc' is run, it will call 'prune --expire 2.weeks.ago' (and 'repack --cruft --cruft-expiration 2.weeks.ago' if using diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt index 90806fd26a..8a90d684a7 100644 --- a/Documentation/git-gc.txt +++ b/Documentation/git-gc.txt @@ -59,6 +59,13 @@ be performed as well. cruft pack instead of storing them as loose objects. `--cruft` is on by default. +--cruft-max-size=<n>:: + When packing unreachable objects into a cruft pack, limit the + size of new cruft packs to be at most `<n>`. Overrides any + value specified via the `gc.cruftMaxSize` configuration. See + the `--cruft-max-size` option of linkgit:git-repack[1] for + more. + --prune=<date>:: Prune loose objects older than date (default is 2 weeks ago, overridable by the config variable `gc.pruneExpire`). diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt index 4017157949..23fd203d79 100644 --- a/Documentation/git-repack.txt +++ b/Documentation/git-repack.txt @@ -74,6 +74,15 @@ to the new separate pack will be written. immediately instead of waiting for the next `git gc` invocation. Only useful with `--cruft -d`. +--cruft-max-size=<n>:: + Repack cruft objects into packs as large as `<n>` before + creating new packs. As long as there are enough cruft packs + smaller than `<n>`, repacking will cause a new cruft pack to + be created containing objects from any combined cruft packs, + along with any new unreachable objects. Cruft packs larger + than `<n>` will not be modified. Only useful with `--cruft + -d`. + --expire-to=<dir>:: Write a cruft pack containing pruned objects (if any) to the directory `<dir>`. This option is useful for keeping a copy of diff --git a/builtin/gc.c b/builtin/gc.c index 1f53b66c7b..b6640abd35 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = { static int pack_refs = 1; static int prune_reflogs = 1; static int cruft_packs = 1; +static char *cruft_max_size; static int aggressive_depth = 50; static int aggressive_window = 250; static int gc_auto_threshold = 6700; @@ -163,6 +164,7 @@ static void gc_config(void) git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); git_config_get_bool("gc.autodetach", &detach_auto); git_config_get_bool("gc.cruftpacks", &cruft_packs); + git_config_get_string("gc.cruftmaxsize", &cruft_max_size); git_config_get_expiry("gc.pruneexpire", &prune_expire); git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire); git_config_get_expiry("gc.logexpiry", &gc_log_expire); @@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack) strvec_push(&repack, "--cruft"); if (prune_expire) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire); + if (cruft_max_size) + strvec_pushf(&repack, "--cruft-max-size=%s", + cruft_max_size); } else { strvec_push(&repack, "-A"); if (prune_expire) @@ -575,6 +580,9 @@ int cmd_gc(int argc, const char **argv, const char *prefix) N_("prune unreferenced objects"), PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire }, OPT_BOOL(0, "cruft", &cruft_packs, N_("pack unreferenced objects separately")), + OPT_STRING(0, "cruft-max-size", &cruft_max_size, + N_("bytes"), + N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")), OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"), PARSE_OPT_NOCOMPLETE), diff --git a/builtin/repack.c b/builtin/repack.c index 44cb261371..56e7f5f43d 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -26,6 +26,9 @@ #define LOOSEN_UNREACHABLE 2 #define PACK_CRUFT 4 +#define DELETE_PACK ((void*)(uintptr_t)1) +#define RETAIN_PACK ((uintptr_t)(1<<1)) + static int pack_everything; static int delta_base_offset = 1; static int pack_kept_objects = -1; @@ -113,6 +116,11 @@ static int has_existing_non_kept_packs(const struct existing_packs *existing) return existing->non_kept_packs.nr || existing->cruft_packs.nr; } +static int pack_is_retained(struct string_list_item *item) +{ + return (uintptr_t)item->util & RETAIN_PACK; +} + static void mark_packs_for_deletion_1(struct string_list *names, struct string_list *list) { @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names, if (len < hexsz) continue; sha1 = item->string + len - hexsz; - /* - * Mark this pack for deletion, which ensures that this - * pack won't be included in a MIDX (if `--write-midx` - * was given) and that we will actually delete this pack - * (if `-d` was given). - */ - if (!string_list_has_string(names, sha1)) - item->util = (void*)1; + + if (pack_is_retained(item)) { + item->util = NULL; + } else if (!string_list_has_string(names, sha1)) { + /* + * Mark this pack for deletion, which ensures + * that this pack won't be included in a MIDX + * (if `--write-midx` was given) and that we + * will actually delete this pack (if `-d` was + * given). + */ + item->util = DELETE_PACK; + } } } +static void retain_cruft_pack(struct existing_packs *existing, + struct packed_git *cruft) +{ + struct strbuf buf = STRBUF_INIT; + struct string_list_item *item; + + strbuf_addstr(&buf, pack_basename(cruft)); + strbuf_strip_suffix(&buf, ".pack"); + + item = string_list_lookup(&existing->cruft_packs, buf.buf); + if (!item) + BUG("could not find cruft pack '%s'", pack_basename(cruft)); + + item->util = (void*)RETAIN_PACK; + strbuf_release(&buf); +} + static void mark_packs_for_deletion(struct existing_packs *existing, struct string_list *names) @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing, } string_list_sort(&existing->kept_packs); + string_list_sort(&existing->non_kept_packs); + string_list_sort(&existing->cruft_packs); strbuf_release(&buf); } @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include, strbuf_release(&path); } +static int existing_cruft_pack_cmp(const void *va, const void *vb) +{ + struct packed_git *a = *(struct packed_git **)va; + struct packed_git *b = *(struct packed_git **)vb; + + if (a->pack_size < b->pack_size) + return -1; + if (a->pack_size > b->pack_size) + return 1; + return 0; +} + +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size, + struct existing_packs *existing) +{ + struct packed_git **existing_cruft, *p; + struct strbuf buf = STRBUF_INIT; + unsigned long total_size = 0; + size_t existing_cruft_nr = 0; + size_t i; + + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); + + for (p = get_all_packs(the_repository); p; p = p->next) { + if (!(p->is_cruft && p->pack_local)) + continue; + + strbuf_reset(&buf); + strbuf_addstr(&buf, pack_basename(p)); + strbuf_strip_suffix(&buf, ".pack"); + + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) + continue; + + if (existing_cruft_nr >= existing->cruft_packs.nr) + BUG("too many cruft packs (found %"PRIuMAX", but knew " + "of %"PRIuMAX")", + (uintmax_t)existing_cruft_nr + 1, + (uintmax_t)existing->cruft_packs.nr); + existing_cruft[existing_cruft_nr++] = p; + } + + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); + + for (i = 0; i < existing_cruft_nr; i++) { + off_t proposed; + + p = existing_cruft[i]; + proposed = st_add(total_size, p->pack_size); + + if (proposed <= max_size) { + total_size = proposed; + fprintf(in, "-%s\n", pack_basename(p)); + } else { + retain_cruft_pack(existing, p); + fprintf(in, "%s\n", pack_basename(p)); + } + } + + for (i = 0; i < existing->non_kept_packs.nr; i++) + fprintf(in, "-%s.pack\n", + existing->non_kept_packs.items[i].string); + + strbuf_release(&buf); +} + static int write_cruft_pack(const struct pack_objects_args *args, const char *destination, const char *pack_prefix, @@ -846,10 +944,18 @@ static int write_cruft_pack(const struct pack_objects_args *args, in = xfdopen(cmd.in, "w"); for_each_string_list_item(item, names) fprintf(in, "%s-%s.pack\n", pack_prefix, item->string); - for_each_string_list_item(item, &existing->non_kept_packs) - fprintf(in, "-%s.pack\n", item->string); - for_each_string_list_item(item, &existing->cruft_packs) - fprintf(in, "-%s.pack\n", item->string); + if (args->max_pack_size && !cruft_expiration) { + unsigned long max_pack_size; + if (!git_parse_ulong(args->max_pack_size, &max_pack_size)) + return error(_("could not parse --cruft-max-size: '%s'"), + args->max_pack_size); + collapse_small_cruft_packs(in, max_pack_size, existing); + } else { + for_each_string_list_item(item, &existing->non_kept_packs) + fprintf(in, "-%s.pack\n", item->string); + for_each_string_list_item(item, &existing->cruft_packs) + fprintf(in, "-%s.pack\n", item->string); + } for_each_string_list_item(item, &existing->kept_packs) fprintf(in, "%s.pack\n", item->string); fclose(in); @@ -912,6 +1018,9 @@ int cmd_repack(int argc, const char **argv, const char *prefix) PACK_CRUFT), OPT_STRING(0, "cruft-expiration", &cruft_expiration, N_("approxidate"), N_("with --cruft, expire objects older than this")), + OPT_STRING(0, "cruft-max-size", &cruft_po_args.max_pack_size, + N_("bytes"), + N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL('d', NULL, &delete_redundant, N_("remove redundant packs, and run git-prune-packed")), OPT_BOOL('f', NULL, &po_args.no_reuse_delta, diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 69509d0c11..5e737c47be 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -303,6 +303,33 @@ test_expect_success 'gc.bigPackThreshold ignores cruft packs' ' ) ' +cruft_max_size_opts="git repack -d -l --cruft --cruft-expiration=2.weeks.ago" + +test_expect_success 'setup for --cruft-max-size tests' ' + git init cruft--max-size && + ( + cd cruft--max-size && + prepare_cruft_history + ) +' + +test_expect_success '--cruft-max-size sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \ + gc --cruft --cruft-max-size=1M && + test_subcommand $cruft_max_size_opts --cruft-max-size=1M <trace2.txt +' + +test_expect_success 'gc.cruftMaxSize sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ + git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft && + test_subcommand $cruft_max_size_opts --cruft-max-size=2M <trace2.txt && + + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ + git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft \ + --cruft-max-size=3M && + test_subcommand $cruft_max_size_opts --cruft-max-size=3M <trace2.txt +' + run_and_wait_for_auto_gc () { # We read stdout from gc for the side effect of waiting until the # background gc process exits, closing its fd 9. Furthermore, the diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index d91fcf1af1..e0de09b77b 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -5,6 +5,7 @@ test_description='git repack works correctly' . ./test-lib.sh objdir=.git/objects +packdir=$objdir/pack test_expect_success '--expire-to stores pruned objects (now)' ' git init expire-to-now && @@ -127,4 +128,248 @@ test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' ) ' +generate_random_blob() { + test-tool genrandom "$@" >blob && + git hash-object -w -t blob blob && + rm blob +} + +pack_random_blob () { + generate_random_blob "$@" && + git repack -d -q >/dev/null +} + +generate_cruft_pack () { + pack_random_blob "$@" >/dev/null && + + ls $packdir/pack-*.pack | xargs -n 1 basename >in && + pack="$(git pack-objects --cruft $packdir/pack <in)" && + git prune-packed && + + echo "$packdir/pack-$pack.mtimes" +} + +test_expect_success '--cruft-max-size creates new packs when above threshold' ' + git init cruft-max-size-large && + ( + cd cruft-max-size-large && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && + git repack --cruft -d && + cruft_foo="$(ls $packdir/pack-*.mtimes)" && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && + git repack --cruft -d --cruft-max-size=1M && + cruft_bar="$(ls $packdir/pack-*.mtimes | grep -v $cruft_foo)" && + + test-tool pack-mtimes $(basename "$cruft_foo") >foo.objects && + test-tool pack-mtimes $(basename "$cruft_bar") >bar.objects && + + grep "^$foo" foo.objects && + test_line_count = 1 foo.objects && + grep "^$bar" bar.objects && + test_line_count = 1 bar.objects + ) +' + +test_expect_success '--cruft-max-size combines existing packs when below threshold' ' + git init cruft-max-size-small && + ( + cd cruft-max-size-small && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && + git repack --cruft -d && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && + git repack --cruft -d --cruft-max-size=10M && + + cruft=$(ls $packdir/pack-*.mtimes) && + test-tool pack-mtimes $(basename "$cruft") >cruft.objects && + + grep "^$foo" cruft.objects && + grep "^$bar" cruft.objects && + test_line_count = 2 cruft.objects + ) +' + +test_expect_success '--cruft-max-size combines smaller packs first' ' + git init cruft-max-size-consume-small && + ( + cd cruft-max-size-consume-small && + + test_commit base && + git repack -ad && + + cruft_foo="$(generate_cruft_pack foo 524288)" && # 0.5 MiB + cruft_bar="$(generate_cruft_pack bar 524288)" && # 0.5 MiB + cruft_baz="$(generate_cruft_pack baz 1048576)" && # 1.0 MiB + cruft_quux="$(generate_cruft_pack quux 1572864)" && # 1.5 MiB + + test-tool pack-mtimes "$(basename $cruft_foo)" >expect.raw && + test-tool pack-mtimes "$(basename $cruft_bar)" >>expect.raw && + sort expect.raw >expect.objects && + + # repacking with `--cruft-max-size=2M` should combine + # both 0.5 MiB packs together, instead of, say, one of + # the 0.5 MiB packs with the 1.0 MiB pack + ls $packdir/pack-*.mtimes | sort >cruft.before && + git repack -d --cruft --cruft-max-size=2M && + ls $packdir/pack-*.mtimes | sort >cruft.after && + + comm -13 cruft.before cruft.after >cruft.new && + comm -23 cruft.before cruft.after >cruft.removed && + + test_line_count = 1 cruft.new && + test_line_count = 2 cruft.removed && + + # the two smaller packs should be rolled up first + printf "%s\n" $cruft_foo $cruft_bar | sort >expect.removed && + test_cmp expect.removed cruft.removed && + + # ...and contain the set of objects rolled up + test-tool pack-mtimes "$(basename $(cat cruft.new))" >actual.raw && + sort actual.raw >actual.objects && + + test_cmp expect.objects actual.objects + ) +' + +test_expect_success 'setup --cruft-max-size with freshened objects' ' + git init cruft-max-size-freshen && + ( + cd cruft-max-size-freshen && + + test_commit base && + git repack -ad && + + foo="$(generate_random_blob foo 64)" && + test-tool chmtime --get -10000 \ + "$objdir/$(test_oid_to_path "$foo")" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect && + test_cmp expect actual + ) +' + +test_expect_success '--cruft-max-size with freshened objects (loose)' ' + ( + cd cruft-max-size-freshen && + + # regenerate the object, setting its mtime to be more recent + foo="$(generate_random_blob foo 64)" && + test-tool chmtime --get -100 \ + "$objdir/$(test_oid_to_path "$foo")" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect && + test_cmp expect actual + ) +' + +test_expect_success '--cruft-max-size with freshened objects (packed)' ' + ( + cd cruft-max-size-freshen && + + # regenerate the object and store it in a packfile, + # setting its mtime to be more recent + # + # store it alongside another cruft object so that we + # do not create an identical copy of the existing + # cruft pack (which contains $foo). + foo="$(generate_random_blob foo 64)" && + bar="$(generate_random_blob bar 64)" && + foo_pack="$(printf "%s\n" $foo $bar | git pack-objects $packdir/pack)" && + git prune-packed && + + test-tool chmtime --get -10 \ + "$packdir/pack-$foo_pack.pack" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect.raw && + echo "$bar $(cat foo.mtime)" >>expect.raw && + sort expect.raw >expect && + test_cmp expect actual + ) +' + +test_expect_success '--cruft-max-size with pruning' ' + git init cruft-max-size-prune && + ( + cd cruft-max-size-prune && + + test_commit base && + foo="$(generate_random_blob foo $((1024*1024)))" && + bar="$(generate_random_blob bar $((1024*1024)))" && + baz="$(generate_random_blob baz $((1024*1024)))" && + + test-tool chmtime -10000 "$objdir/$(test_oid_to_path "$foo")" && + + git repack -d --cruft --cruft-max-size=1M && + + # backdate the mtimes of all cruft packs to validate + # that they were rewritten as a result of pruning + ls $packdir/pack-*.mtimes | sort >cruft.before && + for cruft in $(cat cruft.before) + do + mtime="$(test-tool chmtime --get -10000 "$cruft")" && + echo $cruft $mtime >>mtimes || return 1 + done && + + # repack (and prune) with a --cruft-max-size to ensure + # that we appropriately split the resulting set of packs + git repack -d --cruft --cruft-max-size=1M \ + --cruft-expiration=10.seconds.ago && + ls $packdir/pack-*.mtimes | sort >cruft.after && + + for cruft in $(cat cruft.after) + do + old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" && + new_mtime="$(test-tool chmtime --get $cruft)" && + test $old_mtime -lt $new_mtime || return 1 + done && + + test_line_count = 3 cruft.before && + test_line_count = 2 cruft.after && + test_must_fail git cat-file -e $foo && + git cat-file -e $bar && + git cat-file -e $baz + ) +' + +test_expect_success '--cruft-max-size ignores non-local packs' ' + repo="cruft-max-size-non-local" && + git init $repo && + ( + cd $repo && + test_commit base && + generate_random_blob foo 64 && + git repack --cruft -d + ) && + + git clone --reference=$repo $repo $repo-alt && + ( + cd $repo-alt && + + test_commit other && + generate_random_blob bar 64 && + + # ensure that we do not attempt to pick up packs from + # the non-alternated repository, which would result in a + # crash + git repack --cruft --cruft-max-size=1M -d + ) +' + test_done -- 2.42.0.138.g7e4e42e1aa ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` 2023-09-07 21:52 ` [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` Taylor Blau @ 2023-09-07 23:42 ` Junio C Hamano 2023-09-25 18:01 ` Taylor Blau 2023-09-08 11:21 ` Patrick Steinhardt 1 sibling, 1 reply; 21+ messages in thread From: Junio C Hamano @ 2023-09-07 23:42 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Jeff King, Jonathan Tan Taylor Blau <me@ttaylorr.com> writes: > This all works, but can be costly from an I/O-perspective when a > repository has either (a) many unreachable objects, (b) prunes objects > relatively infrequently/never, or (c) both. I can certainly understand (a). If we need to write a lot of objects into the craft pack, among which many of them are already in the previous generation of craft pack, we would be copying bits from the old to the new craft pack, and having to do so for many objects would involve expensive I/O. But not (b). Whether we prune objects infrequently or we do so very often, as long as the number and on-disk size of objects that has to go into craft packs are the same, wouldn't the cost of I/O pretty much the same? IOW, (b) does not have much to do with how repacking is costly I/O wise, except that it is a contributing factor to make (a) worse, which is the real cause of the I/O cost. > At the time, we decided against implementing more robust support for > multiple cruft packs. This patch implements that support which we were > lacking. > > Introduce a new option `--cruft-max-size` which allows repositories to > accumulate cruft packs up to a given size, after which point a new > generation of cruft packs can accumulate until it reaches the maximum > size, and so on. To generate a new cruft pack, the process works like > so: > > - Sort a list of any existing cruft packs in ascending order of pack > size. > > - Starting from the beginning of the list, group cruft packs together > while the accumulated size is smaller than the maximum specified > pack size. > > - Combine the objects in these cruft packs together into a new cruft > pack, along with any other unreachable objects which have since > entered the repository. The above three steps guarantees that, before the amount of the accumulated cruft objects reaches the "max-size", we would be moving bits from old to new cruft packs and incurring the same I/O cost as in the current system. I suspect (I haven't read what the new code does) the untold fourth point is what improves the new system over the current one, which would be "Do not touch the existing cruft pack(s) that are already large enough, and send enumerated cruft objects that do not appear in these existing cruft pack(s) to a new cruft pack. Do not rewrite these existing cruft pack(s), even if there are some cruft objects that has aged enough, because rewriting these huge cruft packs only to expunge a few objects from them is costly in I/O" or something? In any case, I do not think with only the above three ... > This limits the I/O churn up to a quadratic function of the value > specified by the `--cruft-max-size` option, instead of behaving > quadratically in the number of total unreachable objects. ... I do not quite see how you would limit the I/O churn. > When pruning unreachable objects, we bypass the new paths which combine "paths" here refers to...? code paths, I guess? > +gc.cruftMaxSize:: > + Limit the size of new cruft packs when repacking. When > + specified in addition to `--cruft-max-size`, the command line > + option takes priority. See the `--cruft-max-size` option of > + linkgit:git-repack[1]. Hmph. I am reasonably sure that I will mix the name up and call it gc.maxCruftSize in my configuration file and scratch my head wondering why it is not working. > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt > index 90806fd26a..8a90d684a7 100644 > --- a/Documentation/git-gc.txt > +++ b/Documentation/git-gc.txt > @@ -59,6 +59,13 @@ be performed as well. > cruft pack instead of storing them as loose objects. `--cruft` > is on by default. > > +--cruft-max-size=<n>:: > + When packing unreachable objects into a cruft pack, limit the > + size of new cruft packs to be at most `<n>`. Overrides any > + value specified via the `gc.cruftMaxSize` configuration. See > + the `--cruft-max-size` option of linkgit:git-repack[1] for > + more. At least this side giving --max-cruft-size=<n> (which I think is a lot more natural word order) would cause parse-options to give an error, so it won't risk mistakes go silently unnoticed. > diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt > index 4017157949..23fd203d79 100644 > --- a/Documentation/git-repack.txt > +++ b/Documentation/git-repack.txt > @@ -74,6 +74,15 @@ to the new separate pack will be written. > immediately instead of waiting for the next `git gc` invocation. > Only useful with `--cruft -d`. > > +--cruft-max-size=<n>:: > + Repack cruft objects into packs as large as `<n>` before > + creating new packs. As long as there are enough cruft packs > + smaller than `<n>`, repacking will cause a new cruft pack to > + be created containing objects from any combined cruft packs, > + along with any new unreachable objects. Cruft packs larger > + than `<n>` will not be modified. Only useful with `--cruft > + -d`. Here, the missing fourth point I pointed out above is mentioned, which is good. Describe the unit for <n> (I am assuming that is counted in bytes, honoring the human-friendly suffix, like 100M). There may be some "interesting" behaviour around the size boundary, no? If you pack too many objects, your resulting size may slightly bust <n> and you will get a complaint, but by fixing that "bug", you will always stop short of filling the whole <n> bytes in the produced packfiles, and they will not be excempt from rewriting (becuase they are not "larger than <n>"), which defeats the point of this patch. Describe that <n> is a threshold that we stop soon after passing to explicitly allow us to go beyond it would solve the above problem, I would presume. > diff --git a/builtin/gc.c b/builtin/gc.c > index 1f53b66c7b..b6640abd35 100644 > --- a/builtin/gc.c > +++ b/builtin/gc.c > @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = { > static int pack_refs = 1; > static int prune_reflogs = 1; > static int cruft_packs = 1; > +static char *cruft_max_size; I do not think this type is a good idea. > static int aggressive_depth = 50; > static int aggressive_window = 250; > static int gc_auto_threshold = 6700; > @@ -163,6 +164,7 @@ static void gc_config(void) > git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); > git_config_get_bool("gc.autodetach", &detach_auto); > git_config_get_bool("gc.cruftpacks", &cruft_packs); > + git_config_get_string("gc.cruftmaxsize", &cruft_max_size); > git_config_get_expiry("gc.pruneexpire", &prune_expire); > git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire); > git_config_get_expiry("gc.logexpiry", &gc_log_expire); > @@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack) > strvec_push(&repack, "--cruft"); > if (prune_expire) > strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire); > + if (cruft_max_size) > + strvec_pushf(&repack, "--cruft-max-size=%s", > + cruft_max_size); Even if you are going to pass it intact, as an opaque token, to another program. Rather, you should parse and make sure it is a valid non-negative byte count before calling out the other program (passing the original string down, after parsing it only for error checking the value, and having "repack" do its own parsing, is perfectly fine). > diff --git a/builtin/repack.c b/builtin/repack.c > index 44cb261371..56e7f5f43d 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -26,6 +26,9 @@ > #define LOOSEN_UNREACHABLE 2 > #define PACK_CRUFT 4 > > +#define DELETE_PACK ((void*)(uintptr_t)1) > +#define RETAIN_PACK ((uintptr_t)(1<<1)) Shouldn't these look more similar? That is ((void *)(uintptr_t)(1<<0)) ((void *)(uintptr_t)(1<<1)) > + if (pack_is_retained(item)) { > + item->util = NULL; > + } else if (!string_list_has_string(names, sha1)) { Without the pack_marked_for_deletion(item) macro Patrick suggested, the logic around here looks very uneven. > + /* > + * Mark this pack for deletion, which ensures > + * that this pack won't be included in a MIDX > + * (if `--write-midx` was given) and that we > + * will actually delete this pack (if `-d` was > + * given). > + */ > + item->util = DELETE_PACK; > + } > } > } > > +static void retain_cruft_pack(struct existing_packs *existing, > + struct packed_git *cruft) > +{ > + struct strbuf buf = STRBUF_INIT; > + struct string_list_item *item; > + > + strbuf_addstr(&buf, pack_basename(cruft)); > + strbuf_strip_suffix(&buf, ".pack"); > + > + item = string_list_lookup(&existing->cruft_packs, buf.buf); > + if (!item) > + BUG("could not find cruft pack '%s'", pack_basename(cruft)); > + > + item->util = (void*)RETAIN_PACK; > + strbuf_release(&buf); > +} > + > static void mark_packs_for_deletion(struct existing_packs *existing, > struct string_list *names) > > @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing, > } > > string_list_sort(&existing->kept_packs); > + string_list_sort(&existing->non_kept_packs); > + string_list_sort(&existing->cruft_packs); > strbuf_release(&buf); > } > > @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include, > strbuf_release(&path); > } > > +static int existing_cruft_pack_cmp(const void *va, const void *vb) > +{ > + struct packed_git *a = *(struct packed_git **)va; > + struct packed_git *b = *(struct packed_git **)vb; > + > + if (a->pack_size < b->pack_size) > + return -1; > + if (a->pack_size > b->pack_size) > + return 1; > + return 0; > +} > + > +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size, > + struct existing_packs *existing) > +{ > + struct packed_git **existing_cruft, *p; > + struct strbuf buf = STRBUF_INIT; > + unsigned long total_size = 0; > + size_t existing_cruft_nr = 0; > + size_t i; > + > + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); > + > + for (p = get_all_packs(the_repository); p; p = p->next) { > + if (!(p->is_cruft && p->pack_local)) > + continue; > + > + strbuf_reset(&buf); > + strbuf_addstr(&buf, pack_basename(p)); > + strbuf_strip_suffix(&buf, ".pack"); > + > + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) > + continue; > + > + if (existing_cruft_nr >= existing->cruft_packs.nr) > + BUG("too many cruft packs (found %"PRIuMAX", but knew " > + "of %"PRIuMAX")", Is that a BUG() that somehow miscounted the packs, or can it be a runtime error that may happen when a "git push" is pushing new objects into the repository, creating a new pack we did not know about? Something like the latter should not be marked a BUG(), but > + (uintmax_t)existing_cruft_nr + 1, > + (uintmax_t)existing->cruft_packs.nr); > + existing_cruft[existing_cruft_nr++] = p; > + } > + > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); We use the simplest "from smaller ones to larger ones, combine one by one together until the result gets large enough", which would not give us the best packing, but it is OK because it is not our goal to solve knapsack problem here, I presume? > + for (i = 0; i < existing_cruft_nr; i++) { > + off_t proposed; > + > + p = existing_cruft[i]; > + proposed = st_add(total_size, p->pack_size); > + > + if (proposed <= max_size) { > + total_size = proposed; > + fprintf(in, "-%s\n", pack_basename(p)); > + } else { > + retain_cruft_pack(existing, p); > + fprintf(in, "%s\n", pack_basename(p)); > + } > + } This is exactly what I talked about the possibly funny behaviour around the boundary earlier, but it may be even worse. This time, we may decide that a pack with size <n-epsilon> is to be retained, only because the pack that came next in the existing list happened to be larger than epsilon, but next time around, it may not be the case (i.e. our pack may be smaller than epsilon, the next one in the current round may be larger than epsilon, but before we repack the next time, a new pack that is slightly smaller than epsilon that is larger than our pack may have been created---now our pack will be combined with it), so the algorithm to choose which ones are kept does not depend on the pack itself alone but also depends on its surroundings. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` 2023-09-07 23:42 ` Junio C Hamano @ 2023-09-25 18:01 ` Taylor Blau 0 siblings, 0 replies; 21+ messages in thread From: Taylor Blau @ 2023-09-25 18:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: git, Jeff King, Jonathan Tan On Thu, Sep 07, 2023 at 04:42:48PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > This all works, but can be costly from an I/O-perspective when a > > repository has either (a) many unreachable objects, (b) prunes objects > > relatively infrequently/never, or (c) both. > > I can certainly understand (a). If we need to write a lot of > objects into the craft pack, among which many of them are already in > the previous generation of craft pack, we would be copying bits from > the old to the new craft pack, and having to do so for many objects > would involve expensive I/O. But not (b). Whether we prune objects > infrequently or we do so very often, as long as the number and > on-disk size of objects that has to go into craft packs are the > same, wouldn't the cost of I/O pretty much the same? IOW, (b) does > not have much to do with how repacking is costly I/O wise, except > that it is a contributing factor to make (a) worse, which is the > real cause of the I/O cost. Yeah, (b) on its own isn't enough to cost us a significant amount of I/O overhead. (b) definitely exacerbates (a) if we are repacking relatively frequently. I'll clarify in the patch notes. > > This limits the I/O churn up to a quadratic function of the value > > specified by the `--cruft-max-size` option, instead of behaving > > quadratically in the number of total unreachable objects. > > ... I do not quite see how you would limit the I/O churn. In the above (elided here for brevity) response, you are correct: the idea is that once a cruft pack has grown to whatever threshold you set via `--cruft-max-size`, it is effectively frozen, preventing you from incurring the same I/O churn from it in the future. > > When pruning unreachable objects, we bypass the new paths which combine > > "paths" here refers to...? code paths, I guess? Yep. > > +gc.cruftMaxSize:: > > + Limit the size of new cruft packs when repacking. When > > + specified in addition to `--cruft-max-size`, the command line > > + option takes priority. See the `--cruft-max-size` option of > > + linkgit:git-repack[1]. > > Hmph. > > I am reasonably sure that I will mix the name up and call it > gc.maxCruftSize in my configuration file and scratch my head > wondering why it is not working. I have no strong preference for either "gc.cruftMaxSize" or "gc.maxCruftSize" (or anything else, really), so long as we are consistent with the command-line option. > > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt > > index 90806fd26a..8a90d684a7 100644 > > --- a/Documentation/git-gc.txt > > +++ b/Documentation/git-gc.txt > > @@ -59,6 +59,13 @@ be performed as well. > > cruft pack instead of storing them as loose objects. `--cruft` > > is on by default. > > > > +--cruft-max-size=<n>:: > > + When packing unreachable objects into a cruft pack, limit the > > + size of new cruft packs to be at most `<n>`. Overrides any > > + value specified via the `gc.cruftMaxSize` configuration. See > > + the `--cruft-max-size` option of linkgit:git-repack[1] for > > + more. > > At least this side giving --max-cruft-size=<n> (which I think is a > lot more natural word order) would cause parse-options to give an > error, so it won't risk mistakes go silently unnoticed. Yeah, that's compelling. I'm convinced ;-). > > diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt > > index 4017157949..23fd203d79 100644 > > --- a/Documentation/git-repack.txt > > +++ b/Documentation/git-repack.txt > > @@ -74,6 +74,15 @@ to the new separate pack will be written. > > immediately instead of waiting for the next `git gc` invocation. > > Only useful with `--cruft -d`. > > > > +--cruft-max-size=<n>:: > > + Repack cruft objects into packs as large as `<n>` before > > + creating new packs. As long as there are enough cruft packs > > + smaller than `<n>`, repacking will cause a new cruft pack to > > + be created containing objects from any combined cruft packs, > > + along with any new unreachable objects. Cruft packs larger > > + than `<n>` will not be modified. Only useful with `--cruft > > + -d`. > > Here, the missing fourth point I pointed out above is mentioned, > which is good. > > Describe the unit for <n> (I am assuming that is counted in bytes, > honoring the human-friendly suffix, like 100M). Yep, thanks. > There may be some "interesting" behaviour around the size boundary, > no? If you pack too many objects, your resulting size may slightly > bust <n> and you will get a complaint, but by fixing that "bug", you > will always stop short of filling the whole <n> bytes in the > produced packfiles, and they will not be excempt from rewriting > (becuase they are not "larger than <n>"), which defeats the point of > this patch. Yeah, the boundary conditions are definitely the most interesting part of this patch IMHO. When packing with `--max-cruft-size`, we still do cap the pack size of the resulting pack, so any excess spills over into the next pack. > Describe that <n> is a threshold that we stop soon after passing to > explicitly allow us to go beyond it would solve the above problem, I > would presume. I am not sure I understand what you're getting at here. > > diff --git a/builtin/gc.c b/builtin/gc.c > > index 1f53b66c7b..b6640abd35 100644 > > --- a/builtin/gc.c > > +++ b/builtin/gc.c > > @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = { > > static int pack_refs = 1; > > static int prune_reflogs = 1; > > static int cruft_packs = 1; > > +static char *cruft_max_size; > > I do not think this type is a good idea. Yeah, I marked this as a string because we don't ourselves do anything with it in 'gc', and instead just immediately pass it down to 'repack'. We could parse it ourselves and catch any malformed arguments earlier, though, which sounds worthwhile to me. > > diff --git a/builtin/repack.c b/builtin/repack.c > > index 44cb261371..56e7f5f43d 100644 > > --- a/builtin/repack.c > > +++ b/builtin/repack.c > > @@ -26,6 +26,9 @@ > > #define LOOSEN_UNREACHABLE 2 > > #define PACK_CRUFT 4 > > > > +#define DELETE_PACK ((void*)(uintptr_t)1) > > +#define RETAIN_PACK ((uintptr_t)(1<<1)) > > Shouldn't these look more similar? That is > > ((void *)(uintptr_t)(1<<0)) > ((void *)(uintptr_t)(1<<1)) Yeah, these have been shored up from some helpful review on the repack internals cleanup series I posted earlier. > > + if (existing_cruft_nr >= existing->cruft_packs.nr) > > + BUG("too many cruft packs (found %"PRIuMAX", but knew " > > + "of %"PRIuMAX")", > > Is that a BUG() that somehow miscounted the packs, or can it be a > runtime error that may happen when a "git push" is pushing new > objects into the repository, creating a new pack we did not know > about? Something like the latter should not be marked a BUG(), but This would be the former. We load the set of packs at the beginning of a repack operation from collect_pack_filenames() via a call to get_all_packs(). So the set won't change between our view of it in collect_pack_filenames() and collapse_small_cruft_packs(). IOW, we should expect to see as many cruft packs as we saw in collect_pack_filenames(), and any difference there would indeed be a BUG(). > > + (uintmax_t)existing_cruft_nr + 1, > > + (uintmax_t)existing->cruft_packs.nr); > > + existing_cruft[existing_cruft_nr++] = p; > > + } > > + > > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); > > We use the simplest "from smaller ones to larger ones, combine one > by one together until the result gets large enough", which would not > give us the best packing, but it is OK because it is not our goal to > solve knapsack problem here, I presume? Exactly. Whatever spill-over we generate (if any) will be the seed of the next "generation" of cruft packs, which are allowed to grow and accumulate until they themselves reach the size threshold, at which point the process starts itself over again. > > + for (i = 0; i < existing_cruft_nr; i++) { > > + off_t proposed; > > + > > + p = existing_cruft[i]; > > + proposed = st_add(total_size, p->pack_size); > > + > > + if (proposed <= max_size) { > > + total_size = proposed; > > + fprintf(in, "-%s\n", pack_basename(p)); > > + } else { > > + retain_cruft_pack(existing, p); > > + fprintf(in, "%s\n", pack_basename(p)); > > + } > > + } > > This is exactly what I talked about the possibly funny behaviour > around the boundary earlier, but it may be even worse. This time, > we may decide that a pack with size <n-epsilon> is to be retained, > only because the pack that came next in the existing list happened > to be larger than epsilon, but next time around, it may not be the > case (i.e. our pack may be smaller than epsilon, the next one in the > current round may be larger than epsilon, but before we repack the > next time, a new pack that is slightly smaller than epsilon that is > larger than our pack may have been created---now our pack will be > combined with it), so the algorithm to choose which ones are kept > does not depend on the pack itself alone but also depends on its > surroundings. If I understand your comment correctly, that behavior is as-designed. We try to grow cruft packs by combining other cruft packs that are not yet frozen, and I think that is going to be dependent on the entire set of packs rather than the characteristic of any one pack. An alternative approach might be to combine *all* cruft packs smaller than some threshold, and let any spill-over get handled by pack-objects with its --max-pack-size option. I do not have a strong preference between the two. Thanks, Taylor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` 2023-09-07 21:52 ` [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` Taylor Blau 2023-09-07 23:42 ` Junio C Hamano @ 2023-09-08 11:21 ` Patrick Steinhardt 2023-10-02 20:30 ` Taylor Blau 1 sibling, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2023-09-08 11:21 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Junio C Hamano, Jeff King, Jonathan Tan [-- Attachment #1: Type: text/plain, Size: 6905 bytes --] On Thu, Sep 07, 2023 at 05:52:04PM -0400, Taylor Blau wrote: [snip] > @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names, > if (len < hexsz) > continue; > sha1 = item->string + len - hexsz; > - /* > - * Mark this pack for deletion, which ensures that this > - * pack won't be included in a MIDX (if `--write-midx` > - * was given) and that we will actually delete this pack > - * (if `-d` was given). > - */ > - if (!string_list_has_string(names, sha1)) > - item->util = (void*)1; > + > + if (pack_is_retained(item)) { > + item->util = NULL; > + } else if (!string_list_has_string(names, sha1)) { > + /* > + * Mark this pack for deletion, which ensures > + * that this pack won't be included in a MIDX > + * (if `--write-midx` was given) and that we > + * will actually delete this pack (if `-d` was > + * given). > + */ > + item->util = DELETE_PACK; > + } I find the behaviour of this function a tad surprising as it doesn't only mark a pack for deletion, but it also marks a pack as not being retained anymore. Shouldn't we rather: if (pack_is_retained(item)) { // Theoretically speaking we shouldn't even do this bit here as // we _un_mark the pack for deletion. But at least we shouldn't // be removing the `RETAIN_PACK` bit, I'd think. item->util &= ~DELETE_PACK; } else if (!string_list_has_string(names, sha1)) { // And here we shouldn't discard the `RETAIN_PACK` bit either. item->util |= DELETE_PACK; } > } > } > > +static void retain_cruft_pack(struct existing_packs *existing, > + struct packed_git *cruft) > +{ > + struct strbuf buf = STRBUF_INIT; > + struct string_list_item *item; > + > + strbuf_addstr(&buf, pack_basename(cruft)); > + strbuf_strip_suffix(&buf, ".pack"); > + > + item = string_list_lookup(&existing->cruft_packs, buf.buf); > + if (!item) > + BUG("could not find cruft pack '%s'", pack_basename(cruft)); > + > + item->util = (void*)RETAIN_PACK; > + strbuf_release(&buf); > +} > + Similarly, should we handle potentially pre-existing bits gracefully and `item->util |= RETAIN_PACK`? > static void mark_packs_for_deletion(struct existing_packs *existing, > struct string_list *names) > > @@ -217,6 +247,8 @@ static void collect_pack_filenames(struct existing_packs *existing, > } > > string_list_sort(&existing->kept_packs); > + string_list_sort(&existing->non_kept_packs); > + string_list_sort(&existing->cruft_packs); > strbuf_release(&buf); > } > > @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include, > strbuf_release(&path); > } > > +static int existing_cruft_pack_cmp(const void *va, const void *vb) > +{ > + struct packed_git *a = *(struct packed_git **)va; > + struct packed_git *b = *(struct packed_git **)vb; > + > + if (a->pack_size < b->pack_size) > + return -1; > + if (a->pack_size > b->pack_size) > + return 1; > + return 0; > +} > + > +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size, We might want to use `size_t` to denote file sizes instead of `unsigned long`. > + struct existing_packs *existing) > +{ > + struct packed_git **existing_cruft, *p; > + struct strbuf buf = STRBUF_INIT; > + unsigned long total_size = 0; Here, as well. > + size_t existing_cruft_nr = 0; > + size_t i; > + > + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); > + > + for (p = get_all_packs(the_repository); p; p = p->next) { > + if (!(p->is_cruft && p->pack_local)) > + continue; > + > + strbuf_reset(&buf); > + strbuf_addstr(&buf, pack_basename(p)); > + strbuf_strip_suffix(&buf, ".pack"); > + > + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) > + continue; > + > + if (existing_cruft_nr >= existing->cruft_packs.nr) > + BUG("too many cruft packs (found %"PRIuMAX", but knew " > + "of %"PRIuMAX")", > + (uintmax_t)existing_cruft_nr + 1, > + (uintmax_t)existing->cruft_packs.nr); > + existing_cruft[existing_cruft_nr++] = p; > + } > + > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); > + > + for (i = 0; i < existing_cruft_nr; i++) { > + off_t proposed; This should also be `size_t` given that `st_add` returns `size_t` and not `off_t`. > + p = existing_cruft[i]; > + proposed = st_add(total_size, p->pack_size); > + > + if (proposed <= max_size) { > + total_size = proposed; > + fprintf(in, "-%s\n", pack_basename(p)); > + } else { > + retain_cruft_pack(existing, p); > + fprintf(in, "%s\n", pack_basename(p)); > + } It's a bit funny that we re-check whether we have exceeded the maximum size in subsequente iterations once we hit the limit, but it arguably makes the logic a bit simpler. > + } > + > + for (i = 0; i < existing->non_kept_packs.nr; i++) > + fprintf(in, "-%s.pack\n", > + existing->non_kept_packs.items[i].string); > + > + strbuf_release(&buf); > +} > + > static int write_cruft_pack(const struct pack_objects_args *args, > const char *destination, > const char *pack_prefix, > @@ -846,10 +944,18 @@ static int write_cruft_pack(const struct pack_objects_args *args, > in = xfdopen(cmd.in, "w"); > for_each_string_list_item(item, names) > fprintf(in, "%s-%s.pack\n", pack_prefix, item->string); > - for_each_string_list_item(item, &existing->non_kept_packs) > - fprintf(in, "-%s.pack\n", item->string); > - for_each_string_list_item(item, &existing->cruft_packs) > - fprintf(in, "-%s.pack\n", item->string); > + if (args->max_pack_size && !cruft_expiration) { > + unsigned long max_pack_size; > + if (!git_parse_ulong(args->max_pack_size, &max_pack_size)) > + return error(_("could not parse --cruft-max-size: '%s'"), > + args->max_pack_size); > + collapse_small_cruft_packs(in, max_pack_size, existing); > + } else { > + for_each_string_list_item(item, &existing->non_kept_packs) > + fprintf(in, "-%s.pack\n", item->string); > + for_each_string_list_item(item, &existing->cruft_packs) > + fprintf(in, "-%s.pack\n", item->string); > + } If I understand correctly, we only collapse small cruft packs in case we're not expiring any objects at the same time. Is there an inherent reason why? I would imagine that it can indeed be useful to expire objects contained in cruft packs and then have git-repack(1) recombine whatever is left into larger packs. If the reason is basically "it's complicated" then that is fine with me, we can still implement the functionality at a later point in time. But until then I think that we should let callers know that the two options are incompatible with each other by producing an error when both are passed. Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` 2023-09-08 11:21 ` Patrick Steinhardt @ 2023-10-02 20:30 ` Taylor Blau 0 siblings, 0 replies; 21+ messages in thread From: Taylor Blau @ 2023-10-02 20:30 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Junio C Hamano, Jeff King, Jonathan Tan On Fri, Sep 08, 2023 at 01:21:44PM +0200, Patrick Steinhardt wrote: > On Thu, Sep 07, 2023 at 05:52:04PM -0400, Taylor Blau wrote: > [snip] > > @@ -125,17 +133,39 @@ static void mark_packs_for_deletion_1(struct string_list *names, > > if (len < hexsz) > > continue; > > sha1 = item->string + len - hexsz; > > - /* > > - * Mark this pack for deletion, which ensures that this > > - * pack won't be included in a MIDX (if `--write-midx` > > - * was given) and that we will actually delete this pack > > - * (if `-d` was given). > > - */ > > - if (!string_list_has_string(names, sha1)) > > - item->util = (void*)1; > > + > > + if (pack_is_retained(item)) { > > + item->util = NULL; > > + } else if (!string_list_has_string(names, sha1)) { > > + /* > > + * Mark this pack for deletion, which ensures > > + * that this pack won't be included in a MIDX > > + * (if `--write-midx` was given) and that we > > + * will actually delete this pack (if `-d` was > > + * given). > > + */ > > + item->util = DELETE_PACK; > > + } > > I find the behaviour of this function a tad surprising as it doesn't > only mark a pack for deletion, but it also marks a pack as not being > retained anymore. Shouldn't we rather: > > if (pack_is_retained(item)) { > // Theoretically speaking we shouldn't even do this bit here as > // we _un_mark the pack for deletion. But at least we shouldn't > // be removing the `RETAIN_PACK` bit, I'd think. > item->util &= ~DELETE_PACK; > } else if (!string_list_has_string(names, sha1)) { > // And here we shouldn't discard the `RETAIN_PACK` bit either. > item->util |= DELETE_PACK; > } I think the new version should address these issues. But yeah, I definitely understand your confusion here. I think what's written in this patch is OK, because we check only whether the `->util` field is non-NULL before deleting, which is why we have to remove the RETAINED bit. But the new version looks like this instead: if (pack_is_retained(item)) pack_unmark_for_deletion(item); else if (!string_list_has_string(names, sha1)) pack_mark_for_deletion(item); the RETAINED bits still stick around (pack_unmark_for_deletion() just does `item->util &= ~DELETE_PACK`), but we don't consult them after mark_packs_for_deletion_1() has finished executing. Instead we just check for the existence of the DELETE_PACK bit, rather than whether or not the whole util field is NULL. > > @@ -799,6 +831,72 @@ static void remove_redundant_bitmaps(struct string_list *include, > > strbuf_release(&path); > > } > > > > +static int existing_cruft_pack_cmp(const void *va, const void *vb) > > +{ > > + struct packed_git *a = *(struct packed_git **)va; > > + struct packed_git *b = *(struct packed_git **)vb; > > + > > + if (a->pack_size < b->pack_size) > > + return -1; > > + if (a->pack_size > b->pack_size) > > + return 1; > > + return 0; > > +} > > + > > +static void collapse_small_cruft_packs(FILE *in, unsigned long max_size, > > We might want to use `size_t` to denote file sizes instead of `unsigned > long`. We can safely change these to use size_t, but let's leave OPT_MAGNITUDE alone (and treat that portion as #leftoverbits). > > + p = existing_cruft[i]; > > + proposed = st_add(total_size, p->pack_size); > > + > > + if (proposed <= max_size) { > > + total_size = proposed; > > + fprintf(in, "-%s\n", pack_basename(p)); > > + } else { > > + retain_cruft_pack(existing, p); > > + fprintf(in, "%s\n", pack_basename(p)); > > + } > > It's a bit funny that we re-check whether we have exceeded the maximum > size in subsequente iterations once we hit the limit, but it arguably > makes the logic a bit simpler. Yeah. Those checks are all noops (IOW, once we end up in the else branch, we'll stay there for the rest of the loop). But we don't want to break early, because we have to call retain_cruft_pack() on everything. In theory you could do something like: for (i = 0; i < existing_cruft_nr; i++) { size_t proposed; p = existing_cruft[i]; proposed = st_add(total_size, p->pack_size); if (proposed <= max_size) { total_size = proposed; fprintf(in, "-%s\n", pack_basename(p)); } else { break; } } for (; i < existing_cruft_nr; i++) { retain_cruft_pack(existing, existing_cruft[i]); fprintf(in, "%s\n", pack_basename(existing_cruft[i])); } But I think that the above is slightly more error-prone than what is written in the original patch. I have only the vaguest of preferences towards the former, but I'm happy to change it around if you feel strongly. > If I understand correctly, we only collapse small cruft packs in case > we're not expiring any objects at the same time. Is there an inherent > reason why? I would imagine that it can indeed be useful to expire > objects contained in cruft packs and then have git-repack(1) recombine > whatever is left into larger packs. > > If the reason is basically "it's complicated" then that is fine with me, > we can still implement the functionality at a later point in time. But > until then I think that we should let callers know that the two options > are incompatible with each other by producing an error when both are > passed. Your understanding is correct. We could try to leave existing cruft packs alone when none of their objects are removed as a result of pruning, but that case should be relatively rare. Another thing you could do is handle cruft packs which have only part of their objects being pruned by combining the non-pruned parts into a new pack. The latter should be mostly straightforward to implement, but since we're often ending up with very few cruft objects post-pruning, it likely wouldn't help much. Thanks, Taylor ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 0/3] repack: implement `--cruft-max-size` 2023-09-07 21:51 [PATCH 0/2] repack: implement `--cruft-max-size` Taylor Blau 2023-09-07 21:52 ` [PATCH 1/2] t7700: split cruft-related tests to t7704 Taylor Blau 2023-09-07 21:52 ` [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` Taylor Blau @ 2023-10-03 0:44 ` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 1/3] t7700: split cruft-related tests to t7704 Taylor Blau ` (2 more replies) 2 siblings, 3 replies; 21+ messages in thread From: Taylor Blau @ 2023-10-03 0:44 UTC (permalink / raw) To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt, Eric Sunshine (The earlier round of these patches depended on a couple of in-flight topics, but this series has been rebased onto the tip of 'master'). This is a reroll of my series that introduces more ergonomic options for creating and dealing with multiple cruft packs. Much is unchanged since last time, but some notable changes that have taken place include: - adding a missing commit message / s-o-b to the first patch to split cruft-related tests out of t7700 into t7704. - renamed the option to `--max-cruft-size` (instead of `--cruft-max-size`), and made corresponding changes to the documentation, configuration options, tests, etc. - cleaned up our handling of how we mark packs to be deleted and retained - parsed `--max-cruft-size` as an unsigned long up front instead of blindly passing a char * down to pack-objects to parse it for us. The last of those also affects `--max-pack-size`, which caused this series to grow a new second patch which cleans up the existing code which suffers from the same issue. As usual, a range-diff is available below. Thanks for all of the helpful review on the earlier round, and thanks in advance for your review on this one! Taylor Blau (3): t7700: split cruft-related tests to t7704 builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE builtin/repack.c: implement support for `--max-cruft-size` Documentation/config/gc.txt | 6 + Documentation/git-gc.txt | 7 + Documentation/git-repack.txt | 11 + builtin/gc.c | 7 + builtin/repack.c | 140 +++++++++++-- t/t6500-gc.sh | 27 +++ t/t7700-repack.sh | 121 ----------- t/t7704-repack-cruft.sh | 375 +++++++++++++++++++++++++++++++++++ 8 files changed, 559 insertions(+), 135 deletions(-) create mode 100755 t/t7704-repack-cruft.sh Range-diff against v1: 1: 103e19c75a < -: ---------- builtin/repack.c: extract structure to store existing packs 2: 45d5f15308 < -: ---------- builtin/repack.c: extract marking packs for deletion 3: 30eccbfdcb < -: ---------- builtin/repack.c: extract redundant pack cleanup for --geometric 4: 57e322301d < -: ---------- builtin/repack.c: extract redundant pack cleanup for existing packs 5: 4df4a2e51e < -: ---------- builtin/repack.c: extract `has_existing_non_kept_packs()` 6: 360be582b4 < -: ---------- builtin/repack.c: store existing cruft packs separately 7: ef9c8d775b < -: ---------- builtin/repack.c: drop `DELETE_PACK` macro 8: 873bc16abb < -: ---------- builtin/repack.c: extract common cruft pack loop 9: de6c2a0d70 ! 1: 3ed4ab61f6 t7700: split cruft-related tests to t7704 @@ Metadata ## Commit message ## t7700: split cruft-related tests to t7704 + A small handful of the tests in t7700 (the main script for testing + functionality of 'git repack') are specifically related to cruft pack + operations. + + Prepare for adding new cruft pack-related tests by moving the existing + set into a new test script. + + Signed-off-by: Taylor Blau <me@ttaylorr.com> + ## t/t7700-repack.sh ## @@ t/t7700-repack.sh: test_expect_success '-n overrides repack.updateServerInfo=true' ' test_server_info_missing -: ---------- > 2: 9ec999882d builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE 10: 7e4e42e1aa ! 3: e7beb2060d builtin/repack.c: implement support for `--cruft-max-size` @@ Metadata Author: Taylor Blau <me@ttaylorr.com> ## Commit message ## - builtin/repack.c: implement support for `--cruft-max-size` + builtin/repack.c: implement support for `--max-cruft-size` Cruft packs are an alternative mechanism for storing a collection of unreachable objects whose mtimes are recent enough to avoid being @@ Commit message (for more, see some of the details in 3d89a8c118 (Documentation/technical: add cruft-packs.txt, 2022-05-20)). - This all works, but can be costly from an I/O-perspective when a - repository has either (a) many unreachable objects, (b) prunes objects - relatively infrequently/never, or (c) both. + This all works, but can be costly from an I/O-perspective when + frequently repacking a repository that has many unreachable objects. + This problem is exacerbated when those unreachable objects are rarely + (if every) pruned. Since there is at most one cruft pack in the above scheme, each time we update the cruft pack it must be rewritten from scratch. Because much of @@ Commit message multiple cruft packs. This patch implements that support which we were lacking. - Introduce a new option `--cruft-max-size` which allows repositories to + Introduce a new option `--max-cruft-size` which allows repositories to accumulate cruft packs up to a given size, after which point a new generation of cruft packs can accumulate until it reaches the maximum size, and so on. To generate a new cruft pack, the process works like @@ Commit message pack, along with any other unreachable objects which have since entered the repository. - This limits the I/O churn up to a quadratic function of the value - specified by the `--cruft-max-size` option, instead of behaving - quadratically in the number of total unreachable objects. + Once a cruft pack grows beyond the size specified via `--max-cruft-size` + the pack is effectively frozen. This limits the I/O churn up to a + quadratic function of the value specified by the `--max-cruft-size` + option, instead of behaving quadratically in the number of total + unreachable objects. - When pruning unreachable objects, we bypass the new paths which combine - small cruft packs together, and instead start from scratch, passing in - the appropriate `--max-pack-size` down to `pack-objects`, putting it in - charge of keeping the resulting set of cruft packs sized correctly. + When pruning unreachable objects, we bypass the new code paths which + combine small cruft packs together, and instead start from scratch, + passing in the appropriate `--max-pack-size` down to `pack-objects`, + putting it in charge of keeping the resulting set of cruft packs sized + correctly. This may seem like further I/O churn, but in practice it isn't so bad. We could prune old cruft packs for whom all or most objects are removed, @@ Documentation/config/gc.txt: gc.cruftPacks:: linkgit:git-repack[1]) instead of as loose objects. The default is `true`. -+gc.cruftMaxSize:: ++gc.maxCruftSize:: + Limit the size of new cruft packs when repacking. When -+ specified in addition to `--cruft-max-size`, the command line -+ option takes priority. See the `--cruft-max-size` option of ++ specified in addition to `--max-cruft-size`, the command line ++ option takes priority. See the `--max-cruft-size` option of + linkgit:git-repack[1]. + gc.pruneExpire:: @@ Documentation/git-gc.txt: be performed as well. cruft pack instead of storing them as loose objects. `--cruft` is on by default. -+--cruft-max-size=<n>:: ++--max-cruft-size=<n>:: + When packing unreachable objects into a cruft pack, limit the + size of new cruft packs to be at most `<n>`. Overrides any -+ value specified via the `gc.cruftMaxSize` configuration. See -+ the `--cruft-max-size` option of linkgit:git-repack[1] for ++ value specified via the `gc.maxCruftSize` configuration. See ++ the `--max-cruft-size` option of linkgit:git-repack[1] for + more. + --prune=<date>:: @@ Documentation/git-repack.txt: to the new separate pack will be written. immediately instead of waiting for the next `git gc` invocation. Only useful with `--cruft -d`. -+--cruft-max-size=<n>:: -+ Repack cruft objects into packs as large as `<n>` before ++--max-cruft-size=<n>:: ++ Repack cruft objects into packs as large as `<n>` bytes before + creating new packs. As long as there are enough cruft packs + smaller than `<n>`, repacking will cause a new cruft pack to + be created containing objects from any combined cruft packs, -+ along with any new unreachable objects. Cruft packs larger -+ than `<n>` will not be modified. Only useful with `--cruft -+ -d`. ++ along with any new unreachable objects. Cruft packs larger than ++ `<n>` will not be modified. When the new cruft pack is larger ++ than `<n>` bytes, it will be split into multiple packs, all of ++ which are guaranteed to be at most `<n>` bytes in size. Only ++ useful with `--cruft -d`. + --expire-to=<dir>:: Write a cruft pack containing pruned objects (if any) to the @@ builtin/gc.c: static const char * const builtin_gc_usage[] = { static int pack_refs = 1; static int prune_reflogs = 1; static int cruft_packs = 1; -+static char *cruft_max_size; ++static unsigned long max_cruft_size; static int aggressive_depth = 50; static int aggressive_window = 250; static int gc_auto_threshold = 6700; @@ builtin/gc.c: static void gc_config(void) git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); git_config_get_bool("gc.autodetach", &detach_auto); git_config_get_bool("gc.cruftpacks", &cruft_packs); -+ git_config_get_string("gc.cruftmaxsize", &cruft_max_size); ++ git_config_get_ulong("gc.maxcruftsize", &max_cruft_size); git_config_get_expiry("gc.pruneexpire", &prune_expire); git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire); git_config_get_expiry("gc.logexpiry", &gc_log_expire); @@ builtin/gc.c: static void add_repack_all_option(struct string_list *keep_pack) strvec_push(&repack, "--cruft"); if (prune_expire) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire); -+ if (cruft_max_size) -+ strvec_pushf(&repack, "--cruft-max-size=%s", -+ cruft_max_size); ++ if (max_cruft_size) ++ strvec_pushf(&repack, "--max-cruft-size=%lu", ++ max_cruft_size); } else { strvec_push(&repack, "-A"); if (prune_expire) @@ builtin/gc.c: int cmd_gc(int argc, const char **argv, const char *prefix) N_("prune unreferenced objects"), PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire }, OPT_BOOL(0, "cruft", &cruft_packs, N_("pack unreferenced objects separately")), -+ OPT_STRING(0, "cruft-max-size", &cruft_max_size, -+ N_("bytes"), -+ N_("with --cruft, limit the size of new cruft packs")), ++ OPT_MAGNITUDE(0, "max-cruft-size", &max_cruft_size, ++ N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")), OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"), PARSE_OPT_NOCOMPLETE), ## builtin/repack.c ## @@ - #define LOOSEN_UNREACHABLE 2 #define PACK_CRUFT 4 -+#define DELETE_PACK ((void*)(uintptr_t)1) -+#define RETAIN_PACK ((uintptr_t)(1<<1)) -+ + #define DELETE_PACK 1 ++#define RETAIN_PACK 2 + static int pack_everything; static int delta_base_offset = 1; - static int pack_kept_objects = -1; -@@ builtin/repack.c: static int has_existing_non_kept_packs(const struct existing_packs *existing) - return existing->non_kept_packs.nr || existing->cruft_packs.nr; +@@ builtin/repack.c: static void pack_mark_for_deletion(struct string_list_item *item) + item->util = (void*)((uintptr_t)item->util | DELETE_PACK); } ++static void pack_unmark_for_deletion(struct string_list_item *item) ++{ ++ item->util = (void*)((uintptr_t)item->util & ~DELETE_PACK); ++} ++ + static int pack_is_marked_for_deletion(struct string_list_item *item) + { + return (uintptr_t)item->util & DELETE_PACK; + } + ++static void pack_mark_retained(struct string_list_item *item) ++{ ++ item->util = (void*)((uintptr_t)item->util | RETAIN_PACK); ++} ++ +static int pack_is_retained(struct string_list_item *item) +{ + return (uintptr_t)item->util & RETAIN_PACK; @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name - * (if `-d` was given). - */ - if (!string_list_has_string(names, sha1)) -- item->util = (void*)1; + + if (pack_is_retained(item)) { -+ item->util = NULL; ++ pack_unmark_for_deletion(item); + } else if (!string_list_has_string(names, sha1)) { + /* + * Mark this pack for deletion, which ensures @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name + * will actually delete this pack (if `-d` was + * given). + */ -+ item->util = DELETE_PACK; + pack_mark_for_deletion(item); + } } } @@ builtin/repack.c: static void mark_packs_for_deletion_1(struct string_list *name + if (!item) + BUG("could not find cruft pack '%s'", pack_basename(cruft)); + -+ item->util = (void*)RETAIN_PACK; ++ pack_mark_retained(item); + strbuf_release(&buf); +} + @@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu + return 0; +} + -+static void collapse_small_cruft_packs(FILE *in, unsigned long max_size, ++static void collapse_small_cruft_packs(FILE *in, size_t max_size, + struct existing_packs *existing) +{ + struct packed_git **existing_cruft, *p; + struct strbuf buf = STRBUF_INIT; -+ unsigned long total_size = 0; ++ size_t total_size = 0; + size_t existing_cruft_nr = 0; + size_t i; + @@ builtin/repack.c: static void remove_redundant_bitmaps(struct string_list *inclu + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); + + for (i = 0; i < existing_cruft_nr; i++) { -+ off_t proposed; ++ size_t proposed; + + p = existing_cruft[i]; + proposed = st_add(total_size, p->pack_size); @@ builtin/repack.c: static int write_cruft_pack(const struct pack_objects_args *ar - for_each_string_list_item(item, &existing->cruft_packs) - fprintf(in, "-%s.pack\n", item->string); + if (args->max_pack_size && !cruft_expiration) { -+ unsigned long max_pack_size; -+ if (!git_parse_ulong(args->max_pack_size, &max_pack_size)) -+ return error(_("could not parse --cruft-max-size: '%s'"), -+ args->max_pack_size); -+ collapse_small_cruft_packs(in, max_pack_size, existing); ++ collapse_small_cruft_packs(in, args->max_pack_size, existing); + } else { + for_each_string_list_item(item, &existing->non_kept_packs) + fprintf(in, "-%s.pack\n", item->string); @@ builtin/repack.c: int cmd_repack(int argc, const char **argv, const char *prefix PACK_CRUFT), OPT_STRING(0, "cruft-expiration", &cruft_expiration, N_("approxidate"), N_("with --cruft, expire objects older than this")), -+ OPT_STRING(0, "cruft-max-size", &cruft_po_args.max_pack_size, -+ N_("bytes"), ++ OPT_MAGNITUDE(0, "max-cruft-size", &cruft_po_args.max_pack_size, + N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL('d', NULL, &delete_redundant, N_("remove redundant packs, and run git-prune-packed")), @@ t/t6500-gc.sh: test_expect_success 'gc.bigPackThreshold ignores cruft packs' ' +cruft_max_size_opts="git repack -d -l --cruft --cruft-expiration=2.weeks.ago" + -+test_expect_success 'setup for --cruft-max-size tests' ' ++test_expect_success 'setup for --max-cruft-size tests' ' + git init cruft--max-size && + ( + cd cruft--max-size && @@ t/t6500-gc.sh: test_expect_success 'gc.bigPackThreshold ignores cruft packs' ' + ) +' + -+test_expect_success '--cruft-max-size sets appropriate repack options' ' ++test_expect_success '--max-cruft-size sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \ -+ gc --cruft --cruft-max-size=1M && -+ test_subcommand $cruft_max_size_opts --cruft-max-size=1M <trace2.txt ++ gc --cruft --max-cruft-size=1M && ++ test_subcommand $cruft_max_size_opts --max-cruft-size=1048576 <trace2.txt +' + -+test_expect_success 'gc.cruftMaxSize sets appropriate repack options' ' ++test_expect_success 'gc.maxCruftSize sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ -+ git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft && -+ test_subcommand $cruft_max_size_opts --cruft-max-size=2M <trace2.txt && ++ git -C cruft--max-size -c gc.maxCruftSize=2M gc --cruft && ++ test_subcommand $cruft_max_size_opts --max-cruft-size=2097152 <trace2.txt && + + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ -+ git -C cruft--max-size -c gc.cruftMaxSize=2M gc --cruft \ -+ --cruft-max-size=3M && -+ test_subcommand $cruft_max_size_opts --cruft-max-size=3M <trace2.txt ++ git -C cruft--max-size -c gc.maxCruftSize=2M gc --cruft \ ++ --max-cruft-size=3M && ++ test_subcommand $cruft_max_size_opts --max-cruft-size=3145728 <trace2.txt +' + run_and_wait_for_auto_gc () { @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + echo "$packdir/pack-$pack.mtimes" +} + -+test_expect_success '--cruft-max-size creates new packs when above threshold' ' -+ git init cruft-max-size-large && ++test_expect_success '--max-cruft-size creates new packs when above threshold' ' ++ git init max-cruft-size-large && + ( -+ cd cruft-max-size-large && ++ cd max-cruft-size-large && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + cruft_foo="$(ls $packdir/pack-*.mtimes)" && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && -+ git repack --cruft -d --cruft-max-size=1M && ++ git repack --cruft -d --max-cruft-size=1M && + cruft_bar="$(ls $packdir/pack-*.mtimes | grep -v $cruft_foo)" && + + test-tool pack-mtimes $(basename "$cruft_foo") >foo.objects && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size combines existing packs when below threshold' ' -+ git init cruft-max-size-small && ++test_expect_success '--max-cruft-size combines existing packs when below threshold' ' ++ git init max-cruft-size-small && + ( -+ cd cruft-max-size-small && ++ cd max-cruft-size-small && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && + git repack --cruft -d && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && -+ git repack --cruft -d --cruft-max-size=10M && ++ git repack --cruft -d --max-cruft-size=10M && + + cruft=$(ls $packdir/pack-*.mtimes) && + test-tool pack-mtimes $(basename "$cruft") >cruft.objects && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size combines smaller packs first' ' -+ git init cruft-max-size-consume-small && ++test_expect_success '--max-cruft-size combines smaller packs first' ' ++ git init max-cruft-size-consume-small && + ( -+ cd cruft-max-size-consume-small && ++ cd max-cruft-size-consume-small && + + test_commit base && + git repack -ad && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + test-tool pack-mtimes "$(basename $cruft_bar)" >>expect.raw && + sort expect.raw >expect.objects && + -+ # repacking with `--cruft-max-size=2M` should combine ++ # repacking with `--max-cruft-size=2M` should combine + # both 0.5 MiB packs together, instead of, say, one of + # the 0.5 MiB packs with the 1.0 MiB pack + ls $packdir/pack-*.mtimes | sort >cruft.before && -+ git repack -d --cruft --cruft-max-size=2M && ++ git repack -d --cruft --max-cruft-size=2M && + ls $packdir/pack-*.mtimes | sort >cruft.after && + + comm -13 cruft.before cruft.after >cruft.new && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success 'setup --cruft-max-size with freshened objects' ' -+ git init cruft-max-size-freshen && ++test_expect_success 'setup --max-cruft-size with freshened objects' ' ++ git init max-cruft-size-freshen && + ( -+ cd cruft-max-size-freshen && ++ cd max-cruft-size-freshen && + + test_commit base && + git repack -ad && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size with freshened objects (loose)' ' ++test_expect_success '--max-cruft-size with freshened objects (loose)' ' + ( -+ cd cruft-max-size-freshen && ++ cd max-cruft-size-freshen && + + # regenerate the object, setting its mtime to be more recent + foo="$(generate_random_blob foo 64)" && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size with freshened objects (packed)' ' ++test_expect_success '--max-cruft-size with freshened objects (packed)' ' + ( -+ cd cruft-max-size-freshen && ++ cd max-cruft-size-freshen && + + # regenerate the object and store it in a packfile, + # setting its mtime to be more recent @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size with pruning' ' -+ git init cruft-max-size-prune && ++test_expect_success '--max-cruft-size with pruning' ' ++ git init max-cruft-size-prune && + ( -+ cd cruft-max-size-prune && ++ cd max-cruft-size-prune && + + test_commit base && + foo="$(generate_random_blob foo $((1024*1024)))" && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + + test-tool chmtime -10000 "$objdir/$(test_oid_to_path "$foo")" && + -+ git repack -d --cruft --cruft-max-size=1M && ++ git repack -d --cruft --max-cruft-size=1M && + + # backdate the mtimes of all cruft packs to validate + # that they were rewritten as a result of pruning @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + echo $cruft $mtime >>mtimes || return 1 + done && + -+ # repack (and prune) with a --cruft-max-size to ensure ++ # repack (and prune) with a --max-cruft-size to ensure + # that we appropriately split the resulting set of packs -+ git repack -d --cruft --cruft-max-size=1M \ ++ git repack -d --cruft --max-cruft-size=1M \ + --cruft-expiration=10.seconds.ago && + ls $packdir/pack-*.mtimes | sort >cruft.after && + @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + ) +' + -+test_expect_success '--cruft-max-size ignores non-local packs' ' -+ repo="cruft-max-size-non-local" && ++test_expect_success '--max-cruft-size ignores non-local packs' ' ++ repo="max-cruft-size-non-local" && + git init $repo && + ( + cd $repo && @@ t/t7704-repack-cruft.sh: test_expect_success '--expire-to stores pruned objects + # ensure that we do not attempt to pick up packs from + # the non-alternated repository, which would result in a + # crash -+ git repack --cruft --cruft-max-size=1M -d ++ git repack --cruft --max-cruft-size=1M -d + ) +' + -- 2.42.0.310.g9604b54f73.dirty ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 1/3] t7700: split cruft-related tests to t7704 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau @ 2023-10-03 0:44 ` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE Taylor Blau 2023-10-03 0:44 ` [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` Taylor Blau 2 siblings, 0 replies; 21+ messages in thread From: Taylor Blau @ 2023-10-03 0:44 UTC (permalink / raw) To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt, Eric Sunshine A small handful of the tests in t7700 (the main script for testing functionality of 'git repack') are specifically related to cruft pack operations. Prepare for adding new cruft pack-related tests by moving the existing set into a new test script. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- t/t7700-repack.sh | 121 ------------------------------------- t/t7704-repack-cruft.sh | 130 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 121 deletions(-) create mode 100755 t/t7704-repack-cruft.sh diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 27b66807cd..55ee3eb8ae 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -633,125 +633,4 @@ test_expect_success '-n overrides repack.updateServerInfo=true' ' test_server_info_missing ' -test_expect_success '--expire-to stores pruned objects (now)' ' - git init expire-to-now && - ( - cd expire-to-now && - - git branch -M main && - - test_commit base && - - git checkout -b cruft && - test_commit --no-tag cruft && - - git rev-list --objects --no-object-names main..cruft >moved.raw && - sort moved.raw >moved.want && - - git rev-list --all --objects --no-object-names >expect.raw && - sort expect.raw >expect && - - git checkout main && - git branch -D cruft && - git reflog expire --all --expire=all && - - git init --bare expired.git && - git repack -d \ - --cruft --cruft-expiration="now" \ - --expire-to="expired.git/objects/pack/pack" && - - expired="$(ls expired.git/objects/pack/pack-*.idx)" && - test_path_is_file "${expired%.idx}.mtimes" && - - # Since the `--cruft-expiration` is "now", the effective - # behavior is to move _all_ unreachable objects out to - # the location in `--expire-to`. - git show-index <$expired >expired.raw && - cut -d" " -f2 expired.raw | sort >expired.objects && - git rev-list --all --objects --no-object-names \ - >remaining.objects && - - # ...in other words, the combined contents of this - # repository and expired.git should be the same as the - # set of objects we started with. - cat expired.objects remaining.objects | sort >actual && - test_cmp expect actual && - - # The "moved" objects (i.e., those in expired.git) - # should be the same as the cruft objects which were - # expired in the previous step. - test_cmp moved.want expired.objects - ) -' - -test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' - git init expire-to-5.minutes.ago && - ( - cd expire-to-5.minutes.ago && - - git branch -M main && - - test_commit base && - - # Create two classes of unreachable objects, one which - # is older than 5 minutes (stale), and another which is - # newer (recent). - for kind in stale recent - do - git checkout -b $kind main && - test_commit --no-tag $kind || return 1 - done && - - git rev-list --objects --no-object-names main..stale >in && - stale="$(git pack-objects $objdir/pack/pack <in)" && - mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" && - - # expect holds the set of objects we expect to find in - # this repository after repacking - git rev-list --objects --no-object-names recent >expect.raw && - sort expect.raw >expect && - - # moved.want holds the set of objects we expect to find - # in expired.git - git rev-list --objects --no-object-names main..stale >out && - sort out >moved.want && - - git checkout main && - git branch -D stale recent && - git reflog expire --all --expire=all && - git prune-packed && - - git init --bare expired.git && - git repack -d \ - --cruft --cruft-expiration=5.minutes.ago \ - --expire-to="expired.git/objects/pack/pack" && - - # Some of the remaining objects in this repository are - # unreachable, so use `cat-file --batch-all-objects` - # instead of `rev-list` to get their names - git cat-file --batch-all-objects --batch-check="%(objectname)" \ - >remaining.objects && - sort remaining.objects >actual && - test_cmp expect actual && - - ( - cd expired.git && - - expired="$(ls objects/pack/pack-*.mtimes)" && - test-tool pack-mtimes $(basename $expired) >out && - cut -d" " -f1 out | sort >../moved.got && - - # Ensure that there are as many objects with the - # expected mtime as were moved to expired.git. - # - # In other words, ensure that the recorded - # mtimes of any moved objects was written - # correctly. - grep " $mtime$" out >matching && - test_line_count = $(wc -l <../moved.want) matching - ) && - test_cmp moved.want moved.got - ) -' - test_done diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh new file mode 100755 index 0000000000..d91fcf1af1 --- /dev/null +++ b/t/t7704-repack-cruft.sh @@ -0,0 +1,130 @@ +#!/bin/sh + +test_description='git repack works correctly' + +. ./test-lib.sh + +objdir=.git/objects + +test_expect_success '--expire-to stores pruned objects (now)' ' + git init expire-to-now && + ( + cd expire-to-now && + + git branch -M main && + + test_commit base && + + git checkout -b cruft && + test_commit --no-tag cruft && + + git rev-list --objects --no-object-names main..cruft >moved.raw && + sort moved.raw >moved.want && + + git rev-list --all --objects --no-object-names >expect.raw && + sort expect.raw >expect && + + git checkout main && + git branch -D cruft && + git reflog expire --all --expire=all && + + git init --bare expired.git && + git repack -d \ + --cruft --cruft-expiration="now" \ + --expire-to="expired.git/objects/pack/pack" && + + expired="$(ls expired.git/objects/pack/pack-*.idx)" && + test_path_is_file "${expired%.idx}.mtimes" && + + # Since the `--cruft-expiration` is "now", the effective + # behavior is to move _all_ unreachable objects out to + # the location in `--expire-to`. + git show-index <$expired >expired.raw && + cut -d" " -f2 expired.raw | sort >expired.objects && + git rev-list --all --objects --no-object-names \ + >remaining.objects && + + # ...in other words, the combined contents of this + # repository and expired.git should be the same as the + # set of objects we started with. + cat expired.objects remaining.objects | sort >actual && + test_cmp expect actual && + + # The "moved" objects (i.e., those in expired.git) + # should be the same as the cruft objects which were + # expired in the previous step. + test_cmp moved.want expired.objects + ) +' + +test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' + git init expire-to-5.minutes.ago && + ( + cd expire-to-5.minutes.ago && + + git branch -M main && + + test_commit base && + + # Create two classes of unreachable objects, one which + # is older than 5 minutes (stale), and another which is + # newer (recent). + for kind in stale recent + do + git checkout -b $kind main && + test_commit --no-tag $kind || return 1 + done && + + git rev-list --objects --no-object-names main..stale >in && + stale="$(git pack-objects $objdir/pack/pack <in)" && + mtime="$(test-tool chmtime --get =-600 $objdir/pack/pack-$stale.pack)" && + + # expect holds the set of objects we expect to find in + # this repository after repacking + git rev-list --objects --no-object-names recent >expect.raw && + sort expect.raw >expect && + + # moved.want holds the set of objects we expect to find + # in expired.git + git rev-list --objects --no-object-names main..stale >out && + sort out >moved.want && + + git checkout main && + git branch -D stale recent && + git reflog expire --all --expire=all && + git prune-packed && + + git init --bare expired.git && + git repack -d \ + --cruft --cruft-expiration=5.minutes.ago \ + --expire-to="expired.git/objects/pack/pack" && + + # Some of the remaining objects in this repository are + # unreachable, so use `cat-file --batch-all-objects` + # instead of `rev-list` to get their names + git cat-file --batch-all-objects --batch-check="%(objectname)" \ + >remaining.objects && + sort remaining.objects >actual && + test_cmp expect actual && + + ( + cd expired.git && + + expired="$(ls objects/pack/pack-*.mtimes)" && + test-tool pack-mtimes $(basename $expired) >out && + cut -d" " -f1 out | sort >../moved.got && + + # Ensure that there are as many objects with the + # expected mtime as were moved to expired.git. + # + # In other words, ensure that the recorded + # mtimes of any moved objects was written + # correctly. + grep " $mtime$" out >matching && + test_line_count = $(wc -l <../moved.want) matching + ) && + test_cmp moved.want moved.got + ) +' + +test_done -- 2.42.0.310.g9604b54f73.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 1/3] t7700: split cruft-related tests to t7704 Taylor Blau @ 2023-10-03 0:44 ` Taylor Blau 2023-10-05 11:31 ` Patrick Steinhardt 2023-10-03 0:44 ` [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` Taylor Blau 2 siblings, 1 reply; 21+ messages in thread From: Taylor Blau @ 2023-10-03 0:44 UTC (permalink / raw) To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt, Eric Sunshine The repack builtin takes a `--max-pack-size` command-line argument which it uses to feed into any of the pack-objects children that it may spawn when generating a new pack. This option is parsed with OPT_STRING, meaning that we'll accept anything as input, punting on more fine-grained validation until we get down into pack-objects. This is fine, but it's wasteful to spend an entire sub-process just to figure out that one of its option is bogus. Instead, parse the value of `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the knonw-good result down to pack-objects. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Taylor Blau <me@ttaylorr.com> --- builtin/repack.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/repack.c b/builtin/repack.c index 529e13120d..8a5bbb9cba 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -51,7 +51,7 @@ struct pack_objects_args { const char *window_memory; const char *depth; const char *threads; - const char *max_pack_size; + unsigned long max_pack_size; int no_reuse_delta; int no_reuse_object; int quiet; @@ -242,7 +242,7 @@ static void prepare_pack_objects(struct child_process *cmd, if (args->threads) strvec_pushf(&cmd->args, "--threads=%s", args->threads); if (args->max_pack_size) - strvec_pushf(&cmd->args, "--max-pack-size=%s", args->max_pack_size); + strvec_pushf(&cmd->args, "--max-pack-size=%lu", args->max_pack_size); if (args->no_reuse_delta) strvec_pushf(&cmd->args, "--no-reuse-delta"); if (args->no_reuse_object) @@ -946,7 +946,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) N_("limits the maximum delta depth")), OPT_STRING(0, "threads", &po_args.threads, N_("n"), N_("limits the maximum number of threads")), - OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"), + OPT_MAGNITUDE(0, "max-pack-size", &po_args.max_pack_size, N_("maximum size of each packfile")), OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects, N_("repack objects in packs marked with .keep")), -- 2.42.0.310.g9604b54f73.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE 2023-10-03 0:44 ` [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE Taylor Blau @ 2023-10-05 11:31 ` Patrick Steinhardt 2023-10-05 17:28 ` Taylor Blau 0 siblings, 1 reply; 21+ messages in thread From: Patrick Steinhardt @ 2023-10-05 11:31 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine [-- Attachment #1: Type: text/plain, Size: 2434 bytes --] On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote: > The repack builtin takes a `--max-pack-size` command-line argument which > it uses to feed into any of the pack-objects children that it may spawn > when generating a new pack. > > This option is parsed with OPT_STRING, meaning that we'll accept > anything as input, punting on more fine-grained validation until we get > down into pack-objects. > > This is fine, but it's wasteful to spend an entire sub-process just to > figure out that one of its option is bogus. Instead, parse the value of > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the > knonw-good result down to pack-objects. Tiny nit: s/knonw/known. Other than that this patch looks good to me. Patrick > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: Taylor Blau <me@ttaylorr.com> > --- > builtin/repack.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/builtin/repack.c b/builtin/repack.c > index 529e13120d..8a5bbb9cba 100644 > --- a/builtin/repack.c > +++ b/builtin/repack.c > @@ -51,7 +51,7 @@ struct pack_objects_args { > const char *window_memory; > const char *depth; > const char *threads; > - const char *max_pack_size; > + unsigned long max_pack_size; > int no_reuse_delta; > int no_reuse_object; > int quiet; > @@ -242,7 +242,7 @@ static void prepare_pack_objects(struct child_process *cmd, > if (args->threads) > strvec_pushf(&cmd->args, "--threads=%s", args->threads); > if (args->max_pack_size) > - strvec_pushf(&cmd->args, "--max-pack-size=%s", args->max_pack_size); > + strvec_pushf(&cmd->args, "--max-pack-size=%lu", args->max_pack_size); > if (args->no_reuse_delta) > strvec_pushf(&cmd->args, "--no-reuse-delta"); > if (args->no_reuse_object) > @@ -946,7 +946,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix) > N_("limits the maximum delta depth")), > OPT_STRING(0, "threads", &po_args.threads, N_("n"), > N_("limits the maximum number of threads")), > - OPT_STRING(0, "max-pack-size", &po_args.max_pack_size, N_("bytes"), > + OPT_MAGNITUDE(0, "max-pack-size", &po_args.max_pack_size, > N_("maximum size of each packfile")), > OPT_BOOL(0, "pack-kept-objects", &pack_kept_objects, > N_("repack objects in packs marked with .keep")), > -- > 2.42.0.310.g9604b54f73.dirty > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE 2023-10-05 11:31 ` Patrick Steinhardt @ 2023-10-05 17:28 ` Taylor Blau 2023-10-05 20:22 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Taylor Blau @ 2023-10-05 17:28 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote: > On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote: > > The repack builtin takes a `--max-pack-size` command-line argument which > > it uses to feed into any of the pack-objects children that it may spawn > > when generating a new pack. > > > > This option is parsed with OPT_STRING, meaning that we'll accept > > anything as input, punting on more fine-grained validation until we get > > down into pack-objects. > > > > This is fine, but it's wasteful to spend an entire sub-process just to > > figure out that one of its option is bogus. Instead, parse the value of > > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the > > knonw-good result down to pack-objects. > > Tiny nit: s/knonw/known. > > Other than that this patch looks good to me. Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he doesn't, I don't think it'd be the end of the world... Thanks, Taylor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE 2023-10-05 17:28 ` Taylor Blau @ 2023-10-05 20:22 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2023-10-05 20:22 UTC (permalink / raw) To: Taylor Blau; +Cc: Patrick Steinhardt, git, Jeff King, Eric Sunshine Taylor Blau <me@ttaylorr.com> writes: > On Thu, Oct 05, 2023 at 01:31:02PM +0200, Patrick Steinhardt wrote: >> On Mon, Oct 02, 2023 at 08:44:29PM -0400, Taylor Blau wrote: >> > The repack builtin takes a `--max-pack-size` command-line argument which >> > it uses to feed into any of the pack-objects children that it may spawn >> > when generating a new pack. >> > >> > This option is parsed with OPT_STRING, meaning that we'll accept >> > anything as input, punting on more fine-grained validation until we get >> > down into pack-objects. >> > >> > This is fine, but it's wasteful to spend an entire sub-process just to >> > figure out that one of its option is bogus. Instead, parse the value of >> > `--max-pack-size` with OPT_MAGNITUDE in 'git repack', and then pass the >> > knonw-good result down to pack-objects. >> >> Tiny nit: s/knonw/known. >> >> Other than that this patch looks good to me. > > Oops, good eyes. Perhaps Junio can tweak this when queuing, but if he > doesn't, I don't think it'd be the end of the world... I cannot do so "when queuing" anymore, but let me see if it is still outside 'next' and then run "rebase -i" on it. This is a bit different from what I would have written (I would have passed the original string to underlying subprocess for it to sanity check the value to its own liking, after we checked if it makes sense when interpreted as magnitude, so that the underlying subprocess, if it has stricter limit than we know of, can complain with the string the end-user gave it, not the result of us turning into a large integer string. But that is minor. Thanks, both. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 1/3] t7700: split cruft-related tests to t7704 Taylor Blau 2023-10-03 0:44 ` [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE Taylor Blau @ 2023-10-03 0:44 ` Taylor Blau 2023-10-05 12:08 ` Patrick Steinhardt 2023-10-07 17:20 ` [PATCH] repack: free existing_cruft array after use Jeff King 2 siblings, 2 replies; 21+ messages in thread From: Taylor Blau @ 2023-10-03 0:44 UTC (permalink / raw) To: git; +Cc: Jeff King, Junio C Hamano, Patrick Steinhardt, Eric Sunshine Cruft packs are an alternative mechanism for storing a collection of unreachable objects whose mtimes are recent enough to avoid being pruned out of the repository. When cruft packs were first introduced back in b757353676 (builtin/pack-objects.c: --cruft without expiration, 2022-05-20) and a7d493833f (builtin/pack-objects.c: --cruft with expiration, 2022-05-20), the recommended workflow consisted of: - Repacking periodically, either by packing anything loose in the repository (via `git repack -d`) or producing a geometric sequence of packs (via `git repack --geometric=<d> -d`). - Every so often, splitting the repository into two packs, one cruft to store the unreachable objects, and another non-cruft pack to store the reachable objects. Repositories may (out of band with the above) choose periodically to prune out some unreachable objects which have aged out of the grace period by generating a pack with `--cruft-expiration=<approxidate>`. This allowed repositories to maintain relatively few packs on average, and quarantine unreachable objects together in a cruft pack, avoiding the pitfalls of holding unreachable objects as loose while they age out (for more, see some of the details in 3d89a8c118 (Documentation/technical: add cruft-packs.txt, 2022-05-20)). This all works, but can be costly from an I/O-perspective when frequently repacking a repository that has many unreachable objects. This problem is exacerbated when those unreachable objects are rarely (if every) pruned. Since there is at most one cruft pack in the above scheme, each time we update the cruft pack it must be rewritten from scratch. Because much of the pack is reused, this is a relatively inexpensive operation from a CPU-perspective, but is very costly in terms of I/O since we end up rewriting basically the same pack (plus any new unreachable objects that have entered the repository since the last time a cruft pack was generated). At the time, we decided against implementing more robust support for multiple cruft packs. This patch implements that support which we were lacking. Introduce a new option `--max-cruft-size` which allows repositories to accumulate cruft packs up to a given size, after which point a new generation of cruft packs can accumulate until it reaches the maximum size, and so on. To generate a new cruft pack, the process works like so: - Sort a list of any existing cruft packs in ascending order of pack size. - Starting from the beginning of the list, group cruft packs together while the accumulated size is smaller than the maximum specified pack size. - Combine the objects in these cruft packs together into a new cruft pack, along with any other unreachable objects which have since entered the repository. Once a cruft pack grows beyond the size specified via `--max-cruft-size` the pack is effectively frozen. This limits the I/O churn up to a quadratic function of the value specified by the `--max-cruft-size` option, instead of behaving quadratically in the number of total unreachable objects. When pruning unreachable objects, we bypass the new code paths which combine small cruft packs together, and instead start from scratch, passing in the appropriate `--max-pack-size` down to `pack-objects`, putting it in charge of keeping the resulting set of cruft packs sized correctly. This may seem like further I/O churn, but in practice it isn't so bad. We could prune old cruft packs for whom all or most objects are removed, and then generate a new cruft pack with just the remaining set of objects. But this additional complexity buys us relatively little, because most objects end up being pruned anyway, so the I/O churn is well contained. Signed-off-by: Taylor Blau <me@ttaylorr.com> --- Documentation/config/gc.txt | 6 + Documentation/git-gc.txt | 7 + Documentation/git-repack.txt | 11 ++ builtin/gc.c | 7 + builtin/repack.c | 134 +++++++++++++++++-- t/t6500-gc.sh | 27 ++++ t/t7704-repack-cruft.sh | 245 +++++++++++++++++++++++++++++++++++ 7 files changed, 426 insertions(+), 11 deletions(-) diff --git a/Documentation/config/gc.txt b/Documentation/config/gc.txt index ca47eb2008..83ebc2de55 100644 --- a/Documentation/config/gc.txt +++ b/Documentation/config/gc.txt @@ -86,6 +86,12 @@ gc.cruftPacks:: linkgit:git-repack[1]) instead of as loose objects. The default is `true`. +gc.maxCruftSize:: + Limit the size of new cruft packs when repacking. When + specified in addition to `--max-cruft-size`, the command line + option takes priority. See the `--max-cruft-size` option of + linkgit:git-repack[1]. + gc.pruneExpire:: When 'git gc' is run, it will call 'prune --expire 2.weeks.ago' (and 'repack --cruft --cruft-expiration 2.weeks.ago' if using diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt index 90806fd26a..fa0541b416 100644 --- a/Documentation/git-gc.txt +++ b/Documentation/git-gc.txt @@ -59,6 +59,13 @@ be performed as well. cruft pack instead of storing them as loose objects. `--cruft` is on by default. +--max-cruft-size=<n>:: + When packing unreachable objects into a cruft pack, limit the + size of new cruft packs to be at most `<n>`. Overrides any + value specified via the `gc.maxCruftSize` configuration. See + the `--max-cruft-size` option of linkgit:git-repack[1] for + more. + --prune=<date>:: Prune loose objects older than date (default is 2 weeks ago, overridable by the config variable `gc.pruneExpire`). diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt index 4017157949..fbfc72e1b2 100644 --- a/Documentation/git-repack.txt +++ b/Documentation/git-repack.txt @@ -74,6 +74,17 @@ to the new separate pack will be written. immediately instead of waiting for the next `git gc` invocation. Only useful with `--cruft -d`. +--max-cruft-size=<n>:: + Repack cruft objects into packs as large as `<n>` bytes before + creating new packs. As long as there are enough cruft packs + smaller than `<n>`, repacking will cause a new cruft pack to + be created containing objects from any combined cruft packs, + along with any new unreachable objects. Cruft packs larger than + `<n>` will not be modified. When the new cruft pack is larger + than `<n>` bytes, it will be split into multiple packs, all of + which are guaranteed to be at most `<n>` bytes in size. Only + useful with `--cruft -d`. + --expire-to=<dir>:: Write a cruft pack containing pruned objects (if any) to the directory `<dir>`. This option is useful for keeping a copy of diff --git a/builtin/gc.c b/builtin/gc.c index 00192ae5d3..c97c9fb046 100644 --- a/builtin/gc.c +++ b/builtin/gc.c @@ -52,6 +52,7 @@ static const char * const builtin_gc_usage[] = { static int pack_refs = 1; static int prune_reflogs = 1; static int cruft_packs = 1; +static unsigned long max_cruft_size; static int aggressive_depth = 50; static int aggressive_window = 250; static int gc_auto_threshold = 6700; @@ -163,6 +164,7 @@ static void gc_config(void) git_config_get_int("gc.autopacklimit", &gc_auto_pack_limit); git_config_get_bool("gc.autodetach", &detach_auto); git_config_get_bool("gc.cruftpacks", &cruft_packs); + git_config_get_ulong("gc.maxcruftsize", &max_cruft_size); git_config_get_expiry("gc.pruneexpire", &prune_expire); git_config_get_expiry("gc.worktreepruneexpire", &prune_worktrees_expire); git_config_get_expiry("gc.logexpiry", &gc_log_expire); @@ -347,6 +349,9 @@ static void add_repack_all_option(struct string_list *keep_pack) strvec_push(&repack, "--cruft"); if (prune_expire) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire); + if (max_cruft_size) + strvec_pushf(&repack, "--max-cruft-size=%lu", + max_cruft_size); } else { strvec_push(&repack, "-A"); if (prune_expire) @@ -575,6 +580,8 @@ int cmd_gc(int argc, const char **argv, const char *prefix) N_("prune unreferenced objects"), PARSE_OPT_OPTARG, NULL, (intptr_t)prune_expire }, OPT_BOOL(0, "cruft", &cruft_packs, N_("pack unreferenced objects separately")), + OPT_MAGNITUDE(0, "max-cruft-size", &max_cruft_size, + N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL(0, "aggressive", &aggressive, N_("be more thorough (increased runtime)")), OPT_BOOL_F(0, "auto", &auto_gc, N_("enable auto-gc mode"), PARSE_OPT_NOCOMPLETE), diff --git a/builtin/repack.c b/builtin/repack.c index 8a5bbb9cba..04770b15fe 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -27,6 +27,7 @@ #define PACK_CRUFT 4 #define DELETE_PACK 1 +#define RETAIN_PACK 2 static int pack_everything; static int delta_base_offset = 1; @@ -116,11 +117,26 @@ static void pack_mark_for_deletion(struct string_list_item *item) item->util = (void*)((uintptr_t)item->util | DELETE_PACK); } +static void pack_unmark_for_deletion(struct string_list_item *item) +{ + item->util = (void*)((uintptr_t)item->util & ~DELETE_PACK); +} + static int pack_is_marked_for_deletion(struct string_list_item *item) { return (uintptr_t)item->util & DELETE_PACK; } +static void pack_mark_retained(struct string_list_item *item) +{ + item->util = (void*)((uintptr_t)item->util | RETAIN_PACK); +} + +static int pack_is_retained(struct string_list_item *item) +{ + return (uintptr_t)item->util & RETAIN_PACK; +} + static void mark_packs_for_deletion_1(struct string_list *names, struct string_list *list) { @@ -133,17 +149,39 @@ static void mark_packs_for_deletion_1(struct string_list *names, if (len < hexsz) continue; sha1 = item->string + len - hexsz; - /* - * Mark this pack for deletion, which ensures that this - * pack won't be included in a MIDX (if `--write-midx` - * was given) and that we will actually delete this pack - * (if `-d` was given). - */ - if (!string_list_has_string(names, sha1)) + + if (pack_is_retained(item)) { + pack_unmark_for_deletion(item); + } else if (!string_list_has_string(names, sha1)) { + /* + * Mark this pack for deletion, which ensures + * that this pack won't be included in a MIDX + * (if `--write-midx` was given) and that we + * will actually delete this pack (if `-d` was + * given). + */ pack_mark_for_deletion(item); + } } } +static void retain_cruft_pack(struct existing_packs *existing, + struct packed_git *cruft) +{ + struct strbuf buf = STRBUF_INIT; + struct string_list_item *item; + + strbuf_addstr(&buf, pack_basename(cruft)); + strbuf_strip_suffix(&buf, ".pack"); + + item = string_list_lookup(&existing->cruft_packs, buf.buf); + if (!item) + BUG("could not find cruft pack '%s'", pack_basename(cruft)); + + pack_mark_retained(item); + strbuf_release(&buf); +} + static void mark_packs_for_deletion(struct existing_packs *existing, struct string_list *names) @@ -225,6 +263,8 @@ static void collect_pack_filenames(struct existing_packs *existing, } string_list_sort(&existing->kept_packs); + string_list_sort(&existing->non_kept_packs); + string_list_sort(&existing->cruft_packs); strbuf_release(&buf); } @@ -806,6 +846,72 @@ static void remove_redundant_bitmaps(struct string_list *include, strbuf_release(&path); } +static int existing_cruft_pack_cmp(const void *va, const void *vb) +{ + struct packed_git *a = *(struct packed_git **)va; + struct packed_git *b = *(struct packed_git **)vb; + + if (a->pack_size < b->pack_size) + return -1; + if (a->pack_size > b->pack_size) + return 1; + return 0; +} + +static void collapse_small_cruft_packs(FILE *in, size_t max_size, + struct existing_packs *existing) +{ + struct packed_git **existing_cruft, *p; + struct strbuf buf = STRBUF_INIT; + size_t total_size = 0; + size_t existing_cruft_nr = 0; + size_t i; + + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); + + for (p = get_all_packs(the_repository); p; p = p->next) { + if (!(p->is_cruft && p->pack_local)) + continue; + + strbuf_reset(&buf); + strbuf_addstr(&buf, pack_basename(p)); + strbuf_strip_suffix(&buf, ".pack"); + + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) + continue; + + if (existing_cruft_nr >= existing->cruft_packs.nr) + BUG("too many cruft packs (found %"PRIuMAX", but knew " + "of %"PRIuMAX")", + (uintmax_t)existing_cruft_nr + 1, + (uintmax_t)existing->cruft_packs.nr); + existing_cruft[existing_cruft_nr++] = p; + } + + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); + + for (i = 0; i < existing_cruft_nr; i++) { + size_t proposed; + + p = existing_cruft[i]; + proposed = st_add(total_size, p->pack_size); + + if (proposed <= max_size) { + total_size = proposed; + fprintf(in, "-%s\n", pack_basename(p)); + } else { + retain_cruft_pack(existing, p); + fprintf(in, "%s\n", pack_basename(p)); + } + } + + for (i = 0; i < existing->non_kept_packs.nr; i++) + fprintf(in, "-%s.pack\n", + existing->non_kept_packs.items[i].string); + + strbuf_release(&buf); +} + static int write_cruft_pack(const struct pack_objects_args *args, const char *destination, const char *pack_prefix, @@ -853,10 +959,14 @@ static int write_cruft_pack(const struct pack_objects_args *args, in = xfdopen(cmd.in, "w"); for_each_string_list_item(item, names) fprintf(in, "%s-%s.pack\n", pack_prefix, item->string); - for_each_string_list_item(item, &existing->non_kept_packs) - fprintf(in, "-%s.pack\n", item->string); - for_each_string_list_item(item, &existing->cruft_packs) - fprintf(in, "-%s.pack\n", item->string); + if (args->max_pack_size && !cruft_expiration) { + collapse_small_cruft_packs(in, args->max_pack_size, existing); + } else { + for_each_string_list_item(item, &existing->non_kept_packs) + fprintf(in, "-%s.pack\n", item->string); + for_each_string_list_item(item, &existing->cruft_packs) + fprintf(in, "-%s.pack\n", item->string); + } for_each_string_list_item(item, &existing->kept_packs) fprintf(in, "%s.pack\n", item->string); fclose(in); @@ -919,6 +1029,8 @@ int cmd_repack(int argc, const char **argv, const char *prefix) PACK_CRUFT), OPT_STRING(0, "cruft-expiration", &cruft_expiration, N_("approxidate"), N_("with --cruft, expire objects older than this")), + OPT_MAGNITUDE(0, "max-cruft-size", &cruft_po_args.max_pack_size, + N_("with --cruft, limit the size of new cruft packs")), OPT_BOOL('d', NULL, &delete_redundant, N_("remove redundant packs, and run git-prune-packed")), OPT_BOOL('f', NULL, &po_args.no_reuse_delta, diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index 69509d0c11..0d6b5c3b27 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -303,6 +303,33 @@ test_expect_success 'gc.bigPackThreshold ignores cruft packs' ' ) ' +cruft_max_size_opts="git repack -d -l --cruft --cruft-expiration=2.weeks.ago" + +test_expect_success 'setup for --max-cruft-size tests' ' + git init cruft--max-size && + ( + cd cruft--max-size && + prepare_cruft_history + ) +' + +test_expect_success '--max-cruft-size sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt git -C cruft--max-size \ + gc --cruft --max-cruft-size=1M && + test_subcommand $cruft_max_size_opts --max-cruft-size=1048576 <trace2.txt +' + +test_expect_success 'gc.maxCruftSize sets appropriate repack options' ' + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ + git -C cruft--max-size -c gc.maxCruftSize=2M gc --cruft && + test_subcommand $cruft_max_size_opts --max-cruft-size=2097152 <trace2.txt && + + GIT_TRACE2_EVENT=$(pwd)/trace2.txt \ + git -C cruft--max-size -c gc.maxCruftSize=2M gc --cruft \ + --max-cruft-size=3M && + test_subcommand $cruft_max_size_opts --max-cruft-size=3145728 <trace2.txt +' + run_and_wait_for_auto_gc () { # We read stdout from gc for the side effect of waiting until the # background gc process exits, closing its fd 9. Furthermore, the diff --git a/t/t7704-repack-cruft.sh b/t/t7704-repack-cruft.sh index d91fcf1af1..dc86ca8269 100755 --- a/t/t7704-repack-cruft.sh +++ b/t/t7704-repack-cruft.sh @@ -5,6 +5,7 @@ test_description='git repack works correctly' . ./test-lib.sh objdir=.git/objects +packdir=$objdir/pack test_expect_success '--expire-to stores pruned objects (now)' ' git init expire-to-now && @@ -127,4 +128,248 @@ test_expect_success '--expire-to stores pruned objects (5.minutes.ago)' ' ) ' +generate_random_blob() { + test-tool genrandom "$@" >blob && + git hash-object -w -t blob blob && + rm blob +} + +pack_random_blob () { + generate_random_blob "$@" && + git repack -d -q >/dev/null +} + +generate_cruft_pack () { + pack_random_blob "$@" >/dev/null && + + ls $packdir/pack-*.pack | xargs -n 1 basename >in && + pack="$(git pack-objects --cruft $packdir/pack <in)" && + git prune-packed && + + echo "$packdir/pack-$pack.mtimes" +} + +test_expect_success '--max-cruft-size creates new packs when above threshold' ' + git init max-cruft-size-large && + ( + cd max-cruft-size-large && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && + git repack --cruft -d && + cruft_foo="$(ls $packdir/pack-*.mtimes)" && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && + git repack --cruft -d --max-cruft-size=1M && + cruft_bar="$(ls $packdir/pack-*.mtimes | grep -v $cruft_foo)" && + + test-tool pack-mtimes $(basename "$cruft_foo") >foo.objects && + test-tool pack-mtimes $(basename "$cruft_bar") >bar.objects && + + grep "^$foo" foo.objects && + test_line_count = 1 foo.objects && + grep "^$bar" bar.objects && + test_line_count = 1 bar.objects + ) +' + +test_expect_success '--max-cruft-size combines existing packs when below threshold' ' + git init max-cruft-size-small && + ( + cd max-cruft-size-small && + test_commit base && + + foo="$(pack_random_blob foo $((1*1024*1024)))" && + git repack --cruft -d && + + bar="$(pack_random_blob bar $((1*1024*1024)))" && + git repack --cruft -d --max-cruft-size=10M && + + cruft=$(ls $packdir/pack-*.mtimes) && + test-tool pack-mtimes $(basename "$cruft") >cruft.objects && + + grep "^$foo" cruft.objects && + grep "^$bar" cruft.objects && + test_line_count = 2 cruft.objects + ) +' + +test_expect_success '--max-cruft-size combines smaller packs first' ' + git init max-cruft-size-consume-small && + ( + cd max-cruft-size-consume-small && + + test_commit base && + git repack -ad && + + cruft_foo="$(generate_cruft_pack foo 524288)" && # 0.5 MiB + cruft_bar="$(generate_cruft_pack bar 524288)" && # 0.5 MiB + cruft_baz="$(generate_cruft_pack baz 1048576)" && # 1.0 MiB + cruft_quux="$(generate_cruft_pack quux 1572864)" && # 1.5 MiB + + test-tool pack-mtimes "$(basename $cruft_foo)" >expect.raw && + test-tool pack-mtimes "$(basename $cruft_bar)" >>expect.raw && + sort expect.raw >expect.objects && + + # repacking with `--max-cruft-size=2M` should combine + # both 0.5 MiB packs together, instead of, say, one of + # the 0.5 MiB packs with the 1.0 MiB pack + ls $packdir/pack-*.mtimes | sort >cruft.before && + git repack -d --cruft --max-cruft-size=2M && + ls $packdir/pack-*.mtimes | sort >cruft.after && + + comm -13 cruft.before cruft.after >cruft.new && + comm -23 cruft.before cruft.after >cruft.removed && + + test_line_count = 1 cruft.new && + test_line_count = 2 cruft.removed && + + # the two smaller packs should be rolled up first + printf "%s\n" $cruft_foo $cruft_bar | sort >expect.removed && + test_cmp expect.removed cruft.removed && + + # ...and contain the set of objects rolled up + test-tool pack-mtimes "$(basename $(cat cruft.new))" >actual.raw && + sort actual.raw >actual.objects && + + test_cmp expect.objects actual.objects + ) +' + +test_expect_success 'setup --max-cruft-size with freshened objects' ' + git init max-cruft-size-freshen && + ( + cd max-cruft-size-freshen && + + test_commit base && + git repack -ad && + + foo="$(generate_random_blob foo 64)" && + test-tool chmtime --get -10000 \ + "$objdir/$(test_oid_to_path "$foo")" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect && + test_cmp expect actual + ) +' + +test_expect_success '--max-cruft-size with freshened objects (loose)' ' + ( + cd max-cruft-size-freshen && + + # regenerate the object, setting its mtime to be more recent + foo="$(generate_random_blob foo 64)" && + test-tool chmtime --get -100 \ + "$objdir/$(test_oid_to_path "$foo")" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect && + test_cmp expect actual + ) +' + +test_expect_success '--max-cruft-size with freshened objects (packed)' ' + ( + cd max-cruft-size-freshen && + + # regenerate the object and store it in a packfile, + # setting its mtime to be more recent + # + # store it alongside another cruft object so that we + # do not create an identical copy of the existing + # cruft pack (which contains $foo). + foo="$(generate_random_blob foo 64)" && + bar="$(generate_random_blob bar 64)" && + foo_pack="$(printf "%s\n" $foo $bar | git pack-objects $packdir/pack)" && + git prune-packed && + + test-tool chmtime --get -10 \ + "$packdir/pack-$foo_pack.pack" >foo.mtime && + + git repack --cruft -d && + + cruft="$(ls $packdir/pack-*.mtimes)" && + test-tool pack-mtimes "$(basename $cruft)" >actual && + echo "$foo $(cat foo.mtime)" >expect.raw && + echo "$bar $(cat foo.mtime)" >>expect.raw && + sort expect.raw >expect && + test_cmp expect actual + ) +' + +test_expect_success '--max-cruft-size with pruning' ' + git init max-cruft-size-prune && + ( + cd max-cruft-size-prune && + + test_commit base && + foo="$(generate_random_blob foo $((1024*1024)))" && + bar="$(generate_random_blob bar $((1024*1024)))" && + baz="$(generate_random_blob baz $((1024*1024)))" && + + test-tool chmtime -10000 "$objdir/$(test_oid_to_path "$foo")" && + + git repack -d --cruft --max-cruft-size=1M && + + # backdate the mtimes of all cruft packs to validate + # that they were rewritten as a result of pruning + ls $packdir/pack-*.mtimes | sort >cruft.before && + for cruft in $(cat cruft.before) + do + mtime="$(test-tool chmtime --get -10000 "$cruft")" && + echo $cruft $mtime >>mtimes || return 1 + done && + + # repack (and prune) with a --max-cruft-size to ensure + # that we appropriately split the resulting set of packs + git repack -d --cruft --max-cruft-size=1M \ + --cruft-expiration=10.seconds.ago && + ls $packdir/pack-*.mtimes | sort >cruft.after && + + for cruft in $(cat cruft.after) + do + old_mtime="$(grep $cruft mtimes | cut -d" " -f2)" && + new_mtime="$(test-tool chmtime --get $cruft)" && + test $old_mtime -lt $new_mtime || return 1 + done && + + test_line_count = 3 cruft.before && + test_line_count = 2 cruft.after && + test_must_fail git cat-file -e $foo && + git cat-file -e $bar && + git cat-file -e $baz + ) +' + +test_expect_success '--max-cruft-size ignores non-local packs' ' + repo="max-cruft-size-non-local" && + git init $repo && + ( + cd $repo && + test_commit base && + generate_random_blob foo 64 && + git repack --cruft -d + ) && + + git clone --reference=$repo $repo $repo-alt && + ( + cd $repo-alt && + + test_commit other && + generate_random_blob bar 64 && + + # ensure that we do not attempt to pick up packs from + # the non-alternated repository, which would result in a + # crash + git repack --cruft --max-cruft-size=1M -d + ) +' + test_done -- 2.42.0.310.g9604b54f73.dirty ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` 2023-10-03 0:44 ` [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` Taylor Blau @ 2023-10-05 12:08 ` Patrick Steinhardt 2023-10-05 17:35 ` Taylor Blau 2023-10-05 20:25 ` Junio C Hamano 2023-10-07 17:20 ` [PATCH] repack: free existing_cruft array after use Jeff King 1 sibling, 2 replies; 21+ messages in thread From: Patrick Steinhardt @ 2023-10-05 12:08 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine [-- Attachment #1: Type: text/plain, Size: 4113 bytes --] On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: [snip] > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt > index 90806fd26a..fa0541b416 100644 > --- a/Documentation/git-gc.txt > +++ b/Documentation/git-gc.txt > @@ -59,6 +59,13 @@ be performed as well. > cruft pack instead of storing them as loose objects. `--cruft` > is on by default. > > +--max-cruft-size=<n>:: > + When packing unreachable objects into a cruft pack, limit the > + size of new cruft packs to be at most `<n>`. Overrides any We should probably mention the unit here, which is bytes. [snip] > @@ -806,6 +846,72 @@ static void remove_redundant_bitmaps(struct string_list *include, > strbuf_release(&path); > } > > +static int existing_cruft_pack_cmp(const void *va, const void *vb) > +{ > + struct packed_git *a = *(struct packed_git **)va; > + struct packed_git *b = *(struct packed_git **)vb; > + > + if (a->pack_size < b->pack_size) > + return -1; > + if (a->pack_size > b->pack_size) > + return 1; > + return 0; > +} > + > +static void collapse_small_cruft_packs(FILE *in, size_t max_size, > + struct existing_packs *existing) > +{ > + struct packed_git **existing_cruft, *p; > + struct strbuf buf = STRBUF_INIT; > + size_t total_size = 0; > + size_t existing_cruft_nr = 0; > + size_t i; > + > + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); > + > + for (p = get_all_packs(the_repository); p; p = p->next) { > + if (!(p->is_cruft && p->pack_local)) > + continue; > + > + strbuf_reset(&buf); > + strbuf_addstr(&buf, pack_basename(p)); > + strbuf_strip_suffix(&buf, ".pack"); > + > + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) > + continue; > + > + if (existing_cruft_nr >= existing->cruft_packs.nr) > + BUG("too many cruft packs (found %"PRIuMAX", but knew " > + "of %"PRIuMAX")", > + (uintmax_t)existing_cruft_nr + 1, > + (uintmax_t)existing->cruft_packs.nr); > + existing_cruft[existing_cruft_nr++] = p; > + } > + > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); > + > + for (i = 0; i < existing_cruft_nr; i++) { > + size_t proposed; > + > + p = existing_cruft[i]; > + proposed = st_add(total_size, p->pack_size); > + > + if (proposed <= max_size) { > + total_size = proposed; > + fprintf(in, "-%s\n", pack_basename(p)); > + } else { > + retain_cruft_pack(existing, p); > + fprintf(in, "%s\n", pack_basename(p)); > + } > + } > + > + for (i = 0; i < existing->non_kept_packs.nr; i++) > + fprintf(in, "-%s.pack\n", > + existing->non_kept_packs.items[i].string); As far as I can see, the non-kept packs are passed to git-pack-objects(1) both in the cases where we do collapse small cruft packs and where we don't. Is there any particular reason why we handle those in both code paths separately instead of merging that logic? Is the ordering of packfiles important here? > + strbuf_release(&buf); > +} > + > static int write_cruft_pack(const struct pack_objects_args *args, > const char *destination, > const char *pack_prefix, > @@ -853,10 +959,14 @@ static int write_cruft_pack(const struct pack_objects_args *args, > in = xfdopen(cmd.in, "w"); > for_each_string_list_item(item, names) > fprintf(in, "%s-%s.pack\n", pack_prefix, item->string); > - for_each_string_list_item(item, &existing->non_kept_packs) > - fprintf(in, "-%s.pack\n", item->string); > - for_each_string_list_item(item, &existing->cruft_packs) > - fprintf(in, "-%s.pack\n", item->string); > + if (args->max_pack_size && !cruft_expiration) { > + collapse_small_cruft_packs(in, args->max_pack_size, existing); > + } else { > + for_each_string_list_item(item, &existing->non_kept_packs) > + fprintf(in, "-%s.pack\n", item->string); > + for_each_string_list_item(item, &existing->cruft_packs) > + fprintf(in, "-%s.pack\n", item->string); > + } > for_each_string_list_item(item, &existing->kept_packs) > fprintf(in, "%s.pack\n", item->string); > fclose(in); Patrick [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` 2023-10-05 12:08 ` Patrick Steinhardt @ 2023-10-05 17:35 ` Taylor Blau 2023-10-05 20:25 ` Junio C Hamano 1 sibling, 0 replies; 21+ messages in thread From: Taylor Blau @ 2023-10-05 17:35 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: git, Jeff King, Junio C Hamano, Eric Sunshine On Thu, Oct 05, 2023 at 02:08:11PM +0200, Patrick Steinhardt wrote: > On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: > [snip] > > diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt > > index 90806fd26a..fa0541b416 100644 > > --- a/Documentation/git-gc.txt > > +++ b/Documentation/git-gc.txt > > @@ -59,6 +59,13 @@ be performed as well. > > cruft pack instead of storing them as loose objects. `--cruft` > > is on by default. > > > > +--max-cruft-size=<n>:: > > + When packing unreachable objects into a cruft pack, limit the > > + size of new cruft packs to be at most `<n>`. Overrides any > > We should probably mention the unit here, which is bytes. Perhaps, though I'm OK with omitting it in the name of brevity, but only since we link off to the relevant section in the git-repack(1) documentation (which does include the units there). > > +static void collapse_small_cruft_packs(FILE *in, size_t max_size, > > + struct existing_packs *existing) > > +{ > > + struct packed_git **existing_cruft, *p; > > + struct strbuf buf = STRBUF_INIT; > > + size_t total_size = 0; > > + size_t existing_cruft_nr = 0; > > + size_t i; > > + > > + ALLOC_ARRAY(existing_cruft, existing->cruft_packs.nr); > > + > > + for (p = get_all_packs(the_repository); p; p = p->next) { > > + if (!(p->is_cruft && p->pack_local)) > > + continue; > > + > > + strbuf_reset(&buf); > > + strbuf_addstr(&buf, pack_basename(p)); > > + strbuf_strip_suffix(&buf, ".pack"); > > + > > + if (!string_list_has_string(&existing->cruft_packs, buf.buf)) > > + continue; > > + > > + if (existing_cruft_nr >= existing->cruft_packs.nr) > > + BUG("too many cruft packs (found %"PRIuMAX", but knew " > > + "of %"PRIuMAX")", > > + (uintmax_t)existing_cruft_nr + 1, > > + (uintmax_t)existing->cruft_packs.nr); > > + existing_cruft[existing_cruft_nr++] = p; > > + } > > + > > + QSORT(existing_cruft, existing_cruft_nr, existing_cruft_pack_cmp); > > + > > + for (i = 0; i < existing_cruft_nr; i++) { > > + size_t proposed; > > + > > + p = existing_cruft[i]; > > + proposed = st_add(total_size, p->pack_size); > > + > > + if (proposed <= max_size) { > > + total_size = proposed; > > + fprintf(in, "-%s\n", pack_basename(p)); > > + } else { > > + retain_cruft_pack(existing, p); > > + fprintf(in, "%s\n", pack_basename(p)); > > + } > > + } > > + > > + for (i = 0; i < existing->non_kept_packs.nr; i++) > > + fprintf(in, "-%s.pack\n", > > + existing->non_kept_packs.items[i].string); > > As far as I can see, the non-kept packs are passed to > git-pack-objects(1) both in the cases where we do collapse small cruft > packs and where we don't. Is there any particular reason why we handle > those in both code paths separately instead of merging that logic? Is > the ordering of packfiles important here? No particularly good reason. The ordering isn't important, and you could do something like this: --- 8< --- diff --git a/builtin/repack.c b/builtin/repack.c index 04770b15fe..6e17fc3f51 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -905,10 +905,6 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size, } } - for (i = 0; i < existing->non_kept_packs.nr; i++) - fprintf(in, "-%s.pack\n", - existing->non_kept_packs.items[i].string); - strbuf_release(&buf); } @@ -959,14 +955,13 @@ static int write_cruft_pack(const struct pack_objects_args *args, in = xfdopen(cmd.in, "w"); for_each_string_list_item(item, names) fprintf(in, "%s-%s.pack\n", pack_prefix, item->string); - if (args->max_pack_size && !cruft_expiration) { + if (args->max_pack_size && !cruft_expiration) collapse_small_cruft_packs(in, args->max_pack_size, existing); - } else { - for_each_string_list_item(item, &existing->non_kept_packs) - fprintf(in, "-%s.pack\n", item->string); + else for_each_string_list_item(item, &existing->cruft_packs) fprintf(in, "-%s.pack\n", item->string); - } + for_each_string_list_item(item, &existing->non_kept_packs) + fprintf(in, "-%s.pack\n", item->string); for_each_string_list_item(item, &existing->kept_packs) fprintf(in, "%s.pack\n", item->string); fclose(in); --- >8 --- But I think that having a small amount of duplication is a fair price to pay for being able to see the whole input given to pack-objects outlined in a single function. Thanks, Taylor ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` 2023-10-05 12:08 ` Patrick Steinhardt 2023-10-05 17:35 ` Taylor Blau @ 2023-10-05 20:25 ` Junio C Hamano 1 sibling, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2023-10-05 20:25 UTC (permalink / raw) To: Patrick Steinhardt; +Cc: Taylor Blau, git, Jeff King, Eric Sunshine Patrick Steinhardt <ps@pks.im> writes: > On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: > [snip] >> diff --git a/Documentation/git-gc.txt b/Documentation/git-gc.txt >> index 90806fd26a..fa0541b416 100644 >> --- a/Documentation/git-gc.txt >> +++ b/Documentation/git-gc.txt >> @@ -59,6 +59,13 @@ be performed as well. >> cruft pack instead of storing them as loose objects. `--cruft` >> is on by default. >> >> +--max-cruft-size=<n>:: >> + When packing unreachable objects into a cruft pack, limit the >> + size of new cruft packs to be at most `<n>`. Overrides any > > We should probably mention the unit here, which is bytes. Good suggestion. I'll tweak "at most `<n>`" to "& bytes" or something. ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH] repack: free existing_cruft array after use 2023-10-03 0:44 ` [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` Taylor Blau 2023-10-05 12:08 ` Patrick Steinhardt @ 2023-10-07 17:20 ` Jeff King 2023-10-09 1:24 ` Taylor Blau 1 sibling, 1 reply; 21+ messages in thread From: Jeff King @ 2023-10-07 17:20 UTC (permalink / raw) To: Taylor Blau; +Cc: git, Junio C Hamano, Patrick Steinhardt, Eric Sunshine On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: > +static void collapse_small_cruft_packs(FILE *in, size_t max_size, > + struct existing_packs *existing) > +{ > + struct packed_git **existing_cruft, *p; > + struct strbuf buf = STRBUF_INIT; > [...] > + > + strbuf_release(&buf); > +} Coverity (using the just-merged-to-next version of the workflow file!) flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in 'next', I think we'd want this on top: -- >8 -- Subject: [PATCH] repack: free existing_cruft array after use We allocate an array of packed_git pointers so that we can sort the list of cruft packs, but we never free the array, causing a small leak. Note that we don't need to free the packed_git structs themselves; they're owned by the repository object. Signed-off-by: Jeff King <peff@peff.net> --- builtin/repack.c | 1 + 1 file changed, 1 insertion(+) diff --git a/builtin/repack.c b/builtin/repack.c index a1a893d952..69e8f302c0 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -955,6 +955,7 @@ static void collapse_small_cruft_packs(FILE *in, size_t max_size, existing->non_kept_packs.items[i].string); strbuf_release(&buf); + free(existing_cruft); } static int write_cruft_pack(const struct pack_objects_args *args, -- 2.42.0.884.gc318dcfe19 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH] repack: free existing_cruft array after use 2023-10-07 17:20 ` [PATCH] repack: free existing_cruft array after use Jeff King @ 2023-10-09 1:24 ` Taylor Blau 2023-10-09 17:28 ` Junio C Hamano 0 siblings, 1 reply; 21+ messages in thread From: Taylor Blau @ 2023-10-09 1:24 UTC (permalink / raw) To: Jeff King; +Cc: git, Junio C Hamano, Patrick Steinhardt, Eric Sunshine On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote: > On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: > > > +static void collapse_small_cruft_packs(FILE *in, size_t max_size, > > + struct existing_packs *existing) > > +{ > > + struct packed_git **existing_cruft, *p; > > + struct strbuf buf = STRBUF_INIT; > > [...] > > + > > + strbuf_release(&buf); > > +} > > Coverity (using the just-merged-to-next version of the workflow file!) > flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in > 'next', I think we'd want this on top: Woohoo! I'm glad that this is already paying dividends. > -- >8 -- > Subject: [PATCH] repack: free existing_cruft array after use > > We allocate an array of packed_git pointers so that we can sort the list > of cruft packs, but we never free the array, causing a small leak. Note > that we don't need to free the packed_git structs themselves; they're > owned by the repository object. Thanks, I can't believe I missed this when writing this function. The fix looks obviously correct to me, so this has my: Acked-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] repack: free existing_cruft array after use 2023-10-09 1:24 ` Taylor Blau @ 2023-10-09 17:28 ` Junio C Hamano 0 siblings, 0 replies; 21+ messages in thread From: Junio C Hamano @ 2023-10-09 17:28 UTC (permalink / raw) To: Taylor Blau; +Cc: Jeff King, git, Patrick Steinhardt, Eric Sunshine Taylor Blau <me@ttaylorr.com> writes: > On Sat, Oct 07, 2023 at 01:20:31PM -0400, Jeff King wrote: >> On Mon, Oct 02, 2023 at 08:44:32PM -0400, Taylor Blau wrote: >> ... >> Coverity (using the just-merged-to-next version of the workflow file!) >> flagged a leak here. Since the topic (tb/repack-max-cruft-size) is in >> 'next', I think we'd want this on top: > > Woohoo! I'm glad that this is already paying dividends. > ... > Thanks, I can't believe I missed this when writing this function. The > fix looks obviously correct to me, so this has my: > > Acked-by: Taylor Blau <me@ttaylorr.com> > > Thanks, Will queue. Thanks. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-10-09 17:28 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-07 21:51 [PATCH 0/2] repack: implement `--cruft-max-size` Taylor Blau 2023-09-07 21:52 ` [PATCH 1/2] t7700: split cruft-related tests to t7704 Taylor Blau 2023-09-08 0:01 ` Eric Sunshine 2023-09-07 21:52 ` [PATCH 2/2] builtin/repack.c: implement support for `--cruft-max-size` Taylor Blau 2023-09-07 23:42 ` Junio C Hamano 2023-09-25 18:01 ` Taylor Blau 2023-09-08 11:21 ` Patrick Steinhardt 2023-10-02 20:30 ` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 0/3] repack: implement `--cruft-max-size` Taylor Blau 2023-10-03 0:44 ` [PATCH v2 1/3] t7700: split cruft-related tests to t7704 Taylor Blau 2023-10-03 0:44 ` [PATCH v2 2/3] builtin/repack.c: parse `--max-pack-size` with OPT_MAGNITUDE Taylor Blau 2023-10-05 11:31 ` Patrick Steinhardt 2023-10-05 17:28 ` Taylor Blau 2023-10-05 20:22 ` Junio C Hamano 2023-10-03 0:44 ` [PATCH v2 3/3] builtin/repack.c: implement support for `--max-cruft-size` Taylor Blau 2023-10-05 12:08 ` Patrick Steinhardt 2023-10-05 17:35 ` Taylor Blau 2023-10-05 20:25 ` Junio C Hamano 2023-10-07 17:20 ` [PATCH] repack: free existing_cruft array after use Jeff King 2023-10-09 1:24 ` Taylor Blau 2023-10-09 17:28 ` Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).