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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
