Summary:
Ref T7803. This is a performance hack, not a real order, and isn't really meaningful or pageable.
After D12158, we constraint his query on `dateModified` anyway, which should generally give the database a relatively small result set to examine.
Test Plan: Browsed Differential and Diffusion. Checked query plan, it didn't look too crazy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7803
Differential Revision: https://secure.phabricator.com/D12361
Summary: This moves Markdown rendering from normal fonts to PHUIDocumentView with Source Sans improving readability of this longer form text.
Test Plan:
Test libphutil and Phabricator readmes in my sandbox.
{F363483}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12330
Summary:
Ref T7731. For no particular reason, we currently put `ruleID` and `rulePHID` on `HeraldEffect` objects.
Pretty much all callers need the `HeraldRule` objects instead, and some go to great lengths to get them.
Just attach the `Rule` objects.
Test Plan: Will test thoroughly after next-ish changeset.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12269
Summary:
Ref T7731. Every adapter subclass currently implements this effect in an essentially identical way.
Some day far from now the effects will be modular and this mess will vanish completely, but reduce its sprawl for now.
Test Plan: I'll test this thoroughly at the end of the change sequence since writing rules is a pain.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7731
Differential Revision: https://secure.phabricator.com/D12268
Summary:
Fixes T7199. This still isn't a shining example of perfect code, but the raw amount of copy/paste is much lower than it used to be.
- Reduce code duplication between existing receivers.
- Expose receiving objects in help menus where appropriate.
- Connect some "TODO" receivers.
Test Plan:
- Sent mail to every supported object type.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12249
Summary:
Ref T7199. Convert the single help menu item into a dropdown and allow applications to list multiple items there.
When an application has mail command objects, link them in the menu.
Test Plan:
{F355925}
{F355926}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12244
Summary:
Ref T7199. In the vein of D12231, these options were a bad idea.
- They once served a very narrow, Facebook-specific need (see T1992), except even Facebook only used the Differential setting AFAIK.
- Outside of that special case, they are unused and essentially unusable (generally speaking, they do not meaningfully implement anything modular or replaceable).
- I have no knowledge of any install ever changing these settings, and can imagine no reason why they would.
Moving forward:
- If they really need to, they can fork locally and chagne one line.
- I expect "!actions" to make mail at least somewhat more modular soon, anyway.
- Any derived handlers would break after T7199 and need to be rewritten anyway, so this is just taking advantage of a BC break to do cleanup.
Test Plan:
- Grepped for removed configuration.
- Sent some mail from applications, verified the reply handlers set proper reply addresses.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12233
Summary:
Ref T7199. These were a bad idea which got copy-pasted a bunch.
- There is zero reason to ever set these to different things.
- Unsurprisingly, I don't know of any install which has them set to different things.
Unless I've completely forgotten about it, this option was not motivated by some obscure business need, it was just a bad decision which didn't catch anyone's attention at the time.
We partially remedied the mistake at some point by introducing `metamta.reply-handler-domain`, which works as a default for all applications, but never cleaned this mess up.
Test Plan: Sent some mail from applications, verified it picked up appropraite reply handler domains.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7199
Differential Revision: https://secure.phabricator.com/D12231
Summary: Ref T7689. Ref T4100. This advances the goals of removing `loadViewerHandles()` (only 67 callsites remain!) and letting tokenizers some day take token functions like `viewer()` and `members(differential)`.
Test Plan:
- Sent a new message; used "To".
- I simplified the cancel URI construction slightly because it's moot in all normal cases.
- Edited a thread; used "Add Participants".
- Searched rooms; used "Participants".
- Searched countdowns; used "Authors".
- Created a diff; used "Repository".
- Edited a revision; edited "Projects"; edited "Reveiwers"; edited "Subscribers".
- Searched for revisions; edited "responsible users"; "authors"; "reviwers"; "subscribers"; "repositories".
- Added revision comments; edited "Add Reveiwers"; "Add Subscribers".
- Commented on a commit; edited "Add Auditors"; "Add subscribers".
- Edited a commit; edited "Projects".
- Edited a repository; edited "Projects".
- Searched feed, used "include Users"; "include Proejcts".
- Searched files, used "authors".
- Edited initiative; edited "Projects".
- Searched backers; used "Backers".
- Searched initiatives; used "Owners".
- Edited build plans; edited "Run Command".
- Searched Herald; used "Authors".
- Added signature exemption in Legalpad.
- Searhced legalpad; used "creators"; used "contributors".
- Searched signatures; used "documents"; used "signers".
- Created meme.
- Searched macros; used "Authors".
- Used "Projects" in Maniphest reports.
- Used Maniphest comment actions.
- Edited Maniphest tasks; edited "Assigned To"; edited "CC"; edited "projects".
- Used "parent" in Maniphest task creation workflow.
- Searched for projects; used "assigned to"; "in any projec"; "in all projects"; "not in projects"; "in users' projects"; "authors"; "subscribers".
- Edited Maniphest bug filing domains, used "Default Author".
- Searched for OAuth applications, used "Creators".
- Edited Owners pacakge; edited "Primary Owner"; edited "Owners".
- Searched for Owners packages; used "Owner".
- OMG this UI is OLD
- Edited a paste; edited "Projects".
- Searched for paste; used "Authors".
- Searched user activity log; used "Actors"; used "Users".
- Edited a mock; edited "Projects"; edited "CC".
- Searched for mocks; used "Authors".
- Edited Phortune account; edited "Members".
- Edited Phortune merchant account; edited "Members".
- Searched Phrequent; used "Users".
- Edited Ponder question; sued "projects".
- Searched Ponder; used "Authors"; used "Answered By".
- Added project members.
- Searched for projects; used "Members".
- Edited a Releeph product; edited "Pushers".
- Searched pull requests; searched "Requestors".
- Edited an arcanist project; used "Uses Symbols From".
- Searhced push logs; used "Repositories"; used "Pushers".
- Searched repositories; used "In nay project".
- Used global search; used Authors/owners/Subscribers/In Any Project.
- Edited a slowvote; used "Projects".
- Searched slovotes; used "Authors".
- Created a custom "Users" field; edited and searched for it.
- Made a whole lot of typos in this list. ^^^^^^
Did not test:
- Lint is nontrivial to test locally, I'll test it in production.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100, T7689
Differential Revision: https://secure.phabricator.com/D12224
Summary:
Ref T1460. Overall:
- Pass `objectOwnerPHID` consistently.
- Pass viewer consistently.
- Set the correct draft state for checkboxes on the client.
Test Plan:
- Made inline comments in Differential.
- Made inline comments in Diffusion.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12186
Summary:
This returns the PHID of the current revision owner, or the commit author, if one exists.
NOTE: For drafts, we currently return `null`; I'll fix that in a future change. Should be correct for submitted comments.
Test Plan: Added an inline, nothing seemed broken.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12185
Summary: Fixes T7672. This had two `%d` conversions but only one parameter.
Test Plan: Adjusted limit to 0, viewed a merge, saw proper message.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7672
Differential Revision: https://secure.phabricator.com/D12180
Summary: Fixes T7655. We'll set tighter spacing around edit clusters. Also darkened up the date marker and remove unused `phabricator-transaction-view` CSS that was still scattered around the site.
Test Plan: Test a full and column multi-edit spam. Visited Ponder and Diffusion, noticed no issues using those apps. Grepped for other users of `phabricator-transaction-view`
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T7655
Differential Revision: https://secure.phabricator.com/D12148
Summary: Fixes T5658. Over a long period of time, some cruft can build up here. Only show revisions which have been updated in the last 30 days.
Test Plan:
- Viewed panel in Differential and Diffusion.
- Changed limit from 30 days to 30 seconds and saw no revisions.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5658
Differential Revision: https://secure.phabricator.com/D12158
Summary:
Ref T5644. Ref T7472. Currently, we highlight each line of pattern search results in Diffusion.
- This is incredibly slow for non-PHP languages which need to shell out to Pygments.
- A lot of this highlighting isn't very useful anyway, because it doesn't have any context.
Instead, try to highlight pattern matches but don't highlight the source itself.
Test Plan: {F349637}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7472, T5644
Differential Revision: https://secure.phabricator.com/D12141
Summary:
Ref T1460. See D12126. This is essentially the same change, but for Diffusion.
This is a bit copy/pastey. I'm going to make an effort to lift inline handling into the core before pushing this in, so hopefully that will clean things up a bit.
Test Plan: Submitted stuff in Diffusion and got checkmarks to publish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12128
Summary:
Ref T1460. This just barely works, but throwing it up in case any of it sounds mechanically crazy before we build integrations/UI/etc.
Specifically, these are the behaviors:
- You can mark your own draft comments as "done" before you submit them. The intent is to let reviewers mark their stuff advisory/minor/not-important before they submit it, to hint to authors that they don't expect the feedback to necessarily be addressed (maybe it's a joke, maybe it's just discussion, maybe it's "consider..").
- You can mark others' published comments as "done" if you're the revision/commit author. The intent is to keep this lightweight by not requiring an audit trail of who marked what done when. If anyone could mark anything done, we'd have to have some way to show who marked stuff.
- When you mark stuff done (or unmark it), it goes into a "draft" state, where you see the change but others don't see it yet. The intent is twofold:
- Be consistent with how inlines work.
- Allow us to publish a "epriestley updated this revision + epriestley marked 15 inlines as done" story later if we want. This seems more useful than publishing 15 "epriestley marked one thing as done" stories.
- The actual bit where done-ness publishes isn't implemented.
- UI is bare bones.
- No integration with the rest of the UI yet.
Test Plan: Clicked some checkboxes.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: paulshen, chasemp, epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12033
Summary:
Via HackerOne. We aren't correctly escaping the date, so a user can XSS themselves by setting their date format creatively.
This construction is very unusual and I don't think we do anything similar elsewhere, so I can't come up with a systematic change which would prevent this in the general case.
Test Plan: Set date format to tag junk, got self-XSS before patch and proper escaping after the patch.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D12117
Summary:
Ref T7522. This is mostly useful in the cluster, but could be useful for external installs too.
If you want to import an instance into a test/dry-run state (in the cluster, to test an import; in the general case, to do something like test new hardware or configuration), you currently risk spamming users with a lot of duplicate notifications. In particular, if Phabricator tracks remotes, both instances will continue importing commits and sending email about them. Both instances will try to publish to mirrors, too, which could be bad news, and both instances will try to update linked services.
Instead, provide a flag to let an instance run in "silent mode", which disables all outbound messaging and data.
We need to remember to support this flag on any new outbound channels, but we add about one of those per year so I think that's reasonable.
Test Plan:
- Flipped config.
- Saw it void email, feed and mirroring.
- Didn't test SMS since it's not really in use yet and not convenient to test.
- (Can you think of any publishing I missed?)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7522
Differential Revision: https://secure.phabricator.com/D12109
Summary:
Ref T2009. This is another almost-identical copy of the row scaffolding, which has the same 1up/2up bugs as the 8 other copies of this code.
Turn the "undo" element into an InlineCommentView so we can scaffold it.
Then, scaffold it with the same code as everything else.
Test Plan: Hit "Undo", swapped from 1up to 2up, hit "undo" again, swapped back, tried left/right, everything rendered with proper scaffolding.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D12019
Summary:
Ref T1460. Track and store which comments are threaded replies to other comments, vs merely appearing on the same lines.
This doesn't actually write `hasReplies` yet, since that needs to happen when we un-draft comments on submission.
Test Plan: Made inline comments in Differential and Diffusion, including replies. Replies were marked as "Is Reply".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T1460
Differential Revision: https://secure.phabricator.com/D12017
Summary: Renames the method in PHUIObjectBoxView to match the new PHUIInfoView class.
Test Plan: grepped codebase. Went to Calendar and tried a new status.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D12005
Summary:
Ref T2009. These classes are "Differential" now, but are used elsewhere in diff infrastructure (e.g., Diffusion).
- Rename them to "PHUIDiff".
- Move them to "src/infrastructure/".
- Give them a base class.
Test Plan: Interacted with inlines in unified and side-by-side views.
Reviewers: btrahan
Subscribers: epriestley
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D11996
Summary: Ref T6516. We incorrectly fail to set this flag on repositories created via Conduit, which activates too many actions on old commits.
Test Plan:
- Created a new repository via Conduit, verified it was "importing" after creation.
- Created a new repostiory via web UI, verified it was "importing" after creation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6516
Differential Revision: https://secure.phabricator.com/D11964
Summary: Since this element isn't strictly about errors, re-label as info view instead.
Test Plan: Grepped for all callsites, tested UIExamples and a few other random pages.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11867
Summary:
Fixes T7298. There are two ways to import a repository that you want to host, today:
- Create it as "hosted", then push everything to it.
- Create it as "imported", let it import, then switch it to "hosted".
- (Neither of these work with SVN.)
We don't specifically recommend one or the other, although I believe both should work, and most users seem to go with the first one.
In the first workflow, the new empty repository imports completely and gets marked "imported", so our default behavior is then to publish commits. This can generate a lot of email/notification/feed spam.
If you're a fancy expert you might turn off "publish" before pushing, but normal users will frequently miss this.
Instead, when we receive an "import-like" push to an empty repository, put the repository back into "importing" after we accept the changes.
This has to be heuristic since we can't know for sure if a push is an import or new commits, but here's a simple rule that should do pretty well. We can refine it if necessary.
Test Plan:
- Created a new empty repository.
- Added some debugging code; verified the "commit count" and "empty" rules were calculated properly.
- Pushed 8+ commits and saw the repo go into "importing", import, and leave "importing".
- Pushed 8+ commits again and saw them publish.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7298
Differential Revision: https://secure.phabricator.com/D11827
Summary:
Fixes T7310. We have a whole mechanism for surfacing update errors, but only surface actual update errors, not pull errors.
Instead, surface pull errors too.
Then format them a little more nicely.
Test Plan: {F309769}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7310
Differential Revision: https://secure.phabricator.com/D11821
Summary: Uses PHUIObjectBoxView to display lists of diffs in Differential and Diffusion, unless embedded on a dashboard.
Test Plan:
Test Dashboard panel, Differential home, Commit, and Diff
{F282173}
{F282174}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11659
Summary:
Ref T6941. In the cluster (and in other reasonable setups) we've separated SSH load balancers from HTTP load balancers.
In particular, ELBs will not let you load balance port 22, so this is likely a reasonable/common issue in larger clusters in AWS.
Allow users to specify an alternate host for SSH traffic.
Test Plan: Set host to someting different, saw it reflected in UI.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6941
Differential Revision: https://secure.phabricator.com/D11800
Summary: Ref T7123. Turns out that we might throw ConduitClientException now in proxied scenarios. For all but one callsite remove the try / catch bit and don't issue the call for SVN. For the remaining callsite, also don't issue the call for SVN but keep in the exception logic since its renders a pretty error message in the non-proxied case?
Test Plan: played around with diffusion and things looked okay.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7123
Differential Revision: https://secure.phabricator.com/D11789
Summary: Fixes T7256.
Test Plan: Looked at rXPRF0a7a5f69f5d7 in a local instance. things looked great both pre and post patch.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7256
Differential Revision: https://secure.phabricator.com/D11790
Summary:
Ref T7185. These settings shouldn't be unlocked anywhere. Specifically:
- `reply-handler`: These are on the way out.
- `reply-handler-domain`: Also hopefully on the way out; locked because a compromised administrator account can redirect replies.
- `phabricator.cookie-prefix`: Not dangerous per se, but an admin could have a hard time fixing this if they changed it by accident since their session would become invalid immediately.
Test Plan: Browsed Config.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7185
Differential Revision: https://secure.phabricator.com/D11764
Summary: Adds core and apps grouping to configuration options, makes it somewhat easier to browse config options.
Test Plan: Set each option, review list. Breakdown is nearly 50/50 apps/core.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11722
Summary: Moving towards a consisent 'if header, show in object box' style around Phabricator.
Test Plan:
Grep for uses of RevisionList and make sure double boxes arent set, browse Differential, various searches, a revision, and a commit.
{F282113}
{F282114}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11651
Summary:
Ref T7123. Two general issues:
For proxied repositories, we currently throw a ConduitClientException, vs ConduitException for local repositories. This is inconsistent and we should fix it, but I also want to examine the use of try-the-call-and-throw at these sites since it may be something we can update. In particular, trying a call that we know will always fail is now more expensive (in proxied repositories) than it used to be.
Here, we try-and-throw for merges, but they're //never// supported in Subversion. Just don't bother trying.
Test Plan: Browsed a SVN repository with proxying set up, got a clean commit page.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7123
Differential Revision: https://secure.phabricator.com/D11646
Summary:
Fixes T7122. The way this query works is a little surprising:
- If executed as `withRepositoryIDs(...)`, it assumes you are passing one //or more// repository IDs, so it will never resolve ambiguous identifiers (e.g., "123" instead of "rSVN123").
- If executed as `withRepository(...)`, it knows you are passing exactly one repository and will use that to imply context and resolve these identifiers correctly.
This isn't very obvious from the API, but I'm not sure how to make it more clear.
(Making `withRepositoryIDs()` do the `withRepository(...)` thing if only one ID was passed in would mean its behavior varied if you passed 1 vs 2 repository IDs, which seems worse / morse surprising.)
Test Plan: Various subversion UIs no longer fail to look up commits.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: mormegil, epriestley
Maniphest Tasks: T7122
Differential Revision: https://secure.phabricator.com/D11645
Summary: This sets an icon for each config, makes it easier to scan.
Test Plan:
Reload Config page, see all new icons
{F281089}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11619
Summary: Clean up the error view styling.
Test Plan:
Tested as many as I could find, built additional tests in UIExamples
{F280452}
{F280453}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11605
Summary: Ref T7094. The class DiffusionRequest has other public methods which use getUser() in an unguarded way. Code inspection of the call sites for loadCommit() also leads me to believe the $user is properly set.
Test Plan: clicked around diffusion a bunch and everything seemed to work okay. (happy to test any particular esoteric endpoints that come to mind)
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11585
Summary: Ref T7094.
Test Plan: couldn't really test this - how does one get symbols going nowadays given they are acanist project based?
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11584
Summary: Ref T7094. Could just delete this end point too I guess? Needed to add "withCommitPHIDs" to the differentialrevisionquery to get this done.
Test Plan: used diffusion.getcommits from conduit console and got a sensible result for a query for two commits, one with a diff and one without.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7094
Differential Revision: https://secure.phabricator.com/D11581
Summary: Fixes T7034. Like HTTP, proxy requests to the correct host if a repository has an Almanac service host.
Test Plan: Ran VCS requests through the proxy.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11543
Summary:
Ref T7034.
In a cluster environment, when a user connects with a VCS request over SSH (like `git pull`), the receiving server may need to proxy it to a server which can actually satisfy the request.
In order to proxy the request, we need to know which repository the user is interested in accessing.
Split the SSH workflow into two steps:
# First, identify the repository.
# Then, execute the operation.
In the future, this will allow us to put a possible "proxy the whole thing somewhere else" step in the middle, mirroring the behavior of Conduit.
This is trivially easy in `git` and `hg`. Both identify the repository on the commmand line.
This is fiendishly complex in `svn`, for the same reasons that hosting SVN was hard in the first place. Specifically:
- The client doesn't tell us what it's after.
- To get it to tell us, we have to send it a server capabilities string //first//.
- We can't just start an `svnserve` process and read the repository out after a little while, because we may need to proxy the request once we figure out the repository.
- We can't consume the client protocol frame that tells us what the client wants, because when we start the real server request it won't know what the client is after if it never receives that frame.
- On the other hand, we must consume the second copy of the server protocol frame that would be sent to the client, or they'll get two "HELLO" messages and not know what to do.
The approach here is straightforward, but the implementation is not trivial. Roughly:
- Start `svnserve`, read the "hello" frame from it.
- Kill `svnserve`.
- Send the "hello" to the client.
- Wait for the client to send us "I want repository X".
- Save the message it sent us in the "peekBuffer".
- Return "this is a request for repository X", so we can proxy it.
Then, to continue the request:
- Start the real `svnserve`.
- Read the "hello" frame from it and throw it away.
- Write the data in the "peekBuffer" to it, as though we'd just received it from the client.
- State of the world is normal again, so we can continue.
Also fixed some other issues:
- SVN could choke if `repository.default-local-path` contained extra slashes.
- PHP might emit some complaints when executing the commit hook; silence those.
Test Plan: Pushed and pulled repositories in SVN, Mercurial and Git.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7034
Differential Revision: https://secure.phabricator.com/D11541
Summary: Add a setBorder call to CrumbsView to be more deliberate when a border is drawn. Could not find any CSS hacks to set it conditionally CSS.
Test Plan: Browsed every application that called crumbs and make a design decision. Also fixed a few bad layouts.
Reviewers: btrahan, epriestley
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11533
Summary:
Ref T7019. When we receive a `git clone https://` (or `git push` on HTTP/S), and the repository is not local, proxy the request to the appropriate service.
This has scalability limits, but they are not more severe than the existing limits (T4369) and are about as abstracted as we can get them.
This doesn't fully work in a Phacility context because the commit hook does not know which instance it is running in, but that problem is not unique to HTTP.
Test Plan:
- Pushed and pulled a Git repo via proxy.
- Pulled a Git repo normally.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7019
Differential Revision: https://secure.phabricator.com/D11494
Summary: Removes the 1x application icons, and uses the fonticons instead. Feed was only known location.
Test Plan:
feed, dashboards, grep for use
{F275636}
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11496
Summary: Select a similar or better FontAwesome icon to represent each application
Test Plan: Visual inspection
Reviewers: epriestley, btrahan
Subscribers: hach-que, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11489
Summary:
Ref T2783. I think this served two purposes:
- Improving performance in cases where we "know" a repository is local.
- Preventing loops.
It is now obsolete:
- After D11476, refs can almost always resolve on a fast path.
- As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
- The cluster behavior in D11475 now prevents loops.
Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11477
Summary:
Ref T2783. With service-oriented calls, we take a larger performacne hit than necessary resolving refs.
Instead of resolving refs over the wire, try to resolve them from the database first. This can resolve almost all refs (commit hashes, branch and tag names).
This can't resolve weird refs like `master~50`, and obviously can't resolve invalid refs. In those cases we'll go back to the old logic, call `diffusion.resolverefs`, and end up with the right result.
Test Plan:
- Browsed repositories in Diffusion.
- Verified that service repositories no longer make unnecessary `diffusion.resolverefs` calls for common refs (branch names, commit hashes).
- Resolved refs like `master~50`, saw call to underlying VCS and correct result.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11476
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.
This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.
In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).
Test Plan:
- With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
- With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11475
Summary: Ref T7020. I need this elsewhere, and it's relatively internal anyway.
Test Plan: Browsed around my local, cluster-configured install and saw everything working fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7020
Differential Revision: https://secure.phabricator.com/D11474
Summary: Fixes T7021. When I moved around all the timeline stuff I guess I didn't find this "corner" case, which is wildly common in the post-commit review workflow that we don't use.
Test Plan: pre-patch I could reproduce the issue and post patch I could not. The reproduction case is to have a commit with inline comments and then enough subsequent comments to have a "show older" UI. clicking "show older" now works!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7021
Differential Revision: https://secure.phabricator.com/D11479
Summary: Fixes T7011. Recent refactoring here caused us to begin ignoring URI parameters like `commit`. Most controllers take parameters as a `dblob`, which was still parsed properly.
Test Plan:
- Editing different commits actually edits the desired commits.
- Browsed around some `dblob` pages and verified they still work properly.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T7011
Differential Revision: https://secure.phabricator.com/D11473
Summary: Ref T6822.
Test Plan: `grep`. This method is only called from within `PhutilArgumentWorkflow::__construct`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11415
Summary: Third time lucky... the filename should match the class name now.
Test Plan: `arc lint`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11362
Summary: Fixes T5646. Makes diffusion a much better user experience. Users now see a 404 exception page when they have a bad URI. Previously, they saw a developer-facing raw exception.
Test Plan: played around in diffusion a bunch. most of these changes were fairly mechanical at the end of the day.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5646
Differential Revision: https://secure.phabricator.com/D11299
Summary: Ref T6822.
Test Plan: Visual inspection. These methods are only called from within `PhabricatorController` subclasses.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6822
Differential Revision: https://secure.phabricator.com/D11241
Summary:
Fixes T5966. Accomplishes a few things
- see title
- adds a force-autoclose flag and the plumbing for it
- removes references to some HarborMaster thing that used to key off commits and seems long dead, but forgotten :/
Test Plan:
ran a few commands. These first three had great success:
`./repository reparse --all FIRSTREPO --message --change --herald --owners`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday`
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date yesterday --force-autoclose`
...and these next two showed me some errors as expected:
`./repository reparse --all FIRSTREPO --message --change --herald --owners --min-date garbagedata`
`./repository reparse --all GARBAGEREPO --message --change --herald --owners`
Also, made a diff in a repository with autoclose disabled and commited the diff. Later, reparse the diff with force-autoclose. Verified the diff closed and that the reason "why" had the proper message text.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: joshuaspence, epriestley, Korvin
Maniphest Tasks: T5966
Differential Revision: https://secure.phabricator.com/D10492
Summary: Fixes T6880. If matching commits have no visible/loadable repository, we shouldn't keep going forward in the loop.
Test Plan: Havne't built a repro locally yet so not 100% sure this fixes it.
Reviewers: btrahan, mbishopim3, fabe
Reviewed By: mbishopim3, fabe
Subscribers: mbishopim3, epriestley
Maniphest Tasks: T6880
Differential Revision: https://secure.phabricator.com/D11251
Summary: Use `PhabricatorAuditEditor` instead of `PhabricatorEdgeEditor` when writing reverts edges. This ensures that a transaction is created in addition to the edge.
Test Plan: Reverted a commit and pushed to remote. Saw a row created in `phabricator_audit.audit_transaction_comment`. Interestingly, I can't actually see the transaction at http://phabricator.local/r${CALLSIGN}${REVERTED_COMMIT_HASH}.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D11212
Summary:
Ref T1751. When a commit reverts another commit:
- Add an edge linking them;
- Show the edge in Diffusion.
Next steps are:
- If the reverted commit is associated with a Differential revision, leave a comment;
- Also leave a comment on the commit (no API yet);
- Also trigger an audit by the original commit's author.
Test Plan: Used `scripts/repository/reparse.php --message ...` to parse commits with revert language. Verified they appear correctly in Diffusion, and update Differential.
Reviewers: btrahan, epriestley
Reviewed By: btrahan, epriestley
Subscribers: Korvin, epriestley, cburroughs, joshuaspence, sascha-egerer, aran
Maniphest Tasks: T4896, T1751
Differential Revision: https://secure.phabricator.com/D5846
Summary:
Ref T2783.
This updates PhabricatorRepositoryManagementLookupUsersWorkflow to use ConduitCall to retrieve information about the commit.
Test Plan:
Ran `bin/repository lookup-users rTESTe9683b64d3283f0b2d355fdbf231bc918b5ac0ab --trace` and saw the information returned (by making a request to `diffusion.querycommits` as the omnipotent user, signed with the device key).
Mucked with `cluster.addresses` and saw requests rejected.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
Subscribers: Krenair, epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10403
Summary:
Ref T2783. This is basically a more refined version of D10400, which churned a bit on things like SSH key storage, the actual way the signing protocol shook out, etc.
- When Phabricator tries to make an intra-cluster service call as the omnipotent user, sign it with the host's device key.
- Add `bin/almanac register` to say "this host is X device, identified by private key Y". This stores the keypair locally, adds the public key to Almanac, and trusts it.
Net effect is that once a host has been registered, the daemons can make calls to other nodes as the omnipotent user. This is primarily necessary so they can access repository API methods on remote hosts.
Test Plan:
- Ran `bin/almanac register` with various valid and invalid inputs.
- Verified keys get generated/added/stored properly.
- Made a device-signed cluster Conduit call.
- Made a normal old user-signed cluster Conduit call.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11158
Summary:
Ref T4245 Make repo callsigns optional
This is far from done and still very ugly. I'm just submitting it to check if i'm solving this in the right places.
Right now there's three places with duplicate code and building the identifierMap in the CommitQuery is very ugly.
If we only want to support this in the user frontend then i could hack it into the Markup rule itself and not touch the CommitQuery. Even uglier but more limited in scope...
Generally this approach will need a lot of "check this first and then try the other" in a few places.
I could move the Repository queries into a specialised PhabricatorRepositoryQuery method (withCallsignOrID) but i'm not sure about that.
Test Plan:
- phid.lookup works with R1 and rTEST (which is the same repo)
- R1 and rTEST euqally work in remarkup (tested in comments).
- Reviewed the following syntax also all works:
rTEST
rTESTd773137a7cb9
rTEST:d773137a7cb9
R1
R1:d773137a7cb9
d773137a7cb9
{rTEST}
{rTESTd773137a7cb9}
{rTEST:d773137a7cb9}
{R1}
{R1:d773137a7cb9}
{d773137a7cb9}
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D11050
Summary: Modernize Differential edges to subclass `PhabricatorEdgeType`. Largely based on D11045.
Test Plan: From previous experience, these changes are fairly trivial and safe. I poked around a little to make sure things looked reasonably okay.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, Krenair, epriestley
Differential Revision: https://secure.phabricator.com/D11074
Summary:
Ref T2783. This method is kind of goofballs:
- We send a big list of paths to it.
- It sends back a giant blob of HTML.
Instead, just figure out the path we want locally, then fetch the content with `diffusion.filecontentquery`.
Test Plan:
- Viewed main view and directory view, saw a README.
- See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11099
Summary:
Ref T2783. Currently, the repository edit page does some checks agaisnt the local system to look for binaries and files on disk. These checks don't make sense in a cluster environment.
Ideally, we could make a Conduit call to the host (e.g., add something like `diffusion.querysetupstatus`) to do these checks, but since they're pretty basic config things and cluster installs are advanced, it doesn't seem super worthwhile for now.
Test Plan: Saw fewer checks in a cluster repo.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11102
Summary: This is a fake hash of many 0s which ends up being a bad link. Detect the fake hash and don't print a link. Fixes T6826.
Test Plan: looked at push log and no longer saw a many 0 entry for the first old ref.
Reviewers: epriestley, chad
Reviewed By: chad
Subscribers: Korvin, epriestley
Maniphest Tasks: T6826
Differential Revision: https://secure.phabricator.com/D11096
Summary: Ref T5402. This more or less "fixes" it but there's probably some polish to do?
Test Plan:
stopped and started daemons. error logs look good.
ran bin/storage upgrade. noted that `adjust` added the appropriate indices for active and archive task.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T5402
Differential Revision: https://secure.phabricator.com/D11044
Summary: Fixes T6790. Turn the old method into "new" (old signature) and "newEphemeral". Deploy "newEphemeral" as many places as possible; basically places we are not in the Differential application *and* have no intentions of ever saving the diff. These callsites are also all places we are just trying to get some changesets at the end of the day.
Test Plan: set differential application policy to 'administrators only'. viewed a commit in diffusion and it worked without any errors! i'm just using my thinkin' noodle on the other code paths.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6790
Differential Revision: https://secure.phabricator.com/D11020
Summary:
Ref T2783. When creating a new repository, test for cluster services. If cluster services are available, allocate on a random open service.
Show the service that repositories are allocated on.
Test Plan: Created a new repository, saw it allocate onto an available cluster service.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11003
Summary: Ref T2783. In Diffusion -> Edit Repository, we currently have a section called "Local" with options about where the repository is stored. The current name is misleading in a cluster environment, where storage may not actually be local. Shortly, this will also have an option for cluster storage. Call this "Storage" instead.
Test Plan: Edited a repository and poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11001
Summary:
Ref T5833. This allows services to be typed, to distinguish between different kinds of services. This makes a few things easier:
- It's easier for clients to select the services they're interested in (see note in T5873 about Phacility). This isn't a full-power solution, but gets is some of the way there.
- It's easier to set appropriate permissions around when modifications to the Phabricator cluster are allowed. These service nodes need to be demarcated as special in some way no matter what (see T6741). This also defines a new policy for users who are permitted to create services.
- It's easier to browse/review/understand services.
- Future diffs will allow ServiceTypes to specify more service structure (for example, default properties) to make it easier to configure services correctly. Instead of a free-for-all, you'll get a useful list of things that consumers of the service expect to read.
The "custom" service type allows unstructured/freeform services to be created.
Test Plan:
- Created a new service (and hit error cases).
- Edited an existing service.
- Saw service types on list and detail views.
- Poked around new permission stuff.
- Ran `almanac.queryservices` with service class specification.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5833
Differential Revision: https://secure.phabricator.com/D10995
Summary:
Ref T5955. Ref T2783.
- Removes the "temporary" type. I was going to use this for T3628 but it started taking more time than I wanted to spend on it.
- Add a "cluster" type, which is an internal-only token type used within a cluster. This token value is never shown to the user.
- Automatically generate, use, and cycle cluster tokens.
Test Plan:
- Diffusion (mostly) works with a repository configured to use a remote service.
- Saw cluster tokens generate; terminated a cluster token and saw it regenerate.
- Viewed cluster token in settings panel and saw nice explanatory text instead, as expected (we might just hide these eventually).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T5955
Differential Revision: https://secure.phabricator.com/D10990
Summary:
Ref T2783. This is primarily exploratory and just figuring out what we're blocked on:
- Allow a Repository to be bound to a Service. The Service may eventually define multiple read/write nodes, etc.
- There's no UI to do this binding yet, you have to touch the database manually.
- If a repository is bound to a Service, effect Conduit calls via calls to the remote service instead of executing them in-process.
- These don't actually work yet since there's no authentication (see T5955).
Test Plan:
- Made a nice Service with a nice Binding to a nice Interface on a nice Device.
- Force-associated a repository with the service using a raw MySQL query.
- Saw Phabricator try to make a remote call to the service (on localhost) and fail because of missing auth stuff.
- Also ran `almanac.queryservices`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10982
Summary:
Ref T4712. Specifically...
- Differential
- needed getApplicationTransactionViewObject() implemented
- Audit
- needed getApplicationTransactionViewObject() implemented
- Repository
- one object needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true)
- Ponder
- BONUS BUG FIX - leaving a comment on an answer had a bad redirect URI
- both PonderQuestion and PonderAnswer needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on both "history" controllers
- left a "TODO" on buildAnswers on the question view controller, which is non-standard and should be re-written eventually
- Phortune
- BONUS BUG FIX - fix new user "createNewAccount" code to not fatal
- PhortuneAccount, PhortuneMerchant, and PhortuneCart needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) on Account view, merchant view, and cart view controller
- Fund
- Legalpad
- Nuance
- NuanceSource needed PhabricatorApplicationTransactionInterface implemented
- Releeph (this product is kind of a mess...)
- HACKQUEST - had to manually create an arcanist project to even be able to make a "product" and get started...!
- BONUS BUG FIX - make sure to "setName" on product edit
- ReleephProject (should be ReleepProduct...?), ReleephBranch, and ReleepRequest needed PhabricatorApplicationTransactionInterface implemented
- Harbormaster
- HarbormasterBuildable, HarbormasterBuild, HarbormasterBuildPlan, and HarbormasterBuildStep all needed PhabricatorApplicationTransactionInterface implemented
- setShouldTerminate(true) all over the place
Test Plan: foreach application, viewed the timeline(s) and made sure they still rendered
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4712
Differential Revision: https://secure.phabricator.com/D10925
Summary: These have all been modernized.
Test Plan: Browse Diffusion on a narrow screen.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D10920
Summary:
Fixes T6619. In `{Xnnn key=value, key=value}` we did not require a separator between the object and the key-value part. This could lead to `{rX11aaa}` being parsed as `{rX11 aaa}`, i.e. a reference to `rX11` with parameter `aaa` set.
Instead, require a space or comma before we'll parse key-value parts of embedded objects.
Test Plan:
Added and executed unit tests.
{F242002}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6619
Differential Revision: https://secure.phabricator.com/D10915
Summary: See <https://phabricator.wikimedia.org/T906>. This behavior is a bug; we should remove the button if the user can't use the application.
Test Plan:
- With Macro uninstalled, did these things verifying the button vanished:
- Sent a user a message.
- Edited a revision.
- Edited repository basic information.
- Edited an initiative.
- Edited a Harbormaster build step.
- Added task comments.
- Edited profile blurb.
- Edited blog description.
- Commented on Pholio mock.
- Uploaded Pholio image.
- Edited Phortune merchant.
- Edited Phriction document.
- Edited Ponder answer.
- Edited Ponder question.
- Edited Slowvote poll.
- Edited a comment.
- Reinstalled Macro and saw button come back.
- Used button to put silly text on a funny picture.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D10900
Summary: Fixes T3189. Now if you say #projects in a commit message they will associate nicely with the commit. Also we record transactions about all this project editing fun.
Test Plan: tested migration by associating some projects with commits and verifying they still showed up post migration. tested adding / removing projects by doing so from the UI, noting transactions written nicely as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Projects: #projects
Maniphest Tasks: T3189
Differential Revision: https://secure.phabricator.com/D10877
Summary: Fixes T6152, T6237. This introduces a viewPolicy column to the DifferentialDiff, and re-jiggers the DifferentialDiff policy implementation such that things behave as before once associated with a revision, else use the DifferentialDiff policy column value.
Test Plan: made a diff with a non-standard view policy and noted that policy was still selected in the revision step. arc lint.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6237, T6152
Differential Revision: https://secure.phabricator.com/D10875
Summary: ...if pertinent environment variables are set that is... Fixes T4151. This is the last piece in making repository creation somewhat easier.
Test Plan: made a new repo and noted that http serving was on r/w and ssh serving was still off, as expected for my environment configuration
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4151
Differential Revision: https://secure.phabricator.com/D10839
Summary: Fixes T6395. Ref T6350. I guess I missed this code spot in prior testing / I definitely didn't run an empty commit through it. Works now though.
Test Plan: made an empty commit and observed stuck importing status and errors in phd log. applied patch and commit successfully imported with no errors. made another empty commit and it imported as well
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T6350, T6395
Differential Revision: https://secure.phabricator.com/D10746
Summary: Ref T6345, This adds more consistent color choices to match how Phabricator generally works across Differential/Diffusion per user statuses.
Test Plan: Review a few Audits in my sandbox.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
Subscribers: Korvin, epriestley
Maniphest Tasks: T6345
Differential Revision: https://secure.phabricator.com/D10726
Summary:
Ref T5702. This is a forward-looking change which provides some very broad API improvements but does not implement them. In particular:
- Controllers no longer require `$request` to construct. This is mostly for T5702, directly, but simplifies things in general. Instead, we call `setRequest()` before using a controller. Only a small number of sites activate controllers, so this is less code overall, and more consistent with most constructors not having any parameters or effects.
- `$request` now offers `getURIData($key, ...)`. This is an alternate way of accessing `$data` which is currently only available on `willProcessRequest(array $data)`. Almost all controllers which implement this method do so in order to read one or two things out of the URI data. Instead, let them just read this data directly when processing the request.
- Introduce `handleRequest(AphrontRequest $request)` and deprecate (very softly) `processRequest()`. The majority of `processRequest()` calls begin `$request = $this->getRequest()`, which is avoided with the more practical signature.
- Provide `getViewer()` on `$request`, and a convenience `getViewer()` on `$controller`. This fixes `$viewer = $request->getUser();` into `$viewer = $request->getViewer();`, and converts the `$request + $viewer` two-liner into a single `$this->getViewer()`.
Test Plan:
- Browsed around in general.
- Hit special controllers (redirect, 404).
- Hit AuditList controller (uses new style).
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5702
Differential Revision: https://secure.phabricator.com/D10698
Summary:
I am not sure how valuable this is *as is* - I think it needs different explanations for what happened in mercurial or subversion? I do not know what those explanations are.
Made an error in D10485 - the $hashes that were saved is an array of objects, so it ends up turning into garbage via the wonders of serialization and de-serialization. Fix that by explicitly saving the tree hash.
I would like to make this work for the other VCS types we support, add the "undo / nope" button and call it fixed.
Ref T3686.
Test Plan: clicked "explan why" and saw why
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5693, T3686
Differential Revision: https://secure.phabricator.com/D10489
Summary: Fixes T6201. This stuff didn't fully get updated for ApplicationTransactions. Get it working again (notably, make inline comment text publish) and clean it up a little bit.
Test Plan:
- Published a Differential feed story into Asana with comment text.
- Pulbished a Diffusion feed story into Asana with comment text.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6201
Differential Revision: https://secure.phabricator.com/D10584
Summary:
Fixes T6084. Changes:
- Rename `phabricator.show-beta-applications` to `phabricator.show-prototypes`, to reinforce that these include early-development applications.
- Migrate the config setting.
- Add an explicit "no support" banner to the config page.
- Rename "Beta" to "Prototype" in the UI.
- Use "bomb" icon instead of "half star" icon.
- Document prototype applications in more detail.
- Explicitly document that we do not support these applications.
Test Plan:
- Ran migration.
- Resolved "obsolete config" issue.
- Viewed config setting.
- Browsed prototypes in Applications app.
- Viewed documentation.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6084
Differential Revision: https://secure.phabricator.com/D10493
Summary:
Ref T2783. Fixes T6039.
- Provide `authorPHID` and `committerPHID` to resolve T6039.
- In message parser, store author/email strings.
- In cached results, emit author/email strings.
Test Plan: Called method with and without bypassCache. Used `reparse.php` to repopulate data on an old commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2783, T6039
Differential Revision: https://secure.phabricator.com/D10424
Summary:
Ref T6013. A very long time ago, edges were less clearly low-level infrastructure, and some user-aware stuff got built around edge edits.
This was kind of a mess and I eventually removed it, during or prior to T5245. The big issue was that control flow was really hard to figure out as things went all the way down to the deepest level of infrastructure and then came back up the stack to events and transactions. The new stuff is more top-down and generally seems a lot easier and cleaner.
Consequently, actors are no longer required for edge edits. Remove the parameter.
Test Plan: Poked around; ran unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T6013
Differential Revision: https://secure.phabricator.com/D10412
Summary:
Ref T2783. This populates the following fields in DiffusionQueryCommitsConduitAPIMethod using DiffusionLowLevelCommitQuery when `bypassCache` is set to true:
* `authorName`
* `authorEmail`
* `committerName`
* `committerEmail`
* `message`
* `hashes`
The original outline called for `authorPHID` and `committerPHID` as well (but no `message` field). As far as I can tell, the PHIDs aren't actual a property on `DiffusionCommitRef`, and since the intention of this is to be able to populate a `DiffusionCommitRef`, I haven't included them. Let me know if we really do need the PHIDs here.
Test Plan: Tested using 3 Phabricator instances (one web, one taskmaster and one storage). The web and taskmaster tiers are directed at the Conduit API of the storage tier. Made a `diffusion.querycommits` from the Conduit app on the web tier instance and saw the data populated from the raw VCS data (located on the storage tier).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D10399
Summary: Ref T3307. Only one I thought was tricky was Excel; I went with bytes there like it was email.
Test Plan: played around on a few endpoints but mostly thought carefully
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T3307
Differential Revision: https://secure.phabricator.com/D10392
Summary: Fixes T4387.
Test Plan: Setup a mercurial repository for rabbitmq-server. Browsed around it and things looked good.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4387
Differential Revision: https://secure.phabricator.com/D10380
Summary: Looks like I missed this when implementing custom actions and hence you can't currently use custom actions on the pre-commit adapters.
Test Plan: Added a custom action to a pre-commit Herald rule.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10316
Summary: Fixes T2564. See screenshot.
Test Plan:
{F194796}
- Made a bunch of valid and invalid adjustments here and verified that the branches table showed autoclose state and branches consistent with the settings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2564
Differential Revision: https://secure.phabricator.com/D10349
Summary:
Fixes T4767. I believe 80% of this was actually caused by the author issue fixed in T5771, but this should help make the other 20% debuggable.
- Record why we didn't autoclose a commit when we process it.
- Show branch autoclose status in the main branch table.
- Show commit autoclose status on the edit screen.
- Add documentation about how to find these statuses and what they mean.
Test Plan:
- Read documentation.
- Viewed branches and hovered over the various states.
- Viewed commits in various states and checked the "Autoclose?" field.
- Pushed some commits and saw autoclose activate.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767
Differential Revision: https://secure.phabricator.com/D10348
Summary: be more aggressive about assuming plain-text, use remarkup for no extension, .remarkup, and .md, and last but not least use rainbow for .rainbow. Fixes T5818.
Test Plan: my README rendered just fine post these changes
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: asherkin, epriestley, Korvin
Maniphest Tasks: T5818
Differential Revision: https://secure.phabricator.com/D10340
Summary: Fixes T5942. These are external but currently unmarked.
Test Plan: Visited link, got redirected.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5942
Differential Revision: https://secure.phabricator.com/D10332
Summary:
Fixes T5926. Fixes T5830. Ref T4767. Users currently sometimes have a hard time understanding repository update frequencies. This is compounded by aggressive backoff and incorrect backoff while importing repositories.
- Don't back off while importing repositories. This prevents us from hanging at 99.99% for inactive repositories while waiting for the next update.
- Back off less aggressively in general, and even more gradually during the first 3 days. This should make behavior around weekends better.
- Show update frequency in the UI.
- Provide an explicit "update now" button to call `diffusion.looksoon` in a more user-friendly way.
- Document how backoff policies work and how to adjust behavior.
Test Plan:
- Ran `bin/phd debug pulllocal` and verified backoff worked correctly from debugging output.
- Clicked "Update Now" to get a hint, reloaded page to see it update.
- Read documentation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4767, T5830, T5926
Differential Revision: https://secure.phabricator.com/D10323
Summary:
Fixes T5934. If you hash a password with, e.g., bcrypt, and then lose the bcrypt hasher for some reason, we currently fatal when trying to figure out if we can upgrade.
Instead, detect that the current hasher implementation has vanished and let the user reset their password (for account passwords) or choose a new one (for VCS passwords)>
Test Plan:
Account password:
- Artifically disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Used password reset workflow to change password, saw iterated MD5 hashed password get set.
- Enabled bcrypt hasher again.
- Saw upgrade warning.
- Upgraded password to bcrypt.
VCS password:
- Artificially disabled bcrypt hasher.
- Viewed password panel, saw warnings about missing hasher.
- Reset password.
- Saw iterated md5 password.
- Reenabled bcrypt.
- Upgraded to bcrypt.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5934
Differential Revision: https://secure.phabricator.com/D10325
Summary: Ref T5894. We have a couple more similar cases. Make them all do a decision-based redirect for now.
Test Plan: Did "View Raw File" and such, and also made sure thumbnails still work.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5894
Differential Revision: https://secure.phabricator.com/D10301
Summary: Fixes T5871. These queries get to use the actual column on the commit table since they are about the "aggregate" state of different audits.
Test Plan: issues queries and got sensible results.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5871
Differential Revision: https://secure.phabricator.com/D10271
Summary: Ref T5862. makes the exception work better
Test Plan: issued some queries from audit ui with and without repos - they worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10268
Summary: Fixes T5862. The Diffusion table uses `id` but all the other infrastructure uses `phid` so just do a quick load of the repositories to get the ids. Long term, we should re-key the table by phid I think.
Test Plan: made a query with a repository and got a proper result set
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T5862
Differential Revision: https://secure.phabricator.com/D10245
Summary: Fixes T5579. Modern browsers aggressively autofill credentials, but at least Firefox still behaves slightly better with this flag. Hopefully other browsers will follow suit.
Test Plan: Browsed various interfaces, verifying that login interfaces allow autocomplete while non-login interfaces do not.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5579
Differential Revision: https://secure.phabricator.com/D10253
Summary: Fixes T5869. Ref T4896. This `setID()` method no longer exists.
Test Plan: (WARNING) This is a pain to reproduce locally so I'm just winging it. I'm 99% sure this ID is only used to generate an anchor link. This is a hack to start with, and T4896 will eventualy clean it up properly.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896, T5869
Differential Revision: https://secure.phabricator.com/D10254
Summary:
Fixes T5839. If a repository has been force pushed and garbage collected, we might have a ref cursor in the database which still points at the old commit (which no longer exists).
We'll then run a command like `git log <new hash> --not <old hash>` to figure out which commits are newly pushed, and this will bomb out because `<old hash>` is invalid.
Instead, validate all the `<old hash>` values before we try to make use of them.
Test Plan:
- Forced a repository into a bad state by mucking with the datbase, generating a reproducible failure similar to the one in T5839.
- Applied patch.
- `bin/repository update <callsign> --trace` filtered the bad commit and put the repository into the right state.
- Saw new commits recognized correctly.
- Ran `bin/repository update <callsign>` for a Mercurial and SVN repo as a sanity check.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5839
Differential Revision: https://secure.phabricator.com/D10226
Summary:
Ref T4896. Now that we have a transaction editor, we can delete a giant block of hacks.
I believe this also resolves the commit/task attachment issues @joshuaspence and @mbishopim3 mentioned.
Test Plan: Attached and detached commits and tasks.
Reviewers: btrahan, joshuaspence, mbishopim3
Reviewed By: mbishopim3
Subscribers: mbishopim3, epriestley, joshuaspence
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10138
Summary: Ref T4896. Use the new transaction-oriented `PhabricatorAuditEditor` directly instead of invoking it via the old editor.
Test Plan: Used Conduit to add a comment, use silent mode, and accept a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10126
Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.
There are no longer any readers or writers for metadata, so remove the calls for it.
Test Plan: Added auditors from the web UI.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10123
Summary: Ref T4896. Instead of using custom stuff, use standard stuff.
Test Plan: Viewed a bunch of feed stories and published some over the Asana bridge.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10114
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:
- No more fake proxy writes;
- no more fake detection of `@mentions`.
For now, the old code still applies most of the effects and handles feed and email.
Test Plan:
- Added comments.
- Added comments with inline comments.
- Added just inline comments.
- Added comments with Conduit.
- Previewed comments.
- Added CCs explicitly and with `@mentions`.
- Added auditors.
- Accepted a commit.
Reviewers: joshuaspence, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10109
Summary:
Ref T4896. Currently, subscriptions to commits are stored as auditors with a special "CC" type.
Instead, use normal subscriptions storage, reads and writes.
Test Plan:
- Ran migration and verified data still looked good.
- Viewed commits in UI and saw "subscribers".
- Saw "Automatically Subscribed", clicked Subscribe/Unsubscribe on a non-authored commit, saw subscriptions update.
- Pushed a commit through Herald rules and saw them trigger subscriptions and auditors.
- Used "Add CCs".
- Added CCs with mentions.
Reviewers: btrahan, joshuaspence
Reviewed By: btrahan, joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10103
Summary: This class was renamed in D9991, but the filename is incorrect.
Test Plan: Eyeball it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10117
Summary: Ref T4896. Depends on D10055. This uses core rendering stuff for audit comments, and fixes all the wonkiness with inlines so we can actually land the migration.
Test Plan: Viewed, previewed and edited various types of comments in Diffusion.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10056
Summary:
Ref T4896. Depends on D10052. This is the major/scary migration, but not really so bad. It is substantially similar to D8210, but less complex because there are fewer actions here.
This moves `PhabricatorAuditComment` storage to `PhabricatorAuditTransaction`, then reads `PhabricatorAuditComment`s as a proxy around the new objects.
Test Plan:
- Before migrating, browsed around. Nothing appeared broken.
- Migrated cleanly.
- Viewed old transactions (inlines, comments, accept/reject/etc, add auditors, add ccs, implicit CCs).
- Added all of those comment types.
- Edited a draft.
- Deleted a draft.
- Spot checked the database for sanity.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10055
Summary:
Ref T4896. Depends on D10023. Prepares the code for the final migration.
The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.
Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.
Test Plan:
- Used `diffusion.createcomment` to add comments, raise concern, and accept.
- Previewed commenting, adding auditors/ccs, accepting, raising concern.
- Actually performed commenting, adding auditors/ccs, accepting, raising concern.
- Added a user with mentions.
- Added an explicit CC and a mention user.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10052
Summary:
Handling readmes with no extension is a bit of a hack, but seemed like a small cost.
The Big Win here is that you can commit README.remarkup and README.md and have both Phabricator and GitHub render __with__ //all// ##the## ~~pretty~~ **markup**.
Test Plan: Looked at some readme files.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10047
Summary: Ref T4896. Buries all direct access to the table so we can limit the surface area affected by the migration.
Test Plan:
- Grepped for `PhabricatorAuditComment`.
- Grepped for `audit_comment`.
- Viewed a bunch of comments.
- Added a comment.
- Reindexed a commit.
- Searched for unique term in new comment.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10019
Summary: Ref T4896. Move all direct accesses to the inline comment table behind a small amount of API to make it easier to migrate the table.
Test Plan:
- Grepped for `PhabricatorAuditInlineComment`.
- Grepped for `audit_inlinecomment`.
- Created a draft comment.
- Previewed a draft comment.
- Reloaded page, still saw draft.
- Viewed standalone, still saw draft.
- Made comment, inline published.
- Added a draft, saw both.
- Edited inline comment.
- Reindexed commit.
- Searched for unique word in published comment, found commit.
- Searched for unique word in draft comment, no results.
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T4896
Differential Revision: https://secure.phabricator.com/D10016
Summary: Instead of implementing the `getCapabilityKey` method in all subclasses of `PhabricatorPolicyCapability`, provide a `final` implementation in the base class which uses reflection. See D9837 and D9985 for similar implementations.
Test Plan: N/A
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D10039
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
