From: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
Date: Wed, 22 Jul 2009 16:00:38 GMT
Subject: Re: [PATCH v3] grep: Add --max-depth option.
Message-ID: <20090722180038.16a76b95@gmail.com>
In-Reply-To: <4A66BB64.5070106@gmail.com>

Stephen Boyd <bebarino@gmail.com> wrote:

> 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. 

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