[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

The following files have their line endings (when checked out on disk) changed 
from CRLF to LF by this patch. Seems harmless, but I just wanted to confirm 
that it was expected:

  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-if-constexpr.cpp
  clang-tools-extra/test/modularize/Inputs/CompileError/module.modulemap
  clang-tools-extra/test/modularize/Inputs/MissingHeader/module.modulemap
  clang-tools-extra/test/pp-trace/Inputs/module.map

(The files which should be CRLF according to `.gitattributes` remained CRLF.)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

Most of the files changed here are not affected by the `.gitattributes` file, 
such as

  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include2.h
  
clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h
  
clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-if-constexpr.cpp
  clang-tools-extra/test/modularize/Inputs/CompileError/module.modulemap
  clang-tools-extra/test/modularize/Inputs/MissingHeader/module.modulemap
  clang-tools-extra/test/pp-trace/Inputs/module.map

The only affected files seem to be

  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected

Looking back at D97625 , the main goal of that 
change was to ensure that tests relying on LF endings also work on Windows, and 
the two files that want CRLF did not in fact cause test failures.

So maybe we should simply remove

  # 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

from the `.gitattributes` file again. Then we can keep them stored with CRLF 
endings, and each file has a comment anyway that they rely on them. The files 
have been there for many years and never been a problem.

Could you test if this works for you?

We can still discuss normalizing the other files, but maybe we can get some 
reviewers involved in `clang-tools-extra` for that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

In D124563#3478623 , @aaronpuchert 
wrote:

> So maybe we should simply remove
>
>   # 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
>
> from the `.gitattributes` file again. Then we can keep them stored with CRLF 
> endings, and each file has a comment anyway that they rely on them. The files 
> have been there for many years and never been a problem.

I *think* this would mean that if you're on Windows and have `core.autocrlf` 
set to `input`, when you commit changes to this files, Git will convert them 
back to LF line endings. Not 100% sure of that though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Shoaib Meenai via Phabricator via cfe-commits]

smeenai added a comment.

If I check out this commit and then check out the previous commit, 
`clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp` and 
`clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected` 
become modified in my working directory; their line endings are changed from 
CRLF to LF. That seems undesirable.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Di Mo via Phabricator via cfe-commits]

modimo added a comment.

In D124563#3478561 , @smeenai wrote:

> The following files have their line endings (when checked out on disk) 
> changed from CRLF to LF by this patch. Seems harmless, but I just wanted to 
> confirm that it was expected:
>
>   
> clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include.h
>   
> clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/readability-duplicate-include2.h
>   
> clang-tools-extra/test/clang-tidy/checkers/Inputs/readability-duplicate-include/system/sys/types.h
>   
> clang-tools-extra/test/clang-tidy/checkers/readability-else-after-return-if-constexpr.cpp
>   clang-tools-extra/test/modularize/Inputs/CompileError/module.modulemap
>   clang-tools-extra/test/modularize/Inputs/MissingHeader/module.modulemap
>   clang-tools-extra/test/pp-trace/Inputs/module.map
>
> (The files which should be CRLF according to `.gitattributes` remained CRLF.)

