Re: [PATCH 1/1] revision.c: use new topo-order logic in tests

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 20, 2018, 06:13 UTC
Message-ID: <xmqqsgzw9syx.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <ab7c2dd46fb72523b865ecf34204c7ae31dee416.1542654209.git.gitgitgadget@gmail.com>

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 10 quoted lines

> @@ -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;

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

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?