1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-24 22:40:55 +01:00
Commit graph

13887 commits

Author SHA1 Message Date
epriestley
b5f23b023e Add an "--auto" flag to "bin/differential migrate-hunk"
Summary:
Depends on D19370. See T13124. See PHI549. The particular install in PHI549 migrated a large amount of data via the fallback hunk migration script, which does not compress hunks.

Add a mode to `bin/differential migrate-hunk` that amounts to "compress all the hunks which would benefit from compression".

Test Plan: Ran `bin/differential migrate-hunk` with `--auto`, `--all`, `--to`, `--id`, and `--dry-run` in various mixtures. Forced a bunch of hunks to raw ("byte") format, saw it cleanly upgrade them to compressed ("gzde") format.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19371
2018-04-16 12:27:26 -07:00
epriestley
e3de7d09c0 Add an "--all" flag to "bin/differential migrate-hunk"
Summary:
Depends on D19369. Ref T13120. Add a flag to migrate every hunk.

This isn't terribly useful on its own, but I'm going to add an `--auto` flag next so that you can run `--auto --all` to migrate hunks to the preferred hunk format.

Test Plan: Ran `bin/differential migrate-hunk --all --to text`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19370
2018-04-16 12:26:48 -07:00
epriestley
6d1e007076 Try a more conventional spelling of "Convereted"
Summary: This is a good spelling, but maybe a better spelling is possible.

Test Plan: hmmm

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19369
2018-04-16 12:26:29 -07:00
Austin McKinley
0bf0718fad Add isClusterDevice to Almanac query
Summary: Ref T13076. This will be used by the metric collection system to iterate over the cluster devices.

Test Plan: Created some cluster and non-cluster devices, searched and saw expected results.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T13076

Differential Revision: https://secure.phabricator.com/D19368
2018-04-16 10:05:57 -07:00
epriestley
c46be2a70b Allow Maniphest tasks to be queried by workboard Column PHID via SearchEngine
Summary:
Ref T13120. See PHI571. Fixes T5024. This adds a "View as Query" action to workboard columns, which builds a query in Maniphest that has the current query constraints plus an additional constraint to select only tasks in the specified column.

This is a normal query and can be turned into a dashboard panel, added to a menu, edited, saved as a link, etc.

Much of the complexity here is that finding tasks in a given column isn't entirely straightforward because of how board layout works: when you create a task, it isn't immediately placed in columns. It's only actually added to the "Backlog" column on any boards when someone looks at the board.

To get the right behavior, we must do "board layout" for any queried columns before we can constrain results. This isn't enormously efficient, but should be OK for reasonable boards.

Test Plan:
  - Used "View as Query" for normal columns and milestome columns, got appropriate queries in Maniphest.
  - Applied filters to the board (e.g., "Priorities: wishlist"), then used "View As Query" and had my custom filters respected.
  - Queried some large boards/columns with more than a thousand tasks, got results back within a second or so.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T5024

Differential Revision: https://secure.phabricator.com/D19366
2018-04-13 16:07:44 -07:00
epriestley
ca49fffc1b Fix the legacy "25, 50, 100, unlimited" Harbormaster log links to respect generation selection
Summary:
See PHI565. Ref T13120. Although this older log is on the chopping block (see T13088), there's some migration guidance and other complexity around just replacing it.

Until it gets replaced, make clicking the "number of lines" elements respect the current "Build Generation" setting. Prior to this change, clicking the links would lose the generation information and jump you to the most recent build generation.

Also fix some collateral damage from T13105 where we ended up with white text on a white background in some cases.

Test Plan:
  - Restarted a build to get multiple generations.
  - On each generation, clicked the various "25", "50", etc., links.
  - Saw generation and log window sizes both respected by the links.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19367
2018-04-13 11:55:44 -07:00
epriestley
6556536d06 Allow repository cluster bindings to be marked as not "writable", making them read-only
Summary:
Depends on D19356. Fixes T10883. Ref T13120.

  - Add a "writable" property to the bindings, defaulting to "true" with a nice dropdown.
  - When selecting hosts, allow callers to request a writable host.
  - If the caller wants a writable host, only return hosts if they're writable.
  - In SVN and Mercurial, we sometimes return only writable hosts when we //could// return read-only hosts, but figuring out if these request are read-only or read-write is currently tricky. Since these repositories can't really cluster yet, this shouldn't matter too much today.

