From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Tue, 30 Aug 2011 12:53:15 GMT
Subject: Re: [PATCH 1/3] traverse_trees(): allow pruning with pathspec
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.


>        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