1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-11 08:06:13 +01:00
Commit graph

1288 commits

Author SHA1 Message Date
epriestley
f424f9f2d2 Record more details about where a write is taking place while holding a cluster lock
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
2016-04-18 11:55:27 -07:00
epriestley
368d2d1ddb Improve robustness of cluster version bookkeeping
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
2016-04-18 11:54:59 -07:00
epriestley
fbfe730452 Support more transactions types in RepositoryEditEngine
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
2016-04-17 16:27:02 -07:00
epriestley
92c50de8aa Rough in the new custom URI panel
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
2016-04-17 16:03:24 -07:00
epriestley
51838f990f Copy repository status to a management panel
Summary: Ref T10748. Pretty straightforward. I'd like to put a little "!" icon in the menu if there's a warning/error eventually, but can deal with that latre.

Test Plan: {F1223096}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10748

Differential Revision: https://secure.phabricator.com/D15741
2016-04-17 16:03:03 -07:00
epriestley
e582e9172b Rough in basics + policies + history repository management panels
Summary:
Ref T10748. This is roughly where I'm headed, if it makes some kind of sense? The "Edit" links in sub-sections don't work yet since I haven't built the thing.

Probably depends on D15736.

Test Plan: Manually navigated to `/manage/`, clicked around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10748

Differential Revision: https://secure.phabricator.com/D15737
2016-04-17 16:02:35 -07:00
epriestley
adf42db5ea Trivially implement RepositoryEditEngine and API methods
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
2016-04-17 16:02:13 -07:00
epriestley
0534002894 Add coverage tooltips in Diffusion file browse mode
Summary: Fixes T10816. The way these work is a little unusual since these chunks of file-rendering code are unusuall performance-sensitive, so the Differential version doesn't adapt directly to Diffusion. Both can possibly be unified at some point in the future, although they do slightly different things.

Test Plan: {F1220170}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10816

Differential Revision: https://secure.phabricator.com/D15719
2016-04-15 06:59:38 -07:00
epriestley
ca6da4c2be When proxying the SVN protocol, don't mutate URIs in protocol frames if we're an intracluster proxy
Summary:
Ref T10809. Currently, both the proxy and target may mutate URIs (rewriting "svn+ssh://x/diffusion/Y/" to a path on disk).

I believe this previously worked by fate/chance/luck since both URI variants contain the repository information, but the algorithms were tightened up recently with callsign removal.

Stop rewriting them if we're the intracluster proxy -- they only need to be rewritten on the target host.

Test Plan:
  - Checked out a proxied SVN repository, with and without a callsign.
  - Checked out an unproxied SVN repository, with and without a callsign.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10809

Differential Revision: https://secure.phabricator.com/D15712
2016-04-14 10:15:23 -07:00
June Rhodes
7150aa8e19 Use Conduit in PhabricatorRepositoryGitCommitChangeParserWorker
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
2016-04-14 04:53:03 -07:00
epriestley
c0428b4d6d Make Git prompt for passwords when the user provides a username but not a password
Summary: Fixes T10797. This seems to fix things on my local system.

Test Plan:
  - Cloned with a username, got prompted for a password.
  - Cloned with a username + password.
  - Cloned with a username + bad password (error).

Reviewers: chad

Reviewed By: chad

Subscribers: Grimeh

Maniphest Tasks: T10797

Differential Revision: https://secure.phabricator.com/D15706
2016-04-13 18:59:45 -07:00
epriestley
4244cad990 Move toward multi-master replicated repositories
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
2016-04-12 08:57:57 -07:00
epriestley
58eef68b7c Rough cut of repository cluster status panel
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
2016-04-12 05:38:10 -07:00
epriestley
8a153c1fe9 Rough cut at new "pro" Diffusion edit UI skeleton
Summary:
Ref T4292. This puts a very rough skeleton in place for the new "Manage Repository" UI, somewhat similar to the "Settings" UI.

Right now, it has one panel with no content, and is not reachable from the UI.

Test Plan: {F1214525}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4292

Differential Revision: https://secure.phabricator.com/D15683
2016-04-12 05:37:54 -07:00
epriestley
37b93f4262 Don't require POST to download LFS files from main domain
Summary:
Ref T7789. If you don't have `security.alternate-file-domain` configured, we won't serve binary files over GET.

This is a security measure intended to prevent `<applet src="..." />` attacks and similar, where you upload some "dangerous" binary, include it in another page, and it gets some of the host's permissions because Java/Flash security models are (or were, in the past) goofy.

Allow them to be served over GET if the client is Git LFS. This is safe; these attacks can't add arbitrary HTTP headers.

Test Plan:
Fetched files over GET with and without the LFS header.

```
$ curl -v http://local.phacility.com/file/data/@local/jfht2cxjazi5cmjomfhl/PHID-FILE-sa7mh2pfaocz2adiimeh/netgear_rma.pdf > /dev/null
...
HTTP 302 Redirect
...
```

