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

13509 commits

Author SHA1 Message Date
epriestley
03d059dd26 Don't include resigned reviewers in the Differential "To" list
Summary: Ref T12689. See PHI178. This isn't a complete solution (you may still get mailed via packages/projects) but should fix the obvious issue, where "Resigned" reviewers are incorrectly always sent mail directly.

Test Plan: Had Alice resign, interacted as Bailey, no mail to Alice.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12689

Differential Revision: https://secure.phabricator.com/D18758
2017-11-01 17:22:45 -07:00
epriestley
6ecdadb76a After "Request Review", move revisions with voided "Accepts" into "Ready to Review", not "Waiting on Other Reviewers"
Summary:
Depends on D18756. Fixes T12539. See PHI190. Currently, when this occurs:

  - Alice accepts.
  - Bailey requests review.
  - Alice views her dashboard.

...the revision appears in "Waiting on Other Reviewers" (regardless of whether other reviewers actually exist or not).

Instead, ignore these voided/non-current accepts and let the revisions appear in "Ready to Review", which is more natural.

Test Plan: Went through the steps above. On `master`, saw revision in "Waiting on Other Reviewers". After patch, saw it in "Ready to Review".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12539

Differential Revision: https://secure.phabricator.com/D18757
2017-11-01 17:19:54 -07:00
epriestley
6d36eb9113 Denormalize Diff PHIDs onto Revisions
Summary:
Ref T12539. See PHI190. Currently, each Diff has a `revisionID`, but Revisions do not point at the current active diff. To find the active diff for a given revision, we need to issue a separate query.

Furthermore, this query is inefficient for bulk loads: if we have a lot of revisions, we end up querying for all diff IDs for all those revisions first, then selecting the largest ones and querying again to get the actual diff objects. This strategy could likely be optimized but the query is a mess in any case.

In several cases, it's useful to have the active diff PHID without needing to do a second query -- sometimes for convenience, and sometimes for performance.

T12539 is an example of such a case: it would be nice to refine the bucketing logic (which only depends on active diff PHIDs), but it feels bad to make the page heavier to do it.

For now, this is unused. I'll start using it to fix the bucketing issue, and then we can expand it gradually to address other performance/convenience issues.

Test Plan:
  - Ran migrations, inspected database, saw sensible values.
  - Created a new revision, saw a sensible database value.
  - Updated an existing revision, saw database update properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12539

Differential Revision: https://secure.phabricator.com/D18756
2017-11-01 17:19:38 -07:00
epriestley
80ebe401e5 Tweak padding/spacing on Diffusion blame view for profile pictures
Summary: Give profile images a little more space, fix "/" spacing, add a tooltip.

Test Plan: {F5251205}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18749
2017-10-31 12:56:36 -07:00
epriestley
a4b934cad2 Clean up Differential draft mail behaviors
Summary:
Ref T2543. Fixes two relatively minor things:

  - When builds finish in Harbormaster, send mail "From" the author.
  - Set the `firstBroadcast` flag so that initial mail picks up earlier history (notably, the "reviewers" line).

For now, I'm not setting `firstBroadcast` on explicit "Request Review" (but maybe we should), and not trying to deal with weird cases where you leave a bunch of comments on a draft. Those might be fine as-is or may get tweaked later.

Test Plan: Created a revision with Harbormaster builds, ran builds, saw initial email come "From" the right user with more metadata.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18748
2017-10-31 12:56:03 -07:00
epriestley
bde71324f8 Show small author portraits in Diffusion blame view
Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub).

Test Plan:
My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional:

{F5251056}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18747
2017-10-31 12:10:45 -07:00
epriestley
90d0f8ac6c Revert changes to Diffusion blame view
Summary:
Ref PHI174. This reverts most of these changes:

- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452

These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.

I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.

In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.

I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".

I'm going to follow this up with some additional changes:

  - Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
  - Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
  - Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
  - Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.

Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.

{F5251019}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18746
2017-10-31 11:54:47 -07:00
epriestley
7fa0d066bc Don't run Herald build rules when Differential revisions are updated automatically
Summary: Ref T2543. After D18731, Herald build rules run more often, but now incorrectly try to run builds when Diffusion closes a revision because a commit landed.

Test Plan: Made some mundane updates locally; this is tricky to test comprehensively locally so I'm mostly planning to just push it to `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18745
2017-10-31 11:36:04 -07:00
epriestley
28cec2f8a2 Allow revisions to be held as drafts, even after builds finish
Summary:
Ref T2543. Instead of autosubmitting revisions to "Needs Review" when builds finish, allow them to be held in "Draft" indefinitely.

There's currently no UI for this. I plan to just expose it as `arc diff --draft` for now, in a followup change.

Test Plan:
  - Created a revision (via Conduit) with "hold as draft", saw it hold as draft after builds finished.
  - Created a revision (normally), saw it autosubmit after builds finished.
  - Requested review of a "hold as draft" revision to kick it out of draft state.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18737
2017-10-31 09:39:32 -07:00
epriestley
f5336cd6e7 Return transactions from "differential.parsecommitmessage"
Summary:
Depends on D18740. Prepares `arc` to receive a `--draft` flag by letting us switch to "differential.revision.edit" instead of "differential.createrevision".

To "differential.revision.edit", we need a transaction list, but we can't automatically construct this list from a field map. Return the transaction list alongside the field map.

The next change uses this list (if available) to switch us to the modern API method.

Test Plan: Ran `arc diff` on the experiemntal branch with followup changes, got a new revision.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18741
2017-10-30 15:15:42 -07:00
epriestley
0da3f34728 Provide "differential.diff.search"
Summary: See PHI90. For now, this only provides a limited amount of information, but should satisfy the use case in PHI90 and build toward a more complete version in the future.

Test Plan: Used new Conduit method to retrieve information about diffs.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18744
2017-10-30 15:06:10 -07:00
epriestley
f7f3dd5b20 Don't run Herald build and mail rules when they don't make sense
Summary:
Ref T2543. Fixes T10109.

Currently, Herald only runs in Differential when a change updates the diff. This is partly for historical reasons, and partly because we don't want to restart builds every time someone makes a comment. However, this behavior is inconsistent with other applications (which always trigger on any change), and occasionally confusing to users (in T10109, for example) or otherwise undesirable.

A similar issue is that T2543 has introduced a "Draft" state, where revisions don't send normal mail until builds finish. This interacts poorly with "Send me an email" rules (which shouldn't do anything here) and particularly with "Send me an email + only run these actions the first time the rule matches", since that might have an effect like "do nothing when the revision is created, then never anything again since you already did nothing once".

To navigate both of these issues, let objects tell Herald that certain actions (like mail or builds) are currently forbidden. If a rule uses a field or action which is currently forbidden, the whole rule automatically fails before it executes, but doesn't count toward "only the first time" as far as Herald's tracking of rule execution is concerned.

