refactor: pre-push-hook.sh potential regression #34999

Issue being fixed or feature implemented

With set -- A "$LINE" (quoted), the entire input line becomes $2 as a single string, leaving $3 and $4 empty. Since empty $4 never equals "refs/heads/master", the hook always continues, and the commit-signing verification never executes in either repo.

Using the unquoted form set -- A $LINE (no quotes), which relied on word-splitting to populate $2…$5 correctly, trips ShellCheck's SC2086 rule.

What was done?

Parse the fields directly with read: The fix is both ShellCheck-compliant and functionally correct.

edit: Added # shellcheck disable=SC2034 to bypass false positive x not being used warning.

How Has This Been Tested?

Not tested. I initially PR'd this to Dash, but was recommended to PR to Bitcoin too, since it's inherited.

Breaking Changes

None.

Checklist:

Go over all the following points, and put an x in all the boxes that apply.

• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• I have added or updated relevant unit/integration/functional/e2e tests
• I have made corresponding changes to the documentation
• I have assigned this pull request to a milestone (for repository code-owners and collaborators only)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

When you say this is a "regression", is that since 9a1ad7bc0dd8a0769738ca4dffbeb8d55438b0dc?

Concept ack on fixing this.

Though, the project may not have enough reviewers fluent in Bash, so if no one reviews this, it could make sense to re-write in Python.

When you say this is a "regression", is that since 9a1ad7b?

Concept ack on fixing this.

Though, the project may not have enough reviewers fluent in Bash, so if no one reviews this, it could make sense to re-write in Python.

Yes, it was introduced when the shellcheck rule SC2086 was enabled.

It's unclear to me if anyone even uses this hook, and I would prefer to simply drop it if no one uses it.

Thanks, but LLM output is not accepted if the author can not properly explain, test or could have written the change themselves.

The bottleneck in this project has always been review and testing, not writing code. Development here is intentionally conservative and slow, and reviewer attention is the scarcest resource we have. LLMs have made this worse, anyone can now prompt them and post their output as PRs. There is an infinite amount plausible looking "improvements" for LLMs to suggest and work on.

Unless we fully trust LLMs to both write and review code, humans still have to spend time understanding the proposed changes, which incurs a non-zero cost for every opened PR.

I understand that contributing to this project can be intimidating, and using LLMs may seem tempting, but it really creates more issues for this project than it solves. The best way to help this project, is to review and test changes. You can use LLMs for this, but you shouldn't solely rely on them, or just post their output.

Please reconsider whether it's something you genuinely think the project should pursue, independent of what your LLM suggested.

I'll close this for now.

I think the fix may be correct, but I don't know Bash, so I can't review this.

In any case, this is a maintainer-only script, so it should move to that repo. Done in https://github.com/bitcoin/bitcoin/pull/35235