1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-05 21:26:14 +01:00
Commit graph

16527 commits

Author SHA1 Message Date
epriestley
3337b84073 (stable) Promote 2019 Week 20 2019-05-16 13:57:15 -07:00
epriestley
6bff2cee22 Improve the performance of tab replacement in common cases
Summary:
See PHI1210. For certain large inputs, we spend more time than we need to replacing tabs with spaces. Add some fast paths:

  - When a line only has tabs at the beginning of the line, we don't need to do as much work parsing the rest of the line.
  - When a line has no unicode characters, we don't need to vectorize it to get the right result.

Test Plan:
  - Added test coverage.
  - Profiled this, got a ~60x performance increase on a 36,000 line 3MB text file.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20477
2019-05-16 12:28:39 -07:00
epriestley
c5ecc388a2 Make branch status more clear on Diffusion branches view
Summary:
See PHI1225. Ref T13277. In Diffusion, show "default", "permanent", or "not permanent" when looking at branches.

For repositories with 100 or fewer branches, put default and permanent branches on top.

Test Plan: {F6426814}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: leoluk

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20493
2019-05-16 12:24:39 -07:00
epriestley
e059997e53 Expose subscribers transaction data via Conduit "transaction.search"
Summary:
Depends on D20507. See PHI1232. Previously, see T13255 and D20209.

Since nothing seems to have exploded after "projects" was exposed, give "subscribers" the same treatment.

Test Plan: Added, removed, and modified subscribers. Queried transactions with "transaction.search", saw sensible "type" and data.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20508
2019-05-16 12:19:45 -07:00
epriestley
23bfe0c0f6 Specialize rendering of self-URIs in the form "/X123"
Summary:
Depends on D20510. Ref T5378. When remarkup includes a hyperlink to the current install in the form "/X123" (which is common), load the corresponding object and specialize the rendering.

This doesn't cover everything (notably, no handling for Diffusion paths yet), but does cover a lot of the most common cases.

The "uri" form preserves the URI as written, but adds an icon, tag, and hovercard.

The "{uri}" form is more similar to `{T123}` and shows the object name.

Test Plan: {F6440367}

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Subscribers: joshuaspence

Maniphest Tasks: T5378

Differential Revision: https://secure.phabricator.com/D20512
2019-05-16 12:16:38 -07:00
epriestley
706826bf02 When a user pastes a Phabricator URI into the search box, redirect to the URI
Summary:
Depends on D20509. See PHI1224. Ref T5378. With some frequency, I paste URIs into the global search input (I am dumb).

When I do this dumb thing, redirect to the URI as though the global search was a URI bar.

Maybe only I am dumb like this, but I don't think it'll hurt anything.

Test Plan: pasted a URI and hit return; tried to eat a rock

Reviewers: amckinley, joshuaspence

Reviewed By: joshuaspence

Maniphest Tasks: T5378

Differential Revision: https://secure.phabricator.com/D20510
2019-05-16 11:19:52 -07:00
epriestley
f2feb0378f Remove ancient "PhabricatorQuickSearchEngineExtension" compatibility class
Summary: Ref T5378. This class was renamed more than a year ago, in D19087. Remove the leftover compatiblity layer.

Test Plan: `grep`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5378

Differential Revision: https://secure.phabricator.com/D20509
2019-05-16 11:14:49 -07:00
epriestley
4a3481a493 Enrich "draft", "summary", and "testPlan" transactions from Differential in "transaction.search"
Summary:
See PHI1232, which describes a reasonable use case for wanting information about the "draft" ("Hold as Draft / Do Not Auto-Promote") flag.

Also, flesh out "testPlan" and "summary". It's possible these "blob of remarkup" fields might have metadata some day (e.g., a rendered version or a list of PHIDs or something), but we could add more keys, and we already have some other transactions which work like this.

