| Message ID | 20220803205721.3686361-2-emilyshaffer@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | let feature.experimental imply gc.cruftPacks=true | expand |
Emily Shaffer <emilyshaffer@google.com> writes: > In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via > loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'. > '--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that > doesn't check whether a lone gc run generates these cruft packs. > 'gc.cruftPacks' is never exercised. > > Add some tests to exercise these options to gc in the gc test suite. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index cd6c53360d..e4c2c3583d 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e > grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out > ' > > +test_expect_success 'gc --cruft generates a cruft pack' ' > + git init crufts && > + test_when_finished "rm -fr crufts" && > + ( > + cd crufts && > + test_commit base && > + > + test_commit --no-tag foo && > + test_commit --no-tag bar && > + git reset HEAD^^ && > + > + git gc --cruft && > + > + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && What guarantees that we will have one pack-*.mtimes? I do not mind if we reliably diagnosed it as an error when "git gc --cruft" created two cruft packs, but I do mind if this call to basename receives two files plus .mtimes suffix and misbehaves. Is the fact that it is accompanied by a .mtimes file the only clue that a pack is a "cruft" pack? Given that the usefulness of mtimes based expiration approach is doubted, do we want to rely on it (and having to redesign the test)? I think the right test would be to * make a list of all "in use" objects; * see if there is one (or more) packfile that does not contain any "in use" objects (look at their .idx file). If all packfiles are packs with objects that are still in use, then we did not create a cruft pack. > + test_path_is_file .git/objects/pack/$cruft.pack DQuote the whole thing, i.e. test_path_is_file ".git/objects/pack/$cruft.pack"
On Wed, Aug 03 2022, Emily Shaffer wrote: > In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via > loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'. > '--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that > doesn't check whether a lone gc run generates these cruft packs. > 'gc.cruftPacks' is never exercised. > > Add some tests to exercise these options to gc in the gc test suite. > > Signed-off-by: Emily Shaffer <emilyshaffer@google.com> > --- > t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 36 insertions(+) > > diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh > index cd6c53360d..e4c2c3583d 100755 > --- a/t/t6500-gc.sh > +++ b/t/t6500-gc.sh > @@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e > grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out > ' > > +test_expect_success 'gc --cruft generates a cruft pack' ' > + git init crufts && > + test_when_finished "rm -fr crufts" && Let's "test_when_finished" first, then "git init", the point is to clean up the directory if we fail. > + ( > + cd crufts && > + test_commit base && > + > + test_commit --no-tag foo && > + test_commit --no-tag bar && > + git reset HEAD^^ && > + > + git gc --cruft && > + > + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && > + test_path_is_file .git/objects/pack/$cruft.pack > + ) > +' ...this... > +test_expect_success 'gc.cruftPacks=true generates a cruft pack' ' > + git init crufts && > + test_when_finished "rm -fr crufts" && > + ( > + cd crufts && > + test_commit base && > + > + test_commit --no-tag foo && > + test_commit --no-tag bar && > + git reset HEAD^^ && > + > + git -c gc.cruftPacks=true gc && > + > + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && > + test_path_is_file .git/objects/pack/$cruft.pack > + ) > +' > + ...whole thing seems to be copy/pasted aside from the git options. If so let's factor this into a trivial helper that invokes git "$@", then call it with "gc --cruft" and "-c ..."?
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> +test_expect_success 'gc --cruft generates a cruft pack' ' >> + git init crufts && >> + test_when_finished "rm -fr crufts" && > > Let's "test_when_finished" first, then "git init", the point is to clean > up the directory if we fail. Good advice. We say "rm -fr" not "rm -r" there because we do not want to see a failure to remove if "git init" failed before it manages to create the directory ;-) > >> + ( >> + cd crufts && >> + test_commit base && >> + >> + test_commit --no-tag foo && >> + test_commit --no-tag bar && >> + git reset HEAD^^ && >> + >> + git gc --cruft && >> + >> + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && >> + test_path_is_file .git/objects/pack/$cruft.pack >> + ) >> +' > > ...this... > >> +test_expect_success 'gc.cruftPacks=true generates a cruft pack' ' >> + git init crufts && >> + test_when_finished "rm -fr crufts" && >> + ( >> + cd crufts && >> + test_commit base && >> + >> + test_commit --no-tag foo && >> + test_commit --no-tag bar && >> + git reset HEAD^^ && >> + >> + git -c gc.cruftPacks=true gc && >> + >> + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && >> + test_path_is_file .git/objects/pack/$cruft.pack >> + ) >> +' >> + > > ...whole thing seems to be copy/pasted aside from the git options. > > If so let's factor this into a trivial helper that invokes git "$@", > then call it with "gc --cruft" and "-c ..."? With shell, passing "here is a series of commands to be run in the middle of a boilerplate sequence" is indeed easy to write, but it gets harder to follow and quote correctly, which is why I'd rather not see that pattern overused. A pair of helper functions, one of which prepares a sample history to be used, and the other checks if we created one (or more) cruft packs, may achieve the same conciseness while remaining to be more readable. I.e. test_when_finished "rm -fr crufts" && git init crufts && ( cd crufts && prepare_history && git -c gc.cruftPacks=true gc && cruft_packs_exist ) perhaps?
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh index cd6c53360d..e4c2c3583d 100755 --- a/t/t6500-gc.sh +++ b/t/t6500-gc.sh @@ -202,6 +202,42 @@ test_expect_success 'one of gc.reflogExpire{Unreachable,}=never does not skip "e grep -E "^trace: (built-in|exec|run_command): git reflog expire --" trace.out ' +test_expect_success 'gc --cruft generates a cruft pack' ' + git init crufts && + test_when_finished "rm -fr crufts" && + ( + cd crufts && + test_commit base && + + test_commit --no-tag foo && + test_commit --no-tag bar && + git reset HEAD^^ && + + git gc --cruft && + + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && + test_path_is_file .git/objects/pack/$cruft.pack + ) +' + +test_expect_success 'gc.cruftPacks=true generates a cruft pack' ' + git init crufts && + test_when_finished "rm -fr crufts" && + ( + cd crufts && + test_commit base && + + test_commit --no-tag foo && + test_commit --no-tag bar && + git reset HEAD^^ && + + git -c gc.cruftPacks=true gc && + + cruft=$(basename $(ls .git/objects/pack/pack-*.mtimes) .mtimes) && + test_path_is_file .git/objects/pack/$cruft.pack + ) +' + 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
In 5b92477f89 (builtin/gc.c: conditionally avoid pruning objects via loose, 2022-05-20) gc learned to respect '--cruft' and 'gc.cruftPacks'. '--cruft' is exercised in t5329-pack-objects-cruft.sh, but in a way that doesn't check whether a lone gc run generates these cruft packs. 'gc.cruftPacks' is never exercised. Add some tests to exercise these options to gc in the gc test suite. Signed-off-by: Emily Shaffer <emilyshaffer@google.com> --- t/t6500-gc.sh | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+)