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

Johannes Schindelin [off-list ref] writes:

quoted

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

I guess that you want to do this instead:

	case 0:
		c = NULL;
		/* fall through */

quoted

 	default:
 		revs->max_count--;

...

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

quoted

+	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?

But I 
guess it makes sense showing the rewritten parents as boundary commits, 
not the real parents.

quoted

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