patch, 3 partsTeach git-diff-tree(1) option --max-depth

Please consider these patches authored by Peff. They add option `--max-depth` to the diff machinery.

During the process to upstream the git-blame-tree(1), later named git-last-modified(1), various times[1][2][3] the topic was raised to add option `--max-depth` to the diff machinery. In this patch series this change is added as a separate patch series.

The patches were originally written by Peff[4] and I'm crediting him as the author. I've taken the patches almost verbatim from his fork on GitHub, with some minor tweaks in the commit messages. Because only tiny changes were made, I've kept his Signed-off-by trailers, but I can remove if disagreed on.

The goal of the option `--max-depth` is to stop recursively traversing the tree if the given depth is reached from the pathspec.

These patches add `max_depth` and `max_depth_valid` to `struct diff_options`. This is different from what git-grep(1) does, which uses `max_depth` on `struct pathspec` instead. At the moment I'm on the fence whether this is an issue: while it probably makes sense to consolidate them into the same structs, it does not really make sense to reuse these the struct fields if they are used in two separate code paths.

[1]: https://lore.kernel.org/git/20130318121243.GC14789@sigill.intra.peff.net/ [2]: https://lore.kernel.org/git/20160831054201.ldlwptlmcndjmfwu@sigill.intra.peff.net/ [3]: https://lore.kernel.org/git/Y+%2FmnnJUz75yfWCN@coredump.intra.peff.net/ [4]: https://github.com/peff/git/tree/jk/diff-max-depth

---
Jeff King (3):
      combine-diff: zero memory used for callback filepairs
      within_depth: fix return for empty path
      diff: teach tree-diff a max-depth parameter

 Documentation/diff-options.adoc |  28 +++++++++++
 combine-diff.c                  |   2 +-
 diff-lib.c                      |   5 ++
 diff.c                          |  19 +++++++
 diff.h                          |   9 ++++
 dir.c                           |   2 +-
 t/meson.build                   |   1 +
 t/t4072-diff-max-depth.sh       | 109 ++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                     |  78 ++++++++++++++++++++++++++--
 9 files changed, 248 insertions(+), 5 deletions(-)
---

---

base-commit: e813a0200a7121b97fec535f0d0b460b0a33356c change-id: 20250724-toon-max-depth-25d3c19e2607

Thanks -- Toon

From: Jeff King <peff@peff.net>

In commit 25e5e2bf85 (combine-diff: support format_callback, 2011-08-19), the combined-diff code learned how to make a multi-sourced `diff_filepair` to pass to a diff callback. When we create each filepair, we do not bother to fill in many of the fields, because they would make no sense (e.g. there can be no rename score or broken_pair flag because we do not go through the diffcore filters). However, we did not even bother to zero them, leading to random values. Let's make sure everything is blank with xcalloc(), just as the regular diff code does.

We would potentially want to set the `status` flag to something non-zero, but it is not clear to what. Possibly a new DIFF_STATUS_COMBINED would make sense, as this is not strictly a modification, nor does it fit any other category.

Since it is not yet clear what callers would want, this patch simply leaves it as `0`, the same empty flag that is seen when `diffcore_std` is not used at all.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

From: Jeff King <peff@peff.net>

The within_depth() function is used to check whether pathspecs limited by a max-depth parameter are acceptable. It takes a path to check, a maximum depth, and a "base" depth. It counts the components in the path (by counting slashes), adds them to the base, and compare them to the maximum.

However, if the base does not have any slashes at all, we always return `true`. If the base depth is 0, then this is correct; no matter what the maximum is, we are always within it. However, if the base depth is greater than 0, then we might return an erroneous result.

This ends up not causing any user-visible bugs in the current code. The call sites in dir.c always pass a base depth of 0, so are unaffected. But tree_entry_interesting() uses this function differently: it will pass the prefix of the current entry, along with a `1` if the entry is a directory, in essence checking whether items inside the entry would be of interest. It turns out not to make a difference in behavior, but the reasoning is complex.

Given a tree like:

  file
  a/file
  a/b/file

