diff hgext/phabricator.py @ 41897:c340a8ac7ef3

phabricator: convert conduit response JSON unicode to bytes inside callconduit

Previously the byte conversion was happening piecemeal in callers, and in the
case of createdifferentialrevision not at all, leading to UnicodeEncodeErrors
when trying to phabsend a commit with a description containing characters not
representable in ascii. (issue6040)

Remove all the scattered encoding.unitolocal calls and perform it once, inside
callconduit, on the entire response dict recursively before returning it, in
keeping with the strategy of converting at the earliest opportunity. Convert all
keys used on returned object to bytes.

Modify the phabsend tests to test this by adding a € to the commit message of
alpha.

Differential Revision: https://phab.mercurial-scm.org/D6044

--- a/hgext/phabricator.py	Sat Feb 09 23:01:30 2019 +0100
+++ b/hgext/phabricator.py	Sat Mar 02 18:48:23 2019 +0000
@@ -60,6 +60,7 @@
     parser,
     patch,
     phases,
+    pycompat,
     registrar,
     scmutil,
     smartset,
@@ -219,12 +220,16 @@
         with contextlib.closing(urlopener.open(request)) as rsp:
             body = rsp.read()
     repo.ui.debug(b'Conduit Response: %s\n' % body)
-    parsed = json.loads(body)
-    if parsed.get(r'error_code'):
+    parsed = pycompat.rapply(
+        lambda x: encoding.unitolocal(x) if isinstance(x, pycompat.unicode)
+        else x,
+        json.loads(body)
+    )
+    if parsed.get(b'error_code'):
         msg = (_(b'Conduit Error (%s): %s')
-               % (parsed[r'error_code'], parsed[r'error_info']))
+               % (parsed[b'error_code'], parsed[b'error_info']))
         raise error.Abort(msg)
-    return parsed[r'result']
+    return parsed[b'result']
 
 @vcrcommand(b'debugcallconduit', [], _(b'METHOD'))
 def debugcallconduit(ui, repo, name):
@@ -249,9 +254,9 @@
         return None
     query = callconduit(repo, b'diffusion.repository.search',
                         {b'constraints': {b'callsigns': [callsign]}})
-    if len(query[r'data']) == 0:
+    if len(query[b'data']) == 0:
         return None
-    repophid = encoding.strtolocal(query[r'data'][0][r'phid'])
+    repophid = query[b'data'][0][b'phid']
     repo.ui.setconfig(b'phabricator', b'repophid', repophid)
     return repophid
 
@@ -305,11 +310,11 @@
         drevs = [drev for force, precs, drev in toconfirm.values()]
         alldiffs = callconduit(unfi, b'differential.querydiffs',
                                {b'revisionIDs': drevs})
-        getnode = lambda d: bin(encoding.unitolocal(
-            getdiffmeta(d).get(r'node', b''))) or None
+        getnode = lambda d: bin(
+            getdiffmeta(d).get(b'node', b'')) or None
         for newnode, (force, precset, drev) in toconfirm.items():
             diffs = [d for d in alldiffs.values()
-                     if int(d[r'revisionID']) == drev]
+                     if int(d[b'revisionID']) == drev]
 
             # "precursors" as known by Phabricator
             phprecset = set(getnode(d) for d in diffs)
@@ -328,7 +333,7 @@
             # exists in the repo
             oldnode = lastdiff = None
             if diffs:
-                lastdiff = max(diffs, key=lambda d: int(d[r'id']))
+                lastdiff = max(diffs, key=lambda d: int(d[b'id']))
                 oldnode = getnode(lastdiff)
                 if oldnode and oldnode not in nodemap:
                     oldnode = None
