Re: [PATCH v2 7/8] for-each-ref: add ahead-behind format atom

[Derrick Stolee <derrickstolee@github.com>]

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@github.com>

>> +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