```
$ curl -v -H 'X-Phabricator-Request-Type: git-lfs' http://localcontent.phacility.com/file/data/@local/jfht2cxjazi5cmjomfhl/PHID-FILE-sa7mh2pfaocz2adiimeh/netgear_rma.pdf > /dev/null
...
HTTP 200 Content
...
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D15654
2016-04-07 08:55:32 -07:00
Chad Little
8aad862cd4 Normalize casing on property boxes
Summary: Going to render these all normal case instead of all caps, and bump up the font size. Should be more consistent. Yellow if you green anything orange.

Test Plan: grep, lint

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15645
2016-04-06 15:33:15 -07:00
epriestley
439821c7b2 Don't require one-time tokens to view file resources
Summary:
Ref T10262. This removes one-time tokens and makes file data responses always-cacheable (for 30 days).

The URI will stop working once any attached object changes its view policy, or the file view policy itself changes.

Files with `canCDN` (totally public data like profile images, CSS, JS, etc) use "cache-control: public" so they can be CDN'd.

Files without `canCDN` use "cache-control: private" so they won't be cached by the CDN. They could still be cached by a misbehaving local cache, but if you don't want your users seeing one anothers' secret files you should configure your local network properly.

Our "Cache-Control" headers were also from 1999 or something, update them to be more modern/sane. I can't find any evidence that any browser has done the wrong thing with this simpler ruleset in the last ~10 years.

Test Plan:
  - Configured alternate file domain.
  - Viewed site: stuff worked.
  - Accessed a file on primary domain, got redirected to alternate domain.
  - Verified proper cache headers for `canCDN` (public) and non-`canCDN` (private) files.
  - Uploaded a file to a task, edited task policy, verified it scrambled the old URI.
  - Reloaded task, new URI generated transparently.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10262

Differential Revision: https://secure.phabricator.com/D15642
2016-04-06 14:14:36 -07:00
Chad Little
3317086fdb Convert missing commit page to newPage
Summary: Minor conversion.

Test Plan: Fake a missing commit. View same layout.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15608
2016-04-04 10:34:34 -07:00
epriestley
b07a524b4b Fix resolution of commits in SVN repositories without callsigns
Summary:
Fixes T10721. When trying to load commits by identifier, we would take some bad pathways in Subversion if the repository had no callsign and end up missing the commits.

Fix this logic so it works for either callsigns (e.g., if passed `rXyyy`) or with PHIDs if passed repositories.

Test Plan:
  - Viewed SVN commit in a Subversion repository with no callsign.
  - Added a callsign, looked at it again.
  - Viewed non-SVN commits in callsign and non-callsign repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10721

Differential Revision: https://secure.phabricator.com/D15607
2016-04-04 09:44:36 -07:00
epriestley
060f96079d Fix Diffusion blame columns when disabling blame
Summary:
Fixes T10385. Two issues:

  - `$show_blame` and `$show_color` were improperly swapped.
  - Code to hide these columns got dropped somewhere, probably in my recent-ish rewrite.

Test Plan:
  - Showed/hid blame.
  - Showed/hid colors.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10385

Differential Revision: https://secure.phabricator.com/D15528
2016-03-26 12:06:37 -07:00
epriestley
601aaa5a86 Modularize content sources
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
2016-03-26 11:59:45 -07:00
epriestley
4dc857e36d Fix an issue with incorrect split head detection in Mercurial after pushing a medley of varied changes
Summary:
Fixes T10665. See that task for discussion.

Because `$head_map` is not properly re-initialized for each ref we check, pushes which affect multiple branches (say, "A" and "B") can have information bleed from the first branch check to the second branch.

To trigger a problem behavior, you can push one commit which updates an existing branch, plus one commit which creates a new branch. If they process in the right order, the `$head_map` from the updated branch will bleed into the `$head_map` for the new branch and trigger an incorrect head split detection.

Test Plan:
  - Pushed a set of changes which updated `branch-a` and created `branch-b`.
    - Before change: improper detection of split heads.
    - After change: clean push.
  - Pushed a set of changes which split the head of `branch-d`.
    - Correct detection of split heads.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10665

Differential Revision: https://secure.phabricator.com/D15522
2016-03-25 07:33:55 -07:00
Vlad Albulescu
130e1d1f68 Unbreak regex filename search
Summary:
D9087 adds a nice typeahead but breaks the existing regex
search by quoting the pattern. Ideally, this change won't break the
typeahead, which as far as I can tell doesn't use the `pattern`
argument.

Test Plan:
Not yet.
RFC as to whether this change makes sense, will fix my local setup and resend if so.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15500
2016-03-20 10:15:21 -07:00
epriestley
76f07ec80b Only require view permissions for read-only Git LFS requests
Summary: Ref T7789. Implement proper detection for read-only requests. Previously, we assumed every request was read/write and required lots of permissions, but we don't need "Can Push" permission if you're only cloning/fetching/pulling.

Test Plan:
  - Set push policy to "no one".
  - Fetched, got clean data out of LFS.
  - Tried to push, got useful error.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D15499
2016-03-19 14:23:22 -07:00
Chad Little
d5f9e49e29 Use PHUIStatusListView in Diffusion commit list
Summary: Fixes T10626. Adds proper wrapper

Test Plan: Review spacing on a commit with comitted in the property list.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T10626

Differential Revision: https://secure.phabricator.com/D15498
2016-03-19 15:34:31 +00:00
Chad Little
01885cad1c Couple of Diffusion tweaks
Summary:
 - Fix spacing on InfoView inside collasped boxes
 - Fix spacing on stacked PropertyLists in TwoColumn
 - Fix spacing on Readmes on Tablets
 - Fix unset variable on importing commits

Test Plan: Review each of the above cases.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15496
2016-03-18 12:06:16 -07:00
epriestley
61ab7afc9c Make Diffusion do an alright job on Git LFS objects
Summary: Ref T7789. This isn't the most perfect UI imaginable, but it's similar to what GitHub does and seems reasonable.

Test Plan:
{F1180271}

{F1180272}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D15494
2016-03-18 09:37:15 -07:00
epriestley
a24f001b08 Support pushing data into Git LFS
Summary:
Ref T7789. Ref T10604. This implements the `upload` action, which streams file data into Files.

This makes Git LFS actually work, at least roughly.

Test Plan:
  - Tracked files in an LFS repository.
  - Pushed LFS data (`git lfs track '*.png'; git add something.png; git commit -m ...; git push`).
  - Pulled LFS data (`git checkout master^; rm -rf .git/lfs; git checkout master; open something.png`).
  - Verified LFS refs show up in the gitlfsref table.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7789, T10604

Differential Revision: https://secure.phabricator.com/D15492
2016-03-18 09:36:34 -07:00
epriestley
f07d0ae7c3 Make dates/times more concise in Diffusion
Summary: I think I like this better -- but maybe right-aligned?

Test Plan:
{F1180295}

{F1180296}

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15495
2016-03-18 09:11:09 -07:00
epriestley
f46686ff58 Implement a Git LFS link table and basic batch API
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
2016-03-17 17:15:20 -07:00
Chad Little
8f94aa8a06 Update Diffusion UI
Summary: This updates (all?) of Diffusion/Audit to new UI, included edit and other extra form pages. It's fairly complete but I don't know all the nooks and crannies so to speak to fully verify I didn't mess anything up.

Test Plan: Tested creating new repositories, browsing, searching, auditing. Need more eyes.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15487
2016-03-17 12:05:14 -07:00
epriestley
08b1a33dc3 Implement a Git LFS server which supports no operations
Summary:
Ref T7789. This builds on top of `git-lfs-authenticate` to detect LFS requests, read LFS tokens, and route them to a handler which can do useful things.

This handler promptly drops them on the floor with an error message.

Test Plan:
Here's a transcript showing the parts working together so far:

  - `git-lfs` connects to the server with SSH, and gets told how to connect with HTTP to do uploads.
  - `git-lfs` uses HTTP, and authenticates with the tokens properly.
  - But the server tells it to go away, and that it doesn't support anything, so the operation ultimately fails.

```
$ GIT_TRACE=1 git lfs push origin master
12:45:56.153913 git.c:558               trace: exec: 'git-lfs' 'push' 'origin' 'master'
12:45:56.154376 run-command.c:335       trace: run_command: 'git-lfs' 'push' 'origin' 'master'
trace git-lfs: Upload refs origin to remote [master]
trace git-lfs: run_command: git rev-list --objects master --not --remotes=origin
trace git-lfs: run_command: git cat-file --batch-check
trace git-lfs: run_command: git cat-file --batch
trace git-lfs: run_command: 'git' config -l
trace git-lfs: tq: starting 3 transfer workers
trace git-lfs: tq: running as batched queue, batch size of 100
trace git-lfs: prepare upload: b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69 lfs/dog1.jpg 1/1
trace git-lfs: tq: sending batch of size 1
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload
trace git-lfs: api: batch 1 files
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects/batch
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\/batch\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: api: batch not implemented: 404
trace git-lfs: run_command: 'git' config lfs.batch false
trace git-lfs: tq: batch api not implemented, falling back to individual
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
trace git-lfs: tq: retrying 1 failed transfers
trace git-lfs: ssh: local@localvault.phacility.com git-lfs-authenticate diffusion/18/poems.git upload b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69
trace git-lfs: api: uploading (b7e0aeb82a03d627c6aa5fc1bbfd454b6789d9d9affc8607d40168fa18cf6c69)
trace git-lfs: HTTP: POST http://local.phacility.com/diffusion/POEMS/poems.git/info/lfs/objects
trace git-lfs: HTTP: 404
trace git-lfs: HTTP: {"message":"Git LFS operation \"objects\" is not supported by this server."}
trace git-lfs: HTTP:
Git LFS: (0 of 1 files) 0 B / 87.12 KB
Git LFS operation "objects" is not supported by this server.
Git LFS operation "objects" is not supported by this server.
```

Reviewers: chad

Reviewed By: chad

Subscribers: eadler

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D15485
2016-03-17 08:08:43 -07:00
epriestley
51153a580c Implement "git-lfs-authenticate" over SSH
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
2016-03-17 08:08:00 -07:00
Chad Little
148a50e48b Convert Differential to new layout
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.

To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design

Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15463
2016-03-12 13:04:21 -08:00
epriestley
aaab1011e5 Give AphrontTagView a getViewer(), deprecate getUser()
Summary:
Two minor changes here:

  - Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
  - `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.