Test Plan: Used "transaction.search" to fetch these transaction types, saw type information and metadata.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20507
2019-05-16 10:50:28 -07:00
epriestley
18f0f8b029 Add support for custom "Wait for Approval" instructions
Summary:
See PHI1229. An install has a somewhat duct-taped registration flow which can dump users on the "Wait for Approval" screen without clear guidance. The desired guidance is something like "this is totally normal, just wait a bit for a bot to approve you".

Adding guidance here is generally reasonable and consistent with the intent of this feature.

Test Plan: {F6426583}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: kylec

Differential Revision: https://secure.phabricator.com/D20492
2019-05-16 10:41:52 -07:00
epriestley
e5fe4dffe1 Add a "Published document changed" rule to Herald for Phriction documents
Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.

Test Plan:
  - Wrote a Herald rule using this field.
  - Created a document (rule matched).
  - Edited a document (rule matched).
  - Edited a document, saving as a draft (no match).
  - Edited a draft, updating it (no match).
  - Published a draft docuemnt (rule matched).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20520
2019-05-16 10:40:52 -07:00
epriestley
bf2f3991d2 When using the Herald test console on a transactional object, guess a reasonable set of transactions to simulate
Summary:
Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.

This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.

The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.

(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)

Test Plan: Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20519
2019-05-16 10:32:54 -07:00
epriestley
046888ac2e In Herald, save applied transaction PHIDs in the transcript and display them in the UI
Summary:
Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.

Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.

Test Plan:
  - Viewed a modern transcript, saw a list of transactions.
  - Viewed an older transcript, saw nothing (since there were no transactions in the transcript).

{F6456431}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13283

Differential Revision: https://secure.phabricator.com/D20518
2019-05-16 09:58:16 -07:00
epriestley
a1f08de283 When viewing a workboard, only link to parent workboards in crumbs if parents have workboards
Summary:
Depends on D20516. See PHI1247. In D20331, I made the crumbs on workboards point at ancestor workboards.

However, this isn't a great destination if an ancestor doesn't actually have a workboard. In this case, point at the normal profile URI instead.

Test Plan:
  - Viewed a milestone workboard with a parent that had no workboard. Saw a profile link instead of a workboard link (new behavior).
  - Viewed a milestone workboard with a parent that also had a workboard. Saw a workboard link (existing old behavior still works).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20517
2019-05-15 07:08:54 -07:00
epriestley
104eefaa10 Hide the "added a commit/revision" stories from feed and mail
Summary:
Ref T13276. Previously, these edges were added directly with an `EdgeEditor`, so they did not generate transaction stories.

Now, they're added properly, but they aren't terribly useful in feed/mail. Hide them in those contexts, like we already do with other types of similar edges.

Test Plan: Will verify behavior on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20491
2019-05-14 09:39:26 -07:00
epriestley
3e0391c574 Don't mark subscribes/mentions as "Lock Overridden"
Summary:
See PHI1209. When a task is in "Hard Lock" mode, it's still possible to apply some changes to it. Notably:

  - You can subscribe/unsubscribe.
  - You can mention it on another object.
  - You can add a relationship from some other object to it (e.g., select it as a "Parent Task" for some other task).

Currently, these types of edits will show a "Lock Overridden" timeline emblem icon. However, they should not: you didn't override a lock to make these changes, they just bypass locks.

For now, special case these cases (self subscribe/unsubscribe + inverse edge edits) so they don't get the little icon, since I think this list is exhaustive today.

Some day we should modularize this, but we'd need code like this anyway (since TYPE_SUBSCRIBE is not modular yet), and this seems unlikely to cause problems even if it's a bit rough.

Test Plan:
  - Hard-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it as a non-author. No timeline emblems.
  - Soft-locked a task.
    - Subscribed/unsubscribed, mentioned, relationship'd it, no timeline emblems.
    - Clicked "Edit", answered "yes" to the override prompt, edited it. Got a timeline emblem.
  - Added some comments and stuff to a normal non-locked task, no emblems.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20513