Test Plan:
  - Without any config changes, viewed repositories via web UI and pushed/pulled via SSH and HTTP.
  - Made all nodes in the cluster read-only by disabling "writable", pulled and hit the web UI (worked), tried to push via SSH and HTTP (got errors about read-only).
  - Put everything back, pulled and pushed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T10883

Differential Revision: https://secure.phabricator.com/D19357
2018-04-12 16:10:36 -07:00
epriestley
7c7e6d555b Give getAlmanacServiceURI() an "options" parameter to prepare for read-only devices
Summary:
Depends on D19355. Ref T10883. Ref T13120. Rather than adding a million parameters here, wrap the selector-parameters in an `$options`.

The next change adds a new "writable" option to support forcing selection of writable hosts.

Test Plan: Pulled and pushed via HTTP and SSH, viewed repositories via Diffusion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T10883

Differential Revision: https://secure.phabricator.com/D19356
2018-04-12 16:10:12 -07:00
epriestley
6f810d7813 Turn the "closed" property on cluster repositories into a nice boolean
Summary:
Ref T10883. Ref T13120. There's an existing "closed" property on repository services that stops new repositories from being allocated there.

Turn it into a nice boolean.

Test Plan: Toggled the value on/off using a nice `<select />` with helpful labels instead of a text area.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T10883

Differential Revision: https://secure.phabricator.com/D19355
2018-04-12 16:09:32 -07:00
epriestley
4068aaef61 Toggle revision "shouldBroadcast" correctly when "--draft" is used with prototypes off
Summary:
See PHI573. Ref T13120. Drafts were recently changed so that "draft" and "broadcast" are separate flags, and you can have non-broadcasting revisions in states other than "draft" if builds fail on a draft or you abandon a draft.

However, when draft mode is entered with `arc diff --draft` and you have prototypes off, this flag wasn't being set correctly.

Test Plan: Disabled prototypes, created a revision with `arc diff --draft`, observed that `draft.broadcast` is now correctly `false`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19360
2018-04-12 16:08:43 -07:00
epriestley
c52e10d1ec Respect external unmentionable PHIDs in Differential revision editor
Summary:
See PHI574. Ref T13120. When you `Ref Txx` or `Fixes Txxx`, we mark it "unmentionable" to prevent the task from generating both a reference and a mention.

If you add a reference to an object (like a commit hash) to a custom remarkup field, there's currently no real way to prevent it from generating a mention, except that you can explicitly mark the PHID as unmentionable on the Editor.

This isn't exactly a first-class feature, but we technically do it in `PhabricatorRepositoryCommitMessageParserWorker`, and it probably doesn't hurt or interfere with anything to support it slightly better.

In Differential, respect any existing value and append new values to it rather than overwriting the value.

Test Plan: Edited a revision summary to include `Ref Txxx`, saw only a reference (not a mention) generate.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120

Differential Revision: https://secure.phabricator.com/D19361
2018-04-12 16:07:55 -07:00
epriestley
70056a9072 When creating a file by downloading a URI, truncate the length of the default name
Summary:
See <https://discourse.phabricator-community.org/t/embedding-external-images-url-show-error-for-long-urls/1339>.

When we download a file from a URI, we provide a default name based on the URI. However, if the URI is something like `http://example.com/very-very-very-....-long.jpg` with more than 255 characters, we may suggest a name which won't fit into the `name` column of `PhabricatorFile`.

Instead, suggest a default name no longer than 64 bytes.

Test Plan:
  - Used the `{image ...}` example from the Discourse report locally; got an image with a truncated name.
  - Used a normal `{image ...}`, got an image file with a normal name.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19353
2018-04-12 13:29:53 -07:00
epriestley
37a03402bc When following a link to a particular line ("/example.txt$12"), scroll to that line
Summary:
Depends on D19349. Ref T13105. This was the behavior in Diffusion before with a little hard-coded snippet.

