Re: [RFC/PATCH] Add --exclude-dir option to git grep
- From
David Ripton <dripton@ripton.net>
- Date
- Sep 25, 2010, 02:07 UTC
- Message-ID
- <4C9D596C.4060906@ripton.net>
- In-Reply-To
- <7v1v8iq3tu.fsf@alter.siamese.dyndns.org>
On 09/24/10 16:33, Junio C Hamano wrote:
> Thanks.
Thank you for the code review.
Show 7 quoted lines
> Do you need to run this every time we visit a new directory, expanding > directory components over and over? > > It is not like we are jumping around directory hierarchies, visiting > "foo/bar" and then "xyzzy" and then "foo/baz", but rather we visit > directories in a nicer order (i.e. after leaving "foo/bar" but before > jumping to "xyzzy", we would visit "foo/baz"), don't we?
I agree that there's room for optimization here.
Show 5 quoted lines
>> if (max_depth< 0) >> return 1; > > Isn't this original check much cheaper than the new test based on many > comparisons and should be at the beginning of the function?
Yes.
Show 11 quoted lines
>> @@ -826,6 +886,25 @@ static int help_callback(const struct option *opt, const char *arg, int unset)
>> return -1;
>> }
>>
>> +static int exclude_dir_callback(const struct option *opt, const char *arg,
>> + int unset)
>> +{
>> + struct string_list *exclude_dir_list = opt->value;
>> + char *s1 = (char *)arg;
>
> What is this cast for?It avoids:
"builtin/grep.c:893: warning: initialization discards qualifiers from pointer target type"
Show 7 quoted lines
>> + /* We do not want leading or trailing slashes. */
>> + while (*s1 == '/') {
>> + s1++;
>> + }
>
> Can the result of this loop become an empty string, and what happens to
> the rest of the logic when it happens?If the string is just forward slashes, then it will become an empty string, which will strdup successfully, and then that particular --exclude-dir will have no effect. Just tested that case and did not find a bug.
>> + char *s2 = strdup(s1); > > decl-after-statement.
Oops.
Sadly, "gcc -Wall -std=c89" does not warn for this. ("-pedantic" does.)> Use xstrdup().
Okay.
Show 6 quoted lines
>> + while (*s2&& s2[strlen(s2)-1] == '/') {
>> + s2[strlen(s2)-1] = '\0';
>> + }
>
> Don't scan s2 repeatedly to find its end by calling strlen(s2) on it.
> Find its length once, and scan backwards from there yourself.Okay. I'll try to send out a revised version of this patch soon.
-- David Ripton dripton@ripton.net