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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Fixes T13145. The list controllers properly support public access already, but some of the view/detail controllers did not.
Allow logged-out users to browse builds, buildables, plans, etc., provided they can see the corresponding objects.
Test Plan: As a logged-out user, browsed around builds, build plans, logs, etc., without hitting any login pages.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13145
Differential Revision: https://secure.phabricator.com/D19459
Summary:
See PHI689. It can be difficult to distinguish between cards with the same number but different expiration dates (common when the bank sends you a new card).
For now, show the expiration date on the cart checkout screen.
Test Plan: Viewed a cart checkout screen with multiple cards, saw expiration dates.
Reviewers: amckinley
Differential Revision: https://secure.phabricator.com/D19462
Summary:
Fixes T13148. Ref T13105. The new document rendering engine for images let them overflow the UI bounds.
Add `max-width: 100%;` to keep them contained.
Test Plan:
- Viewed a very wide image in Safari, Firefox and Chrome. Saw sensible rendering.
- Also viewed a normal image, saw normal behavior.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13148, T13105
Differential Revision: https://secure.phabricator.com/D19457
Summary:
Fixes T13147. In D19437, I changed this logic to support deleting the `""` (empty string) token, but `[].pop()` returns `undefined`, not `null`, if the list is empty and I didn't think to try deleting an empty input.
Fix the logic so we don't end up in a loop if the input is empty.
Test Plan:
- In any browser, deleted all tokens in a tokenizer; then pressed delete again.
- Before: tab hangs in an infinte loop.
- After: smooth sailing.
Reviewers: amckinley, avivey
Reviewed By: avivey
Maniphest Tasks: T13147
Differential Revision: https://secure.phabricator.com/D19456
Summary:
Depends on D19443. Creates a workflow for populating the new identity table by iterating over commits, either one repo at a time or all at once. Locally caches identities to avoid fetching them `inf` times. An actual migration that invokes this workflow will come in another revision that won't land until at least next week.
Performance is ~2k commits in 4.9s on my local machine.
Test Plan: Ran locally a few times with a few different states of the `repository_identity` table.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: jcox, Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19446
Summary: Depends on D19429. Depends on D19423. Ref T12164. This creates new columns `authorIdentityPHID` and `committerIdentityPHID` on commit objects and starts populating them. Also adds the ability to explicitly set an Identity's assignee to "unassigned()" to null out an incorrect auto-assign. Adds more search functionality to identities. Also creates a daemon task for handling users adding new email address and attempts to associate unclaimed identities.
Test Plan: Imported some repos, watched new columns get populated. Added a new email address for a previous commit, saw daemon job run and assign the identity to the new user. Searched for identities in various and sundry ways.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19443
Summary: Depends on D19423. Ref T12164. Adds controllers capable of listing and editing `PhabricatorRepositoryIdentity` objects. Starts creating those objects when commits are parsed.
Test Plan: Reparsed some revisions, observed objects getting created in the database. Altered some `Identity` objects using the controllers and observed effects in the database. No attempts made to validate behavior under "challenging" author/committer strings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19429
Summary: Ref T12164. Start building initial objects for managing `RepositoryIdentity` objects. This won't land until much more of the infrastructure is in place.
Test Plan: Ran `bin/storage upgrade` and observed expected table.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T12164
Differential Revision: https://secure.phabricator.com/D19423
Summary:
Ref T13142. When commits are pushed, we try to handle them on one of two pathways:
- Normal changes: we load these into memory and potentially apply Herald content rules to them.
- "Enormous" changes: we don't load these into memory and skip content rules for them.
The goal is to degrade gracefully when users push huge changes: they should work, just not support all the features.
However, some changes can slip through the cracks right now:
- If you push a lot of commits at once, we'll try to cache all of the changes smaller than 1GB in memory. This can require an arbitrarily large amount of RAM.
- We calculate sizes by just looking at the `strlen()` of the diff, but a changeset takes more RAM in PHP than the raw diff does. So even if a diff is "only" 500MB, it can take much more memory than that. On systems with relatively little memory available, this may result in OOM while processing changes that are close to the "enormous" limit.
This change makes two improvements:
- Instead of caching everything, cache only 64MB of things.
- For most pushes, this is the same, since they have less than 64MB of diffs.
- For pushes of single very large changes, this is a bit slower (more CPU) since we have to do some work twice.
- For pushes of many changes, this is slower (more CPU) since we have to do some work twice, but, critically, doesn't require unlimited memory.
- Instead of flagging changes as "enormous" at 1GB, flag them as "enormous" at 256MB.
- This reduces how much memory is required to process the largest "non-enormous" changes.
- This also gets us under Git's hard-coded 512MB "always binary" cutoff; see T13143.
- This is still completely gigantic and way larger than any normal change should be.
An additional improvement would be to try to reduce the amount of memory we need to use to hold a change in process memory. I think the other changes here alone will fix the immediate issue in PHI657, but it would be nice if the "largest non-enormous change" required only a couple gigs of RAM.
Test Plan:
- Used `ini_set('memory_limit', '1G')` to artificially limit memory to 1GB.
- Pushed a series of two commits which add two 550MB text files (Temporarily, I added a `--binary` flag to trick Git into showing real diffs for these, see T13143.)
- Got a memory limit error.
- Applied the "cache only 64MB of stuff" and "consider 256MB, not 1GB, to be enormous" changes.
- Pushed again, got properly rejected as enormous.
- Added `memory_get_usage()` calls to measure how actual memory size and reported "size" estimate compare. For these changes, saw a 639MB diff require 31,479MB of memory, i.e. a factor of about 50x. This is, uh, pretty not great.
- Allowed enormous changes, pushed again, push went through.
Reviewers: amckinley
Maniphest Tasks: T13142
Differential Revision: https://secure.phabricator.com/D19455
Summary:
Ref T13141. Currently, during first-time setup we don't surface all the details about connection exceptions that we could: the underlying exception is discarded inside cluster connection management.
This isn't a huge issue since the reason for connection problems is usually fairly obvious, but in at least one case (see T13141) we hit a less-than-obvious exception.
Instead, store the original exception and propagate the message up the stack so users have more information about the problem.
Test Plan:
- Configured an intentionally bad MySQL username.
- Restarted Apache and loaded Phabricator.
- Got a more helpful exception with a specific authentication error message.
{F5622361}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13141
Differential Revision: https://secure.phabricator.com/D19454
Summary:
Ref T13137. See that task for discussion.
When we show a diff-of-diffs, we often render stubs for files which didn't change between the diffs. These stubs usually aren't a big deal, but for certain types of changes (like refactors) they can create a lot of clutter.
Instead, hide these stubs and show a notice that we hid them.
Test Plan:
- Created a revision affecting 4 files.
- Updated it with a diff that changed only 1 of the 4 files.
- Added an inline comment to a different file.
- Viewed the diff of diffs.
- Before: 4 changesets with two "nothing changed" stubs.
- After: 2 changesets with the stubs hidden.
{F5621083}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19453
Summary:
Ref T13137. The "analyze/cache data about changesets" step is becoming more involved. We recently added detection for generated code to support "Ignore generated changes" in Owners, and I now plan to hash the new file content so we can hide changes which have no effect.
Before adding this new hashing step, pull the "detect copied code" and "detect generated code" stuff out and move them to a separate `ChangesetEngine`. Then support doing a changeset rebuild directly with `bin/differential rebuild-changesets`.
This simplifies things a bit and makes testing easier since you don't need to keep creating new revisions to re-run copy/generated/hash logic.
Test Plan: Ran `bin/differential rebuild-changesets --revision Dxxx`, saw changesets rebuild. See also next change.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13137
Differential Revision: https://secure.phabricator.com/D19452
Summary:
Fixes T13140. See PHI660.
Recent versions of Subversion can send a `(get-file true false false )` protocol frame with extra space between "false" and "false". This is allowed by the protocol spec, but never normally happens, and we do not parse it correctly.
Instead, parse it correctly.
Test Plan:
- Added unit tests.
- Ran `svn proplist svn+ssh://.../diffusion/X/file.c` under SVN 1.10 before and after the change.
- Before: indefinite hang.
- After: completed in finite time.
Reviewers: amckinley, asherkin
Reviewed By: amckinley, asherkin
Maniphest Tasks: T13140
Differential Revision: https://secure.phabricator.com/D19451
Summary:
See D19446. This should make it easier to process larger, more complex result sets in constant memory.
Today, `LiskMigrationIterator` takes constant memory but can't apply `needX()` reqeusts or `withY(...)` constraints.
Using a raw `Query` can handle this stuff, but requires memory proportional to the size of the result set.
Offer the best of both worlds: constant memory and full access to the power of `Query` classes.
Test Plan:
Used this script to iterate over every commit, saw sensible behavior:
```name=list-commits.php
<?php
require_once 'scripts/init/init-script.php';
$viewer = PhabricatorUser::getOmnipotentUser();
$query = id(new DiffusionCommitQuery())
->setViewer($viewer);
$iterator = new PhabricatorQueryIterator($query);
foreach ($iterator as $commit) {
echo $commit->getID()."\n";
}
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19450
Summary: Ref PHI662. Viewing a dashboard you don't have permission to view (in the Dashboard application) currently fatals while building crumbs, since we fail to build the ` ... > Dashboard 123 > ...` crumb.
Test Plan:
- Viewed a dashboard I didn't have permission to view in the Dashboards application.
- Before patch, fatal when calling `getID()` on a non-object.
- After patch, sensible policy error page.
- Viewed a dashboard I can view, saw sensible crumbs.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19449
Summary:
See <https://hackerone.com/reports/351361>. We currently require MFA on the screen leading into the user create flow, but not the actual create flow.
That is, `/people/create/` (which is just a "choose a type of account" page) requires MFA, but `/people/new/<type>/` does not, even though this is the actual creation page.
Requiring MFA to create users isn't especially critical: creating users isn't really a dangerous action. The major threat is probably just that an attacker can extend their access to an install by creating an account which they have credentials for.
It also isn't consistently enforced: you can invite users or approve users without an MFA check.
So there's an argument for just removing the check. However, I think the check is probably reasonable and that we'd likely prefer to add some more checks eventually (e.g., require MFA to approve or invite) since these actions are rare and could represent useful tools for an attacker even if they are not especially dangerous on their own. This is also the only way to create bot or mailing list accounts, so this check does //something// on its own, at least.
Test Plan:
- Visited `/people/new/standard/` as an admin with MFA configured.
- Before patch: no MFA prompt.
- After patch: MFA prompt.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19448
