On Wed, 4 Jun 2008, Linus Torvalds wrote:
> 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);
read_tree_desc(&desc, tree), which would use the hash in the tree object,
which wouldn't need to be parse, I assume.
> 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.
I think that your current unpack_trees() benefits from the fact that, if
some but not all of the trees are the same, you only uncompress the tree
objects once and store one copy of the resulting buffer. So, I don't think
long-term caching is worthwhile, but sharing between concurrent users
might be.
-Daniel
*This .sig left intentionally blank*
--
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
