Summary:
Currently, we can sit in a sleep() for up to 15 seconds (or longer, in some cases), even if there's a parse request.
Try polling the DB every second to see if we should wake up instead. This might or might not produce significant improvements, but seems OK to try and see.
Also a small fix for logging branch names with `%` in them, etc.
Test Plan: Ran `phd debug pulllocal` and pushed commits, saw them parse within a second.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7998
Summary:
Ref T4327. This moves the last pieces of discovery responsibility out of the PullLocal daemon and into the DiscoveryEngine.
(This makes it easier to discover repositories in unit tests in the future, since we don't need to build a PullLocal daemon and can just build a DiscoveryEngine.)
Test Plan: Ran `phd debug pulllocal`. Ran `repostory discover`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7997
Summary: be sure to use array() (and not null) so we don't fatal if we have no signatures
Test Plan: no more fatal when editing a signature-less document
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8000
Summary: we need this for legalese. Ref T3116
Test Plan: made a legalpad document with underlines. also re-gened docs and noted underlines worked
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7996
Summary:
Ref T4327. Consolidates and simplifies infrastructure:
- Moves Git discovery into DiscoveryEngine.
- Collapses a bunch of the Git and Mercurial code related to stream discovery.
- Removes all cach code from PullLocal daemon (it's no longer called).
- Adds basic unit tests for Git and Mercurial discovery.
Various cleanup:
- Makes GitStream and MercurialStream extend a common base.
- Improves performance of MercurialStream in some cases, by requiring fewer commits be output and parsed.
- Makes mirroring exceptions easier to debug.
- Fixes discovery of Mercurial repositories with multiple branch heads.
- Adds some missing `pht()`.
Test Plan:
I tested this fairly throughly because I think this phase is complete:
- Made new repositories in multiple VCSes and did full imports.
- Particularly, I reimported Arcanist to make sure that TODO was resolved. I think it was related to the toposort stuff.
- Pushed commits to multiple VCSes.
- Pushed commits to a non-close branch, then pushed a merge commit. Observed commits import initially as non-close, then get flagged for close.
- Started full daemons and resolved various minor issues that showed up in the daemon log until everything ran cleanly.
- Basically spent about 30 minutes banging on this in every way I could think of to try to break it. I found and fixed some minor stuff, but it seems solid.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7987
Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.
In particular:
- As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
- Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
- We no longer need to do a separate discovery step on closable branches.
- We no longer need to keep track of `seenOnBranches`.
Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7985
Summary:
Ref T4327. This provides a more general RefCursor-based way to identify closeable commits, and moves away from the messy `seenOnBranches` stuff. Basically:
- When a closeable ref (like the branch "master") is updated, query the VCS for all commits which are ancestors of the new ref, but not ancestors of the existing closeable heads we previously knew about. This is basically all the commits which have been merged or moved onto the closeable ref.
- Take these commits and set the "closeable" flag on the ones which don't have it yet, then queue new tasks to reprocess them.
I haven't removed the old stuff yet, but will do that shortly.
Test Plan:
- Ran `bin/repository discover` and `bin/repository refs` on a bunch of different VCSes and VCS states. The effects seemed correct (e.g., new commits were marked as closeable).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7984
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary: Ref T4327. Moves us one small step forward toward testable change parsers by separating out this unrelated logic from the PullLocal daemon. We will also probably want to run this logic so we can do remote path lookups to limit the role of Arcanist Projects in the future, which is why I made the URI type (here, only "git") a parameter rather than calling this a `GitURINormalizer` or something.
Test Plan:
- Ran unit tests.
- Ran `repository discover` on a correctly-configured remote repository.
- Ran `repository discover` on an incorrectly-configured remote, got an error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7981
Summary: doh
Test Plan: no more fatal on home page
Reviewers: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7993
Summary: ...needs to add a LegalpadDocumentSignatureQuery class to get this done, which is also re-deployed everywhere we were issuing raw queries. Ref T3116.
Test Plan: viewed some signatures. Verified color and footer icons showed up how I wanted them to.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7986
Summary: Not sure this would have avoided the issue, but I remember a couple of other people asking about migrations, so try to make it more clear/obvious that the backup tools are also useful for migrations. Although this is reasonably obvious when you think about it, it's not very obvious when you're trying to do a migration, and maybe making it more explicit will help.
Test Plan: Read new documentation.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7992
Summary:
From @chad. This setup link should open in a new window so you don't lose your context in resolving setup issues.
@chad, this was the only one I could find immediately, let me know if you remember seeing others that I missed.
Test Plan: Faked an error, clicked the link, got a new tab.
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7991
Summary: Cookie-prefix should fix phabricator instances where x.com and x.y.com have conflicting cookie names
Test Plan: Pushed branch to dev.phab.example.com, logged into phab.example.com and into dev.phab.example.com.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7979
Summary:
does a few smallish things... Ref T3116
- adds an action to "sign document", thus improving visiblity of this feature from 0 to some value more than 0
- adds a crumb on the edit page to get back to the view page
- warns the user on the edit page IFF signatures exist for the current version that their edits could invalidate those signatures
- adds a "needSignatures" option to the Document Query class
Test Plan: click the new UI elements and they worked. edited a document with signatures, noted warning UI, edited anyway, noted warning UI correctly disappeared on new edit. also verified a single signature had the correct translation
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7983
Summary:
Ref T4310. Fixes T3720. This change:
- Removes concurrent session limits. Instead, unused sessions are GC'd after a while.
- Collapses all existing "web-1", "web-2", etc., sessions into "web" sessions.
- Dramatically simplifies the code for establishing a session (like omg).
Test Plan: Ran migration, checked Sessions panel and database for sanity. Used existing session. Logged out, logged in. Ran Conduit commands.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7978
Summary:
Ref T3116. This creates a policy rule where you can require a signature on a given legalpad document.
NOTE: signatures must be for the *latest* document version.
Test Plan: made a task have a custom policy requiring a legalpad signature. verified non-signers were locked out.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3116
Differential Revision: https://secure.phabricator.com/D7977
Summary:
Ref T3720. Ref T4310. Currently, we limit the maximum number of concurrent sessions of each type. This is primarily because sessions predate garbage collection and we had no way to prevent the session table from growing fairly quickly and without bound unless we did this.
Now that we have GC (and it's modular!) we can just expire unused sessions after a while and throw them away:
- Add a `sessionExpires` column to the table, with a key.
- Add a GC for old sessions.
- When we establish a session, set `sessionExpires` to the current time plus the session TTL.
- When a user uses a session and has used up more than 20% of the time on it, extend the session.
In addition to this, we could also rotate sessions, but I think that provides very little value. If we do want to implement it, we should hold it until after T3720 / T4310.
Test Plan:
- Ran schema changes.
- Looked at database.
- Tested GC:
- Started GC.
- Set expires on one row to the past.
- Restarted GC.
- Verified GC nuked the session.
- Logged in.
- Logged out.
- Ran Conduit method.
- Tested refresh:
- Set threshold to 0.0001% instead of 20%.
- Loaded page.
- Saw a session extension ever few page loads.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7976
Summary:
Ref T4310. Ref T3720. Two major things are going on here:
- I'm making this table work more like a standard table, which, e.g., makes `delete()` simpler to implement.
- Currently, the primary key is `(userPHID, type)`. I want to get rid of this, issue unlimited sessions, and GC old sessions. This means we can't have a unique key on `(userPHID, type)` anymore. This removes it as the primary key and adds it as a normal key instead. There's no functional change -- the code to generate sessions guarantees that it will never write duplicate rows or write additional rows -- but allows us to drop the `-1`, `-2` qualifiers in the future.
- Also of note, our task is made far simpler here because MySQL will automatically assign values to new `AUTO_INCREMENT` columns, so we don't have to migrate to get real IDs.
Test Plan: Ran migrations, verified table looked sane. Logged out, logged in.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3720, T4310
Differential Revision: https://secure.phabricator.com/D7975
Summary: This modularizes the rest of the GC submethods. Turned out there was nothing tricky.
Test Plan: Ran `bin/phd debug garbage` and got reasonable looking behavior and output.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7971
Summary:
The GC is a big block of hard-coded application GCs right now. Among other things, this means third parties can't tap into the infrastructure.
Modularize it into `GarbageCollector` classes. This implements only one to prove the new stuff works; I'll followup with the rest in the next diff or few depending on how much mess I run into.
Test Plan: Used `bin/phd debug garbage` to run the collector in debug mode, observed reasonable output and behavior.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7970
Summary: Adds "verified" and "secretKey" to Legalpad document signatures. For logged in users using an email address they own, things are verified right away. Otherwise, the email is sent a verification letter. When the user clicks the link the signature is marked verified.
Test Plan: signed the document with a bogus email address not logged in. verified the email that would be sent looked good from command line. followed link and successfully verified bogus email address
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, asherkin
Maniphest Tasks: T4283
Differential Revision: https://secure.phabricator.com/D7930
Summary: Fixes T4317. Update the "inline comment" control to a RemarkupControl. This could maybe use some padding/spacing/design touches eventually but seems OK for the moment.
Test Plan: {F101825}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T4317
Differential Revision: https://secure.phabricator.com/D7969
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary:
Ref T3857.
- Always send mail via daemons. This lets us get rid of this config, and is generally much more performant.
- After D7964, we warn if daemons aren't running.
Test Plan: Sent some mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7965
Summary:
Currently, we try to mostly-kind-of-work if daemons aren't running (for example, we send mail in-process). I want to stop doing this. A major motivator is that `metamta.send-immediately` is confusing for a lot of users and frequently the cause of performance problems. Increasingly, functionality of applications depends on the daemons (Harbormaster, Drydock, Nuance all require daemons to do anything at all). They're also fairly stable/robust/well-tested and no reasonable install should be running without them.
This will let us simplify or remove some flags (like `metamta.send-immediately`) and simplify some other processes like search indexing.
Test Plan: Stopped daemons, loaded warnings, saw daemon warning. Started daemons, reloade, no warning.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7964
Summary: Ref T4310. Ref T3720. We use bare strings to refer to session types in several places right now; use constants instead.
Test Plan: grep; logged out; logged in; ran Conduit commands.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7963
Summary: Ref T4310. Ref T3720. Session operations are currently part of PhabricatorUser. This is more tightly coupled than needbe, and makes it difficult to establish login sessions for non-users. Move all the session management code to a `SessionEngine`.
Test Plan:
- Viewed sessions.
- Regenerated Conduit certificate.
- Verified Conduit sessions were destroyed.
- Logged out.
- Logged in.
- Ran conduit commands.
- Viewed sessions again.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310, T3720
Differential Revision: https://secure.phabricator.com/D7962
Summary: Adds the ability to set icons into Tags.
Test Plan: tested on UIExamples page.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7961
Summary: Ref T4310. Ref T3720. Partly, this makes it easier for users to understand login sessions. Partly, it makes it easier for me to make changes to login sessions for T4310 / T3720 without messing anything up.
Test Plan: {F101512}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3720, T4310
Differential Revision: https://secure.phabricator.com/D7954
Summary:
Ref T4310. This is a small step toward separating out the session code so we can establish sessions for `ExternalAccount` and not just `User`.
Also fix an issue with strict MySQL and un-admin / un-disable from web UI.
Test Plan: Logged in, logged out, admined/de-admin'd user, added email address, checked user log for all those events.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4310
Differential Revision: https://secure.phabricator.com/D7953
Summary: Add the 'Depends On' field to releeph requests. This will help the release engineers to be aware of the dependencies and make sure pick them altogether.
Test Plan: Check sandbox. This field shows up when a revision has some dependencies.
Reviewers: JoelB, lifeihuang, epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7946
Summary: See IRC. This is a likely fix for @DctrWatson's error: we're returning `null` but should return `array()` to indicate no values.
Test Plan: Will make @DctrWatson do it.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7950
Summary: This might be `null`.
Test Plan: Loaded, got exception, applied patch, no exception. Viewed a method with an actual message too, that also worked.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7955
Summary: Ref T1344. We try to do a bad edge query with no sources right now if there are no tasks in a project.
Test Plan:
- Hit exception, applied patch, no exception.
- Other boards still have tasks.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7951
Summary:
Ref T1344. When rendering a task's projects, add "(Board)" afterward if the task is on a non-default board.
This is mostly a "get the data there" change, we can probably make the design nicer.
Test Plan: {F101232}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7945
Summary: Ref T1344. Write edges and read them when reloading the board.
Test Plan: After reload, stuff stays mostly where I put it. In-column order isn't always persisted correctly yet.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7944
Summary: Ref T1344. Makes requests to the server, which are received and ignored. Performs appropriate locking/unlocking/enabling/disabling on the client.
Test Plan: Dragged stuff around, saw it enable/disable/send correctly.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7943
Summary:
Ref T1344. Makes the UI/UX a little nicer; still no actual backend stuff. This changes:
- When you drop an item onto a different column, the item actually moves.
- Empty columns render with a special CSS class now, but no nodes in the list. This cleans up some JS jankiness. I made the "empty" columns have a light blue background for now. We could put some sort of subtle background image in them instead, or some kind of call to action if it's not redundant with other UI.
Test Plan: {F101208}
Reviewers: chad, btrahan
Reviewed By: chad
CC: chad, aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7942
Summary:
Ref T1344. Allows you to drag tasks within a column and between columns, and handles all the multi-column state / targeting / ghosting stuff.
This is a UI-only change; you can't actually do anything meaningful with these yet.
Roughly, I added the idea of a DraggableList existing within a "group" of draggable lists. Normally, that group only has one item, but on boards it has all of the columns. Then I made all of the relevant operations just apply to the whole group of lists.
Test Plan:
- Verified existing funtionality in Maniphest and ApplicationSearch is unaffected, by dragging around tasks to reprioritize them and dragging around search items.
- Dragged tasks between columns on a board view.
{F101196}
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7941
Summary: Ref T1344. I need to put sigils on these for drag-and-drop.
Test Plan: Renders the same. Put sigils on 'em.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7940
Summary:
Ref T1344. This fixes two issues with DraggableList:
- In lists which allowed it, you could drag the top item above itself and get a dashed-border ghost item. This didn't make sense and didn't behave well. Just don't treat this operation as valid.
- In lists which allowed it, you could drag any non-top item to the topmost position, then drag it to an invalid position. The dashed-border ghost item would not be removed properly if this happend.
- Also fix some minor leftovers with Celerity.
Test Plan:
- Dragged the first item above itself; now an invalid operation with no ghost.
- Dragged another item to the first position then back to its original position; ghost vanishes.
- Clean lint.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7939
Summary:
Ref T2015. Several fixes:
- `checkForCancellation()` no longer exists, and isn't relevant for resumable stops. Throw it away for now.
- Fix an issue where a build could pass even if the final step failed.
- `phlog()` exceptions so they show up in `bin/harbormaster` and the daemon logs.
- Write an exception log if a step fails.
- Add a "throw an exception" step to debug this stuff more easily.
Test Plan:
- Grepped for `checkForCancellation()`.
- Ran a failing build where the final step caused the failure.
- Observed `phlog()` in `bin/harbormaster` output.
- Observed log in web UI:
{F101168}
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7935
Summary: Cleans up some older layouts to new stuffs.
Test Plan: Test with and without a diff ID.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7949
Summary:
Ref T1344. Autogenerates a "Backlog" column if one does not exist. Assigns all tasks to the backlog column.
For now, this column is always called "Backlog", but we could let it be called other things later.
Test Plan: Loaded a project, got a backlog column, created some columns.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7938
Summary: Ref T1344. Minor tweaks for crumb/link stuff -- @mikn has been doing some work here recently and I want to unblock him.
Test Plan: Viewed board; viewed column edit screen.
Reviewers: chad, btrahan
Reviewed By: chad
CC: mikn, aran
Maniphest Tasks: T1344
Differential Revision: https://secure.phabricator.com/D7937
Summary: Made the edit path of the Edit controller work (before only create worked), also added in the link for the cog icon to actually land you on the edit page for the correct column. Some cleanup too. *cough*
Test Plan:
Click on gear
Look at fancy title you are about to edit
Change it
Enjoy changed title
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7933
Summary:
Fixes T4306. We should clear notifications about a question and its answers when viewing a question page.
(Eventually we might have an answer detail page and send the notification there, and then only clear there, but this cleans things up for now.)
Test Plan: Loaded question page, verified answers appeared as page objects.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4306
Differential Revision: https://secure.phabricator.com/D7928
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.
Test Plan: Test out the forms, see errors without the text.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran, hach-que
Differential Revision: https://secure.phabricator.com/D7924
Test Plan: Did searches in Diffusion using all 3 Hosted values
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7927
Summary: This adds in the create flow for the Project board columns on the super secret board page which totally doesn't do anything right now.
Test Plan:
1. Apply diff.
2. Go to super secret page.
3. Click link close to top with a way too long name.
4. Enter a name for the column.
5. Enjoy a new column briefly before realising you cannot remove it.
6. Stay happy!
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: tmaroschik, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7925
Summary: This also cleans up some code a little bit. Most of the gymnastics are to make sure we call `needProjectPHIDs()` appropriately.
Test Plan: Created new commit and revision rules with this field. Ran commits and revisions through the test console. Field behavior seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7923
Summary: Ref T2015. Allow configuration of default edit/view policies for blueprints. Add create policy. Remove administrative exception in policies.
Test Plan: Configured these settings and created (or, with a restrictive create setting, tried to create) blueprints.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7921
Summary:
Ref T2015. Adds human-readable names to Drydock blueprints.
Also the new patches stuff is so much nicer.
Test Plan: Edited, created, and reviewed migrated blueprints.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7918
Summary:
Via Asana. The tags on Differential mail are wrong in two cases:
- Transactions which submit inline comments but no comment text are not labeled as "comments", but should be.
- Non-close, non-comment transactions are not labeled at all, but should be labeled "other".
Test Plan: Submitted a no-comments, inlines-only transaction and got a message with proper `X-Phabricator-Mail-Tags` header.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7912
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.
Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7914
Test Plan: Created comments with 'silent' both true and empty, received notifcation for only the latter.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7916
Summary: Minor, sets the list to flush to invoke the correct CSS for the ObjectBox.
Test Plan: Reload Page
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7909
Summary: Test flight on Phabricator editing! I mostly looked at other code, thought it was well written and wrote my own code in the other code's image.
Test Plan: Look at icons appearing!
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7911
Summary: Ref T2015. This workflow is a little weird (runs in a dialog, no edit-before-create step, lots of internal classnames). Make it a little more standard.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7908
Summary: Fixes T4290. At least one of the fields (`realname`) may have a list of items, and `strlen(array('first', 'last'))` produces the warning and stack trace in T4290.
Test Plan:
- Edited `realname` from an array value to an array value.
- Hit error.
- Applied patch.
- No more error.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4290
Differential Revision: https://secure.phabricator.com/D7905
Summary: use the loadReviewedBy function, which seems to do what we want -- returns a reviewer IFF the last thing was an accept
Test Plan: i believe in the power of loadReviewedBy
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran, aarwine
Differential Revision: https://secure.phabricator.com/D7903
Summary: Ref T4289. Make it clear that this provider does not currently work with JIRA 5.
Test Plan: Viewed JIRA provider from `/auth/`, saw warnings.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4289
Differential Revision: https://secure.phabricator.com/D7906
Summary:
Updates table design to use new standards, work well in PHUIObjectBox. Fixes T4142
Comma
Test Plan: Tested on Diffusion, Settings, will roll out to more places soon
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T4142
Differential Revision: https://secure.phabricator.com/D7901
Summary: See github issue 413. This diff adds color to the commit view as the user expects *AND* adds green to both audit and commit views. I looked in the history (D6184) and I can't tell how expected green was, but it feels nice to me given differential color coding.
Test Plan: looked at lists of audits and commits with pretty colors.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7900
Summary: Ref T1049. Creates convenience actions at the Buildable level to stop, resume, or restart all builds.
Test Plan:
- Stopped all builds.
- Resumed all builds.
- Restarted all builds.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7899
Summary:
Ref T1049. Improves the UI:
- Pending commands, like "stopping", are shown separately from the current status.
- Pending commands are shown on the list view.
- Builds can be restarted, stopped and resumed from the list view.
- Add a missing crumb.
Test Plan:
{F99022}
{F99023}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7898
Summary: Ref T1049. The logic in the BuildEngine is a little different from the logic on the Build itself. Make these more consistent, and make queued commands more private.
Test Plan: Restarted, stopped, and resumed a build.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7897
Summary:
Ref T1049. Currently you can cancel a build, but now that we're tracking a lot more state we can stop, resume, and restart builds.
When the user issues a command against a build, I'm writing it into an auxiliary queue (`HarbormasterBuildCommand`) and then reading them out in the worker. This is mostly to avoid race messes where we try to `save()` the object in multiple places: basically, the BuildEngine is the //only// thing that writes to Build objects, and it holds a lock while it does it.
Test Plan:
- Created a plan which runs "sleep 2" a bunch of times in a row.
- Stopped, resumed, and restarted it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7892
Summary:
Ref T1049. Currently, the Harbormaster worker looks like this:
foreach (step) {
run_step(step);
}
This means steps can't ever be run in parallel. Instead, split it into two workers. The "Build" worker starts things off, and basically does:
update_build();
(We could theoretically do this in the original process because it should never take very long, but since there's a lock and it's a little bit complex it seemed cleaner to separate it.)
The "Target" worker runs an individual target (like a command, or an HTTP request, or whatever), then updates the build:
run_one_step(step);
update_build();
The new `update_build()` mechanism in `HarbormasterBuildEngine` does this, roughly:
figure_out_overall_status_of_all_steps();
if (build is done) { done(); }
if (build is fail) { fail(); }
foreach (step that is ready to run) {
queue_target_worker_for_step(step);
}
So, overall:
- The part of the code that updates Builds is completely separated from the part of the code that updates Targets.
- Targets can run in parallel.
Test Plan:
- Ran a bunch of builds via `bin/harbormaster build`.
- Ran a bunch of builds via web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7890
Summary:
If you have private replies on and a Macro reply handler set, we try to access `getMailKey()` and fail. See P1039 for a trace.
(Thanks to @Korvin for picking this up.)
Test Plan: Set configuration, repro'd the exception, applied the patch, then disabled/enabled a macro.
Reviewers: btrahan
Reviewed By: btrahan
CC: Korvin, aran
Differential Revision: https://secure.phabricator.com/D7896
Summary:
Currently, to add new migration patches you need to:
- Add a file to `resources/sql/patches/`; then
- add an entry to `src/infrastructure/storage/blahblah/BlahBlahBlah.php`.
The second step isn't actually necessary, and we've been using this system for a long time without any issues arising.
Instead of requiring manual adjustments to the patch list, infer the patch specifications from the files on disk so you don't need to do step 2.
Also, simplify the existing data, which can //mostly// be derived from patch names. There are a few exceptions/errors, noted inline, which are preserved for compatibility.
Test Plan:
- For the new genration of `name` and `type`, I added code to check that the old and new values were the same before converting. This caught the two inline exceptions ("emailtableport", "drydockresouces").
- Added new patches to `autopatches/` and ran `bin/storage status` to verify they got picked up correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7894
Summary: "Run" is clearer than "Apply". This has already been changed in Harbormaster itself.
Test Plan: used eyeballs
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7889
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.
Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.
Test Plan:
- Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
- Set it back, pushed OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7887
Summary:
pretty simple. did the bare minimum in the editor, etc. to be able to create an item from the conduit console.
I put the work in the editor for initializing new values, rather than some initializeNewItem method, mainly because Items don't have policy directly but instead policy will be defined by the queue(s) the item is in. The editor is definitely going to host this work, so it felt like it might be better to do it this way in time...? anyway, easy to make an initializeNew method instead if you want to have that paradigm going all the time.
Test Plan: made an item from teh conduit console - success. verified errors for missing data as well
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7879
Summary: if you rename a project in such a way that the old slug and the new slug are the same, there are errors when the phriction document is updated. detect this case and don't bother updating the document since there is no change. Fixes Github issue 474.
Test Plan: made a project "testTest". Viewed the wiki page. Created the wiki page. Renamed the project "TestTest". Before patch, this error'd, post patch it works!
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7888
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.
Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:
- Add an optional byte limit to `diffusion.rawdiffquery`.
- Make the query with a 1GB limit.
- Reduce the diff timeout from 15 hours to 15 minutes.
- Add a "Changeset is enormous" field. This field is true for changes which are too large to process.
This generally makes behaviors more sane:
- We'll always make progress in Herald in a reasonable amount of time.
- Installs can write global rules to handle (or reject) these types of changes.
Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7885
Summary:
Fixes T4189. Ref T4151. Allows repositories to have additional custom hooks for operations which can't be expressed with Herald (one such operation is lint).
This adds only local hook directories, since they're easier to use with existing hooks than global directories. I might add global directories eventually.
This doesn't support Mercurial since we have no demand for it and it's more complicated (we lose compatibility and power by just dropping a `hooks.d/` somewhere).
Test Plan:
- Pulled hosted SVN and Git repos to verify the hook directories generate correctly.
- Added a variety of hooks to the hook directories (echo + pass, fail).
- Pushed commits and verified the hooks fired (output expected info, or failed).
- Verified push log reflected the correct error code ("3", external) and detail ("nope.sh") when rejecting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4151, T4189
Differential Revision: https://secure.phabricator.com/D7884
Summary:
Fixes T4264. Adds:
- New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
- Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
- The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.
Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7883
Summary: Ref T4264. Ref T4262. Ref T2628. Ref T3190. To write Herald object rules which bind to a project, I want to take the low budget approach and have the user just type `#project` into a text field. Formally recognize `#project` as an object name, by moving all the existing stuff from the remarkup rule to the PHID type declaration.
Test Plan: Typed `#project` into jump nav and `phid.lookup` in Conduit. Typed `#project` into Remarkup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3190, T4264, T2628, T4262
Differential Revision: https://secure.phabricator.com/D7882
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary:
~~Set PATH for repository's hook, so the environment.append-paths can used~~
repository's hook may can't find php path if user's profile like bash_profile is not loaded.
Test Plan: check the hook generated is contain the right path
Reviewers: epriestley, #blessed_reviewers
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7743
Summary: Ref T4222. Adds the map name to Celerity resource URIs, so we can serve out of any map.
Test Plan: Poked around, verified URIs have "/phabricator/" in them now.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7877
Summary:
Ref T4222. Currently, CelerityResourceResponse holds response resources in flat maps. Instead, specify which map resources appear in.
Also, provide `requireResource()` and `initBehavior()` APIs on the Controller and View base classes. These provide a cleaner abstraction over `require_celerity_resource()` and `Javelin::initBehavior()`, but are otherwise the same. Move a few callsites over.
Test Plan:
- Reloaded pages.
- Browsed around Differential.
Reviewers: btrahan, hach-que
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7876
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary:
Ref T4222. Earlier, I adjusted the root from `webroot/` to `webroot/rsrc/`. However, this means that all the `/rsrc/x/y/z.jpg` fragments in CSS are no longer recognized as resource names.
Since we have like 9,000 things in CSS that do `url(/rsrc/xyz.jpg)` and I don't want to fix/test them all, so just make them work as-is. There's no real reason either setting is better than the other.
(Both URLs also work fine, but the parsed one will be better once we have real CDN support.)
Test Plan: Verified CSS gets managed resource URIs transformed into it.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7875
Summary: Ref T4222. This doesn't actually support multiple sources yet, but moves us closer by getting rid of some dead and exceedingly-singletoney code.
Test Plan: Browsed around, looked at Phame blogs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7874
Summary:
Ref T4222. This fixes some issues with Phame's resource construction.
Phame requires a fully virtual resource source, and since I want to run wordpress templates unmodified some day I don't want to build resource maps for skins.
Move all the stuff that depends on resource lists being discoverable at build time to `CelerityPhysicalResources`, and only generate maps for subclasses.
The root `CelerityResources` can now construct virtual resources; construct a virtual resource for Phame and use it.
Test Plan: Off-domain blogs work correctly now. On-domain blogs with custom skins work correctly now.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7873
Summary:
Ref T4222.
- Removes the old map and changes the CelerityResourceMap API to be entirely driven by the new map.
- The new map is about 50% smaller and organized more sensibly.
- This removes the `/pkg/` URI component. All resources are now required to have unique names, so we can tell if a resource is a package or not by looking at the name.
- Removes some junky old APIs.
- Cleans up some other APIs.
- Added some feedback for `bin/celerity map`.
- `CelerityResourceMap` is still a singleton which is inextricably bound to the Phabricator map; this will change in the future.
Test Plan:
- Reloaded pages.
- Verified packaging works by looking at generated includes.
- Forced minification on and verified it worked.
- Forced no-timestamps on and verified it worked.
- Rebuilt map.
- Ran old script and verified error message.
- Checked logs.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: chad, aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7872
Summary: Ref T4222. These are the last two "return a big ball of mud" methods. Make the API stronger so I can swap out the implementations.
Test Plan: Reloaded pages.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7871
Summary:
Ref T4222. A few diffs from now, `CelerityResourceMap` will have a `CelerityResources` inside of it:
- Rename `resolvePackage()` to `getResourceNamesForPackageHash()`. This isn't a functional change, it's just making it clear what it does.
- Add `getResourceDataForName()`, to push details about storage into `CelerityResources`.
Test Plan: Reloaded a bunch of pages, rebuilt map.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7869
Summary: Ref T4222. Same deal as D7867, but for this other super nebulous "return a blob of stuff" method.
Test Plan: Regenerated map, browsed around, etc.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7868
Summary: Ref T4222. Currently, this exposes a bunch of information about the Celerity internals. This information is difficult to preserve exactly with the new maps. Strengthen the API by providing more specific capabilities.
Test Plan: Regenerated map, browsed around.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7867
Summary: Ref T4222. Continues porting `scripts/celerity_mapper.php` functionality into `bin/celerity map`. This is pretty much a `1:1` port with no functional changes, but hopefully the code is a little better factored. Particularly, more responsibilities are pluggalbe through `CelerityResources` now.
Test Plan: Ran `bin/celerity map` and inexpected the `var_dump()` output, which appeared to make sense.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7865
Summary:
Ref T4222. Moves us toward a more modern Celerity CLI, and moves map discovery into the classtree. This is a little bit bulky (and means you can't ship completely standalone celerity maps) but has the advantage of being completely standard, and we could subclass maps into an auto-discovering map later if we have a need for it.
This doesn't affect the existing Celerity stuff. I'm going to build the new stuff in parallel, and then swap us over at the end.
Test Plan: Ran `bin/celerity map`, got reasonable-looking output.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7864
Summary: Ref T4222. This was used by Facebook while developing Releeph, but should no longer be necessary since Releeph is in the upstream. I can't get an answer out of Facebook about whether they still use it or not (see T4227), so nuke it. We're going to replace it with a more general mechanism (see T4222).
Test Plan: Regenerated celerity map. Browsed some pages, still got resources.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4222
Differential Revision: https://secure.phabricator.com/D7863
Summary: This removes the people box and adds a members property list item it's place. We may want some show/hide/see all if a project has more than //n// members, but these seems more reasonable than previous layout.
Test Plan: Tested a project with and without members, grepped for removed CSS, and tested mobile and desktop layouts.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7870
Summary: SInce we added the background color, no need to add extra padding for ObjectItemList.
Test Plan: reload a project page, no white padding in object box.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7866
Summary: Currently we markup `rXabcd`, but not `rX` on its own. Mark these up as repository object names.
Test Plan: Typed `rPOEMS`, `rPOEMS1`, `rPOEMS139893189`, etc.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran, poop
Differential Revision: https://secure.phabricator.com/D7859
Summary: Ref T4136. After Passphrase, user policies work correctly in this dropdown. Providing this option improves consistency and makes it easier to create, e.g., a private repository (where "no one" does not include the viewer, because they don't own the resulting object).
Test Plan: Set an object's policy to my user policy.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4136
Differential Revision: https://secure.phabricator.com/D7858
Summary:
Fixes T4242. It's currently possible to set nonsense defaults and create repositories with unintended policies, because policy configuration isn't part of creation. Instead:
- put a policy page into the creation workflow;
- require the selection of valid policies (i.e., prevent creating a repository you can't view / edit).
Test Plan:
- Created imported and hosted repositories, hit policy selection.
- Edited policies of existing repositories.
- Tried to set nonsense policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4242
Differential Revision: https://secure.phabricator.com/D7856
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.
Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.
Test Plan:
- Created non-object rules.
- Created object rules.
- Triggered object rules against matching and unmatching objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3506, T4264
Differential Revision: https://secure.phabricator.com/D7853
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.
Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7852
Summary: This isn't as explicit as it could be.
Test Plan: Reading.
Reviewers: poop
Reviewed By: poop
CC: aran
Differential Revision: https://secure.phabricator.com/D7861
Summary:
If the repo isn't bare, than we need copy it's remote instead of using it.
This will probably not work if an SSH key is provided to phabricator, and in any case you must delete
all workspaces that were already created.
This will make landing those repos slower; I plan to just delete and re-clone all repos on my instance.
It will probably be simpler to just make a bare-repo a requirement of all the git-landing work.
Test Plan: landed from hosted and un-hosted repos, checked git-remote url in each newly cloned workspace.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7592
Summary: Some discussion on IRC. This is more consistent with other disabled items, which are click-to-explain.
Test Plan: Viewed UI, clicked link.
Reviewers: btrahan, dctrwatson, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7857
Summary: Allow individual changesets to overflow-x: auto, Fixes T4164
Test Plan: view large diff, see some scrollbars, some without
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4164
Differential Revision: https://secure.phabricator.com/D7855
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.
Instead, bind them to the repository or revision in question.
(This code could use a bit of cleanup at some point.)
Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4270
Differential Revision: https://secure.phabricator.com/D7849
Summary:
Ref T4264. This gets most of the plumbing in for "object" rules, which will bind to a specific object, like a repository or project.
It does not yet let you actually create these rules.
Test Plan: Ran `storage upgrade`, created/edited rules, browsed Herald.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7847
Summary: Ref T4264. Instead of a dropdown, make this step more informative.
Test Plan: {F93928}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7846
Summary:
Ref T4264. Currently, you choose a rule's content type (revision, commit, hook) and rule type (global, personal) on the same screen.
- I want to make some rule types unavailable for some content types (e.g., personal hooks make little sense).
- I want to make content type selection use a radio control instead of a dropdown, so it can explain what the content types do in more detail.
- For new "object" hooks, I want to add a third step where you'll pick an object to bind to.
Split rule creation out into two steps. I think this won't get complicated enough for `PHUIPagedFormView`, but maybe I'll swap it in if this gets messier than I think.
Test Plan: Created some Herald rules, used back/cancel/etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7845
Summary: If `0` isn't an ancestor of the current branch, the `0::x` construction fails. This is uncommon, but not wildly unreasonable. The `ancestors()` construction is simpler anyway.
Test Plan: Viewed some `hg` repos locally (change history, file history) without anything suspicious cropping up.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7844
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.
Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).
Test Plan: {F93406}
Reviewers: btrahan
Reviewed By: btrahan
CC: SEJeff, aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7841
Summary:
Ref T2015. Not directly related to Drydock, but I bumped into this. All these scripts currently enumerate their workflows explicitly.
Instead, use `PhutilSymbolLoader` to automatically discover workflows. This reduces code duplication and errors (see all the bad `extends` this diff fixes) and lets third parties add new workflows (not clearly valuable?).
Test Plan: Ran `bin/x help` for each modified script.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7840
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.
Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.
Test Plan: Lint; ran some of the scripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7838
Summary: Ref T2015. All the Drydock query classes share the application method; move it into a shared base class to slightly shrink the codebase.
Test Plan: Browsed query UIs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7837
Summary:
Ref T2015. Moves a bunch of raw object loads into modern policy-aware queries.
Also straightens out the Log and Lease policies a little bit: there are legitimate states where these objects are not attached to a resource (particularly, while a lease is being acquired). Handle these more gracefully.
Test Plan: Lint / browsed stuff.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7836
Summary:
Ref T2015. Currently, Drydock has a `wait-for-lease` workflow which is invoked in the background by the `lease` workflow.
The goal of this mechanism is to allow `bin/drydock lease` to print out logs as the lease is acquired. However, this predates the `runAllTasksInProcess` flags, and they provide a simpler and more robust way (potentially with `--trace` and `PhutilConsole`) to do synchronous execution and debug logging.
Simplify this whole mechanism: just run everything in-process in `bin/drydock lease`, and do logging via `--trace`. We could thread a `PhutilConsole` through things too, but this seems good enough for now.
Also various cleanup/etc.
Test Plan: Ran `bin/drydock lease`. Ran `bin/harbormaster build X --plan Y`, for `Y` being a Drydock-dependent build plan.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7835
Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least.
Test Plan: See screenshot.
Reviewers: chad, wrotte
Reviewed By: chad
CC: aran
Maniphest Tasks: T4266
Differential Revision: https://secure.phabricator.com/D7843
Summary:
Ref T4266. This possibly moves us towards getting reasonable timeline grouping:
- Always sort icon stories to the top.
- Render one timestamp for the whole group, using the earliest tranaction date.
- Move any "Edit", "Edited", or "Preview" links to the top.
- Rendering just one timestamp implicitly fixes the JS issues.
- For stories without an icon, indent them if any member of the group has an icon.
Test Plan: See screenshots.
Reviewers: chad, wotte
Reviewed By: chad
CC: aran
Maniphest Tasks: T4266
Differential Revision: https://secure.phabricator.com/D7842
Summary:
Ref T2015. After introducing ApplicationSearch, the left nav turned into a soupy mess. Split the major sections into four separate areas, and unify them with a simple console.
This also reverts all the prefix stuff, since the results were awful and I don't anticipate it ever being the best solution to any UX problem.
Test Plan:
Browsed blueprints, resources, leases and logs.
Here's the new console:
{F93279}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7833
Summary: Ref T2015. Update DrydockLog for policy awareness and give it a policy query.
Test Plan: Browsed all the log interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7831
Summary: Ref T2015. This turns the side nav into a bigger mess for now, but uses ApplicationSearch for blueprints.
Test Plan: Queried blueprints in the UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7829
Summary:
Ref T2015. These never got updated to the new stuff, move them out of the old `Constants` class and let them load handles, etc.
Also some half-cleanup of some Blueprint/BlueprintImplementation stuff.
Test Plan: Used `phid.query` to query a Resource, Lease, and Blueprint.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7828
Summary: See thread; fixes fatal. The actual name of this method is `getHarbormaster...`.
NOTE: This fixes a fatal in Differential which impedes review, so I'm pushing it as-is.
Test Plan: Browsed a revision.
Reviewers: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7834
Summary:
Ref T2015. Applies ApplicationSearch to DrydockLease.
This makes the left nav in Drydock a little funky. It will probably get worse for a bit before it gets better, since I want to bring everything to ApplicationSearch and then sort out the details.
Test Plan: Queried leases in Drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7827
Summary: Ref T2015. DrydockLease predates widespread adoption of policies. Make it -- and its query -- policy aware.
Test Plan: Browsed leases from the web UI. Grepped for callsites.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D7826
Summary:
Ref T1049. Adds `bin/harbormaster` and `bin/harbormaster build` for applying plans from the console. Since this gets `--trace`, it's much easier to debug what's going on.
This doesn't work properly with some of the Drydock steps yet, I need to look at those. I think `setRunAllTasksInProcess` probably obsoletes some of the mechanisms. It might also not work with "Wait for Builds" but I didn't check.
Test Plan: Used `bin/harbormaster` to run a bunch of builds. Ran builds from web UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7825
Summary:
Ref T1049. Generally, it's useful to separate test/trial/manual runs from production/automatic runs.
For example, you don't want to email a bunch of people that the build is broken just because you messed something up when writing a new build plan. You'd rather try it first, then promote it into production once you have some good runs.
Similarly, test runs generally should not affect the outside world, etc. Finally, some build steps (like "wait for other buildables") may want to behave differently when run in production/automation than when run in a testing environment (where they should probably continue immediately).
So, formalize the distinction between automatic buildables (those created passively by the system in response to events) and manual buildables (those created explicitly by users). Add filtering, and stop the automated parts of the system from interacting with the manual parts (for example, we won't show manual results on revisions).
This also moves the "Apply Build Plan" to a third, new home: instead of the sidebar or Buildables, it's now on plans. I think this generally makes more sense given how things have developed. Broadly, this improves isolation of test environments.
Test Plan: Created some builds, browsed around, used filters, etc.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7824
Summary: Ref T1049. Adds "Repository", "Revision", "Diff" and "Commit" as searchable fields.
Test Plan: Used all the fields to filter things.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7823
Summary:
Fixes T4195. Allows you to write a rule against a commit's branches.
This completes outstanding work on T4195.
Test Plan: Pushed to Git and Mercurial repositories and verified branches were selected correctly by examining transcripts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7820
Summary: See comments in <https://secure.phabricator.com/D6331#comment-3> -- make the Conduit Token and Conduit Certificate interfaces readonly and select-on-click.
Test Plan:
- Viewed `/conduit/token/`, verified it was readonly and selected on click.
- Viewed `/settings/panel/conduit/`, likewise.
Reviewers: Avish, btrahan, wotte
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7819
Summary:
Ref T4257. Currently, the pull logic looks like this:
if (new) {
create();
} else {
if (hosted) {
install_hooks();
} else {
update();
}
}
This means that the first time you run `repository pull`, hooks aren't installed, which makes debugging trickier. Instead, reorganize the logic:
if (new) {
create();
} else {
if (!hosted) {
update();
}
}
if (hosted) {
install_hooks();
}
Test Plan: Ran `bin/repository pull` on a new `hg` repo and got hooks installed immediately.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4257
Differential Revision: https://secure.phabricator.com/D7818
Summary:
Fixes T4257. The `hg heads` command exits with an error code and no output in an empty repository.
Just ignore the error code: we don't have a great way to distinguish between errors, and we ran another `hg` command moments before, so we have at least some confidence it isn't a PATH sort of thing.
Test Plan: Created a new Mercurial repository and pushed to hit the error in T4257. Applied this fix and got a clean push with an accurate push log.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4257
Differential Revision: https://secure.phabricator.com/D7817
Summary: A user is reporting a re-lock in this daemon, which I can't
reproduce, but might be possible if this throws. Stop it from throwing in
a way which evades unlock.
See: <https://github.com/facebook/phabricator/issues/476>
Auditors: btrahan
Summary: add support for "assignee" conditions
Test Plan: Create a Herald rule where condition is assignee, and create a task assign to someone.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7813
Summary:
Fixes two issues:
- When rendering a task's details, we currently issue a policy-oblivious query. Instead, issue a policy-aware query.
- The formatting is a little bit weird, with the top half in a box and the bottom half with an older style. Make them consistent.
Test Plan: Looked at the detail pages for several tasks in queue.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D7812
Summary:
Ref T4195. This allows you to write rules which disallow merge commits.
Also make the reject message a little more useful.
Test Plan:
remote: This push was rejected by Herald push rule H27.
remote: Change: commit/daed0d448404
remote: Rule: No Merges
remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7809
Summary: Ref T4195. Ref T2783. We have an old-school implementation of this; move it into a LowLevel query and make callers all run through Conduit. I need the LowLevel query for hooks, to implement an "is merge commit" Herald rule.
Test Plan:
- Ran query via Conduit for SVN, Mercurial, Git.
- Parsed a commit which closed a revision, attach/closed worked correctly.
- Browsed Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195, T2783
Differential Revision: https://secure.phabricator.com/D7808
Summary: Refs T4195. Fixes T3936. You can't currently write rules like "block commits unless they're attached to an **accepted** revision"; allow that.
Test Plan: Pushed commits into a rule with this field, saw it work / not crash.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, mbishopim3
Maniphest Tasks: T3936, T4195
Differential Revision: https://secure.phabricator.com/D7807
Summary: Ref T4195. Allows you to write revision-based commit hooks, e.g. block all commits with no corresponding revision.
Test Plan:
Here's are the fields populating:
{F90989}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7806
Summary: Ref T4195. I need to query commit metadata to figure out which revision a commit is associated with. Move this out of the MessageParser so the code can be called from the HookEngine.
Test Plan: Used `reparse.php` to reparse a variety of SVN, Mercurial and Git commits. Used `var_dump()` to verify sensible fields were returned.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7805
Summary: Ref T4195. I need this for the Herald pre-commit rules, and it generally simplifies things.
Test Plan: Used `reparse.php` plus `var_dump()` to inspect refs in Git, Mercurial and SVN repos. They all looked correct and reparsed correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7804
Summary:
There's no particular reason to allow the user to edit the clone URI field in Diffusion; editing it has no meaning and if you fat finger the keyboard, it's quite possible that the user will either accidentally clear and/or modify the URI before copying (bit me this morning).
Adding a readonly attribute to the input field allows the same benefit (URI is easily selectable) while preventing such accidental input. Fixes T4246.
Test Plan: Verified that the desired behavior is present in both Chrome, Safari, and Firefox. Field remains selectable with one click, but field is not editable.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4246
Differential Revision: https://secure.phabricator.com/D7810
Summary: Ref T4195. Adds "Author" and "Committer" fields.
Test Plan:
Created a rule using these fields:
{F90897}
...then pushed git, mercurial and svn commits and verified the correct values populated in the transcript:
{F90898}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7802
Summary:
Ref T4195. To implement the "Author" and "Committer" rules, I need to resolve author/committer strings into Phabricator users.
The code to do this is currently buried in the daemons. Extract it into a standalone query.
I also added `bin/repository lookup-users <commit>` to test this query, both to improve confidence I'm getting this right and to provide a diagnostic command for users, since there's occasionally some confusion over how author/committer strings resolve into valid users.
Test Plan:
I tested this using `bin/repository lookup-users` and `reparse.php --message` on Git, Mercurial and SVN commits. Here's the `lookup-users` output:
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIS3
Examining commit rINIS3...
Raw author string: epriestley
Phabricator user: epriestley (Evan Priestley )
Raw committer string: null
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rPOEMS165b6c54f487c8
Examining commit rPOEMS165b6c54f487...
Raw author string: epriestley <git@epriestley.com>
Phabricator user: epriestley (Evan Priestley )
Raw committer string: epriestley <git@epriestley.com>
Phabricator user: epriestley (Evan Priestley )
>>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIH6d24c1aee7741e
Examining commit rINIH6d24c1aee774...
Raw author string: epriestley <hg@yghe.net>
Phabricator user: epriestley (Evan Priestley )
Raw committer string: null
>>> orbital ~/devtools/phabricator $
The `reparse.php` output was similar, and all VCSes resolved authors correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1731, T4195
Differential Revision: https://secure.phabricator.com/D7801
Summary: Ref T4195. Even though we use `svnlook` in the hook itself, I need this query elsewhere, so provide it and merge the classes into one which does the right thing.
Test Plan:
- Used `reparse.php` to reparse messages for Git, SVN and Mercurial commits, using `var_dump()` to examine the commit refs for sanity.
- Used `reparse.php` to reparse changes for an SVN commit.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7800
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.
Test Plan: {F90902}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4249
Differential Revision: https://secure.phabricator.com/D7803
Summary: There were a number of places that were generating nonsense queries for both hosted and non-hosted subversion repositories.
Test Plan: Attempted several activities in Diffusion with both a hosted and non-hosted subversion repository, including viewing various types of diffs and raw files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7799
Summary: Ref T4010. I'll hold this for a bit, but we should eventually drop this table once the dust has settled.
Test Plan: Ran storage upgrade.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4010
Differential Revision: https://secure.phabricator.com/D7372
Summary: If you push a large binary and the data crosses multiple data frames, we can end up in a loop in the parser.
Test Plan:
After this change, I was able to push a 95MB binary in 7s, which seems reasonable:
>>> orbital ~/repos/INIS $ svn st
A large2.bin
>>> orbital ~/repos/INIS $ ls -alh
total 390648
drwxr-xr-x 6 epriestley admin 204B Dec 18 17:14 .
drwxr-xr-x 98 epriestley admin 3.3K Dec 16 11:19 ..
drwxr-xr-x 7 epriestley admin 238B Dec 18 17:14 .svn
-rw-r--r-- 1 epriestley admin 80B Dec 18 15:07 README
-rw-r--r-- 1 epriestley admin 95M Dec 18 16:53 large.bin
-rw-r--r-- 1 epriestley admin 95M Dec 18 17:14 large2.bin
>>> orbital ~/repos/INIS $ time svn commit -m 'another large binary'
Adding (bin) large2.bin
Transmitting file data .
Committed revision 25.
real 0m7.215s
user 0m5.327s
sys 0m0.407s
>>> orbital ~/repos/INIS $
There may be room to improve this by using `PhutilRope`.
Reviewers: wrotte, btrahan, wotte
Reviewed By: wotte
CC: aran
Differential Revision: https://secure.phabricator.com/D7798
Summary: Ref T4195. Same as D7793, but for mercurial. (As usual, SVN needs some goofy nonsense instead, so the next diff will just make this field work.)
Test Plan: Ran `reparse.php` on Git and Mercurial commits, var_dump'd the output and it looked correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7795
Summary: Ref T4195. I need to issue this command from the pre-commit hook to get commit bodies for hooks.
Test Plan: Ran `reparse.php --message --trace` and dumped the $ref, which looked correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7793
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary: Ref T4195. Add Mercurial support to the content hook phase.
Test Plan:
Here are some `commit` push logs for a Mercurial repo:
{F90689}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7792
Summary: Ref T4195. Adds support for diff content rules.
Test Plan: Pushed SVN and Git changes through, saw them generate reasonable transcripts. Mercurial still isn't hooked up to this phase.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7791
Summary: Ref T4195. This doesn't provide any interesting fields yet (content, affected paths, commit message) but fires the hook correctly.
Test Plan: Added a blocking hook and saw it fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7789
Summary: Allow Herald rules to be referred to with `H123`, etc., like other object types are. Herald rules now have proper PHIDs and an increasingly prominent role in triggering application actions. Although I suspect users will rarely use `H123` in Remarkup to mention rules, this can simplify some of the interfaces which relate objects across systems.
Test Plan: Looked at various interfaces and saw `H123` names. Mentioned `H123` in remarkup.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7786
Summary:
These just got copy/pasted like crazy, the base class has the correct default implementation.
(I'm adding "H" for Herald Rules, which is why I was in this code.)
I also documented the existing prefixes at [[ Object Name Prefixes ]].
Test Plan: Verified base implementation. Typed some object names into the jump nav.
Reviewers: btrahan
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7785
Summary:
A few users have hit cases where Herald transcripts of large commits exceed the MySQL packet limit, because one of the fields in the transcript is an enomrous textual diff.
There's no value in saving these huge amounts of data. Transcripts are useful for understanding the action of Herald rules, but can be reconstructed later. Instead of saving all of the data, limit each field to 4KB of data.
For strings, we just truncate at 4KB. For arrays, we truncate after 4KB of values.
Test Plan: Ran unit tests. Artificially decreased limit and ran transcripts, saw them truncate properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: frgtn, aran
Differential Revision: https://secure.phabricator.com/D7783
Summary:
Ref T615. Ref T4237. With `--debug`, Mercurial will echo an "ignoring untrusted configuration option" warning **to stdout** if `.hgrc` has the wrong owner.
However, we need `--debug` to make `{parents}` usable, at least until the patches I got into the upstream are widely deployed. So after getting `--debug` output, strip off any leading warnings.
These warnings should always be in English, at least, since we set `LANG` explicitly.
Test Plan: Unit tests. @asherkin, maybe you can confirm this? I can't actually get the warning, but I think my `hg` in PATH is just a bit out of date.
Reviewers: asherkin, btrahan
Reviewed By: asherkin
CC: asherkin, aran
Maniphest Tasks: T615, T4237
Differential Revision: https://secure.phabricator.com/D7784
Summary: Ref T4195. Herald rules gained PHIDs only recently, propagate them to HeraldEffect to make some of the hook stuff eaiser.
Test Plan: iiam
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7781
Summary: Ref T4195. We need these in Herald, since HeraldTranscripts need to refer to a PHID which they acted upon.
Test Plan:
Ran migration, got PHIDs:
mysql> select phid from repository_pushlog limit 3;
+--------------------------------+
| phid |
+--------------------------------+
| PHID-PSHL-25jnc6cjgzw5rwqgmr7r |
| PHID-PSHL-2vrvmtslkrj5yv7nxsv2 |
| PHID-PSHL-34x262zkrwoka6mplony |
+--------------------------------+
3 rows in set (0.00 sec)
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7780
Summary: Ref T4195. SVN has no such thing as refs (I was thinking about writing a quasi-ref anyway like `HEAD: r23 -> r24`, but I'm not sure it would actually be useful). And content is very easy to build.
Test Plan: Pushed some stuff to SVN, got logs from it.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7766
Summary: Ref T4195. This doesn't actually work like I thought it did: it only fires locally, when you run `hg tag`. Mercurial tags are also weird and basically don't make any sense and everyone should use bookmarks instead. We could implement some flavor of this eventually, but I'd like to see users request it first. They can implement their own with content-based hooks once those work, anyway.
Test Plan: This code didn't do anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7765
Summary:
Ref T4195. This pulls the central logic of HookEngine up one level and makes all the git stuff genrate PushLogs.
In future diffs, everything will generate PushLogs and we can hand those off to Herald.
Test Plan:
Pushed a pile of valid/invalid stuff:
{F89256}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7761
Summary: Fixes T4241. Ref T4206. See T4241 for a description here. Generally, when we connect a fat pipe (`git-upload-pack`) to a narrow one (`git` over SSH) we currently read limitless data into memory. Instead, throttle reads until writes catch up. This is now possible because of the previous changes in this sequence.
Test Plan:
- Ran `git clone` and `git push` on the entire Wine repository.
- Observed CPU and memory usage.
- Memory usage was constant and low, CPU usage was high only during I/O (which is expected, since we have to actually do work, although thre might be room to further reduce this).
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4241, T4206
Differential Revision: https://secure.phabricator.com/D7776
Summary: Until we implement an "enum" type for config, make this a bit harder to get wrong. A user entered "TLS", but the correct value is "tls". The documentation is consistent about this, but the behavior is sitll surprsing.
Test Plan: eyeballed it
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7778
Summary:
Fixes T4239. Currently, if you go to `/maniphest/?authors=alincoln`, operations dependent on the query key (like "Save Custom Query..." and "Export to Excel...") don't have a query key to work with. Make sure they have one.
Also remove a stray `phlog()`.
Test Plan: "Save Custom Query...", etc., now work on GET queries.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4239
Differential Revision: https://secure.phabricator.com/D7777
Summary: Ref T4189. Updates the Phabricator stuff to use the new, more sensible semantics from D7769. Basically, this works correctly now and doesn't need workarounds.
Test Plan: Pushed Wine repo in 1m13s.
Reviewers: btrahan, zeeg
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7770
Summary: Not every revision belongs to a repository, so we might end up here with `$repo` still equal to `null`. Don't fatal if we do.
Test Plan: iiam
Reviewers: btrahan, hach-que, zeeg
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7771
Summary: Patches can exceed the 30 second time out in most PHP installations. This removes the patch preview from the version controller so that users can still see the information (although they may not be able to download the actual patch).
Test Plan: Viewed a version and saw that the patch didn't appear.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7767
Summary: This implements support for enforcing and setting policies in Phragment.
Test Plan: Set policies and ensured they were enforced successfully.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7751
Summary:
Ref T4107. Two issues:
- With strict MySQL settings, we try to insert `null` into the non-nullable `messageCount` field. Add an `initializeNew...` method.
- If we don't create a new conpherence (for example, because the message body is empty), we fatal on `getPHID()` right now.
Also, make this stuff a little easier to test.
Test Plan: Used `mail_handler.php` to receive empty conpherence mail, and new-thread conpherence mail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4107
Differential Revision: https://secure.phabricator.com/D7760
Summary:
See <https://github.com/facebook/phabricator/issues/467>. @dctrwatson also ran into an issue where we were trying to `setPass()` a GitURI.
- For Git and Mercurial, properly generate credential URIs where relevant.
- Don't try to `setPass()` on Git-style URIs.
This isn't perfect but should clean things up a bit.
Test Plan: Added unit tests. Lots of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: dctrwatson, aran
Differential Revision: https://secure.phabricator.com/D7759
Summary: This is a small fix for Phortune so that policies don't prevent the user accounts from being implicitly created when they first visit Phortune.
Test Plan: Visited Phortune and it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7758
Summary: Fixes T4224. If you `git merge-base A B`, and they have //no// ancestor, the command exits with an error. Assume errors mean "no ancestry" and continue.
Test Plan: Completely rewrite a repository with a `--force` push.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4224
Differential Revision: https://secure.phabricator.com/D7756
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.
Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4225
Differential Revision: https://secure.phabricator.com/D7757
Summary: We run `git` on a different port than 22, so would like to reflect this change in the UI.
Test Plan: Set diffusion.ssh-port in settings, then make sure it's reflected on the Diffusion repository Clone URI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7755
Summary: Fixes T4223. The output of `ls-tree` is partially delimited by spaces
and partially delimited by `\t`. The code I added in D7744 to help debug the
issue in T4159 doesn't work properly for files with 7 or more bytes in their
filesize, because the internals use `%7s`.
Auditors: btrahan
Summary:
This provides a `phragment.getstate` and a `phragment.getpatch` Conduit method.
`phragment.getstate` - This returns the current state of the fragment and all of it's children.
`phragment.getpatch` - This accepts a base path and a mapping of paths to hashes. The mapping is for the caller to specify the current state of the files it has. This returns a list of patches that the caller needs to apply to it's files to get to the latest version.
Test Plan:
Ran the following script in a folder which had content matching a fragment and it's children:
```
#!/bin/bash
STATE=""
for i in $(find ./ -type f); do
HASH=$(cat $i | sha1sum | awk '{ print $1 }')
BASE=${i:2}
STATE="$STATE,\"$BASE\":\"$HASH\""
done
STATE=${STATE:1}
STATE="{$STATE}"
echo '{"path":"tychaia3.zip","state":'$STATE'}' | arc --conduit-uri=http://phabricator.local/ call-conduit phragment.getpatch
```
and I got:
```
{"error":null,"errorMessage":null,"response":[]}
```
I updated one of the child fragments with a new file and ran the script again (patch has been omitted due to it's size):
```
{"error":null,"errorMessage":null,"response":[{"path":"Content\/TitleFont.xnb","hash_old":"4a927d7b90582e50cdd330de9f4b59b0cc5eb5c7","hash_new":"25867504642a3a403102274c68fbb9b430c1980f","patch":"..."}]}
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, staticshock
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7739
Summary: This adds a build step which will block a build from continuing if there are previous builds of the build plan still running.
Test Plan: Configured a build plan with a wait of 60 seconds and a "wait for previous builds", then started a build. While that was still building, reconfigured the plan to have a wait time of 3 seconds, started it, and saw it move into the "Waiting" status. When the 60 second build finished, both builds passed.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7745
Summary:
This came up recently in a discussion with @lifeihuang, and then tangentally with @hach-que. Make it easier for users to get a sense of whether they might need to add more daemons. Although we've improved the transparency of daemons, it's not easy for non-experts to determine at a glance how close to overflowing the queue is.
This number is approximate, but should be good enough for determining if your queue is more like 25% or 95% full.
If this goes over, say, 80%, it's probably a good idea to think about adding a couple of daemons. If it's under that, you should generally be fine.
Test Plan: {F88331}
Reviewers: btrahan, hach-que, lifeihuang
Reviewed By: btrahan
CC: hach-que, lifeihuang, aran, chad
Differential Revision: https://secure.phabricator.com/D7747
Summary: Most checks were actually in place, but `ExecFuture` throws a `CommandException` which wasn't taken into account.
Test Plan: look at the first command and no longer saw an exception. Also, other commits worked as well.
Reviewers: richardvanvelzen
Reviewed By: richardvanvelzen
CC: krisbuist, Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7730
Summary:
Ref T1049. See discussion in D7745. We have some specific interest in this for D7745, but generally we want to consume tasks with expired leases in roughly FIFO order, just like we consume new tasks in roughly FIFO order. Currently, when we select an expired task we order them by `id`, but this is the original insert order, not lease expiration order. Instead, order by `leaseExpires`.
This query is actually much better than the old one was, since the WHERE part is `leaseExpries < VALUE`.
Test Plan: Ran `EXPLAIN` on the query. Ran a taskmaster in debug mode and saw it lease new and expired tasks successfully.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7746
Summary: This adds a build step in Harbormaster for publishing file artifacts as fragments in Phragment.
Test Plan:
Created a build plan with the following steps:
* Lease Host
* Upload Artifact
* Publish Fragment
Ran the build plan against a buildable and saw the fragment get created in Phragment. Ran the plan again and saw the fragment get updated with a new version. Modified the file that got uploaded and ran the plan again, checked the history of the fragment, and saw the differences represented as a Diff-Match-Patch patch.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7742
Summary:
Ref T4212. This implements snapshots in Phragment, which allows you to take a snapshot of a fragment at a given point in time, and download a ZIP of the snapshot as it was in this state.
There's also functionality for deleting and promoting snapshots. You can promote a snapshot to either the latest version or any other snapshot of the fragment.
Test Plan: Clicked around, took some snapshots, promoted them to different points and deleted snapshots. Also downloaded ZIPs of the snapshots and saw the right versions coming through for all the files downloaded.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205, T4212
Differential Revision: https://secure.phabricator.com/D7741
Summary:
This functionality allows users to revert a fragment to a previous version from the history page.
Reverting a version actually creates a new version pointing at the same file as the version being "reverted" to. In this sense it acts pretty much like Git and other distributed VCS where once you have published a commit the only way to undo your changes is to create a new commit that reverts those changes.
Test Plan: Reverted a fragment to a version before it was deleted, then reverted it to when it was deleted and saw the new versions have the correct file PHIDs (including null for the deletion).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7738
Summary: This updates Phragment so that fragments that are currently considered deleted have a disabled status and have an additional attribute 'Deleted'. It also places this effect on versions (in the history controller) that actually involve deleting the file.
Test Plan: Viewed deleted fragments and versions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7737
Summary: When the code to update based on ZIP went to look up the child fragments, it explicitly used the paths provided in the ZIP. This meant that we could never detect omissions because there'd never be a scenario where a child fragment would return but not exist in the ZIP. To fix this, the query should be using `withLeadingPath` instead of `withPaths`.
Test Plan: Uploaded a ZIP that omitted a file and saw the `deleteFile` get called (by placing debugging output in the code).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7736
Summary: This logic causes an exception because getPHID() is called on a fragment that has no latest version. This fixes the code so that in this scenario, it returns an empty array (with no path to be added to the ZIP).
Test Plan: Downloaded the ZIP successfully after the patch was applied.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7735
Summary:
This adds support for viewing individual versions on a fragment as well as comparing versions and downloading diff_match_patch-based patches.
It does not use the side-by-side diff format as while it works for small changes, it quickly becomes impossible to distingush what changes have been made due to the diff_match_patch format.
Test Plan: Clicked on versions and downloaded patches.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7734
Summary: This fixes the update-from-ZIP functionality so that it will automatically detect directories in the ZIP that do not have explicit entries. Some ZIP programs do not create directory entries explicitly, so if we fail to do this then there's no way for users to access the sub-fragments (even though they exist, there is no directory fragment to click through).
Test Plan: Created and updated fragments from a ZIP that had implicit directories in it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, staticshock
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7733
Summary:
This implements support for creating and updating fragments from ZIP files. It allows you to upload a ZIP via the Files application, create a fragment from it, and have it recursively imported into Phragment. Updating that folder with another ZIP will recursively create, update and delete files as appropriate.
The logic for creating and updating fragments from files has also been centralized into the PhragmentFragment class. Directories are also now supported; a directory fragment is simply a fragment that has no patches; thus a directory fragment can be converted to a file fragment by uploading a first patch for it.
Test Plan: Uploaded ZIP files through the interface and saw all of the fragments get created and updated as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7729
Summary: Depends on D7727. This adds support for downloading a fragment and all it's children as a ZIP file. Fragments that have children automatically become directories in the ZIP file.
Test Plan: Downloaded a fragment as a ZIP and was able to extract the contents successfully.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7728
Summary: Depends on D7726. This adds a history controller (for viewing a list of patches associated with a fragment) and an update controller, for creating a new patch of a fragment.
Test Plan: Updated and viewed history of fragments.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7727
Summary: Ref T4205. This is an initial implementation of Phragment. You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).
Test Plan: Clicked around and created fragments.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4205
Differential Revision: https://secure.phabricator.com/D7726
Summary:
If you do something like this:
// Missing $user->getPHID()!
$object->setUserPHID($user)->save();
...you get a very unhelpful exception:
Expected a scalar or null for %s conversion. Query: %s
This doesn't give you any hints about what's wrong. Instead, provide a more useful exception:
Unable to insert or update object of class DifferentialRevision, field 'title' has a nonscalar value.
Test Plan: {F87614}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7725
Summary:
A user sent a message to Phabricator which looked like:
On blah blah blah ?
On <date>, <user> wrote:
> blah blah blah
The current algorithm is too aggressive and thinks lines 1-3 are //all// the "On ... wrote:" string. Instead, patch only the most recent "On".
Test Plan: Added a failing test and made it pass.
Reviewers: btrahan, zeeg
Reviewed By: zeeg
CC: aran
Differential Revision: https://secure.phabricator.com/D7732
Summary: This adds a handful of 'Main Header' colors to change the look of Phabricator very slightly. I know I would probably set my dev header to a different color.
Test Plan: Tested each css class and color, can add more in the future.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7731
Summary: This implements support for explicitly marking the sequence of build steps. Users can now drag and re-order build steps in plans, and artifact dependencies are re-calculated so that if you move "Run Command" before "Lease Host", the "Run Command" step has it's artifact setting cleared and thus the step becomes invalid.
Test Plan: Re-ordered build steps and observed dependencies being correctly recalculated.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7715
Summary: This implements a build step for uploading an artifact from a build machine to Phabricator. It uses SFTP so that it will work on both UNIX and Windows build machines.
Test Plan: Ran an "Upload Artifact" build against a Windows machine (with FreeSSHD installed). The artifact uploaded to Phabricator, appeared on the build view and the file contents could be viewed from Phabricator.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7582
Summary: This locks push logs down a little bit and makes them slightly more administrative. Primarily, don't show IPs to googlebot, etc.
Test Plan: Viewed push logs as edit and non-edit users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7722
Summary:
`PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:
- the query requests two or more policies;
- the object satisfies at least one of those policies; and
- policy exceptions are not enabled.
This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.
(The next diff relies on this behavior, which is how I caught it.)
Test Plan:
- Added a failing unit test and made it pass.
- Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7721
Summary:
Ref T4195. Like the previous diffs, these both create a useful log and give us an object to hand off to Herald.
Surface this information in Diffusion, too, and clean things up a little bit.
Test Plan: {F87565}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7718
Summary: Ref T4195. Add UI options to filter push logs by pusher and repository. Add a link from the repository view page to the push logs.
Test Plan: Viewed a hosted repository, clicked logs link, saw logs. Filtered lgos by repo/pusher.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7713
Summary: Ref T4195. Stores remote address and protocol in the logs, where possible.
Test Plan: Pushed some stuff, looked at the log, saw data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7711
Summary:
Ref T4195. This log serves two purposes:
- It's a log, so you can see what happened. Particularly, in Git/Hg, there is no other way to tell:
- Who //pushed// a change (vs committed / authored)?
- When was a change pushed?
- What was the old value of some tag/branch before someone destroyed it?
- We can hand these objects off to Herald to implement pre-commit rules.
This is a very basic implementation, but gets some data written and has a basic UI for it.
Test Plan: {F87339}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7705
Summary:
Report from Asana. In some unclear circumstances, we my attempt to resolve duplicate refs which currently ends up hitting a duplicate key error.
Instead, reference the same external object if we happen to be handed duplicate refs.
Test Plan:
Used this script to reproduce the issue. Applied the fix; issue went away:
#!/usr/bin/env php
<?php
require_once 'scripts/__init_script__.php';
$args = new PhutilArgumentParser($argv);
$args->parseStandardArguments();
$ref = id(new DoorkeeperObjectRef())
->setApplicationType(DoorkeeperBridgeAsana::APPTYPE_ASANA)
->setApplicationDomain(DoorkeeperBridgeAsana::APPDOMAIN_ASANA)
->setObjectType(DoorkeeperBridgeAsana::OBJTYPE_TASK)
->setObjectID(7253737283629); // Use a new task ID which we've never pulled.
$refs = array(clone $ref, clone $ref);
$asana_user = id(new PhabricatorPeopleQuery())
->setViewer(PhabricatorUser::getOmnipotentUser())
->withUsernames(array('asana'))
->executeOne();
$resolved_refs = id(new DoorkeeperImportEngine())
->setViewer($asana_user)
->setRefs($refs)
->execute();
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7709
Summary: Gives a slight bg color to ObjectList (cards only) when it's embedded in an ObjectBox.
Test Plan: Review External Accounts
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7717
Summary: We were getting a weird double box here, missed it my first pass
Test Plan: Review logged in Maniphest and Paste, as well as logged out versions. Test Login flow.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7716
Summary: I inadvertantly removed this rule and I am very fond of it.
Test Plan: Browse history in Diffusion, header links are dark again
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7714
Summary: This cleans up the UI of closed tasks in Maniphest task view, removes the Foot and sets view to disabled.
Test Plan: Searched for all tasks
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7712
Summary: These arguments prevent stderr from being routed correctly for Linux hosts and break Windows entirely. Removing them fixes the issue.
Test Plan: Removed those options and both Linux and Windows hosts had their output fed back into Harbormaster correctly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1029
Differential Revision: https://secure.phabricator.com/D7710
Summary: Currently the "Edit Build Plan" page crashes if there are any build steps with invalid implementations (because the implementation class has been removed or renamed). This updates the Edit Build Plan page so that steps with invalid implementations can be deleted.
Test Plan: Looked at a build plan with invalid configurations and deleted it's steps.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1049
Differential Revision: https://secure.phabricator.com/D7708
Summary: This migrates the "Run Remote Command" build step over to use Drydock hosts and Harbormaster artifacts.
Test Plan:
Created a build plan with a "Lease Host" step and a "Run Command" step. Configured the "Run Command" step to use the artifact from the "Lease Host" step.
Saw the results:
{F87377}
{F87378}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049, T4111
Differential Revision: https://secure.phabricator.com/D7707
Summary:
This adds LeaseHostBuildStepImplementation for getting leases on hosts in Drydock via Harbormaster. It stores the resulting lease in an artifact.
There is also a few bug fixes as well.
Test Plan: Created a build plan with a "Lease Host" build step. Ran the build plan and saw the build pass and the artifact in the database.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049, T4111
Differential Revision: https://secure.phabricator.com/D7706
Summary: This implements build targets as outlined in D7582. Build targets represent an instance of a build step particular to the build. Logs and artifacts have been adjusted to attach to build targets instead of build / build step pairs.
Test Plan: Ran builds and clicked around the interface. Everything seemed to work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1049
Differential Revision: https://secure.phabricator.com/D7703
Summary: Ref T4151. These are a (common) variant of "ssh-dsa" keys (which are somewhat theoretical, but show up on Google).
Test Plan: syntax
Reviewers: btrahan, dctrwatson, phpcodemonkey
Reviewed By: phpcodemonkey
CC: aran
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D7704
Summary:
Ref T1715. When the user clicks "Download Raw Diff" in Differential, we try to build a diff of exactly what they're seeing. However:
- This doesn't work if any of the changes have multiple hunks, and fixing it seems hard.
- I suspect this diff is never actually useful anyway? And probably kind of confusing in the best case. You can't really apply it to anyhting, since you'd have to apply another diff first.
Instead, just build the right-side diff, which should align well with user expectation and doesn't suffer from the multi-hunk bug.
Some day, we could maybe add some of the fancy options in T1715.
See: <https://github.com/facebook/phabricator/issues/461>
Test Plan: Downloaded a multi-hunk diff, got the original back and applied it cleanly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1715
Differential Revision: https://secure.phabricator.com/D7694
Summary: This updates DrydockSSHCommandInterface to correctly hold open the private key credentials for the life of the interface so that remote commands will execute correctly with a text-based private key.
Test Plan: Created a text-based private key, created a resource based on it and leased against it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111
Differential Revision: https://secure.phabricator.com/D7700
Summary: Depends on D7695. This updates preallocated hosts to use Passphrase credentials. Due to the way SSH private key text credentials work (the TempFile disappears before SSH commands can be executed), this only supports file-based private keys at the moment.
Test Plan:
Created a Passphrase credential for a file-based SSH key. Allocated a resource with:
```
bin/drydock create-resource --blueprint 1 --name "My Linux Host" --attributes platform=linux,host=localhost,port=22,path=/var/drydock,credential=2
```
and successfully leased it.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T1049
Differential Revision: https://secure.phabricator.com/D7697
Summary: This prevents issues when the user hasn't provided the appropriate attributes for a preallocated host.
Test Plan: Attempted to lease against a resource with omitted attributes, got an exception thrown before any SSH commands occurred.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1049
Differential Revision: https://secure.phabricator.com/D7695
Summary: Fixes T4198. We don't currently show "(Maniphest) > T123 > Edit" on the edit screen, which is inconsistent. Add the "T123" crumb.
Test Plan: {F87177}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Maniphest Tasks: T4198
Differential Revision: https://secure.phabricator.com/D7699
Summary: The link pointed to `create/`, which gives as `404`.
Test Plan: clicked the link. It worked.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: chad
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7698
Summary: Fixes D7669
Test Plan: test many participants and many threads in Conpherence, mobile and desktop.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7696
Summary: Touch up /notifications/ for desktop and mobile
Test Plan: Tested read and unread notifications on mobile and desktop
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7671
Summary:
Ref T4189. This doesn't add any rules yet, but does all the heavy lifting to figure out what's changed and put it in a consuamble (if somewhat ad-hoc) datastructure, which lists all the ref and tag modifications and all the new commits in a consistent way.
From here, it should be fairly straightforward to add top-level rules (e.g., ff pushes only).
Test Plan: Output is huge, see comments.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7687
Summary: This adds the ability to float action buttons inside ObjectHeaderView.
Test Plan: Tested a UI Example on desktop and mobile. Will test on Notifications next.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7684
Summary: There's no guarantee that the local path has a trailing "/". We
should probably guarantee that at some point, but just add one
unconditionally for now.
Auditors: btrahan
Summary:
//(this diff used to be about applying policies to blueprints)//
This restructures Drydock so that blueprints are instances in the DB, with an associated implementation class. Thus resources now have a `blueprintPHID` instead of `blueprintClass` and DrydockBlueprint becomes a DAO. The old DrydockBlueprint is renamed to DrydockBlueprintImplementation, and the DrydockBlueprint DAO has a `blueprintClass` column on it.
This now just implements CAN_VIEW and CAN_EDIT policies for blueprints, although they are probably not enforced in all of the places they could be.
Test Plan: Used the `create-resource` and `lease` commands. Closed resources and leases in the UI. Clicked around the new and old lists to make sure everything is still working.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4111, T2015
Differential Revision: https://secure.phabricator.com/D7638
Summary: Ref T4189. Fixes T2066. Mercurial has a //lot// of hooks so I'm not 100% sure this is all we need to install (we may need separate hooks for tags/bookmarks) but it should cover most of what we're after at least.
Test Plan:
- `bin/repository pull`'d a Mercurial repo and got a hook install.
- Pushed to a Mercurial repository over SSH and HTTP, with good/bad hooks. Saw hooks fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2066, T4189
Differential Revision: https://secure.phabricator.com/D7685
Summary:
Ref T4189. This adds SVN support, which was a little more messy than I though. Principally, we can not use `PHABRICATOR_USER` for Subversion, because it strips away the entire environment for "security reasons".
Instead, use `--tunnel-user` plus `svnlook author` to figure out the author.
Also fix "ssh://" clone URIs, which needs to be "svn+ssh://".
Test Plan:
- Made SVN commits through the hook.
- Made Git commits, too, to make sure I didn't break anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7683
Summary:
Ref T4189. T4189 describes most of the intent here:
- When updating hosted repositories, sync a pre-commit hook into them instead of doing a `git fetch`.
- The hook calls into Phabricator. The acting Phabricator user is sent via PHABRICATOR_USER in the environment. The active repository is sent via CLI.
- The hook doesn't do anything useful yet; it just veifies basic parameters, does a little parsing, and exits 0 to allow the commit.
Test Plan:
- Performed Git pushes and pulls over SSH and HTTP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7682
Summary: Cleans up the UI a little on the Object Selector.
Test Plan: Test various layouts, attach and unattach tasks and diffs.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7661
Summary: Make Conpherence usable when there is a setup issue as well. Fixes T3427
Test Plan: Test an ongoing thread, create a new thread. Test mobile and tablet layouts.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3427
Differential Revision: https://secure.phabricator.com/D7669
Summary: This makes the pager a little more button like.
Test Plan: tested UIexamples and a search query
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7670
Summary: DrydockResource has been updated to be policy-aware (although there are no policy columns).
Test Plan: Clicked around in Drydock, viewed resources and leases, everything still seemed to work.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3605, T4111
Differential Revision: https://secure.phabricator.com/D7595
Summary: If there is no /query in the URL, the default query would be lost when clicking Next, causing the search form to be shown on the second page. This is not so likely to happen on a standard Phabricator installation because the default query is Assigned, and few people will have 100+ tasks assigned.
Test Plan:
* Go to /maniphest/query/edit/
* Move Open Tasks to the top
* Go to /maniphest/
* Click Next on the bottom right
* See only tasks that are actually open
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7667
Test Plan: Go to a project page, make browser window narrow, click to expand action list.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7666
Summary: `LANG=C` is smooshing UTF-8 in some cases. See IRC.
Test Plan: User confirmed this works.
Reviewers: btrahan, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7659
Summary:
By default in search application document status field is "Open and Closed Documents".
Often searching with this default status I get confused that open and closed items in
search result are not distinguished.
Test Plan: Search and see open/closed issues distinguished.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: epriestley, aran, Korvin
Differential Revision: https://secure.phabricator.com/D7626
Summary: Request from Asana. Adds an option for adding tasks to projects.
Test Plan: Used `bin/feed republish` to create and update Asana tasks with projects configured. Saw them end up in the right projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7655
Summary: See D7653. This is exclusively for Asana, who uses Differential for a post-commit, Audit-like workflow but has a small set of requirements for it to be a good fit (just this) and a large set of requirements for Diffusion/Audit to be a good fit.
Test Plan: Set the flag, verified "Accepted" revisions are no longer on the dashboard.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7654
Summary:
Currently, "Closed" and "Abandoned" are treated as "closed". I want to add a flag which treats "Accepted" as "Closed", too, for Asana and other companies who use an Asana-like workflow.
The background here is that their workflow is a bit weird. They basically do audits, but have a lot of things which Diffusion doesn't do well right now. This one change makes Differential fit their workflow fairly well, even though it's an audit workflow.
To prepare for this, normalize the definition of "closed" better. We have a few callsites which explicitly check for "ABANDONED || CLOSED", and normalizing this is cleaner anyway.
Also delete the very old COMMITTED status, which has been obsolete for over a year.
Test Plan: Browsed around most/all of the affected interfaces.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7653
Summary: Fixes T4148. LDAPS works with "ldaps://", it just isn't documented or clear.
Test Plan: {F84893}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4148
Differential Revision: https://secure.phabricator.com/D7652
Summary: Fixes T4149. This could be a little cleaner (configurable time limits, explicit timeout errors) but stop the major case of looping/infinite commands.
Test Plan: Added `sleep 5 &&` and set timeout to 1, saw an error + kill.
Reviewers: btrahan, skyronic
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4149
Differential Revision: https://secure.phabricator.com/D7651
Summary:
Fixes T4158. Two possible refinements:
- Maybe we should make all of these things respect `ManiphestCapabilityEditAssign::CAPABILITY`, etc. I think it's reasonable either way, and this is probably more intuitive and useful for most cases.
- Maybe we should check that you can see the policies before copying them. Again, this is sort of reasonable either way.
Test Plan: Created a new task from a template, saw that it inherited policies.
Reviewers: btrahan, hach-que
Reviewed By: hach-que
CC: aran
Maniphest Tasks: T4158
Differential Revision: https://secure.phabricator.com/D7649
Summary: Fixes T3687. Instead of rendering "JIRA Issues" in Differential using plain links, render them using Doorkeeper tags so they get the nice "enhance with object name" effect.
Test Plan: {F84886}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3687
Differential Revision: https://secure.phabricator.com/D7648
Summary: Touched up the layout, css of this page
Test Plan: Viewed linked and linkable accounts. Tested mobile layout
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7644
Summary: Fixes T4155. See discussion in T4155.
Test Plan: @mbishopim3 confirmed this fixes his issue.
Reviewers: btrahan, chad
Reviewed By: chad
CC: mbishopim3, aran
Maniphest Tasks: T4155
Differential Revision: https://secure.phabricator.com/D7646
Summary: Adds an action icon for GitHub.
Test Plan: I couldnt see how to update that icon in Differential. But. Photoshop.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7635
Summary: This was broken in rP51fb1ca16d7f.
Test Plan: Imported a repository with file:/// location, it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7636
Summary: Fixes T2230. This isn't a total walk in the park to configure, but should work for early adopters now.
Test Plan: Read documentation, browsed UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7634
Summary:
Ref T4038. This adds everything except the actual pushing part for mirrors.
This isn't the most beautiful or sophisticated UI, but I want get the authoritative repositories self-hosted and get users beta-ing hosting as soon as possible. We can do transactions, etc., later on.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4038
Differential Revision: https://secure.phabricator.com/D7632
Summary: Fixes T4122. Ref T2230. Instead of storing credentials on each repository, store them in Passphrase. This allows easy creation/management of many repositories which share credentials.
Test Plan:
- Upgraded repositories.
- Created and edited repositories.
- Pulled HTTP and SSH repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230, T4122
Differential Revision: https://secure.phabricator.com/D7629
Summary: Ref T4122. Add an edge to keep track of where a credential is used, and show it in the UI.
Test Plan:
See "Used By":
{F84099}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4122
Differential Revision: https://secure.phabricator.com/D7628
