Re: [PATCH 09/10] Introduce get_octopus_merge_bases() in commit.c

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jun 6, 2008, 12:28 UTC
Message-ID: <alpine.DEB.1.00.0806061316350.1783@racer>
In-Reply-To: <7vmylzw1l4.fsf@gitster.siamese.dyndns.org>

Hi,

On Thu, 5 Jun 2008, Junio C Hamano wrote:

Show 10 quoted lines

> diff --git a/commit.c b/commit.c
> index 94d5b3d..816eed1 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -531,26 +531,33 @@ static struct commit *interesting(struct commit_list *list)
>  	return NULL;
>  }
>  
> -static struct commit_list *merge_bases(struct commit *one, struct commit *two)
> +static struct commit_list *merge_bases_many(struct commit *one, int n, struct commit **twos)

Clever, but for performance reasons I think it might be better to use a commit list here. This can be a local commit_list in the helper that calls merge_bases_many() for the two-head case.

Show 26 quoted lines

>  {
>  	struct commit_list *list = NULL;
>  	struct commit_list *result = NULL;
> +	int i;
>  
> -	if (one == two)
> -		/* We do not mark this even with RESULT so we do not
> -		 * have to clean it up.
> -		 */
> -		return commit_list_insert(one, &result);
> +	for (i = 0; i < n; i++) {
> +		if (one == twos[i])
> +			/* We do not mark this even with RESULT so we do not
> +			 * have to clean it up.
> +			 */
> +			return commit_list_insert(one, &result);
> +	}
>  
>  	if (parse_commit(one))
>  		return NULL;
> -	if (parse_commit(two))
> -		return NULL;
> +	for (i = 0; i < n; i++) {
> +		if (parse_commit(twos[i]))
> +			return NULL;
> +	}

Distracting curly brackets.

Show 8 quoted lines

>  	one->object.flags |= PARENT1;
> -	two->object.flags |= PARENT2;
>  	insert_by_date(one, &list);
> -	insert_by_date(two, &list);
> +	for (i = 0; i < n; i++) {
> +		twos[i]->object.flags |= PARENT2;
> +		insert_by_date(twos[i], &list);
> +	}

Ah, now that is clever: I thought we would get into a lot of problems because we would need a bit for every initial commit. But what you are basically doing is reusing PARENT2 for all the merge bases that have been found for the heads up to, but not including, the current one.

Maybe this should be documented.

But this is not yet enough, right?  You still have to have a loop

merge_bases <- calculate merge bases from the first two heads

foreach head starting from the 3rd one
	merge_bases <- calculate merge_bases_many(this_head, merge_bases)

No?

And these merge_bases are commit_lists, therefore, as mentioned above, the signature should not take an array, but the commit_lists.

Ciao, Dscho