Re: [PATCH] revision walker: Fix --boundary when limited

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

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

Show 9 quoted lines

>> +	if (revs->reverse) {
>> +		l = NULL;
>> +		while ((c = get_revision_1(revs)))
>> +			commit_list_insert(c, &l);
>> +		revs->commits = l;
>> +		revs->reverse = 0;
>>  	}
>
> Clever!

It is not clever, but just is stupid and WRONG. It just shows how little I care about --reverse ;-).

revision_1() is to get the full list without non limit limiters, so the above loop would not even deplete the max_count but literally grabs everything.

Arguably, you can do "rev-list --reverse -2 master" to get the first two commits with this, but it was not intended. We would need to grab (skip + max_count) from get_revision_1() and set max_count to -1 before leaving the if () {} here, or something like that.

Show 9 quoted lines

> I guess that you want to do this instead:
>
> 	case 0:
> 		c = NULL;
> 		/* fall through */
>>  	default:
>>  		revs->max_count--;
>
> ...

Actually that is not correct either. The other patch fixes it (but not the unintended semantic change to --reverse).

>> +	for (l = c->parents; l; l = l->next) {
>
> AFAICT this changes behaviour: c->parents were possibly rewritten.

Well, the behaviour of max with boundary in 'master' did the same thing, as what was in revs->commits are rewritten parents of commits we already returned, didn't it?

Show 10 quoted lines

> But I 
> guess it makes sense showing the rewritten parents as boundary commits, 
> not the real parents.
>
>> +		struct object *p;
>> +		p = &(l->item->object);
>> +		if (p->flags & CHILD_SHOWN)
>
> ... or
> 		if (p->flags & (CHILD_SHOWN | SHOWN))

"If itself is shown it cannot be a boundary, and if we know we marked it as potential boundary we do not have to mark it again"... I think you are right.