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

From: Junio C Hamano <junkio@cox.net>
Date: Mar 6, 2007, 02:17 UTC
Message-ID: <7v4pozuncr.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.63.0703060253140.22628@wbgn013.biozentrum.uni-wuerzburg.de>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 23 quoted lines

> On Mon, 5 Mar 2007, Junio C Hamano wrote:
>
>> This removes the flag internally used by revision traversal to
>> decide which commits are indeed boundaries and renames it to
>> CHILD_SHOWN.  builtin-bundle uses the symbol for its
>> verification, but I think the logic it uses it is wrong.  The
>> flag is still useful but it is local to the git-bundle, so it is
>> renamed to PREREQ_MARK.
>
> The idea was to bail out of revision walking when that prerequisite was 
> found, _even_ if it happened to be uninteresting.
>
> 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.

> This should help performance, as not all reachable commits are traversed 
> any more.

You prevented that with "while (i &&" part already, didn't you?

> 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?

Show 11 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.