Subject: Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes

From: SZEDER Gábor <[email-masked]>
Date: Tue, 16 Jan 2018 18:11:30 +0100

> Patch generated by Coccinelle (and contrib/coccinelle/strbuf.cocci).

Interesting.

The static analysis build job on Travis CI runs 'make coccicheck', so
it should have caught this.  However, I've looked at more build job
results than I could count while working on some Travis CI related
patches in the last few weeks, but I've never noticed Coccinelle
proposing anything.

Now I see that Coccinelle's, and in turn 'make coccicheck's exit code
only indicates that Coccinelle managed to finish without any errors
(e.g. no wrong --options, missing files or whatnot), but it has
nothing to do with whether it found something to transform or not.  To
find out the latter, we have to check the resulting
'contrib/coccinelle/*.cocci.patch' files or look closer at 'make
coccicheck's standard output.  If any of those '*.cocci.patch' files
are not empty and make's output contains lines like:

  SPATCH result: contrib/coccinelle/strbuf.cocci.patch

then Coccinelle found something.

Well, OK, I think that might be an acceptable behavior for developers
running 'make coccicheck' themselves, but totally unsuitable for
automated Travis CI build jobs, because it doesn't draw our attention
to Coccinelle's findings.  And the only means to draw attention in
an automated setting is to fail the buid job.

Good, I quickly whipped up a patch to fail the build job if any of the
resulting '*.cocci.patch' files are not empty and also to include
their content in the trace log, to see how that would work out, and...

> "make coccicheck" doesn't propose any other changes for current master.

... and found that 'make coccicheck' on Travis CI _does_ propose
further changes for current master.  You can find the build job's
output including those proposed changes here:

  https://travis-ci.org/szeder/git/jobs/329296813#L586

The proposed changes coming from 'array.cocci' are replacing memmove()
calls with MOVE_ARRAY().  After a (very) cursory look they all seem to
make sense to me; at least after applying those changes Git can be
built and the test suite still succeeds.

Unfortunately, most of the changes coming from 'strbuf.cocci' don't
make any sense, they appear to be the mis-application of the "use
strbuf_addstr() instead of strbuf_addf() to add a single string" rule:

  -             strbuf_addf(&sb_repo, "%d", counter);
  +             strbuf_addstr(&sb_repo, counter);

It seems that those rules need some refinement, but I have no idea
about Coccinelle and this is not the time for me to dig deeper.

What makes all this weird is that running 'make coccicheck' on my own
machine doesn't produce any of these additional proposed changes, just
like at René's.  Can it be related to differing Coccinelle versions?
Travis CI installs 1.0.0~rc19.deb-3; I have 1.0.4.deb-2.

Gábor