Re: [PATCH 3/3] diff: teach tree-diff a max-depth parameter
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Aug 7, 2025, 05:55 UTC
- Message-ID
- <aJQ_x787aceaudef@pks.im>
- In-Reply-To
- <87jz3gtshx.fsf@iotcl.com>
On Wed, Aug 06, 2025 at 04:49:30PM +0200, Toon Claes wrote:
Show 24 quoted lines
> 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.
Show 13 quoted lines
> 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