Re: [PATCH 2/4] builtin/pack-objects.c: support `--max-pack-size` with `--cruft`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 29, 2023, 17:42 UTC
- Message-ID
- <xmqqr0nld9u9.fsf@gitster.g>
- In-Reply-To
- <b6d945197faaef8243bddf78f672a340404e6ac4.1693262936.git.me@ttaylorr.com>
Taylor Blau <me@ttaylorr.com> writes:
Show 8 quoted lines
> When pack-objects learned the `--cruft` option back in b757353676 > (builtin/pack-objects.c: --cruft without expiration, 2022-05-20), we > explicitly forbade `--cruft` with `--max-pack-size`. > > At the time, there was no specific rationale given in the patch for not > supporting the `--max-pack-size` option with `--cruft`. (As best I can > remember, it's because we were trying to push users towards only ever > having a single cruft pack, but I cannot be sure).
I am reasonably sure it was the case but then I do not recall we ever discussing how the second cruft pack gets consolidated into one by combining it with the existing one.
Show 13 quoted lines
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c > index 868efe7e0f..a046994a43 100644 > --- a/builtin/pack-objects.c > +++ b/builtin/pack-objects.c > @@ -1190,8 +1190,7 @@ static void write_pack_file(void) > unsigned char hash[GIT_MAX_RAWSZ]; > char *pack_tmp_name = NULL; > > - if (pack_to_stdout) > - f = hashfd_throughput(1, "<stdout>", progress_state); > + if (pack_to_stdout) f = hashfd_throughput(1, "<stdout>", progress_state); > else > f = create_tmp_packfile(&pack_tmp_name);
An unintended change, I am sure ;-)
It is very surprising that absolutely no real change is needed to allow cruft packs to honor the settings, other than removing the seemingly artificial inter-option-compatibility roadblocks (all hunks for it omitted above as they were trivially obvious). I am sure the first hunk to fold an "if" statement onto a single line is not what makes the feature to actually work ;-)
Show 81 quoted lines
> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index 45667d4999..fc5fedbe9b 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -573,23 +573,54 @@ test_expect_success 'cruft repack with no reachable objects' '
> )
> '
>
> -test_expect_success 'cruft repack ignores --max-pack-size' '
> +write_blob () {
> + test-tool genrandom "$@" >in &&
> + git hash-object -w -t blob in
> +}
> +
> +find_pack () {
> + for idx in $(ls $packdir/pack-*.idx)
> + do
> + git show-index <$idx >out &&
> + if grep -q "$1" out
> + then
> + echo $idx
> + fi || return 1
> + done
> +}
> +
> +test_expect_success 'cruft repack with --max-pack-size' '
> git init max-pack-size &&
> (
> cd max-pack-size &&
> test_commit base &&
> +
> # two cruft objects which exceed the maximum pack size
> - test-tool genrandom foo 1048576 | git hash-object --stdin -w &&
> - test-tool genrandom bar 1048576 | git hash-object --stdin -w &&
> + foo=$(write_blob foo 1048576) &&
> + bar=$(write_blob bar 1048576) &&
> + test-tool chmtime --get -1000 \
> + "$objdir/$(test_oid_to_path $foo)" >foo.mtime &&
> + test-tool chmtime --get -2000 \
> + "$objdir/$(test_oid_to_path $bar)" >bar.mtime &&
> git repack --cruft --max-pack-size=1M &&
> find $packdir -name "*.mtimes" >cruft &&
> - test_line_count = 1 cruft &&
> - test-tool pack-mtimes "$(basename "$(cat cruft)")" >objects &&
> - test_line_count = 2 objects
> + test_line_count = 2 cruft &&
> +
> + foo_mtimes="$(basename $(find_pack $foo) .idx).mtimes" &&
> + bar_mtimes="$(basename $(find_pack $bar) .idx).mtimes" &&
> + test-tool pack-mtimes $foo_mtimes >foo.actual &&
> + test-tool pack-mtimes $bar_mtimes >bar.actual &&
> +
> + echo "$foo $(cat foo.mtime)" >foo.expect &&
> + echo "$bar $(cat bar.mtime)" >bar.expect &&
> +
> + test_cmp foo.expect foo.actual &&
> + test_cmp bar.expect bar.actual &&
> + test "$foo_mtimes" != "$bar_mtimes"
> )
> '
>
> -test_expect_success 'cruft repack ignores pack.packSizeLimit' '
> +test_expect_success 'cruft repack with pack.packSizeLimit' '
> (
> cd max-pack-size &&
> # repack everything back together to remove the existing cruft
> @@ -599,9 +630,12 @@ test_expect_success 'cruft repack ignores pack.packSizeLimit' '
> # ensure the same post condition is met when --max-pack-size
> # would otherwise be inferred from the configuration
> find $packdir -name "*.mtimes" >cruft &&
> - test_line_count = 1 cruft &&
> - test-tool pack-mtimes "$(basename "$(cat cruft)")" >objects &&
> - test_line_count = 2 objects
> + test_line_count = 2 cruft &&
> + for pack in $(cat cruft)
> + do
> + test-tool pack-mtimes "$(basename $pack)" >objects &&
> + test_line_count = 1 objects || return 1
> + done
> )
> '