From: Jean-Noël Avila <avila.jn@gmail.com>
Date: Thu, 06 Dec 2012 09:02:12 GMT
Subject: Re: [PATCH] Add directory pattern matching to attributes
Message-ID: <50C05F14.8080809@gmail.com>
In-Reply-To: <7vlidcjcm9.fsf@alter.siamese.dyndns.org>

On 06/12/2012 00:35, Junio C Hamano wrote:
> "Jean-Noël AVILA" <jn.avila@free.fr> writes:
>
>> -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.
>
>>   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.

Thank you for your comments. Changing the whole attr.h interface header
is definitely not a good option, but at some point, we may need more 
information
on the path to be able to match a path pattern against it.

>
> 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 is quite different from the pattern matching documented for gitignore.

Moreover,

  $ mkdir -p not-ignored-dir/ignored-dir
  $ echo test >not-ignored-dir/ignored-dir/ignored
  $ echo 'ignored-dir/*' >.gitattributes
  $ git add not-ignored-dir .gitattributes
  $ git commit -m '.'
  $ git archive HEAD | tar tf -
.gitattributes
not-ignored-dir/
not-ignored-dir/ignored-dir/
not-ignored-dir/ignored-dir/ignored


>
> This patch seems to be based on a stale codebase.
Sorry. I thought the patch submissions had to be based on the 'maint' 
branch.

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

OK. I like the idea of proposing a path ending with '/' when it is meant 
to be
directory. This would not change the interface attr.h . I will rework 
with this idea.

Thank you.