Re: [PATCH 2/4] revision traversal: retire BOUNDARY_SHOW

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Mar 6, 2007, 02:05 UTC
Message-ID: <Pine.LNX.4.63.0703060253140.22628@wbgn013.biozentrum.uni-wuerzburg.de>
In-Reply-To: <7vzm6rurst.fsf@assigned-by-dhcp.cox.net>

Hi,

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

Show 6 quoted lines

> This removes the flag internally used by revision traversal to
> decide which commits are indeed boundaries and renames it to
> CHILD_SHOWN.  builtin-bundle uses the symbol for its
> verification, but I think the logic it uses it is wrong.  The
> flag is still useful but it is local to the git-bundle, so it is
> renamed to PREREQ_MARK.

The idea was to bail out of revision walking when that prerequisite was found, _even_ if it happened to be uninteresting.

Just marking the parents uninteresting unless they are prerequisites would be better, probably.

Show 6 quoted lines

>  
>  	i = req_nr;
>  	while (i && (commit = get_revision(&revs)))
> -		if (commit->object.flags & BOUNDARY_SHOW)
> +		if (commit->object.flags & PREREQ_MARK)
>  			i--;

The above-mentioned idea, then would be: instead of just i--; do

		{
			struct commit_list *parent = commit->parents;
			i--;
			for (; parent; parent = parent->next)
				if (!(parent->item->object.flag & 
						PREREQ_MARK))
					parent->item->object.flag |=
						UNINTERESTING;
		}

This should help performance, as not all reachable commits are traversed any more.

Show 11 quoted lines

>  
>  	for (i = 0; i < req_nr; i++)
> diff --git a/revision.c b/revision.c
> index 5d137ea..2d27ccf 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1285,17 +1285,21 @@ struct commit *get_revision(struct rev_info *revs)
>  			commit_list_insert(c, &l);
>  		revs->commits = l;
>  		revs->reverse = 0;
> +		c = NULL;

Why? It gets assigned here anyway:

Show 8 quoted lines

>  	}
>  
>  	/*
>  	 * Now pick up what they want to give us
>  	 */
> -	c = get_revision_1(revs);
> +	if (!(c = get_revision_1(revs)))
> +		return NULL;

Show 7 quoted lines

>  	while (0 < revs->skip_count) {
>  		revs->skip_count--;
>  		c = get_revision_1(revs);
>  		if (!c)
>  			break;
> +		/* Although we grabbed it, it is not shown. */
> +		c->object.flags &= ~SHOWN;

Wasn't --skip meant for something like gitweb, where you just want to skip the first <n> commits from the list, but do not want to change the list otherwise?

Show 8 quoted lines

> @@ -1305,6 +1309,9 @@ struct commit *get_revision(struct rev_info *revs)
>  	case -1:
>  		break;
>  	case 0:
> +		/* Although we grabbed it, it is not shown. */
> +		if (c)
> +			c->object.flags &= ~SHOWN;
>  		c = NULL;

Is this really relevant in practice?

Ciao, Dscho