walking the tree and calling tree_entry_interesting() will yield the following results:

  (with max_depth=0):
      file: yes
         a: yes
    a/file: no
       a/b: no

  (with max_depth=1):
      file: yes
         a: yes
    a/file: yes
       a/b: no

So we have inconsistent behavior in considering directories interesting. If they are at the edge of our depth but at the root, we will recurse into them, but then find all of their entries uninteresting (e.g., in the first case, we will look at "a" but find "a/*" uninteresting). But if they are at the edge of our depth and not at the root, then we will not recurse (in the second example, we do not even bother entering "a/b").

This turns out not to matter because the only caller which uses max-depth pathspecs is cmd_grep(), which only cares about blob entries. From its perspective, it is exactly the same to not recurse into a subtree, or to recurse and find that it contains no matching entries. Not recursing is merely an optimization.

It is debatable whether tree_entry_interesting() should consider such an entry interesting. The only caller does not care if it sees the tree itself, and can benefit from the optimization. But if we add a "max-depth" limiter to regular diffs, then a diff with DIFF_OPT_TREE_IN_RECURSIVE would probably want to show the tree itself, but not what it contains.

This patch just fixes within_depth(), which means we consider such entries uninteresting (and makes the current caller happy). If we want to change that in the future, then this fix is still the correct first step, as the current behavior is simply inconsistent.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 dir.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Tue, Jul 29, 2025 at 08:57:43PM +0200, Toon Claes wrote:

s/compare/&s/

Okay, well-explained.

I haven't yet read the cover letter, but I guess that the scenario where we'll care about this is in git-last-modified(1) if we want to teach that command a `--max-depth` parameter?

So... do we end up with this now?

   (with max_depth=1):
       file: yes
          a: yes
     a/file: no
        a/b: no

I think it would be nice to include that in the message to show the change in behaviour at a glance.

Just for my own understanding: the only difference is when we don't have even a single matching slash, as we don't verify `depth > max_depth` in that case. So in theory, we could modify the function to the following equivalent:

	int within_depth(const char *name, int namelen,
				int depth, int max_depth)
	{
		const char *cp = name, *cpe = name + namelen;

		if (depth > max_depth)
			return 0;

		while (cp < cpe) {
			if (*cp++ != '/')
				continue;
			depth++;
			if (depth > max_depth)
				return 0;
		}
		return 1;
	}

