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
Summary: Provide an implementation for the `getName` method rather than automagically determining the application name.
Test Plan: Saw reasonable application names in the launcher.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D10027
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.
Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.
Reviewers: btrahan, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: hach-que, epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9982
Summary:
Ref T4420. If a datasource does not specify an icon explicitly, check if the PHID type has a default, and use that.
This leaves us with only Projects and some special stuff setting explicit icons, and reduces code duplication.
Test Plan: Used typeahead to find all affected object types.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9894
Summary: Ref T4420. Bring the global search up to date.
Test Plan: Typed various things into global search.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9889
Summary: Ref T4420. Call this "auditor" since that's what it is.
Test Plan:
- Edited auditors in auditor search.
- Edited auditors in "add auditors" in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9888
Summary: Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.
Test Plan: Added projects to some objects, viewed transactions in transaction record.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9849
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.
Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9848
Summary:
Ref T5245. These were a bad idea.
We no longer need actors for edge edits either, so remove those. Generally, edges have fit into the policy model as pure/low-level infrastructure, and they do not have any policy or capability information in and of themselves.
Test Plan: `grep`
Reviewers: chad, btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Maniphest Tasks: T5245
Differential Revision: https://secure.phabricator.com/D9840
Summary:
Fixes T5613. A branch may have multiple heads in Mercurial, but `executeOne()` expects exactly one result.
Load them all instead. Equivalently, we could `limit(1)`, but it's likely that we'll use the cursors in the future to reduce the number of VCS operations we do, so this is probably a little more along the lines where we're headed.
Test Plan: Poked around some repos.
Reviewers: chad, richardvanvelzen
Reviewed By: richardvanvelzen
Subscribers: epriestley
Maniphest Tasks: T5613
Differential Revision: https://secure.phabricator.com/D9918
Summary:
Ref T1493.
- When viewing an invalid branch, show a "there is no such branch" message.
- When viewing an empty repository, show a "this repository is empty" message.
Test Plan:
- Viewed empty, bad branch, and nonempty in Git.
- Viewed empty, bad branch, and nonempty in Mercurial.
- Viewed empty and nonempty in Subversion.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T1493
Differential Revision: https://secure.phabricator.com/D9912
Summary: Fixes T5336. Currently, `PhabricatorWorkerLeaseQuery` is basically FIFO. It makes more sense for the queue to be a priority-queue, and to assign higher priorities to alerts (email and SMS).
Test Plan: Created dummy tasks in the queue (with different priorities). Verified that the priority field was set correctly in the DB and that the priority was shown on the `/daemon/` page. Started a `PhabricatorTaskmasterDaemon` and verified that the higher priority tasks were executed before lower priority tasks.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5336
Differential Revision: https://secure.phabricator.com/D9871
Summary: Ref T4420. Update "projects" source.
Test Plan:
- Edited projects on a Differential revision.
- Edited projects on a commit.
- Edited projects on a repository.
- Edited projects in feed search.
- Edited projects in a Herald rule field.
- Edited projects in a Herald rule action.
- Edited projects in Maniphest batch editor.
- Edited projects on Maniphest task.
- Edited projects in "Associate Projects..." action in Maniphest.
- Edited projects on Maniphest search in "all projects", "any project" and "not projects" fields.
- Edited projects on a Paste.
- Edited projects on a Pholio mock.
- Edited projects on a custom policy rule.
- Edited projects on a Ponder question.
- Edited projects on a Diffusion search query.
- Edited projects on a global search query.
- Edited projects on a slowvote.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9884
Summary:
Ref T4420.
- Allow tokenizers to accept either a `Datasource` object (new style) or a URI (old style).
- Read URI and placeholder text from object, if available.
- Swap the "repositories" datasource (which seemed like the simplest one) over to the new stuff.
- Tweak/update the repo tokens a little bit.
Test Plan:
- Used tokenizer in Herald, Differential (search), Differential (edit), Push Logs.
- Grepped for other callsites.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9874
Summary:
Fixes T5534. If you `git push origin :refs/tags/doesnotexist` (for some non-existing tag), we get a change where both the old and new refs are empty.
We incorrectly call this an "add", because the old ref is empty. Instead, call this a "delete", but skip the logic which would normally mark it dangerous.
(Possibly we should just reject these outright, but Git allows them, so stick with that for now.)
Test Plan:
Pushed nonexistent refs:
```
$ git push origin :refs/tags/doesnotexist
remote: warning: Allowing deletion of corrupt ref.
To ssh://dweller@localhost/diffusion/POEMS/
- [deleted] doesnotexist
$
```
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5534
Differential Revision: https://secure.phabricator.com/D9800
Summary:
Ref T4715. Some minor stuff I caught locally while poking around:
- Since we don't `GROUP BY`, we can still get duplicate commits. These get silently de-duplicated by `loadAllFromArray()` because that returns an array keyed by `id`, but we fetch too much data and this can cause us to execute too many queries to fill pages. Instead, `GROUP BY` if we joined the audit table.
- After adding `GROUP BY`, getting the audit IDs out of the query is no longer reliable. Instead, query audits by the commit PHIDs. This is approximately equiavlent.
- Since we always `JOIN`, we currently never return commits that don't have any audits. If we don't know that all results will have an audit, just `LEFT JOIN`.
- Add some `!== null` to catch the `withIDs(array())` issue that we hit with Khan Academy a little while ago.
Test Plan:
- Verified that "All Commits" shows commits with no audits of any kind.
- Verified that the raw data comes out of the query without duplicates.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5433, T4715
Differential Revision: https://secure.phabricator.com/D8879
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary:
Ref T5446.
- For all callsites which do not specify a value, set `false` explicitly.
- Make `true` the default.
Test Plan: Used `grep`, then manually went through everything.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5446
Differential Revision: https://secure.phabricator.com/D9687
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary:
Fixes T5304. Mercurial features a "{branches}" template keyword, documented as:
```
branches List of strings. The name of the branch on which the
changeset was committed. Will be empty if the branch name
was default.
```
At some time long in the past, I misinterpreted this to mean "list of branches where the branch head is a descendant of the commit". It is more like "list of zero or one elements, possibly containing the name of the branch the commit was originally made to, if that branch was not 'default'".
In fact, it seems like this is because a //very// long time in the past, Mercurial worked roughly like I expected:
> Ages ago (2005), we had a very different and ultimately unworkable
> approach to named branches that worked vaguely like .hgtags and allowed
> multiple branch names per revision.
http://marc.info/?l=mercurial-devel&m=129883069414855
This appears to be deprecated in modern Mercurial (it's not in the modern web documentation) although I can't find a commit about it so maybe that's just a documentation issue.
In any case, `{branches}` seems to never be useful: `{branch}` provides the same information without the awkward "default-if-empty" case.
Switch from `{branches}` to either `{branch}` (where that's good enough, notably in the hook engine) or `(descendants(%s) and head())`, which is equivalent to `--contains` in Git.
This fixes pushing to branches with spaces in their names, and makes the "Branches" / "Contains" queries moderately more consistent.
Test Plan:
- Pushed to a Mercurial branch with a space in it.
- Viewed list of branches in a Mercurial repository.
- Viewed containing branches of a Mercurial commit in Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5304
Differential Revision: https://secure.phabricator.com/D9453
Summary:
Updated some old css to point at the new icon set
Fixes T5357
Test Plan: View it
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5357
Differential Revision: https://secure.phabricator.com/D9578
Summary: The CSS rule tends to miss many tables, make the rule more universal and add borders as needed.
Test Plan: Test a Revision and Diffusion
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9516
Summary: This implements showing the buildable status in Diffusion and unifies some of the logic used to calculate and render build and buildable statuses.
Test Plan: Looked at diffs and commits with statuses, they rendered fine. Looked at Diffusion and saw buildable status appear (with a manual buildable and manual buildables included in the query).
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9496
Summary:
Via HackerOne. There are two attacks here:
- Configuring mirroring to a `file://` URI to place files on disk or overwrite another repository. This is not particularly severe.
- Configuring cloning from a `file://` URI to read repositories you should not have access to. This is more severe.
Historically, repository creation and editing explicitly supported `file://` URIs to deal with use cases where you had something else managing repositories on the same machine. Since there were no permissions, repository management was admin-only, and you couldn't mirror, this was fine.
As we've evolved, this use case is a tiny minority use case and the security implications of `file://` URIs overwhelm the utility it provides. Prevent the use of `file://` URIs. Existing configured repositories won't stop working, you just can't add any new ones.
Also prevent `localPath` from being set via Conduit (see T4039).
Test Plan:
- Tried to create a `file://` repository.
- Tried to create a `file://` mirror.
- Tried to create a `file://` repository via Conduit.
- Created a non-`file://` repository.
- Created a non-`file://` mirror.
- Created a non-`file://` repository via Conduit.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9513
Summary: This UI recommends `bin/remove destroy X`, but should recommend `bin/remove destroy rX` (with `r`), because the remove script now takes any object monogram. The older script was repository-specific, so it only took the callsign.
Test Plan: {F166042}
Reviewers: putnam, chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9512
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.
Test Plan: Will run `arc unit` on another host.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9443
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary:
Fixes T5197. `hg log --rev x --rev y` means "rev x, and also rev y".
Use `--rev x:y`, which means "all commits between x and y, inclusive".
Test Plan: Pushed 4 commits at once, got 4 commits in push log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5197
Differential Revision: https://secure.phabricator.com/D9309
Summary: Ref T5197. When searching for split branch heads, we incorrectly consider descendant heads of other branches. This can cause us to detect a split tip when one does not exist (the old tip is the branch tip, but other descendant heads exist). Instead, consider only heads on the same branch.
Test Plan:
Repro is something like this:
- `hg update default`
- `hg branch branch1; hg commit ...`
- `hg push`
- `hg update default; hg commit ...`
- `hg push` - Previously, we would find the head of `branch1` and incorrectly account for it as a head of `default`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5197
Differential Revision: https://secure.phabricator.com/D9308
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:
- Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
- Old tile size methods are replaced with `isPinnnedByDefault()`.
- Shortened some short descriptions.
- `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
- Added a marker for third-party / extension applications.
Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D9358
Summary: Currently, repositories can be deleted using `./bin/repository delete`. It makes sense to expose this operate to the `./bin/remove` script as well, for consistency.
Test Plan: Deleted a repository with `./bin/remove rTEST`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9350
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.
Open to feedback.
Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)
{F160052}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T5176
Differential Revision: https://secure.phabricator.com/D9297
Summary:
Fixes T5199. We try to save these options in user preferences, but logged-out users don't have preferences.
Instead, just use GET links for logged-out users.
Test Plan:
- As a logged-out user, toggled blame and highlight on and off.
- As a logged-in user, toggled blame and highlight on and off.
Reviewers: btrahan, vrana
Reviewed By: vrana
Subscribers: epriestley
Maniphest Tasks: T5199
Differential Revision: https://secure.phabricator.com/D9310
Summary: Highlighing and URL are fixed on click - now the edit button too.
Test Plan: click on lines with and without value in "Editr Link" (And without %l in it).
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9227
Summary: Fixes T5113. This was caught in the crossfire of cleaning up the DiffusionRequest "commit" properties.
Test Plan: Loaded `/rXnnnn` with some of the `nnn` missing.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5113
Differential Revision: https://secure.phabricator.com/D9253
Summary:
Updates policy, headers, typeaheads to FA over policy icons
Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late
Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names
Browsed numerous places, checked new dropdowns, see pudgy people.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4739
Differential Revision: https://secure.phabricator.com/D9179
Summary:
Ref T4994. This stuff works:
- You can dump a blob of coverage information into `diffusion.updatecoverage`. This wipes existing coverage information and replaces it.
- It shows up when viewing files.
- It shows up when viewing commits.
This stuff does not work:
- When viewing files, the Javascript hover interaction isn't tied in yet.
- We always show this information, even if you're behind the commit where it was generated.
- You can't do incremental updates.
- There's no aggregation at the file (this file has 90% coverage), diff (the changes in this commit are 90% covered), or directory (the code in this directory has 90% coverage) levels yet.
- This is probably not the final form of the UI, storage, or API, so you should expect occasional changes over time. I've marked the method as "Unstable" for now.
Test Plan:
- Ran `save_lint.php` to check for collateral damage; it worked fine.
- Ran `save_lint.php` on a new branch to check creation.
- Published some fake coverage information.
- Viewed an affected commit.
- Viewed an affected file.
{F151915}
{F151916}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: jhurwitz, epriestley, zeeg
Maniphest Tasks: T5044, T4994
Differential Revision: https://secure.phabricator.com/D9022
Summary: Changes to using FontAwesome
Test Plan:
Testing UIExamples and each of the pages (except releelph)
{F155942}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9157
Summary: Fixes T5050. This might not be 100% right in all edge cases, but it worked on everything I tried.
Test Plan:
- Pushed a branch deletion.
- Pushed a branch creation.
- Pushed a brnach creation + deletion.
- Pushed a brnach deletion + creation.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5050
Differential Revision: https://secure.phabricator.com/D9122
Summary: Ref T5050. This fixes the immediate error (bad pht()) but doesn't fix the other error (can't `--close-branch`) yet.
Test Plan: Pushed a `--close-branch` commit, got a first-level error instead of an error about an error.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5050
Differential Revision: https://secure.phabricator.com/D9119
Summary:
Allows you to quickly search for files within a repository. Roughly:
- We build a big tree of everything and ship it to the client.
- The client implements a bunch of Sublime-ish magic to find paths.
Test Plan: {F154007}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9087
Summary: Ref T4986. Move push logs to a View, then have all the stuff that needs to use it use that View.
Test Plan: Viewed push logs and transaction detail in Diffusion. Created a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9104
Summary: Ref T2683. This field is //almost// entirely redundant with `symbolicCommit`. Improve how some of the diff query stuff works a bit, then remove it.
Test Plan: Browsed around in all interfaces, looked at a bunch of diffs, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9099
Summary:
Ref T2683. Currently, DiffusionRequest has four different "commitey" things:
- `commit`
- `rawCommit`
- `symbolicCommit`
- `stableCommit`
Of these, only two are actually distinct, useful values: `symbolicCommit` (which holds the value the request originally contained, if one existed) and `stableCommit` (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier).
- `rawCommit` is equivalent to `symbolicCommit` and can be simply removed.
- `commit` has some sketchy magic around it that needs to be pulled out before it can be jettisoned.
Test Plan: Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9098
Summary: Ref T2683. Throw a more tailored exception to allow callers to distinguish between bad refs (which are expected, if users try to visit garbage branches) and other types of errors.
Test Plan: Tried to view branch "alksndfklansdf". Viewed branch "master".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9094
Summary:
Ref T2683. This should probably just be `diffusion.filecontentquery` but keep things as they are for now.
This method uses a commit, so accept one. Soon, this will save a bit of work.
Test Plan: Viewed readmes in main and browse views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9093
Summary:
Ref T2683. The old name was a bit confusing because it meant "the type of the thing the symbol represents": a "commit type" should logically always be "commit".
(Currently, this is only used to detect when we're looking at a tag.)
Test Plan: Looked at a tag. Looked at some other non-tag things. Browsed around, `grep`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9092
Summary:
Ref T2683. This is closely related to "symbolicCommit", but has an inconsistent "name" on the end.
Also, `diffusion.searchquery` uses this parameter inconsistently.
Test Plan:
- `grep`ed for callsites.
- Ran searches in Git and Mercurial repositories.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9091
Summary:
Ref T2683. This has no callsites, and the functionality is covered by the `initFromConduit` flag.
This simplifies the code and reduces then number of internal `diffusion.resolverefs` calls we make on, e.g., the Git repository page from 7 to 2.
Test Plan: Grepped for these symbols.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9090
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.
Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9088
Summary:
Currently, Diffusion has very complex views. After three years I'm not really used to them and rarely use many of these options.
Simplify the browse and history views:
- Put the browse view on top.
- Move dates to the right.
- Remove "History" and "Edit" links from the browse view. You can access these actions by clicking the file/path.
- Remove "Browse" link from the history view. You can access this action by clicking the commit.
- Remove "Change Type", which is essentially never useful, from the history view.
- Add some tweaks for mobile.
Test Plan: {F153931}
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley, zeeg
Differential Revision: https://secure.phabricator.com/D9085
Summary: Ref T2683. Normally not a big deal, but if a readme has some codeblocks missing the cache can slow things down.
Test Plan:
- Verified we hit the cache.
- Verified TOC still works.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T5028, T2683
Differential Revision: https://secure.phabricator.com/D9049
Summary: Ref T2683. By resolving the stable name earlier, we can save a resolve when viewing branch heads. This is ~100ms in Mercurial, and roughly 25% of page weight. It's less bad in Git.
Test Plan: Saw page cost go down in "Services" tab, particularly for Mercurial browse views.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9048
Summary:
Ref T2683. At least locally, browse views are now nearly instantaneous, even in Mercurial. We also fall back to what we were doing before if we miss or take too long, so this shouldn't make things very much worse even in extreme cases.
For a local `hg` repo, the time we spend pulling browse stuff has dropped from ~3,000ms to ~20ms. This is probably atypical, but not completely crazy or rigged or anything.
Test Plan: Viewed Git, Subversion and Mercurial repositories and observed dramatically better performance in Git and Mercurial as they took advantage of the cache.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, jhurwitz
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D9047
Summary:
Ref T2683. Further reduces query count of last modified loads; we're now at 11 instead of 200+.
(This works in SVN but could be further optimized.)
Test Plan:
Loaded SVN, Mercurial, Git:
{F34864}
{F34865}
{F34866}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, vrana, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5256
Summary:
This code is currently quite complicated because we pull history data inline for SVN files, and via ajax for everything else (SVN dirs, everything in Git and Hg).
Always pull over ajax; batch some of the queries.
Test Plan: {F34860}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, vrana, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5255
Summary: Ref T2683. Instead of sending one request for each path's history, send one request for all of it. This permits optimizations which are not currently available to us. It degrades the user experience a tiny bit in theory, but on my machine it's actually way faster already.
Test Plan: Loaded a browse page.
Reviewers: vrana, btrahan
Reviewed By: btrahan
Subscribers: epriestley, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5254
Summary: Ref T2683. This query is currently unbatched and happens inside a view. Leave it inside the view for now, but separate it and make it batchable.
Test Plan: {F34848}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, vrana, aran
Maniphest Tasks: T2683
Differential Revision: https://secure.phabricator.com/D5252
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.
Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9052
Summary: Some profiling using XHProf in the Dark Console showed me that Diffusion was wasting a ton of time on array_merge. This change sped up the loading of a large file in Diffusion from 16.8 seconds to 2.4 seconds.
Test Plan: Load files in Diffusion. They all look good. Also, use a PHP shell to try to manually verify that I still kinda remember some PHP and, yes, this is functionally equivalent to what was there before.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9032
Summary: Ref T4986. I think this is the last of the easy ones, there are about 10 not-quite-so-trivial ones left.
Test Plan:
- Viewed app results.
- Created panels.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9025
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.
Test Plan:
For each engine:
- Viewed the application;
- created a panel to issue the query.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4986
Differential Revision: https://secure.phabricator.com/D9017
Summary:
Fixes T4960. Users `chmod +x` this, and then bash chokes on it.
Phabricator "owns" this file anyway, so there is no real ambiguity here: this should never be a hook script.
Test Plan:
- Did `chmod +x README`.
- Made a commit.
- Added `z.sh`, got blocked.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T4960
Differential Revision: https://secure.phabricator.com/D8981
Summary:
Partially reverts D8903. This was hacky to begin with, but completely breaks if the filetree is enabled (`$view` is not an array).
Just toss it until we have a more structured way to insert it into the document properly. I don't think it's especially important (the Herald warning is way more important).
Test Plan: Multiple users reported that stuff is no longer broken.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8914
Summary: This fixes a crash that happens when visiting Diffusion pages due to an undefined variable. `$title` is only defined if it has a status to show, but then it uses it anyway and fails.
Test Plan: Pages stopped crashing and people stopped complaining.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8906
Summary: 'cuz things fail a bunch until importing is done. Fixes T4094.
Test Plan: set isImporting to return true. Browsed Diffusion and saw helpful warnings everywhere. Browse Herald transcript and saw a helpful warning
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4094
Differential Revision: https://secure.phabricator.com/D8903
Summary:
Fixes T4911. See D8879. This gives us the correct query in cases where there are no audits.
This doesn't try to do the GROUP BY stuff yet.
Test Plan:
- Viewed a commit in Diffusion with no audits, got a commit detail page.
- Viewed "All Commits" in web UI, saw commits without any audits included in the list.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4911
Differential Revision: https://secure.phabricator.com/D8882
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)
Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4715
Differential Revision: https://secure.phabricator.com/D8805
Summary:
When showing contents of a file with the blame mode enabled, tooltips pops out
when the mouse hovers over previous commit linkes on left side. The last part of the
tooltips is the author's name. If an author is unregistered, the name becomes
<span>name</span>.
{F147724}
This doesn't happen if the author is registered.
Test Plan:
Check tooltips after making the change.
{F147725}
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8869
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.
The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.
Test Plan: Ran `diffusion.querycommits`. Requested a commit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T3662
Differential Revision: https://secure.phabricator.com/D8823
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.
Test Plan: Looked at a revision, task, paste, macro, etc.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8812
Summary:
Fixes T4759.
Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.
Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4759
Differential Revision: https://secure.phabricator.com/D8727
Summary: Fixes T4687. This was also pretty easy...!
Test Plan: made a package with a test user as owner. added package as owner. looked right on commit page. logged in as test user and verified audit showed up on home page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, epriestley, Korvin
Maniphest Tasks: T4687
Differential Revision: https://secure.phabricator.com/D8705
Summary: Ref T4687. Trickier part is adding packages; will require some typeahead core changes
Test Plan: add a project as an auditor succuessfully!
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4687
Differential Revision: https://secure.phabricator.com/D8704
Summary: This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)
Test Plan:
Loaded this diff in Chrome, Firefox, IE9, and IE8:
{F137505}
(That's a screenshot from Chrome, but it looks about the same in the other browsers.)
Reviewers: chad, #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, chad
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D8686
Summary: Ref T4065. Give administrators an "Edit Settings" link from profiles, which allows selective edit of settings panels. Enable Conduit, SSH Keys, and VCS Password.
Test Plan:
- Used these panels for a bot.
- Used these panels on my own account.
- Tried to use these panels for a non-bot account, was denied.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4065
Differential Revision: https://secure.phabricator.com/D8668
Summary: Fixes T4697. When pushing moved/copied files, SVN sends an "add-file" protocol frame which has a URI in it that needs translation from external format ("/diffusion/X/") to internal format ("/path/to/svn").
Test Plan:
- Copied/moved files and committed them in SVN.
- Added files (no copy/move) and committed them in SVN.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4697
Differential Revision: https://secure.phabricator.com/D8654
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.
For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.
Overall, this is basically the same as commit email, but more suitable for some workflows.
Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:
{F134929}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8618
Summary:
Ref T4677. This shows a more detailed view of an entire "git push", "hg push", or "svn commit".
This is mostly to give push summary emails a reasonable, stable URI to link to for T4677.
Test Plan:
- Pushed into SVN, Git and Mercurial.
- Viewed partial and imported event records.
{F134864}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8616
Summary:
Ref T4677. Currently, we record individual actions in a push as PhabricatorRepositoryPushLogs, but tie them together only loosely with a `transactionKey`.
Provide a real PushEvent object, and move some of the denormalized fields to it. This primarily just gives us more robust infrastructure for building, e.g., email about pushes, for T4677, since we can act on real PHIDs rather than passing awkward identifiers around.
Test Plan:
- Performed migration.
- Looked at database for consistency.
- Browsed/queried push logs.
- Pushed a bunch of stuff.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4677
Differential Revision: https://secure.phabricator.com/D8615
Summary:
- Point them at the new Diviner.
- Make them a little less cumbersome to write.
Test Plan: Found almost all of these links in the UI and clicked them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D8553
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.
When we care about this, use `\z` instead, which matches "end of input" only.
This allowed registration of `"username\n"` and similar.
Test Plan:
- Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
- Fixed the ones where this seemed like it could have an impact.
- Added and executed unit tests.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8516
Summary:
Currently, disabling Herald only disables feed, notifications and email. Historically, audits didn't really create external effects so it made sense for Herald to only partially disable itself.
With the advent of Harbormaster/Build Plans, it makes more sense for Herald to just stop doing anything. When this option is disabled, stop all audit/build/publish/feed/email actions for the repository.
Test Plan: Ran `scripts/repository/reparse.php --herald`, etc.
Reviewers: dctrwatson, btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Differential Revision: https://secure.phabricator.com/D8509
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:
- Committer identities must be authentic.
- Committer identities must be resolvable.
- Author identities must be resolvable.
Test Plan: Created some rules using these new rules and ran them.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4594
Differential Revision: https://secure.phabricator.com/D8507
Summary:
- Fixes T4588.
- See D8501.
- Adds a "Tags" field for Herald commit emails.
- Fixes a bug in `tagsquery` when filtering by commit name.
- Make `tagsquery` just return nothing instead of fataling against Mercurial/Subversion.
Test Plan: Used `bin/repository/reparse.php --herald` to exercise this code.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: aran, epriestley
Maniphest Tasks: T4588
Differential Revision: https://secure.phabricator.com/D8502
Summary: see title. Fixes T4549.
Test Plan: made a readme that had some headers and observed a nice ToC
Reviewers: chad, epriestley
Reviewed By: epriestley
Subscribers: aran, epriestley, Korvin
Maniphest Tasks: T4549
Differential Revision: https://secure.phabricator.com/D8490
Summary:
It appears a change to the way the configuration was loaded into ArcanistRepositoryAPI in rARCa2285b2b broke the save_lint script.
This updates the DiffusionLintSaveRunner to use the configuration correctly, allowing the linter to run
Test Plan: cd /your/project; ../../../path/to/phabricator/scripts/repository/save_lint.php
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: aran, epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D8487
Summary:
Ref T2222. This has some minor functionality regressions:
- The plain diff page no longer shows unit/test status. I want to give diffs separate custom fields for this.
- It was technically possible to shove more data on the list view, although this doensn't affect the default config.
Test Plan: Looked at list view, diff detail view. Grepped for changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8470
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary: We have a dozen users who has `(...)` in their 'real name', like 'Jimmy (He) Zhang', and it's causing the diffusion file browser problems when blame is enabled. The parser does not expect those parenthesis and the lines of code will be empty if they were last touched by a user like that.
Test Plan: Try it
Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8429
Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.
As it is, the URI instructs the user to check out the whole repository.
Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.
Test Plan:
- Created a remote SVN repository with "Import Only", saw path include it.
- Verified no "Clone As" options, no "Clone As" in URI.
- Switched it to hosted, saw "Clone As" options appear and work properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, staticshock
Differential Revision: https://secure.phabricator.com/D8375
Summary: Add in more ObjectBoxes
Test Plan: Test aphlict.swf, see new menu and button to download.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8305
Summary: For images and text, show the "Raw" buttons on the file's ObjectBox
Test Plan: View an image and a text file in Diffusion, click on the download link in each.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4467
Differential Revision: https://secure.phabricator.com/D8302
Summary: Moves this single action to the File Contents box in Diffusion Browse. Also fixes a PHUIObjectBox missing when enable highlighting is on.
Test Plan: Enable/Disable Highlighting. See disabled Editor button.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4467
Differential Revision: https://secure.phabricator.com/D8300
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.
Instead, only show upgrade prompts when the password exists.
Test Plan:
- Added a password to an existing account with no password via password reset.
- Added a VCS password to an existing account with no VCS password.
- Observed no fatals / nonsense behaviors.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4463
Differential Revision: https://secure.phabricator.com/D8282
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.
Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.
Test Plan:
- Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
- Drafts and flags still appear properly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8277
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.
Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T3496
Differential Revision: https://secure.phabricator.com/D8275
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.
Test Plan:
- Viewed VCS settings.
- Used VCS password after migration.
- Set VCS password.
- Upgraded VCS password by using it.
- Used VCS password some more.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4443
Differential Revision: https://secure.phabricator.com/D8272
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.
The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:
- We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
- We have way way too much config now.
- With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
- We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.
Removing this config is an easy fix which makes the codebase simpler.
I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.
Test Plan: Used `secure.phabricator.com` for years with this setting enabled.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D8232
Summary: we were calling a member method on a diffusion hash. not sure why. Fixes T4402
Test Plan: clicked about, no fatals and seemed to move sensical backwards in time
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4402
Differential Revision: https://secure.phabricator.com/D8194
Summary: ...by the surprising step of changing how this data is stored from id to phid. Also a small fix to not allow "disabled" rules to be used as herald rule conditions, i.e. can't make a rule that depends on a disabled rule.
Test Plan: viewed existing herald rule that had a rule condition and noted nice new display using handle. made a new rule that had a rule condition and verified it worked correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8186
Summary:
Ref T4387. By using `hg locate` to attempt to only list files in the given path
browsing diffusion is a bit faster. In a repo of about 600M it shaves a rough 100ms
off viewing the root of the project.
Test Plan: Looked around in diffusion and saw it showed everything including .files, which was nice
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4387
Differential Revision: https://secure.phabricator.com/D8163
Summary:
Ref T156. @vlada recently implemented filename search in Diffusion, this cleans up the UI a little bit:
- Instead of showing one search box with two different buttons, let the submit buttons appear to the right of the text boxes and separate the search modes.
- Clean up the results a little bit (don't show columns which don't exist).
Test Plan: {F107260}
Reviewers: vlada, btrahan, chad
Reviewed By: chad
CC: vlada, chad, aran
Maniphest Tasks: T156
Differential Revision: https://secure.phabricator.com/D8125
Summary:
Ref T156. Adds basic filename search support for Diffusion,
currently only for Git repositories.
This is preliminary, and it's up for discussion:
- is the UI in the right place;
- what should the search query syntax be (e.g. whether
to put `*`s in the beginning and end of it);
- how to best approach it for Mercurial and/or SVN;
- what's the cleanest result format for `lsquery` (I went
for the minimum necessary change to `DiffusionBrowseSearchController`).
Test Plan:
Browse to a repository in Diffusion, and use both
`Search File Names` and `Search File Content`.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T156
Differential Revision: https://secure.phabricator.com/D8093
Summary: Fixes T4175. In cases where the arguments have only always-safe characters, we can produce a more human-readable URI.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8100
Summary:
Ref T4175. This allows these URIs to all be valid for Git and Mercurial:
/diffusion/X/
/diffusion/X/anything.git
/diffusion/X/anything/
This mostly already works, it just needed a few tweaks.
Test Plan: Cloned git and hg working copies using HTTP and SSH.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8098
Summary:
Ref T4175.
- Add a configurable name for the clone-as directory, so you can have "Bits & Pieces" clone as "bits~n~pieces/" or simliar.
- By default, use "reasonable" heruistics to choose such a name.
- Generate a copy/pasteable clone commmand with this directory name.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8097
Summary:
Hosted repositories have muddied this distinction somewhat. In some cases, we only want to use the real remote URI, and the call is only relevant for imported repositories.
In other cases, we want the URI we'd plug into `git clone`.
Move this logic into `PhabricatorRepository` and make the distinction more clear.
Test Plan: Viewed SVN, Git, and Mercurial hosted and remote repositories, all the URIs looked reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D8096
Summary:
Fixes T1353. Also some minor unrelated cleanup:
- `openTransaction()` / `saveTransaction()` exist now, fix TODOs.
- Fix some instructions.
- Make `diffusion.branchquery` return empty for SVN rather than fataling.
Test Plan:
- Added a branches rule.
- Ran a dry run against commits in different VCSes.
{F105574}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, Nopik
Maniphest Tasks: T1353
Differential Revision: https://secure.phabricator.com/D8086
Summary:
Fixes T4344. `diffusion.getcommits` is nasty old bad news. Implement a modern query method.
This method provides limit/paging in a somewhat abstract way so it's sort of ultramodern, but I didn't want the default behavior to return a million rows. I'll probably move more stuff toward this over time, now that cursor paging is pervasive. Here, we needed extra metadata (the identifier map) anyway.
Test Plan: Used console to execute command.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4344
Differential Revision: https://secure.phabricator.com/D8077
Summary:
A few users have hit this and found it confusing. Currently, it means "more than 99.95%", which is very different from "100%". Instead:
- show an extra digit of precision; and
- cap the display at "99.99%", so it's more clear that work is still happening.
Test Plan: Faked it and saw it cap at 99.99%.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8058
Summary:
Fixes T4143. This mitigates the "use a botnet to slowly try to login to every user account using the passwords '1234', 'password', 'asdfasdf', ..." attack, like the one that hit GitHub.
(I also donated some money to Openwall as a thanks for compiling this wordlist.)
Test Plan:
- Tried to register with a weak password; registered with a strong password.
- Tried to set VCS password to a weak password; set VCS password to a strong password.
- Tried to change password to a weak password; changed password to a strong password.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, chad
Maniphest Tasks: T4143
Differential Revision: https://secure.phabricator.com/D8048
Summary: Minor, adds the Callsign and changes to cards view when listing repositories.
Test Plan: Reload sandbox list of repositories, see new items.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8036
Summary: Fixes T3238. Ref T4327. Although the instructions are fairly clear on this, it's easy to miss them. Make sure the root the user enters matches the real root.
Test Plan: Added unit tests. Used `bin/repository discover` to hit the check explicitly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3238, T4327
Differential Revision: https://secure.phabricator.com/D8020
This seems to be a specific of how browsers are dealing with
spaces/tabs. Multiple spaces works just fine, but multiple
tabs were treating as a single space which breaks indentation.
Now made it so tabs are replaced with 4 spaces. Not ideal but
still better than fully unreadable code. This also matches to
how differential is handling tabs.
Ref T2495. See: <https://github.com/facebook/phabricator/issues/487>
Reviewed by: epriestley
Summary:
Ref T4327. This is general cleanup since I was in this area of the code. Primarily, the Mercurial implementation here was completely broken and wrong:
- It returned only one branch, but a commit can be present on many branches.
- It did not account for multiple branch heads.
- It returned a result implying the branch head pointed at the queried commit, which is no consistent or accurate.
Simplify the amount of API we're dealing with by collapsing this method into the very similar `diffusion.branchquery` method.
Test Plan: Looked at mercurial and git repositories and commits, branch information seemed correct.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8003
Summary: Ref T4327. At some point these two very similar classes got introduced. Collapse `DiffusionBranchInformation` into the nearly identical `DiffusionRepositoryRef`, which enjoys slightly more generality and support.
Test Plan: Viewed branch overview and detail pages. Ran `repository refs` and `repository discover`. Grepped for removed symbols.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8002
Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.
In particular:
- As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
- Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
- We no longer need to do a separate discovery step on closable branches.
- We no longer need to keep track of `seenOnBranches`.
Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7985
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.
Test Plan: Test out the forms, see errors without the text.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran, hach-que
Differential Revision: https://secure.phabricator.com/D7924
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.
Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7914
Test Plan: Created comments with 'silent' both true and empty, received notifcation for only the latter.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7916
Summary:
Updates table design to use new standards, work well in PHUIObjectBox. Fixes T4142
Comma
Test Plan: Tested on Diffusion, Settings, will roll out to more places soon
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T4142
Differential Revision: https://secure.phabricator.com/D7901
Summary:
Fixes T4276. This adds "Change is enormous" to pre-commit content rules so we can, e.g., just reject these and not worry about them elsewhere.
Also, use the same numeric limits across the mechanisms so there's a consistent definition of an "enormous" changeset.
Test Plan:
- Set enormous limit to 15 bytes, pushed some changes, got blocked by a rule.
- Set it back, pushed OK.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7887
Summary:
Ref T4276. When a change is larger than 2GB, PHP can not read the entire change into a string, so Herald can not process it.
Additionally, we already have a time limit for practical reasons, but it's huge (probably incorrectly). To deal with these things:
- Add an optional byte limit to `diffusion.rawdiffquery`.
- Make the query with a 1GB limit.
- Reduce the diff timeout from 15 hours to 15 minutes.
- Add a "Changeset is enormous" field. This field is true for changes which are too large to process.
This generally makes behaviors more sane:
- We'll always make progress in Herald in a reasonable amount of time.
- Installs can write global rules to handle (or reject) these types of changes.
Test Plan: Set limit to 25 bytes instead of 1GB and ran test console on various changes.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4276
Differential Revision: https://secure.phabricator.com/D7885
Summary:
Fixes T4189. Ref T4151. Allows repositories to have additional custom hooks for operations which can't be expressed with Herald (one such operation is lint).
This adds only local hook directories, since they're easier to use with existing hooks than global directories. I might add global directories eventually.
This doesn't support Mercurial since we have no demand for it and it's more complicated (we lose compatibility and power by just dropping a `hooks.d/` somewhere).
Test Plan:
- Pulled hosted SVN and Git repos to verify the hook directories generate correctly.
- Added a variety of hooks to the hook directories (echo + pass, fail).
- Pushed commits and verified the hooks fired (output expected info, or failed).
- Verified push log reflected the correct error code ("3", external) and detail ("nope.sh") when rejecting.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4151, T4189
Differential Revision: https://secure.phabricator.com/D7884
Summary:
Fixes T4264. Adds:
- New "Repository's projects" field to Herald pre-commit rules, so you can write global rules which act based on projects.
- Allows pre-ref/pre-content rules to bind to projects, and fire for all repositories in that project, so users with limited power can write rules which apply to many repositories.
- The pre-ref and pre-content classes were starting to share a fair amount of code, so I made them both extend an abstract base class.
Test Plan: Wrote new pre-ref and pre-content rules bound to projects, then pushed commits into repositories in those projects and not in those projects. The "repository projects" field populated, and the rules fired for repositories in the relevant projects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7883
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary: Currently we markup `rXabcd`, but not `rX` on its own. Mark these up as repository object names.
Test Plan: Typed `rPOEMS`, `rPOEMS1`, `rPOEMS139893189`, etc.
Reviewers: btrahan, dctrwatson
Reviewed By: btrahan
CC: aran, poop
Differential Revision: https://secure.phabricator.com/D7859
Summary:
Fixes T4242. It's currently possible to set nonsense defaults and create repositories with unintended policies, because policy configuration isn't part of creation. Instead:
- put a policy page into the creation workflow;
- require the selection of valid policies (i.e., prevent creating a repository you can't view / edit).
Test Plan:
- Created imported and hosted repositories, hit policy selection.
- Edited policies of existing repositories.
- Tried to set nonsense policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4242
Differential Revision: https://secure.phabricator.com/D7856
Summary:
Ref T4264. Allows you to create "Object" rules, in addition to Global and Personal rules. If you choose to create an Object rule, you'll be prompted to select an object on a new screen. You must be able to edit and object in order to create rules for it.
Ref T3506. This makes "All" the default filter for the transcript view, which should reduce confusion on smaller installs.
Test Plan:
- Created non-object rules.
- Created object rules.
- Triggered object rules against matching and unmatching objects.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3506, T4264
Differential Revision: https://secure.phabricator.com/D7853
Summary: Ref T4264. Lays the groundwork for new "Object" rule types. Prevents personal "Hook" rules, which don't make any sense.
Test Plan: Created new Maniphest (global/personal available) and Ref Hook (global only) rules.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4264
Differential Revision: https://secure.phabricator.com/D7852
Summary: Some discussion on IRC. This is more consistent with other disabled items, which are click-to-explain.
Test Plan: Viewed UI, clicked link.
Reviewers: btrahan, dctrwatson, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7857
