From: Derrick Stolee <derrickstolee@github.com>
Date: Wed, 15 Mar 2023 16:17:53 GMT
Subject: Re: [PATCH v2 8/8] commit-reach: add tips_reachable_from_bases()
Message-ID: <bbf4921b-851b-7630-c185-f7235f6b28d4@github.com>
In-Reply-To: <230315.864jqmxfd9.gmgdl@evledraar.gmail.com>

On 3/15/2023 10:13 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Mar 10 2023, Derrick Stolee via GitGitGadget wrote:
> 
>> From: Derrick Stolee <derrickstolee@github.com>

(omitting the_repository stuff which will be reflected in v3)

>> -test_perf 'ahead-behind counts: git rev-list' '
>> -	for r in $(cat refs)
>> -	do
>> -		git rev-list --count "HEAD..$r" || return 1
>> -	done
> 
> Why does this change require deleting the old perf test? Your commit 7/8
> notes this test, but here we're deleting it, let's keep it and instead
> note if the results changed, or stayed the same?
>
> More generally, your commit message says:
> 
>> Add extra tests for this behavior in t6600-test-reach.sh as the
>> interesting data shape of that repository can sometimes demonstrate
>> corner case bugs.

This note is about t6600-test-reach.sh, not p1500-graph-walks.sh.

Not only does the previous message note this perf test, it has this to say:

  The 'git rev-list' test exists in this change as a demonstration, but it
  will be removed in the next change to avoid wasting time on this
  comparison.
 
> And here for a supposed optimization commit you're adding new tests, but
> when I try them with the C code at 7/8 they pass.
> 
> So it seems we should add them earlier, and this is a pure-optimization
> commit, but one that's a bit confused about what goes where? :)

I can make it more clear why we are removing it in this commit.

Thanks,
-Stolee