my_activity: Port gerrit to the new gerrit_util API.

gerrit_util.py

Index: gerrit_util.py
diff --git a/gerrit_util.py b/gerrit_util.py
index 7c44f03107468de934940bd1bf7646630818c53f..78998e7f547c871b3d74dea7175b73e51f65f201 100755
--- a/gerrit_util.py
+++ b/gerrit_util.py
@@ -65,11 +65,14 @@ def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
   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')
@@ -84,7 +87,7 @@ def CreateHttpConn(host, path, reqtype='GET', headers=None, body=None):
   conn = GetConnectionClass()(host)
   conn.req_host = host
   conn.req_params = {
-      'url': '/a/%s' % path,
+      'url': url,
       'method': reqtype,
       'headers': headers,
       'body': body,
@@ -180,6 +183,42 @@ def QueryChanges(host, param_dict, first_param=None, limit=None, o_params=None,
   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."""