From: Derrick Stolee <stolee@gmail.com>
Date: Tue, 20 Nov 2018 18:45:17 GMT
Subject: Re: [PATCH 1/1] revision.c: use new topo-order logic in tests
Message-ID: <921606a2-3deb-2481-347a-6a2f5571ec9e@gmail.com>
In-Reply-To: <xmqqsgzw9syx.fsf@gitster-ct.c.googlers.com>

On 11/20/2018 1:13 AM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> @@ -3143,6 +3144,9 @@ int prepare_revision_walk(struct rev_info *revs)
>>   		commit_list_sort_by_date(&revs->commits);
>>   	if (revs->no_walk)
>>   		return 0;
>> +	if (revs->limited &&
>> +	    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
>> +		revs->limited = 0;
>>   	if (revs->limited) {
>>   		if (limit_list(revs) < 0)
>>   			return -1;
> That is equivalent to say
>
> 	if (git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
> 		revs->limited = 0;

Not exactly equivalent, because we can use short-circuiting to avoid the 
git_env_bool check, but I see what you mean.

> Wouldn't that make the codepath that involves limit_list()
> completely unreachable while testing, though?

Testing with GIT_TEST_COMMIT_GRAPH=0 would still hit limit_list(). Both 
modes are important to test (for instance, to ensure we still have 
correct behavior without a commit-graph file).

> The title only mentions "topo-order" logic, but the topo-order is
> not the only reason why limited bit can be set, is it?  When showing
> merges, simplifying merges, or post-processing to show ancestry
> path, or showing a bottom-limited revision range, the limited bit is
> automatically set because all of these depend on first calling
> limit_list() and then postprocessing its result.  Doesn't it hurt
> these cases to unconditionally drop limited bit?

You're right that we only want to do this in the topo-order case, so 
perhaps the diff should instead be:

  	if (revs->no_walk)
  		return 0;
+	if (revs->topo_order &&
+	    git_env_bool(GIT_TEST_COMMIT_GRAPH, 0))
+		revs->limited = 0;
  	if (revs->limited) {
  		if (limit_list(revs) < 0)
  			return -1;

Thanks,
-Stolee