Re: [PATCH] Speedup recursive by flushing index only once for all entries

[Sergey Vlasov <vsu@altlinux.ru>]

[-- Attachment #1: Type: text/plain, Size: 2363 bytes --]

On Fri, Jan 12, 2007 at 10:23:37AM -0800, Junio C Hamano wrote:
> Sergey Vlasov <vsu@altlinux.ru> writes:
> 
> > On Wed, 10 Jan 2007 11:28:14 -0800 Junio C Hamano wrote:
> >
> >> This revamps the merge-recursive implementation following the
> >> outline in:
> >> ...
> > This commit broke t3401-rebase-partial.sh:
> > ...
> > ...and it is still used here - however, after the patch *result is
> > uninitialized at this point.
> 
> Very true.  This untested patch should fix it.

BTW, the same code does not crash on another (x86_64) machine;
however, valgrind-3.2.1 complains:

==20571== Use of uninitialised value of size 8
==20571==    at 0x411FF2: sha1_to_hex (sha1_file.c:125)
==20571==    by 0x405D90: merge_trees (merge-recursive.c:1071)
==20571==    by 0x406044: merge (merge-recursive.c:1163)
==20571==    by 0x40641D: main (merge-recursive.c:1245)

After the patch valgrind does not complain anymore.

> Note that this stops (relative to the older
> version of merge-recursive that always wrote a tree even when it
> was not needed) reporting the tree object name for outermost
> merge, but I think that reporting was primarily meant for people
> who are debugging merge-recursive and did not have a real
> value.  We could even remove the whole printf(), which I tend to
> prefer.

If that printf() is just a debug output, we should definitely remove
it - the merge output is verbose enough already.

> diff --git a/merge-recursive.c b/merge-recursive.c
> index 5237021..40c12aa 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1066,15 +1066,17 @@ static int merge_trees(struct tree *head,
>  		path_list_clear(re_head, 0);
>  		path_list_clear(entries, 1);
>  
> -	} else {
> +	}
> +	else
>  		clean = 1;
> +
> +	if (index_only) {
> +		*result = git_write_tree();

Hmm, can git_write_tree() return NULL at this point?  Does the code in
the if (unmerged_index()) {...} branch above resolve all unmerged
index entries?  It probably should, if I understand the
merge-recursive logic...

>  		printf("merging of trees %s and %s resulted in %s\n",
>  		       sha1_to_hex(head->object.sha1),
>  		       sha1_to_hex(merge->object.sha1),
>  		       sha1_to_hex((*result)->object.sha1));
>  	}
> -	if (index_only)
> -		*result = git_write_tree();
>  
>  	return clean;
>  }

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]