Linus Torvalds <torvalds@xxxxxxxx> writes:
> 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...
-
: 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