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

4784 commits

Author SHA1 Message Date
epriestley
9f38aaa5de Add "raw author name" and "raw committer name" as Herald fields for commit content hooks
Summary:
Ref T4195. A legitimate rule which needs this field is "do not allow commits as root". Interestingly, we have exactly one commit as root in each Phabricator, Arcanist and libphutil.

Since the committer and author don't need to be Phabricator accounts (just the Pusher), the existing "Committer" and "Author" fields can't express this rule (they'll be empty).

Test Plan: {F93406}

Reviewers: btrahan

Reviewed By: btrahan

CC: SEJeff, aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7841
2013-12-27 13:16:00 -08:00
epriestley
ce78bf1de4 Make all bin/* scripts locate their workflows dynamically
Summary:
Ref T2015. Not directly related to Drydock, but I bumped into this. All these scripts currently enumerate their workflows explicitly.

Instead, use `PhutilSymbolLoader` to automatically discover workflows. This reduces code duplication and errors (see all the bad `extends` this diff fixes) and lets third parties add new workflows (not clearly valuable?).

Test Plan: Ran `bin/x help` for each modified script.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7840
2013-12-27 13:15:48 -08:00
epriestley
e397103bf2 Extend all "ManagementWorkflow" classes from a base class
Summary:
Ref T2015. Not directly related to Drydock, but I've wanted to do this for a bit.

Introduce a common base class for all the workflows in the scripts in `bin/*`. This slightly reduces code duplication by moving `isExecutable()` to the base, but also provides `getViewer()`. This is a little nicer than `PhabricatorUser::getOmnipotentUser()` and gives us a layer of indirection if we ever want to introduce more general viewer mechanisms in scripts.

Test Plan: Lint; ran some of the scripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7838
2013-12-27 13:15:40 -08:00
epriestley
1ba52fac86 Introduce DrydockQuery to slightly reduce code duplication
Summary: Ref T2015. All the Drydock query classes share the application method; move it into a shared base class to slightly shrink the codebase.

Test Plan: Browsed query UIs.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7837
2013-12-27 13:15:30 -08:00
epriestley
9b0fa5747b Make Drydock more broadly aware of policies
Summary:
Ref T2015. Moves a bunch of raw object loads into modern policy-aware queries.

Also straightens out the Log and Lease policies a little bit: there are legitimate states where these objects are not attached to a resource (particularly, while a lease is being acquired). Handle these more gracefully.

Test Plan: Lint / browsed stuff.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7836
2013-12-27 13:15:19 -08:00
epriestley
1650874004 Modernize Drydock CLI management of task execution
Summary:
Ref T2015. Currently, Drydock has a `wait-for-lease` workflow which is invoked in the background by the `lease` workflow.

The goal of this mechanism is to allow `bin/drydock lease` to print out logs as the lease is acquired. However, this predates the `runAllTasksInProcess` flags, and they provide a simpler and more robust way (potentially with `--trace` and `PhutilConsole`) to do synchronous execution and debug logging.

Simplify this whole mechanism: just run everything in-process in `bin/drydock lease`, and do logging via `--trace`. We could thread a `PhutilConsole` through things too, but this seems good enough for now.

Also various cleanup/etc.

Test Plan: Ran `bin/drydock lease`. Ran `bin/harbormaster build X --plan Y`, for `Y` being a Drydock-dependent build plan.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7835
2013-12-27 13:15:12 -08:00
epriestley
536c606dde Implment ApplicationTransaction grouping rules
Summary: Ref T4266. This implements rules similar to the old rules. With D7842, maybe this is reasonable? I think it's not like grotesquely bad, at least.

Test Plan: See screenshot.

Reviewers: chad, wrotte

Reviewed By: chad

CC: aran

Maniphest Tasks: T4266

Differential Revision: https://secure.phabricator.com/D7843
2013-12-27 05:51:15 -08:00
epriestley
5fdd800bb6 Provide a Drydock "Console" controller
Summary:
Ref T2015. After introducing ApplicationSearch, the left nav turned into a soupy mess. Split the major sections into four separate areas, and unify them with a simple console.

This also reverts all the prefix stuff, since the results were awful and I don't anticipate it ever being the best solution to any UX problem.

Test Plan:
Browsed blueprints, resources, leases and logs.

Here's the new console:

{F93279}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7833
2013-12-26 12:30:36 -08:00
epriestley
9c9a9a919e Use ApplicationSearch for DrydockLog
Summary: Ref T2015. Move logs over to ApplicationSearch.

Test Plan: Browsed logs in UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7832
2013-12-26 12:30:29 -08:00
epriestley
3db5833622 Make DrydockLog policy-aware
Summary: Ref T2015. Update DrydockLog for policy awareness and give it a policy query.

Test Plan: Browsed all the log interfaces.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7831
2013-12-26 12:30:17 -08:00
epriestley
bc3912e641 Use ApplicationSearch for Drydock Resources
Summary: Ref T2015. Bring ApplicationSearch to Resources, too.

Test Plan: Browsed/queried resources in UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7830
2013-12-26 12:30:10 -08:00
epriestley
fa00e86f87 Use ApplicationSearch for Drydock Blueprints
Summary: Ref T2015. This turns the side nav into a bigger mess for now, but uses ApplicationSearch for blueprints.

Test Plan: Queried blueprints in the UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7829
2013-12-26 12:30:04 -08:00
epriestley
1a82743491 Make Drydock Lease and Resource PHIDs use newer PHID infrastructure
Summary:
Ref T2015. These never got updated to the new stuff, move them out of the old `Constants` class and let them load handles, etc.

Also some half-cleanup of some Blueprint/BlueprintImplementation stuff.

Test Plan: Used `phid.query` to query a Resource, Lease, and Blueprint.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7828
2013-12-26 12:29:58 -08:00
epriestley
f9f70dc2e9 Use correct method name in Harbormaster UI event listener
Summary: See thread; fixes fatal. The actual name of this method is `getHarbormaster...`.

NOTE: This fixes a fatal in Differential which impedes review, so I'm pushing it as-is.

Test Plan: Browsed a revision.

Reviewers: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7834
2013-12-26 11:37:25 -08:00
epriestley
70244d3a12 Use ApplicationSearch to manage DrydockLeases
Summary:
Ref T2015. Applies ApplicationSearch to DrydockLease.

This makes the left nav in Drydock a little funky. It will probably get worse for a bit before it gets better, since I want to bring everything to ApplicationSearch and then sort out the details.

Test Plan: Queried leases in Drydock.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7827
2013-12-26 10:42:00 -08:00
epriestley
6b2d480fe7 Make DrydockLease a policy-aware object
Summary: Ref T2015. DrydockLease predates widespread adoption of policies. Make it -- and its query -- policy aware.

Test Plan: Browsed leases from the web UI. Grepped for callsites.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Maniphest Tasks: T2015

Differential Revision: https://secure.phabricator.com/D7826
2013-12-26 10:41:36 -08:00
epriestley
aad6b57c36 Add bin/harbormaster to make builds easier to debug
Summary:
Ref T1049. Adds `bin/harbormaster` and `bin/harbormaster build` for applying plans from the console. Since this gets `--trace`, it's much easier to debug what's going on.

This doesn't work properly with some of the Drydock steps yet, I need to look at those. I think `setRunAllTasksInProcess` probably obsoletes some of the mechanisms. It might also not work with "Wait for Builds" but I didn't check.

Test Plan: Used `bin/harbormaster` to run a bunch of builds. Ran builds from web UI.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7825
2013-12-26 10:40:52 -08:00
epriestley
ac19c55822 Formalize "manual" buildables in Harbormaster
Summary:
Ref T1049. Generally, it's useful to separate test/trial/manual runs from production/automatic runs.

For example, you don't want to email a bunch of people that the build is broken just because you messed something up when writing a new build plan. You'd rather try it first, then promote it into production once you have some good runs.

Similarly, test runs generally should not affect the outside world, etc. Finally, some build steps (like "wait for other buildables") may want to behave differently when run in production/automation than when run in a testing environment (where they should probably continue immediately).

So, formalize the distinction between automatic buildables (those created passively by the system in response to events) and manual buildables (those created explicitly by users). Add filtering, and stop the automated parts of the system from interacting with the manual parts (for example, we won't show manual results on revisions).

This also moves the "Apply Build Plan" to a third, new home: instead of the sidebar or Buildables, it's now on plans. I think this generally makes more sense given how things have developed. Broadly, this improves isolation of test environments.

Test Plan: Created some builds, browsed around, used filters, etc.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7824
2013-12-26 10:40:43 -08:00
epriestley
4a56e26a67 Allow buildables to be queried by container and underlying buildable
Summary: Ref T1049. Adds "Repository", "Revision", "Diff" and "Commit" as searchable fields.

Test Plan: Used all the fields to filter things.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7823
2013-12-26 10:40:34 -08:00
epriestley
60288edf80 Allow Harbormaster build plans to be disabled
Summary: Fixes T4187. Ref T1049. Allows build plans to be enabled or disabled.

Test Plan: Enabled and disabled build plans.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4187, T1049

Differential Revision: https://secure.phabricator.com/D7822
2013-12-26 10:40:22 -08:00
epriestley
adcc4ee1db Add a "branches" rule for Herald commit rules
Summary:
Fixes T4195. Allows you to write a rule against a commit's branches.

This completes outstanding work on T4195.

Test Plan: Pushed to Git and Mercurial repositories and verified branches were selected correctly by examining transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7820
2013-12-26 10:40:16 -08:00
epriestley
44c9a94abe Make conduit certificate readonly and select-on-click
Summary: See comments in <https://secure.phabricator.com/D6331#comment-3> -- make the Conduit Token and Conduit Certificate interfaces readonly and select-on-click.

Test Plan:
  - Viewed `/conduit/token/`, verified it was readonly and selected on click.
  - Viewed `/settings/panel/conduit/`, likewise.

Reviewers: Avish, btrahan, wotte

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7819
2013-12-23 10:43:53 -08:00
epriestley
81dcf6378d Make repository pull install hooks the first time
Summary:
Ref T4257. Currently, the pull logic looks like this:

  if (new) {
    create();
  } else {
    if (hosted) {
      install_hooks();
    } else {
      update();
    }
  }

This means that the first time you run `repository pull`, hooks aren't installed, which makes debugging trickier. Instead, reorganize the logic:

  if (new) {
    create();
  } else {
    if (!hosted) {
      update();
    }
  }

  if (hosted) {
    install_hooks();
  }

Test Plan: Ran `bin/repository pull` on a new `hg` repo and got hooks installed immediately.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4257

Differential Revision: https://secure.phabricator.com/D7818
2013-12-23 10:43:49 -08:00
epriestley
6daa2b6c2e Fix a commit hook issue with the initial commit to Mercurial repositories
Summary:
Fixes T4257. The `hg heads` command exits with an error code and no output in an empty repository.

Just ignore the error code: we don't have a great way to distinguish between errors, and we ran another `hg` command moments before, so we have at least some confidence it isn't a PATH sort of thing.

Test Plan: Created a new Mercurial repository and pushed to hit the error in T4257. Applied this fix and got a clean push with an accurate push log.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4257

Differential Revision: https://secure.phabricator.com/D7817
2013-12-23 10:43:45 -08:00
epriestley
d9a04fcb53 Minor, make sure we release locks in pull daemon if we fail in unusual ways
Summary: A user is reporting a re-lock in this daemon, which I can't
reproduce, but might be possible if this throws. Stop it from throwing in
a way which evades unlock.

See: <https://github.com/facebook/phabricator/issues/476>

Auditors: btrahan
2013-12-22 08:51:07 -08:00
xiaogaozi
54a0dd8139 Herald - add support for "assignee" conditions
Summary: add support for "assignee" conditions

Test Plan: Create a Herald rule where condition is assignee, and create a task assign to someone.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7813
2013-12-22 08:47:56 -08:00
epriestley
e27bbb9aa5 Enable the "Accepted Differential Revision" field for Herald post-commit hooks
Summary: I implemented this field, but didn't actually enable it.

Auditors: btrahan
2013-12-21 11:11:52 -08:00
epriestley
c462713584 Minor cleanup for task rendering in Daemons
Summary:
Fixes two issues:

  - When rendering a task's details, we currently issue a policy-oblivious query. Instead, issue a policy-aware query.
  - The formatting is a little bit weird, with the top half in a box and the bottom half with an older style. Make them consistent.

Test Plan: Looked at the detail pages for several tasks in queue.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Differential Revision: https://secure.phabricator.com/D7812
2013-12-20 18:02:32 -08:00
epriestley
dd3ed6fdd8 Fix breadcrumb issue on Macro Create page
Summary: uhoh

Test Plan: !!!

Reviewers: frgtn, Korvin, btrahan

Reviewed By: Korvin

CC: aran

Differential Revision: https://secure.phabricator.com/D7811
2013-12-20 14:06:21 -08:00
epriestley
a64d127e25 Add "is merge commit" Herald field for pre-commit rules
Summary:
Ref T4195. This allows you to write rules which disallow merge commits.

Also make the reject message a little more useful.

Test Plan:
  remote: This push was rejected by Herald push rule H27.
  remote: Change: commit/daed0d448404
  remote:   Rule: No Merges
  remote: Reason: No merge commits allowed. If you must push a merge, include "@force-merge" in the commit message.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7809
2013-12-20 12:39:40 -08:00
epriestley
9c938701c3 Modernize Diffusion commitparentsquery
Summary: Ref T4195. Ref T2783. We have an old-school implementation of this; move it into a LowLevel query and make callers all run through Conduit. I need the LowLevel query for hooks, to implement an "is merge commit" Herald rule.

Test Plan:
  - Ran query via Conduit for SVN, Mercurial, Git.
  - Parsed a commit which closed a revision, attach/closed worked correctly.
  - Browsed Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195, T2783

Differential Revision: https://secure.phabricator.com/D7808
2013-12-20 12:39:21 -08:00
epriestley
72c73d644b Add an "Accepted Differential revision" field to Commit and pre-commit Content Herald rules
Summary: Refs T4195. Fixes T3936. You can't currently write rules like "block commits unless they're attached to an **accepted** revision"; allow that.

Test Plan: Pushed commits into a rule with this field, saw it work / not crash.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, mbishopim3

Maniphest Tasks: T3936, T4195

Differential Revision: https://secure.phabricator.com/D7807
2013-12-20 12:39:13 -08:00
epriestley
2436458b90 Implement "Differential Revision" fields in Herald pre-commit content adapter
Summary: Ref T4195. Allows you to write revision-based commit hooks, e.g. block all commits with no corresponding revision.

Test Plan:
Here's are the fields populating:

{F90989}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7806
2013-12-20 12:39:01 -08:00
epriestley
d7c4edab28 Move commit message/metadata field query to a separate class
Summary: Ref T4195. I need to query commit metadata to figure out which revision a commit is associated with. Move this out of the MessageParser so the code can be called from the HookEngine.

Test Plan: Used `reparse.php` to reparse a variety of SVN, Mercurial and Git commits. Used `var_dump()` to verify sensible fields were returned.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7805
2013-12-20 12:38:44 -08:00
epriestley
23332241b2 Move commit hash querying to DiffusionLowLevelCommitQuery
Summary: Ref T4195. I need this for the Herald pre-commit rules, and it generally simplifies things.

Test Plan: Used `reparse.php` plus `var_dump()` to inspect refs in Git, Mercurial and SVN repos. They all looked correct and reparsed correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7804
2013-12-20 12:38:15 -08:00
William R. Otte
ff13bb8538 Render the clone URI input field as readonly.
Summary:
There's no particular reason to allow the user to edit the clone URI field in Diffusion; editing it has no meaning and if you fat finger the keyboard, it's quite possible that the user will either accidentally clear and/or modify the URI before copying (bit me this morning).

Adding a readonly attribute to the input field allows the same benefit (URI is easily selectable) while preventing such accidental input.  Fixes T4246.

Test Plan: Verified that the desired behavior is present in both Chrome, Safari, and Firefox. Field remains selectable with one click, but field is not editable.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4246

Differential Revision: https://secure.phabricator.com/D7810
2013-12-20 13:47:25 -06:00
epriestley
134c8f5547 Add "Author" and "Committer" fields to Herald pre-commit content hooks
Summary: Ref T4195. Adds "Author" and "Committer" fields.

Test Plan:
Created a rule using these fields:

{F90897}

...then pushed git, mercurial and svn commits and verified the correct values populated in the transcript:

{F90898}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7802
2013-12-19 11:05:31 -08:00
epriestley
d667b12206 Provide a standalone query for resolution of commit author/committer into Phabricator users
Summary:
Ref T4195. To implement the "Author" and "Committer" rules, I need to resolve author/committer strings into Phabricator users.

The code to do this is currently buried in the daemons. Extract it into a standalone query.

I also added `bin/repository lookup-users <commit>` to test this query, both to improve confidence I'm getting this right and to provide a diagnostic command for users, since there's occasionally some confusion over how author/committer strings resolve into valid users.

Test Plan:
I tested this using `bin/repository lookup-users` and `reparse.php --message` on Git, Mercurial and SVN commits. Here's the `lookup-users` output:

  >>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIS3
  Examining commit rINIS3...
  Raw author string: epriestley
  Phabricator user: epriestley (Evan Priestley   )
  Raw committer string: null
  >>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rPOEMS165b6c54f487c8
  Examining commit rPOEMS165b6c54f487...
  Raw author string: epriestley <git@epriestley.com>
  Phabricator user: epriestley (Evan Priestley   )
  Raw committer string: epriestley <git@epriestley.com>
  Phabricator user: epriestley (Evan Priestley   )
  >>> orbital ~/devtools/phabricator $ ./bin/repository lookup-users rINIH6d24c1aee7741e
  Examining commit rINIH6d24c1aee774...
  Raw author string: epriestley <hg@yghe.net>
  Phabricator user: epriestley (Evan Priestley   )
  Raw committer string: null
  >>> orbital ~/devtools/phabricator $

The `reparse.php` output was similar, and all VCSes resolved authors correctly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T1731, T4195

Differential Revision: https://secure.phabricator.com/D7801
2013-12-19 11:05:17 -08:00
epriestley
f750d5f8dc Provide a low-level SVN commit query, and merge the VCS query types
Summary: Ref T4195. Even though we use `svnlook` in the hook itself, I need this query elsewhere, so provide it and merge the classes into one which does the right thing.

Test Plan:
  - Used `reparse.php` to reparse messages for Git, SVN and Mercurial commits, using `var_dump()` to examine the commit refs for sanity.
  - Used `reparse.php` to reparse changes for an SVN commit.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7800
2013-12-19 11:05:06 -08:00
epriestley
d64dd7e2e8 Allow global Commit herald rules to trigger audits by users
Summary: Ref T4249. Currently, a global rule can only trigger project audits. Although there probably aren't a huge number of use cases for triggering users from global rules, it works fine and it's somewhat confusing not to allow it.

Test Plan: {F90902}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4249

Differential Revision: https://secure.phabricator.com/D7803
2013-12-19 10:33:15 -08:00
William R. Otte
264bef58c4 Various fixes for hosted and non-hosted subversion queries in Diffusion.
Summary: There were a number of places that were generating nonsense queries for both hosted and non-hosted subversion repositories.

Test Plan: Attempted several activities in Diffusion with both a hosted and non-hosted subversion repository, including viewing various types of diffs and raw files.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7799
2013-12-19 10:32:26 -06:00
epriestley
151f01ae94 Implement "Body" field in Herald pre-commit content hooks
Summary: Ref T4195. Adds support for writing rules against commit message bodies.

Test Plan: Pushed git, hg, svn commits and verified their bodies populated correctly in transcripts.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7796
2013-12-19 06:56:01 -08:00
epriestley
f37832aed7 Fix loop in svnserve workflow for large binaries
Summary: If you push a large binary and the data crosses multiple data frames, we can end up in a loop in the parser.

Test Plan:
After this change, I was able to push a 95MB binary in 7s, which seems reasonable:

  >>> orbital ~/repos/INIS $ svn st
  A       large2.bin
  >>> orbital ~/repos/INIS $ ls -alh
  total 390648
  drwxr-xr-x   6 epriestley  admin   204B Dec 18 17:14 .
  drwxr-xr-x  98 epriestley  admin   3.3K Dec 16 11:19 ..
  drwxr-xr-x   7 epriestley  admin   238B Dec 18 17:14 .svn
  -rw-r--r--   1 epriestley  admin    80B Dec 18 15:07 README
  -rw-r--r--   1 epriestley  admin    95M Dec 18 16:53 large.bin
  -rw-r--r--   1 epriestley  admin    95M Dec 18 17:14 large2.bin
  >>> orbital ~/repos/INIS $ time svn commit -m 'another large binary'
  Adding  (bin)  large2.bin
  Transmitting file data .
  Committed revision 25.

  real	0m7.215s
  user	0m5.327s
  sys	0m0.407s
  >>> orbital ~/repos/INIS $

There may be room to improve this by using `PhutilRope`.

Reviewers: wrotte, btrahan, wotte

Reviewed By: wotte

CC: aran

Differential Revision: https://secure.phabricator.com/D7798
2013-12-18 17:48:29 -08:00
epriestley
92bc76aae0 Move mercurial commit metadata parsing into a LowLevel query
Summary: Ref T4195. Same as D7793, but for mercurial. (As usual, SVN needs some goofy nonsense instead, so the next diff will just make this field work.)

Test Plan: Ran `reparse.php` on Git and Mercurial commits, var_dump'd the output and it looked correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7795
2013-12-18 17:48:19 -08:00
epriestley
f048053c75 Move git commit metadata parsing into a LowLevelQuery
Summary: Ref T4195. I need to issue this command from the pre-commit hook to get commit bodies for hooks.

Test Plan: Ran `reparse.php --message --trace` and dumped the $ref, which looked correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7793
2013-12-18 17:48:06 -08:00
epriestley
a5dc9067af Provide convenience method addTextCrumb() to PhabricatorCrumbsView
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.

Test Plan:
  - This was mostly automated, then I cleaned up a few unusual sites manually.
  - Bunch of grep / randomly clicking around.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7787
2013-12-18 17:47:34 -08:00
William R. Otte
dc43123494 Get the proper Subversion URI for hosted repositories.
Summary: The commit change parser worker was incorrectly getting the remote uri; calling getSubversionPathURI instead behaves properly for hosted repositories.  Fixes T4236.

Test Plan: Observed that my hosted subversion repositories finally had history in Diffusion.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4236

Differential Revision: https://secure.phabricator.com/D7794
2013-12-18 15:01:36 -08:00
epriestley
d90f44ef20 Support content pre-commit hooks in Mercurial
Summary: Ref T4195. Add Mercurial support to the content hook phase.

Test Plan:
Here are some `commit` push logs for a Mercurial repo:

{F90689}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7792
2013-12-18 14:19:18 -08:00
epriestley
5f4df0f3e3 Support "changed filename" and "file content" fields for commit content Herald rules
Summary: Ref T4195. Adds support for diff content rules.

Test Plan: Pushed SVN and Git changes through, saw them generate reasonable transcripts. Mercurial still isn't hooked up to this phase.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7791
2013-12-18 14:18:58 -08:00
epriestley
e115f11f80 Provide basic commit content hooks for Herald
Summary: Ref T4195. This doesn't provide any interesting fields yet (content, affected paths, commit message) but fires the hook correctly.

Test Plan: Added a blocking hook and saw it fire.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7789
2013-12-18 14:18:45 -08:00
epriestley
1ff3ef382d Give Herald rules a standard "Hnnn" object name
Summary: Allow Herald rules to be referred to with `H123`, etc., like other object types are. Herald rules now have proper PHIDs and an increasingly prominent role in triggering application actions. Although I suspect users will rarely use `H123` in Remarkup to mention rules, this can simplify some of the interfaces which relate objects across systems.

Test Plan: Looked at various interfaces and saw `H123` names. Mentioned `H123` in remarkup.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7786
2013-12-18 12:00:18 -08:00
epriestley
5863f792a6 Remove many redundant implementations of canLoadNamedObject()
Summary:
These just got copy/pasted like crazy, the base class has the correct default implementation.

(I'm adding "H" for Herald Rules, which is why I was in this code.)

I also documented the existing prefixes at [[ Object Name Prefixes ]].

Test Plan: Verified base implementation. Typed some object names into the jump nav.

Reviewers: btrahan

Reviewed By: btrahan

CC: hach-que, aran

Differential Revision: https://secure.phabricator.com/D7785
2013-12-18 12:00:01 -08:00
epriestley
181bfffaa1 Truncate object fields in Herald transcripts
Summary:
A few users have hit cases where Herald transcripts of large commits exceed the MySQL packet limit, because one of the fields in the transcript is an enomrous textual diff.

There's no value in saving these huge amounts of data. Transcripts are useful for understanding the action of Herald rules, but can be reconstructed later. Instead of saving all of the data, limit each field to 4KB of data.

For strings, we just truncate at 4KB. For arrays, we truncate after 4KB of values.

Test Plan: Ran unit tests. Artificially decreased limit and ran transcripts, saw them truncate properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: frgtn, aran

Differential Revision: https://secure.phabricator.com/D7783
2013-12-18 11:59:53 -08:00
epriestley
b0f7e7a6af Work around hg echoing warnings to stdout under --debug
Summary:
Ref T615. Ref T4237. With `--debug`, Mercurial will echo an "ignoring untrusted configuration option" warning **to stdout** if `.hgrc` has the wrong owner.

However, we need `--debug` to make `{parents}` usable, at least until the patches I got into the upstream are widely deployed. So after getting `--debug` output, strip off any leading warnings.

These warnings should always be in English, at least, since we set `LANG` explicitly.

Test Plan: Unit tests. @asherkin, maybe you can confirm this? I can't actually get the warning, but I think my `hg` in PATH is just a bit out of date.

Reviewers: asherkin, btrahan

Reviewed By: asherkin

CC: asherkin, aran

Maniphest Tasks: T615, T4237

Differential Revision: https://secure.phabricator.com/D7784
2013-12-17 18:04:01 -08:00
epriestley
3386920971 Add Herald support for blocking ref changes
Summary: Ref T4195. Allows users to write Herald rules which block ref changes. For example, you can write a rule like `alincoln can not create branches`, or `no one can push to the branch "frozen"`.

Test Plan:
This covers a lot of ground. I created and pushed a bunch of rules, then looked at transcripts, in general. Here are some bits in detail:

Here's a hook-based reject message:

  >>> orbital ~/repos/POEMS $ git push
  Counting objects: 5, done.
  Delta compression using up to 8 threads.
  Compressing objects: 100% (3/3), done.
  Writing objects: 100% (3/3), 274 bytes, done.
  Total 3 (delta 2), reused 0 (delta 0)
  remote: +---------------------------------------------------------------+
  remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
  remote: +---------------------------------------------------------------+
  remote:             \
  remote:              \                    ^    /^
  remote:               \                  / \  // \
  remote:                \   |\___/|      /   \//  .\
  remote:                 \  /V  V  \__  /    //  | \ \           *----*
  remote:                   /     /  \/_/    //   |  \  \          \   |
  remote:                   @___@`    \/_   //    |   \   \         \/\ \
  remote:                  0/0/|       \/_ //     |    \    \         \  \
  remote:              0/0/0/0/|        \///      |     \     \       |  |
  remote:           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
  remote:        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
  remote:                    ,-}        _      *-.|.-~-.           .~    ~
  remote:   \     \__/        `/\      /                 ~-. _ .-~      /
  remote:    \____(Oo)           *.   }            {                   /
  remote:    (    (--)          .----~-.\        \-`                 .~
  remote:    //__\\  \ DENIED!  ///.----..<        \             _ -~
  remote:   //    \\               ///-._ _ _ _ _ _ _{^ - - - - ~
  remote:
  remote:
  remote: This commit was rejected by Herald pre-commit rule H24.
  remote: Rule: No Branches Called Blarp
  remote: Reason: "blarp" is a bad branch name
  remote:
  To ssh://dweller@localhost/diffusion/POEMS/
   ! [remote rejected] blarp -> blarp (pre-receive hook declined)
  error: failed to push some refs to 'ssh://dweller@localhost/diffusion/POEMS/'

Here's a transcript, showing that all the field values populate sensibly:

{F90453}

Here's a rule:

{F90454}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7782
2013-12-17 15:23:55 -08:00
epriestley
baa756a027 Add rulePHID to HeraldEffect
Summary: Ref T4195. Herald rules gained PHIDs only recently, propagate them to HeraldEffect to make some of the hook stuff eaiser.

Test Plan: iiam

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7781
2013-12-17 15:23:45 -08:00
epriestley
f28d3089d7 Assign PHIDs to PushLogs
Summary: Ref T4195. We need these in Herald, since HeraldTranscripts need to refer to a PHID which they acted upon.

Test Plan:
Ran migration, got PHIDs:

  mysql> select phid from repository_pushlog limit 3;
  +--------------------------------+
  | phid                           |
  +--------------------------------+
  | PHID-PSHL-25jnc6cjgzw5rwqgmr7r |
  | PHID-PSHL-2vrvmtslkrj5yv7nxsv2 |
  | PHID-PSHL-34x262zkrwoka6mplony |
  +--------------------------------+
  3 rows in set (0.00 sec)

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7780
2013-12-17 15:23:23 -08:00
epriestley
2216a5e6ef Add Subversion ref and content logs to pre-commit hooks
Summary: Ref T4195. SVN has no such thing as refs (I was thinking about writing a quasi-ref anyway like `HEAD: r23 -> r24`, but I'm not sure it would actually be useful). And content is very easy to build.

Test Plan: Pushed some stuff to SVN, got logs from it.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7766
2013-12-17 11:11:52 -08:00
epriestley
11b8e57ae0 Remove "pretag" hook support in Mercurial
Summary: Ref T4195. This doesn't actually work like I thought it did: it only fires locally, when you run `hg tag`. Mercurial tags are also weird and basically don't make any sense and everyone should use bookmarks instead. We could implement some flavor of this eventually, but I'd like to see users request it first. They can implement their own with content-based hooks once those work, anyway.

Test Plan: This code didn't do anything.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7765
2013-12-17 09:18:48 -08:00
epriestley
74251b3636 Support bookmark hook operations in Mercurial
Summary: Ref T4195. Turns bookmark mutations in Mercurial into log objects.

Test Plan:
Pushed a pile of bookmarks and got logs:

{F89313}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7764
2013-12-17 08:34:30 -08:00
epriestley
6f3a99eb39 Generate ref updates in Mercurial hooks
Summary: Ref T4195. Mercurial is not my favorite VCS.

Test Plan:
Hit the split branches case:

  >>> orbital ~/repos/INIH $ hg push --force
  pushing to ssh://dweller@local.aphront.com/diffusion/INIH
  searching for changes
  remote: adding changesets
  remote: adding manifests
  remote: adding file changes
  remote: added 2 changesets with 2 changes to 1 files (+1 heads)
  remote: +---------------------------------------------------------------+
  remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
  remote: +---------------------------------------------------------------+
  remote:             \
  remote:              \                    ^    /^
  remote:               \                  / \  // \
  remote:                \   |\___/|      /   \//  .\
  remote:                 \  /V  V  \__  /    //  | \ \           *----*
  remote:                   /     /  \/_/    //   |  \  \          \   |
  remote:                   @___@`    \/_   //    |   \   \         \/\ \
  remote:                  0/0/|       \/_ //     |    \    \         \  \
  remote:              0/0/0/0/|        \///      |     \     \       |  |
  remote:           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
  remote:        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
  remote:                    ,-}        _      *-.|.-~-.           .~    ~
  remote:   \     \__/        `/\      /                 ~-. _ .-~      /
  remote:    \____(Oo)           *.   }            {                   /
  remote:    (    (--)          .----~-.\        \-`                 .~
  remote:    //__\\  \ DENIED!  ///.----..<        \             _ -~
  remote:   //    \\               ///-._ _ _ _ _ _ _{^ - - - - ~
  remote:
  remote:
  remote: DANGEROUS CHANGE: The change you're attempting to push splits the head of branch 'default' into multiple heads: 802c785c3dd9, e73400db39b0. This is inadvisable and dangerous.
  remote: Dangerous change protection is enabled for this repository.
  remote: Edit the repository configuration before making dangerous changes.
  remote:
  remote: transaction abort!
  remote: rollback completed
  remote: abort: pretxnchangegroup.phabricator hook exited with status 1

Hit the divergent heads case:

  >>> orbital ~/repos/INIH $ hg push --force
  pushing to ssh://dweller@local.aphront.com/diffusion/INIH
  searching for changes
  remote: adding changesets
  remote: adding manifests
  remote: adding file changes
  remote: added 1 changesets with 1 changes to 1 files (+1 heads)
  remote: +---------------------------------------------------------------+
  remote: |      * * * PUSH REJECTED BY EVIL DRAGON BUREAUCRATS * * *     |
  remote: +---------------------------------------------------------------+
  remote:             \
  remote:              \                    ^    /^
  remote:               \                  / \  // \
  remote:                \   |\___/|      /   \//  .\
  remote:                 \  /V  V  \__  /    //  | \ \           *----*
  remote:                   /     /  \/_/    //   |  \  \          \   |
  remote:                   @___@`    \/_   //    |   \   \         \/\ \
  remote:                  0/0/|       \/_ //     |    \    \         \  \
  remote:              0/0/0/0/|        \///      |     \     \       |  |
  remote:           0/0/0/0/0/_|_ /   (  //       |      \     _\     |  /
  remote:        0/0/0/0/0/0/`/,_ _ _/  ) ; -.    |    _ _\.-~       /   /
  remote:                    ,-}        _      *-.|.-~-.           .~    ~
  remote:   \     \__/        `/\      /                 ~-. _ .-~      /
  remote:    \____(Oo)           *.   }            {                   /
  remote:    (    (--)          .----~-.\        \-`                 .~
  remote:    //__\\  \ DENIED!  ///.----..<        \             _ -~
  remote:   //    \\               ///-._ _ _ _ _ _ _{^ - - - - ~
  remote:
  remote:
  remote: DANGEROUS CHANGE: The change you're attempting to push creates new, divergent heads for the branch 'default': f56af4232aa9. This is inadvisable and dangerous.
  remote: Dangerous change protection is enabled for this repository.
  remote: Edit the repository configuration before making dangerous changes.
  remote:
  remote: transaction abort!
  remote: rollback completed
  remote: abort: pretxnchangegroup.phabricator hook exited with status 1

Did a bunch of good/bad pushes:

{F89300}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7763
2013-12-17 08:34:15 -08:00
epriestley
2725586baf Restructure HookEngine to use PushLog records for all operations
Summary:
Ref T4195. This pulls the central logic of HookEngine up one level and makes all the git stuff genrate PushLogs.

In future diffs, everything will generate PushLogs and we can hand those off to Herald.

Test Plan:
Pushed a pile of valid/invalid stuff:

{F89256}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4195

Differential Revision: https://secure.phabricator.com/D7761
2013-12-17 08:32:33 -08:00
epriestley
537f2eabee Make it harder to misconfigure phpmailer.smtp-protocol
Summary: Until we implement an "enum" type for config, make this a bit harder to get wrong. A user entered "TLS", but the correct value is "tls". The documentation is consistent about this, but the behavior is sitll surprsing.

Test Plan: eyeballed it

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7778
2013-12-16 12:30:56 -08:00
epriestley
00f192cebb Save ApplicationSearch queries generated from GET parameters in the request
Summary:
Fixes T4239. Currently, if you go to `/maniphest/?authors=alincoln`, operations dependent on the query key (like "Save Custom Query..." and "Export to Excel...") don't have a query key to work with. Make sure they have one.

Also remove a stray `phlog()`.

Test Plan: "Save Custom Query...", etc., now work on GET queries.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4239

Differential Revision: https://secure.phabricator.com/D7777
2013-12-16 12:30:51 -08:00
epriestley
0011068d7a Fix an issue in Harbormaster when a buildable has no repository
Summary: Not every revision belongs to a repository, so we might end up here with `$repo` still equal to `null`. Don't fatal if we do.

Test Plan: iiam

Reviewers: btrahan, hach-que, zeeg

Reviewed By: hach-que

CC: aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7771
2013-12-13 18:20:42 -08:00
Chris Colborne
707b39c5b5 Fix comitted typo in Diffusion
See: <https://github.com/facebook/phabricator/pull/468>

Reviewed by: epriestley
2013-12-13 06:53:13 -08:00
James Rhodes
8aee78b24d Remove patch preview from Phragment version controller
Summary: Patches can exceed the 30 second time out in most PHP installations.  This removes the patch preview from the version controller so that users can still see the information (although they may not be able to download the actual patch).

Test Plan: Viewed a version and saw that the patch didn't appear.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7767
2013-12-13 15:01:03 +11:00
James Rhodes
86ec4d6021 Implement policies in Phragment
Summary: This implements support for enforcing and setting policies in Phragment.

Test Plan: Set policies and ensured they were enforced successfully.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7751
2013-12-13 14:42:12 +11:00
epriestley
7f45824984 Fix two issues with creating Conpherence threads via mail on some configurations
Summary:
Ref T4107. Two issues:

  - With strict MySQL settings, we try to insert `null` into the non-nullable `messageCount` field. Add an `initializeNew...` method.
  - If we don't create a new conpherence (for example, because the message body is empty), we fatal on `getPHID()` right now.

Also, make this stuff a little easier to test.

Test Plan: Used `mail_handler.php` to receive empty conpherence mail, and new-thread conpherence mail.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4107

Differential Revision: https://secure.phabricator.com/D7760
2013-12-12 10:59:28 -08:00
epriestley
d846f6508b Fix some repository URI handling issues in Git and Mercurial
Summary:
See <https://github.com/facebook/phabricator/issues/467>. @dctrwatson also ran into an issue where we were trying to `setPass()` a GitURI.

  - For Git and Mercurial, properly generate credential URIs where relevant.
  - Don't try to `setPass()` on Git-style URIs.

This isn't perfect but should clean things up a bit.

Test Plan: Added unit tests. Lots of `grep`.

Reviewers: btrahan

Reviewed By: btrahan

CC: dctrwatson, aran

Differential Revision: https://secure.phabricator.com/D7759
2013-12-12 09:45:27 -08:00
James Rhodes
1d9bf6f82b Fix Phortune so it allows users to create their accounts implicitly
Summary: This is a small fix for Phortune so that policies don't prevent the user accounts from being implicitly created when they first visit Phortune.

Test Plan: Visited Phortune and it worked.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7758
2013-12-12 11:19:03 +11:00
epriestley
61c934449d Fix fatal in Git hook when a --force push completely rewrites a ref
Summary: Fixes T4224. If you `git merge-base A B`, and they have //no// ancestor, the command exits with an error. Assume errors mean "no ancestry" and continue.

Test Plan: Completely rewrite a repository with a `--force` push.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4224

Differential Revision: https://secure.phabricator.com/D7756
2013-12-11 14:46:46 -08:00
epriestley
6b1ec35cf3 Allow Herald rules to check for revisions with no reviewers
Summary: Fixes T4225. Adds the NON_EXISTS condition to Herald for "Reviewers", and adds a few more conditions which have reasonable meanings.

Test Plan: Used test console to check a revision with reviewers, and another without reviewers. Both produced the expected results.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4225

Differential Revision: https://secure.phabricator.com/D7757
2013-12-11 14:46:39 -08:00
Matt Robenolt
c3d9c28382 Specify an ssh port for Diffusion when running against the grain
Summary: We run `git` on a different port than 22, so would like to reflect this change in the UI.

Test Plan: Set diffusion.ssh-port in settings, then make sure it's reflected on the Diffusion repository Clone URI.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, dctrwatson

Differential Revision: https://secure.phabricator.com/D7755
2013-12-11 12:11:13 -08:00
epriestley
052c83a613 Fix error detection for "ls-tree" output
Summary: Fixes T4223. The output of `ls-tree` is partially delimited by spaces
and partially delimited by `\t`. The code I added in D7744 to help debug the
issue in T4159 doesn't work properly for files with 7 or more bytes in their
filesize, because the internals use `%7s`.

Auditors: btrahan
2013-12-11 07:15:10 -08:00
James Rhodes
b8b7bf8ad9 Provide phragment.getstate and phragment.getpatch Conduit methods
Summary:
This provides a `phragment.getstate` and a `phragment.getpatch` Conduit method.

`phragment.getstate` - This returns the current state of the fragment and all of it's children.

`phragment.getpatch` - This accepts a base path and a mapping of paths to hashes.  The mapping is for the caller to specify the current state of the files it has.  This returns a list of patches that the caller needs to apply to it's files to get to the latest version.

Test Plan:
Ran the following script in a folder which had content matching a fragment and it's children:

```
#!/bin/bash

STATE=""
for i in $(find ./ -type f); do
    HASH=$(cat $i | sha1sum | awk '{ print $1 }')
    BASE=${i:2}
    STATE="$STATE,\"$BASE\":\"$HASH\""
done
STATE=${STATE:1}
STATE="{$STATE}"

echo '{"path":"tychaia3.zip","state":'$STATE'}' | arc --conduit-uri=http://phabricator.local/ call-conduit phragment.getpatch
```

and I got:

```
{"error":null,"errorMessage":null,"response":[]}
```

I updated one of the child fragments with a new file and ran the script again (patch has been omitted due to it's size):

```
{"error":null,"errorMessage":null,"response":[{"path":"Content\/TitleFont.xnb","hash_old":"4a927d7b90582e50cdd330de9f4b59b0cc5eb5c7","hash_new":"25867504642a3a403102274c68fbb9b430c1980f","patch":"..."}]}
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, staticshock

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7739
2013-12-11 11:19:23 +11:00
James Rhodes
270c8d27ab Implement "Wait for Previous Builds" build step
Summary: This adds a build step which will block a build from continuing if there are previous builds of the build plan still running.

Test Plan: Configured a build plan with a wait of 60 seconds and a "wait for previous builds", then started a build.  While that was still building, reconfigured the plan to have a wait time of 3 seconds, started it, and saw it move into the "Waiting" status.  When the 60 second build finished, both builds passed.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7745
2013-12-10 11:02:34 +11:00
Aviv Eyal
de969ab540 plug octocat icon to landing button
Test Plan: look at shiny button

Reviewers: epriestley, chad, #blessed_reviewers

Reviewed By: chad

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7749
2013-12-09 13:29:34 -08:00
epriestley
b8b7e583ad Show a queue utilization statistic in the Daemon console
Summary:
This came up recently in a discussion with @lifeihuang, and then tangentally with @hach-que. Make it easier for users to get a sense of whether they might need to add more daemons. Although we've improved the transparency of daemons, it's not easy for non-experts to determine at a glance how close to overflowing the queue is.

This number is approximate, but should be good enough for determining if your queue is more like 25% or 95% full.

If this goes over, say, 80%, it's probably a good idea to think about adding a couple of daemons. If it's under that, you should generally be fine.

Test Plan: {F88331}

Reviewers: btrahan, hach-que, lifeihuang

Reviewed By: btrahan

CC: hach-que, lifeihuang, aran, chad

Differential Revision: https://secure.phabricator.com/D7747
2013-12-09 13:22:22 -08:00
epriestley
52462a46c0 Raise a better error for malformed git ls-tree
Summary: Ref T4159. See T4159 for discussion.

Test Plan: Faked the error and generated a reasonable error message.

Reviewers: hach-que, btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4159

Differential Revision: https://secure.phabricator.com/D7744
2013-12-09 13:22:13 -08:00
epriestley
5d27aeb240 Fix the exception when watching the first commit of a mercurial repo
Summary: Most checks were actually in place, but `ExecFuture` throws a `CommandException` which wasn't taken into account.

Test Plan: look at the first command and no longer saw an exception. Also, other commits worked as well.

Reviewers: richardvanvelzen

Reviewed By: richardvanvelzen

CC: krisbuist, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D7730
2013-12-09 09:20:40 -08:00
James Rhodes
8fd256a1fd Implement step for publishing files as fragments to Phragment in Harbormaster
Summary: This adds a build step in Harbormaster for publishing file artifacts as fragments in Phragment.

Test Plan:
Created a build plan with the following steps:

  * Lease Host
  * Upload Artifact
  * Publish Fragment

Ran the build plan against a buildable and saw the fragment get created in Phragment.  Ran the plan again and saw the fragment get updated with a new version.  Modified the file that got uploaded and ran the plan again, checked the history of the fragment, and saw the differences represented as a Diff-Match-Patch patch.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7742
2013-12-09 09:34:09 +11:00
James Rhodes
8bb6e807f0 Implement snapshots in Phragment
Summary:
Ref T4212.  This implements snapshots in Phragment, which allows you to take a snapshot of a fragment at a given point in time, and download a ZIP of the snapshot as it was in this state.

There's also functionality for deleting and promoting snapshots.  You can promote a snapshot to either the latest version or any other snapshot of the fragment.

Test Plan: Clicked around, took some snapshots, promoted them to different points and deleted snapshots.  Also downloaded ZIPs of the snapshots and saw the right versions coming through for all the files downloaded.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205, T4212

Differential Revision: https://secure.phabricator.com/D7741
2013-12-09 08:24:50 +11:00
James Rhodes
e165787725 Implement "Revert to Version" functionality in Phragment
Summary:
This functionality allows users to revert a fragment to a previous version from the history page.

Reverting a version actually creates a new version pointing at the same file as the version being "reverted" to.  In this sense it acts pretty much like Git and other distributed VCS where once you have published a commit the only way to undo your changes is to create a new commit that reverts those changes.

Test Plan: Reverted a fragment to a version before it was deleted, then reverted it to when it was deleted and saw the new versions have the correct file PHIDs (including null for the deletion).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7738
2013-12-09 07:57:53 +11:00
James Rhodes
dabc7ea28d Render deleted files in Phragment as disabled
Summary: This updates Phragment so that fragments that are currently considered deleted have a disabled status and have an additional attribute 'Deleted'.  It also places this effect on versions (in the history controller) that actually involve deleting the file.

Test Plan: Viewed deleted fragments and versions.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7737
2013-12-09 07:52:24 +11:00
James Rhodes
67b37a5c1d Fix deletion detection when updating based on ZIP
Summary: When the code to update based on ZIP went to look up the child fragments, it explicitly used the paths provided in the ZIP.  This meant that we could never detect omissions because there'd never be a scenario where a child fragment would return but not exist in the ZIP.  To fix this, the query should be using `withLeadingPath` instead of `withPaths`.

Test Plan: Uploaded a ZIP that omitted a file and saw the `deleteFile` get called (by placing debugging output in the code).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7736
2013-12-09 07:48:54 +11:00
James Rhodes
acd5d5ae4a Correctly handle downloading ZIP with empty directories in Phragment
Summary: This logic causes an exception because getPHID() is called on a fragment that has no latest version.  This fixes the code so that in this scenario, it returns an empty array (with no path to be added to the ZIP).

Test Plan: Downloaded the ZIP successfully after the patch was applied.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7735
2013-12-08 12:15:13 +11:00
James Rhodes
c48cfb31bc Implement viewing versions and downloading patches in Phragment
Summary:
This adds support for viewing individual versions on a fragment as well as comparing versions and downloading diff_match_patch-based patches.

It does not use the side-by-side diff format as while it works for small changes, it quickly becomes impossible to distingush what changes have been made due to the diff_match_patch format.

Test Plan: Clicked on versions and downloaded patches.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7734
2013-12-08 12:14:34 +11:00
James Rhodes
5728ef7836 Implicitly detect directories when updating a fragment from a ZIP
Summary: This fixes the update-from-ZIP functionality so that it will automatically detect directories in the ZIP that do not have explicit entries.  Some ZIP programs do not create directory entries explicitly, so if we fail to do this then there's no way for users to access the sub-fragments (even though they exist, there is no directory fragment to click through).

Test Plan: Created and updated fragments from a ZIP that had implicit directories in it.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran, staticshock

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7733
2013-12-08 12:10:00 +11:00
epriestley
7a5c3cc854 Fix undefined variables in Subversion
Summary: These variables won't be in scope in Subversion.

See: <https://secure.phabricator.com/rP2ff5541fc59c4be7abd733a39e12db8358004f7a>

Auditors: btrahan
2013-12-07 11:12:38 -08:00
epriestley
99ad978e90 Add UI for choosing header color
Summary: See D7731. Fixes T4194.

Test Plan: {F88020}

Reviewers: chad, btrahan

Reviewed By: chad

CC: aran, mbishopim3

Maniphest Tasks: T4194

Differential Revision: https://secure.phabricator.com/D7740
2013-12-07 10:46:09 -08:00
James Rhodes
25e7b7d53c Implement support for creating and updating fragments from ZIPs
Summary:
This implements support for creating and updating fragments from ZIP files.  It allows you to upload a ZIP via the Files application, create a fragment from it, and have it recursively imported into Phragment.  Updating that folder with another ZIP will recursively create, update and delete files as appropriate.

The logic for creating and updating fragments from files has also been centralized into the PhragmentFragment class.  Directories are also now supported; a directory fragment is simply a fragment that has no patches; thus a directory fragment can be converted to a file fragment by uploading a first patch for it.

Test Plan: Uploaded ZIP files through the interface and saw all of the fragments get created and updated as expected.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7729
2013-12-07 13:37:18 +11:00
James Rhodes
ccd4ae5638 Implement "Download ZIP" controller for Phragment
Summary: Depends on D7727.  This adds support for downloading a fragment and all it's children as a ZIP file.  Fragments that have children automatically become directories in the ZIP file.

Test Plan: Downloaded a fragment as a ZIP and was able to extract the contents successfully.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7728
2013-12-07 13:25:22 +11:00
James Rhodes
f7f5a5dd34 Implement update and history controllers in Phragment
Summary: Depends on D7726.  This adds a history controller (for viewing a list of patches associated with a fragment) and an update controller, for creating a new patch of a fragment.

Test Plan: Updated and viewed history of fragments.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7727
2013-12-07 12:56:55 +11:00
James Rhodes
4c143ad3b2 Phragment v0
Summary: Ref T4205.  This is an initial implementation of Phragment.  You can create and browse fragments in the system (but you can't yet view a fragment's patches / history).

Test Plan: Clicked around and created fragments.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4205

Differential Revision: https://secure.phabricator.com/D7726
2013-12-07 12:43:49 +11:00
epriestley
f69793184e Fix over-matching of quoted text for message bodies beginning with "On..."
Summary:
A user sent a message to Phabricator which looked like:

  On blah blah blah ?

  On <date>, <user> wrote:
  > blah blah blah

The current algorithm is too aggressive and thinks lines 1-3 are //all// the "On ... wrote:" string. Instead, patch only the most recent "On".

Test Plan: Added a failing test and made it pass.

Reviewers: btrahan, zeeg

Reviewed By: zeeg

CC: aran

Differential Revision: https://secure.phabricator.com/D7732
2013-12-06 15:47:37 -08:00
James Rhodes
79d153b85d Implement explicit build step ordering in Harbormaster
Summary: This implements support for explicitly marking the sequence of build steps.  Users can now drag and re-order build steps in plans, and artifact dependencies are re-calculated so that if you move "Run Command" before "Lease Host", the "Run Command" step has it's artifact setting cleared and thus the step becomes invalid.

Test Plan: Re-ordered build steps and observed dependencies being correctly recalculated.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7715
2013-12-06 14:12:15 +11:00
James Rhodes
dd01535ed6 Implement "Upload Artifact" build step
Summary: This implements a build step for uploading an artifact from a build machine to Phabricator.  It uses SFTP so that it will work on both UNIX and Windows build machines.

Test Plan: Ran an "Upload Artifact" build against a Windows machine (with FreeSSHD installed).  The artifact uploaded to Phabricator, appeared on the build view and the file contents could be viewed from Phabricator.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T1049

Differential Revision: https://secure.phabricator.com/D7582
2013-12-06 14:11:05 +11:00
epriestley
a1f3233481 Don't show client IP in push logs unless viewer can edit the repository
Summary: This locks push logs down a little bit and makes them slightly more administrative. Primarily, don't show IPs to googlebot, etc.

Test Plan: Viewed push logs as edit and non-edit users.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7722
2013-12-05 17:01:07 -08:00
epriestley
faaaff0b6f Fix an error in the PolicyFilter algorithm
Summary:
`PhabricatorPolicyFilter` has a bug right now where it lets through objects incorrectly if:

  - the query requests two or more policies;
  - the object satisfies at least one of those policies; and
  - policy exceptions are not enabled.

This would be bad, but there's only one call in the codebase which satisfies all of these conditions, in the Maniphest batch editor. And it's moot anyway because edit operations get another policy check slightly later. So there is no policy/security impact from this flaw.

(The next diff relies on this behavior, which is how I caught it.)

Test Plan:
  - Added a failing unit test and made it pass.
  - Grepped the codebase for `requireCapabilities()` and verified that there is no security impact. Basically, 99% of callsites use `executeOne()`, which throws anyway and moots the filtering.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Differential Revision: https://secure.phabricator.com/D7721
2013-12-05 17:00:53 -08:00