Re: [PATCH] revision: avoid work after --max-count is reached

From: Jeff King <peff@peff.net>
Date: Jul 13, 2012, 21:20 UTC
Message-ID: <20120713212050.GB10767@sigill.intra.peff.net>
In-Reply-To: <7v7gu78izl.fsf@alter.siamese.dyndns.org>

On Fri, Jul 13, 2012 at 02:10:54PM -0700, Junio C Hamano wrote:

Show 17 quoted lines

> Jeff King <peff@peff.net> 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.

Show 8 quoted lines

> 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