(Not saying we should, I'm just double checking my understanding).

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Well, yes git-last-modified(1) will be able to use this option as well. And it will be useful there. But those patches are still in flight in a separate series.

In the commit following this one we see `--max-depth` being used in git-diff-tree(1). Maybe I can do a better job explaining how this changes affects that patch. Let me see how I can phrase this.

The change in behavior happens when max_depth equals zero. In that case the example would be:

    (with max_depth=0):
        file: yes
           a: yes
      a/file: no
         a/b: no

So no change actually. But that's due to how tree_entry_interesting() calls within_depth(). It doesn't pass the path of the entry, but the base of it. I'm not sure using tree_entry_interesting() here in the commit message to explain the change is the best idea, but it's the callsite that /might/ be affected. But it isn't. So it's pretty involved trying to put this change into words.

To be honest, I wasn't sure no more. I decided to see if I can write a unit test. This is what I came up with:

    void test_dir__within_depth(void)
    {
    	struct test_case {
    		const char *path;
    		int depth;
    		int max_depth;
    		int expect;
    	} test_cases[] = {
    		/* depth = 0; max_depth = 0 */
    		{ "",         0, 0, 1 },
    		{ "file",     0, 0, 1 },
    		{ "a",        0, 0, 1 },
    		{ "a/file",   0, 0, 0 },
    		{ "a/b",      0, 0, 0 },
    		{ "a/b/file", 0, 0, 0 },

    		/* depth = 0; max_depth = 1 */
    		{ "",         0, 1, 1 },
    		{ "file",     0, 1, 1 },
    		{ "a",        0, 1, 1 },
    		{ "a/file",   0, 1, 1 },
    		{ "a/b",      0, 1, 1 },
    		{ "a/b/file", 0, 1, 0 },

    		/* depth = 1; max_depth = 1 */
    		{ "",         1, 1, 1 },
    		{ "file",     1, 1, 1 },
    		{ "a",        1, 1, 1 },
    		{ "a/file",   1, 1, 0 },
    		{ "a/b",      1, 1, 0 },
    		{ "a/b/file", 1, 1, 0 },

    		/* depth = 1; max_depth = 0 */
    		{ "",         1, 0, 0 },
    		{ "file",     1, 0, 0 },
    		{ "a",        1, 0, 0 },
    		{ "a/file",   1, 0, 0 },
    		{ "a/b",      1, 0, 0 },
    		{ "a/b/file", 1, 0, 0 },
    	};

    	for (size_t i = 0; i < ARRAY_SIZE(test_cases); i++) {
    		int result = within_depth(test_cases[i].path, strlen(test_cases[i].path),
    					  test_cases[i].depth, test_cases[i].max_depth);
    		cl_assert_equal_b(result, test_cases[i].expect);
    	}
    }

The change in this patch affect the last batch of tests (the batch with 'depth = 1; max_depth = 0'). Running the test on both this patch and your suggestion gives the same results, so yes your suggestion has the same output.

Do you think it's worth to add such unit test?

I did add `""` because I've seen cmd_grep() might be passing an empty path.

On Wed, Aug 06, 2025 at 04:30:32PM +0200, Toon Claes wrote:

Sure, if we can easily test this via such a unit test I think it would be a good addition.

Patrick

From: Jeff King <peff@peff.net>

When you are doing a tree-diff, there are basically two options: do not recurse into subtrees at all, or recurse indefinitely. While most callers would want to always recurse and see full pathnames, some may want the efficiency of looking only at a particular level of the tree. This is currently easy to do for the top-level (just turn off recursion), but you cannot say "show me what changed in subdir/, but do not recurse".

This patch adds a max-depth parameter which is measured from the closest pathspec match, so that you can do:

  git log --raw --max-depth=1 -- a/b/c

and see the raw output for a/b/c/, but not those of a/b/c/d/ (instead of the raw output you would see for a/b/c/d).

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 Documentation/diff-options.adoc |  28 +++++++++++
 diff-lib.c                      |   5 ++
 diff.c                          |  19 +++++++
 diff.h                          |   9 ++++
 t/meson.build                   |   1 +
 t/t4072-diff-max-depth.sh       | 109 ++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                     |  78 ++++++++++++++++++++++++++--
 7 files changed, 246 insertions(+), 3 deletions(-)

On Tue, Jul 29, 2025 at 08:57:44PM +0200, Toon Claes wrote:

Hm, okay. So what happens if I pass both "a/b" and "a/b/c"? Would I see the contents of both trees?

This partially answers my question, as we talk about the scenario where we have "foo/bar/baz", but no "foo/qux". If we had the latter, would that also be displayed here because it's 1 level deep from the closest matching pathspec ("foo")?

Let's also note that wildcard pathspecs aren't supported.

This and the following error messages should be made translatable.

We're missing error handling in case the argument is not an integer. We should probably use `git_parse_unsigned()` instead.

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?

Double "assume that".

So in that case, shouldn't we convert `max_depth` to be unsigned and `max_depth_valid` to be a boolean?

Let's mark the `name` parameter as `const` both here and in `should_recurse()` so that it becomes clear that it shouldn't be modified.

`nr` is of type `int` indeed, so the loop index is correct even though it feels wrong. But that's nothing we have to fix as part of this patch series. We could inline the declaration though and make it loop-local.

Let's use `BUG()` instead. This patch series must be old, the function has been introduced 8 years ago in d8193743e0 (usage.c: add BUG() function, 2017-05-12).

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Yes, the max-depth applies to both pathspecs and entries that match either of them are shown.

"foo/qux" is indeed within 1 level deep from "foo", so yes "foo/qux" will be shown. "foo/qux/duck" not obviously.

Ah yes, good point. Will add.

True. Will do.

Absolutely. Well, TIL!

But how would you determine `max_depth_valid`?

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.

Thanks!

As I mentioned above, -1 should mean "No limit", but should enable recursion. Now, that doesn't mean we can't make the changes you suggest, we just have to take that into account.

:+1:

Ack.

Good catch. Even looking at your suggestion, at first I didn't realize it wasn't using BUG(). ;)

