On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> @@ -316,10 +328,18 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
Not related to the patch, but a bit of comment how this function works
and how it expects info->fn() to return would be appreciated.
> struct name_entry *entry = xmalloc(n*sizeof(*entry));
> int i;
> struct tree_desc_x *tx = xcalloc(n, sizeof(*tx));
> + struct strbuf base = STRBUF_INIT;
> + int interesting = 1;
I suspect making "base" an argument to traverse_trees() may save us a
few more alloc/free (or a lot, depends on how crowded the tree is).
There are only two users of traverse_trees(): merge-tree.c and
unpack-trees.c, changes should be manageable.
> @@ -376,16 +396,22 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)
> mask |= 1ul << i;
> if (S_ISDIR(entry[i].mode))
> dirmask |= 1ul << i;
> + e = &entry[i];
> }
Why? "e" is not used in that loop or anywhere after that.
> if (!mask)
> break;
> - ret = info->fn(n, mask, dirmask, entry, info);
> - if (ret < 0) {
> - error = ret;
> - if (!info->show_all_errors)
> - break;
> + interesting = prune_traversal(e, info, &base, interesting);
> + if (interesting < 0)
> + break;
I don't really understand this function to comment. But I guess when
interesting < 0, we only skip info->fn() and assume it returns "mask"
(its user unpack_callback() only returns either "mask" or -1). So the
code at the end of the main loop
for (i = 0; i < n; i++)
if (mask & (1ul << i))
update_extended_entry(tx + i, entry + i);
should not be skipped (by putting a break here). IOW, there should be
no "break;" here.
> }
> - mask &= ret;
> ret = 0;
While at there, remove "ret = 0;" too? I think this statement is useless.
--
Duy
--
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