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.
> 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.
> 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"));
> @@ 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.
> + 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...
> ++ 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.
> ++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
