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

Fix ensure_single_trailing_newline() on macs. #16835

Merged
merged 1 commit into from Apr 7, 2024

Conversation

drwells
Copy link
Member

@drwells drwells commented Apr 2, 2024

I'm not sure exactly why mac's version of sed produces different output here but it is easy enough to use something simpler. Just check if the last character is a newline: if not, then append a newline character.

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

Nice simplification, but I feel like I dont understand why it has to be so complicated in the first place. Would my suggestion make it even simpler?

This would avoid using sed overall, which like you say behaves subtly different on Mac and Linux.

contrib/utilities/indent_common.sh Outdated Show resolved Hide resolved
contrib/utilities/indent_common.sh Outdated Show resolved Hide resolved
@drwells
Copy link
Member Author

drwells commented Apr 3, 2024

/rebuild

Copy link
Member

@gassmoeller gassmoeller left a comment

Choose a reason for hiding this comment

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

👍

I'm not sure exactly why mac's version of sed produces different output here but
it is easy enough to use something simpler. Just check if the last character is
a newline: if not, then append a newline character.
@drwells
Copy link
Member Author

drwells commented Apr 3, 2024

Actually - looking at this again, this change fixes the situation on macs where a file does not not end in \n but it doesn't prevent a case where a file ends in \n\n\n\n. I'll look into this later this week.

@gassmoeller
Copy link
Member

Oh, I see, I didnt realize that was one of the purposes of this function as well. Then you probably have to keep the first sed command of the original (I think it came from here: https://stackoverflow.com/questions/7359527/removing-trailing-starting-newlines-with-sed-awk-tr-and-friends). Or was that the one that didnt work on Mac?

@drwells
Copy link
Member Author

drwells commented Apr 3, 2024

The case which didn't work on a mac was enforcing that files end in a newline (i.e., one instead of zero newlines) - this patch fixes that part.

@gassmoeller
Copy link
Member

I think according to this answer the following should be pretty compatible and easy to understand:

# remove all trailing newlines and add exactly one newline instead
printf '%s\n' "$(cat "$file")" > "$file"

The command substitution into printf removes all potentially existing newlines at the end of the file, and printf adds exactly one newline because of the format given. Also works for file names with special characters and spaces. Couldnt test it on a Mac though.

@tamiko tamiko merged commit abd8f55 into dealii:master Apr 7, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants