Re: [PATCH] Add directory pattern matching to attributes

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2012, 23:35 UTC
Message-ID: <7vlidcjcm9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201212052310.30690.jn.avila@free.fr>

"Jean-Noël AVILA" <jn.avila@free.fr> writes:

Show 7 quoted lines

> -static void prepare_attr_stack(const char *path)
> +static void prepare_attr_stack(const char *path, unsigned mode)
>  {
>  	struct attr_stack *elem, *info;
>  	int dirlen, len;
> @@ -645,28 +645,43 @@ static void prepare_attr_stack(const char *path)
>  }

Why?

The new "mode" parameter does not seem to be used in this function at all.

Show 44 quoted lines

>  static int path_matches(const char *pathname, int pathlen,
> -			const char *pattern,
> +			const unsigned mode, char *pattern,
>  			const char *base, int baselen)
>  {
> -	if (!strchr(pattern, '/')) {
> +	size_t len;
> +	char buf[PATH_MAX];
> +	char * lpattern = buf;
> +	len = strlen(pattern);
> +	if (PATH_MAX <= len)
> +		return 0;
> +	strncpy(buf,pattern,len);
> +	buf[len] ='\0';
> +	if (len && lpattern[len - 1] == '/') {
> +		if (S_ISDIR(mode))
> +			lpattern[len - 1] = '\0';
> +		else
> +			return 0;
> +	}
> +	if (!strchr(lpattern, '/')) {
>  		/* match basename */
>  		const char *basename = strrchr(pathname, '/');
>  		basename = basename ? basename + 1 : pathname;
> -		return (fnmatch_icase(pattern, basename, 0) == 0);
> +		return (fnmatch_icase(lpattern, basename, 0) == 0);
>  	}
>  	/*
>  	 * match with FNM_PATHNAME; the pattern has base implicitly
>  	 * in front of it.
>  	 */
> -	if (*pattern == '/')
> -		pattern++;
> +	if (*lpattern == '/')
> +		lpattern++;
>  	if (pathlen < baselen ||
>  	    (baselen && pathname[baselen] != '/') ||
>  	    strncmp(pathname, base, baselen))
>  		return 0;
>  	if (baselen != 0)
>  		baselen++;
> -	return fnmatch_icase(pattern, pathname + baselen, FNM_PATHNAME) == 0;
> +	return fnmatch_icase(lpattern, pathname + baselen, FNM_PATHNAME) == 0;
>  }

It appears to me that you are forcing the caller to tell this function if the path is a directory, but in the attribute system, the caller does not necessarily know if the path it is passing is meant to be a directory or a regular file. "check-attr" is meant to be usable against a path that does not even exist on the working tree, so using stat() or lstat() in it is not a solution. In other words, it is unfair (read: unworkable) to force it to append a trailing slash after the path it calls this function with.

If you are interested in export-subst, all is not lost, though:

	$ git init
        $ mkdir a
        $ >a/b
        $ echo a export-ignore >.gitattributes
        $ git add a/b .gitattributes
	$ git commit -m initial
        $ git archive HEAD | tar tf -
        .gitattributes
        $ exit

You could change the "echo" to

	$ echo "a/*" export-ignore >.gitattributes

as well, but it seems to create an useless empty directory "a/" in the output, which I think is an unrelated bug in "git archive".

This patch seems to be based on a stale codebase. I do not think I'd be opposed to change the sementics to allow the callers that know that a path is a directory to optionally pass mode parameter by ending the pathname with slash (in other words, have "git check-attr" ask about a directory 'a' by saying "git check-attr export-subst a/", and lose the "mode" argument in this patch), or keep the "mode" parameter and instead allow "git check-attr" to ask about a directory that does not exist in the working tree by a more explicit "git check-attr --directory export-ignore a" or something. Such an enhancement should be done on top of the current codebase.

Thanks.