Stephen Boyd <bebarino@xxxxxxxxx> wrote:
> 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.
Sorry, I did that when I sent v2, but this time when I remembered about
ti, it was already too late :)
> Looks like you renamed it to --max-depth, added some new inline
> functions, and fixed up the test style issues?
>
Yes, I followed your and René's suggestions and added ability to
specify max number of levels.
> >
> > 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.
Yep, that's true. My primary reason for writing two functions was to
split logic in accept_subdir() from dull counting in count_chars().
Also, count_chars() is a generic function and may be used anywhere.
>
> >
> > @@ -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.
I can do that.
Thanks for your comments.
Michał Kiedrowicz
--
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