On Wed, Aug 06, 2025 at 04:49:30PM +0200, Toon Claes wrote:

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

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

Please consider these patches authored by Peff. They add option `--max-depth` to the diff machinery.

During the process to upstream the git-blame-tree(1), later named git-last-modified(1), various times[1][2][3] the topic was raised to add option `--max-depth` to the diff machinery. In this patch series this change is added as a separate patch series.

The patches were originally written by Peff[4] and I'm crediting him as the author. I've taken the patches almost verbatim from his fork on GitHub, with some minor tweaks in the commit messages. Because only tiny changes were made, I've kept his Signed-off-by trailers, but I can remove if disagreed on.

The goal of the option `--max-depth` is to stop recursively traversing the tree if the given depth is reached from the pathspec.

These patches add `max_depth` and `max_depth_valid` to `struct diff_options`. This is different from what git-grep(1) does, which uses `max_depth` on `struct pathspec` instead. At the moment I'm on the fence whether this is an issue: while it probably makes sense to consolidate them into the same structs, it does not really make sense to reuse these the struct fields if they are used in two separate code paths.

[1]: https://lore.kernel.org/git/20130318121243.GC14789@sigill.intra.peff.net/ [2]: https://lore.kernel.org/git/20160831054201.ldlwptlmcndjmfwu@sigill.intra.peff.net/ [3]: https://lore.kernel.org/git/Y+%2FmnnJUz75yfWCN@coredump.intra.peff.net/ [4]: https://github.com/peff/git/tree/jk/diff-max-depth

---
Changes in v2:
- Added unit-tests for within_depth() in dir.c. Originally the patch
  was a oneline change by Peff, but I've added a bunch of code and
  extended the commit message, so I've set myself as the author and set
  a Based-on-patch-by trailer for Peff. I hope that's okay?
- Added support for --max-depth=-1 and extended code comments why we
  need the max_depth_valid flag. With these modification it did no
  longer feel appropriate to keep Peff's Signed-off-by trailer.
- Made die() messages translatable.
- Small tweaks to the docs.
- Added some const-correctness.
- Switched from `die("BUG: ...")` to `BUG(...)`.
- Link to v1: https://lore.kernel.org/r/20250729-toon-max-depth-v1-0-c177e39c40fb@iotcl.com

---
Jeff King (2):
      combine-diff: zero memory used for callback filepairs
      diff: teach tree-diff a max-depth parameter

Toon Claes (1):
      within_depth: fix return for empty path

 Documentation/diff-options.adoc |  28 ++++++++++
 Makefile                        |   1 +
 combine-diff.c                  |   2 +-
 diff-lib.c                      |   5 ++
 diff.c                          |  24 +++++++++
 diff.h                          |   8 +++
 dir.c                           |   2 +-
 t/meson.build                   |   2 +
 t/t4072-diff-max-depth.sh       | 116 ++++++++++++++++++++++++++++++++++++++++
 t/unit-tests/u-dir.c            |  47 ++++++++++++++++
 tree-diff.c                     |  78 +++++++++++++++++++++++++--
 11 files changed, 308 insertions(+), 5 deletions(-)
---

Range-diff versus v1:

