Add .gitattributes file#1396
Conversation
thisisalexandercook
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'm wondering if it is worth it to also handle the CRLF vs. LF mismatch in a .gitattributes file, would it eliminate the need to use this command? Some information on this here:
Hi, I think that is a better idea. I added the |
thisisalexandercook
left a comment
There was a problem hiding this comment.
Thanks for adding the attributes file, a few comments...
| *.class binary | ||
| *.png binary | ||
| *.jpg binary | ||
| *.pdf binary No newline at end of file |
There was a problem hiding this comment.
Two things:
(1) We have all of these binary files in our repo except .jpg. Wondering if we should only include file extensions we currently use or future proof it?
(2) I think the red circle with a - at the bottom of this diff means you don't have a newline char at the end of this file.
There was a problem hiding this comment.
(1) That was my intention, I must've forgot to delete it from the template.
(2) Will fix that
| Ensure line endings are handled properly. If necessary, run | ||
| \code{git config --global core.autocrlf true} to prevent line ending issues between | ||
| Windows and Unix-based systems. | ||
|
|
There was a problem hiding this comment.
I think it probably makes sense to leave some comment in the manual about this. Maybe mention its automated through .gitattributes and discuss its equivalence to using this command
There was a problem hiding this comment.
Added descriptions about that.
| :mainEnd | ||
| if "%OS%"=="Windows_NT" endlocal | ||
| :omega |
There was a problem hiding this comment.
Do we have a diff here because of the .gitattributes change?
There was a problem hiding this comment.
Yes, I think that is caused by git add --renormalize .
There was a problem hiding this comment.
ah I see, thanks for clarifying. I’m wondering if we actually don't want this file normalized to LF in the repo, since it’s a Windows-only batch file.
Here’s how I understand * text=auto: when we stage/commit a file, Git normalizes its contents to LF before writing the blob to the object database. This is great for cross-platform consistency because history is always LF-based, but on Windows a user with core.autocrlf=true will still check the file out with CRLF locally, so editing it works fine.
Does this seem correct? And would we rather force CRLF in the repo for this one file so that even users without core.autocrlf get a windows-esque formatted batch file? I guess our goal here is to remove the need for users to have specific format settings, but still allow flexibility if they are set.
There was a problem hiding this comment.
I updated the .gitattributes to force bat files to have CRLF endings. However, I am not able to revert this change by applying git add --renormalize . again because the file is already stored as a CRLF file.
❯ file gradlew.bat
gradlew.bat: DOS batch file text, ASCII text, with CRLF line terminators
So it seems the file is already in the correct CRLF format — please let me know if that aligns with what we wanted for Windows batch files.
There was a problem hiding this comment.
I had to drop the commit to revert the change I made to gradlew.bat, and my local git now always think that gradlew.bat is modified after I dropped the commit. I am not sure why.
| :mainEnd | ||
| if "%OS%"=="Windows_NT" endlocal | ||
| :omega |
There was a problem hiding this comment.
ah I see, thanks for clarifying. I’m wondering if we actually don't want this file normalized to LF in the repo, since it’s a Windows-only batch file.
Here’s how I understand * text=auto: when we stage/commit a file, Git normalizes its contents to LF before writing the blob to the object database. This is great for cross-platform consistency because history is always LF-based, but on Windows a user with core.autocrlf=true will still check the file out with CRLF locally, so editing it works fine.
Does this seem correct? And would we rather force CRLF in the repo for this one file so that even users without core.autocrlf get a windows-esque formatted batch file? I guess our goal here is to remove the need for users to have specific format settings, but still allow flexibility if they are set.
| @@ -0,0 +1,9 @@ | |||
| # Set the default behavior, in case people don't have core.autocrlf set. | |||
| * text=auto | |||
|
|
|||
There was a problem hiding this comment.
| gradlew.bat text eol=crlf |
Can test to see if this works if omitting this file seems correct
90e3071 to
d437d8b
Compare
| *.jpg binary | ||
| *.pdf binary | ||
|
|
||
| *.bat text eol=crlf |
There was a problem hiding this comment.
Ive tested this in action and it appears to be working correctly. To test you can do the following:
- run
git add --renormalize .
This will remove ^M from .bat file - run
git commit -m format: normalized .bat files for repo - switch to some other branch then switch back to the one for this PR with the
.gitattributesupdates - run
git show HEAD:gradlew.bat | cat -v
Checks the line endings of the commited.batfile, should see no ^M endings - run
cat -v gradlew.bat
Shows line ending of the working tree version ofgradlew.bat, should see ^M
So maybe we want .bat files commited to the repo with LF endings?Then the entire repo is LF normalized and the .gitattributes file handles working tree conversions depending on the local machine.
There was a problem hiding this comment.
I thought your original suggestion was to ignore .bat files because it was window-only. But yes, I don't see anything wrong with using LF normalization in the entire repo.
There was a problem hiding this comment.
Yes, I think my understanding of how the .gitattributes file worked was inverted RE: our conversation this morning. Its not that it converts a git blob to CRLF on commit, it does it on checkout and to the worktree only. So with this, your original push that convert the .bat to LF in the repo makes sense.
There was a problem hiding this comment.
I see, I applied the normalization again.
|
CI is repeatedly failing with: It seems a bit odd that this is related to this PR, but other PRs are passing. As there is also some issue with changes to gradlew.bat, can you start from a fresh branch and apply just your desired changes? Thanks again for the contribution! |
Hi, sorry for the delay. Yes, the build works for me locally. The changes to gradlew.bat was intended, so it was my desired change. From the discussion with @thisisalexandercook, normalizing the line endings of gradlew.bat was a good idea. I am not sure why the changes in this PR would cause the accept function to be called recursively without a termination. Does the |
|
Thanks for the update @YutongZhuu ! |
Sure, I will do that. |
|
@wmdietl Hi, could you give me the permission to run the workflow? I will apply commits one by one to see which one breaks the CI. Sorry for the delay. |
I don't see a simple way to give you just that permission. If there is some other PR you want to make, open that. Once you have a merged PR, CI should automatically run for you. |
I am not sure what you meant, I have to merge a PR to run the CI? |
|
@YutongZhuu you can also make CI run in your own fork by opening a PR and turning on GitHub actions. |
Oh, I didn't know that. Thank you for letting me know! |
thisisalexandercook
left a comment
There was a problem hiding this comment.
Thanks for reopening this! It is curious that CI passed when run again. My only comment is that it is best practice not to submit a PR using your forks master branch.
Thanks for the note! I will not do that again in the future. |
|
CI is failing with the stack overflow again: |
|
Some targets, mostly Other targets, e.g. the junit and nonjunit targets, pass, even though they also execute We need to figure out why adding the |
I think the problem comes from the changes in gradlew.bat. I normalized the line endings in that file. I will revert that change to see whether the CI passes or not. Maybe we should open another PR for the gradlew.bit changes since the initial intention of this PR was only adding a gitattribute file. Let's not worry about that here. |
|
@YutongZhuu CI is still failing with the same exception. It would have been weird if the reformatting of However, I don't have an idea why adding .gitattributes would cause this CI failure. |
On think the CI passes on this commit: Maybe there are some conflicts (I don't mean git conflicts) with the master branch since it fails after you merged the master into this branch? I am not sure. |
Sure, try opening a new PR again. If it contains the same changes, it hopefully is at least reproducible. I've also asked Copilot, let's see whether it suggests anything actionable in #1548. |
|
I'm trying the change suggested in #1548, but it seems very unlikely to me that that would fix this issue - the deep nesting of JDK files would exist in all PRs. |
|
@wmdietl maybe something interesting... In the failing runs, the logs show So here, since But why doesn’t this happen every time @wmdietl runs/re-runs CI on a PR? Because |
Thanks for tracking this down, @thisisalexandercook ! In the With the much larger stack size, the task now fails with: However, in the other tasks, I see: Could you file issues for:
@YutongZhuu Please make a PR from a branch. It was great to figure out this issue with our CI system. Let's try to document it better and avoid running into this issue in the future. Thanks! |
Opened, #1549. |

I believe this command:
git config --global core.autocrlf trueshould only be used on Windows. I got confused by this so I thought it would be a good idea to be more explicit.Thanks for reviewing.