Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename disable-dnscache-service-win to bat and minor tweaks #1912

Merged
merged 2 commits into from Mar 7, 2022
Merged

Rename disable-dnscache-service-win to bat and minor tweaks #1912

merged 2 commits into from Mar 7, 2022

Conversation

XhmikosR
Copy link
Contributor

@XhmikosR XhmikosR commented Mar 6, 2022

Now it's consistent with the other bat file and .editorconfig/.gitattributes apply to this file too.

@XhmikosR XhmikosR changed the title Rename disable-dnscache-service-win to bat Rename disable-dnscache-service-win to bat and minor tweaks Mar 6, 2022
Copy link
Contributor

@funilrys funilrys left a comment

This may be a breaking change for the people who automatically download the "latest" cmd file - without git.

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 6, 2022

@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 7, 2022

To see why I have already explicitly set up CRLF in our .gitattributes/.editorconfig: https://serverfault.com/questions/429594/is-it-safe-to-write-batch-files-with-unix-line-endings

So, we could add .cmd too but IMHO this is simpler and more consistent.

@StevenBlack
Copy link
Owner

StevenBlack commented Mar 7, 2022

I'm going to merge this now. Nissar's @funilrys comment is a good one, I think the remediation path for that appears straightforward, I hope that's true 🙃

@StevenBlack StevenBlack merged commit b04eb1e into StevenBlack:master Mar 7, 2022
19 of 20 checks passed
@XhmikosR
Copy link
Contributor Author

XhmikosR commented Mar 7, 2022

The thing is that Windows cmd can fail with LF (with cryptic error messages) and this was the fix with the less changes.

@XhmikosR XhmikosR deleted the bat branch Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants