Patchwork D7506: phabricator: add a "phabstatus" show view

login
register
mail settings
Submitter phabricator
Date Nov. 22, 2019, 4 p.m.
Message ID <differential-rev-PHID-DREV-qnonht52irt7owphagqc-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/43441/
State Superseded
Headers show

Comments

phabricator - Nov. 22, 2019, 4 p.m.
dlax created this revision.
Herald added subscribers: mercurial-devel, Kwan, mjpieters.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  We add a "phabstatus" show view (called as "hg show phabstatus") which
  renders a dag with underway revisions associated with a differential
  revision and displays their status.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS




To: dlax, #hg-reviewers
Cc: mjpieters, Kwan, mercurial-devel
phabricator - Nov. 22, 2019, 5:36 p.m.
mharbison72 added a comment.


  I like it.
  
  Any idea why the changeset isn't colored, unlike `hg show stack`?  It might also be a little more readable if the URI and status line were tabbed in, but maybe colored cset hashes would make that unnecessary?
  
  I'm also interested in coloring the status value, but I can tinker with that after it's landed, unless you already have plans.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 22, 2019, 9:09 p.m.
dlax added a comment.
dlax planned changes to this revision.


  In D7506#110225 <https://phab.mercurial-scm.org/D7506#110225>, @mharbison72 wrote:
  
  > I like it.
  
  Thanks!
  
  > Any idea why the changeset isn't colored, unlike `hg show stack`?  It might also be a little more readable if the URI and status line were tabbed in, but maybe colored cset hashes would make that unnecessary?
  
  Yes, I messed up with adding a custom template whereas I can actually reuse "hg show"'s one. Will fix (along with another issue in the algorithm I just found out.)
  
  > I'm also interested in coloring the status value, but I can tinker with that after it's landed, unless you already have plans.
  
  You mean having a different color depending on status value, or a fixed one? I have no plan anyways, so if you have good ideas, I'll leave this up to you.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 22, 2019, 10:35 p.m.
mharbison72 added a comment.


  In D7506#110249 <https://phab.mercurial-scm.org/D7506#110249>, @dlax wrote:
  
  > In D7506#110225 <https://phab.mercurial-scm.org/D7506#110225>, @mharbison72 wrote:
  >
  >> I'm also interested in coloring the status value, but I can tinker with that after it's landed, unless you already have plans.
  >
  > You mean having a different color depending on status value, or a fixed one? I have no plan anyways, so if you have good ideas, I'll leave this up to you.
  
  Something like green for accepted/closed, red for needs revision, neutral-ish for needs review, strikethrough for abandoned.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 23, 2019, 3:57 a.m.
mharbison72 added a comment.


  I'm not sure why, but this version seems to also show obsolete revisions.  I've got a bunch of `x` and `*` nodes in hg-committed right now.  I didn't see that before, although that was on a different clone that I don't have access to now.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 23, 2019, 4:41 a.m.
