Re: [PATCH v3] grep: Add --max-depth option.
- From
Stephen Boyd <bebarino@gmail.com>
- Date
- Jul 22, 2009, 07:10 UTC
- Message-ID
- <4A66BB64.5070106@gmail.com>
- In-Reply-To
- <1248213064-1541-1-git-send-email-michal.kiedrowicz@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?
Show 27 quoted lines
>
> 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.
Show 7 quoted lines
>
> @@ -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.