Re: [PATCH 2/3] within_depth: fix return for empty path

[Patrick Steinhardt <ps@pks.im>]

On Wed, Aug 06, 2025 at 04:30:32PM +0200, Toon Claes wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > 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).
> 
> 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?

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

Patrick