Patchwork D6514: phabricator: use parents.set to always set dependencies

login
register
mail settings
Submitter phabricator
Date June 12, 2019, 4:44 p.m.
Message ID <differential-rev-PHID-DREV-z6xj4kc2ouxv4snix5bj-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/40442/
State Superseded
Headers show

Comments

phabricator - June 12, 2019, 4:44 p.m.
Kwan created this revision.
Kwan added a reviewer: durin42.
Kwan added a comment.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.


  Is it worth adding a test using histedit, or is the use in existing tests enough?

REVISION SUMMARY
  Now that Mercurial's Phabricator instance has been updated to a version that
  supports the parents.set transaction on revision.edit we can use that to set
  dependency relationships in patch stacks instead of abusing the summary.
  This has the advantage that we can use it on every `phabsend` so commit
  reordering is picked up without spamming changes like abusing the summary would,
  and using parents.set will clear previous parents unlike parents.add.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/phabricator.py
  tests/phabricator/phabsend-create-public.json
  tests/phabricator/phabsend-update-alpha-create-beta.json

CHANGE DETAILS




To: Kwan, durin42, #hg-reviewers
Cc: mercurial-devel
phabricator - June 12, 2019, 4:48 p.m.
This revision is now accepted and ready to land.
durin42 added a comment.
durin42 accepted this revision.


  I think the existing changes are enough. I'm overjoyed at this feature: this has been the single biggest pain point I've experienced with phabricator both as an author and as a reviewer. Thank you!

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6514/new/

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

To: Kwan, durin42, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/phabricator/phabsend-update-alpha-create-beta.json b/tests/phabricator/phabsend-update-alpha-create-beta.json
--- a/tests/phabricator/phabsend-update-alpha-create-beta.json
+++ b/tests/phabricator/phabsend-update-alpha-create-beta.json
@@ -768,7 +768,7 @@ 
                         "phab.mercurial-scm.org"
                     ]
                 },
-                "body": "api.token=cli-hahayouwish&transactions%5B0%5D%5Btype%5D=update&transactions%5B0%5D%5Bvalue%5D=PHID-DIFF-uhbyhoejzbniwwzj2q5c&transactions%5B1%5D%5Btype%5D=summary&transactions%5B1%5D%5Bvalue%5D=Depends+on+D1190&transactions%5B2%5D%5Btype%5D=summary&transactions%5B2%5D%5Bvalue%5D=+&transactions%5B3%5D%5Btype%5D=title&transactions%5B3%5D%5Bvalue%5D=create+beta+for+phabricator+test",
+                "body": "api.token=cli-hahayouwish&transactions%5B0%5D%5Btype%5D=update&transactions%5B0%5D%5Bvalue%5D=PHID-DIFF-uhbyhoejzbniwwzj2q5c&transactions%5B1%5D%5Btype%5D=parents.set&transactions%5B1%5D%5Bvalue%5D%5B0%5D=PHID-DREV-kikesmfxhzpfaxbzgj3l&transactions%5B2%5D%5Btype%5D=title&transactions%5B2%5D%5Bvalue%5D=create+beta+for+phabricator+test",
                 "uri": "https://phab.mercurial-scm.org//api/differential.revision.edit",
                 "method": "POST"
             },
diff --git a/tests/phabricator/phabsend-create-public.json b/tests/phabricator/phabsend-create-public.json
--- a/tests/phabricator/phabsend-create-public.json
+++ b/tests/phabricator/phabsend-create-public.json
@@ -700,7 +700,7 @@ 
                         "phab.mercurial-scm.org"
                     ]
                 },
