On Fri, Jul 13, 2012 at 02:10:54PM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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.
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html