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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
Summary:
Ref T156. @vlada recently implemented filename search in Diffusion, this cleans up the UI a little bit:
- Instead of showing one search box with two different buttons, let the submit buttons appear to the right of the text boxes and separate the search modes.
- Clean up the results a little bit (don't show columns which don't exist).
Test Plan: {F107260}
Reviewers: vlada, btrahan, chad
Reviewed By: chad
CC: vlada, chad, aran
Maniphest Tasks: T156
Differential Revision: https://secure.phabricator.com/D8125
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
Summary: Fixes T4175. In cases where the arguments have only always-safe characters, we can produce a more human-readable URI.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8100
Summary:
Ref T4175. This allows these URIs to all be valid for Git and Mercurial:
/diffusion/X/
/diffusion/X/anything.git
/diffusion/X/anything/
This mostly already works, it just needed a few tweaks.
Test Plan: Cloned git and hg working copies using HTTP and SSH.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8098
Summary:
Ref T4175.
- Add a configurable name for the clone-as directory, so you can have "Bits & Pieces" clone as "bits~n~pieces/" or simliar.
- By default, use "reasonable" heruistics to choose such a name.
- Generate a copy/pasteable clone commmand with this directory name.
Test Plan: Looked at some repositories.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T4175
Differential Revision: https://secure.phabricator.com/D8097
Summary:
Hosted repositories have muddied this distinction somewhat. In some cases, we only want to use the real remote URI, and the call is only relevant for imported repositories.
In other cases, we want the URI we'd plug into `git clone`.
Move this logic into `PhabricatorRepository` and make the distinction more clear.
Test Plan: Viewed SVN, Git, and Mercurial hosted and remote repositories, all the URIs looked reasonable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D8096
Summary:
A few users have hit this and found it confusing. Currently, it means "more than 99.95%", which is very different from "100%". Instead:
- show an extra digit of precision; and
- cap the display at "99.99%", so it's more clear that work is still happening.
Test Plan: Faked it and saw it cap at 99.99%.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D8058
Summary: Minor, adds the Callsign and changes to cards view when listing repositories.
Test Plan: Reload sandbox list of repositories, see new items.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8036
Summary: Fixes T3238. Ref T4327. Although the instructions are fairly clear on this, it's easy to miss them. Make sure the root the user enters matches the real root.
Test Plan: Added unit tests. Used `bin/repository discover` to hit the check explicitly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3238, T4327
Differential Revision: https://secure.phabricator.com/D8020
This seems to be a specific of how browsers are dealing with
spaces/tabs. Multiple spaces works just fine, but multiple
tabs were treating as a single space which breaks indentation.
Now made it so tabs are replaced with 4 spaces. Not ideal but
still better than fully unreadable code. This also matches to
how differential is handling tabs.
Ref T2495. See: <https://github.com/facebook/phabricator/issues/487>
Reviewed by: epriestley
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
Summary: Ref T4327. At some point these two very similar classes got introduced. Collapse `DiffusionBranchInformation` into the nearly identical `DiffusionRepositoryRef`, which enjoys slightly more generality and support.
Test Plan: Viewed branch overview and detail pages. Ran `repository refs` and `repository discover`. Grepped for removed symbols.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D8002
Summary:
Ref T4327. Simplify the git discovery process so I can move it to the DiscoveryEngine, so I can make change parsing testable.
In particular:
- As an optimization, we process closeable branches ("master") first, then process uncloseable branches ("epriestley-devel"). This means that in the common case we can insert a commit as closeable immediately when it is discovered, the first pass through the pipeline will get it right, and the "ref update" step will never need to do any meaningful work.
- Commits which do not initially appear on a closeable branch, but later move to one (via merges or ref moves) will now be caught in the ref update step, have the closeable flag set, and have a message step re-queued.
- We no longer need to do a separate discovery step on closable branches.
- We no longer need to keep track of `seenOnBranches`.
Test Plan: Ran discovery on repositories after pushing commits, got reasonable results.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7985
Summary: Fixes T3857. Earlier work made this trivial and just left product questions, which I've answered by requiring the daemons to run on reasonable installs.
Test Plan: Ran `bin/search index` and `bin/search index --background`. Observed indexes write in the former case and tasks queue in the latter case. Commented with a unique string on a revision and searched for it a moment later, got exactly one result (that revision), verifying that reindexing works correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3857
Differential Revision: https://secure.phabricator.com/D7966
Summary: This removes the bulk of the "Form Errors" text, some variations likely exists. These are a bit redundant and space consuming. I'd also like to back ErrorView more into PHUIObjectBox.
Test Plan: Test out the forms, see errors without the text.
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran, hach-que
Differential Revision: https://secure.phabricator.com/D7924
Summary: Two basic changes here, first we fixed up the Diffusion headers to roll out more PHUIObjectBoxes. Second we added some specific styles for when Errors are inside an ObjectBox at the first position.
Test Plan: Tested a number of different layouts for browsing respositories as well as wherever I could find cases with PHUIObjectBox Form Errors (see images attached). Still some minor tightening due after this diff, but didnt want to overload it.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7914
Summary:
Updates table design to use new standards, work well in PHUIObjectBox. Fixes T4142
Comma
Test Plan: Tested on Diffusion, Settings, will roll out to more places soon
Reviewers: epriestley, btrahan
CC: Korvin, epriestley, aran
Maniphest Tasks: T4142
Differential Revision: https://secure.phabricator.com/D7901
Summary: Ref T4264. Ref T2628. Ref T3102. Allows you to associate repositories with projects. In the future, you'll be able to write Herald object rules against projects, use Herald fields like "Repository's projects", and search by project.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3102, T4264, T2628
Differential Revision: https://secure.phabricator.com/D7881
Summary:
Repositories currently have a no-UI "shortcut" feature which is only used by Facebook (and I'm not sure it's even used). As implemented, this feature is policy-oblivious and kind of nonsensical. Throw it away.
I'm open to reimplementing this, but I want to see some level of interest in it before I do. The new implementation would add shortcuts to each repository, similar to how mirrors work. My original plan was to follow this up with such an implementation (it's half-implemented in my sandbox), but as I worked through it I'm not sure it's really valuable.
Test Plan: Browsed repository list, grep.
Reviewers: btrahan
Reviewed By: btrahan
CC: FacebookPOC, aran
Differential Revision: https://secure.phabricator.com/D7862
Summary:
Fixes T4242. It's currently possible to set nonsense defaults and create repositories with unintended policies, because policy configuration isn't part of creation. Instead:
- put a policy page into the creation workflow;
- require the selection of valid policies (i.e., prevent creating a repository you can't view / edit).
Test Plan:
- Created imported and hosted repositories, hit policy selection.
- Edited policies of existing repositories.
- Tried to set nonsense policies.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4242
Differential Revision: https://secure.phabricator.com/D7856
Summary: Some discussion on IRC. This is more consistent with other disabled items, which are click-to-explain.
Test Plan: Viewed UI, clicked link.
Reviewers: btrahan, dctrwatson, asherkin
Reviewed By: asherkin
CC: aran
Differential Revision: https://secure.phabricator.com/D7857
Summary:
Fixes T4270. When you download raw file content, diffs, and patches we currently give them default (all users) visibility.
Instead, bind them to the repository or revision in question.
(This code could use a bit of cleanup at some point.)
Test Plan: Hit the patch and content download links in Diffusion and the patch download link in Differential, got restricted files with accurate policy bindings.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4270
Differential Revision: https://secure.phabricator.com/D7849
Summary: Ref T4195. Ref T2783. We have an old-school implementation of this; move it into a LowLevel query and make callers all run through Conduit. I need the LowLevel query for hooks, to implement an "is merge commit" Herald rule.
Test Plan:
- Ran query via Conduit for SVN, Mercurial, Git.
- Parsed a commit which closed a revision, attach/closed worked correctly.
- Browsed Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195, T2783
Differential Revision: https://secure.phabricator.com/D7808
Summary:
There's no particular reason to allow the user to edit the clone URI field in Diffusion; editing it has no meaning and if you fat finger the keyboard, it's quite possible that the user will either accidentally clear and/or modify the URI before copying (bit me this morning).
Adding a readonly attribute to the input field allows the same benefit (URI is easily selectable) while preventing such accidental input. Fixes T4246.
Test Plan: Verified that the desired behavior is present in both Chrome, Safari, and Firefox. Field remains selectable with one click, but field is not editable.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4246
Differential Revision: https://secure.phabricator.com/D7810
Summary: We currently have a lot of calls to `addCrumb(id(new PhabricatorCrumbView())->...)` which can be expressed much more simply with a convenience method. Nearly all crumbs are only textual.
Test Plan:
- This was mostly automated, then I cleaned up a few unusual sites manually.
- Bunch of grep / randomly clicking around.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: hach-que, aran
Differential Revision: https://secure.phabricator.com/D7787
Summary:
Ref T4195. This pulls the central logic of HookEngine up one level and makes all the git stuff genrate PushLogs.
In future diffs, everything will generate PushLogs and we can hand those off to Herald.
Test Plan:
Pushed a pile of valid/invalid stuff:
{F89256}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7761
Summary: We run `git` on a different port than 22, so would like to reflect this change in the UI.
Test Plan: Set diffusion.ssh-port in settings, then make sure it's reflected on the Diffusion repository Clone URI.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran, dctrwatson
Differential Revision: https://secure.phabricator.com/D7755
Summary: This locks push logs down a little bit and makes them slightly more administrative. Primarily, don't show IPs to googlebot, etc.
Test Plan: Viewed push logs as edit and non-edit users.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7722
Summary:
Ref T4195. Like the previous diffs, these both create a useful log and give us an object to hand off to Herald.
Surface this information in Diffusion, too, and clean things up a little bit.
Test Plan: {F87565}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7718
Summary: Ref T4195. Add UI options to filter push logs by pusher and repository. Add a link from the repository view page to the push logs.
Test Plan: Viewed a hosted repository, clicked logs link, saw logs. Filtered lgos by repo/pusher.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7713
Summary: Ref T4195. Stores remote address and protocol in the logs, where possible.
Test Plan: Pushed some stuff, looked at the log, saw data.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7711
Summary:
Ref T4195. This log serves two purposes:
- It's a log, so you can see what happened. Particularly, in Git/Hg, there is no other way to tell:
- Who //pushed// a change (vs committed / authored)?
- When was a change pushed?
- What was the old value of some tag/branch before someone destroyed it?
- We can hand these objects off to Herald to implement pre-commit rules.
This is a very basic implementation, but gets some data written and has a basic UI for it.
Test Plan: {F87339}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4195
Differential Revision: https://secure.phabricator.com/D7705
Summary: Ref T4189. Fixes T2066. Mercurial has a //lot// of hooks so I'm not 100% sure this is all we need to install (we may need separate hooks for tags/bookmarks) but it should cover most of what we're after at least.
Test Plan:
- `bin/repository pull`'d a Mercurial repo and got a hook install.
- Pushed to a Mercurial repository over SSH and HTTP, with good/bad hooks. Saw hooks fire.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2066, T4189
Differential Revision: https://secure.phabricator.com/D7685
Summary:
Ref T4189. This adds SVN support, which was a little more messy than I though. Principally, we can not use `PHABRICATOR_USER` for Subversion, because it strips away the entire environment for "security reasons".
Instead, use `--tunnel-user` plus `svnlook author` to figure out the author.
Also fix "ssh://" clone URIs, which needs to be "svn+ssh://".
Test Plan:
- Made SVN commits through the hook.
- Made Git commits, too, to make sure I didn't break anything.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7683
Summary:
Ref T4189. T4189 describes most of the intent here:
- When updating hosted repositories, sync a pre-commit hook into them instead of doing a `git fetch`.
- The hook calls into Phabricator. The acting Phabricator user is sent via PHABRICATOR_USER in the environment. The active repository is sent via CLI.
- The hook doesn't do anything useful yet; it just veifies basic parameters, does a little parsing, and exits 0 to allow the commit.
Test Plan:
- Performed Git pushes and pulls over SSH and HTTP.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4189
Differential Revision: https://secure.phabricator.com/D7682
Summary: This was broken in rP51fb1ca16d7f.
Test Plan: Imported a repository with file:/// location, it worked.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7636
Summary: Fixes T2230. This isn't a total walk in the park to configure, but should work for early adopters now.
Test Plan: Read documentation, browsed UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7634
Summary:
Ref T4038. This adds everything except the actual pushing part for mirrors.
This isn't the most beautiful or sophisticated UI, but I want get the authoritative repositories self-hosted and get users beta-ing hosting as soon as possible. We can do transactions, etc., later on.
Test Plan: See screenshots.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4038
Differential Revision: https://secure.phabricator.com/D7632
Summary: Fixes T4122. Ref T2230. Instead of storing credentials on each repository, store them in Passphrase. This allows easy creation/management of many repositories which share credentials.
Test Plan:
- Upgraded repositories.
- Created and edited repositories.
- Pulled HTTP and SSH repositories.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230, T4122
Differential Revision: https://secure.phabricator.com/D7629
Summary:
Ref T2230. When fully set up, we have up to three users who all need to write into the repositories:
- The webserver needs to write for HTTP receives.
- The SSH user needs to write for SSH receives.
- The daemons need to write for "git fetch", "git clone", etc.
These three users don't need to be different, but in practice they are often not likely to all be the same user. If for no other reason, making them all the same user requires you to "git clone httpd@host.com", and installs are likely to prefer "git clone git@host.com".
Using three different users also allows better privilege separation. Particularly, the daemon user can be the //only// user with write access to the repositories. The webserver and SSH user can accomplish their writes through `sudo`, with a whitelisted set of commands. This means that even if you compromise the `ssh` user, you need to find a way to escallate from there to the daemon user in order to, e.g., write arbitrary stuff into the repository or bypass commit hooks.
This lays some of the groundwork for a highly-separated configuration where the SSH and HTTP users have the fewest privileges possible and use `sudo` to interact with repositories. Some future work which might make sense:
- Make `bin/phd` respect this (require start as the right user, or as root and drop privileges, if this configuration is set).
- Execute all `git/hg/svn` commands via sudo?
Users aren't expected to configure this yet so I haven't written any documentation.
Test Plan:
Added an SSH user ("dweller") and gave it sudo by adding this to `/etc/sudoers`:
dweller ALL=(epriestley) SETENV: NOPASSWD: /usr/bin/git-upload-pack, /usr/bin/git-receive-pack
Then I ran git pushes and pulls over SSH via "dweller@localhost". They successfully interacted with the repository on disk as the "epriestley" user.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7589
Summary:
Ref T4039. This fixes an issue where a user with the ability to create repositories could view repositories he is otherwise not permitted to see, by following these steps:
- Suppose you want to see repository "A".
- Create a repository with the same VCS, called "B".
- Edit the local path, changing "/var/repo/B" to "/var/repo/A".
- Now it points at a working copy of a repository you can't see.
- Although you won't be able to make it through discovery (the pull will fail with the wrong credentials), you can read some information out of the repository directly through the Diffusion UI, probably?
I'm not sure this was really practical to execute since there are a bunch of sanity checks along most/all of the major pathways, but lock it down since normal users shouldn't be editing it anyway. In the best case, this would make a mess.
Test Plan: {F81391}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4039
Differential Revision: https://secure.phabricator.com/D7580
Summary:
Small step forward which improves existing stuff or lays groudwork for future stuff:
- Currently, to check for email verification, we have to single-query the email address on every page. Instead, denoramlize it into the user object.
- Migrate all the existing users.
- When the user verifies an email, mark them as `isEmailVerified` if the email is their primary email.
- Just make the checks look at the `isEmailVerified` field.
- Add a new check, `isUserActivated()`, to cover email-verified plus disabled. Currently, a non-verified-but-not-disabled user could theoretically use Conduit over SSH, if anyone deployed it. Tighten that up.
- Add an `isApproved` flag, which is always true for now. In a future diff, I want to add a default-on admin approval queue for new accounts, to prevent configuration mistakes. The way it will work is:
- When the queue is enabled, registering users are created with `isApproved = false`.
- Admins are sent an email, "[Phabricator] New User Approval (alincoln)", telling them that a new user is waiting for approval.
- They go to the web UI and approve the user.
- Manually-created accounts are auto-approved.
- The email will have instructions for disabling the queue.
I think this queue will be helpful for new installs and give them peace of mind, and when you go to disable it we have a better opportunity to warn you about exactly what that means.
Generally, I want to improve the default safety of registration, since if you just blindly coast through the path of least resistance right now your install ends up pretty open, and realistically few installs are on VPNs.
Test Plan:
- Ran migration, verified `isEmailVerified` populated correctly.
- Created a new user, checked DB for verified (not verified).
- Verified, checked DB (now verified).
- Used Conduit, People, Diffusion.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D7572
Summary: We don't actually support this yet, so hide the configuration.
Test Plan: Edited branches for an hg repo.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7563
Summary:
Ref T2230. As far as I can tell, getting SVN working over HTTP is incredibly complicated. It's all DAV-based and doesn't appear to have any kind of binary we can just execute and pass requests through to. Don't support it for now.
- Disable it in the UI.
- Make sure all the error messages are reasonable.
Test Plan: Tried to HTTP an SVN repo. Tried to clone a Git repo with SVN, got a good error message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7562
Summary:
Ref T2230. This is substantially more complicated than Git, but mostly because Mercurial's protocol is a like 50 ad-hoc extensions cobbled together. Because we must decode protocol frames in order to determine if a request is read or write, 90% of this is implementing a stream parser for the protocol.
Mercurial's own parser is simpler, but relies on blocking reads. Since we don't even have methods for blocking reads right now and keeping the whole thing non-blocking is conceptually better, I made the parser nonblocking. It ends up being a lot of stuff. I made an effort to cover it reasonably well with unit tests, and to make sure we fail closed (i.e., reject requests) if there are any parts of the protocol I got wrong.
A lot of the complexity is sharable with the HTTP stuff, so it ends up being not-so-bad, just very hard to verify by inspection as clearly correct.
Test Plan:
- Ran `hg clone` over SSH.
- Ran `hg fetch` over SSH.
- Ran `hg push` over SSH, to a read-only repo (error) and a read-write repo (success).
Reviewers: btrahan, asherkin
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7553
Summary: Ref T2230. Fixes T4079. As it turns out, this is Git being weird. See comments for some detials about what's going on here.
Test Plan: Created shallow and deep Git clones.
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4079, T2230
Differential Revision: https://secure.phabricator.com/D7554
Summary:
Fixes T4067. The way `DiffusionCommitQuery` works prevents it from loading SVN identifiers in some cases without additional constraints, since "12345" might be an SVN revision 12345, or it might be the first 5 characters of a Git commit hash.
Introduce `withRepository()` as a shorthand for `withDefaultRepository()` + `withRepositoryIDs()`. This tells the query to:
- Only look in the given repository; and
- use the more liberal identifier resolution rules while doing so.
The practical impact this has is that blame tooltips in SVN work again. The other queries which are fixed here were never run in SVN (which doesn't have first-class branches or tags); I've cleaned them up only for completeness.
Test Plan:
- Viewed blame in SVN, saw information again instead of empty tooltip.
- Viewed brnaches/tags in Mercurial and Git.
{F79226}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4067
Differential Revision: https://secure.phabricator.com/D7523
Summary: Ref T2230. This is easily the worst thing I've had to write in a while. I'll leave some notes inline.
Test Plan: Ran `hg clone http://...` on a hosted repo. Ran `hg push` on the same. Changed sync'd both ways.
Reviewers: asherkin, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7520
Summary: This is starting to get a bit sizable and it turns out Mercurial is sort of a beast, so split the VCS serve stuff into a separate controller.
Test Plan: Pushed and pulled an authenticated Git repository.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, hach-que
Differential Revision: https://secure.phabricator.com/D7494
Summary:
Expands on D7488, which looks way better than the config checks. I'm leaving the config checks for now, but maybe we should just get rid of them? This advice is delivered in a far more timely way.
- Check for normal VCS binaries too.
- Link to `environment.append-paths`.
- Get rid of untranslated names (I think they're probably not too useful?)
Test Plan: See screenshots.
Reviewers: hach-que, btrahan
Reviewed By: hach-que
CC: aran
Differential Revision: https://secure.phabricator.com/D7495
Summary:
Currently if 'git-http-backend' is not on the PATH, there is no visible message to the user other than "info/refs: is this a valid git repository?" when trying to clone. This adds a setup check so that if there are any Git repositories in use, it will check for the existance of the "git-http-backend" binary in the PATH.
I believe this is shipped by default alongside the git package on most distros, but in some (such as OpenSUSE), this binary isn't on the PATH by default.
Test Plan: Removed `/usr/lib/git` from my `environment.append-paths` and saw the message appear. Added it back and the message went away.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4050
Differential Revision: https://secure.phabricator.com/D7488
Summary: Fixes the junk I broke in D7484. Before that, tag content was a side effect of resolving the ref name. Now, fetch it explicitly in `diffusion.tagsquery`.
Test Plan: Looked at a tag, saw the annotation/message.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7485
Summary: Adds summary (description) and test plan icons to make these area's more unique and differentiated over general sections.
Test Plan: Test a diff, a commit, a task
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D7493
Summary: This disables CSRF checking around the `$repository->writeStatusMessage` so that pushing changes over HTTP to Git repositories doesn't fail miserably.
Test Plan: Applied this fix and I could `git push` to hosted repositories again.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4052
Differential Revision: https://secure.phabricator.com/D7490
Summary: This fixes an issue where Git authentication would always fail on an install with `policy.allow-public` set to false. This is because when public access is allowed, anonymous users can query the user list. However, when public access is not allowed, you have to be authenticated before you can read any of the user objects.
Test Plan:
Prior to this fix, I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
fatal: unable to access 'http://phabricator.local/diffusion/TEST/': The requested URL returned error: 403
```
when `policy.allow-public` is false. After this fix I get:
```
james@james-laptop:~/git/8> git clone http://phabricator.local/diffusion/TEST/
Cloning into 'TEST'...
remote: Counting objects: 102, done.
remote: Compressing objects: 100% (71/71), done.
remote: Total 102 (delta 6), reused 0 (delta 0)
Receiving objects: 100% (102/102), 9.89 KiB | 0 bytes/s, done.
Resolving deltas: 100% (6/6), done.
Checking connectivity... done
```
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T4049
Differential Revision: https://secure.phabricator.com/D7489
Summary:
Ref T2230. This will need some more refinement, but basically it adds a "Create" vs "Import" step before we go through the paged workflow.
- If you choose "Create", we skip the remote URI / auth stuff, and then set the "hosted" flag.
- If you choose "Import", we do what we do now.
Test Plan: Created and imported repos.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7475
Summary:
- Warn about "Read/Write" instead of disabling it, to prevent edits which mutate it after changing a hosted repository to an unhosted one.
- Warn about authenticated connections with HTTPS auth disabled, and link to the relevant setting.
- When "Autoclose" is disabled, show that "Autoclose Branches" won't have an effect.
- For hosted repositories, show the HTTP and SSH clone URIs.
- Make them easy to copy/paste.
- Link to credential management.
- Show if they're read-only.
- This could be a bit nicer-looking than it is.
Test Plan: Looked at repositories in a bunch of states and made various edits to them.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7471
Summary: Depends on D7642. This updates the authentication logic so that HTTP writes can be made to Git repositories hosted by Phabricator.
Test Plan: Set the policy to allow me to push and I was able to. Changed the policy to disallow push and I was no longer able to push.
Reviewers: #blessed_reviewers, hach-que
Reviewed By: hach-que
CC: Korvin, epriestley, aran
Maniphest Tasks: T2230
Differential Revision: https://secure.phabricator.com/D7468
Summary:
Ref T2350. Fixes T2231.
- Adds log flags around discovery.
- Adds message flags for "needs update". This is basically an out-of-band hint to the daemons that a repository should be pulled sooner than normal. We set the flag when users push a revision, and expose a Conduit method that `arc land` will be able to use.
Test Plan: See screenshots.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2350, T2231
Differential Revision: https://secure.phabricator.com/D7467
