Re: [PATCH v2 5/5] last-modified: change default max-depth to 0

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

Toon Claes <toon@iotcl.com> writes:

> diff --git a/Documentation/git-last-modified.adoc b/Documentation/git-last-modified.adoc
> index a3992db3f2..57136baf3b 100644
> --- a/Documentation/git-last-modified.adoc
> +++ b/Documentation/git-last-modified.adoc
> @@ -27,6 +27,7 @@ OPTIONS
>  `--recursive`::
>  	Instead of showing tree entries, step into subtrees and show all entries
>  	inside them recursively.
> +	This is identical as setting `--max-depth=-1`.

When I heard that the default value of max-depth will be 0, the
first thing I wondered was "how would I spell unlimited in the new
world order?", and the documentation for "--max-depth", not
"--recursive", would have been the place I expected to fish for
necessary information.

Over there, there is "A negative value means no limit", so saying
"identical as setting --max-depth to a negative value" here would
match the description over there better, or the user will be left
wonderign if "-1" is merely an example that is negative, or if it is
more special than other negative values and if so in what way.

> @@ -36,7 +37,7 @@ OPTIONS
>  `--max-depth=<depth>`::
>  	For each pathspec given on the command line, descend at most `<depth>`
>  	levels of directories. A negative value means no limit.
> -	Setting a positive value implies `--recursive`.
> +	The default depth is 0.
>  	Cannot be combined with wildcards in the pathspec.

> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index 842700bc6a..a10e711beb 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -481,14 +481,10 @@ static int last_modified_init(struct last_modified *lm, struct repository *r,
>  	lm->rev.no_commit_id = 1;
>  	lm->rev.diff = 1;
>  	lm->rev.diffopt.flags.no_recursive_diff_tree_combined = 1;
> -	lm->rev.diffopt.flags.recursive = lm->recursive;
> +	lm->rev.diffopt.flags.recursive = 1;

Hmph, so this will always be recursive?

>  	lm->rev.diffopt.flags.tree_in_recursive = lm->show_trees;
> +	lm->rev.diffopt.max_depth = lm->max_depth;
> +	lm->rev.diffopt.max_depth_valid = !lm->recursive && lm->max_depth >= 0;

Not saying --recursive would keep lm->recursive==0 and non-negative
value of --max-depth will flip max_depth_valid on.  Saying
"--recursive" or giving a negative "--max-value" would make
max_depth_valid false, and it allows traversal all the way down to
leaves.

It may be correct, but feels quite convoluted.  I wonder if we can
get rid of lm->recursive altogether now as a clean-up, and have
"--recursive" truly do what the documentation claimed is equivalent
earlier, i.e. OPT_SET_INT(0, "recursive", &lm.max_depth, -1).  Would
that simplify the logic a bit and make it easier to reason about the
logic around here, I wonder?