Surprising interaction of "binary" and "eol" gitattributes

> I would expect that the following .gitattributes file
> 
>     *       eol=crlf
>     *.png   -text
> 
> would leave EOL translation turned off for PNG files. In other words, I
> would expect that explicitly setting "-text" would take precedence over
> the fact that setting "eol" implies that a file should be considered to
> be "text".
> 
> I would even more strongly expect
> 
>     *       eol=crlf
>     *.png   binary
> 
> to turn off EOL translation for PNG files.
> 
> But in fact, in both of the above cases, EOL translation is turned *on*
> for PNG files.
> 
> I propose that "-text" should override any setting for "eol" (which
> would of course fix both problems, since "binary" is equivalent to
> "-diff -merge -text"). What do people think?
> 
> Michael
> 

> I would expect that the following .gitattributes file
>
>     *       eol=crlf
>     *.png   -text
>
> would leave EOL translation turned off for PNG files. In other words, I
> would expect that explicitly setting "-text" would take precedence over
> the fact that setting "eol" implies that a file should be considered to
> be "text".
>
> I would even more strongly expect
>
>     *       eol=crlf
>     *.png   binary
>
> to turn off EOL translation for PNG files.
>
> But in fact, in both of the above cases, EOL translation is turned *on*
> for PNG files.
>
> I propose that "-text" should override any setting for "eol" (which
> would of course fix both problems, since "binary" is equivalent to
> "-diff -merge -text"). What do people think?

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> I would expect that the following .gitattributes file
>>
>>      *       eol=crlf
>>      *.png   -text
>>
>> would leave EOL translation turned off for PNG files. In other words, I
>> would expect that explicitly setting "-text" would take precedence over
>> the fact that setting "eol" implies that a file should be considered to
>> be "text".
>>
>> I would even more strongly expect
>>
>>      *       eol=crlf
>>      *.png   binary
>>
>> to turn off EOL translation for PNG files.
>>
>> But in fact, in both of the above cases, EOL translation is turned *on*
>> for PNG files.
>>
>> I propose that "-text" should override any setting for "eol" (which
>> would of course fix both problems, since "binary" is equivalent to
>> "-diff -merge -text"). What do people think?
> Hmm, is there really something that needs a new proposal and
> opinions?
>
> The way I read the flow in convert.c is:
>
>      convert_to_git() uses input_crlf_action() to figure out what
>      crlf_to_git() conversion is necessary.
>
>      input_crlf_action() looks at text_attr and says CRLF_BINARY when
>      it is CRLF_BINARY without looking at eol_attr at all.
>
>      text_attr above is ca.crlf_action in convert_to_git().
>
>      The whole ca.* comes from convert_attrs() inspecting attributes
>      on the incoming path.
>
>      convert_attrs() inspects "eol" and "text" attributes, among
>      others, and sets crlf_action by calling git_path_check_crlf().
>
>      git_path_check_crlf() looks at the 'text' attribute; if it is
>      set to false, it returns CRLF_BINARY.
>
>      crlf_to_git() when given crlf_action==CRLF_BINARY is a no-op.
>
> So, with the above attributes where anything is eol=crlf by default
> and in addition *.png is binary (which contains -text), we shouldn't
> get any crlf munging.  Am I reading/following the code incorrectly?
>
> Puzzled....
> --

> On 03/05/2015 11:08 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> I would expect that the following .gitattributes file
>>>
>>>      *       eol=crlf
>>>      *.png   -text
>>>
>>> would leave EOL translation turned off for PNG files. In other words, I
>>> would expect that explicitly setting "-text" would take precedence over
>>> the fact that setting "eol" implies that a file should be considered to
>>> be "text".
>>>
>>> I would even more strongly expect
>>>
>>>      *       eol=crlf
>>>      *.png   binary
>>>
>>> to turn off EOL translation for PNG files.
>>>
>>> But in fact, in both of the above cases, EOL translation is turned *on*
>>> for PNG files.
>>>
>>> I propose that "-text" should override any setting for "eol" (which
>>> would of course fix both problems, since "binary" is equivalent to
>>> "-diff -merge -text"). What do people think?
>> Hmm, is there really something that needs a new proposal and
>> opinions?
>>
>> The way I read the flow in convert.c is:
>>
>>      convert_to_git() uses input_crlf_action() to figure out what
>>      crlf_to_git() conversion is necessary.
>>
>>      input_crlf_action() looks at text_attr and says CRLF_BINARY when
>>      it is CRLF_BINARY without looking at eol_attr at all.
>>
>>      text_attr above is ca.crlf_action in convert_to_git().
>>
>>      The whole ca.* comes from convert_attrs() inspecting attributes
>>      on the incoming path.
>>
>>      convert_attrs() inspects "eol" and "text" attributes, among
>>      others, and sets crlf_action by calling git_path_check_crlf().
>>
>>      git_path_check_crlf() looks at the 'text' attribute; if it is
>>      set to false, it returns CRLF_BINARY.
>>
>>      crlf_to_git() when given crlf_action==CRLF_BINARY is a no-op.
>>
>> So, with the above attributes where anything is eol=crlf by default
>> and in addition *.png is binary (which contains -text), we shouldn't
>> get any crlf munging.  Am I reading/following the code incorrectly?
>>
>> Puzzled....
>> -- 
> 
> I need to admit that I can't reproduce it here,
> the following should trigger it, but all test cases pass
> 
> 
> diff --git a/t/t0027-auto-crlf.sh b/t/t0027-auto-crlf.sh
> 
> index 452320d..22f031d 100755
> --- a/t/t0027-auto-crlf.sh
> +++ b/t/t0027-auto-crlf.sh
> @@ -37,7 +37,8 @@ create_gitattributes () {
>                 echo "*.txt text" >.gitattributes
>                 ;;
>                 -text)
> -               echo "*.txt -text" >.gitattributes
> +               echo "* eol=crlf" >.gitattributes
> +               echo "*.txt -text" >>.gitattributes
>                 ;;
>                 crlf)
>                 echo "*.txt eol=crlf" >.gitattributes
> 

