Merge pull request #21803 from crasbe/pr/check_commits

dist/tools: add no-merge and colon check to commit-msg check

CONTRIBUTING.md

@@ -149,6 +149,10 @@ It is possible to check if your code follows these conventions:
   simple as it is.
   ```
 
+* The commit message is automatically checked by the static tests for keywords
+  that prevent a merge, for example "fixup" or "DONOTMERGE". The full list
+  of these keywords is stored in `dist/tools/pr_check/no_merge_keywords`.
+
 ### Pull Requests
 [pull requests]: #pull-requests
 
@@ -304,6 +308,9 @@ $ git commit --fixup <prefix2 commit hash>
 
 ### Squash commits after review
 
+*** Note: If the static tests warn you about a no-merge keyword, please look
+at [our commit conventions][commit-conventions].
+
 Squashing a commit is done using the rebase subcommand of git in interactive
 mode:
 
@@ -323,7 +330,7 @@ phase, squashing commits can be performed in a single command:
 $ git rebase -i --autosquash
 ```
 
-**Watch out: Don't squash your commit until a maintainer asks you to do it.**
+***Watch out: Don't squash your commit until a maintainer asks you to do it.***
 
 Otherwise the history of review changes is lost and for large PRs, it
 makes it difficult for the reviewer to follow them. It might also happen that

dist/tools/commit-msg/check.sh

@@ -6,6 +6,7 @@
 # General Public License v2.1. See the file LICENSE in the top level
 # directory for more details.
 
+# shellcheck source=/dev/null
 . "$(dirname "$0")/../ci/github_annotate.sh"
 
 github_annotate_setup
@@ -13,7 +14,7 @@ github_annotate_setup
 MSG_MAX_LENGTH=50
 MSG_STRETCH_LENGTH=72
 
-if tput colors 2> /dev/null > /dev/null && [ $(tput colors) -ge 8 ]; then
+if tput colors 2> /dev/null > /dev/null && [ "$(tput colors)" -ge 8 ]; then
     CERROR1="\033[1;31m"
     CWARN1="\033[1;33m"
     CERROR2="\033[0;31m"
@@ -31,14 +32,14 @@ if [ -n "${BASH_VERSION}" ]; then
     # properly in _escape
     ECHO_ESC='echo -e'
 else
-    ECHO='echo'
+    ECHO_ESC='echo'
 fi
 
 # If no branch but an option is given, unset BRANCH.
 # Otherwise, consume this parameter.
 BRANCH="${1}"
 if echo "${BRANCH}" | grep -q '^-'; then
-    if [ $(git rev-parse --abbrev-ref HEAD) != "master" ]; then
+    if [ "$(git rev-parse --abbrev-ref HEAD)" != "master" ]; then
         BRANCH="master"
     else
         BRANCH=""
@@ -55,13 +56,15 @@ if [ -z "${BRANCH}" ]; then
 fi
 
 ERROR="$(git log \
-    --no-merges --pretty=format:'%s' $(git merge-base ${BRANCH} HEAD)..HEAD | \
-    while read msg; do
-        msg_length=$(echo "${msg}" | awk '{print length($0)}')
+    --no-merges --pretty=format:'%s' "$(git merge-base "${BRANCH}" HEAD)"..HEAD | \
+    while read -r msg; do
+        ERROR=0
+        MSG=""
 
-        if [ ${msg_length} -gt ${MSG_MAX_LENGTH} ]; then
-            ERROR=0
-            if [ ${msg_length} -gt ${MSG_STRETCH_LENGTH} ]; then
+        # Check if the message is too long.
+        msg_length="$(echo "${msg}" | awk '{print length($0)}')"
+        if [ "${msg_length}" -gt "${MSG_MAX_LENGTH}" ]; then
+            if [ "${msg_length}" -gt "${MSG_STRETCH_LENGTH}" ]; then
                 MSG="Commit message is longer than ${MSG_STRETCH_LENGTH} characters:"
                 ERROR=1
                 echo "error"
@@ -69,6 +72,23 @@ ERROR="$(git log \
                 MSG="Commit message is longer than ${MSG_MAX_LENGTH}"
                 MSG="${MSG} (but < ${MSG_STRETCH_LENGTH}) characters:"
             fi
+        fi
+
+        # Check if the message has a colon and if the first colon is followed
+        # by a space.
+        if echo "$msg" | grep -q ":"; then
+            if ! echo "$msg" | grep -Eq '^[^:]*: '; then
+                MSG="The colon after the area designation is not followed by"
+                MSG="${MSG} a space."
+            fi
+        else
+            MSG="The commit message is missing a colon after the area"
+            MSG="${MSG} designation."
+            ERROR=1
+            echo "error"
+        fi
+
+        if [ -n "${MSG}" ]; then
             if github_annotate_is_on; then
                 if [ ${ERROR} -eq 1 ]; then
                     github_annotate_error_no_file "${MSG} \"${msg}\""

