fix: limit local branch commit range for git hooks #1487
Conversation
Signed-off-by: sujitaw <[email protected]>
WalkthroughThe PR updates two Husky hook scripts: Changes
Sequence Diagram(s)sequenceDiagram
participant User as Git User
participant PrePush as .husky/pre-push
participant Git as Git
participant Validator as Unsigned-commit Check
User->>PrePush: git push
activate PrePush
PrePush->>Git: read stdin -> tmp file
PrePush->>PrePush: parse local_sha, remote_sha, local_ref
alt initial push (remote_sha = 0000...)
rect rgb(235,245,255)
PrePush->>PrePush: determine target_branch (from local_ref or HEAD)
PrePush->>Git: list local branches
Git-->>PrePush: branches
PrePush->>PrePush: compute merge-base distances
PrePush->>PrePush: pick closest base_branch or fallback (default_branch/origin/HEAD/main)
PrePush->>Git: compute base_commit (merge-base)
PrePush->>PrePush: set range = base_commit..local_sha (or local_sha fallback)
PrePush->>User: echo "commit range ${range}"
end
else non-initial push
PrePush->>PrePush: set range = remote_sha..local_sha (unchanged)
end
PrePush->>Validator: run unsigned-commit checks on range
Validator-->>PrePush: results
PrePush-->>User: allow or reject push
deactivate PrePush
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Potential attention points:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
sujitaw
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.husky/commit-msg (2)
4-10: Make color variables portable in POSIX sh
"\033[..."won’t produce colors viaechoin POSIXsh. Define colors usingprintfso variables contain the actual ESC char.-# ANSI colors -RED="\033[0;31m" -YELLOW="\033[0;33m" -CYAN="\033[0;36m" -GREEN="\033[0;32m" -RESET="\033[0m" +# ANSI colors (portable) +RED=$(printf '\033[0;31m') +YELLOW=$(printf '\033[0;33m') +CYAN=$(printf '\033[0;36m') +GREEN=$(printf '\033[0;32m') +RESET=$(printf '\033[0m')
21-22: Use printf for colored output; minor DCO check nitEnsure color escapes render by using
printf %b. Also consider anchoring the DCO check to the start of the line to avoid false positives like “Re-signed-off-by”.- echo "${RED} Commit message missing Signed-off-by line ${RESET}" - echo "${GREEN} Use: git commit -s ... ${RESET}" + printf "%b\n" "${RED}Commit message missing Signed-off-by line${RESET}" + printf "%b\n" "${GREEN}Use: git commit -s ...${RESET}"Optional (outside this hunk):
- Replace
grep -q "Signed-off-by:"withgrep -qi '^Signed-off-by:'for stricter matching..husky/pre-push (1)
39-39: Gate debug output behind a flagReduce noise on normal pushes; print range only when debugging.
- echo "commit range ${range}" + [ -n "$HUSKY_DEBUG" ] && echo "commit range $range"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.husky/commit-msg(2 hunks).husky/pre-push(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: GHkrishna
PR: credebl/platform#1469
File: .husky/pre-push:34-35
Timestamp: 2025-09-30T06:35:15.488Z
Learning: In the credebl/platform repository's pre-push hook (.husky/pre-push), when checking commit signatures, user GHkrishna encountered an edge case where pulling squashed commits from other contributors causes signature validation issues because those commits are signed with others' GPG keys. The user prefers a solution that distinguishes local commits from upstream commits to apply signature validation only to the current user's own commits.
🔇 Additional comments (1)
.husky/pre-push (1)
108-108: LGTMHelpful success message; no issues.
GHkrishna
changed the title
Signed-off-by: sujitaw <[email protected]>
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.husky/pre-push (1)
73-73: Consider making diagnostic output optional or removing before production.The commit range echo is helpful for debugging but may be noisy for regular use. Since this PR is in draft status, this might be intentional for testing.
Consider one of these options before merging:
- Remove the line entirely
- Make it conditional:
- echo "commit range ${range}" + [ -n "$DEBUG" ] && echo "commit range ${range}"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.husky/pre-push(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: GHkrishna
Repo: credebl/platform PR: 1469
File: .husky/pre-push:34-35
Timestamp: 2025-09-30T06:35:15.488Z
Learning: In the credebl/platform repository's pre-push hook (.husky/pre-push), when checking commit signatures, user GHkrishna encountered an edge case where pulling squashed commits from other contributors causes signature validation issues because those commits are signed with others' GPG keys. The user prefers a solution that distinguishes local commits from upstream commits to apply signature validation only to the current user's own commits.
Learnt from: GHkrishna
Repo: credebl/platform PR: 1468
File: .husky/pre-push:47-56
Timestamp: 2025-09-29T12:14:52.484Z
Learning: User GHkrishna prefers allowing automatic rebase in pre-push hooks for signing commits, considering it acceptable since they're rebasing their own local commits before sharing.
📚 Learning: 2025-09-30T06:35:15.488Z
Learnt from: GHkrishna
Repo: credebl/platform PR: 1469
File: .husky/pre-push:34-35
Timestamp: 2025-09-30T06:35:15.488Z
Learning: In the credebl/platform repository's pre-push hook (.husky/pre-push), when checking commit signatures, user GHkrishna encountered an edge case where pulling squashed commits from other contributors causes signature validation issues because those commits are signed with others' GPG keys. The user prefers a solution that distinguishes local commits from upstream commits to apply signature validation only to the current user's own commits.
Applied to files:
.husky/pre-push
📚 Learning: 2025-09-29T12:14:52.484Z
Learnt from: GHkrishna
Repo: credebl/platform PR: 1468
File: .husky/pre-push:47-56
Timestamp: 2025-09-29T12:14:52.484Z
Learning: User GHkrishna prefers allowing automatic rebase in pre-push hooks for signing commits, considering it acceptable since they're rebasing their own local commits before sharing.
Applied to files:
.husky/pre-push
🔇 Additional comments (3)
.husky/pre-push (3)
1-3: Verify whether commenting out husky.sh sourcing is intentional.The
husky.shsourcing is commented out, which may affect environment setup or path configuration. If this is for testing purposes, it should be uncommented before merging to production.
25-66: Excellent implementation of the commit range logic!The initial push range computation has been significantly improved and properly addresses the previous review feedback. The code now:
- ✅ Uses the ref being pushed rather than
HEADto handle cases where the pushed branch differs from the current checkout- ✅ Correctly excludes the target branch with
grep -vxFfor exact matching- ✅ Guards
git merge-baseoperations with|| continueand empty checks to prevent failures- ✅ Implements a robust multi-level fallback strategy (origin/HEAD → default_branch → main)
- ✅ Provides a safe minimal fallback (just
local_sha) when no common history existsThis implementation properly distinguishes local commits from upstream commits, addressing the learnings about signature validation edge cases.
Based on learnings
140-142: LGTM!Proper handling of the temporary file with correct input redirection and cleanup. The use of
rm -fensures the script won't fail even if the file doesn't exist.
| if [ ! -s "$TMP_FILE" ]; then | ||
| echo "⚠️ Validations won't apply when current branch and remote branch being pushed to are different." | ||
| fi |
There was a problem hiding this comment.
Clarify the warning message to accurately reflect the condition.
The warning message states "Validations won't apply when current branch and remote branch being pushed to are different," but it's triggered when stdin is empty, which can occur in various scenarios beyond just differing branches (e.g., nothing to push, certain git operations).
Consider revising to:
- echo "⚠️ Validations won't apply when current branch and remote branch being pushed to are different."
+ echo "⚠️ No refs to push - skipping commit signature validation."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [ ! -s "$TMP_FILE" ]; then | |
| echo "⚠️ Validations won't apply when current branch and remote branch being pushed to are different." | |
| fi | |
| if [ ! -s "$TMP_FILE" ]; then | |
| echo "⚠️ No refs to push - skipping commit signature validation." | |
| fi |
🤖 Prompt for AI Agents
In .husky/pre-push around lines 18 to 20, the warning text incorrectly ties the
condition to differing branches while the check actually detects an empty stdin
(no ref info), which can happen for reasons like nothing to push or certain git
operations; update the message to accurately state that no ref info was received
(stdin is empty) and therefore pre-push validations may be skipped, and
optionally include brief examples (e.g., nothing to push, special git workflows)
to make the warning clear.
There was a problem hiding this comment.
@coderabbitai when pushing to other remote branches for eg : my current local branch is develop and i perform git push origin main in that scenario too the refs are empty .
is there any other way to address this scenario
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Research git pre-push hook documentation and behavior
# Check if there are any environment variables or other ways to detect the scenario
# First, let's check what information is typically available in a pre-push hook
echo "=== Git pre-push hook documentation ==="
git help hooks 2>/dev/null | grep -A 30 "pre-push" | head -40
echo ""
echo "=== Checking git environment variables that might help ==="
# List common git environment variables that might be available
cat << 'EOF'
Common Git environment variables that might be available in pre-push hook:
- GIT_DIR
- GIT_WORK_TREE
- GIT_INDEX_FILE
- GIT_PREFIX
- GIT_REFLOG_ACTION
- GIT_PUSH_OPTION_COUNT
- GIT_PUSH_OPTION_<n>
Note: The pre-push hook receives information via stdin, not command-line arguments.
EOF
echo ""
echo "=== Understanding when stdin is empty ==="
cat << 'EOF'
The pre-push hook receives on stdin (one line per ref being pushed):
<local ref> <local sha1> <remote ref> <remote sha1>
stdin is empty when:
1. Nothing to push (local ref is already up-to-date with remote)
2. Specified ref doesn't exist locally
3. Push is a dry-run or gets rejected before refs are determined
For 'git push origin main' from 'develop' branch:
- If local 'main' exists and differs from 'origin/main': stdin will have data
- If local 'main' doesn't exist: error before hook runs
- If local 'main' is up-to-date with 'origin/main': stdin will be empty (nothing to push)
EOF
Length of output: 1030
🌐 Web query:
git pre-push hook empty stdin scenarios
💡 Result:
Short answer — pre-push hook stdin will be empty when there are no refs/objects being sent, or when something else has already consumed/failed to forward that input.
Common scenarios
- Nothing to push (git finds no changed refs) — git will not write any lines to the hook’s stdin. [1]
- Your hook/script (or a helper it calls, e.g. git-lfs) reads stdin before your read-loop, so subsequent reads see EOF. Move the read-loop before anything that consumes stdin. [2]
- A wrapper/tool running the hook (some hook managers) doesn’t forward git’s stdin into the actual hook process, so the hook sees empty stdin. Check the wrapper’s implementation/config. [3]
What to do
- Write your hook to handle empty stdin gracefully (e.g., use while read ...; do …; done and treat zero iterations as “nothing to check”). [1]
- Ensure no commands run before the read-loop that might consume stdin. [2]
- If using a hook manager/wrapper, verify it forwards stdin or run the hook directly. [3]
Sources
[1] Stack Overflow: “Git-hooks pre-push script does not receive input via STDIN”.
[2] Stack Overflow: “git pre-push hook stdin is empty” (notes about git-lfs and ordering).
[3] GitHub issue / wrapper discussion (lefthook) and Git hooks docs about pre-push input handling.
@sujitaw, great question! Let me explain what's happening in your scenario.
Why stdin is empty
When you run git push origin main from your develop branch:
- Git compares your local
mainbranch withorigin/main - If they're identical (nothing new to push), Git doesn't send any ref information to the pre-push hook
- This results in empty stdin
The pre-push hook only receives stdin with ref information when there are actual commits/refs to push. It doesn't receive the command-line arguments (like which branch you specified), so it can't distinguish between:
- "Nothing to push because refs are up-to-date"
- "Pushing a different branch than current HEAD"
- Other scenarios that result in empty stdin
Current behavior is correct
The current code handles this appropriately:
- Detects empty stdin
- Shows a warning
- Continues without blocking (which is correct - you shouldn't block a no-op push)
- Skips validation since there are no commits to validate
Suggested improvement
The only thing we can improve is making the warning message more accurate and informative:
if [ ! -s "$TMP_FILE" ]; then
echo "⚠️ No refs to push - skipping commit signature validation."
echo " (This occurs when local and remote refs are already in sync)"
fiThis message is more accurate because:
- It correctly states why validation is skipped (no refs to push)
- It doesn't make assumptions about branch differences
- It provides context about what "no refs" means
Bottom line: There's no programmatic way in the pre-push hook to detect your specific scenario differently from other empty-stdin cases, but that's okay—the hook should skip validation when there's nothing to push, regardless of the reason. The improved warning message makes this clearer to users.
🧠 Learnings used
Learnt from: GHkrishna
Repo: credebl/platform PR: 1469
File: .husky/pre-push:34-35
Timestamp: 2025-09-30T06:35:15.488Z
Learning: In the credebl/platform repository's pre-push hook (.husky/pre-push), when checking commit signatures, user GHkrishna encountered an edge case where pulling squashed commits from other contributors causes signature validation issues because those commits are signed with others' GPG keys. The user prefers a solution that distinguishes local commits from upstream commits to apply signature validation only to the current user's own commits.
Learnt from: GHkrishna
Repo: credebl/platform PR: 1468
File: .husky/pre-push:47-56
Timestamp: 2025-09-29T12:14:52.484Z
Learning: User GHkrishna prefers allowing automatic rebase in pre-push hooks for signing commits, considering it acceptable since they're rebasing their own local commits before sharing.
|
@sujitaw, since this PR is in sync with credebl/studio#1257 I think we can merge this PR too |
What
Summary by CodeRabbit
Bug Fixes
Improvements