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

Linus Torvalds [off-list ref] writes:

So the following is wrong:

quoted

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):

quoted

@@ -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...