Re: [PATCH/RFC 2/2] Make path-limiting be incremental when possible.

From: Junio C Hamano <junkio@cox.net>
Date: Mar 31, 2006, 07:02 UTC
Message-ID: <7vr74jw0zj.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <Pine.LNX.4.64.0603302225160.27203@g5.osdl.org>

Linus Torvalds <torvalds@osdl.org> writes:

Show 28 quoted lines

> So the following is wrong:
>
>> diff --git a/revision.c b/revision.c
>> index 0e3f074..a55c0d1 100644
>> --- a/revision.c
>> +++ b/revision.c
>> @@ -404,10 +404,6 @@ static void limit_list(struct rev_info *
>>  		list = list->next;
>>  		free(entry);
>>  
>> -		if (revs->max_age != -1 && (commit->date < revs->max_age))
>> -			obj->flags |= UNINTERESTING;
>> -		if (revs->unpacked && has_sha1_pack(obj->sha1))
>> -			obj->flags |= UNINTERESTING;
>>  		add_parents_to_list(revs, commit, &list);
>>  		if (obj->flags & UNINTERESTING) {
>>  			mark_parents_uninteresting(commit);
>...
> ..but the later part of the patch looks fine (modulo testing, of course):
>
>> @@ -786,7 +773,9 @@ struct commit *get_revision(struct rev_i
>>  		if (revs->min_age != -1 && (commit->date > revs->min_age))
>>  			continue;
>>  		if (revs->max_age != -1 && (commit->date < revs->max_age))
>> -			return NULL;
>> +			continue;
>> +		if (revs->unpacked && has_sha1_pack(commit->object.sha1))
>> +			continue;

OK, so let's say I agree with you that --unpacked and --since are "stop early" rules. I fully agree with that from usability and implementation point of view, but I now wonder if the "output filter" in get_revision() and "stop early" in limit_list() would do the same thing. That is, if we are _otherwise_ limited, limit_list() would use them for "stop early" rule, but if we end up not calling limit_list() we would simply filter them and keep going (which is what my patch did for both timestamp and packedness), or we could also stop there.

I am not sure which one is correct, but I suspect whichever we do in get_revision() we would get inconsistent results between a case where we call limit_list() and we do not. That is, the set of commits we are going to show when --(topo|date)-order is given and not given can be different. Is this acceptable?

I would say, from the implementation point of view, it should be tolerated, but...