| Message ID | 20220420233148.2384208-1-heiko@ist.eigentlich.net |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [Openvpn-devel] Add git pre-commit hook script to uncrustify | expand |
Definitive NACK due to licensing concern mentioned below. > Heiko Hund <heiko@ist.eigentlich.net> hat am 21.04.2022 01:31 geschrieben: > > > The script is self installing if you call it with "install" as the first > parameter. Once installed as the pre-commit hook it will check files to > be committed according to the rules in uncrustify.conf and abort the > commit if there's formatting issues. The script produces a patch in /tmp > which can be git apply'ed to fix all issues found. > > The script was originally authored by David Martin [1] and slightly > modified to fit our needs. At the time it had a 2-clause BSD license. > > [1] https://github.com/ddddavidmartin/Pre-commit-hooks > > Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> > --- > dev-tools/git-pre-commit-uncrustify.sh | 146 +++++++++++++++++++++++++ > 1 file changed, 146 insertions(+) > create mode 100644 dev-tools/git-pre-commit-uncrustify.sh > > diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-uncrustify.sh > new file mode 100644 > index 00000000..64e5e396 > --- /dev/null > +++ b/dev-tools/git-pre-commit-uncrustify.sh > @@ -0,0 +1,146 @@ > +#!/bin/sh > + > +# git pre-commit hook that runs an Uncrustify stylecheck. > +# Features: > +# - abort commit when commit does not comply with the style guidelines > +# - create a patch of the proposed style changes > +# > +# More info on Uncrustify: http://uncrustify.sourceforge.net/ > + > +# This file was taken from a set of unofficial pre-commit hooks available > +# at https://github.com/ddddavidmartin/Pre-commit-hooks and modified to > +# fit the openvpn project's needs The BSD-2 license requires keeping the original copyright and license statement. While the original pre-commit-uncrustify file had none of those, there is a LICENSE file included in that repo. So I think you need to either copy that LICENSE file and reference it here or just paste it completely into this file. > +# exit on error > +set -e > + > + > +# If called so, install this script as pre-commit hook > +if [ "$1" = "install" ] ; then > + ROOTDIR=$(git rev-parse --show-toplevel) > + HOOKSDIR="$ROOTDIR/.git/hooks" Actually, the correct way to find the actual hooks directory is git rev-parse --git-path hooks That takes all configuration like $GIT_DIR and core.hooksDir into account. > + TARGET="$HOOKSDIR/pre-commit" > + > + if [ -e "$TARGET" ] ; then > + printf "$TARGET file exists. Won't overwrite.\n" Any reason we're using printf here instead of echo? The latter would avoid the ugly line endings. [...] > +# create a filename to store our generated patch > +prefix="ovpn-fmt" > +suffix="$(date +%C%y%m%d%H%M%S)" > +patch="/tmp/$prefix-$suffix.patch" Are insecure tmp files still a concern? Because this is definitely an insecure tmp file... [...] Regards, -- Frank Lichtenheld
Acked-by: Gert Doering <gert@greenie.muc.de> I've minimally tested this - clone the repo - run "sh dev-tools/git-pre-commit-uncrustify.sh install" - edit a file, introduce some of our typical things (tab, c++ comment) - try to commit, get a refusal message, with diff, plus pointer-to-patch - run "git apply $thatpatch" - errors fixed, commit applies .. and it does exactly what it says on the lid. Great stuff. Your patch has been applied to the master branch. commit 01424c932115a1b159f304e97a2dead254dc4c6d Author: Heiko Hund Date: Thu Apr 21 01:31:48 2022 +0200 Add git pre-commit hook script to uncrustify Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> Acked-by: Gert Doering <gert@greenie.muc.de> Message-Id: <20220420233148.2384208-1-heiko@ist.eigentlich.net> URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg24124.html Signed-off-by: Gert Doering <gert@greenie.muc.de> -- kind regards, Gert Doering
Hi, On Thu, Apr 21, 2022 at 10:01:29AM +0200, Frank Lichtenheld wrote: > Definitive NACK due to licensing concern mentioned below. Streams crossed here, but I saw your NAK before pushing. So, yes, all valid concerns and I withdraw my ACK. > [...] > > +# create a filename to store our generated patch > > +prefix="ovpn-fmt" > > +suffix="$(date +%C%y%m%d%H%M%S)" > > +patch="/tmp/$prefix-$suffix.patch" > > Are insecure tmp files still a concern? Because this is definitely an insecure tmp file... Especially this one is something I only noticed after I sent my mail - so this definitely needs to to mktemp-style temp files, or a private tmp directory (in the local repo?) gert
Hi all, Did you consider to use the pre-commit framework [1] written in Python? There is a maintained hook for uncrustify [2]. This would allow it to easily integrate other linters/checks. See the list of supported hooks [3]. I would understand if you don't like to add yet another dependency, but in the case of Python, it's already part of the development environment. [1] https://pre-commit.com/ [2] https://github.com/pocc/pre-commit-hooks [3] https://pre-commit.com/hooks.html David On Thu, 21 Apr 2022 at 10:17, Gert Doering <gert@greenie.muc.de> wrote: > > Hi, > > On Thu, Apr 21, 2022 at 10:01:29AM +0200, Frank Lichtenheld wrote: > > Definitive NACK due to licensing concern mentioned below. > > Streams crossed here, but I saw your NAK before pushing. > > So, yes, all valid concerns and I withdraw my ACK. > > > [...] > > > +# create a filename to store our generated patch > > > +prefix="ovpn-fmt" > > > +suffix="$(date +%C%y%m%d%H%M%S)" > > > +patch="/tmp/$prefix-$suffix.patch" > > > > Are insecure tmp files still a concern? Because this is definitely an insecure tmp file... > > Especially this one is something I only noticed after I sent my mail - > so this definitely needs to to mktemp-style temp files, or a private > tmp directory (in the local repo?) > > gert > -- > "If was one thing all people took for granted, was conviction that if you > feed honest figures into a computer, honest figures come out. Never doubted > it myself till I met a computer with a sense of humor." > Robert A. Heinlein, The Moon is a Harsh Mistress > > Gert Doering - Munich, Germany gert@greenie.muc.de > _______________________________________________ > Openvpn-devel mailing list > Openvpn-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/openvpn-devel
Hi On Donnerstag, 21. April 2022 10:16:03 CEST Gert Doering wrote: > On Thu, Apr 21, 2022 at 10:01:29AM +0200, Frank Lichtenheld wrote: > > Definitive NACK due to licensing concern mentioned below. > > Streams crossed here, but I saw your NAK before pushing. > So, yes, all valid concerns and I withdraw my ACK. Will post a v2 with the license inline . > > > +# create a filename to store our generated patch > > > +prefix="ovpn-fmt" > > > +suffix="$(date +%C%y%m%d%H%M%S)" > > > +patch="/tmp/$prefix-$suffix.patch" > > > > Are insecure tmp files still a concern? Because this is definitely an > > insecure tmp file... > Especially this one is something I only noticed after I sent my mail - > so this definitely needs to to mktemp-style temp files, or a private > tmp directory (in the local repo?) I don't really see the need for secure tmp files here, as there's no chance to exploit anything by racing for the file(name). Or am I just not creative enough? Cheers, Heiko
Hi David On Donnerstag, 21. April 2022 13:41:58 CEST David Schneider wrote: > Did you consider to use the pre-commit framework [1] written in > Python? There is a maintained hook for uncrustify [2]. > This would allow it to easily integrate other linters/checks. See the > list of supported hooks [3]. I stumbled across this, but it seemed too big for the purpose. Was looking for something small that could integrate into the dev-tools. That said, I don't think it holds anyone back from using the pre-commit stuff with the repo's uncrustify.conf. So, if you already do use it please feel free to add some short howto to the developer documentation (something I missed doing, but will add in v2). Heiko
Hi Frank On Donnerstag, 21. April 2022 10:01:29 CEST Frank Lichtenheld wrote: > > +# If called so, install this script as pre-commit hook > > +if [ "$1" = "install" ] ; then > > + ROOTDIR=$(git rev-parse --show-toplevel) > > + HOOKSDIR="$ROOTDIR/.git/hooks" > > Actually, the correct way to find the actual hooks directory is > git rev-parse --git-path hooks > That takes all configuration like $GIT_DIR and core.hooksDir into account. Nice one, will add in v2. > > + TARGET="$HOOKSDIR/pre-commit" > > + > > + if [ -e "$TARGET" ] ; then > > + printf "$TARGET file exists. Won't overwrite.\n" > > Any reason we're using printf here instead of echo? The latter > would avoid the ugly line endings. Consistency. The script uses printf everywhere. Heiko
Hi, On Thu, Apr 21, 2022 at 02:41:18PM +0200, Heiko Hund wrote: > > Especially this one is something I only noticed after I sent my mail - > > so this definitely needs to to mktemp-style temp files, or a private > > tmp directory (in the local repo?) > > I don't really see the need for secure tmp files here, as there's no chance to > exploit anything by racing for the file(name). Or am I just not creative > enough? The classic approach is "create a symlink from /tmp/$known_name to something you want overwritten", and on next "git commit", some file owned by the committer gets overwritten (or created as empty) Since an attacker can not control the content *and* the comitter won't do this as root (at least I hope so) *and* you actually need to have different users on the machine, the risk is comparatively low, but still, this is a code construct that should be avoided. gert
diff --git a/dev-tools/git-pre-commit-uncrustify.sh b/dev-tools/git-pre-commit-uncrustify.sh new file mode 100644 index 00000000..64e5e396 --- /dev/null +++ b/dev-tools/git-pre-commit-uncrustify.sh @@ -0,0 +1,146 @@ +#!/bin/sh + +# git pre-commit hook that runs an Uncrustify stylecheck. +# Features: +# - abort commit when commit does not comply with the style guidelines +# - create a patch of the proposed style changes +# +# More info on Uncrustify: http://uncrustify.sourceforge.net/ + +# This file was taken from a set of unofficial pre-commit hooks available +# at https://github.com/ddddavidmartin/Pre-commit-hooks and modified to +# fit the openvpn project's needs + +# exit on error +set -e + + +# If called so, install this script as pre-commit hook +if [ "$1" = "install" ] ; then + ROOTDIR=$(git rev-parse --show-toplevel) + HOOKSDIR="$ROOTDIR/.git/hooks" + TARGET="$HOOKSDIR/pre-commit" + + if [ -e "$TARGET" ] ; then + printf "$TARGET file exists. Won't overwrite.\n" + printf "Aborting installation.\n" + exit 1 + fi + + read -p "Install as $TARGET? [y/N] " INPUT + [ "$INPUT" = "y" ] || exit 0 + cp "$0" "$TARGET" + chmod +x $TARGET + exit 0 +fi + +# check whether the given file matches any of the set extensions +matches_extension() { + local filename="$(basename -- "$1")" + local extension=".${filename##*.}" + local ext + + for ext in .c .h ; do [ "$ext" = "$extension" ] && return 0; done + + return 1 +} + +# necessary check for initial 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 + +UNCRUSTIFY=$(command -v uncrustify) +UNCRUST_CONFIG="$PWD/dev-tools/uncrustify.conf" + +# make sure the config file and executable are correctly set +if [ ! -f "$UNCRUST_CONFIG" ] ; then + printf "Error: uncrustify config file not found.\n" + printf "Expected to find it at $UNCRUST_CONFIG.\n" + printf "Aborting commit.\n" + exit 1 +fi + +if [ -z "$UNCRUSTIFY" ] ; then + printf "Error: uncrustify executable not found.\n" + printf "Is it installed and in your \$PATH?\n" + printf "Aborting commit.\n" + exit 1 +fi + +# create a filename to store our generated patch +prefix="ovpn-fmt" +suffix="$(date +%C%y%m%d%H%M%S)" +patch="/tmp/$prefix-$suffix.patch" + +# create one patch containing all changes to the files +# sed to remove quotes around the filename, if inserted by the system +# (done sometimes, if the filename contains special characters, like the quote itself) +git diff-index --cached --diff-filter=ACMR --name-only $against -- | \ +sed -e 's/^"\(.*\)"$/\1/' | \ +while read file +do + # ignore file if we do check for file extensions and the file + # does not match any of the extensions specified in $FILE_EXTS + if ! matches_extension "$file"; then + continue; + fi + + # escape special characters in the source filename: + # - '\': backslash needs to be escaped + # - '*': used as matching string => '*' would mean expansion + # (curiously, '?' must not be escaped) + # - '[': used as matching string => '[' would mean start of set + # - '|': used as sed split char instead of '/', so it needs to be escaped + # in the filename + # printf %s particularly important if the filename contains the % character + file_escaped_source=$(printf "%s" "$file" | sed -e 's/[\*[|]/\\&/g') + + # escape special characters in the target filename: + # phase 1 (characters escaped in the output diff): + # - '\': backslash needs to be escaped in the output diff + # - '"': quote needs to be escaped in the output diff if present inside + # of the filename, as it used to bracket the entire filename part + # phase 2 (characters escaped in the match replacement): + # - '\': backslash needs to be escaped again for sed itself + # (i.e. double escaping after phase 1) + # - '&': would expand to matched string + # - '|': used as sed split char instead of '/' + # printf %s particularly important if the filename contains the % character + file_escaped_target=$(printf "%s" "$file" | sed -e 's/[\"]/\\&/g' -e 's/[\&|]/\\&/g') + + # uncrustify our sourcefile, create a patch with diff and append it to our $patch + # The sed call is necessary to transform the patch from + # --- $file timestamp + # +++ - timestamp + # to both lines working on the same file and having a a/ and b/ prefix. + # Else it can not be applied with 'git apply'. + "$UNCRUSTIFY" -q -c "$UNCRUST_CONFIG" -f "$file" | \ + diff -u -- "$file" - | \ + sed -e "1s|--- $file_escaped_source|--- \"a/$file_escaped_target\"|" -e "2s|+++ -|+++ \"b/$file_escaped_target\"|" >> "$patch" +done + +# if no patch has been generated all is ok, clean up the file stub and exit +if [ ! -s "$patch" ] ; then + rm -f "$patch" + exit 0 +fi + +# a patch has been created, notify the user and exit +printf "Formatting of some code does not follow the project guidelines.\n" + +if [ $(wc -l < $patch) -gt 80 ] ; then + printf "The file $patch contains the necessary fixes.\n" +else + printf "Here's the patch that fixes the formatting:\n\n" + cat $patch +fi + +printf "\nYou can apply these changes with:\n git apply $patch\n" +printf "(from the root directory of the repository) and then commit again.\n" +printf "\nAborting commit.\n" + +exit 1
The script is self installing if you call it with "install" as the first parameter. Once installed as the pre-commit hook it will check files to be committed according to the rules in uncrustify.conf and abort the commit if there's formatting issues. The script produces a patch in /tmp which can be git apply'ed to fix all issues found. The script was originally authored by David Martin [1] and slightly modified to fit our needs. At the time it had a 2-clause BSD license. [1] https://github.com/ddddavidmartin/Pre-commit-hooks Signed-off-by: Heiko Hund <heiko@ist.eigentlich.net> --- dev-tools/git-pre-commit-uncrustify.sh | 146 +++++++++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 dev-tools/git-pre-commit-uncrustify.sh