my_activity: Port gerrit to the new gerrit_util API.

gerrit_util.py

 # Copyright (c) 2013 The Chromium OS Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
 """
 Utilities for requesting information for a gerrit server via https.
 
 https://gerrit-review.googlesource.com/Documentation/rest-api.html
 """
 
@@ skipping 47 matching lines @@
   else:
     raise RuntimeError(
         "Don't know how to work with protocol '%s'" % protocol)
 
 
 def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
   """Opens an https connection to a gerrit service, and sends a request."""
   headers = headers or {}
   bare_host = host.partition(':')[0]
   auth = NETRC.authenticators(bare_host)
+
   if auth:
     headers.setdefault('Authorization', 'Basic %s' % (
         base64.b64encode('%s:%s' % (auth[0], auth[2]))))
+    url = '/a/%s' % path

[szager1, 2013/10/31 18:15:45]

It's always possible that the Authorization header will be passed to this
method, rather than taken from NETRC.  So, it's better to:

 if auth:
   headers.setdefault(...)
 else:
   LOGGER.debug('No authorization found in netrc')
 if 'Authorization' in headers and not path.startswith('a/'):
   url = '/a/%s' % path
 else:
   url = '/%s' % path

[deymo, 2013/10/31 20:42:12]

Done.

   else:
     LOGGER.debug('No authorization found')
+    url = '/%s' % path
   if body:
     body = json.JSONEncoder().encode(body)
     headers.setdefault('Content-Type', 'application/json')
   if LOGGER.isEnabledFor(logging.DEBUG):
     LOGGER.debug('%s %s://%s/a/%s' % (reqtype, GERRIT_PROTOCOL, host, path))

[szager1, 2013/10/31 18:15:45]

Use the new 'url' variable.

[deymo, 2013/10/31 20:42:12]

Done.

     for key, val in headers.iteritems():
       if key == 'Authorization':
         val = 'HIDDEN'
       LOGGER.debug('%s: %s' % (key, val))
     if body:
       LOGGER.debug(body)
   conn = GetConnectionClass()(host)
   conn.req_host = host
   conn.req_params = {
-      'url': '/a/%s' % path,
+      'url': url,
       'method': reqtype,
       'headers': headers,
       'body': body,
   }
   conn.request(**conn.req_params)
   return conn
 
 
 def ReadHttpResponse(conn, expect_status=200, ignore_404=True):
   """Reads an http response from a connection into a string buffer.
@@ skipping 75 matching lines @@
   if sortkey:
     path = '%s&N=%s' % (path, sortkey)
   if limit:
     path = '%s&n=%d' % (path, limit)
   if o_params:
     path = '%s&%s' % (path, '&'.join(['o=%s' % p for p in o_params]))
   # Don't ignore 404; a query should always return a list, even if it's empty.
   return ReadHttpJsonResponse(CreateHttpConn(host, path), ignore_404=False)
 
 
+def QueryAllChanges(host, param_dict, first_param=None, limit=500,

[szager1, 2013/10/31 18:15:45]

This method name is not suggestive of what the code does; it sounds like it's
going to return all changes on the server.  And generator methods should have
the word 'generate' in their name.

Maybe 'GenerateChanges' ?

Also, the parameter list and docstring need not be so verbose.  I suggest
something like this:

def GenerateChanges(*args, **kwargs):
  """
  Iterate over the results of a change query, automatically making continuation
requests if necessary.

  Args: Refer to QueryChanges, above.
  """

[deymo, 2013/10/31 20:42:12]

The point of having the documentation here is that you can access it not only
from the source file but also from the python interpreter (like with "python -c
'import gerrit_util; help(gerrit_util.QueryAllChanges)'"  or simply
"gerrit_util.QueryAllChanges?" if you are using ipython).

Having a pointer on the documentation and variadic arguments hides the real list
of arguments you can pass here, which is indeed fixed. On the other hand,
although the names of the arguments are the same, the meaning is not exactly the
same due to the nature of this function. The argument limit is less relevant
here: you can pass any value there and you will get the whole list anyway (at
least once the gerrit-team fixes all the bugs there ;-) ). The sortkey argument
is a starting point for the query, but other sortkey values will be passed to
QueryChanges from this function.

I'll try to make a balance here.

+                    o_params=None, sortkey=None):
+  """
+  Queries a gerrit-on-borg server for all the changes matching the query terms.
+
+  A single query to gerrit-on-borg is limited on the number of results by the
+  limit parameter on the request (see QueryChanges) and the server maximum
+  limit. This function uses the "_more_changes" and "_sortkey" attributes on
+  the returned changes to iterate all of them making multiple queries to the
+  server, regardless the query limit.
+
+  Args:
+    param_dict: A dictionary of search parameters, as documented here:
+        http://gerrit-documentation.googlecode.com/svn/Documentation/2.6/user-search.html
+    first_param: A change identifier.
+    limit: Maximum number of requested changes per query.
+    o_params: A list of additional output specifiers, as documented here:
+        https://gerrit-review.googlesource.com/Documentation/rest-api-changes.html#list-changes
+    sortkey: The value of the "_sortkey" attribute where starts from. None to
+        start from the first change.
+
+  Returns:
+    A generator object to the list of returned changes, possibly unbound.
+  """
+  more_changes = True
+  while more_changes:
+    page = QueryChanges(host, param_dict, first_param, limit, o_params, sortkey)
+    for cl in page:
+      yield cl
+
+    more_changes = [cl for cl in page if '_more_changes' in cl]

[szager1, 2013/10/31 18:15:45]

This logic is a bit obscure, since '_more_changes' should only be set on the
last cl.  The code would be more readable like this:

 while True:
   page = QueryChanges(...)
   for cl in page:
     yield cl
   if not page or '_more_changes' not in page[-1]:
     break
   sortkey = page[-1]['_sortkey']

[deymo, 2013/10/31 20:42:12]

I try to avoid the "while True:" loops with breaks in the middle. The logic in
those loops is more confusing because you need to follow execution to know when
does the loop stop. The "while more_changes:" clearly shows the intention of the
loop (run while there are more changes to fetch).

The loop's body is more robust because it accepts _more_changes anywhere and
continues from there. This is because gerrit supports the P query argument to
fetch the results in reverse mode. Even if QueryChanges doesn't currently use
the P query argument (just the N), getting the _sortkey value from wherever the
_more_changes was set seems more appropriate to me.

+    sortkey = None
+    if more_changes:
+      sortkey = more_changes[0]['_sortkey']
+
+
 def MultiQueryChanges(host, param_dict, change_list, limit=None, o_params=None,
                       sortkey=None):
   """Initiate a query composed of multiple sets of query parameters."""
   if not change_list:
     raise RuntimeError(
         "MultiQueryChanges requires a list of change numbers/id's")
   q = ['q=%s' % '+OR+'.join([urllib.quote(str(x)) for x in change_list])]
   if param_dict:
     q.append(_QueryString(param_dict))
   if limit:
@@ skipping 192 matching lines @@
         username = review.get('email', jmsg.get('name', ''))
         raise GerritError(200, 'Unable to set %s label for user "%s"'
                        ' on change %s.' % (label, username, change))
   jmsg = GetChangeCurrentRevision(host, change)
   if not jmsg:
     raise GerritError(
         200, 'Could not get review information for change "%s"' % change)
   elif jmsg[0]['current_revision'] != revision:
     raise GerritError(200, 'While resetting labels on change "%s", '
                    'a new patchset was uploaded.' % change)