mharbison72 added a comment.


  In D7506#110288 <https://phab.mercurial-scm.org/D7506#110288>, @mharbison72 wrote:
  
  > I'm not sure why, but this version seems to also show obsolete revisions.  I've got a bunch of `x` and `*` nodes in hg-committed right now.  I didn't see that before, although that was on a different clone that I don't have access to now.
  
  After further review, the obsolete revisions have unstable children going way back, so that's why they show.  But I'm super confused by the output.  This isn't the whole output, but the middle or so that corresponds to the latest(!) commits:
  
    : : : : | | : | | : : | o  d77be1 filemerge: fix a missing attribute usage
    : : : : | | : | | : : | |  https://phab.mercurial-scm.org/D7465 Needs Review
    : : : : | | : | | : : | | o  ef1696 makefile: use Python 3 by default (BC)
    : : : : | | : | | : : | |/   https://phab.mercurial-scm.org/D7258 Accepted
    +-+-+-------+-------+-+---o  640bae cleanup: update references to /help/ that should now be /helptext/
    : : : : | | : | | : : | |    https://phab.mercurial-scm.org/D7472 Closed
    : : : : | | : | | : : | | o  20a3bf rust-dirs: address failing tests for `dirs` impl with a temporary fix
    : : : : | | : | | : : | | |  https://phab.mercurial-scm.org/D7503 Closed
    : : : : | | : | | : : | | | o  e8373b (@) windows: further build fixes for the WiX installer
    : : : : | | : | | : : | | | |  https://phab.mercurial-scm.org/D7509 Closed
    : : : : | | : | | : : | | | | o  347b42 logcmdutil: call _exthook() in changesettemplater
    : : : : | | : | | : : | | | | |  https://phab.mercurial-scm.org/D7505 Needs Review
    : : : : | | : | | : : | | | | | o  4b1de5 phabricator: add a "phabstatus" show view
    : : : : | | : | | : : | | | | | |  https://phab.mercurial-scm.org/D7506 Needs Review
    : : : : | | : | | : : | | | | | | @  f6920a phabricator: add a "phabstatus" template keyword
    : : : : | | : | | : : | | | | | | |  https://phab.mercurial-scm.org/D7507 Needs Review
    +-+-+-------o---+---+ | | | | | | |  ce20b8 format: format commands.py, which recently regressed
    : : : : | |  / / / / / / / / / / /   https://phab.mercurial-scm.org/D7064 Closed
  
  Show work looks like (truncated to latest):
  
    $ hg show work
    @  f6920a phabricator: add a "phabstatus" template keyword
    o  4b1de5 phabricator: add a "phabstatus" show view
    o  347b42 logcmdutil: call _exthook() in changesettemplater
    o  e8373b (@) windows: further build fixes for the WiX installer
    o  20a3bf rust-dirs: address failing tests for `dirs` impl with a temporary fix
    o  640bae cleanup: update references to /help/ that should now be /helptext/
    o    7eb701 merge with stable
    |\
    | o    88a306 (stable) singlehead: making config item a bool again
    | :\
    | : \
    | : :\
    | : : : o  06aac2 xxx-pytype: annotate various things flagged for future examination
    +-------o  6c9c22 webutil: drop an extra parameter on `join()`
    | : : : o  ef1696 makefile: use Python 3 by default (BC)
    | : : : | o  d77be1 filemerge: fix a missing attribute usage
    | : : : |/
    +-------o  538726 filemerge: drop a default argument to appease pytype
  
  And show stack looks like:
  
    $ hg show stack
      @  f6920 phabricator: add a "phabstatus" template keyword
      o  4b1de phabricator: add a "phabstatus" show view
      o  347b4 logcmdutil: call _exthook() in changesettemplater
      o  e8373 (@) windows: further build fixes for the WiX installer
      o  20a3b rust-dirs: address failing tests for `dirs` impl with a temporary fix
      o  640ba cleanup: update references to /help/ that should now be /helptext/
     /   (stack base)
    o  7eb70 merge with stable
  
  Is the goal to limit to the current stack, or all non public commits?  I can see a use case for both.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 23, 2019, 10:15 a.m.
