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

From: Jeff King <peff@peff.net>
Date: Jul 14, 2012, 08:10 UTC
Message-ID: <20120714081033.GA32547@sigill.intra.peff.net>
In-Reply-To: <7vwr2771k0.fsf@alter.siamese.dyndns.org>

On Fri, Jul 13, 2012 at 03:12:47PM -0700, Junio C Hamano wrote:

Show 18 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> In principle, SHOWN is only given when get_revision_internal gives
> the commit back to be shown, and the parents of the returned commit
> are painted CHILD_SHOWN.  This should be the only place to paint
> commit as CHILD_SHOWN.
> 
> A handful of places set the bit to commits that would be shown if
> some options that further limit what is shown by topological
> property (e.g. --left-only, --cherry-pick), which may cause that a
> parent of a commit that was omitted due to these conditions may
> later be marked incorrectly as a boundary inside
> create_boundary_commit_list().

I think what confused me is that I thought I saw get_revision_1 setting SHOWN in the reflog case. But of course it is _clearing_ the flag, since the reflog walker may show the commit multiple times. And no other code paths in get_revision_1 set it (nor should they, since as you say, it is about us actually handing back the commit to be shown).

> BOUNDARY is only given in create_boundary_commit_list() using
> CHILD_SHOWN and SHOWN, and that should happen only once when
> get_revision_1() runs out of the commits.

Yeah, agreed.

Show 8 quoted lines

> By the way, cherry_pick_list() pays attention to BOUNDARY, but I
> think it is written overly defensively to protect itself from future
> callsites.  With the current code structure, it is only called from
> limit_list() and get_revision_*() functions are never called until
> limit_list() returns (and again create_boundary_commit_list() that
> is called from get_revision_internal() is the only place that sets
> BOUNDARY, so the commits cherry_pick_list() sees would never have
> that bit set.

I think we wouldn't be impacting it even so, because the commits that make it to create_boundary_commit_list (and therefore get marked with BOUNDARY) should be identical. We only put items into the boundary_commits list when we are about to return them from get_revision_internal, and with my patch that will not change (because prior to my patch, we would have thrown away the commit after checking max_count anyway).

So I think after looking again that this change is the right thing to do, and there shouldn't be any side effects. Thanks for the careful review.

-Peff