View Issue Details
ID | Project | Category | View Status | Date Submitted | Last Update |
---|---|---|---|---|---|
0002540 | OpenFOAM | Bug | public | 2017-05-05 14:57 | 2017-05-08 17:00 |
Reporter | MattijsJ | Assigned To | henry | ||
Priority | none | Severity | trivial | Reproducibility | always |
Status | resolved | Resolution | fixed | ||
Platform | GNU/Linux | OS | OpenSuSE | OS Version | 13.2 |
Product Version | dev | ||||
Fixed in Version | dev | ||||
Summary | 0002540: pre-commit hook failure for UPstream.C | ||||
Description | Checking for 'NULL' in pre-commit-hook is too strict src/Pstream/mpi/UPstream.C:372: MPI_Group newGroup = MPI_GROUP_NULL | ||||
Tags | No tags attached. | ||||
|
It is not clear what you would like changed to what, could you provide a patch? |
|
Sorry, it's my fault on this one. I had made a contribution back in September, report #2267, for the pre-commit hook to catch occurrences of "NULL" that should be "nullptr". I didn't 'grep' OpenFOAM's whole source code to double check if it was OK to simply search for "NULL". Anyway, the fixed file "pre-commit-hook" is attached for replacing "bin/tools/pre-commit-hook" in OpenFOAM-dev. Below is the patch for visual check before looking into the changes: diff --git a/bin/tools/pre-commit-hook b/bin/tools/pre-commit-hook index f3a0b57..f284578 100755 --- a/bin/tools/pre-commit-hook +++ b/bin/tools/pre-commit-hook @@ -287,7 +287,7 @@ checkNonStandardCodePatterns() (*.[CH]) # Directly report the incorrect markers git grep -n --color \ - -e '> >' -e 'NULL' \ + -e '> >' -e '[=( ,-/]NULL[);, -/]' \ $scope"$f" ;; esac However, this will still trigger with the file "src/meshTools/triSurface/triSurfaceTools/geompack/geompack.C", but since that's pretty much imported code from another project, it's unlikely it will need to be changed. pre-commit-hook (9,393 bytes)
#!/bin/bash #---------------------------------*- sh -*------------------------------------- # ========= | # \\ / F ield | OpenFOAM: The Open Source CFD Toolbox # \\ / O peration | # \\ / A nd | Copyright (C) 2011-2016 OpenFOAM Foundation # \\/ M anipulation | #------------------------------------------------------------------------------ # License # This file is part of OpenFOAM. # # OpenFOAM is free software: you can redistribute it and/or modify it # under the terms of the GNU General Public License as published by # the Free Software Foundation, either version 3 of the License, or # (at your option) any later version. # # OpenFOAM is distributed in the hope that it will be useful, but WITHOUT # ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License # for more details. # # You should have received a copy of the GNU General Public License # along with OpenFOAM. If not, see <http://www.gnu.org/licenses/>. # # Script # pre-commit-hook # # Description # pre-commit hook for git. # Copy or link this file as ".git/hooks/pre-commit" # # Eg, # ( # cd $WM_PROJECT_DIR/.git/hooks && # ln -sf ../../bin/tools/pre-commit-hook pre-commit # ) # # Hook receives: empty # # Checks for # - illegal code, e.g. <TAB> # - columns greater than 80 for *.[CH] files # # Note # Using "git commit --no-verify" it is possible to override the hook. # # By supplying arguments to the hook, it can also be used to manually # test the specified files/directories for standards conformance. # #------------------------------------------------------------------------------ hookName="pre-commit" headerSeparator="-----------------------------------" die() { echo "$hookName hook failure" 1>&2 echo $headerSeparator 1>&2 echo '' 1>&2 echo "$@" 1>&2 echo '' 1>&2 exit 1 } #----------------------------------------------------------------------------- # Check content that will be added by this 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 # called manually with arguments for the files/directories to be tested? if [ "$#" -gt 0 ] then case "$1" in -h | -help) die "interactive usage: supply list of files/directories to check" ;; esac # obtain list of all specified files/directories fileList=$(git ls-files -- $@ 2>/dev/null) else # list of all files to be committed fileList=$(git diff-index --cached --name-only $against --) fi # # no files changed: can skip all the checks # this usage can correspond to a 'git commit --amend' # [ -n "$fileList" ] || exit 0 unset badFiles # join list of files with this amount of space Indent=" " # # report bad files and die if there are any # dieOnBadFiles() { if [ -n "$badFiles" ] then echo "$hookName hook failure" 1>&2 echo $headerSeparator 1>&2 echo "$@" 1>&2 echo '' 1>&2 echo "File(s):" 1>&2 echo "$badFiles" 1>&2 echo '' 1>&2 exit 1 fi } # # qualify 'git grep' to check cached value or from a specific commit # gitScope() { if [ "$#" -gt 0 ] then echo "$1:" else echo "--cached -- " fi } # # check for bad strings, characters, etc # checkIllegalCode() { echo "$hookName: check bad strings/characters etc ..." 1>&2 reBad="("$'\t'")" msgBad="<TAB>" scope=$(gitScope $@) badFiles=$( for f in $fileList do case "$f" in # exclude potential makefiles (*[Mm]akefile* | wmake/rules/* | *.f* | *.v[cf]proj | *.pdf | *.png | *.html | *.gif | *.css | *.gz) ;; (*) fileType=`file -b $f` if [ "$fileType" != "data" ] then # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -E -hn -e "$reBad" $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" fi ;; esac done ) dieOnBadFiles "Remove/correct bad '$msgBad' references" } # # limit line length to 80-columns # checkLineLength() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # limit line length to 80-columns, except C++ comment lines # checkLineLengthNonComments() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' \ --and --not -e '^ *//' \ $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # limit line length to 80-columns, except #directive lines # checkLineLengthNonDirective() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' \ --and --not -e '^ *#' \ $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # check for non-standard code patterns # checkNonStandardCodePatterns() { echo "$hookName: checking for non-standard code ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # Directly report the incorrect markers git grep -n --color \ -e '> >' -e '[=( ,-/]NULL[);, -/]' \ $scope"$f" ;; esac done ) dieOnBadFiles "$(cat<<MESSAGE Please revise the files reported below for the following non-standard code: 1. Spaced ending of multi-level template parameters are not allowed, such as: List<List<scalar> > which instead should be: List<List<scalar>> 2. The use of the 'NULL' macro should be replaced by 'nullptr' $headerSeparator MESSAGE )" } # # check that OpenFOAM Foundation copyright is current # checkCopyright() { year=$(date +%Y) echo "$hookName: check copyright ..." 1>&2 badFiles=$( for f in $fileList do startYear=`grep "Copyright.*OpenFOAM" $f | sed 's/[^0-9]*\([0-9]*\).*/\1/g'` endYear=`grep "Copyright.*-.*OpenFOAM" $f | sed 's/[^-]*-\([0-9]*\).*/\1/g'` #echo "startYear=$startYear endYear=$endYear" if [ "$startYear" != "" ] then if [ "$endYear" != "" ] then # Date is of type 2011-2012 OpenFOAM Foundation if [ "$year" != "$endYear" ] then echo "Updated copyright for: $f" 1>&2 echo "$f" sed -i -e "s/$startYear-$endYear OpenFOAM/$startYear-$year OpenFOAM/g" $f fi else # Date is of type 2011 OpenFOAM Foundation if [ "$year" != "$startYear" ] then echo "$f" echo "Updated copyright for: $f" 1>&2 sed -i -e "s/$startYear OpenFOAM/$startYear-$year OpenFOAM/g" $f fi fi fi done ) dieOnBadFiles "Some copyright dates were automatically updated; Please check these before pushing" } #------------------------------------------------------------------------------ # Main code : do all checks # # builtin whitespace check to avoid trailing space, including CR-LF endings bad=$(git diff-index --cached --check $against --) || die "$bad" # check for illegal code, e.g. <TAB>, etc checkIllegalCode # ensure code conforms to 80 columns max checkLineLengthNonDirective # check for non-standard code patterns checkNonStandardCodePatterns checkCopyright exit 0 #------------------------------------------------------------------------------ |
|
pre-commit: check bad strings/characters etc ... pre-commit: check line lengths ... pre-commit: checking for non-standard code ... Does this change work OK for you? I tried to commit it and get the error: pre-commit: check copyright ... Updated copyright for: bin/tools/pre-commit-hook sed: -e expression #1, char 6: unterminated `s' command pre-commit hook failure ----------------------------------- Some copyright dates were automatically updated; Please check these before pushing File(s): bin/tools/pre-commit-hook |
|
Grep has a "-w" option which I think does what we want. I can't get it to function in a single call with the other search pattern, but if we're happy with two git-grep-s, then the following should work: diff --git a/bin/tools/pre-commit-hook b/bin/tools/pre-commit-hook index f3a0b57a1..d3cbfa47f 100755 --- a/bin/tools/pre-commit-hook +++ b/bin/tools/pre-commit-hook @@ -286,9 +286,8 @@ checkNonStandardCodePatterns() case "$f" in (*.[CH]) # Directly report the incorrect markers - git grep -n --color \ - -e '> >' -e 'NULL' \ - $scope"$f" + git grep -n --color -e '> >' $scope$f + git grep -w -n --color -e 'NULL' $scope$f ;; esac done |
|
@Henry: Sorry, I ironically didn't check the script with itself. I'm also getting the issue with the "unterminated `s' command" and I'm looking into it now. @Will: Yes, '-w' is a lot better! 'git grep' is able to use '--and'/'--or' arguments, which I've tested just now, so that we can use a single call. But I'm currently still looking into the previous issue with sed. |
|
For the 'Copyright' search, had to restrict the search to the first occurrence. This was because the script's own lookup was finding the lookup string. Attached are the following files: - 'pre-commit-hook_v2' for replacing 'bin/tools/pre-commit-hook' - 'pre-commit-hook_v2.patch' for easier inspection of the changes made. This includes Will's suggestion of using '-w' and also by using '--or' that 'git grep' allows us to do, which the regular 'grep' doesn't have. pre-commit-hook_v2 (9,395 bytes)
#!/bin/bash #---------------------------------*- sh -*------------------------------------- # ========= | # \\ / F ield | OpenFOAM: The Open Source CFD Toolbox # \\ / O peration | # \\ / A nd | Copyright (C) 2011-2017 OpenFOAM Foundation # \\/ M anipulation | #------------------------------------------------------------------------------ # License # This file is part of OpenFOAM. # # OpenFOAM is free software: you can redistribute it and/or modify it # under the terms of the GNU General Public License as published by # the Free Software Foundation, either version 3 of the License, or # (at your option) any later version. # # OpenFOAM is distributed in the hope that it will be useful, but WITHOUT # ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or # FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License # for more details. # # You should have received a copy of the GNU General Public License # along with OpenFOAM. If not, see <http://www.gnu.org/licenses/>. # # Script # pre-commit-hook # # Description # pre-commit hook for git. # Copy or link this file as ".git/hooks/pre-commit" # # Eg, # ( # cd $WM_PROJECT_DIR/.git/hooks && # ln -sf ../../bin/tools/pre-commit-hook pre-commit # ) # # Hook receives: empty # # Checks for # - illegal code, e.g. <TAB> # - columns greater than 80 for *.[CH] files # # Note # Using "git commit --no-verify" it is possible to override the hook. # # By supplying arguments to the hook, it can also be used to manually # test the specified files/directories for standards conformance. # #------------------------------------------------------------------------------ hookName="pre-commit" headerSeparator="-----------------------------------" die() { echo "$hookName hook failure" 1>&2 echo $headerSeparator 1>&2 echo '' 1>&2 echo "$@" 1>&2 echo '' 1>&2 exit 1 } #----------------------------------------------------------------------------- # Check content that will be added by this 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 # called manually with arguments for the files/directories to be tested? if [ "$#" -gt 0 ] then case "$1" in -h | -help) die "interactive usage: supply list of files/directories to check" ;; esac # obtain list of all specified files/directories fileList=$(git ls-files -- $@ 2>/dev/null) else # list of all files to be committed fileList=$(git diff-index --cached --name-only $against --) fi # # no files changed: can skip all the checks # this usage can correspond to a 'git commit --amend' # [ -n "$fileList" ] || exit 0 unset badFiles # join list of files with this amount of space Indent=" " # # report bad files and die if there are any # dieOnBadFiles() { if [ -n "$badFiles" ] then echo "$hookName hook failure" 1>&2 echo $headerSeparator 1>&2 echo "$@" 1>&2 echo '' 1>&2 echo "File(s):" 1>&2 echo "$badFiles" 1>&2 echo '' 1>&2 exit 1 fi } # # qualify 'git grep' to check cached value or from a specific commit # gitScope() { if [ "$#" -gt 0 ] then echo "$1:" else echo "--cached -- " fi } # # check for bad strings, characters, etc # checkIllegalCode() { echo "$hookName: check bad strings/characters etc ..." 1>&2 reBad="("$'\t'")" msgBad="<TAB>" scope=$(gitScope $@) badFiles=$( for f in $fileList do case "$f" in # exclude potential makefiles (*[Mm]akefile* | wmake/rules/* | *.f* | *.v[cf]proj | *.pdf | *.png | *.html | *.gif | *.css | *.gz) ;; (*) fileType=`file -b $f` if [ "$fileType" != "data" ] then # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -E -hn -e "$reBad" $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" fi ;; esac done ) dieOnBadFiles "Remove/correct bad '$msgBad' references" } # # limit line length to 80-columns # checkLineLength() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # limit line length to 80-columns, except C++ comment lines # checkLineLengthNonComments() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' \ --and --not -e '^ *//' \ $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # limit line length to 80-columns, except #directive lines # checkLineLengthNonDirective() { echo "$hookName: check line lengths ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # parse line numbers from grep output: # <lineNr>: contents lines=$(git grep -hn -e '^.\{81,\}' \ --and --not -e '^ *#' \ $scope"$f" | sed -e 's@:.*@@' | tr '\n' ' ' ) [ -n "$lines" ] && echo "$Indent$f -- lines: $lines" ;; esac done ) dieOnBadFiles "Limit code to 80 columns before pushing" } # # check for non-standard code patterns # checkNonStandardCodePatterns() { echo "$hookName: checking for non-standard code ..." 1>&2 scope=$(gitScope $@) badFiles=$( for f in $fileList do # limit to *.[CH] files case "$f" in (*.[CH]) # Directly report the incorrect markers git grep -n --color -e '> >' \ --or -w -e 'NULL' \ $scope"$f" ;; esac done ) dieOnBadFiles "$(cat<<MESSAGE Please revise the files reported below for the following non-standard code: 1. Spaced ending of multi-level template parameters are not allowed, such as: List<List<scalar> > which instead should be: List<List<scalar>> 2. The use of the 'NULL' macro should be replaced by 'nullptr' $headerSeparator MESSAGE )" } # # check that OpenFOAM Foundation copyright is current # checkCopyright() { year=$(date +%Y) echo "$hookName: check copyright ..." 1>&2 badFiles=$( for f in $fileList do startYear=`grep -m 1 "Copyright.*OpenFOAM" $f | sed 's/[^0-9]*\([0-9]*\).*/\1/g'` endYear=`grep -m 1 "Copyright.*-.*OpenFOAM" $f | sed 's/[^-]*-\([0-9]*\).*/\1/g'` #echo "startYear=$startYear endYear=$endYear" if [ "$startYear" != "" ] then if [ "$endYear" != "" ] then # Date is of type 2011-2012 OpenFOAM Foundation if [ "$year" != "$endYear" ] then echo "Updated copyright for: $f" 1>&2 echo "$f" sed -i -e "s/$startYear-$endYear OpenFOAM/$startYear-$year OpenFOAM/g" $f fi else # Date is of type 2011 OpenFOAM Foundation if [ "$year" != "$startYear" ] then echo "$f" echo "Updated copyright for: $f" 1>&2 sed -i -e "s/$startYear OpenFOAM/$startYear-$year OpenFOAM/g" $f fi fi fi done ) dieOnBadFiles "Some copyright dates were automatically updated; Please check these before pushing" } #------------------------------------------------------------------------------ # Main code : do all checks # # builtin whitespace check to avoid trailing space, including CR-LF endings bad=$(git diff-index --cached --check $against --) || die "$bad" # check for illegal code, e.g. <TAB>, etc checkIllegalCode # ensure code conforms to 80 columns max checkLineLengthNonDirective # check for non-standard code patterns checkNonStandardCodePatterns checkCopyright exit 0 #------------------------------------------------------------------------------ |
|
pre-commit-hook_v2.patch (1,477 bytes)
diff --git a/bin/tools/pre-commit-hook b/bin/tools/pre-commit-hook index f3a0b57..1549eed 100755 --- a/bin/tools/pre-commit-hook +++ b/bin/tools/pre-commit-hook @@ -3,7 +3,7 @@ # ========= | # \\ / F ield | OpenFOAM: The Open Source CFD Toolbox # \\ / O peration | -# \\ / A nd | Copyright (C) 2011-2016 OpenFOAM Foundation +# \\ / A nd | Copyright (C) 2011-2017 OpenFOAM Foundation # \\/ M anipulation | #------------------------------------------------------------------------------ # License @@ -286,8 +286,8 @@ checkNonStandardCodePatterns() case "$f" in (*.[CH]) # Directly report the incorrect markers - git grep -n --color \ - -e '> >' -e 'NULL' \ + git grep -n --color -e '> >' \ + --or -w -e 'NULL' \ $scope"$f" ;; esac @@ -324,8 +324,8 @@ checkCopyright() badFiles=$( for f in $fileList do - startYear=`grep "Copyright.*OpenFOAM" $f | sed 's/[^0-9]*\([0-9]*\).*/\1/g'` - endYear=`grep "Copyright.*-.*OpenFOAM" $f | sed 's/[^-]*-\([0-9]*\).*/\1/g'` + startYear=`grep -m 1 "Copyright.*OpenFOAM" $f | sed 's/[^0-9]*\([0-9]*\).*/\1/g'` + endYear=`grep -m 1 "Copyright.*-.*OpenFOAM" $f | sed 's/[^-]*-\([0-9]*\).*/\1/g'` #echo "startYear=$startYear endYear=$endYear" if [ "$startYear" != "" ] then |
|
Thanks Bruno. Resolved by commit 31862aeb6fcca00884fd1c7847662dd180655774 |
Date Modified | Username | Field | Change |
---|---|---|---|
2017-05-05 14:57 | MattijsJ | New Issue | |
2017-05-05 15:45 | henry | Note Added: 0008103 | |
2017-05-06 21:08 | wyldckat | File Added: pre-commit-hook | |
2017-05-06 21:08 | wyldckat | Note Added: 0008106 | |
2017-05-06 21:08 | wyldckat | Assigned To | => henry |
2017-05-06 21:08 | wyldckat | Status | new => assigned |
2017-05-07 09:24 | henry | Note Added: 0008108 | |
2017-05-07 11:01 | will | Note Added: 0008114 | |
2017-05-07 11:18 | wyldckat | Note Added: 0008115 | |
2017-05-07 11:36 | wyldckat | File Added: pre-commit-hook_v2 | |
2017-05-07 11:36 | wyldckat | Note Added: 0008116 | |
2017-05-07 11:36 | wyldckat | File Added: pre-commit-hook_v2.patch | |
2017-05-08 17:00 | henry | Status | assigned => resolved |
2017-05-08 17:00 | henry | Resolution | open => fixed |
2017-05-08 17:00 | henry | Fixed in Version | => dev |
2017-05-08 17:00 | henry | Note Added: 0008120 |