Re: [PATCH 1/2] Reimplement read_tree_recursive() using tree_entry_interesting()

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 24, 2011, 19:55 UTC
Message-ID: <7vlj048f97.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1300977675-6243-1-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> +static int read_tree_1(struct tree *tree, struct strbuf *base,
> +		       int stage, struct pathspec *pathspecs,
> +		       read_tree_fn_t fn, void *context)

Micronit: call the variable pathspec, not pathspecs.

An instance of the structure contains a specification to limit the set of paths to operate on as a set of pathspec elements, and this single set is collectively called a "struct pathspec"; you are not passing an array that has one or more pathspecs in it here (i.e. pathspecs[2] in this function does not make sense).

It looks like the result is easier to follow, perhaps mostly thanks to the (re)use of strbuf instead of allocating a new path when going deeper.

Show 18 quoted lines

> @@ -108,10 +60,22 @@ int read_tree_recursive(struct tree *tree,
>  	init_tree_desc(&desc, tree->buffer, tree->size);
>  
>  	while (tree_entry(&desc, &entry)) {
> -		if (!match_tree_entry(base, baselen, entry.path, entry.mode, match))
> +		if (all_interesting)
> +			retval = all_interesting > 0;
> +		else {
> +			retval = tree_entry_interesting(&entry, base, 0, pathspecs);
> +			if (retval == 2)
> +				all_interesting = 1;
> +			else if (retval == -1) {
> +				all_interesting = -1;
> +				retval = 0;
> +			}
> +		}
> +		if (!retval)
>  			continue;

This "if all-interesting then avoid calling into an expensive matcher" logic looks familiar but is not something that came from the parts deleted by this patch. Where did I see it? Is that something we can share in a common helper function? Is such a sharing worth pursuing, considering that the above is a reasonably trivial logic in a tight loop?

"That's how the return value of tree-entry-interesting is designed to be used, and it is unsurprising that all the callers will fall into that pattern" is a perfectly acceptable answer, I guess.

Thanks.