refactor: pre-push-hook.sh potential regression#7264
Conversation
✅ No Merge Conflicts DetectedThis PR currently has no conflicts with other open PRs. |
|
✅ Review complete (commit 3b99477) |
WalkthroughThe pre-push hook's stdin parsing was changed from reading a single unstructured line and using positional parameters to reading four tab-delimited fields into named variables: Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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. The fix is both ShellCheck-compliant and functionally correct.
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
Clean fix replacing fragile set -- A "$LINE" word-splitting with read -r named variables. The old code relied on positional parameters after set --, which is brittle when stdin lines contain unexpected whitespace or special characters. The new code uses read -r local_ref local_oid remote_ref remote_oid which directly maps to git's pre-push stdin format. Also fixes the error-path call to verify-commits.py which was missing the $local_oid argument. No issues found.
Reviewed commit: 3b99477
thepastaclaw
left a comment
There was a problem hiding this comment.
Code Review
PR correctly fixes a regression in contrib/verify-commits/pre-push-hook.sh where the previous set -- A "$LINE" form collapsed git's tab-delimited pre-push line into a single argument, making the refs/heads/master check always fail and silently skipping signature verification. The replacement with read -r local_ref local_oid remote_ref remote_oid matches git's documented pre-push stdin format and is ShellCheck-clean (the SC2034 suppression for unused local_ref/remote_oid is justified). Both agents and CodeRabbit reported no remaining defects at HEAD 3b99477.
Reviewed commit: 3b99477
Fresh dispatcher run for this queue item. A same-SHA review already existed, so this records the fresh verification without duplicating inline threads.
Code Review
Reviewed commit: 3b99477
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…$5correctly, 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=SC2034to bypass false positivex not being usedwarning.How Has This Been Tested?
Not tested.
Breaking Changes
None.
Checklist:
Go over all the following points, and put an
xin all the boxes that apply.