Re: [PATCH v2 2/2] tree_entry_interesting: make recursive mode default
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 15, 2012, 03:12 UTC
- Message-ID
- <7v8vl9wtfg.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1326533003-19686-2-git-send-email-pclouds@gmail.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.
Show 10 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 9 quoted lines
> @@ -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?
Show 11 quoted lines
> 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).
Show 13 quoted lines
> 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?
Show 8 quoted lines
> @@ -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?
Show 18 quoted lines
> @@ -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...