Skip to content

feat: add pre-push PR size check local hook #2939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
swasti29 wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

feat: add pre-push PR size check local hook #2939

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
acefd3f
feat: add pre-push PR size check local hook
Apr 17, 2026
c822e72
fix: align local hook exclusions with unified workflow
Apr 17, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 110 additions & 0 deletions .githooks/pr-size-check.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
#!/usr/bin/env bash
Copy link
Copy Markdown

@azure-pipelines azure-pipelines Bot Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pull request does not update CHANGELOG.md.

Please consider if this change would be noticeable to a partner or user and either update CHANGELOG.md or resolve this conversation.

# ============================================================
# PR Size Check — Reusable Hook Logic
#
# Do NOT call this directly. It is sourced by the pre-push hook
# with repo-specific config already set:
#
# MAX_LINES — line change limit (default: 500)
# EXTRA_EXCLUDES — bash array of additional regex patterns
# ============================================================

MAX_LINES="${MAX_LINES:-500}"

# ── Common exclusions (all repos) ────────────────────────────
# Kept in sync with .github/workflows/pr-size-check-reusable.yml
COMMON_EXCLUDES=(
"\.pbxproj$"
"\.xcscheme$"
"\.xcsettings$"
"\.xcconfig$"
"\.xctestplan$"
"\.xcworkspace/"
"\.xcodeproj/"
"\.plist$"
"\.entitlements$"
"\.storyboard$"
"\.xib$"
"\.xcassets/"
"\.modulemap$"
"\.xcprivacy$"
"\.strings$"
"\.stringsdict$"
"\.xcstrings$"
"\.ya?ml$"
"\.lock$"
"\.(png|jpe?g|gif|bmp|svg|webp|heic|heif|tiff?|ico|pdf|icns)$"
"\.(mp4|mov|m4v|avi|mpe?g|webm|mp3|wav|aiff?|m4a)$"
"\.md$"
"\.mdx$"
Comment on lines +14 to +39
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says COMMON_EXCLUDES is “Kept in sync with .github/workflows/pr-size-check-reusable.yml”, but the patterns don’t currently match (e.g., the workflow excludes only a subset of image extensions and does not list .strings/.stringsdict/.xcstrings, while this hook does; the workflow’s media list differs as well).

Impact: Local pre-push warnings may disagree with CI PR Size Check results, creating confusion and reducing trust in the hook.

Recommendation: Update COMMON_EXCLUDES to exactly mirror the workflow’s COMMON_EXCLUDE list, or generate both from a single source of truth to prevent drift.

Copilot uses AI. Check for mistakes.
)
Comment on lines +14 to +40
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: The local hook’s COMMON_EXCLUDES doesn’t match the repo’s “single source of truth” workflow exclusions (e.g., it additionally excludes *.strings, *.stringsdict, and *.xcstrings).
Impact: Local warnings won’t mirror the GitHub PR Size Check results, which can lead to false negatives (no local warning but CI fails) or inconsistent expectations.
Recommendation: Align this list exactly with .github/workflows/pr-size-check-reusable.yml common exclusions, or clearly document any intentional differences in the hook header comment.

Copilot uses AI. Check for mistakes.

# ── Merge common + repo-specific exclusions ───────────────────
ALL_EXCLUDES=("${COMMON_EXCLUDES[@]}" "${EXTRA_EXCLUDES[@]}")

# ── Resolve base branch — tries origin/dev, origin/main, origin/master ──────
BASE_BRANCH=""
for candidate in origin/dev origin/main origin/master; do
if git show-ref --verify --quiet "refs/remotes/${candidate}"; then
BASE_BRANCH="$candidate"
break
fi
done

if [ -z "$BASE_BRANCH" ]; then
echo "ℹ️ PR size check skipped: no remote base branch found."
exit 0
fi

MERGE_BASE=$(git merge-base HEAD "$BASE_BRANCH" 2>/dev/null)
if [ -z "$MERGE_BASE" ]; then
echo "ℹ️ PR size check skipped: no common ancestor with ${BASE_BRANCH}."
exit 0
fi

# ── Count lines changed (excluding patterns & binaries) ──────
TOTAL_LINES=0
while IFS=$'\t' read -r added deleted filepath; do
[ -z "$filepath" ] && continue

# Check exclusion first
excluded=false
for pattern in "${ALL_EXCLUDES[@]}"; do
if [[ "$filepath" =~ $pattern ]]; then
excluded=true
break
fi
done
[ "$excluded" = true ] && continue