2019-05-14 09:27:24 -07:00
epriestley
ee89c2fad9 Fix a fatal when navigating to a Workboard after removing the Workboard menu item
Summary:
See PHI1247. If you remove the Workboard from a project profile menu, then navigate to the URI, we currently fatal when trying to select the "Workboard" item.

Instead, only try to select the item if some matching item is present.

Test Plan:
  - Disabled the workboard on a project, navigated to `/board/`, got a sensible page with no navigation item selected instead of a fatal.
  - Viewed a normal workboard, saw the correct selection.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20516
2019-05-14 09:26:02 -07:00
epriestley
1d58f14469 Remove WePay support from Phortune, and Restful/Httpful dependencies
Summary: Ref PHI1166. I'm documenting our dependencies, and we have approximately 5,000 lines of external code to support WePay as a Phortune provider. We don't use it, I'm almost certain it doesn't work, and we have no plans to use it in the near future. If we did pursue it, I'd probably just wrap the API in a 100-line `WePayFuture` anyway since 5K lines of dependencies to make a couple method calls is ridiculous.

Test Plan: Grepped for `wepay`, `httpful`, `restful`.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: aurelijus

Differential Revision: https://secure.phabricator.com/D20521
2019-05-14 09:14:53 -07:00
epriestley
7a8d489ebd Fix a HunkParser typo, "identcal" -> "identical"
Summary: See rP3f8eccdaec8f85933bfea6ce5348bde25ced0489.

Test Plan: Read carefully.

Reviewers: amckinley, avivey

Reviewed By: avivey

Subscribers: avivey

Differential Revision: https://secure.phabricator.com/D20499
2019-05-07 11:16:55 -07:00
epriestley
5dc4e76eb9 Fix the direction of the commit/revision edge
Summary: Ref T13276. This edge is pointed the wrong way. Point it the right way.

Test Plan: Will verify production works better.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20490
2019-05-02 07:28:49 -07:00
epriestley
aba7d1406a Really fix movable panels inside tab panels by changing the JX.Request serializer
Summary:
Depends on D20475. Ref T13272. Currently, if you `JX.Request` with `data` like `{x: null}`, we submit that as `?x=null`, i.e. as though `null` was the string `"null"`.

This is weird and almost certainly never intended/desiarable. In particular, it causes a bug where panels embedded inside tab panels are incorrectly draggable.

It's possible this breaks something which relied on the buggy behavior, but that seems unlikely.

Test Plan: Tried to drag a panel inside a tab panel, it really truly didn't work.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20476
2019-05-01 15:38:40 -07:00
epriestley
c43618a3a8 Add "Move Left" and "Move Right" to dashboard tab panels
Summary: Depends on D20474. Ref T13272. Provide an easy way to rearrange tabs on a tab panel, by moving them left or right from the context menu.

Test Plan: Moved tabs left and right. Tried to move them off the end of the tab list, no luck.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20475
2019-05-01 15:35:53 -07:00
epriestley
9d716379a3 Add a "Customize Query" action to query panels to make it easier to make minor query adjustments
Summary:
Depends on D20473. Ref T13272. Fixes T7216. If you want to tweak the query a panel uses, you currently have to complete 7 Great Labors.

Instead, add a "Customize Query" action which lets you update the query inline.

Test Plan: {F6402171}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272, T7216

Differential Revision: https://secure.phabricator.com/D20474
2019-05-01 11:01:09 -07:00
epriestley
a15b0063df Add "Revision has passing builds" Herald rules for commit content (pushes) and commits (discovery)
Summary:
Depends on D20469. Ref T13276. See PHI1159. See PHI953. See PHI901.

Allow Herald to detect when "arc land" would (or did) warn users about failed or ongoing builds. This respects the "Warn on Landing" build plan behavior.

To accomplish this:

  - When we close a revision, set a "wrong build state" flag if it lands in the wrong build state.
  - If the revision is closed when we hit Herald, look for the flag.
  - If not (common for push rules, can happen for commit rules if we race against the revision update worker), hit Harbormaster ourselves and check the current state.

Test Plan:

  - Wrote a "Require Green" rule.
  - Ran it against various commits with various build states (good, not good).
  - Fiddled with "Warn on Landing" and saw the effect in rule evaluation.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20470
2019-05-01 10:02:47 -07:00
epriestley
a78f141b3f Unify code for parsing "Reverts X" magic, and when something "reverts <hash>", also revert associated revisions
Summary:
Depends on D20468. Ref T13276. See PHI1008.

When a commit or revision "reverts <hash>", and that's the hash of a commit which has a revision, also write a "reverts" edge to the revision.

Test Plan:
Created "reverts X" objects for:

  - a revision;
  - a commit;
  - a commit with a revision (both got marked properly).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20469
2019-05-01 09:47:40 -07:00
epriestley
56f5c2443b Consolidate readers of "differential.revisionID" property
Summary:
Depends on D20467. Ref T13277. Currently, the "MessageParserWorker" writes this property on commits, then Herald and Audit both read it.

Make them share code so this property has one writer and one reader. This property isn't great, but at least now the badness is hidden.

Currently, we can't just use edges because they may not have been written yet. I am likely to just do this, soon:

  - Just write the edges (in "MessageParserWorker").
  - Hide the edges from mail.

However, we'll sort-of lose the "revisionMatchData" explanation thing if I do that. Maybe this is fine? But when commits match because hashes match, it legitimately isn't obvious.

For now, just reduce the amount of harm/badness here.

Test Plan:
  - Ran `bin/repository reparse --publish ...`.
  - Ran a Herald "Audit" rule using the "Accepted Differential revision" field.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20468
2019-05-01 09:46:17 -07:00
epriestley
45f01dc716 Restore "Limit" to dashboard Query panels
Summary: See PHI1220. Ref T13272. I accidentally left the ability to set a query limit behind when updating this.

Test Plan: Edited a query panel, set/removed the limit, tried to set an invalid limit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20472
2019-05-01 08:41:43 -07:00
epriestley
dfbc4c1cd3 Fix an issue where editing a dashboard panel from a dashboard could duplicate the panel
Summary:
Depends on D20472. Ref T13272. Currently, when you edit a panel from a dashboard, we try to add the panel to the dashboard. This always works since dashboards no longer enforce panel uniqueness, and you can end up with duplicate panels.

Instead, only add panels if we're creating them.

Test Plan:
  - Edited an existing panel, no duplication.
  - Created a new panel, saw it added to the dashboard.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13272

Differential Revision: https://secure.phabricator.com/D20473
2019-04-30 08:27:20 -07:00
epriestley
5462a52b8a Update Quickstart SQL
Summary:
This hasn't been updated in a bit more than a year (last updated in D18594) and we've accumulated a fair number of SQL patches. Update it.

This is mostly automatic (with `bin/storage quickstart`), except:

  - Manual edit to one migration for a missed callsite to `DashboardInstall`.
  - Replaced two InnoDB tables that still have FULLTEXT indexes with MyISAM (see rP6cedd4a95cfc).

This is not really possible to review and more for reference than examination. `bin/storage quickstart` has historically worked correctly.

Test Plan: I have great faith that `bin/storage quickstart` is a script which creates a big `.sql` file.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D20480
2019-04-30 08:19:39 -07:00
epriestley
648d5ce190 Set a URI on Auth Messages, so the "Change Details" dialog from the transaction record has a cancel button
Summary:
If you edit an auth message in Auth > Customize Messages, then click "Show Details" in the transaction record, the resulting dialog uses the object's handle's URI to generate a "cancel" button.

Since these handles currently have no URI, the dialog currently has no cancel/done button to close it.

Test Plan: Edited an auth message, clicked "Show Details", was now able to click "Done" to close the dialog.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20471
2019-04-30 06:59:04 -07:00
epriestley
6d13b9fb7a Make very minor generality improvements to the scope selector
Summary:
See PHI985. I think we pretty much need to start applying language-specific rules, but we can apply at least one more relatively language-agnostic rule: don't match lines which are indented 3+ levels.

In C++, we may have symbols like this:

```
class X {
  public:
    int m() { ... }
}
```

..but I believe no mainstream language puts symbol definitions 3+ levels deep.

Also clean up some of the tab handling very slightly.

Test Plan: Tests pass, looked at some C++ code and got slightly better (but still not great) matches.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20479
2019-04-30 06:52:29 -07:00
epriestley
6eae75d8f2 When building a synthetic diff in Differential, adjust diff properties correctly
Summary:
See PHI1218. When rendering "A vs B", we currently show the properties of diff A without modification.

Instead, take properties from the same place we're taking change details.

See T12664 for a followup.

Test Plan:
  - In diff A, removed "+x" from a file.
  - In diff B, changed the file but did not remove "+x" from it.
  - Diffed B vs A.
    - Before change: UI incorrectly shows "+x" removed (both sides incorrect, just showing the change from diff A).
    - After change: UI shows 100644 -> null, which is half right.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20478
2019-04-30 06:51:40 -07:00
epriestley
0d9362355b In ApplicationTransactionEditor, determine new objects with "getID()" instead of "getPHID()"
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-reload-object-that-hasnt-been-loaded/2677>.

When editing "Config" objects, they currently get a PHID set outside of the TransactionEditor. They probably should not, but fixing that is likely an involved change.

This causes us to incorrectly fail to detect `$is_new` correctly and try to `reload()` and object with no ID.

To work around this, test for new objects with `getID()` instead of `getPHID()`.

Test Plan: Edited any config value with the web UI.

Reviewers: amckinley

Differential Revision: https://secure.phabricator.com/D20482
2019-04-29 07:37:03 -07:00
epriestley
c0a4d1de13 Merge the "Herald" and "Owners" daemon workers into a single "Publish" worker
Summary:
Depends on D20466. Ref T13277. Currently:

  - The "Owners" worker writes ownership relationships (e.g., commit X affects package Y, because it touches a path in package Y) -- these are just edges.
  - It also triggers audits.
  - Then it queues a "Herald" worker.
  - This formally publishes the commit and triggers Herald.

These aren't really separate steps and can happen more easily in one shot. Merge them.

Test Plan:
  - Ran `bin/repository reparse --publish` to republish various commits, got sensible behavior.
  - Grepped for "IMPORTED_OWNERS", "IMPORTED_HERALD", "--herald", "--owners", and "--force-local" flags.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20467
2019-04-25 09:45:59 -07:00
epriestley
0fab41ff3c Show "hold reasons" on commit page, not on "Edit" page
Summary:
Depends on D20465. Ref T13277. Currently, when a commit is unpublished, we put a single line about it on the "Edit Commit" page. This is pretty much impossible to find.

Move it to the main page. This treatment is more big/bold than I'd probably like to end up, but we should probably overshoot on the explanatory text until users get used to this behavior.

Also, allow searching for only published / unpublished commits.

Test Plan: {F6395705}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20466
2019-04-25 09:22:49 -07:00
epriestley
8f43c773b8 Remove nearly all remaining references to "Autoclose"
Summary:
Depends on D20464. Ref T13277. Broadly:

  - Move all the "should publish X" and "why aren't we publishing X" stuff to a separate class (`PhabricatorRepositoryPublisher`).
  - Rename things to be more consistent with modern terminology ("Publish", "Permanent Refs").

Test Plan:
This could use some trial-by-fire on `secure`, but:

  - Grepped for all symbols.
  - Viewed various commits.
  - Reparsed commits.
  - Here's a commit with an explanation:

{F6394569}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20465
2019-04-24 08:29:41 -07:00
epriestley
45b9859f02 Remove "--force-autoclose" from "bin/repository reparse"
Summary: Depends on D20463. Ref T13277. This flag was added some time before 2015 and I don't think I've ever used it. Just get rid of it.

Test Plan: Grepped for `force-autoclose`, `forceAutoclose`, `AUTOCLOSE_FORCED`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13277