@@ -361,7 +366,7 @@
 def writediffproperties(ctx, diff):
     """write metadata to diff so patches could be applied losslessly"""
     params = {
-        b'diff_id': diff[r'id'],
+        b'diff_id': diff[b'id'],
         b'name': b'hg:meta',
         b'data': json.dumps({
             b'user': ctx.user(),
@@ -373,7 +378,7 @@
     callconduit(ctx.repo(), b'differential.setdiffproperty', params)
 
     params = {
-        b'diff_id': diff[r'id'],
+        b'diff_id': diff[b'id'],
         b'name': b'local:commits',
         b'data': json.dumps({
             ctx.hex(): {
@@ -408,7 +413,7 @@
     transactions = []
     if neednewdiff:
         diff = creatediff(ctx)
-        transactions.append({b'type': b'update', b'value': diff[r'phid']})
+        transactions.append({b'type': b'update', b'value': diff[b'phid']})
     else:
         # Even if we don't need to upload a new diff because the patch content
         # does not change. We might still need to update its metadata so
@@ -433,7 +438,7 @@
     desc = ctx.description()
     info = callconduit(repo, b'differential.parsecommitmessage',
                        {b'corpus': desc})
-    for k, v in info[r'fields'].items():
+    for k, v in info[b'fields'].items():
         if k in [b'title', b'summary', b'testPlan']:
             transactions.append({b'type': k, b'value': v})
 
@@ -455,13 +460,13 @@
     result = callconduit(repo, b'user.search', query)
     # username not found is not an error of the API. So check if we have missed
     # some names here.
-    data = result[r'data']
-    resolved = set(entry[r'fields'][r'username'].lower() for entry in data)
+    data = result[b'data']
+    resolved = set(entry[b'fields'][b'username'].lower() for entry in data)
     unresolved = set(names) - resolved
     if unresolved:
         raise error.Abort(_(b'unknown username: %s')
                           % b' '.join(sorted(unresolved)))
-    return [entry[r'phid'] for entry in data]
+    return [entry[b'phid'] for entry in data]
 
 @vcrcommand(b'phabsend',
          [(b'r', b'rev', [], _(b'revisions to send'), _(b'REV')),
@@ -538,7 +543,7 @@
             revision, diff = createdifferentialrevision(
                 ctx, revid, lastrevid, oldnode, olddiff, actions)
             diffmap[ctx.node()] = diff
-            newrevid = int(revision[r'object'][r'id'])
+            newrevid = int(revision[b'object'][b'id'])
             if revid:
                 action = b'updated'
             else:
@@ -580,9 +585,8 @@
             for i, rev in enumerate(revs):
                 old = unfi[rev]
                 drevid = drevids[i]
-                drev = [d for d in drevs if int(d[r'id']) == drevid][0]
+                drev = [d for d in drevs if int(d[b'id']) == drevid][0]
                 newdesc = getdescfromdrev(drev)
-                newdesc = encoding.unitolocal(newdesc)
                 # Make sure commit message contain "Differential Revision"
                 if old.description() != newdesc:
                     if old.phase() == phases.public:
@@ -613,8 +617,8 @@
 
 # Map from "hg:meta" keys to header understood by "hg import". The order is
 # consistent with "hg export" output.
-_metanamemap = util.sortdict([(r'user', b'User'), (r'date', b'Date'),
-                              (r'node', b'Node ID'), (r'parent', b'Parent ')])
+_metanamemap = util.sortdict([(b'user', b'User'), (b'date', b'Date'),
+                              (b'node', b'Node ID'), (b'parent', b'Parent ')])
 
 def _confirmbeforesend(repo, revs, oldmap):
     url, token = readurltoken(repo)
@@ -644,7 +648,7 @@
 
 def _getstatusname(drev):
     """get normalized status name from a Differential Revision"""
-    return drev[r'statusName'].replace(b' ', b'').lower()
+    return drev[b'statusName'].replace(b' ', b'').lower()
 
 # Small language to specify differential revisions. Support symbols: (), :X,
 # +, and -.
@@ -762,8 +766,8 @@
         drevs = callconduit(repo, b'differential.query', params)
         # Fill prefetched with the result
         for drev in drevs:
-            prefetched[drev[r'phid']] = drev
-            prefetched[int(drev[r'id'])] = drev
+            prefetched[drev[b'phid']] = drev
+            prefetched[int(drev[b'id'])] = drev
         if key not in prefetched:
             raise error.Abort(_(b'cannot get Differential Revision %r')
                               % params)
@@ -777,12 +781,12 @@
         while queue:
             params = queue.pop()
             drev = fetch(params)
-            if drev[r'id'] in visited:
+            if drev[b'id'] in visited:
                 continue
-            visited.add(drev[r'id'])
-            result.append(int(drev[r'id']))
-            auxiliary = drev.get(r'auxiliary', {})
-            depends = auxiliary.get(r'phabricator:depends-on', [])
+            visited.add(drev[b'id'])
+            result.append(int(drev[b'id']))
+            auxiliary = drev.get(b'auxiliary', {})
+            depends = auxiliary.get(b'phabricator:depends-on', [])
             for phid in depends:
                 queue.append({b'phids': [phid]})
         result.reverse()
@@ -802,7 +806,7 @@
     for r in ancestordrevs:
         tofetch.update(range(max(1, r - batchsize), r + 1))
     if drevs:
-        fetch({r'ids': list(tofetch)})
+        fetch({b'ids': list(tofetch)})
     validids = sorted(set(getstack(list(ancestordrevs))) | set(drevs))
 
     # Walk through the tree, return smartsets
@@ -836,12 +840,12 @@
     This is similar to differential.getcommitmessage API. But we only care
     about limited fields: title, summary, test plan, and URL.
     """
-    title = drev[r'title']
-    summary = drev[r'summary'].rstrip()
-    testplan = drev[r'testPlan'].rstrip()
+    title = drev[b'title']
+    summary = drev[b'summary'].rstrip()
+    testplan = drev[b'testPlan'].rstrip()
     if testplan:
         testplan = b'Test Plan:\n%s' % testplan
-    uri = b'Differential Revision: %s' % drev[r'uri']
+    uri = b'Differential Revision: %s' % drev[b'uri']
     return b'\n\n'.join(filter(None, [title, summary, testplan, uri]))
 
 def getdiffmeta(diff):
@@ -881,17 +885,17 @@
     Note: metadata extracted from "local:commits" will lose time zone
     information.
     """
-    props = diff.get(r'properties') or {}
-    meta = props.get(r'hg:meta')
-    if not meta and props.get(r'local:commits'):
-        commit = sorted(props[r'local:commits'].values())[0]
+    props = diff.get(b'properties') or {}
+    meta = props.get(b'hg:meta')
+    if not meta and props.get(b'local:commits'):
+        commit = sorted(props[b'local:commits'].values())[0]
         meta = {
-            r'date': r'%d 0' % commit[r'time'],
-            r'node': commit[r'rev'],
-            r'user': r'%s <%s>' % (commit[r'author'], commit[r'authorEmail']),
+            b'date': b'%d 0' % commit[b'time'],
+            b'node': commit[b'rev'],
+            b'user': b'%s <%s>' % (commit[b'author'], commit[b'authorEmail']),
         }
-        if len(commit.get(r'parents', ())) >= 1:
-            meta[r'parent'] = commit[r'parents'][0]
+        if len(commit.get(b'parents', ())) >= 1:
+            meta[b'parent'] = commit[b'parents'][0]
     return meta or {}
 
 def readpatch(repo, drevs, write):
@@ -901,14 +905,14 @@
     "differential.query".
     """
     # Prefetch hg:meta property for all diffs
-    diffids = sorted(set(max(int(v) for v in drev[r'diffs']) for drev in drevs))
+    diffids = sorted(set(max(int(v) for v in drev[b'diffs']) for drev in drevs))
     diffs = callconduit(repo, b'differential.querydiffs', {b'ids': diffids})
 
     # Generate patch for each drev
     for drev in drevs:
-        repo.ui.note(_(b'reading D%s\n') % drev[r'id'])
+        repo.ui.note(_(b'reading D%s\n') % drev[b'id'])
 
-        diffid = max(int(v) for v in drev[r'diffs'])
+        diffid = max(int(v) for v in drev[b'diffs'])
         body = callconduit(repo, b'differential.getrawdiff',
                            {b'diffID': diffid})
         desc = getdescfromdrev(drev)
@@ -923,7 +927,7 @@
                 header += b'# %s %s\n' % (_metanamemap[k], meta[k])
 
         content = b'%s%s\n%s' % (header, desc, body)
-        write(encoding.unitolocal(content))
+        write(content)
 
 @vcrcommand(b'phabread',
          [(b'', b'stack', False, _(b'read dependencies'))],
@@ -979,7 +983,7 @@
         if i + 1 == len(drevs) and opts.get(b'comment'):
             actions.append({b'type': b'comment', b'value': opts[b'comment']})
         if actions:
-            params = {b'objectIdentifier': drev[r'phid'],
+            params = {b'objectIdentifier': drev[b'phid'],
                       b'transactions': actions}
             callconduit(repo, b'differential.revision.edit', params)