Changes to roll_deps.py

tools/roll_deps.py

 #!/usr/bin/python2
 
 # Copyright 2014 Google Inc.
 #
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """Skia's Chromium DEPS roll script.
 
 This script:
@@ skipping 107 matching lines @@
             ' a temporary will be cloned.  Defaults to SKIA_GIT_CHECKOUT'
             '_PATH, if that environment variable is set.',
             default=os.environ.get('SKIA_GIT_CHECKOUT_PATH'))
         option_parser.add_option(
             '', '--search_depth', type='int', default=100,
             help='How far back to look for the revision.')
         option_parser.add_option(
             '', '--git_path', help='Git executable, defaults to "git".',
             default='git')
         option_parser.add_option(
-            '', '--save_branches', help='Save the temporary branches',
-            action='store_true', dest='save_branches', default=False)
+            '', '--save_branches',
+            help='Save the temporary branches (default)',
+            action='store_true', dest='save_branches', default=True)
+        option_parser.add_option(
+            '', '--delete_branches', help='Delete the temporary branches',
+            action='store_false', dest='save_branches', default=True)

[borenet, 2014/01/07 21:39:34]

If these are changing the same variable, why do we need both?  Can we only
include the flag that gives the non-default option?

[hal.canary, 2014/01/08 18:57:38]

Done.

         option_parser.add_option(
             '', '--verbose', help='Do not suppress the output from `git cl`.',
             action='store_true', dest='verbose', default=False)
         option_parser.add_option(
             '', '--skip_cl_upload', help='Skip the cl upload step; useful'
             ' for testing or with --save_branches.',
             action='store_true', default=False)
 
         default_bots_help = (
             'Comma-separated list of bots, defaults to a list of %d bots.'
@@ skipping 121 matching lines @@
                  'origin/master'], cwd=skia_dir)
 
         if revision < 0  or not git_hash:
             raise DepsRollError('Git hash can not be found.')
         return revision, git_hash
     finally:
         if use_temp:
             shutil.rmtree(skia_dir)
 
 
+class ChangeDir(object):
+    """Use with a with-statement to temporarily change directories."""
+    # pylint: disable=I0011,R0903
+
+    def __init__(self, directory):
+        self._directory = directory
+
+    def __enter__(self):
+        cwd = os.getcwd()
+        os.chdir(self._directory)
+        self._directory = cwd
+
+    def __exit__(self, etype, value, traceback):
+        os.chdir(self._directory)
+
+
 class GitBranchCLUpload(object):
     """Class to manage git branches and git-cl-upload.
 
     This class allows one to create a new branch in a repository based
     off of origin/master, make changes to the tree inside the
     with-block, upload that new branch to Rietveld, restore the original
     tree state, and delete the local copy of the new branch.
 
     See roll_deps() for an example of use.
 
@@ skipping 25 matching lines @@
         self.issue = None
 
     def stage_for_commit(self, *paths):
         """Calls `git add ...` on each argument.
 
         Args:
             *paths: (list of strings) list of filenames to pass to `git add`.
         """
         self._file_list.extend(paths)
 
+    def set_message(self, message):
+        """Change the message."""
+        self._message = message
+
     def __enter__(self):
         git = self._config.git
         def branch_exists(branch):
             """Return true iff branch exists."""
             return 0 == subprocess.call(
                 [git, 'show-ref', '--quiet', branch])
         def has_diff():
             """Return true iff repository has uncommited changes."""
             return bool(subprocess.call([git, 'diff', '--quiet', 'HEAD']))