# Binary/submodule: git diff --numstat reports "-" for these
if [ "$added" = "-" ] || [ "$deleted" = "-" ]; then
echo "⚠️ Binary or submodule change in non-excluded file: $filepath"
continue
fi

TOTAL_LINES=$((TOTAL_LINES + added + deleted))
done < <(git diff --numstat "$MERGE_BASE"...HEAD 2>/dev/null)

# ── Warn if over limit ────────────────────────────────────────
if [ "$TOTAL_LINES" -gt "$MAX_LINES" ]; then
echo ""
echo "⚠️ WARNING: Your push contains ~${TOTAL_LINES} line changes (limit: ${MAX_LINES})."
Comment on lines +59 to +92
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pre-push logic always computes changes for HEAD (git merge-base HEAD … and git diff …HEAD). However, pre-push can be invoked when pushing refs that are not the currently checked-out branch (or multiple refs), and Git provides the local/remote refs and SHAs on stdin.

Impact: The hook can under/over-report the size of what’s actually being pushed, which defeats the purpose of a pre-push guard.

Recommendation: Parse the pre-push stdin lines and run the diff calculation for each local_sha being pushed (or at least use the SHA of the current ref being pushed) instead of hard-coding HEAD.

Suggested change
MERGE_BASE=$(git merge-base HEAD "$BASE_BRANCH" 2>/dev/null)
if [ -z "$MERGE_BASE" ]; then
echo "ℹ️ PR size check skipped: no common ancestor with ${BASE_BRANCH}."
exit 0
fi
# ── Count lines changed (excluding patterns & binaries) ──────
TOTAL_LINES=0
while IFS=$'\t' read -r added deleted filepath; do
[ -z "$filepath" ] && continue
# Check exclusion first
excluded=false
for pattern in "${ALL_EXCLUDES[@]}"; do
if [[ "$filepath" =~ $pattern ]]; then
excluded=true
break
fi
done
[ "$excluded" = true ] && continue
# Binary/submodule: git diff --numstat reports "-" for these
if [ "$added" = "-" ] || [ "$deleted" = "-" ]; then
echo "⚠️ Binary or submodule change in non-excluded file: $filepath"
continue
fi
TOTAL_LINES=$((TOTAL_LINES + added + deleted))
done < <(git diff --numstat "$MERGE_BASE"...HEAD 2>/dev/null)
# ── Warn if over limit ────────────────────────────────────────
if [ "$TOTAL_LINES" -gt "$MAX_LINES" ]; then
echo ""
echo "⚠️ WARNING: Your push contains ~${TOTAL_LINES} line changes (limit: ${MAX_LINES})."
# ── Read refs being pushed from pre-push stdin ────────────────
PUSH_REFS=()
while IFS=' ' read -r local_ref local_sha remote_ref remote_sha; do
[ -z "$local_ref" ] && continue
PUSH_REFS+=("${local_ref}|${local_sha}|${remote_ref}|${remote_sha}")
done
if [ "${#PUSH_REFS[@]}" -eq 0 ]; then
PUSH_REFS+=("HEAD|$(git rev-parse HEAD 2>/dev/null)||")
fi
# ── Count lines changed (excluding patterns & binaries) per ref ──
TOTAL_LINES=0
MAX_PUSH_LINES=0
MAX_PUSH_REF=""
PROCESSED_REFS=0
for push_ref in "${PUSH_REFS[@]}"; do
IFS='|' read -r local_ref local_sha remote_ref remote_sha <<< "$push_ref"
# Deleted ref; nothing local to diff.
if [[ "$local_sha" =~ ^0+$ ]]; then
continue
fi
MERGE_BASE=$(git merge-base "$local_sha" "$BASE_BRANCH" 2>/dev/null)
if [ -z "$MERGE_BASE" ]; then
echo "ℹ️ PR size check skipped for ${local_ref}: no common ancestor with ${BASE_BRANCH}."
continue
fi
REF_TOTAL_LINES=0
while IFS=$'\t' read -r added deleted filepath; do
[ -z "$filepath" ] && continue
# Check exclusion first
excluded=false
for pattern in "${ALL_EXCLUDES[@]}"; do
if [[ "$filepath" =~ $pattern ]]; then
excluded=true
break
fi
done
[ "$excluded" = true ] && continue
# Binary/submodule: git diff --numstat reports "-" for these
if [ "$added" = "-" ] || [ "$deleted" = "-" ]; then
echo "⚠️ Binary or submodule change in non-excluded file for ${local_ref}: $filepath"
continue
fi
REF_TOTAL_LINES=$((REF_TOTAL_LINES + added + deleted))
done < <(git diff --numstat "$MERGE_BASE"...$local_sha 2>/dev/null)
PROCESSED_REFS=$((PROCESSED_REFS + 1))
if [ "$REF_TOTAL_LINES" -gt "$MAX_PUSH_LINES" ]; then
MAX_PUSH_LINES=$REF_TOTAL_LINES
MAX_PUSH_REF="$local_ref"
fi
done
if [ "$PROCESSED_REFS" -eq 0 ]; then
echo "ℹ️ PR size check skipped: no pushable local refs with a common ancestor to ${BASE_BRANCH}."
exit 0
fi
TOTAL_LINES=$MAX_PUSH_LINES
# ── Warn if over limit ────────────────────────────────────────
if [ "$TOTAL_LINES" -gt "$MAX_LINES" ]; then
echo ""
echo "⚠️ WARNING: Your push includes ${MAX_PUSH_REF} with ~${TOTAL_LINES} line changes (limit: ${MAX_LINES})."