Test Plan:
  - Grepped for `->user`.
  - Attempted to fix all callsites inside `*View` classes.
  - Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D15412
2016-03-06 09:27:38 -08:00
epriestley
7d4b323da2 Store Almanac "service types" instead of "service classes"
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
2016-02-26 06:21:50 -08:00
epriestley
5b9d8aeae7 Fix two issues with callsign-free repositories
Summary:
Ref T4245. These callsites don't quite do the right thing if a repository has no callsign.

See also <https://github.com/phacility/phabricator/pull/822>.

Test Plan: Made a comment on a commit in a repository with no callsign.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15344
2016-02-26 06:13:46 -08:00
epriestley
ee6070a984 Add a couple of missing needProperties() Almanac calls
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
2016-02-23 13:50:49 -08:00
Ben Alpert
31927476e3 Reorder audit actions to match Differential
Summary:
These trip me up every time because Differential has:

> Comment, Accept, Request Changes, Resign, Commandeer, Add Reviewers, Add Subscribers

while audits currently show:

> Comment, Add Subscribers, Add Auditors, Accept, Raise Concern, Resign

Now they're more or less in the same order which helps with muscle memory.

Test Plan: Careful inspection.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15323
2016-02-21 12:16:04 -08:00
Chad Little
5eecff91cd Move Diffusion ReadmeView to PHUIDocumentProView
Summary: Swapping out to PHUIDocumentProView to remove all calls to PHUIDocumentView.

Test Plan: Review the Phabricator Readme.MD in Diffusion

Reviewers: epriestley, avivey

Reviewed By: avivey

Subscribers: avivey, Korvin

Differential Revision: https://secure.phabricator.com/D15308
2016-02-19 04:18:33 +00:00
epriestley
dc7d0b4a56 Make repository callsigns optional
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
2016-02-18 10:36:13 -08:00
epriestley
dfc8f8bcb4 Make callsigns editable on repository basic information
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
2016-02-18 10:34:32 -08:00
epriestley
b63eb09cac Don't require a callsign to set a repository's local path
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
2016-02-18 10:33:10 -08:00
epriestley
74a79aa634 Make serving repositories work with alternate URIs
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
2016-02-18 10:06:03 -08:00
epriestley
c2b8dd28d8 Support ID-based repository URIs, and canonicalize repository URIs
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
2016-02-18 09:56:28 -08:00
epriestley
93d7b01222 Remove uncalled DiffusionRequest->getCallsign()
Summary: Ref T4245. This has no callers.

