Re: [PATCH v3] grep: Add --max-depth option.

[Stephen Boyd <bebarino@gmail.com>]

Michał Kiedrowicz wrote:
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---

Would have been nice to see a comment here saying what's changed since
v2. Looks like you renamed it to --max-depth, added some new inline
functions, and fixed up the test style issues?

>
> diff --git a/builtin-grep.c b/builtin-grep.c
> index f477659..2ed2507 100644
> --- a/builtin-grep.c
> +++ b/builtin-grep.c
> @@ -52,26 +52,58 @@ static int grep_config(const char *var, const char *value, void *cb)
>  	return git_color_default_config(var, value, cb);
>  }
>  
> +static inline int count_chars(const char *str, char c)
> +{
> +	int num = 0;
> +
> +	if (!str)
> +		return 0;
> +
> +	for (; *str; str++)
> +		if (*str == c)
> +			num++;
> +	return num;
> +}
> +
> +static inline int accept_subdir(const char *path, int max_depth)
> +{
> +	return max_depth < 0 || count_chars(path, '/') <= max_depth;
> +}
> +

If count_chars() is only used by accept_subdir() why not just manually
inline the loop in accept_subdir()? Plus, you rightly return early if
max_depth is -1, but what if max_depth is something small like 1? Then
you still count all the slashes when you could return upon seeing the
second slash.

>
> @@ -692,6 +724,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
>  		OPT_SET_INT('I', NULL, &opt.binary,
>  			"don't match patterns in binary files",
>  			GREP_BINARY_NOMATCH),
> +		OPT_INTEGER(0, "max-depth", &opt.max_depth,
> +				"descend at most <n> levels"),

Please use OPTION_INTEGER here so you can use "<depth>" instead of
"<n>". This will make it more consistent with the documentation.