Git hooks

[fjpanag]

CI some times gets overwhelmed by commits. In some cases there are commits that are just a waste since they are doomed to fail for trivial reasons (e.g. style errors). (I myself am guilty of doing this, sorry...)

We faced the same problem where I work, and I came up with a solution that reduced such bad commits. This is a proposition, in case the same solution can be seen as advantageous in NuttX.

I used git hooks to check the code, before it is committed, or pushed. Using the git hooks, you can specify a script to be run for specific git actions. This script can check the code in a similar fashion as the CI does, but locally.

Commits that are "bad" for trivial reasons are filtered out quickly, without wasting CI time. New developers can get the ropes quicker, as they see the result of their changes immediately. Force pushes are reduced, and the actual code to be reviewed is of better quality.

Of course, these checks have to be very simple and quick, or else they are just a nuisance and defeat the purpose of CI.
We currently use this for style errors and spell checking. Indeed nxstyle is very fast, and seems a good candidate for such a pre-commit check. Other use cases is to check for filenames validity, filter-out malicious code, ensure no secrets are to be leaked etc etc.

---

The thing with git hooks, however, is that they are not part of the repository, so they cannot be "cloned" as the code is.
They have to be installed manually for every clone of the repo.

This means that anyone has the option to install them or not, or have a different version of the script etc. It still is up to everyone's discretion to actually use them (correctly) or not, just as it is to use nxstyle.

To "force" everyone to use the hooks, and ensure that they are always up-to-date, I used our "workspace configuration script". Every time that you configure your workspace, the script is installed automatically by a simple ln. The actual script is stored in the repository so it is under version control.

NuttX has a similar script: tools/configure.sh. It can be used in a similar fashion. Every time this is called (which is in fact necessary for everyone working on the code), it can install the hook script automatically. And then everyone can have their code checked while they are working, without extra steps (that can be forgotten, or omitted).

[davids5]

@fjpanag - these are all good ideas. They were discussed when the project joined Apache, but before most of CI and GH usage was known to all. Now that we have had time in this environment. We should discuss and reconsider adding the hooks and support scripts.

Here is my take: I agree that yes we should have a hooks and install script: The install script runs once on 'configure or make' and you can opt-out. If not it installs the hooks. The commits hook should give the user an option to ignore and commit anyway.

[fjpanag]

Just to be clear. I don't mean hooks to substitute (part of) the CI. Just to complement it.

The commits hook should give the user an option to ignore and commit anyway.

Hooks provide this out of the box.
In my case I can run:

$ git config hooks.ignorestyle true

My script contains:

skipstyle=$(git config --bool hooks.ignorestyle)

if [ "$skipstyle" != "true" ]
then
# do the style check
fi

So I can skip specific checks at will.

Another use case would be to "encourage" a behavior instead of "forcing" it.
For example, if the commit message has to have a specific format, the hook may provide a warning, instead of having a hard fail in CI.

[davids5]

FWIW this is what I have in my nuttx repo - it could you some work, but the way it works is what I am referring to.

#!/bin/sh
#
# Copyright (c) 2012 - 2019, PX4 Development Team
# All rights reserved.
#
# Redistribution and use in source and binary forms, with or without
# modification, are permitted provided that the following conditions are met:
#
# * Redistributions of source code must retain the above copyright notice, this
#  list of conditions and the following disclaimer.
# 
# * Redistributions in binary form must reproduce the above copyright notice,
#  this list of conditions and the following disclaimer in the documentation
#   and/or other materials provided with the distribution.
# 
# * Neither the name of the copyright holder nor the names of its
#   contributors may be used to endorse or promote products derived from
#  this software without specific prior written permission.
# 
# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
# AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE
# DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE
# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
# SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
# CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
# OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
# OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
# 
#
# An example hook script to verify what is about to be committed.
# Called by "git commit" with no arguments.  The hook should
# # exit with non-zero status after issuing an appropriate message if
# it wants to stop the commit.
#
# To enable this hook, rename this file to "pre-commit".

if git rev-parse --verify HEAD >/dev/null 2>&1
then
    against=HEAD
else
    # Initial commit: diff against an empty tree object
    against=4b825dc642cb6eb9a060e54bf8d69288fbee4904
fi

# If you want to allow non-ascii filenames set this variable to true.
allownonascii=$(git config hooks.allownonascii)

# Redirect output to stderr.
exec 1>&2

# Cross platform projects tend to avoid non-ascii filenames; prevent
# them from being added to the repository. We exploit the fact that the
# printable range starts at the space character and ends with tilde.
if [ "$allownonascii" != "true" ] &&
    # Note that the use of brackets around a tr range is ok here, (it's
    # even required, for portability to Solaris 10's /usr/bin/tr), since
    # the square bracket bytes happen to fall in the designated range.
    test $(git diff --cached --name-only --diff-filter=A -z $against |
      LC_ALL=C tr -d '[ -~]\0' | wc -c) != 0
then
    echo "Error: Attempt to add a non-ascii file name."
    echo
    echo "This can cause problems if you want to work"
    echo "with people on other platforms."
    echo
    echo "To be portable it is advisable to rename the file ..."
    echo
    echo "If you know what you are doing you can disable this"
    echo "check using:"
    echo
    echo "  git config hooks.allownonascii true"
    echo
    exit 1
fi

# If there are whitespace errors, print the offending file names and fail.
git diff-index --check --cached $against --
if [ $? -ne 0 ]
then
        exit 1
fi

# Check for code style, only in changed files
for i in `git diff --cached --name-only --diff-filter=ACM`
do
    echo $i:
        FILENAME=$(basename $i)
        FILE_EXT="${FILENAME#*.}"
    echo FILENAME=$FILENAME
    echo FILE_EXT=$FILE_EXT
        if [ "$FILE_EXT" = "c" ] || [ "$FILE_EXT" = "h" ]; then
        ./tools/nxstyle -m 80 $i 
    fi
    echo 
done
exec < /dev/tty
while true; do
    read -p "[pre-commit hook] Commit? (Y/n) " yn
    if [ "$yn" = "" ]; then
       yn='Y'
    fi
    case $yn in
          [Yy] ) exit 0; break;;
          [Nn] ) exit 1;;
          * ) echo "Please answer y or n for yes or no.";;
      esac
    done

[fjpanag]

That's a much better idea.

Does this work for IDEs or GUI git clients? Or only using CLI?

[xiaoxiang781216]

checkpatch.sh is designed to work with git hook in mind, one line is enough in your pre-commit: git diff --cached | ./tools/checkpatch.sh - This trick even print from checkpatch.sh usage:

USAGE: ../../nuttx/tools/checkpatch.sh [options] [list|-]

Options:
-h
-c spell check with codespell(install with: pip install codespell)
-r range check only (coupled with -p or -g)
-p <patch file names> (default)
-g <commit list>
-f <file list>
-  read standard input mainly used by git pre-commit hook as below:
   git diff --cached | ./tools/checkpatch.sh -
Where a <commit list> is any syntax supported by git for specifying git revision, see GITREVISIONS(7)
Where a <patch file names> is a space separated list of patch file names or wildcard. or *.patch

We even dissuss before to add an install opton in checkpatch.sh to generate pre-commit automatically.

[fjpanag]

Closed via #9159.