git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "SZEDER Gábor" <szeder.dev@gmail.com>
To: "René Scharfe" <l.s.r@web.de>
Cc: "SZEDER Gábor" <szeder.dev@gmail.com>,
	"Git List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Stefan Beller" <sbeller@google.com>,
	"Lars Schneider" <larsxschneider@gmail.com>
Subject: Re: [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes
Date: Tue, 16 Jan 2018 18:11:30 +0100	[thread overview]
Message-ID: <20180116171130.16568-1-szeder.dev@gmail.com> (raw)
In-Reply-To: <422ae05b-6541-38f3-7638-4bee1b766d91@web.de>

> 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


  parent reply	other threads:[~2018-01-16 17:11 UTC|newest]

Thread overview: 11+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-01-15 17:10 [PATCH] describe: use strbuf_add_unique_abbrev() for adding short hashes René Scharfe
2018-01-16 13:48 ` Derrick Stolee
2018-01-16 17:11 ` SZEDER Gábor [this message]
2018-01-18 21:40   ` René Scharfe
2018-01-18 22:40     ` SZEDER Gábor
2018-01-18 23:02       ` Lars Schneider
2018-01-19 17:53       ` René Scharfe
2018-01-22 17:50         ` [PATCH] Use MOVE_ARRAY SZEDER Gábor
2018-01-22 22:44           ` Jeff King
2018-01-22 23:26             ` Junio C Hamano
2018-01-22 23:34               ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180116171130.16568-1-szeder.dev@gmail.com \
    --to=szeder.dev@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=l.s.r@web.de \
    --cc=larsxschneider@gmail.com \
    --cc=sbeller@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).