FSFE/fsfe-website
30
33
Fork 96
Code Issues 65 Pull Requests 15 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
2 changed files with 4 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pre-commit.Dockerfile
+1 -1
View File
@@ -45,4 +45,4 @@ COPY pre-commit.entrypoint.sh ./
# Set the workdir
WORKDIR /website-source
ENTRYPOINT ["bash", "/website-source-during-build/pre-commit.entrypoint.sh"]
ENTRYPOINT ["sh", "/website-source-during-build/pre-commit.entrypoint.sh"]
pre-commit.entrypoint.sh
+3 -3
View File
@@ -1,5 +1,5 @@
#!/usr/bin/env bash
set -euo pipefail
fkobi marked this conversation as resolved Outdated
delliott commented 2025-12-04 09:08:15 +00:00
Outdated
Review
Copy Link
Copy Source

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
Outdated
Review
Copy Link
Copy Source

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
Outdated
Review
Copy Link
Copy Source

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
Outdated
Review
Copy Link
Copy Source

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
#!/bin/sh
set -eu
# Get the changed files
target_branch="${1:-master}"
@@ -8,7 +8,7 @@ files="$(git diff --name-only "$source_branch" "$target_branch")"
files_args=""
for file in $files; do
if [ -f "$file" ]; then
files_args+="--file $file "
files_args="$files_args --file $file"
fi
done