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

On Thu, 30 Mar 2006, Junio C Hamano wrote:

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.

They don't.

What ends up not working very well at all is the combination of 
"--topo-order" and the output filter in get_revision. It will return NULL 
when we see the first commit out of date-order, even if we have other 
commits coming.

So we really should do the "past the date order" thing in get_revision() 
only if we have _not_ done it already in limit_list().

Something like this.

The easiest way to test this is with just

	gitk --since=3.days.ago

on the kernel tree. Without this patch, it tends to be pretty obviously 
broken.

Signed-off-by: Linus Torvalds <torvalds@osdl.org>

		Linus

---

diff --git a/revision.c b/revision.c
index a8a54b6..558ed01 100644
--- a/revision.c
+++ b/revision.c

@@ -783,10 +783,14 @@ struct commit *get_revision(struct rev_i
 
 		/*
 		 * If we haven't done the list limiting, we need to look at
-		 * the parents here
+		 * the parents here. We also need to do the date-based limiting
+		 * that we'd otherwise have done in limit_list().
 		 */
-		if (!revs->limited)
+		if (!revs->limited) {
+			if (revs->max_age != -1 && (commit->date < revs->max_age))
+				continue;
 			add_parents_to_list(revs, commit, &revs->commits);
+		}
 		if (commit->object.flags & SHOWN)
 			continue;
 		if (!(commit->object.flags & BOUNDARY) &&

@@ -794,8 +798,6 @@ struct commit *get_revision(struct rev_i
 			continue;
 		if (revs->min_age != -1 && (commit->date > revs->min_age))
 			continue;
-		if (revs->max_age != -1 && (commit->date < revs->max_age))
-			return NULL;
 		if (revs->no_merges &&
 		    commit->parents && commit->parents->next)
 			continue;

This just moves code around to consolidate the part that sets
revs->limited to one place based on various flags.

Signed-off-by: Junio C Hamano <redacted>

---
 * Just the preparation for the next step...

 revision.c |   20 +++++++-------------
 1 files changed, 7 insertions(+), 13 deletions(-)

53069686601d156dea3787a100ffc4e35c78040f

diff --git a/revision.c b/revision.c
index 07cc86f..cdd29c5 100644
--- a/revision.c
+++ b/revision.c

@@ -552,32 +552,26 @@ int setup_revisions(int argc, const char
 			}
 			if (!strncmp(arg, "--max-age=", 10)) {
 				revs->max_age = atoi(arg + 10);
-				revs->limited = 1;
 				continue;
 			}
-			if (!strncmp(arg, "--min-age=", 10)) {
-				revs->min_age = atoi(arg + 10);
-				revs->limited = 1;
-				continue;
-			}
 			if (!strncmp(arg, "--since=", 8)) {
 				revs->max_age = approxidate(arg + 8);
-				revs->limited = 1;
 				continue;
 			}
 			if (!strncmp(arg, "--after=", 8)) {
 				revs->max_age = approxidate(arg + 8);
-				revs->limited = 1;
 				continue;
 			}
+			if (!strncmp(arg, "--min-age=", 10)) {
+				revs->min_age = atoi(arg + 10);
+				continue;
+			}
 			if (!strncmp(arg, "--before=", 9)) {
 				revs->min_age = approxidate(arg + 9);
-				revs->limited = 1;
 				continue;
 			}
 			if (!strncmp(arg, "--until=", 8)) {
 				revs->min_age = approxidate(arg + 8);
-				revs->limited = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--all")) {

@@ -596,13 +590,11 @@ int setup_revisions(int argc, const char
 			}
 			if (!strcmp(arg, "--topo-order")) {
 				revs->topo_order = 1;
-				revs->limited = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--date-order")) {
 				revs->lifo = 0;
 				revs->topo_order = 1;
-				revs->limited = 1;
 				continue;
 			}
 			if (!strcmp(arg, "--parents")) {

@@ -644,7 +636,6 @@ int setup_revisions(int argc, const char
 			}
 			if (!strcmp(arg, "--unpacked")) {
 				revs->unpacked = 1;
-				revs->limited = 1;
 				continue;
 			}
 			*unrecognized++ = arg;

@@ -707,6 +698,9 @@ int setup_revisions(int argc, const char
 		commit = get_commit_reference(revs, def, sha1, 0);
 		add_one_commit(commit, revs);
 	}
+
+	if ((revs->max_age != -1) || revs->topo_order || revs->unpacked)
+		revs->limited = 1;
 
 	if (revs->prune_data) {
 		diff_tree_setup_paths(revs->prune_data);

-- 
1.3.0.rc1.gf385

This makes git log --since=7.days to be streamable.

Signed-off-by: Junio C Hamano <redacted>

---

 revision.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

bbbc8c3a8d455e1f5d15c3764eba70250b5479e9

diff --git a/revision.c b/revision.c
index cdd29c5..728b6d1 100644
--- a/revision.c
+++ b/revision.c

@@ -699,7 +699,7 @@ int setup_revisions(int argc, const char
 		add_one_commit(commit, revs);
 	}
 
-	if ((revs->max_age != -1) || revs->topo_order || revs->unpacked)
+	if (revs->topo_order || revs->unpacked)
 		revs->limited = 1;
 
 	if (revs->prune_data) {

-- 
1.3.0.rc1.gf385