Adds a presubmit that looks for version control conflicts.

Adds a presubmit that looks for version control conflicts.

R=maruel@chromium.org,thakis@chromium.org
BUG=None


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=168715

[Nico, 2012-11-17 05:11:03]

While this is a good idea, I thought subversion took care of not submitting when
any files are in a conflict state. Maybe the CQ and/or git don't do this nicely?

Brett, how did you end up checking in version control conflicts? Did you do
anything weird locally?

(I'm trying to understand what happened here. Adding a presubmit check for this
is kind of like adding a presubmit check that compiles all code.)

(maruel: This is something that would've been caught by the cq building all
targets. We should probably add rlz_unittests to the chromium_builder_targets)

[M-A Ruel, 2012-11-17 11:56:02]

On 2012/11/17 05:11:03, Nico wrote:
> (maruel: This is something that would've been caught by the cq building all
> targets. We should probably add rlz_unittests to the chromium_builder_targets)

I've done that yesterday as I explained on irc and will be enabled as soon as I
restart the Try Server;
https://chromiumcodereview.appspot.com/11348094

[Dan Beam, 2012-11-17 20:36:17]

On 2012/11/17 11:56:02, Marc-Antoine Ruel wrote:
> On 2012/11/17 05:11:03, Nico wrote:
> > (maruel: This is something that would've been caught by the cq building all
> > targets. We should probably add rlz_unittests to the
chromium_builder_targets)
> 
> I've done that yesterday as I explained on irc and will be enabled as soon as
I
> restart the Try Server;
> https://chromiumcodereview.appspot.com/11348094

That's great but it still means you have to issue like X try runs and wait for
one to fail instead of getting notified pretty cheaply and immediately, right?

[Nico, 2012-11-17 20:38:36]

On Sat, Nov 17, 2012 at 12:36 PM,  <dbeam@chromium.org> wrote:
> On 2012/11/17 11:56:02, Marc-Antoine Ruel wrote:
>>
>> On 2012/11/17 05:11:03, Nico wrote:
>> > (maruel: This is something that would've been caught by the cq building
>> > all
>> > targets. We should probably add rlz_unittests to the
>
> chromium_builder_targets)
>
>> I've done that yesterday as I explained on irc and will be enabled as soon
>> as
>
> I
>>
>> restart the Try Server;
>> https://chromiumcodereview.appspot.com/11348094
>
>
> That's great but it still means you have to issue like X try runs and wait
> for
> one to fail instead of getting notified pretty cheaply and immediately,
> right?

Right, but in the limit this means adding checks for all kinds of
common compiler errors to the presubmit script, which feels weird to
me.

>
> https://codereview.chromium.org/11417044/

[Dan Beam, 2012-11-17 20:42:44]

On 2012/11/17 20:38:36, Nico wrote:
> On Sat, Nov 17, 2012 at 12:36 PM,  <mailto:dbeam@chromium.org> wrote:
> > On 2012/11/17 11:56:02, Marc-Antoine Ruel wrote:
> >>
> >> On 2012/11/17 05:11:03, Nico wrote:
> >> > (maruel: This is something that would've been caught by the cq building
> >> > all
> >> > targets. We should probably add rlz_unittests to the
> >
> > chromium_builder_targets)
> >
> >> I've done that yesterday as I explained on irc and will be enabled as soon
> >> as
> >
> > I
> >>
> >> restart the Try Server;
> >> https://chromiumcodereview.appspot.com/11348094
> >
> >
> > That's great but it still means you have to issue like X try runs and wait
> > for
> > one to fail instead of getting notified pretty cheaply and immediately,
> > right?
> 
> Right, but in the limit this means adding checks for all kinds of
> common compiler errors to the presubmit script, which feels weird to
> me.

This would also be useful in stuff that's not compiled (yet) like
JS/CSS/HTML/Python in which errors may not be caught right away (say one-off
scripts or webui pages that aren't checked much but often conflict, like
about:credits).

> 
> >
> > https://codereview.chromium.org/11417044/

[brettw, 2012-11-17 21:17:37]

When I checked this in I thought I resolved all the conflicts, and did "git
commit -a".

I think the presubmit checks are already too slow, and I'm not sure this check
has that much value that compiling the code wouldn't give.

[Dan Beam, 2012-11-17 21:51:16]

On 2012/11/17 21:17:37, brettw wrote:
> When I checked this in I thought I resolved all the conflicts, and did "git
> commit -a".

I think it's very common to rebase or merge and not compile. Most of the times I
rebase are when I'm about to try/CQ/dcommit a change and some automated tool
tells me "patch failed to apply" or "file needs update". Many folks would
probably deem it reasonable not to recompile after a simple rebase/merge or
resolving a simple conflict.  Additionally if this is in platform specific code
and it's ifdef'd out on you platform you won't see a failure until a try or
build breaks.

> I think the presubmit checks are already too slow, and I'm not sure this check
> has that much value that compiling the code wouldn't give.

I think this check should be very fast. I can quantify how long it would take if
you'd like, but I think it will always be faster than waiting on a local/remote
compile/try or a tree break + revert or fix.

[M-A Ruel, 2012-11-17 22:25:47]

On 2012/11/17 21:51:16, Dan Beam wrote:
> > I think the presubmit checks are already too slow, and I'm not sure this
check
> > has that much value that compiling the code wouldn't give.
> 
> I think this check should be very fast. I can quantify how long it would take
if
> you'd like, but I think it will always be faster than waiting on a
local/remote
> compile/try or a tree break + revert or fix.

I think it's worth enabling timing information in that file. From what I can
tell, this change adds a negligible amount of time, the ChangedContent() return
value is cached, I looked up.

