Re: [PATCH v2] builtin/pack-objects.c: introduce `pack.extraCruftTips`
- From
Derrick Stolee <derrickstolee@github.com>
- Date
- May 3, 2023, 14:01 UTC
- Message-ID
- <b29ff03e-5504-af04-72c6-f98abde1442f@github.com>
- In-Reply-To
- <73ad7b90e1fe6c15f41ff828651f7ab06076ffd8.1683072587.git.me@ttaylorr.com>
On 5/2/2023 8:09 PM, Taylor Blau wrote:
> This patch introduces a new multi-valued configuration option, > `pack.extraCruftTips` as an alternative means to mark certain objects in > the cruft pack as rescued, even if they would otherwise be pruned.
Show 19 quoted lines
> Range-diff against v1: > 1: 8af478ebe3 ! 1: 73ad7b90e1 builtin/pack-objects.c: introduce `pack.extraCruftTips` > @@ Commit message > > - to point a reference at them, which may be undesirable or > infeasible, > + - to track them via the reflog, which may be undesirable since the > + reflog's lifetime is limited to that of the reference it's tracking > + (and callers may want to keep those unreachable objects around for > + longer) > - to extend the grace period, which may keep around other objects that > the caller *does* want to discard, > - or, to force the caller to construct the pack of objects they want > @@ Documentation/config/pack.txt: pack.deltaCacheLimit:: > ++ > +Output must contain exactly one hex object ID per line, and nothing > +else. Objects which cannot be found in the repository are ignored. > ++Multiple hooks are supported, but all must exit successfully, else no > ++cruft pack will be generated.
Thanks for these updates.
Show 19 quoted lines
> pack.threads::
> Specifies the number of threads to spawn when searching for best
> @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct
> + add_pending_object(revs, obj, "");
> + }
> +
> ++ ret = finish_command(&cmd);
> ++
> +done:
> + if (out)
> + fclose(out);
> @@ builtin/pack-objects.c: static void enumerate_and_traverse_cruft_objects(struct
> + for (i = 0; i < programs->nr; i++) {
> + ret = enumerate_extra_cruft_tips_1(&revs,
> + programs->items[i].string);
> -+ if (!ret)
> ++ if (ret)
> + break;
> + }If we have a failure, then we stop immediately and report a failure (from this bit outside the range-diff's context):
> + if (ret)
> + die(_("unable to enumerate additional cruft tips"));Show 6 quoted lines
> @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v > + cruft_new="$(git rev-parse HEAD)" && > + > + git checkout main && > -+ git branch -D old new && > ++ git branch -D discard old new &&
Good update.
Show 14 quoted lines
> + git reflog expire --all --expire=all &&
> +
> + # mark cruft.old with an mtime that is many minutes
> @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'cruft objects are freshend v
> + mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> + git show-index <${mtimes%.mtimes}.idx >cruft &&
> + cut -d" " -f2 cruft | sort >cruft.actual &&
> -+ test_cmp cruft.expect cruft.actual
> ++ test_cmp cruft.expect cruft.actual &&
> ++
> ++ # Ensure that the "old" objects are removed after
> ++ # dropping the pack.extraCruftTips hook.
> ++ git config --unset pack.extraCruftTips &&
> ++ git repack --cruft --cruft-expiration=now -d &&Double-checking that removing the tips correctly removes the objects at this time is good, but...
Show 15 quoted lines
> ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> ++ git show-index <${mtimes%.mtimes}.idx >cruft &&
> ++ cut -d" " -f2 cruft | sort >cruft.actual &&
> ++
> ++ git rev-list --objects --no-object-names $cruft_new >cruft.raw &&
> ++ cp cruft.expect cruft.old &&
> ++ sort cruft.raw >cruft.expect &&
> ++ test_cmp cruft.expect cruft.actual &&
> ++
> ++ # ensure objects which are no longer in the cruft pack were
> ++ # removed from the repository
> ++ for object in $(comm -13 cruft.expect cruft.old)
> ++ do
> ++ test_must_fail git cat-file -t $object || return 1
> ++ done...it would be good to do this check before the removal of the hook to be sure the objects from 'discard' are removed as part of the step with the hook. I can imagine a world where the hook-based approach erroneously keeps the 'discard' objects in the non-cruft pack only for them to be deleted by the repack where hooks are disabled, and having a test would help guarantee we don't live in that hypothetical world.
I would also be satisfied with checking just the commits that were previously referenced by 'discard' and 'old', but this more thorough check is also good.
Show 50 quoted lines
> ++test_expect_success 'multi-valued pack.extraCruftTips' '
> ++ git init repo &&
> ++ test_when_finished "rm -fr repo" &&
> ++ (
> ++ cd repo &&
> ++
> ++ test_commit base &&
> ++ git branch -M main &&
> ++
> ++ git checkout --orphan cruft.a &&
> ++ git rm -fr . &&
> ++ test_commit --no-tag cruft.a &&
> ++ cruft_a="$(git rev-parse HEAD)" &&
> ++
> ++ git checkout --orphan cruft.b &&
> ++ git rm -fr . &&
> ++ test_commit --no-tag cruft.b &&
> ++ cruft_b="$(git rev-parse HEAD)" &&
> ++
> ++ git checkout main &&
> ++ git branch -D cruft.a cruft.b &&
> ++ git reflog expire --all --expire=all &&
> ++
> ++ echo "echo $cruft_a" | write_script extra-tips.a &&
> ++ echo "echo $cruft_b" | write_script extra-tips.b &&
> ++ echo "false" | write_script extra-tips.c &&
> ++
> ++ git rev-list --objects --no-object-names $cruft_a $cruft_b \
> ++ >cruft.raw &&
> ++ sort cruft.raw >cruft.expect &&
> ++
> ++ # ensure that each extra cruft tip is saved by its
> ++ # respective hook
> ++ git config --add pack.extraCruftTips ./extra-tips.a &&
> ++ git config --add pack.extraCruftTips ./extra-tips.b &&
> ++ git repack --cruft --cruft-expiration=now -d &&
> ++
> ++ mtimes="$(ls .git/objects/pack/pack-*.mtimes)" &&
> ++ git show-index <${mtimes%.mtimes}.idx >cruft &&
> ++ cut -d" " -f2 cruft | sort >cruft.actual &&
> ++ test_cmp cruft.expect cruft.actual &&
> ++
> ++ # ensure that a dirty exit halts cruft pack generation
> ++ git config --add pack.extraCruftTips ./extra-tips.c &&
> ++ test_must_fail git repack --cruft -d 2>err &&
> ++ grep "unable to enumerate additional cruft tips" err &&
> ++
> ++ # and that the existing cruft pack is left alone
> ++ test_path_is_file "$mtimes"
> + )This new test looks good to me.
Thanks, -Stolee