> 
>> Oops, I misunderstood an internal bug report. In seems that it is the
>> following scenario that is incorrect:
>>
>>     *.png text=auto eol=crlf
> Hm, I don't know if we support this combination at all.

> The current logic supports auto-detection of text/binary,
> * text=auto
> (the files will get the line ending from core.eol or core.autocrlf)
> 
> or the  the setting of a line ending:
> *.sh eol=lf
> *.bat eol=crlf
> 
> 
> Is there a special use-case, which needs the combination of both ?

> On 03/06/2015 10:30 PM, Torsten Bögershausen wrote:
>> 
>>> Oops, I misunderstood an internal bug report. In seems that it is the
>>> following scenario that is incorrect:
>>>
>>>     *.png text=auto eol=crlf
>> Hm, I don't know if we support this combination at all.
>
> The user can specify this combination in a .gitattributes file and we
> have to react to it *some way*.
>
> Theoretically we could document that
> this combination is undefined and/or emit an error if we see this
> combination, but we don't do so.
>
>> The current logic supports auto-detection of text/binary,
>> * text=auto
>> (the files will get the line ending from core.eol or core.autocrlf)
>> 
>> or the  the setting of a line ending:
>> *.sh eol=lf
>> *.bat eol=crlf
>> 
>> 
>> Is there a special use-case, which needs the combination of both ?
>
> I'm still trying to infer the spirit of the current behavior, so caveats
> here.
>
> This comes from a real-life scenario where a user, somewhere early in
> .gitattributes, had
>
>     * text
>     * eol=crlf
>
> and then later (this could be in a subdirectory) tried to carve out
> exceptions to this rule by using
>
>     *.png binary
>     * text=auto
>
> Intuitively it *feels* like either of the later lines should suppress
> EOL translation in PNG files (assuming the PNG file has a NUL byte in
> the first 8k, which this one did).

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> [...]
>> It seems to me that setting "text=auto" should mean that Git uses its
>> heuristic to guess whether a particular file is text or not, and then
>> treats the file as if it had "text" or "-text" set. If the latter, then
>> EOL translation should be suppressed.
> 
> ... I think this makes even more sense. I do not think the code is
> set up to do so.  To be honest, eol_attr thing introduced in
> fd6cce9e (Add per-repository eol normalization, 2010-05-19) always
> confuses me whenever I follow this codepath.

>> It also seems to me that "binary" should imply "-eol".
> 
> I thought that "eol" attribute is not even looked at when you say
> "binary"; that is what I recall finding out when I dug into this
> earlier in the thread.

> Well, that's true, but the "eol" attribute can regain its effect if
> "binary" is followed by "text" or "text=auto". So I guess the simplest
> question is as follows. Suppose I have the following .gitattributes:
>
>     a.foo eol=crlf
>     a.foo binary
>     a.foo text
>
> It is obvious in this case that a.foo should be treated as a text file.
> Should it be processed with "eol=crlf", or should the intervening
> "binary" imply "-eol"?

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Well, that's true, but the "eol" attribute can regain its effect if
>> "binary" is followed by "text" or "text=auto". So I guess the simplest
>> question is as follows. Suppose I have the following .gitattributes:
>>
>>      a.foo eol=crlf
>>      a.foo binary
>>      a.foo text
>>
>> It is obvious in this case that a.foo should be treated as a text file.
>> Should it be processed with "eol=crlf", or should the intervening
>> "binary" imply "-eol"?
> I would say former.  You find out what attributes apply to a path
> and then consider the collective effect of these attributes that
> survived.
>
> So the second "No it is not text" which is overruled by the "oops,
> no that is text" later should not get in the picture, I would say.
>
> As binary is not just -text and turns other things off, those other
> things will be off after these three.

> On 03/10/2015 11:54 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>
>>> Well, that's true, but the "eol" attribute can regain its effect if
>>> "binary" is followed by "text" or "text=auto". So I guess the simplest
>>> question is as follows. Suppose I have the following .gitattributes:
>>>
>>>      a.foo eol=crlf
>>>      a.foo binary
>>>      a.foo text
>>>
>> As binary is not just -text and turns other things off, those other
>> things will be off after these three.
> Not sure if I follow:
> Whenever you specify -text, the eol doesn't matter, or what do I miss ?

> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> Well, that's true, but the "eol" attribute can regain its effect if
>> "binary" is followed by "text" or "text=auto". So I guess the simplest
>> question is as follows. Suppose I have the following .gitattributes:
>>
>>      a.foo eol=crlf
>>      a.foo binary
>>      a.foo text
>>
>> It is obvious in this case that a.foo should be treated as a text file.
>> Should it be processed with "eol=crlf", or should the intervening
>> "binary" imply "-eol"?
>
> I would say former.  You find out what attributes apply to a path
> and then consider the collective effect of these attributes that
> survived.
>
> So the second "No it is not text" which is overruled by the "oops,
> no that is text" later should not get in the picture, I would say.
>
> As binary is not just -text and turns other things off, those other
> things will be off after these three.

>> I would say former.  You find out what attributes apply to a path
>> and then consider the collective effect of these attributes that
>> survived.
>>
>> So the second "No it is not text" which is overruled by the "oops,
>> no that is text" later should not get in the picture, I would say.
>>
>> As binary is not just -text and turns other things off, those other
>> things will be off after these three.
>
> Is that how attribute lookup works? I.e., given a path, all attributes
> are collected?
>
> Isn't it more like: Here we are interested in the "eol" attribute of
> this file named "a.foo". And the lookup would find the first line that
> says "eol=crlf". Elsewhere, we are interested in the "binary"
> attribute of the file named "a.foo", and lookup would find the second
> line that sets the "binary" attribute. And again elsewhere, we ask for
> the "text" attribute, and we find the last line that sets the "text"
> property.
>
> Am I totally off track?

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Isn't it more like: Here we are interested in the "eol" attribute of
>> this file named "a.foo". And the lookup would find the first line that
>> says "eol=crlf". Elsewhere, we are interested in the "binary"
>> attribute of the file named "a.foo", and lookup would find the second
>> line that sets the "binary" attribute. And again elsewhere, we ask for
>> the "text" attribute, and we find the last line that sets the "text"
>> property.
>>
>> Am I totally off track?
>
> In the codepath in question, we say "we are interested in text and
> eol attributes", grab the values (set/unset/set-to-value/unspecified)
> for these two for the path we are interested in from all the
> applicable gitattributes file and then act on the result.

> On 03/06/2015 10:30 PM, Torsten Bögershausen wrote:
>>
>>> Oops, I misunderstood an internal bug report. In seems that it is the
>>> following scenario that is incorrect:
>>>
>>>     *.png text=auto eol=crlf
>> Hm, I don't know if we support this combination at all.
> 
> The user can specify this combination in a .gitattributes file and we
> have to react to it *some way*. Theoretically we could document that
> this combination is undefined and/or emit an error if we see this
> combination, but we don't do so.
> 
>> The current logic supports auto-detection of text/binary,
>> * text=auto
>> (the files will get the line ending from core.eol or core.autocrlf)
>>
>> or the  the setting of a line ending:
>> *.sh eol=lf
>> *.bat eol=crlf
>>
>>
>> Is there a special use-case, which needs the combination of both ?
> 
> I'm still trying to infer the spirit of the current behavior, so caveats
> here.
> 
> This comes from a real-life scenario where a user, somewhere early in
> .gitattributes, had
> 
>     * text
>     * eol=crlf
> 
> and then later (this could be in a subdirectory) tried to carve out
> exceptions to this rule by using
> 
>     *.png binary
>     * text=auto

> Intuitively it *feels* like either of the later lines should suppress
> EOL translation in PNG files (assuming the PNG file has a NUL byte in
> the first 8k, which this one did).
> 
> It seems to me that setting "text=auto" should mean that Git uses its
> heuristic to guess whether a particular file is text or not, and then
> treats the file as if it had "text" or "-text" set. If the latter, then
> EOL translation should be suppressed.
> 
> It also seems to me that "binary" should imply "-eol".
> 
> Michael
> 

> On 10.03.15 20:25, Michael Haggerty wrote:
>> [...]
>> I'm still trying to infer the spirit of the current behavior, so caveats
>> here.
>>
>> This comes from a real-life scenario where a user, somewhere early in
>> .gitattributes, had
>>
>>     * text
>>     * eol=crlf
>>
>> and then later (this could be in a subdirectory) tried to carve out
>> exceptions to this rule by using
>>
>>     *.png binary
>>     * text=auto
> Hm,
> I can see 2 problems here:
> the "binary" attribute does not exist at all.
>
> I sometimes which we had it, but we don't.
> There is "text" and "-text", and that is it.

> The other problem is the order of the lines, which is fully
> intuitive for each person who has ever written a "matching parser".
> 
> The parser matches each file namr on it's own, depending on the matching:
> 
> *.png -text
> * text=auto
> means that all png files are binary, and ALL files are "auto".
> 
> Guess what happens to the png's ?
> 
> The second rule wins, as it is the last rule processed.
> 
> git check-attr text *
> A.png: text: auto
> B.txt: text: auto