From bdcccb97769b2247e22cb8e1dc05b8274d46fafd Mon Sep 17 00:00:00 2001
From: teor <teor@torproject.org>
Date: Tue, 20 Aug 2019 12:21:02 +1000
Subject: [PATCH] scripts/git: Remove a duplicate practracker call from the
 pre-push hook

The pre-push hook already calls the pre-commit hook, which calls
practracker.

Also update the script comments to avoid similar issues in future.

Fixes bug 31462; bugfix on 0.4.1.1-alpha.
---
 changes/bug31462                |  4 ++++
 changes/ticket30979             |  8 +++++---
 scripts/git/pre-commit.git-hook | 16 +++++++++++++---
 scripts/git/pre-push.git-hook   | 18 +++++++-----------
 4 files changed, 29 insertions(+), 17 deletions(-)
 create mode 100644 changes/bug31462

diff --git a/changes/bug31462 b/changes/bug31462
new file mode 100644
index 0000000000..54ab990bb8
--- /dev/null
+++ b/changes/bug31462
@@ -0,0 +1,4 @@
+  o Minor bugfixes (git hooks):
+    - Remove a duplicate call to practracker from the pre-push hook.
+      The pre-push hook already calls the pre-commit hook, which calls
+      practracker. Fixes bug 31462; bugfix on 0.4.1.1-alpha.
diff --git a/changes/ticket30979 b/changes/ticket30979
index 8ae9b3c418..ffe1bfb4ab 100644
--- a/changes/ticket30979
+++ b/changes/ticket30979
@@ -1,5 +1,7 @@
   o Minor features (git hooks):
-    - Our pre-push git hook now checks for a special file
-      before running practracker, so that it only runs on branches
-      that are based on master.
+    - Our pre-commit git hook now checks for a special file
+      before running practracker, so that practracker only runs on branches
+      that are based on master. Since the pre-push hook calls the pre-commit
+      hook, practracker will also only run before pushes of branches based
+      on master.
       Closes ticket 30979.
diff --git a/scripts/git/pre-commit.git-hook b/scripts/git/pre-commit.git-hook
index 2a29837198..37060fdbe8 100755
--- a/scripts/git/pre-commit.git-hook
+++ b/scripts/git/pre-commit.git-hook
@@ -4,7 +4,8 @@
 # tor git repo and make sure it has permission to execute.
 #
 # This is pre-commit git hook script that prevents commiting your changeset if
-# it fails our code formatting or changelog entry formatting checkers.
+# it fails our code formatting, changelog entry formatting, module include
+# rules, or best practices tracker.
 
 workdir=$(git rev-parse --show-toplevel)
 
@@ -40,6 +41,15 @@ if test -e scripts/maint/checkIncludes.py; then
     python scripts/maint/checkIncludes.py
 fi
 
-if [ -e scripts/maint/practracker/practracker.py ]; then
-  python3 ./scripts/maint/practracker/practracker.py "$workdir"
+# Only call practracker if ${PT_DIR}/.enable_practracker_in_hooks exists
+# We do this check so that we can enable practracker in hooks in master, and
+# disable it on maint branches
+PT_DIR=scripts/maint/practracker
+
+if [ -e "${PT_DIR}/practracker.py" ]; then
+    if [ -e "${PT_DIR}/.enable_practracker_in_hooks" ]; then
+        if ! python3 "${PT_DIR}/practracker.py" "$workdir"; then
+            exit 1
+        fi
+    fi
 fi
diff --git a/scripts/git/pre-push.git-hook b/scripts/git/pre-push.git-hook
index 40a3bffa79..f4504c4215 100755
--- a/scripts/git/pre-push.git-hook
+++ b/scripts/git/pre-push.git-hook
@@ -1,10 +1,11 @@
 #!/usr/bin/env bash
 
 # git pre-push hook script to:
+# 0) Call the pre-commit hook, if it is available
 # 1) prevent "fixup!" and "squash!" commit from ending up in master, release-*
 #    or maint-*
 # 2) Disallow pushing branches other than master, release-*
-#    and maint-* to origin (e.g. gitweb.torproject.org).
+#    and maint-* to origin (e.g. gitweb.torproject.org)
 #
 # To install this script, copy it into .git/hooks/pre-push path in your
 # local copy of git repository. Make sure it has permission to execute.
@@ -21,6 +22,11 @@ z40=0000000000000000000000000000000000000000
 
 upstream_name=${TOR_UPSTREAM_REMOTE_NAME:-"upstream"}
 
+# Are you adding a new check to the git hooks?
+#  - Common checks belong in the pre-commit hook
+#  - Push-only checks belong in the pre-push hook
+#
+# Call the pre-commit hook for the common checks, if it is executable.
 workdir=$(git rev-parse --show-toplevel)
 if [ -x "$workdir/.git/hooks/pre-commit" ]; then
   if ! "$workdir"/.git/hooks/pre-commit; then
@@ -28,16 +34,6 @@ if [ -x "$workdir/.git/hooks/pre-commit" ]; then
   fi
 fi
 
-PT_DIR=scripts/maint/practracker
-
-if [ -e "${PT_DIR}/practracker.py" ]; then
-    if [ -e "${PT_DIR}/.enable_practracker_in_hooks" ]; then
-        if ! python3 "${PT_DIR}/practracker.py" "$workdir"; then
-            exit 1
-        fi
-    fi
-fi
-
 remote="$1"
 
 remote_name=$(git remote --verbose | grep "$2" | awk '{print $1}' | head -n 1)