Copilot uses AI. Check for mistakes.
echo " Excluded paths match the PR Size Check workflow."
echo " Consider splitting this into smaller PRs."
echo ""
echo " To push anyway: git push --no-verify"
echo " To cancel: Ctrl+C"
echo ""
if [ -t 1 ] && [ -r /dev/tty ]; then
read -r -p " Push anyway? [y/N]: " response < /dev/tty
if [[ ! "$response" =~ ^[Yy]$ ]]; then
echo "Push cancelled."
exit 1
fi
else
echo " Non-interactive shell detected; allowing push."
Comment on lines +99 to +106
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interactive detection uses -t 1 (stdout) but the prompt reads from /dev/tty. In cases where stdout is redirected (but a controlling TTY exists), this will incorrectly treat the shell as non-interactive and allow the push without prompting.

Impact: The “confirm or cancel” behavior becomes unreliable, especially in scripted or wrapped git invocations that still have a TTY.

Recommendation: Gate prompting on the TTY you actually read from (e.g., check -r /dev/tty and/or -t 0 / -t 2, or test -t /dev/tty where supported) rather than -t 1.

Suggested change
if [ -t 1 ] && [ -r /dev/tty ]; then
read -r -p " Push anyway? [y/N]: " response < /dev/tty
if [[ ! "$response" =~ ^[Yy]$ ]]; then
echo "Push cancelled."
exit 1
fi
else
echo " Non-interactive shell detected; allowing push."
if [ -r /dev/tty ] && [ -t 0 ] < /dev/tty; then
read -r -p " Push anyway? [y/N]: " response < /dev/tty
if [[ ! "$response" =~ ^[Yy]$ ]]; then
echo "Push cancelled."
exit 1
fi
else
echo " No interactive terminal available; allowing push."

Copilot uses AI. Check for mistakes.
fi
fi

exit 0
17 changes: 17 additions & 0 deletions .githooks/pre-push
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#!/usr/bin/env bash
# ============================================================
# Pre-push hook — PR Size Soft Warning
# Repo: microsoft-authentication-library-for-objc
#
# Fires on every push to any remote branch.
# Bypass with: git push --no-verify
# ============================================================

EXTRA_EXCLUDES=(
"^MSAL/test/"
"^MSAL/IdentityCore/"
"^Samples/"
)

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
source "$SCRIPT_DIR/pr-size-check.sh"
5 changes: 5 additions & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
setup:
git config core.hooksPath .githooks
@echo "✅ Git hooks configured. Pre-push PR size check is now active."
Comment on lines +2 to +3
Copy link

Copilot AI Apr 17, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue: The Makefile recipe lines under setup are not indented with a tab, so make setup will error with “missing separator”.
Impact: The documented setup step won’t work, so contributors won’t be able to enable the hooks via make setup.
Recommendation: Indent each recipe line with a literal tab (or set .RECIPEPREFIX and use that prefix consistently) so the target executes correctly.

Suggested change
git config core.hooksPath .githooks
@echo "✅ Git hooks configured. Pre-push PR size check is now active."
git config core.hooksPath .githooks
@echo "✅ Git hooks configured. Pre-push PR size check is now active."

Copilot uses AI. Check for mistakes.

.PHONY: setup
Loading