Use `-text` git attribute instead of `text eol=...'

[MForster]

Summary

These automatic conversions lead to issues in various workflows, and all
we want here are files that retain their line endings under all
circumstances. binary captures that perfectly well and leads to fewer
issues.

[MForster]

Force-add crlf.cpp(.expected) with CRLF line endings.

[labath]

I believe (based on some similar issues I was solving in lldb) that -text instead of binary is sufficient to prevent git from fidlling with the contents, while maintaining the ability to diff the files.

Other than that, I am in favour of this approach, but I'd recommend getting this reviewed by someone on the clang-tools team.

[MForster]

Use -text instead of binary.

[ilya-biryukov]

LGTM

[ilya-biryukov]

NIT: change the commit message to say -text instead of binary

[MForster]

In D124606#3479798, @ilya-biryukov wrote:

NIT: change the commit message to say -text instead of binary

Ah, sorry, saw this too late...

[MForster]

In D124606#3479805, @MForster wrote:

In D124606#3479798, @ilya-biryukov wrote:

NIT: change the commit message to say -text instead of binary

Ah, sorry, saw this too late...

Turns out I had already renamed the commit :-). I also updated the code review title...

[aaronpuchert]

I think this change broke again what D97625 was trying to fix. These are text files and we want them to keep the specified line endings regardless of the local git configuration. See also @smeenai's comment D124563#3478625.

clang-tools-extra/test/.gitattributes

7–15

My understanding is that -text removes an attribute, but what attribute is there to remove? There is no global .gitattributes that would set it in the first place. This change just reverts to the status quo before D97625, or rather before that file was moved.

[labath]

clang-tools-extra/test/.gitattributes

7–15

Are you sure about that? Git seems to distinguish between "unset" and "unspecified" values of an attribute:

Each attribute can be in one of these states for a given path:

Set
    The path has the attribute with special value "true"; this is specified by listing only the
    name of the attribute in the attribute list.

Unset
    The path has the attribute with special value "false"; this is specified by listing the
    name of the attribute prefixed with a dash - in the attribute list.

Set to a value
    The path has the attribute with specified string value; this is specified by listing the
    name of the attribute followed by an equal sign = and its value in the attribute list.

Unspecified
    No pattern matches the path, and nothing says if the path has or does not have the
    attribute, the attribute for the path is said to be Unspecified.

And then it says this about the text attribute in particular:

Set
    Setting the text attribute on a path enables end-of-line normalization and marks the
    path as a text file. End-of-line conversion takes place without guessing the content
    type.

Unset
    Unsetting the text attribute on a path tells Git not to attempt any end-of-line
    conversion upon checkin or checkout.

Set to string value "auto"
    When text is set to "auto", the path is marked for automatic end-of-line conversion. If
    Git decides that the content is text, its line endings are converted to LF on checkin.
    When the file has been committed with CRLF, no conversion is done.

Unspecified
    If the text attribute is unspecified, Git uses the core.autocrlf configuration variable
    to determine if the file should be converted.

[ilya-biryukov]

In D124606#3480012, @aaronpuchert wrote:

I think this change broke again what D97625 was trying to fix. These are text files and we want them to keep the specified line endings regardless of the local git configuration. See also @smeenai's comment D124563#3478625.

This change should be doing exactly that.
The behaviour in the mentioned comment should apply when the text attribute is unspecified, not when it's explicitly unset.
From git docs:

Unset
Unsetting the text attribute on a path tells Git not to attempt any end-of-line conversion upon checkin or checkout.

Unspecified
If the text attribute is unspecified, Git uses the core.autocrlf configuration variable to determine if the file should be converted.

clang-tools-extra/test/.gitattributes

7–15

The git docs seem to be pretty clear that unsetting the attribute should do the right thing:

Unset
Unsetting the text attribute on a path tells Git not to attempt any end-of-line conversion upon checkin or checkout.

It seems git is supposed to do the right thing for us here, unless I misunderstand something.

[ilya-biryukov]

@labath, ah, sorry, you beat me to it.

[MForster]

This comment was removed by MForster.

[aaronpuchert]

Fair enough, but don't we want to enforce LF or CRLF, respectively? An editor could inadvertently change the line endings, and someone might not notice before committing. Explicitly specifying the right line endings gives us the proper line endings even if some editors mess things up.

[aaronpuchert]

Also I still don't understand what specifically this is fixing. What exactly was wrong about the previous configuration?

The commit message talks about "various workflows" but which workflows are that? Does that mean no one can use .gitattributes with something else than -text in LLVM?

[labath]

In D124606#3480164, @aaronpuchert wrote:

Fair enough, but don't we want to enforce LF or CRLF, respectively?

Sure, but is the version control system the right tool to do that? I think it'd be better to have the test itself confirm the consistency of the data (or convert it into the right format at runtime) and not rely on magical conversions within the VC tool.

An editor could inadvertently change the line endings, and someone might not notice before committing.

From what I understood, most of these tests would immediately break (due to hardcoded offsets) if the line endings were changed.

In D124606#3480187, @aaronpuchert wrote:

Also I still don't understand what specifically this is fixing. What exactly was wrong about the previous configuration?

The branch switching issue (discussed in D124563) is one. We also ran into problems when importing this into our version control system (as it did not do the .gitattributes conversion) -- although one could say that is not an upstream problem...

[aaronpuchert]

In D124606#3480281, @labath wrote:

In D124606#3480164, @aaronpuchert wrote:

An editor could inadvertently change the line endings, and someone might not notice before committing.

From what I understood, most of these tests would immediately break (due to hardcoded offsets) if the line endings were changed.

Ok, that's a valid point.

The branch switching issue (discussed in D124563) is one.

That is an issue, but I don't think this change fixes it. D124563 did normalize the file endings and switching between branches after that should work fine. Switching between branches before that will remain broken because the files had not been normalized. I don't think any change can fix that because they can't retroactively change the inconsistent snapshots.

We also ran into problems when importing this into our version control system (as it did not do the .gitattributes conversion) -- although one could say that is not an upstream problem...

Got it, so some implementations simply can't deal with this.

[Meinersbur]

In D124606#3479793, @ilya-biryukov wrote:

LGTM

This and the commit (@MForster ) was too hasty. There should have been time for people discussing D124563 and D97625 to react, and should have been added as reviewers. I didn't even see @ilya-biryukov participating in the other discussions.

[ilya-biryukov]

In D124606#3484740, @Meinersbur wrote:

In D124606#3479793, @ilya-biryukov wrote:

LGTM

This and the commit (@MForster ) was too hasty. There should have been time for people discussing D124563 and D97625 to react, and should have been added as reviewers. I didn't even see @ilya-biryukov participating in the other discussions.

We committed this to unbreak our internal integrate at Google. Integrates are time-critical for us, so we normally choose to unbreak early and have post-commit discussions instead.
We should have probably communicated this better in commit messages and reviews.

clang-tools-extra/test/.gitattributes

 # CRLF (Windows) line endings take two bytes instead of one, so any tests that
 # rely on or check fixed character -offset, Offset: or FileOffset: locations
 # will fail when run on input files checked out with different line endings.
 
 # These test input files rely on one-byte Unix (LF) line-endings, as they use
 # fixed -offset, FileOffset:, or Offset: numbers in their tests.
-clang-apply-replacements/ClangRenameClassReplacements.cpp text eol=lf
-clang-apply-replacements/Inputs/basic/basic.h text eol=lf
-clang-apply-replacements/Inputs/format/no.cpp text eol=lf
-clang-apply-replacements/Inputs/format/yes.cpp text eol=lf
-clang-tidy/infrastructure/export-diagnostics.cpp text eol=lf
+clang-apply-replacements/ClangRenameClassReplacements.cpp binary
+clang-apply-replacements/Inputs/basic/basic.h binary
+clang-apply-replacements/Inputs/format/no.cpp binary
+clang-apply-replacements/Inputs/format/yes.cpp binary
+clang-tidy/infrastructure/export-diagnostics.cpp binary
 
 # These test input files rely on two-byte Windows (CRLF) line endings.
-clang-apply-replacements/Inputs/crlf/crlf.cpp text eol=crlf
-clang-apply-replacements/Inputs/crlf/crlf.cpp.expected text eol=crlf
+clang-apply-replacements/Inputs/crlf/crlf.cpp binary
+clang-apply-replacements/Inputs/crlf/crlf.cpp.expected binary