Remove that snippet ("diffusion-jump-to") and add a more general-purpose snippet to SourceView.

This is a tiny bit hacky still (and probably doesn't work quite right with Quicksand) but gets things working again and works in all of Files, Paste, and Diffusion.

Test Plan: Followed links to particular lines in Paste, Files and Diffusion; got scrolled to the right place.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19350
2018-04-11 17:29:22 -07:00
epriestley
5b3a351852 Use pseudoelements, not Zero Width Space, to implement copy/paste behavior in Paste/Diffusion
Summary:
Depends on D19348. Ref T13105. When copying text from Paste or Diffusion, we'd like to copy only source, not line numbers.

We currently accomplish this with zero-width spaces plus a trigger that fires on "copy" in Paste and Diffusion. This is quite gross.

In the new-style Harbormaster logs, we use an approach that seems slightly better: CSS psuedoelements.

This isn't a complete solution (see also PHI504 / T5032) but puts us in a slightly better place.

Use it in Paste/Files/Diffusion too.

This gives us good behavior in all browsers in Files and Paste.

This gives us good behavior in Chrome and Firefox in Diffusion. Safari will copy (but not visually select) blame information in Diffusion. I think we can live with that for now.

Test Plan: Selected and copy/pasted stuff in Diffusion, Files, and Paste. Got good behavior everywhere except Safari + Diffusion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19349
2018-04-11 17:28:46 -07:00
epriestley
ea9187ea92 Allow Almanac properties to be set and deleted via Conduit
Summary:
Depends on D19342. Ref T12414. Ref T13120. This adds an EditEngine extension for editing Almanac properties.

The actual wire format is a little weird. Normally, we'd have a transaction for each property, but since you can pick any property names you want we can't really do that (we'd have to generate infinite transactions).

The transaction wire format anticipates that transactions may eventually get some kind of metadata -- each transaction looks like this:

```
{
  "type": "title",
  "value": "Example title"
}
```

...and we can add more keys there. For example, I could have made this transaction look like this:

```
{
  "type": "property.set",
  "almanac.property.key": "some-key",
  "value": "some-value"
}
```

However, I don't want to just accept any possible key freely, and it might be a decent chunk of work to formalize this better. It also doesn't feel great.

I just built special transaction types intead, so you:

```
{
  "type": "property.set",
  "value": {
   "some-key": "some-value",
   ...
  }
}
```

Internally, we may generate more than one transaction as a result (if the "value" has more than one key).

This feels a bit more natural and is probably easier for clients to use anyway.

Test Plan: Set and deleted Service, Device and Binding properties via the API.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19343
2018-04-11 10:42:10 -07:00
epriestley
c1558031c2 Make various small quality-of-life improvements for Almanac properties
Summary:
Depends on D19341. Ref T12414. Ref T13120.

  - Fix a bug where default-valued properties didn't get rendered in grey as they're supposed to (as a hint that the value isn't customized).
  - When resetting a builtin property won't do anything, visually disable the button as a hint.
  - Allow Services to specify properties on their Bindings.
  - Specify that repository bindings have a "protocol" property, so it becomes an explicit thing in the UI. Previously, you had to read the documentation to figure this out.
  - When editing bindings, use the EditField and its configuration if possible. This turns the "Protocol" property into a dropdown in the UI where you select between "http", "https" and "ssh".
  - Give the "protocol" binding a smart default based on the port number of the corresponding interface.

Test Plan:
  - Viewed properties on Services, Devices and Bindings.
  - Saw them render sensibly, and grey out + grey button when a builtin value has a default setting.
  - Saw "Protocol" appear as a default property on repository cluster bindings and get a smart value.
  - Edited "protocol", got a nice dropdown.

{F5518791}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19342
2018-04-11 10:38:41 -07:00
epriestley
d56a37b636 Allow Almanac Bindings to be enabled/disabled via API and support the "properties" attachment
Summary:
Depends on D19340. Ref T12414. Ref T13120. See T12414 for some discussion about direction here.

Since I think retaining "enabled/disabled" as a simple flag is reasonable, expose it via the API for readers and writers.

Also expose binding properties.

Test Plan:
  - Searched for bindings and properties with "alamanc.binding.search".
  - Enabled and disabled bindings with "almanac.binding.edit".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19341
2018-04-11 10:38:09 -07:00
epriestley
208504a5e3 Provide "almanac.binding.search" and "almanac.binding.edit"
Summary:
Depends on D19338. Ref T13120. Ref T12414. These are the last of the new API methods.

This stuff still doesn't work:

  - You can't actually enable/disable bindings yet. I want to take a look at the use cases and consider changing "disabled" to "status", or providing a different way to solve the problem.
  - You can't edit properties via the API. I expect to enable this for all `AlmanacPropertyInterface` objects with an extension in a future change.

Test Plan:
  - Searched for bindings via API.
  - Viewed binding web UI for API methods.
  - Created bindings via API.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19340
2018-04-11 10:37:38 -07:00
epriestley
e502df509d Implement "almanac.interface.search" and "almanac.interface.edit"
Summary: Depends on D19337. Ref T13120. Ref T12414. These are slightly more substantive than namespace/network, but pretty much standard fare.

Test Plan:
  - Searched for interfaces with "almanac.interface.search".
  - Created and edited interfaces with "almanac.interface.edit".
  - Created and edited interfaces with web UI since some stuff got tweaked.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19338
2018-04-11 10:35:03 -07:00
epriestley
10947c8684 Add "almanac.namespace.edit" and "almanac.namespace.search" API methods
Summary: Depends on D19336. Ref T13120. Ref T12414. These are simple, straightforward, and uninteresting.

Test Plan:
  - Searched for namespaces with "almanac.namespace.search".
  - Created and edited namespaces with "almanac.namespace.edit".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19337
2018-04-11 10:34:30 -07:00
epriestley
9022e14082 Use a more conventional spelling of "Almanac" for "almanac.service.edit" class
Summary: Depends on D19335. Ref T13120. Ref T12414. There are many good ways to spell "almanac", but stick with convention here.

Test Plan: (O_O)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19336
2018-04-11 10:34:04 -07:00
epriestley
a8c4da13c0 Add "almanac.network.edit" and "almanac.network.search" API methods
Summary: Depends on D19334. Ref T13120. Ref T12414. These are pretty straightforward, but no one really has a use case for them anyway today so they're primarily just for completeness.

Test Plan:
  - Queried networks with `almanac.network.search`.
  - Created and edited networks with `almanac.network.edit`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19335
2018-04-11 10:33:41 -07:00
epriestley
4bce3fc8e6 Modularize Almanac property transactions
Summary:
Depends on D19329. Ref T13120. Ref T12414. Recent changes have mostly modularized Almanac transactions, but the "property" transactions remained written in an older style with the logic on the Editor/Transaction classes.

This moves them to modern modular transactions. These end up being a little bit copy-pastey, but it doesn't feel too terribly bad.

Test Plan: Created, edited, and deleted properties on services, devices and bindings. Grepped for removed constants.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19334
2018-04-11 10:33:18 -07:00
epriestley
71c77fcc3a Modularize transactions for Almanac Device
Summary:
Depends on D19328. Ref T13120. Ref T12414.

Prior work has left us with just a NAME transaction here, which is straightforward to modularize.

Test Plan:
  - Created and renamed devices.
  - Tried to set no name, a bad name, a duplicate name (got errors).
  - Tried to create/rename into a namespace I could not edit (got an error).
  - Grepped for `AlmanacDeviceTransaction::`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19329
2018-04-11 10:31:46 -07:00
epriestley
4e156a0385 Remove TYPE_INTERFACE transaction from Almanac Device
Summary:
Depends on D19325. Ref T13120. Ref T12414.

This no longer has any callers in the upstream or in Phacility support libraries, so get rid of it.

This will make modularizing Device transactions significantly easier, since the other transactions are reasonable, normal sorts of transactions.

For existing devices, this leaves some "author edited this object." transactions in the log. I might just leave those since they aren't really hurting anything, or maybe I'll clean them up or hide them later once I have more confidence that these changes are stable.

Test Plan: Grepped for `TYPE_INTERFACE` and `AlmanacDeviceTransaction`, found no callsites.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19328
2018-04-11 10:31:25 -07:00
epriestley
d240969e47 Use Interface transactions, not Device transactions, to destroy Interfaces
Summary:
Depends on D19324. Ref T13120. Ref T12414.

This moves "Destroy Interface" to use Interface transactions instead of Device transactions, so we can ultimately get rid of the complex and difficult-to-modernize `AlmanacDeviceTransaction::TYPE_INTERFACE`.

This transaction is a bit weird since it makes the interface delete itself, but this should work OK for now. At some point in the future I'd probably want to change this into more of a "disable" action, but I don't think we face any immediate peril by retaining this behavior for now.

Test Plan:
  - Destroyed interfaces on devices using the web UI, saw them vanish.
  - Ran daemons, nothing fataled/exploded even though the transaction is weird and destroys the object it affects.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19325
2018-04-11 10:30:15 -07:00
epriestley
6ccf35f9a2 Edit Interfaces in Almanac with EditEngine
Summary:
Depends on D19323. Ref T13120. Ref T12414.

Move editing to modern stuff and fix some implementation errors from D19323 (mostly copy/paste stuff).

Test Plan:
  - Created and edited interfaces.
  - Tried to create/edit an interface with a bogus/empty address/port, got errors.
  - Tried to create an interface on a bogus device, got an error.
  - Tried to create an interface on a device I could not edit, got an error.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19324
2018-04-11 10:29:50 -07:00
epriestley
f9c6a69d9c Add skeleton code for Almanac Interfaces to have real transactions
Summary:
Depends on D19322. Ref T13120. Ref T12414.

Currently, `AlmanacDevice` has a bit of a beast of a `TYPE_INTERFACE` transaction that fully creates a complex Interface object. This isn't very flexible or consistent, and Interfaces are complex enough to reasonably have their own object behaviors (for example, they have their own PHIDs).

The complexity of this transaction makes modularizing `AlmanacDevice` transactions tricky. To simplify this, move Interface toward having its own set of normal transactions.

This change just adds some reasonable-looking transactions; it doesn't actually hook them up in the UI or make them reachable. I'll test that they actually work as I swap the UI over.

We may also have some code using the `TYPE_INTERFACE` transaction in Phacility support stuff, so that may need to wait a week to actually phase out.

Test Plan: Ran `bin/storage upgrade` and `arc liberate`. This code isn't reachable yet.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19323
2018-04-11 10:29:26 -07:00
epriestley
580409b562 Modularize Almanac Network transactions
Summary: Depends on D19321. Ref T13120. Ref T12414. Move transactions for Almanac Networks (just "name") to ModularTransactions.

Test Plan:
  - Created a new network.
  - Renamed a network.
  - Tried to create a network with no name (got an error).
  - Grepped for `AlmanacNetworkTransaction::`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19322
2018-04-11 10:29:05 -07:00
epriestley
f62494355d Modularize Almanac Binding transactions
Summary: Depends on D19320. Ref T13120. Ref T12414. Move transactions for Almanac Bindings to ModularTransactions.

Test Plan:
  - Created a new binding.
  - Tried to create a duplicate binding, got an error.
  - Edited a binding to rebind it to a different device.
  - Disabled and enabled bindings.
  - Grepped for `AlmanacBindingTransaction::` constants.

When a binding is created, it currently renders a bad "changed the interface from ??? to X" transaction. This is because creation isn't currently using EditEngine. I plan to swap it shortly, which will turn this into a real "Create" transaction and fix the issue.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19321
2018-04-11 10:28:42 -07:00
epriestley
5ada1211cd Modularize Almanac Namespace transactions
Summary: Depends on D19318. Ref T13120. Ref T12414. Move transactions for Almanac Namespaces ("name" is the only meaningful one) to ModularTransactions.

Test Plan:
  - Created a new namespace.
  - Edited a namespace.
  - Tried to choose no name, an invalid name, a duplicate name, and a name in a namespace I can't edit; got appropriate errors.
  - Grepped for `AlmanacNamespaceTransaction::TYPE_NAME`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19320
2018-04-11 10:24:10 -07:00
epriestley
6983479e4f Allow "almanac.service.edit" to create services
Summary:
Depends on D19317. Ref T13120. Ref T12414. See PHI145. See PHI473.

This adds a Conduit-only "type" transaction for Almanac services. This is very similar to the approach in D18849 for Drydock blueprints.

Test Plan:
  - Tried to create an empty service via "almanac.service.edit", was told to pick a type.
  - Tried to pick a bad type, was told to pick a good type.
  - Created a new Almanac service via "almanac.service.edit".
  - Tried to edit the service to change the type, wasn't allowed to.
  - Created and edited via the web UI, nothing changed from before.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19318
2018-04-11 10:23:50 -07:00
epriestley
c428f60a97 Partially modularize AlmanacService transactions
Summary:
Ref T13120. Ref T12414. See PHI145. See PHI473. This partially modernizes AlmanacService transactions by moving them to ModularTransactions.

This isn't complete because the "update property" and "remove property" transactions aren't modularized. They still //work//, since the parent Editor implements them, but they no longer render properly on the timeline since the `Transaction` object no longer has rendering logic for them.

Tentatively, I'm going to try to convert the rest of the Almanac objects and then modularize those transactions. (Currently, all of Binding, Device, Namespace and Service support properties, although they can only actually be edited on Service, Device and Binding.)

If that turns out to be really tricky for some reason I can just copy/paste the timeline rendering for now, but I think it won't be too hard.

Test Plan:
  - Created and edited Services.
  - Tried to create a service with: a bad name, no name, a name which put it in a namespace I can't edit (got errors in all cases).
  - Edited and removed properties. The edits worked, the timeline just renders a generic story now ('X edited this object (transaction type "almanac:property:update").').

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19317
2018-04-11 10:22:34 -07:00
Austin McKinley
0755482bf0 Add transactions for installing/uninstalling applications
Summary: Fixes T11476.

Test Plan:
 - Installed/uninstalled the Conpherence application
 - Observed correct timeline stories
 - Observed correct config in database
 - Observed 404 for application page

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T11476

Differential Revision: https://secure.phabricator.com/D19339
2018-04-11 08:54:55 -07:00
Austin McKinley
d398bcd67c Fix argument ordering in error message
Summary:
Before:
```
$ ./config set phabricator.base-uri local.phacility.com:8080
Usage Exception: Config option 'http://' is invalid. The URI must start with https://' or 'phabricator.base-uri'.
```
After:
```
$ ./config set phabricator.base-uri local.phacility.com:8080
Usage Exception: Config option 'phabricator.base-uri' is invalid. The URI must start with http://' or 'https://'.
```

Test Plan: See above

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19330
2018-04-10 10:18:51 -07:00
epriestley
1680211702 Remove dead "Service Lock" code from Almanac
Summary:
Depends on D19315. Ref T13120. Ref T12414. See PHI145. See PHI473. I want to move Almanac services to ModularTransactions but ran into this old piece of dead/unused code along the way.

Long ago, Almanac services could be individually "locked", but this didn't really work out very well. It was replaced by "Can Manage Cluster Services" in D15339 and prior changes, but not all of the old "Lock" code got cleaned up.

I don't expect to restore this feature, so clean it up now.

Test Plan:
  - Grepped for `AlmanacServiceTransaction::TYPE_LOCK`, `TYPE_LOCK`, etc.
  - Grepped for `updateServiceLock()`, no callsites.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T12414

Differential Revision: https://secure.phabricator.com/D19316
2018-04-09 11:38:04 -07:00
epriestley
72ab8640c5 Narrowly fix web UI fatal for "almanac.service.edit" Conduit API method
Summary:
See T13120. See T12414. See PHI145. See PHI473. Almanac services require a type before they can do anything, and EditEngine currently builds one with no type. We then fatal when trying to do mundane things like generate documentation.

Instead, build a generic but complete Service for documentation generation in the web UI. This is similar to the previous Drydock Blueprint change from D18849 (or some earlier diff in that series).

(You still probably can't use this method to //create// a service; I'll fix that in the next change.)

Test Plan:
  - Viewed "almanac.service.edit" in the web UI.
    - Before: immediate fatal ("No Almanac service type "" exists!").
    - After: Page works. No claims about the method doing anything useful.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19315
2018-04-09 11:37:39 -07:00
epriestley
472bc3d90a Colorize lines in blame under DocumentEngine, to show relative age of changes
Summary:
Depends on D19313. Ref T13105. Fixes T13015. We lost the coloration for ages in the switch to Document Engine.

Restore it, and use a wider range of colors to make the information more clear.

Test Plan: Viewed some blame, saw a nice explosion of bright colors. This is a cornerstone of good design.

Maniphest Tasks: T13105, T13015

Differential Revision: https://secure.phabricator.com/D19314
2018-04-09 06:11:47 -07:00
epriestley
cf75d63b49 When lines 12, 13, 14, etc all blame to the same change, only show it once
Summary:
Depends on D19312. Ref T13105. For readability, render only one link for each contiguous block of changes.

Also make the actual rendering logic a little more defensible.

Test Plan: Viewed some files with blame, saw one render per chunk instead of one per line.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19313
2018-04-09 06:11:06 -07:00
epriestley
eca7dc25f2 Use javelin_tag(), not phutil_tag(), to render revision blame tooltips properly
Summary: Depends on D19310. Ref T13105. The "meta" value was not populating correctly because this used `phutil_tag()`.

Test Plan: Will verify on `secure`.

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19311
2018-04-09 06:10:09 -07:00
epriestley
09c6d42b95 Mostly make blame work with DocumentEngine
Summary: Ref T13105. This needs refinement but blame sort of works again, now.

Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.

Reviewers: mydeveloperday

Reviewed By: mydeveloperday

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19309
2018-04-09 04:48:21 -07:00
epriestley
90a614778c Make repository symbol references work with DocumentEngine
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
2018-04-09 04:47:28 -07:00
epriestley
0363febeb2 Disable default syntax highlighting for large files in DocumentEngine
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
2018-04-09 04:47:08 -07:00
epriestley
6dea2ba3b3 Fix DocumentEngine line behaviors in Diffusion
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
2018-04-09 04:46:47 -07:00
epriestley
1fde4a9450 Move Diffusion browse rendering to DocumentEngine, breaking almost all features
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
2018-04-09 04:46:26 -07:00
epriestley
245132a0b2 Pull file Document Engine rendering out of "Files" application controllers
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
2018-04-09 04:45:58 -07:00
epriestley
7d4e25614d Remove the ability to disable blame in Diffusion
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
2018-04-09 04:45:16 -07:00
epriestley
9bb338c038 Revert the alternate menu names for applications
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
2018-04-08 10:20:24 -07:00
epriestley
af87f414e8 Stop the debugging view for typeahead datasources from fataling
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
2018-04-08 06:16:56 -07:00
epriestley
f01c2e3694 Remove "Large Changes" documentation and make some minor behavioral improvements
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
2018-04-05 06:40:46 -07:00
epriestley
1b363a831e When a revision changes more than 1,000 files, don't show the changes on the main page
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
2018-04-05 06:40:22 -07:00
epriestley
8c8e7f07b5 Add a standalone view for browsing changesets of very large revisions
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
2018-04-05 06:35:06 -07:00
epriestley
3de002c841 Fix a commit hook issue where pushing dangerous changes would fatal before hitting the dragon bureaucrats
Summary: See <https://discourse.phabricator-community.org/t/php-fatal-when-using-git-push-d/1317>. The behavioral changes for Herald on initial import from D19265 could leave `$all_updates` undefined if we throw early enough.

Test Plan: Pushed a dangerous change, saw dragon bureaucrats again.

Differential Revision: https://secure.phabricator.com/D19297
2018-04-05 06:19:49 -07:00
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
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
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
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
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
epriestley
c5e4bd8187 Fix some minor errors (DarkConsole warning, unstable Ferret sort)
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
2018-03-18 15:12:25 -07:00
epriestley
7e43b74055 Give all commands from DiffusionCommandEngine a default 15 minute timeout
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
2018-03-16 17:22:03 -07:00