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

Prepare PR for TQ #10

Merged
merged 7 commits into from
Nov 27, 2023
Merged

Prepare PR for TQ #10

merged 7 commits into from
Nov 27, 2023

Conversation

rmfranken
Copy link
Member

Deleted sync-fork action
Fixed \r being an unrecognized character by bash, causing it to not find the entrypoint.sh file. Don't fully understand how this was not a problem before, but it doesn't work without this line in the dockerfile:
RUN sed -i 's/\r//g' entrypoint.sh

@rmfranken rmfranken assigned rmfranken and cmdoret and unassigned rmfranken Nov 21, 2023
@rmfranken
Copy link
Member Author

Ok, I have it setup so that it makes sure the end of line feeds are always lf if the file is a .sh file. At first I thought to just do it for all files, but maybe that's a little bit over-reachy, as the problem only shows itsself in shellscript for me. What do you think? Do we risk breaking anything by applying it to all files in a repo, or is the "add it to the gitattributes if it causes problems" a better practice? (more fine grained, but also more debugging effort potentially)

@rmfranken rmfranken requested a review from cmdoret November 22, 2023 09:55
@cmdoret
Copy link
Member

cmdoret commented Nov 23, 2023

This is looks good, but just to be careful and explicit perhaps we could do:

# By default, normalize line endings
* text=auto

# Force Windows batch files to use CRLF
*.{cmd,[cC][mM][dD]} text eol=crlf
*.{bat,[bB][aA][tT]} text eol=crlf
*.{ics,[iI][cC][sS]} text eol=crlf

# Force Unix bash files to use LF
*.sh text eol=lf

IIUC the docs, this means:

  • Let git infer which files are text and which are binary. Implicitely, those that are text will always be converted to lf (\n) by git.
  • Convert .sh files to lf (I think this could be omitted)
  • Convert .bat files to crlf (`\r\n') as they are only executed on windows

@rmfranken
Copy link
Member Author

rmfranken commented Nov 23, 2023

The .bat is a good point! And using auto might actually not require the explicit mentioning of bat since:

When text is set to "auto", Git decides by itself whether the file is text or binary. If it is text and the file was not already in Git with CRLF endings

The way I interpret that sentence is: If you upload a .bat with CRLF endings the first time its uploaded, it will remain as such. If you are only editing a file on windows (causing it to be "re-uploaded" with CRLF), it will result to it being converted to back to LF when git-pushing back.

That being said, we can do both - also for documentation sake to make it clear we expect LF endings on non-bat files, and CRLF on bat.

Much safer way of dealing with line-endings and explictly documents some common microsoft/windows specific files where crlf is expected.
Copy link
Member

@cmdoret cmdoret left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and works on my end 🙌

@rmfranken rmfranken merged commit e4e6fd1 into master Nov 27, 2023
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.

2 participants