Re: [PATCH 3/3] Use struct tree in diff-tree

On Sun, 29 Jan 2006, Daniel Barkalow wrote:
>
> It had been open-coding a tree parser. This updates the programs that
> call diff_tree() to send it the struct tree instead of a buffer and
> size.

Please don't.

parse_tree() is extremely broken, and expensive. 

The "struct tree_desc" is a much better abstraction, and avoids all 
overhead. Yes, it's slightly more opaque, and the interfaces could be 
improved: for example, instead of having a

	desc.buf = read_object_with_reference(new, "tree", &desc.size, NULL);
	if (!desc.buf)
		die("unable to read tree");

it might make make sense to introduce a function that does this for you, 
ie just a

	if (populate_tree_descriptor(new, &desc) < 0)
		die("unable to read tree");
	...
	free_tree_descriptor(&desc);

which is perhaps more readable and maintainable.

The "diff_tree()" functions are _extremely_ performance-critical, arguably 
more so than _any_ other part of git. Diffing two trees is one of _the_ 
most common operations, especially so when you want to follow just a 
subset of files with "git-rev-list -- <filename>*", and it's extremely 
important that you don't do malloc()/free() all the time.

So using "struct tree" and the general tree-parsing functions is _wrong_. 
Really REALLY wrong.

From what I can tell, your version doesn't even do the "free()". Which 
probably means that not only is it slower, but I bet that if you have a 
big repository like the kernel, and you do a slightly more complex 
git-rev-list with multiple files, you'll use up tons and tons and tons of 
memory.

Junio, please don't apply this.

		Linus
-
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html