Summary:
The old query was effectively
SELECT DISTINCT revision.* FROM `differential_revision` revision
JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id
AND relationship.objectPHID in
('PHID-USER-a113b9ae4ee9524d0a20'))
OR revision.authorPHID = 'PHID-USER-a113b9ae4ee9524d0a20'
LEFT JOIN `differential_viewtime` viewtime ON
viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
AND viewtime.objectPHID = revision.phid
AND GREATEST(1304022277, IFNULL(viewtime.viewTime, 0)) <
revision.dateModified
ORDER BY dateModified DESC;
I'm not a db performance expert but it looks like the problem is that we
have to scan all revisions
mysql> EXPLAIN SELECT DISTINCT revision.* FROM `differential_revision`
revision JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id AND relationship.objectPHID
in ('PHID-USER-a113b9ae4ee9524d0a20')) OR revision.authorPHID =
'PHID-USER-a113b9ae4ee9524d0a20' LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
AND viewtime.objectPHID = revision.phid AND GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revision.dateModified ORDER BY
dateModified DESC;
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
| id | select_type | table | type | possible_keys | key |
key_len | ref | rows | Extra |
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
| 1 | SIMPLE | revision | ALL | PRIMARY,authorPHID | NULL |
NULL | NULL | 254127 | Using temporary; Using filesort |
| 1 | SIMPLE | viewtime | ref | PRIMARY | PRIMARY |
66 | const | 17 | Distinct |
| 1 | SIMPLE | relationship | index | PRIMARY,objectPHID | objectPHID |
72 | NULL | 966900 | Using where; Using index; Distinct |
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
The new query is a lot faster
mysql> EXPLAIN SELECT revs.* FROM ( (SELECT revision.* FROM
`differential_revision` revision WHERE revision.authorPHID in
('PHID-USER-a113b9ae4ee9524d0a20')) UNION (SELECT revision.* FROM
`differential_revision` revision JOIN differential_relationship rel WHERE
rel.revisionId = revision.Id AND rel.objectPHID =
'PHID-USER-a113b9ae4ee9524d0a20')) as revs LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID = 'PHID-USER-a113b9ae4ee9524d0a20' AND
viewtime.objectPHID = revs.phid WHERE GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revs.dateModified ORDER BY revs.dateModified;
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
| id | select_type | table | type | possible_keys | key |
key_len | ref | rows | Extra
|
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
| 1 | PRIMARY | <derived2> | ALL | NULL | NULL |
NULL | NULL | 3021 | Using filesort
|
| 1 | PRIMARY | viewtime | ref | PRIMARY | PRIMARY |
66 | const | 17 | Using where
|
| 2 | DERIVED | revision | ref | authorPHID | authorPHID |
67 | | 1040 | Using where
|
| 3 | UNION | rel | ref | PRIMARY,objectPHID | objectPHID |
66 | | 3822 | Using where; Using
index |
| 3 | UNION | revision | eq_ref | PRIMARY | PRIMARY |
4 | phabricator_differential.rel.revisionID | 1 |
|
| NULL | UNION RESULT | <union2,3> | ALL | NULL | NULL
| NULL | NULL | NULL |
|
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
Test Plan:
Loaded differential updates with new query, made sure page loaded quickly. Ran
the query from the command-line, it took about .4 seconds.
Reviewed By: Girish
Reviewers: tuomaspelkonen, jungejason, Girish
Commenters: btrahan
CC: aran, btrahan, elynde, Girish
Differential Revision: 181
Summary:
The 'All Revisions and Reviews' Query takes about 2 seconds when I run
it from the mysql command-line:
SELECT revision.*
FROM `differential_revision` revision LEFT JOIN
`differential_relationship` relationship
ON revision.id = relationship.revisionID
AND relationship.relation = 'revw'
WHERE revision.authorPHID in ('PHID-USER-a113b9ae4ee9524d0a20') OR
relationship.objectPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
GROUP BY revision.id ORDER BY dateModified DESC
2419 rows in set (2.05 sec)
This takes about 0.1-0.2 seconds. Just dug into this because I guess
phabricator is haven't a bunch of mysql timeouts.
I don't know what the hell I'm doing; this is just faster
Test Plan:
Loaded 'All Revisions and Reviews' in sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/related/
Made sure it had same results as the version in prod
https://phabricator.fb.com/differential/filter/related/
Still slow to generate all that html
Reviewed By: epriestley
Reviewers: epriestley, aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 182
Summary:
Moved this to the "phabricator-www" project.
Test Plan:
N/A
Reviewed By: scottmac
Reviewers: scottmac
CC: aran, scottmac
Differential Revision: 219
Summary:
- Provides an "all daemons" view to look at more than the first 15 daemons.
- Provides a "combined log" view with a large page size, to quickly look at
the log across all the daemons, making it easier to find issues when you have a
bunch of the same daemon and only one is having issues.
- When viewing the web console on the same host as a daemon, show whether it's
running or not.
Test Plan:
Clicked the various daemon log interfaces.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 215
Summary:
This isn't terribly elegant but it solves the problem without loss of
generality. We can pursue a more finessed solution later if it seems prudent.
Test Plan:
Created a revision matched by a blanket herald rule, and then commented on it.
Comment email had X-Herald-Rules header in it.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 218
Summary:
The diff view page should point to the revision, if the diff has
already been attached to one. The form to select the revision was also
removed in this case.
Let's me know if it should be possible to reattach a diff to a different
revision.
Test Plan:
Tested that a new diff created with '--preview' option was not attached to
any revision. After attaching the diff manually, made sure that the diff view
page
showed the link to the revision correctly.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: aran, epriestley
Differential Revision: 216
Summary:
I only actually enabled it in Remarkup previously.
Test Plan:
Created a python diff, got syntax highlighted.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 202
Summary:
Orient potential contributors to stuff they should read first, the Facebook CLA,
how they can get started, and the general philosophy of the project.
Test Plan:
read the document
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, davidrecordon
Commenters: davidrecordon
CC: aran, epriestley, davidrecordon
Differential Revision: 208
Summary:
Adds a pager control to the "Files" tool so you can page through files if there
are >100.
Test Plan:
Set page size to a smaller number, paged through files.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 211
Summary:
Some users have had problems with the database initialization process, simplify
it by creating a new "initialize.sql" dump at v34.
I also populated this dump with the right landing screen (so all the tools
actually have links) and a default avatar.
Test Plan:
Dropped all databases, initialized according to documentation, ended up in a
good state with sensible defaults.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 210
Summary:
there are several places we open an 'r' connection but use it
for writing. Fix them.
Test Plan:
ran parse_one_commit.php against one revision which executes
the code with problem. It used to throw exception. Now it works fine.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish
Commenters: aran
CC: aran, Girish
Differential Revision: 213
Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.
The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.
Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.
Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200
Summary:
See T129, some older git doesn't have %B and we can reasonably fake it with %s
and %b.
Test Plan:
Reparsed all of the Phabricator repository with this worker, commit messages
look fine.
Reviewed By: rm
Reviewers: rm, aran, jungejason, tuomaspelkonen
CC: aran, rm
Differential Revision: 209
Summary:
Currently you can still punch through Lisk isolation by calling
establishConnection(), and we do that all over the place. Rename getConnection()
to establishConnection() so that all existing callers are safe, and rename
establishConnection() to establishLiveConnection() so that it's not surprising
when this fails to stub in unit tests.
Not wedded to the name if anyone thinks "establishExternalConnection" or
something is clearer.
Test Plan:
Loaded site, browsed around, ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 201
Summary:
Amazon SES does not allow us to set a Message-ID header, which means
that threads are incorrect in Mail.app (and presumably other applications
which respect In-Reply-To and References) because the initial email does not
have anything which attaches it to the rest of the thread. To fix this, never
rely on Message-ID if the mailer doesn't support Message-ID.
(In the Amazon SES case, Amazon generates its own Message-ID which we can't
know ahead of time).
I additionally used all the Lisk isolation from the other tests to make this
testable and wrote tests for it.
I also moved the idea of a thread ID lower in the stack and out of
DifferentialMail, which should not be responsible for implementation details.
NOTE: If you push this, it will cause a one-time break of threading for
everyone using Outlook since I've changed the seed for generating Thread-Index.
I feel like this is okay to avoid introducing more complexity here.
Test Plan:
Created and then updated a revision, messages delivered over Amazon
SES threaded correctly in Mail.app. Verified headers. Unit tests.
Reviewed By: rm
Reviewers: aran, tuomaspelkonen, jungejason, rm
Commenters: aran
CC: aran, rm, epriestley
Differential Revision: 195
Summary:
Allow Lisk to be put into process-isolated mode which establishes
only isolated connections. By default, put it into this mode when running
unit tests. Build some simple unit tests around object insertion and
updating.
NOTE: The one flaw in this is that $dao->establishConnection() still
punches through the isolation layer. I need to do an API change to fix this
though so I'm holding it for now. It will probably just rename getConnection()
to establishConnection() and then rename establishConnection() to something
scary like establishLiveExternalConnection().
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 194
Summary:
This provides a new connection which doesn't connect to
anything, so effects can be isolated to the current process (for
unit testing).
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 193
Summary:
Make it slightly less terrible. Make sure to use tables for layout.
Test Plan:
Looked at it.
Reviewed By: tomo
Reviewers: aran, tomo, tuomaspelkonen, jungejason
CC: aran, epriestley, tomo
Differential Revision: 196
Summary:
The correct name of this key is 'github.application-secret', not
'github.secret'. Make DarkConsole check that all the masked keys exist to
prevent this from happening again. This isn't super important since this
is just intended to protected against casual security lapses (taking a
screenshot with DarkCnosole's "Config" tab open, for instance) but it's easy
to check for so it seems worthwhile to get right.
Test Plan:
Loaded page without the actual config file change, got an exception.
Fixed the config, reloaded the page, good news goats (really trying to get this
to catch on since goats are adorable).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 189
Summary:
In a basically reasonable configuration where you connect
with a non-privileged user from the web workflow, upgrade_schema.php
won't have enough privileges. Allow the user to override the normal
auth with -u and -p.
Test Plan:
Tried to do a schema upgrade with an underprivileged user,
got a useful error message instead of garbage.
Reviewed By: Girish
Reviewers: Girish, davidrecordon, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, Girish
Differential Revision: 191
Summary:
This global 'const' syntax was introduced in PHP 5.3:
http://www.php.net/manual/en/language.constants.syntax.php
We're PHP 5.2.x elsewhere so just use define(). Made the constant a little more
specific too.
Test Plan:
Ran upgrade_schema.php script.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish, davidrecordon
CC: jungejason, aran, epriestley, Girish
Differential Revision: 190
Summary:
DifferentialRevision stores this field as a dictionary but the
Editor incorrectly passed it to Herald as a raw value array. Ideally the
property should be called unsubscribedDict or something but I'm increasingly
thinking we're going to centralize subscriptions for Adjutant or some similar
system so I'm disinclined to pursue the schema change just yet. I provided an
explicit raw-value-oriented API, at least.
Test Plan:
With two accounts, A and B: created universal Herald CC rule with
user B, created a revision with user A, had user B unsubscribe, had user A
update the revision. User B was not resubscribed.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ola, aran, epriestley
Differential Revision: 188
Summary:
Thanks to erling for the report. This was XSSable, although you could
only get yourself.
Test Plan:
Made a comment like "</textarea><h1>" and edited it before and after
the patch. Proper behavior with this patch.
Reviewed By: aran
Reviewers: erling, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 187
Summary:
If you don't have a custom renderer, this variable does not get set
and emits a warning when you try to read it.
Test Plan:
Loaded page before and after change, warnings went away (lines
145 and 154 in old file).
Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: jungejason, aran, epriestley
Differential Revision: 186
Summary:
Just minor bookkeeping, but the current regexp is too liberal and
will match things which can't possibly be revision hashes.
Test Plan:
Typed things which should and shouldn't be revision links, they
got handled properly.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 185
Summary:
This is evil, don't run with it enabled. Somehow got enabled on my
laptop?
Test Plan:
Loaded phabricator with magic_quotes_gpc enabled, was rebuffed.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 184
Summary:
This permits individual deployments to better configure their
database configuration, e.g. to allow more dynamic configuration that reacts
to database moves or master/slave replication.
Test Plan:
Browse
Reviewed By: epriestley
Reviewers: Girish, epriestley
CC: aran, epriestley
Differential Revision: 183
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser. At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.
Task ID: 549940
Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
Summary:
Add ability to define mysql slaves and then use that connection on 'r'
connection modes. 'w' connections go to the master server.
Test Plan:
- php -l and checkModule
- worked in my devbox
Reviewed By: jungejason
Reviewers: dpepper, tuomaspelkonen, jungejason
CC: jungejason, aran
Revert Plan:
sure
Differential Revision: 175
Summary:
This adds a new view to differential called Updates.
The high-level goal of Updates is to enabled differential to be
effectively used without email notifications. I've tried doing things
like automatically deleting differential emails where I'm in the 'to'
line since they show up on the main diffential page but then there's
always the chance an important diff flies by without me seeing it. Also,
sometimes someone comments on a diff post-commit but differential
doesn't surface those diffs.
I re-created a test db on my devserver using mysqldump to get data on
revs > 230000 so I would have some test data. We need to add a simple
viewtime table but I didn't want to do that in production. Here's the
table:
CREATE TABLE differential_viewtime (
viewerPHID varchar(64) not null,
objectPHID varchar(64) not null,
viewTime int unsigned not null,
PRIMARY KEY (viewerPHID, objectPHID)
);
Issues:
-Once we turn this on, all diffs will be 'unviewed'. What do you think
about a 'Clear All' button or something?
-Maybe we should add a pager
This feature would be insanely useful, let me know what you think.
Test Plan:
Loaded Updates in my sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/updates/
Clicked a diff, then went back, made sure diff disappeared from Updates
list
Reviewed By: tuomaspelkonen
Reviewers: epriestley, jungejason, tuomaspelkonen
Commenters: epriestley
CC: epriestley, elynde, tuomaspelkonen
Differential Revision: 169
Summary:
Take Tuomas's first diff. Sorry, my bad on diff feedback.
Test Plan:
Load D240353 without any options and make sure 'ignore-trailing'
selected.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: epriestley, aran, tuomaspelkonen
Differential Revision: 172
Summary:
'ignore-all' is very confusing, especially for Python, because the indentation
is incorrect.
Test Plan:
Tested loading D240353 without any options and made sure 'ignore-trailing' was
selected and the diff looked correct.
Reviewed By: aran
Reviewers: jungejason, simpkins, aran, dpepper
Commenters: dpepper
CC: grglr, aran, dpepper, tuomaspelkonen
Differential Revision: 171
Summary: add a conduit method to enable querying revisions' phid from
their revision_IDs, and another one to update the task->revision assoc.
Test Plan: for querying revision_phid method, tested empty, one, and two
revisions in the query. For the one to update the task->revision assoc,
I have another diff in facebook which verified it add and remove assoc
correctly.
Reviewers: tuomaspelkonen, epriestley
CC:
Differential Revision: 165
Summary:
differential.getrevision now returns commits for a given revision.
URI is also return in differential.getrevision.
Test Plan:
Tested from Conduit Console UI that the calls were working
correctly.
Reviewed By: jungejason
Reviewers: jungejason
CC: epriestley, simpkins, dpepper, jungejason
Differential Revision: 163
conduit call.
Summary:
The reviewer information was not available for revisions before this.
Test Plan:
Tested with Conduit Console that correct reviewers were returned.
Reviewed By: jungejason
Reviewers: jungejason
Commenters: epriestley
CC: epriestley, tuomaspelkonen, jungejason
Differential Revision: 160
Summary:
Phabricator did not support giving the task ids in the commit message.
Currently there is no default implementation for this, but there is a
Facebook specific implementation. At some point default implementation
for Maniphest tasks to revisions will be added.
Test Plan:
Tested with the Facebook specific implementation that task ids are
recognized in the commit message and tasks are automatically attached
to revisions.
The task attached to this revision was added from the commit message.
Reviewed By: jungejason
Reviewers: jungejason, epriestley
CC: dpepper, edward, gpatangay, tuomaspelkonen, jungejason
Differential Revision: 240996
Summary:
Having the filename in the <title> will make it easier to browse through many
many tabs of diffusion
Test Plan:
Open up a file or directory in diffusion and ensure that it shows only the file
name.
Reviewed By: tuomaspelkonen
Reviewers: epriestley, tuomaspelkonen, jungejason
Commenters: epriestley
CC: epriestley, aizatto, tuomaspelkonen
Differential Revision: 150
Summary:
Add a method to get the information about a revision, including its full
list of diffs.
We could add an option to just return the diff IDs if we wanted. For my
use case, I need the full set of information for each diff, so fewer
round trips is better. This is also how the old json.php page used to
work.
Test Plan:
Tuomas tested it in his sandbox.
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, epriestley
CC: tuomaspelkonen
Differential Revision: 156
Summary:
Currently if I go to leave an inline comment and then decide
against it and hit <tab>-<enter>, the inline comment edit dialog closes
but the overlay doesn't disappear due to a JS error around trying to
access the markup property of null. This fixes that by setting the
payload to an array with empty markup so that the JS works and the
overlay disappears.
Test Plan:
clicked on the line number column, then pressed ok and
confirmed the dialog closed, the overlay disappeared and there were no
JS errors. also tested that for clicking the reply link and when editing
an existing inline comment.
Reviewed By: aran
Reviewers: epriestley, aran
CC: aran, bh
Revert Plan:
OK
Differential Revision: 154
Summary:
Remove reordering code for package array as it's ordered in the first place
Test Plan:
Called API through conduit console, still works
Reviewed By: epriestley
Reviewers: epriestley, tuomaspelkonen
CC: ju, dpepper, epriestley
Blame Revision:
126
Differential Revision: 141
