Re: [PATCH 3/3] diff: teach tree-diff a max-depth parameter

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 06, 2025 at 04:49:30PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, Jul 29, 2025 at 08:57:44PM +0200, Toon Claes wrote:
> >> diff --git a/diff.c b/diff.c
> >> index dca87e164f..c03a59ac3b 100644
> >> --- a/diff.c
> >> +++ b/diff.c
> >> @@ -5894,6 +5909,10 @@ struct option *add_diff_options(const struct option *opts,
> >>  		OPT_CALLBACK_F(0, "diff-filter", options, N_("[(A|C|D|M|R|T|U|X|B)...[*]]"),
> >>  			       N_("select files by diff type"),
> >>  			       PARSE_OPT_NONEG, diff_opt_diff_filter),
> >> +		OPT_CALLBACK_F(0, "max-depth", options, N_("<depth>"),
> >> +			       N_("maximum tree depth to recurse"),
> >> +			       PARSE_OPT_NONEG, diff_opt_max_depth),
> >> +
> >>  		{
> >>  			.type = OPTION_CALLBACK,
> >>  			.long_name = "output",
> >
> > Okay. We don't use `OPT_UNSIGNED()` because we also want to impliy the
> > `recursive` flag. Wouldn't it be simpler though to use `OPT_UNSIGNED()`
> > and then set the flag in `diff_setup_done()` like we already do for a
> > couple of other options?
> 
> But how would you determine `max_depth_valid`?

Ideally we wouldn't need it as we could just initialize with a default
value.

> And, without realizing, you touch a good point here. Looking at the
> git-grep(1) docs, it says:
> 
>     For each <pathspec> given on command line, descend at most <depth>
>     levels of directories. A value of -1 means no limit.
> 
> And:
> 
>     -r::
>     --recursive::
>     	Same as `--max-depth=-1`; this is the default.
> 
> We should handle -1 the same, that's currently not the case.

True. Do we have the same default? If so, couldn't we drop
`max_depth_valid` and initialize it with -1 from the start?

If that's not easily possible we can just stick with what you have.

Patrick