Changed script to pickup right chromium revision regardless of what's passed in

Changed script to pickup right chromium revision regardless of what's passed in
so that the blink revision in DEPS matches the one in VERSIONS.
Also fixed good and bad versions to point to actual builds to make sure VERSIONS
file exists and can be read.

R=mmoss@chromium.org, rsesek@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=225102

[Michael Moss, 2013-09-20 21:10:12]

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:250: return revlist
You probably want to always return 'revlist' (i.e. outside this if block), to be
consistent with previous behavior. In particular, if this block isn't run, then
when it's used below with:

if len(revlist) ...

It will throw an exception for calling len() on NoneType.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:665: chromium revision |rev."""
Last line in the comment looks like copy-paste error.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:671: rev = revisions[idx-1]
What happens if 'rev' is the first item in 'revisions' (idx==0)?

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:676: return rev, revisions_final
It's probably worth noting that when you modify 'revisions_final' above, you are
modifying the list that the caller sees. In other words, it's kind of redundant,
and bit a confusing, to include it in the return values.

If you don't intend to modify it, then you need to make a copy (e.g.
"revisions_copy = list(revisions_final)") and return the copy. If you aren't
worried about that side-effect, then maybe don't return it, so it's more obvious
that it's intended to be modified "in place". And adding something about that
side-effect to the docstring wouldn't hurt.

[Robert Sesek, 2013-09-23 16:37:15]

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:172: 
remove

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:230: # set good and bad revisions to be legit revisions
Proper punctuation, capitalization, and grammar for all comments is required.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:232: if (self.good_revision < self.bad_revision):
nit: no () around conditions unless necessary for precedence

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:239: #fix chromium rev so that the deps blink revision
matches VERSIONS file
nit: same as above, but add a space after #

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:634: def GetBlinkDEPSREvisionForChromiumRevision(rev):
Lowercase the E in Revision

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:645: raise Exception('Could not get blink revision for cr
rev %d' % rev)
Capitalize Blink and spell out Chromium. This is a message people may see. It
should be clear.

[kareng, 2013-09-23 21:20:32]

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:172: 
On 2013/09/23 16:37:15, rsesek wrote:
> remove

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:230: # set good and bad revisions to be legit revisions
On 2013/09/23 16:37:15, rsesek wrote:
> Proper punctuation, capitalization, and grammar for all comments is required.

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:232: if (self.good_revision < self.bad_revision):
On 2013/09/23 16:37:15, rsesek wrote:
> nit: no () around conditions unless necessary for precedence

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:239: #fix chromium rev so that the deps blink revision
matches VERSIONS file
On 2013/09/23 16:37:15, rsesek wrote:
> nit: same as above, but add a space after #

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:250: return revlist
On 2013/09/20 21:10:12, Michael Moss wrote:
> You probably want to always return 'revlist' (i.e. outside this if block), to
be
> consistent with previous behavior. In particular, if this block isn't run,
then
> when it's used below with:
> 
> if len(revlist) ...
> 
> It will throw an exception for calling len() on NoneType.

yes that was a mistake of tabbing. ty!

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:634: def GetBlinkDEPSREvisionForChromiumRevision(rev):
On 2013/09/23 16:37:15, rsesek wrote:
> Lowercase the E in Revision

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:645: raise Exception('Could not get blink revision for cr
rev %d' % rev)
On 2013/09/23 16:37:15, rsesek wrote:
> Capitalize Blink and spell out Chromium. This is a message people may see. It
> should be clear.

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:665: chromium revision |rev."""
On 2013/09/20 21:10:12, Michael Moss wrote:
> Last line in the comment looks like copy-paste error.

Done.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:671: rev = revisions[idx-1]
On 2013/09/20 21:10:12, Michael Moss wrote:
> What happens if 'rev' is the first item in 'revisions' (idx==0)?

heh good, point. i changed so now it doesn't continue otherwise, which means we
return what was sent in. i could also error out wasn't sure what's best to do.

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:676: return rev, revisions_final
so you think its better to copy and return the copy? isn't that more costly?

 On 2013/09/20 21:10:12, Michael Moss wrote:
> It's probably worth noting that when you modify 'revisions_final' above, you
are
> modifying the list that the caller sees. In other words, it's kind of
redundant,
> and bit a confusing, to include it in the return values.
> 
> If you don't intend to modify it, then you need to make a copy (e.g.
> "revisions_copy = list(revisions_final)") and return the copy. If you aren't
> worried about that side-effect, then maybe don't return it, so it's more
obvious
> that it's intended to be modified "in place". And adding something about that
> side-effect to the docstring wouldn't hurt.

[Michael Moss, 2013-09-23 22:01:56]

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py
File tools/bisect-builds.py (right):

https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
tools/bisect-builds.py:676: return rev, revisions_final
On 2013/09/23 21:20:32, kareng wrote:
> so you think its better to copy and return the copy? isn't that more costly?

Modifying in-place is fine with me, just make it explicit in docstring and
return value.

[kareng, 2013-09-24 00:40:59]

On 2013/09/23 22:01:56, Michael Moss wrote:
> https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py
> File tools/bisect-builds.py (right):
> 
>
https://codereview.chromium.org/23629048/diff/11001/tools/bisect-builds.py#ne...
> tools/bisect-builds.py:676: return rev, revisions_final
> On 2013/09/23 21:20:32, kareng wrote:
> > so you think its better to copy and return the copy? isn't that more costly?
> 
> Modifying in-place is fine with me, just make it explicit in docstring and
> return value.

ok i think i did it :)

[Michael Moss, 2013-09-24 14:46:59]

lgtm

[Robert Sesek, 2013-09-24 22:08:32]

LGTM w/ a nit

https://codereview.chromium.org/23629048/diff/10003/tools/bisect-builds.py
File tools/bisect-builds.py (right):

https://codereview.chromium.org/23629048/diff/10003/tools/bisect-builds.py#ne...
tools/bisect-builds.py:663: for blink bisect, deps and REVISIONS file might not
match since
Capitalize DEPS and is it REVISIONS or VERSIONS (which you wrote on line 238)?

[kareng, 2013-09-24 23:46:05]

On 2013/09/24 22:08:32, rsesek wrote:
> LGTM w/ a nit
> 
> https://codereview.chromium.org/23629048/diff/10003/tools/bisect-builds.py
> File tools/bisect-builds.py (right):
> 
>
https://codereview.chromium.org/23629048/diff/10003/tools/bisect-builds.py#ne...
> tools/bisect-builds.py:663: for blink bisect, deps and REVISIONS file might
not
> match since
> Capitalize DEPS and is it REVISIONS or VERSIONS (which you wrote on line 238)?

good catch REVISIONS, changed and capitalized DEPS. ok to land?

[kareng, 2013-09-24 23:49:33]

Committed patchset #9 manually as r225102 (presubmit successful).