VM: Enforce correctly formatted C++ code

VM: Enforce correctly formatted C++ code

Currently the presubmit script for 'git cl upload' will just warn the developer
that code is incorrectly formatted and asks whether to proceed. This makes
developers sometimes just ignore the warning and upload incorrectly formatted
C++ code.

This causes incorrectly formatted code to be committed to the repository and
will cause a presubmit warning/question for all developers afterwards.

This CL will therefore turn the warning into an error to ensure our c++ code is
always correctly formatted.

R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/554c3c9683264ab9840a2c2371942b5a9f5e1fae

[kustermann, 2016-11-22 13:16:30]

kustermann@google.com changed reviewers:
+ vegorov@google.com, zra@google.com

[Cutch, 2016-11-22 17:19:15]

On 2016/11/22 13:16:31, kustermann wrote:

I also don't like the current behaviour but I have a suggestion for an
improvement:

if the source isn't formatted correctly automatically run the formatter and
amend the commit.

$ git cl format $PROJECT_ROOT/runtime
$ git commit -a --amend

[kustermann, 2016-11-23 08:05:05]

On 2016/11/22 17:19:15, Cutch wrote:
> On 2016/11/22 13:16:31, kustermann wrote:
> 
> I also don't like the current behaviour but I have a suggestion for an
> improvement:
> 
> if the source isn't formatted correctly automatically run the formatter and
> amend the commit.
> 
> $ git cl format $PROJECT_ROOT/runtime
> $ git commit -a --amend

Personally I would be very surprised if a "git cl upload" changes files on my
checkout or rewrites my git history.

In the past we had similar "auto magic" which caused confusion/issues (one
recent example is the runtime/.svn directory which suddenly started appearing,
which then needed to be removed manually to build properly again, now it
disappeared again).

We could also briefly talk about it in the next meeting and hear other's
opinions.

[zra, 2016-11-27 04:04:55]

I agree with Martin that committing the changes automatically would be a little
surprising. Why don't we see how it goes like this, and make it automatic as a
second step, if needed.

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py
File runtime/PRESUBMIT.py (right):

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py#newcode58
runtime/PRESUBMIT.py:58: messages = stream.getvalue().split('\n')
I don't remember exactly how the messages look, but this looks a little fragile.
If the goal here is to avoid printing the "Continue (y/N)" line, then maybe chop
off that line instead of taking only the first line.

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py#newcode59
runtime/PRESUBMIT.py:59: message = messages[0] if messages else 'Unknown
formatting error.'
Did you see this happen? Instead of printing 'Unknown formatting error.' which
indicates a problem with the source code in the CL, I think this should fail
some other way to indicate that this bit of code isn't getting what it expects.
Maybe leave the check out, and let it throw the exception.

[kustermann, 2016-11-30 21:40:33]

ptal

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py
File runtime/PRESUBMIT.py (right):

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py#newcode58
runtime/PRESUBMIT.py:58: messages = stream.getvalue().split('\n')
On 2016/11/27 04:04:55, zra wrote:
> I don't remember exactly how the messages look, but this looks a little
fragile.
> If the goal here is to avoid printing the "Continue (y/N)" line, then maybe
chop
> off that line instead of taking only the first line.

The goal is not to prevent the "Continue (y/N)" line. The goal was to just
extract the message itself (I cannot access the protected members
`presubmit_result._message`.

The code comes from here
(https://github.com/coreos/depot_tools/blob/master/presubmit_support.py#L132):

  def handle(self, output):
    output.write(self._message)
    output.write('\n')
    for index, item in enumerate(self._items):
      output.write('  ')
      # Write separately in case it's unicode.
      output.write(str(item))
      if index < len(self._items) - 1:
        output.write(' \\')
      output.write('\n')
    if self._long_text:
      output.write('\n***************\n')
      # Write separately in case it's unicode.
      output.write(self._long_text)
      output.write('\n***************\n')
    if self.fatal:
      output.fail()

Done - I'll just use the entire message.

https://codereview.chromium.org/2525633002/diff/1/runtime/PRESUBMIT.py#newcode59
runtime/PRESUBMIT.py:59: message = messages[0] if messages else 'Unknown
formatting error.'
On 2016/11/27 04:04:55, zra wrote:
> Did you see this happen? Instead of printing 'Unknown formatting error.' which
> indicates a problem with the source code in the CL, I think this should fail
> some other way to indicate that this bit of code isn't getting what it
expects.
> Maybe leave the check out, and let it throw the exception.

Done - It was just a precaution, I'll let it throw then.

[zra, 2016-11-30 21:44:55]

lgtm

[kustermann, 2016-11-30 22:52:59]

Description was changed from

==========
VM: Enforce correctly formatted C++ code

Currently the presubmit script for 'git cl upload' will just warn the developer
that code is incorrectly formatted and asks whether to proceed. This makes
developers sometimes just ignore the warning and upload incorrectly formatted
C++ code.

This causes incorrectly formatted code to be committed to the repository and
will cause a presubmit warning/question for all developers afterwards.

This CL will therefore turn the warning into an error to ensure our c++ code is
always correctly formatted.
==========

to

==========
VM: Enforce correctly formatted C++ code

Currently the presubmit script for 'git cl upload' will just warn the developer
that code is incorrectly formatted and asks whether to proceed. This makes
developers sometimes just ignore the warning and upload incorrectly formatted
C++ code.

This causes incorrectly formatted code to be committed to the repository and
will cause a presubmit warning/question for all developers afterwards.

This CL will therefore turn the warning into an error to ensure our c++ code is
always correctly formatted.

R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/554c3c9683264ab9840a2c2371942b5a9f5e1fae
==========

[kustermann, 2016-11-30 22:53:00]

Committed patchset #2 (id:20001) manually as
554c3c9683264ab9840a2c2371942b5a9f5e1fae (presubmit successful).