Jeff King [off-list ref] writes:
There's no need to make this get_revision_1 call when our
counter runs out. If we are not in --boundary mode, we will
just throw away the result and immediately return NULL. If
we are in --boundary mode, then we will still throw away the
result, and then start showing the boundary commits.
However, as git_revision_1 does not impact the boundary
list, it should not have an impact.
We used to reset 'c' to NULL ("throw away the result"), run
create_boundary_commit_list(), and ask ourselves to pop the boundary
it computed.
Now we don't call get_revision_1() and leave 'c' to NULL as
initialized ("avoid work"), and do the same.
The state create_boundary_commit_list() sees would slightly be
different, as we used to dig one level deeper, smudging more commits
with bits, but the only bits that may be smudged by this digging
that may matter in create_commit_list() is CHILD_SHOWN and SHOWN,
but by definition the commits around the commit the extra call to
get_revision_1() would have returned are marked with neither during
that extra call, so I think this conversion does not affect the
boundary list.
So I think I like this change. If anything, it makes the loop
structure simpler and a bit easier to understand.
On Fri, Jul 13, 2012 at 02:10:54PM -0700, Junio C Hamano wrote:
Jeff King [off-list ref] writes:
quoted
There's no need to make this get_revision_1 call when our
counter runs out. If we are not in --boundary mode, we will
just throw away the result and immediately return NULL. If
we are in --boundary mode, then we will still throw away the
result, and then start showing the boundary commits.
However, as git_revision_1 does not impact the boundary
list, it should not have an impact.
We used to reset 'c' to NULL ("throw away the result"), run
create_boundary_commit_list(), and ask ourselves to pop the boundary
it computed.
Now we don't call get_revision_1() and leave 'c' to NULL as
initialized ("avoid work"), and do the same.
Right.
The state create_boundary_commit_list() sees would slightly be
different, as we used to dig one level deeper, smudging more commits
with bits, but the only bits that may be smudged by this digging
that may matter in create_commit_list() is CHILD_SHOWN and SHOWN,
but by definition the commits around the commit the extra call to
get_revision_1() would have returned are marked with neither during
that extra call, so I think this conversion does not affect the
boundary list.
Yeah, this was my analysis, too. Though reading get_revision-1, it seems
like we can actually set SHOWN, but I wasn't able to trigger any change
of behavior in practice. I think it is because we must set both SHOWN
and BOUNDARY to have an effect, and we do not do so.
So the only questionable thing would be: are there commits with BOUNDARY
set but not SHOWN that could be affected by calling get_revision_1? For
that matter, if such a commit existed, would the current behavior even
be correct? We would not have actually shown the commit, so if such a
case did exist, I wonder if we would be fixing a bug.
-Peff