Summary:
We don't use versioned URIs for images, so when they change users may get old versions.
This was a particular issue with the recent logo change, which several users reported cache-related issues from.
Instead, use Celerity to manage image URI versions in addition to CSS/JS.
This is complicated, because we need to rewrite image URIs inside of CSS, which means the hash of a CSS file has to be derived from the current image data. Otherwise, when we updated an image the CSS wouldn't update, so we wouldn't be any better off.
So basically we:
- Find all the "raw" files, and put them into the map.
- Find all the CSS/JS, perform content-altering transformations on it (i.e., not minification) based on the partial map, and then put it into the map based on transformed hashes.
(If we wanted, we could now do CSS variables or whatever for "free", more or less.)
Test Plan:
- Regenerated celerity map, browsed site, verified images generated with versioned URIs.
- Moved "blue" flag image over "green" flag image, regenerated map, verified "green" flag image and the associated CSS changed hashes.
- Added transformation unit tests; ran unit tests.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1073
Differential Revision: https://secure.phabricator.com/D2146
Summary: People are hella lazy and don't want to do this themselves.
Test Plan: Generated a symbol file with duplicates and piped it in, got an import under --ignore-duplicates.
Reviewers: kdeggelman, btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2145
Summary: detect all revisions that don't have a diff, then delete them.
Test Plan:
we have been using this script for several months in
Facebook and it's working well.
Reviewers: epriestley, nh, btrahan
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T605
Differential Revision: https://secure.phabricator.com/D2061
Summary: For production servers, minify CSS and JS by stripping comments, whitespace, etc.
Test Plan: Looked at CSS/JS, it was much smaller.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T264
Differential Revision: https://secure.phabricator.com/D2034
Summary:
We spend a significant amount of time running includes, even with APC. However, we have rigidly structured includes and can safely run them all in workers before requests occur.
Right now, requests go like this:
- Apache spawns a worker.
- Client sends an HTTP request.
- Apache interprets it.
- Apache sees it's ".php", so it hands it off to the PHP SAPI.
- The PHP SAPI starts the PHP interpreter in the worker.
- The request is handled, etc.
Instead, we want to do this:
- Worker spawns and loads the world.
- Client sends an HTTP request.
- Webeserver interprets it.
- Sees it's a ".php", hands it off to the SAPI.
- SAPI executes it on a loaded world.
No SAPIs I know of support this, but I added support to PHP-FPM fairly easily (in the sense that it took me 6 hours and I have a hacky, barely-working mess). Over HTTP (vs HTTPS) the performance improvement is pretty dramatic.
HPHP doesn't significantly defray this cost so we're probably quite a bit faster (to the user) under nginx+PHP-FPM than HPHP after this works for real.
I have the php-fpm half of this patch in a messy state, I'm going to try to port it to be vs php 5.4.
Test Plan: Ran a patched php-fpm, browsed around, site works, appears dramatically faster.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2030
Summary:
- Still really really rough.
- Adds a full synchronous mode for debugging.
- Adds some logging.
- It can now allocate EC2 machines and put webroots on them in a hacky, terrible way.
- Adds a base query class.
Test Plan: oh hey look a test page? http://ec2-50-18-65-151.us-west-1.compute.amazonaws.com:2011/
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D2026
Summary:
I noticed that documentation said it is possible to have 'ctags' symbol import, so I hacked a quick version. I tested it on Python based project and successfuly imported symbols.
It is limited to classes right now, as the importer script complained about not-unique method names (there are a lot of 'get' & 'post' methods accross classes in my project).
If you would have any feedback about this, I would definetly try to wrap it up for possibly merging into main repository.
Test Plan:
Required 'ctags' tool (ctags.sourceforge.net/) Tested to work with version 5.8+ and didn't work with 3.x.
1. `find . -type f '*.py' | ./generate_ctags_symbols.php > /tmp/symbols`
2. `./import_project_symbols.php` < /tmp/symbols
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: seporaitis, aran, epriestley
Maniphest Tasks: T1034
Differential Revision: https://secure.phabricator.com/D1995
Summary:
libphutil has some basic environmental sanity checks that we should use when initializing scripts in Phabricator. Principally this:
https://secure.phabricator.com/diffusion/PHU/browse/master/scripts/__init_script__.php;db643ee9f5f524e7$26
Without this, the default ini may set CLI errors to go to some logfile, which means exceptions aren't shown on stderr.
See https://github.com/facebook/phabricator/issues/98/
Test Plan:
- Ran "php -derror_log=/dev/null -f ./bin/phd debug adslkfnasdfnalks" prior to change; got confusing lack of output.
- Ran "phd -derror_log=/dev/null -f ./bin/phd debug asdkflnaslfdnala" after change; got exception on stderr.
Reviewers: btrahan, killermonk
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1950
Summary: I'm not aware of an easy way to get this information through normal tools. I'm sure there's some fancy GUI client that has it but this seems worth keeping around.
Test Plan: ran script, got helpful information about data sizes
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1916
Summary:
Smaller diffs, better blame, less noise on a file which is usually ignored.
Less conflicts in merge!
Test Plan: php -l __celerity_resource_map__.php
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1909
Summary: Tests and docstubs don't provide anything and they are not required in celerity map.
Test Plan: View Options in Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1893
Summary: If someone accidentally pushes a bunch of commits, revisions might get marked as "Committed" incorrectly. This will restore them to their previous state without too much fuss.
Test Plan: Ran the script on some commits to undo them, it seemed to work correctly.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1877
Summary: This is the script used for conversion: P319
Test Plan:
Update diff with UTF-8 characters in description.
`sql/upgrade_schema.php`
Verify data in DB and that it looks good on web.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T327
Differential Revision: https://secure.phabricator.com/D1830
Summary: It would be better to test if a key is passworded, but I couldn't figure out how to do that.
Test Plan: Ran "test_connection.php"
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T924
Differential Revision: https://secure.phabricator.com/D1856
Summary: Urgent request.
Test Plan: Ran "add_macro.php" on some things; verified results in web console.
Reviewers: btrahan, tcook
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1832
Summary: New implicit fallthrough linter detected a few issues; none of these have behavioral impacts but they can clearly be tightened up. See D1824.
Test Plan: Lint; inspection.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1825
Summary: If you import a repository you may trigger a large number of irrelevant audits. Provide a tool to nuke them.
Test Plan: Ran "audit.php Q" (does not exist), "audit.php P" (phabricator) from various repository states.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904, T940
Differential Revision: https://secure.phabricator.com/D1791
Summary:
When switching from using the MetaMTADaemon to a Taskmaster for sending mail,
if there are messages queued for delivery, they need to be re-queued into the
task system. This patch does that.
Task ID: #
Blame Rev:
Test Plan:
Ran it.
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1780
Summary:
1) Dry run option allows you to run the script to see what patches would get
applied (without actually applying them).
2) Max version allows you to do a partial upgrade.
Task ID: #
Blame Rev:
Test Plan:
ran a dry run twice (to make sure it didn't do anything) and no max version
ran with a dry run and max version to check max version logic works correctly
ran with just a max version, and only patches up to that point got applied
Revert Plan:
Tags:
Reviewers: epriestley, btrahan, jungejason
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1758
Summary:
- Update the Javelin submodule to pick up recent fixes (like D1749).
- Update the package definitions do do a slightly better job of packaging
resources.
Test Plan:
Up and down work in tokenizers now. Pages load slightly fewer
resources.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T927
Differential Revision: https://secure.phabricator.com/D1751
Summary:
With this script, I am able to add bookmarklet to my browser which edits the
controller responsible for displaying current page by a single click.
Plus it can be useful also for other uses.
Test Plan:
./aphrontpath.php /
./aphrontpath.php D123
./aphrontpath.php /D123
./aphrontpath.php /D123/ # doesn't exist
./aphrontpath.php https://secure.phabricator.com/D123
./aphrontpath.php https://secure.phabricator.com/D123?x=2
./aphrontpath.php https://secure.phabricator.com/D123#comment-1
./aphrontpath.php differential
./aphrontpath.php /differential/
./aphrontpath.php /w/ # rewritten by custom config
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T349
Differential Revision: https://secure.phabricator.com/D1746
Summary:
Some browsers will still sniff content types even with "Content-Type" and
"X-Content-Type-Options: nosniff". Encode "<" and ">" to prevent them from
sniffing the content as HTML.
See T865.
Also unified some of the code on this pathway.
Test Plan: Verified Opera no longer sniffs the Conduit response into HTML for
the test case in T865. Unit tests pass.
Reviewers: cbg, btrahan
Reviewed By: cbg
CC: aran, epriestley
Maniphest Tasks: T139, T865
Differential Revision: https://secure.phabricator.com/D1606
Summary:
"rev-parse --verify" is actually a terrible test, it only survived my test cases
because I put garbage into the tables with RAND() or similar, not
properly-formatted garbage.
Use "cat-file -t" instead to ensure we perform an object-existence test.
Test Plan: Ran "reconcile.php P" locally, ran "cat-file -t" and "rev-parse
--verify" on properly-formatted but invalid hashes like
"aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", worked with @rguerin to resolve
commit issues.
Reviewers: btrahan, rguerin
Reviewed By: rguerin
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1590
Summary:
@rguerin ran into an issue in his install where Phabricator appears to have
discovered commits which no longer exist, and thus is failing to proceed with
its repository import.
It's not clear how we got into this state. Previously, it was possible by, e.g.,
parsing a different repository's working copy and then switching them back, but
there are now safeguards against that.
I'm taking a three-pronged approach to try to sort this out:
- Provide a script to get out of this state (this script) and reconcile
Phabricator's view of a repository with an authoritative copy of it. This
basically "un-discovers" any discovered commits which don't actually exist (any
queued tasks to parse them will fail permanently when they fail to load the
commit object).
- Add more logging to the discovery daemon so we can figure out where commits
came from.
- Improve Diffusion's UI when stuff is partially discovered (T776).
(This script should also clean up some nonsense on secure.phabricator.com from a
botched Diviner import.)
Test Plan: Ran "reconcile.php" with bogus commits and bogus differential/commit
links, had them expunged. Will work with @rguerin to see if this resolves
things.
Reviewers: btrahan, rguerin
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1552
Summary: Run the actual resource allocation for Drydock out-of-process via the
task queue.
Test Plan: Ran "drydock_control.php", saw it insert a task and wait for task
completion. Ran "phd debug taskmaster" and saw it run the task.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1470
Summary:
Rough cut of Drydock. This is very basic and doesn't do much of use yet (it
//does// allocate EC2 machines as host resources and expose interfaces to them),
but I think the overall structure is more or less reasonable.
== Interfaces
Vision: Applications interact with Drydock resources through DrydockInterfaces,
like **command**, **filesystem** and **httpd** interfaces. Each interface allows
applications to perform some kind of operation on the resource, like executing
commands, reading/writing files, or configuring a web server. Interfaces have a
concrete, specific API:
// Filesystem Interface
$fs = $lease->getInterface('filesystem'); // Constants, some day?
$fs->writeFile('index.html', 'hello world!');
// Command Interface
$cmd = $lease->getInterface('command');
echo $cmd->execx('uptime');
// HTTPD Interface
$httpd = $lease->getInterface('httpd');
$httpd->restart();
Interfaces are mostly just stock, although installs might add new interfaces if
they expose different ways to interact with resources (for instance, a resource
might want to expose a new 'MongoDB' interface or whatever).
Currently: We have like part of a command interface.
== Leases
Vision: Leases keep track of which resources are in use, and what they're being
used for. They allow us to know when we need to allocate more resources (too
many sandcastles on the existing hosts, e.g.) and when we can release resources
(because they are no longer being used). They also give applications something
to hold while resources are being allocated.
// EXAMPLE: How this should work some day.
$allocator = new DrydockAllocator();
$allocator->setResourceType('sandcastle');
$allocator->setAttributes(
array(
'diffID' => $diff->getID(),
));
$lease = $allocator->allocate();
$diff->setSandcastleLeaseID($lease->getID());
// ...
if ($lease->getStatus() == DrydockLeaseStatus::STATUS_ACTIVE) {
$sandcastle_link = $lease->getInterface('httpd')->getURI('/');
} else {
$sandcastle_link = 'Still building your sandcastle...';
}
echo "Sandcastle for this diff: ".$sandcastle_link;
// EXAMPLE: How this actually works now.
$allocator = new DrydockAllocator();
$allocator->setResourceType('host');
// NOTE: Allocation is currently synchronous but will be task-driven soon.
$lease = $allocator->allocate();
Leases are completely stock, installs will not define new lease types.
Currently: Leases exist and work but are very very basic.
== Resources
Vision: Resources represent some actual thing we've put somewhere, whether it's
a host, a block of storage, a webroot, or whatever else. Applications interact
through resources by acquiring leases to them, and then getting interfaces
through these leases. The lease acquisition process has a side effect of
allocating new resources if a lease can't be acquired on existing resources
(e.g., the application wants storage but all storage resources are full) and
things are configured to autoscale.
Resources may themselves acquire leases in order to allocate. For instance, a
storage resource might first acquire a lease to a host resource. A 'test
scaffold' resource might lease a storage resource and a mysql resource.
Not all resources are auto-allocate: the entry-level version of Drydock is that
you manually allocate a couple boxes and configure them through the web console.
Then, e.g., 'storage' / 'webroot' resources allocate on top of them, but the
host pool itself does not autoscale.
Resources are completely stock, they are abstract shells representing any
arbitrary thing.
Currently: Resource exist ('host' only) but are very very basic.
== Blueprints
Vision: Blueprints contain instructions for building interfaces to, (possibly)
allocating, updating, managing, and destroying a specific type of resource in a
specific location. One way to think of them is that they are scripts for
creating and deleting resources. For example, the LocalHost, RemoteHost and
EC2Host blueprints can all manage 'host' resources.
Eventually, we will support more types of resources (storage, webroot,
sandcastle, test scaffold, phacility deployment) and more providers for resource
types, some of which will be in the Phabricator mainline and some of which will
be custom.
Blueprints are very custom and specific to application types, so installs will
define new blueprints if they are making significant use of Drydock.
Currently: They exist but have few capabilities. The stock blueprints do nearly
nothing useful. There is a technically functional blueprint for host allocation
in EC2.
== Allocator
This is just the actual code to execute the lease acquisition process.
Test Plan: Ran "drydock_control.php" script, it allocated a machine in EC2,
acquired a lease on it, interfaced with it, and then released the lease. Ran it
again, got a fresh lease on the existing resource.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1454
Summary: We purge the cache for primary comments only right now, not inline
comments.
Test Plan: Ran "purge_cache.php --differential".
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T683
Differential Revision: https://secure.phabricator.com/D1451
Summary: Revisit of D1254. Don't require lowercase, just standardize the logic.
The current implementation has nonuniform logic -- PeopleEditController forbids
uppercase.
Test Plan: Ran unit tests, see also D1254.
Reviewers: btrahan, jungejason, aran
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1415
Summary:
- Run "phd stop" before stopping apache. This is essentially a smoke test for
PHABRICATOR_ENV being set.
- Run documentation generation after everything else. Between the pull and the
restart we have some minor exposure to APC issues with deleted files and
out-of-date module definitions, and this limits that.
- Pull commands out of (x && y) stuff, this prevents "set -e" from working
correctly.
Test Plan: Ran upgrade script locally.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D1419
Summary: ..."ssh" is in quotes 'cuz this is step 1 and there's no ssh in sight
at the moment.
Test Plan:
ran api.php PHID-USER-xee4ju2teq7mflitwfcs differential.query a few times...
- tried valid input, it worked!
- tried bad input, it worked in that it failed and told me so!
ran api.php crap_user differential.query a few times...
- verified error message with respect to crap_user
ran api.php PHID-USER-xee4ju2teq7mflitwfcs crap_method a few times...
- verified error message with respect to crap_method
visited http://phabricator.dev/conduit/method/differential.query a few times...
- tried valid input, it worked!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D1357
Summary:
add support in purge_cache.php. We might want to add support
for maniphest comments and differential comments later.
Test Plan:
- purging all worked
- purging individual changeset worked
- when input is not integer, correct error message reported
Reviewers: epriestley, btrahan, nh
Reviewed By: nh
CC: nh, aran, jungejason, epriestley
Maniphest Tasks: T683
Differential Revision: https://secure.phabricator.com/D1372
Summary: Make it a little easier to create a bunch of accounts if your company
has more than like 5 employees.
Test Plan: Ran "add_user.php" to create new users. Created new users from the
web console.
Reviewers: btrahan, jungejason, rguerin
Reviewed By: btrahan
CC: aran, btrahan, rguerin
Differential Revision: https://secure.phabricator.com/D1336
Summary: Some day we might have a fancy daemon for this, but for now at least
provide some instructions on using the existing importers, etc., to index
project symbols.
Test Plan:
- Generated documentation, read over the result.
- Ran the example code.
Reviewers: btrahan, jungejason, davidreuss
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T315
Differential Revision: https://secure.phabricator.com/D1262
Summary:
- Update Javelin to HEAD -- this doesn't pick up anything in particular, but
lets us smoke test some stuff like {D1217}.
- Do a little more packaging since we've picked up a handful of 10-line
behaviors and such for various UI tweaks.
Test Plan:
- Generally, this should be very low-risk.
- Browed Maniphest, Differential, Diffusion and tried to hit all the JS
interactions.
- Looked over the Javelin changes we're pulling in to see if I forgot
anything. The only API change I caught was removal of "JX.defer()", but that was
already cleared in Phabricator in D803.
Reviewers: aran, btrahan, jungejason
Reviewed By: aran
CC: aran
Differential Revision: 1240
Summary: See task; installs occasionally need to do this themselves, and a
script is much better than me telling them to truncate tables.
Test Plan:
Ran various flavors of this command:
- purge_cache.php
- purge_cache.php derp
- purge_cache.php --help
- purge_cache.php --all
- purge_cache.php --differential
- purge_cache.php --differential --maniphest
Then I verified the actual behavior:
- Visited a Differential revision with comments, observed cache update in
'Services' tab.
- Visited a Maniphest task with comments, observed cache update in 'Services'
tab.
- Reloaded a diff standalone view, got a cache update.
Reviewers: Makinde, btrahan, jungejason
Reviewed By: jungejason
CC: aran, jungejason
Maniphest Tasks: T676
Differential Revision: 1214
Summary:
For each commit, find the affected packages, and provide a way to
search by package.
Test Plan:
create commits that touch and don't touch two packages, and verify
that they display correctly in all the UI pages.
Reviewers: epriestley, blair, nh, tuomaspelkonen
Reviewed By: epriestley
CC: benmathews, aran, epriestley, btrahan, jungejason, mpodobnik, prithvi
Maniphest Tasks: T83
Differential Revision: 1208
Summary:
If mails are not sent in UTF-8 we cannot just it verbatim, so we have to
encode it into UTF-8 if it is not the case. Mime headers use different
encodings like "quoted-printable", which we have to handle.
It looks like "Subject: =?iso-8859-1?Q?opr=E6t_s=E5_den_task?=", and can
be decoded by ##iconv_mime_decode##.
Furthermore the body of the email might be in various encodings as well,
which we attempt to pull from the content-type header of the plain text
part of the mail.
Test Plan:
Attempted receiving mails in a variety of flavors. These could be
converted to test-cases once i know if this is a sane solution. Got
expected results from mails sent with Windows-1252 and ISO-8859-1.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, davidreuss
Differential Revision: 1093
Summary: by default it was looking for php to be in /usr/bin/php on some systems
this is incorrect. It should be using /usr/bin/env php for calling php with the
correct path.
Test Plan: patch and try to run.
Reviewers: epriestley
Reviewed By: epriestley
CC: svemir, aran, epriestley
Differential Revision: 1136
Summary: create CelerityResourceGraph, which extends AbstractDirectedGraph.
since we've done a bunch of work already to load the resource graph into memory
CelerityResourceGraph simply stores a copy and makes loadEdges work off that
stored copy.
Test Plan:
made phabricator-prefab require herald-rule-editor
~/code/phabricator> ./scripts/celerity_mapper.php webroot
Finding static resources...
Processing 154
files..........................................................................................................................................................
[2011-11-22 11:28:29] EXCEPTION: (Exception) Cycle detected in resource graph:
phabricator-prefab => herald-rule-editor => phabricator-prefab at
[/Users/btrahan/Dropbox/code/phabricator/scripts/celerity_mapper.php:173]
fixed phabricator-prefab requiring herald-rule-editor. re-ran celerity_mapper
and no errors!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, btrahan, epriestley
Differential Revision: 1132
Summary:
This makes debugging issues a lot easier, since any exceptions thrown
gets logged and can be seen at /mail/received/, for inspection.
Test Plan:
Created a task via the public-author functionality, and saw no errors
without this patch applied. Even worse, the exception was never shown at
command line either. Not sure what's up with that. Output buffering, or
whatever?
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, davidreuss
Differential Revision: 1042
Summary:
make sure all symboles can be loaded to avoid issues like missing
methods in descendants of abstract base class.
Test Plan:
ran it and verified it passes; remove a method in a descendant class
and verified that the test failed.
Reviewers: epriestley, nh
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 1023
