From: David Ripton <dripton@ripton.net>
Date: Sat, 25 Sep 2010 02:07:40 GMT
Subject: Re: [RFC/PATCH] Add --exclude-dir option to git grep
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.

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

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

>> @@ -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"

>> +	/* 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.

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