Re: [PATCH 2/4] revision traversal: retire BOUNDARY_SHOW

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 6, 2007, 02:29 UTC
Message-ID: <Pine.LNX.4.63.0703060322270.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7v4pozuncr.fsf@assigned-by-dhcp.cox.net>

Hi,

On Mon, 5 Mar 2007, Junio C Hamano wrote:

Show 17 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Just marking the parents uninteresting unless they are prerequisites 
> > would be better, probably.
> >
> >>  
> >>  	i = req_nr;
> >>  	while (i && (commit = get_revision(&revs)))
> >> -		if (commit->object.flags & BOUNDARY_SHOW)
> >> +		if (commit->object.flags & PREREQ_MARK)
> >>  			i--;
> >
> > The above-mentioned idea, then would be: instead of just i--; do
> 
> I explicitly wanted to avoid that.  If the generated bundle had
> prereqs that are not independent of each other, doing that would
> hide some prereqs.

Yes. I wanted to say SHOWN instead of UNINTERESTING, but reading the source again, it seems that add_parents_to_list() is called in get_revision_1() even if the commit is marked SHOWN.

So forget about that idea of mine.

> > This should help performance, as not all reachable commits are traversed 
> > any more.
> 
> You prevented that with "while (i &&" part already, didn't you?

Well, yes. I also wanted to prevent going down all paths, though.

Show 5 quoted lines

> > Wasn't --skip meant for something like gitweb, where you just want to 
> > skip the first <n> commits from the list, but do not want to change 
> > the list otherwise?
> 
> Yes, but does the patch change that?

I _think_ that yes, it could. If the timestamp of HEAD is older than of HEAD^, and you say "git log HEAD^ HEAD", then HEAD^ will be actually shown first, right? If you then say "--skip 1", HEAD will be shown first, and then HEAD^...

Show 12 quoted lines

> >> @@ -1305,6 +1309,9 @@ struct commit *get_revision(struct rev_info *revs)
> >>  	case -1:
> >>  		break;
> >>  	case 0:
> >> +		/* Although we grabbed it, it is not shown. */
> >> +		if (c)
> >> +			c->object.flags &= ~SHOWN;
> >>  		c = NULL;
> >
> > Is this really relevant in practice?
> 
> Absolutely.  But I have further updates on this series.

Sorry for interrupting your patch spree :-)

Ciao, Dscho