dlax added a comment.
dlax planned changes to this revision.


  In D7506#110321 <https://phab.mercurial-scm.org/D7506#110321>, @mharbison72 wrote:
  
  > In D7506#110288 <https://phab.mercurial-scm.org/D7506#110288>, @mharbison72 wrote:
  >
  >> I'm not sure why, but this version seems to also show obsolete revisions.  I've got a bunch of `x` and `*` nodes in hg-committed right now.  I didn't see that before, although that was on a different clone that I don't have access to now.
  >
  > After further review, the obsolete revisions have unstable children going way back, so that's why they show.  But I'm super confused by the output.  This isn't the whole output, but the middle or so that corresponds to the latest(!) commits:
  
  Hm, I don't have this issue. Even with visible obsolete revisions with a phab differential, the output seem consistent with "show work".
  The revisions shown by the "phabstatus" view is just a subset of that of "work" view (subset of revisions with a differential). Do you see more revisions in "phabstatus" than in "work" or am I misunderstanding something?
  Or perhaps this is because of a wrong sorting of the filtered set? I'll make sure the final is still topo sorted and send another version in a moment.
  
  > Is the goal to limit to the current stack, or all non public commits?  I can see a use case for both.
  
  I don't use the "stack" view often, so my goal is to map to the "work" view which I use more. But perhaps it'd make sense to have "hg show phabwork" and "hg show phabstack"? (We can rename the view, and add the other one later on.)

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Nov. 23, 2019, 4:12 p.m.
mharbison72 added a comment.


  In D7506#110351 <https://phab.mercurial-scm.org/D7506#110351>, @dlax wrote:
  
  > The revisions shown by the "phabstatus" view is just a subset of that of "work" view (subset of revisions with a differential). Do you see more revisions in "phabstatus" than in "work" or am I misunderstanding something?
  
  No, there are 89 entries in work, and 35 in phabstatus.  (I used `../hg show phabstatus | egrep '[0-9a-f]{6}' | wc -l` to tame the output.)  My confusion was that things that are direct descendants were showing like they were all separate heads.  (e.g. the revision marked `@` and the `@` bookmark 3 above it in phabstatus //should// be a direct line.)  This latest update made things look way more sane.  The remaining old junk in my output is likely because I tend to leave anonymous branches laying around, and it's adding upstream parents for context.
  
  > Or perhaps this is because of a wrong sorting of the filtered set? I'll make sure the final is still topo sorted and send another version in a moment.
  >
  >> Is the goal to limit to the current stack, or all non public commits?  I can see a use case for both.
  >
  > I don't use the "stack" view often, so my goal is to map to the "work" view which I use more. But perhaps it'd make sense to have "hg show phabwork" and "hg show phabstack"? (We can rename the view, and add the other one later on.)
  
  I had forgotten about the extension TBH.  Because I tend to leave junk around, I'd probably use the stack view much more.  Narrowing to the stack also seems useful now that the author has been removed since the first revision of this.  So I think having both is a good idea.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Dec. 10, 2019, 3:57 p.m.
This revision now requires changes to proceed.
pulkit added a comment.
pulkit requested changes to this revision.


  `test-check-module-imports.t` says hi!

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers, pulkit
Cc: mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Dec. 11, 2019, 11:37 p.m.
spectral added a comment.


  I don't think `from . import show` works generally. I received errors because I imported phabricator via a path (pointing directly at the .py file, something like `[extensions] phabricator = /home/spectral/src/hg/hgext/phabricator.py`), not by doing something like `extensions.phabricator=`. I received the following errors:
  
    $ hg co 70060915c3f2
    $ make local
    $ ./hg st
    *** failed to import extension phabricator from /usr/local/google/home/spectral/src/hg/hg/hgext/phabricator.py: Attempted relative import in non-package
    $ make local PYTHON=python3
    $ python3 ./hg st
    *** failed to import extension phabricator from /usr/local/google/home/spectral/src/hg/hg/hgext/phabricator.py: attempted relative import with no known parent package

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers, pulkit
Cc: spectral, mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Dec. 12, 2019, 7:49 a.m.
dlax added a comment.


  In D7506#111947 <https://phab.mercurial-scm.org/D7506#111947>, @spectral wrote:
  
  > I don't think `from . import show` works generally.
  
  I did that because `test-check-module-imports.t` complained otherwise when using `from hgext import show`:
  
    hgext/phabricator.py:89: import should be relative: hgext
  
  Also, there's already a similar `from . import rebase` in `hgext/split.py` so I thought that relative import was fine.
  
  I'm happy to change if there's a solution, though.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers, pulkit
Cc: spectral, mharbison72, mjpieters, Kwan, mercurial-devel
phabricator - Dec. 12, 2019, 6:19 p.m.
spectral added a comment.


  In D7506#111966 <https://phab.mercurial-scm.org/D7506#111966>, @dlax wrote:
  
  > In D7506#111947 <https://phab.mercurial-scm.org/D7506#111947>, @spectral wrote:
  >
  >> I don't think `from . import show` works generally.
  >
  > I did that because `test-check-module-imports.t` complained otherwise when using `from hgext import show`:
  >
  >   hgext/phabricator.py:89: import should be relative: hgext
  >
  > Also, there's already a similar `from . import rebase` in `hgext/split.py` so I thought that relative import was fine.
  > I'm happy to change if there's a solution, though.
  
  The normal solution is to just load the extension (I think that's what evolve does, since it has a hard dependency on rebase functionality), or to configure an `extensions.afterloaded()` (which won't be called if the extension isn't loaded, or is loaded with a non-standard name), but I think this probably isn't a huge deal... I'm only importing via weird syntax because the phabricator extension didn't previously get installed on my machine, but now it is part of the installation so I can just use the normal syntax. It's also a developer/advanced feature, so I don't think this will cause many problems.
  
  The only thing I'm concerned about is cargo culting... this is more evidence that this is an acceptable practice, but that's probably already too late (there's stuff in mercurial/configitems.py that says that things like shelve might already be doing something similar (also to the rebase extension), and as you said, split is already doing this).
  
  Sorry for not realizing that my method of importing the extension was outdated/unnecessary/odd before my message implying you needed to make a change here - hopefully you didn't spend too much time on it.

REPOSITORY
  rHG Mercurial

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

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

To: dlax, #hg-reviewers, pulkit
Cc: spectral, mharbison72, mjpieters, Kwan, mercurial-devel

Patch

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -11,6 +11,10 @@ 
 revisions in a format suitable for :hg:`import`, and a ``phabupdate`` command
 to update statuses in batch.
 
+A "phabstatus" view for :hg:`show` is also provided; it displays status
+information of Phabricator differentials associated with unfinished
+changesets.
+
 By default, Phabricator requires ``Test Plan`` which might prevent some
 changeset from being sent. The requirement could be disabled by changing
 ``differential.require-test-plan-field`` config server side.
@@ -60,9 +64,12 @@ 
     encoding,
     error,
     exthelper,
+    formatter,
+    graphmod,
     httpconnection as httpconnectionmod,
     match,
     mdiff,
+    logcmdutil,
     obsutil,
     parser,
     patch,
@@ -80,6 +87,8 @@ 
     procutil,
     stringutil,
 )
+from hgext.show import showview
+
 
 # Note for extension authors: ONLY specify testedwith = 'ships-with-hg-core' for
 # extensions which SHIP WITH MERCURIAL. Non-mainline extensions should
@@ -462,6 +471,29 @@ 
     return result
 
 
+def getdrevmap(repo, nodelist):
+    """Return a dict mapping each node in `nodelist` to their Differential
+    Revision ID (or None).
+    """
+    result = {}
+    for node in nodelist:
+        result[node] = None
+        ctx = repo[node]
+        # Check commit message
+        m = _differentialrevisiondescre.search(ctx.description())
+        if m:
+            result[node] = int(m.group('id'))
+            continue
+        # Check tags
+        for tag in repo.nodetags(node):
+            m = _differentialrevisiontagre.match(tag)
+            if m:
+                result[node] = int(m.group(1))
+                break
+
+    return result
+
+
 def getdiff(ctx, diffopts):
     """plain-text diff without header (user, commit message, etc)"""
     output = util.stringio()
@@ -1648,3 +1680,41 @@ 
 
                 return templateutil.hybriddict({b'url': url, b'id': t,})
     return None
+
+
+phabstatus_tmpl = (
+    b'{label("changeset.{phase}{if(troubles, \' changeset.troubled\')}", '
+    b'shortest(node, 5))} '
+    b'[{label("log.branch", branch)}] '
+    b'{label("log.description", desc|firstline)} '
+    b'({label("log.user", author|user)})\n'
+)
+
+
+@showview(b'phabstatus')
+def phabstatusshowview(ui, repo):
+    """Phabricator differiential status"""
+    revs = repo.revs('sort(_underway(), topo)')
+    drevmap = getdrevmap(repo, [repo[r].node() for r in revs])
+    revs, drevids, revsbydrevid = [], [], {}
+    for node, drevid in pycompat.iteritems(drevmap):
+        if drevid is not None:
+            revs.append(repo[node].rev())
+            drevids.append(drevid)
+            revsbydrevid[drevid] = repo[node].rev()
+
+    revs = smartset.baseset(revs)
+    drevs = callconduit(ui, b'differential.query', {b'ids': drevids})
+    drevsbyrev = {revsbydrevid[int(drev[b'id'])]: drev for drev in drevs}
+
+    def phabstatus(ctx):
+        drev = drevsbyrev[ctx.rev()]
+        ui.write(b"%(uri)s %(statusName)s\n" % drev)
+
+    revdag = graphmod.dagwalker(repo, revs)
+
+    ui.setconfig(b'experimental', b'graphshorten', True)
+    spec = formatter.lookuptemplate(ui, None, phabstatus_tmpl)
+    displayer = logcmdutil.changesettemplater(ui, repo, spec, buffered=True)
+    displayer._exthook = phabstatus
+    logcmdutil.displaygraph(ui, repo, revdag, displayer, graphmod.asciiedges)