Page MenuHomePhabricator

phabricator: add a "phabstatus" show view
ClosedPublic

Authored by dlax on Nov 22 2019, 11:00 AM.

Details

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.

The revisions shown is a subset of that shown by "work" view, only
including revisions with known by Phabricator.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

dlax created this revision.Nov 22 2019, 11:00 AM
dlax updated this revision to Diff 18315.Nov 22 2019, 11:02 AM

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.

dlax planned changes to this revision.Nov 22 2019, 4:09 PM

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.

dlax updated this revision to Diff 18320.Nov 22 2019, 4:13 PM
In D7506#110249, @dlax 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.

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.

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.

dlax planned changes to this revision.Nov 23 2019, 5:15 AM

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.)

dlax edited the summary of this revision. (Show Details)Nov 23 2019, 5:26 AM
dlax updated this revision to Diff 18338.
In 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.

pulkit accepted this revision.Dec 10 2019, 10:46 AM
This revision is now accepted and ready to land.Dec 10 2019, 10:46 AM
pulkit requested changes to this revision.Dec 10 2019, 10:57 AM

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

This revision now requires changes to proceed.Dec 10 2019, 10:57 AM
dlax updated this revision to Diff 18589.Dec 10 2019, 2:46 PM
pulkit accepted this revision.Dec 11 2019, 5:52 AM
This revision is now accepted and ready to land.Dec 11 2019, 5:52 AM
This revision was automatically updated to reflect the committed changes.

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
dlax added a comment.Dec 12 2019, 2:49 AM

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.

In D7506#111966, @dlax 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.