On 11/20/2018 1:13 AM, Junio C Hamano wrote:
"Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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
