Michał Kiedrowicz wrote:
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@xxxxxxxxx>
> ---
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html