Hi,
On Mon, 5 Mar 2007, Junio C Hamano wrote:
> - rip out the boundary logic from the commit walker. Placing
> "negative" commits in the revs->commits list was Ok if all we
> cared about "boundary" was the UNINTERESTING limiting case,
> but conceptually it was wrong.
I agree.
> It loses more code than it adds, even when the new gc_boundary()
> function, which is purely for early optimization, is counted.
I have the slight suspicion that it might be faster if gc_boundary() was
not called at all... But that's probably a subject for later.
> Note that this patch is purely for eyeballing and discussion
> only. It breaks git-bundle's verify logic because the logic
> does not use BOUNDARY_SHOW flag for its internal computation
> anymore. After we correct it not to attempt to affect the
> boundary processing by setting the BOUNDARY_SHOW flag, we can
> remove BOUNDARY_SHOW from revision.h and use that bit assignment
> for the new CHILD_SHOWN flag.
Yes, git-bundle should be easy enough to fix.
> + struct commit_list *l;
> +
> + if (revs->boundary == 2) {
> + unsigned i;
> + struct object_array *array = &revs->boundary_commits;
> + struct object_array_entry *objects = array->objects;
> + for (i = 0; i < array->nr; i++) {
An easy optimisation would be iterate in the other direction, resetting
array->nr after the loop. Of course, this does not preserve the order...
> + if (revs->reverse) {
> + l = NULL;
> + while ((c = get_revision_1(revs)))
> + commit_list_insert(c, &l);
> + revs->commits = l;
> + revs->reverse = 0;
> }
Clever!
>
> - /* Check the max_count ... */
> + /*
> + * Now pick up what they want to give us
> + */
> + c = get_revision_1(revs);
> + while (0 < revs->skip_count) {
> + revs->skip_count--;
> + c = get_revision_1(revs);
> + if (!c)
> + break;
> + }
> +
> + /*
> + * Check the max_count.
> + */
> switch (revs->max_count) {
> case -1:
> break;
> case 0:
> - if (revs->boundary) {
> - struct commit_list *list = revs->commits;
> - while (list) {
> - list->item->object.flags |=
> - BOUNDARY_SHOW | BOUNDARY;
> - list = list->next;
> - }
> - /* all remaining commits are boundary commits */
> - revs->max_count = -1;
> - revs->limited = 1;
> - } else
> - return NULL;
> + c = NULL;
> + break;
I guess that you want to do this instead:
case 0:
c = NULL;
/* fall through */
> default:
> revs->max_count--;
so that --reverse actually works (otherwise, the while() loop would
deplete max_count, and then max_count would be 0, and nothing would be
returned).
> + /*
> + * boundary commits are the commits that are parents of the
> + * ones we got from get_revision_1() but they themselves are
> + * not returned from get_revision_1(). Before returning
> + * 'c', we need to mark its parents that they could be boundaries.
> + */
> +
> + for (l = c->parents; l; l = l->next) {
AFAICT this changes behaviour: c->parents were possibly rewritten. 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))
?
Ciao,
Dscho
-
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