| pulkit |
| hg-reviewers |
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.
| Automatic diff as part of commit; lint not applicable. |
| Automatic diff as part of commit; unit tests not applicable. |
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.
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.
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.
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.
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.)
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.
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
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.