Differential Revision: https://secure.phabricator.com/D20464
2019-04-24 08:24:06 -07:00
epriestley
a2d3d8edeb Move "update related object after commit" to a separate worker in the task queue
Summary:
Depends on D20462. Ref T13276. Currently, the "Message" parser also updates related tasks and revisions when a commit is published.

For PHI1165, which ran into a race with message parsing, I originally believed we needed to separate this logic and lock + yield to avoid the race. D20462 provides what is probably a better approach for avoiding the race.

Still, I think separating these "update related revisions" and "updated related tasks" chunks into separate workers is a net improvement. There may still be some value in doing lock + yield in the future to deal with other issues, and when we occasionally run into problems with pulling a diff out of the repository to update the revision (usually because the diff is too big) this isolates the problem better and allows the commit to import.

I think the only thing to watch out for here is that Herald may now run before the revision and commit are attached to one another. This is fine for all current Herald rules, we just need to be mindful in implementing new rules.

Test Plan: Used `bin/repository reparse --message` on various commits, including commits that close revisions and close tasks.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20463
2019-04-24 06:32:27 -07:00
epriestley
b3b1cc64bd When applying transactions, acquire a read lock sooner
Summary:
Depends on D20461. Ref T13276. Ref T13054.

Currently, we acquire the transaction read lock after populating "old values" in transactions and filtering transactions with no effect.

This isn't early enough to prevent all weird chaotic races: if two processes try to apply a "close revision" transaction at the same time, this can happen:

```
PROCESS A             PROCESS B
Old Value = Open      Old Value = Open
Transaction OK: Yes   Transaction OK: Yes
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      Apply Transactions
                      New Value = Closed
                      Release Lock
```

That's not great: both processes apply an "Open -> Closed" transaction since this was a valid transaction from the viewpoint of each process when it did the checks.

We actually want this:

```
PROCESS A             PROCESS B
Acquire Read Lock     Acquire Read Lock
Got Read Lock!        Wait...
Old Value = Open      Wait...
Transaction OK: Yes   Wait...
Apply Transactions    Wait...
New Value = Closed    Wait...
Release Lock          Wait...
                      Got Read Lock!
                      >>> Old Value = Closed
                      >>> Transaction Has No Effect!
                      >>> Do Nothing / Abort
                      Release Lock
```

Move the "lock" part up so we do that.

This may cause some kind of weird second-order effects, but T13054 went through pretty cleanly and we have to do this to get correct behavior, so we can survive those if/when they arise.

Test Plan:
  - Added a `sleep(10)` before the lock.
  - Ran `bin/repository message --reparse X` in two console windows, where X is a commit that closes revision Y and Y is open.
    - Before patch: both windows closed the revision and added duplicate transactions.
    - After patch: only one of the processes had an effect.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jmeador

Maniphest Tasks: T13276, T13054

Differential Revision: https://secure.phabricator.com/D20462
2019-04-24 05:57:29 -07:00
epriestley
42c02557e4 Remove all remaining readers and writers for TABLE_COMMIT
Summary: Depends on D20459. Ref T13276. I'll file a followup to actually destroy the table.

Test Plan:
- Grepped for `TABLE_COMMIT`.
- Ran `bin/storage upgrade -f`, got a clean bill of health.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20461
2019-04-24 05:55:23 -07:00
epriestley
ec0085fd0c Align "RevisionQuery->needCommitPHIDs()" to use Edges, not the legacy table
Summary:
Depends on D20458. Ref T13276. Although I'm not thrilled about "needCommitPHIDs()", it has a few callers, including custom fields. Allow "need + attach + get" to survive for now since they're reasonably modern, at least.

However, use edges instead of "TABLE_COMMIT" and require `need...()` + `get...()`, removing the direct `load...()`.

Also remove `RevisionQuery->withCommitPHIDs(...)`, which has no callers.

