Summary:
Refer to discussion on D21677#275541
Refs D21681 (arcanist changes)
Phabricator has several uses of the `--debug` flag being used with Mercurial. Use of this flag causes additional output to be added which Phabricator needs, however the behavior of `--debug` is not guaranteed to be stable, and in newer versions of Mercurial there have been additional output that has caused Phabricator to choke on parsing the output. This change removes several uses of `--debug` in favor of using `--template` with the `hg log` or `hg annotate` commands in combination with the `{p1.node}` or `{p2.node}` template format.
The use of `{p1node}` format in templates was added in [[ https://www.mercurial-scm.org/wiki/WhatsNew/Archive#Mercurial_2.4_.282012-11-01.29 | Mercurial 2.4 (2012) ]]. This format was deprecated in [[ https://www.mercurial-scm.org/wiki/WhatsNew#Mercurial_4.9_.282019-02-01.29 | Mercurial 4.9 (2019) ]] in favor of using `{p1.node}` format which is unclear when this new format was added (presumably earlier than Mercurial 4.9).
The use of `--template` with `hg annotate` is only officially supported in [[ https://www.mercurial-scm.org/wiki/Release4.6 | Mercurial 4.6 (2018) ]], though does appear to work in 4.5 but is not documented.
Since the `{p1node}` format was introduced in 2.4 this bumps the required version of `hg` to 2.4 (from 1.9). Since the `annotate --template` feature wasn't added until 4.6 (which is still fairly recent), the use of it is gated on a capability test, but still preferred for use where possible to avoid extraneous output from `--debug` flag.
Test Plan:
I verified I could do the following in a mercurial repository, while having mercurial 5.8 installed:
1. Navigate and view files in Diffusion under e.g. `/source/test-repo/`.
2. While viewing a file in Diffusion verified that I could view the blame of the file and the history/annotations looked accurate for the files I was browsing.
3. From the blame sidebar, select to view a commit which loaded and displayed changes properly.
4. View the history of the repository under e.g. `/source/test-repo/history/default/`. I verified the history looked correct and the tree-like structure showing relationship of commits also looked accurate.
I setup mercurial to run version 4.4, created a new repository, added some commits, and verified all the above behavior still works properly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21679
Summary:
Ref T13658. This just scrubs some of the simple references from the codebase.
Most of what's left is in documentation which won't be relevant for a fork and/or which I need to separately revise (or more-or-less delete) at some point anyway.
I removed the "install RHEL" and "install Ubuntu" scripts outright since I don't have any reasonable way to test them and don't plan to maintain them.
Test Plan: Grepped for "phacility", "epriestley"; ran unit tests.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21678
Summary:
With newer versions of Mercurial come newer debug messages which need filtered out.
1. In the scenario of Phabricator observing a hosted Mercurial repository which exists on a server in a multi-user environment it's possible that a repository computes branch cache at a tip revision which is not present. When this happens Mercurial will include in the debug output this information. This message indicates that the cache is going to be re-computed. See https://www.mercurial-scm.org/pipermail/mercurial/2014-June/047239.html.
2. Likely in some version with added or improved support for `pager` the debug info seems to indicate when a pager is being invoked for a command. This seems to print out regularly despite piping the stdout.
3. If the repository on Phabricator ever had the `largefiles` extension enabled then some additional details about "updated patterns" will print out.
Test Plan:
I verified an observed repository's history could be browsed, specifically the history of files which previously resulted in "Undefined offset: 1".
Added a unit test to check the results of `filterMercurialDebugOutput()`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21677
Summary: Found a few typos which could be updated.
Test Plan:
I tested the Configuration page change by navigating to `/config` and verifying the page title set in the browser as well as the page title text on the page
|Before|After|
|---|---|
|{F9013208}|{F9013210}|
|{F9013300}|{F9013301}|
I verified the Conduit error message by navigating to `/auth/start/?__conduit__=1`
{F9013289}
The CircleCI error message was not verified due to the involvement of testing with CircleCI however the change is very minor and has very little risk of impacting any functionality.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D21675
Summary: See T13657. An install has "watcher" packages which should not allow owners to "Force Accept" other packages.
Test Plan:
- Created package A, which I own, on "/", with "Weak" authority.
- Created package B, which I do not own, on "/src".
- Created a revision which touches "/src" and added package B as a reviewer.
- Attempted to accept the revision...
- Before patch: permitted to "Force Accept" for package B.
- After patch: not allowed to "Force Accept" for package B.
- Verified that setting package "A" back to "Strong" authority allows a force-accept for package B.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D21674
Summary: Ref PHI2071. This path is incorrect; the correct path is `local.json`.
Test Plan: Looked in my `conf/local/` directory.
Differential Revision: https://secure.phabricator.com/D21663
Summary:
The datepicker could step by the wrong number of months, due to the date rolling over to the next month when the number of days in the month is exceeded. For example, going forward from January 31 would jump to March 3, while going backward from July 31 would only go to July 1.
Push the date back to ensure that the datepicker stays in the correct month when switching.
Test Plan: Changed months starting from an assortment of dates.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: artms, Korvin
Differential Revision: https://secure.phabricator.com/D21673
Summary: This makes the set of hooks easily extensible, as a first step toward integrating more 3rd party CI in phorge.
Test Plan: Send requests to `/harbormaster/hook/circleci/` and `/harbormaster/hook/buildkite/` and check they run the proper handler.
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: Matthew, speck, tobiaswiese
Maniphest Tasks: T15018
Differential Revision: https://we.phorge.it/D25005
Summary: This makes the whole setup easier, future proof and reproducible.
Test Plan:
cd support/aphlict/server/
npm install
See that ws gets installed as expected.
Reviewers: O1 Blessed Committers, Matthew
Reviewed By: O1 Blessed Committers, Matthew
Subscribers: Matthew, Ekubischta, speck, tobiaswiese
Maniphest Tasks: T15019
Differential Revision: https://we.phorge.it/D25006
Test Plan: Looked at new files, made sure the only changes were to rename the files in line with the documentation
Reviewers: O1 Blessed Committers, eax
Reviewed By: O1 Blessed Committers, eax
Subscribers: speck, tobiaswiese
Maniphest Tasks: T15017
Differential Revision: https://we.phorge.it/D25010
Summary: This commit also removes references to support pacts and updates links to point to the new upstream.
Test Plan: Generated Diviner documentation on a local install and verified that the changes look good.
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: chris, speck, tobiaswiese
Maniphest Tasks: T15012
Differential Revision: https://we.phorge.it/D25007
Summary:
Update the `.arcconfig` file to point to our decided-upon URL for hosting
Refs T15006
Test Plan:
I used `arc which` and verified it identified `https://we.phorge.it`:
```lang=console
REPOSITORY
To identify the repository associated with this working copy, arc followed this process:
Configuration value "repository.callsign" is empty.
This repository has no VCS UUID (this is normal for git/hg).
The remote URI for this working copy is
"ssh://git@we.phorge.it/source/phorge.git".
Found a unique matching repository.
This working copy is associated with the Phorge repository.
```
Reviewers: avivey, chris, tobiaswiese
Reviewed By: avivey, tobiaswiese
Maniphest Tasks: T15006
Differential Revision: https://we.phorge.it/D25001
Summary: Ref T13614. Provide "bin/repository lock" to temporarily lock repositories for manual maintenance.
Test Plan:
- Read instructions.
- Used `bin/repository lock` according to the instructions.
- Saw Storage tab in Diffusion report lock held during maintenance, released after it completes.
- Saw "maintenance" push log generated and repository version bump.
- Tried to lock some invalid repositories.
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21671
Summary:
Ref T13614. When a script holds the write lock but modifies the repository directly (rather than by pushing), the repository version won't change when the script releases the write lock. Thus, the writes may not propagate to other nodes (it depends which node lucks out and accepts the next write).
To guarantee that writes propagate, allow these scripts to pretend they pushed the repository. These are bare-bones valid events flagged as "Maintenance".
Test Plan:
- Wrote a script to hold the write lock, wait (or pretend to do something), then release the write lock.
- Applied patches, modified script to use new APIs ("newMaintenanceEvent()").
- Ran script, saw repository verison bump and relevant push logs:
{F8814923}
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21670
Summary:
Ref T13614. When an omnipotent user calls "synchronizeWorkingCopyBeforeWrite()", we record a WorkingCopyVersion record with a null "userPHID". The UI then renders this as "Unknown Object (????)".
Improve this behavior:
- When no PHID is available, just render nothing in the UI (this doesn't seem meaningfully different from no version existing at all).
- Allow callers to provide an acting user PHID, similar to Editor.
There's currently no way to perform this kind of write legitimately in the upstream, but T13614 is providing one.
Test Plan:
- Wrote a script that calls "synchronizeWorkingCopyBeforeWrite()" as the omnipotent user.
- Ran script, saw "Unknown Object (????)" in the UI.
- Applied UI fix, saw empty UI.
- Applied "acting as" fix, modified script to act as the Diffusion application, ran script, saw "Diffusion" attribution in UI.
{F8814806}
Maniphest Tasks: T13614
Differential Revision: https://secure.phabricator.com/D21669
Summary:
Ref T13650. Currently, viewing the API console help page for this method fatals because it constructs a generic, untyped panel.
As a step toward improving this, generate a concrete panel type instead. This isn't the best possible fix, see T13650 for discussion.
Test Plan: Viewed "dashboard.panel.edit" API page, now saw a usable page.
Maniphest Tasks: T13650
Differential Revision: https://secure.phabricator.com/D21668
Summary:
As backstory: I accidentally added the subscriber `PHID-USER-abcd` to `T1` on this install by calling `maniphest.edit`. I intended to edit `T1` on my local install.
This edit is permitted for messy technical reasons, described in T13429. It's not valid, but it's hard to prevent.
The state we reach is also possible even if the edit is rejected (i.e., someone can go manually update the database).
Regardless of how we get into this state, the state (a non-user subscriber) breaks the UI on the task page when it attempts to test if the subscriber can see the task.
To prevent this, only claim that a Handle can have capabilities if the handle is complete. If the handle is incomplete (an invalid or restricted object), it either can't be meaningfully tested for capabilities or the viewer isn't allowed to know them.
Test Plan:
Viewed `T1` on this install, saw a fatal. Applied the same edit to `T1` locally, got the same fatal. Applied patch, no more fatal. Now saw "Unknown Object (User)" in subscriber curtain.
Specifically, the fatal is:
> Attempting to test capability "view" for handle of type "USER", but this capability has not been attached.
Differential Revision: https://secure.phabricator.com/D21662
Summary: Ref T13559. Substantially correct the client logic for "Save" and "Cancel" actions to handle unusual cases.
Test Plan:
Quoting behavior:
- Quoted a comment.
- Cancelled the quoted comment without modifying anything.
- Reloaded page.
- Before changes: quoted comment still exists.
- After changes: quoted comment is deleted.
- Looked at comment count in header, saw consistent behavior (before: weird behavior).
Empty suggestion behavior:
- Created a new comment on a suggestable file.
- Clicked "Suggest Edit" to enable suggestions.
- Without making any text or suggestion changes, clicked "Save".
- Before changes: comment saves, but is empty.
- After changes: comment deletes itself without undo.
General behavior:
- Created and saved an empty comment (deletes itself).
- Created and saved a nonempty comment (saves as draft).
- Created and saved an empty comment with an edit suggestion (saves).
- Created and saved an empty comment with a suggestion to entirely delete lines -- that is, no suggestion text (saves).
- Edited a comment, saved without changes (save).
- Edited a comment, save deleting all text (saves -- note that this is intentionally without undo, since this is a lot of steps to do by accident).
- Cancel editing an unchanged comment (cancels without undo).
- Cancel editing a changed comment (cancels with undo).
- Undo'd, got text back.
- Cancel new comment with no text (deletes without undo).
- Cancel new comment with text (deletes with undo).
- Undo'd, got text back.
- Saved a quoted comment with no changes (saves -- note that this is intentionally not a "delete", since just quoting someone seems fine if you click "Save" -- maybe you want to come back to it later).
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21654
Summary:
Ref T13559. Various client decisions depend on the "initial" or "committed" states of inline comments. Previously, these were informally constructed from "mostly similar" available values, or glossed over in some cases.
On the server, save the initial state when creating a comment. Save the committed state when applying a "save" operation. Send all three states to the client.
On the client, load and track all three states explicitly.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21653
Summary:
Ref T13559. Currently, the default text for inline comment side-loads in a bizarre way. Instead, when a user creates an inline comment, load the inline context and set it as part of the initial content state.
This allows the side channel (and the code that puts the text in place at the last second on the client) to be removed.
Test Plan: Created inlines, clicked "Suggest Edit". See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21652
Summary:
Ref T13559. To allow the client to make correct decisions about what buttons mean, track an explicit "Committed" content state.
This is the last version of the comment that has been saved on the server, and does not exist if the comment has never been saved.
Test Plan: Created comments, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21651
Summary:
Ref T13559. If you create comments A and B, then delete comments A and B, then undo the deletion of A, the UI undoes the deletion of B instead.
This is becasue the undo rows are shipped down with a static scalar metadata reference. When copied multiple times to create multiple undo rows, they reference the same data object.
Preventing this in the general case is a problem with greater scope. For now, just avoid rendering these rows with any metadata so they don't alias a single data object.
Test Plan:
- Created comments A, B.
- Deleted comments A, B.
- Clicked "Undo" on A.
- Before: Deletion of "B" undone.
- After: Deletion of "A" undone.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21650
Summary:
Ref T13559. When the user clicks the "Cancel" button, we sometimes take it to mean "delete" (when the comment is empty).
Both the client and server make a decision about this, and they may not agree, which causes the client to fall out of sync.
Make the client responsible for deciding whether it wants to interpret a click on the "Cancel" button as a "revert" or a "delete".
Test Plan: Cancelled empty and nonempty comments, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21649
Summary:
Ref T13559. When you click "Save" on an inline comment and it's empty, we may actually delete the comment.
Currently, the client and server both make decisions about whether the comment should be deleted. These decisions may not agree, causing the client state to fall out of sync.
Make the client authoritative about whether it wants to handle the user clicking the "Save" button as an intent to save or an intent to delete.
Test Plan: Saved empty and nonempty inlines. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21648
Summary:
Ref T13559. Instead of directly reading form state, make all callers use the "active" state instead. The state reads the form.
No functional changes, just clarifying responsiblites.
Test Plan: Created inlines, etc. See followup changes.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21647
Summary:
Ref T13559. Rather than reading from the document, make client inlines actively track their current "active" state.
The "active" state is what the user currently sees in the client UI.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21646
Summary:
Ref T13559. In an effort to ultimately fix the "quote + cancel" bug, begin formalizing content states on the client.
This creates a "ContentState" client object and moves the authoritative storage for the "hasSuggestion" property to it.
Test Plan: Created inlines, etc. See followups.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21645
Summary:
Ref T13559. D21261 added caching here, but the logic in rebuilding inlines wasn't quite correct, and could lead to us double-appending.
Instead, when rebuilding, unconditionally discard the old list.
Test Plan:
- Added inline comments to a file in Differential.
- Marked some done.
- Scrolled so the inline comment header was visible, saw "X / Y Comments" button in header.
- Clicked "Show 20 more lines" on the changeset with inlines (or toggle "View Unified" / "View Side-by-Side", or other interactions likely work too).
- Before: saw "X / Y" change improperly (because inlines in that file were double-counted).
- After: saw stable count.
- Grepped for "differential-inline-comment-refresh", got no hits, concluded this event has no listeners.
Maniphest Tasks: T13559
Differential Revision: https://secure.phabricator.com/D21642
Summary:
Ref T13647. The ref discovery process prunes commits that no longer exist in the repository before executing "git log <new heads> --not <old heads>" to identify newly published commits.
If we don't do this, the "git log" command will fail if any old head has been pruned from the repository.
Currently, this test for missing commits starts with a call to "git for-each-ref" to attempt to resolve symbols as tag or branch names, but:
- this is painfully slow in repositories with many refs; and
- this is incorrect (not consistent with "git" behavior) for 40-character hex strings, which Git will never resolve as symbolic names.
Instead, when a symbol is a 40-character hex string, skip "git for-each-ref" and jump directly to "git cat-file --batch-check".
Test Plan:
- Ran `bin/repository update` in a repository with 65K refs and extra debugging info.
- Before: took ~30s, three calls to `git for-each-ref`.
- After: took ~20s, two calls to `git for-each-ref`. Same resolution result on queries.
Maniphest Tasks: T13647
Differential Revision: https://secure.phabricator.com/D21658
Summary: Ref T13641. These conditions are swapped, and "activeBindings" loads more data than necessary while "bindings" doesn't load enough.
Test Plan: Called method with each attachment, got good results instead of an exception.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21657
Summary:
Ref T13644. Ref T13638.
- Double-encode the symbol that is used as a path component, similar to Diffusion.
- Fix an outdated reference to ".path", which provided context for symbol lookup.
- Prevent command-clicking headers from looking up the path as a symbol.
Test Plan:
- Command-clicked headers, no longer got a symbol.
- Command-clicked stuff with "/", saw it double-encoded and decoded properly.
- Command-clicked normal symbols, saw "path" populate correctly.
Maniphest Tasks: T13644, T13638
Differential Revision: https://secure.phabricator.com/D21641
Summary:
Ref T9764. These stars are inconsistent, not accessible, and generally weird. They predate icons.
Update them to use icons instead.
Test Plan:
{F8545721}
{F8545722}
{F8545723}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21640
Summary:
Ref T9764. These "star" icons are unclear, inconsistent, and not friendly to colorblind users.
They date from a time long ago when the product didn't have icons.
Modernize them and make them more consistent with the similar statuses in Harbormaster.
Test Plan:
{F8545690}
{F8545691}
{F8545692}
Maniphest Tasks: T9764
Differential Revision: https://secure.phabricator.com/D21639
Summary: Ref T9499. When using the manual "Update Diff" workflow on the web, the "Repository" field isn't pre-filled properly. This can lead to revisions losing their repository after a manual update.
Test Plan: Did a manual update of a revision with a repository, saw it stick.
Maniphest Tasks: T9499
Differential Revision: https://secure.phabricator.com/D21638
Summary:
See PHI2032. The Conduit UI shows some "Example Custom Constraints" that are intended to be generic, but use of "statuses" is misleading since many objects have a status and most of them don't support these specific values.
Make it more clear that these are generic values.
Test Plan: Read new text.
Differential Revision: https://secure.phabricator.com/D21637
Summary: Ref T13641. Now that we can provide an "Active Devices" query, provide it and make it the default.
Test Plan: Viewed Almanac devices, got a list of active devices. Clicked "All Devices" to get all devices.
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21636
Summary:
Ref T13641. Phabricator sometimes makes intracluster requests that authenticate as a device.
Forbid these requests from authenticating as a disabled device.
Test Plan:
- Ran `bin/ssh-exec --phabricator-ssh-device ...` as an enabled/disabled device (worked; sensible error).
- Made Conduit calls as an enable/disabled device (worked; sensible error).
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21635
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean upgrade, saw migrated values in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21634
Summary: Ref T13065. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw migrated data in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13065
Differential Revision: https://secure.phabricator.com/D21633
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw keys migrate in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21632
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report, saw mail keys migrated to mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21631
Summary: Ref T13065. Ref T13641. Migrate "mailKey" and drop the column.
Test Plan: Ran "bin/storage upgrade", got a clean report and saw binding mail keys in the mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21630
Summary: Ref T13641. Ref T13065. Migrate and drop the onboard "mailKey" column for Almanac Services.
Test Plan: Ran storage migration, got clean report, saw migrated value in mail properties table.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641, T13065
Differential Revision: https://secure.phabricator.com/D21629
Summary: Ref T13641. Make "active bindings" a real query and make callers that only care about active bindings only query for active bindings.
Test Plan:
- Queried for "bindings" and "activeBindings" via Conduit.
- Disabled/enabled devices, saw binding status update in UI.
- Loaded Diffusion cluster layout.
- Grepped for `needBindings()`, `getActiveBindings()`, etc.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21628
Summary:
Ref T13641. Add a "status" property with most of the relevant support code.
This currently has no impact on use of the device or bindings by Diffusion or Drydock: they ignore the status of devices bound to services.
Test Plan:
- Created a new device.
- Changed the status of a device via web and API.
- Queried for devices via API.
- Searched for active and disabled devices.
- Viewed UI in list view, detail view.
- Used typeahead to add a new binding to an interface on a disabled device, got disabled hint in typeahead UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13641
Differential Revision: https://secure.phabricator.com/D21627
