1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2025-01-22 12:41:19 +01:00
Commit graph

395 commits

Author SHA1 Message Date
epriestley
843bfb4fd8 Add a "commits" attachment to "differential.diff.search" for retrieving local commit information
Summary:
Ref T13124. See PHI593.

When you `arc diff` in a Git or Mercurial repository, we upload some information about the local commits in your working copy which the change was generated from.

In the future (for example, with T1508) we may increase the prominence of this feature.

Provide a stable way to read this information back via the API. This roughly mirrors the information we provide about commits in "diffusion.commit.search", although the latter is less fleshed-out today.

Test Plan: Used `differential.diff.search` to retrieve commit information about Git, Mercurial, and Subversion diffs. (There's no info for Subversion, but it doesn't crash or anything.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13124

Differential Revision: https://secure.phabricator.com/D19386
2018-04-19 17:25:06 -07:00
epriestley
6556536d06 Allow repository cluster bindings to be marked as not "writable", making them read-only
Summary:
Depends on D19356. Fixes T10883. Ref T13120.

  - Add a "writable" property to the bindings, defaulting to "true" with a nice dropdown.
  - When selecting hosts, allow callers to request a writable host.
  - If the caller wants a writable host, only return hosts if they're writable.
  - In SVN and Mercurial, we sometimes return only writable hosts when we //could// return read-only hosts, but figuring out if these request are read-only or read-write is currently tricky. Since these repositories can't really cluster yet, this shouldn't matter too much today.

Test Plan:
  - Without any config changes, viewed repositories via web UI and pushed/pulled via SSH and HTTP.
  - Made all nodes in the cluster read-only by disabling "writable", pulled and hit the web UI (worked), tried to push via SSH and HTTP (got errors about read-only).
  - Put everything back, pulled and pushed.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T10883

Differential Revision: https://secure.phabricator.com/D19357
2018-04-12 16:10:36 -07:00
epriestley
7c7e6d555b Give getAlmanacServiceURI() an "options" parameter to prepare for read-only devices
Summary:
Depends on D19355. Ref T10883. Ref T13120. Rather than adding a million parameters here, wrap the selector-parameters in an `$options`.

The next change adds a new "writable" option to support forcing selection of writable hosts.

Test Plan: Pulled and pushed via HTTP and SSH, viewed repositories via Diffusion.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13120, T10883

Differential Revision: https://secure.phabricator.com/D19356
2018-04-12 16:10:12 -07:00
epriestley
09c6d42b95 Mostly make blame work with DocumentEngine
Summary: Ref T13105. This needs refinement but blame sort of works again, now.

Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.

Reviewers: mydeveloperday

Reviewed By: mydeveloperday

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19309
2018-04-09 04:48:21 -07:00
epriestley
1fde4a9450 Move Diffusion browse rendering to DocumentEngine, breaking almost all features
Summary:
Ref T13105. This breaks about 9,000 features but moves Diffusion to DocumentEngine for rendering. See T13105 for a more complete list of all the broken stuff.

But you can't bake a software without breaking all the features every time you make a change, right?

Test Plan: Viewed various files in Diffusion, used DocumentEngine features like highlighting and rendering engine selection.

Reviewers: mydeveloperday

Reviewed By: mydeveloperday

Subscribers: mydeveloperday

Maniphest Tasks: T13105

Differential Revision: https://secure.phabricator.com/D19302
2018-04-09 04:46:26 -07:00
epriestley
51461f18c1 When publishing buildables in Differential, ignore autobuilds (local lint and unit)
Summary:
Depends on D19280. Ref T13110. Although Harbormaster cares about all builds, Differential does not practically care about local lint and unit results in determining build status.

In Differential, orient publishing around "remote builds" instead of "builds".

This does not yet change any of the draft logic, it just makes the timeline story use newer logic.

Test Plan: Used `bin/harbormaster publish` (with some guard-clause removal) to publish some buildables to revisions without anything crashing.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19281
2018-04-03 11:02:12 -07:00
epriestley
95c9d403f4 Make objects implementing BuildableInterface produce a BuildableEngine
Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.

I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.

Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.

I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.

Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.

Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13110

Differential Revision: https://secure.phabricator.com/D19278
2018-04-03 10:57:51 -07:00
epriestley
f583406ba9 Drop uniqueness constraint on PushEvent request ID
Summary: See <https://discourse.phabricator-community.org/t/pushing-to-mercurial-repository-fails/1275/1>. Mercurial may invoke hooks multiple times per push.

Test Plan: Pushed to Mercurial, saw key constraint failure.

Differential Revision: https://secure.phabricator.com/D19257
2018-03-26 07:02:15 -07:00
epriestley
df3c937dab Record lock timing information on PushEvents
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:

  - `writeWait`: Time spent waiting for a write lock.
  - `readWait`: Time spent waiting for a read lock.
  - `hostWait`: Roughly, total time spent on the leaf node.

The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.

Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".

Maniphest Tasks: T13109

Differential Revision: https://secure.phabricator.com/D19250
2018-03-22 13:46:01 -07:00
epriestley
69bff489d4 Generate a random unique "Request ID" for SSH requests so processes can coordinate better
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.

The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.

Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.

Maniphest Tasks: T13109

Differential Revision: https://secure.phabricator.com/D19249
2018-03-22 13:44:30 -07:00
epriestley
859b274970 Provide more information to users during git push while waiting for write locks
Summary:
Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting.

While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them.

Test Plan:
Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance:

> # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)...

Reviewers: asherkin

Reviewed By: asherkin

Subscribers: asherkin

Maniphest Tasks: T13109

Differential Revision: https://secure.phabricator.com/D19247
2018-03-22 13:42:18 -07:00
epriestley
c64aae052f Make sure auditors are attached to commits on new pathways
Companion change to D19022 for commits. Mentioning and subscribing to commits
can load them without audit data.
2018-02-09 17:09:00 -08:00
epriestley
1cd3a59378 When users resign from revisions, stop expanding projects/packages to include them
Summary:
Depends on D19019. Ref T13053. Fixes T12689. See PHI178.

Currently, if `@alice` resigns from a revision but `#alice-fan-club` is still a subscriber or reviewer, she'll continue to get mail. This is undesirable.

When users are associated with an object but have explicitly disengaged in an individal role (currently, only resign in audit/differential) mark them "unexpandable", so that they can no longer be included through implicit membership in a group (a project or package).

`@alice` can still get mail if she's a explicit recipient: as an author, owner, or if she adds herself back as a subscriber.

Test Plan:
  - Added `@ducker` and `#users-named-ducker` as reviewers. Ducker got mail.
  - Resigned as ducker, stopped getting future mail.
  - Subscribed explicitly, got mail again.
  - (Plus some `var_dump()` sanity checking in the internals.)

Reviewers: amckinley

Maniphest Tasks: T13053, T12689

Differential Revision: https://secure.phabricator.com/D19021
2018-02-08 06:29:13 -08:00
epriestley
75bc86589f Add date range filtering for activity, push, and pull logs
Summary: Ref T13049. This is just a general nice-to-have so you don't have to export a 300MB file if you want to check the last month of data or whatever.

Test Plan: Applied filters to all three logs, got appropriate date-range result sets.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18970
2018-01-30 15:36:22 -08:00
epriestley
5b22412f24 Support data export on push logs
Summary: Depends on D18967. Ref T13049. Nothing too fancy going on here.

Test Plan: Exported push logs, looked at the export, seemed sensible.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13049

Differential Revision: https://secure.phabricator.com/D18968
2018-01-30 11:19:20 -08:00
epriestley
778dfff277 Make minor correctness and display improvements to pull logs
Summary:
Depends on D18915. Ref T13046.

  - Distinguish between HTTP and HTTPS.
  - Use more constants and fewer magical strings.
  - For HTTP responses, give them better type information and more helpful UI behaviors.

Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

Differential Revision: https://secure.phabricator.com/D18917
2018-01-23 14:13:18 -08:00
epriestley
e6a9db56a9 Add a basic view for repository pull logs
Summary:
Depends on D18912. Ref T13046. Add a UI to browse the existing pull log table.

The actual log still has some significant flaws, but get the basics working.

Test Plan: {F5391909}

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13046

Differential Revision: https://secure.phabricator.com/D18914
2018-01-23 14:10:10 -08:00
epriestley
753c4c5ff1 Remove the "PhabricatorRepositoryVCSPassword" class and table
Summary:
Ref T13043. After D18898, this has been migrated to new, more modern storage and no longer has any readers or writers.

One migration from long ago (early 2014) is affected. Since this is ancient and the cost of dropping this is small (see inline), I just dropped it.

I'll note this in the changelog.

Test Plan: Ran migrations, got a clean bill of health from `storage status`. Grepped for removed symbol.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13043

Differential Revision: https://secure.phabricator.com/D18899
2018-01-23 10:56:37 -08:00
epriestley
53b25db918 Prevent enormous changes from being pushed to repositoires by default
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".

If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.

Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.

Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.

Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.

Reviewers: amckinley

Reviewed By: amckinley

Subscribers: joshuaspence

Maniphest Tasks: T13031

Differential Revision: https://secure.phabricator.com/D18850
2018-01-04 10:02:29 -08:00
epriestley
e34b4bbd90 Move the Git LFS gate to dedicated (non-prototype) config
Summary: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible).

Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T7789

