Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec
- From
Nguyen Thai Ngoc Duy <pclouds@gmail.com>
- Date
- Aug 30, 2011, 12:53 UTC
- Message-ID
- <CACsJy8CmZv7Fuhw+m6X2CVO4vWLYFQNmStu1jJmhVXSi4mPxJQ@mail.gmail.com>
- In-Reply-To
- <1314653603-7533-2-git-send-email-gitster@pobox.com>
On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano <gitster@pobox.com> 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.
Show 5 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Show 10 quoted lines
> 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