suggestion: whitespace cleanup needed

[cbs228]

The present codebase (70d48dadf166) has a number of whitespace-related issues. These issues can make it more difficult to write clean and minimal PRs:

• Some files are committed with CRLF line endings or perhaps with a mix of LF and CRLF. Git normally wants to commit only LF and apply platform-native line endings on checkout. (See core.autocrlf)

• Some files have things that git considers to be "whitespace errors"

Whitespace errors are a problem because:

1. Very simple-minded text editors tend to leave them all over the place
2. They make it difficult to review diffs and format-patches. Since you can't always see the trailing whitespace, it is not clear that a patch introduces it.
3. In some languages, unintended trailing space can lead to bugs. Bash will escape a newline with \, but not if it is followed by whitespace!
4. They cause merge conflicts for no good reason

There are many ways to strip whitespace. Here is one of the faster bash-y ways I can think of with sed and regular expressions:

# run in the repo root
find . \
  \( \
    -iname '*.[ch]' -or \
    -iname '*.html' -or \
    -iname '*.js' -or \
    -iname '*.md' -or \
    -iname '*.txt' \
  \) \
  -exec sed -i -r -e 's/ +\t/\t/' -e 's/\s+\r?$//' -- \{\} +;

(EDIT: a more complicated find is required to grab all the files; the globs don't match everything.)

This icky-looking mess finds all source and header files and

1. (first -e) trims space before tabs (a whitespace error)
2. (second -e) strips trailing whitespace and converts line endings to LF

You can verify that this command only changes spaces with

git diff --ignore-all-space

which should return empty since only spaces are changed. You can additionally check

git diff --check

which should return empty to indicate that there are no whitespace errors.

Since this touches a lot of the code, it is probably best to wait until there are no pending PRs or other changes awaiting review or merge.

Most editors and IDEs have an option to automatically strip trailing whitespace. Enable it to prevent it from introducing these in the future. In my projects, I usually use a Github Action to check PRs for whitespace errors for me.

Whitespace errors may sound like a small thing, but they can make it much harder for new contributors to submit good PRs.

[pflarue]

It appears that most everything in this repository has DOS style CRLF line endings. However, no tool that I've used on Linux or Windows has complained about this. They all seem to detect and continue to use what they find. So, I'll agree that this is unusual, but convince me that it is a problem. If consistency is the only goal, then changing the few cases of Unix style line endings to CRLF would be less disruptive.

As for the other whitespace errors/inconsistencies, I noticed these but didn't think that making what I considered purely cosmetic changes to fix them was worthwhile, and therefore I've also not paid careful attention to this with my own commits. You've given me reasons to consider this further.

[cbs228]

So, I'll agree that this is unusual, but convince me that it is a problem

There is a difference between the committed line endings and the line endings in the working copy. On a Linux host, I see:

$ git ls-files --eol
i/crlf  w/crlf  attr/                   ARDOPC/HostInterface.c
i/crlf  w/crlf  attr/                   ARDOPC/KISSModule.c
i/lf    w/lf    attr/                   ARDOPC/LCM1602-IIC.c
i/lf    w/lf    attr/                   ARDOPC/LCM1602-IIC.h
i/lf    w/lf    attr/                   ARDOPC/Main.c
⋮
i/crlf  w/crlf  attr/                   ARDOPC/webgui/webgui.html
i/crlf  w/crlf  attr/                   ARDOPC/webgui/webgui.js
i/lf    w/lf    attr/                   ARDOPC/webgui_html.c
i/lf    w/lf    attr/                   ARDOPC/webgui_js.c
i/lf    w/lf    attr/                   ARDOPC/xrs.c

The default behavior on all platforms is to commit LF for all new text files. On Windows, the default behavior is to use CRLF in the working copy but still commit LF. Even if we converted every file to CRLF line endings and committed that, all new text files on most Git configs will still commit LF. Even on Windows.

In addition,

• My git diff on Linux also tends to show ^M for every CR, which is cluttered
• "Everyone else" uses LF commits. If we vendor or git subtree sources from another project, they will almost certainly have LF.

In the present ardop repo, there is no consistency between line endings—even in related files. This is not really a problem per se, but it does bother me in ways that are difficult to put into words. ¯\(ツ)/¯

In addition to the above whitespace fixes, it is probably a good idea to adopt a .gitattributes file:

* text=auto
*.md text
*.txt text

Makefile text=auto
*.c text=auto
*.css text=auto
*.h text=auto
*.html text=auto
*.js text=auto

*.bash          text eol=lf
*.sh            text eol=lf

*.doc binary
*.pdf binary

*.vcproj        text eol=crlf
*.{cmd,[cC][mM][dD]} text eol=crlf
*.{bat,[bB][aA][tT]} text eol=crlf

and then

git add --renormalize .

(Long-winded explanation.) This will force LF commits on all platforms. The working copy line endings remain per the user's preference unless forced with eol=lf or eol=crlf.

One place where .gitattributes is super important is for WSL and other Linux VMs on Windows. If you check out a .sh shell script on Windows, then run it over a shared folder on Linux, it should still work. (Yes, people do this. Then they complain to me about it.) The eol= forcing ensures the file has the right line endings.

Is all of this absolutely necessary? No. But it makes our repo look neater and more like bigger, well-maintained projects. It is much less disruptive to do it now than when we—hopefully—gain more PR activity in the future.

[pflarue]

Pull Request #63 merged into the develop branch today has made the changes requested. This will be merged into the master branch after time has been allowed to search for and fix bugs that may have been introduced by this and the other branches merged into develop today.

@cbs228, Please review the condition of the develop branch following this merge. If you see any additional changes that you think should have been made, please submit an additional pull request so that we can finish getting things cleaned up before starting work on additional changes (including response to some of the other outstanding Issues).

[cbs228]

PR #53 LGTM! The only remaining whitespace error my find didn't find was in the LICENSE file.

All developers are now strongly encouraged to avoid whitespace "errors" in their pull requests

Those instructions should probably go in a CONTRIBUTING.md file eventually. Otherwise nobody will see them. Github has some recommendations for that. But that is another issue for another time.

[pflarue]

I've added contributing.md to my to-do list as part of a plan to work on documentation in general.