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

15366 commits

Author SHA1 Message Date
epriestley
81a7c9ac43 Remove the execution time limit (if any) before sinking HTTP responses
Summary:
Ref T12907. At least part of the problem is that we can hit PHP's `max_execution_time` limit on the file download pathway.

We don't currently set this to anything in the application itself, but PHP often sets it to 30s by default (and we have it set to 30s in production).

When writing responses, remove this limit. This limit is mostly a protection against accidental loops/recurison/etc., not a process slot protection. It doesn't really protect process slots anyway, since it doesn't start counting until the request starts executing, so you can (by default) //send// the request as slowly as you want without hitting this limit.

By releasing the limit this late, hopefully all the loops and recursion issues have already been caught and we're left with mostly smooth sailing.

We already remove this limit when sending `git clone` responses in `DiffusionServeController` and nothing has blown up. This affects `git clone http://` and similar.

(I may have had this turned off locally and/or just be too impatient to wait 30s, which is why I haven't caught this previously.)

Test Plan:
  - Poked around and downloaded some files.
  - Will `curl ...` in production and see if that goes better.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T12907

Differential Revision: https://secure.phabricator.com/D19547
2018-07-30 10:56:05 -07:00
epriestley
9cf3b3bbf8 Count lines in build log slices more cheaply
Summary:
See PHI766. Ref T13164. Build log chunk processing does a `preg_split()` on slices, but this isn't terribly efficient.

We can get the same count more cheaply by just using `substr_count()` a few times.

(I also tried `preg_match_all()`, which was between the two in speed.)

Test Plan:
- Used `bin/harbormaster rebuild-log --id X --force` to rebuild logs. Verified that the linemap is identical before/after this change.
- Saw local time for the 18MB log in PHI766 drop from ~1.7s to ~900ms, and `preg_split()` drop out of the profiler (we're now spending the biggest chunk of time on `gzdeflate()`).

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19545
2018-07-30 08:25:17 -07:00
epriestley
690a460c8e Allow mailers to be explicitly marked as inbound or outbound
Summary:
See PHI785. Ref T13164. In this case, an install wants to receive mail via Mailgun, but not configure it (DKIM + SPF) for outbound mail.

Allow individual mailers to be marked as not supporting inbound or outbound mail.

Test Plan:
  - Added and ran unit tests.
  - Went through some mail pathways locally, but I don't have every inbound/outbound configured so this isn't totally conclusive.
  - Hit `bin/mail send-test` with a no-outbound mailer.
  - I'll hold this until after the release cut so it can soak on `secure` for a bit.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19546
2018-07-30 08:25:06 -07:00
epriestley
9e451879d9 Add a "Changes Since Last Action" view to Differential revisions
Summary:
Ref T13151. See PHI616. Fixes T8163.

This adds `/D123/new/`, which shows the changes to the revision since the last timeline action you took.

It also adds a link to this view to diff update emails.

Test Plan:
  - Followed this link with a recent comment and no touches since update, ended up with sensible diff selections.
  - Updated revision, generated email, saw an appropriate link.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151, T8163

Differential Revision: https://secure.phabricator.com/D19541
2018-07-27 12:27:33 -07:00
epriestley
a5d3aea67c Carry the "silent" transaction flag through inverse edge edits
Summary:
See PHI751. Ref T13164. We added a "silent" flag for Editors somewhat recently (currently reachable only for bulk edits with `bin/bulk ...` command).

However, this flag doesn't carry through to the sub-editor when we make inverse edge edits. These are edits like "X is a parent of Y", which cause an implicit "Y is a child of X" edit to occur.

Pass the flag through.

Test Plan:
  - Rigged the relationships controller to make silent edits.
  - Changed the parents of a revision from the web UI. Saw no mail or feed stories.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19543
2018-07-27 12:27:16 -07:00
epriestley
cb99396c64 Make the "Is this JSON?" DocumentEngine heuristic a little tighter
Summary:
See PHI749. Ref T13164. We currently misdetect files starting with `[submodule ...` as JSON.

Make this a bit stricter:

  - If the file is short, just see if it's actually literally real JSON.
  - If the file is long, give up.

This should get the right result in pretty much all the cases people care about, I think. We could make the long-file guesser better some day.

Test Plan: Detected a `[submodule ...` file (no longer JSON) and a `{"duck": "quack"}` file (still JSON).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13164

Differential Revision: https://secure.phabricator.com/D19544
2018-07-27 12:27:02 -07:00
epriestley
727bc2234c Capitalize "OPcache" more consistently
Summary: Fixes T13174. PHP spells this "OPcache" (lowercase "c"); we're inconsistent. Be more consistent.

Test Plan:
  - `git grep OPCache`
  - `git grep -i opcache | grep -v opcache | grep -v OPcache`

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13174

Differential Revision: https://secure.phabricator.com/D19538
2018-07-26 12:15:57 -07:00
epriestley
9e1a1577c3 Make the meme cache case-sensitive
Summary:
Fixes T13172. At one point we always capitalized all the text, and the cache uses capitalized text.

However, we stopped capitalizing the text at some point. Modern memes are more more subtle than old memes, and when we eventualy add support for things like "explodey brain" we'll certainly want to support mixed case.

Practically, this stops you from changing the capitalization of a cached meme. Get rid of the cache transform.

Test Plan:
none lul

(I don't have `gd` installed locally and buiding it requires building libjpeg and libpng or giving up and using `brew`. I'l vet this in production.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13172

Differential Revision: https://secure.phabricator.com/D19537
2018-07-26 12:15:32 -07:00
epriestley
682c3bc9ee When migrating files between storage engines with "bin/files migrate ...", skip expired temporary files
Summary:
See T7148. This just cheats us out of a weird sort of race where we:

  - Dump an instance, including some `F123` which is a temporary file which expires in 3 minutes.
  - A few minutes later, the daemons delete the data for that file.
  - A few minutes after that, we try to `bin/files migrate --copy` to copy the data from S3 into the MySQL blob store.
  - This fails since the data is already gone.

Instead, just skip these files since they're already dead to us.

Test Plan: Faked this locally, will migrate the PHI769 instance on `aux001`.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19536
2018-07-26 06:22:23 -07:00
Austin McKinley
ee7879e626 Change bin/storage destroy to be less scary when removing test data
Summary: I've pulled up this code probably three different times to make sure that the big scary warning does, in fact, still get printed even when passing `--unitest-fixtures` to `bin/storage destroy`. Make the warning message less scary if only removing test data.

Test Plan: Ran with and without `--unitest-fixtures` and saw expected warnings. After agreeing to warnings, test data was deleted as expected. Did not test `bin/storage destroy` without `--unittest-fixtures`.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19535
2018-07-25 12:14:41 -07:00
Kenneth Endfinger
6bdd74584e
Fix file encoding migration
Summary:
See [[ https://discourse.phabricator-community.org/t/file-encryption-corruption-when-trying-to-encode-existing-files/1605 | Discourse ]]

When migrating to aes-256-cbc, integrity hashes were not updated, so data was not properly

Test Plan:
I ran [[ https://gist.github.com/kaendfinger/3e0d78350af0ebe4e74b2c8a79707bae | this test script ]] to ensure it worked.
I created some files with lipsum, ensured that after encoding them with aes-256-cbc, they were not able to be cat'd.
After applying this patch and rerunning the script, it worked successfully.

Reviewers: epriestley, amckinley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Tags: #files, #storage

Differential Revision: https://secure.phabricator.com/D19533
2018-07-23 17:41:10 -05:00
Kenneth Endfinger
6b6e1f0ba8
Fix Lipsum generators for Differential Revisions and Pastes
Summary:
When generating test data to solve a bug I have encountered, I noticed Lipsum was not working correctly for Differential Revisions and Pastes.

It seemed like they weren't updated after some refactoring. This fixes that by updating them.

Test Plan: Run Lipsum for all objects, and note that it has much less failure.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D19534
2018-07-23 15:05:51 -05:00
epriestley
77d7bb7af0 Document the Ferret "=" operator and improve related documentation
Summary:
Depends on D19529. See PHI778.

  - Document the "name" constraint as deprecated. All callers are likely better served by the "query" constraint.
  - Guide users toward the "query" constraint a little better.
  - Document the `=` syntax.

Test Plan: Read various new documentation.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19531
2018-07-23 12:44:43 -07:00
epriestley
71d4fa41c9 Support the Ferret "=" (exact match) operator in the actual query engine
Summary:
Ref PHI778. In D18492, I added support for parsing this operator, but did not actually implement it in the query engine.

Implementation is fairly straightforward. This supports querying for objects by exact title with `title:="exact title"`. This is probably a bad idea, but sometimes maybe useful anyway.

Test Plan: Queried for `title:="xxx"`, found only exact matches.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: ahoffer2

Differential Revision: https://secure.phabricator.com/D19529
2018-07-23 12:44:00 -07:00
epriestley
dee453c94d Give Config the "" (SPARKLE LIKE NEW) emoji instead of "☺" (STUPID LOOKING FACE)
Summary: Fixes T13171. Open to suggestions but that face looks real, real dumb on High Sierra.

Test Plan: Visited Config, saw a serious professional emoji in the page title.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13171

Differential Revision: https://secure.phabricator.com/D19530
2018-07-23 12:43:25 -07:00
epriestley
dce6dd5d02 Add an explicit "null" to a missed diffusion.branchquery callsite to fix Diffusion "Branches" page
Summary:
See PHI775. See D19499. Originally, see PHI720.

D19499 broke the standalone "Branches" page for commits. Normally, you reach this by taking these steps:

  - View a commit which is contained by 11 or more branches.
  - Click the "More Branches..." link in the "Branches" field.
  - You should be taken to a list of all branches which contain the commit.

The change to the 'branch' parameter was adjusted in the query that builds the "x, y, z, More Branches..." list, but not on the actual "Branches" list with the full list. Adjust it.

Test Plan:
  - Set display limit to 1, viewed a commit on "master" and "stable", clicked "More Branches".
  - Before: saw only "master".
  - After: saw both "master" and "stable".

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19532
2018-07-23 11:21:11 -07:00
epriestley
96a8b89fa8 When building a config stack, stop SiteSource objects from poisoning the cache
Summary:
Ref T13168. I'm not sure how this worked before, but I ran into this issue on my new laptop.

SiteSource accesses `PhabrictatorEnv::getEnvConfig('phabricator.base-uri')` when local, which may poison the cache and lock the value since we don't later discard the cache.

Specifically, when I access `http://locala.phacility.com`, I was getting an error like "You made a request for locala.phacility.com, but no configured site can serve this request.". This was because the base-uri was being incorrectly frozen as "local.phacility.com". The expectation is that it will match, so the standard PlatformSite will serve the request.

Test Plan:
  - Before change: "no configured site" error.
  - After change: local instance works properly.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13168

Differential Revision: https://secure.phabricator.com/D19526
2018-07-20 16:07:41 -07:00
epriestley
eb80a5ede1 Make the Conduit auth error for an unrecognized public key a little more useful
Summary: Ref T13168. This is just a small quality-of-life fix: we can disclose which public key we're talking about because public keys are public.

Test Plan:
  - Hit public key error (through my own bumbling / not reading or following instructions). Specifically, I haven't associated the key with a device in Almanac.
  - Before: vague error.
  - After: more specific error with enough key material that I could grep for it.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: yelirekim

Maniphest Tasks: T13168

Differential Revision: https://secure.phabricator.com/D19516
2018-07-20 09:43:54 -07:00
Austin McKinley
67283c7a45 Add test plan to differential.revision.search
Summary: Ref T13151. Ref PHI622.

Test Plan: Loaded a revision in the Conduit UI; observed presence of `testPlan` field.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19518
2018-07-20 08:40:27 -07:00
epriestley
185c28f307 Update parent/child revision timeline messages to use modern language ("parent revision")
Summary:
See PHI746. See also T11833, perhaps. Ref T13151.

Long ago, parent revisions were called "dependent revisions". This was changed to "parent revisions" in the action UI to improve clarity, but not changed in the timeline stories.

Update the timeline stories to use the same language the actions in the UI use.

Test Plan:
{F5732876}

{F5732877}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19514
2018-07-13 09:02:10 -07:00
epriestley
4214b56a4f Make the dashboard panel datasource work properly with hundreds of panels
Summary:
Ref T13151. See PHI727. Update the dashboard widget/panel datasource to actually query results using what the user typed.

The current approach is blind to what the user typed when pulling results from the database, and gets limited to an artificially small number of results somewhere in the pipeline.

Test Plan:
  - Queried for panels with text queries.
  - Queried for panels with `W123` queries.
  - This is substantially similar to the Owners datasource, which received a similar update in D17142 and has worked well since then.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19511
2018-06-28 08:54:29 -07:00
epriestley
ca1349ab45 Add an explicit example of constraining "transaction.search" to the webhook documentation
Summary:
Depends on D19509. Ref T13151. See PHI725. `transaction.search` supports "constraints" and the documentation mentions it in passing, but doesn't really show how to do it, and the method is not automatically self-documenting because it isn't a "real" `*.search` method.

Add an example of how to use the `contraints` parameter to retrieve information about only the relevant transactions.

Also remove a refernece to `requestb.in`, which is now defunct.

Test Plan: Called `transaction.search` with similar parameters.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19510
2018-06-28 08:52:16 -07:00
epriestley
a94528ee4a Expose Differential actions for "transaction.search" in a basic way
Summary:
See PHI725. Ref T13151. These actions are somewhat unusual and I considered different ways to represent them (make them look like "status" transactions; build multiple synthetic transactions) but ultimately landed on the simplest approach of just exposing them more or less as they exist internally.

I haven't included data for any of them. Most don't really have any data, but "accept" does. I'm holding off on providing more data until after T731, which may shake up the internal format.

Test Plan: Applied most of these transactions against a revision, queried for it with `transaction.search`, got distinguishable transactions out.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19509
2018-06-28 08:51:55 -07:00
Alex Vandiver
11f1c13915 Append the intermediate chain to the "cert" parameter in Aphlict
Summary:
Per the documentation[1], any intermediate chain is to be
appended to the "cert" parameter.  The "ca" parameter controls the
root CA used to authenticate the client certificate, if one is
provided, and is not used for intermediate certificate chains -- nor
has it ever been.  It is not clear how this could have worked in the
past[2].

[1] https://nodejs.org/api/tls.html#tls_tls_createsecurecontext_options
[2] D15709

Test Plan:
Before this diff, with node 4.2.6 from Ubuntu packages:
```
$ openssl s_client -connect phabricator.dropboxer.net:22280 -verify 5 -CApath /etc/ssl/certs/
verify depth is 5
CONNECTED(00000003)
depth=0 C = US, ST = California, L = San Francisco, O = "Dropbox, Inc", OU = Dropbox Ops, CN = phabricator.dropboxer.net
verify error:num=20:unable to get local issuer certificate
verify return:1
depth=0 C = US, ST = California, L = San Francisco, O = "Dropbox, Inc", OU = Dropbox Ops, CN = phabricator.dropboxer.net
verify error:num=27:certificate not trusted
verify return:1
depth=0 C = US, ST = California, L = San Francisco, O = "Dropbox, Inc", OU = Dropbox Ops, CN = phabricator.dropboxer.net
verify error:num=21:unable to verify the first certificate
verify return:1
---
Certificate chain
 0 s:/C=US/ST=California/L=San Francisco/O=Dropbox, Inc/OU=Dropbox Ops/CN=phabricator.dropboxer.net
   i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert SHA2 High Assurance Server CA
```

After:
```
$ openssl s_client -connect phabricator.dropboxer.net:22280 -verify 5 -CApath /etc/ssl/certs/
verify depth is 5
CONNECTED(00000003)
depth=2 C = US, O = DigiCert Inc, OU = www.digicert.com, CN = DigiCert High Assurance EV Root CA
verify return:1
depth=1 C = US, O = DigiCert Inc, OU = www.digicert.com, CN = DigiCert SHA2 High Assurance Server CA
verify return:1
depth=0 C = US, ST = California, L = San Francisco, O = "Dropbox, Inc", OU = Dropbox Ops, CN = phabricator.dropboxer.net
verify return:1
---
Certificate chain
 0 s:/C=US/ST=California/L=San Francisco/O=Dropbox, Inc/OU=Dropbox Ops/CN=phabricator.dropboxer.net
   i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert SHA2 High Assurance Server CA
 1 s:/C=US/ST=California/L=San Francisco/O=Dropbox, Inc/OU=Dropbox Ops/CN=phabricator.dropboxer.net
   i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert SHA2 High Assurance Server CA
 2 s:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert SHA2 High Assurance Server CA
   i:/C=US/O=DigiCert Inc/OU=www.digicert.com/CN=DigiCert High Assurance EV Root CA
```

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Differential Revision: https://secure.phabricator.com/D18181
2018-06-26 19:34:28 +00:00
epriestley
f94cee8628 Fix querying for transactions over "transaction.search" when the object does not support comments
Summary: See PHI725. Ref T13151. We currently try to load comments unconditionally, but not all objects (like projects) have comments. Only try to load comments if an object actually has comments.

Test Plan: Queried for an object with no comments, like project `#masonry`, via `transaction.search`.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19507
2018-06-26 07:59:01 -07:00
epriestley
cac3dc4983 Give "create" transactions a readable type in "transaction.search"
Summary:
Ref T13151. See PHI725. By default, "transaction.search" doesn't provide details about transactions because many have bad/weird/policy-violating internal types or fields.

The "create" transaction is simple and straightforward, so label it to allow callers to distinguish it.

Test Plan:
  - Created a new task.
  - Called `transaction.search` on it.
  - Saw the labelled "create" transaction.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: swisspol

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19505
2018-06-22 17:42:35 -07:00
epriestley
d84f866ca0 When search indexers contend for a lock, just yield
Summary:
Depends on D19503. Ref T13151. See PHI719. If you have something like a script which updates an object in a loop, we can end up queueing many search reindex tasks.

These tasks may reasonably contend for the lock, especially if the object is larger (lots of text and/or lots of comments) and indexing takes a few seconds.

This isn't concerning, and the indexers should converge to good behavior quickly once the updates stop.

Today, they'll spew a bunch of serious-looking lock exceptions into the log. Instead, just yield so it's more clear that there's (normally) no cause for concern here.

Test Plan: Ran `bin/search index Txxx --force` on a large object in multiple windows with a 0 second lock, saw an explicit yield instead of a lock exception.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19504
2018-06-22 17:41:45 -07:00
epriestley
14e911a0d8 Index only the first 1,000 comments on any object
Summary:
Depends on D19502. Ref T13151. See PHI719. An install ended up with an object with 111,000+ comments on it because someone wrote a script to treat it like a logfile.

Although we seem to do mostly okay with this (locally, it only takes about 30s to index a similar object) we'll hit a wall somewhere (since we need to hold everything in memory), and it's hard to imagine a legitimate object with more than 1,000 comments. Just ignore comments past the first thousand.

(Conpherence threads may legitimately have more than 1,000 comments, but go through a different indexer.)

Test Plan:
  - Piped some comments into `maniphest.edit` in a loop to create a task with 100K comments.
  - Ran `bin/search index Txxx --force` to reindex it, with `--trace`.
    - Before: task indexed in about 30s.
    - After: script loaded comments with LIMIT 1000 and indexed in a couple seconds.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19503
2018-06-22 17:41:05 -07:00
epriestley
cbc71e75fa When queueing search index tasks, include the "objectPHID" in the task metadata
Summary:
Ref T13151. See PHI719. One minor hiccup in debugging the issue (which ended up being "revision has 100K comments") was that the `SearchWorker` did not show which object it was indexing.

Add `'objectPHID'` to the queue call so you can see which object is affected from the web UI.

Test Plan:
  - Stopped daemons.
  - Used `bin/search index D123 --background` to queue a search task.
  - Viewed task details in web UI from `/daemon/`.
    - Before change: no indication of which object was being indexed.
    - After change: page helpfully shows that the task is indexing D123.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19502
2018-06-22 17:40:32 -07:00
epriestley
b1f4a974fe Fix two minor breadcrumb issues in Config
Summary:
Fixes T13159. Two issues here:

  - When viewing a particular config setting, there's an extra "Config" crumb.
  - On the page for a config group, the link to the parent group has an extra "/config/" in it.

Test Plan:
  - Viewed a page for a particular setting, no longer saw an extra "Config" crumb.
  - Viewed a page for a setting group, clicked parent crumb, got taken to a real page.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13159

Differential Revision: https://secure.phabricator.com/D19501
2018-06-22 17:38:50 -07:00
epriestley
8ab8c390b7 If "branch" is provided to "diffusion.branchquery", use it as the "<pattern>" argument to "git branch --contains ..."
Summary:
Ref T13151. See PHI720. If you want to test if commit X appears on specific branch Y, `git branch --contains X -- Y` is faster than (effectively) `git branch --contains X | grep Y`.

Since this call has a "branch" parameter anyway, use it as the pattern argument if provided.

Test Plan:
  - Called the API method with no parameters, got all branches.
  - Called the API method with `master`, got just master.
  - Called the API method with `maste*`, got master. This behavior is not officially supported and may change in the future.
  - Viewed a commit, still saw all branches.
    - Grepped for `diffusion.branchquery` and verified that no remaining callsites pass a default "branch" parameter.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19499
2018-06-22 17:38:19 -07:00
epriestley
6136b83275 Fix changeset construction special case for empty commits in pre-commit hooks
Summary: Fixes T13155. Ref T13151. A recent change (D19455) changed the return format here, but I missed this special case for empty commits.

Test Plan:
  - T13155 has a good set of reproduction instructions.
  - Pushed an empty commit.
    - Before: bunch of warning log spew.
    - After: clean logs.

Reviewers: amckinley, avivey

Reviewed By: avivey

Maniphest Tasks: T13155, T13151

Differential Revision: https://secure.phabricator.com/D19500
2018-06-21 16:43:20 -07:00
Austin McKinley
9db5ad3476 Allow null identities to be attached to commit objects
Summary: I landed D19491 a little aggressively, so allow this field to be null until after the migration goes out.

Test Plan: Loaded commits without identity objects; did not get any errors.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19496
2018-06-20 08:35:36 -07:00
Austin McKinley
05f333dfba Attach identities to commits and users to identities
Summary: Ref T12164. Make it easier to work with identity objects by attaching them to commits and attaching users to identities.

Test Plan: Loaded some commits with `->needIdentities(true)` and checked the resulting objects.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12164

Differential Revision: https://secure.phabricator.com/D19491
2018-06-18 15:31:41 -07:00
Austin McKinley
787c59744b Correctly attach users to identities
Summary: This never worked.

Test Plan: Ran `bin/repository rebuild-identities` and viewed identity objects with `currentEffectiveUserID`s and no longer got errors about attempting to attach `null` objects instead of `PhabricatorUser` objects.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19495
2018-06-18 15:21:11 -07:00
epriestley
a7c681b549 Don't set mail HTML bodies if there's no actual HTML body
Summary:
See <https://discourse.phabricator-community.org/t/commit-6011085b0fcd-breaks-sending-certain-email/1571>. Some mailers get upset if we `setHTMLBody(...)` with an empty string.

There's some possible argument they should be more graceful about this, but it's reasonably pretty ambiguous.

Only try to set the HTML body if we actually have a nonempty HTML body.

Test Plan:
  - Configured an "smtp" mailer.
  - Ran `echo hi | ./bin/mail send-test --to someone@somewhere.com --subject test`.
  - Before: error about empty message body.
  - After: no more message body error.

Reviewers: amckinley

Reviewed By: amckinley

Differential Revision: https://secure.phabricator.com/D19494
2018-06-15 14:01:40 -07:00
epriestley
1459fb3037 Make re-running rebuild-identities a bit faster and add a little progress information
Summary:
Ref T13151. Ref T12164. Two small tweaks:

  - If we aren't actually going to change anything, just skip the writes. This makes re-running/resuming a lot faster (~20x, locally).
  - Print when we touch a commit so there's some kind of visible status.

This is just a small quality-of-life tweak that I wrote anyway while investigating T13152, and will make finishing off db024, db025 and db010 manually a little easier.

Test Plan:
  - Set `authorIdentityPHID` + `committerIdentityPHID` to `NULL`.
  - Ran `rebuild-identities`, saw status information.
  - Ran `rebuild-identiites` again, saw it go faster with status information.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151, T12164

Differential Revision: https://secure.phabricator.com/D19484
2018-06-12 13:18:54 -07:00
epriestley
6011085b0f Respect "metamta.email-body-limit" when building mail HTML bodies
Summary:
Ref T13151. See T11767. See PHI686. Although we limit outbound mail text bodies, the limit doesn't currently apply to attachments, HTML bodies, or headers. T11767 discusses improving this in the general case.

In the wild, an install hit an issue (see PHI686) where edits to Phriction pages generate very large HTML bodies. Check and respect the limit when building HTML bodies.

If we don't have enough room for the HTML body, we just drop it. We have the text body to fall back to, and HTML is difficult to truncate safely.

Test Plan: Added unit tests and made them pass.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19489
2018-06-12 12:02:15 -07:00
epriestley
c5b13a6be3 Allow object subtypes to be changed via bulk editor
Summary:
Ref T13151. See PHI683. Ref T12314.

You can currently change object subtypes via Conduit (`maniphest.edit`) but not via the web UI.

Changing object subtypes is inherently a somewhat-perilous operation that likely has a lot of rough edges we'll need to smooth over eventually, mostly around changing an object from subtype X to subtype Y, where some field exists on one but not the other. This isn't a huge issue, just not entirely intuitive.

It should also, in theory, be fairly rare.

As a reasonable middle ground, provide web UI access via the bulk editor. This makes it possible, but doesn't clutter the UI up with a rarely-used option with rough edges.

Test Plan:
  - With subtypes not configured, saw a normal bulk editor with no new option.
  - With subtypes configured, swapped tasks subtypes via bulk editor.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151, T12314

Differential Revision: https://secure.phabricator.com/D19490
2018-06-12 11:58:44 -07:00
epriestley
62a402491a Allow encrypted mail to be more specific about which object is affected
Summary:
Depends on D19487. Ref T13151. See PHI647. For some objects, like revisions, we can build slightly more useful secure email without actually disclosing anything.

In the general case, the object monogram may disclose information (`#acquire-competitor`) but most do not, so applications can whitelist an acceptable nondisclosing subject and link.

Support doing this, and make Differential do it. When we don't have a whitelisted URI but do know the object the mail is about, include a generic PHID-based URI; these are always nondisclosing.

Test Plan:
  - Without the Differential changes, sent normal mail (no changes) and secure mail (new generic PHID-based link).
  - With the Differential changes, sent secure mail; got richer subject and body link.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19488
2018-06-12 11:55:18 -07:00
epriestley
94752278f4 Add a generic PHID-based object redirection controller
Summary:
Ref T13151. See PHI647. This allows us to link to any object by PHID, without disclosing information in the monogram (like `#fire-steve`).

This capability is relevant when building "secure mail", to provide a link to the object regardless of whether the monogram discloses information or not.

Test Plan: Visited `/object/D123/` (redirect), `/object/xyz/` (404), `/object/PHID-DREV-.../` (redirect).

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19487
2018-06-12 11:54:59 -07:00
epriestley
cbff913432 Add a "members of all projects" (vs "...any project") custom policy rule to the upstream
Summary:
Ref T13151. See PHI702. An install is interested in a "members of all projects" (vs "members of any project", which is currently implemented) rule.

Although this is fairly niche, I think it's reasonable and doesn't have much of a maintenance cost.

This could already be implemented as an extension, but it would have to copy/paste a bunch of code.

Test Plan:
  - Ran unit tests.
  - Used the UI to select this policy for a task, with various values. Joined/left projects to satisfy/fail the rule. Behavior seemed correct.
  - Used the UI to select the existing policy rule ("any project"), joined/left projects to satisfy/fail the rule. Doesn't look like I broke anything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19486
2018-06-12 11:51:51 -07:00
Alex Vandiver
59b95f9397 Fix typo in "button"
Test Plan: Observation.

Reviewers: #blessed_reviewers, amckinley, epriestley

Reviewed By: #blessed_reviewers, amckinley, epriestley

Subscribers: Korvin, amckinley, epriestley

Differential Revision: https://secure.phabricator.com/D19483
2018-06-08 15:09:07 -07:00
epriestley
f375427177 Use more consistent diff coloration in unified diffs
Summary:
Ref T13151. See PHI701. Unified diffs are currently missing the logic to apply the "old-full" and "new-full" classes, which results in a too-light coloration for fully added or removed lines.

Make this logic consistent with the two-up renderer so we use the same colors in both.

Test Plan: Viewed diffs and swapped between 1-up and 2-up renderers, now saw the same coloration.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19482
2018-06-08 09:39:34 -07:00
epriestley
4e84d4d458 Allow the haunted comment panel ("Z") to take up more vertical room
Summary:
Ref T13151. See PHI685. When you haunt the panel, we only let it take up part of the screen. Let it take up slightly more of the screen so that it's more likely to fit completely on-screen without needing to scroll.

The behavior when it does scroll is fine (you get a scrollbar if your OS/browser is set up to show them) so this is a bit trivial/silly, but seems fine and doesn't have a big JS maintenance cost or anything.

Test Plan: Pressed "Z", resized my window to a weird tiny useless size, got slightly better (I guess) behavior.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19480
2018-06-07 13:19:35 -07:00
epriestley
13dd3014a7 Fix loop in QueryIterator when row count is an exact multiple of page size
Summary: Ref T13152. The pager does a bit of magic here and doesn't populate `nextPageID` when it knows it got an exact final page. The logic misfired in this case and sent us back to the start.

Test Plan:
  - Set page size to 1 to guarantee rows were an exact multiple of page size.
  - Ran `rebuild-identities` (I no-op'd the actual logic to make it faster).
  - Before: looped forever.
  - After: clean exit after processing everything.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13152

Differential Revision: https://secure.phabricator.com/D19479
2018-06-07 13:17:34 -07:00
epriestley
7acda7e94e Truncate package names in diff table of contents views
Summary:
Ref T13151. See PHI654. Depends on D19477. If you have long package names, the table of contents (e.g., in Differential) can end up expanding to be gigantic.

Getting tables to behave nicely is hard (or, at least, I can't figure it out after spending a decent amount of time on it; see also `AphrontTableView::renderSingleDisplayLine()`). I tried a bunch of things and Googled for a bit but didn't make any progress on finding a CSS solution. Just truncate the package names to get reasonable behavior without falling down any kind of CSS rabbit hole.

Test Plan:
  - Created a package named "Very long package name...".
  - Created a package named "MMMMMMMMMMMMMMMMMMMMMM...".
  - Had them own a file in a Differential revision, viewed that revision.
  - Before: table is pushed out to several times the browser window width and everything is kind of a mess.
  - After: package names get truncated to something reasonable.

{F5652953}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19478
2018-06-07 13:17:01 -07:00
epriestley
2951e0c86b Include owners packages in the MailableFunction datasource
Summary:
Ref T13151. See PHI684. Currently, the `MailableFunction` datasource does not include Owners packages, but they are valid subscribers and the `Mailable` datasource includes them.

Include them in the `MailableFunction` datasource, too.

Test Plan: Searched for revisions with particular package subscribers, got expected results in the UI (tokenizer knew about packages) and response.

Reviewers: amckinley, jmeador

Reviewed By: jmeador

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19476
2018-06-07 12:02:50 -07:00
epriestley
7729c51cc4 Fix an issue where scrolling down, then up, then down fails to show changeset header in Differential
Summary: Ref T13151. See PHI616. There's a bug where the current banner changeset isn't cleared correctly when we hide the banner.

Test Plan:
  - View revision with several changesets.
  - Scroll down slowly through first changeset until banner appears.
  - Scroll up until banner disappears.
  - Scroll back down.
  - Before: banner fails to reappear (code still thinks it's visible and we don't want to update it).
  - After: banner reappears correctly.

Reviewers: amckinley, jmeador

Reviewed By: jmeador

Maniphest Tasks: T13151

Differential Revision: https://secure.phabricator.com/D19474
2018-06-07 12:02:18 -07:00
Austin McKinley
b8b2d1672d Prevent creation of empty repository identities
Summary: Fixes issue reported in https://secure.phabricator.com/rPf191a66490b194785fae28c062b71be99bb14584#43240

Test Plan: Imported an SVN repo, observed clean import instead of daemon exception.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D19466
2018-06-05 16:13:59 -07:00