Summary:
fixes T8260. Only turn on symbol links if:
- The repository has any configuration about symbols, or
- There actually are symbols in the repository.
Test Plan: Look at revisions and files in various states of configurations and having symbols.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T8260
Differential Revision: https://secure.phabricator.com/D12946
Summary:
Fixes T7977.
- Move Indexed Languages and See Symbols From config to Repository
- Make symbol search skip projects
This also makes the default languages to Everything instead of Nothing.
Test Plan:
- Browse files, click symbols.
- Use quick search to find symbols
- Browse revision, click symbols
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7977
Differential Revision: https://secure.phabricator.com/D12687
Summary: Fixes T7220. Ref T7977. Changes symbols from being bound to an Arcanist project to being bound to a repository.
Test Plan:
- Added symbols and then applied migrations, symbols seemed to be migrated successfully.
- Tested the `/diffusion/symbol/$SYMBOL_NAME` endpoint.
- Tested the `/diffusion/symbol/$SYMBOL_NAME` endpoint with the `?repositories=$REPOSITORY_PHID` parameter.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: avivey, Korvin, epriestley
Maniphest Tasks: T7977, T7220
Differential Revision: https://secure.phabricator.com/D12608
Summary:
Fixes T6160. Ref T7100.
- When resolving ambiguous branch references, ignore closed heads unless there are no other options.
- Hide closed heads by default on the main page.
- Show branch open/closed state in Mercurial.
Test Plan: Browsed a previously-ambiguous Mercurial repository because of multiple branch heads, no longer ambiguous.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6160, T7100
Differential Revision: https://secure.phabricator.com/D12552
Summary: Ref T6160. Ref T7100. Mercurial branch heads can be closed; track this state so we can be smarter about it.
Test Plan: Closed a branch, run `repository update`, saw it close in the cursor table.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6160, T7100
Differential Revision: https://secure.phabricator.com/D12550
Summary:
Ref T7100. Ref T7108. Ref T6160. Several issues:
- High load for mercurial repositories with huge numbers of branches (T7108).
- In Mercurial, we resolve refs individually (one `hg` call per ref).
- Each repository update also updates all refs, which requires resolving all of them.
- For repositories with a huge number of branches,
- We don't distinguish between closed branches (a Mercurial-only concept) and open branches (T6160).
- In Git, when a branch is merged, it ceases to exist.
- In Mercurial, when a branch is merged, it still exists, it's just "closed". Normally, no one cares about these branches.
- In the low-level query, correctly identify which refs we resolve as branches.
- In the low-level query, correctly mark closed branches as closed.
- This marginally improves ref handling in general (see T7100).
Test Plan:
{F384366}
{F384367}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6160, T7108, T7100
Differential Revision: https://secure.phabricator.com/D12548
Summary:
Ref T7100. When a user navigates to a branch like "default" which is ambiguous:
- don't fatal;
- choose one alternative to resolve it to (currently more or less at random);
- sometimes show what we did in the UI.
Also, add a new table to show the alternatives.
This will get refined in followup changes.
Test Plan:
{F384335}
{F384336}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7100
Differential Revision: https://secure.phabricator.com/D12547
Summary: Fixes T7507, Create diffusion repo rejection should not navigate away from diffusion.
Test Plan: Login as non-admin, open diffusion, attempt to create new repo, rejection dialog should appear over page instead of navigating to new page.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7507
Differential Revision: https://secure.phabricator.com/D12557
Summary:
Ref T4100. I can simplify the logic a bit here by moving some rendering into the datasources, but a few TokenizerControls currently don't have datasources.
Require datasources and always provide datasources.
Test Plan:
- Used previously-datasourceless controls (e.g., "Add Reviewers").
- Used normal controls.
- Manually verified that no other controls are missing datasources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4100
Differential Revision: https://secure.phabricator.com/D12456
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: 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 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 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: 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: 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.
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: 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: 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 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:
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