dist/tools/pr_check/check.sh

@@ -7,18 +7,26 @@
 # directory for more details.
 #
 
+# shellcheck source=/dev/null
 . "$(dirname "$0")/../ci/github_annotate.sh"
 
-: "${RIOTBASE:=$(cd $(dirname $0)/../../../; pwd)}"
-cd $RIOTBASE
+: "${RIOTBASE:="$(cd "$(dirname "$0")"/../../../ || exit; pwd)"}"
+cd "$RIOTBASE" || exit
 
 : "${RIOTTOOLS:=${RIOTBASE}/dist/tools}"
 
 EXIT_CODE=0
 
+# Keywords that should trigger the commit message check and prevent an accidental
+# merge of something not meant to be merged.
+# The pretty-print format of the commit messages is always the following:
+# `a5e4f038b8 commit message`
+# This has to be reflected in the RegEx matching pattern.
+NOMERGE_KEYWORD_FILE="$(dirname "$0")/no_merge_keywords"
+
 github_annotate_setup
 
-if tput colors &> /dev/null && [ $(tput colors) -ge 8 ]; then
+if tput colors &> /dev/null && [ "$(tput colors)" -ge 8 ]; then
     CERROR="\e[1;31m"
     CRESET="\e[0m"
 else
@@ -32,30 +40,28 @@ else
     RIOT_MASTER="master"
 fi
 
-keyword_filter() {
-    grep -i \
-        -e "^    [0-9a-f]\+ .\{0,2\}SQUASH" \
-        -e "^    [0-9a-f]\+ .\{0,2\}FIX" \
-        -e "^    [0-9a-f]\+ .\{0,2\}REMOVE *ME" \
-        -e "^    [0-9a-f]\+ .\{0,2\}Update"
-}
-
-SQUASH_COMMITS="$(git log $(git merge-base HEAD "${RIOT_MASTER}")...HEAD --pretty=format:"    %h %s" | \
-                  keyword_filter)"
+SQUASH_COMMITS="$(git log "$(git merge-base HEAD "${RIOT_MASTER}")"...HEAD --pretty=format:"%h %s" | \
+                grep -i -f "${NOMERGE_KEYWORD_FILE}")"
 
 if [ -n "${SQUASH_COMMITS}" ]; then
     if github_annotate_is_on; then
-        echo "${SQUASH_COMMITS}" | while read commit; do
-            ANNOTATION="Commit needs to be squashed: \"${commit}\""
-            ANNOTATION="${ANNOTATION}\n\nPLEASE ONLY SQUASH WHEN ASKED BY A "
+        ANNOTATION=""
+        while read -r commit; do
+            ANNOTATION="${ANNOTATION}Commit needs to be squashed or contains a no-merge keyword: \"${commit}\"\n"
+        done < <(echo "${SQUASH_COMMITS}")
+
+        if [ -n "${ANNOTATION}" ]; then
+            ANNOTATION="${ANNOTATION}\nPLEASE ONLY SQUASH WHEN ASKED BY A "
             ANNOTATION="${ANNOTATION}MAINTAINER!"
             ANNOTATION="${ANNOTATION}\nSee: "
-            ANNOTATION="${ANNOTATION}https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#squash-commits-after-review"
+            ANNOTATION="${ANNOTATION}https://github.com/RIOT-OS/RIOT/blob/master/CONTRIBUTING.md#squash-commits-after-review\n"
             github_annotate_error_no_file "${ANNOTATION}"
-        done
+        fi
     else
-        echo -e "${CERROR}Pull request needs squashing:${CRESET}" 1>&2
-        echo -e "${SQUASH_COMMITS}"
+        echo -e "${CERROR}Pull request needs squashing or contains no-merge keywords:${CRESET}" 1>&2
+        while IFS= read -r line; do
+            echo -e "    ${line}"
+        done <<< "${SQUASH_COMMITS}"
     fi
     EXIT_CODE=1
 fi

dist/tools/pr_check/no_merge_keywords

@@ -0,0 +1,12 @@
+[0-9a-f]\+ SQUASH
+[0-9a-f]\+ FIX
+[0-9a-f]\+ REMOVE *ME
+[0-9a-f]\+ Update
+[0-9a-f]\+ \<DONOTMERGE\>
+[0-9a-f]\+ \<DO NOT MERGE\>
+[0-9a-f]\+ \<DON'T MERGE\>
+[0-9a-f]\+ \<NO MERGE\>
+[0-9a-f]\+ \<DELETE ME\>
+[0-9a-f]\+ \<DELETEME\>
+[0-9a-f]\+ \<WIP\>
+[0-9a-f]\+ \<TEMP\>