Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default

[Junio C Hamano <gitster@pobox.com>]

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

> This patch decouples the use of recursive field. The max_depth feature
> switch is now controlled by max_depth_valid field. diff-tree recursion
> is controlled by onelevel_only, which makes it recursive by default.

> diff --git a/builtin/grep.c b/builtin/grep.c
> index 9ce064a..c081bf4 100644
> --- a/builtin/grep.c
> +++ b/builtin/grep.c
> @@ -1051,7 +1051,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  	paths = get_pathspec(prefix, argv + i);
>  	init_pathspec(&pathspec, paths);
>  	pathspec.max_depth = opt.max_depth;
> -	pathspec.recursive = 1;
> +	pathspec.max_depth_valid = 1;

We initialize opt.max_depth to "-1" (unlimited) and then let it be
overridden by the command line, and we set it to pathspec, so max_depth is
always valid, even when it is "-1", and is to be honoured.

> diff --git a/dir.c b/dir.c
> index 0a78d00..5af3567 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -258,7 +258,7 @@ int match_pathspec_depth(const struct pathspec *ps,
>  	int i, retval = 0;
>  
>  	if (!ps->nr) {
> -		if (!ps->recursive || ps->max_depth == -1)
> +		if (!ps->max_depth_valid || ps->max_depth == -1)
>  			return MATCHED_RECURSIVELY;
>  		if (within_depth(name, namelen, 0, ps->max_depth))

When there is no pathspec given, if we do not have a valid depth limiter,
or a valid depth limiter says "-1" (unlimited), it is always a match.
Otherwise we have check the depth. Looks correct.


> @@ -275,7 +275,7 @@ int match_pathspec_depth(const struct pathspec *ps,
>  		if (seen && seen[i] == MATCHED_EXACTLY)
>  			continue;
>  		how = match_pathspec_item(ps->items+i, prefix, name, namelen);
> -		if (ps->recursive && ps->max_depth != -1 &&
> +		if (ps->max_depth_valid && ps->max_depth != -1 &&
>  		    how && how != MATCHED_FNMATCH) {
>  			int len = ps->items[i].len;
>  			if (name[len] == '/')

Likewise. When there is a max_depth defined from the caller, and we did
not get the desired match, we check if we can go deeper to retry the
match. Looks correct.

These assume that tree-diff (the sole user of onelevel_only) never calls
into this function and ask to limit the recursion, though. Is it a good
thing for the longer-term code health?

In any case, both of the above seem to work without max_depth_valid.  If
the caller does not want to use depth limiting, it can set max_depth to
"-1", and all the code after this patch that check ps->max_depth_valid can
pretend that max_depth_valid is set to 1, no?

> diff --git a/tree-diff.c b/tree-diff.c
> index 28ad6db..fbc683c 100644
> --- a/tree-diff.c
> +++ b/tree-diff.c
> @@ -137,8 +137,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
>  	enum interesting t2_match = entry_not_interesting;
>  
>  	/* Enable recursion indefinitely */
> -	opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
> -	opt->pathspec.max_depth = -1;
> +	opt->pathspec.onelevel_only = !DIFF_OPT_TST(opt, RECURSIVE);

The comment "Enable recursion indefinitely" seems stale (not a problem
with this change).

> diff --git a/tree-walk.c b/tree-walk.c
> index 492c7cd..fdecacc 100644
> --- a/tree-walk.c
> +++ b/tree-walk.c
> @@ -588,7 +588,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
>  		entry_not_interesting : all_entries_not_interesting;
>  
>  	if (!ps->nr) {
> -		if (!ps->recursive || ps->max_depth == -1)
> +		if (!ps->max_depth_valid || ps->max_depth == -1)
>  			return all_entries_interesting;
>  		return within_depth(base->buf + base_offset, baselen,
>  				    !!S_ISDIR(entry->mode),

When there is no pathspec given, if we do not have a valid depth limiter
(i.e. caller is diff-tree), or a valid depth limiter says "-1" (unlimited), 
everything in this tree is interesting. If we have depth limit, we need to
check it.

But how would onelevel_only option interact with this codepath? We used to
have recursive == false and max_depth == -1 in a non-recursive diff-tree,
so the old code would have returned all_entries_interesting. Now we rely
on max_depth_valid being invalid. Again, wouldn't this work without
max_depth_valid if max_depth is set to "-1" in diff-tree?

> @@ -609,7 +609,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
>  			if (!match_dir_prefix(base_str, match, matchlen))
>  				goto match_wildcards;
>  
> -			if (!ps->recursive || ps->max_depth == -1)
> +			if (!ps->max_depth_valid || ps->max_depth == -1)
>  				return all_entries_interesting;
>  			return within_depth(base_str + matchlen + 1,

Likewise. Everything is interesting in a matched entry, when not
depth-limited. Otherwise we would need to check the depth. Looks correct.

Again, how would onelevel_only option interact with this part?

> @@ -634,7 +634,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,
>  				 * Match all directories. We'll try to
>  				 * match files later on.
>  				 */
> -				if (ps->recursive && S_ISDIR(entry->mode))
> +				if (!ps->onelevel_only && S_ISDIR(entry->mode))
>  					return entry_interesting;
>  			}
>  
> @@ -665,7 +665,7 @@ match_wildcards:
>  		 * in future, see
>  		 * http://thread.gmane.org/gmane.comp.version-control.git/163757/focus=163840
>  		 */
> -		if (ps->recursive && S_ISDIR(entry->mode))
> +		if (!ps->onelevel_only && S_ISDIR(entry->mode))
>  			return entry_interesting;
>  	}
>  	return never_interesting; /* No matches */

Before we had recursive and max_depth. Now you have three instead of two.
The only thing we are trying to say with these three is if we want to
allow unlimited recursion, no recursion or recursion limited to a certain
depth. An integer option ought to work, and various codepaths that used to
look at the old two variables are converted to look at only just a few of
the new three variables, and never all three of them.

That makes my head hurt and makes me suspect there is something
fundamentally wrong in the patch.  Sigh...