Test Plan:
- Grepped for `loadCommitPHIDs` (only two hits, the private `RevisionQuery` method).
- Called "differential.getrevision", got commits.
- Viewed a revision, saw "Commits: ...".
- Grepped for `withCommitPHIDs()`, no callers on `RevisionQuery` (some other query classes have methods with this name).
- Called "differential.query", got commits.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20459
2019-04-24 05:54:38 -07:00
epriestley
7e8dc0742b Remove all callers to "DifferentialRevision->loadIDsByCommitPHIDs()"
Summary: Depends on D20457. Ref T13276. Kill all remaining callers to this method and delete it.

Test Plan:
- Grepped for `loadIDsByCommitPHIDs`.
- Viewed blame again to make sure I didn't break it.
- Viewed "History" view for commits with revisions.
- Viewed "Graph" view for commits with revisions.
- Viewed "Merged Commits" table for commits with revisions.
- Viewed "Compare" table for commits with revisions.
- Viewed "Repository" main page history table for commits with revisions.
- Grepped for `linkRevision`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20458
2019-04-24 05:53:35 -07:00
epriestley
fc92cf4382 Move BlameController away from ancient "TABLE_COMMIT"
Summary:
Ref T13276. Differential has a pre-edge "TABLE_COMMIT" with about a half-dozen weird callers I'd like to get rid of.

Move blame to use edges instead. (Bonus: this makes blame respect edge edits in the UI.)

Since there are some more callers to clean up this code may move into some "RelatedObjectQueryThing" class or something, but I'm taking it one step at a time for now.

Test Plan: {F6394106}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13276

Differential Revision: https://secure.phabricator.com/D20457
2019-04-24 05:44:40 -07:00
epriestley
70c643c685 Simplify implementation of "pure" Chart functions
Summary:
Depends on D20445. Ref T13279. I'm not sure what the class tree of functions actually looks like, and I suspect it isn't really a tree, so I'm hesitant to start subclassing. Instead, try adding some `isSomethingSomething()` methods.

We have some different types of functions:

  # Some functions can be evaluated anywhere, like "constant(3)", which always evaluates to 3.
  # Some functions can't be evaluated anywhere, but have values everywhere in some domain. This is most interesting functions, like "number of open tasks". These functions also usually have a distinct set of interesting points, and are constant between those points (any count of anything, like "open points in project" or "tasks closed by alice", etc).
  # Some functions can be evaluated almost nowhere and have only discrete values. This is most of the data we actually store, which is just "+1" when a task is opened and "-1" when a task is closed.

