On Wed, 4 Jun 2008, Jeff King wrote:
>
> The following patch fixes it for me, but I really have no idea if there
> isn't something more subtle at work. Sending to Linus, since "git blame"
> points the surrounding code to you, and to Daniel, since the new clone
> and the commit walker are your areas.
Ack. This is correct.
That said, a *lot* of code does this pattern (freeing the tree buffer
after use, without marking the tree non-parsed), and I suspect the only
reason I'm blamed is because this got copied from some other use of that
same model.
Normally it's fine, because the whole object use is temporary, but as you
point out, doing things in the same process will re-use old object info.
It's one of the subtler implications of doing built-ins without fork/exec.
It is possible that we should clean this up by adding some kind of
static void forget_tree(struct tree *tree)
{
free(tree->buffer);
tree->buffer = NULL;
tree->size = 0;
tree->parsed = 0;
}
to make this more robust and obvious. That said, a lot of the users are
basically the type
if (parse_tree(tree) < 0)
die(..);
init_tree_desc(&desc, tree->buffer, tree->size);
while (tree_entry(&desc, &entry)) {
...
}
forget_tree();
and quite frankly, it's rather possible that we should get rid of the
"void *buffer" and "unsigned long size" in the tree *entirely*, because
the above would likely be better written as
buffer = read_tree_desc(&desc);
while (tree_entry(&desc, &entry)) {
...
}
free(buffer);
and make "struct tree" smaller, and not ever need parsing at all!
I think that realisitcially, all tree users are of that format, and nobody
really wants to save the buffer (because buffer re-use is fairly unlikely,
an re-generating it isn't all that expensive).
But that would be a much bigger patch. And maybe I'm wrong, and some uses
really do want the longer-term caching because they end up re-using the
tree a lot. So it would need more thinking about.
Linus
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
