Summary:
Fixes T12251. Ref T13189. See PHI610. The difficulty here is that we don't want to disclose Phabricator account information to Buildkite. We're comfortable disclosing information from `git`, etc.
- For commits, use the Identity to provide authorship information from Git.
- For revisions, use the local commit information on the Diff to provide the Git/Mercurial/etc author of the HEAD commit.
Test Plan:
- Built commits and revisions in Buildkite via Harbormaster.
- I can't actually figure out how to see author information on the Buildkite side, but the values look sane when dumped locally.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189, T12251
Differential Revision: https://secure.phabricator.com/D19614
Summary: Ref T13189. See PHI690. When a lease is first acquired or activated, note the time. This supports better visibility into queue lengths. For now, this is only queryable via DB and visible in the UI, but can be more broadly exposed in the future.
Test Plan: Landed a revision, saw the leases get sensible timestamps for acquisition/activation.
Reviewers: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19613
Summary: Ref T13189. Summaries do not appear to be meaningfully rendered with Remarkup so just drop the engine. See D19610 for the previous change in this vein.
Test Plan: Viewed config list with option summaries.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19612
Summary:
Depends on D19609. Ref T13189. At some point, we switched from RemarkupEngine to RemarkupView and lost this piece of hack-magic.
Restore the hack-magic. It's still hack-magic instead of a real rule, but things are at least cleaner than they were before.
Test Plan: Viewed `auth.require-approval`, etc. Saw references to other config options linked properly.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19610
Summary: Ref T13189. When there are no setup issues, we currently double-render a weird setup issues box underneath the notice. Get rid of it.
Test Plan: Viewed page with and without setup issues, saw less awkward UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19609
Summary:
Depends on D19607. Ref T13189. See PHI642. Ref T13186.
Some transactions can sometimes be applied to objects you can not edit. Currently, using `*.edit` to edit an object always explicitly requires CAN_EDIT.
Now that individual transactions require CAN_EDIT by default and can reduce or replace this requirement, stop requiring CAN_EDIT to reach the editor.
The only expected effect of this change is that low-permission edits (like disabling a user, leaving a project, or leaving a thread) can now work via `*.edit`.
Test Plan:
- Tried to perform a normal edit (changing a task title) against an object with no CAN_EDIT. Still got a permissions error.
- As a non-admin, disabled other users while holding the "Can Disable Users" permission.
- As a non-admin, got a permissions error while trying to disable other users while not holding the "Can Disable Users" permission.
Reviewers: amckinley
Maniphest Tasks: T13189, T13186
Differential Revision: https://secure.phabricator.com/D19608
Summary:
Depends on D19606. Ref T13189. See PHI642.
- Disabling/enabling users no longer requires admin. Now, you just need "Can Disable Users".
- Update the UI to appropriately show the action in black or grey depending on what clicking the button will do.
- For "Approve/Disapprove", fix a couple bugs, then let them go through without respect for "Can Disable Users". This is conceptually a different action, even though it ultimately sets the "Disabled" flag.
Test Plan:
- Disabled/enabled users from the web UI as various users, including a non-administrator with "Can Disable Users".
- Hit permissions errors from the web UI as various users, including an administrator without "Can Disable Users".
- Saw the "Disable/Enable" action activate properly based on whether clicking the button would actually work.
- Disapproved a user without "Can Disable Users" permission, tried to re-disapprove a user.
- Approved a user, tried to reapprove a user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19607
Summary:
Depends on D19605. Ref T13189. See PHI642. This adds a separate "Can Disable Users" capability, and makes the underlying transaction use it.
This doesn't actually let you weaken the permission, since all pathways need more permissions:
- `user.edit` needs CAN_EDIT.
- `user.disable/enable` need admin.
- Web UI workflow needs admin.
Upcoming changes will update these pathways.
Without additional changes, this does let you //strengthen// the permission.
This also fixes the inability to disable non-bot users via the web UI.
Test Plan:
- Set permission to "No One", tried to disable users. Got a tailored policy error.
- Set permission to "All Users", disabled/enabled a non-bot user.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19606
Summary:
Depends on D19604. Ref T13189. See PHI642. Deprecates these in favor of "user.edit", redefines them in terms of it, and removes the old `disableUser()` method.
I kept the "is admin" permissions check for consistency, since these methods have always said "(admin only)". This check may not be the most tailored check soon, but we can just keep executing it in addition to the real check.
For now, this change stops this method from actually disabling non-bot users (since it implicitly adds a CAN_EDIT requirement, and even administrators don't have that). An upcoming change will fix that.
Test Plan: Enabled and disabled a (bot) user via these methods. Checked API UI, saw them marked as "disabled".
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19605
Summary:
Ref T13189. See PHI642. Upgrades the "Disable" action in the web UI to be transaction-based.
This technically breaks things a little (you can't disable non-bot users, since they now require CAN_EDIT and you won't have it) but an upcoming change will fix the permissions issue.
Test Plan: Disabled and enabled a (bot) user from the web UI.
Reviewers: amckinley
Maniphest Tasks: T13189
Differential Revision: https://secure.phabricator.com/D19604
Summary:
Depends on D19585. Ref T13164.
Almost all transactions require CAN_EDIT on the object, but they generally do not enforce this directly today. Instead, this is effectively enforced by Controllers, API methods, and EditEngine doing a `CAN_EDIT` check when loading the object to be edited.
A small number of transactions do not require CAN_EDIT, and instead require only a weaker/lesser permission. These are:
- Joining a project which you have CAN_JOIN on.
- Leaving a project which isn't locked.
- Joining a Conpherence thread you can see (today, no separate CAN_JOIN permission for Conpherence).
- Leaving a Conpherence thread.
- Unsubscribing.
- Using the special `!history` command from email.
Additionally, these require CAN_INTERACT, which is weaker than CAN_EDIT:
- Adding comments.
- Subscribing.
- Awarding tokens.
Soon, I want to add "disabling users" to this list, so that you can disable users if you have "Can Disable User" permission, even if you can not otherwise edit users.
It's possible this list isn't exhaustive, so this change might break something by adding a policy check to a place where we previously didn't have one. If so, we can go weaken that policy check to the appropriate level.
Enforcement of these special cases is currently weird:
- We mostly don't actually enforce CAN_EDIT in the Editor; instead, it's enforced before you get to the editor (in EditEngine/Controllers).
- To apply a weaker requirement (like leaving comments or leaving a project), we let you get through the Controller without CAN_EDIT, then apply the weaker policy check in the Editor.
- Some transactions apply a confusing/redundant explicit CAN_EDIT policy check. These mostly got cleaned up in previous changes.
Instead, the new world order is:
- Every transaction has capability/policy requirements.
- The default is CAN_EDIT, but transactions can weaken this explicitly they want.
- So now we'll get requirements right in the Editor, even if Controllers or API endpoints make a mistake.
- And you don't have to copy/paste a bunch of code to say "yes, every transaction should require CAN_EDIT".
Test Plan:
- Tried to add members to a Conpherence thread I could not edit (permissions error).
- Left a Conpherence thread I could not edit (worked properly).
- Joined a thread I could see but could not edit (worked properly).
- Tried to join a thread I could not see (permissions error).
- Implemented `requireCapabilites()` on ManiphestTransactionEditor and tried to edit a task (upgrade guidance error).
- Mentioned an object I can not edit on another object (works).
- Mentioned another object on an object I can not edit (works).
- Added a `{F...}` reference to an object I can not edit (works).
- Awarded tokens to an object I can not edit (works).
- Subscribed/unsubscribed from an object I can not edit (works).
- Muted/unmuted an object I can not edit (works).
- Tried to do other types of edits to an object I can not edit (correctly results in a permissions error).
- Joined and left a project I can not edit (works).
- Tried to edit and add members to a project I can not edit (permissions error).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19586
Summary:
Ref T13187. See PHI811. If the file tree is enabled and visible, set the default tab to "History".
- This is a bit magic.
- It won't work entirely on mobile (we can't tell that you're on mobile on the server, so we'll pick the "History" tab even though the file tree isn't actually visible on your device).
- There's no corresponding logic in Diffusion. Diffusion doesn't have the same tab layout, but this makes things somewhat inconsistent.
So I don't love this, but we can try it and see if it's confusing or helpful on the balance.
Test Plan: With filetree on and off, reloaded revisions. Saw appropriate tab selected by default.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19601
Summary:
Ref T13187. See PHI836. The "action" comment actions in Differential (Accept, Reject, etc) render a single line of descriptive text. This is currently slightly misaligned.
Give it similar sizing information to the label element to the left, so it lines up properly.
Test Plan:
Note that "Request Review" and "This revision will be..." are now aligned:
{F5828077}
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19600
Summary: Ref T13187. Ref T13176. With drafts, we actually want to suppress this link on "first broadcast" (the first time we send mail), not on "new object" (when the revision is created as a draft).
Test Plan: Poked at this locally, will keep an eye on it in production.
Reviewers: amckinley
Maniphest Tasks: T13187, T13176
Differential Revision: https://secure.phabricator.com/D19598
Summary: Ref T13187. See PHI807. The documentation currently does not make it very clear that this is a local setting, per `phd` process. Make it more clear.
Test Plan: {F5827757}
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19597
Summary:
Ref T13187. See PHI834. Mercurial has somewhat-recently (changeset is from Jan 2018) introduced a new "protocaps" command, that appears in Mercurial 4.7 and possibly before then.
We must explicitly enumerate all protocol commands because you can't decode the protocol without knowing how many arguments the command expects, so enumerate it.
(Also fix an issue where the related error message had an extra apostrophe.)
Test Plan:
- Ran `hg clone ...` with client and server on Mercurial 4.7.
- Before: fatal on unknown "protocaps" command.
- Midway: better typography in error message.
- After: clean clone.
Reviewers: amckinley
Maniphest Tasks: T13187
Differential Revision: https://secure.phabricator.com/D19596
Summary:
Depends on D19594. See PHI823. Ref T13164.
- Add a label for the "X" button in comment areas, like "Remove Action: Change Subscribers".
- Add a label for the floating header display options menu in Differential.
- Add `role="button"` to `PHUIButtonView` objects that we render with an `<a ...>` tag.
Test Plan:
Viewed a revision with `?__aural__=true`:
- Saw "Remove Action: ..." label.
- Saw "Display Options" label.
- Used inspector to verify that some `<a class="button" ...>` now have `<a class="button" role="button" ...>`. This isn't exhaustive, but at least improves things. A specific example is the "edit", "reply", etc., actions on inline comments.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19595
Summary: Ref T12164. Updates another controller to use identities.
Test Plan:
Pretty ad-hoc, but loaded the main pages of several different repos with and without repo identities. I'm not totally convinced the `author` from this data structure is actually being used:
```
$return = array(
'commit' => $modified,
'date' => $date,
'author' => $author,
'details' => $details,
);
```
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19580
Summary:
Ref T13164. See PHI823. (See that issue for some more details and discussion.)
Add aural labels to various buttons which were missing reasonable aural labels.
The "Search" button (magnifying glass in the global search input) had an entire menu thing inside it. I moved that one level up and it doesn't look like it broke anything (?). All the other changes are pretty straightforward.
Test Plan:
{F5806497}
{F5806498}
- Will follow up on the issue to make sure things are in better shape for the reporting user.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19594
Summary:
Depends on D19585. Ref T13164. This is a precursor for D19586, which causes Editors to start doing more explicit CAN_EDIT checks.
Passwords have an Editor, but don't actually define a CAN_EDIT capability. Define one (you can edit a password if you can edit the object the password is associated with).
(Today, this object is always a User -- this table just unified VCS passwords and Account passwords so they can be handled more consistently.)
Test Plan:
- With D19586, ran unit tests and got a pass.
- Edited my own password.
- Tried to edit another user's password and wasn't permitted to.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19592
Summary: Depends on D19584. Ref T13164. This check is an //extra// check: you need EDIT //and// this capability. Thus, we can do it in validation without issues.
Test Plan:
- This code isn't reachable today: all methods of applying this transaction do a separate check for "Can Lock" upfront.
- Commented out the "Can Lock" check in the LockController, tried to lock as a user without permission. Was rejected with a policy exception.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19585
Summary:
Depends on D19583. Ref T13164. This continues the work of getting rid of `requireCapabilities()`.
This check is valid, but can be a `validateTransactions()` check instead. This is generally more consistent with how other applications work (e.g., creating subprojects).
The UI for this isn't terribly great: you get a policy error //after// you try to create the object. But that's how it worked before, so this isn't any worse than it was. The actual policy exception is (very) slightly more clear now (raised against the right object).
Test Plan:
- Created a child as a user with permission to do so to make sure I didn't break that.
- Set edit permission on `a/` to just me, tried to create `a/b/` as another user, got a policy exception since they can't edit the parent.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19584
Summary: Depends on D19582. Ref T13164. It's not possible to reach the editor without passing through a CAN_EDIT check, and it shouldn't be necessarily to manually specify that edits require CAN_EDIT by default.
Test Plan: Grepped for `RepositoryEditor`, verified that all callsites pass through a CAN_EDIT check.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19583
Summary:
Depends on D19581. Ref T13164. This method has no effect:
- You must always have CAN_EDIT to reach an Editor in the first place.
- Per previous change, I'm going to restructure this so transactions explicitly check CAN_EDIT by default anyway.
Test Plan: Tried to edit and hide a project column as a user without permission, hit global permission checks long before reaching this method.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19582
Summary:
Depends on D19579. Fixes T10003. These have been deprecated with a setup warning about their impending removal for about two and a half years.
Ref T13164. See PHI642. My overall goal here is to simplify how we handle transactions which have special policy behaviors. In particular, I'm hoping to replace `ApplicationTransactionEditor->requireCapabilities()` with a new, more clear policy check.
A problem with `requireCapabilities()` is that it doesn't actually enforce any policies in almost all cases: the default is "nothing", not CAN_EDIT. So it ends up looking like it's the right place to specialize policy checks, but it usually isn't.
For "Disable", I need to be able to weaken the check selectively (you can disable users if you have the permission, even if you can't edit them otherwise). We have a handful of other edits which work like this (notably, leaving and joining projects) but they're very rare.
Test Plan: Grepped for all removed classes. Edited a Maniphest task.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164, T10003
Differential Revision: https://secure.phabricator.com/D19581
Summary:
Depends on D19577. Ref T13164. See PHI642. This adds modern transaction-oriented enable/disable support.
Currently, this also doesn't let you disable normal users even when you're an administrator. I'll refine the policy model later in this change series, since that's also the goal here (let users set "Can Disable Users" to some more broad set of users than "Administrators").
This also leaves us with two different edit pathways: the old UserEditor one and the new UserTransactionEditor one. The next couple diffs will redefine the other pathways in terms of this pathway.
Test Plan:
- Enabled/disabled a bot.
- Tried to disable another non-bot user. This isn't allowed yet, since even as an administrator you don't have CAN_EDIT on them and currently need it: right now, there's no way for a particular set of transactions to say they can move forward with reduced permissions.
- Tried to enable/disable myself. This isn't allowed since you can't enable/disable yourself.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19579
Summary:
Depends on D19576. Ref T13164. See PHI642. This adds an EditEngine for users and a `user.edit` modern API method.
For now, all it supports is editing real name, blurb, title, and icon (same as "Edit Profile" from the UI).
Test Plan:
- Edited my stuff via the new API method.
- Tried to edit another user, got rejected by policies.
- Tried to create a user, got rejected by policies.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19577
Summary:
Ref T13164. See PHI642. I'd like to provide a third-generation `user.edit` API endpoint and make `user.enable` and `user.disable` obsolete before meddling with policy details, even if it isn't full-fledged yet.
Users do already have a transactions table and a Transaction-based editor, but it's only used for editing title, real name, etc. All of these are custom fields, so their support comes in automatically through CustomField extension code.
Realign it for modular transactions so new code will be fully modern. There are no actual standalone transaction types yet so this diff is pretty thin.
Test Plan:
- Grepped for `UserProfileEditor`.
- Edited a user's title/real name/icon.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19576
Summary: Currently the symbol generation scripts fail if passed a list containing no files because `explode("\n", $input)` returns `array("")` rather than `array()`. This means that a generic Harbormaster Build Plan with a step which executes `find . -type f -name '*.php' | ./scripts/generate_php_symbols.php` won't work because it fails in repositories that don't contain any PHP code.
Test Plan: Ran `echo | generate_php_symbols` and saw no output instead of an exception.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19588
Summary: Depends on D19491.
Test Plan: Viewed some commits where the identity was mapped to a user and another that wasn't; saw the header render either a link to the user or the identity object.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19492
Summary:
Ref T13164. See PHI748. Path validation has a 750ms timeout which blames to rP5038ab850c, in 2011.
Production path validation is sometimes taking more than 750ms, particularly on the initial page load where we may validate many paths simultaneously.
I have no idea why we have this timeout, and it isn't consistent with how we perform other AJAX requests. Just remove it.
Test Plan:
- Reproduced issue in production, saw all validation calls failing at 750ms. Actual underlying calls succeed, they just take more than 750ms to resolve.
- Loaded path validator locally, got green checkmark.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19575
Summary:
Ref T13164. See PHI788. The issue requests a "created" timestamp.
Also add filtering for repository, state, and author.
Test Plan:
Used all filters.
{F5795085}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19574
Summary:
Fixes T13184. In Almanac, interfaces are always added to devices. However, if you "Add New Interface" and then "Cancel", you go to the nonexistent `/interface/` page.
Instead, return to the device page.
Test Plan: From a device page, clicked "Add Interface" and then "Cancel". Ended up back where I was.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13184
Differential Revision: https://secure.phabricator.com/D19573
Summary:
Ref T13164. See PHI785. See D19546. I think I didn't test the updated error messaging here entirely properly, since I have some tasks in queue which error out here ("Missing argument 1 to newMailers(...)").
This is an error condition already, but we want to get through this call so we can raise a tailored message.
Test Plan: Tasks which errored out here now succeed. This condition is only reachable if you misconfigure things in the first place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19572
Summary:
Ref T13164. See PHI693. In Differential, you can {nav View Options > View Standalone} to get a standalone view of a single changeset. You can also arrive here via the big changeset list for revisions affecting a huge number of files.
We currently suggest that all the keyboard shortcuts work, but some do not. In particular, the "Next File" and "Previous File" keyboard shortcuts (and some similar shortcuts) do not work. In the main view, the next/previous files are on the same page. In the standalone view, we'd need to actually change the URI.
Ideally, we should do this (and, e.g., put prev/next links on the page). As a first step toward that, hide the nonfunctional shortcuts to stop users from being misled.
Test Plan:
- Viewed a revision in normal and standalone views.
- No changes in normal view, and all keys still work ("N", "P", etc).
- In standalone view, "?" no longer shows nonfunctional key commands.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19571
Summary: Ref T12164. Defines a new manual activity that suggests rebuilding repository identities before Phabricator begins to rely on them.
Test Plan:
- Ran migration, observed expected setup issue: {F5788217}
- Ran `bin/config done identities` and observed setup issue get marked as done.
- Ran `/bin/storage upgrade --apply phabricator:20170912.ferret.01.activity.php` to make sure I didn't break the reindex migration; observed reindex setup issue appear as expected.
- Ran `./bin/config done reindex` and observed reindex issue cleared as expected.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19497
Summary:
Fixes T12397. Ref T13164. See PHI801.
Several installs have hit various use cases where the path on disk where Phabricator lives changes at runtime. Currently, `bin/ssh-auth` caches a flat file which includes the path to `bin/ssh-exec`, so this may fall out of date if `phabricator/` moves.
These use cases have varying strengths of legitimacy, but "we're migrating to a new set of hosts and the pool is half old machines and half new machines" seems reasonably compelling and not a problem entirely of one's own making.
Test Plan:
- Compared output on `master` to output after change, found them byte-for-byte identical.
- Moved `phabricator/` to `phabricator2/`, ran `bin/ssh-auth`, got updated output.
- Added a new SSH key, saw it appear in the output.
- Grepped for `AUTHFILE_CACHEKEY` (no hits).
- Dropped the cache, verified that the file regenerates cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164, T12397
Differential Revision: https://secure.phabricator.com/D19568
Summary:
Ref T13164. See PHI790. Older versions of APCu reported cache keys as "key" from `apcu_cache_info()`. APC and newer APCu report it as "info".
Check both indexes for compatibility.
Test Plan:
- Locally, with newer APCu, saw no behavioral change.
- Will double check on `admin`, which has an older APCu with the "key" behavior.
- (I hunted this down by dumping `apcu_cache_info()` on `admin` to see what was going on.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19569
Summary:
Ref T13164. In PHI801, an install reported a particular slow Conduit method call.
Conduit calls aren't easily profilable with normal tools (for example, `arc call-conduit --xprofile ...` gives you a profile of the //client//). They can be profiled most easily with `bin/conduit call ... --xprofile`.
However, `bin/conduit call` currently doesn't let you pick a user to execute the command on behalf of, so it's not terribly useful for profiling `*.edit`-style methods which do a write: these need a real acting user.
Test Plan:
Ran `bin/conduit call --method user.whoami --as epriestley ...` with valid, invalid, and no acting users.
```
$ echo '{}' | ./bin/conduit call --method user.whoami --as epriestley --input -
Reading input from stdin...
{
"result": {
"phid": "PHID-USER-icyixzkx3f4ttv67avbn",
"userName": "epriestley",
"realName": "Evan Priestley",
...
```
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19566
Summary:
Ref T13164. PHI805 incidentally includes some `bin/storage probe` output for 100GB+ tables which renders wrong.
We have the tools to render it properly, so stop doing this manually and let ConsoleTable figure out the alignment.
Test Plan:
Faked very large table sizes, ran `bin/storage probe`:
{F5785946}
(Then, un-faked the very large table sizes and ran it again, got sensible output.)
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19567
Summary:
Ref T13164. See PHI766. Currently, when file data is stored in small chunks, we submit each chunk to the indexing engine.
However, chunks are never surfaced directly and can never be found via any search/query, so this work is pointless. Just skip it.
(It would be nice to do this a little more formally on `IndexableInterface` or similar as `isThisAnIndexableObject()`, but we'd have to add like a million empty "yes, index this always" methods to do that, and it seems unlikely that we'll end up with too many other objects like these.)
Test Plan:
- Ran `bin/harbormaster rebuild-log --id ... --force` before and after change, saw about 200 fewer queries after the change.
- Uploaded a uniquely named file and searched for it to make sure I didn't break that.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19563
Summary:
Ref T13164. See PHI725. For real "*.search" methods, parameters get validated and you get an error if you use an empty list as a constraint.
Since "transaction.search" isn't really a normal "*.search" method, it doesn't benefit from this. Just do the check manually for now.
Test Plan: Made `transaction.search` calls with no constraints (got results); a valid costraint (got fewer results); and an invalid empty constraint (got an exception).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19562
Summary:
Ref T13164. See PHI797. The last edit is available in the page header, but it's not precise (just says "180 days ago") and a little weird (it's unusual for us to put that kind of information in the header).
Add a precise timestamp to the footer for now. I'd imagine re-examining this the next time Phriction gets some UI work and maybe trying to integrate timeline/transactions more cleanly (see also T1894).
Test Plan: Looked at a wiki page, then edited it. Saw precise "Last Edit" timestamp adjacent to "Last Author".
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19560
Summary:
See D19558. This method has no callers and just wraps `diffusion.historyquery`, since D5960 (2013).
This was introduced in D315 (which didn't make it out of FB, I think) inside Facebook for unclear purposes in 2011.
Test Plan: Grepped for callers, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: artms
Differential Revision: https://secure.phabricator.com/D19559
Summary:
`diffusion.getrecentcommitsbypath` fails with 500 error when non existing callsign is passed:
```
>>> UNRECOVERABLE FATAL ERROR <<<
Call to a member function getCommit() on null
```
Expected Behavior:
Return more graceful error notifying caller that such callsign/repository does not exist
Reproduction steps:
Open conduit: https://secure.phabricator.com/conduit/method/diffusion.getrecentcommitsbypath/
Enter:
callsign: "obviouslynotexisting"
path: "/random"
Click call method
Test Plan: after applying patch - call no longer fails with 500s
Reviewers: Pawka, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19558
