"git check-attr" lists macros as being "set" -- feature or bug?

"git check-attr" lists macros as being "set" -- feature or bug?

[Michael Haggerty]

I am doing some work on git-check-attr, and I noticed something funny:
If a macro is used to set or clear attributes on a file in
.gitattributes, then the name of the macro itself is listed as an
attribute on that file.  Example:

$ git init
Initialized empty Git repository in /tmp/foo/.git/
$ echo '[attr]notest !test' > .gitattributes
$ echo 'no notest' >> .gitattributes

# This is expected:
$ git check-attr test -- no
no: test: unspecified

# This I found surprising:
$ git check-attr notest -- no
no: notest: set

I don't see the correct behavior documented anywhere.  If this is
considered a bug, then I offer to fix it.  If it is considered a
feature, then I offer to document it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: "git check-attr" lists macros as being "set" -- feature or bug?

[Jeff King]

On Tue, Jul 26, 2011 at 02:10:11PM +0200, Michael Haggerty wrote:

> I am doing some work on git-check-attr, and I noticed something funny:
> If a macro is used to set or clear attributes on a file in
> .gitattributes, then the name of the macro itself is listed as an
> attribute on that file.  Example:
> 
> $ git init
> Initialized empty Git repository in /tmp/foo/.git/
> $ echo '[attr]notest !test' > .gitattributes
> $ echo 'no notest' >> .gitattributes
> 
> # This is expected:
> $ git check-attr test -- no
> no: test: unspecified
> 
> # This I found surprising:
> $ git check-attr notest -- no
> no: notest: set
> 
> I don't see the correct behavior documented anywhere.  If this is
> considered a bug, then I offer to fix it.  If it is considered a
> feature, then I offer to document it.

I don't know the original rationale, but it seems like the only sane
behavior to me. If you care about the attributes that the macro is
setting, then you will test for those attributes, not caring whether
they came from a macro or not. The only time you will ask about the
macro itself is if you care whether the macro itself is set (though the
point of macros is that you don't care about them, but only the
well-known names of underlying attributes. So I think you would only
want to see this if you were doing some maintenance on your
gitattributes files themselves, rather than simply reading their
values).

But maybe I'm missing something.  Can you describe a use case where you
would want "notest" to be listed as "unspecified" in your example?

-Peff

Re: "git check-attr" lists macros as being "set" -- feature or bug?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know the original rationale, but it seems like the only sane
> behavior to me.

I know the original rationale, which was "we didn't see any need to single
out macros at all, and the code does what was the most straightforward
from the point of view of the implementation". If the resulting behaviour
ended up to be also sane, that would just mean that the implementation was
good ;-).

I agree that showing the macros as set/unset just like elemental
attributes is the only sane thing to do.  Majority of end users do not
care about how some attributes are macros (to cause other attributes to be
set or unset), would set "binary" to their JPEG files, and would expect
the check-attr to say "binary is set for this path", without having to
know that "binary" affects "diff" and "text".

When we say "check-attr tells you if the named attribute is set", do we
say "but macros cannot be examined this way" in the documentation?  If
not, I do not think we need any cluttering update.

It is a separate issue if macros should also be listed as the new feature
that lists all attributes given to a path. I tend to think the macro
attributes as well as the other attributes they set should all be shown.

Re: "git check-attr" lists macros as being "set" -- feature or bug?

[Michael Haggerty]

On 07/26/2011 09:43 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>> I don't know the original rationale, but it seems like the only sane
>> behavior to me.

I agree that the behavior is sane (and also convenient).

> When we say "check-attr tells you if the named attribute is set", do we
> say "but macros cannot be examined this way" in the documentation?  If
> not, I do not think we need any cluttering update.

I was confused by the following things:

* The word "macro" in other contexts (e.g., C macros) typically refers
to something that is fully replaced by its substitution text, leaving no
trace of the original macro or its name.

* In gitattributes(5), the following misleading text:

> ------------
> *.jpg -text -diff
> ------------
>
> but that may become cumbersome, when you have many attributes.  Using
> attribute macros, you can specify groups of attributes set or unset at
> the same time.  The system knows a built-in attribute macro, `binary`:
>
> ------------
> *.jpg binary
> ------------
>
> which is equivalent to the above.

It is *not* equivalent to the above, rather it is equivalent to

*.jpg binary -text -diff

(neglecting, of course, the recursive expansion of "binary").

I will soon send a documentation patch.

> It is a separate issue if macros should also be listed as the new feature
> that lists all attributes given to a path. I tend to think the macro
> attributes as well as the other attributes they set should all be shown.

I agree.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/