D6876: phabricator: support automatically obsoleting old revisions of pulled commits

[mharbison72 (Matt Harbison)]

mharbison72 created this revision.
Herald added subscribers: mercurial-devel, Kwan.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This is basically an import of the `pullcreatemarkers` extension[1] from the FB
  repo, with minor adjustments to `getmatchingdiff()` to work with modern hg.
  Since this is very phabricator specific, it makes more sense to me to bundle it
  into the existing extension.  It wasn't very obvious from the old name what
  functionality was provided, and it may make sense to do this in other scenarios
  besides `hg pull`.
  
  There are two use cases that I can see- first, ensuring that old revisions are
  cleaned up for a contributor (I seem to recall something I submitted recently
  needed to be explicitly pruned, though most submissions do clean up
  automatically).  Second, any `hg phabread | hg import -` would otherwise need to
  be manually cleaned up.  The latter is annoying enough that I tend not to grab
  the code and try it when reviewing.
  
  It is currently guarded by a config option (off by default), because @marmoute
  expressed concerns about duplicate marker creation if the pushing reviewer also
  creates a marker.  I don't think that's possible here, since the obsolete
  revisions are explicitly excluded.  But maybe there are other reasons someone
  wouldn't want older revisions obsoleted.  The config name reflects the fact that
  I'm not sure if other things like import should get this too.
  
  I suspect that we could wrap a function deeper in the pull sequence to improve
  both the code and the UX.  For example, when pulling an obsolete marker, it can
  print out a warning that the working directory parent is obsolete, but that
  doesn't happen here.  (It won't happen with this test.  It *should* without the
  `--bypass` option, but doesn't.)  It should also be possible to not have to
  query the range of new revisions, and maybe it can be added to the existing
  transaction.
  
  [1] https://bitbucket.org/facebook/hg-experimental/src/default/hgext3rd/pullcreatemarkers.py

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  hgext/phabricator.py
  tests/test-phabricator.t

CHANGE DETAILS

diff --git a/tests/test-phabricator.t b/tests/test-phabricator.t
--- a/tests/test-phabricator.t
+++ b/tests/test-phabricator.t
@@ -136,7 +136,8 @@
   D1253 - skipped - 1acd4b60af38: create comment for phabricator test
 
 Phabreading a DREV with a local:commits time as a string:
-  $ hg phabread --test-vcr "$VCR/phabread-str-time.json" D1285
+  $ hg phabread --test-vcr "$VCR/phabread-str-time.json" D1285 > ../d1285.diff
+  $ cat ../d1285.diff
   # HG changeset patch
   # User test <test>
   # Date 1562019844 0
@@ -153,5 +154,20 @@
   @@ * @@ (glob)
   +test
   
+  $ hg init ../simple
+  $ hg -R ../simple import -q ../d1285.diff
+  $ hg import --bypass -q ../d1285.diff
+  $ hg pull -f ../simple --config experimental.evolution.createmarkers=True \
+  >                      --config experimental.phabricator.fabricate-obsmarkers=True
+  pulling from ../simple
+  searching for changes
+  warning: repository is unrelated
+  requesting all changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 1 changesets with 0 changes to 1 files (+1 heads)
+  new changesets b68c34204469
+  (run 'hg heads' to see heads, 'hg merge' to merge)
 
   $ cd ..
diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -57,6 +57,7 @@
     exthelper,
     httpconnection as httpconnectionmod,
     mdiff,
+    obsolete,
     obsutil,
     parser,
     patch,
@@ -87,7 +88,11 @@
 command = eh.command
 configtable = eh.configtable
 templatekeyword = eh.templatekeyword
+uisetup = eh.finaluisetup
 
+eh.configitem(b'experimental', b'phabricator.fabricate-obsmarkers',
+    default=False,
+)
 # developer config: phabricator.batchsize
 eh.configitem(b'phabricator', b'batchsize',
     default=12,
@@ -176,6 +181,52 @@
                        optionalrepo=optionalrepo)(inner)
     return decorate
 
+def getmatchingdiff(ctx):
+    m = _differentialrevisiondescre.search(ctx.description())
+    if m:
+        return b"D%s" % m.group(r'id')
+
+    return None
+
+ at eh.wrapcommand(b"pull")
+def _pull(orig, ui, repo, *args, **opts):
+    if not (obsolete.isenabled(repo, obsolete.createmarkersopt)
+            and ui.configbool(b'experimental',
+                              b'phabricator.fabricate-obsmarkers')):
+        return orig(ui, repo, *args, **opts)
+    maxrevbeforepull = len(repo.changelog)
+    r = orig(ui, repo, *args, **opts)
+    maxrevafterpull = len(repo.changelog)
+
+    # Collect the diff number of the landed diffs
+    landeddiffs = {}
+    for rev in range(maxrevbeforepull, maxrevafterpull):
+        n = repo[rev]
+        if n.phase() == phases.public:
+            diff = getmatchingdiff(n)
+            if diff is not None:
+                landeddiffs[diff] = n
+
+    if not landeddiffs:
+        return r
+
+    # Try to find match with the drafts
+    tocreate = []
+    unfiltered = repo.unfiltered()
+    for rev in unfiltered.revs("draft() - obsolete()"):
+        n = unfiltered[rev]
+        diff = getmatchingdiff(n)
+        if diff in landeddiffs and landeddiffs[diff].rev() != n.rev():
+            tocreate.append((n, (landeddiffs[diff],)))
+
+    if not tocreate:
+        return r
+
+    with unfiltered.lock(), unfiltered.transaction('phabpullcreatemarkers'):
+        obsolete.createmarkers(unfiltered, tocreate)
+
+    return r
+
 def urlencodenested(params):
     """like urlencode, but works with nested parameters.
 



To: mharbison72, #hg-reviewers
Cc: Kwan, mercurial-devel, marmoute