Merge lp:~milo/linaro-patchmetrics/update-patches-fix into lp:linaro-patchmetrics

[Milo Casagrande]

This MP aims at fixing small issues experienced when running the update-committed-patches.py script from patchwork.
For general information about the changes, please refer to the commit message.

What was not captured in that message is:
- Some PEP8 fixes
- New settings introduced used to define a file path where to store the SQLite DB

[lp:~milo/linaro-patchmetrics/update-patches-fix]

383. By Milo Casagrande on 2013-09-19

Fixed output collection for rev-list git command.

384. By Milo Casagrande on 2013-09-20

Fixed missing exception.

385. By Milo Casagrande on 2013-10-16

Fixed formatting.

386. By Milo Casagrande on 2013-10-16

Merged from trunk.

387. By Milo Casagrande on 2013-10-16

Removed unused imports, added comment.

388. By Milo Casagrande on 2013-10-16

Merged from trunk.

389. By Milo Casagrande on 2013-10-16

Updated to use new Crowd code.

390. By Milo Casagrande on 2013-10-17

Merged from trunk.

391. By Milo Casagrande on 2013-12-03

Merged from trunk.

[Paul Sokolovsky (pfalcon)]

r383: It says it "Fixed output collection for rev-list git command.", but it's unclear what exactly is fixed. Replacement code doesn't look to be better then original, it actually gotta have stdout/stderr read deadlock bug. But it's definitely more verbose and magic-ridden (like use of 2-arg iter, "b" string prefix etc.) Please consider these rhetoric remarks - if such changes are done, it's nice to give more details, though this specific case is minor.

[Paul Sokolovsky (pfalcon)]

84 + if settings.AUTH_CROWD_APPLICATION_USER is None:
85 + print >> sys.stderr, ("CROWD_CREDENTIALS_FILE not defined in "
86 + "settings.py.\n")

apparently cut&paste error in msg.

[Paul Sokolovsky (pfalcon)]

Otherwise, logic looks good, but there really should be expiration of old cached users.

review: Approve

[Milo Casagrande (milo)]

On Wed, Dec 4, 2013 at 6:18 PM, Paul Sokolovsky
<email address hidden> wrote:
> Review: Approve
>
> Otherwise, logic looks good, but there really should be expiration of old cached users.

Thanks Paul,

I'll work on that and propose another merge.

--
Milo Casagrande | Automation Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCsThanks Paul,

[lp:~milo/linaro-patchmetrics/update-patches-fix]

392. By Milo Casagrande on 2013-12-05

Fixed print statement.

=== modified file 'apps/patchmetrics/bin/get-linaro-membership.py'
--- apps/patchmetrics/bin/get-linaro-membership.py	2013-10-17 15:53:51 +0000
+++ apps/patchmetrics/bin/get-linaro-membership.py	2013-12-05 10:08:30 +0000
@@ -48,7 +48,6 @@
     linaro = lp.people['linaro']
 
     if settings.AUTH_CROWD_APPLICATION_USER:
-        print "Using Crowd groups"
         whitelisted_groups = []
         if settings.CROWD_GROUPS_WHITELIST is None:
             print "No CROWD_GROUPS_WHITELIST defined, all groups will be used."

=== modified file 'apps/patchmetrics/crowd.py'
--- apps/patchmetrics/crowd.py	2013-09-27 16:32:30 +0000
+++ apps/patchmetrics/crowd.py	2013-12-05 10:08:30 +0000
@@ -20,7 +20,6 @@
 import base64
 import httplib
 import urllib
-import ConfigParser
 import json
 import types
 
@@ -182,6 +181,26 @@
 
         return user
 
+    def is_valid_user(self, email):
+        """Handy function to check if a user exists or not.
+
+        :param email: The user email.
+        :return True or False.
+        """
+        params = {"username": email}
+
+        resource = "/user?{0}".format(urllib.urlencode(params))
+        api_url = "/crowd/rest/usermanagement/1{0}".format(resource)
+
+        valid = True
+        try:
+            self._get(api_url)
+        except CrowdNotFoundException:
+            # In case of other exceptions, raise them.
+            valid = False
+
+        return valid
+
     def _get_rest_usermanagement(self, resource):
         api_url = "/{0}{1}".format(self._uri, resource)
         return self._get(api_url)
@@ -211,4 +230,5 @@
             raise CrowdForbiddenException(
                 "Access forbidden to fulfill the request")
         else:
-            raise CrowdException("Unknown Crowd status {0}".format(resp.status))
+            raise CrowdException(
+                "Unknown Crowd status {0}".format(resp.status))

=== modified file 'apps/patchwork/bin/update-committed-patches.py'
--- apps/patchwork/bin/update-committed-patches.py	2013-10-17 12:22:58 +0000
+++ apps/patchwork/bin/update-committed-patches.py	2013-12-05 10:08:30 +0000
@@ -10,26 +10,47 @@
 # accepted once they reach the master branch.
 
 import _pythonpath
