1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-11 17:32:41 +01:00
Commit graph

844 commits

Author SHA1 Message Date
epriestley
02c3200867 Respond more gracefully when a git push deletes a nonexistent ref
Summary:
Fixes T5534. If you `git push origin :refs/tags/doesnotexist` (for some non-existing tag), we get a change where both the old and new refs are empty.

We incorrectly call this an "add", because the old ref is empty. Instead, call this a "delete", but skip the logic which would normally mark it dangerous.

(Possibly we should just reject these outright, but Git allows them, so stick with that for now.)

Test Plan:
Pushed nonexistent refs:

```
  $ git push origin :refs/tags/doesnotexist
  remote: warning: Allowing deletion of corrupt ref.
  To ssh://dweller@localhost/diffusion/POEMS/
   - [deleted]         doesnotexist
  $
```

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5534

Differential Revision: https://secure.phabricator.com/D9800
2014-07-10 10:17:17 -07:00
epriestley
16648c28bc Add GROUP BY to commit query
Summary:
Ref T4715. Some minor stuff I caught locally while poking around:

  - Since we don't `GROUP BY`, we can still get duplicate commits. These get silently de-duplicated by `loadAllFromArray()` because that returns an array keyed by `id`, but we fetch too much data and this can cause us to execute too many queries to fill pages. Instead, `GROUP BY` if we joined the audit table.
  - After adding `GROUP BY`, getting the audit IDs out of the query is no longer reliable. Instead, query audits by the commit PHIDs. This is approximately equiavlent.
  - Since we always `JOIN`, we currently never return commits that don't have any audits. If we don't know that all results will have an audit, just `LEFT JOIN`.
  - Add some `!== null` to catch the `withIDs(array())` issue that we hit with Khan Academy a little while ago.

Test Plan:
  - Verified that "All Commits" shows commits with no audits of any kind.
  - Verified that the raw data comes out of the query without duplicates.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5433, T4715

Differential Revision: https://secure.phabricator.com/D8879
2014-07-10 10:16:26 -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
46d9bebc84 Remove all device = true from page construction
Summary: Fixes T5446. Depends on D9687.

Test Plan: Mostly regexp'd this. Lint doesn't complain.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley, hach-que

Maniphest Tasks: T5446

Differential Revision: https://secure.phabricator.com/D9690
2014-06-23 15:18:14 -07:00
epriestley
ca6bd26475 Set device to false for all pages which don't specify device readiness
Summary:
Ref T5446.

  - For all callsites which do not specify a value, set `false` explicitly.
  - Make `true` the default.

Test Plan: Used `grep`, then manually went through everything.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5446

Differential Revision: https://secure.phabricator.com/D9687
2014-06-23 15:15:11 -07:00
Joshua Spence
13fa199090 Remove trailing whitespace.
Summary: OMG!!! Trailing whitespace.

Test Plan: No more trailing whitespace.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9688
2014-06-24 03:23:44 +10:00
epriestley
b20884a842 Substantially support character encodings and "Highlight As" in changesets
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.

Test Plan: (See screenshots...)

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T832, T4045, T5179

Differential Revision: https://secure.phabricator.com/D9294
2014-06-20 11:49:41 -07: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
Gareth Evans
f1fa8fccb6 Fix old icon css
Summary:
Updated some old css to point at the new icon set
Fixes T5357

Test Plan: View it

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T5357

Differential Revision: https://secure.phabricator.com/D9578
2014-06-16 12:26:52 -07:00
Chad Little
27c2299407 Remove double border on tables in object boxes
Summary: The CSS rule tends to miss many tables, make the rule more universal and add borders as needed.

Test Plan: Test a Revision and Diffusion

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9516
2014-06-13 11:36:01 -07:00
James Rhodes
ed76c2be1d Implement showing buildable status in Diffusion
Summary: This implements showing the buildable status in Diffusion and unifies some of the logic used to calculate and render build and buildable statuses.

Test Plan: Looked at diffs and commits with statuses, they rendered fine.  Looked at Diffusion and saw buildable status appear (with a manual buildable and manual buildables included in the query).

Reviewers: #blessed_reviewers, epriestley, chad

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9496
2014-06-14 02:28:00 +10:00
epriestley
d401036bd8 Prevent the use of file:// URIs in Diffusion
Summary:
Via HackerOne. There are two attacks here:

  - Configuring mirroring to a `file://` URI to place files on disk or overwrite another repository. This is not particularly severe.
  - Configuring cloning from a `file://` URI to read repositories you should not have access to. This is more severe.

Historically, repository creation and editing explicitly supported `file://` URIs to deal with use cases where you had something else managing repositories on the same machine. Since there were no permissions, repository management was admin-only, and you couldn't mirror, this was fine.

As we've evolved, this use case is a tiny minority use case and the security implications of `file://` URIs overwhelm the utility it provides. Prevent the use of `file://` URIs. Existing configured repositories won't stop working, you just can't add any new ones.

Also prevent `localPath` from being set via Conduit (see T4039).

Test Plan:
  - Tried to create a `file://` repository.
  - Tried to create a `file://` mirror.
  - Tried to create a `file://` repository via Conduit.
  - Created a non-`file://` repository.
  - Created a non-`file://` mirror.
  - Created a non-`file://` repository via Conduit.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9513
2014-06-13 07:07:00 -07:00
epriestley
993b73596a Fix wording of 'bin/remove' prompt for repositories
Summary: This UI recommends `bin/remove destroy X`, but should recommend `bin/remove destroy rX` (with `r`), because the remove script now takes any object monogram. The older script was repository-specific, so it only took the callsign.

Test Plan: {F166042}

Reviewers: putnam, chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9512
2014-06-13 07:06:53 -07:00
Joshua Spence
d0128afa29 Applied various linter fixes.
Summary: Applied some more linter fixes that I previously missed because my global `arc` install was out-of-date.

Test Plan: Will run `arc unit` on another host.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9443
2014-06-09 16:04:12 -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
0d87fef573 Fix an issue where Mercurial pushes would consider only the first and last commits
Summary:
Fixes T5197. `hg log --rev x --rev y` means "rev x, and also rev y".

Use `--rev x:y`, which means "all commits between x and y, inclusive".

Test Plan: Pushed 4 commits at once, got 4 commits in push log.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5197

Differential Revision: https://secure.phabricator.com/D9309
2014-06-03 17:08:13 -07:00
epriestley
66af361f10 Fix a Mercurial issue where split heads would be detected incorrectly
Summary: Ref T5197. When searching for split branch heads, we incorrectly consider descendant heads of other branches. This can cause us to detect a split tip when one does not exist (the old tip is the branch tip, but other descendant heads exist). Instead, consider only heads on the same branch.

Test Plan:
Repro is something like this:

  - `hg update default`
  - `hg branch branch1; hg commit ...`
  - `hg push`
  - `hg update default; hg commit ...`
  - `hg push` - Previously, we would find the head of `branch1` and incorrectly account for it as a head of `default`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5197

Differential Revision: https://secure.phabricator.com/D9308
2014-06-03 17:07:49 -07:00
epriestley
2264c2b4b5 Merge the new navigation design
For discussion, see T5241.
2014-06-03 15:50:08 -07:00
epriestley
6df1a02413 (Redesign) Clean up older "Tile" code
Summary:
This does some backend cleanup of the tile stuff, and some general cleanup of other application things:

  - Users who haven't customized preferences get a small, specific set of pinned applications: Differential, Maniphest, Diffusion, Audit, Phriction, Projects (and, for administrators, Auth, Config and People).
  - Old tile size methods are replaced with `isPinnnedByDefault()`.
  - Shortened some short descriptions.
  - `shouldAppearInLaunchView()` replaced by less ambiguous `isLaunchable()`.
  - Added a marker for third-party / extension applications.

Test Plan: Faked away my preferences and viewed the home page, saw a smaller set of default pins.

Reviewers: chad

Reviewed By: chad

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D9358
2014-06-03 15:47:27 -07:00
Joshua Spence
0d03bbe43c Allow repositories to be deleted using ./bin/remove.
Summary: Currently, repositories can be deleted using `./bin/repository delete`. It makes sense to expose this operate to the `./bin/remove` script as well, for consistency.

Test Plan: Deleted a repository with `./bin/remove rTEST`.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9350
2014-06-02 17:11:58 -07:00
epriestley
81d95cf682 Make default view of "Applications" app a full-page launcher
Summary:
This probably needs some tweaks, but the idea is to make it easier to browse and access applications without necessarily needing them to be on the homepage.

Open to feedback.

Test Plan:
(This screenshot merges "Organization", "Communication" and "Core" into a single "Core" group. We can't actually do this yet because it wrecks the homepage.)

{F160052}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T5176

Differential Revision: https://secure.phabricator.com/D9297
2014-05-29 12:17:54 -07:00
epriestley
2aef04a78a Fix Diffusion blame/highlight for logged-out users
Summary:
Fixes T5199. We try to save these options in user preferences, but logged-out users don't have preferences.

Instead, just use GET links for logged-out users.

Test Plan:
  - As a logged-out user, toggled blame and highlight on and off.
  - As a logged-in user, toggled blame and highlight on and off.

Reviewers: btrahan, vrana

Reviewed By: vrana

Subscribers: epriestley

Maniphest Tasks: T5199

Differential Revision: https://secure.phabricator.com/D9310
2014-05-27 17:37:26 -07:00
Aviv Eyal
9bba4cda2f Diffusion browser: Update editor link when clicking on a line
Summary: Highlighing and URL are fixed on click - now the edit button too.

Test Plan: click on lines with and without value in "Editr Link" (And without %l in it).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9227
2014-05-22 15:33:30 -07:00
epriestley
38b17157fa Use stable commit identifier to load repository commit
Summary: Fixes T5113. This was caught in the crossfire of cleaning up the DiffusionRequest "commit" properties.

Test Plan: Loaded `/rXnnnn` with some of the `nnn` missing.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5113

Differential Revision: https://secure.phabricator.com/D9253
2014-05-22 10:39:06 -07:00
Chad Little
3a81f8c68d Convert rest of SPRITE_STATUS to FontAwesome
Summary:
Updates policy, headers, typeaheads to FA over policy icons

Need advice - can't seem to place where icons come from on Typeahead? Wrong icons and wrong colors.... it is late

Test Plan:
- grepped for SPRITE_STATUS
- grepped for sprite-status
- grepped for setStatus for headers
- grepped individual icons names

Browsed numerous places, checked new dropdowns, see pudgy people.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4739

Differential Revision: https://secure.phabricator.com/D9179
2014-05-18 16:10:54 -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
Chad Little
31cd9b2169 Update PHUIStatusItemView to FontAwesome
Summary: Changes to using FontAwesome

Test Plan:
Testing UIExamples and each of the pages (except releelph)

{F155942}

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9157
2014-05-16 18:59:02 -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
0ad0669916 Allow branch deletions to be pushed in Mercurial
Summary: Fixes T5050. This might not be 100% right in all edge cases, but it worked on everything I tried.

Test Plan:
  - Pushed a branch deletion.
  - Pushed a branch creation.
  - Pushed a brnach creation + deletion.
  - Pushed a brnach deletion + creation.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5050

Differential Revision: https://secure.phabricator.com/D9122
2014-05-14 16:59:15 -07:00
epriestley
fe0c98facc Pass correct number of parameters to pht() when closing a branch in Mercurial
Summary: Ref T5050. This fixes the immediate error (bad pht()) but doesn't fix the other error (can't `--close-branch`) yet.

Test Plan: Pushed a `--close-branch` commit, got a first-level error instead of an error about an error.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T5050

Differential Revision: https://secure.phabricator.com/D9119
2014-05-14 16:59:02 -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
82102cd95a Move Push log rendering to SearchEngine
Summary: Ref T4986. Move push logs to a View, then have all the stuff that needs to use it use that View.

Test Plan: Viewed push logs and transaction detail in Diffusion. Created a panel.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4986

Differential Revision: https://secure.phabricator.com/D9104
2014-05-13 14:00:24 -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
b80b851600 Throw a more tailored exception after failing to resolve a ref
Summary: Ref T2683. Throw a more tailored exception to allow callers to distinguish between bad refs (which are expected, if users try to visit garbage branches) and other types of errors.

Test Plan: Tried to view branch "alksndfklansdf". Viewed branch "master".

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9094
2014-05-13 13:52:33 -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
112c9e6b5e Rename "commitType" to "symbolicType"
Summary:
Ref T2683. The old name was a bit confusing because it meant "the type of the thing the symbol represents": a "commit type" should logically always be "commit".

(Currently, this is only used to detect when we're looking at a tag.)

Test Plan: Looked at a tag. Looked at some other non-tag things. Browsed around, `grep`.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9092
2014-05-13 13:52:03 -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
Chad Little
0120388a75 Found some missing icons
Summary: Did a more exhaustive grep on setIcon and found 99.9% of the icons.

Test Plan: I verified icon names on UIExamples, but unable to test some of the more complex flows visually. Mostly a read and replace.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9088
2014-05-13 07:45:39 -07:00
epriestley
77b4c3145a Simplify Diffusion main view
Summary:
Currently, Diffusion has very complex views. After three years I'm not really used to them and rarely use many of these options.

Simplify the browse and history views:

  - Put the browse view on top.
  - Move dates to the right.
  - Remove "History" and "Edit" links from the browse view. You can access these actions by clicking the file/path.
  - Remove "Browse" link from the history view. You can access this action by clicking the commit.
  - Remove "Change Type", which is essentially never useful, from the history view.
  - Add some tweaks for mobile.

Test Plan: {F153931}

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley, zeeg

Differential Revision: https://secure.phabricator.com/D9085
2014-05-12 19:57:12 -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
53e9df8a02 Slightly reduce the cost of resolving refs
Summary: Ref T2683. By resolving the stable name earlier, we can save a resolve when viewing branch heads. This is ~100ms in Mercurial, and roughly 25% of page weight. It's less bad in Git.

Test Plan: Saw page cost go down in "Services" tab, particularly for Mercurial browse views.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D9048
2014-05-12 11:47:30 -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
e03deb7d4a Always pull extra browse information over Ajax, and batch some of the queries
Summary:
This code is currently quite complicated because we pull history data inline for SVN files, and via ajax for everything else (SVN dirs, everything in Git and Hg).

Always pull over ajax; batch some of the queries.

Test Plan: {F34860}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, vrana, aran

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D5255
2014-05-12 11:47:27 -07:00
epriestley
df59f4b047 Batch all supplementary information in Diffusion browse views
Summary: Ref T2683. Instead of sending one request for each path's history, send one request for all of it. This permits optimizations which are not currently available to us. It degrades the user experience a tiny bit in theory, but on my machine it's actually way faster already.

Test Plan: Loaded a browse page.

Reviewers: vrana, btrahan

Reviewed By: btrahan

Subscribers: epriestley, aran

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D5254
2014-05-12 11:47:26 -07:00
epriestley
ac020bc420 Implement a lint count query
Summary: Ref T2683. This query is currently unbatched and happens inside a view. Leave it inside the view for now, but separate it and make it batchable.

Test Plan: {F34848}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley, vrana, aran

Maniphest Tasks: T2683

Differential Revision: https://secure.phabricator.com/D5252
2014-05-12 11:47:25 -07:00
Chad Little
b2f3001ec4 Replace Sprite-Icons with FontAwesome
Summary: The removes the sprite sheet 'icons' and replaces it with FontAwesome fonts.

Test Plan:
- Grep for SPRITE_ICONS and replace
- Grep for sprite-icons and replace
- Grep for PhabricatorActionList and choose all new icons
- Grep for Crumbs and fix icons
- Test/Replace PHUIList Icon support
- Test/Replace ObjectList Icon support (foot, epoch, etc)
- Browse as many pages as I could get to
- Remove sprite-icons and move remarkup to own sheet
- Review this diff in Differential

Reviewers: btrahan, epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin, hach-que

Differential Revision: https://secure.phabricator.com/D9052
2014-05-12 10:08:32 -07:00
Jacob Hurwitz
9d0d1ac42f Speed up DiffusionBrowseFileController by removing call to array_merge
Summary: Some profiling using XHProf in the Dark Console showed me that Diffusion was wasting a ton of time on array_merge. This change sped up the loading of a large file in Diffusion from 16.8 seconds to 2.4 seconds.

Test Plan: Load files in Diffusion. They all look good. Also, use a PHP shell to try to manually verify that I still kinda remember some PHP and, yes, this is functionally equivalent to what was there before.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D9032
2014-05-09 18:06:29 -07:00
epriestley
e6aff100f2 Move even more rendering into SearchEngine
Summary: Ref T4986. I think this is the last of the easy ones, there are about 10 not-quite-so-trivial ones left.

Test Plan:
  - Viewed app results.
  - Created panels.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4986

Differential Revision: https://secure.phabricator.com/D9025
2014-05-09 12:28:02 -07:00
epriestley
78b89711cb Move a bunch more rendering into SearchEngine
Summary: Ref T4986. These are mostly mechanical now, I skipped a couple of slightly tricky ones. Still a bunch to go.

Test Plan:
For each engine:

  - Viewed the application;
  - created a panel to issue the query.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4986

Differential Revision: https://secure.phabricator.com/D9017
2014-05-08 20:04:19 -07:00
Yonas Yanfa
a928caf41d Fix typos on Import Repository page
See: <https://github.com/facebook/phabricator/pull/590>

Reviewed by: epriestley
2014-05-05 14:28:49 -07:00
epriestley
74faacee4d Never try to run README as a commit hook
Summary:
Fixes T4960. Users `chmod +x` this, and then bash chokes on it.

Phabricator "owns" this file anyway, so there is no real ambiguity here: this should never be a hook script.

Test Plan:
  - Did `chmod +x README`.
  - Made a commit.
  - Added `z.sh`, got blocked.

Reviewers: btrahan, chad

Reviewed By: chad

Subscribers: epriestley

Maniphest Tasks: T4960

Differential Revision: https://secure.phabricator.com/D8981
2014-05-05 10:54:53 -07:00
epriestley
3fde020049 Make many actions require high security
Summary:
Ref T4398. Protects these actions behind a security barrier:

  - Link external account.
  - Retrieve Conduit token.
  - Reveal Passphrase credential.
  - Create user.
  - Admin/de-admin user.
  - Rename user.
  - Show conduit certificate.
  - Make primary email.
  - Change password.
  - Change VCS password.
  - Add SSH key.
  - Generate SSH key.

Test Plan: Tried to take each action and was prompted for two-factor.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4398

Differential Revision: https://secure.phabricator.com/D8921
2014-04-30 17:44:59 -07:00
epriestley
366861f106 Revert the global "not imported yet" warning in Diffusion until we have better support
Summary:
Partially reverts D8903. This was hacky to begin with, but completely breaks if the filetree is enabled (`$view` is not an array).

Just toss it until we have a more structured way to insert it into the document properly. I don't think it's especially important (the Herald warning is way more important).

Test Plan: Multiple users reported that stuff is no longer broken.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8914
2014-04-30 11:39:14 -07:00
epriestley
d5f874b493 Unfatal "Create Repository" UI
See: <https://github.com/facebook/phabricator/issues/584>

Not all controllers in Diffusion have a DiffusionRequest.

Auditors: btrahan
2014-04-30 03:47:54 -07:00
James Rhodes
466af33147 Fix Diffusion crash
Summary: This fixes a crash that happens when visiting Diffusion pages due to an undefined variable.  `$title` is only defined if it has a status to show, but then it uses it anyway and fails.

Test Plan: Pages stopped crashing and people stopped complaining.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8906
2014-04-29 19:06:52 -07:00
Aviv Eyal
31580f19d9 fix query for doorkeeper
Summary: Got exception in daemon logs.

Test Plan: rerun tasks, not exception.

Reviewers: btrahan, epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8905
2014-04-29 15:12:04 -07:00
Bob Trahan
7ed28dacb5 Diffusion + Herald - warn users if importing repository
Summary: 'cuz things fail a bunch until importing is done. Fixes T4094.

Test Plan: set isImporting to return true. Browsed Diffusion and saw helpful warnings everywhere. Browse Herald transcript and saw a helpful warning

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4094

Differential Revision: https://secure.phabricator.com/D8903
2014-04-29 15:07:00 -07:00
epriestley
8716e734f0 Make JOIN changes to CommitQuery only
Summary:
Fixes T4911. See D8879. This gives us the correct query in cases where there are no audits.

This doesn't try to do the GROUP BY stuff yet.

Test Plan:
  - Viewed a commit in Diffusion with no audits, got a commit detail page.
  - Viewed "All Commits" in web UI, saw commits without any audits included in the list.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4911

Differential Revision: https://secure.phabricator.com/D8882
2014-04-28 08:25:51 -07:00
Bob Trahan
2ecc04c159 Audit - move over to application search
Summary: ...also kills off "PhabricatorAuditCommitQuery" and "PhabricatorAuditQuery", by moving the work to "DiffusionCommitQuery". Generally cleans up some code around the joint on this too. Also provides policies for audit requests, which is basically the policy for the underlying commit. Fixes T4715. (For the TODO I added about files, I just grabbed T4713.)

Test Plan:
Audit: verified the three default views all showed the correct things, including highligthing. did some custom queries and got the correct results.
Diffusion: verified "blame view" still worked. verified paths were highlighted for packages i owned.
Home: verified audit boxes showed up with proper commits w/ audits
bin/audit: played around with it via --dry-run and got the right audits back

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4715

Differential Revision: https://secure.phabricator.com/D8805
2014-04-27 09:43:05 -07:00
Kyle Jao
e8c6c81b6e Fix for showing an unregistered author name in the tooltip of diffusion revision link
Summary:
When showing contents of a file with the blame mode enabled, tooltips pops out
when the mouse hovers over previous commit linkes on left side. The last part of the
tooltips is the author's name. If an author is unregistered, the name becomes
<span>name</span>.

{F147724}

This doesn't happen if the author is registered.

Test Plan:
Check tooltips after making the change.
{F147725}

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8869
2014-04-26 12:51:48 -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
f1245f4f34 Remove flavor text for action buttons
Summary: A small but appreciable number of users find flavor on buttons confusing. Remove this flavor. This retains flavor in headers, error messages, etc., which doesn't cause confusion.

Test Plan: Looked at a revision, task, paste, macro, etc.

Reviewers: chad, btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8812
2014-04-18 17:51:46 -07:00
lkassianik
01552d85de Show Projects bucket unconditionally in repository summary screen
Summary: fixes T4753

Test Plan: looked at repository with projects, looked at repository with no projects

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4753

Differential Revision: https://secure.phabricator.com/D8730
2014-04-08 19:48:31 -07:00
Ben Alpert
a7272dfb03 Switch back to zwsp for oncopy line marker
Summary:
Fixes T4759.

Turns out Chrome on windows doesn't really like the word joiner character. We'll switch back to zwsp but make it `position: absolute;` so it doesn't turn into a line break.

Test Plan: Looked at diffs in IE9 and Chrome Windows. Made sure copying still works as expected.

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4759

Differential Revision: https://secure.phabricator.com/D8727
2014-04-08 17:55:48 -07:00
Bob Trahan
c408168c25 Diffusion - Warn users to explicitly provide PATH for SVN hosted repositories
Summary: Fixes T4547.

Test Plan: saw the warning, looked good

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4547

Differential Revision: https://secure.phabricator.com/D8706
2014-04-04 12:47:10 -07:00
Bob Trahan
f67a853fe7 Audit - add ability to add a package as an auditor
Summary: Fixes T4687. This was also pretty easy...!

Test Plan: made a package with a test user as owner. added package as owner. looked right on commit page. logged in as test user and verified audit showed up on home page.

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: chad, epriestley, Korvin

Maniphest Tasks: T4687

Differential Revision: https://secure.phabricator.com/D8705
2014-04-04 12:25:03 -07:00
Bob Trahan
6b5308c981 Audit - add ability to add user or projects as auditors
Summary: Ref T4687. Trickier part is adding packages; will require some typeahead core changes

Test Plan: add a project as an auditor succuessfully!

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: epriestley, Korvin

Maniphest Tasks: T4687

Differential Revision: https://secure.phabricator.com/D8704
2014-04-04 11:29:10 -07:00
Ben Alpert
9fedd343eb Break long words in differential two-up view
Summary: This should prevent long lines from making the code width different between files, which can be annoying. (And of course, it stops long lines from making a giant scrollbar too.)

Test Plan:
Loaded this diff in Chrome, Firefox, IE9, and IE8:

{F137505}

(That's a screenshot from Chrome, but it looks about the same in the other browsers.)

Reviewers: chad, #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin, chad

Maniphest Tasks: T2004

Differential Revision: https://secure.phabricator.com/D8686
2014-04-03 09:40:00 -07:00
epriestley
04b9f94602 Give administrators selective access to System Agent settings panels
Summary: Ref T4065. Give administrators an "Edit Settings" link from profiles, which allows selective edit of settings panels. Enable Conduit, SSH Keys, and VCS Password.

Test Plan:
  - Used these panels for a bot.
  - Used these panels on my own account.
  - Tried to use these panels for a non-bot account, was denied.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4065

Differential Revision: https://secure.phabricator.com/D8668
2014-04-02 12:06:05 -07:00
epriestley
a6b1fac117 Fix SVN translation of "add-file" protocol frames over SSH
Summary: Fixes T4697. When pushing moved/copied files, SVN sends an "add-file" protocol frame which has a URI in it that needs translation from external format ("/diffusion/X/") to internal format ("/path/to/svn").

Test Plan:
  - Copied/moved files and committed them in SVN.
  - Added files (no copy/move) and committed them in SVN.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4697

Differential Revision: https://secure.phabricator.com/D8654
2014-04-01 08:23:48 -07:00
epriestley
1aad40b7bf Allow users to receive email about pushes via Herald
Summary:
Fixes T4677. Implements a "send an email" pre-receive action, which sends push summaries.

For use cases where features are often pushed as a large number of commits (e.g., checkpoint commits are retained), using commit emails means users get a ton of email. Instead, this allows you to get an email about a push, which summarizes what changed.

Overall, this is basically the same as commit email, but more suitable for some workflows.

Test Plan:
Wrote some rules, then made a bunch of pushes. Got email like this:

{F134929}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8618
2014-03-26 13:51:15 -07:00
epriestley
75c47c6ae0 Provide an "event" page for push logs, which shows details on all events in a given push
Summary:
Ref T4677. This shows a more detailed view of an entire "git push", "hg push", or "svn commit".

This is mostly to give push summary emails a reasonable, stable URI to link to for T4677.

Test Plan:
  - Pushed into SVN, Git and Mercurial.
  - Viewed partial and imported event records.

{F134864}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8616
2014-03-26 13:51:09 -07:00
epriestley
a5f55d506f Provide a real object ("PhabricatorRepositoryPushEvent") to represent an entire push transaction
Summary:
Ref T4677. Currently, we record individual actions in a push as PhabricatorRepositoryPushLogs, but tie them together only loosely with a `transactionKey`.

Provide a real PushEvent object, and move some of the denormalized fields to it. This primarily just gives us more robust infrastructure for building, e.g., email about pushes, for T4677, since we can act on real PHIDs rather than passing awkward identifiers around.

Test Plan:
  - Performed migration.
  - Looked at database for consistency.
  - Browsed/queried push logs.
  - Pushed a bunch of stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8615
2014-03-26 13:51:06 -07:00
epriestley
38cc38eaf6 Modernize documentation links
Summary:
  - Point them at the new Diviner.
  - Make them a little less cumbersome to write.

Test Plan: Found almost all of these links in the UI and clicked them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Differential Revision: https://secure.phabricator.com/D8553
2014-03-17 15:01:31 -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
d27cd5fb99 Disable Herald more aggressively when it's turned off for a repository
Summary:
Currently, disabling Herald only disables feed, notifications and email. Historically, audits didn't really create external effects so it made sense for Herald to only partially disable itself.

With the advent of Harbormaster/Build Plans, it makes more sense for Herald to just stop doing anything. When this option is disabled, stop all audit/build/publish/feed/email actions for the repository.

Test Plan: Ran `scripts/repository/reparse.php --herald`, etc.

Reviewers: dctrwatson, btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Differential Revision: https://secure.phabricator.com/D8509
2014-03-12 18:16:50 -07:00
epriestley
193e8a54fc Add "pusher is committer" to Herald as a pre-commit rule
Summary:
Fixes T4594. Also, allow "exists" / "does not exist" to be run against author/committer. This allows construction of rules like:

  - Committer identities must be authentic.
  - Committer identities must be resolvable.
  - Author identities must be resolvable.

Test Plan: Created some rules using these new rules and ran them.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: aran, epriestley

Maniphest Tasks: T4594

Differential Revision: https://secure.phabricator.com/D8507
2014-03-12 15:24:33 -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
Michael Peters
8b6c86e27d Fix the script that saves lint for a repo into the database and updates diffusion.
Summary:
It appears a change to the way the configuration was loaded into ArcanistRepositoryAPI in rARCa2285b2b broke the save_lint script.
This updates the DiffusionLintSaveRunner to use the configuration correctly, allowing the linter to run

Test Plan: cd /your/project; ../../../path/to/phabricator/scripts/repository/save_lint.php

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: aran, epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D8487
2014-03-11 13:07:45 -07:00
epriestley
3910d0d5e1 Remove field selector on Diff view and Revision List View
Summary:
Ref T2222. This has some minor functionality regressions:

  - The plain diff page no longer shows unit/test status. I want to give diffs separate custom fields for this.
  - It was technically possible to shove more data on the list view, although this doensn't affect the default config.

Test Plan: Looked at list view, diff detail view. Grepped for changes.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T2222

Differential Revision: https://secure.phabricator.com/D8470
2014-03-11 13:02:10 -07:00
Joshua Spence
e11adc4ad7 Added some additional assertion methods.
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.

This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.

Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8460
2014-03-08 19:16:21 -08:00
Peng Li
81d9935efe Allow parenthesis in author name
Summary: We have a dozen users who has `(...)` in their 'real name', like 'Jimmy (He) Zhang', and it's causing the diffusion file browser problems when blame is enabled. The parser does not expect those parenthesis and the lines of code will be empty if they were last touched by a user like that.

Test Plan: Try it

Reviewers: wez, lifeihuang, JoelB, #blessed_reviewers, epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8429
2014-03-06 11:28:46 -08:00
epriestley
a3c1dcb928 Produce a more tailored checkout URI for Subversion
Summary:
For imported SVN repositories with an "Import Only" path, we produce a `/path/to/root/` URI, but should produce `/path/to/root/then/to/import/only/`.

As it is, the URI instructs the user to check out the whole repository.

Also, don't show the "Clone As" fragment in the URI for remote repositories, and prevent it from being edited for nonhosted repositories. This is generally more consistent with user expectation.

Test Plan:
  - Created a remote SVN repository with "Import Only", saw path include it.
  - Verified no "Clone As" options, no "Clone As" in URI.
  - Switched it to hosted, saw "Clone As" options appear and work properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, staticshock

Differential Revision: https://secure.phabricator.com/D8375
2014-02-28 13:04:41 -08:00
Joshua Spence
6270114767 Various linter fixes.
Summary:
- Removed trailing newlines.
- Added newline at EOF.
- Removed leading newlines.
- Trimmed trailing whitespace.
- Spelling fix.
- Added newline at EOF

Test Plan: N/A

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley

CC: hach-que, chad, Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8344
2014-02-26 12:44:58 -08:00
Chad Little
a4d4bf8196 Add ObjectBox around Diffusion Binary Files
Summary: Add in more ObjectBoxes

Test Plan: Test aphlict.swf, see new menu and button to download.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8305
2014-02-22 14:08:04 -08:00
Chad Little
8662b27f89 Move raw file download icon to file box in Diffusion
Summary: For images and text, show the "Raw" buttons on the file's ObjectBox

Test Plan: View an image and a text file in Diffusion, click on the download link in each.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4467

Differential Revision: https://secure.phabricator.com/D8302
2014-02-21 15:21:38 -08:00
Chad Little
ce5eafe7f1 Move "Open in Editor" to File Box
Summary: Moves this single action to the File Contents box in Diffusion Browse. Also fixes a PHUIObjectBox missing when enable highlighting is on.

Test Plan: Enable/Disable Highlighting. See disabled Editor button.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4467

Differential Revision: https://secure.phabricator.com/D8300
2014-02-21 14:43:24 -08:00
epriestley
55a94d8aba Don't prompt to upgrade unset passwords
Summary:
Fixes T4463. When your VCS or account password is not set, we test it for upgrade anyway. This doesn't make sense and throws shortly into the process because the empty hash isn't parseable.

Instead, only show upgrade prompts when the password exists.

Test Plan:
  - Added a password to an existing account with no password via password reset.
  - Added a VCS password to an existing account with no VCS password.
  - Observed no fatals / nonsense behaviors.

Reviewers: btrahan, chad

Reviewed By: chad

CC: aran

Maniphest Tasks: T4463

Differential Revision: https://secure.phabricator.com/D8282
2014-02-20 08:12:04 -08:00
epriestley
50ed42761c Don't issue a bazillion queries to load Differential object lists
Summary:
Ref T3496. Currently, we call loadAssets() on each revision table, which invokes a new revision query and a pile of subqueries.

Instead, add `needFlags()` and `needDrafts()` to `RevisionQuery`. Some day these could perhaps be more generic.

Test Plan:
  - Viewed home, differential, etc., no longer saw 9203809238 queries being run for no reason.
  - Drafts and flags still appear properly.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T3496

Differential Revision: https://secure.phabricator.com/D8277
2014-02-18 17:57:45 -08:00
Bob Trahan
dcd7a316d2 Differential - add DifferentialDraft to track whether revisions have draft feedback or not
Summary: ...do it somewhat generically, so we could fairly easily add this to other applications. Fixes T3496. I got a wee bit lazy and decided not to migrate existing drafts. My excuses aside from laziness are doing it this way will let us see if anyone complains, we can always do a migration later if people do complain, and there's likely to be a lot of garbage data for older / bigger installs, and the migration didn't seem worth itgiven it would also likely be expensive in these cases.

Test Plan: made a draft inline comment on DX and observed DX had a note icon on Differential home page. made a draft comment on DX and observed DX had a note icon on Differential home page. deleted a draft inline comment and noted icon disappeared from Differential homepage. Submitted a draft comment + inline comment and noted icon disappeared.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T3496

Differential Revision: https://secure.phabricator.com/D8275
2014-02-18 16:25:16 -08:00
epriestley
b96ab5aadf Modernize VCS password storage to use shared hash infrastructure
Summary: Fixes T4443. Plug VCS passwords into the shared key stretching. They don't use any real stretching now (I anticipated doing something like T4443 eventually) so we can just migrate them into stretching all at once.

Test Plan:
  - Viewed VCS settings.
  - Used VCS password after migration.
  - Set VCS password.
  - Upgraded VCS password by using it.
  - Used VCS password some more.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4443

Differential Revision: https://secure.phabricator.com/D8272
2014-02-18 14:09:36 -08:00
epriestley
a0262c0b4f Remove tokenizer.ondemand, and always load on demand
Summary:
Ref T4420. Tokenizers currently operate in "preload" or "ondemand" modes. In the former mode, which is default, they'll try to load the entire result list when a page loads.

The theory here was that this would slightly improve the experience for small installs, and once they got big enough they could switch to "ondemand". In practice, several issues have arisen:

  - We generally don't have a good mechanism for telling installs that they should tweak perf config -- `metamta.send-immediately` is the canonical example here. Some large installs are probably affected negatively by not knowing to change this setting, and having settings like this is generally annoying.
  - We have way way too much config now.
  - With the advent of ApplicationSearch, pages like Maniphest make many redundant loads to prefill sources like projects. Most of the time, this data is not used. It's far simpler to switch everything to ondemand than try to deal with this, and dealing with this would mean creating two very complex divergent pathways in the codebase for a mostly theoretical performance benefit which only impacts tiny installs.
  - We've been using `tokenizer.ondemand` forever on `secure.phabricator.com` since we have many thousands of user accounts, and it doesn't seem sluggish and works properly.

Removing this config is an easy fix which makes the codebase simpler.

I've retained the ability to use preloaded sources, since they may make sense in some cases (in at least one case -- task priorities -- adding a static source pathway might make sense), and they're part of Javelin itself. However, the code will no longer ever go down that pathway.

Test Plan: Used `secure.phabricator.com` for years with this setting enabled.

Reviewers: btrahan, chad

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T4420

Differential Revision: https://secure.phabricator.com/D8232
2014-02-14 10:24:40 -08:00
Bob Trahan
8739ea4dc1 Diffusion - fix browse file bug
Summary: we were calling a member method on a diffusion hash. not sure why.  Fixes T4402

Test Plan: clicked about, no fatals and seemed to move sensical backwards in time

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Maniphest Tasks: T4402

Differential Revision: https://secure.phabricator.com/D8194
2014-02-11 10:38:27 -08:00
Bob Trahan
1830868007 Herald - make herald condition of herald rule display better
Summary: ...by the surprising step of changing how this data is stored from id to phid. Also a small fix to not allow "disabled" rules to be used as herald rule conditions, i.e. can't make a rule that depends on a disabled rule.

Test Plan: viewed existing herald rule that had a rule condition and noted nice new display using handle. made a new rule that had a rule condition and verified it worked correctly.

Reviewers: epriestley

Reviewed By: epriestley

CC: Korvin, epriestley, aran

Differential Revision: https://secure.phabricator.com/D8186
2014-02-10 14:40:09 -08: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