FSFE/fsfe-website
27
33
Fork 93
Code Issues 64 Pull Requests 19 Wiki Activity

Migrate pre-commit.entrypoint.sh to POSIX sh #5540

Merged
fkobi merged 5 commits from posix-entrypoint into master 2025-12-11 09:07:03 +00:00
Conversation 2 Commits 5 Files Changed 2 +4 -4
fkobi commented 2025-12-04 08:33:41 +00:00
Contributor
Copy Link

The script was basically posix with a bashism.
POSIX sh is lighter and thus better.

The script was basically posix with a bashism. POSIX sh is lighter and thus better.
fkobi added the build label 2025-12-04 08:33:41 +00:00
delliott was assigned by fkobi 2025-12-04 08:33:41 +00:00
tobiasd requested review from delliott 2025-12-04 08:58:16 +00:00
delliott requested changes 2025-12-04 09:08:28 +00:00
Dismissed
pre-commit.entrypoint.sh Outdated
@@ -1,5 +1,4 @@
#!/usr/bin/env bash
set -euo pipefail
delliott commented 2025-12-04 09:08:15 +00:00
Member
Copy Link

Any particular reason to remove -euo pipefail?

-eu at least is supported by posix: https://pubs.opengroup.org/onlinepubs/007904875/utilities/set.html

In general one wants as much strictness and as little undefined behaviour as possible for shell scripts, which is why pretty much all well made shell scripts have -euo pipefail set.

Any particular reason to remove `-euo pipefail`? `-eu` at least is supported by posix: https://pubs.opengroup.org/onlinepubs/007904875/utilities/set.html In general one wants as much strictness and as little undefined behaviour as possible for shell scripts, which is why pretty much all well made shell scripts have `-euo pipefail` set.
fkobi commented 2025-12-04 09:58:22 +00:00
Author
Contributor
Copy Link

I have explained all the changes in the commit messages but TLDR is that they were ineffective.
If you look below I've kept the -e as it was the only relevant option.

If one declares "this might go wrong" a person new to the codebase will look for "this" as a real possibility.
Thus removing it improves maintainability.

I have explained all the changes in the commit messages but TLDR is that they were ineffective. If you look below I've kept the `-e` as it was the only relevant option. If one declares "this might go wrong" a person new to the codebase will look for "this" as a real possibility. Thus removing it improves maintainability.
delliott commented 2025-12-10 10:54:31 +00:00
Member
Copy Link

I am afraid I am going to have to disagree a little on the idea that the options are pointless.

Firstly, it is a broadly agreed shell scripting best practice to always enable such checks, and the argument that the checks are unlikely to catch errors at present is a sign that the checks have worked.

To be clear, you are absolutely correct that at present there is no real way at present for undefined or pipefail to be triggered.

This is partly because during development when I made errors that triggered them, I rewrote the code to not trigger them, or allow for doing so.

Additionally, just because there are no hazards at present does not mean the script will not grow in future, and hazards detectable by the sets be introduced. It is unlikely someone will be careful enough to consider that having added pipes they should now add pipefail, and with its absence we now have undefined behavior states. Much easier to just add it now, and its not like it has any performance cost. Another example: without undefined a later careless commit could easily remove the initial definition of files_arg. The code would still work perfectly, but would be logically flawed and more open to later issues.

Also, when I see a script with the options set I think to myself, "Great, I do not have to look for those types of errors as they are caught automatically". Whereas without them I have to be on guard for all kinds of undefined behavior myself.

Setting the options has no meaningful runtime cost or build time cost (save an extra ~10 bytes in the docker image ;D) and is both conventional and sensible defensive programming.

I am afraid I am going to have to disagree a little on the idea that the options are pointless. Firstly, it is a broadly agreed shell scripting best practice to always enable such checks, and the argument that the checks are unlikely to catch errors at present is a sign that the checks have worked. To be clear, you are absolutely correct that at present there is no real way at present for `undefined` or `pipefail` to be triggered. This is partly because during development when I made errors that triggered them, I rewrote the code to not trigger them, or allow for doing so. Additionally, just because there are no hazards at present does not mean the script will not grow in future, and hazards detectable by the sets be introduced. It is unlikely someone will be careful enough to consider that having added pipes they should now add `pipefail`, and with its absence we now have undefined behavior states. Much easier to just add it now, and its not like it has any performance cost. Another example: without `undefined` a later careless commit could easily remove the initial definition of `files_arg`. The code would still work perfectly, but would be logically flawed and more open to later issues. Also, when I see a script with the options set I think to myself, "Great, I do not have to look for those types of errors as they are caught automatically". Whereas without them I have to be on guard for all kinds of undefined behavior myself. Setting the options has no meaningful runtime cost or build time cost (save an extra ~10 bytes in the docker image ;D) and is both conventional and sensible defensive programming.
fkobi commented 2025-12-11 08:59:00 +00:00
Author
Contributor
Copy Link

on the idea that the options are pointless

I do not think that so I did not communicate that thought anywhere.

I understand your position on it and see your points so I have reverted the change.

> on the idea that the options are pointless I do not think that so I did not communicate that thought anywhere. --- I understand your position on it and see your points so I have reverted the change.
👍 1
fkobi marked this conversation as resolved
fkobi force-pushed posix-entrypoint from 7358e5e7c2 to d21e637e6e 2025-12-04 09:22:03 +00:00 Compare
tobiasd changed title from Migrate pre-commit.entrypoint.sh to POSIX sh to WIP: Migrate pre-commit.entrypoint.sh to POSIX sh 2025-12-04 09:48:09 +00:00
fkobi requested review from delliott 2025-12-10 10:13:21 +00:00
delliott approved these changes 2025-12-11 08:58:22 +00:00
delliott left a comment
Member
Copy Link

Looks good, thanks!

Looks good, thanks!
fkobi force-pushed posix-entrypoint from 57bde3783d to 12eb772f15 2025-12-11 08:59:47 +00:00 Compare
fkobi changed title from WIP: Migrate pre-commit.entrypoint.sh to POSIX sh to Migrate pre-commit.entrypoint.sh to POSIX sh 2025-12-11 09:00:04 +00:00
fkobi force-pushed posix-entrypoint from 12eb772f15 to 7e246bcdbc 2025-12-11 09:01:30 +00:00 Compare
fkobi force-pushed posix-entrypoint from 7e246bcdbc to bd6a2eda19 2025-12-11 09:04:20 +00:00 Compare
fkobi merged commit 09b117d306 into master 2025-12-11 09:07:03 +00:00
fkobi deleted branch posix-entrypoint 2025-12-11 09:07:04 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
delliott
Labels
Clear labels
bug
build
cgi Scripting
design
disruptive
Requires careful coordination and documentation changes
 documentation
duplicate
easy
enhancement
Improving what is already there
 feature-request
help wanted
javascript
priority/low
question
system-hackers
tagging
text
translations
wait/bugfix
wait/inprogress
wait/misc
wait/proofread
wontfix
xsl
No Label build
Milestone
No items
No Milestone
Assignees
Clear assignees
albert alex.busch alex.sander anaghz ao bcludts bonnie cryptie dario delliott dfajfer dmaphy doczkal eal egnun eventregbot fi floriansnow gabriel.ku guido hugo ineiev jn jzarl linus lucabon lucas.lasota max.mehl mk monochromec mweimann nico.rikken patrick reinhard renovate-bot repentinus schiessle sofiaritz tobiasd vincent
No Assignees delliott
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: FSFE/fsfe-website#5540
Delete Branch "posix-entrypoint"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?