+        def get_svn_revision():
+            """Works in both git and git-svn. returns a string"""
+            last_log_message = subprocess.check_output(
+                [git, 'log', '-n', '1', '--format=format:%B'])
+            svn_format = (
+                '(git-svn-id: svn://svn.chromium.org/chrome/trunk/src@|'
+                'SVN changes up to revision )([0-9]+)')
+            search = re.search(svn_format, last_log_message)
+            if not search:
+                raise DepsRollError(
+                    'Revision number missing from Chromium origin/master.')
+            return search.group(2)
         self._stash = has_diff()
         if self._stash:
             check_call([git, 'stash', 'save'])
         try:
             self._original_branch = strip_output(
                 [git, 'symbolic-ref', '--short', 'HEAD'])
         except (subprocess.CalledProcessError,):
             self._original_branch = strip_output(
                 [git, 'rev-parse', 'HEAD'])
 
         if not self._branch_name:
             self._branch_name = self._config.default_branch_name
 
         if branch_exists(self._branch_name):
             check_call([git, 'checkout', '-q', 'master'])
             check_call([git, 'branch', '-q', '-D', self._branch_name])
 
         check_call(
             [git, 'checkout', '-q', '-b',
              self._branch_name, 'origin/master'])
-
-        svn_info = subprocess.check_output(['git', 'svn', 'info'])
-        svn_info_search = re.search(r'Last Changed Rev: ([0-9]+)\W', svn_info)
-        assert svn_info_search
-        self._svn_info = svn_info_search.group(1)
+        self._svn_info = get_svn_revision()
 
     def __exit__(self, etype, value, traceback):
         # pylint: disable=I0011,R0912
         git = self._config.git
         def quiet_check_call(*args, **kwargs):
             """Call check_call, but pipe output to devnull."""
             with open(os.devnull, 'w') as devnull:
                 check_call(*args, stdout=devnull, **kwargs)
 
         for filename in self._file_list:
@@ skipping 90 matching lines @@
         revision: (int) Skia SVN revision.
         git_hash: (string) Skia Git hash.
 
     Returns:
         a tuple containing textual description of the two issues.
 
     Raises:
         OSError: failed to execute git or git-cl.
         subprocess.CalledProcessError: git returned unexpected status.
     """
+    def search_within_file(path, pattern, default=None):

[borenet, 2014/01/07 21:31:12]

I think this function could live outside of roll_deps, even if it's only used
here.  Personally, I find that nested functions which are more than a couple of
lines disrupt the visual flow and make things harder to read.  This particular
function seems like it could be useful within a separate, misc utility module.

[hal.canary, 2014/01/08 18:57:38]

Done.

It's in a utility class.  If you want to make a utility module, give us a second
client for it.

+        """Search for regular expression in a file.
+
+        Opens a file for reading and searches line by line for a match to
+        the regex and returns the parenthesized group named return for the
+        first match.

[borenet, 2014/01/07 21:31:12]

Please document that the pattern can't contain newlines.

[hal.canary, 2014/01/08 18:57:38]

Done.

