Johannes Schindelin <Johannes.Schindelin@xxxxxx> writes:
>> + 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 */
>> default:
>> revs->max_count--;
>
> ...
Actually that is not correct either. The other patch fixes it
(but not the unintended semantic change to --reverse).
>> + 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.
>
>> + 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.
-
To unsubscribe from this list: 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