Hi,

On Mon, 5 Mar 2007, Junio C Hamano wrote:

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

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

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

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

quoted text
> +	if (revs->reverse) {
> +		l = NULL;
> +		while ((c = get_revision_1(revs)))
> +			commit_list_insert(c, &l);
> +		revs->commits = l;
> +		revs->reverse = 0;
>  	}

Clever!

quoted text
>  
> -	/* 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 */
quoted text
>  	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).

quoted text
> +	/*
> +	 * 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.

quoted text
> +		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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html