From c28b4f0e126da7693f04b9dceda74ccc90614a9b Mon Sep 17 00:00:00 2001 From: Peter Dillinger Date: Tue, 28 Apr 2026 15:56:32 -0700 Subject: [PATCH] Fixes and enhancements to pre-push hook (#14680) Summary: The previous pre-push hook auto-formatted, committed, and re-pushed on behalf of the user. This was fragile: it's not clear whether the format fix should amend the current commit or create a new one. Adding a commit breaks populating the summary for creating a new PR. Also, the hook's internal re-push would drop flags like --set-upstream from the original command. Replace with a simple check-only approach that blocks the push and tells the user what to fix. Also add a new check for untracked source files (.cc, .h, .py, etc.) in tracked directories (excluding third-party/). These typically indicate files that were forgotten in the commit, which would cause the pushed code to fail to build. Pull Request resolved: https://github.com/facebook/rocksdb/pull/14680 Test Plan: manual Reviewed By: xingbowang Differential Revision: D102855227 Pulled By: pdillinger fbshipit-source-id: 7dc2c4e7a2b2c392bf8da74d7ea43883c8c075a9 --- Makefile | 3 ++ githooks/pre-push | 106 +++++++++++++++++----------------------------- 2 files changed, 42 insertions(+), 67 deletions(-) diff --git a/Makefile b/Makefile index 2e6c329b14..f4a07cbf3d 100644 --- a/Makefile +++ b/Makefile @@ -1268,6 +1268,8 @@ check-format: build_tools/format-diff.sh -c +# Crude alternative to setup-hooks: copies hooks into .git/hooks/ instead of +# using core.hooksPath. The copies won't track changes to githooks/. install-hooks: @echo "Installing git hooks from githooks/..." @if [ -d githooks ]; then \ @@ -1283,6 +1285,7 @@ install-hooks: exit 1; \ fi +# Reverse of install-hooks (not needed if using setup-hooks / core.hooksPath). uninstall-hooks: @echo "Removing installed git hooks..." @for hook in githooks/*; do \ diff --git a/githooks/pre-push b/githooks/pre-push index 9763b37478..5f0d524005 100755 --- a/githooks/pre-push +++ b/githooks/pre-push @@ -1,89 +1,61 @@ #!/usr/bin/env bash -# pre-push hook: auto-format changed code before pushing. +# pre-push hook: block pushing on common mistakes. # -# Runs `make format-auto` to apply clang-format to files changed relative to -# the upstream merge base. If any files are reformatted, they are committed as -# a new "format" commit and the push is retried automatically. +# 1. Checks for untracked source files in tracked directories, which likely +# need to be committed or removed before the pushed code will build. +# 2. Runs `make check-format` to verify formatting. # -# Install with: -# make install-hooks +# Automatically active via `make all` / `make check` (sets core.hooksPath). # # Skip with: # git push --no-verify set -e -REMOTE="$1" -URL="$2" - # Only act when we have a terminal (skip in CI / automated pushes) if [ ! -t 1 ] && [ -z "$ROCKSDB_FORMAT_HOOK" ]; then exit 0 fi -# Read the refs being pushed from stdin -PUSH_REFS=() -while read local_ref local_oid remote_ref remote_oid; do - PUSH_REFS+=("$local_ref:$remote_ref") -done +FAILED=0 +# --- Check for untracked source files in tracked directories --- +SUSPECT_FILES=$(git ls-files --others --exclude-standard \ + | grep -E '\.(cc|cpp|c|h|hpp|java|py|mk|sh)$' \ + | grep -v '^third-party/' \ + | while IFS= read -r f; do + top_dir="${f%%/*}" + if [ "$top_dir" = "$f" ] || [ -n "$(git ls-files "$top_dir/" | head -1)" ]; then + echo "$f" + fi + done) + +if [ -n "$SUSPECT_FILES" ]; then + echo "" + echo "===========================================================" + echo " pre-push: untracked source files in tracked directories." + echo " Push blocked. These may need to be committed, or the" + echo " pushed code could fail to build:" + echo "" + echo "$SUSPECT_FILES" | sed 's/^/ /' + echo "" + echo " Commit them, add to .gitignore, delete, or --no-verify." + echo "===========================================================" + FAILED=1 +fi + +# --- Check formatting --- echo "pre-push: running format check..." -# Save any uncommitted work so format-auto only touches committed code -NEED_STASH=0 -if ! git diff --quiet || ! git diff --cached --quiet; then - git stash push -q -m "pre-push-hook-$(date +%s)" - NEED_STASH=1 -fi - -cleanup() { - if [ "$NEED_STASH" -eq 1 ]; then - git stash pop -q 2>/dev/null || true - fi -} -trap cleanup EXIT - -# Run format check (check-only mode first to see if anything needs fixing) -if make check-format 2>/dev/null; then - echo "pre-push: formatting is clean." - exit 0 -fi - -echo "pre-push: formatting issues detected, auto-fixing..." -make format-auto - -# Check if format-auto actually changed anything -if git diff --quiet; then - echo "pre-push: formatting is clean after auto-fix." - exit 0 -fi - -# Create a new format commit -git add -A -git commit -m "format: auto-format code" --no-verify -q - -# Build the refspecs using the current (post-format) branch HEAD -REFSPECS=() -for ref_pair in "${PUSH_REFS[@]}"; do - remote_ref="${ref_pair#*:}" - REFSPECS+=("HEAD:$remote_ref") -done - -# Retry push, skipping the hook to avoid recursion -if git push --no-verify "$REMOTE" "${REFSPECS[@]}" 2>&1; then +if ! make check-format; then echo "" echo "===========================================================" - echo " pre-push: automatically formatted with a new commit and" - echo " pushed. You can ignore the 'failed to push' message below" - echo " — that is git cancelling the stale push; your code is" - echo " already on the remote." + echo " pre-push: formatting issues detected. Push blocked." + echo " Run 'make format-auto' then amend your commit and retry." echo "===========================================================" - exit 1 + FAILED=1 else - echo "" - echo "===========================================================" - echo " pre-push: format commit was added but push failed." - echo " Please run 'git push' again manually." - echo "===========================================================" - exit 1 + echo "pre-push: formatting is clean." fi + +exit $FAILED