Differential Revision: https://secure.phabricator.com/D18825
2017-12-18 09:12:22 -08:00
Dmitri Iouchtchenko
9bd6a37055 Fix spelling
Summary: Noticed a couple of typos in the docs, and then things got out of hand.

Test Plan:
  - Stared at the words until my eyes watered and the letters began to swim on the screen.
  - Consulted a dictionary.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D18693
2017-10-09 10:48:04 -07:00
epriestley
8982e3e52d Update major RefCursor callsites to work properly with RefPosition
Summary:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.

I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.

Test Plan:
  - This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
  - Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
  - Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
  - Browed around Diffusion in various repositories.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18614
2017-09-15 10:21:32 -07:00
epriestley
5cf62f86d7 Remove obsolete columns from RefCursor table
Summary:
Ref T11823. This change isn't standalone, but prepares for the more involved code change by dropping obsolete columns from the RefCursor table and adding the unique key we need to prevent the ambiguous/duplicate refs issue.

This data was moved to the RefPosition table in D18612.

Test Plan: Ran storage upgrade. See next revision for more substantial testing of this change series.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18613
2017-09-15 10:21:12 -07:00
epriestley
9d5a2b3b4f Add a RefPosition table to hold branch/tag positions once the RefCursor table is split
Summary:
Ref T11823. Currently, we have a "RefCursor" table which stores rows like `<branch or tag name, commit it is pointing at>` with some more data.

