Anki-Android icon indicating copy to clipboard operation
Anki-Android copied to clipboard

File terminators not enforced as LF, CRLF can sneak in but should be disallowed

Open mikehardy opened this issue 1 year ago • 6 comments

Checked for duplicates?

  • [X] This issue is not a duplicate

Does it also happen in the desktop version?

  • [X] This bug does not occur in the latest version of Anki

What are the steps to reproduce this bug?

Search code tree for CRLF terminators, or attempt to commit a file that has them:

mike@isabela:~/work/ankidroid/Anki-Android/AnkiDroid/src (main) % for FILE in `find . |grep '\.kt'`; do file $FILE |grep CRLF; done
./androidTest/java/com/ichi2/anki/ReviewerTest.kt: ASCII text, with CRLF line terminators
./test/java/com/ichi2/anki/previewer/PreviewerViewModelTest.kt: ASCII text, with CRLF line terminators
./test/java/com/ichi2/anki/ConstantUniquenessTest.kt: ASCII text, with CRLF line terminators
./main/java/com/ichi2/anki/preferences/AccessibilitySettingsFragment.kt: ASCII text, with CRLF line terminators
./main/java/com/ichi2/anki/pages/ImageOcclusion.kt: ASCII text, with CRLF line terminators

Expected behaviour

There should be no CRLF terminators in the files in the project, additionally, if you attempt to commit any our ktlint rules should fix them and worst case if you commit them anyway it should fail CI via a lint-rules check

Additionally I believe it is possible for git to be configured such that it attempts to fix these as well ?

Actual behaviour

CRLF will happily just go right into the tree

Debug info

doesn't matter as it is infrastructure in the project (ktlint, lint-rules, git settings, etc)

but it's happening as of this typing which is commit hash 74feec0a3dd157d9c3a332f6a3819d2a9154effc which is current main as of June 3rd 2024 @ 11:34a UTC-5

(Optional) Anything else you want to share?

No response

Research

  • [X] I am reporting a bug specific to AnkiDroid (Android app)
  • [X] I have checked the manual and the FAQ and could not find a solution to my issue
  • [X] (Optional) I have confirmed the issue is not resolved in the latest alpha release (instructions)

mikehardy avatar Jun 03 '24 16:06 mikehardy

The desired solution for this should take the shape of:

  • investigate git settings to see if we can put something in .gitattributes related to this as a help
  • investigate ktlint settings to see if there is a setting that will detect CRLFs and disallow them or auto-fix them as part of our commit hook that runs ktlintformat (appears there is an --ignore-whitespace switch in the commit hook so ... unknown what exactly to do there without investigate)
  • making sure that CI either runs ktlint in a way that enforces no CRLFs as terminators or add a lint-rule that checks it and fails on current code
  • a commit that does a dos2unix on the files that have CRLF terminators
  • squash merge the result and add that commit on main to the ignore-refs (not entirely sure how to do this, @david-allison knows?)

Note that CRLF terminators may be okay in some file types - this is unknown. But in our main source types (.ts, .gradle, .kt, .xml files at least) I don't think they should be there

mikehardy avatar Jun 03 '24 16:06 mikehardy

I'll be working on it.

Aditya13s avatar Jun 03 '24 16:06 Aditya13s

squash merge the result and add that commit on main to the ignore-refs (not entirely sure how to do this, @david-allison knows?)

Aim for 1 commit in the PR, but multiple are acceptable

After the commits are in main (and therefore the IDs are stable), edit https://github.com/ankidroid/Anki-Android/blob/a4ac1605ec11cb2272ce7ed5ad4ebf61776ddd6b/.git-blame-ignore-revs and add the additional commits in a follow-up commit

david-allison avatar Jun 03 '24 17:06 david-allison

cool @Aditya13s thanks - doesn't have a whole lot to do with coding specifically but issues like this that are codebase-wide standards conformance issues and how to really truly actually provably fix them and make sure they stay fixed are one of those things that becomes important on big projects. So, hopefully useful skills for rest of professional life if you haven't already developed them. And if you already have then should be quick :-)

mikehardy avatar Jun 03 '24 17:06 mikehardy

@mikehardy I added *.kt text eol=lf in .gitattributes which changes crlf to lf, which files are commiting. I never worked with ktlint. Can you help me to start with it.

Aditya13s avatar Jun 20 '24 06:06 Aditya13s

Sorry I don't have time to help, and I'm not that familiar with it myself

mikehardy avatar Jun 20 '24 11:06 mikehardy