+import atexit
 import sys
+
 from datetime import datetime
-from operator import attrgetter
-
-from lockfile import FileLock, LockFailed, LockTimeout
-
+from django.conf import settings
 from django.db.models import Q
 from dulwich.repo import Repo
+from lockfile import FileLock, LockFailed, LockTimeout
+from operator import attrgetter
 
+from patchmetrics.crowd import Crowd
+from patchwork.db import PatchworkDB
 from patchwork.models import Project, State
 from patchwork.utils import (
     ensure_source_checkout_for_project,
     get_commits_to_parse,
-    get_diff_for_commit,
+    get_diff_for_linaro_commit,
     get_patches_matching_commit,
     GitException
-    )
+)
 
 
 def main():
+
+    if settings.AUTH_CROWD_APPLICATION_USER is None:
+        print >> sys.stderr, ("AUTH_CROWD_APPLICATION_USER not defined in "
+                              "settings.py.\n")
+        sys.exit(1)
+    else:
+        crwd = Crowd(settings.AUTH_CROWD_APPLICATION_USER,
+                     settings.AUTH_CROWD_APPLICATION_PASSWORD,
+                     settings.AUTH_CROWD_SERVER_REST_URI)
+
+    if settings.CROWD_USERS_DB_FILE is None:
+        print >> sys.stderr, ("CROWD_USERS_DB_FILE not defined in "
+                              "settings.py.\n")
+        sys.exit(1)
+
+    cache_db = PatchworkDB(settings.CROWD_USERS_DB_FILE)
+    atexit.register(cache_db.close)
+
     projects = Project.objects.exclude(
         Q(source_tree__isnull=True) | Q(source_tree=''))
 
@@ -66,7 +87,7 @@
         for commit_id in commit_ids:
             project.last_seen_commit_ref = commit_id
             project.save()
-            diff = get_diff_for_commit(root, commit_id)
+            diff = get_diff_for_linaro_commit(root, commit_id, crwd, cache_db)
             if diff is None:
                 # Some commits result in empty diffs (e.g. only changing file
                 # permissions); in these cases we just skip them.

=== added file 'apps/patchwork/db.py'
--- apps/patchwork/db.py	1970-01-01 00:00:00 +0000
+++ apps/patchwork/db.py	2013-12-05 10:08:30 +0000
@@ -0,0 +1,120 @@
+# Copyright (C) 2013 Linaro
+#
+# Author: Milo Casagrande <[e-mail]>
+# This file is part of the Patchmetrics package.
+#
+# Patchmetrics is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# Patchmetrics is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Patchwork; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+
+import sqlite3
+
+
+class PatchworkDB(object):
+    """A cache backed by a SQLite DB to store users.
+
+    This is used to provide a users cache in order to speed up the process
+    of analyzing patches from a git repository.
+
+    Users are stored with their email address, and a boolean value indicating
+    if the user is a valid Linaro user. A valid Linaro user is a user that
+    exists in Linaro Login (Crowd).
+
+    There is no user logic here, it is just used as a cache backend.
+    """
+
+    def __init__(self, db_file):
+        self.db_file = db_file
+        self._connection = None
+        self._cursor = None
+
+        self._init_db()
+
+    def _init_db(self):
+        if not self.table_exists('users'):
+            self.cursor.execute('''CREATE TABLE users
+                (id TEXT PRIMARY KEY, valid INTEGER, ts TIMESTAMP)''')
+
+    def close(self):
+        """Closes the DB connection."""
+        # Force a commit, even if we are in autocommit mode.
+        self.save()
+        self.connection.close()
+
+    def save(self):
+        """Performs a commit on the DB."""
+        self.connection.commit()
+
+    @property
+    def connection(self):
+        """The connection to the DB."""
+        if self._connection is None:
+            self._connection = sqlite3.connect(self.db_file)
+        return self._connection
+
+    @property
+    def cursor(self):
+        """The connection cursor."""
+        if self._cursor is None:
+            self._cursor = self.connection.cursor()
+        return self._cursor
+
+    def table_exists(self, table):
+        """Checks if a table exists in the DB.
+
+        :param table: The name of the table to check.
+        """
+        self.cursor.execute('''SELECT name FROM sqlite_master WHERE
+            type='table' AND name=?''', [table])
+
+        exists = False
+        if self.cursor.fetchone():
+            exists = True
+        return exists
+
+    def insert_user(self, email, valid, timestamp):
+        """Inserts a user in the DB.
+
+        :param email: The user email, this is the unique key in the DB.
+        :param valid: If the user is a valid Linaro user.
+        :param timestamp: When the user was added in the cache.
+        """
+        self.cursor.execute('''INSERT INTO users(id, valid, ts)
+            VALUES (?, ?, ?)''', (email, valid, timestamp))
+
+    def update_user(self, email, valid, timestamp):
+        """Updates a user in the DB."""
+        self.cursor.execute('''UPDATE users SET valid=?, ts=? WHERE id=?''',
+                            (valid, timestamp, email))
+
+    def get_user(self, email):
+        """Retrieves a user from the DB.
+
+        :param email: The user email.
+        :return A tuple with the data found, None otherwise.
+        """
+        self.cursor.execute('''SELECT * FROM users where id=?''', [email])
+        return self.cursor.fetchone()
+
+    def user_exists(self, email):
+        """Verifies if a user exists in the DB.
+
+        :param email: The user email.
+        :return True or False.
+        """
+        result = self.get_user(email)
+
+        exists = False
+        if result:
+            exists = True
+        return exists

