-
Notifications
You must be signed in to change notification settings - Fork 38.8k
script: improve scripted-diff check #21002
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
Conversation
test/lint/commit-script-check.sh
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected a generic warning that the command failed, maybe something more like:
if ! (eval "$SCRIPT"); then
echo "Warning: Script failed (non-zero exit code)"
if echo "$SCRIPT" | grep -q "sed"; then
echo "Suggestion: did you use BSD sed syntax?"
echo "Example: sed -i '' 's/foo/bar/' filename (Bad)"
fi
fiThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sed doesn't have an exit code for "error", so checking the exit code wouldn't work either way.
$ sed -i -e 's/FOOBAR/foobar/g' ./src/init.cpp && echo $?
0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"sed -i '' -e 's/FOOBAR/foobar/g' ./src/init.cpp && echo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the OpenBSD 6.9 man pages for sed, it states EXIT STATUS The sed utility exits 0 on success, and >0 if an error occurs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failing to replace is not an error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The line "git --no-pager diff --exit-code $commit && echo "OK" || (echo "Failed"; false) || RET=1" should check if the script failed to change anything, do you want to check each command and error if if has no change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to not check for non zero exit codes but only check if the output of the script is the same as the commit?
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
|
Hi @4D617278, would you mind squashing your commits as described in these guidelines? |
…a nonportable command which is present in the script when the script fails Fix previous sed command that sometimes fails when using BSD sed, error "expected EOF..."
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Looks like ##19815 is already fixed? |
Print generic warning if script fails and add extensible way to check for non portable commands. Tested on OpenBSD and Gentoo Linux using the command line with OpenBSD version 6.9 and GNU sed version 4.8 (Gentoo). Checked for POSIX compliance on https://www.shellcheck.net/. Fixes #19815