lint: Require scripted-diff script to succeed #35547

Previous version of commit-script-check.sh would succeed as long as git diff succeeded.

Can be verified through adding a failing scripted diff commit such as:

git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\nadsasd\n-END VERIFY SCRIPT-\n'

...and running...

cargo run --manifest-path ./test/lint/test_runner/Cargo.toml -- --lint=scripted_diff

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35547.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

lgtm ACK 2a36d6a5614f994d3236a203ea2ac112440186ae

Thanks, this was on my todo-list for years, but I didn't know Bash enough to do this or review this, but the change is nice conceptually.

Maybe for fun, someone could check if all historic scripted diffs pass this new rule? IIRC some were failing, but my memory may be off.

ACK 2a36d6a5614f994d3236a203ea2ac112440186ae

I have this, and other checks, on my re-write #35275 as well (https://github.com/furszy/bitcoin-core/commits/2026_scripted-diff/). I never got around to pushing it after the discussion there, will see if can start decoupling some of it at least.

ACK 2a36d6a5614f994d3236a203ea2ac112440186ae.

Thanks, this was on my todo-list for years, but I didn't know Bash enough to do this or review this, but the change is nice conceptually.

To expand on this: The problem is that this fix here is incomplete. What we want is to catch any error in any command, not just if the last command in the script failed.

For example, this script fails in almost all steps, but passes with this pull request:

$ git commit --allow-empty -m $'scripted-diff: foo\n\n-BEGIN VERIFY SCRIPT-\n  false;falseasfsafsaf;true;false|cat; echo "${NO_UN_SET}"|cat  \n-END VERIFY SCRIPT-\n' && ./test/lint/commit-script-check.sh HEAD~..HEAD ; echo $?
[detached HEAD fa06c35] scripted-diff: foo
Running script for: fa06c35f70e798ec75684085ff27fb39d0babd07
  false;falseasfsafsaf;true;false|cat; echo "${NO_UN_SET}"|cat
./test/lint/commit-script-check.sh: line 44: falseasfsafsaf: command not found

OK
0

There are set -o errexit -o pipefail -o xtrace -o nounset, but I don't know enough Bash good practise to enforce this here.

It would be good to properly fix this somehow, because this pull doesn't really do that.

Maybe for fun, someone could check if all historic scripted diffs pass this new rule? IIRC some were failing, but my memory may be off.

My memory returned, and this was indeed last done in #29692: E.g: https://github.com/bitcoin/bitcoin/commit/fb65dde147f63422c4148b089c2f5be0bf5ba80f (2021): throws "sed: no input files" a few times, but does produce the right result

Maybe for fun, someone could check if all historic scripted diffs pass this new rule? IIRC some were failing, but my memory may be off.

My memory returned, and this was indeed last done in #29692: E.g: fb65dde (2021): throws "sed: no input files" a few times, but does produce the right result

fb65dde147f63422c4148b089c2f5be0bf5ba80f was renaming unspendables_unclaimed_rewards to total_unspendables_unclaimed_rewards and later m_unspendables_unclaimed_rewards to m_total_unspendables_unclaimed_rewards. I tried running it on top of this PR and it does fail, but modifying the sed-expression to s/\b$1\b/$2/g to match on word boundaries makes it succeed. So I think this is still a step in the right direction.

Re #35547 (comment): I agree set -o errexit -o pipefail -o xtrace -o nounset might be worth exploring too for a more complete fix but I'm no bash-guru either.

So I think this is still a step in the right direction.

Yes, certainly. Seeing ancient scripts fail is useful to see that this concept will likely catch real issues in the future. There is also no value in trying to keep compatibility to let ancient scripts pass. If someone really wanted to do this, they could just use the prior version of this Shell script.

Re #35547 (comment): I agree set -o errexit -o pipefail -o xtrace -o nounset might be worth exploring too for a more complete fix but I'm no bash-guru either.

Ok, fixed in https://github.com/bitcoin/bitcoin/pull/35560

I have this, and other checks, on my re-write #35275 as well (https://github.com/furszy/bitcoin-core/commits/2026_scripted-diff/). I never got around to pushing it after the discussion there, will see if can start decoupling some of it at least.

I looked there and can't see this check in the branch.

In theory one could directly rewrite the bash to Python subprocess.run (possibly with bash -c), however this may not be worth the review effort. Untested (possibly broken) diff:

diff --git a/test/lint/commit-script-check.sh b/test/lint/commit-script-check.sh
index 08fed21..cfb1fae 100755
--- a/test/lint/commit-script-check.sh
+++ b/test/lint/commit-script-check.sh
@@ -1,4 +1,5 @@
-#!/usr/bin/env bash
+#!/usr/bin/env python3
+#
 # Copyright (c) 2017-present The Bitcoin Core developers
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
@@ -11,51 +12,63 @@
 # The resulting script should exactly transform the previous commit into the current
 # one. Any remaining diff signals an error.
 