=== modified file 'apps/patchwork/parser.py'
--- apps/patchwork/parser.py	2012-10-18 09:28:17 +0000
+++ apps/patchwork/parser.py	2013-12-05 10:08:30 +0000
@@ -20,7 +20,9 @@
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
 
+import StringIO
 import re
+import types
 
 try:
     import hashlib
@@ -32,7 +34,14 @@
 _hunk_re = re.compile('^\@\@ -\d+(?:,(\d+))? \+\d+(?:,(\d+))? \@\@')
 _filename_re = re.compile('^(---|\+\+\+) (\S+)')
 
+
 def parse_patch(text):
+    text_file = None
+    if isinstance(text, types.StringTypes):
+        text_file = StringIO.StringIO(text)
+    else:
+        text_file = text
+
     patchbuf = ''
     commentbuf = ''
     buf = ''
@@ -63,20 +72,15 @@
     lc = (0, 0)
     hunk = 0
 
-
-    for line in text.split('\n'):
-        line += '\n'
-
+    for line in text_file:
         if state == 0:
             if line.startswith('diff ') or line.startswith('===') \
                     or line.startswith('Index: '):
                 state = 1
                 buf += line
-
             elif line.startswith('--- '):
                 state = 2
                 buf += line
-
             else:
                 commentbuf += line
 
@@ -89,11 +93,9 @@
             if line.startswith('+++ '):
                 state = 3
                 buf += line
-
             elif hunk:
                 state = 1
                 buf += line
-
             else:
                 state = 0
                 commentbuf += buf + line
@@ -102,7 +104,6 @@
         elif state == 3:
             match = _hunk_re.match(line)
             if match:
-
                 def fn(x):
                     if not x:
                         return 1
@@ -113,16 +114,13 @@
                 state = 4
                 patchbuf += buf + line
                 buf = ''
-
             elif line.startswith('--- '):
                 patchbuf += buf + line
                 buf = ''
                 state = 2
-
             elif hunk:
                 state = 1
                 buf += line
-
             else:
                 state = 0
                 commentbuf += buf + line
@@ -161,6 +159,7 @@
 
     return (patchbuf, commentbuf)
 
+
 def hash_patch(str):
     # normalise spaces
     str = str.replace('\r', '')
@@ -213,12 +212,12 @@
     from optparse import OptionParser
 
     parser = OptionParser()
-    parser.add_option('-p', '--patch', action = 'store_true',
-            dest = 'print_patch', help = 'print parsed patch')
-    parser.add_option('-c', '--comment', action = 'store_true',
-            dest = 'print_comment', help = 'print parsed comment')
-    parser.add_option('-#', '--hash', action = 'store_true',
-            dest = 'print_hash', help = 'print patch hash')
+    parser.add_option('-p', '--patch', action='store_true',
+                      dest='print_patch', help='print parsed patch')
+    parser.add_option('-c', '--comment', action='store_true',
+                      dest='print_comment', help='print parsed comment')
+    parser.add_option('-#', '--hash', action='store_true',
+                      dest='print_hash', help='print patch hash')
 
     (options, args) = parser.parse_args()
 

=== modified file 'apps/patchwork/utils.py'
--- apps/patchwork/utils.py	2013-10-16 12:29:19 +0000
+++ apps/patchwork/utils.py	2013-12-05 10:08:30 +0000
@@ -17,8 +17,10 @@
 # along with Patchwork; if not, write to the Free Software
 # Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
 
+import StringIO
 import atexit
 import chardet
+import datetime
 import os
 import re
 import subprocess
@@ -395,6 +397,123 @@
     return patch
 
 