1:  b16919a12d = 1:  f551775c58 combine-diff: zero memory used for callback filepairs
2:  c67ed5edee ! 2:  739374a2f8 within_depth: fix return for empty path
    @@
      ## Metadata ##
    -Author: Jeff King <peff@peff.net>
    +Author: Toon Claes <toon@iotcl.com>
     
      ## Commit message ##
         within_depth: fix return for empty path
    @@ Commit message
         The within_depth() function is used to check whether pathspecs limited
         by a max-depth parameter are acceptable. It takes a path to check, a
         maximum depth, and a "base" depth. It counts the components in the
    -    path (by counting slashes), adds them to the base, and compare them to
    +    path (by counting slashes), adds them to the base, and compares them to
         the maximum.
     
         However, if the base does not have any slashes at all, we always return
    @@ Commit message
         to change that in the future, then this fix is still the correct first
         step, as the current behavior is simply inconsistent.
     
    -    Signed-off-by: Jeff King <peff@peff.net>
    +    This has the effect the function tree_entry_interesting() now behaves
    +    like following on the first example:
    +
    +      (with max_depth=0):
    +          file: yes
    +             a: no
    +        a/file: no
    +           a/b: no
    +
    +    Meaning we won't step in "a/" no more to realize all "a/*" entries are
    +    uninterested, but we stop at the tree entry itself.
    +
    +    Based-on-patch-by: Jeff King <peff@peff.net>
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
    + ## Makefile ##
    +@@ Makefile: THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/%
    + THIRD_PARTY_SOURCES += $(UNIT_TEST_DIR)/clar/clar/%
    + 
    + CLAR_TEST_SUITES += u-ctype
    ++CLAR_TEST_SUITES += u-dir
    + CLAR_TEST_SUITES += u-example-decorate
    + CLAR_TEST_SUITES += u-hash
    + CLAR_TEST_SUITES += u-hashmap
    +
      ## dir.c ##
     @@ dir.c: int within_depth(const char *name, int namelen,
      		if (depth > max_depth)
    @@ dir.c: int within_depth(const char *name, int namelen,
      }
      
      /*
    +
    + ## t/meson.build ##
    +@@
    + clar_test_suites = [
    +   'unit-tests/u-ctype.c',
    ++  'unit-tests/u-dir.c',
    +   'unit-tests/u-example-decorate.c',
    +   'unit-tests/u-hash.c',
    +   'unit-tests/u-hashmap.c',
    +
    + ## t/unit-tests/u-dir.c (new) ##
    +@@
    ++#include "unit-test.h"
    ++#include "dir.h"
    ++
    ++#define TEST_WITHIN_DEPTH(path, depth, max_depth, expect) do { \
    ++		int actual = within_depth(path, strlen(path), \
    ++					  depth, max_depth); \
    ++		if (actual != expect) \
    ++			cl_failf("path '%s' with depth '%d' and max-depth '%d': expected %d, got %d", \
    ++				 path, depth, max_depth, expect, actual); \
    ++	} while (0)
    ++
    ++void test_dir__within_depth(void)
    ++{
    ++	/* depth = 0; max_depth = 0 */
    ++	TEST_WITHIN_DEPTH("",         0, 0, 1);
    ++	TEST_WITHIN_DEPTH("file",     0, 0, 1);
    ++	TEST_WITHIN_DEPTH("a",        0, 0, 1);
    ++	TEST_WITHIN_DEPTH("a/file",   0, 0, 0);
    ++	TEST_WITHIN_DEPTH("a/b",      0, 0, 0);
    ++	TEST_WITHIN_DEPTH("a/b/file", 0, 0, 0);
    ++
    ++	/* depth = 0; max_depth = 1 */
    ++	TEST_WITHIN_DEPTH("",         0, 1, 1);
    ++	TEST_WITHIN_DEPTH("file",     0, 1, 1);
    ++	TEST_WITHIN_DEPTH("a",        0, 1, 1);
    ++	TEST_WITHIN_DEPTH("a/file",   0, 1, 1);
    ++	TEST_WITHIN_DEPTH("a/b",      0, 1, 1);
    ++	TEST_WITHIN_DEPTH("a/b/file", 0, 1, 0);
    ++
    ++	/* depth = 1; max_depth = 1 */
    ++	TEST_WITHIN_DEPTH("",         1, 1, 1);
    ++	TEST_WITHIN_DEPTH("file",     1, 1, 1);
    ++	TEST_WITHIN_DEPTH("a",        1, 1, 1);
    ++	TEST_WITHIN_DEPTH("a/file",   1, 1, 0);
    ++	TEST_WITHIN_DEPTH("a/b",      1, 1, 0);
    ++	TEST_WITHIN_DEPTH("a/b/file", 1, 1, 0);
    ++
    ++	/* depth = 1; max_depth = 0 */
    ++	TEST_WITHIN_DEPTH("",         1, 0, 0);
    ++	TEST_WITHIN_DEPTH("file",     1, 0, 0);
    ++	TEST_WITHIN_DEPTH("a",        1, 0, 0);
    ++	TEST_WITHIN_DEPTH("a/file",   1, 0, 0);
    ++	TEST_WITHIN_DEPTH("a/b",      1, 0, 0);
    ++	TEST_WITHIN_DEPTH("a/b/file", 1, 0, 0);
    ++
    ++
    ++}
