Gerrit Code-Style verification -- was ESC / Rome discussion ...

[David Ostrovsky]

On Mon, Oct 30 08:35:36 UTC 2017, Miklos Vajna wrote:

>On Fri, Oct 27, 2017 at 04:49:55PM +0200, Jan Holesovsky wrote:
>> Good point - another reason why not to do the rewriting completely
>> automatically server side I guess.
>> 
>> But still, I see the Thorsten's point why it would be easier for
people
>> in many cases; that's why I proposed the 'automatic, but ending up
as
>> an additional changeset' way, that at least gives a chance to
inspect &
>> do something about that.
>
>I see the benefit of that, and I can accept that as a compromise,
>though
>I fear a bit it introduces another set of problems:
[...]

Many OSS projects in the wild enforce code styles. Some projects,
even enforce multiple different code style checks for different
languages and even different checks for a single language.

One example is tensoflow:
https://github.com/tensorflow/tensorflow/blob/c44f67a7ed5870fe8a1c0d625
7ce597ca2ef7564/tensorflow/tools/ci_build/ci_sanity.sh

is using:

* pylint
* pep8
* buildifier (Bazel BUILD files)
* clang_format_check

The way it's implemented, obviously, is the CI build check.

In Gerrit Code Review, we've added dedicates Gerrit Label: Code-Style,
that blocks the change, when voted -1, and +1 is required for a change
to be submittable.

I would not recommend any magic, neither on the client nor on the
server (Gerrit) side: no pre-commit hooks, no automagical new patch set
creation, in case Code-Style check(s) are failed (say both checks
verify and code-style are failed, and creation of new patch set would
not fix the root cause for verification failure, like compiler error or
similar; the patch set was created by human and should be fxied by
human).

Of course, a script should be provided to run the check at any time by
contributor, it should not be integrated in the commit hook. That how I
would expect it to be implemented. When I push to gerrit, see below,
all checks should be run as part of Gerrit Code-Syle verification job.
For those who don't use gerrit, and push directly to master, well, they
should check the style on their own, but hey, they already verify on
their own the commit on 4 platform locally, so running locally the
code-style checks shouldn't be a problem.

Next reason why client side pre-commit hook wouldn't solve all use
cases: it's possible to create a modification in Gerrit,
using inline edit feature. Any client pre-commit hooks wouldn't be
executed in that use case.

My advice would be to add Code-Style label to gerrit, say Code-Style,
like we did in Gerrit Code Review upstream, in this change: [1],
with the range [-1,+1]. In additional add new Jenkins job that is
running the checks and is voting on the new label.

So that a normal change in Gerrit Code Review project must now have max
positive votes on all these labels, to be submittable, e.g.: [2].

Code-Review +2
Code-Style +1
Library-Compliance +1
Verified +1

And no, I'm not volunteering to do the job.

* [1] https://gerrit-review.googlesource.com/#/c/gerrit/+/108216
* [2] https://gerrit-review.googlesource.com/#/c/gerrit/+/122170