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

tools/bisect-builds.py

Index: tools/bisect-builds.py
===================================================================
--- tools/bisect-builds.py	(revision 223632)
+++ tools/bisect-builds.py	(working copy)
@@ -169,6 +169,7 @@
       next-marker is not None, then the listing is a partial listing and another
       fetch should be performed with next-marker being the marker= GET
       parameter."""
+

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

remove

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

Done.

       handle = urllib.urlopen(url)
       document = ElementTree.parse(handle)
 
@@ -221,11 +222,33 @@
     # Download the revlist and filter for just the range between good and bad.
     minrev = min(self.good_revision, self.bad_revision)
     maxrev = max(self.good_revision, self.bad_revision)
-    revlist = map(int, self.ParseDirectoryIndex())
-    revlist = [x for x in revlist if x >= int(minrev) and x <= int(maxrev)]
+    revlist_all = map(int, self.ParseDirectoryIndex())
+
+    revlist = [x for x in revlist_all if x >= int(minrev) and x <= int(maxrev)]
     revlist.sort()
-    return revlist
 
+    # set good and bad revisions to be legit revisions

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

Proper punctuation, capitalization, and grammar for all comments is required.

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

Done.

+    if revlist:
+      if (self.good_revision < self.bad_revision):

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

nit: no () around conditions unless necessary for precedence

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

Done.

+        self.good_revision = revlist[0]
+        self.bad_revision = revlist[-1]
+      else:
+        self.bad_revision = revlist[0]
+        self.good_revision = revlist[-1]
+
+      #fix chromium rev so that the deps blink revision matches VERSIONS file

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

nit: same as above, but add a space after #

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

Done.

+      if (self.base_url == WEBKIT_BASE_URL):
+        revlist_all.sort()
+        self.good_revision, revlist = FixChromiumRevForBlink(revlist,
+                                                             revlist_all,
+                                                             self,
+                                                             self.good_revision)
+        self.bad_revision, revlist = FixChromiumRevForBlink(revlist,
+                                                            revlist_all,
+                                                            self,
+                                                            self.bad_revision)
+      return revlist

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

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.

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

yes that was a mistake of tabbing. ty!

+
   def GetOfficialBuildsList(self):
     """Gets the list of official build numbers between self.good_revision and
     self.bad_revision."""
@@ -608,10 +631,23 @@
   return (revlist[minrev], revlist[maxrev])
 
 
+def GetBlinkDEPSREvisionForChromiumRevision(rev):

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

Lowercase the E in Revision

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

Done.

+  """Returns the blink revision that was in REVISIONS file at
+  chromium revision |rev|."""
+  # . doesn't match newlines without re.DOTALL, so this is safe.
+  blink_re = re.compile(r'webkit_revision\D*(\d+)')
+  url = urllib.urlopen(DEPS_FILE % rev)
+  m = blink_re.search(url.read())
+  url.close()
+  if m:
+    return int(m.group(1))
+  else:
+    raise Exception('Could not get blink revision for cr rev %d' % rev)

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

Capitalize Blink and spell out Chromium. This is a message people may see. It
should be clear.

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

Done.

+
+
 def GetBlinkRevisionForChromiumRevision(self, rev):
   """Returns the blink revision that was in REVISIONS file at
   chromium revision |rev|."""
-  # . doesn't match newlines without re.DOTALL, so this is safe.
   file_url = "%s/%s%d/REVISIONS" % (self.base_url,
                                     self._listing_platform_dir, rev)
   url = urllib.urlopen(file_url)
@@ -622,7 +658,23 @@
   else:
     raise Exception('Could not get blink revision for cr rev %d' % rev)
 
+def FixChromiumRevForBlink(revisions_final, revisions, self, rev):
+  """Returns the chromium revision that has the correct blink revision
+  for blink bisect, deps and REVISIONS file might not match since
+  blink snapshots point to tip of tree blink.
+  chromium revision |rev."""

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

Last line in the comment looks like copy-paste error.

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

Done.

 
+  blink_deps_rev = GetBlinkDEPSREvisionForChromiumRevision(rev)
+
+  while (GetBlinkRevisionForChromiumRevision(self, rev) > blink_deps_rev):
+    idx = revisions.index(rev)
+    rev = revisions[idx-1]

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

What happens if 'rev' is the first item in 'revisions' (idx==0)?

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

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.

+    if rev not in revisions_final:
+      revisions_final.insert(0, rev)
+
+  revisions_final.sort()
+  return rev, revisions_final

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

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.

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

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:

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

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

+
 def GetChromiumRevision(url):
   """Returns the chromium revision read from given URL."""
   try: