1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-12-01 03:02:43 +01:00
Commit graph

95 commits

Author SHA1 Message Date
epriestley
156b156e77 Give Conduit params/return/errors protected visibility
Summary:
Ref T7803. Ref T5873. I want to drive Conduit through more shared infrastructure, but can't currently add parameters automatically.

Put a `getX()` around the `defineX()` methods so the parent can provide default behaviors.

Also like 60% of methods don't define any special error types; don't require them to implement this method. I want to move away from this in general.

Test Plan:
  - Ran `arc unit --everything`.
  - Called `conduit.query`.
  - Browsed Conduit UI.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: hach-que, epriestley

Maniphest Tasks: T5873, T7803

Differential Revision: https://secure.phabricator.com/D12380
2015-04-13 11:58:35 -07:00
Bob Trahan
3fcc3fdedf Diffusion - be sure to properly unserialize result from conduit query
Summary: Fixes T7256.

Test Plan: Looked at rXPRF0a7a5f69f5d7 in a local instance. things looked great both pre and post patch.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7256

Differential Revision: https://secure.phabricator.com/D11790
2015-02-17 13:54:59 -08:00
Bob Trahan
e1dcbc4386 Policy - lock down DiffusionSymbolQuery repo-loading code
Summary: Ref T7094.

Test Plan: couldn't really test this - how does one get symbols going nowadays given they are acanist project based?

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11584
2015-01-31 18:36:36 -08:00
Bob Trahan
bdb3adeee4 Policy - clean up the deprecated diffusion.getcommits
Summary: Ref T7094. Could just delete this end point too I guess? Needed to add "withCommitPHIDs" to the differentialrevisionquery to get this done.

Test Plan: used diffusion.getcommits from conduit console and got a sensible result for a query for two commits, one with a diff and one without.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T7094

Differential Revision: https://secure.phabricator.com/D11581
2015-01-30 11:51:16 -08:00
epriestley
8d087ae738 Remove 'initFromConduit' option from Diffusion
Summary:
Ref T2783. I think this served two purposes:

  - Improving performance in cases where we "know" a repository is local.
  - Preventing loops.

It is now obsolete:

  - After D11476, refs can almost always resolve on a fast path.
  - As T2783 moves forward, we can usually no longer know when a repository is local without actually looking it up -- almost everything is allowed to run anywhere.
  - The cluster behavior in D11475 now prevents loops.

Test Plan: `grep`, browsed around. This didn't really do much of anything anymore.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2783

Differential Revision: https://secure.phabricator.com/D11477
2015-01-23 13:31:45 -08:00
epriestley
d94d1da610 Proxy Diffusion Conduit API calls
Summary:
Fixes T7020. When an external user makes a Conduit request to Diffusion but the repository isn't hosted locally, we need to proxy it.

This also adds a guard layer to prevent requests from getting infinitely proxied inside the cluster.

In "trivial" configurations (where the repository is a service repository, but the service is on the local device) I'm making us always proxy anyway. This basically makes it reasonable to test this stuff (otherwise you'd have to set up two different installs) and this configuration doesn't make much sense in real life (if you're using multiple machines, making one a dedicating daemons+repo box is almost certainly the most reasonable configuration, even for a cluster size of 2).

Test Plan:
  - With a service-hosted repository, made Diffusion conduit calls and browsed the UI. Verified requests got proxied once, then resovled.
  - With a non-service repository, made Diffusion conduit calls and browsed UI. Verified requests were handled in-process immediately.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T7020

Differential Revision: https://secure.phabricator.com/D11475
2015-01-23 13:30:52 -08:00
epriestley
cae8c49745 Fix diffusion.readmequery to work in a cluster enviroment
Summary:
Ref T2783. This method is kind of goofballs:

  - We send a big list of paths to it.
  - It sends back a giant blob of HTML.

Instead, just figure out the path we want locally, then fetch the content with `diffusion.filecontentquery`.

Test Plan:
  - Viewed main view and directory view, saw a README.
  - See screenshots.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2783

Differential Revision: https://secure.phabricator.com/D11099
2014-12-31 11:54:52 -08:00
Joshua Spence
39ca2fdf64 Use new FutureIterator instead of Futures
Summary: Ref T6829. Deprecate the `Futures()` function.

Test Plan: N/A

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6829

Differential Revision: https://secure.phabricator.com/D11077
2014-12-30 23:13:38 +11:00
Joshua Spence
3cf9a5820f Minor formatting changes
Summary: Apply some autofix linter rules.

Test Plan: `arc lint` and `arc unit`

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D10585
2014-10-08 08:39:49 +11:00
epriestley
ac4247ea59 Provide more information from diffusion.querycommits
Summary:
Ref T2783. Fixes T6039.

  - Provide `authorPHID` and `committerPHID` to resolve T6039.
  - In message parser, store author/email strings.
  - In cached results, emit author/email strings.

Test Plan: Called method with and without bypassCache. Used `reparse.php` to repopulate data on an old commit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2783, T6039

Differential Revision: https://secure.phabricator.com/D10424
2014-09-05 12:27:55 -07:00
James Rhodes
d7f51325e3 Populate results of DiffusionQueryCommitsConduitAPIMethod with DiffusionLowLevelCommitQuery
Summary:
Ref T2783.  This populates the following fields in DiffusionQueryCommitsConduitAPIMethod using DiffusionLowLevelCommitQuery when `bypassCache` is set to true:

  * `authorName`
  * `authorEmail`
  * `committerName`
  * `committerEmail`
  * `message`
  * `hashes`

The original outline called for `authorPHID` and `committerPHID` as well (but no `message` field).  As far as I can tell, the PHIDs aren't actual a property on `DiffusionCommitRef`, and since the intention of this is to be able to populate a `DiffusionCommitRef`, I haven't included them.  Let me know if we really do need the PHIDs here.

Test Plan: Tested using 3 Phabricator instances (one web, one taskmaster and one storage).  The web and taskmaster tiers are directed at the Conduit API of the storage tier.  Made a `diffusion.querycommits` from the Conduit app on the web tier instance and saw the data populated from the raw VCS data (located on the storage tier).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T2783

Differential Revision: https://secure.phabricator.com/D10399
2014-09-03 22:49:44 +10:00
Bob Trahan
d1936711a0 Diffusion - replace last hg manifest call with hg locate
Summary: Fixes T4387.

Test Plan: Setup a mercurial repository for rabbitmq-server. Browsed around it and things looked good.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4387

Differential Revision: https://secure.phabricator.com/D10380
2014-08-28 13:08:42 -07:00
Bob Trahan
c1e8d97069 Diffusion - re-jigger how README files get rendered
Summary: be more aggressive about assuming plain-text, use remarkup for no extension, .remarkup, and .md, and last but not least use rainbow for .rainbow. Fixes T5818.

Test Plan: my README rendered just fine post these changes

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: asherkin, epriestley, Korvin

Maniphest Tasks: T5818

Differential Revision: https://secure.phabricator.com/D10340
2014-08-22 15:49:03 -07:00
Joshua Spence
f055736eca Rename PhutilRemarkupRule subclasses
Summary: Ref T5655. Depends on D9993.

Test Plan: See D9993.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D9994
2014-08-05 00:55:43 +10:00
epriestley
508260e4fe Apply diffusion.createcomment directly with transaction editor in Audit
Summary: Ref T4896. Use the new transaction-oriented `PhabricatorAuditEditor` directly instead of invoking it via the old editor.

Test Plan: Used Conduit to add a comment, use silent mode, and accept a commit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10126
2014-08-02 14:45:09 -07:00
epriestley
688f245a95 Use transactions to apply "add auditors" action in Audit
Summary:
Ref T4896. Move the write for "Add Auditors" inside the new Editor.

There are no longer any readers or writers for metadata, so remove the calls for it.

Test Plan: Added auditors from the web UI.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10123
2014-08-02 14:44:35 -07:00
epriestley
5b969fb5b8 Provide a transaction editor to perform Audit row writes
Summary:
Ref T4896. Replaces more custom stuff with standard stuff. In particular:

  - No more fake proxy writes;
  - no more fake detection of `@mentions`.

For now, the old code still applies most of the effects and handles feed and email.

Test Plan:
  - Added comments.
  - Added comments with inline comments.
  - Added just inline comments.
  - Added comments with Conduit.
  - Previewed comments.
  - Added CCs explicitly and with `@mentions`.
  - Added auditors.
  - Accepted a commit.

Reviewers: joshuaspence, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10109
2014-08-02 00:06:25 -07:00
Joshua Spence
68f1ca896d Fix misspelled file name
Summary: This class was renamed in D9991, but the filename is incorrect.

Test Plan: Eyeball it

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10117
2014-08-02 17:05:49 +10:00
epriestley
608e1d20b4 Write separate comments for every action in Audit
Summary:
Ref T4896. Depends on D10023. Prepares the code for the final migration.

The transaction table stores one row per distinct effect (e.g., add CCs) rather than one row per user action (e.g., "add CCs + comment"). We can double-read that table as long as the code doesn't expect transactions/comments to have multiple different effects, and doesn't try to write any such rows.

Everywhere that we were writing a big "X + Y" comment, write two separate "X" and "Y" comments instead. Like D10023, this disrupts the UI a little (you get more boxes), but that will be resolved once the rendering code swaps over. Otherwise, this retains the existing behavior.

Test Plan:
- Used `diffusion.createcomment` to add comments, raise concern, and accept.
- Previewed commenting, adding auditors/ccs, accepting, raising concern.
- Actually performed commenting, adding auditors/ccs, accepting, raising concern.
- Added a user with mentions.
- Added an explicit CC and a mention user.

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T4896

Differential Revision: https://secure.phabricator.com/D10052
2014-07-28 15:00:18 -07:00
Asher Baker
556bca3099 Order readme files based on how well we can get the markup right.
Summary:
Handling readmes with no extension is a bit of a hack, but seemed like a small cost.

The Big Win here is that you can commit README.remarkup and README.md and have both Phabricator and GitHub render __with__ //all// ##the## ~~pretty~~ **markup**.

Test Plan: Looked at some readme files.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10047
2014-07-25 06:43:26 -07:00
epriestley
20589389de Fix some issues with new Conduit method implementations
Summary: Ref T5655. A few of these were missed.

Test Plan:
Checked all other methods like this:

```
    foreach ($method_map as $k => $v) {
      $v = preg_replace('/ConduitAPIMethod$/', '', $v);
      $k = str_replace('.', '', $k);
      $v = strtolower($v);
      if ($k != $v) {
        echo "{$k} x {$v}!\n";
      }
    }
    echo "OK\n";
```

Reviewers: hach-que, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D10049
2014-07-24 21:57:03 -07:00
epriestley
3fca1b2d2d Fix some missing renames of Application classes
Summary: I think these got caught in the crossfire between Conduit and
Applications. Ref T5655.

Auditors: joshuaspence
2014-07-24 18:03:59 -07:00
Joshua Spence
023dee0d3b Rename Conduit classes
Summary: Ref T5655. Rename Conduit classes and provide a `getAPIMethodName` method to declare the API method.

Test Plan:
```
> echo '{}' | arc --conduit-uri='http://phabricator.joshuaspence.com' call-conduit user.whoami
Waiting for JSON parameters on stdin...
{"error":null,"errorMessage":null,"response":{"phid":"PHID-USER-lioqffnwn6y475mu5ndb","userName":"josh","realName":"Joshua Spence","image":"http:\/\/phabricator.joshuaspence.com\/res\/1404425321T\/phabricator\/3eb28cd9\/rsrc\/image\/avatar.png","uri":"http:\/\/phabricator.joshuaspence.com\/p\/josh\/","roles":["admin","verified","approved","activated"]}}
```

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin, hach-que

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D9991
2014-07-25 10:54:15 +10:00
Joshua Spence
86c399b657 Rename PhabricatorApplication subclasses
Summary: Ref T5655. Some discussion in D9839. Generally speaking, `Phabricator{$name}Application` is clearer than `PhabricatorApplication{$name}`.

Test Plan:
# Pinned and uninstalled some applications.
# Applied patch and performed migrations.
# Verified that the pinned applications were still pinned and that the uninstalled applications were still uninstalled.
# Performed a sanity check on the database contents.

Reviewers: btrahan, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: hach-que, epriestley, Korvin

Maniphest Tasks: T5655

Differential Revision: https://secure.phabricator.com/D9982
2014-07-23 10:03:09 +10:00
epriestley
d4b2bfa2f4 Modernize commit/edge transaction when parsing commit messages
Summary: Ref T5245. With work elsewhere (notably, D9839) we can remove this TODO and use real transactions.

Test Plan: Pushed a `closes Txxx` commit and got a close + transaction.

Reviewers: chad, btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

Maniphest Tasks: T5245

Differential Revision: https://secure.phabricator.com/D9848
2014-07-17 15:42:06 -07:00
Joshua Spence
8756d82cf6 Remove @group annotations
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.

Test Plan: Eye-ball it.

Reviewers: #blessed_reviewers, epriestley, chad

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9859
2014-07-10 08:12:48 +10:00
epriestley
5660684d7f Never use "{branches}" in Mercurial
Summary:
Fixes T5304. Mercurial features a "{branches}" template keyword, documented as:

```
    branches      List of strings. The name of the branch on which the
                  changeset was committed. Will be empty if the branch name
                  was default.
```

At some time long in the past, I misinterpreted this to mean "list of branches where the branch head is a descendant of the commit". It is more like "list of zero or one elements, possibly containing the name of the branch the commit was originally made to, if that branch was not 'default'".

In fact, it seems like this is because a //very// long time in the past, Mercurial worked roughly like I expected:

> Ages ago (2005), we had a very different and ultimately unworkable
> approach to named branches that worked vaguely like .hgtags and allowed
> multiple branch names per revision.

http://marc.info/?l=mercurial-devel&m=129883069414855

This appears to be deprecated in modern Mercurial (it's not in the modern web documentation) although I can't find a commit about it so maybe that's just a documentation issue.

In any case, `{branches}` seems to never be useful: `{branch}` provides the same information without the awkward "default-if-empty" case.

Switch from `{branches}` to either `{branch}` (where that's good enough, notably in the hook engine) or `(descendants(%s) and head())`, which is equivalent to `--contains` in Git.

This fixes pushing to branches with spaces in their names, and makes the "Branches" / "Contains" queries moderately more consistent.

Test Plan:
  - Pushed to a Mercurial branch with a space in it.
  - Viewed list of branches in a Mercurial repository.
  - Viewed containing branches of a Mercurial commit in Diffusion.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5304

Differential Revision: https://secure.phabricator.com/D9453
2014-06-20 11:48:31 -07:00
Joshua Spence
0a62f13464 Change double quotes to single quotes.
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.

Test Plan: Eyeballed it.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9431
2014-06-09 11:36:50 -07:00
epriestley
a74545c9da Provide a rough, unstable API for reporting coverage into Diffusion
Summary:
Ref T4994. This stuff works:

  - You can dump a blob of coverage information into `diffusion.updatecoverage`. This wipes existing coverage information and replaces it.
  - It shows up when viewing files.
  - It shows up when viewing commits.

This stuff does not work:

  - When viewing files, the Javascript hover interaction isn't tied in yet.
  - We always show this information, even if you're behind the commit where it was generated.
  - You can't do incremental updates.
  - There's no aggregation at the file (this file has 90% coverage), diff (the changes in this commit are 90% covered), or directory (the code in this directory has 90% coverage) levels yet.
  - This is probably not the final form of the UI, storage, or API, so you should expect occasional changes over time. I've marked the method as "Unstable" for now.

Test Plan:
  - Ran `save_lint.php` to check for collateral damage; it worked fine.
  - Ran `save_lint.php` on a new branch to check creation.
  - Published some fake coverage information.
  - Viewed an affected commit.
  - Viewed an affected file.

{F151915}

{F151916}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: jhurwitz, epriestley, zeeg

Maniphest Tasks: T5044, T4994

Differential Revision: https://secure.phabricator.com/D9022
2014-05-17 16:10:54 -07:00
epriestley
f2d72ced4b Fix a missed call to setCommit() on DiffusionRequest
See: <https://github.com/facebook/phabricator/issues/608>

Test Plan: Viewed a Subversion diff. Grepped for `setCommit()` again.

Auditors: btrahan
2014-05-15 04:45:53 -07:00
epriestley
436f0563e8 Add a SublimeText-style repository typeahead
Summary:
Allows you to quickly search for files within a repository. Roughly:

  - We build a big tree of everything and ship it to the client.
  - The client implements a bunch of Sublime-ish magic to find paths.

Test Plan: {F154007}

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D9087
2014-05-13 14:08:21 -07:00
epriestley
23ada21d35 Remove commit from DiffusionRequest
Summary: Ref T2683. This field is //almost// entirely redundant with `symbolicCommit`. Improve how some of the diff query stuff works a bit, then remove it.

Test Plan: Browsed around in all interfaces, looked at a bunch of diffs, etc.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9099
2014-05-13 13:53:06 -07:00
epriestley
347252fda8 Improve clarity of commit and symbol handling in DiffusionRequest
Summary:
Ref T2683. Currently, DiffusionRequest has four different "commitey" things:

  - `commit`
  - `rawCommit`
  - `symbolicCommit`
  - `stableCommit`

Of these, only two are actually distinct, useful values: `symbolicCommit` (which holds the value the request originally contained, if one existed) and `stableCommit` (which resolves that value, or the value implied by its omission, into a stable, permanent commit identifier).

  - `rawCommit` is equivalent to `symbolicCommit` and can be simply removed.
  - `commit` has some sketchy magic around it that needs to be pulled out before it can be jettisoned.

Test Plan: Viewed SVN, Git, and Mercurial repositories. Viewed brwose/history/change/tag/branch/etc views.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9098
2014-05-13 13:52:48 -07:00
epriestley
ce3f9211e4 Let diffusion.readmequery accept a commit
Summary:
Ref T2683. This should probably just be `diffusion.filecontentquery` but keep things as they are for now.

This method uses a commit, so accept one. Soon, this will save a bit of work.

Test Plan: Viewed readmes in main and browse views.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9093
2014-05-13 13:52:20 -07:00
epriestley
9a2c68fd88 Rename "stableCommitName" to "stableCommit"
Summary:
Ref T2683. This is closely related to "symbolicCommit", but has an inconsistent "name" on the end.

Also, `diffusion.searchquery` uses this parameter inconsistently.

Test Plan:
  - `grep`ed for callsites.
  - Ran searches in Git and Mercurial repositories.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9091
2014-05-13 13:51:45 -07:00
epriestley
7cc9720d60 Remove shouldCreateDiffusionRequest from Diffusion conduit methods
Summary:
Ref T2683. This has no callsites, and the functionality is covered by the `initFromConduit` flag.

This simplifies the code and reduces then number of internal `diffusion.resolverefs` calls we make on, e.g., the Git repository page from 7 to 2.

Test Plan: Grepped for these symbols.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9090
2014-05-13 13:51:33 -07:00
epriestley
cfa265f020 Make sure READMEs can hit the markup cache in Diffusion
Summary: Ref T2683. Normally not a big deal, but if a readme has some codeblocks missing the cache can slow things down.

Test Plan:
  - Verified we hit the cache.
  - Verified TOC still works.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5028, T2683

Differential Revision: https://secure.phabricator.com/D9049
2014-05-12 11:47:31 -07:00
epriestley
e13369d208 Use RepositoryGraphCache to service diffusion.lastmodifiedquery
Summary:
Ref T2683. At least locally, browse views are now nearly instantaneous, even in Mercurial. We also fall back to what we were doing before if we miss or take too long, so this shouldn't make things very much worse even in extreme cases.

For a local `hg` repo, the time we spend pulling browse stuff has dropped from ~3,000ms to ~20ms. This is probably atypical, but not completely crazy or rigged or anything.

Test Plan: Viewed Git, Subversion and Mercurial repositories and observed dramatically better performance in Git and Mercurial as they took advantage of the cache.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, jhurwitz

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9047
2014-05-12 11:47:29 -07:00
epriestley
e34ee684e1 Batch execution of LastModified query
Summary:
Ref T2683. Further reduces query count of last modified loads; we're now at 11 instead of 200+.

(This works in SVN but could be further optimized.)

Test Plan:
Loaded SVN, Mercurial, Git:

{F34864}
{F34865}
{F34866}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, vrana, aran

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D5256
2014-05-12 11:47:28 -07:00
epriestley
65913162e7 Allow discovered but unparsed commits to be requested in Releeph
Summary:
Ref T3662. Releeph blocks users from requsting unparsed commits, but there's no real technical reason for this.

The `releephwork.getorigcommitmessage` method assumes data exists, but should be replaced with `diffusion.querycommits` anyway.

Test Plan: Ran `diffusion.querycommits`. Requested a commit.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T3662

Differential Revision: https://secure.phabricator.com/D8823
2014-04-20 11:55:29 -07:00
epriestley
969d0c3e8d Use "\z" instead of "$" to anchor validating regular expressions
Summary:
Via HackerOne. In regular expressions, "$" matches "end of input, or before terminating newline". This means that the expression `/^A$/` matches two strings: `"A"`, and `"A\n"`.

When we care about this, use `\z` instead, which matches "end of input" only.

This allowed registration of `"username\n"` and similar.

Test Plan:
  - Grepped codebase for all calls to `preg_match()` / `preg_match_all()`.
  - Fixed the ones where this seemed like it could have an impact.
  - Added and executed unit tests.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8516
2014-03-13 12:42:41 -07:00
epriestley
a4a4322d7a Add a "tags" field to Diffusion commit
Summary:
  - Fixes T4588.
  - See D8501.
  - Adds a "Tags" field for Herald commit emails.
  - Fixes a bug in `tagsquery` when filtering by commit name.
  - Make `tagsquery` just return nothing instead of fataling against Mercurial/Subversion.

Test Plan: Used `bin/repository/reparse.php --herald` to exercise this code.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4588

Differential Revision: https://secure.phabricator.com/D8502
2014-03-12 11:30:52 -07:00
Bob Trahan
740757fd9b Diffusion - add ToC for readme files
Summary: see title. Fixes T4549.

Test Plan: made a readme that had some headers and observed a nice ToC

Reviewers: chad, epriestley

Reviewed By: epriestley

Subscribers: aran, epriestley, Korvin

Maniphest Tasks: T4549

Differential Revision: https://secure.phabricator.com/D8490
2014-03-11 15:52:16 -07:00
epriestley
dfd53fb008 Return "uri" field in diffusion.querycommits
Summary: See IRC transcript.

Test Plan: {F110975}

Reviewers: btrahan, spicyj

Reviewed By: spicyj

CC: aran

Differential Revision: https://secure.phabricator.com/D8172
2014-02-08 16:32:20 -08:00
Richard van Velzen
5771d13952 Speed up diffusion_browsequery for mercurial repositories
Summary:
Ref T4387. By using `hg locate` to attempt to only list files in the given path
browsing diffusion is a bit faster. In a repo of about 600M it shaves a rough 100ms
off viewing the root of the project.

Test Plan: Looked around in diffusion and saw it showed everything including .files, which was nice

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4387

Differential Revision: https://secure.phabricator.com/D8163
2014-02-07 09:39:49 -08:00
Vlad Albulescu
2d27324bef Basic filename search support for Diffusion
Summary:
Ref T156. Adds basic filename search support for Diffusion,
currently only for Git repositories.

This is preliminary, and it's up for discussion:
  - is the UI in the right place;
  - what should the search query syntax be (e.g. whether
    to put `*`s in the beginning and end of it);
  - how to best approach it for Mercurial and/or SVN;
  - what's the cleanest result format for `lsquery` (I went
    for the minimum necessary change to `DiffusionBrowseSearchController`).

Test Plan:
Browse to a repository in Diffusion, and use both
`Search File Names` and `Search File Content`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T156

Differential Revision: https://secure.phabricator.com/D8093
2014-02-01 08:33:03 -08:00
epriestley
ffed36fd99 Minor, fix a method signature warning. 2014-01-29 07:29:35 -08:00
epriestley
e9be9ecfbc Add a "branches" rule for commits
Summary:
Fixes T1353. Also some minor unrelated cleanup:

  - `openTransaction()` / `saveTransaction()` exist now, fix TODOs.
  - Fix some instructions.
  - Make `diffusion.branchquery` return empty for SVN rather than fataling.

Test Plan:
  - Added a branches rule.
  - Ran a dry run against commits in different VCSes.

{F105574}

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, Nopik

Maniphest Tasks: T1353

Differential Revision: https://secure.phabricator.com/D8086
2014-01-28 14:43:13 -08:00
epriestley
d112b6c15d Add diffusion.querycommits and deprecate diffusion.getcommits
Summary:
Fixes T4344. `diffusion.getcommits` is nasty old bad news. Implement a modern query method.

This method provides limit/paging in a somewhat abstract way so it's sort of ultramodern, but I didn't want the default behavior to return a million rows. I'll probably move more stuff toward this over time, now that cursor paging is pervasive. Here, we needed extra metadata (the identifier map) anyway.

Test Plan: Used console to execute command.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4344

Differential Revision: https://secure.phabricator.com/D8077
2014-01-27 17:14:21 -08:00
epriestley
35ccda922a Merge diffusion.commitbranchesquery into diffusion.branchquery
Summary:
Ref T4327. This is general cleanup since I was in this area of the code. Primarily, the Mercurial implementation here was completely broken and wrong:

  - It returned only one branch, but a commit can be present on many branches.
  - It did not account for multiple branch heads.
  - It returned a result implying the branch head pointed at the queried commit, which is no consistent or accurate.

Simplify the amount of API we're dealing with by collapsing this method into the very similar `diffusion.branchquery` method.

Test Plan: Looked at mercurial and git repositories and commits, branch information seemed correct.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4327

Differential Revision: https://secure.phabricator.com/D8003
2014-01-17 16:11:04 -08:00