Validate a branch that we parse when running cherry_picker --continue (#266)

abadger · Mariatta · commit c3412349efc6 · 2018-07-12T19:33:16.000-07:00
When running cherry_picker --continue we count on being able to get
certain information from the branch's name which cherry_picker
constructed earlier.  Verify as best we can that the branch name is one
which cherry_picker could have constructed.
diff --git a/cherry_picker/cherry_picker/cherry_picker.py b/cherry_picker/cherry_picker/cherry_picker.py
@@ -74,12 +74,7 @@ def upstream(self):
 
     @property
     def sorted_branches(self):
-        def version_from_branch(branch):
-            try:
-                return tuple(map(int, re.match(r'^.*(?P<version>\d+(\.\d+)+).*$', branch).groupdict()['version'].split('.')))
-            except AttributeError as attr_err:
-                raise ValueError(f'Branch {branch} seems to not have a version in its name.') from attr_err
-
+        """Return the branches to cherry-pick to, sorted by version."""
         return sorted(
             self.branches,
             reverse=True,
@@ -354,12 +349,15 @@ def continue_cherry_pick(self):
             click.echo(f"Current branch ({cherry_pick_branch}) is not a backport branch.  Will not continue. \U0001F61B")
 
     def check_repo(self):
-        # CPython repo has a commit with
-        # SHA=7f777ed95a19224294949e1b4ce56bbffcb1fe9f
-        cmd = ['git', 'log', '-r', self.config['check_sha']]
+        """
+        Check that the repository is for the project we're configured to operate on.
+
+        This function performs the check by making sure that the sha specified in the config
+        is present in the repository that we're operating on.
+        """
         try:
-            subprocess.check_output(cmd, stderr=subprocess.STDOUT)
-        except subprocess.SubprocessError:
+            validate_sha(self.config['check_sha'])
+        except ValueError:
             raise InvalidRepoException()
 
 
@@ -421,11 +419,52 @@ def cherry_pick_cli(dry_run, pr_remote, abort, status, push, config_path,
 def get_base_branch(cherry_pick_branch):
     """
     return '2.7' from 'backport-sha-2.7'
+
+    raises ValueError if the specified branch name is not of a form that
+        cherry_picker would have created
     """
     prefix, sha, base_branch = cherry_pick_branch.split('-', 2)
+
+    if prefix != 'backport':
+        raise ValueError('branch name is not prefixed with "backport-".  Is this a cherry_picker branch?')
+
+    if not re.match('[0-9a-f]{7,40}', sha):
+        raise ValueError(f'branch name has an invalid sha: {sha}')
+
+    # Validate that the sha refers to a valid commit within the repo
+    # Throws a ValueError if the sha is not present in the repo
+    validate_sha(sha)
+
+    # Subject the parsed base_branch to the same tests as when we generated it
+    # This throws a ValueError if the base_branch doesn't meet our requirements
+    version_from_branch(base_branch)
+
     return base_branch
 
 
+def validate_sha(sha):
+    """
+    Validate that a hexdigest sha is a valid commit in the repo
+
+    raises ValueError if the sha does not reference a commit within the repo
+    """
+    cmd = ['git', 'log', '-r', sha]
+    try:
+        subprocess.check_output(cmd, stderr=subprocess.STDOUT)
+    except subprocess.SubprocessError:
+        raise ValueError(f'The sha listed in the branch name, {sha}, is not present in the repository')
+
+
+def version_from_branch(branch):
+    """
+    return version information from a git branch name
+    """
+    try:
+        return tuple(map(int, re.match(r'^.*(?P<version>\d+(\.\d+)+).*$', branch).groupdict()['version'].split('.')))
+    except AttributeError as attr_err:
+        raise ValueError(f'Branch {branch} seems to not have a version in its name.') from attr_err
+
+
 def get_current_branch():
     """
     Return the current branch
diff --git a/cherry_picker/cherry_picker/test.py b/cherry_picker/cherry_picker/test.py
@@ -32,19 +32,36 @@ def changedir(d):
     os.chdir(cwd)
 
 
-def test_get_base_branch():
-    # The format of cherry-pick branches we create are "backport-{SHA}-{base_branch}"
-    cherry_pick_branch = 'backport-afc23f4-2.7'
+@mock.patch('subprocess.check_output')
+def test_get_base_branch(subprocess_check_output):
+    # The format of cherry-pick branches we create are::
+    #     backport-{SHA}-{base_branch}
+    subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
+    cherry_pick_branch = 'backport-22a594a-2.7'
     result = get_base_branch(cherry_pick_branch)
     assert result == '2.7'
 
 
-def test_get_base_branch_which_has_dashes():
-    cherry_pick_branch ='backport-afc23f4-baseprefix-2.7-basesuffix'
+@mock.patch('subprocess.check_output')
+def test_get_base_branch_which_has_dashes(subprocess_check_output):
+    subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
+    cherry_pick_branch = 'backport-22a594a-baseprefix-2.7-basesuffix'
     result = get_base_branch(cherry_pick_branch)
     assert result == 'baseprefix-2.7-basesuffix'
 
 
+@pytest.mark.parametrize('cherry_pick_branch', ['backport-22a594a',  # Not enough fields
+                                                'prefix-22a594a-2.7',  # Not the prefix we were expecting
+                                                'backport-22a594a-base',  # No version info in the base branch
+                                                ]
+                         )
+@mock.patch('subprocess.check_output')
+def test_get_base_branch_invalid(subprocess_check_output, cherry_pick_branch):
+    subprocess_check_output.return_value = b'22a594a0047d7706537ff2ac676cdc0f1dcb329c'
+    with pytest.raises(ValueError):
+        get_base_branch(cherry_pick_branch)
+
+
 @mock.patch('subprocess.check_output')
 def test_get_current_branch(subprocess_check_output):
     subprocess_check_output.return_value = b'master'
