feat(windows): improve errors with CR character

[lucascolley]

brush version

brush 0.4.0 (cargo:0.4.0)

bash version (for comparison)

GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin)

Platform / OS

Windows

Steps to reproduce

❯ bat foo.sh
─────┬──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
     │ File: foo.sh
─────┼──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────
   1 │ echo "hi"
   2 │
   3 │ echo "world"
─────┴──────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────

Actual behavior (brush)

❯ brush foo.sh
hi
error: command not found:
world

Expected behavior

❯ bash foo.sh
hi
world

Additional context

No response

[reubeno]

@lucascolley Thanks for reporting this!

Can you confirm whether your script file being used as input has DOS-style line endings (CRLF) or UNIX-style ones (LF)?

[lucascolley]

hey @reubeno, the error is observed with CRLF, but it works as intended with LF

[avih]

First of all, the brush behavior is identical to bash on linux, with the slight difference that bash prints the not-found command in a more readable way - as test.sh: line 2: $'\r': command not found.

GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin)

Are you absolutely sure you're testing this version? because this seems to be cygwin bash, which, for me, also behaves exactly line bash on linux (and brush on windows), printing exactly the same error. Specificallly this version, which I believe is the latest in cygwin at this time:

GNU bash, version 5.2.21(1)-release (x86_64-pc-cygwin)

However, msys2 bash, which prints this with bash --version (it's slightly outdated):

GNU bash, version 5.1.16(1)-release (x86_64-pc-msys)

Does behave like the "expected behavior", and does not error.

This is because msys2 has various hacks which try to deal better with windows, and which the more pure cygwin does not have. Many times these hacks are double-edged sword.

One of those hacks is indeed trying to deal with CRLF line ending in bash, and sometimes also elsewhere - but not universally.

For instance, while msys2 bash can handle this specific case, some other shells in msys2 - like dash - error just like brush..

This is not a simple bug where "empty lines in shell scripts with CRLF lines print an error".

The core issue is that this line is not empty in the normal unix way, because it has a CR char, followed by newline, which, for bash, is the command CR - which doesn't exist.

If one wants to "fix" this case, then there are many others which need consideration, for instance:

• Should the builtin echo terminate lines with LF or CRLF?
• Should the builtin printf with format string of "\n" print LF or CRLF?
• Should command substitution like x=$(cat foo.sh) strip all trailing CRLF pairs? should the remaining lines have CRLF endings at the value of var $x? or should they be converted to LF?
• Etc, etc, etc.

Whether EOL is LF or CRLF has many more implications that just this test case, and it needs a holistic approach which covers the various affected cases consistently, and officially (as in - documented, so that users can know what to expect in each case).

The vast majority of editors on windows can choose between LF and CRLF line ending, and it's trivial to ensure that all shell scripts have normal unix LF-only EOL.

Adding various hacks to make brush "CRLF compatible" can be a real pandora's box, with endless list of edge cases to take care of, and which end up behaving inconsistently, or making some things harder or less concise or impossible to do.

One of the values of posix shell, compared to cmd.exe, is that it's concise. Having it accept both LF and CRLF makes it not-concise, because then it becomes a lot harder to control output line ending or process inputs concisely.

For instance, it's trivial to write utility shell scripts dos2unix and unix2dos, but good luck writing such scripts if the shell accepts both LF and CRLF endings interchangably.

[lucascolley]

Thanks for the detailed response!

GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin)

Are you absolutely sure you're testing this version?

Yeah, I just double-checked that I see GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin). That's coming from https://prefix.dev/channels/conda-forge/packages/git?platform=win-64, which packages https://github.com/git-for-windows/git.

---

It sounds like CRLF compatibility may not be a goal of the project, then, at least in the short-to-medium term. Do you have any advice for avoiding CRLF code in codebases I maintain, then? The main reason I'm looking to adopt brush is for windows compatibility, but if I can't reliably avoid CRLF compatibility problems then that might be challenging.

[avih]

Yeah, I just double-checked that I see GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin).

Right. I just checked latest git-for-windows, and indeed its bash identifies with x86_64-pc-cygwin, although as far as I know it's actually msys2 bash - and not cygwin, and has always been msys2 and not cygwin (msys2 is a downstream fork of cygwin).

As mentioned, the latest bash currently in cygwin (proper - not msys2) behaves like brush and like bash on linux.

Anyway, the ability to sort-of handle CRLF is specifically an msys2 bash thing, among other hacks which msys2 has where it tries to make it easier for windows users, and, as mentioned, these hacks can be double-edged swords.

It sounds like CRLF compatibility may not be a goal of the project

I'm not a contributor to brush and I can't speak for the maintainers. I'm just a user, and merely playing with it occasionally. It's not my daily driver.

But since I'm very familiar with both (posix) shell, and issues it might have on windows, I was pointing out that this example script error is just a symptom of a much bigger subject of how to handle CRLF, and handling it correctly while keeping the conciseness - which posix sh otherwise has - is not a simple task, and requires a holistic approach with a much bigger scope than just this script error.

Do you have any advice for avoiding CRLF code in codebases I maintain, then? The main reason I'm looking to adopt brush is for windows compatibility, but if I can't reliably avoid CRLF compatibility problems then that might be challenging.

I might be able to help, but I don't know the details or the scope of your issues. In my global git config I force LF line ending (not automatic), so whenever I clone something it always has LF ending, and all my editors preserve it, so I never have to deal with CRLF (if I do, the there's always dos2unix, and all editors do know to convert between LF and CRLF if needed, though I rarely need it).

