1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-11 08:06:13 +01:00
Commit graph

16287 commits

Author SHA1 Message Date
epriestley
e70c9f72a4 Show revision sizes using a perplexing, inexplicable symbol code
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
2018-04-03 12:49:27 -07:00
epriestley
e40aec0210 When a revision has more than 7 reviewers, render only the first 7 in the list view
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
2018-04-03 12:47:43 -07:00
epriestley
592d72e006 Move PhabricatorModularTransaction slightly closer to having "final" methods again
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
2018-04-03 11:13:58 -07:00
epriestley
6f520e0534 Clean up an old transaction state flag
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
2018-04-03 11:13:31 -07:00
epriestley
804f9817c3 When a draft's builds fail and it demotes to "Changes Planned + Draft", notify the author (only) via email
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
2018-04-03 11:11:28 -07:00
epriestley
f4f3311312 When reclaiming an "Abandoned + Draft" revision, return it to "Draft", not "Needs Review"
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
2018-04-03 11:11:06 -07:00
epriestley
adf8fdef0e When remote builds fail, demote revisions to "Changes Planned + But, Still A Draft"
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
2018-04-03 11:10:45 -07:00
epriestley
d9bd36039f When a non-broadcasting revision is updated, put it in "Draft", not "Needs Review"
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
2018-04-03 11:10:13 -07:00
epriestley
615d27c8e9 Show an additional "Draft" tag on non-broadcasting revisions in a non-draft state
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
2018-04-03 11:09:49 -07:00
epriestley
38e788c99a Partially decouple revision broadcasting from revision draft state
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
2018-04-03 11:09:26 -07:00
epriestley
3b5a7d1c88 Rename the Differential "hasBroadcast" flag to "shouldBroadcast"
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
2018-04-03 11:09:02 -07:00
epriestley
f350b9e464 Explicitly condition Differential draft promotion on only "impactful" builds
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
2018-04-03 11:06:46 -07:00
epriestley
51461f18c1 When publishing buildables in Differential, ignore autobuilds (local lint and unit)
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
2018-04-03 11:02:12 -07:00
epriestley
ada0c9126c Provide a modular buildable transaction in Diffusion
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
2018-04-03 11:01:37 -07:00
epriestley
c20b4e365b Move structural build publishing logic to BuildEngine, provide "bin/harbormaster publish"
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
2018-04-03 10:58:27 -07:00
epriestley
95c9d403f4 Make objects implementing BuildableInterface produce a BuildableEngine
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
2018-04-03 10:57:51 -07:00
epriestley
651f91cd54 Fix YouTube remarkup rule being blocked by Content-Security-Policy header
Summary: Ref T13116. See PHI526. Currently, the YouTube remarkup rule writes an `<iframe ...>` but does not adjust the Content-Security-Policy appropriately.

Test Plan: Pasted a YouTube link; viewed it in Safari, Chrome and Firefox.

Maniphest Tasks: T13116

Differential Revision: https://secure.phabricator.com/D19277
2018-04-02 16:11:02 -07:00
epriestley
7189cb7ba8 Support text encoding and syntax highlighting options in document rendering
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
2018-03-30 11:28:52 -07:00
epriestley
ccbc8a430f Make Jupyter notebooks use the fast builtin Python highlighter
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
2018-03-30 11:26:48 -07:00
epriestley
7eaa27683e Make closed/disabled results in the remarkup autocomplete more visually clear
Summary:
Ref T13114. See PHI522. Although it looks like results are already ordered correctly, the override rendering isn't accommodating disabled results gracefully.

Give closed results a distinctive look (grey + strikethru) so it's clear when you're autocompleting `@mention...` into a disabled user.

Test Plan: {F5497621}

Maniphest Tasks: T13114

Differential Revision: https://secure.phabricator.com/D19272
2018-03-30 08:47:00 -07:00
epriestley
66392e5b8b Add a rough "bin/repository unpublish" workflow to attempt to cleanup improperly published repositories
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
2018-03-30 08:46:11 -07:00
epriestley
9fbf4ee58c Provide comment actions for tokenizer custom fields
Summary:
Ref T13114. See PHI519. An install is interested in modifying a tokenizer custom field from the comment area. Provide this capability.

This patch is fairly narrow but should solve the immediate need.

Test Plan: Added, removed, and modified a tokenizer custom field using the comment action dropdown.

Maniphest Tasks: T13114

Differential Revision: https://secure.phabricator.com/D19270
2018-03-29 14:10:15 -07:00
epriestley
7f9a9bc800 Make Harbormaster objects destructible
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
2018-03-29 13:01:14 -07:00
epriestley
7915445543 Fix two issues with Differential updates and Owners
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
2018-03-29 11:32:23 -07:00
epriestley
93cb6e3bde Make updating a revision with the same active diff a no-op
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
2018-03-29 09:59:39 -07:00
epriestley
74216ea8e0 Disable Herald and enormous change protection for repository initial imports
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
2018-03-29 08:05:07 -07:00
epriestley
5cb6832572 Fix usage of fprintf() in bin/drydock command
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
2018-03-28 16:19:55 -07:00
epriestley
b586ee065a Stop evaluating Herald rules when writing "someone mentioned this somewhere else." transactions
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
2018-03-28 15:35:34 -07:00
epriestley
c5b244bfd0 Render directly embedded image data represented as a string in Jupyter notebooks
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
2018-03-28 15:08:44 -07:00
epriestley
b7d3101e7c Minor document rendering fixes: dropdown for synchronous files, URI normalization for default renderers
Summary:
Depends on D19258. Ref T13105.

  - When the default renderer is an Ajax renderer, don't replace the URI. For example, when viewing a Jupyter notebook, the URI should remain `/F123`, not instantly change to `/view/123/jupyter/`.
  - Fix an issue where non-ajax renderers could fail to display the dropdown menu properly.

