Re: [PATCH] tree_entry_interesting: make recursive mode default

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

There is a bit of history behind this. Some time ago, t_e_i() only
supported prefix matching. diff-tree supported recursive and
non-recursive mode but it did not make any difference to prefix
matching.

Later on, t_e_i() gained limited recursion support to unify a similar
matching code used by git-grep. It introduced two new fields in struct
pathspec: max_depth and recursive. "recursive" field functions as a
feature switch so that this feature is off by default.

Some time after that, t_e_i() further gained wildcard support. With
wildcard matching, recursive and non-recursive diff-tree
mattered. "recursive" field was reused to distinguish recursion in
diff-tree.

This choice has a side effect that by default wildcard matching is in
non-recursive mode, which is not true. All current call sites except
"diff-tree without -r" (grep, traverse_commit_list, tree-walk and
general tree diff) prefer recursive mode.

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 nonrecursive_diff_tree, which makes it recursive by
default.

Thanks, but I am curious about two (and a half) things.

 - The "max_depth" option has perfectly good and natural "invalid"
   sentinel values (i.e. 0 or negative). Why do we need a separate
   bitfield?

 - Special casing the non-recursive mode of diff-tree is perfectly
   acceptable, but nonrecursive_diff_tree does not sound like a very good
   name for it for two reasons. Perhaps there may be other users that want
   the "surface only" behaviour, so having "diff_tree" in the name limits
   its future (re)use. Also an option that is named negatively inevitably
   invites "if (!opt->non_whatever)" double negative. Can we come up with
   a better name, perhaps "onelevel_only" or something?

 - Shouldn't "onelevel_only" be the same as limiting to a single depth
   with "max_depth"?

2012/1/12 Junio C Hamano [off-list ref]:

Thanks, but I am curious about two (and a half) things.

 - The "max_depth" option has perfectly good and natural "invalid"
  sentinel values (i.e. 0 or negative). Why do we need a separate
  bitfield?

We want infinite recursion by default, by max_depth default value is
0, which is non-recursive.

 - Special casing the non-recursive mode of diff-tree is perfectly
  acceptable, but nonrecursive_diff_tree does not sound like a very good
  name for it for two reasons. Perhaps there may be other users that want
  the "surface only" behaviour, so having "diff_tree" in the name limits
  its future (re)use. Also an option that is named negatively inevitably
  invites "if (!opt->non_whatever)" double negative. Can we come up with
  a better name, perhaps "onelevel_only" or something?

I'm bad at naming, any other names are welcome. onelevel_only sounds good.

 - Shouldn't "onelevel_only" be the same as limiting to a single depth
  with "max_depth"?

I thought of that but not sure it's equivalent to max_depth == 1 or
max_depth == 0, so I separated it for safety and clarity. max_depth
feature is driven by git-grep and there were a few interpretations how
it should behave last time, so I'm not sure if its behavior may change
in future.
-- 
Duy