It might become off topic, buf if you listed your exact issues more specifically, maybe someone might be able to help.

[lucascolley]

Yeah, I just double-checked that I see GNU bash, version 5.3.15(1)-release (x86_64-pc-cygwin).

Right. I just checked latest git-for-windows, and indeed its bash identifies with x86_64-pc-cygwin, although as far as I know it's actually msys2 bash - and not cygwin, and has always been msys2 and not cygwin (msys2 is a downstream fork of cygwin).

how confusing! Nice to know...

Do you have any advice for avoiding CRLF code in codebases I maintain, then? The main reason I'm looking to adopt brush is for windows compatibility, but if I can't reliably avoid CRLF compatibility problems then that might be challenging.

I might be able to help, but I don't know the details or the scope of your issues. In my global git config I force LF line ending (not automatic), so whenever I clone something it always has LF ending, and all my editors preserve it, so I never have to deal with CRLF (if I do, the there's always dos2unix, and all editors do know to convert between LF and CRLF if needed, though I rarely need it).

It might become off topic, buf if you listed your exact issues more specifically, maybe someone might be able to help.

Thanks. I suppose the gist of the issue is that I work on large codebases like https://github.com/scipy/scipy which have not historically enforced LF. I guess one way to do it would be to do a blanket audit once with dos2unix, then start enforcing LF with a linter. I suppose I'm interested to know whether that linting can be (partially) enforced at the git repo level or whether it is best done by a stand-alone external script.

(I realise this is probably the kind of question an LLM can do a good job of answering, but since you are here...)

[avih]

Thanks. I suppose the gist of the issue is that I work on large codebases like https://github.com/scipy/scipy which have not historically enforced LF.

Well, as far as I can tell, this is not windows-specific project, meaning that whatever shell scripts it includes - are also intended to run on linux/bsd/etc.

Which means that of those scripts, if they happen to have CRLF line ending, then they would just be broken outside of windows. Making them work in bash on windows would not fix CRLF issues with these files elsewhere.

Is that correct so far?

So assuming there's no issues with shell scripts in this repo on unix platforms, this means that at least the shell scripts don't have "hard" CRLF line ending where it matters.

So I think, but I'm not an expert on that, that indeed either the user should configure their git with LF ending, or somehow enforce that at the project level centrally (as a git config and/or normalize all the files, e.g. using dos2unix). But it's better to ask it at the git-for-windows repo. I'm pretty sure you're not the first person or project which has to deal with that.

[lucascolley]

So assuming there's no issues with shell scripts in this repo on unix platforms, this means that at least the shell scripts don't have "hard" CRLF line ending where it matters.

So I think, but I'm not an expert on that, that indeed either the user should configure their git with LF ending, or somehow enforce that at the project level centrally. But it's better to ask it at the git-for-windows repo. I'm pretty sure you're not the first person or project which has to deal with that.

Hmm yeah, this is starting to sound like a user issue on my side on my windows box. Thanks for your time, I think I'll be able to figure it out from there.

---

Now back to brush: I think it would be awesome if instead of the opaque error error: command not found:, brush communicated to the user that it has hit the CR character, and hinted towards a fix (save the file with LF and retry).

[avih]

Now back to brush: I think it would be awesome if instead of the opaque error error: command not found:, brush communicated to the user that it has hit the CR character, and hinted towards a fix (save the file with LF and retry).

Maybe, but again, the scope of this is bigger that what you suggest.

For instance, this CR which brush (and bash on linux or cygwin) considers part of the line (i.e. before the LF) actually exists at all those lines - not just the empty lines.

But because the commands in this script are just echo, then it doesn't care. echo just adds these CR to the output. If you pipe the output of this script into xxd or some hex viewer, you'd see that the lines have CRLF - that CR comes from the script source, not because echo ends lines with CRLF. You could compare the output of the same script which has unix LF endings, and you'll see that the HEX output is different.

But sometimes the command does care about CR at the end of the line. For instance:

# ensure this file has CRLF line ending
sed s/x/y/g myfile.txt

this won't work, because instead of the file myfile.txt, it will add CR to the file name - because that's what immediately follows the name - and before the line ends at LF.

So it's not just an error on empty lines. It affects scripts everywhere, but sometimes it's not very visible, like if the command happens to be echo, and it just appends an invisible CR to the output.

As I said, handling CRLF consistently requires a more holistic, deterministic, and documented approach. It's not making sure that just one case becomes a bit better.

[lucascolley]

So it's not just an error on empty lines. It affects scripts everywhere, but sometimes it's not very visible, like if the command happens to be echo, and it just appends an invisible CR to the output.

As I said, handling CRLF consistently requires a more holistic, deterministic, and documented approach. It's not making sure that just one case becomes a bit better.

Makes sense, yeah, happy with whatever the maintainers deem appropriate. I think that handling the empty-line case would still be worth it, as it would probably be relatively common to hit. But easy to talk when you are not the one who has to maintain it 🙃

[avih]

I think that handling the empty-line case would still be worth it, as it would probably be relatively common to hit

I think a better and more generic approach would be to do what bash does with error messages: replace non-printable chars with more visible representation. E.g. with this CR the error looks like:

test.sh: line 2: $'\r': command not found

which is the dollar-quoting form which was added in POSIX.2024 (but existed in bash prior to that).

So at least the user knows that there's some CR there which bash errored on.

[lucascolley]

that would also be nice!