+
+        For example:
+            pattern = '^root(:[^:]*){4}:(?P<return>[^:]*)'
+            search_within_file('/etc/passwd', pattern)
+        should return root's home directory (/root on my system).
+
+        Args:
+            path: (string) filename
+            pattern: (string) to be passed to re.compile
+            default: what to return if no match
+
+        Returns:
+            A string or whatever default is
+        """
+        with open(path, 'r') as input_stream:
+            pattern_object = re.compile(pattern)
+            for line in input_stream:
+                match = pattern_object.search(line)
+                if match:
+                    return match.group('return')
+        return default
+
+    def search_within_string(input_string, pattern, default=None):

[borenet, 2014/01/07 21:31:12]

Same comment about this function: I think it could go at the module level.

[hal.canary, 2014/01/08 18:57:38]

Done.

+        """Search for regular expression in a string.
+
+        Args:
+            input_string: (string) to be searched
+            pattern: (string) to be passed to re.compile
+            default: what to return if no match
+
+        Returns:
+            A string or whatever default is
+        """
+        match = re.search(pattern, input_string)
+        return match.group('return') if match else default
+
     git = config.git
-    cwd = os.getcwd()
-    os.chdir(config.chromium_path)
-    try:
+    with ChangeDir(config.chromium_path):
         check_call([git, 'fetch', '-q', 'origin'])
         master_hash = strip_output(
             [git, 'show-ref', 'origin/master', '--hash'])
 
+        branch = None
+
         # master_hash[8] gives each whitespace CL a unique name.
         message = ('whitespace change %s\n\nThis CL was created by'
                    ' Skia\'s roll_deps.py script.\n') % master_hash[:8]
-        branch = branch_name(message) if config.save_branches else None
+        if config.save_branches:
+            branch = 'control_%s' % master_hash[:8]

[borenet, 2014/01/07 21:31:12]

Maybe we should have a short_hash(hash) function?

[hal.canary, 2014/01/08 18:57:38]

In Python, string[:8] is clear enough.

 
         codereview = GitBranchCLUpload(config, message, branch)
         with codereview:
             with open('build/whitespace_file.txt', 'a') as output_stream:
                 output_stream.write('\nCONTROL\n')
             codereview.stage_for_commit('build/whitespace_file.txt')
         whitespace_cl = codereview.issue
         if branch:
             whitespace_cl = '%s\n    branch: %s' % (whitespace_cl, branch)
-        control_url_match = re.search('https?://[^) ]+', codereview.issue)
-        if control_url_match:
-            message = ('roll skia DEPS to %d\n\nThis CL was created by'
-                       ' Skia\'s roll_deps.py script.\n\ncontrol: %s'
-                       % (revision, control_url_match.group(0)))
-        else:
-            message = ('roll skia DEPS to %d\n\nThis CL was created by'
-                       ' Skia\'s roll_deps.py script.') % revision
-        branch = branch_name(message) if config.save_branches else None
-        codereview = GitBranchCLUpload(config, message, branch)
+
+        control_url = search_within_string(
+            codereview.issue, '(?P<return>https?://[^) ]+)', '?')
+
+        if config.save_branches:
+            branch = 'roll_%d_%s' % (revision, master_hash[:8])

[borenet, 2014/01/07 21:39:34]

I just found this: http://stackoverflow.com/a/6065944
Which gives some git branch naming conventions.  Groups of branches piqued my
interest... we could name the branches according to a scheme like this:

branch = 'depsroll/%s/%s' % (revision, 'roll' or 'control')

That way the branches would be organized into a convenient depsroll group, then
divided by revision, so that the roll and control are paired. Thoughts?

[hal.canary, 2014/01/08 18:57:38]

Good idea, but defering to robert.

+        codereview = GitBranchCLUpload(config, '?', branch)
         with codereview:
+            old_revision = search_within_file(
+                'DEPS', '"skia_revision": "(?P<return>[0-9]+)",', '?')
+            assert revision != int(old_revision)

[borenet, 2014/01/07 21:31:12]

Can we move the whitespace CL uploading to after the DEPS roll CL?  Otherwise,
we could leak whitespace CLs if this script is repeatedly run on up-to-date
code.

[hal.canary, 2014/01/08 18:57:38]

I moved that assert up.  And turned it into a check that returns early when
false.

+            codereview.set_message(
+                'roll skia DEPS to %d\n\nold revision:%s\n'
+                'new revision:%d\nThis CL was created by'
+                ' Skia\'s roll_deps.py script.\n\ncontrol: %s'
+                % (revision, old_revision, revision, control_url))
             change_skia_deps(revision, git_hash, 'DEPS')
             codereview.stage_for_commit('DEPS')
         deps_cl = codereview.issue
         if branch:
             deps_cl = '%s\n    branch: %s' % (deps_cl, branch)
 
         return deps_cl, whitespace_cl
-    finally:
-        os.chdir(cwd)
 
 
 def find_hash_and_roll_deps(config, revision):
     """Call find_hash_from_revision() and roll_deps().
 
     The calls to git will be verbose on standard output.  After a
     successful upload of both issues, print links to the new
     codereview issues.
 
     Args:
@@ skipping 32 matching lines @@
     if not test_git_executable(options.git_path):
         option_parser.error('Invalid git executable.')
 
     config = DepsRollConfig(options)
     find_hash_and_roll_deps(config, options.revision)
 
 
 if __name__ == '__main__':
     main(sys.argv[1:])