On 3/15/2023 9:57 AM, Ævar Arnfjörð Bjarmason wrote: > > On Fri, Mar 10 2023, Derrick Stolee via GitGitGadget wrote: > >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> +test_description='Commit walk performance tests' >> +. ./perf-lib.sh >> + >> +test_perf_large_repo >> + >> +test_expect_success 'setup' ' >> + git for-each-ref --format="%(refname)" "refs/heads/*" "refs/tags/*" >allrefs && >> + sort -r allrefs | head -n 50 >refs && > > Some of the point of test_perf_large_repo is being able to point the > test to an arbitrary sized repo, why "head -n 50" here, instead of just > doing that filtering when preparing the test repo? I think it's too much work to expect that the tester removes all but a small number of refs for testing here. Using all refs on a repo with may refs would be too slow to be helpful. This is especially important when running the entire perf suite on a repo where a large number of refs is _desired_ for some of the other tests. >> +test_expect_success 'ahead-behind requires an argument' ' >> + test_must_fail git for-each-ref \ >> + --format="%(ahead-behind)" 2>err && >> + grep "expected format: %(ahead-behind:<ref>)" err >> +' >> + >> +test_expect_success 'missing ahead-behind base' ' >> + test_must_fail git for-each-ref \ >> + --format="%(ahead-behind:refs/heads/missing)" 2>err && >> + grep "failed to find '\''refs/heads/missing'\''" err >> +' >> + > > Is this grep instead of test_cmp for brevity, or because we'll catch > this late and spew out other output as well? > > I'd think it would be worth testing that we only emit an error. Even if > you don't want a full test_cmp we could check the line count too to > assert that... A full test_cmp is a little more annoying to write, but is a stronger test, so sure. >> +# Run this before doing any signing, so the test has the same results >> +# regardless of the GPG prereq. >> +test_expect_success 'git tag --format with ahead-behind' ' >> + test_when_finished git reset --hard tag-one-line && >> + git commit --allow-empty -m "left" && >> + git tag -a -m left tag-left && >> + git reset --hard HEAD~1 && >> + git commit --allow-empty -m "right" && >> + git tag -a -m left tag-right && > > Do we really need this --allow-empty insted of just using "test_commit"? > I.e. is being TREESAME here important? You missed this in the commit message: >> [...] Also, the >> test in t7004 is carefully located to avoid being dependent on the GPG >> prereq. It also avoids using the test_commit helper, as that will add >> ticks to the time and disrupt the expected timestampes in later tag >> tests. (And I see the "timestampes" typo now.) Thanks, -Stolee