Soon, I'd like to be able to show ("all tasks" - "open tasks") and draw a chart of closed tasks. This is somewhat tricky because the two datasets are of the second class of function (straight lines connecting dots) but their "interesting" x values won't be the same (users don't open and close tasks every second, or at the same time).

The "subtract X Y" function will need to be able to know that `subtract "all tasks" 3` and `subtract "all tasks" "closed tasks"` evaluate slightly differently.

To make this worse, the data we actually //store// is of the third class of function (just the "derivative" of the line chart), then we accumulate it in the application after we pull it out of the database. So the code will need to know that `subtract "derivative of all tasks" "derivative of closed tasks"` is meaningless, or the UI needs to make that clear, or it needs to interpret it to mean "accumulate the derivative into a line first".

Anyway, I'll sort that out in future changes. For now, simplify the easy case of functions in class (1), where they're just actual functions.

Add "shift(function, number)" and "scale(function, number)". These are probably like "mul" and "add" but they can't take two functions -- the second value must always be a constant. Maybe these will go away in the future and become `add(function, constant(3))` or something?

Test Plan: {F6382885}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20446
2019-04-23 12:29:45 -07:00
epriestley
edaf17f3fe Make chart function argument parsing modular/flexible with 900 pages of error messages
Summary:
Depends on D20444. Ref T13279. Instead of ad-hoc parsing and messages, formalize chart function arguments.

Also, add a whole lot of extra type checking.

Test Plan: Built and charted various functions with various valid and invalid argument lists, got sensible-seeming errors and results.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13279

Differential Revision: https://secure.phabricator.com/D20445
2019-04-23 12:26:32 -07:00
epriestley
edda40aa3b (stable) Change "Revision Close" story to use commit identities only with no fallback to commit data
Summary:
See PHI1213. If we don't have identities for "revision X closed by commit Y" stories, just do the plain non-attribution rendering rather than trying to fall back. Falling back won't work since we don't load the data, which should be OK now since identities seem like they're in generally good shape.

(We could probably just throw out the fallback behavior instead, but we can always clean things up later.)

Test Plan: Forced no commit identity on a revision, loaded it, saw reasonable story rendering.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20460
2019-04-23 11:57:46 -07:00
epriestley
7b8ac020b5 Change "Revision Close" story to use commit identities only with no fallback to commit data
Summary:
See PHI1213. If we don't have identities for "revision X closed by commit Y" stories, just do the plain non-attribution rendering rather than trying to fall back. Falling back won't work since we don't load the data, which should be OK now since identities seem like they're in generally good shape.

(We could probably just throw out the fallback behavior instead, but we can always clean things up later.)

Test Plan: Forced no commit identity on a revision, loaded it, saw reasonable story rendering.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D20460
2019-04-23 11:57:33 -07:00
epriestley
2ffc4be4e7 (stable) Fix "before/after" cursor paging for API call "feed.query"
Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/undefined-method-setafterid-when-calling-feed-query/2653>.

This older API call needs an update to the newer paging/cursor API.

Test Plan: Called `feed.query` with an "after" parameter.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: Itms

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20456
2019-04-23 11:57:05 -07:00
epriestley
b0b8926c75 Fix "before/after" cursor paging for API call "feed.query"
Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/undefined-method-setafterid-when-calling-feed-query/2653>.

This older API call needs an update to the newer paging/cursor API.

Test Plan: Called `feed.query` with an "after" parameter.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: Itms

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20456
2019-04-23 11:51:16 -07:00
epriestley
29b2125e3f (stable) Fix an issue where internal paging of notifications could fail if some notifications are not visible
Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/notification-page-throws-unrecoverable-fatal-error/2651/>.

The "notifications" query currently uses offset paging for no apparent reason (just a legacy issue?), so some of the paging code is only reachable internally.

  - Stop it from using offset paging, since modern cursor paging is fine here (and Feed has used cursor paging for a long time).
  - Fix the non-offset paging to work like Feed.

Also:

  - Remove a couple of stub methods with no callsites after cursor refactoring.

Test Plan:
  - Set things up so I had more than 100 notifications and some in the first 100 were policy filtered, to reproduce the issue (I just made `FeedStory` return `NO_ONE` as a visibility policy).
  - Applied the patch, notifications now page cleanly.
  - Verified that "Next Page" took me to the right place in the result list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hskiba

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20455
2019-04-23 11:45:17 -07:00
epriestley
a4a1143b18 Fix an issue where internal paging of notifications could fail if some notifications are not visible
Summary:
Ref T13266. See <https://discourse.phabricator-community.org/t/notification-page-throws-unrecoverable-fatal-error/2651/>.

The "notifications" query currently uses offset paging for no apparent reason (just a legacy issue?), so some of the paging code is only reachable internally.

  - Stop it from using offset paging, since modern cursor paging is fine here (and Feed has used cursor paging for a long time).
  - Fix the non-offset paging to work like Feed.

Also:

  - Remove a couple of stub methods with no callsites after cursor refactoring.

Test Plan:
  - Set things up so I had more than 100 notifications and some in the first 100 were policy filtered, to reproduce the issue (I just made `FeedStory` return `NO_ONE` as a visibility policy).
  - Applied the patch, notifications now page cleanly.
  - Verified that "Next Page" took me to the right place in the result list.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: hskiba

Maniphest Tasks: T13266

Differential Revision: https://secure.phabricator.com/D20455
2019-04-23 11:45:04 -07:00