3:  b3eb4863c2 ! 3:  2987dbc007 diff: teach tree-diff a max-depth parameter
    @@ Commit message
         and see the raw output for a/b/c/, but not those of a/b/c/d/
         (instead of the raw output you would see for a/b/c/d).
     
    -    Signed-off-by: Jeff King <peff@peff.net>
    +    Co-authored-by: Toon Claes <toon@iotcl.com>
         Signed-off-by: Toon Claes <toon@iotcl.com>
     
      ## Documentation/diff-options.adoc ##
    @@ Documentation/diff-options.adoc: endif::git-format-patch[]
      	reverted with `--ita-visible-in-index`. Both options are
      	experimental and could be removed in future.
      
    -+--max-depth=<n>::
    -+
    -+	Limit diff recursion to `<n>` levels (implies `-r`). The depth
    -+	is measured from the closest pathspec. Given a tree containing
    -+	`foo/bar/baz`, the following list shows the matches generated by
    -+	each set of options:
    ++--max-depth=<depth>::
    ++	For each pathspec given on command line, descend at most `<depth>`
    ++	levels of directories. A value of `-1` means no limit.
    ++	Cannot be combined with wildcards in the pathspec.
    ++	Given a tree containing `foo/bar/baz`, the following list shows the
    ++	matches generated by each set of options:
     ++
     +--
     + - `--max-depth=0 -- foo`: `foo`
    @@ diff-lib.c: void run_diff_files(struct rev_info *revs, unsigned int option)
      	struct index_state *istate = revs->diffopt.repo->index;
      
     +	if (revs->diffopt.max_depth_valid)
    -+		die("max-depth is not supported for worktree diffs");
    ++		die(_("max-depth is not supported for worktree diffs"));
     +
      	diff_set_mnemonic_prefix(&revs->diffopt, "i/", "w/");
      
    @@ diff-lib.c: static int diff_cache(struct rev_info *revs,
      	opts.pathspec = &revs->diffopt.pathspec;
      	opts.pathspec->recursive = 1;
     +	if (revs->diffopt.max_depth_valid)
    -+		die("max-depth is not supported for index diffs");
    ++		die(_("max-depth is not supported for index diffs"));
      
      	init_tree_desc(&t, &tree->object.oid, tree->buffer, tree->size);
      	return unpack_trees(1, &t, &opts);
    @@ diff.c: static int diff_opt_rotate_to(const struct option *opt, const char *arg,
     +	struct diff_options *options = opt->value;
     +
     +	BUG_ON_OPT_NEG(unset);
    ++
    ++	if (!git_parse_int(arg, &options->max_depth))
    ++		return error(_("invalid value for '%s': '%s'"),
    ++			     "--max-depth", arg);
    ++
     +	options->flags.recursive = 1;
    -+	options->max_depth = strtol(arg, NULL, 10);
    -+	options->max_depth_valid = 1;
    ++	options->max_depth_valid = options->max_depth >= 0;
    ++
     +	return 0;
     +}
     +
    @@ diff.h: struct diff_options {
      	int no_free;
     +
     +	/*
    -+	 * The extra "valid" flag is a slight hack. The value "0" is perfectly
    -+	 * valid for max-depth. We would normally use -1 to indicate "not set",
    -+	 * but there are many code paths which assume that assume that just
    -+	 * zero-ing out a diff_options is enough to initialize it.
    ++	 * The value '0' is a valid max-depth (for no recursion), and value '-1'
    ++	 * also (for unlimited recursion), so the extra "valid" flag is used to
    ++	 * determined whether the user specified option --max-depth.
     +	 */
     +	int max_depth;
     +	int max_depth_valid;
    @@ t/t4072-diff-max-depth.sh (new)
     +	check_one tree "$*" '' 1 file one/file one/two
     +	check_one tree "$*" '' 2 file one/file one/two/file one/two/three
     +	check_one tree "$*" '' 3 file one/file one/two/file one/two/three/file
    ++	check_one tree "$*" '' -1 file one/file one/two/file one/two/three/file
     +	check_one tree "$*" one 0 one
     +	check_one tree "$*" one 1 one/file one/two
     +	check_one tree "$*" one 2 one/file one/two/file one/two/three
    @@ t/t4072-diff-max-depth.sh (new)
     +	check_one tree "$*" one/two 0 one/two
     +	check_one tree "$*" one/two 1 one/two/file one/two/three
     +	check_one tree "$*" one/two 2 one/two/file one/two/three/file
    ++	check_one tree "$*" one/two 2 one/two/file one/two/three/file
     +	check_one tree "$*" one/two/three 0 one/two/three
     +	check_one tree "$*" one/two/three 1 one/two/three/file
     +}
    @@ t/t4072-diff-max-depth.sh (new)
     +	check_one "$@" one/two 2 one/two/file one/two/three/file
     +	check_one "$@" one/two/three 0
     +	check_one "$@" one/two/three 1 one/two/three/file
    ++
    ++	# Value '-1' for '--max-depth is the same as recursion without limit,
    ++	# and thus should always succeed.
    ++	local expect=
    ++	check_one "$@" '' -1 file one/file one/two/file one/two/three/file
     +}
     +
     +# Check as a modification...
    @@ tree-diff.c
     +		(!path[dirlen] || path[dirlen] == '/');
     +}
     +
    -+static int check_recursion_depth(struct strbuf *name,
    ++static int check_recursion_depth(const struct strbuf *name,
     +				 const struct pathspec *ps,
     +				 int max_depth)
     +{
    @@ tree-diff.c
     +	return 0;
     +}
     +
    -+static int should_recurse(struct strbuf *name, struct diff_options *opt)
    ++static int should_recurse(const struct strbuf *name, struct diff_options *opt)
     +{
     +	if (!opt->flags.recursive)
     +		return 0;
    @@ tree-diff.c
     +	 * against any internal munging.
     +	 */
     +	if (opt->pathspec.has_wildcard)
    -+		die("BUG: wildcard pathspecs are incompatible with max-depth");
    ++		BUG("wildcard pathspecs are incompatible with max-depth");
     +
     +	return check_recursion_depth(name, &opt->pathspec, opt->max_depth);
     +}

---

base-commit: 64cbe5e2e8a7b0f92c780b210e602496bd5cad0f change-id: 20250724-toon-max-depth-25d3c19e2607

Thanks -- Toon

From: Jeff King <peff@peff.net>

In commit 25e5e2bf85 (combine-diff: support format_callback, 2011-08-19), the combined-diff code learned how to make a multi-sourced `diff_filepair` to pass to a diff callback. When we create each filepair, we do not bother to fill in many of the fields, because they would make no sense (e.g. there can be no rename score or broken_pair flag because we do not go through the diffcore filters). However, we did not even bother to zero them, leading to random values. Let's make sure everything is blank with xcalloc(), just as the regular diff code does.

We would potentially want to set the `status` flag to something non-zero, but it is not clear to what. Possibly a new DIFF_STATUS_COMBINED would make sense, as this is not strictly a modification, nor does it fit any other category.

Since it is not yet clear what callers would want, this patch simply leaves it as `0`, the same empty flag that is seen when `diffcore_std` is not used at all.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 combine-diff.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

The within_depth() function is used to check whether pathspecs limited by a max-depth parameter are acceptable. It takes a path to check, a maximum depth, and a "base" depth. It counts the components in the path (by counting slashes), adds them to the base, and compares them to the maximum.

However, if the base does not have any slashes at all, we always return `true`. If the base depth is 0, then this is correct; no matter what the maximum is, we are always within it. However, if the base depth is greater than 0, then we might return an erroneous result.

This ends up not causing any user-visible bugs in the current code. The call sites in dir.c always pass a base depth of 0, so are unaffected. But tree_entry_interesting() uses this function differently: it will pass the prefix of the current entry, along with a `1` if the entry is a directory, in essence checking whether items inside the entry would be of interest. It turns out not to make a difference in behavior, but the reasoning is complex.

Given a tree like:

  file
  a/file
  a/b/file

walking the tree and calling tree_entry_interesting() will yield the following results:

  (with max_depth=0):
      file: yes
         a: yes
    a/file: no
       a/b: no

  (with max_depth=1):
      file: yes
         a: yes
    a/file: yes
       a/b: no

So we have inconsistent behavior in considering directories interesting. If they are at the edge of our depth but at the root, we will recurse into them, but then find all of their entries uninteresting (e.g., in the first case, we will look at "a" but find "a/*" uninteresting). But if they are at the edge of our depth and not at the root, then we will not recurse (in the second example, we do not even bother entering "a/b").

This turns out not to matter because the only caller which uses max-depth pathspecs is cmd_grep(), which only cares about blob entries. From its perspective, it is exactly the same to not recurse into a subtree, or to recurse and find that it contains no matching entries. Not recursing is merely an optimization.

It is debatable whether tree_entry_interesting() should consider such an entry interesting. The only caller does not care if it sees the tree itself, and can benefit from the optimization. But if we add a "max-depth" limiter to regular diffs, then a diff with DIFF_OPT_TREE_IN_RECURSIVE would probably want to show the tree itself, but not what it contains.

This patch just fixes within_depth(), which means we consider such entries uninteresting (and makes the current caller happy). If we want to change that in the future, then this fix is still the correct first step, as the current behavior is simply inconsistent.

This has the effect the function tree_entry_interesting() now behaves like following on the first example:

  (with max_depth=0):
      file: yes
         a: no
    a/file: no
       a/b: no

Meaning we won't step in "a/" no more to realize all "a/*" entries are uninterested, but we stop at the tree entry itself.

Based-on-patch-by: Jeff King <peff@peff.net>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 Makefile             |  1 +
 dir.c                |  2 +-
 t/meson.build        |  1 +
 t/unit-tests/u-dir.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+), 1 deletion(-)