Then, forbid mail for draft revisions, and forbid builds for revisions which didn't just get updated. Forbidding mail fixes the issues with "Send me an email" that were created by the introduction of the draft state.

Finally, make Herald run on every revision update, not just substantive updates to the diff. This resolves T10109.

Test Plan:
Created revisions via the draft -> submit workflow, saw different transcripts. Here's a mail action being forbidden for a draft:

{F5237324}

Here's a build action being forbidden for a "mundane" update:

{F5237326}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T10109, T2543

Differential Revision: https://secure.phabricator.com/D18731
2017-10-27 08:44:12 -07:00
epriestley
d1b4c9f50b Add a missing key on DrydockLease
Summary: Depends on D18734. See PHI176. We run this query on the main Drydock lease web UI, among other places. There is currently no `status` key which can satisfy it.

Test Plan:
Viewed Drydock lease page to get the query.

Ran ##explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101;## before and after the change.

I don't have a ton of leases locally so the un-key'd EXPLAIN isn't //that// bad, but still shows that we're getting a better key. Before:

```
mysql> explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101;
+----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+
| id | select_type | table         | type  | possible_keys | key     | key_len | ref  | rows | Extra       |
+----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+
|  1 | SIMPLE      | drydock_lease | index | NULL          | PRIMARY | 4       | NULL |  101 | Using where |
+----+-------------+---------------+-------+---------------+---------+---------+------+------+-------------+
1 row in set (0.00 sec)
```

After:

```
mysql> explain SELECT * FROM `drydock_lease` WHERE (status IN ('pending', 'acquired', 'active')) ORDER BY `id` DESC LIMIT 101;
+----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+
| id | select_type | table         | type  | possible_keys | key        | key_len | ref  | rows | Extra                                 |
+----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+
|  1 | SIMPLE      | drydock_lease | range | key_status    | key_status | 130     | NULL |    5 | Using index condition; Using filesort |
+----+-------------+---------------+-------+---------------+------------+---------+------+------+---------------------------------------+
1 row in set (0.00 sec)
```

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18735
2017-10-26 18:20:38 -07:00
epriestley
b04d9fdc0b Add a missing key to PhabricatorFile for destroying Files
Summary: See PHI176. Depends on D18733. We issue a query when deleting files that currently doesn't hit any keys.

Test Plan:
Ran `./bin/remove destroy --force --trace F56376` to get the query.

Ran ##SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1## before and after the change.

Before:

```
mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1;
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
| id | select_type | table | type | possible_keys | key  | key_len | ref  | rows  | Extra       |
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
|  1 | SIMPLE      | file  | ALL  | NULL          | NULL | NULL    | NULL | 33866 | Using where |
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
1 row in set (0.01 sec)
```

After:

```
mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1;
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
| id | select_type | table | type | possible_keys | key        | key_len | ref         | rows | Extra                              |
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
|  1 | SIMPLE      | file  | ref  | key_engine    | key_engine | 388     | const,const |  190 | Using index condition; Using where |
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
1 row in set (0.00 sec)
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18734
2017-10-26 18:20:12 -07:00
epriestley
fbfed82efd Add a missing DaemonLogEvent key for garbage collection
Summary: See PHI176. This is issued periodically by the garbage collector. Normally this table is relatively small-ish so this missing key isn't hugely noticeable.

Test Plan:
Ran `./bin/garbage collect --collector daemon.processes --trace` to get the query the GC runs.

Ran ##DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100## before and after the key, saw a much better query plan afterward:

Before:

```
mysql> explain DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100;
+----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+
| id | select_type | table           | type | possible_keys | key  | key_len | ref  | rows  | Extra       |
+----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+
|  1 | SIMPLE      | daemon_logevent | ALL  | NULL          | NULL | NULL    | NULL | 19325 | Using where |
+----+-------------+-----------------+------+---------------+------+---------+------+-------+-------------+
1 row in set (0.00 sec)
```

After:

```
mysql> explain DELETE FROM `daemon_logevent` WHERE epoch < 1508443504 LIMIT 100;
+----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+
| id | select_type | table           | type  | possible_keys | key       | key_len | ref   | rows | Extra       |
+----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+
|  1 | SIMPLE      | daemon_logevent | range | key_epoch     | key_epoch | 4       | const |    1 | Using where |
+----+-------------+-----------------+-------+---------------+-----------+---------+-------+------+-------------+
1 row in set (0.00 sec)
```

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18733
2017-10-26 18:19:46 -07:00
epriestley
a03da0c2af Add a missing artifactIndex key to HarbormasterBuildArtifact
Summary: See PHI176. We issue a query with only `artifactIndex` from `BuildTarget`, but don't have an applicable key.

Test Plan: This isn't on the normal Harbormaster execution path so I'm not 100% sure I have a local repro, but will confirm with customer.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18732
2017-10-26 18:18:24 -07:00
epriestley
f1204c8c45 Convert Ponder Questions to Ferret engine
Summary: See PHI177. Ref T12974. PonderQuestion was overlooked during the Ferret engine conversions.

Test Plan:
Ran migrations, searched for questions, got results:

{F5241185}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18736
2017-10-26 18:18:04 -07:00
epriestley
beaf0ad9a6 Attribute revision promotion from "Draft" to "Needs Review" to the author
Summary:
Ref T2543. When Harbormaster finishes builds and promotes a draft revision to review, we currently publish "Harbormaster requested review of...".

Instead, attribute this action to the author, since that's more natural and more useful.

Test Plan: Promoted a diff locally, saw it attributed to me rather than Harbormaster.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18730
2017-10-26 12:57:47 -07:00
epriestley
28a24c333f Fix a couple of other missing getApplicationTransactionCommentObject() implementations
Summary:
See PHI165. See D18715. These objects (projects, blogs) also need implementations now.

(I thought about making this method `abstract` or doing try/catch to maybe make this more robust, but I think this should be the end of it, and those changes have mild complexity/compatibility/risk issues.)

Test Plan: Changed `bin/search index` to index only one document of each type, ran `bin/search index --all --force`, saw no more comment-related errors.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18729
2017-10-24 09:05:23 -07:00
epriestley
1d213dc1fa Clean up virtual "_ft_rank" column for query construction of Ferret objects
Summary:
Ref T12974. Ferret object queries SELECT a virtual "_ft_rank" column for relevance ordering.

Currently, they always SELECT this column. That's fine and doesn't hurt anything, but makes developing and debugging things kind of a pain since every query has this `, blah blah _ft_rank` junk.

Instead, construct this column only if we're actually going to use it.

Mostly, this cleans up DarkConsole / query logs a bit.

Test Plan:
Viewed normal query results on various pages, viewed global search results, ordered Maniphest tasks by normal stuff and by "Relevance".

Viewed DarkConsole, saw no more "_ft_rank" junk on normal pages.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18728
2017-10-23 16:18:04 -07:00
epriestley
683df399e7 Simplify UNION/ORDER query construction in DifferentialRevisionQuery
Summary: Ref T12680. Use the slightly sleeker construction from D18722 in Differential.

Test Plan: Viewed revision list, reordered by date modified.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12680

Differential Revision: https://secure.phabricator.com/D18727
2017-10-23 16:17:47 -07:00
epriestley
5975b9d28e (stable) Correct a method signature in DifferentialDraftField
Summary:
Ref T12190. See <https://discourse.phabricator-community.org/t/exception-preventing-access-to-differential-application/606>.

(I have a followup to fix the root issue.)

Test Plan: Loaded Differential with an eye on the error log in PHP7, no longer saw warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12190

Differential Revision: https://secure.phabricator.com/D18723
2017-10-23 10:54:39 -07:00
epriestley
e3a48dde1d Correct a method signature in DifferentialDraftField
Summary:
Ref T12190. See <https://discourse.phabricator-community.org/t/exception-preventing-access-to-differential-application/606>.

(I have a followup to fix the root issue.)

Test Plan: Loaded Differential with an eye on the error log in PHP7, no longer saw warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12190

Differential Revision: https://secure.phabricator.com/D18723
2017-10-23 10:33:53 -07:00
epriestley
157f47cd14 Rewrite CommitQuery to use UNION for performance
Summary:
Ref T12680. See PHI167. See that task for discussion.

Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to".

I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now.

Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12680

Differential Revision: https://secure.phabricator.com/D18722
2017-10-23 10:32:24 -07:00
epriestley
63e6b2553e Simply how Differential drafts ignore Harbormaster autobuilds
Summary:
Ref T2543. When a revision is created, we check if any builds are waiting/failed, and submit it for review immediately if we aren't waiting for anything.

In doing this, we ignore builds with only autotargets, since these are client-side and failures from local `arc lint` / `arc unit` should not count (the user has already chosen to ignore/skip them).

The way we do this has some issues:

  - Herald may have started builds, but they may still be PENDING and not have any targets yet. In this case, we'll see "no non-autotargets" and ignore the build, which is wrong.
  - We have to load targets but don't really care about them, which is more work than we really need to do.
  - And it's kind of complex, too.

Instead, just let `BuildQuery` filter out "autobuilds" (builds generated from autoplans) with a JOIN.

Test Plan: Ran `arc diff` with builds configured, got a clean "Draft" state instead of an incorrect promotion directly to "Needs Review".

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18721
2017-10-23 10:31:48 -07:00
epriestley
24ff632612 (stable) Promote 2017 Week 42 2017-10-21 05:57:58 -07:00
epriestley
672247eff3 Add aural "+" and "-" hints to unified diffs for users who use screenreaders
Summary: See PHI160 for discussion.

Test Plan:
With `?__aural__=1`, saw aural hints:

{F5229986}

Without, saw normal visual diff.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18718
2017-10-20 11:09:46 -07:00
epriestley
65f13b156f Improve "refengine" performance for testing large numbers of Mercurial branches
Summary:
See PHI158. In the RefEngine, we test if any old branch positions have been removed from the repository. This is uncommon (but not impossible) in Mercurial, and corresponds to users deleting branches in Git.

Currently, we end up running `hg log` for each position, in parallel. Because of Python's large startup overhead, this can be resource intensive for repositories with a large number of branches.

We have to do this in the general case because the caller may be asking us to resolve `tip`, `newfeature`, `tip~3`, `9`, etc. However, in the specific case where the refs are 40-digit hashes, we can bulk resolve them if they exist, like this:

```
hg log ... --rev (abcd or def0 or ab12 or ...)
```

In the general case, we could probably do less of this than we currently do (instead of testing all old heads, we could prune the list by removing commits which we know are still pointed to by current heads) but that's a slightly more involved change and the effect here is already dramatic.

Test Plan:
Verified that CPU usage drops from ~110s -> ~0.9s:

Before:

```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.

