On Fri, Jul 13, 2012 at 03:12:47PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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. > 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 -- 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