Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec
From: Nguyen Thai Ngoc Duy <hidden>
Date: 2016-06-15 22:51:57
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec · Junio C Hamano <hidden>
- 2016-06-15 · Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec · Nguyen Thai Ngoc Duy <hidden>
On Tue, Aug 30, 2011 at 4:33 AM, Junio C Hamano [off-list ref] wrote:
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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