Re: [PATCH v2 5/5] last-modified: change default max-depth to 0
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 16, 2026, 18:55 UTC
- Message-ID
- <xmqq1pjp8k11.fsf@gitster.g>
- In-Reply-To
- <20260116-toon-last-modified-zzzz-v2-5-79e44f2806fe@iotcl.com>
Toon Claes <toon@iotcl.com> writes:
Show 9 quoted lines
> 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.
Show 7 quoted lines
> @@ -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.
Show 10 quoted lines
> 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?