Fix toplevel_root parsing in trychange.py

trychange.py

 #!/usr/bin/env python
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Client-side script to send a try job to the try server. It communicates to
 the try server by either writting to a svn repository or by directly connecting
 to the server by HTTP.
 """
 
@@ skipping 131 matching lines @@
       settings_file = self.ReadRootFile(self.codereview_settings_file)
       if settings_file:
         for line in settings_file.splitlines():
           if not line or line.lstrip().startswith('#'):
             continue
           k, v = line.split(":", 1)
           self.codereview_settings[k.strip()] = v.strip()
     return self.codereview_settings.get(key, '')
 
   def _GclStyleSettings(self):
-    """Set default settings based on the gcl-style settings from the
-    repository."""
+    """Set default settings based on the gcl-style settings from the repository.
+
+    This method needs to be called after self.top_level_root has been set.

[M-A Ruel, 2012/12/19 13:46:12]

Add an assert instead?

[kjellander_chromium, 2012/12/19 16:16:15]

Actually I think it's a bit confusing to say it has to be set, since there seem
to be code paths that don't require it. I changed the comment to something more
explaining instead.

+    """
     settings = {
       'port': self.GetCodeReviewSetting('TRYSERVER_HTTP_PORT'),
       'host': self.GetCodeReviewSetting('TRYSERVER_HTTP_HOST'),
       'svn_repo': self.GetCodeReviewSetting('TRYSERVER_SVN_URL'),
       'project': self.GetCodeReviewSetting('TRYSERVER_PROJECT'),
       'root': self.GetCodeReviewSetting('TRYSERVER_ROOT'),
       'patchlevel': self.GetCodeReviewSetting('TRYSERVER_PATCHLEVEL'),
     }
     logging.info('\n'.join(['%s: %s' % (k, v)
                             for (k, v) in settings.iteritems() if v]))
     for (k, v) in settings.iteritems():
+      # Do not overwrite options already set using command line flags.
       if v and getattr(self.options, k) is None:
         setattr(self.options, k, v)
 
   def AutomagicalSettings(self):
     """Determines settings based on supported code review and checkout tools.
     """
-    self._GclStyleSettings()
-    # Try to find gclient or repo root.
+    # Try to find gclient or repo root first.
     if not self.options.no_search:
       self.toplevel_root = gclient_utils.FindGclientRoot(self.checkout_root)
       if self.toplevel_root:
         logging.info('Found .gclient at %s' % self.toplevel_root)
       else:
         self.toplevel_root = gclient_utils.FindFileUpwards(
             os.path.join('..', '.repo'), self.checkout_root)
         if self.toplevel_root:
           logging.info('Found .repo dir at %s'
                        % os.path.dirname(self.toplevel_root))
 
+      # Parse TRYSERVER_* settings from codereview.settings before falling back
+      # on setting self.options.root manually further down. Otherwise
+      # TRYSERVER_ROOT would never be used in codereview.settings.
+      self._GclStyleSettings()
+
       if self.toplevel_root and not self.options.root:
         assert os.path.abspath(self.toplevel_root) == self.toplevel_root
         self.options.root = gclient_utils.PathDifference(self.toplevel_root,
                                                          self.checkout_root)
+    else:
+      self._GclStyleSettings()
 
   def ReadRootFile(self, filename):
     cur = self.checkout_root
     root = self.toplevel_root or self.checkout_root
 
     assert cur.startswith(root), (root, cur)
     while cur.startswith(root):
       filepath = os.path.join(cur, filename)
       if os.path.isfile(filepath):
         logging.info('Found %s at %s' % (filename, cur))
@@ skipping 675 matching lines @@
     return 1
   except (gclient_utils.Error, subprocess2.CalledProcessError), e:
     print >> sys.stderr, e
     return 1
   return 0
 
 
 if __name__ == "__main__":
   fix_encoding.fix_encoding()
   sys.exit(TryChange(None, None, False))