[PATCH 0/2] travis-ci: fail if Coccinelle found something to transform

[PATCH 0/2] travis-ci: fail if Coccinelle found something to transform

[SZEDER Gábor]

The first patch makes the static analysis build job on Travis CI
faster by running it with 'make -j2'.

The second patch makes it more more useful by failing the build job if
Coccinelle finds something to transform, thereby drawing our attention
to undesired code patterns trying to enter the codebase.


With these patches applied, the static analysis build job would fail
on current 'pu' because of two small issues on two branches:

  - js/range-diff: Dscho has sent out v4 of this series over the
    weekend, which already incorporates Coccinelle's suggestion, so
    it's basically done.
  
  - pb/bisect-helper-2: this topic has not seen an update in about 9
    months, so I'll send a followup patch 3/2 to be applied on top or
    squashed in, whichever is deemed better.


SZEDER Gábor (2):
  travis-ci: run Coccinelle static analysis with two parallel jobs
  travis-ci: fail if Coccinelle static analysis found something to
    transform

 ci/run-static-analysis.sh | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

-- 
2.18.0.408.g42635c01bc

[PATCH 1/2] travis-ci: run Coccinelle static analysis with two parallel jobs

[SZEDER Gábor]

Currently the static analysis build job runs Coccinelle using a single
'make' job.  Using two parallel jobs cuts down the build job's run
time from around 10-12mins to 6-7mins, sometimes even under 6mins
(there is quite large variation between build job runtimes).  More
than two parallel jobs don't seem to bring further runtime benefits.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/run-static-analysis.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index fe4ee4e06b..fa719c9ef9 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh
@@ -5,6 +5,6 @@
 
 . ${0%/*}/lib-travisci.sh
 
-make coccicheck
+make --jobs=2 coccicheck
 
 save_good_tree
-- 
2.18.0.408.g42635c01bc

[PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

[SZEDER Gábor]

Coccinelle's and in turn 'make coccicheck's exit code only indicates
that Coccinelle managed to finish its analysis without any errors
(e.g. no unknown --options, no missing files, no syntax errors in the
semantic patches, etc.), but it doesn't indicate whether it found any
undesired code patterns to transform or not.  To find out the latter,
one has to look closer at 'make coccicheck's standard output and look
for lines like:

  SPATCH result: contrib/coccinelle/<something>.cocci.patch

And this only indicates that there is something to transform, but to
see what the suggested transformations are one has to actually look
into those '*.cocci.patch' files.

This makes the automated static analysis build job on Travis CI not
particularly useful, because it neither draws our attention to
Coccinelle's findings, nor shows the actual findings.  Consequently,
new topics introducing undesired code patterns graduated to master
on several occasions without anyone noticing.

The only way to draw attention in such an automated setting is to fail
the build job.  Therefore, modify the 'ci/run-static-analysis.sh'
build script to check all the resulting '*.cocci.patch' files, and
fail the build job if any of them turns out to be not empty.  Include
those files' contents, i.e. Coccinelle's suggested transformations, in
the build job's trace log, so we'll know why it failed.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 ci/run-static-analysis.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
index fa719c9ef9..5688f261d0 100755
--- a/ci/run-static-analysis.sh
+++ b/ci/run-static-analysis.sh
@@ -7,4 +7,23 @@
 
 make --jobs=2 coccicheck
 
+set +x
+
+fail=
+for cocci_patch in contrib/coccinelle/*.patch
+do
+	if test -s "$cocci_patch"
+	then
+		echo "$(tput setaf 1)Coccinelle suggests the following changes in '$cocci_patch':$(tput sgr0)"
+		cat "$cocci_patch"
+		fail=UnfortunatelyYes
+	fi
+done
+
+if test -n "$fail"
+then
+	echo "$(tput setaf 1)error: Coccinelle suggested some changes$(tput sgr0)"
+	exit 1
+fi
+
 save_good_tree
-- 
2.18.0.408.g42635c01bc

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

[Derrick Stolee]

On 7/23/2018 9:02 AM, SZEDER Gábor wrote:
> Coccinelle's and in turn 'make coccicheck's exit code only indicates
> that Coccinelle managed to finish its analysis without any errors
> (e.g. no unknown --options, no missing files, no syntax errors in the
> semantic patches, etc.), but it doesn't indicate whether it found any
> undesired code patterns to transform or not.  To find out the latter,
> one has to look closer at 'make coccicheck's standard output and look
> for lines like:
>
>    SPATCH result: contrib/coccinelle/<something>.cocci.patch
>
> And this only indicates that there is something to transform, but to
> see what the suggested transformations are one has to actually look
> into those '*.cocci.patch' files.
>
> This makes the automated static analysis build job on Travis CI not
> particularly useful, because it neither draws our attention to
> Coccinelle's findings, nor shows the actual findings.  Consequently,
> new topics introducing undesired code patterns graduated to master
> on several occasions without anyone noticing.
>
> The only way to draw attention in such an automated setting is to fail
> the build job.  Therefore, modify the 'ci/run-static-analysis.sh'
> build script to check all the resulting '*.cocci.patch' files, and
> fail the build job if any of them turns out to be not empty.  Include
> those files' contents, i.e. Coccinelle's suggested transformations, in
> the build job's trace log, so we'll know why it failed.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>   ci/run-static-analysis.sh | 19 +++++++++++++++++++
>   1 file changed, 19 insertions(+)
>
> diff --git a/ci/run-static-analysis.sh b/ci/run-static-analysis.sh
> index fa719c9ef9..5688f261d0 100755
> --- a/ci/run-static-analysis.sh
> +++ b/ci/run-static-analysis.sh
> @@ -7,4 +7,23 @@
>   
>   make --jobs=2 coccicheck
>   
> +set +x
> +
> +fail=
> +for cocci_patch in contrib/coccinelle/*.patch
> +do
> +	if test -s "$cocci_patch"
> +	then
> +		echo "$(tput setaf 1)Coccinelle suggests the following changes in '$cocci_patch':$(tput sgr0)"
> +		cat "$cocci_patch"
> +		fail=UnfortunatelyYes
> +	fi
> +done
> +
> +if test -n "$fail"
> +then
> +	echo "$(tput setaf 1)error: Coccinelle suggested some changes$(tput sgr0)"
> +	exit 1
> +fi
> +
>   save_good_tree

This looks like the right way to report a failure. It provides as much 
information as possible. I agree that we should have this as part of CI 
so we find issues more quickly.

Thanks!

-Stolee

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

[SZEDER Gábor]

On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:

> The only way to draw attention in such an automated setting is to fail
> the build job.  Therefore, modify the 'ci/run-static-analysis.sh'
> build script to check all the resulting '*.cocci.patch' files, and
> fail the build job if any of them turns out to be not empty.  Include
> those files' contents, i.e. Coccinelle's suggested transformations, in
> the build job's trace log, so we'll know why it failed.

And this is how it looks like "in action":

  https://travis-ci.org/git/git/jobs/408269979#L570

Re: [PATCH 2/2] travis-ci: fail if Coccinelle static analysis found something to transform

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> On Mon, Jul 23, 2018 at 3:02 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
>> The only way to draw attention in such an automated setting is to fail
>> the build job.  Therefore, modify the 'ci/run-static-analysis.sh'
>> build script to check all the resulting '*.cocci.patch' files, and
>> fail the build job if any of them turns out to be not empty.  Include
>> those files' contents, i.e. Coccinelle's suggested transformations, in
>> the build job's trace log, so we'll know why it failed.
>
> And this is how it looks like "in action":
>
>   https://travis-ci.org/git/git/jobs/408269979#L570

;-)

[PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C

[SZEDER Gábor]

bisect--helper: use oid_to_hex()

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/bisect--helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
index fc02f889e6..eac4c27787 100644
--- a/builtin/bisect--helper.c
+++ b/builtin/bisect--helper.c
@@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
 		if (!get_oid(head, &head_oid) &&
 		    !starts_with(head, "refs/heads/")) {
 			strbuf_reset(&start_head);
-			strbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));
+			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
 		} else if (!get_oid(head, &head_oid) &&
 			   skip_prefix(head, "refs/heads/", &head)) {
 			/*
-- 
2.18.0.408.g42635c01bc

Re: [PATCH 3/2 for pb/bisect-helper-2] squash! bisect--helper: `bisect_start` shell function partially in C

[Derrick Stolee]

On 7/23/2018 9:06 AM, SZEDER Gábor wrote:
> bisect--helper: use oid_to_hex()
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>   builtin/bisect--helper.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c
> index fc02f889e6..eac4c27787 100644
> --- a/builtin/bisect--helper.c
> +++ b/builtin/bisect--helper.c
> @@ -547,7 +547,7 @@ static int bisect_start(struct bisect_terms *terms, int no_checkout,
>   		if (!get_oid(head, &head_oid) &&
>   		    !starts_with(head, "refs/heads/")) {
>   			strbuf_reset(&start_head);
> -			strbuf_addstr(&start_head, sha1_to_hex(head_oid.hash));
> +			strbuf_addstr(&start_head, oid_to_hex(&head_oid));
>   		} else if (!get_oid(head, &head_oid) &&
>   			   skip_prefix(head, "refs/heads/", &head)) {
>   			/*

This patch looks obviously correct. Thanks!

-Stolee

Re: [PATCH 0/2] travis-ci: fail if Coccinelle found something to transform

[Derrick Stolee]

On 7/23/2018 9:02 AM, SZEDER Gábor wrote:
> The first patch makes the static analysis build job on Travis CI
> faster by running it with 'make -j2'.
>
> The second patch makes it more more useful by failing the build job if
> Coccinelle finds something to transform, thereby drawing our attention
> to undesired code patterns trying to enter the codebase.
>
>
> With these patches applied, the static analysis build job would fail
> on current 'pu' because of two small issues on two branches:
>
>    - js/range-diff: Dscho has sent out v4 of this series over the
>      weekend, which already incorporates Coccinelle's suggestion, so
>      it's basically done.
>    
>    - pb/bisect-helper-2: this topic has not seen an update in about 9
>      months, so I'll send a followup patch 3/2 to be applied on top or
>      squashed in, whichever is deemed better.
>
>
> SZEDER Gábor (2):
>    travis-ci: run Coccinelle static analysis with two parallel jobs
>    travis-ci: fail if Coccinelle static analysis found something to
>      transform
>
>   ci/run-static-analysis.sh | 21 ++++++++++++++++++++-
>   1 file changed, 20 insertions(+), 1 deletion(-)

Thanks, Szeder.

I agree that we should use all possible automation as part of CI to 
catch things early. I like the direction here.

Reviewed-by: Derrick Stolee <dstolee@microsoft.com>