Summary: Ref T13105. Ref T13047. This makes symbol indexes work with DocumentEngine in Files, and restores support in Diffusion.
Test Plan: Command-clicked stuff, got taken to the symbol index with reasonable metadata in Diffusion, Differential and Files.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105, T13047
Differential Revision: https://secure.phabricator.com/D19307
Summary: Ref T13105. See also T7895. When users render very large files as source via DocumentEngine, skip highlighting.
Test Plan: Fiddled with the limit, viewed files, saw highlighting degrade.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19306
Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:
- Adding `$1-3` to the URI didn't work correctly with query parameters.
- Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.
Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19305
Summary:
Ref T13105. This breaks about 9,000 features but moves Diffusion to DocumentEngine for rendering. See T13105 for a more complete list of all the broken stuff.
But you can't bake a software without breaking all the features every time you make a change, right?
Test Plan: Viewed various files in Diffusion, used DocumentEngine features like highlighting and rendering engine selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Subscribers: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19302
Summary:
Ref T13105. This separates document rendering from the Controllers which trigger it so it can be reused elsewhere (notably, in Diffusion).
This shouldn't cause any application behavior to change, it just pulls the rendering logic out so it can be reused elsewhere.
Test Plan: Viewed various types of files in Files; toggled rendering, highlighting, and encoding.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19301
Summary: Ref T13105. Given that we now load blame with AJAX, it's not clear that there's any benefit to disabling it. This would also interact oddly with the document engine.
Test Plan: Viewed files in Diffusion, no longer saw blame-related options.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19300
Summary: This reverts D18524. See that revision for discussion.
Test Plan: Viewed home menu, saw application names as menu items.
Differential Revision: https://secure.phabricator.com/D19308
Summary: Fixes T13119. Ref T13120. This isn't the world's most elegant patch, but restores the debugging version of this view to service.
Test Plan: Viewed debugging phage (at `/typeahead/class/`). Used the actual proxy (by changing a datasource custom field from the comment area).
Maniphest Tasks: T13120, T13119
Differential Revision: https://secure.phabricator.com/D19304
Summary:
Depends on D19296. Ref T13110.
- Remove the "Large Changesets" documentation since we now degrade very large changesets and I don't have any evidence that anyone has ever tried to follow any of the recommendations in this document.
- Remove references to it.
- When an older revision doesn't have denormalized size information on the Revision object itself, don't render a scale element (instead of rendering a bogus one).
- Try to improve terminology consistency around "Large Change" (100-1000 files) vs "Very Large Change" (1000+ files) vs "Enormous Change" (too large to hold in memory).
Test Plan: Viewed revisions; grepped for documentation.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19298
Summary: Depends on D19295. Ref T13110. Degrade the review UX when users try to interact with changes which are too large to receive human review.
Test Plan: Reduced the "very large" limit, browsed some changes, saw various elements degrade.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19296
Summary: Ref T13110. Installs have various reasons for sending unreviewable changes (changes where the text of the change will never be reviewed by a human) through Differential anyway. Prepare for accommodating this more gracefully by building a standalone changeset list page which paginates the changesets.
Test Plan: Clicked the new "Changeset List" button on a revision, was taken to a separate page.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19295
Summary: Ref T13110. See PHI230. Show revision sizes on a roughly logarithmic scale from 1-7 stars. See D16322 for theorycrafting on this element.
Test Plan: Looked at some revisions, saw plausible-looking size markers.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19294
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.
Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.
Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19293
Summary: Depends on D19290. Ref T13110. Differential still has some hacks in place which require these methods to "very temporarily" be nonfinal, but the badness can be slightly reduced nowadays.
Test Plan: Loaded some pages, nothing fataled.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19291
Summary: Depends on D19289. Ref T13110. This flag has been obsolete for some time and has no callers.
Test Plan: Grepped for `hasReviewTransaction`, no hits.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19290
Summary:
Depends on D19288. Ref T13110. In addition to kicking revisions back to "Changes Planned" when builds fail, notify the author that they need to fix their awful garbage change.
(The actual email could be more useful than it currently is.)
Test Plan: Created a revision with failing remote builds, saw email about the problem generate.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19289
Summary: Depends on D19287. Ref T13110. Currently, "Abandon" and then "Reclaim" moves you out of "Draft" without setting the "Should Broadcast" flag. Keep these revisions in draft instead.
Test Plan: Reclaimed an abandoned + draft revision, got a draft revision instead of a "needs review + nonbroadcast" revision (which isn't a meaningful state).
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19288
Summary:
Depends on D19286. Ref T13110. After builds fail remote builds, put revisions back in the author's queue.
This doesn't actually notify the author quite yet.
Test Plan: Made a failing build plan run on revisions, created a revision, saw it demote after builds failed.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19287
Summary: Depends on D19285. Ref T13110. When you update an "Abandoned + But, Never Promoted" revision or (in the future) a "Changes Planned + But, Never Promoted" revision, return it to the "Draft" state rather than promoting it.
Test Plan: Updated an "Abandoned + Draft" revision, saw it return to "Draft".
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19286
Summary:
Depends on D19284. Ref T13110. It's now possible to get a revision into a "Abandoned + But, Never Promoted From Draft" state. Show this in the header and provide the draft hint above the comment area.
Also, remove `shouldBroadcast()`. The method `getShouldBroadcast()` now has the same meaning.
Finally, migrate existing drafts to `shouldBroadcast = false` and default `shouldBroadcast` to `true`. If we don't do this, every older revision becomes a non-broadcasting revision because this flag was not explicitly set on revision creation before, only on promotion out of draft.
Test Plan: Ran migration; abandoned draft revisions and ended up in a draft + abandoned state.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19285
Summary:
Depends on D19283. Ref T13110. To enable "Changes Planned + But, Still A Draft" and "Abandoned + But, Never Promoted From Draft" states, decouple the "broadcast" flag from the "draft" state.
Broadcast behavior is now based only on the `shouldBroadcast` flag, and revisions in any state may have this flag.
Revisions gain this flag when created as a non-draft, or when they leave the draft state for the first time.
There are probably still some ways you can get the wrong result here -- maybe abandon + update -- but those can be cleaned up as they arise.
Test Plan: Kinda poked it a bit but I'll vet this more heavily at the end of this sequence.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19284
Summary:
Depends on D19282. Ref T13110. I want to introduce "Changes Planned + Still A Draft" and "Abandoned + Still A Draft" states, at a minimum.
I think the "hasBroadcast" flag is effectively identical to a hypothetical "stillADraft" flag, so rename it to "shouldBroadcast" to better match its intended behavior.
This just changes labels, not any behavior.
Test Plan: Grepped for `hasBroadcast` and `HAS_BROADCAST`.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19283
Summary:
Depends on D19281. This increases consistency between build timeline publishing and revision draft promotion.
There's no real behavioral change here (switching how publishing worked already changed the beahvior) but this sends more callsites down the same code paths.
Since the builds we're looking at include completed builds, change the term "active" to "impactful". This describes the same set of builds, but hopefully describes them more accurately.
Test Plan: Created a local revision, saw it plausibly interact with draft status and promote. There are a lot of moving parts here and some stuff may well have slipped through.
Differential Revision: https://secure.phabricator.com/D19282
Summary:
Depends on D19280. Ref T13110. Although Harbormaster cares about all builds, Differential does not practically care about local lint and unit results in determining build status.
In Differential, orient publishing around "remote builds" instead of "builds".
This does not yet change any of the draft logic, it just makes the timeline story use newer logic.
Test Plan: Used `bin/harbormaster publish` (with some guard-clause removal) to publish some buildables to revisions without anything crashing.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19281
Summary:
Depends on D19279. Ref T13110. This implements the existing publishing logic for buildables, but does so via ModularTransactions instead of a core transaction type.
Since each application is implementing build transactions independently, this removes the core type.
Next, Differential will get a similar treatment.
Test Plan: Used `bin/harbormaster publish` (with some commenting-out-guard-clauses) to publish a commit Buildable; saw unchanged feed behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19280
Summary:
Depends on D19278. Ref T13110. This moves most of the structural logic for publishing builds to BuildableEngine and provides a `bin/harbormaster publish` to make publishing easy to retry/debug.
This intentionally removes the bit which actually does anything when builds publish. Followup changes will implement application-specific versions of the publishing logic in Differential and Diffusion.
Test Plan: Ran `bin/harbormaster publish Bxxx`, saw it do nothing (but not crash).
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19279
Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.
I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.
Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.
I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.
Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.
Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19278
Summary: Depends on D19273. Ref T13105. Adds "Change Text Encoding..." and "Highlight As..." options when rendering documents, and makes an effort to automatically detect and handle text encoding.
Test Plan:
- Uploaded a Shift-JIS file, saw it auto-detect as Shift-JIS.
- Converted files between encodings.
- Highlighted various things as "Rainbow", etc.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19274
Summary:
Ref T13105. This is silly, but "py" and "python" end up in different places today, and "py" is ~100x faster than "python".
See also T3626 for longer-term plans on this.
Test Plan: Reloaded a Jupyter notebook, saw it render almost instantly instead of taking a few seconds.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19273
Summary:
Ref T13114. See PHI514. This makes some attempt to undo the damage caused by incorrectly publishing a repository.
Don't run this.
Test Plan: Yikes.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19271
Summary:
Ref T13114. See PHI511. Ref T13072. This makes Buildables, Builds, Targets and Artifacts destructible with `bin/remove destroy`.
This might not be totally exhaustive. In particular:
- File artifacts won't destroy the file. This is sort of okay because file artifacts are currently just a file reference, but probably shouldn't be how things work in the long term.
- `BuildCommand` doesn't get cleaned up, but `BuildMessage` does on `Build`. See T13072 for more.
Test Plan: Used `bin/remove destroy` to nuke a bunch of builds, buildables, etc. Loaded stuff in the web UI and it all looked like it got nuked properly.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13114, T13072
Differential Revision: https://secure.phabricator.com/D19269
Summary:
Ref T13114.
- Followup fix for D19267, which didn't work correctly with //new// revision creation.
- Followup fix for changes in T11015. Some of the querying logic was still handling "/x.y" and "/x.y/" differently. Instead, normalize consistently to "/x.y/"
Test Plan:
- Created a new revision cleanly.
- Created a package owning only a `example.txt` file and saw Differential find it as an owning package in the table of contents.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19268
Summary: Ref T13114. See PHI515. Updating a revision with the same, currently active diff became an error at some point (probably D19175). This is inconsistent; make it an allowable no-op instead.
Test Plan:
- Updated a revision's diff via Conduit.
- Updated to the same diff, no-op.
- Tried to update a different revision, error ("already attached elsewhere").
- Updated with a different diff.
- Tried to update with the original diff, error ("previously attached version").
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19267
Summary: See PHI514. Ref T13114. Ref T8951. When a push is an "initial import" (a push of at least 7 commits to an empty repository) don't run Herald or enormous change protection.
Test Plan: Pushed some non-initial changes to a repository, and some initial changes.
Maniphest Tasks: T13114, T8951
Differential Revision: https://secure.phabricator.com/D19265
Summary: See PHI513. `fprintf()` takes `(thing, pattern, args, ...)` but we aren't passing a `pattern`, so if the command returns a "%" in the output we get an error.
Test Plan:
- Installed `bytes`, a great useful program which prints all the bytes, on my HoaxOS(tm) system (see D19102).
```
epriestley@orbital ~/dev/phabricator $ ./bin/drydock command --lease 76287 -- bytes # Before patch.
[2018-03-29 02:09:08] ERROR 2: fprintf(): Too few arguments at [/Users/epriestley/dev/core/lib/phabricator/src/applications/drydock/management/DrydockManagementCommandWorkflow.php:60]
arcanist(head=experimental, ref.master=b8c9c385a7f5, ref.experimental=925c60e7b837), corgi(head=master, ref.master=6371578c9d32), instances(head=master, ref.master=d983b9517924), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=hoax1, ref.master=b586ee065a75, ref.hoax1=f8d7480bbdd1, custom=4), phutil(head=master, ref.master=1ad42491e44a), secure(head=master, ref.master=988cf9bd7958), services(head=master, ref.master=6b3fb8d8dd0a)
#0 fprintf(resource, string) called at [<phabricator>/src/applications/drydock/management/DrydockManagementCommandWorkflow.php:60]
#1 DrydockManagementCommandWorkflow::execute(PhutilArgumentParser) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:441]
#2 PhutilArgumentParser::parseWorkflowsFull(array) called at [<phutil>/src/parser/argument/PhutilArgumentParser.php:333]
#3 PhutilArgumentParser::parseWorkflows(array) called at [<phabricator>/scripts/drydock/drydock_control.php:21]
epriestley@orbital ~/dev/phabricator $ ./bin/drydock command --lease 76287 -- bytes # After patch.
!"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\]^_`abcdefghijklmnopqrstuvwxyz{|}~????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????
```
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19264
Summary: Ref T13114. See PHI510. Firing Herald on mentioned objects tends to feel arbitrary and can substantially slow down edits which mention many objects.
Test Plan: Mentioned tasks on other tasks; verified that the normal path is hit normally, the new Herald-free path is hit on the mentioned object, and both still work fine and show up in the timeline.
Maniphest Tasks: T13114
Differential Revision: https://secure.phabricator.com/D19263
Summary: Depends on D19259. Ref T13105. Some examples represent image data as `["da", "ta"]` while others represent it as `"data"`. Accept either.
Test Plan: Rendered example notebooks with both kinds of images.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19260
Summary: Ref T13105. Currently, logged-out users can't render documents via the endpoint even if they otherwise have access to the file.
Test Plan: Viewed a file as a logged-out user and re-rendered it via Ajax.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19258
Summary:
Ref T13105. This adds various small client-side improvements to document rendering.
- In the menu, show which renderer is in use.
- Make linking to lines work.
- Make URIs persist information about which rendering engine is in use.
- Improve the UI feedback for transitions between document types.
- Load slower documents asynchronously by default.
- Discard irrelevant requests if you spam the view menu.
Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19256
Summary: Depends on D19254. This engine just formats JSON files in a nicer, more readable way.
Test Plan: Looked at some JSON files, saw them become formatted nicely.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Differential Revision: https://secure.phabricator.com/D19255
Summary: Ref T13105. Allow normal text files to be rendered as documents, and add a "source code" rendering engine.
Test Plan: Viewed some source code.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19254
Summary:
Depends on D19252. Ref T13105. This very roughly renders Jupyter notebooks.
It's probably better than showing the raw JSON, but not by much.
Test Plan:
- Viewed various notebooks with various cell types, including markdown, code, stdout, stderr, images, HTML, and Javascript.
- HTML and Javascript are not live-fired since they're wildly dangerous.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19253
Summary:
Depends on D19251. Ref T13105. This adds rendering engine support for PDFs.
It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time).
This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112.
Test Plan:
- Viewed PDFs in Files, got a link to view them in a new tab.
- Clicked the link in Safari, Chrome, and Firefox; got inline PDFs.
- Verified primary CSP is still `object-src 'none'` with `curl ...`.
- Interacted with the vanilla lightbox element to check that it still works.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19252
Summary:
Ref T13105. Although Markdown is trickier to deal with, we can handle Remarkup easily.
This may need some support for encoding options.
Test Plan: Viewed `.remarkup` files, got remarkup document presentation by default. Viewed other text files, got an option to render as remarkup.
Reviewers: avivey
Reviewed By: avivey
Subscribers: mydeveloperday, avivey
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19251
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:
- `writeWait`: Time spent waiting for a write lock.
- `readWait`: Time spent waiting for a read lock.
- `hostWait`: Roughly, total time spent on the leaf node.
The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.
Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19250
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting.
While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them.
Test Plan:
Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance:
> # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)...
Reviewers: asherkin
Reviewed By: asherkin
Subscribers: asherkin
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19247
Summary:
See PHI466. Ref T13108. Somewhat recently, new rules were added so that "Resigning" from a revision takes you off the default recipient list, even if you're still a member of a project or package that is still a reviewer or subscriber.
However, these rules don't currently apply to the similar expansion which occurs in notifications. If you resign from a revision you may still get some notifications (just not email) if a package or project you're a member of is a reviewer or subscriber.
(Possibly these should eventually share more code, but just get things working for now.)
Test Plan:
- Created a revision as A.
- Added B as a reviewer.
- Added a package B is an owner for as a reviewer.
- As B, resigned. (Make sure B is also not an explicit subscriber.)
- Commented on the revision as A.
- Before: B is included in the expanded notification recipient list.
- After: B is no longer included in the expanded notification recipient list.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19244
Summary:
This change prevents the following error when using PHP 7.2:
```
ERROR 2: count(): Parameter must be an array or an object that implements Countable at [/usr/local/lib/php/phabricator/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php:132]
```
A similar issue was fixed in D18964
Test Plan: Tested in a live system.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D19242
Summary:
Depends on D19238. Ref T13105. Give document engines some reasonable automatic support for degrading gracefully when someone tries to hexdump a 100MB file or similar.
Also, make "Video" sort above "Audio" for files which could be rendered either way.
Test Plan: Viewed audio, video, image, and other files. Adjusted limits and saw full, partial, and fallback/error rendering.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19239
Summary: Depends on D19237. Ref T13105. This adds a (very basic) "Hexdump" engine (mostly just to have a second option to switch to) and a selector for choosing view modes.
Test Plan: Viewed some files, switched between audio/video/image/hexdump.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19238
Summary:
Ref T13105. This change begins modularizing document rendering. I'm starting in Files since it's the use case with the smallest amount of complexity.
Currently, we hard-coding the inline rendering for images, audio, and video. Instead, use the modular engine pattern to make rendering flexible and extensible.
There aren't any options for switching modes yet and none of the renderers do anything fancy. This API is also probably very unstable.
Test Plan: Viewwed images, audio, video, and other files. Saw reasonable renderings, with "nothing can render this" for any other file type.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19237
Summary:
DarkConsole could warn when "Analyze Query Plans" was not active.
`msort()` is not stable, so Ferret results with similar relevance could be returned out-of-order.
Test Plan: Saw fewer traces and more-stable result ordering.
Differential Revision: https://secure.phabricator.com/D19236
Summary:
Ref T13108. See PHI364. See the task and issue for discussion.
If a `git fetch` during synchronization hangs, the whole node currently hangs. While the causes of a `git fetch` hang aren't clear, we don't expect synchronization to ever reasonably take more than 15 minutes, so add a default timeout.
Test Plan: Will deploy and observe; this is difficult to reproduce or test directly.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19235
Summary:
See PHI430. Ref T13102. When the "Build Status" element raises a policy exception, we currently fatal the whole page rather than raising a normal policy error.
This is because the policy check happens very late in page construction, long after we've made the decision to show the page instead of a policy error, and gets treated as a rendering error.
In turn, this is because the rendering is event-based rather than using a more modern Engine + EngineExtension sort of construct, so some of the actual logic runs way later than it should.
Since unwinding all of this isn't trivial and the current behavior is materially bad, limit the damage here for now by just hiding the element. See T13088 for notes on handling this in a more nuanced way in the future.
Test Plan:
- Created a revision visible to "Public".
- Ran a build against it with a build plan visible to "All Users".
- Viewed revision in an incognito window.
- Before patch: Policy fatal with a red "rendering phase" error box.
- After patch: Mostly-functional page with a missing "Build Status" element.
- Viewed revision as a user with a normal session, saw the same UI before and after the change.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19232
Summary:
Ref T13102. See PHI461. An install is interested in querying projects by slug.
I think I omitted this capability originally only because we're not consistent about what slugs are called (they are "Slugs" internally, but "Hashtags" in the UI).
However, this ship has sort of already sailed because the results have a "slug" field. Just expose this as "slugs" for consistency with the existing API field and try to smooth thing over with a little documentation hint.
Test Plan: Queried for projects by slug, got the desired results back.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19230
Summary: Ref T13106. When profiling service queries, there's no convenient way to easily get a sense of why a query was issued. Add a mode to collect traces for each query to make this more clear. This is rough, but works well enough to be useful.
Test Plan: Clicked "Analyze Query Plans", got stack traces for each service call.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19221
Summary:
See PHI457. There's no real reason not to allow this, it just wasn't clear if it was useful. See D18626.
An install had a user `arc diff` and then sprint out the door to take a very long vacation before the builds finished. One failed, so the revision is stuck as a draft forever. This seems like a reasonable motivation for allowing "Commandeer".
Test Plan: Successfully commandeered a draft.
Differential Revision: https://secure.phabricator.com/D19228
Summary: See PHI448. Ref T13106. The current implementation here can end up in an infinite stack if, e.g., a project uses "Visible to: Subscribers".
Test Plan: Will push.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19226
Summary: Depends on D19224. Ref T13106. Computing this is expensive and the value is not used. This came from D15432, but we never actually shipped that feature.
Test Plan: Saw local query cost drop from 139 to 110 with no change in functionality. Grepped for removed symbols.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19225
Summary:
Depends on D19223. Ref T13106. When we're loading a file, we currently test if it's a transformed version of another file (usually, a thumbnail) and apply policy behavior if it is.
We know that builtins and profile images are never transforms and that the policy behavior for these files doesn't matter anyway. Skip loading transforms for these files.
Test Plan: Saw local queries drop from 146 to 139 with no change in behavior.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19224
Summary:
Depends on D19222. Ref T13106. We currently execute an edge query (and possibly an object query) when loading builtin files, but this is never necessary because we know these files are always visible.
Instead, skip this logic for builtin files and profile image files; these files have global visibility and will never get a different policy result because of file attachment information.
(In theory, we could additionally skip this for files with the most open visibility policy or some other trivially visible policy like the user's PHID, but we do actually care about the attachment data some of the time.)
Test Plan: Saw queries drop from 151 to 145 on local test page. Checked file attachment data in Files, saw it still working correctly.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19223
Summary: Depends on D19221. Ref T13106. When we fall back to default profile images for projects, bulk load them instead of doing individual queries.
Test Plan: Saw local task drop from 199 queries to 151 queries with the same actual outcome. Saw custom and default profile images on the project list page.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19222
Summary:
See PHI451. Ref T13102. DarkConsole uses an ancient inline "onclick" handler to expand the stack traces for errors.
The new Content-Security-Policy prevents this from functioning.
Replace this with a more modern behavior-driven action instead.
Test Plan:
- Clicked some errors in DarkConsole, saw stack traces appear.
- Grepped for `onclick` and `jsprintf()` to see if I could find any more of these, but came up empty.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19218
Summary:
See PHI446. Ref T13088. Currently, there's no way to access older generations of a build unless you know the secret `?g=1` URI magic.
When a build has multiple generations, show a history table and let users click to see older run information.
This is currently very basic. It would be nice to show when each generation started, who started/restarted it, and what the build status was at the time the build was restarted. There's currently no convenient source for this information so just add a bare-bones, working version of this for now.
Test Plan:
Viewed pending, single-run and multi-restart builds. Saw table on builds with more than one generation. Clicked table entries to see different build data.
{F5471160}
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19217
Summary:
Fixes T12994. We need `MYSQLI_ASYNC` to implement client-side query timeouts, and we need MySQLi + MySQL Native Driver to get `MYSQLI_ASYNC`.
Recommend users install MySQLi and MySQL Native Driver if they don't have them. These are generally the defaults and best practice anyway, but Ubuntu makes it easy to use the older stuff.
All the cases we're currently aware of stem from `apt-get install php5-mysql` (which explicitly selects the non-native driver) so issue particular guidance about `php5-mysqlnd`.
Test Plan:
- Faked both issues locally, reviewed the text.
- Will deploy to `secure`, which currently has the non-native driver.
Maniphest Tasks: T12994
Differential Revision: https://secure.phabricator.com/D19216
Summary:
Ref T13102. An install has a custom rule for bridging JIRA references via Doorkeeper and would like to be able to render them as `JIRA-123` instead of `JIRA JIRA-123 Full JIRA title`.
I think it's reasonable to imagine future support upstream for `JIRA-123`, `{JIRA-123}`, and so on, although we do not support these today. We can take a small step toward eventual support by letting the rendering pipeline understand different view modes.
This adds an optional `name` (the default text rendered before we do the OAuth sync) and an optional `view`, which can be `short` or `full`.
Test Plan:
I tested this primarily with Asana, since it's less of a pain to set up than JIRA. The logic should be similar, hopefully.
I changed `DoorkeeperAsanaRemarkupRule` to specify `name` and `view`, e.g `'view' => (mt_rand(0, 1) ? 'short' : 'full')`. Then I made a bunch of Asana references in a comment and saw them randomly go short or long.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19215
Summary:
Fixes T5741. We break GIFs apart with "-coalesce" which completely rasterizes each frame, but stitch them back together without specifying "-dispose".
This produces the default "-dispose none" behavior, which causes GIF frames to "pile up" if they contain transparency.
Instead, use "-dispose background" so that the previous frame is erased before each new frame is drawn.
Test Plan: See T5741 for additional details.
Maniphest Tasks: T5741
Differential Revision: https://secure.phabricator.com/D19214
Summary:
Fixes T13104. The "Subscribers" policy implementation still uses older logic to query project membership and misses parent projects and milestones which a user is a member of.
Instead of doing an edge query for explicit membership, use a project query to find all projects the viewer belongs to.
Test Plan:
- Created a parent project A.
- Created a subproject B.
- As Bailey, created a task with "Visible To: Bailey, Subscribers".
- Added parent project A as a task subscriber.
Then:
- As Alice, verified I could not see the task.
- As Alice, joined subproject B.
- Before patch: still unable to see the task.
- After patch: can see the task.
- Removed parent project A as a subscriber, verified I could no longer see the task.
Maniphest Tasks: T13104
Differential Revision: https://secure.phabricator.com/D19213
Summary:
See PHI431. Ref T13102. An install is interested in a custom "non-sticky" accept action, roughly.
On the one hand, this is a pretty hacky patch. However, I suspect it inches us closer to T731, and I'm generally comfortable with exploring the realms of "Accept Next Update", "Unblock Without Accepting", etc., as long as most of it doesn't end up enabled by default in the upstream.
Test Plan:
- Accepted and updated revisions normally, saw accepts respect global stickiness.
- Modified the "Accept" action to explicitly be unsticky, saw nonsticky accept behavior after update.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19211
Summary: See PHI433. Ref T13102. Users in the wild have mixed expecations about exactly what "draft" means. Recent changes have tried to make behavior more clear. As part of clarifying messaging, make it explicit that `@mention` does not work on drafts by showing users a warning when they try to `@mention` a user.
Test Plan:
- Mentioned users on drafts, got a warning.
- Posted normal comments on drafts, no warning.
- Posted normal/mention comments on non-drafts, no warning.
Maniphest Tasks: T13102
Differential Revision: https://secure.phabricator.com/D19210
Summary: Ref T13103. Make favicons customizable, and perform dynamic compositing to add marker to indicate things like "unread messages".
Test Plan: Viewed favicons in Safari, Firefox and Chrome. With unread messages, saw pink dot composited into icon.
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19209
Summary: See PHI439. Use slightly richer "dominion" return values for consistency.
Test Plan: Fetched results with `owners.search` API method.
Differential Revision: https://secure.phabricator.com/D19208
Summary:
See PHI439. This fills in additional information about Owners packages.
Also removes dead `primaryOwnerPHID`.
Test Plan: Called `owners.search` and reviewed the results. Grepped for `primaryOwnerPHID`.
Differential Revision: https://secure.phabricator.com/D19207
Summary: See PHI433. This beefs up reminder texts for drafts a little bit since some users in the wild aren't always seeing/remembering the existing, fairly subtle hints.
Test Plan: Created a reivsion with `--draft`, viewed it, saw richer reminders.
Differential Revision: https://secure.phabricator.com/D19204
Summary:
Depends on D19201. Ref T13101. This likely produces relatively stable-ish image references for email.
They currently TTL after 30 days but this makes the jokes more exclusive and special so it's a feature, not a bug.
Test Plan: I'm just going to test this in production because I'm a ninja superstar developer.
Maniphest Tasks: T13101
Differential Revision: https://secure.phabricator.com/D19203
Summary:
Depends on D19198. Ref T13101. Ref T5258. Pull compositing logic out of the `Controller`.
This is moving toward fixing memes in email.
Test Plan: Used new and old memes. Used API memes.
Maniphest Tasks: T13101, T5258
Differential Revision: https://secure.phabricator.com/D19200
Summary: See PHI432. Ref T13099. Short names never made it to the UI/API but seem stable now, so support them.
Test Plan: {F5465173}
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19202
Summary:
Ref T13101. This is a minimal change to make "{meme ...}" work with the new Content-Security-Policy by using an Ajax request to generate the image and then swapping the source on the client.
This could be much cleaner (see T5258, etc).
Test Plan: Used `{meme, src=cat6, above=i am, below=cat}`, chuckled completely unironically.
Maniphest Tasks: T13101
Differential Revision: https://secure.phabricator.com/D19196
Summary: Depends on D19194. Fixes T4190. This should be in good-enough shape now to release and support more generally.
Test Plan: Used `{image ...}` in remarkup.
Maniphest Tasks: T4190
Differential Revision: https://secure.phabricator.com/D19195
Summary: Depends on D19193. Ref T13101. Fixes T4190. Before we render a fancy AJAX placeholder, check if we already have a valid cache for the image. If we do, render a direct `<img />` tag. This is a little cleaner and, e.g., avoids flicker in Safari, at least.
Test Plan: Rendered `{image ...}` rules in remarkup with new and existing URIs.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19194
Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.
This isn't the most polished implementation imaginable, but it's much less mysterious about errors.
Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19193
Summary:
Ref T13101. Ref T4190. This rule is currently single-phase but I'd like to check for a valid proxied image in cache already and just emit an `<img ... />` tag pointing at it if we have one.
To support batching these lookups, split the rule into a parse phase (where we extract URIs) and a markup phase (where we build tags).
Test Plan: Used `{img ...}` in Remarkup with no apparent behavioral changes. (This change should do nothing on its own.)
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19192
Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.
Test Plan:
- Edited paths: include/exclude paths, from different repositories, different actual paths.
- Used "Add New Path" to add rows, got repository selector prepopulated with last value.
- Used "remove".
- Used validation typeahead, got reasonable behaviors?
The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.
Maniphest Tasks: T13099, T12590
Differential Revision: https://secure.phabricator.com/D19191
Summary:
Depends on D19189. Ref T12590. The "validate" and "complete" endpoints for this UI could incorrectly return redirect responses. These aren't critical to the behavior of Owners, but they're nice to have, and shouldn't redirect.
Instead, skip the canonicalizing redirect for AJAX requests.
Test Plan: Edited Owners paths in a repository with a short name, got completion/validation again.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19190
Summary: Ref T12590. This is ancient code which was used to prefill `/trunk/tfb/www/` or similar at Facebook. I don't think it ever had a UI and no install has asked for this feature since 2011.
Test Plan: Grepped for affected symbols, edited paths in Owners.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19189
Summary: See PHI425. See T12678. This should be an integer, but may be a string.
Test Plan: Called `differential.revision.edit`, observed integer in result instead of string.
Differential Revision: https://secure.phabricator.com/D19186
Summary: Depends on D19184. Ref T11015. Now that we have a digest index column, we can improve some of the queries a bit.
Test Plan:
- Ran queries from revision pages before and after with and without EXPLAIN.
- Saw the same results with much better EXPLAIN plans.
- Fragment size is now fixed at 12 bytes per fragment, so we can shove more of them in a single query.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19185
Summary:
Depends on D19183. Ref T11015. Currently, adding a trailing slash works great and omitting it mysteriously doesn't work.
Store a normalized version with an unconditional trailing slash for the lookup logic to operate on, and a separate display version which tracks what the user actually typed.
Test Plan:
- Entered "/src/main.c", "/src/main.c/", saw them de-duplicate.
- Entered "/src/main.c", saw it stay that way in the UI but appear as "/src/main.c/" internally.
- Added a rule for "/src/applications/owners" (no slash), created a revision touching paths in that directory, saw Owners fire for it.
- Changed the display value of a path only ("/src/main.c" to "/src/main.c/"), saw the update reflected in the UI without any beahvioral change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19184
Summary:
Depends on D19182. Ref T11015. This changes `path` from `text255` to `longtext` because paths may be arbitrarily long.
It adds `pathDisplay` to prepare for display paths and storage paths having different values. For now, `pathDisplay` is copied from `path` and always has the same value.
Test Plan:
- Ran migration, checked database for sanity (all `pathDisplay` and `path` values identical).
- Added new paths, saw `pathDisplay` and `path` get the same values.
- Added an unreasonably enormous path with far more than 255 characters.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19183
Summary:
Depends on D19181. Ref T11015. This nukes duplicates from the table if they exist, then adds a unique key.
(Duplicates should not exist and can not be added with any recent version of the web UI.)
Test Plan:
- Tried to add duplicates with web UI, didn't have any luck.
- Explicitly added duplicates with manual `INSERT`s.
- Viewed packages in web UI and saw duplicates.
- Ran migrations, got a clean purge and a nice unique key.
- There's still no way to actually hit a duplicate key error in the UI (unless you can collide hashes, I suppose), this is purely a correctness/robustness change.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19182
Summary: Ref T11015. This supports making path names arbitrarily long and putting a proper unique key on the table.
Test Plan:
- Migrated, checked database, saw nice digested indexes.
- Edited a package, saw new rows update with digested indexes.
Maniphest Tasks: T11015
Differential Revision: https://secure.phabricator.com/D19181
Summary: Ref T13099. See PHI424. Fixes T11664. Several installs are interested in having these behaviors available in Owners by default and they aren't difficult to provide, it just makes the UI kind of messy. But I think there's enough general interest to justify it, now.
Test Plan: Created a package which owns "/" with a "With Non-Owner Author" review rule which I own. Created a revision, no package reviewer. Changed rule to "All", updated revision, got package reviewer.
Maniphest Tasks: T13099, T11664
Differential Revision: https://secure.phabricator.com/D19180
Summary:
Ref T13099. Ref T12787. See PHI417. Differential has new "irresponsible" warnings in the timeline somewhat recently, but these publish feed stories that don't link to the revision or have other relevant details, so they're confusing on the balance.
These have a high strength so they render on top, but we actually just want to hide them from the feed and let "abraham closed Dxyz by committing rXzzz." be the primary story.
Modularize things more so that we can get this behavior. Also, respect `shouldHideForFeed()` at display time, not just publishing time.
Test Plan: Used `bin/differential attach-commit` on a non-accepted revision to "irresponsibly land" a revision. Verified that feed story now shows "closed by commit" instead of "closed irresponsibly".
Maniphest Tasks: T13099, T12787
Differential Revision: https://secure.phabricator.com/D19179
Summary: Ref T13100. Since rules may begin failing for PRCE configuration reasons soon, provide a more complete explanation of possible causes in the UI.
Test Plan: Faked this, hit it via test console, saw explanation in web UI.
Maniphest Tasks: T13100
Differential Revision: https://secure.phabricator.com/D19178
Summary:
Ref T13099. See that task for discussion. Chrome is unhappy with an MFA form submitting to an endpoint which redirects you to an OAuth URI.
Instead, do the redirect entirely on the client.
Chrome's rationale here isn't obvious, so we may be able to revert this at some point.
Test Plan: Went through the OAuth flow locally, was redirected on the client. Will verify in production.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19177
Summary: Depends on D19175. Ref T13099. This fills in "close" and "update" transactions so that they show which commit(s) caused the action.
Test Plan: Used `transaction.search` to query some revisions, saw commit PHID information.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19176
Summary: Ref T13099. Move most of the "Update" logic to modular transactions
Test Plan: Created and updated revisions. Flushed the task queue. Grepped for `TYPE_UPDATE`. Reviewed update transactions in the timeline and feed.
Maniphest Tasks: T13099
Differential Revision: https://secure.phabricator.com/D19175
Summary: Depends on D19173. Ref T13096. Adds an optional, disabled-by-default lock log to make it easier to figure out what is acquiring and holding locks.
Test Plan: Ran `bin/lock log --enable`, `--disable`, `--name`, etc. Saw sensible-looking output with log enabled and daemons restarted. Saw no additional output with log disabled and daemons restarted.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19174
Summary:
Ref T13096. Currently, we do a fair amount of clever digesting and string manipulation to build lock names which are less than 64 characters long while still being reasonably readable.
Instead, do more of this automatically. This will let lock acquisition become simpler and make it more possible to build a useful lock log.
Test Plan: Ran `bin/repository update`, saw a reasonable lock acquire and release.
Maniphest Tasks: T13096
Differential Revision: https://secure.phabricator.com/D19173
Summary:
See PHI416. If you raise a lint message in a deleted file, we don't render any text on the right hand side so the message never displays.
This is occasionally still legitimate/useful, e.g. to display a "don't delete this file" message. At least for now, show these messages on the left.
Test Plan: Posted a lint message on a deleted file via `harbormaster.sendmessage`, viewed revision, saw file expand with synthetic inline for lint.
Differential Revision: https://secure.phabricator.com/D19171
Summary: See PHI193. Previously, see similar D18763. Skip this legacy-style policy check when creating a project, since we know you can add members, even if the policy doesn't actually resolve in your favor.
Test Plan:
- Created a project with edit policy "Members of project" and myself, plus any other user (so the code goes down this path, not the "join/leave" path) as members.
Differential Revision: https://secure.phabricator.com/D19169
Summary:
See PHI413. You can pre-generate these with `bin/people profileimage --all`, but they're needlessly expensive to generate.
Streamline the workflow and cache some of the cacheable parts to reduce the generation cost.
Test Plan:
- Ran `bin/people profileimage --all` and saw cost drop from {nav 15.801s > 4.839s}.
- Set `defaultProfileImagePHID` to `NULL` in `phabricator_user.user` and purged caches with `bin/cache purge --all`.
- Loaded user directory.
- Saw default images regenerate relatively quickly.
Differential Revision: https://secure.phabricator.com/D19168
Summary:
Depends on D19166. Ref T13088. When the user scrolls away from a followed log, break the focus lock.
Let users stop following a live log.
Show when lines are added more clearly.
Don't refresh quite as quickly give users a better shot at clicking the stop button.
These behaviors can probably be refined but are at least more plausible and less actively user-hostile than the first version of this behavior was.
Test Plan: Used `write-log --rate` to write a large log slowly. Clicked "Follow Log", followed for a bit. Scrolled away, still got live updates but no more scroll lock. Clicked stop, no more updates.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19167
Summary:
Depends on D19165. Ref T13088. Currently, in other applications, we use Zero Width Spaces and Javascript "copy" listeners to prevent line numbers from being copied. This isn't terribly elegant.
Modern browsers support a second approach: using psuedo-elements with `content`. Try this in Harbormaster since it's conceptually cleaner, at least. One immediate drawback is that Command-F can't find this text either.
Test Plan: In Safari, Chrome and Firefox, highlighted ranges of lines and copy/pasted text. Got just text (no line numbers) in all cases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19166
Summary: Depends on D19164. Ref T13088. Now that the JS behaviors are generic, use them on the Harbormaster standalone page.
Test Plan: Clicked lines and dragged across line ranges. Reloaded pages. Saw expected highlighting behavior in the client and on the server across reloads.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19165
Summary:
Depends on D19162. Ref T13088. When a user links to `$1234`, we need to render a default view of the log with a piece at the head, a piece at the end, and a piece in the middle.
We also need to figure out the offset for line 1234, or multiple offsets for "1234-2345".
Since the logic views/reads mostly anticipated this it isn't too much of a mess, although there are a couple of bugs this exposes with view specifications that use combinations of parameters which were previously impossible.
Test Plan: Viewed a large log with no line marker. Viewed `$1`. Viewed `$end`. Viewed `$35-40`, etc. Expanded context around logs.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19163
Summary: Ref T13088. This lifts the code for parsing "$x-y" line ranges in URIs into AphrontRequest so Diffusion, Paste, Harbormaster, etc., can share it.
Test Plan: Viewed lines, line ranges, no lines, negative line ranges, line ranges with 0, and extremely long line ranges in Paste.
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19162
Summary:
Ref T4340. Some "Register/Login" and "Link External Account" buttons are forms which submit to third-party sites. Whitelist these targets when pages render an OAuth form.
Safari, at least, also prevents a redirect to a third-party domain after a form submission to the local domain, so when we first redirect locally (as with Twitter and other OAuth1 providers) we need to authorize an additional URI.
Test Plan: Clicked all my registration buttons locally without hitting CSP issues.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19159
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary:
Depends on D19154. Ref T13094. This controller was removed at some point and this route no longer works.
I plan to add a new `download/` route to let us tighten the `form-action` Content Security Policy.
Test Plan: Grepped for the route and controller, no hits.
Maniphest Tasks: T13094
Differential Revision: https://secure.phabricator.com/D19155
Summary: Depends on D19152. Ref T13088. This adds live log tailing. It is probably not the final version of this feature because it prevents escape once you begin tailing a log.
Test Plan: Used `bin/harbormaster write-log --rate ...` to write a log slowly. Viewed it in the web UI. Clicked "Follow Log". Followed the log until the write finished, a lifetime later.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19153
Summary: Depends on D19151. Ref T13088. While dramatically less exciting than using `lolcat` and less general than `pv`, this should do the job adequately.
Test Plan: Piped a sizable log into `bin/harbormaster write-log` with `--rate 2048`, saw a progress bar. Loaded the log in the web UI and saw it grow as the page reloaded.
Reviewers: yelirekim
Reviewed By: yelirekim
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19152
Summary:
Depends on D19150. Ref T13088. Allow clients to retrieve information about build logs, including log data, over the API.
(To fetch log data, take the `filePHID` to `file.search`, then issue a normal GET against the URI. Use a `Content-Range` header to get part of the log.)
Test Plan: Ran `harbormaster.log.search`, got sensible-looking results.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19151
Summary: Depends on D19149. Ref T13088. Since the new log requires a bunch of log reprocessing, the cutover is going to require at least some time for installs to run migrations. Add a link in the UI to ease the transition, smooth over some behaviors a little, and fix a fetch issue where we'd request past the end of the log (since this is now enforced).
Test Plan: Viewed a traditional Harbormaster build, saw links to the new standalone log pages.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19150
Summary: Depends on D19148. Ref T13088. The new rendering always executes range requests for data it needs, and we can satisfy these requests by loading the smallest number of chunks which span that range.
Test Plan: Piped 50,000 lines of Apache log into Harbormaster, viewed it in the new UI, got sensible rendering times and a reasonable amount of data actually going over the wire.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19149
Summary: Ref T13088. This variable bled through from an earlier loop and caused us to drop some of the lines in the middle.
Test Plan: Clicked "Show More", got an equal number of header and footer lines.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19148
Summary: Ref T13093. Depends on D19145. See PHI398. Previously, see D18933. This provides the current viewer to `ConduitCall` so that we don't try to use device credentials from unprivileged web hosts.
Test Plan: Evaluated the "Branches" field locally, saw an appropriate field value.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19146
Summary:
Ref T13093. See PHI396. These are possibly somewhat niche, but reasonable to support and consistent with the existing "Pusher's projects".
Also relabel "Pusher's projects" and "Project tags" for consistency and, hopefully, clarity.
Test Plan:
- Created new "commit" and "hook: commit content" Herald rules which run against "Author's projects" and "Committer's projects".
- Test console'd the "Commit" rules.
- Pushed through the "Hook" rule.
- In all cases, saw fields populate appropriately.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19145
Summary:
See PHI399. Ref T4340. This header provides an additional layer of protection against various attacks, including XSS attacks which embed inline `<script ...>` or `onhover="..."` content into the document.
**style-src**: The "unsafe-inline" directive affects both `style="..."` and `<style>`. We use a lot of `style="..."`, some very legitimately, so we can't realistically get away from this any time soon. We only use one `<style>` (for monospaced font preferences) but can't disable `<style>` without disabling `style="..."`.
**img-src**: We use "data:" URIs to inline small images into CSS, and there's a significant performance benefit from doing this. There doesn't seem to be a way to allow "data" URIs in CSS without allowing them in the document itself.
**script-src** and **frame-src**: For a small number of flows (Recaptcha, Stripe) we embed external javascript, some of which embeds child elements (or additional resources) into the document. We now whitelist these narrowly on the respective pages.
This won't work with Quicksand, so I've blacklisted it for now.
**connect-src**: We need to include `'self'` for AJAX to work, and any websocket URIs.
**Clickjacking**: We now have three layers of protection:
- X-Frame-Options: works in older browsers.
- `frame-ancestors 'none'`: does the same thing.
- Explicit framebust in JX.Stratcom after initialization: works in ancient IE.
We could probably drop the explicit framebust but it wasn't difficult to retain.
**script tags**: We previously used an inline `<script>` tag to start Javelin. I've moved this to `<data data-javelin-init ...>` tags, which seems to work properly.
**`__DEV__`**: We previously used an inline `<script>` tag to set the `__DEV__` mode flag. I tried using the "initialization" tags for this, but they fire too late. I moved it to `<html data-developer-mode="1">`, which seems OK everywhere.
**CSP Scope**: Only the CSP header on the original request appears to matter -- you can't refine the scope by emitting headers on CSS/JS. To reduce confusion, I disabled the headers on those response types. More headers could be disabled, although we're likely already deep in the land of diminishing returns.
**Initialization**: The initialization sequence has changed slightly. Previously, we waited for the <script> in bottom of the document to evaluate. Now, we go fishing for tags when domcontentready fires.
Test Plan:
- Browsed around in Firefox, Safari and Chrome looking for console warnings. Interacted with various Javascript behaviors. Enabled Quicksand.
- Disabled all the framebusting, launched a clickjacking attack, verified that each layer of protection is individually effective.
- Verified that the XHProf iframe in Darkconsole and the PHPAST frame layout work properly.
- Enabled notifications, verified no complaints about connecting to Aphlict.
- Hit `__DEV__` mode warnings based on the new data attribute.
- Tried to do sketchy stuff with `data:` URIs and SVGs. This works but doesn't seem to be able to do anything dangerous.
- Went through the Stripe and Recaptcha workflows.
- Dumped and examined the CSP headers with `curl`, etc.
- Added a raw <script> tag to a page (as though I'd found an XSS attack), verified it was no longer executed.
Maniphest Tasks: T4340
Differential Revision: https://secure.phabricator.com/D19143
Summary:
Depends on D19141. Ref T13088. Some of the fundamental log behaviors like "loading the correct rows" are now a bit better behaved.
The UI is a little less garbage, too.
Test Plan: Viewed some logs and loaded more context by clicking the buttons.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19142
Summary: Depends on D19139. Ref T13088. This doesn't actually work, but is close enough that a skilled attacker might be able to briefly deceive a small child.
Test Plan:
- Viewed some very small logs under very controlled conditions, saw content.
- Larger logs vaguely do something resembling working correctly.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19141
Summary:
Depends on D19138. Ref T13088. When we want to read the last part of a logfile //and show accurate line numbers//, we need to be able to get from byte offsets to line numbers somehow.
Our fundamental unit must remain byte offsets, because a test can emit an arbitrarily long line, and we should accommodate it cleanly if a test emits 2GB of the letter "A".
To support going from byte offsets to line numbers, compute a map with periodic line markers throughout the offsets of the file. From here, we can figure out the line numbers for arbitrary positions in the file with only a constant amount of work.
Test Plan: Added unit tests; ran unit tests.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19139
Summary: Depends on D19137. Ref T13088. This allows `rebuild-log` to skip work if the chunks are already compressed. It also prepares for a future GC which is looking for "text" or "gzip" chunks to throw away in favor of archival into Files; such a GC can use this column to find collectable logs and then write "file" to it, meaning "chunks are gone, this data is only available in Files".
Test Plan: Ran migration, saw logs populate as "text". Ran `rebuild-log`, saw logs rebuild as "gzip".
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19138
Summary: Depends on D19136. Ref T13088. Since it's probably impractical to do all the migrations these changes imply during `bin/storage upgrade`, provide some support for performing them online.
Test Plan: Ran `bin/harbormaster rebuild-log` with `--all`, `--id`, and with and without `--force`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19137
Summary: Depends on D19135. Ref T13088. Denormalize the total log size onto the log itself. This makes reasoning about the log at display time easier, and we don't need to fish around in the database as much to figure out what we're dealing with.
Test Plan: Ran `bin/harbormaster rebuild-log`, saw an existing log populate. Ran `bin/harbormaster write-log`, saw new log write with proper length information.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19136
Summary:
Depends on D19134. Ref T13088. Future changes will support API writers, so push the log lock into the Log object.
Allow open/close ("this process is writing to this log") to be separate from live/final ("this log is still generating more data").
Test Plan: Wrote logs with `bin/harbormater write-log` and updated logs with `bin/harbormaster rebuild-log`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19135
Summary: Depends on D19133. Ref T13088. Allows build logs to be formally destroyed, cleaning up their chunks and file data.
Test Plan:
- Used `bin/remove destroy` to destroy a log, verified chunks and files were removed.
- Used `bin/harbormaster rebuild-log` to force a log to rebuild, verified files were destroyed and regenerated.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19134
Summary: Depends on D19132. Ref T13088. This implements an extremely skeletal dedicated log page with a more-or-less functional "Download Log" button.
Test Plan: Downloaded a recent log. Tried to download an old (un-finalized) log, couldn't. Used `bin/harbormaster write-log` to get a convenient standalone link to a log.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19133
Summary: Depends on D19131. Ref T13088. During log finalization, stream the log into Files to support "Download Log", archive to Files, and API access.
Test Plan: Ran `write-log` and `rebuild-log`, saw Files objects generate with log content and appropriate permissions.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19132
Summary: Depends on D19130. Ref T13088. Currently, when a build log is closed we compress it in the same process. Separate this out into a dedicated worker since the plan is to do a lot more work during finalization, none of which needs to happen inline during builds (or, particuarly, inline during a Conduit call for API writes in the future).
Test Plan: Ran `bin/harbormaster write-log --trace`, saw compression run inline.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19131
Summary: Ref T13088. This is currently minimal but the modify-execute development loop on build logs is extremely long without it.
Test Plan: Ran `echo hi | ./bin/harbormaster write-log --target 12345`, saw the log show up in the web UI.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13088
Differential Revision: https://secure.phabricator.com/D19130
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary: Ref T13090. The doc string in "any()" wasn't specified correctly and the help page wasn't getting enough supporting data to build properly.
Test Plan: Viewed "Reference: Advanced Functions" for a custom datasource field and got more helpful help.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19128
Summary: Depends on D19126. Ref T13090. For datasource custom fields, this proxies the datasource and provides "none()" and "any()" functions to allow you to search for objects with no values or any values.
Test Plan:
- Created a custom "Owning Group" field in Maniphest using a Projects datasource.
- For a task with no owner assigned, searched for "none()" (hit) and "any()" (miss).
- Assigned the task to an owning project.
- Searched for "none()" (miss), "any()" (hit), the project it is now a member of (hit) and some random other project (miss).
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19127
Summary: Ref T13090. Ref T13077. This adds `parentPaths` and `ancestorPaths` constraints to `phriction.document.query`. These should be a little more usable than the internal `slugPrefix` / `depth` stuff -- that's technically more powerful, but requires callers to know more slug normalization rules. We could perhaps expose `minDepth` / `maxDepth` in the future.
Test Plan: Ran valid and invalid `parentPaths` and `ancestorPaths` queries for `/`, `aaa/`, `AAA/`, etc. Got sensible-seeming results.
Maniphest Tasks: T13090, T13077
Differential Revision: https://secure.phabricator.com/D19125
Summary:
Fixes T13087. Ref T13090. An install ran into a situation where mail was being double-delivered, and it wasn't immediately clear where in the pipeline the issue lay.
This change adds some headers which should rule out (or, at least, render very unlikely) some possible causes if we encounter similar issues in the future.
The `X-Phabricator-Mail-ID` header stores the ID of the `MetaMTAMail` storage object so we can distinguish between two messages sent to two different targets and one message which may have been split or re-sent. It also makes it easier to know what to `bin/mail show-outbound --id <id>` and where to find the message in the web UI for additional information.
The `X-Phabricator-Send-Attempt` is a unique value per attempt. If two mail messages are delivered with the same attempt value, the split is probably downstream from Phabricator. If they have different attempt values, the split is probably in Phabricator.
(In this case, the split was somewhere downstream from us, since sending mail with `/usr/bin/mail` also resulted in duplicates.)
Test Plan: Send some mail, inspected it with `bin/mail show-outbound --id <id>`, saw new headers with sensible/expected values.
Maniphest Tasks: T13090, T13087
Differential Revision: https://secure.phabricator.com/D19124
Summary:
See PHI385. Ref T13054. Ref T13083. The dashboard "arrange" operations (add, remove, move) rely on doing `$dashboard->setThing(...)` and then applying transactions.
This no longer works after the read locking change from T13054. To make this function again, just add an explicit `save()` after layout adjustment. This should be more nuanced eventually, but all arrange operations are nonfunctional in a corrupting way at HEAD of `master`/`stable`, so stop the bleeding first.
Test Plan:
- Created new empty and template dashboards.
- Moved panels.
- Added new and existing panels.
- Removed panels.
Maniphest Tasks: T13083, T13054
Differential Revision: https://secure.phabricator.com/D19123
Summary:
Depends on D19121. Ref T13083. Group transactions and show groups in the debugging view.
Fix some of the most obvious issues with fact generation:
- No more 0-point facts.
- Engine can now generate at least one of every type of fact.
Test Plan: Generated facts, viewed them in the debugging view, fact generation largely appeared to align with reality. No more "no facts in storage" facts.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19122
Summary:
Depends on D19120. Ref T13083. When you write a fact engine, it's currently somewhat difficult to figure out exactly what it's doing. It would also be difficult to diagnose bugs or report them to the upstream.
To ease this, add a page which shows all the facts an object generates. This allows you to iterate on an engine quickly without needing to reanalyze facts, take a screenshot, easily compare the timeline to the fact view, etc.
Test Plan: Viewed the object fact page for several objects.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19121
Summary:
Depends on D19119. Ref T13083. This is probably still very buggy, but I'm planning to build support tools to make debugging facts easier shortly.
This generates a large number of datapoints, at least, and can render some charts which aren't all completely broken in an obvious way.
Test Plan: Ran `bin/fact analyze --all`, got some charts with lines that went up and down in the web UI.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19120
Summary:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