-export LC_ALL=C
-if test -z "$1"; then
-    echo "Usage: $0 <commit>..."
-    exit 1
-fi
+import os
+from subprocess import PIPE, run
+import sys
+if len(sys.argv) < 2:
+    sys.exit(f"Usage: {sys.argv[0]} <commit>...")
 
-if ! sed --help 2>&1 | grep -q 'GNU'; then
-    echo "Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.";
-    exit 1;
-fi
+if run(["bash", "-c", "sed --help 2>&1 | grep -q 'GNU'"]).returncode != 0:
+    sys.exit("Error: the installed sed package is not compatible. Please make sure you have GNU sed installed in your system.")
 
-if ! grep --help 2>&1 | grep -q 'GNU'; then
-    echo "Error: the installed grep package is not compatible. Please make sure you have GNU grep installed in your system.";
-    exit 1;
-fi
+if run(["bash", "-c", "grep --help 2>&1 | grep -q 'GNU'"]).returncode != 0:
+    sys.exit("Error: the installed grep package is not compatible. Please make sure you have GNU grep installed in your system.")
+
+def git_output(*args: str) -> str:
+    return run(["git", *args], stdout=PIPE, text=True).stdout
 
 RET=0
-PREV_BRANCH=$(git name-rev --name-only HEAD)
-PREV_HEAD=$(git rev-parse HEAD)
-for commit in $(git rev-list --reverse "$1"); do
-    if git rev-list -n 1 --pretty="%s" "$commit" | grep -q "^scripted-diff:"; then
-        git checkout --quiet "$commit"^ || exit
-        SCRIPT="$(git rev-list --format=%b -n1 "$commit" | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d')"
-        if test -z "$SCRIPT"; then
-            echo "Error: missing script for: $commit" >&2
-            echo "Failed" >&2
-            RET=1
-        else
-            echo "Running script for: $commit" >&2
-            echo "$SCRIPT" >&2
-            if bash -o errexit -o nounset -o pipefail -c "$SCRIPT" && git --no-pager diff --exit-code "$commit"; then
-                echo "OK" >&2
-            else
-                echo "Failed" >&2
-                RET=1
-            fi
-        fi
-        git reset --quiet --hard HEAD
-     else
-        if git rev-list "--format=%b" -n1 "$commit" | grep -q '^-\(BEGIN\|END\)[ a-zA-Z]*-$'; then
-            echo "Error: script block marker but no scripted-diff in title of commit $commit" >&2
-            echo "Failed" >&2
-            RET=1
-        fi
-    fi
-done
-git checkout --quiet "$PREV_BRANCH" 2>/dev/null || git checkout --quiet "$PREV_HEAD"
-exit $RET
+
+prev_branch = git_output("name-rev", "--name-only", "HEAD").rstrip("\n")
+prev_head = git_output("rev-parse", "HEAD").rstrip("\n")
+
+for commit in git_output("rev-list", "--reverse", sys.argv[1]).splitlines():
+    if run(["bash", "-c", f"git rev-list -n 1 --pretty=%s {commit} | grep -q '^scripted-diff:'"]).returncode == 0:
+        checkout = run(["git", "checkout", "--quiet", f"{commit}^"], check=True)
+
+        script = run(["bash", "-c", f"git rev-list --format=%b -n1 {commit} | sed '/^-BEGIN VERIFY SCRIPT-$/,/^-END VERIFY SCRIPT-$/{//!b};d'"], stdout=PIPE, text=True).stdout.rstrip("\n")
+        if script == "":
+            print(f"Error: missing script for: {commit}", file=sys.stderr)
+            print("Failed", file=sys.stderr)
+            RET = 1
+        else:
+            print(f"Running script for: {commit}", file=sys.stderr)
+            print(script, file=sys.stderr)
+            script_status = run(
+                ["bash", "-o", "errexit", "-o", "nounset", "-o", "pipefail", "-c", script]
+            ).returncode
+            if script_status == 0:
+                diff_status = run(
+                    ["git", "--no-pager", "diff", "--exit-code", commit]
+                ).returncode
+            else:
+                diff_status = script_status
+            if script_status == 0 and diff_status == 0:
+                print("OK", file=sys.stderr)
+            else:
+                print("Failed", file=sys.stderr)
+                RET = 1
+
+        run(["git", "reset", "--quiet", "--hard", "HEAD"])
+    elif run(["bash", "-c", f"git rev-list --format=%b -n1 {commit} | grep -q '^-\\(BEGIN\\|END\\)[ a-zA-Z]*-$'"]).returncode == 0:
+        print(
+            f"Error: script block marker but no scripted-diff in title of commit {commit}",
+            file=sys.stderr,
+        )
+        print("Failed", file=sys.stderr)
+        RET = 1
+
+checkout = run(["git", "checkout", "--quiet", prev_branch], stderr=subprocess.DEVNULL)
+if checkout.returncode != 0:
+    run(["git", "checkout", "--quiet", prev_head])
+sys.exit(RET)