Test Plan:
  - Ran `git grep -i 'getCallsign('` and visually verified that no callers could reasonably be `DiffusionRequest` objects (there are only 23 remaining sites, and about half are `$this->...` in `PhabricatorRepository`.
  - Browsed around directory/file/branch/content/diff/etc pages in Diffusion.

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15295
2016-02-17 17:17:35 -08:00
epriestley
925a0d3d59 Pass repository PHID to custom hooks in PHABRICATOR_REPOSITORY instead of callsign
Summary:
Ref T4245. We pass this exclusively for use by additional third-party hooks.

This is technically a backward compatibility break, but I suspect it doesn't affect anyone:

  - Probably almost no one is using this (there are few reasons to, even for the tiny number of installs with custom commit hooks).
  - If they are, there's a good chance the PHID will work anyway, since nearly all scripts and Conduit methods will accept it in place of a callsign now, and if it's in logging or debugging code the PHID is a reasonable substitute
  - Even if it doesn't just keep working, the break should be very obvious in most reasonable cases.

I'll call this out explicitly in the changelog, though -- almost everything else will just continue working, but this is a strict compatibility break.

Test Plan:
  - Ugh.
  - Picked a hosted Git repo out of Diffusion.
  - Went to the path on disk.
  - Went into `hooks/`.
  - Went into `pre-receive-phabricator.d/`.
  - Wrote this hook and gave it `chmod +x`:

```name=stuff.sh
#!/bin/sh

echo $PHABRICATOR_REPOSITORY >> /tmp/stuff.log
```

  - Pushed to the repository.
  - Saw a PHID show up in the log:

```
$ cat /tmp/stuff.log
PHID-REPO-bqkcdp47euwnwlasrsrh
```

Reviewers: chad, avivey

Reviewed By: avivey

Subscribers: avivey

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D15294
2016-02-17 17:10:44 -08:00
epriestley
c6d91938e8 Show first 10 branches, then "More Branches" for commits on huge numbers of branches
Summary: Fixes T9562. We already do this for tags, but didn't have similar logic for branches. Implement that logic.

Test Plan:
  - Set limit to 1, saw "More branches", clicked it, got the correct results.
  - Verified that branch table with no specified commit still works properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9562

Differential Revision: https://secure.phabricator.com/D15284
2016-02-16 15:10:01 -08:00
epriestley
58c2141ffd Fix limit calculation for largeish Mercurial repsositories
Summary:
Fixes T10304. In Mercurial, we must enumerate the whole file tree. Currently, we incorrectly count files within directories (which won't be shown) toward the "100 file" limit at top level, so directories with more than 100 subpaths are truncated improperly.

This is approxiately the same as @richardvanvelzen's fix.

Test Plan: Viewed a large Mercurial repository, saw a complete directory listing.

Reviewers: chad

Reviewed By: chad

Subscribers: richardvanvelzen

Maniphest Tasks: T10304

Differential Revision: https://secure.phabricator.com/D15282
2016-02-16 14:14:59 -08:00
Chad Little
f35509e30e Update to use PHUIRemarkupView everywhere possible
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).

Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15281
2016-02-16 14:05:53 -08:00
epriestley
5e3754828f Fix handling of gzip in VCS responses
Summary:
Fixes T10264. I'm reasonably confident that this is the chain of events here:

First, prior to 8269fd6e, we would ignore "Content-Encoding" when reading inbound bodies. So if a request was gzipped, we would read a gzipped body, then give `git-http-backend` a gzipped body with "Content-Encoding: gzip". Everything matched normally, so that was fine, except in the cluster.

In the cluster, we'd accept "gzip + compressed body" and proxy it, but not tell cURL that it was already compressed. cURL would think it was raw data, so it would arrive on the repository host with a compressed body but no "Content-Encoding: gzip". Then we'd hand it to git in the same form. This caused the issue in 8269fd6e: handing it compressed data, but no "this is compressed" header.

To fix this, I made us decompress the encoding when we read the body, so the cluster now proxies raw data instead of proxying gzipped data. This fixed the issue in the cluster, but created a new issue on non-cluster hosts. The new issue is that we accept "gzip + compressed body" and decompress the body, but then pass the //original// header to `git-http-backend`. So now we have the opposite problem from what we originally had: a "gzip" header, but a raw body.

To fix //this//, we could do two things:

  - Revert 8269fd6e, then change the proxy request to preserve "Content-Encoding" instead.
  - Stop telling `git-http-backend` that we're handing it compressed data when we're handing it raw data.

I did the latter here because it's an easier change to make and test, we'll need to interact with the raw data later anyway, to implement repository virtualization in connection with T8238.

Test Plan: See T10264 for users confirming this fix.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10264

Differential Revision: https://secure.phabricator.com/D15258
2016-02-12 08:08:58 -08:00
Chad Little
a39d0344c6 Add commit id to header in Diffusion
Summary: Also adds the commit to the header underneath the title. Ref T7628

Test Plan: Review a few Diffusion pages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T7628

Differential Revision: https://secure.phabricator.com/D15246
2016-02-11 11:33:03 -08:00
Chad Little
f163d83935 Don't link commit uri in Crumbs
Summary: These are not needed I think? and handy for cut and paste. Fixes T7628

Test Plan: cut and paste easier from commit hash.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T7628

Differential Revision: https://secure.phabricator.com/D15245
2016-02-11 11:21:43 -08:00
Aviv Eyal
4c1140a3a7 unbreak exception handling
Summary: fix T10306

Test Plan: ¯\_(ツ)_/¯

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Maniphest Tasks: T10306

Differential Revision: https://secure.phabricator.com/D15229
2016-02-09 18:20:22 +00:00
epriestley
4132ba0853 Improve type and icon information in typeahead
Summary:
Ref T10289. This probably doesn't cover everything but should do a little bit better.

Although we should mabye just exlude milestones from this menu completely?

Test Plan: {F1093937}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10289

Differential Revision: https://secure.phabricator.com/D15191
2016-02-05 12:48:20 -08:00
Chad Little
6bb24e1d0c Move PhabricatorHovercard to PHUIHovercard
Summary: No UI changes, just some search and replace for UI consistency.

Test Plan: Test person and object hovercards still work. UIExamples too.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15172
2016-02-03 16:26:30 +00:00
epriestley
9d125b459e Use large text columns to store IP addresses
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
2016-02-02 10:13:14 -08:00
Mike Riley
d41aaba2a1 Fix coverage line index lookup in diffusion browser
Summary: I believe this got clobbered in rP8b6edaa4e238a809fe78e6d14ad0705545f8179f. This index doesn't seem to be present in the line dictionary and we're now relying on `$line_index` for the current position.

Test Plan:
before {F1085522}
after {F1085521}

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D15156
2016-02-01 19:19:52 +00:00
epriestley
e5947e08d3 Apply phutil_utf8ize() to stderr output from VCS commands prior to logging
Summary: Ref T10228. Commands like `git-http-backend` can emit errors with raw bytes in the output. Sanitize these if present so we can log them in JSON format.

Test Plan: Edited this into production. >_> sneaky sneaky <_<

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10228

Differential Revision: https://secure.phabricator.com/D15144
2016-01-30 16:46:23 -08:00
epriestley
a5f6223553 Show blame colors on all lines, instead of only the first affected line
Summary: Fixes T10226. I just made a mistake here when rewriting this recently.

Test Plan: {F1079166}

Reviewers: chad, avivey

Reviewed By: avivey

Maniphest Tasks: T10226

Differential Revision: https://secure.phabricator.com/D15131
2016-01-28 11:17:15 -08:00
Chad Little
fe5cd4ca2c Move FontIcon calls to Icon
Summary: Normalizes all `setFontIcon` calls to `setIcon`.

Test Plan: UIExamples, Almanac, Apps list, etc.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, hach-que, yelirekim

Differential Revision: https://secure.phabricator.com/D15129
2016-01-28 08:48:45 -08:00
epriestley
30473549ac Add a basic pull event log for debugging repository cloning
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
2016-01-28 08:18:34 -08:00
Chad Little
36158dbdc0 Convert all calls to 'IconFont' to just 'Icon'
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.

Test Plan: tested uiexamples, lots of other random pages.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D15125
2016-01-27 20:59:27 -08:00
epriestley
b53d61c909 Fix bad call to getShortName()
Summary: Fixes T10212. This method was removed in D14990, but I missed a callsite.

Test Plan: Disabling blame now works nicely.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10212

Differential Revision: https://secure.phabricator.com/D15100
2016-01-23 17:07:27 -08:00
epriestley
b51a859636 Allow diffusion.filecontentquery to load data for arbitrarily large files
Summary:
Fixes T10186. After D14970, `diffusion.filecontentquery` puts the content in a file and returns the file PHID.

However, it does this in a way that doesn't go through the chunking engine, so it will fail for files larger than the chunk threshold (generally, 8MB).

Instead, stream the file from the underlying command directly into chunked storage.

Test Plan:
  - Made a commit including a really big file: 4dcd4c492b
  - Used `diffusion.filecontentquery` to load file content.
  - Parsed/imported commit locally.
  - Used `diffusion.filecontentquery` to load content for smaller files (README, etc).

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10186

Differential Revision: https://secure.phabricator.com/D15072
2016-01-21 09:52:43 -08:00
Aviv Eyal
98d2370672 Add link to Herald Transaction when blocking pushes
Summary: Ref T9719.

Test Plan: Tried to push, get dragon and a link.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Maniphest Tasks: T9719

Differential Revision: https://secure.phabricator.com/D15027
2016-01-15 03:01:47 +00:00
epriestley
1eab16c395 Move repository URIs to a dedicated index
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
2016-01-13 09:34:31 -08:00
epriestley
a061bd2d09 Parse and display commit authorship date in Git in Diffusion
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
2016-01-11 09:32:37 -08:00
epriestley
9fb929dff3 Show repositories in global autocomplete dropdown
Summary: Fixes T9523.

Test Plan: {F1059225}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9523

Differential Revision: https://secure.phabricator.com/D14994
2016-01-11 09:32:23 -08:00
epriestley
b848ab87b3 Introduce "local names" for commits
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
2016-01-11 09:18:30 -08:00
epriestley
96ebd35824 Change repository "Clone/Checkout As" to "Short Name"
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
2016-01-11 09:17:32 -08:00
epriestley
0b3d10c3da Enforce sensible, unique clone/checkout names for repositories
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
2016-01-11 02:06:44 -08:00
epriestley
c7520cd9f2 Improve rendering of commit branching graph
Summary:
Fixes T9323. Two minor fixes:

  - On the first commit, don't render a downward line.
  - Clean up a 1px spacing issue that had cropped up a while ago when we added icons or something, I think.

Test Plan:
Before:

{F1057248}

After:

{F1057249}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9323

Differential Revision: https://secure.phabricator.com/D14974
2016-01-08 11:52:07 -08:00
epriestley
d1fb2f7fb9 Make diffusion.filecontentquery return file PHIDs instead of raw content
Summary:
Fixes T9319. Proxied requests (e.g., in the cluster) for binary files (like images) currently fail because we can not return binary data over Conduit in JSON.

Although Conduit will eventually support binary-safe encodings, a cleaner approach to this is just to return a `filePHID` instead of the raw content. This is generally faster and more flexible, and gives us more opportunities to add caching later.

After making the call, the client pulls the file data separately.

We also no longer need to return a complex data structure because we don't do blame over this call any longer.

Test Plan:
  - Viewed images in Diffusion.
  - Viewed READMEs in Diffusion.
  - Used `bin/differential attach-commit rX Dy` to hit attach pathway.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9319

Differential Revision: https://secure.phabricator.com/D14970
2016-01-08 09:29:16 -08:00
epriestley
449da36c2f Use a path digest when building blame cache keys
Keys have a maximum length of 128, and long paths could cause key lengths to exceed this.

Auditors: chad
2016-01-06 19:12:57 -08:00
epriestley
d725dedb1e Fix two minor issues with blame that involves revisions
Summary: I was looking at some random un-revisioney repository for most of my testing and missed these.

Test Plan: Viewed blame of a file with some revisions.

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14965
2016-01-06 18:55:48 -08:00
epriestley
438100691d Don't let blame run for longer than 15 seconds
Summary: Fixes T2450. If we spend more than 15 seconds in blame, just cut it off.

Test Plan:
  - Changed timeout to 0.01 seconds.
  - Did blame on a non-highlighted file, got no blame, saw warning.
  - Did blame on a highlighted file, got no blame.
    - Note: you don't get a warning here because of Ajax stuff. It'd be kind of tricky to add and doesn't seem like a big deal so I'm planning to leave it as-is for now.

Reviewers: chad

Reviewed By: chad

Subscribers: 20after4, chasemp

Maniphest Tasks: T2450

Differential Revision: https://secure.phabricator.com/D14964
2016-01-06 18:44:20 -08:00
epriestley
9ab1b5a22d Make mundane performance improvements to Diffusion browse views
Summary:
Ref T2450. This reorganizes code to improve performance.

Mostly, there are a lot of things which are unique per commit (author name, links, short name, etc), but we were rendering them for every line.

This often meant we'd render the same author's name thousands of times. This is slower than rendering it only once.

In 99% of interfaces this doesn't matter, but blame is weird and it's significant on big files.

Test Plan:
Locally, `__phutil_library_map__.php` now has costs of roughly:

  - 550ms for main content (from 650ms before the patch).
  - 1,500ms for blame content (frrom 1,800ms before the patch).

So this isn't huge, is a decent ~20%-ish performance gain for shuffling some stuff around.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2450

Differential Revision: https://secure.phabricator.com/D14963
2016-01-06 18:43:51 -08:00
Fabian Stelzer
e8d3071452 Implement a git blame cache
Summary: Ref T2450. Ref T2453. Add a repository_blamecache table and cache git blame information

Test Plan: View files in Diffusion with enabled blame

Reviewers: fabe, chad, #blessed_reviewers

Reviewed By: chad, #blessed_reviewers

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T2453, T2450

Differential Revision: https://secure.phabricator.com/D10600
2016-01-06 18:43:30 -08:00
epriestley
0759b84d77 Improve construction of commit queries from blame lookups
Summary:
Ref T2450. File blame tends to have the same commit a lot of times, and we don't do lookups like this efficiently right now.

In particular, for a file like `__phutil_library_map__.php`, we would issue a query with ~9,000 clauses like this:

```
(repositoryID = 1 AND commitIdentifier LIKE "XYZ%")
```

...but only a few hundred of those identifiers were unique. Instead, issue only one clause per unique identifier.

MySQL also seems to do a little better on "commitIdentifier = X" if we have the full hash, so special case that slightly.

Test Plan:
  - Issuing a query for only unique identifiers dropped the cost from 400ms to 100ms locally.
  - Swapping to `=` if we have the full hash dropped the cost from 100ms to 75ms locally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2450

Differential Revision: https://secure.phabricator.com/D14962
2016-01-06 18:43:04 -08:00
epriestley
741118a08f Improve Diffusion behavior for directories with impressive numbers of files
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
2016-01-06 14:19:55 -08:00
epriestley
91a01e5703 Improve Diffusion browse performance for large files
Summary:
When looking at a large file in Diffusion:

  - disable highlighting if it's huge and show a note about why;
  - pick up a few other optimizations.

Test Plan: Locally, this improves the main render of `__phutil_library_map__.php` from 3,200ms to 600ms for me, at the cost of syntax highlighting (we can eventually add view options and let users re-enable it).

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14959
2016-01-06 09:24:35 -08:00
epriestley
9728c65e93 Drive blame generation through diffusion.blame
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
2016-01-06 09:24:21 -08:00
epriestley
f561dc172d Implement a dedicated "diffusion.blame" API method
Summary:
Fixes T2451. Several motivations here, from strongest to weakest:

  - Currently, getting blame and file content are closely entwined. This makes fixing T9319 more difficult, and I want to fix it. I want to separate blame from content so there's more flexibility in how we approach this issue.
  - This makes pursuing T2450 easier, if it turns out to be a meaningful win.
  - If we can get a win on blame performance, we can do `arc blame` eventually if we want.

Test Plan:
  - Blamed in SVN, Git and Mercurial.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2451

Differential Revision: https://secure.phabricator.com/D14957
2016-01-06 09:24:03 -08:00
epriestley
94d79c11a9 Show import progress on repository main page
Summary: Fixes T9192.

Test Plan: {F1055042}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9192

Differential Revision: https://secure.phabricator.com/D14951
2016-01-05 14:02:59 -08:00
epriestley
f9a5cd2bbd Fix all remaining weird Diffusion request processing
Summary: Ref T4245. This is the last of it, and covers the clone/push stuff.

Test Plan:
  - Cloned git.
  - Pushed git.
  - Cloned mercurial.
  - Pushed mercurial.
  - Visited a `blah.git` URL in my browser just because; got redirected to a human-facing UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14949
2016-01-05 14:01:53 -08:00
epriestley
38f2008e68 Modernize Diffusion lint controllers
Summary: Ref T4245. On their best day these don't work all that well, but I'm pretty sure I didn't make anything worse.

Test Plan:
  - Viewed global lint.
  - Viewed lint for a repository.
  - Viewed lint details for a particular message.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14948
2016-01-05 14:01:20 -08:00
epriestley
3cbc239bc6 Modernize most somewhat-weird Diffusion controllers
Summary: Ref T4245. This gets everything else except serving HTTP requests (complicated) and lint (quite weird).

Test Plan:
  - Viewed a diff.
  - Viewed externals.
  - Viewed history table to see last modified.
  - Did path completion and validation in Owners.
  - Did tree path search in Diffusion.
  - Viewed a repository.
  - Created a new repository.
  - Looked up symbols.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14947
2016-01-05 14:00:57 -08:00
epriestley
649f882720 Slightly modernize all Diffusion edit endpoints
Summary: Ref T4245. Prepares edit endpoints for more flexible repository identifiers.

Test Plan:
  - Added, edited, deleted mirror.
  - Created repository.
  - Edited basic repository information.
  - Edited policies for a repository.
  - Activated/deactivated repository.
  - Updated a repository.
  - Hit "Delete" dialog for a repository.
  - Edited hosting.
  - Toggled dangerous changes.
  - Edited branches.
  - Edited automation.
  - Tested automation.
  - Edited storage.
  - Edited staging.
  - Edited encoding.
  - Edited symbols.
  - Edited branches.
  - Edited actions.
  - Tried to do edits as an unprivileged user.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14945
2016-01-05 14:00:36 -08:00
epriestley
2bfc5ff92e Modernize more Diffusion controllers
Summary: Ref T4245. Standardize how context is read, minor updates / modernizations / consistency tweaks.

Test Plan:
  - Viewed a change.
  - Viewed brnaches.
  - Edited a commit.
  - Viewed tags.
  - Viewed history.
  - Added, edited and deleted a mirror.
  - Viewed push events.
  - Viewed a particular event.
  - Viewed ref disambiguation.
  - Viewed repository list.
  - Ran automation test.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14944
2016-01-05 14:00:20 -08:00
epriestley
f1c298203a Return no results from grep repository queries on error
Summary: Fixes T7852. Although `1` could also indicate other kinds of problems, assume it means "no results".

Test Plan: Searched for nonsense strings in Git and Mercurial. Searched for valid strings in Git and Mercurial.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7852

Differential Revision: https://secure.phabricator.com/D14943
2016-01-05 13:59:12 -08:00
epriestley
8b6edaa4e2 Merge and modernize Browse controllers in Diffusion
Summary:
Ref T4245. Browsing is huge and currently split across 5 files using controller delegation.

Although having a huge file isn't great, I think the way it is split up is currently worse, and it gets weird with more flexible repository identifiers.

So this is mostly merging five controllers into one, then a bit of modernization.

I think this can probably be split up better by pulling some of it out into views, instead of using delegation.

Test Plan: Browsed files, directories, and search results.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14942
2016-01-05 13:58:55 -08:00
epriestley
7de17fb75e Modernize tag and branch controllers in Diffusion
Summary: Ref T4245. Prepares these controllers to accept alternate identifers, plus minor spacing and layout fixes.

Test Plan: Viewed tags, viewed branches.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14941
2016-01-05 13:58:36 -08:00
epriestley
fb3b4ee532 Make CommitController more flexible about handling URIs
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
2016-01-05 13:56:27 -08:00
epriestley
07e2596aa1 Move generateDiffusionURI() into PhabricatorRepository
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
2016-01-05 04:47:06 -08:00
epriestley
bcfd6bdd81 Move various other callsites away from callsigns
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
2016-01-04 06:54:42 -08:00
epriestley
9febfb26a0 Convert diffusion.looksoon to use repository identifiers instead of callsigns
Summary:
Ref T4245. Like everything else, accept more identifiers.

This needs a change in `arc`, which I've made a note about elsewhere.

Test Plan: Used "Update Now" from web UI, saw update get scheduled.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14932
2016-01-04 06:54:20 -08:00
epriestley
be5b89687e Separate external editor integration from callsigns
Summary: Ref T4245. Pass the whole repository in so it can do something else in a future change.

Test Plan: Loaded changesets in Diffusion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14931
2016-01-04 06:54:01 -08:00
epriestley
b1388c5ca1 Remove diffusion.getcommits Conduit API method
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
2016-01-04 06:53:45 -08:00
epriestley
35e7a1f3c0 Continue reducing callers to getCallsign()
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
2016-01-04 06:53:32 -08:00
epriestley
9d84eb4c74 Allow Conduit API methods in Diffusion to accept any repository identifier
Summary:
Ref T4245. Broaden support to include "ABCD", "rABCD", "1234", "R1234", etc.

This doesn't change the old behavior, just accepts more stuff.

Test Plan:
  - Browsed Diffusion.
  - Made various calls via API console.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14928
2016-01-04 06:51:04 -08:00
epriestley
37532a0bf0 Remove various additional calls to getCallsign()
Summary: Ref T4245. These are all straightforward to remove.

Test Plan:
- Edited paths in a package.
- Ran `bin/audit delete --repositories ...` with various identifiers.
- Searched by repository for `R3`, `rAAAA` in Harbormaster.
- Did a Herald dry run on a commit.
- Browsed commits, made comments.
- Viewed a Releeph product list.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D14927
2016-01-02 11:04:22 -08:00
epriestley
1b4f5e38ce Fix an issue with rendering some commit hovercards
Summary: This logic wasn't quite right.

Test Plan: Hovered over a recognized commit, got a valid hovercard

Reviewers: chad

Reviewed By: chad

Differential Revision: https://secure.phabricator.com/D14925
2016-01-02 11:03:12 -08:00
epriestley
84a570a61b fix broken link for project creating button
Summary: fix T10074

Test Plan: click the "create project" button

Reviewers: joshuaspence, #blessed_reviewers, epriestley

Reviewed By: joshuaspence, #blessed_reviewers, epriestley

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T10074

Differential Revision: https://secure.phabricator.com/D14917
2015-12-31 05:59:05 -08:00
epriestley
bdc517485c Modernize Hovercard implementation
Summary:
Ref T8980. Move away from events to EngineExtensions.

This also simplifies hovercards a bit:

  - Removes tasks from revision cards.
  - Removes blockers/blocked from task cards.
  - Removes "Send Message" from user cards.

These mostly felt cluttery to me. Open to arguments to retain them. I think we can make better use of the space, though (e.g., flags, projects + board columns).

Test Plan:
  - Viewed people, task, revision, commit and project hovercards.

{F1043256}

{F1043257}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8980

Differential Revision: https://secure.phabricator.com/D14878
2015-12-24 12:18:28 -08:00
epriestley
2868a69f65 Remove all setObjectURI() from ActionListViews
Summary:
Ref T10004. After D14804, we get this behavior by default and no longer need to set it explicitly.

(If some endpoint did eventually need to set it explicitly, it could just change what it passes to `setHref()`, but I believe we currently have no such endpoints and do not foresee ever having any.)

Test Plan:
  - As a logged out user, clicked various links in Differential, Maniphest, Files, etc., always got redirected to a sensible place after login.
  - Grepped for `setObjectURI()`, `getObjectURI()` (there are a few remaining callsites, but to a different method with the same name in Doorkeeper).

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que

Maniphest Tasks: T10004

Differential Revision: https://secure.phabricator.com/D14805
2015-12-17 08:30:22 -08:00
epriestley
8dcdc7534d Add a DiffusionRefDatasource for typeahead'ing branches, tags, bookmarks and refs
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
2015-12-10 14:21:24 -08:00
epriestley
b4681aa0c8 Remove dead link to Perforce blog about Git Fusion
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
2015-12-09 08:51:25 -08:00
epriestley
2353121dee Add "Is merge commit?" to commit fields in Herald
Summary: Fixes T5788. We already have this as a pre-commit field, add it as a post-commit field too.

Test Plan: Ran this rule on a merge commit. Also ran it on a non-merge commit. Both got the correct value.

Reviewers: avivey, chad

Reviewed By: avivey, chad

Subscribers: avivey

Maniphest Tasks: T5788

Differential Revision: https://secure.phabricator.com/D14685
2015-12-05 13:07:56 -08:00
epriestley
5963c4c9e0 Fix history link in Diffusion non-root browse views
Summary:
Fixes T9798. That task has good repro instructions.

In sub-views, we don't link the "History" icon correctly -- we only link it to `history/README` instead of `history/path/to/README`. Add the full path.

Also canonicalize the paths in a slightly prettier and more consistenty way.

Test Plan: Viewed root and non-root browse tables, saw links show up properly.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9798

Differential Revision: https://secure.phabricator.com/D14491
2015-11-16 09:00:29 -08:00
David Fisher
c589af51e8 add "update" mode to Diffusion coverage Conduit
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
2015-11-09 16:52:34 -08:00
Joshua Spence
c35b564f4d Various translation improvements
Summary: Depends on D14070.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D14073
2015-11-03 07:02:46 +11:00
Aviv Eyal
724f6ddda5 return this in DiffusionCommitQuery
Test Plan: chain another call after this

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D14364
2015-10-28 23:25:41 +00:00
epriestley
a763f9510e Add some Drydock documentation plus "Test Configuration" for repository automation
Summary:
Ref T182. Ref T9252.

  - Adds a "Test" repository operation that just runs `git status` to see if things work.
  - Adds a button for it in Edit Repository.
  - Shows operation status on the operation detail view to make this workflow work a little better.
  - Adds a lot of words. Words words words words.

Test Plan:
  - Tested repository operation.
  - Read words.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T182, T9252

Differential Revision: https://secure.phabricator.com/D14349
2015-10-27 18:04:02 +00:00
epriestley
4c1463eb56 Probably fix bad URI construction for Diffusion symbols
Summary: Ref T9532.

Test Plan: I don't have this configured locally but this seems very likely to be the correct fix. This list should be a list of PHIDs, but is a list of PHIDs followed by one PhabricatorRepository object.

Reviewers: avivey, chad

Reviewed By: chad

Maniphest Tasks: T9532

Differential Revision: https://secure.phabricator.com/D14311
2015-10-20 09:03:47 -07:00
epriestley
df5a031b54 Allow "Repository Automation" to be configured for repositories
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
2015-10-13 15:45:59 -07:00
Christopher Speck
812c41a18a Conditionally use hg files vs. hg locate depending on version of Mercurial
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
2015-10-12 17:50:26 -07:00
Christopher Speck
32d4ae8cb2 Added an intercept to Mercurial's capabilities command to remove bundle2.
Summary:
If Mercurial 3.4+ is used to host repositories in Phabricator, any clients using 3.5+ will receive an exception after the bundle is pushed up. Clients will also fail to update phases for changesets pushed up.

Before directly responding to mercurial clients with all capabilities, this change filters out the 'bundle2' capability so the client negotiates using a legacy bundle wire format instead.

Test Plan:
Server: Mercurial 3.5
Client: Mercurial 3.4

Test with both HTTP and SSH protocols:
1. Create a local commit on client
2. Push commit to server
3. Verify the client emits something like:
```
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
```

Closes T9450

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Maniphest Tasks: T9450

Differential Revision: https://secure.phabricator.com/D14241
2015-10-10 07:14:48 -07:00
epriestley
2bfa0e087e Improve consistency and Harbormaster integration of Diffusion
Summary:
Ref T9123. Two major Harbormaster-related UI changes in Diffusion:

  - Tags table now shows tag build status.
  - Branches table now shows branch build status.

Then some minor consistency / qualtiy of life changes:

  - Picked a nicer looking "history" icon?
  - Branches table now uses the same "history" icon as other tables.
  - Tags table now has a "history" link.
  - Browse table now has a "history" link.
  - Dates now use more consistent formatting.
  - Column order is now more consistent.
  - Use of style is now more consistent.

Test Plan:
{F865056}

{F865057}

{F865058}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9123

Differential Revision: https://secure.phabricator.com/D14242
2015-10-06 07:38:15 -07:00
epriestley
b2e89a9e48 Fix several error handling issues with Subversion commits in Diffusion
Summary:
Ref T9513. I checked this briefly but didn't do a very thorough job of it.

  - Don't try to query merges for Subversion, since it doesn't support them.
  - Fix up "existsquery" to work properly (and efficiently) for both hosted and imported repositories.
  - Fix up "parentsquery" to have similar behavior on invalid commits to other VCSes (throw an exception).

Test Plan:
  - No more merges warning on SVN.
  - Hosted SVN gets the right exists result now.
  - Visiting "r23980283789287" now 404's instead of "not parsed yet".

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9513

Differential Revision: https://secure.phabricator.com/D14239
2015-10-05 15:57:41 -07:00
epriestley
0db86cce7d Improve Diffusion behavior for no-longer-existing commits
Summary:
Ref T9028. When users push a commit, then later delete it (e.g., by deleting the branch which contained it) we currently explode when trying to view it.

Instead, degrade gradually if some information is not available.

Test Plan:
  - Looked at valid commits with parents, refs, branches and merges.
  - Looked at invalid commits.
  - Looked at a previously valid, now-deleted + gc'd commit:

{F859273}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9028

Differential Revision: https://secure.phabricator.com/D14227
2015-10-02 16:11:03 -07:00
epriestley
2728a9f964 Allow builds to have parameters
Summary: Ref T9352. See D13635. Build targets can have variables already, but let builds have them too. This mostly enables future use cases (sub-builds, more sophisticated build triggers).

Test Plan: With a custom Herald rule + action like the one in T9352, updated a revision and saw it generate multiple builds with varying parameters.

Reviewers: chad, hach-que

Reviewed By: hach-que

Maniphest Tasks: T9352

Differential Revision: https://secure.phabricator.com/D14222
2015-10-02 06:32:08 -07:00
epriestley
6d5c9e897d Hide "Revision" column in Diffusion history view if Differential is uninstalled
Summary: Fixes T9481. If the viewer does not have access to Differential (for example, because it is not installed), hide the "Revision" column in Diffusion.

Test Plan:
  - Viewed history, saw "Revision" column.
  - Uninstalled Differential, reloaded, no "Revision" column.

Reviewers: chad

Reviewed By: chad

Subscribers: revi

Maniphest Tasks: T9481

Differential Revision: https://secure.phabricator.com/D14188
2015-09-29 07:09:12 -07:00
epriestley
fa943f744b Stop all object mentions from matching after "@"
Summary:
Fixes T9479. Currently, `@aaaaaaaa` may try to match as a commit hash, and `@C123456` may try to match as a Countdown reference. These should only match as user mentions.

Prevent object mention rules from matching after `@`. We already prevent them after `-` and `#`, and already prevented the username rule after `@` (i.e., preventing `@@user`).

Test Plan:
Created some "interesting" users locally and `@mentioned` them:

{F850779}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9479

Differential Revision: https://secure.phabricator.com/D14186
2015-09-29 06:43:49 -07:00
Chad Little
799d86dc65 Show file UI box as collapsed in Diffusion
Summary: This UI should have a Collapsed PHUIObjectBoxView.

Test Plan: review a file in sandbox

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14139
2015-09-21 12:29:17 -07:00
Chad Little
666f19e504 Make icon setting in Section Headers easier/consistent
Summary: You can already pass other icons, but this makes it a bit simpler.

Test Plan: Test Maniphest, Badges

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D14131
2015-09-19 11:29:01 -07:00
Chad Little
d199560a6b Add a box around the pager in Diffusion
Summary: Fixes T9392, adds some sweet sweet margin to the pager.

Test Plan: See pager with new padding, test different pages, breakpoints.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T9392

Differential Revision: https://secure.phabricator.com/D14098
2015-09-11 08:59:56 -07:00
epriestley
4e181a5611 Clean up browse/history links in Diffusion
Summary:
Fixes T9126. In particular:

  - Add "Browse" links to all history views.
  - Use icons to show "Browse" and "History" links, instead of text.
  - Use FontAwesome.
  - Generally standardize handling of these elements.

This might need a little design attention, but I think it's an improvement overall.

Test Plan:
  - Viewed repository history.
  - Viewed branch history.
  - Viewed file history.
  - Viewed table of contents on a commit.
  - Viewed merged changes on a merge commit.
  - Viewed a directory containing an external.
  - Viewed a deleted file.

{F788419}

{F788420}

{F788421}

{F788422}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9126

Differential Revision: https://secure.phabricator.com/D14096
2015-09-10 19:28:49 -07:00
epriestley
20ce1a905f Replace AphrontUsageException with AphrontMalformedRequestException
Summary:
Ref T1806. Ref T7173. Context here is that I want to fix "you can not log in to this instance" being a confusing mess with an opaque error. To do this without hacks, I want to:

  - clean up some exception handling behavior (this diff);
  - modularize exception handling (next diff);
  - replace confusing, over-general exceptions with tailored ones in the Phacility cluster, using the new modular stuff.

This cleans up an awkward "AphrontUsageException" which does some weird stuff right now. In particular, it is extensible and extended in one place in Diffusion, but that extension is meaningless.

Realign this as "AphrontMalformedRequestException", which is a better description of what it is and does: raises errors before we can get as far as normal routing and site handling.

Test Plan: Hit some of these exceptions, saw the expected "abandon all hope" error page.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T1806, T7173

Differential Revision: https://secure.phabricator.com/D14047
2015-09-03 10:04:17 -07:00
epriestley
28621244ad Fix abrupt failure mode for uncloned repositories in Diffusion
Summary:
Fixes T9080. We try to list alternatives for the current ref (for example, if you're viewing a branch named "master" but there's also a tag named "master", or, in Mercurial, there are several branches named "master") but fail to abruptly if we can't get the list.

It's fine if we can't get the list; just continue. This is common when the repository hasn't cloned yet.

Test Plan: In a local repository with bad credentials, tried to do anything before and after. Before: completely blocked by error; after: things work normally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9080

Differential Revision: https://secure.phabricator.com/D14044
2015-09-03 10:03:31 -07:00
epriestley
13516cf35f Fix an issue with "packages(...)" in typeaheads
Summary:
Fixes T9302. This datasource wasn't resolving package PHIDs correctly for the actual query.

Also fixes an issue with the "Affected packages that need audit" Herald rule.

Test Plan: Ran a "Needs Audit" query with only packages, and only `packages(user)`.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9302

Differential Revision: https://secure.phabricator.com/D14029
2015-09-01 15:35:25 -07:00
epriestley
bce0698a0f Modernize Audit search engine
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
2015-08-31 10:17:54 -07:00
epriestley
9790f93a83 Show all packages with a claim on a file in the table of contents view
Summary:
Ref T9218. See discussion there for rationale; I think this is the right behavior to pursue.

The screenshot below is pretty ugly. I think it's a lot worse than most real-world cases will be, since you have to sort of opt-in to having crazy levels of overlapping packages, and it's perfectly normal/reasonable for files owned by one package. Owners is powerful enough to let you specify sub-packages with exclusive ownership.

That said, this may be more typical than I hope. I don't think we can reduce the complexity here much for free, but it would might be reasonable to add some view options (e.g.: group by package?, show only packages I own?, show packages as icons with a tooltip?) if it's an issue.

Test Plan: {F734956}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9218

Differential Revision: https://secure.phabricator.com/D13940
2015-08-20 04:46:17 -07:00
Chad Little
e0faa66772 Allow Owners Packages to be archived
Summary: Fixes T8428. Adds status to packages, allows setting and application search. I presume though these need checked elsewhere?

Test Plan: New package, edit package, archive package, run search queries.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T8428

Differential Revision: https://secure.phabricator.com/D13925
2015-08-18 13:36:05 -07:00
epriestley
6010e80e5c Show packages in table of contents views in Diffusion and Differential
Summary:
Fixes T8004.

  - For paths which are part of a package, show the package.
  - Highlight paths which are part of a package you (the viewer) have authority over.

Test Plan:
{F725418}

  - Viewed owned and unowned chagnes in Diffusion and Differential.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8004

Differential Revision: https://secure.phabricator.com/D13923
2015-08-17 10:14:22 -07:00
epriestley
a3393c3ecb Fix Owners lookups for the repository root
Summary:
Ref T9201. At the root of a repository the current path is `null`, but the OwnersQuery wants strings.

This could be resolved a couple different ways, but just cast the arguments to strings since that seems reasonable enough.

Test Plan: Browsed root of a repository.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9201

Differential Revision: https://secure.phabricator.com/D13919
2015-08-16 17:59:14 -07:00
epriestley
36a50ccbae Provide a more modern way to load packages owning a set of files
Summary:
Ref T8320. Ref T8004. This just tries to generally modernize

It also replaces the nonfunctional "Find Owners" link with a new property that just shows owning packages.

Test Plan:
  - Created and edited packages.

{F720478}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T8004, T8320

Differential Revision: https://secure.phabricator.com/D13911
2015-08-15 13:06:10 -07:00
epriestley
8149399312 Convert commits to use unified table of contents
Summary:
Fixes T2183. We now use the same rendering element in both places.

Intentional changes:

  - Package highlighting is out, coming back to both apps in next diff.
  - removed redundant-feeling "Change" link. The information is now shown with a character ("M", "V", etc.) and the page is a click away under "History". Clicking the path also jumps you to substantially similar content. (We could restore it fairly easily, I just think it's probably the least useful thing in the table right now.)

Test Plan: Viewed a bunch of commits in Diffusion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2183

Differential Revision: https://secure.phabricator.com/D13910
2015-08-15 12:57:20 -07:00
Joshua Spence
368f359114 Use PhutilClassMapQuery instead of PhutilSymbolLoader
Summary: Use `PhutilClassMaQuery` instead of `PhutilSymbolLoader`, mostly for consistency. Depends on D13588.

Test Plan: Poked around a bunch of pages.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D13589
2015-08-14 07:49:01 +10:00
Joshua Spence
79f2e81f38 Various linter fixes
Summary: Self-explanatory.

Test Plan: Eyeball it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D13808
2015-08-08 10:19:45 +10:00
epriestley
386d2b62f8 Fix some copy/paste slop with BLOCK actions in Herald transcripts
Summary: Fixes T9060. These actions still work fine, but the transcripts got messed up a bit.

Test Plan: Viewed transcripts with blocking actions.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9060

Differential Revision: https://secure.phabricator.com/D13782
2015-08-04 07:32:19 -07:00
epriestley
a3e2f655eb Modularize the Diffusion/Differential "Block" Herald actions
Also removes HeraldCustomAction. This completes action modularization.

Ref T8726.
2015-08-03 14:33:28 -07:00
epriestley
6f6d88794b Modularize the Diffusion "Add Auditors" Herald action
Ref T8726.
2015-08-03 14:33:27 -07:00
epriestley
776caa507b Modularize the Harbormaster "Run build plan" Herald action
Ref T8726. Modularizes "Run build plan" in Differential and Diffusion.
2015-08-03 14:33:26 -07:00
epriestley
51fead17cf Modularize "Send an Email" Herald actions
Summary: Ref T8726. No surprises.

Test Plan:
Created rules using both action variants, applied upgrade, saw rules still work correctly.

{F658842}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13701
2015-08-03 14:33:23 -07:00
epriestley
8ae08a3de7 Make "Add Subscribers" and "Remove Subscribers" Herald actions modular
Summary: Ref T8726. Converts these actions to be modular. No real surprises in this change.

Test Plan:
{F658709}

  - Wrote some rules.
  - Migrated them forward.
  - Used a bunch of these rules.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13699
2015-08-03 14:33:22 -07:00
epriestley
8d9bd791f7 Modularize Herald "flag" action, plus update transcripts
Summary:
Ref T8726. This modularizes "Mark with flag", plus rebuilds transcripts in a more modern/flexible way. The big transcript stuff is:

  - Transcripts are now translatable.
  - Transcripts can now show multiple outputs from a single action. For example, an action like "add A, B, C to subscribers" can now say "added A; B is invalid; C was already subscribed".

Test Plan: {F637784}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, eadler, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13649
2015-08-03 14:33:21 -07:00
epriestley
56dd5211f0 Make Herald action modularization more aggressive
Summary:
Ref T8726. Herald actions are technically sort-of modular already, but make them more aggressively modular similar to `HeraldField`.

I plan to obsolete and replace `HeraldCustomAction`.

Test Plan: Saw actions in nice groups; created and ran a "Do Nothing" action. Transcripts are a bit rough for now.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: joshuaspence, epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13646
2015-08-03 14:33:20 -07:00
epriestley
f1222f956a Correctly clear draft markers when deleting an inline comment
Summary:
Fixes T8917. Prior to T2618, deleting inlines prompted users, then really deleted the rows.

After T2618, we delete immediately and offer "Undo". However, some interactions with drafts were missed, and we were only clearing the "this revision has a draft" flag on one of the delete pathways (when you delete all the comment text, then save the comment).

Make both the "Delete" action and the "Delete All Comment Text + Save" workflows do the same thing: mark the row as deleted, and clear any relevant drafts.

Test Plan:
  - Made an inline comment on a clean revision with no "draft comments" marker in the list view.
  - Used "delete" to delete it.
  - After applying the patch, verified that no "draft commetns" marker appears in the list view.
  - Used Delete and Edit + Remove Text + Save to delete comments in Differential and Diffusion.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T8917

Differential Revision: https://secure.phabricator.com/D13665
2015-07-21 11:36:46 -07:00
epriestley
0306eb70ed Group and order Herald fields
Summary:
Ref T8726. Some adapters now have a large number of fields, and we lost the sort-of-human-readable implicit ordering when fields were modularized.

Instead, group and sort fields.

Test Plan: {F603066}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T8726

Differential Revision: https://secure.phabricator.com/D13619
2015-07-16 14:13:13 -07:00