Patchwork D121: phabricator: use Phabricator's last node information

login
register
mail settings
Submitter phabricator
Date July 18, 2017, 4:20 a.m.
Message ID <differential-rev-PHID-DREV-yluckp2d3cl3vcexeib6-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22469/
State Superseded, archived
Headers show

Comments

phabricator - July 18, 2017, 4:20 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This makes it more strict when checking whether or not we should update a
  Differential Revision. For example,
  
    a) Alice updates D1 to content 1.
    b) Bob updates D1 to content 2.
    c) Alice tries to update D1 to content 1.
  
  Previously, `c)` will do nothing because `phabsend` detects the patch is not
  changed. A more correct behavior is to override Bob's update here, hence the
  patch.
  
  This also makes it possible to return a reaonsable "last node" when there is
  no tags but only `Differential Revision` commit messages.

TEST PLAN
    for i in A B C; do echo $i > $i; hg ci -m $i -A $i; done
    
    hg phabsend 0::
    # D40: created
    # D41: created
    # D42: created
    
    echo 3 >> C; hg amend; hg phabsend .
    # D42: updated
    
    hg tag --local --hidden -r 2 -f D42
    # move tag to the previous version
    
    hg phabsend .
    # D42: skipped (previously it would be "updated")
    
    rm -rf .hg; hg init
    hg phabread --stack D42 | hg import -
    hg phabsend .
    # D42: updated
    hg tag --local --remove D42
    hg commit --amend
    hg phabsend .
    # D42: updated (no new diff uploaded, previously it will update new diff)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D121

AFFECTED FILES
  contrib/phabricator.py

CHANGE DETAILS




EMAIL PREFERENCES
  https://phab.mercurial-scm.org/settings/panel/emailpreferences/

To: quark, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -147,19 +147,22 @@ 
     for node in nodelist with known previous sent versions, or associated
     Differential Revision IDs.
 
-    Examines all precursors and their tags. Tags with format like "D1234" are
-    considered a match and the node with that tag, and the number after "D"
-    (ex. 1234) will be returned.
+    Examines commit messages like "Differential Revision:" to get the
+    association information.
 
-    If tags are not found, examine commit message. The "Differential Revision:"
-    line could associate this changeset to a Differential Revision.
+    If such commit message line is not found, examines all precursors and their
+    tags. Tags with format like "D1234" are considered a match and the node
+    with that tag, and the number after "D" (ex. 1234) will be returned.
+
+    The ``old node``, if not None, is guaranteed to be the last diff of
+    corresponding Differential Revision, and exist in the repo.
     """
     url, token = readurltoken(repo)
     unfi = repo.unfiltered()
     nodemap = unfi.changelog.nodemap
 
     result = {} # {node: (oldnode or None, drev)}
-    toconfirm = {} # {node: (oldnode, {precnode}, drev)}
+    toconfirm = {} # {node: (force, {precnode}, drev)}
     for node in nodelist:
         ctx = unfi[node]
         # For tags like "D123", put them into "toconfirm" to verify later
@@ -169,35 +172,48 @@ 
                 for tag in unfi.nodetags(n):
                     m = _differentialrevisiontagre.match(tag)
                     if m:
-                        toconfirm[node] = (n, set(precnodes), int(m.group(1)))
+                        toconfirm[node] = (0, set(precnodes), int(m.group(1)))
                         continue
 
-        # Check commit message
+        # Check commit message. If "Differential Revision:" exists and matches
+        # the URL. Trust it blindly even if precursors do not match.
         m = _differentialrevisiondescre.search(ctx.description())
         if m:
-            result[node] = (None, int(m.group(1)))
+            toconfirm[node] = (1, set(precnodes), int(m.group(1)))
 
     # Double check if tags are genuine by collecting all old nodes from
     # Phabricator, and expect precursors overlap with it.
     if toconfirm:
-        confirmed = {} # {drev: {oldnode}}
-        drevs = [drev for n, precs, drev in toconfirm.values()]
-        diffs = callconduit(unfi, 'differential.querydiffs',
-                            {'revisionIDs': drevs})
-        for diff in diffs.values():
-            drev = int(diff[r'revisionID'])
-            oldnode = bin(encoding.unitolocal(getdiffmeta(diff).get(r'node')))
-            if node:
-                confirmed.setdefault(drev, set()).add(oldnode)
-        for newnode, (oldnode, precset, drev) in toconfirm.items():
-            if bool(precset & confirmed.get(drev, set())):
-                result[newnode] = (oldnode, drev)
-            else:
+        drevs = [drev for force, precs, drev in toconfirm.values()]
+        alldiffs = callconduit(unfi, 'differential.querydiffs',
+                               {'revisionIDs': drevs})
+        getnode = lambda d: bin(encoding.unitolocal(
+            getdiffmeta(d).get(r'node', ''))) or None
+        for newnode, (force, precset, drev) in toconfirm.items():
+            diffs = [d for d in alldiffs.values()
+                     if int(d[r'revisionID']) == drev]
+
+            # "precursors" as known by Phabricator
+            phprecset = set(getnode(d) for d in diffs)
+
+            # Ignore if precursors (Phabricator and local repo) do not overlap,
+            # and force is not set (when commit message says nothing)
+            if not force and not bool(phprecset & precset):
                 tagname = 'D%d' % drev
                 tags.tag(repo, tagname, nullid, message=None, user=None,
                          date=None, local=True)
                 unfi.ui.warn(_('D%s: local tag removed - does not match '
                                'Differential history\n') % drev)
+                continue
+
+            # Find the last node using Phabricator metadata, and make sure it
+            # exists in the repo
+            lastdiff = max(diffs, key=lambda d: int(d[r'id']))
+            oldnode = getnode(lastdiff)
+            if oldnode and oldnode not in nodemap:
+                oldnode = None
+
+            result[newnode] = (oldnode, drev)
 
     return result