Re: [PATCH] Speedup recursive by flushing index only once for all entries
- From
Sergey Vlasov <vsu@altlinux.ru>
- Date
- Jan 12, 2007, 21:07 UTC
- Message-ID
- <20070112210705.GB4562@procyon.home>
- In-Reply-To
- <7vr6u0t87q.fsf@assigned-by-dhcp.cox.net>
On Fri, Jan 12, 2007 at 10:23:37AM -0800, Junio C Hamano wrote:
Show 13 quoted lines
> 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.
Show 7 quoted lines
> 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.
Show 15 quoted lines
> 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...
Show 10 quoted lines
> 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;
> }