Re: [RFC/PATCH] Add --exclude-dir option to git grep

[Junio C Hamano <gitster@pobox.com>]

David Ripton <dripton@ripton.net> 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@ripton.net>

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.