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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi,

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

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

>  {
>  	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.

>  	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