I don't mind this check, so technically lgtm from me, and optimizing the
performance of changed content should be a separate CL.

Background information:
ChangedContent is super slow on svn, especially on Windows. It is comparatively
faster on git IIRC but I don't have timing information offhand so it'd be good
to compare. Work had been done 2 (?) years ago to remove all the calls to
ChangedContent() in the presubmit_canned_checks but src/PRESUBMIT.py killed the
optimization.

It's important that 200 files CLs have reasonable presubmit duration, at least
on developer-friendly setups.

[Evan Stade, 2012-11-19 20:17:22]

On 2012/11/17 21:17:37, brettw wrote:
> When I checked this in I thought I resolved all the conflicts, and did "git
> commit -a".
> 
> I think the presubmit checks are already too slow, and I'm not sure this check
> has that much value that compiling the code wouldn't give.

I have never committed an unresolved conflict, but I have uploaded it to
codereview. This was caught in review (and also by trybots) which in turn wasted
my time and the reviewer's time. Therefore I think it's useful. Personally I
have never been annoyed by the length of presubmit checks.

[Nico, 2012-11-19 22:22:04]

Ok, LGTM with numbers to show that this doesn't impact presubmit time (just run
presubmit with and without this change)

[Dan Beam, 2012-11-20 00:24:06]

Excuse the ASCII art, but here's some numbers I got from just running this
command:

  for i in {1..10}; do git cl presubmit 2>&1 | grep 'to calculate.'; done

on a 200 file change created by doing:

  cc=$(find chrome/browser/ui/ -type f -iname '*.cc' | head -200); for f in $cc;
do echo '' | tee -a "$f" >/dev/null; done

and checking in the results.

Like maruel@ said, ChangedContents() should be cached after the first access and
other tests are already using this, so it should be cached and perform similarly
on all platforms (I'm on a Z620 on Linux, Precise, x64).


@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ Without my presubmit:                                            @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@

Presubmit checks took 10.0s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.0s to calculate.
Presubmit checks took 10.1s to calculate.
-- Presubmit checks took 10.2s to calculate. -- (dropped, highest)
-- Presubmit checks took 9.9s to calculate. -- (dropped, lowest)
Presubmit checks took 10.0s to calculate.

== 80.5s for 8 runs

~10.0625 sec / run
-----------------
   200 files

== 0.0503125 sec / run / file


@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ With my presubmit                                                @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@

Presubmit checks took 10.2s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.1s to calculate.
-- Presubmit checks took 10.0s to calculate. -- (dropped, lowest)
Presubmit checks took 10.2s to calculate.
Presubmit checks took 10.0s to calculate.
Presubmit checks took 10.1s to calculate.
Presubmit checks took 10.2s to calculate.
Presubmit checks took 10.1s to calculate.
-- Presubmit checks took 10.3s to calculate. -- (dropped, highest)

== 81s for 8 runs

~10.125 sec / run
-----------------
   200 files

== 0.050625 sec / run / file


@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
@ Overall stats                                                    @
@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@

  10.1250 s / run
- 10.0625 s / run
----------
== 0.0625 sec / run added

0.0625 sec / run
-----------------
   200 files

== 0.0003125 s / run / file added

[Nico, 2012-11-20 00:25:17]

Ok, lgtm

On Mon, Nov 19, 2012 at 4:24 PM,  <dbeam@chromium.org> wrote:
> Excuse the ASCII art, but here's some numbers I got from just running this
> command:
>
>   for i in {1..10}; do git cl presubmit 2>&1 | grep 'to calculate.'; done
>
> on a 200 file change created by doing:
>
>   cc=$(find chrome/browser/ui/ -type f -iname '*.cc' | head -200); for f in
> $cc;
> do echo '' | tee -a "$f" >/dev/null; done
>
> and checking in the results.
>
> Like maruel@ said, ChangedContents() should be cached after the first access
> and
> other tests are already using this, so it should be cached and perform
> similarly
> on all platforms (I'm on a Z620 on Linux, Precise, x64).
>
>
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
> @ Without my presubmit:                                            @
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>
> Presubmit checks took 10.0s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.0s to calculate.
> Presubmit checks took 10.1s to calculate.
> -- Presubmit checks took 10.2s to calculate. -- (dropped, highest)
> -- Presubmit checks took 9.9s to calculate. -- (dropped, lowest)
> Presubmit checks took 10.0s to calculate.
>
> == 80.5s for 8 runs
>
> ~10.0625 sec / run
> -----------------
>    200 files
>
> == 0.0503125 sec / run / file
>
>
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
> @ With my presubmit                                                @
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>
> Presubmit checks took 10.2s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.1s to calculate.
> -- Presubmit checks took 10.0s to calculate. -- (dropped, lowest)
> Presubmit checks took 10.2s to calculate.
> Presubmit checks took 10.0s to calculate.
> Presubmit checks took 10.1s to calculate.
> Presubmit checks took 10.2s to calculate.
> Presubmit checks took 10.1s to calculate.
> -- Presubmit checks took 10.3s to calculate. -- (dropped, highest)
>
> == 81s for 8 runs
>
> ~10.125 sec / run
> -----------------
>    200 files
>
> == 0.050625 sec / run / file
>
>
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
> @ Overall stats                                                    @
> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>
>   10.1250 s / run
> - 10.0625 s / run
> ----------
> == 0.0625 sec / run added
>
> 0.0625 sec / run
> -----------------
>    200 files
>
> == 0.0003125 s / run / file added
>
> https://chromiumcodereview.appspot.com/11417044/

[commit-bot: I haz the power, 2012-11-20 00:40:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/11417044/9001

[commit-bot: I haz the power, 2012-11-20 03:45:04]

Change committed as 168715