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