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

From: Linus Torvalds <torvalds@osdl.org>
Date: Jan 31, 2006, 16:53 UTC
Message-ID: <Pine.LNX.4.64.0601310846380.7301@g5.osdl.org>
In-Reply-To: <Pine.LNX.4.64.0601291405250.25300@iabervon.org>

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