real	0m14.676s
user	1m24.714s
sys	0m21.645s
```

After:

```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.

real	0m0.861s
user	0m0.882s
sys	0m0.213s
```

  - Manually resolved `blue`, `tip`, `9`, etc., got expected results.
  - Tried to resolve invalid hashes, got expected result (no resolution).

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18717
2017-10-20 11:09:14 -07:00
epriestley
cbcab60fbb Remove obsolete instructions from information on prototype applications
Summary: Most of this document is no longer relevant, since we're happy to work on prototypes if you're paying us and no longer have any meaningful free support.

Test Plan: Read document.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18719
2017-10-20 11:00:00 -07:00
epriestley
01b1854fb4 Fix an issue with attempting to index comments on packages
Summary: See rPcd14194a329788d5fff6365bcade278fd18f3612 for a similar change. Implement `getApplicationTransactionCommentObject()` to return `null` explicitly.

Test Plan: Ran `bin/search index --type ownerspackage`, got indexing after change.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18715
2017-10-20 09:38:45 -07:00
epriestley
1755ec2429 Show more detailed hints about draft revisions in the UI
Summary: Ref T2543. When revisions are in the draft state, tell the user what we're waiting for or why they aren't moving forward.

Test Plan: {F5228840}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18714
2017-10-20 08:40:17 -07:00
epriestley
bfabe49c5a Start revisions in "Draft" if prototypes are enabled
Summary: Ref T2543. This is a less ambitious version of the rule in D18628, which I backed off from, since I think this probably still has a fair number of loose ends to tie up.

Test Plan: Created a revision locally.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18713
2017-10-20 08:39:58 -07:00
epriestley
d36f98a15a Clarify acceptable values for --threshold in search ngrams
Summary: See D18710.

Test Plan: o_O

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18712
2017-10-17 14:32:25 -07:00
epriestley
63d1230ade Parameterize the common ngrams threshold
Summary:
Ref T13000. Since other changes have generally made the ngrams table manageable, I'm not planning to enable common ngrams by default at this time.

Instead, make the threshold configurable with "--threshold" so we can guide installs through tuning this if they want (e.g. PHI110), and tune hosted instances.

(This might eventually become automatic, but just smoothing this bit off for now feels reasonable to me.)

Test Plan: Ran with `--reset`, and with various invalid and valid `--threshold` arguments.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18710
2017-10-17 14:13:49 -07:00
epriestley
9365c6c999 (stable) Promote 2017 Week 41 2017-10-13 14:50:04 -07:00
epriestley
c5e8de9450 Make bin/storage dump insert CREATE DATABASE and USE statements
Summary:
Ref T13000. The new approach for dumping database-by-database means that we don't get CREATE DATABASE or USE statements, which makes importing the dump again inconvenient.

Manually stitch these into the dump.

Test Plan:
  - Used `bin/storage dump --namespace ...` to dump a smaller local instance.
  - Used `bin/storage destroy --namespace ...`, to destroy the namespace, then inported the dump cleanly.
  - Verified that each CREATE DATABASE statement appears only once.
  - Verified that `bin/storage renamespace --live` can correctly process this file.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18707
2017-10-13 14:35:18 -07:00
epriestley
acb145b3d7 Allow duplicates and merged-in tasks to be queried with edge.search
Summary: See PHI147.

Test Plan: Called the method from the web UI, got sensible results.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18706
2017-10-13 13:12:50 -07:00
epriestley
3f53718d10 Modularize rate/connection limits in Phabricator
Summary:
Depends on D18702. Ref T13008. This replaces the old hard-coded single rate limit with multiple flexible limits, and defines two types of limits:

  - Rate: reject requests if a client has completed too many requests recently.
  - Connection: reject requests if a client has too many more connections than disconnections recently.

The connection limit adds +1 to the score for each connection, then adds -1 for each disconnection. So the overall number is how many open connections they have, at least approximately.

Supporting multiple limits will let us do limiting by Hostname and by remote address (e.g., a specific IP can't exceed a low limit, and all requests to a hostname can't exceed a higher limit).

Configuring the new limits looks something like this:

```
PhabricatorStartup::addRateLimit(new PhabricatorClientRateLimit())
  ->setLimitKey('rate')
  ->setClientKey($_SERVER['REMOTE_ADDR'])
  ->setLimit(5);

