{"thread":{"id":"41368","subject":"[PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","startedAt":"2016-02-08T08:59:18Z","lastAt":"2016-02-09T23:14:29Z","messageCount":10,"participants":["larsxschneider@gmail.com","Jeff King","Lars Schneider","Junio C Hamano","Stefan Beller","Ramsay Jones","Roberto Tyley"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"277715","messageId":"1454921958-53129-1-git-send-email-larsxschneider@gmail.com","threadId":"41368","inReplyTo":null,"subject":"[PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-08T08:59:18Z","receivedAt":"2016-02-08T08:59:18Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"From: Lars Schneider <larsxschneider@gmail.com>\n\nThe global Travis-CI environment variable CFLAGS did not override the\nCFLAGS variable in the makefile. Pass CFLAGS as make variable to\noverride it properly.\n\nIn addition to that, add '-Wdeclaration-after-statement' to make a\nTravis-CI build fail (because of '-Werror') if the code does not adhere\nto the Git coding style.\n\nInspired-by: Jeff King <peff@peff.net>\nSigned-off-by: Lars Schneider <larsxschneider@gmail.com>\n---\n\nI made this patch because Peff pointed out to me that \"git style doesn't\nallow declaration-after-statement\" [1]. I wonder if it would make sense\nto add this check even in the makefile [2]? I am no make expert, but I\nalso wonder why we don't use the override directive [3] for the CFLAGS?\nAFAIK this would allow a make invocation like this:\n\nmake target CFLAGS+=-Wdeclaration-after-statement\n\nThanks,\nLars\n\n[1] http://www.spinics.net/lists/git/msg267273.html\n[2] https://github.com/git/git/blob/ff4ea6004fb48146330d663d64a71e7774f059f9/Makefile#L377\n[3] https://www.gnu.org/software/make/manual/make.html#Override-Directive\n\n .travis.yml | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/.travis.yml b/.travis.yml\nindex c3bf9c6..29abff4 100644\n--- a/.travis.yml\n+++ b/.travis.yml\n@@ -20,7 +20,7 @@ env:\n     - DEFAULT_TEST_TARGET=prove\n     - GIT_PROVE_OPTS=\"--timer --jobs 3\"\n     - GIT_TEST_OPTS=\"--verbose --tee\"\n-    - CFLAGS=\"-g -O2 -Wall -Werror\"\n+    - CFLAGS=\"-g -O2 -Wall -Werror -Wdeclaration-after-statement\"\n     - GIT_TEST_CLONE_2GB=YesPlease\n     # t9810 occasionally fails on Travis CI OS X\n     # t9816 occasionally fails with \"TAP out of sequence errors\" on Travis CI OS X\n@@ -68,7 +68,7 @@ before_install:\n     echo \"$(tput setaf 6)Git-LFS Version$(tput sgr0)\";\n     git-lfs version;\n\n-before_script: make --jobs=2\n+before_script: make CFLAGS=\"$CFLAGS\" --jobs=2\n\n script: make --quiet test\n\n--\n2.5.1\n"},{"id":"277724","messageId":"20160208122551.GD24217@sigill.intra.peff.net","threadId":"41368","inReplyTo":"1454921958-53129-1-git-send-email-larsxschneider@gmail.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-08T12:25:51Z","receivedAt":"2016-02-08T12:25:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@gmail.com wrote:\n\n> From: Lars Schneider <larsxschneider@gmail.com>\n> \n> The global Travis-CI environment variable CFLAGS did not override the\n> CFLAGS variable in the makefile. Pass CFLAGS as make variable to\n> override it properly.\n\nMakes sense.\n\n> In addition to that, add '-Wdeclaration-after-statement' to make a\n> Travis-CI build fail (because of '-Werror') if the code does not adhere\n> to the Git coding style.\n\nI think this is a good step, but is probably the tip of the iceberg. I\ntypically use:\n\n  -Wall -Werror -Wdeclaration-after-statement\n  -Wpointer-arith -Wstrict-prototypes -Wvla\n  -Wold-style-declaration -Wold-style-definition\n\nJunio does his integration testing with a similar set, which I think you\ncan find in one of the scripts in his \"todo\" branch.\n\n> I made this patch because Peff pointed out to me that \"git style doesn't\n> allow declaration-after-statement\" [1]. I wonder if it would make sense\n> to add this check even in the makefile [2]?\n\nI think we keep the default CFLAGS to a pretty tame set, so that we\nbuild out of the box on a large number of compilers. I know I have run\ninto problems with -Wold-style-* on clang (though perhaps that is no\nlonger the case, as I haven't tried recently), let alone truly antique\ncompilers.\n\nI have, however, wondered if it would make sense to codify this\nknowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1\nroughly mean \"you are a git dev, you have a reasonably modern compiler,\nand want to be as careful as possible before sending your patches\".\n\n> I am no make expert, but I\n> also wonder why we don't use the override directive [3] for the CFLAGS?\n> AFAIK this would allow a make invocation like this:\n> \n> make target CFLAGS+=-Wdeclaration-after-statement\n\nI think it is rather the opposite. If we used:\n\n  override CFLAGS+= ...\n\nin the Makefile, then if a user did:\n\n  make CFLAGS=...\n\nwe would add to their CFLAGS (whereas without the override, our\nappending is ignored). Our Makefile solves that in a different way,\nthough. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made\nup of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the\nformer, and we set the latter based on Makefile knobs (e.g., NO_CURL,\netc).\n\n-Peff\n"},{"id":"277797","messageId":"9FBB5AFB-D44C-43A1-A0C5-A5A45675A122@gmail.com","threadId":"41368","inReplyTo":"20160208122551.GD24217@sigill.intra.peff.net","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Lars Schneider","fromEmail":"larsxschneider@gmail.com","sentAt":"2016-02-09T10:06:17Z","receivedAt":"2016-02-09T10:06:17Z","isPatch":true,"sender":{"key":"larsxschneider@gmail.com","avatar":"https://avatars.githubusercontent.com/u/477434?v=4"},"body":"\nOn 08 Feb 2016, at 13:25, Jeff King <peff@peff.net> wrote:\n\n> On Mon, Feb 08, 2016 at 09:59:18AM +0100, larsxschneider@gmail.com wrote:\n> \n>> From: Lars Schneider <larsxschneider@gmail.com>\n>> \n>> The global Travis-CI environment variable CFLAGS did not override the\n>> CFLAGS variable in the makefile. Pass CFLAGS as make variable to\n>> override it properly.\n> \n> Makes sense.\n> \n>> In addition to that, add '-Wdeclaration-after-statement' to make a\n>> Travis-CI build fail (because of '-Werror') if the code does not adhere\n>> to the Git coding style.\n> \n> I think this is a good step, but is probably the tip of the iceberg. I\n> typically use:\n> \n>  -Wall -Werror -Wdeclaration-after-statement\n>  -Wpointer-arith -Wstrict-prototypes -Wvla\n>  -Wold-style-declaration -Wold-style-definition\n> \n> Junio does his integration testing with a similar set, which I think you\n> can find in one of the scripts in his \"todo\" branch.\n\nI collected the warnings from Junio's Make [1] script and merged them with \nyours. This is the resulting warning list for clang and gcc:\n\n-Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition \n-Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla\n\nGcc processes one extra warning that clang doesn't like:\n-Wold-style-declaration\n\nThe build runs clean with all these checks enabled.\n\n[1] https://github.com/git/git/blob/todo/Make\n\n\n>> I made this patch because Peff pointed out to me that \"git style doesn't\n>> allow declaration-after-statement\" [1]. I wonder if it would make sense\n>> to add this check even in the makefile [2]?\n> \n> I think we keep the default CFLAGS to a pretty tame set, so that we\n> build out of the box on a large number of compilers. I know I have run\n> into problems with -Wold-style-* on clang (though perhaps that is no\n> longer the case, as I haven't tried recently), let alone truly antique\n> compilers.\n> \n> I have, however, wondered if it would make sense to codify this\n> knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1\n> roughly mean \"you are a git dev, you have a reasonably modern compiler,\n> and want to be as careful as possible before sending your patches\".\n\nThat make sense. However, in \"development mode\" I don't like Werror.\nHow about two knobs? One called DEVELOPER which enables all the warnings\nabove and one called CONTINUOUS_INTEGRATION that enables Werror\nin addition?\n\n\n>> I am no make expert, but I\n>> also wonder why we don't use the override directive [3] for the CFLAGS?\n>> AFAIK this would allow a make invocation like this:\n>> \n>> make target CFLAGS+=-Wdeclaration-after-statement\n> \n> I think it is rather the opposite. If we used:\n> \n>  override CFLAGS+= ...\n> \n> in the Makefile, then if a user did:\n> \n>  make CFLAGS=...\n> \n> we would add to their CFLAGS (whereas without the override, our\n> appending is ignored). Our Makefile solves that in a different way,\n> though. We pass $(ALL_CFLAGS) to the compiler, and that in turn is made\n> up of $(CFLAGS) and $(BASIC_CFLAGS). We assume the user tweaks the\n> former, and we set the latter based on Makefile knobs (e.g., NO_CURL,\n> etc).\nI see. Thanks for the explanation.\n\nRegarding CI checks:\n\nJeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]\nwhere I found checkpatch.pl [3]. Would it make sense to check all commits\nthat are not in next/master/maint with this script on Travis-CI?\n\nStefan Beller recently mentioned \"Adhere to the common coding style of \nGit\" [4] where he removed explicit NULL checks. This kind of stuff could be\nchecked automatically with checkpatch.pl as far as I can see.\n\n[2] http://www.spinics.net/lists/git/msg267445.html\n[3] https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl\n[4] http://permalink.gmane.org/gmane.comp.version-control.git/280842\n\nThanks,\nLars\n"},{"id":"277805","messageId":"20160209173622.GA28699@sigill.intra.peff.net","threadId":"41368","inReplyTo":"9FBB5AFB-D44C-43A1-A0C5-A5A45675A122@gmail.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-02-09T17:36:22Z","receivedAt":"2016-02-09T17:36:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 09, 2016 at 11:06:17AM +0100, Lars Schneider wrote:\n\n> I collected the warnings from Junio's Make [1] script and merged them with \n> yours. This is the resulting warning list for clang and gcc:\n> \n> -Wdeclaration-after-statement -Wno-format-zero-length -Wold-style-definition \n> -Woverflow -Wpointer-arith -Wstrict-prototypes -Wunused -Wvla\n\nSounds right.\n\n> > I have, however, wondered if it would make sense to codify this\n> > knowledge in the Makefile with an optional knob. E.g., let DEVELOPER=1\n> > roughly mean \"you are a git dev, you have a reasonably modern compiler,\n> > and want to be as careful as possible before sending your patches\".\n> \n> That make sense. However, in \"development mode\" I don't like Werror.\n> How about two knobs? One called DEVELOPER which enables all the warnings\n> above and one called CONTINUOUS_INTEGRATION that enables Werror\n> in addition?\n\nI'm curious why you don't want -Werror. Junio is going to compile the\nresult of applying your patch with it, so it makes sense to me to catch\nproblems as early as possible.\n\nAnd while there are certainly false positives from gcc's warnings, we\nwork to squelch them whether -Werror is in effect or not. So IMHO,\n-Werror is mostly about making sure you _see_ the warnings and don't\nlose them in a flood of compilation messages.\n\n> Regarding CI checks:\n> \n> Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]\n> where I found checkpatch.pl [3]. Would it make sense to check all commits\n> that are not in next/master/maint with this script on Travis-CI?\n> \n> Stefan Beller recently mentioned \"Adhere to the common coding style of \n> Git\" [4] where he removed explicit NULL checks. This kind of stuff could be\n> checked automatically with checkpatch.pl as far as I can see.\n\nPerhaps. I'm not sure that people actually use checkpatch.pl for git.\nOut of curiosity, I tried:\n\n  mkdir out\n  git format-patch -o out v2.6.0..v2.7.0\n  checkpatch.pl out/*\n\nIt's rather noisy, and after skimming, I'd say (subjectively) that only\na small fraction are actual style issues we try to enforce. So it would\ncertainly need a fair bit of tweaking for regular use, I think.\n\n-Peff\n"},{"id":"277807","messageId":"xmqqlh6t4vya.fsf@gitster.mtv.corp.google.com","threadId":"41368","inReplyTo":"20160209173622.GA28699@sigill.intra.peff.net","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T17:47:41Z","receivedAt":"2016-02-09T17:47:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Perhaps. I'm not sure that people actually use checkpatch.pl for git.\n>\n> Out of curiosity, I tried:\n>\n>   mkdir out\n>   git format-patch -o out v2.6.0..v2.7.0\n>   checkpatch.pl out/*\n>\n> It's rather noisy, and after skimming, I'd say (subjectively) that only\n> a small fraction are actual style issues we try to enforce. So it would\n> certainly need a fair bit of tweaking for regular use, I think.\n>\n> -Peff\n\nFWIW, I use the attached (it assumes a recent kernel checkout at\ncertain location and checkpatch-original.pl being a symlink to it)\noccasionally.  I found patches from some people are consistently\nclean and suspect they may be running checkpatch themselves.\n\n-- >8 -- Meta/CP (not on 'todo' branch) -- >8 --\n#!/bin/sh\n\n# Run checkpatch on the series\nMeta=$(git rev-parse --show-cdup)Meta\ncp0=\"$Meta/checkpatch-original.pl\"\ncp1=\"$Meta/checkpatch.pl\"\ntmp=\"/var/tmp/CP.$$\"\n\nmkdir \"$tmp\" || exit\ntrap 'rm -fr \"$tmp\"' 0\n\nif\t! test -f \"$cp1\" ||\n\ttest \"$cp0\" -nt \"$cp1\"\nthen\n\tcat \"$cp0\" >\"$cp1\" &&\n\t(cd \"$Meta\" &&\npatch -p1 <<\\EOF\n--- a/checkpatch.pl\n+++ b/checkpatch.pl\n@@ -282,6 +282,8 @@\n \tReviewed-by:|\n \tReported-by:|\n \tSuggested-by:|\n+\tHelped-by:|\n+\tMentored-by:|\n \tTo:|\n \tCc:\n )};\n@@ -2338,7 +2340,7 @@\n \n # check for new typedefs, only function parameters and sparse annotations\n # make sense.\n-\t\tif ($line =~ /\\btypedef\\s/ &&\n+\t\tif (0 && $line =~ /\\btypedef\\s/ &&\n \t\t    $line !~ /\\btypedef\\s+$Type\\s*\\(\\s*\\*?$Ident\\s*\\)\\s*\\(/ &&\n \t\t    $line !~ /\\btypedef\\s+$Type\\s+$Ident\\s*\\(/ &&\n \t\t    $line !~ /\\b$typeTypedefs\\b/ &&\n@@ -2607,8 +2609,7 @@\n \n \t\t\t\t# No spaces for:\n \t\t\t\t#   ->\n-\t\t\t\t#   :   when part of a bitfield\n-\t\t\t\t} elsif ($op eq '->' || $opv eq ':B') {\n+\t\t\t\t} elsif ($op eq '->') {\n \t\t\t\t\tif ($ctx =~ /Wx.|.xW/) {\n \t\t\t\t\t\tERROR(\"SPACING\",\n \t\t\t\t\t\t      \"spaces prohibited around that '$op' $at\\n\" . $hereptr);\n\nEOF\n)\nfi || exit\n\ncat \"$@\" | git mailsplit -b -o\"$tmp\" >/dev/null\n\nfor mail in \"$tmp\"/*\ndo\n\t(\n\t\tgit mailinfo -k \"$mail.msg\" \"$mail.patch\" >\"$mail.info\" <\"$mail\"\n\t\techo\n\t\tcat \"$mail.msg\"\n\t\tprintf \"%s\\n\" -- \"---\"\n\t\tcat \"$mail.patch\"\n\t) >\"$mail.mbox\"\n\tperl \"$Meta/checkpatch.pl\" $ignore --no-tree --max-line-length=120 \"$mail.mbox\" || {\n\t\tgrep \"Subject: \" \"$mail.info\"\n\t\tprintf \"%s\\n\" -- \"------------------------------------------------\"\n\t}\ndone\n"},{"id":"277812","messageId":"xmqq4mdh4tel.fsf@gitster.mtv.corp.google.com","threadId":"41368","inReplyTo":"9FBB5AFB-D44C-43A1-A0C5-A5A45675A122@gmail.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T18:42:42Z","receivedAt":"2016-02-09T18:42:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Lars Schneider <larsxschneider@gmail.com> writes:\n\n> Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]\n> where I found checkpatch.pl [3]. Would it make sense to check all commits\n> that are not in next/master/maint with this script on Travis-CI?\n\nThat does not help very much.  These changes are already shown to\npeople and dirtied their eyes, and most likely I've already have\nwasted time tweaking the glitches out locally.  The damage has\nalready been done.\n\nIt would make a lot of sense if the checkpatch is called inside\nRoberto Tyley's \"pull-request-to-patch-submission\" thing, though.\n"},{"id":"277813","messageId":"CAGZ79kb=rkRnP1SnMLtyNkn8DeCAtqoDriYc7051u-Z9isKZoQ@mail.gmail.com","threadId":"41368","inReplyTo":"xmqq4mdh4tel.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-02-09T18:46:04Z","receivedAt":"2016-02-09T18:46:04Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Feb 9, 2016 at 10:42 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Lars Schneider <larsxschneider@gmail.com> writes:\n>\n>> Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]\n>> where I found checkpatch.pl [3]. Would it make sense to check all commits\n>> that are not in next/master/maint with this script on Travis-CI?\n>\n> That does not help very much.  These changes are already shown to\n> people and dirtied their eyes, and most likely I've already have\n> wasted time tweaking the glitches out locally.  The damage has\n> already been done.\n>\n> It would make a lot of sense if the checkpatch is called inside\n> Roberto Tyley's \"pull-request-to-patch-submission\" thing, though.\n\nLast time I reported a bug/feature request, Roberto was glad to be\ncc'd as he doesn't read the mailing list in full. So I cc'd him here.\n"},{"id":"277821","messageId":"56BA514A.8040209@ramsayjones.plus.com","threadId":"41368","inReplyTo":"xmqqlh6t4vya.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2016-02-09T20:51:22Z","receivedAt":"2016-02-09T20:51:22Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 09/02/16 17:47, Junio C Hamano wrote:\n> Jeff King <peff@peff.net> writes:\n> \n>> Perhaps. I'm not sure that people actually use checkpatch.pl for git.\n>>\n>> Out of curiosity, I tried:\n>>\n>>   mkdir out\n>>   git format-patch -o out v2.6.0..v2.7.0\n>>   checkpatch.pl out/*\n>>\n>> It's rather noisy, and after skimming, I'd say (subjectively) that only\n>> a small fraction are actual style issues we try to enforce. So it would\n>> certainly need a fair bit of tweaking for regular use, I think.\n>>\n>> -Peff\n> \n> FWIW, I use the attached (it assumes a recent kernel checkout at\n> certain location and checkpatch-original.pl being a symlink to it)\n> occasionally.  I found patches from some people are consistently\n> clean and suspect they may be running checkpatch themselves.\n> \n> -- >8 -- Meta/CP (not on 'todo' branch) -- >8 --\n> #!/bin/sh\n> \n> # Run checkpatch on the series\n> Meta=$(git rev-parse --show-cdup)Meta\n> cp0=\"$Meta/checkpatch-original.pl\"\n> cp1=\"$Meta/checkpatch.pl\"\n> tmp=\"/var/tmp/CP.$$\"\n> \n> mkdir \"$tmp\" || exit\n> trap 'rm -fr \"$tmp\"' 0\n> \n> if\t! test -f \"$cp1\" ||\n> \ttest \"$cp0\" -nt \"$cp1\"\n> then\n> \tcat \"$cp0\" >\"$cp1\" &&\n> \t(cd \"$Meta\" &&\n> patch -p1 <<\\EOF\n> --- a/checkpatch.pl\n> +++ b/checkpatch.pl\n> @@ -282,6 +282,8 @@\n>  \tReviewed-by:|\n>  \tReported-by:|\n>  \tSuggested-by:|\n> +\tHelped-by:|\n> +\tMentored-by:|\n>  \tTo:|\n>  \tCc:\n>  )};\n> @@ -2338,7 +2340,7 @@\n>  \n>  # check for new typedefs, only function parameters and sparse annotations\n>  # make sense.\n> -\t\tif ($line =~ /\\btypedef\\s/ &&\n> +\t\tif (0 && $line =~ /\\btypedef\\s/ &&\n>  \t\t    $line !~ /\\btypedef\\s+$Type\\s*\\(\\s*\\*?$Ident\\s*\\)\\s*\\(/ &&\n>  \t\t    $line !~ /\\btypedef\\s+$Type\\s+$Ident\\s*\\(/ &&\n>  \t\t    $line !~ /\\b$typeTypedefs\\b/ &&\n> @@ -2607,8 +2609,7 @@\n>  \n>  \t\t\t\t# No spaces for:\n>  \t\t\t\t#   ->\n> -\t\t\t\t#   :   when part of a bitfield\n> -\t\t\t\t} elsif ($op eq '->' || $opv eq ':B') {\n> +\t\t\t\t} elsif ($op eq '->') {\n>  \t\t\t\t\tif ($ctx =~ /Wx.|.xW/) {\n>  \t\t\t\t\t\tERROR(\"SPACING\",\n>  \t\t\t\t\t\t      \"spaces prohibited around that '$op' $at\\n\" . $hereptr);\n> \n> EOF\n> )\n> fi || exit\n> \n> cat \"$@\" | git mailsplit -b -o\"$tmp\" >/dev/null\n> \n> for mail in \"$tmp\"/*\n> do\n> \t(\n> \t\tgit mailinfo -k \"$mail.msg\" \"$mail.patch\" >\"$mail.info\" <\"$mail\"\n> \t\techo\n> \t\tcat \"$mail.msg\"\n> \t\tprintf \"%s\\n\" -- \"---\"\n> \t\tcat \"$mail.patch\"\n> \t) >\"$mail.mbox\"\n> \tperl \"$Meta/checkpatch.pl\" $ignore --no-tree --max-line-length=120 \"$mail.mbox\" || {\n> \t\tgrep \"Subject: \" \"$mail.info\"\n> \t\tprintf \"%s\\n\" -- \"------------------------------------------------\"\n> \t}\n> done\n\nI have used checkpatch on some small (new) projects from a 'style'\ntarget in the makefile. (Checking both *.c and *.h)\n\nIf we try something similar on git, (while on current pu branch):\n\n$ git diff\ndiff --git a/Makefile b/Makefile\nindex fd80e94..3d6f1d8 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2247,6 +2247,14 @@ test-%$X: test-%.o GIT-LDFLAGS $(GITLIBS)\n check-sha1:: test-sha1$X\n        ./test-sha1.sh\n \n+ST_OBJ = $(patsubst %.o,%.st,$(C_OBJ))\n+\n+$(ST_OBJ): %.st: %.c GIT-CFLAGS FORCE\n+       checkpatch.pl --no-tree --show-types --ignore=NEW_TYPEDEFS -f $<\n+\n+.PHONY: style $(ST_OBJ)\n+style: $(ST_OBJ)\n+\n SP_OBJ = $(patsubst %.o,%.sp,$(C_OBJ))\n \n $(SP_OBJ): %.sp: %.c GIT-CFLAGS FORCE\n$ make -k style >zzz 2>&1\n$ grep \"^WARNING:\" zzz | wc -l\n7320\n$ grep \"^ERROR:\" zzz | wc -l\n1155\n$ \n\nTo try and summarize a little:\n\n$ grep '^WARNING:' zzz | sort | uniq -c\n      1 WARNING:AVOID_EXTERNS: externs should be avoided in .c files\n     18 WARNING:BRACES: braces {} are not necessary for any arm of this statement\n     82 WARNING:BRACES: braces {} are not necessary for single statement blocks\n      5 WARNING:CVS_KEYWORD: CVS style keyword markers, these will _not_ be updated\n     23 WARNING:DEEP_INDENTATION: Too many leading tabs - consider code refactoring\n      2 WARNING:DEFAULT_NO_BREAK: switch default: should use break\n     33 WARNING:INDENTED_LABEL: labels should not be indented\n    539 WARNING:LEADING_SPACE: please, no spaces at the start of a line\n      1 WARNING:LINE_CONTINUATIONS: Avoid unnecessary line continuations\n   3422 WARNING:LONG_LINE: line over 80 characters\n     14 WARNING:MISSING_BREAK: Possible switch case/default not preceeded by break or fallthrough comment\n      1 WARNING:ONE_SEMICOLON: Statements terminations use 1 semicolon\n     19 WARNING:PREFER_PRINTF: __printf(string-index, first-to-check) is preferred over __attribute__((format(printf, string-index, first-to-check)))\n      7 WARNING:QUOTED_WHITESPACE_BEFORE_NEWLINE: unnecessary whitespace before a quoted newline\n     17 WARNING:RETURN_VOID: void function return statements are not generally useful\n      4 WARNING:SINGLE_STATEMENT_DO_WHILE_MACRO: Single statement macros should not use a do {} while (0) loop\n      2 WARNING:SIZEOF_PARENTHESIS: sizeof msg should be sizeof(msg)\n      1 WARNING:SIZEOF_PARENTHESIS: sizeof opts should be sizeof(opts)\n      3 WARNING:SIZEOF_PARENTHESIS: sizeof sa should be sizeof(sa)\n      2 WARNING:SIZEOF_PARENTHESIS: sizeof sin should be sizeof(sin)\n      1 WARNING:SIZEOF_PARENTHESIS: sizeof timeout_buf should be sizeof(timeout_buf)\n     15 WARNING:SPACE_BEFORE_TAB: please, no space before tabs\n   2401 WARNING:SPACING: Missing a blank line after declarations\n      1 WARNING:SPACING: space prohibited before semicolon\n    180 WARNING:SPACING: space prohibited between function name and open parenthesis '('\n      6 WARNING:SPACING: Unnecessary space before function pointer arguments\n    280 WARNING:SPLIT_STRING: quoted string split across lines\n     51 WARNING:STATIC_CONST_CHAR_ARRAY: char * array declaration might be better as static const\n      3 WARNING:STATIC_CONST_CHAR_ARRAY: static char array declaration should probably be static const char\n     37 WARNING:STATIC_CONST_CHAR_ARRAY: static const char * array should probably be static const char * const\n     11 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (10, 12)\n      9 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (10, 14)\n      9 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (12, 14)\n      6 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (14, 18)\n      1 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 16)\n      1 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 18)\n      2 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (16, 20)\n      4 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (18, 20)\n      3 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (18, 22)\n     35 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (2, 4)\n      3 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (24, 28)\n      1 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (24, 30)\n      2 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (24, 31)\n     21 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (2, 6)\n      2 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (32, 36)\n      3 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (4, 6)\n     17 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (6, 10)\n      5 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 10)\n      8 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 12)\n      1 WARNING:SUSPECT_CODE_INDENT: suspect code indent for conditional statements (8, 15)\n      1 WARNING:TRAILING_SEMICOLON: macros should not use a trailing semicolon\n      4 WARNING:VOLATILE: Use of volatile is usually wrong: see Documentation/volatile-considered-harmful.txt\n$ \n\nHmm, I'm a little surprised by 2401 'Missing a blank line after declarations'! ;-)\n\n$ grep '^ERROR:' zzz | sort | uniq -c\n    181 ERROR:ASSIGN_IN_IF: do not use assignment in if condition\n     42 ERROR:CODE_INDENT: code indent should use tabs where possible\n      6 ERROR:COMPLEX_MACRO: Macros with complex values should be enclosed in parenthesis\n    266 ERROR:ELSE_AFTER_BRACE: else should follow close brace '}'\n     50 ERROR:INITIALISED_STATIC: do not initialise statics to 0 or NULL\n      1 ERROR:OPEN_BRACE: open brace '{' following enum go on the same line\n     37 ERROR:OPEN_BRACE: open brace '{' following function declarations go on the next line\n      6 ERROR:OPEN_BRACE: open brace '{' following struct go on the same line\n      1 ERROR:OPEN_BRACE: open brace '{' following union go on the same line\n     83 ERROR:OPEN_BRACE: that open brace { should be on the previous line\n     16 ERROR:POINTER_LOCATION: \"foo * bar\" should be \"foo *bar\"\n     10 ERROR:POINTER_LOCATION: \"foo* bar\" should be \"foo *bar\"\n      1 ERROR:POINTER_LOCATION: \"foo** bar\" should be \"foo **bar\"\n      1 ERROR:POINTER_LOCATION: \"foo * const * bar\" should be \"foo * const *bar\"\n     42 ERROR:POINTER_LOCATION: \"(foo*)\" should be \"(foo *)\"\n     10 ERROR:POINTER_LOCATION: \"(foo**)\" should be \"(foo **)\"\n      6 ERROR:RETURN_PARENTHESES: return is not a function, parentheses are not required\n      2 ERROR:SPACING: need consistent spacing around '-' (ctx:WxV)\n      1 ERROR:SPACING: need consistent spacing around '*' (ctx:WxV)\n     25 ERROR:SPACING: need consistent spacing around '+' (ctx:WxV)\n      5 ERROR:SPACING: space prohibited after that '!' (ctx:BxW)\n      2 ERROR:SPACING: space prohibited after that '!' (ctx:ExW)\n      1 ERROR:SPACING: space prohibited after that '~' (ctx:WxW)\n      1 ERROR:SPACING: space prohibited after that '*' (ctx:WxW)\n     59 ERROR:SPACING: space prohibited after that open parenthesis '('\n      2 ERROR:SPACING: space prohibited after that open square bracket '['\n     36 ERROR:SPACING: space prohibited before that close parenthesis ')'\n      2 ERROR:SPACING: space required after that close brace '}'\n      1 ERROR:SPACING: space required after that ';' (ctx:BxV)\n      1 ERROR:SPACING: space required after that ',' (ctx:OxV)\n      4 ERROR:SPACING: space required after that ',' (ctx:VxO)\n     58 ERROR:SPACING: space required after that ',' (ctx:VxV)\n      1 ERROR:SPACING: space required after that ';' (ctx:WxV)\n      3 ERROR:SPACING: space required before that '-' (ctx:OxV)\n      1 ERROR:SPACING: space required before that '&' (ctx:OxV)\n      1 ERROR:SPACING: space required before that '*' (ctx:VxB)\n      2 ERROR:SPACING: space required before that '*' (ctx:VxV)\n     26 ERROR:SPACING: space required before the open parenthesis '('\n      4 ERROR:SPACING: spaces prohibited around that '->' (ctx:WxW)\n      1 ERROR:SPACING: spaces required around that '=' (ctx:VxE)\n      3 ERROR:SPACING: spaces required around that '==' (ctx:VxV)\n     11 ERROR:SPACING: spaces required around that '=' (ctx:VxV)\n      4 ERROR:SPACING: spaces required around that '>' (ctx:VxV)\n      2 ERROR:SPACING: spaces required around that '=' (ctx:VxW)\n     20 ERROR:SPACING: spaces required around that ':' (ctx:VxW)\n      1 ERROR:SPACING: spaces required around that '<=' (ctx:WxV)\n      1 ERROR:SPACING: spaces required around that '!=' (ctx:WxV)\n      6 ERROR:SWITCH_CASE_INDENT_LEVEL: switch and case should be at the same indent\n    105 ERROR:TRAILING_STATEMENTS: trailing statements should be on next line\n      4 ERROR:TRAILING_WHITESPACE: trailing whitespace\n$ \n\nI don't know if that helps! :-D\n\nATB,\nRamsay Jones\n"},{"id":"277846","messageId":"CAFY1edbCSPZg892=pVQ=TDo6PRhUdees36Jp7LQ2YB3_jQd4uQ@mail.gmail.com","threadId":"41368","inReplyTo":"xmqq4mdh4tel.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Roberto Tyley","fromEmail":"roberto.tyley@gmail.com","sentAt":"2016-02-09T23:03:40Z","receivedAt":"2016-02-09T23:03:40Z","isPatch":true,"sender":{"key":"roberto.tyley@gmail.com","avatar":"https://avatars.githubusercontent.com/u/52038?v=4"},"body":"On 9 February 2016 at 18:42, Junio C Hamano <gitster@pobox.com> wrote:\n> Lars Schneider <larsxschneider@gmail.com> writes:\n>> Jeff Merkey made me aware of http://kernelnewbies.org/FirstKernelPatch [2]\n>> where I found checkpatch.pl [3]. Would it make sense to check all commits\n>> that are not in next/master/maint with this script on Travis-CI?\n>\n> That does not help very much.  These changes are already shown to\n> people and dirtied their eyes, and most likely I've already have\n> wasted time tweaking the glitches out locally.  The damage has\n> already been done.\n>\n> It would make a lot of sense if the checkpatch is called inside\n> Roberto Tyley's \"pull-request-to-patch-submission\" thing, though.\n\nI've not personally run checkpatch.pl (as Peff mentioned, it's not\nactually a documented part of the Git project's recommend contribution\nworkflow) - I'm still trying to understand whether it will restrict\nit's errors to just the things that are introduced in a patch, or if\nit will indiscriminately mention existing problems too (of which I\nguess there are many already present in the live Git codebase?). If it\nmentions _existing_ problems, I wouldn't personally want it in any\nautomated flow until it can be tuned to find the trees of master/maint\ntotally clean. At that point it could be added to the Travis build,\nand GitHub would automatically reflect the Travis status in any\ngit/git PR.\n\nI like the idea of giving helpful guidance to users on how to make\ntheir patches cleaner - I'm not that enthusiastic about submitGit\ninvoking the checkpatch.pl script directly at this point, given that\nit lives in a separate project (the linux kernel) and the version\nJunio uses is patched off _that_ - I'm lazy enough to not want to try\nto get that all to work reliably on a little transient Heroku box.\n"},{"id":"277847","messageId":"xmqq7fid1noq.fsf@gitster.mtv.corp.google.com","threadId":"41368","inReplyTo":"CAFY1edbCSPZg892=pVQ=TDo6PRhUdees36Jp7LQ2YB3_jQd4uQ@mail.gmail.com","subject":"Re: [PATCH v1] travis-ci: override CFLAGS properly, add -Wdeclaration-after-statement","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-02-09T23:14:29Z","receivedAt":"2016-02-09T23:14:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Roberto Tyley <roberto.tyley@gmail.com> writes:\n\n> I've not personally run checkpatch.pl (as Peff mentioned, it's not\n> actually a documented part of the Git project's recommend contribution\n> workflow) - I'm still trying to understand whether it will restrict\n> it's errors to just the things that are introduced in a patch,\n\nYes, it works as a filter to pass a submitted patch through to catch\nissues on the new lines.  You can obviously diff the current state\nwith an empty tree and feed the result to it, which would give you\nall the existing issues ;-) but that is not very useful.\n"}]}