Because Mercurial can have a single branch pointing at several different places, this table must allow multiple rows with the same branch or tag name.

Among other things, this means there isn't a single PHID which can be used to identify a branch name in a stable way. However, we have several UIs where we want to be able to do this.

Some specific examples where we run into trouble: in Mercurial, if there are 5 heads for "default", that means there are 5 phids. And currently, if someone deletes a branch, we lose the PHID for it. Instead, we'd rather retain it so the whole world doesn't break if you accidentally delete a branch and then fix it a little later.

(I'll likely hold this until the rest of the logic is fleshed out a little more in followup changes.)

Test Plan: Ran `bin/storage upgrade`, saw the table get created without warnings.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T11823

Differential Revision: https://secure.phabricator.com/D18602
2017-09-15 10:19:17 -07:00
epriestley
b1b638bd14 Support the Ferret engine in Diffusion
Summary: Ref T12819. More ferret engine support.

Test Plan: Indexed and searched commits and repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12819

Differential Revision: https://secure.phabricator.com/D18572
2017-09-07 13:41:04 -07:00
epriestley
ab018e1b49 When destorying a repository, print a notification about removing the working copy
Summary:
Fixes T12946. `bin/remove destroy` does not remove working copies: it's more dangerous than usual, and we can't do it in the general (clustered) case.

Print a notification message after destroying a repository.

Test Plan:
  - Destroyed a repository, got a hint about the working copy.
  - Destroyed a task, things worked normally.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12946

Differential Revision: https://secure.phabricator.com/D18313
2017-08-01 08:57:39 -07:00
Chad Little
a6b550ba03 Move Clone Repository to Dialog
Summary: This moves the clone details on the Repository Home to a button / dialog. Functionally this is to pull content on the page way up, while giving full space to all the clone options. I think we can build this into some FancyJS if needed, but this seems to clean ui the UI dramatically with little overhead. I don't want to attempt the JS dropdown unless we're sure that's the best path (it exposes the most common URI by default, saving a click).

Test Plan: Tested hg, svn, git repositories and the raw URL page. Test close button.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Differential Revision: https://secure.phabricator.com/D18203
2017-07-11 13:16:47 -07:00
Chad Little
d0898116d8 Add a graph view page to Diffusion
Summary: Fixes T12840. This adds a parallel "graph" button next to history on home and on the history list page. I'll think more about better placement of how to get to this page with the upcoming redesign that's still sitting in Pholio.

Test Plan: View History, View Graph, Try pager, go to a file, click view history, see no graph button.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12840

Differential Revision: https://secure.phabricator.com/D18131
2017-06-19 17:57:20 +02:00
Chad Little
83a89166ee Add profile images to Repositories
Summary: Builds out some images to use to identify repositories. Fixes T12825.

Test Plan:
Try setting custom, built in, and null images.

{F4998175}

{F4998192}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin

Maniphest Tasks: T12825

Differential Revision: https://secure.phabricator.com/D18116
2017-06-12 07:51:39 -07:00
epriestley
0d5538672c Detect unsynchronizable repositories on multiple cluster hosts
Summary:
Ref T12613. Currently, the SVNTEST and HGTEST repositories are improperly configured on `secure`. These repositories use VCS systems which do not support synchronization, so they can not be served from cluster services with multiple hosts.

However, I've incorrectly configured them the same way as all the Git repositories, which support synchronization. This causes about 50% of requests to randomly fail (when they reach the wrong host).

Detect this issue and warn the user that the configuration is not valid.

It should be exceptionally difficult for normal installs to run into this.

Test Plan:
  - Mostly faked these conditions locally, verified that `secure` really has this configuration.
  - I'll push this, verify that the issue is detected correctly in production, then fix the config which should resolve the intermittent issues with SVNTEST.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T12613

Differential Revision: https://secure.phabricator.com/D17774
2017-04-24 10:43:05 -07:00
Mukunda Modell
9e2f263bb4 Add repositories to fulltext search index.
Summary:
This implements a simplistic `PhabricatorRepositoryFulltextEngine`
Currently only the repository name, description, timestamps and
status are indexed.

Note: I had to change the `search index` workflow to disambiguate
PhabricatorRepository from PhabricatorRepositoryCommit

Test Plan:
* ran `./bin/search index --type PhabricatorRepository --force`
 * searched for some repositories. Saw reasonable results matching on either title or description.
* Edited a repository in the web ui
 * Added unique key words to the repo description.
 * I was then able to find that repo by searching for the new keywords.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin

Tags: #search, #diffusion

Differential Revision: https://secure.phabricator.com/D17300
2017-03-28 07:58:22 +00:00
Jakub Vrana
a778151f28 Fix errors found by PHPStan
Test Plan: Ran `phpstan analyze -a autoload.php phabricator/src`.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D17371
2017-02-17 10:10:15 +00:00
epriestley
bd9e54b621 Navigage Buildkite builds with more nuance
Summary:
Ref T12173.

  - If we want to fetch a tag, Buildkite needs it as a "branch" (this means more like "ref to fetch").
  - The API gets upset if we pass "refs/tags/...", so just pass the tag name without the prefix, which works.
  - Do a better job with commits and pass a real branch to fetch.

Test Plan:
  - Built a commit with Buildkite.
  - Build a revision with Buildkite.

Reviewers: chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T12173

Differential Revision: https://secure.phabricator.com/D17282
2017-01-31 17:26:45 -08:00
epriestley
5e7a091737 Write an explicit edge for commit membership in packages
Summary:
Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit.

This auditor is used to power the "Commits in this package" query in Owners.

This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable.

Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical!

I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners.

Test Plan:
  - Ran `bin/audit update-owners` with various arguments.
  - Viewed packages in web UI, saw them load the proper commits.
  - Queried by packages in Diffusion explicitly.
  - Clicked the "View All" link in Owners and got to the right search UI.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17264
2017-01-30 15:23:34 -08:00
epriestley
97cac83e9b Add a "Needs Verification" state to Audit
Summary:
Fixes T2393. This allows authors to explicitly say "I think I fixed everything, please accept my commit now thank you".

Also improves behavior of "re-accept" and "re-reject" after new auditors you have authority over get added.

Test Plan:
  - Kicked a commit back and forth between an author and auditor by alternately using "Request Verification" and "Raise Concern".
  - Verified it showed up properly in bucketing for both users.
  - Accepted, added a project, accepted again (works now; didn't before).
  - Audited on behalf of projects / packages.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2393

Differential Revision: https://secure.phabricator.com/D17252
2017-01-25 13:08:59 -08:00
epriestley
ca182c7f48 Clean up "Audit Authority" code, at least mostly
Summary:
Ref T2393. We had three copies of this code ("which packages/projects can a user accept on behalf of?"). I removed one in D17250. This consolidates the other two.

This still isn't perfect and it should probably live in a Query or something some day, but there's some weird stuff going on with the viewer in the editor context, and at least the code handles the viewer correctly now and isn't living somewhere weird and totally unrelated to auditing, and the callsites don't need to do a bunch of extra work.

This also moves towards fixing the "re-accept if you've already accepted but then a new package you have authority over was added" bug, which we fixed recently in Differential. This should be less common in Audit, but should still be fixed.

Test Plan: Viewed and audited commits with a mixture of user, package, and project auditors. Saw actions apply to the expected set of auditors.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T2393

Differential Revision: https://secure.phabricator.com/D17251
2017-01-25 13:08:25 -08:00
epriestley
545dad319e Add an "Auditors" rule for Commits
Summary: Fixes T5889. You can't write a rule like "if no other Herald rules did anything...", but you can use this rule to check for Owners or an explicit "Auditors" field doing things.

Test Plan: Using the test console, ran an "Auditors" rule against a commit with and without an auditor. Got expected pass/fail outcomes.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T5889

Differential Revision: https://secure.phabricator.com/D17221
2017-01-18 10:05:30 -08:00
epriestley
903e37a21b Show yellow "draft" bubble in Audit
Summary: Fixes T6660. Uses the new stuff in Audit to build an EditEngine-aware icon.

Test Plan: {F2364304}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T6660

Differential Revision: https://secure.phabricator.com/D17208
2017-01-16 10:28:59 -08:00
epriestley
19525ed81a Add diffusion.commit.search Conduit API method
Summary: Ref T10978. This is bare bones, but the SearchEngine is at least mostly in reasonable shape now, so get it in place and freeze the old stuff. I previously froze `audit.query`, which did much the same thing.

Test Plan: Issued some queries with the API, technically got results back.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978

Differential Revision: https://secure.phabricator.com/D17194
2017-01-12 13:23:29 -08:00
epriestley
c05cb1ba6d Make "Audit Requested" put commits into the "Needs Audit" state
Summary: Fixes T7504. I think that task legitimately describes a bug and that the current behavior is counterintuitive.

Test Plan: Manually added an auditor to a commit with none; saw it become "Audit Required" as an overall state.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T7504

Differential Revision: https://secure.phabricator.com/D17185
2017-01-11 14:59:16 -08:00
epriestley
255e3fb1e4 Allow auditors to be added and removed from commits in a modern way
Summary: Ref T10978. Ref T7676. Make auditors work more like reviewers, so they can be freely added or removed.

Test Plan:
  - Interacted with auditors via "Edit Commit" and API.
  - Comment area is still oldschool and doesn't work yet.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T10978, T7676

Differential Revision: https://secure.phabricator.com/D17181
2017-01-11 13:56:34 -08:00
epriestley
ef05bf335d Allow Harbormaster builds to publish to a different object
Summary:
Fixes T9276. Fixes T8650. The story so far:

  - We once published build updates to Revisions.
  - An unrelated fix (D10911) sent them to the Diffs instead of Revisions, which isn't useful, since you can't see a diff's timeline anywhere.
  - This also caused a race condition, where the RevisionEditor and DiffEditor would update the diff simultaneously (T8650).
  - The diff update was just disabled to avoid the race (part of D13441).
  - Instead, allow the updates to go somewhere else. In this case, we send commit updates to the commit but send diff updates to the revision so you can see 'em.
  - Since everything will be using the revision editor now, we should either get proper lock behavior for free or it should be easy to add if something whack is still happening.
  - Overall, this should pretty much put us back in working order like we were before D10911.

This behavior is undoubtedly refinable, but this should let us move forward.

Test Plan:
Saw a build failure in timeline:

{F2304575}

Reviewers: chad

Reviewed By: chad

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T9276, T8650

Differential Revision: https://secure.phabricator.com/D17139
2017-01-04 13:46:39 -08:00
epriestley
e6ddd6d0e9 Cache Almanac URIs for repositories
Summary:
Ref T11954. This is kind of complex and I'm not sure I want to actually land it, but it gives us a fairly good improvement for clustered repositories so I'm leaning toward moving forward.

When we make (or receive) clustered repository requests, we must first load a bunch of stuff out of Almanac to figure out where to send the request (or if we can handle the request ourselves).

This involves several round trip queries into Almanac (service, device, interfaces, bindings, properties) and generally is fairly slow/expensive. The actual data we get out of it is just a list of URIs.

Caching this would be very easy, except that invalidating the cache is difficult, since editing any binding, property, interface, or device may invalidate the cache for indirectly connected services and repositories.

To address this, introduce `PhabricatorCacheEngine`, which is an extensible engine like `PhabricatorDestructionEngine` for propagating cache updates. It has two modes:

  - Discover linked objects (that is: find related objects which may need to have caches invalidated).
  - Invalidate caches (that is: nuke any caches which need to be nuked).

Both modes are extensible, so third-party code can build repository-dependent caches or whatever. This may be overkill but even if Almanac is the only thing we use it for it feels like a fairly clean solution to the problem.

With `CacheEngine`, make any edit to Almanac stuff propagate up to the Service, and then from the Service to any linked Repositories.

Once we hit repositories, invalidate their caches when Almanac changes.

Test Plan:
  - Observed a 20-30ms performance improvement with `ab -n 100`.
  - (The main page making Conduit calls also gets a performance improvement, although that's a little trickier to measure directly.)
  - Added debugging code to the cache engine stuff to observe the linking and invalidation phases.
  - Made invalidation throw; verified that editing properties, bindings, etc, properly invalidates the cache of any indirectly linked repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11954

Differential Revision: https://secure.phabricator.com/D17000
2016-12-06 09:14:45 -08:00
epriestley
fc1adf9875 Modernize UI for "Compare" in Diffusion
Summary: Ref T929. We've made some UI updates since D15330.

Test Plan: {F2079125}

Reviewers: avivey, chad

Reviewed By: chad

Maniphest Tasks: T929

Differential Revision: https://secure.phabricator.com/D16990
2016-12-05 18:10:11 -08:00
epriestley
005d8493b0 Pass GIT_ENVIRONMENTAL_MAGIC through to hook subprocesses to support Git 2.11.0
Summary:
Fixes T11940. In 2.11.0, Git has made a change so that newly-pushed changes are held in a temporary area until the hook accepts or rejects them.

This magic temporary area is only readable if the appropriate `GIT_ENVIRONMENTAL_MAGIC` variables are available. When executing `git` commands, pass them through from the calling context.

We're intentionally conservative about which variables we pass, and with good reason (see "httpoxy" in T11359). I think this continues to be the correct default behavior.

Test Plan:
  - Upgraded to Git 2.11.0.
  - Tried to push over SSH, got a hook error.
  - Applied patch.
  - Pulled and pushed over SSH.
  - Pulled and pushed over HTTP.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11940

Differential Revision: https://secure.phabricator.com/D16988
2016-12-05 12:45:30 -08:00
epriestley
bf1cbc2499 Don't let users pick "whatever.git" as a repository short name, make "." work
Summary:
Fixes T11902.

  - Periods now work in short names.
  - If you try to name something ".git", no dice.

Test Plan:
  - Tried to name something "quack.git", was politely rejected.
  - Named something "quack.notgit", and it worked fine.
  - Cloned Mercurial and Git repositories over SSH with ".git" and non-".git" variants without hitting any issues.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11902

Differential Revision: https://secure.phabricator.com/D16908
2016-11-21 15:47:20 -08:00
epriestley
e6c82c0994 Fix an issue with generating browser URIs in an SVN repository
Summary: Fixes T11866. This got converted wrong when doing the `/source/` stuff.

Test Plan: Browsed the root directory of a Subversion repository in Diffusion.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11866

Differential Revision: https://secure.phabricator.com/D16860
2016-11-15 07:15:20 -08:00
epriestley
6a62fca950 Support slightly prettier repository URIs in Diffusion
Summary: Fixes T4245. When a repository has a short name, use `/source/shortname/` as its primary URI.

Test Plan:
  - Cloned Git repositories from shortnames via HTTP and SSH.
  - Cloned Mercurial repositories from shortnames via HTTP and SSH.
  - Cloned Subversion repositories from shortnames via SSH.
  - Browsed Git, Mercurial and Subversion repositories.
  - Added and removed short names to various repositories.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T4245

Differential Revision: https://secure.phabricator.com/D16851
2016-11-13 12:42:12 -08:00
epriestley
706c21375e Remove empty implementations of describeAutomaticCapabilities()
Summary:
This has been replaced by `PolicyCodex` after D16830. Also:

  - Rebuild Celerity map to fix grumpy unit test.
  - Fix one issue on the policy exception workflow to accommodate the new code.

Test Plan:
  - `arc unit --everything`
  - Viewed policy explanations.
  - Viewed policy errors.

Reviewers: chad

Reviewed By: chad

Subscribers: hach-que, PHID-OPKG-gm6ozazyms6q6i22gyam

Differential Revision: https://secure.phabricator.com/D16831
2016-11-09 15:24:22 -08:00
epriestley
5d1359d78f Fix an issue where repository message counts would never reset
Summary:
Fixes T11705. I did not realize that `ON DUPLICATE KEY UPDATE` was order-dependent, so the "reset" clause of this `IF(...)` never actually worked.

Reorder it so we check if we're changing the message type //first//, then actually change the message type.

This makes the count reset properly when a failing repository succeeds, or a working repository fails.

Test Plan:
  - On `master`, forced a working repository to fail a `bin/repository update`, saw the message change types (expected) but keep the old count (wrong!).
  - With this patch, repeated the process and saw the count reset properly.
  - Ran the patch, verified counts reset to 0.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11705

Differential Revision: https://secure.phabricator.com/D16623
2016-09-28 15:02:26 -07:00