Summary:
Ref T4292. We currently synchronize hosted, clustered, Git repositories when we receive an SSH pull or push.
Additionally:
- Synchronize before HTTP reads and writes.
- Synchronize reads before Conduit requests.
We could relax Conduit eventually and allow Diffusion to say "it's OK to give me stale data".
We could also redirect some set of these actions to just go to the up-to-date host instead of connecting to a random host and synchronizing it. However, this potentially won't work as well at scale: if you have a larger number of servers, it sends all of the traffic to the leader immediately following a write. That can cause "thundering herd" issues, and isn't efficient if replicas are in different geographical regions and the write just went to the east coast but most clients are on the west coast. In large-scale cases, it's better to go to the local replica, wait for an update, then serve traffic from it -- particularly given that writes are relatively rare. But we can finesse this later once things are solid.
Test Plan:
- Pushed and pulled a Git repository over HTTP.
- Browsed a Git repository from the web UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15758
Summary:
Ref T4292. Before we write or read a hosted, clustered Git repository over SSH, check if another version of the repository exists on another node that is more up-to-date.
If such a version does exist, fetch that version first. This allows reads and writes of any node to always act on the most up-to-date code.
Test Plan: Faked my way through this and got a fetch via `bin/repository update`; this is difficult to test locally and needs more work before we can put it in production.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15757
Summary:
Ref T4292. This consolidates code for figuring out which user we should connect to hosts with.
Also narrows a lock window.
Test Plan: Browsed Diffusion, pulled and pushed through an SSH proxy.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15754
Summary:
Ref T4292. Ref T10366. Depends on D15751. Today, generating repository commands is purely a function of the repository, so they use protocols and credentials based on the repository configuration.
For example, a repository with an SSH "remote URI" always generate SSH "remote commands".
This needs to change in the future:
- After T10366, repositories won't necessarily just have one type of remote URI. They can only have one at a time still, but the repository itself won't change based on which one is currently active.
- For T4292, I need to generate intracluster commands, regardless of repository configuration. These will have different protocols and credentials.
Prepare for these cases by separating out command construction, so they'll be able to generate commands in a more flexible way.
Test Plan:
- Added unit tests.
- Browsed diffusion.
- Ran `bin/phd debug pull` to pull a bunch of repos.
- Ran daemons.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292, T10366
Differential Revision: https://secure.phabricator.com/D15752
Summary: Ref T4292. This will let the UI and future `bin/repository` tools give administrators more tools to understand problems when reporting or resolving them.
Test Plan:
- Pushed fully clean repository.
- Pushed previously-pushed repository.
- Forced write to abort, inspected useful information in the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15748
Summary:
Ref T4292. Small fixes:
- There was a bug with the //first// write, where we'd write 1 but expect 0. Fix this.
- Narrow the window where we hold the `isWriting` lock: we don't need to wait for the client to finish.
- Release the lock even if something throws.
- Use a more useful variable name.
Test Plan:
- Made new writes to a fresh cluster repository.
- Made sequential writes.
- Made concurrent writes.
- Made good writes and bad writes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15747
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
Summary:
Fixes T10830. Ref T10366. I wasn't writing to this table yet so I didn't build it, but the fact that `bin/storage adjust` would complain slipped my mind.
- Add the table.
- Make the tests run `adjust`. This is a little slow (a few extra seconds) but we could eventually move some steps like this to run server-side only.
Test Plan: Ran `bin/storage upgrade -f`, got a clean `adjust`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10830
Differential Revision: https://secure.phabricator.com/D15744
Summary:
Ref T10748. This supports more transaction types in the modern editor and improves validation so Conduit benefits.
You can technically create repositories via `diffusion.repository.edit` now, although they aren't very useful.
Test Plan:
- Used `diffusion.repository.edit` to create and edit repositories.
- Used `/editpro/` to edit repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10748
Differential Revision: https://secure.phabricator.com/D15740
Summary: Ref T10748. Ref T10366. No support for editing and no impact on the UI, but get some of the basics in place.
Test Plan: {F1223279}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10748
Differential Revision: https://secure.phabricator.com/D15742
Summary: Ref T10748. Ref T10337. This technically implements this stuff, but it does not do anything useful yet. This skips all the hard stuff.
Test Plan:
- Technically used `diffusion.repository.search` to get repository information.
- Technically used `diffusion.repository.edit` to change a repository name.
- Used `editpro/` to edit a repository name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10337, T10748
Differential Revision: https://secure.phabricator.com/D15736
Summary:
This nearly works but I didn't have time to get back to it and it isn't stable enough to turn on in the cluster yet.
We have enough other stuff going out this week, so just disable it before `stable` gets cut. Should be ready by next week if things go well.
Test Plan: Fetched a Git SSH repo locally.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15731
Summary:
Ref T2783. This allows this worker to run on a machine different to the one that stores the repository, by routing the execution of Git over Conduit calls.
This API method is super gross, but fixing it isn't straightforward and it runs into other complicated considerations. We can fix it later; for now, just define it as "internal" to limit how much mess this creates.
"Internal" methods do not appear on the console.
Test Plan: Ran `bin/repository reparse --change <commit> --trace` on several commits, saw daemons make a Conduit call instead of running a `git` command.
Reviewers: hach-que, chad
Reviewed By: chad
Subscribers: joshuaspence, Korvin, epriestley
Maniphest Tasks: T2783
Differential Revision: https://secure.phabricator.com/D11874
Summary: Fixes T10789. If we aren't configured with a device, we never grabbed a lock in the first place, and should not expect one to be held.
Test Plan: Pushed non-cluster-configured Git SSH repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10789
Differential Revision: https://secure.phabricator.com/D15692
Summary:
Ref T4292. This mostly implements the locking/versioning logic for multi-master repositories. It is only active on Git SSH pathways, and doesn't actually do anything useful yet: it just does bookkeeping so far.
When we read (e.g., `git fetch`) the logic goes like this:
- Get the read lock (unique to device + repository).
- Read all the versions of the repository on every other device.
- If any node has a newer version:
- Fetch the newer version.
- Increment our version to be the same as the version we fetched.
- Release the read lock.
- Actually do the fetch.
This makes sure that any time you do a read, you always read the most recently acknowledged write. You may have to wait for an internal fetch to happen (this isn't actually implemented yet) but the operation will always work like you expect it to.
When we write (e.g., `git push`) the logic goes like this:
- Get the write lock (unique to the repository).
- Do all the read steps so we're up to date.
- Mark a write pending.
- Do the actual write.
- Bump our version and mark our write finished.
- Release the write lock.
This allows you to write to any replica. Again, you might have to wait for a fetch first, but everything will work like you expect.
There's one notable failure mode here: if the network connection between the repository node and the database fails during the write, the write lock might be released even though a write is ongoing.
The "isWriting" column protects against that, by staying locked if we lose our connection to the database. This will currently "freeze" the repository (prevent any new writes) until an administrator can sort things out, since it'd dangerous to continue doing writes (we may lose data).
(Since we won't actually acknowledge the write, I think, we could probably smooth this out a bit and make it self-healing //most// of the time: basically, have the broken node rewind itself by updating from another good node. But that's a little more complex.)
Test Plan:
- Pushed changes to a cluster-mode repository.
- Viewed web interface, saw "writing" flag and version changes.
- Pulled changes.
- Faked various failures, got sensible states.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15688
Summary:
Ref T4292. This adds some very basic cluster/device data to the new management view. Nothing interesting yet.
Also deal with disabled bindings a little more cleanly.
Test Plan: {F1214619}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4292
Differential Revision: https://secure.phabricator.com/D15685
Summary:
Ref T10756. When repositories are properly configured for the cluster (which is hard to set up today), be smart about which repositories are expected to exist on the current host, and only pull them.
This generally allows daemons to pretty much do the right thing no matter how many copies are running, although there may still be some lock contention issues that need to be sorted out.
Test Plan: {F1214483}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10756
Differential Revision: https://secure.phabricator.com/D15682
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary: Ref T9456. Some rough edges and we can't complete the build yet since I haven't written a webhook, but this mostly seems to be working.
Test Plan:
- Ran this build on some stuff.
- Ran a normal HTTP step build to make sure I didn't break that.
{F880301}
{F880302}
{F880303}
Reviewers: chad
Reviewed By: chad
Subscribers: JustinTulloss, joshma
Maniphest Tasks: T9456
Differential Revision: https://secure.phabricator.com/D14286
Summary: Ref T7789. Make sure these get cleaned up when a repository is destroyed.
Test Plan:
- Created a new repository.
- Pushed some LFS data to it.
- Used `bin/remove destroy` to nuke it.
- Verified the LFS stuff was cleaned up and the underlying files were destroyed (`SELECT * FROM repository_gitlfsref`, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15493
Summary:
Ref T7789. This implements:
- A new table to store the `<objectHash, filePHID>` relationship between Git LFS files and Phabricator file objects.
- A basic response to `batch` commands, which return actions for a list of files.
Test Plan:
Ran `git lfs push origin master`, got a little further than previously:
```
epriestley@orbital ~/dev/scratch/poemslocal $ git lfs push origin master
Git LFS: (2 of 1 files) 174.24 KB / 87.12 KB
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
Git LFS operation "upload/b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69" is not supported by this server.
```
With `GIT_TRACE=1`, this shows the batch part of the API going through.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15489
Summary:
Ref T7789. This implements a (probably) usable "git-lfs-authenticate" on top of the new temporary token infrastructure.
This won't actually do anything yet, since nothing reads the tokens.
Test Plan:
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate'
phabricator-ssh-exec: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate x'
phabricator-ssh-exec: Unrecognized repository path "x". Expected a path like "/diffusion/X/" or "/diffusion/123/".
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22'
Exception: Expected `git-lfs-authenticate <path> <operation>`, but received too few arguments.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 y'
Exception: Git LFS operation "y" is not supported by this server.
```
```
$ ./bin/ssh-exec --phabricator-ssh-user admin --ssh-command 'git-lfs-authenticate diffusion/22 upload'
{"header":{"Authorization":"Basic QGdpdC1sZnM6NmR2bDVreWVsaXNuMmtnNXBtbnZwM3VlaWhubmI1bmI="},"href":"http:\/\/local.phacility.com\/diffusion\/22\/new-callsign-free-repository.git\/info\/lfs"}
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D15482
Summary:
Ref T10560. Reverts D15460. See that task for discussion: we dug up some more information to explain the behavior, and this key was just sort of sidestepping an analyze/cardinality estimate issue on the index.
With proper cardinality estimates it shouldn't be used, so just nuke it.
Test Plan: Ran `bin/storage adjust`, saw key drop.
Reviewers: eadler, chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15486
Summary:
Ref T10560. I don't fully understand what MySQL is doing here, but it looks like this key improves the problematic dataset in practice.
(It makes sense that this key helps, I'm just not sure why the two separate keys and the UNION ALL are so bad.)
This key isn't hugely expensive to add, so we can try it and see if there are still issues.
Test Plan: Ran `bin/storage adjust`, saw key added to table. Used `SHOW CREATE TABLE ...` to verify the key exists. Used `EXPLAIN SELECT ...` to make sure MySQL actually uses it.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10560
Differential Revision: https://secure.phabricator.com/D15460
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.
I don't anticipate needing this in the future.
Test Plan: Grepped for this method.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15409
Summary:
Ref T10457. This makes diffs/revisions show the revision as the buildable title, and commits show the commit as the title.
Previously, the title was "Buildable X".
Also makes icons/colors/labels more consitent.
Test Plan: {F1131885}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10457
Differential Revision: https://secure.phabricator.com/D15355
Summary:
Ref T10449. Currently, we store classes (like "AlmanacClusterRepositoryServiceType") in the database.
Instead, store types (like "cluster.repository").
This is a small change, but types are a little more flexible (they let us freely reanme classes), a little cleaner (fewer magic strings in the codebase), and a little better for API usage (they're more human readable).
Make this minor usability change now, before we unprototype.
Also make services searchable by type.
Also remove old Almanac API endpoints.
Test Plan:
- Ran migration, verified all data migrated properly.
- Created, edited, rebound, and changed properties of services.
- Searched for services by service type.
- Reviewed available Conduit methods.
Reviewers: chad
Reviewed By: chad
Subscribers: yelirekim
Maniphest Tasks: T10449
Differential Revision: https://secure.phabricator.com/D15346
Summary: Fixes T10432. I missed these in making properties non-default.
Test Plan: Diffusion now works again in a cluster configuration.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10432
Differential Revision: https://secure.phabricator.com/D15337
Summary:
Ref T4245. This could still use a little UI smoothing, but:
- Don't require a callsign on the create flow (you can add one later in "Edit Basic Information" if you want).
- Allow existing callsigns to be removed.
Test Plan:
- Created a new repository with no callsign.
- Cloned it; pushed to it.
- Browsed around Diffusion a bunch.
- Visited a commit URI.
- Added a callsign to it.
- Removed the callsign again.
- Referenced it with `R22` in remarkup.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15305
Summary:
Ref T4245. This is a prelude to removing them from the "create" screen.
Currently, if you try to delete the callsign you get an unceremonious database error, but the next diff (or maybe two) will permit that, so I didn't put any "this is required yada yada" text in.
This could also maybe use some big flashing warning lights and a "if you edit this, all your URIs break" but I'll save that for later.
Test Plan: Changed the callsign for a repository.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15304
Summary: Ref T4245. When creating new repositories, set a default local path based on the repository ID instead of callsign.
Test Plan:
- Created a new repository.
- Saw it get a reasonable, ID-based local path.
- Edited a repository to make sure the `applyFinalEffects()` wasn't doing anything whacky.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15303
Summary: Ref T4245. Consolidates the URI parsing/rewriting logic so that repositories can be served from either `/diffusion/XYZ/` or `/diffusion/123/`, over both HTTP and SSH.
Test Plan:
- Pulled a Git repository by ID and callsign over HTTP and SSH.
- Pulled a Mercurial repository by ID and callsign over HTTP and SSH.
- Pulled a Subversion repository by ID and callsign over SSH (no HTTP support for SVN).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15302
Summary:
Ref T4245. Make `/diffusion/123/` work, but redirect the user to `/diffusion/XYZ/` if the repository has a callsign.
(Right now, every repository has a callsign, so this always redirects.)
Also redirect `/R123:abcdef` if the repository has a callsign.
Also also, move the Pull garbage collector somewhere more sensible.
Test Plan:
- Added test coverage.
- Visited `/diffusion/1/`, was redirected.
- Visited `/diffusion/R1:abcdef`, was redirected.
- Browsed Diffusion normally.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15301
Summary:
Ref T4245. Two effects:
- First, let hooks work for future repositories without callsigns.
- Second, provide a better error when users push directly to hosted repositories.
Test Plan: Ran `bin/commit-hook PHID-REPO-xxx`.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D15293
Summary: Fixes T10259. There was no real reason to do this `ip2long()` stuff in the first place -- it's very slightly smaller, but won't work with ipv6 and the savings are miniscule.
Test Plan:
- Ran migration.
- Viewed logs in web UI.
- Pulled and pushed.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10259
Differential Revision: https://secure.phabricator.com/D15165
Summary:
Ref T10228. This is currently quite limited:
- No UI.
- No SSH support.
My primary goal is to debug the issue in T10228. In the long run we can expand this to be a bit fancier.
Test Plan:
Made various valid and invalid clones, got sucess responses and not-so-successful responses, viewed the log table for general corresponding messages and broad sanity.
Ran GC via `bin/phd debug trigger`, no issues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10228
Differential Revision: https://secure.phabricator.com/D15127
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.
Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.
Test Plan:
- Watched a project I was not a member of.
- Clicked the feed watch/unwatch button.
{F1064909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6183, T10054
Differential Revision: https://secure.phabricator.com/D15063
Summary: Ref T4245. Fixes T10172. These regular expressions were simply incorrect: they intend `<start> (form one | form two) <end>` but were written as `(<start> form one) | (form two <end>)` which allowed stuff like "R2/R13" to be interpreted as a monogram because it matches `(<start> form one)`.
Test Plan: Parsed commit `ba46ffa6169c` from RTEMS repository, see T10172. Before patch, got an identical trace; after patch, clean import.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T4245, T10172
Differential Revision: https://secure.phabricator.com/D15049
Summary:
Ref T4705 (there are also some other adjacent related tasks dealing with URIs).
Currently, we issue a "get repositories matching URIs: ..." query by loading every possible repository and then checking their URIs in PHP.
Instead, put URIs in a separate table. I plan for each repository to potentially have multiple URIs soon, so this prepares for that.
Test Plan:
- Ran migrations.
- Looked at index table, made sure it appeared sensible.
- Ran some queries by `uri` to find repositories, found the repositories I expected.
- Updated the remote URI of a repository, saw queries / index update appropriately.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4705
Differential Revision: https://secure.phabricator.com/D15005
Summary: Fixes T8826. Git tracks an "author date", which may be different from the "committed date". We don't currently extract/show this; do so.
Test Plan: {F1059235}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8826
Differential Revision: https://secure.phabricator.com/D14995
Summary: Ref T4245. Full commit display names (like `rPaaaa`) are going to be obnoxious soon in some cases (e.g., `rPaaaa` becomes `R123:aaaa`, which is much uglier) so reduce how often we show the repository in cases where it isn't really necessary to include it.
Test Plan:
- Saw no more `rX` on repository list view for Git/Mercurial (still present for Subversion).
- Saw no more `rX` on various repository detail views, except when referencing other commits (e.g., mentions).
- Grepped for removed `getShortName()`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14990
Summary:
Ref T4245.
- Rename "Clone/Checkout As" to "Short Name" in the UI.
- Allow any repository to have a short name, not just hosted repositories.
Test Plan:
- Ran migration.
- Reviewed old transactions, saw they looked good.
- Edited an existing repository's short name.
- Gave an imported repository a new short name.
- Removed a repository's short name.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14989
Summary:
Ref T4245. This allows `bin/repository update bread` to work, in addition to `rBREAD`, `R123`, `123`, `BREAD`, etc., if a repository has a short name set.
This primarily affects CLI commands (like `bin/repository`) and Conduit API calls. It has no normal user-facing impact.
Test Plan: Ran `bin/repository update bread` and such.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14988
Summary:
Fixes T7938.
- Primarily, users can currently shoot themselves in the foot by putting `../../etc/passwd` and other similar nonsense in these fields (this is not dangerous, but also does not work). Require sensible names.
- Enforce uniqueness so these names can be used in URIs and as identifiers in the future.
- (This doesn't start actually using them for anything fancy yet.)
Test Plan:
- Gave several repositories clone names: a valid name, two duplicate names, an invalid, name, some with no names.
- Ran migrations.
- Got clean conversion for valid names, appropriate errors for invalid/duplicate names.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7938
Differential Revision: https://secure.phabricator.com/D14986
Summary:
Ref T9319. See D14967. As before, this is making a deeply-buried, complex operation easier to test by providing a CLI command.
This adds `bin/differential attach-commit rXnnnn Dnnnn` to pretend that `rXnnnn` was just committed and matched `Dnnnn`.
Test Plan:
- Ran `bin/differential attach-commit X Y` for several different values, saw updates in the UI.
- Faked the message parser to make sure stuff still worked there.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14968
Summary: Fixes T9701. I don't want to try to autofix this because destroying the directory could destroy important files, but we can improve the error message.
Test Plan: Faked a failure, ran `repository update X`, got a more tailored error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9701
Differential Revision: https://secure.phabricator.com/D14971
Summary:
Ref T9319. When we discover a commit, we sometimes update the corresponding revision with a "this is the actual committed change" diff and send out a link to the changes between review and commit.
This is currently very difficult to test, because it only happens the first time and you have to either go set up a bunch of objects or add a bunch of special casing to the parser to hit the workflow.
I'm making some changes to how it pulls file content. To make those changes easier to test, first start extracting this stuff so the code can be run with `bin/differential extract ...` instead of needing to do a bunch of more complicated setup steps.
Test Plan:
- Ran `bin/differential extract ...` to extract diffs from commits.
- Forced my way through the daemon workflow by faking out a bunch of flags, got a clean extract + attach + update. After this patch, this should rarely be necessary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9319
Differential Revision: https://secure.phabricator.com/D14967
Summary:
Fixes T4366. Two years ago, Facebook put 16,000 files in a directory. Today, the page has nearly loaded.
Paginate large directories.
Test Plan:
- Viewed home and browse views in Git, Mercurial and Subversion.
I put an artificially small page size (5) on home:
{F1055653}
I pushed 16,000 files to a directory and paged through them. Here's the last page, which rendered in about 200ms:
{F1055655}
Our behavior is a bit better than GitHub here, which shows only the first 1,000 files, disables pagination, and can't retrieve history for the files:
{F1055656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4366
Differential Revision: https://secure.phabricator.com/D14956
Summary:
Ref T2450. Ref T9319. This is still a bit messy, but not quite so bad as it was: instead of using a single call to get both blame information and file content, use `diffusion.blame` for blame information.
This will make optimizations to both blame and file content easier.
Test Plan: Viewed a bunch of blame (color on/off, blame on/off).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2450, T9319
Differential Revision: https://secure.phabricator.com/D14958
Summary: Fixes T9191. This is pretty fluff but doesn't hurt anything, I guess.
Test Plan: Viewed repository list, saw an importing repository get a little icon.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9191
Differential Revision: https://secure.phabricator.com/D14950
Summary:
Ref T4245. This adds support for both ID-based and callsign-based routes, although the ID-based routes don't occur anywhere.
Also moves toward simplifying the DiffusionRequest stuff.
Test Plan: Visited normal callsign-based commit pages; visited new ID-based commit pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14940
Summary: Ref T4245. This further reduces the reliance on callsigns in Diffusion.
Test Plan:
- Pretty reasonable test coverage already exists.
- Browsed repository list, browse view, history view, content view, change view, commit view, tag view, branch view of repositories.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14937
Summary: Ref T4245. These mostly relate to building URIs.
Test Plan: Tried to hunt down as many of these in the UI as I could. Some are a bit tricky but they should be low-risk.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14933
Summary:
Ref T4245. This was obsoleted long ago and has no callers in Phabricator or Arcanist.
Also some minor cleanup.
Test Plan: `grep` for callers everywhere.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14930
Summary: Ref T4245. More of the same, just narrowing down the easy cases.
Test Plan:
- Called `diffusion.querycommit`.
- Browsed branches.
- Browsed repository.
- Browsed directory.
- Searched for stuff.
- Viewed a commit.
- Viewed a file diff.
- Edited a commit.
- Viewed history.
- Viewed tags.
- Viewed push log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14929
Summary: Ref T4245. These are all descriptive or UI-facing.
Test Plan:
- Ran `bin/repository pull ...` with various identifiers.
- Ran `bin/repository mirror ...` with various identifiers.
- Ran `bin/repository discover ...` with various identifiers.
- Ran `bin/phd debug pull X Y --not Z` with various identifiers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14926
Summary:
Ref T4245. Prepare these scripts for a callsign-free world. This also makes them more flexible and easier to use.
The following are now valid ways to identify a repository for these scripts: ID (`3`), PHID (`PHID-REPO-wxyz`), R<ID> (`R3`), r<CALLSIGN> (`rSKYNET`), CALLSIGN (`SKYNET`).
In the future, a human-readable label (`skynet`) may also become valid.
Test Plan:
- Ran `bin/repository reparse --all ...` with `rX`, `X`, `3`, `R3`.
- Ran `bin/repository reparse --change ...` with `rXaaa`, including short versions.
- Ran `bin/repository update ...` with `rX`, `X`, `3`, `R3`.
- Ran `bin/repository refs ...` with various identifiers.
- Ran `bin/repository pull ...` with various identifiers.
- Ran `bin/repository mirror ...` with various identifiers.
- Ran `bin/repository mark-imported ...` with various identifiers.
- Ran `bin/repository list`.
- Ran `bin/repository importing ...` with various identifiers and examined output.
- Ran `bin/repository edit ...` with various identifiers.
- Ran `bin/repository discover ...` with various identifiers.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14924
Summary: Ref T4245. Before doing any hard work here, we can dramatically reduce the number of things that make calls to `getCallsign()` to make navigating things easier. Almost all of them only care about a monogram, URI, or display name.
Test Plan:
- Searched for `r uniquename` in jump nav.
- Ran `bin/repository reparse --change rXXXyyyyy --trace`, observed query against bad commit table.
- Ran `bin/search index rXXXyyyy --trace --force`, observed proper title when indexing commit.
- Browed repository list, saw proper `rXXX` and appropriate link targets.
- Mentioned `rXXX` in Remarkup, got a link to the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4245
Differential Revision: https://secure.phabricator.com/D14923
Summary: Ref T9979. This simplifies/standardizes the code a bit, but mostly gives us more consistent class names and structure.
Test Plan:
- Used `bin/search index --type ...` to index documents of every indexable type.
- Searched for documents by unique text, found them.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14842
Summary:
Ref T9979. This is currently hard-coded but can be done in a generic way.
This has one minor behavioral changes: answer text is no longer included in the question text index in Ponder. I'm not planning to accommodate that for now since I don't want to dig this hole any deeper than I already have. This behavior should be different anyway (e.g., index the answer, then show the question in the results or something).
Test Plan:
- Put a unique word in a Maniphest comment.
- Searched for the word.
- Found the task.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14837
Summary:
Ref T9890. Ref T9979. Several adjacent goals:
- The `SearchEngine` vs `ApplicationSearchEngine` thing is really confusing. There are also a bunch of confusing class names and class relationships within the fulltext indexing. I want to rename these classes to be more standard (`IndexEngine`, `IndexEngineExtension`, etc). Rename `SearchIndexer` to `IndexEngine`. A future change will rename `SearchEngine`.
- Add the index locks described in T9890.
- Structure things a little more normally so future diffs can do the "EngineExtension" thing more cleanly.
Test Plan:
Indexing:
- Renamed a task to have a unique word in the title.
- Ran `bin/search index Txxx`.
- Searched for unique word.
- Found task.
Locking:
- Added a `sleep(10)` after the `lock()` call.
- Ran `bin/search index Txxx` in two windows.
- Saw first one lock, sleep 10 seconds, index.
- Saw second one give up temporarily after failing to grab the lock.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9890, T9979
Differential Revision: https://secure.phabricator.com/D14834
Summary: Adds basic NUX to Dashboards, Herald, Repositories, Maniphest. Note Herald and Dashboard Panels don't fine the nux for some reason, assume they will when modernized?
Test Plan: Read text, click buttons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14844
Summary: Fixes T9966. In this unusual, difficult-to-reach case, we throw `Exception` (which has no censoring) instead of `CommandException` (which has censoring). Throw `CommandException` instead.
Test Plan:
- Hacked up a bunch of stuff in order to hit this: disabled origin validation, origin correction, and pointed repository at a bad domain.
- Verified message is now censored correctly.
{F1022217}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9966
Differential Revision: https://secure.phabricator.com/D14745
Summary:
Ref T9952. This adds a typeahead so you can pick a branch to target.
It does not choose a default branch, the user must pick a branch explicitly.
Test Plan:
- Landed rGITTESTd587fada48fc to `master` (by typing "master").
- Landed rGITTEST86c339b2ef01 to `notmaster` (by typing "notmaster").
{F1020531}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14733
Summary: Ref T9952. This will let me put a "Branch: [____]" control on the "Land Revision" dialog so users can choose a branch to target.
Test Plan: Used `/typeahead/class/` to vet basic behavior.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14732
Summary: Ref T9952. See discussion there. This change is primarily aimed at letting me build a typeahead of branches in a repository so that we can land to arbitrary branches a few diffs from now.
Test Plan:
- Ran migrations.
- Verified database populated properly with PHIDs (`SELECT * FROM repository_refcursor;`).
- Ran `bin/repository update`.
- Viewed a Git repository in Diffusion.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9952
Differential Revision: https://secure.phabricator.com/D14731
Summary: Fixes T9941. I think someone from Perforce emailed us about 10 years ago and I added this link in response, but I haven't seen other interest in Perforce since then. Link is now dead.
Test Plan:
- {nav Diffusion > Create Repository > Import Existing}, no more Perforce link.
- Grepped for `perforce`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9941
Differential Revision: https://secure.phabricator.com/D14720
Summary:
This diff adds a new mode argument to the Diffusion Conduit API with two options:
- "overwrite": the default, maintains the current behavior of deleting all coverage
in the specified branch before uploading the new coverage
- "update": does not delete old coverage, but will overwrite previous
coverage information if it's for the same file and commit
`DiffusionRequest::loadCoverage` already loads a file's coverage from the
latest available commit, so uploading coverage for different files in different
commits with "update" will result in seeing the latest uploaded coverage in
Diffusion.
Test Plan: manual local verification
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D14428
Summary:
Exposes the serve-over-http and serve-over-ssh options for a repository
to the `bin/repository edit` endpoint.
Test Plan: Ran `bin/repository` with the new options over several hundred repos
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, chasemp, 20after4, epriestley
Differential Revision: https://secure.phabricator.com/D14250
Summary:
Ref T182. This doesn't do anything interesting yet and is mostly scaffolding, but here's roughly the workflow. From previous revision, you can configure "Repository Automation" for a repository:
{F875741}
If it's configured, a new "Land Revision" button shows up:
{F875743}
Once you click it you get a big warning dialog that it won't work, and then this shows up at the top of the revision (completely temporary/placeholder UI, some day a nice progress bar or whatever):
{F875747}
If you're lucky, the operation eventually sort of works:
{F875750}
It only runs `git show` right now, doesn't actually do any writes or anything.
Test Plan:
- Clicked "Land Revision".
- Watched `phd debug task`.
- Saw it log `git show` to output.
- Verified operation success in UI (by fiddling URL, no way to get there normally yet).
Reviewers: chad
Reviewed By: chad
Subscribers: revi
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14266
Summary:
Ref T182. This allows you to assign blueprints that a repository can use to perform working copy operations. Eventually, this will support "merge this" in Differential, etc.
This is just UI for now, with no material effects.
Most of this diff is just taking logic that was in the existing "Blueprints" CustomField and putting it in more general places so Diffusion (which does not use CustomFields) can also access it.
Test Plan:
- Configured repository automation for a repository.
- Removed repository automation for a repository.
Reviewers: chad
Reviewed By: chad
Subscribers: avivey
Maniphest Tasks: T182
Differential Revision: https://secure.phabricator.com/D14259
Summary:
In Mercurial 3.2 the `locate` command was deprecated in favor of `files` command. This change updates the DiffusionLowLevelMercurialPathsQuery command to conditionally use `locate` or `files` based on the version of Mercurial used.
Closes T7375
Test Plan:
My test/develop Phabricator instance is setup to run Mercurial 3.5.1.
The test procedure to verify valid file listings are being returned:
1. I navigated to `http://192.168.0.133/conduit/method/diffusion.querypaths/`
2. I populated the following fields:
- path: `"/"`
- commit: `"d721d5b57fc9ef72e47ff9d4e0c583d74a46590c"`
- callsign: `"HGTEST"`
3. I submitted request and verified that result contained all files in the repository:
```
{
"0": "README",
"1": "alpha/beta/trifle",
"2": "test/Chupacabra.cow",
"3": "test/socket.ks"
}
```
I repeated the above steps after setting up Mercurial 2.6.2, which I installed in the following manner:
1. I downloaded Mercurial 2.6.2 source and run `make local` which will only compile it to work from its own directory (`/opt/mercurial-2.6.2`)
2. I linked `/usr/local/bin/hg -> /opt/mercurial-2.6.2/hg` (there's also a `/usr/bin/hg` which is a link to `/usr/local/bin/hg`)
3. I navigated to my home directory and verify that `hg --version` returns 2.6.2.
4. I restarted phabricator services (probably unnecessary).
With the Multimeter application active
1. I verified that `/usr/local/bin/hg` referred to version 2.6
2. I ran the same conduit call from the conduit application
3. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg locate`.
4. I swapped out mercurial versions for 3.5.1
5. I ran the same conduit call from the conduit application
6. I verified that `http://192.168.0.133/multimeter/?type=2&group=label` incremented values for `bin.hg files`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T7375
Differential Revision: https://secure.phabricator.com/D14253
Summary:
Fixes T8728. As far as I can tell, I simply got this wrong in D11826. This is not the proper name for the preference.
That change primarily focused on the "spammy junk during import" issue, and the code did get the importing flag right. It looks like my testing in D11827 focused on "during import" and just missed this case.
Test Plan: Grepped for `disable-herald`. Grepped for `herald-disable`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8728
Differential Revision: https://secure.phabricator.com/D14181
Summary: Ref T9252. This is still crude in a few ways but basically works, at least for commits.
Test Plan:
- Made a build plan with just this build step.
- Ran `bin/harbormaster build --plan 10 ...` on a commit.
- It actually built a working copy, leased it, took no action, and released the lease. MAGIC~~~
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9252
Differential Revision: https://secure.phabricator.com/D14160
Summary:
Ref T9458. This is basically the same as D13319, but the "Tags" field didn't get covered in that change.
Specifically, the issue is:
- We try to generate mail to a disabled user (later, we'll drop it without delivering it, but that filtering doesn't happen yet).
- The disabled user doesn't have permission to use Conduit (or any other Conduit-related problem occurs).
- We fail here, then retry generating the mail again later.
Instead, just degrade to not building the field and showing what went wrong.
Test Plan:
- Pushed some commits, saw mail generate.
- Added a fake exception to the field, saw the mail generate with an error message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9458
Differential Revision: https://secure.phabricator.com/D14142
Summary: As described in T7959, it looks like Diffusion does not provide Mercurial the required HTTP credentials when pulling from an external repository.
Test Plan: Add an external Mercurial repository to Diffusion, that requires HTTP authentication. A private BitBucket repository for example.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Projects: #mercurial, #diffusion
Differential Revision: https://secure.phabricator.com/D14092
Summary:
Fixes T9279. Modernizes the SearchEngine and Query classes. User-facing changes:
- Added order by commit date, default to order by commit date with newest commits first.
- Added explicit "Needs Audit by".
- Added new `packages(...)` typeahead function.
- Picked up automatic subscribers, projects, and order fields.
This changes behavior a little bit: we previously attempted to exclude, e.g., commits which a package you own needs to audit, but which you have resigned from. This is difficult in general and I think it needs a more comprehensive solution. This shouldn't impact users much, anyway.
Test Plan: {F767628}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9279
Differential Revision: https://secure.phabricator.com/D14013
Summary:
Fixes T7370. Two changes:
- Make the default to show nothing, instead of showing all the data. This is a better default because the data is sometimes sensitive. Workers should have to opt in to revealing it.
- For TargetWorkers, link to the target (technically the build, for now, since there's no dedicated target detail page).
Test Plan: {F698325}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7370
Differential Revision: https://secure.phabricator.com/D13845
Summary: Ref T7149. These formalize the local path adjustment step for cluster imports, rather than relying on an ad-hoc script.
Test Plan: Used `list-paths` and `move-paths` to list and move repositories.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7149
Differential Revision: https://secure.phabricator.com/D13621
Summary: Fixes T7604. This is the big scary change which drops the "arcanist project" fields from the database permanently.
Test Plan:
`grep`ped for the following to ensure that I had found all remaining references:
- `/arcanistProject/i`
- `/arcanist_project/i`
- `/projectName/i`
- `/project_name/i`
- `/project_id/i`
WARNING: Wait at least one month before landing this.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D12899
Summary: Return `$this` from setter methods for consistency. I started writing a [[https://secure.phabricator.com/differential/diff/32506/ | linter rule]] to detect this, but I don't think it is trivial to do this properly.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D13422
Summary: Ref T8099, functionally I prefer to be able to set anything 'table-like' with `setTable` for design consistency. This looses the restriction and did some light grepping for other missed cases.
Test Plan: Test new UI, grep for other missing cases.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13471
Summary: Fixes T8698. This isn't actually a table.
Test Plan: Viewed push log, no more fatal.
Reviewers: chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8698
Differential Revision: https://secure.phabricator.com/D13468
Summary:
Ref T8493. Diffusion is probably the strongest upstream use case we have for Spaces right now, so I want to get us on it to kick the tires a bit.
Small amount of hackiness around the multi-page form thing but it shouldn't create any problems.
Test Plan:
- Created a new repo.
- Edited a repo.
- Tried invalid edits, saw value preserved.
- Viewed edit full detail screen, saw space info.
- Viewed repo detail view, saw space.
- Viewed repo list view, saw space.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8493
Differential Revision: https://secure.phabricator.com/D13414
Summary: Fixes T6839. Sometimes, worker tasks go astray for whatever reason. This automates the step of `bin/repository importing | xargs | mangle mangle | bin/repostiory reparse`.
Test Plan: Ran various flavors of the command, got good looking results.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6839
Differential Revision: https://secure.phabricator.com/D13362
Summary: Ref T8099. This adds a new class which all search engines return for layout. I thought about this a number of ways, and I think this is the cleanest path. Each Engine can return whatever UI bits they needs, and AppSearch or Dashboard picks and lays the bits out as needed. In the AppSearch case, interfaces like Notifications, Calendar, Legalpad all need more custom layouts. I think this also leaves a resonable path forward for NUX as well. Also, not sure I implemented the class correctly, but assume thats easy to fix?
Test Plan: Review and do a search in each application changed. Grep for all call sites.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T8099
Differential Revision: https://secure.phabricator.com/D13332
