Changes to roll_deps.py

tools/roll_deps.py

Index: tools/roll_deps.py
diff --git a/tools/roll_deps.py b/tools/roll_deps.py
index 42b917415c6bca83c3ff628f2916c52bd9d8ce62..4250088ad269ce9cfceef1574ba99a1555520c34 100755
--- a/tools/roll_deps.py
+++ b/tools/roll_deps.py
@@ -125,8 +125,12 @@ class DepsRollConfig(object):
             '', '--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)
@@ -268,6 +272,22 @@ def find_revision_and_hash(config, revision):
             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.
 
@@ -313,6 +333,10 @@ class GitBranchCLUpload(object):
         """
         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):
@@ -322,6 +346,18 @@ class GitBranchCLUpload(object):
         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'])
@@ -342,11 +378,7 @@ class GitBranchCLUpload(object):
         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
@@ -457,18 +489,61 @@ def roll_deps(config, revision, git_hash):
         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:
@@ -478,17 +553,22 @@ def roll_deps(config, revision, git_hash):
         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
@@ -496,8 +576,6 @@ def roll_deps(config, revision, git_hash):
             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):