Good catch. Looking at git documentation 
(https://git-scm.com/docs/gitattributes#_text) by virtue of applying `* 
text=auto` the line endings will be stored internally as LF and then use the 
system settings on checkout. Looking on my linux box I see LF endings but 
checking this out on Windows I see CRLF endings so this behavior is correct. 
This diff is displaying index changes which does move from CRLF->LF. I didn't 
see any test failures on Linux and these files will not have changed on Windows 
so this should be good.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Di Mo via Phabricator via cfe-commits]

modimo added a comment.

In D124563#3478627 , @smeenai wrote:

> If I check out this commit and then check out the previous commit, 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp` and 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected`
>  become modified in my working directory; their line endings are changed from 
> CRLF to LF. That seems undesirable.

Good catch, I see it as well. The `.gitattributes` change needs to be atomic 
with the renormalization change. If I checkout from main after the 
`.gitattributes` change (`git checkout ac5f7be6a868`) I'll see the files as 
dirty. However, if I checkout the revision before (`git checkout 
ac5f7be6a868~1`) this goes away. I think the way to go is to revert 
ac5f7be6a868 
 then land 
everything as a single stack to prevent this issue.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

Regarding `crlf.cpp`(`.expected`), adding the modified changes to a commit 
indeed solved the problem I described in 
https://reviews.llvm.org/rGac5f7be6a8688955a282becf00eebc542238a86b#1080897 and 
I was working with that so far. Still don't know why Linux git doesn't do the 
same?

Don't know about the other files, but according to 
https://github.com/git/git/commit/af6e0fe3a589d58bfd508c1c6ccbeb38586eb06b, 
this seems the right thing to do.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D124563#3478625 , @smeenai wrote:

> I *think* this would mean that if you're on Windows and have `core.autocrlf` 
> set to `input`, when you commit changes to this files, Git will convert them 
> back to LF line endings. Not 100% sure of that though.

As far as I recall this was done because there are many ways how the line 
endings can be accidentally changed (`core.autocrlf` being one of then, others 
are `clang-format` or text editors that do not retain line endings) and 
committed. Removing the files from `.gitignore` undo the intended line ending 
pin from D97625 .


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D124563#3478625 , @smeenai wrote:

> I *think* this would mean that if you're on Windows and have `core.autocrlf` 
> set to `input`, when you commit changes to this files, Git will convert them 
> back to LF line endings. Not 100% sure of that though.

Ok, that's a good point.

In D124563#3478642 , @modimo wrote:

> Good catch. Looking at git documentation 
> (https://git-scm.com/docs/gitattributes#_text) by virtue of applying `* 
> text=auto` the line endings will be stored internally as LF and then use the 
> system settings on checkout.

How about we remove `* text=auto` for now? Or would that break something?

Perhaps we can discuss `* text=auto` on the entire code base separately.

In D124563#3478726 , @Meinersbur 
wrote:

> Still don't know why Linux git doesn't do the same?

Linux checks the modified time stamp before looking at file contents. After 
`touch`ing the files I can also reproduce it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D124563#3478747 , @aaronpuchert 
wrote:

> How about we remove `* text=auto` for now? Or would that break something?

The documentation  says that:

> If you want to ensure that text files that any contributor introduces to the 
> repository have their line endings normalized, you can set the `text` 
> attribute to "auto" for `all` files.
>
>   *   text=auto

And for now we don't actually want that. So I'd say remove that line and 
normalize the two remaining files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Michael Kruse via Phabricator via cfe-commits]

Meinersbur added a comment.

In D124563#3478627 , @smeenai wrote:

> If I check out this commit and then check out the previous commit, 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp` and 
> `clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected`
>  become modified in my working directory; their line endings are changed from 
> CRLF to LF. That seems undesirable.

In the current top-of-tree, the gitattributes line endings and the actual line 
endings contradict each other, leading to such inconsistencies.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert added a comment.

In D124563#3478653 , @modimo wrote:

> I think the way to go is to revert ac5f7be6a868 
>  then 
> land everything as a single stack to prevent this issue.

Doesn't change anything about existing commits though. There is no way to fix 
that now I'm afraid. We can only fix it for future commits.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124563: Renormalize line endings after ac5f7be6a8688955a282becf00eebc542238a86b

[Aaron Puchert via Phabricator via cfe-commits]

aaronpuchert updated this revision to Diff 425670.
aaronpuchert edited the summary of this revision.
aaronpuchert removed a subscriber: drodriguez.
aaronpuchert added a comment.
Herald added a subscriber: jdoerfert.

Drop `* text=auto`, so that we renormalize only the files that need it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D124563/new/

https://reviews.llvm.org/D124563

Files:
  clang-tools-extra/test/.gitattributes
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
  clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected


Index: 
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
===
--- 
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
+++ 
clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
@@ -1,6 +1,6 @@
-
-// This file intentionally uses a CRLF newlines!
-
-void foo() {
-  int *x = nullptr;
-}
+
+// This file intentionally uses a CRLF newlines!
+
+void foo() {
+  int *x = nullptr;
+}
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
===
--- clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
@@ -1,6 +1,6 @@
-
-// This file intentionally uses a CRLF newlines!
-
-void foo() {
-  int *x = 0;
-}
+
+// This file intentionally uses a CRLF newlines!
+
+void foo() {
+  int *x = 0;
+}
Index: clang-tools-extra/test/.gitattributes
===
--- clang-tools-extra/test/.gitattributes
+++ clang-tools-extra/test/.gitattributes
@@ -2,10 +2,6 @@
 # rely on or check fixed character -offset, Offset: or FileOffset: locations
 # will fail when run on input files checked out with different line endings.
 
-# Most test input files should use native line endings, to ensure that we run
-# tests against both line ending types.
-* text=auto
-
 # 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


Index: clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
===
--- clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp.expected
@@ -1,6 +1,6 @@
-
-// This file intentionally uses a CRLF newlines!
-
-void foo() {
-  int *x = nullptr;
-}
+
+// This file intentionally uses a CRLF newlines!
+
+void foo() {
+  int *x = nullptr;
+}
Index: clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
===
--- clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
+++ clang-tools-extra/test/clang-apply-replacements/Inputs/crlf/crlf.cpp
@@ -1,6 +1,6 @@
-
-// This file intentionally uses a CRLF newlines!
-
-void foo() {
-  int *x = 0;
-}
+
+// This file intentionally uses a CRLF newlines!
+
+void foo() {
+  int *x = 0;
+}
Index: clang-tools-extra/test/.gitattributes
===
--- clang-tools-extra/test/.gitattributes
+++ clang-tools-extra/test/.gitattributes
@@ -2,10 +2,6 @@
 # rely on or check fixed character -offset, Offset: or FileOffset: locations
 # will fail when run on input files checked out with different line endings.
 
-# Most test input files should use native line endings, to ensure that we run
-# tests against both line ending types.
-* text=auto
-
 # 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
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits