Summary:
Otherwise useless query is executed:
lang=sql
SELECT c.*
FROM `repository_commit` c
ORDER BY c.epoch DESC
Test Plan: /diffusion/X/browse/x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2186
Test Plan:
/rX1
Browse in Diffusion
Open in Editor
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2180
Summary:
There have been a couple of requests for this since bookmarks are "out this year like woah" and "totally uncool dude".
Allow users to save named custom queries and make them the /maniphest/ default if they so desire.
A little messy. :/
Test Plan: Saved, edited, deleted custom queries. Made custom query default; made 'no default' default. Verified default behavior. Issued a modified search from a custom query.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, 20after4
Maniphest Tasks: T923, T1034
Differential Revision: https://secure.phabricator.com/D1964
Summary:
When Phabricator is configured to generate patch email, we'll fatal if the patch contains binaries and is generating to Git because ArcanistBundle can't load the binary data. Provide a callback to load the data. See D2174.
(This may cause us to generate absolutely enormous emails, but you get what you asked for...)
Test Plan: Created a diff with an image under "send git patches" email configuration.
Reviewers: Makinde, btrahan, vrana, jungejason
Reviewed By: Makinde
CC: aran
Differential Revision: https://secure.phabricator.com/D2175
Summary:
PHP arrays have an internal "current position" marker. (I think because foreach() wasn't introduced until PHP 4 and there was no way to get rid of it by then?)
A few functions affect the position of the marker, like reset(), end(), each(), next(), and prev(). A few functions read the position of the marker, like each(), next(), prev(), current() and key().
For the most part, no one uses any of this because foreach() is vastly easier and more natural. However, we sometimes want to select the first or last key from an array. Since key() returns the key //at the current position//, and you can't guarantee that no one will introduce some next() calls somewhere, the right way to do this is reset() + key(). This is cumbesome, so we introduced head_key() and last_key() (like head() and last()) in D2161.
Switch all the reset()/end() + key() (or omitted reset() since I was feeling like taking risks + key()) calls to head_key() or last_key().
Test Plan: Verified most of these by visiting the affected pages.
Reviewers: btrahan, vrana, jungejason, Koolvin
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2169
Summary:
There was a typo:
`PHID-!!!!-NO_PROJECT` instead of
`PHID-!!!!-NO-PROJECT`
Also use `<em>` to differentiate from project named "(No Project)".
Test Plan:
/maniphest/report/project/
Click on (No Project).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2167
Test Plan: I didn't repro it probably because of custom rules.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T1090
Differential Revision: https://secure.phabricator.com/D2150
Summary: See T1021. Raise configuration or implementation exceptions immediately. When all engines fail, raise an aggregate exception with details.
Test Plan: Forced all engines to fail, received an aggregate exception. Forced an engine to fail with a config exception, recevied it immediately.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1021
Differential Revision: https://secure.phabricator.com/D2157
Summary: In Safari, Firefox and Chrome, respect cursor position and selection ranges.
Test Plan: Dragged-and-dropped files into the middle of text, end of text, and a selected text range in Safari, Firefox and Chrome. Copy/pasted files into similar cases in Chrome. Got expected, normal behavior in all cases.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1016
Differential Revision: https://secure.phabricator.com/D2155
Summary: See discussion in T789. Covered the obvious cases, at least. We can refine this as we get a larger sample size.
Test Plan: Unit test coverage.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T789
Differential Revision: https://secure.phabricator.com/D2154
Summary: See T993. MySQL fails very very softly if you request an engine which does not exist. Detect and fail if the InnoDB engine is missing or broken.
Test Plan: Faked InnoDB missing, got a failure. Ran normally, got success.
Reviewers: btrahan, vrana, jungejason
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T993
Differential Revision: https://secure.phabricator.com/D2160
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: These elements look heavy and out of place right now.
Test Plan: Looked at error views in uiexample page.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2144
Summary: Currently, we show them everything. Instead, show them an explicit notice.
Test Plan: Looked at "My Projects" feed with no projects.
Reviewers: btrahan, vrana, jungejason
Reviewed By: jungejason
CC: aran
Maniphest Tasks: T1015
Differential Revision: https://secure.phabricator.com/D2143
Summary:
- Make some effort to simplify the code.
- Make "Skip Past This Commit" work in Git and Mercurial.
- Make blame work in Mercurial.
- Add tooltip hover state to show more information about commits.
Test Plan: Viewed blame views in SVN, Git, Hg. Clicked line numbers, hovered/clicked commits, hovered/clicked "blame past..."
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Maniphest Tasks: T378
Differential Revision: https://secure.phabricator.com/D2142
Summary: See D2080. The introduction of `arc land`, defaulting to `origin/master`, and --auto enormously simplifies the documentation.
Test Plan: Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T894
Differential Revision: https://secure.phabricator.com/D2082
Summary:
The older logic was incorrect:
- It chose `change.left` for `data.on_right` being true.
- 'O' and 'N' mean 'old' and 'new', not 'left' and 'right'. In diff-of-diffs, both sides are 'N'.
So, select the changeset ID correctly (pick the right side one for on_right), and select the new file prefix correctly (N for new, O for old).
Test Plan: Waved my mouse over some inline comments in a diff-of-diffs, got reasonable-looking reticles.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1076
Differential Revision: https://secure.phabricator.com/D2138
I think the issue is that we don't set the left-side changesetID correctly. This seems to work correctly locally, but I'm not sure I got a good repro. Pushing to verify the production test cases provided in T1076.
Auditors: vrana, btrahan
Summary:
The audit tools has many false positive about Author Not
Matching with Revision. The fix is to set the authorPHID which was
missing in the existing code
Test Plan:
run reparse.php and it doesn't generate false positive result
anymore.
Reviewers: nh, epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2119
Summary: renderMiniPanel() renders the entire <p>.
Test Plan: Looked at page source for homepage, verified there was no double </p>.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1079
Differential Revision: https://secure.phabricator.com/D2128
Summary: This separates common MySQL stuff (identifiers and comments escaping, error codes, connection retries) from PHP extension specific stuff (connect, query, fetch, errors, escape string).
Test Plan:
/
Use `AphrontMySQLiDatabaseConnection` in `PhabricatorLiskDAO`, load homepage, edit task, save task.
Reviewers: epriestley
Reviewed By: epriestley
CC: nh, aran
Differential Revision: https://secure.phabricator.com/D2113
Summary: I've found it useful mainly on smaller screen or with lots of comments.
Test Plan: Show Diff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2122
Summary:
This introduces some boundary checking for
PhabricatorOwnersOwner::loadAffiliatedUserPHIDs() if it gets passed an empty
array, which happened when I ran arc diff and it called
differential.createrevision.
Test Plan: ran arc diff
Reviewers: epriestley, meitros, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2112
Summary:
MySQL doesn't treat `\` as escaping character in ##``##.
This isn't probably SQL injection hole because I've found no calls of this method with user input.
But better safe than sorry.
See also [[http://dev.mysql.com/doc/refman/5.1/en/server-sql-mode.html#sqlmode_no_backslash_escapes | NO_BACKSLASH_ESCAPES]].
Test Plan:
lang=sql
SELECT `a\`b`; -- Throws: Syntax error near '`'.
-- Should throw: Unknown column 'a`b'.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2109
Summary:
I've considered that user may have set editor but not checked out Phabricator repositories.
But stack trace is useful mainly for developers.
Test Plan:
Click on path in Unhandled Exception.
Repeat with disabled editor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2107
Summary: ...also make the pager usage in ChatLog use the nice formatWhereClause functionality
Test Plan: set $page_size = 2 and paged around the data a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Maniphest Tasks: T905
Differential Revision: https://secure.phabricator.com/D2106
Summary: Use Edges to attach Commits and Tasks. Note, no "edit attached commits" interface from tasks yet since the search backend needs a little work to list commits in a sensible way.
Test Plan: Attached commits to tasks. Looked at commits, looked at tasks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2105
Summary:
various stripe stuff, including
- external stripe library
- payment form
- test controller to play with payment form, sample business logic
My main questions / discussion topics are...
- is the stripe PHP library too big? (ie should I write something more simple just for phabricator?)
-- if its cool, what is the best way to include the client? (ie should I make it a submodule rather than the flat copy here?)
- is the JS I wrote (too) ridiculous?
-- particularly unhappy with the error message stuff being in JS *but* it seemed the best choice given the most juicy error messages come from the stripe JS such that the overall code complexity is lowest this way.
- how should the stripe JS be included?
-- flat copy like I did here?
-- some sort of external?
-- can we just load it off stripe servers at request time? (I like that from the "if stripe is down, stripe is down" perspective)
- wasn't sure if the date control was too silly and should just be baked into the form?
-- for some reason I feel like its good to be prepared to walk away from Stripe / switch providers here, though I think this is on the wrong side of pragmatic
Test Plan: - played around with sample client form
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2096
Summary:
We have a lot of cases where we store object relationships, but it's all kind of messy and custom. Some particular problems:
- We go to great lengths to enforce order stability in Differential revisions, but the implementation is complex and inelegant.
- Some relationships are stored on-object, so we can't pull the inverses easily. For example, Maniphest shows child tasks but not parent tasks.
- I want to add more of these and don't want to continue building custom stuff.
- UIs like the "attach stuff to other stuff" UI need custom branches for each object type.
- Stuff like "allow commits to close tasks" is notrivial because of nonstandard metadata storage.
Provide an association-like "edge" framework to fix these problems. This is nearly identical to associations, with a few differences:
- I put edge metadata in a separate table and don't load it by default, to keep edge rows small and allow large metadata if necessary. The on-edge metadata seemed to get abused a lot at Facebook.
- I put a 'seq' column on the edges to ensure they have an explicit, stable ordering within a source and type.
This isn't actually used anywhere yet, but my first target is attaching commits to tasks for T904.
Test Plan: Made a mock page that used Editor and Query. Verified adding and removing edges, overwriting edges, writing and loading edge data, sequence number generation.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, 20after4
Differential Revision: https://secure.phabricator.com/D2088
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2103
Summary: I looooove JS! It makes me giddy with glee!
Test Plan: Picked dates. See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2086
Summary:
When system agent adds a comment then he is added to CC.
When I amend and update then I get message "Commit message references nonexistent ..."
Test Plan: Update revision with system agent in CC.
Reviewers: epriestley
Reviewed By: epriestley
CC: michalburger1, aran
Differential Revision: https://secure.phabricator.com/D2100
Test Plan:
Jump to head.
Go to doctor.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2097
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around Diffusion.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2094
Summary:
- The UI is pretty straightforward, since Handle just works (tm)
- Added two methods to the owners object to handle the new layer of
indirection. Then ran git grep PhabricatorOwnersOwner and changed
callsites as appropriate.
Sending this to get a round of feedback before I test the non-trivial
changes in this diff.
Test Plan:
- owners tool: edit, view, list for basic functionality.
- phlog for the two new methods I added
Reviewers: epriestley, blair, jungejason
CC: aran
Differential Revision: https://secure.phabricator.com/D2079
Summary: NOTE: This is not produced by a script so there might be errors. Please review carefully.
Test Plan: Browse around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2091
Summary: Delete some dead code in Feed along the way.
Test Plan:
/feed/
/search/
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2092
Summary:
Most setters returns `$this` but some don't.
I guess it's not by purpose.
Test Plan:
arc lint
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2085
Summary: As title
Test Plan: hit diffcamp, owners to test HandleData
Reviewers: epriestley, btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D2063
Summary:
I wanted to search for D1234 in texts of other documents.
But search tool always redirects me.
I've left the redirect behavior for simple search forms (header and home) and removed it from full search form.
I don't consider this complete because the first result in search for D1234 should be of course D1234 which is not the case currently.
I am not sure how to solve it:
- We can display a special result in this case.
- We can index the documents so that they will be searchable also for short strings.
I tend to use the first solution because revisions can be truncated at arbitrary length (rX1f1f1f should display revision rX1f1f1f1f1f1f1f).
Test Plan: Search for D1234, rX123, T4.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley, ddfisher
Differential Revision: https://secure.phabricator.com/D1905
Summary: Added a regex to remove the text
Test Plan: Tested a few messages, from mail application them gmail, both seemed fine, will add unit tests
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2078
Summary:
Show application names, then a human-readable description of what they're for.
Eventually we'll have better help / tutorial / onboarding / etc systems too.
Test Plan: See screenshot.
Reviewers: btrahan, mgummelt
Reviewed By: btrahan
CC: aran, davidreuss
Differential Revision: https://secure.phabricator.com/D2075
Summary:
I've found it quite useful to know day of week of most displayed dates.
This is useful in periodic workflows (e.g. cut on Sunday, push on Tuesday).
It adds the day only to recent dates to save some space (similar approach as `ls -l`).
Test Plan: /
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T1034
Differential Revision: https://secure.phabricator.com/D1984
Summary:
Like the title says, similar to Facebook Tasks.
Not sure how I really feel about this, but I guess it's kind of OK? I never used
this feature in Facebook Tasks but I think some people like it.
The drag-and-drop to repri across priorities feels okayish.
Because subpriority is a double and we just split the difference when
reprioritizing, you lose ~a bit of precision every time you repri two tasks
against each other and so you can break it by swapping the priorities of two
tasks ~50 times. This case is pretty silly and pathological. We can add some
code to deal with this at some point if necessary.
I think this also fixes the whacky task layout widths once and for all.
(There are a couple of minor UI glitches like headers not vanishing and header
counts not updating that I'm not fixing because I am lazy.)
Test Plan: Dragged and dropped tasks around.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley, mgummelt
Maniphest Tasks: T859
Differential Revision: https://secure.phabricator.com/D1731
Summary:
We'll incorrectly send CCWelcome mail to users who would be added as CCs but are blocked by the new "$dont_add" stuff, for
example when a revision is updated and the user has a Herald rule which triggers them getting CC'd. See D2057.
Potentially a better fix for this would be to have "addCCs" return a list of the CCs it actually added, rather than duplicating the
logic of removing CCs in two places. However, that's not trivial since it's just a wrapper around alterRelationships() which is nasty
and would need a more complicated return type. I think this whole thing will get a refactoring pass at some point -- I want to build a
more generic "associations"-like datastore and replace some of the ad-hoc associations with it. So maybe I can clean it up when that
happens. For now, this should fix the immediate problem.
Test Plan: Updated a revision, didn't get CC welcomed.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2072
Summary:
- Remove the "Priority" column, since this is indicated by the color swatch, to save space.
- Reduce the "Updated" column from datetime to date only, since time isn't incredibly useful, to save space.
- Show the first two projects a task is associated with, and "..." if there are more.
- Show "None" (for "no owner") in a lighter color.
Test Plan: Looked at tasks on homepage and in Maniphest.
Reviewers: btrahan, 20after4
Reviewed By: btrahan
CC: aran, edward
Maniphest Tasks: T967
Differential Revision: https://secure.phabricator.com/D2065
Summary: Change CSS style name from code to pre. This depends on D2067.
Test Plan: Viewed the html from Firefox
Reviewers: epriestley
Reviewed By: epriestley
CC: jungejason, aran
Maniphest Tasks: T207
Differential Revision: https://secure.phabricator.com/D2068
Summary:
Format a date as 'today', 'yesterday', or 'Mar 27 2012'. Optionally,
the final example can be rendered 'on Mar 27 2012' for things like:
$excuse =
'I fell out of a window '.
phabricator_on_rel_date($time, $me);
Test Plan: Tested in my sandbox!!!!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2035
Summary:
Boolean search supports operators, such as phrase search.
It can be further improved by setting [[http://dev.mysql.com/doc/mysql/en/server-system-variables.html#sysvar_ft_boolean_syntax | ft_boolean_syntax]] to `' |-><()~*:""&^'` (note the leading space):
Default value uses no operator for "optional word" and `+` for "mandatory word".
This value uses no operator for "mandatory word" and `|` for "optional word".
Test Plan: Search for "Enter the name" (with quotes).
Reviewers: epriestley
Reviewed By: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D2064
Summary:
Update the Herald documentation with information about global vs personal rules.
NOTE: Some of the features mentioned here don't exist yet (flags, delete rules), I'll add them before I land this.
Test Plan: Read documentation
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2031
Summary:
- Differential, Maniphest and Diffusion use slightly different styles for the object detail panels.
- Instead, use the same styles and CSS.
- Add object actions to Diffusion, including "Flag".
Test Plan: Looked at revisions, tasks and commit. Flagged and unflagged commits.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2062
Summary:
**Who can delete global rules?**: I discussed this with @jungejason. The current behavior is that the rule author or any administrator can delete a global rule, but this
isn't consistent with who can edit a rule (anyone) and doesn't really make much sense (it's an artifact of the global/personal split). I proposed that anyone can delete a
rule but we don't actually delete them, and log the deletion. However, when it came time to actually write the code for this I backed off a bit and continued actually
deleting the rules -- I think this does a reasonable job of balancing accountability with complexity. So the new impelmentation is:
- Personal rules can be deleted only by their owners.
- Global rules can be deleted by any user.
- All deletes are logged.
- Logs are more detailed.
- All logged actions can be viewed in aggregate.
**Minor Cleanup**
- Merged `HomeController` and `AllController`.
- Moved most queries to Query classes.
- Use AphrontFormSelectControl::renderSelectTag() where appropriate (this is a fairly recent addition).
- Use an AphrontErrorView to render the dry run notice (this didn't exist when I ported).
- Reenable some transaction code (this works again now).
- Removed the ability for admins to change rule authors (this was a little buggy, messy, and doesn't make tons of sense after the personal/global rule split).
- Rules which depend on other rules now display the right options (all global rules, all your personal rules for personal rules).
- Fix a bug in AphrontTableView where the "no data" cell would be rendered too wide if some columns are not visible.
- Allow selectFilter() in AphrontNavFilterView to be called without a 'default' argument.
Test Plan:
- Browsed, created, edited, deleted personal and gules.
- Verified generated logs.
- Did some dry runs.
- Verified transcript list and transcript details.
- Created/edited all/any rules; created/edited once/every time rules.
- Filtered admin views by users.
Reviewers: jungejason, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2040
Summary:
Herald rules are adding CC also for Author and Reviewer.
See also D1397.
I was considering also just don't displaying the extra CC but this is probably better.
There are still cases where there could be reviewer in CC (e.g. by making reviewer from CC or by direct edit) but I think it's not a big problem.
Beeing both Reviewer and CC can be actually useful (e.g. if you resign than you still are in CC) but it's not that useful to justify this:
Author: vrana
Reviewers: epriestley
CCs: vrana, epriestley
Test Plan: Comment on revision where I am author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2057
Summary:
I think this feature is probably good, but Differential is also really starting to get a lot of stuff which is not the diff in it. Not sure how best to deal with that.
The mixed table styles are also pretty ugly.
So I guess this is more feedback / proof-of-concept, I think I want to try to improve it somehow before I land it.
Test Plan: Looked at some diffs, some had an awkward, ugly list of diffs affecting the same files.
Reviewers: bill, aran, btrahan
Reviewed By: aran
CC: aran, epriestley
Maniphest Tasks: T829
Differential Revision: https://secure.phabricator.com/D2027
Summary:
These are the issues identified by the linter in D2052. I don't think any cause bugs, but they are all reasonable errors to raise and the linter correctly
detected that they are suspicious.
Test Plan: Mostly inspection.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2053
Summary: We may overwrite $comment as a side effect of iteration.
Test Plan: Made some audit comments as different users.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2050
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
When there is a single line Test Plan (or anything else) then `arc amend` puts it on the same line as label.
It is a problem with indented line (as in this diff) because next run of `arc diff` will trim the leading spaces.
Test Plan: arc amend
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2054
Summary:
The full path field of the DiffusionRepositoryPath object is used by the
DiffusionBrowseController when viewing a directory with a readme file, so
we should set this field.
Test Plan: loaded a directory containing a readme in a svn repo
Reviewers: epriestley, jungejason, emiraga
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2045
Test Plan: Looked at /differential/ with and without flags.
Reviewers: nh, btrahan
Reviewed By: nh
CC: aran, epriestley
Maniphest Tasks: T1055
Differential Revision: https://secure.phabricator.com/D2044
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:
- When an inline comment preview corresponds to an inline comment on the page, link to it. Just punt in the tough case where the inline is on some other page.
- In "haunted" mode, "z" now toggles through three modes: normal, comment area only, and comment + previews.
Test Plan:
- Viewed visible and not-visible inline comment previews, clicked "View" links.
- Tapped "z" a bunch to toggle haunt modes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T517, T214
Differential Revision: https://secure.phabricator.com/D2041
Summary:
I'd like to use this regex elsewhere and copying and pasting is
bad.
Test Plan: none
Reviewers: casey, epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2038
Summary: 'cuz I miss out on chat room goodness and can't paginate around in the current version
Test Plan: setup a phabot and spammed it in phabot-test. with new test data, set $page_limit = 1 and paged about -- looks good!
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Maniphest Tasks: T990
Differential Revision: https://secure.phabricator.com/D2032
Summary:
Flags are a personal collection of things you want to take a look at later. You can use several different colors and add notes.
Not really sure if this is actually a good idea or not but it was easy to build.
Planned features:
- Allow Herald rules to add flags.
- In the "edit flag" dialog, have a "[x] Subscribe Me" checkbox that CCs you.
- Support Diffusion.
- Support Phriction.
- Always show flags on an object if you have them (in every view)?
- Edit dialog feels a little heavy?
- More filtering in /flag/ tool.
- Add a top-level links somewhere?
Test Plan: Added, edited and removed flags from things. Viewed flags in flag view.
Reviewers: aran, btrahan
Reviewed By: btrahan
CC: aran, epriestley, Koolvin
Maniphest Tasks: T1041
Differential Revision: https://secure.phabricator.com/D2024
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:
If a file was last modified at revision 50 and you look at revision 55, we currently 404. Instead, always identify the last modification.
Also simplify some of the query objects.
Test Plan: Viewed after-modification revisions for several files in SVN.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran, epriestley
Maniphest Tasks: T851
Differential Revision: https://secure.phabricator.com/D2028
Summary: See T955. We jump to an awkard place right now; jump above the comment instead.
Test Plan: Clicked inline comment anchor links.
Reviewers: Makinde, btrahan
Reviewed By: Makinde
CC: aran, epriestley
Maniphest Tasks: T955
Differential Revision: https://secure.phabricator.com/D2029
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: D2023 adds a new '*' token to javelinsymbols (indicating that a behavior is 'installed'). This fixes a sanity-check regex in PhabricatorJavelinLinter that validates the output of javelinsymbols so that it is aware of this new token type.
Test Plan:
Patched javelinsymbols.cpp from D2023 to externals/javelin/support/javelinsymbols, build the new javelinsymbols binary, then ran
arc lint --lintall webroot/rsrc/js/application/core/behavior-drag-and-drop-textarea.js
(before this diff, that throws an error -- after it works with no lint)
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2025
Summary: Show parent commit information to make it easier to understand merges.
Test Plan: Looked at commits in SVN, hg, git.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2021
Summary: COMPLETELY ORIGINAL IDEA
Test Plan: Browsed around Phabricator, got helpful readmes in some cases.
Reviewers: davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2022
Summary: This header allows recipients to distinguish between CCs generated by Herald and CCs generated by humans.
Test Plan: Created a Herald rule to add a bunch of CC's to every revision. Created a revision. Added some CCs manually. Verified that only manual CCs appeared in the "Explicit" header.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T808
Differential Revision: https://secure.phabricator.com/D2018
Summary:
See some discussion in D2002. Add two new actions:
- Resign: (auditor only) closes your open request (user request ONLY) by putting it in a "resigned" state.
- Close: (author only) closes all open requests by putting them in a "closed" state.
@davidreuss, this is probably conflict-city with D2002 -- I'll wait for you to land first and then handle the merge on my end.
Test Plan: Resigned from and closed audits.
Reviewers: 20after4, davidreuss, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T904
Differential Revision: https://secure.phabricator.com/D2013
Summary: A bunch of installs are doing this to varying degrees of success anyway, make it easier and nudge them toward a more consistent approach.
Test Plan: Set a custom logo, viewed normal and admin pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T700
Differential Revision: https://secure.phabricator.com/D2019
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: Adds an "allactive" filter in addition to the all projects filter.
Test Plan:
visit All Active view (/project/filter/allactive/) and
see that it lists all projects except those which have been archived.
Visit other filter views to be sure nothing else got broken.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2010
Summary:
$stdout from the previous run would be reused if an exception
occurred
Test Plan: that's a negative, ghostrider.
Reviewers: epriestley, jungejason
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D2008
Summary:
When a commit is a merge, show what it merged.
Also fix some bugs:
- Mercurial queries may contain ":", but mercurial rev ranges may also contain ":". A rev range with a branch that has a ":" in it is ambigiuous, e.g. branch "a:b" might appear in a rev range like "a🅱️0", which can not be parsed. Use stable commit names instead.
- Mercurial stable commit name implementation was broken, fix it.
- Extend DiffusionHistoryQuery from DiffusionQuery to share code.
- Fix a bug where Mercurial's main browse list would not show the most recent commit if it was a merge commit.
Test Plan: Generated a bunch of mercurial/git merge commits and looked at them, they seemed to accurately represent the repository state.
Reviewers: btrahan, Makinde
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T961
Differential Revision: https://secure.phabricator.com/D2005
Summary:
We render a huge picture of a PDF for PDFs right now, etc. This is hella dumb.
Also allow users to force this rendering style, and change the link name.
Test Plan: Uploaded image and non-image files, used layout=link and name=....
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1040
Differential Revision: https://secure.phabricator.com/D2006
Summary:
- Show the canonical (i.e., shorter) commit identifier in the subject.
- For commits without a revision, put the commit summary in the subject.
Test Plan: Ran "scripts/repository/reparse.php <commit> --herald" for a number of different commits (with revision, without revision); got more useful email subjects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1028
Differential Revision: https://secure.phabricator.com/D2004
Summary: $link gets reused later in the function, use a different variable name to avoid broken nonsense.
Test Plan: Clicked users/projects links.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1038
Differential Revision: https://secure.phabricator.com/D2003
Summary:
In the Differential revision list views:
- Allow you to filter by mailables (notably, mailing lists).
- Allow you to filter by user (including disabled users).
Test Plan: Filtered by a mailing list.
Reviewers: btrahan, nh
CC: aran, epriestley
Maniphest Tasks: T1031
Differential Revision: https://secure.phabricator.com/D1994
Test Plan:
Run setup with 'differential.attach-task-class' set to:
- ''
- 'FacebookTasksAttacher'
- 'X'
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1999
Summary: I'll mark this one up inline since it's all separate bugs.
Test Plan:
- Created a diff with eight changes: (newline absent -> newline present, newline present -> newline absent, newline present -> newline present, newline absent -> newline absent) x (short file with change near end, long file with change near middle).
- Viewed diff in Ignore All, Ignore Most, Ignore Trailing and Show All whitespace modes.
- All 32 results seemed sensible.
- Really wish this stuff was better factored and testable. Need to fix it. :(
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T1030
Differential Revision: https://secure.phabricator.com/D1992
Summary: Apparently I spent like a good month copy/pasting slightly different versions of this logic all over the codebase.
Test Plan: Selected "View Options -> Browse in Diffusion" for a chagneset, got a URI with a branch name in it under Git.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1993
Summary:
- We need to sort Projects explicitly because we go through task-by-task which ruins the ordering. My test case was just small enough not to notice.
- Push "No Project" to the bottom explicitly.
- Simplify/fix the pull of "Unassigned" to the top.
- Fix a return type.
Test Plan: Tested the various sorting cases against a larger test data set.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D1988
