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

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

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

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

Jeff King <peff@peff.net> writes:

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.

On 07/26/2011 09:43 PM, Junio C Hamano wrote:

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

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:

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.

I agree.

Michael