PhabricatorStartup::addRateLimit(new PhabricatorClientConnectionLimit())
  ->setLimitKey('conn')
  ->setClientKey($_SERVER['REMOTE_ADDR'])
  ->setLimit(2);
```

Test Plan:
  - Configured limits as above.
  - Made a lot of requests, got cut off by the rate limit.
  - Used `curl --limit-rate -F 'data=@the_letter_m.txt' ...` to upload files really slowly. Got cut off by the connection limit. With `enable_post_data_reading` off, this correctly killed the connections //before// the uploads finished.
  - I'll send this stuff to `secure` before production to give it more of a chance.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13008

Differential Revision: https://secure.phabricator.com/D18703
2017-10-13 13:12:05 -07:00
epriestley
9777c66576 Allow Phabricator to run with "enable_post_data_reading" disabled
Summary:
Ref T13008. Depends on D18701. The overall goal here is to make turning `enable_post_data_reading` off not break things, so we can run rate limiting checks before we read file uploads.

The biggest blocker for this is that turning it off stops `$_FILES` from coming into existence.

This //appears// to mostly work. Specifically:

  - Skip the `max_post_size` check when POST is off, since it's meaningless.
  - Don't read or scrub $_POST at startup when POST is off.
  - When we rebuild REQUEST and POST before processing requests, do multipart parsing if we need to and rebuild FILES.
  - Skip the `is_uploaded_file()` check if we built FILES ourselves.

This probably breaks a couple of small things, like maybe `__profile__` and other DarkConsole triggers over POST, and probably some other weird stuff. The parsers may also need more work than they've received so far.

I also need to verify that this actually works (i.e., lets us run code without reading the request body) but I'll include that in the change where I update the actual rate limiting.

Test Plan:
  - Disabled `enable_post_data_reading`.
  - Uploaded a file with a vanilla upload form (project profile image).
  - Uploaded a file with drag and drop.
  - Used DarkConsole.
  - Submitted comments.
  - Created a task.
  - Browsed around.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13008

Differential Revision: https://secure.phabricator.com/D18702
2017-10-13 13:11:11 -07:00
epriestley
821c7ac833 For backup persitsence, mark the "common ngrams" table as a data table, not an index table
Summary:
Ref T13000. Garbage collecting common ngrams is slow because MySQL isn't all that great at deleting rows quickly. See PHI96, where it looks like it's going to take a week to GC ngrams for a ~million objects at a relatively conservative 0.15 threshold.

In the event of a restore, we can reduce the impact by persisting this table so the ngrams just don't get built when the reindex happens.

Test Plan: Viewed schema in Config, saw common ngrams tables marked as "Data" instead of "Index".

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18696
2017-10-11 11:07:43 -07:00
Dmitri Iouchtchenko
5897294fa9 Add spelling TODOs
Summary: Ref T13005. Added reminders not to copy/paste.

Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13005

Differential Revision: https://secure.phabricator.com/D18695
2017-10-09 11:56:53 -07:00
Dmitri Iouchtchenko
cf3e198b9f Change 'tempate' to 'template'
Summary: Fixed typo. See D18693.

Test Plan: None.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D18694
2017-10-09 11:56:06 -07:00
Dmitri Iouchtchenko
9bd6a37055 Fix spelling
Summary: Noticed a couple of typos in the docs, and then things got out of hand.

Test Plan:
  - Stared at the words until my eyes watered and the letters began to swim on the screen.
  - Consulted a dictionary.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18693
2017-10-09 10:48:04 -07:00
epriestley
8edd04aff3 (stable) Fix "bin/storage dump" with no "--output"
Ref T13004. (I distinctly remember testing this, but must have tweaked things afterward.)
2017-10-07 13:24:15 -07:00
epriestley
4fd9d2d4bb Fix "bin/storage dump" with no "--output"
Ref T13004. (I distinctly remember testing this, but must have tweaked things afterward.)
2017-10-07 13:23:18 -07:00
epriestley
0361591da0 (stable) Correct "bin/storage dump" command construction with passwords
Fixes T13004. This should mirror the other branch.
2017-10-07 05:00:39 -07:00
epriestley
1ee7b3ab8c Correct "bin/storage dump" command construction with passwords
Fixes T13004. This should mirror the other branch.
2017-10-07 04:59:29 -07:00
epriestley
58598e1963 (stable) Promote 2017 Week 40 2017-10-06 14:19:27 -07:00
epriestley
85011a46d0 Bail out of PhabricatorRepositoryGraphCache more aggressively after cache fills
Summary:
Ref PHI109. Ref T11786. We currently test elapsed time every 64 iterations (since iterations are normally very fast), but at least one install is seeing the page timeout after 30 seconds.

One reason could be that cache fills may occur, and are likely to be much slower than normal iterations. In an extreme case, we could do 64 cache fills before checking the time. Tweak thing so that we always check the time after doing a cache fill, regardless of how many iterations have elapsed since the last attempt.

Additionally, this API method currently accepts an arbitrary number of paths, but implicitly limits each cache query to 500ms. If more than 60 paths are passed, this may exceed 30s. Only let the cache churn for a maximum of 10s across all paths.

If this is more the latter issue than the former, this might replace the GraphCache timeouts with `git` timeouts, but at least our understanding of what's going on here will improve.

Test Plan: This is difficult to test convincingly locally, since I can't reproduce the original issue. It still works after these changes, but it worked fine before these changes too.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11786

Differential Revision: https://secure.phabricator.com/D18692
2017-10-06 14:12:58 -07:00
epriestley
17e83b53d5 Add "bin/search query" for debugging query execution
Summary:
Ref T13000. Currently, queries can only be executed from the web UI, which requires logging in as a user. I really want to avoid doing that wherever we can, but being able to execute queries on an instance (and, particularly, see the ngrams and timings on the underlying lookups) would have been helpful in several cases.

Improve tooling a bit in advance of the "common ngrams" stuff going out since it seems likely that it will be useful if issues arise.

Test Plan: Ran `bin/search query --query ...`, got useful minimal output. Ran with `--trace` to get internals.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18690
2017-10-06 08:50:34 -07:00
epriestley
66df5b1493 Add a garbage collector for common ngrams
Summary:
Ref T13000. After an ngram is marked as "common", we can delete it from the storage table.

Currently, the only way to get ngrams marked as "common" is to manually run `bin/search ngrams`, so this has no impact on normal installs.

Test Plan: Ran `bin/garbage collect`, saw it start chewing through my local Maniphest ngrams table and removing common ngrams.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18687
2017-10-05 11:41:18 -07:00
epriestley
c767c971ca Add "persistence" types (data, cache, or index) to tables, and tweak what "storage dump" dumps
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).

By default, `bin/storage dump` dumps data and index tables, but not cache tables.

With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.

Test Plan:
  - Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
  - Verified dump has the same number of `CREATE TABLE` statements as before the changes.
  - Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):

{F5210886}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18682
2017-10-04 12:09:33 -07:00
epriestley
02e1440ef2 Dump tables one at a time, rather than all at once
Summary:
Ref T13000. This allows us to be more selective about which tables we dump data for, to reduce the size of backups and exports. The immediate goal is to make large `ngrams` tables more manageable in the cluster, but this generally makes all backups and exports faster and easier.

Here, tables are dumped one at a time. A followup change will sometimes add the `--no-data` flag, to skip dumping readthrough caches and (optionally) rebuildable indexes.

Test Plan: Compared a dump from `master` and from this branch, found them to be essentially identical. The new dump has a little more header information in each section. Verified each contains the same number of `CREATE TABLE` statements.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18679
2017-10-04 12:08:52 -07:00
epriestley
0ea5d668d1 Enable hovercards for the "Task Graph" UI in Maniphest
Summary: See PHI118. Enables hovercards to support peeking at tags and other details if you, e.g., create numerous identical subtasks of each task.

Test Plan: {F5210816}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18681
2017-10-04 11:12:01 -07:00
epriestley
b9fd526250 Fix a fatal on user email settings when account.editable is disabled
Summary:
If `account.editable` is set to false, we try to add a `null` button and fatal:

> Argument 1 passed to PHUIHeaderView::addActionLink() must be an instance of PHUIButtonView, null given, called in /srv/phabricator/phabricator/src/applications/settings/panel/PhabricatorSettingsPanel.php on line 290

Instead, don't try to render `null` as a button.

Test Plan:
  - Configured `account.editable` false.
  - Viewed email address settings.
  - Before: fatal.
  - After: page works, no button is provided.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18677
2017-10-04 10:16:30 -07:00
Austin McKinley
bc9de7ecee Typo fix
Summary: vive la resistance

Test Plan: doitlive

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18674
2017-10-03 15:10:13 -07:00
epriestley
3e589cdd73 Add a workflow for populating (or depopulating) the common ngrams table
Summary:
Depends on D18672. Ref T13000. This does an on-demand build of the common ngrams table.

Plan here is:

  - Push to `secure`.
  - Build the common ngrams table here.
  - See if stuff breaks?

If it looks okay on this dataset, we can build out the GC support and try it in production.

Test Plan:
  - Locally, my dataset has a bunch of `bin/lipsum` tasks with similar, common words.
  - Verified that ipsum terms now skip ngrams. For "lorem ipsum" search performance actually IMPROVED by skipping the ngrams table (12s to 9s).
  - Queried for normal terms, got very fast results using the ngram table, as normal.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18673
2017-10-03 13:28:19 -07:00
epriestley
1de130c9f5 Allow the Ferret engine to remove "common" ngrams from the index
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.

If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.

In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.

Specifically, I plan to do this:

  - A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
  - A new GC process deletes ngrams that have been added to the common table from the existing indexes.

Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.

Test Plan:
  - Ran some queries and indexes.
  - Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13000

Differential Revision: https://secure.phabricator.com/D18672
2017-10-03 13:27:42 -07:00
epriestley
89fe84f978 Add a "/source/..." URI for Diffusion commits which redirects
Summary:
See PHI112. The install presumably wants to generate links to Diffusion commits from an external tool, but only knows the short name of the repository.

Provide a `/source/phabricator/commit/abcdef908273` URI which redirects to the canonical URI for the commit.

Test Plan:
  - Visited `/source/` URI for a commit, got a redirect.
  - Visited normal URI for a commit, got a commit page.
  - Visited `/branches/` and `/tags/` for a `/source/` repository, got proper pages.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18676
2017-10-03 13:27:03 -07:00
epriestley
4390a274c1 (stable) In "Move Tasks to Column...", show only visible columns
Summary:
See PHI94. I considered this initially but wasn't sure about it. However, PHI94 brings up the good point that we already use a similar rule in Maniphest.

For consistency, only show visible columns here too.

Test Plan: Used "Move tasks to column..." on a board with visible and hidden columns, only saw visbile columns offered in the dropdown.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18668
2017-10-02 11:41:13 -07:00
epriestley
f9110b87ab In "Move Tasks to Column...", show only visible columns
Summary:
See PHI94. I considered this initially but wasn't sure about it. However, PHI94 brings up the good point that we already use a similar rule in Maniphest.

For consistency, only show visible columns here too.

Test Plan: Used "Move tasks to column..." on a board with visible and hidden columns, only saw visbile columns offered in the dropdown.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18668
2017-10-02 11:41:02 -07:00
epriestley
8297f7a182 (stable) Fix transaction queries using withComments() for transactions with no comments
Summary:
See <https://discourse.phabricator-community.org/t/daemons-tasks-crashing-in-a-loop-during-reindex/506/1>. Some object types (for example, Passphrase Credentials) support indexing but not commenting.

Make `withComments(...)` work properly if the transaction type does not support comments.

Test Plan:
Indexed a credential (no comments) and a revision (comments) with `bin/search index --trace ...`.

Before, credential fataled.

After, credetial succeeds, and skips the transaction query.

Before and after, the revision queries the transaction table.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18667
2017-10-02 09:10:07 -07:00
epriestley
cd14194a32 Fix transaction queries using withComments() for transactions with no comments
Summary:
See <https://discourse.phabricator-community.org/t/daemons-tasks-crashing-in-a-loop-during-reindex/506/1>. Some object types (for example, Passphrase Credentials) support indexing but not commenting.

Make `withComments(...)` work properly if the transaction type does not support comments.

Test Plan:
Indexed a credential (no comments) and a revision (comments) with `bin/search index --trace ...`.

Before, credential fataled.

After, credetial succeeds, and skips the transaction query.

Before and after, the revision queries the transaction table.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18667
2017-10-02 09:09:53 -07:00
epriestley
ce9e30b020 (stable) Promote 2017 Week 39 2017-09-29 17:51:50 -07:00
epriestley
5c73c169fd Rough cut of "Move tasks to column..."
Summary:
Ref T5523. See PHI50. See PHI40. This isn't perfect, but should improve things.

Add a "Move tasks to column..." action to workboards which moves all visible tasks in a column to another column, either on the same board or on a different board.

This is a two-step process so that I don't have to write Javascript, and because I'm not 100% sure this is what users actually want/need. If it sticks, the UI could be refined later.

  - The first dialog asks you to choose a project, defaulting ot the current project.
  - The second dialog asks you to choose a column on that project's board.

Test Plan:
  - Moved tasks on the same board.
  - Moved tasks to a different board.
  - Tried to move tasks to the starting column, got a sensible error.
  - Tried to move tasks to no project, got a sensible error.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T5523

Differential Revision: https://secure.phabricator.com/D18665
2017-09-29 17:38:52 -07:00
epriestley
fe646ec328 Mark Owners package reviewers which own nothing in the current diff
Summary:
Ref PHI91. When Owners (or Herald, or manual user action) adds package reviewers to a revision, later updates to the revision make some of them less relevant or irrelevant.

Provide a hint when a package reviewer doesn't own any of the paths that a diff changes. Humans can then decide if the reviewer is obsolete/irrelevant or not.

This is a rough cut to get the feature working, design could probably use some tweaking if it sticks.

Test Plan: {F5204309}

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: jboning

Differential Revision: https://secure.phabricator.com/D18663
2017-09-29 15:06:00 -07:00
epriestley
a39f5e1113 Correct bad context path when doing pattern search inside a repository
Summary:
Ref PHI101. It looks like this was maybe copy/pasted by mistake in recent design refactoring.

We need to pass the full path, not the `basename()` of the path, to the search form.

Test Plan: Searched inside `scripts/test/`, found results inside `scripts/test/`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18664
2017-09-29 14:51:49 -07:00
epriestley
a3a6c4ed2e Fix fatal when searching for "r matey prepare to be boarded"
Summary:
See <https://discourse.phabricator-community.org/t/unrecoverable-fatal-error-on-repository-search-in-top-search-bar/503/2>.

The Ferret engine replaced `withNameContains()`, but I missed this obscure callsite.

Test Plan:
  - Searched for `r matey prepare to be boarded`.
  - Before: fatal.
  - After: no fatal.
  - Also searched for `r <actual repository name>`, got repository.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18661
2017-09-29 09:45:39 -07:00
epriestley
5e276df9c0 (stable) Allow the fulltext index to select only transactions with comments
Summary:
Ref T12997. Although we can't query by transaction type (since we can't easily enumerate all possible types which may have comments -- inline types may also have comments), we //can// just check if there's a comment row or not.

This reduces the amount of garbage we need to load to rebuild indexes for unusual objects with hundreds and hundreds of mentions.

Test Plan:
  - Used batch editor to mention a task 700 times.
  - Indexed it before and after this change, saw index time drop from {nav 1600ms > 160ms}.
  - Made some new comments on it, verified that they still indexed/queried properly.
  - Browsed around, made normal transactions, made inline comments.
  - Added a unique word to an inline comment, indexed revision, searched for word, found revision.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12997

Differential Revision: https://secure.phabricator.com/D18660
2017-09-28 12:55:32 -07:00
epriestley
b75a4151c8 Allow the fulltext index to select only transactions with comments
Summary:
Ref T12997. Although we can't query by transaction type (since we can't easily enumerate all possible types which may have comments -- inline types may also have comments), we //can// just check if there's a comment row or not.

This reduces the amount of garbage we need to load to rebuild indexes for unusual objects with hundreds and hundreds of mentions.

Test Plan:
  - Used batch editor to mention a task 700 times.
  - Indexed it before and after this change, saw index time drop from {nav 1600ms > 160ms}.
  - Made some new comments on it, verified that they still indexed/queried properly.
  - Browsed around, made normal transactions, made inline comments.
  - Added a unique word to an inline comment, indexed revision, searched for word, found revision.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12997

Differential Revision: https://secure.phabricator.com/D18660
2017-09-28 12:55:23 -07:00
epriestley
0d2065e76c (stable) When we purge the request cache, also force PHP to collect cycles
Summary:
Ref T12997. See that task for more details. Briefly, an unusual dataset (where commits are mentioned hundreds of times by other commits) is causing some weird memory behavior in the daemons.

Forcing PHP to GC cycles explicitly after each task completes seems to help with this, by cleaning up some of the memory between tasks. A more thorough fix might be to untangle the `$xactions` structure, but that's significantly more involved.

Test Plan:
  - Did this locally in a controlled environment, saw an immediate collection of a 500MB `$xactions` cycle.
  - Put a similar change in production, memory usage seemed less to improve. It's hard to tell for sure that this does anything, but it shouldn't hurt.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12997

Differential Revision: https://secure.phabricator.com/D18657
2017-09-28 12:37:36 -07:00
epriestley
9875af739f When we purge the request cache, also force PHP to collect cycles
Summary:
Ref T12997. See that task for more details. Briefly, an unusual dataset (where commits are mentioned hundreds of times by other commits) is causing some weird memory behavior in the daemons.

Forcing PHP to GC cycles explicitly after each task completes seems to help with this, by cleaning up some of the memory between tasks. A more thorough fix might be to untangle the `$xactions` structure, but that's significantly more involved.

Test Plan:
  - Did this locally in a controlled environment, saw an immediate collection of a 500MB `$xactions` cycle.
  - Put a similar change in production, memory usage seemed less to improve. It's hard to tell for sure that this does anything, but it shouldn't hurt.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12997

Differential Revision: https://secure.phabricator.com/D18657
2017-09-28 12:37:22 -07:00
epriestley
c034306320 (stable) Improve performance of Ferret engine ngram extraction, particularly for large input strings
Summary:
See PHI87. Ref T12974. The `array_slice()` method of splitting the string apart can perform poorly for large input strings. I think this is mostly just the large number of calls plus building and returning an array being not entirely trivial.

We can just use `substr()` instead, as long as we're a little bit careful about keeping track of where we're slicing the string if it has UTF8 characters.

Test Plan:
  - Created a task with a single, unbroken blob of base64 encoded data as the description, roughly 100KB long.
  - Saw indexing performance improve from ~6s to ~1.5s after patch.
  - Before: https://secure.phabricator.com/xhprof/profile/PHID-FILE-nrxs4lwdvupbve5lhl6u/
  - After: https://secure.phabricator.com/xhprof/profile/PHID-FILE-6vs2akgjj5nbqt7yo7ul/

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18649
2017-09-27 10:41:50 -07:00
epriestley
086a125ad5 Improve performance of Ferret engine ngram extraction, particularly for large input strings
Summary:
See PHI87. Ref T12974. The `array_slice()` method of splitting the string apart can perform poorly for large input strings. I think this is mostly just the large number of calls plus building and returning an array being not entirely trivial.

We can just use `substr()` instead, as long as we're a little bit careful about keeping track of where we're slicing the string if it has UTF8 characters.

Test Plan:
  - Created a task with a single, unbroken blob of base64 encoded data as the description, roughly 100KB long.
  - Saw indexing performance improve from ~6s to ~1.5s after patch.
  - Before: https://secure.phabricator.com/xhprof/profile/PHID-FILE-nrxs4lwdvupbve5lhl6u/
  - After: https://secure.phabricator.com/xhprof/profile/PHID-FILE-6vs2akgjj5nbqt7yo7ul/

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18649
2017-09-27 10:41:39 -07:00
epriestley
9288cad0ed (stable) Improve Ferret engine indexing performance for large blocks of text
Summary:
See PHI87. Ref T12974. Currently, we do a lot more work here than we need to: we call `phutil_utf8_strtolower()` on each token, but can do it once at the beginning on the whole block.

Additionally, since ngrams don't care about order, we only need to convert unique tokens into ngrams. This saves us some `phutil_utf8v()`. These calls can be slow for large inputs.

Test Plan:
  - Created a ~4MB task description.
  - Ran `bin/search index Txxx --profile ...` to profile indexing performance before and after the change.
  - Saw total runtime drop form 38s to 9s.
  - Before: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-wiht5d7lkyazaywwxovw/>
  - After: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-efxv56q2hulr6kjrxbx6/>

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18647
2017-09-27 10:25:09 -07:00
epriestley
a1d9a2389d Improve Ferret engine indexing performance for large blocks of text
Summary:
See PHI87. Ref T12974. Currently, we do a lot more work here than we need to: we call `phutil_utf8_strtolower()` on each token, but can do it once at the beginning on the whole block.

Additionally, since ngrams don't care about order, we only need to convert unique tokens into ngrams. This saves us some `phutil_utf8v()`. These calls can be slow for large inputs.

Test Plan:
  - Created a ~4MB task description.
  - Ran `bin/search index Txxx --profile ...` to profile indexing performance before and after the change.
  - Saw total runtime drop form 38s to 9s.
  - Before: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-wiht5d7lkyazaywwxovw/>
  - After: <https://secure.phabricator.com/xhprof/profile/PHID-FILE-efxv56q2hulr6kjrxbx6/>

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12974

Differential Revision: https://secure.phabricator.com/D18647
2017-09-27 08:15:40 -07:00
epriestley
7ae4d93043 (stable) Promote 2017 Week 38 2017-09-22 11:48:44 -07:00
epriestley
1ac52c09e7 Improve search highlighting for CJK and substring queries
Summary:
Fixes T12995. Currently, the result highlighter (which shows //where// terms matched) only works in "term" mode, not in "substring" mode.

Provide better feedback and behvaior:

  - When a term is a substring term, color it a little differently and add a tooltip. (This is partly to make it easier to debug/diagnose things, probably not enormously valuable to users.)
  - When a term is a substring term, highlight it anywhere in the results.

Test Plan:
Queried for latin and CJK terms.

Here is CJK being highlighted:

{F5192195}

Here is substring vs non-substring implicit behavior:

{F5192196}

Here's ONLY terms being highlighted:

{F5192198}

Here's terms and substrings, since the query now has a substring:

{F5192201}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12995

Differential Revision: https://secure.phabricator.com/D18635
2017-09-22 11:34:46 -07:00
epriestley
36df39761e Create revisions into "Draft", publish them when builds finish
Summary:
Ref T2543. This doesn't stand alone since mail still goes out normally, but gets this piece working: new revisions start as "Draft", then after updates if there are no builds they go into "Needs Review".

This should work in general because builds update revisions when they complete, to publish a "Harbormaster finished build yada yada" transaction. So either we'll un-draft immediately, or un-draft after the last build finishes.

I'll hold this until the mail and some other stuff (like UI hints) are in slightly better shape since I think it's probably too rough on its own.

Test Plan: Created revisions locally, saw them un-draft after builds.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18628
2017-09-21 07:21:21 -07:00
epriestley
fca553f142 Prepare revision mail for the "Draft" status
Summary:
Ref T2543. Currently, we always do some special things when a revision is created, mostly adding more stuff to the mail.

With drafts, we want to suppress initial mail and send this big, rich mail only when the revision actually moves out of "draft".

Prepare the code for this, with the actual methods hard-coded to the current behavior. This will probably take some tweaking but I think I got most of it.

Test Plan: Banged around in Differential so it sent some mail, saw normal mail without anything new.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18627
2017-09-21 07:21:07 -07:00
epriestley
c7af663523 Align most revision actions to the new "Draft" state
Summary:
Ref T2543. Most actions are not available for drafts.

Authors can "Request Review" (move out of draft to become a normal revision) or "Abandon".

Non-authors can't do anything (maybe we'll let them do something later -- like "Commandeer"? -- if there's a good reason).

Test Plan: Viewed a draft revision as an author and non-author, saw fewer actions available.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18626
2017-09-21 07:20:48 -07:00
epriestley
f089f41e50 (stable) Update an old SSH redirect URI when editing a bot's SSH keys
Summary: See PHI79. When you edit another user's SSH keys (normally, for a bot account) we currently redirect you to an older URI.

Test Plan:
  - Viewed a bot's profile page.
  - Clicked "Edit Settings" on the Manage page.
  - Went to "SSH Keys".
  - Uploaded an SSH key.
  - Before: redirected to a 404 after finishing the workflow.
  - After: redirected to the same page after the workflow.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18633
2017-09-20 14:46:43 -07:00
epriestley
5112dac491 Update an old SSH redirect URI when editing a bot's SSH keys
Summary: See PHI79. When you edit another user's SSH keys (normally, for a bot account) we currently redirect you to an older URI.

Test Plan:
  - Viewed a bot's profile page.
  - Clicked "Edit Settings" on the Manage page.
  - Went to "SSH Keys".
  - Uploaded an SSH key.
  - Before: redirected to a 404 after finishing the workflow.
  - After: redirected to the same page after the workflow.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18633
2017-09-20 14:46:31 -07:00
epriestley
3fbad684c1 More completely explain why we're refusing to send reset mail to an unverified address
Summary:
See PHI78. The user was getting this message and (reasonably) interpreted it to mean "reset mail can never be sent to unverified addresses".

Reword it to be more clear, albeit an entire paragraph long. I don't really have a good solution in these cases where we'd need a whole page to explain what's happening (this, plus "we can't tell you which address you should use because an attacker could get information if we did" and "this rule defuses the risk that an opportunistic attacker may try to compromise your account after you add an email you don't own by mistake"). We could write it up separately and link to it, but I feel like that stuff tends to get out of date.

Just land somewhere in the middle.

Test Plan: {F5189105}

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18630
2017-09-20 10:46:22 -07:00
epriestley
52fff638ca (stable) Fix an error in Diffusion when the Owners application is uninstalled
Summary:
See <https://discourse.phabricator-community.org/t/undefined-view-when-owners-is-uninstalled/451>.

When Owners is not installed, Diffusion can fatal with a bad `$view`.

Test Plan:
  - Uninstall Owners.
  - View the content of any file in Diffusion.
  - Before: fatal on `$view` undefined.
  - After: Valid page with no owners information.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18629
2017-09-19 09:42:57 -07:00
epriestley
03e5d69817 Fix an error in Diffusion when the Owners application is uninstalled
Summary:
See <https://discourse.phabricator-community.org/t/undefined-view-when-owners-is-uninstalled/451>.

When Owners is not installed, Diffusion can fatal with a bad `$view`.

Test Plan:
  - Uninstall Owners.
  - View the content of any file in Diffusion.
  - Before: fatal on `$view` undefined.
  - After: Valid page with no owners information.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18629
2017-09-19 09:42:42 -07:00
epriestley
23867c1487 Add a "Draft" state for revisions, and action bucket support
Summary:
Ref T2543. There's no way to put revisions into this state yet, but start adding support for when there is.

Adds the status constant, plus support for bucketing them.

Test Plan:
  - Manually put a revision in "Draft" state by updating the database directly.
  - Verified my drafts showed up in a "Drafts" section on the bucket view.
  - Verified others' drafts did not appear on the action bucket view.
  - Viewed revisions, queried for "Draft" revisions, etc (stuff we get for free).

{F5186781}

{F5186782}

{F5186783}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T2543

Differential Revision: https://secure.phabricator.com/D18625
2017-09-18 14:01:00 -07:00
epriestley
156adccef0 Fix an issue where "bin/differential migrate-hunk" could decompress data
Summary:
Fixes T12986. I caught this bug in the changes from D18584: when we moved a large hunk to file storage, we would decompress it but keep the "deflated" flag. This could cause confusion when loading it later. I missed this in testing since I wasn't exhaustive enough in checking hunks and didn't run into a compressed one.

Instead of compressing on `save()`, compress during the normal workflow.

We currently never advise users to run this workflow so I didn't bother trying to clean up possible existing migrations.

Test Plan:
  - Ran `bin/differential migrate-hunk` on compressed hunks, moving them to and from file storage. Saw them work correctly and remain compressed.
  - Created new small (uncompressed) and large (compressed) hunks, verified they work properly and get compressed (if applicable).
  - Used `bin/cache purge --caches changeset` to clear changeset caches and make sure the actual table was being hit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12986

Differential Revision: https://secure.phabricator.com/D18624
2017-09-18 14:00:41 -07:00
epriestley
94ab0c9afb Spell "Relevance" correctly
Summary: Despite how I (and everyone else?) pronounce it, it is spelled with an "a". See PHI38.

Test Plan: Googled both spellings.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18622
2017-09-18 09:36:55 -07:00
epriestley
3f3ce88c7b (stable) Fix "Author's projects" Herald rules for revisions and diffs
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.

Also fix a bad typeahead datasource.

Test Plan:
  - Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
  - Tried to set "Author's projects" to a user, no longer could.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18619
2017-09-15 18:00:00 -07:00
epriestley
51b810b0eb Fix "Author's projects" Herald rules for revisions and diffs
Summary:
See PHI71. These didn't get properly updated when we wrote Subprojects and Milestones, and should use materialized members, not raw members. Swap the query so projects you are an indirect member of (e.g., milestones you are a member of the parent for, and parent projects you are a member of a subproject of) are included in the result list.

Also fix a bad typeahead datasource.

Test Plan:
  - Ran a dry run with the test console, saw project PHIDs for milestones and parent projects in the raw field value.
  - Tried to set "Author's projects" to a user, no longer could.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18619
2017-09-15 17:59:49 -07:00
epriestley
76cdaa4f2d (stable) Swap "-R" and "serve" argument order for Mercurial
Summary: See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391/13>. I missed that `-R` is order-sensitive.

Test Plan: Verified both orders work on 3.5.2.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18616
2017-09-15 13:07:53 -07:00
epriestley
b352cacdd9 Swap "-R" and "serve" argument order for Mercurial
Summary: See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391/13>. I missed that `-R` is order-sensitive.

Test Plan: Verified both orders work on 3.5.2.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18616
2017-09-15 13:07:43 -07:00
epriestley
49b7181780 Update utility "bin/repository parents" workflow to work with RefPosition
Summary:
Ref T11823. I think this is the last callsite which relies on the old data format: `bin/repository parents` rebuilds a cache which we don't currently use very heavily.

Update it to work with the new data.

Test Plan: Ran `bin/repository parents <repository> --trace`, saw successful script execution and reasonable-looking output.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18615
2017-09-15 10:21:51 -07:00
epriestley
8982e3e52d Update major RefCursor callsites to work properly with RefPosition
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.

I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.

Test Plan:
  - This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
  - Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
  - Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
  - Browed around Diffusion in various repositories.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18614
2017-09-15 10:21:32 -07:00
epriestley
5cf62f86d7 Remove obsolete columns from RefCursor table
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.

This data was moved to the RefPosition table in D18612.

Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18613
2017-09-15 10:21:12 -07:00
epriestley
9d5a2b3b4f Add a RefPosition table to hold branch/tag positions once the RefCursor table is split
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.

Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.

Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.

Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.

(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)

Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18602
2017-09-15 10:19:17 -07:00
epriestley
dc22aba9df (stable) Promote 2017 Week 37 2017-09-15 10:06:22 -07:00
epriestley
bd923d1ce0 Provide an explicit "-R" flag to "hg serve"
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391>.

The Mercurial commit is helpful in particular: <https://www.mercurial-scm.org/repo/hg/rev/77eaf9539499>

We weren't vulnerable to the security issue (users can not control any part of the command) but pass the working directory explicitly to get past the new safety check.

I left `setCWD()` in place (a few lines below) just because it can't hurt, and in some other contexts it sometimes matter (for example, if commit hooks execute, they might inherit the parent CWD here or in other VCSes).

Test Plan:
  - Cloned from a Mercurial repo locally over HTTP.
  - Verified that SSH cloning already uses `-R` (it does, see `DiffusionMercurialServeSSHWorkflow`).
  - Did not actually upgrade to Mercurial 4.0/4.1.3 to completely verify this, but a user in the Discourse thread asserted that a substantially similar fix worked correctly.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18611
2017-09-15 08:57:11 -07:00
epriestley
5ae3af6691 Fix an outdated HTML anchor link in Diffusion table of contents
Summary:
See <https://discourse.phabricator-community.org/t/navigating-to-changed-files-in-diffusion-does-not-work-anymore/433>.

In D18465, I updated these but this hard-coded the anchor for some reason (???) and I missed it while `grep`-ing.

Test Plan: Viewed a commit (`/rXYZaaaa`) and clicked a file link in the table of contents. Got modern `#change-...` anchor and navigation into the document.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D18609
2017-09-15 08:37:36 -07:00