Test Plan:
  - Viewed a Jupyter notebook, stayed on the same URI.
  - Changed rendering, got different URIs.
  - Viewed a JSON file and toggled renderers via dropdown.

Reviewers: mydeveloperday

Reviewed By: mydeveloperday

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19259
2018-03-28 15:07:21 -07:00
epriestley
38999e25ac Support logged-out access to the document rendering endpoint
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
2018-03-28 15:01:36 -07:00
epriestley
f583406ba9 Drop uniqueness constraint on PushEvent request ID
Summary: See <https://discourse.phabricator-community.org/t/pushing-to-mercurial-repository-fails/1275/1>. Mercurial may invoke hooks multiple times per push.

Test Plan: Pushed to Mercurial, saw key constraint failure.

Differential Revision: https://secure.phabricator.com/D19257
2018-03-26 07:02:15 -07:00
epriestley
bba1b185f8 Improve minor client behaviors for document rendering
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
2018-03-23 14:09:31 -07:00
epriestley
4906364751 Add a JSON document rendering engine
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
2018-03-23 12:29:05 -07:00
epriestley
d2727d24da Add an abstract "Text" document engine and a "Source" document engine
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
2018-03-23 12:28:43 -07:00
epriestley
cbf3d3c371 Add a very rough, proof-of-concept Jupyter notebook document engine
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
2018-03-23 07:14:45 -07:00
epriestley
fb4ce851c4 Add a PDF document "rendering" engine
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
2018-03-23 07:14:17 -07:00
epriestley
8b658706a8 Add a basic Remarkup document rendering engine
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
2018-03-23 07:07:50 -07:00
epriestley
df3c937dab Record lock timing information on PushEvents
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
2018-03-22 13:46:01 -07:00
epriestley
69bff489d4 Generate a random unique "Request ID" for SSH requests so processes can coordinate better
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
2018-03-22 13:44:30 -07:00
epriestley
e010aaca43 accidentally a word
Summary: Sometimes I dream I am a small turtle.

Test Plan: squeak squeak

Differential Revision: https://secure.phabricator.com/D19248
2018-03-22 13:43:10 -07:00
epriestley
859b274970 Provide more information to users during git push while waiting for write locks
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
2018-03-22 13:42:18 -07:00
epriestley
c8583b016d When workflow dialog buttons are clicked, disable the button
Summary:
Depends on D19245. Fixes T11145. Ref T13108. See PHI488. Disable workflow buttons when they're clicked to prevent accidental client-side double submission.

This might have some weird side effects but we should normally never need to re-use a workflow dialog form so it's not immediately obvious that this can break anything.

Test Plan:
  - Added `sleep(1)` to the Mute controller and the Maniphest task controller.
  - Added `phlog(...)` to the Mute controller.
  - Opened the mute dialog, mashed the button a thousand times.
    - Before: Saw a bunch of logs.
    - After: Button immediately disables, saw only one log.

Maniphest Tasks: T13108, T11145

Differential Revision: https://secure.phabricator.com/D19246
2018-03-21 11:58:13 -07:00
epriestley
9e278a89ba If a Workflow form receives a redirect response, don't re-enable the submit buttons
Summary:
See PHI488. Ref T13108. Currently, there is a narrow window between when the response returns and when the browser actually follows the redirect where the form is live and you can click the button again.

This is relativey easy if Phabricator is running //too fast// since the button may be disabled only momentarily. This seems to be easier in Firefox/Chrome than Safari.

Test Plan:
  - In Firefox and Chrome, spam-clicked a comment submit button.
    - Before: could sometimes get a double-submit.
    - After: couldn't get a double-submit.
    - This could probably be reproduced more reliabily by adding a `sleep(1)` to whatever we're redirecting //to//.
  - Submitted an empty comment, got a dialog plus a still-enabled form (so this doesn't break the non-redirect case).

Maniphest Tasks: T13108

Differential Revision: https://secure.phabricator.com/D19245
2018-03-21 11:56:21 -07:00
epriestley
6ed123e080 Propagate "unexpandable" PHIDs to feed notification recipient expansion
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
2018-03-21 11:55:52 -07:00
Tino Breddin
73b68bc2a6 Fix a possible count(null) in DifferentialRevisionActionTransaction
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
2018-03-21 07:39:34 -07:00
Timo Tijhof
3bf8d5682e Fix outdated link for Font Awesome icon set
Summary:
The current link has a redirect for a while now, from
http://fortawesome.github.io/Font-Awesome/ to https://fontawesome.com

However, since the release of Version 5, the docs no longer
match the icons that are valid for use in Phabricator, which
uses Version 4.

Update the reference to link to the same logical content as before.

Test Plan: The content now lives at <https://fontawesome.com/v4.7.0/icons/>.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D19241
2018-03-20 15:50:52 -07:00
epriestley
4aafce6862 Add filesize limits for document rendering engines and support partial/complete rendering
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
2018-03-19 15:18:34 -07:00
epriestley
f646153f4d Add an async driver for document rendering and a crude "Hexdump" document engine
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
2018-03-19 15:18:05 -07:00
epriestley
01f22a8d06 Roughly modularize document rendering in Files
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
2018-03-19 15:17:04 -07:00