-                "body": "api.token=cli-hahayouwish&transactions%5B0%5D%5Btype%5D=update&transactions%5B0%5D%5Bvalue%5D=PHID-DIFF-4pugk2zedyh2xm27uuvh&transactions%5B1%5D%5Btype%5D=summary&transactions%5B1%5D%5Bvalue%5D=Depends+on+D1192&transactions%5B2%5D%5Btype%5D=summary&transactions%5B2%5D%5Bvalue%5D=+&transactions%5B3%5D%5Btype%5D=title&transactions%5B3%5D%5Bvalue%5D=create+draft+change+for+phabricator+testing",
+                "body": "api.token=cli-hahayouwish&transactions%5B0%5D%5Btype%5D=update&transactions%5B0%5D%5Bvalue%5D=PHID-DIFF-4pugk2zedyh2xm27uuvh&transactions%5B1%5D%5Btype%5D=parents.set&transactions%5B1%5D%5Bvalue%5D%5B0%5D=PHID-DREV-qb4xy3abx7eu4puizvjl&transactions%5B2%5D%5Btype%5D=title&transactions%5B2%5D%5Bvalue%5D=create+draft+change+for+phabricator+testing",
                 "uri": "https://phab.mercurial-scm.org//api/differential.revision.edit",
                 "method": "POST"
             },
diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -431,12 +431,13 @@ 
     }
     callconduit(ctx.repo().ui, b'differential.setdiffproperty', params)
 
-def createdifferentialrevision(ctx, revid=None, parentrevid=None, oldnode=None,
-                               olddiff=None, actions=None, comment=None):
+def createdifferentialrevision(ctx, revid=None, parentrevphid=None,
+                               oldnode=None, olddiff=None, actions=None,
+                               comment=None):
     """create or update a Differential Revision
 
     If revid is None, create a new Differential Revision, otherwise update
-    revid. If parentrevid is not None, set it as a dependency.
+    revid. If parentrevphid is not None, set it as a dependency.
 
     If oldnode is not None, check if the patch content (without commit message
     and metadata) has changed before creating another diff.
@@ -465,14 +466,10 @@ 
         diff = olddiff
     writediffproperties(ctx, diff)
 
-    # Use a temporary summary to set dependency. There might be better ways but
-    # I cannot find them for now. But do not do that if we are updating an
-    # existing revision (revid is not None) since that introduces visible
-    # churns (someone edited "Summary" twice) on the web page.
-    if parentrevid and revid is None:
-        summary = b'Depends on D%d' % parentrevid
-        transactions += [{b'type': b'summary', b'value': summary},
-                         {b'type': b'summary', b'value': b' '}]
+    # Set the parent Revision every time, so commit re-ordering is picked-up
+    if parentrevphid:
+        transactions.append({b'type': b'parents.set',
+                             b'value': [parentrevphid]})
 
     if actions:
         transactions += actions
@@ -583,9 +580,9 @@ 
     drevids = [] # [int]
     diffmap = {} # {newnode: diff}
 
-    # Send patches one by one so we know their Differential Revision IDs and
+    # Send patches one by one so we know their Differential Revision PHIDs and
     # can provide dependency relationship
-    lastrevid = None
+    lastrevphid = None
     for rev in revs:
         ui.debug(b'sending rev %d\n' % rev)
         ctx = repo[rev]
@@ -595,10 +592,11 @@ 
         if oldnode != ctx.node() or opts.get(b'amend'):
             # Create or update Differential Revision
             revision, diff = createdifferentialrevision(
-                ctx, revid, lastrevid, oldnode, olddiff, actions,
+                ctx, revid, lastrevphid, oldnode, olddiff, actions,
                 opts.get(b'comment'))
             diffmap[ctx.node()] = diff
             newrevid = int(revision[b'object'][b'id'])
+            newrevphid = revision[b'object'][b'phid']
             if revid:
                 action = b'updated'
             else:
@@ -612,8 +610,9 @@ 
                 tags.tag(repo, tagname, ctx.node(), message=None, user=None,
                          date=None, local=True)
         else:
-            # Nothing changed. But still set "newrevid" so the next revision
-            # could depend on this one.
+            # Nothing changed. But still set "newrevphid" so the next revision
+            # could depend on this one and "newrevid" for the summary line.
+            newrevphid = querydrev(repo, str(revid))[0][b'phid']
             newrevid = revid
             action = b'skipped'
 
@@ -628,7 +627,7 @@ 
         ui.write(_(b'%s - %s - %s: %s\n') % (drevdesc, actiondesc, nodedesc,
                                              desc))
         drevids.append(newrevid)
-        lastrevid = newrevid
+        lastrevphid = newrevphid
 
     # Update commit messages and remove tags
     if opts.get(b'amend'):