+def get_diff_for_linaro_commit(root, commit_id, crowd, db):
+    """Gets a diff from a commit id for a Linaro made change.
+
+    If the author or the committer of the commit is not a Linaro user, then
+    the commit will not be considered.
+
+    This is a slightly different implementation than `get_diff_for_commit`,
+    where here it checks if the user is a valid Linaro account before taking
+    in account the patch.
+
+    :param root: The path on the file system where the repository is stored.
+    :param commit_id: The commmit ID to parse.
+    :param crowd: A Crowd object used to determine if the user is from Linaro.
+    :param db: A PatchworkDB object used as a cache for users.
+    """
+    stdout = ""
+    patch = None
+    author = None
+    committer = None
+
+    # We create a custom view on the commit id diff. We extract the author
+    # and committer emails, and check if they are from Linaro people, otherwise
+    # we do not even consider the commit and move one.
+    # Most of the time patchwork/patchmetrics spends a lot of time calculating
+    # diffs for patches that might not even be from Linaro people.
+    # The first format prints on stdout the author and commiter emails.
+    # The rest of the format string is exactly as what is shown with a simple
+    # `git show COMMIT_ID`.
+    format_string = "%ae%n%ce%n"
+    format_string += "commit %H%n"
+    format_string += "Author: %aN <%aE>%n"
+    format_string += "Date: %ad%n"
+
+    cmd_args = ['git', 'show', '--format=format:{0}'.format(format_string),
+                commit_id]
+    proc = subprocess.Popen(cmd_args, cwd=root, bufsize=4096,
+                            stdout=subprocess.PIPE, stderr=DEVNULL)
+
+    while True:
+        for line in iter(proc.stdout.readline, b''):
+            stdout += line
+        if proc.poll() is not None:
+            break
+
+    if proc.returncode != 0:
+        print ("Error retrieving diff for commit {0}".format(commit_id))
+    else:
+        # Since with big diffs a lot of time is spent decoding the output from
+        # the git command, we first read two lines from the output (author and
+        # commiter emails), perform validations on those, and in case one of
+        # them is a valid Linaro user we continue with output decoding and
+        # diff parsing.
+        string_file = StringIO.StringIO(stdout)
+
+        author = string_file.readline().strip().decode('utf-8')
+        committer = string_file.readline().strip().decode('utf-8')
+
+        valid_patch = False
+        if author == committer:
+            valid_patch |= is_linaro_user(author, db, crowd)
+        else:
+            valid_patch |= is_linaro_user(author, db, crowd)
+            valid_patch |= is_linaro_user(committer, db, crowd)
+
+        if valid_patch:
+            try:
+                diff = stdout.decode('utf-8')
+            except UnicodeDecodeError:
+                try:
+                    # chardet.detect is rather slow so we only use it when we
+                    # fail # to decode from utf-8.
+                    encoding = chardet.detect(stdout)['encoding']
+                    diff = stdout.decode(encoding)
+                except UnicodeDecodeError:
+                    print ("Skipping commit {0} as we can't find the "
+                           "appropriate charset to decode "
+                           "it".format(commit_id))
+                    return None
+
+            # This is a double action, but it is still faster then decoding
+            # everytime the output even when the patch does not come from a
+            # Linaro user.
+            string_file = StringIO.StringIO(diff)
+            # We need to pass the StringIO object pointing to the 3rd line
+            # since the first two have already been read, and they contains
+            # information not needed for the patch.
+            for i in range(0, 2):
+                string_file.readline()
+            patch, _ = parse_patch(string_file)
+
+    return patch
+
+
+def is_linaro_user(user, db, crowd):
+    """Verifies if a user is a valid Linaro account.
+
+    :param user: The email address to verify.
+    :param db: A PatchworkDB instance for the cache backend.
+    :param crowd: A Crowd instance to retrieve user data if necessary.
+    """
+    cached_user = db.get_user(user)
+
+    is_valid = False
+    if cached_user:
+        # TODO: we need to set a timeout and check again even if the user
+        # is cached, it might have been removed from Crowd.
+        is_valid |= cached_user[1]
+    else:
+        # XXX: The first time this runs, it might hit the Crowd server hard,
+        # since none of the users will be cached.
+        valid_user = crowd.is_valid_user(user)
+        is_valid |= valid_user
+        db.insert_user(user, valid_user, datetime.datetime.now())
+
+    return is_valid
+
+
 def get_patches_matching_commit(project, commit_diff, commit_msg,
                                 commit_author):
     """Return the Patch objects that are likely matches to the given commit.

=== modified file 'apps/settings.py'
--- apps/settings.py	2013-10-23 19:18:06 +0000
+++ apps/settings.py	2013-12-05 10:08:30 +0000
@@ -147,6 +147,9 @@
 # File with a list of groups that should be created in the DB. If other groups
 # are taken out from Crowd, and they are not here, they will be skipped.
 CROWD_GROUPS_WHITELIST = None
+# Full path to a DB file.
+# This is used to have a cache of which users exists in the Crowd database.
+CROWD_USERS_DB_FILE = None
 
 # Where to store the git repos that we scan to identify which patches have
 # been committed.