replacer: {file.*} global placeholder strips trailing newline

[steffenbusch]

Fixes #6392

[francislavoie]

What the heck, test failed on Windows for some reason 🤔

    replacer_test.go:445: Expected value 'foo' for key 'file.caddytest/integration/testdata/foo_with_trailing_newline.txt' got 'foo
'
    replacer_test.go:445: Expected value 'foo
        ' for key 'file.caddytest/integration/testdata/foo_with_multiple_trailing_newlines.txt' got 'foo
        
'
--- FAIL: TestReplacerNew (0.01s)

[francislavoie]

Oh, Windows might have \r\n instead. That's annoying.

[mholt]

Why do we call HasSuffix? TrimSuffix is only effective if the input has that suffix.

[steffenbusch]

This way, we make sure to remove only the very last newline due to HasSuffix in combination with return strings.TrimSuffix.

For example, let's have this content in a file:

hello world\r\n
\r\n
\r\n

What we want:

hello world\r\n
\r\n

And this code without HasSuffix :

// Remove a single trailing newline, regardless of its type
content = strings.TrimSuffix(content, "\r\n")
content = strings.TrimSuffix(content, "\n")
return strings.TrimSuffix(content, "\r"), true

content after 1. TrimSuffix:

hello world\r\n
\r\n

content after 2. TrimSuffix:

hello world\r\n
\r

return after 3. TrimSuffix:

hello world\r\n

[kanashimia]

Can't you just do

content = strings.TrimSuffix(content, "\n")
content = strings.TrimSuffix(content, "\r")
return content

?

Also technically for windows the convention is different from unix platforms, so removing the newline there is kinda redundant, but like all good software developers we can just ignore that fact.

[steffenbusch]

Also, technically, the convention for Windows differs from Unix platforms.

That's a good point! The consideration of different EOL styles was only prompted by test failures on the Windows platform.

@mholt and @francislavoie, would it be acceptable if I add these two lines to .gitignore?

caddytest/integration/testdata/foo_with_trailing_newline.txt eol=lf
caddytest/integration/testdata/foo_with_multiple_trailing_newlines.txt eol=lf

This would allow us to simplify the code and test cases back to the original handling of just \n (400b08d).

When these two test files are checked out on the Windows platform for the Windows test cases, the files should have \n instead of \r\n and the original simple tests should pass successfully.

[mohammed90]

When these two test files are checked out on the Windows platform for the Windows test cases

But the test cases are to reflect users' files as-is on Windows, which may have \r\n. I think configuring that in gitignore is just cheating the test, no?

[steffenbusch]

But on windows, unlike on Linux due to the unix convention (see also https://superuser.com/questions/745111/why-is-vim-adding-a-newline-is-this-a-convention/745135#745135, you don't tpyically have an trailing newline for a file.

[mholt]

I think the concern is that if you author the files on your local Windows machine, then you upload to a Linux server, the Windows-style line endings are relevant because they don't just get converted automatically.

[kanashimia]

If you author the code in windows machine using a software that follows windows conventions and don't just add a training newline yourself then you will get a file without \r\n at the end.

The real concern is when that is not the case, for example someone shares you a file with unix conventions, and it gets converted to windows ones because your editor is bad, or you just do something stupid with the tooling for that to happen.

Anyway I think it is correct to remove line ending there because: 1. current error for mismatched acme key is not that much helpful 2. if you use windows you should expect such bullshit to happen, just cope with it 3. usecase of having working secrets outweighs the usecase of displaying a file with a single empty line at the end. 4. it requires less thinking

[mohammed90]

I concede to kanashimia's point 👍🏼

[mholt]

Sounds good to me now. To clarify, is the consensus that this is ready to merge?

[steffenbusch]

Sounds good to me now. To clarify, is the consensus that this is ready to merge?

@francislavoie & @mohammed90 could you please provide feedback to @mholt on whether there is a consensus that this Pull Request is ready to merge? Thanks!

[mohammed90]

Yes, this looks good 👍🏼