From: Jeff King <peff@peff.net>

When you are doing a tree-diff, there are basically two options: do not recurse into subtrees at all, or recurse indefinitely. While most callers would want to always recurse and see full pathnames, some may want the efficiency of looking only at a particular level of the tree. This is currently easy to do for the top-level (just turn off recursion), but you cannot say "show me what changed in subdir/, but do not recurse".

This patch adds a max-depth parameter which is measured from the closest pathspec match, so that you can do:

  git log --raw --max-depth=1 -- a/b/c

and see the raw output for a/b/c/, but not those of a/b/c/d/ (instead of the raw output you would see for a/b/c/d).

Co-authored-by: Toon Claes <toon@iotcl.com>
Signed-off-by: Toon Claes <toon@iotcl.com>
---
 Documentation/diff-options.adoc |  28 ++++++++++
 diff-lib.c                      |   5 ++
 diff.c                          |  24 +++++++++
 diff.h                          |   8 +++
 t/meson.build                   |   1 +
 t/t4072-diff-max-depth.sh       | 116 ++++++++++++++++++++++++++++++++++++++++
 tree-diff.c                     |  78 +++++++++++++++++++++++++--
 7 files changed, 257 insertions(+), 3 deletions(-)

Toon Claes <toon@iotcl.com> writes:

This round seems to have attracted no comments, after seeing and reacting to a few comments in the previous rounds. Should this be a part of the first batch after 2.51 final gets tagged?

Thanks.

On Thu, Aug 14, 2025 at 08:15:44AM -0700, Junio C Hamano wrote:

My nits all got addressed judging by the range diff, so I'm happy with this version.

Thanks!

Patrick