Summary:
Ref T12296. This cache is used to cache Git ref heads (branches, tags, etc). Reasonable repositories may have more than 2048 of these.
When we miss the cache, we need to single-get refs to check them, which is relatively expensive.
Increasing the size of the cache to 65535 should only require about 7.5MB of RAM.
Additionally, fill only as much of the cache as actually fits. The FIFO nature of the cache can get us into trouble otherwise.
If we insert "A, B, C, D" and then lookup A, B, C, D, but the cache has maximum size 3, we get this:
- Insert A, B, C, D: cache is now "B, C, D".
- Lookup A: miss, single get, insert, purge, cache is now "C, D, A".
- Lookup B: miss, singel get, insert, purge, cache is now "D, A, B".
Test Plan:
- Reduced cache size to 5, observed reasonable behavior on the `array_slice()` locally with `bin/repository update` + `var_dump()`.
- Used this script to estimate the size of 65535 cache entries as 7.5MB:
```
epriestley@orbital ~ $ cat size.php
<?php
$cache = array();
$mem_start = memory_get_usage();
for ($ii = 0; $ii < 65535; $ii++) {
$cache[sha1($ii)] = true;
}
echo number_format(memory_get_usage() - $mem_start)." bytes\n";
epriestley@orbital ~ $ php -f size.php
7,602,176 bytes
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12296
Differential Revision: https://secure.phabricator.com/D17409
Summary:
Ref T12298. The trigger daemon already has routine long-term sleep, and few external events can impact when it should ideally wake up. The relevant events are:
- Someone creates a new Nuance source (ideally, we should wake up right away and start polling it).
- Someone creates a Calendar event about 16 minutes in the future (ideally, we should send them a reminder in about a minute).
- Someone changes GC config to be extremely aggressive (ideally, we should immediately respect the change).
None of these cases are very important. We don't hibernate for more than 3 minutes, so the worst case is that your Nuance source takes 3 minutes to start importing or your Calendar notification comes two minutes too late (13 minutes before the event instead of 15).
This change makes GC sightly more CPU-expensive on average: currently, we do a GC sweep every 4 hours. After this change, we'll end up doing one every 3 minutes, because we lose the fact that we did a sweep recently when the daemon restarts.
We could fix this by keeping track of when the last GC sweep was in the database, instead of in the Daemon process, but the cost of a sweep is normally very small so I don't plan to do this anytime soon.
Test Plan:
- Ran `bin/phd debug trigger`, saw daemon go through 3-minute hibernate + restart cycles.
- Ran `bin/phd debug task`, saw daemon run normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17408
Summary: Ref T6049. This moves Phurl to modular transactions.
Test Plan: Everything works here, add phurl, edit phurl, use phurl. Test various error states. Left a TODO on the validate dupe keys, not sure how to implement that in modular-land.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T6049
Differential Revision: https://secure.phabricator.com/D17405
Summary:
Fixes T12304. If you have a Herald rule which tries to add a commit author as an auditor, it fails validation when trying to apply.
Stop trying to apply these transactions, and explicitly tell the user why. Differential already uses a similar ruleset around reviewers, but Audit was using older code.
Test Plan:
- Wrote a Herald rule to add A, B and C as auditors.
- Committed as A.
- After change, saw B and C added with transacript guidance that A was the author.
{F3235660}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12304
Differential Revision: https://secure.phabricator.com/D17404
Summary:
Fixes T12302. Currently, we aren't merging multiple "AddAuditors" transactions correctly.
This can occur when Herald triggers multiple auditor rules.
Instead, merge them.
Test Plan:
- Wrote two different Herald rules that add auditors.
- Pushed a commit which triggered them.
- After the change, saw all the auditors get added correctly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12302
Differential Revision: https://secure.phabricator.com/D17403
Summary: Ref T12270. This converts Badges to modular transactions for editing and awarding.
Test Plan: Add Badge, edit badge, award and revoke... Still going to test this some more but feel free to comment on anything obviously wrong?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17402
Summary: Ref T12297. This could be fancier, but should make pulling profiles off `admin.phacility.com` significantly more realistic.
Test Plan: Dragged and dropped some profiles to upload them, then reviewed them via web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17401
Summary:
Ref T12297. This slightly modernizes the XHProf UI. Not included here:
- Some of the code acts like samples have PHIDs, but they currently do not. I plan to add them in the next change.
- I've intentionally left the actual list untouched for now -- it has some old/buggy code (like `flag-6` is no longer an icon) that I'll fix in a future change.
Test Plan: {F3224264}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17400
Summary: Ref T12297. When a page is generated with the profiler active, keep it active by adding a `__profile__` input to any forms we generate.
Test Plan: Hit Conduit API page with `__profile__` active, saw it reflected in forms.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12297
Differential Revision: https://secure.phabricator.com/D17399
Summary:
Fixes T12172. Fixes T12060. This allows runtime code building CSS for mail to read CSS variables, then makes all the code do that.
It reverts the non-colorblind red/green to the colors in use before T12060, which seem better for non-colorblind users since no one really complained?
Test Plan:
- Viewed code diffs in Web UI.
- Viewed prose diffs in Web UI.
- Viewed code diffs in email.
- Viewed prose diffs in email.
All modes respected the accessibility color scheme.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12172, T12060
Differential Revision: https://secure.phabricator.com/D17269
Summary: Ref T10390. Basically hides policy controls when creating a panel on a dashboard. Shows when you edit them or through normal workflow. I think we should maybe also get rid of view policy? Not sure the benefit since results will be filtered anyways. Maybe Text panels? Not sure the use case.
Test Plan: Add a panel, edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: hskiba, Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17393
Summary:
Fixes T12301. In D17372, this changed to use generic EditEngines instead of the proper runtime engine. Normally this doesn't matter, but can in this case.
After loading the configurations normally, swap their attached engines for the specific configured runtime engine we're currently executing.
Test Plan: Clicked "Create Form" from the Maniphest form list, saw it go to "Create Maniphest Form", not "Create Generic Meta-Form".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12301
Differential Revision: https://secure.phabricator.com/D17398
Summary:
Fixes T12306. Currently, we warn about daemons not running even if they're in normal "alive" states, particularly "waiting to restart after a failure".
This check was made more strict in D12088, back when we tried to version check running daemons. Since we implemented auto-restart-after-config-change we don't do this anymore, so it should be fine to make this more lax again.
Test Plan:
- Faked an exception for all tasks.
- Before patch: reloading the daemon setup error sometimes raised a false positive ("waiting" daemon detected as dead).
- After patch: daemon setup error no longer triggers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12306
Differential Revision: https://secure.phabricator.com/D17397
Summary:
Ref T12298. This updates `bin/phd` for minor changes to daemon configuration. In particular:
- Every daemon now has an autoscale pool (for trigger/pull, the maximum pool size is 1).
- Pools now have labels to make debugging a little easier.
- Some minor structural changes.
Test Plan: See D17389.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17390
Summary: Ref T10390. Simplifies dropdown by rolling out canUseInPanel in useless panels
Test Plan: Add a query panel, see less options.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17341
Summary:
See downstream <https://phabricator.kde.org/T5404>. This code was doing some `.firstChild` shenanigans which didn't survive some UI refactoring.
This whole UI is a little iffy but just unbreak it for now.
Test Plan: Allowed and rejected desktop notifications, got largely reasonable UI rendering.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17388
Summary: I broke this at the last second in D17374. `getStrList()` doesn't read arrays. It probably should (more modern analogs do) but don't rock the boat in the leadup to the release cut.
Test Plan: Hovered over a thing, saw a hovercard and no `getStrList()` error in my logs.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17383
Summary:
Ref T12287. See D17361. That fixed a mostly-theoretical bug with crumbs named things like "MMMMMMMMMMMMMMMMMMMM", but caused a less-theoretical buggy side effect in Safari.
For now, just keep the "MMMMMMMM" crumbs around since that's the easiest/least-bad/safest fix prior to the release cut. We can fix this more broadly when we have more time to look at it.
Test Plan: Looked at profiles, saw the entire name for "hector" and the too-long-crumb for "MMMMM".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12287
Differential Revision: https://secure.phabricator.com/D17382
Summary:
Ref T12268. Ref T12157. When you mention or interact with a user who is unlikely to be able to respond (for example, because their account is disabled), we try to show a colored dot to provide a hint about this.
Recently, we no longer send any normal mail to unverified addresses. However, the rules for showing a dot haven't been updated yet, so they only care about this if `auth.require-verification` is set. This can be misleading, because if you say `Hey @alice, what do you think about this?` and she hasn't verified her email, you may not get a response.
Update the rule so users with unverified email addresses get a grey dot in all cases. The hint is basically "you shouldn't expect a response from this user".
Make the meaning of this hint more clear on the hovercard and profile.
Also:
- Allow the non-ajax version of the hovercard page (which is basically only useful for testing hovercards) accept `?names=...` so you can just plug usernames, hashtags, etc., in there.
- Fix a bug where the user's join date was based on their profile creation date instead of account creation date on the hovercard. Users may not have a profile creation date (if they never changed any account details), and it may be different from their account creation date.
Test Plan: {F2998517}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12268, T12157
Differential Revision: https://secure.phabricator.com/D17374
Summary:
Fixes T12281. Some forms (like Settings) can't actually create new objects. Currently, though, you can select them and add them to profile menus; if you do, they fail when building an item.
Kick them out of the typeahead, and decline to render them in menus.
Test Plan:
Added "Create Settings" to a menu, no longer fatals after patch (item vanished from menu, still editable normally to get rid of it).
Tried to add another "Create Settings", no longer available in typehaead.
Added some normal stuff.
Viewed a choose-among-forms dropdown in Maniphest, which still worked normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12281
Differential Revision: https://secure.phabricator.com/D17372
Summary: See D14617. This could probably go either way but we don't currently need `$engine` in `newStandardEditField()`, so just get rid of it.
Test Plan: Edited a task with standard custom fields defined.
Reviewers: vrana, chad
Reviewed By: vrana
Differential Revision: https://secure.phabricator.com/D17370
Summary:
See D16676. When an export has an unsupported mode (bad database value, out-of-date object, etc) the intent of this code is to put it into the `<select />` so that you can save the form without silently changing the object.
However, it incorrectly calls `array_shift()` instead of `array_unshift()`.
Test Plan:
Edited a Calendar export with an invalid mode, saw the mode appear properly in the dropdown:
{F2957321}
Reviewers: vrana, chad
Reviewed By: vrana
Differential Revision: https://secure.phabricator.com/D17369
Summary: Ref T12270. Any project, badge, dashboard, etc, that uses names in crumbs can over generate a long title. Restrict to a sane but generous width.
Test Plan: Make a project with a really long name, test various crumb layouts, boards, tasks, desktop, mobile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12270
Differential Revision: https://secure.phabricator.com/D17361
Summary: Ref T10798. Cleans up the UI a little and adds a sidenav.
Test Plan: Review badge and recipients in sandbox.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10798
Differential Revision: https://secure.phabricator.com/D17358
Summary: Fixes T10473. Clever, didn't know we could do this, but works well. Renders out the tab names by ', '.
Test Plan:
Add a tab panel, change some names, review transactions.
{F2929594}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10473
Differential Revision: https://secure.phabricator.com/D17359
Summary: Fixes T12248. Adds a flag for movable panels, and only allows those to be moved. Also cleaned up some CSS rules missing once a panel was drug into a new position.
Test Plan: Try to drag a tab panel content pane, cannot. Drag normal pane, see CSS, grab and drag same panel back, CSS looks the same.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12248
Differential Revision: https://secure.phabricator.com/D17356
Summary: Fixes T11449. Feels.... magical? Probably a more efficient way of doing this, but only 6 tabs so...
Test Plan: Create a tab panel in old UI. Edit panel in new UI. Create a panel in new UI, edit panel in new UI.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11449
Differential Revision: https://secure.phabricator.com/D17355
Summary: Fixes T10145. I went with "don't add two panels", since panels are easy to create, I expect this to be a reasonable limit until we have better use cases.
Test Plan: Try to add the same panel twice, get error. Add panel normally fine, move panels fine, edit panels fine.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10145
Differential Revision: https://secure.phabricator.com/D17351
Summary: Bump up the CSS scope, since we altered the normal rule for device-desktop
Test Plan: /diviner/ in sandbox
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17352
Summary: Ref T10390, turns "add existing panel" into a typeahead, and add lots more information to search.
Test Plan: Add an existing panel, click the search icon, see more information (type, engine).
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17348
Summary: Fixes T10612. We're writing a new panel to any dashboard even if it already exists. No need when just updating a panel title.
Test Plan: Add "welcome" panel to column 2 of a clean dashboard. Edit title, save. See correct panel in correct place.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10612
Differential Revision: https://secure.phabricator.com/D17349
Summary: Fixes T12160. Lightbox thread view should be visible if file is public.
Test Plan:
Add a file to a task, log out, click on file in task, get lightbox and no error. Expand comments, see login box.
{F2867067}
{F2867088}
{F2867098}
{F2867114}
{F2867124}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12160
Differential Revision: https://secure.phabricator.com/D17347
Summary: Fixes T12258. I think these constants are just flipped.
Test Plan: Kinda winged it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12258
Differential Revision: https://secure.phabricator.com/D17346
Summary: Aligns the search result list to the right of the input box, adds a little space, removes a double border.
Test Plan: Search on desktop, mobile, tablet breakpoints. Double check normal typeahead results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17345
Summary:
Ref T12237. This adds a UI cue for users who have unverified primary addresses, since we no longer send them mail.
Also adds a new `bin/mail unverify` to unverify an address (for example, because mail is bouncing).
Test Plan:
- Unverified my address, saw setup issue.
- Verified my address, no more setup issue.
{F2861820}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12237
Differential Revision: https://secure.phabricator.com/D17344
Summary: Fixes T12253.
Test Plan:
- Before change: used "Quote Comment", saw "In null, alice wrote:" in quoted text.
- After change: used "Quote Comment", saw proper reference to the commit/page. Clicked reference, was sent to the comment properly.
{F2859093}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12253
Differential Revision: https://secure.phabricator.com/D17343
Summary: Ref T10390. This removes the "Copy Dashboard" feature, which was more of a crutch to assist in the complexity of building and maintaining dashboards. I think we're close enough now that removing this and adding in some simpler edit dialogs should negate any benefit to keeping this around. Also removed an un-used "Uninstall Dashboard" dialog.
Test Plan: Visit manage, edit, no longer see option to copy dashboard. grep /dashboards/ for "copy" and remove all traces. Add some panels to a dashboard I own.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10390
Differential Revision: https://secure.phabricator.com/D17338
Summary: Fixes T4984. This is about as fancy as I want to get this pass. Adds in the list of panel titles and the author. This does give me a rough idea what's on each dashboard.
Test Plan:
Visit a list of dashboards and see various authors and panels.
{F2810876}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T4984
Differential Revision: https://secure.phabricator.com/D17340
Test Plan: attempted to create a new auth provider; observed that "enabled" ui element does not render. viewed existing auth provider and observed that "enabled" ui element still renders
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Maniphest Tasks: T12245
Differential Revision: https://secure.phabricator.com/D17337
Summary: Fixes T12252.
Test Plan:
I just faked this, but likely repro is:
- Call method `x.y`.
- Remove method `x.y` from the codebase.
- View log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12252
Differential Revision: https://secure.phabricator.com/D17342
Summary: Fixes T12224. This brings "Autopay" on the View controller into line with how it works on the Edit controller.
Test Plan:
- Viewed subscriptions with no autopay, valid autopay, and deleted autopay.
{F2750725}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12224
Differential Revision: https://secure.phabricator.com/D17334
Summary: Fixes T12213. Removes truncation and allows titles to be full width if needed.
Test Plan:
Chrome / Firefox / Safari on Mac, mobile and desktop widths.
{F2754679}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12213
Differential Revision: https://secure.phabricator.com/D17336
