public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: "Michał Kiedrowicz" <michal.kiedrowicz@gmail•com>
To: Stephen Boyd <bebarino@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH v3] grep: Add --max-depth option.
Date: Wed, 22 Jul 2009 18:00:38 +0200	[thread overview]
Message-ID: <20090722180038.16a76b95@gmail.com> (raw)
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

next prev parent reply	other threads:[~2009-07-22 16:01 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2009-07-21 21:51 [PATCH v3] grep: Add --max-depth option Michał Kiedrowicz
2009-07-22  7:10 ` Stephen Boyd
2009-07-22 16:00   ` Michał Kiedrowicz [this message]
2009-07-22 17:08   ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20090722180038.16a76b95@gmail.com \
    --to=michal.kiedrowicz@gmail$(echo .)com \
    --cc=bebarino@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox