David Ripton <dripton@xxxxxxxxxx> writes:
> It works much like the same option in recent versions of GNU grep.
> Any directory name which matches the option will not be searched.
>
> For example, "git grep --exclude-dir Documentation malloc"
>
> Signed-off-by: David Ripton <dripton@xxxxxxxxxx>
Thanks.
> +/* Return a sorted string_list of all possible directories within path.
> + *
> + * e.g. if path is "foo/bar/baz", then return a string_list with:
> + * "bar"
> + * "bar/baz"
> + * "baz"
> + * "foo"
> + * "foo/bar"
> + * "foo/bar/baz"
> + */
> +static struct string_list subdirs(const char *path)
> +{
> +...
> +}
> +
> /*
> * Return non-zero if max_depth is negative or path has no more then max_depth
> * slashes.
> */
> -static int accept_subdir(const char *path, int max_depth)
> +static int accept_subdir(const char *path, int max_depth,
> + struct string_list exclude_dir_list)
> {
> + struct string_list subdir_list = subdirs(path);
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?
For example, if we are about to visit "foo/bar/baz", that would mean we
were in "foo/bar" and already checked that our exclude list is Ok with
either "foo", "foo/bar" or "bar"; shouldn't we be skipping the test for
these three expansions at least? IOW, when checking against the exclude
list, shouldn't we be testing with "baz", "bar/baz" and "foo/bar/baz" and
nothing else?
> + int i;
> + for (i = 0; i < subdir_list.nr; i++) {
> + if (string_list_has_string(&exclude_dir_list, subdir_list.items[i].string)) {
> + string_list_clear(&subdir_list, 0);
> + return 0;
> + }
> + }
> + string_list_clear(&subdir_list, 0);
> +
> 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?
> @@ -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?
> + /* 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?
> + char *s2 = strdup(s1);
decl-after-statement. Use xstrdup().
> + 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.
--
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