within_depth: fix return for empty path

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Makefile b/Makefile
index 70d1543b6b8688bf348f03f5e9cc1690fe547249..b5fce1205d9e51cceafa59e5dc1265ced6d94a26 100644 (file)
--- a/Makefile
+++ b/Makefile
@@ -1356,6 +1356,7 @@ 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

diff --git a/dir.c b/dir.c
index a374972b6243b62da4db8e7d974d55549cdd98a7..2ee108eeb6d5d67fe14a0021ed7772611fb8a360 100644 (file)
--- a/dir.c
+++ b/dir.c
@@ -277,7 +277,7 @@ int within_depth(const char *name, int namelen,
                if (depth > max_depth)
                        return 0;
        }
-       return 1;
+       return depth <= max_depth;
 }
 
 /*

diff --git a/t/meson.build b/t/meson.build
index d052fc3e23d2ec78199d987d21394148ff0f890b..56ea96f04ade18b63606754e65b0da2adb793c03 100644 (file)
--- a/t/meson.build
+++ b/t/meson.build
@@ -1,5 +1,6 @@
 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',

diff --git a/t/unit-tests/u-dir.c b/t/unit-tests/u-dir.c
new file mode 100644 (file)
index 0000000..2d0adaa
--- /dev/null
+++ b/t/unit-tests/u-dir.c
@@ -0,0 +1,47 @@
+#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);
+
+
+}