{"thread":{"id":"48939","subject":"[PATCH 0/2] travis-ci: fail if Coccinelle found something to transform","startedAt":"2018-07-23T13:02:48Z","lastAt":"2018-07-26T16:49:35Z","messageCount":9,"participants":["SZEDER Gábor","Derrick Stolee","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"353337","messageId":"20180723130230.22491-1-szeder.dev@gmail.com","threadId":"48939","inReplyTo":null,"subject":"[PATCH 0/2] travis-ci: fail if Coccinelle found something to transform","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:02:28Z","receivedAt":"2018-07-23T13:02:48Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"The first patch makes the static analysis build job on Travis CI\nfaster by running it with 'make -j2'.\n\nThe second patch makes it more more useful by failing the build job if\nCoccinelle finds something to transform, thereby drawing our attention\nto undesired code patterns trying to enter the codebase.\n\n\nWith these patches applied, the static analysis build job would fail\non current 'pu' because of two small issues on two branches:\n\n - js/range-diff: Dscho has sent out v4 of this series over the\n weekend, which already incorporates Coccinelle's suggestion, so\n it's basically done.\n \n - pb/bisect-helper-2: this topic has not seen an update in about 9\n months, so I'll send a followup patch 3/2 to be applied on top or\n squashed in, whichever is deemed better.\n\n\nSZEDER Gábor (2):\n travis-ci: run Coccinelle static analysis with two parallel jobs\n travis-ci: fail if Coccinelle static analysis found something to\n transform\n\n ci/run-static-analysis.sh | 21 ++++++++++++++++++++-\n 1 file changed, 20 insertions(+), 1 deletion(-)\n\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353338","messageId":"20180723130230.22491-2-szeder.dev@gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-1-szeder.dev@gmail.com","subject":"[PATCH 1/2] travis-ci: run Coccinelle static analysis with two parallel jobs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:02:29Z","receivedAt":"2018-07-23T13:02:54Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Currently the static analysis build job runs Coccinelle using a single\n'make' job. Using two parallel jobs cuts down the build job's run\ntime from around 10-12mins to 6-7mins, sometimes even under 6mins\n(there is quite large variation between build job runtimes). More\nthan two parallel jobs don't seem to bring further runtime benefits.\n\nSigned-off-by: SZEDER Gábor \n---\n ci/run-static-analysis.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex fe4ee4e06b..fa719c9ef9 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -5,6 +5,6 @@\n \n . ${0%/*}/lib-travisci.sh\n \n-make coccicheck\n+make --jobs=2 coccicheck\n \n save_good_tree\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353339","messageId":"20180723130230.22491-3-szeder.dev@gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-1-szeder.dev@gmail.com","subject":"[PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:02:30Z","receivedAt":"2018-07-23T13:03:00Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"Coccinelle's and in turn 'make coccicheck's exit code only indicates\nthat Coccinelle managed to finish its analysis without any errors\n(e.g. no unknown --options, no missing files, no syntax errors in the\nsemantic patches, etc.), but it doesn't indicate whether it found any\nundesired code patterns to transform or not. To find out the latter,\none has to look closer at 'make coccicheck's standard output and look\nfor lines like:\n\n SPATCH result: contrib/coccinelle/.cocci.patch\n\nAnd this only indicates that there is something to transform, but to\nsee what the suggested transformations are one has to actually look\ninto those '*.cocci.patch' files.\n\nThis makes the automated static analysis build job on Travis CI not\nparticularly useful, because it neither draws our attention to\nCoccinelle's findings, nor shows the actual findings. Consequently,\nnew topics introducing undesired code patterns graduated to master\non several occasions without anyone noticing.\n\nThe only way to draw attention in such an automated setting is to fail\nthe build job. Therefore, modify the 'ci/run-static-analysis.sh'\nbuild script to check all the resulting '*.cocci.patch' files, and\nfail the build job if any of them turns out to be not empty. Include\nthose files' contents, i.e. Coccinelle's suggested transformations, in\nthe build job's trace log, so we'll know why it failed.\n\nSigned-off-by: SZEDER Gábor \n---\n ci/run-static-analysis.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\nindex fa719c9ef9..5688f261d0 100755\n--- a/ci/run-static-analysis.sh\n+++ b/ci/run-static-analysis.sh\n@@ -7,4 +7,23 @@\n \n make --jobs=2 coccicheck\n \n+set +x\n+\n+fail=\n+for cocci_patch in contrib/coccinelle/*.patch\n+do\n+\tif test -s \"$cocci_patch\"\n+\tthen\n+\t\techo \"$(tput setaf 1)Coccinelle suggests the following changes in '$cocci_patch':$(tput sgr0)\"\n+\t\tcat \"$cocci_patch\"\n+\t\tfail=UnfortunatelyYes\n+\tfi\n+done\n+\n+if test -n \"$fail\"\n+then\n+\techo \"$(tput setaf 1)error: Coccinelle suggested some changes$(tput sgr0)\"\n+\texit 1\n+fi\n+\n save_good_tree\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353340","messageId":"20180723130603.22703-1-szeder.dev@gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-1-szeder.dev@gmail.com","subject":"[PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-23T13:06:03Z","receivedAt":"2018-07-23T13:06:19Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"bisect--helper: use oid_to_hex()\n\nSigned-off-by: SZEDER Gábor \n---\n builtin/bisect--helper.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex fc02f889e6..eac4c27787 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n \t\tif (!get_oid(head, &head_oid) &&\n \t\t !starts_with(head, \"refs/heads/\")) {\n \t\t\tstrbuf_reset(&start_head);\n-\t\t\tstrbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));\n+\t\t\tstrbuf_addstr(&start_head, oid_to_hex(&head_oid));\n \t\t} else if (!get_oid(head, &head_oid) &&\n \t\t\t skip_prefix(head, \"refs/heads/\", &head)) {\n \t\t\t/*\n-- \n2.18.0.408.g42635c01bc\n\n"},{"id":"353342","messageId":"2266ffb0-b4b6-26bd-9ea8-76aaa325bcc6@gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T13:23:34Z","receivedAt":"2018-07-23T13:23:40Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:02 AM, SZEDER Gábor wrote:\n> Coccinelle's and in turn 'make coccicheck's exit code only indicates\n> that Coccinelle managed to finish its analysis without any errors\n> (e.g. no unknown --options, no missing files, no syntax errors in the\n> semantic patches, etc.), but it doesn't indicate whether it found any\n> undesired code patterns to transform or not. To find out the latter,\n> one has to look closer at 'make coccicheck's standard output and look\n> for lines like:\n>\n> SPATCH result: contrib/coccinelle/.cocci.patch\n>\n> And this only indicates that there is something to transform, but to\n> see what the suggested transformations are one has to actually look\n> into those '*.cocci.patch' files.\n>\n> This makes the automated static analysis build job on Travis CI not\n> particularly useful, because it neither draws our attention to\n> Coccinelle's findings, nor shows the actual findings. Consequently,\n> new topics introducing undesired code patterns graduated to master\n> on several occasions without anyone noticing.\n>\n> The only way to draw attention in such an automated setting is to fail\n> the build job. Therefore, modify the 'ci/run-static-analysis.sh'\n> build script to check all the resulting '*.cocci.patch' files, and\n> fail the build job if any of them turns out to be not empty. Include\n> those files' contents, i.e. Coccinelle's suggested transformations, in\n> the build job's trace log, so we'll know why it failed.\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> ci/run-static-analysis.sh | 19 +++++++++++++++++++\n> 1 file changed, 19 insertions(+)\n>\n> diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh\n> index fa719c9ef9..5688f261d0 100755\n> --- a/ci/run-static-analysis.sh\n> +++ b/ci/run-static-analysis.sh\n> @@ -7,4 +7,23 @@\n> \n> make --jobs=2 coccicheck\n> \n> +set +x\n> +\n> +fail=\n> +for cocci_patch in contrib/coccinelle/*.patch\n> +do\n> +\tif test -s \"$cocci_patch\"\n> +\tthen\n> +\t\techo \"$(tput setaf 1)Coccinelle suggests the following changes in '$cocci_patch':$(tput sgr0)\"\n> +\t\tcat \"$cocci_patch\"\n> +\t\tfail=UnfortunatelyYes\n> +\tfi\n> +done\n> +\n> +if test -n \"$fail\"\n> +then\n> +\techo \"$(tput setaf 1)error: Coccinelle suggested some changes$(tput sgr0)\"\n> +\texit 1\n> +fi\n> +\n> save_good_tree\n\nThis looks like the right way to report a failure. It provides as much \ninformation as possible. I agree that we should have this as part of CI \nso we find issues more quickly.\n\nThanks!\n\n-Stolee\n\n"},{"id":"353343","messageId":"bcecc802-099e-7e7f-d977-e6f545ff2fee@gmail.com","threadId":"48939","inReplyTo":"20180723130603.22703-1-szeder.dev@gmail.com","subject":"Re: [PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T13:24:03Z","receivedAt":"2018-07-23T13:24:13Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:06 AM, SZEDER Gábor wrote:\n> bisect--helper: use oid_to_hex()\n>\n> Signed-off-by: SZEDER Gábor \n> ---\n> builtin/bisect--helper.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index fc02f889e6..eac4c27787 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,\n> \t\tif (!get_oid(head, &head_oid) &&\n> \t\t !starts_with(head, \"refs/heads/\")) {\n> \t\t\tstrbuf_reset(&start_head);\n> -\t\t\tstrbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));\n> +\t\t\tstrbuf_addstr(&start_head, oid_to_hex(&head_oid));\n> \t\t} else if (!get_oid(head, &head_oid) &&\n> \t\t\t skip_prefix(head, \"refs/heads/\", &head)) {\n> \t\t\t/*\n\nThis patch looks obviously correct. Thanks!\n\n-Stolee\n\n"},{"id":"353344","messageId":"583d2499-c4f2-47a1-48b0-6beb2d4d9d9f@gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-1-szeder.dev@gmail.com","subject":"Re: [PATCH 0/2] travis-ci: fail if Coccinelle found something to transform","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-07-23T13:25:11Z","receivedAt":"2018-07-23T13:25:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2018 9:02 AM, SZEDER Gábor wrote:\n> The first patch makes the static analysis build job on Travis CI\n> faster by running it with 'make -j2'.\n>\n> The second patch makes it more more useful by failing the build job if\n> Coccinelle finds something to transform, thereby drawing our attention\n> to undesired code patterns trying to enter the codebase.\n>\n>\n> With these patches applied, the static analysis build job would fail\n> on current 'pu' because of two small issues on two branches:\n>\n> - js/range-diff: Dscho has sent out v4 of this series over the\n> weekend, which already incorporates Coccinelle's suggestion, so\n> it's basically done.\n> \n> - pb/bisect-helper-2: this topic has not seen an update in about 9\n> months, so I'll send a followup patch 3/2 to be applied on top or\n> squashed in, whichever is deemed better.\n>\n>\n> SZEDER Gábor (2):\n> travis-ci: run Coccinelle static analysis with two parallel jobs\n> travis-ci: fail if Coccinelle static analysis found something to\n> transform\n>\n> ci/run-static-analysis.sh | 21 ++++++++++++++++++++-\n> 1 file changed, 20 insertions(+), 1 deletion(-)\n\nThanks, Szeder.\n\nI agree that we should use all possible automation as part of CI to \ncatch things early. I like the direction here.\n\nReviewed-by: Derrick Stolee \n\n"},{"id":"353621","messageId":"CAM0VKjmqwByERhPZLqZfg6JNEEeMkKQU1XuD93aW+3yfc2gucA@mail.gmail.com","threadId":"48939","inReplyTo":"20180723130230.22491-3-szeder.dev@gmail.com","subject":"Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-26T09:08:28Z","receivedAt":"2018-07-26T09:08:43Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor wrote:\n\n> The only way to draw attention in such an automated setting is to fail\n> the build job. Therefore, modify the 'ci/run-static-analysis.sh'\n> build script to check all the resulting '*.cocci.patch' files, and\n> fail the build job if any of them turns out to be not empty. Include\n> those files' contents, i.e. Coccinelle's suggested transformations, in\n> the build job's trace log, so we'll know why it failed.\n\nAnd this is how it looks like \"in action\":\n\n https://travis-ci.org/git/git/jobs/408269979#L570\n"},{"id":"353658","messageId":"xmqqy3dyq6w4.fsf@gitster-ct.c.googlers.com","threadId":"48939","inReplyTo":"CAM0VKjmqwByERhPZLqZfg6JNEEeMkKQU1XuD93aW+3yfc2gucA@mail.gmail.com","subject":"Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-26T16:49:31Z","receivedAt":"2018-07-26T16:49:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor wrote:\n>\n>> The only way to draw attention in such an automated setting is to fail\n>> the build job. Therefore, modify the 'ci/run-static-analysis.sh'\n>> build script to check all the resulting '*.cocci.patch' files, and\n>> fail the build job if any of them turns out to be not empty. Include\n>> those files' contents, i.e. Coccinelle's suggested transformations, in\n>> the build job's trace log, so we'll know why it failed.\n>\n> And this is how it looks like \"in action\":\n>\n> https://travis-ci.org/git/git/jobs/408269979#L570\n\n;-)\n"}]}