From: Junio C Hamano Date: Fri, 16 Jan 2026 18:55:22 GMT Subject: Re: [PATCH v2 5/5] last-modified: change default max-depth to 0 Message-ID: In-Reply-To: <20260116-toon-last-modified-zzzz-v2-5-79e44f2806fe@iotcl.com> Toon Claes 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=`:: > For each pathspec given on the command line, descend at most `` > 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?