Re: [PATCH/WIP] attr: make attributes depend on file type

From: Michael J Gruber <git@drmicha.warpmail.net>
Date: Mar 1, 2011, 07:46 UTC
Message-ID: <4D6CA45F.5000001@drmicha.warpmail.net>
In-Reply-To: <7v39n8jbj2.fsf@alter.siamese.dyndns.org>

Junio C Hamano venit, vidit, dixit 28.02.2011 18:48:

Show 26 quoted lines

> Jeff King <peff@peff.net> writes:
> 
>> The only downside is that it is technically a regression if somebody was
>> using gitattributes for the bizarrely named file "symlink:". It seems
>> pretty unlikely, but possibly we should be carving out a syntactic
>> namespace like:
>>
>>   ^[a-z]+:
>>
>> or even:
>>
>>   ^[a-z]+(=[^:]*)?:
> 
> Or even '^:[a-z]+(=[^,=])?(,[a-z]+(=[^,=])?)*:' to
> 
>  (1) always have some special character at the beginning, to limit the
>      extent of the damage to existing funny pathnames (i.e. to collide
>      your pathname must begin with such a special character).  This also
>      has a nice side effect of making it clear that something special is
>      going on; and
> 
>  (2) allow more than one such special on the line, comma-separated.
> 
> But that is just a small bikeshed.  Other than that, I like what Michael
> and you are aiming for (I am only commenting on the general direction at
> this moment, as I haven't looked at the patch at all yet).

Thanks, Junio, and Jeff, for the helpful comments. I isolated the naming convention (now "symlink:") to a single spot in attr.c so that changing it (then ":symlink:") is always simple while this develops. (I was afraid that the low likelikehood that someone's file names would collide could be too much already.)

Having a standard pattern applied to regular files only is an intended side-effect; more precisely: to non-symlinks right now. We could tighten that to IS_REG, of course, and easily add others (or :other:, :nonreg:).

What I really would need help with is checking all call sites of git_checkattr(), which gained a new mode parameter: I used the proper mode where available (from diff_filespec, e.g., i.e.: taking mode from index or tree) or by looking it up in the fs (using lstat() in builtin/check-attr.c), and I used mode=0 in other cases, assuming that this conveys IS_REG and that we're sure those call sites are about regular files or blobs. I tried to avoid unmotivated lstat().

Michael