Summary: Ref T13105. This needs refinement but blame sort of works again, now.
Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19309
Summary:
Ref T13105. This breaks about 9,000 features but moves Diffusion to DocumentEngine for rendering. See T13105 for a more complete list of all the broken stuff.
But you can't bake a software without breaking all the features every time you make a change, right?
Test Plan: Viewed various files in Diffusion, used DocumentEngine features like highlighting and rendering engine selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Subscribers: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19302
Summary: Ref T13105. Given that we now load blame with AJAX, it's not clear that there's any benefit to disabling it. This would also interact oddly with the document engine.
Test Plan: Viewed files in Diffusion, no longer saw blame-related options.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19300
Summary:
See PHI489. Ref T13110. At least for now, this just shows "..." at the end since you can click the revision to see the whole list anyway.
Also remove the older-style external Handle passing in favor of lazy construction via HandlePool.
Test Plan: Viewed revisions, fiddled with the 7 limit, got sensible-seeming "..." behavior.
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19293
Summary: Depends on D19190. Fixes T12590. Ref T13099. Replaces the barely-usable, gigantic, poorly ordered "<select />" control with a tokenizer. Attempts to fix various minor issues.
Test Plan:
- Edited paths: include/exclude paths, from different repositories, different actual paths.
- Used "Add New Path" to add rows, got repository selector prepopulated with last value.
- Used "remove".
- Used validation typeahead, got reasonable behaviors?
The error behavior if you delete the repository for a path is a little sketchy still, but roughly okay.
Maniphest Tasks: T13099, T12590
Differential Revision: https://secure.phabricator.com/D19191
Summary:
Depends on D19189. Ref T12590. The "validate" and "complete" endpoints for this UI could incorrectly return redirect responses. These aren't critical to the behavior of Owners, but they're nice to have, and shouldn't redirect.
Instead, skip the canonicalizing redirect for AJAX requests.
Test Plan: Edited Owners paths in a repository with a short name, got completion/validation again.
Maniphest Tasks: T12590
Differential Revision: https://secure.phabricator.com/D19190
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary:
Ref T13090. The default width changed recently to become much wider, but the behavior on this control isn't great. Instead:
- Pick a default width somewhere between the two.
- Make the width sticky across show/hide (pressing "f" twice remembers your width instead of resetting it).
- Make the width sticky across reloads (dragging the bar, then reloading the page keeps the bar in the same place).
Test Plan:
- Without settings, loaded page: got medium-width bar.
- Dragged bar wide/narrow, toggled on/off with "f", got persistent width.
- Dragged bar wide/narrow, reloaded page, got persistent width.
- Dragged bar wide/narrow, toggled it off, reloaded page, toggled it on, got persistent width.
Maniphest Tasks: T13090
Differential Revision: https://secure.phabricator.com/D19129
Summary:
See D18176. This query has no effect (other than wasting resources) and the result is unused.
`$repository` already has the URI loaded because we load them unconditionally during request initialization.
Test Plan: Viewed repository URIs.
Subscribers: jmeador
Differential Revision: https://secure.phabricator.com/D19036
Summary:
Depends on D18972. Ref T13049.
Currently, the "flags" columns renders an inscrutible bitmask which you have to go hunt down in the code. Show a list of flags in human-readable text instead.
The "code" column renders a meaningless integer code. Show a text description instead.
The pull logs and push logs pages don't have a crumb to go back up out of the current query. Add one.
Test Plan: Viewed push logs, no more arcane numbers. Saw and clicked crumbs on each log page.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18973
Summary:
Depends on D18939. Ref T13047. Symbol lookup can be activated from a diff (in Differential or Diffusion) or from the static view of a file at a particular commit.
In the latter case, we need to figure out the path a little differently. The character and line number approaches still work as written.
Test Plan:
- Command-clicked symbols in the Diffusion browse view with blame on and off; saw path, line and char populate properly.
- Command-clicked symbols in Differential diff view to check I didn't break anything.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18940
Summary: Depends on D18937. Ref T13047. When available, provide character positions so external indexers can return more accurate results.
Test Plan: Clicked symbols in Safari, Firefox and Chrome, got sensible-looking character positions.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18939
Summary:
Depends on D18936. Ref T13047. Third parties can define external symbol sources that let users jump to PHP or Python documentation or query some server.
Give these queries more information so they can try to get better results: the path and line where the symbol appeared, and any known repository scope.
Test Plan: Wrote a fake external source that used this data, command-clicked a symbol in Differential, saw a fake external symbol result.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13047
Differential Revision: https://secure.phabricator.com/D18937
Summary:
Depends on D18915. Ref T13046.
- Distinguish between HTTP and HTTPS.
- Use more constants and fewer magical strings.
- For HTTP responses, give them better type information and more helpful UI behaviors.
Test Plan: Pulled over SSH and HTTP. Reviewed resulting logs from the web UI. Hit errors like missing/invalid credentials.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18917
Summary:
Depends on D18912. Ref T13046. Add a UI to browse the existing pull log table.
The actual log still has some significant flaws, but get the basics working.
Test Plan: {F5391909}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18914
Summary:
Ref T13043. Migrate VCS passwords away from their dedicated table to new the new shared infrastructure.
Future changes will migrate account passwords and remove the old table.
Test Plan:
- Ran migrations.
- Cloned with the same password that was configured before the migrations (worked).
- Cloned with a different, invalid password (failed).
- Changed password.
- Cloned with old password (failed).
- Cloned with new password (worked).
- Deleted password in web UI.
- Cloned with old password (failed).
- Set password to the same password as it currently is set to (worked, no "unique" collision).
- Set password to account password. !!This (incorrectly) works for now until account passwords migrate, since the uniqueness check can't see them yet.!!
- Set password to a new unique password.
- Cloned (worked).
- Revoked the password with `bin/auth revoke`.
- Verified web UI shows "no password set".
- Verified that pull no longer works.
- Verified that I can no longer select the revoked password.
- Verified that accounts do not interact:
- Tried to set account B to account A's password (worked).
- Tried to set account B to a password revoked on account A (worked).
- Spot checked the `password` and `passwordtransaction` tables for saniity.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18898
Summary:
Fixes T13040. To reproduce:
- View a file with blame enabled, where some line has an associated revision (say, `D123`).
- Edit `D123` so it exists and is a valid revision, but the viewer can't see it.
- Reload the page.
Instead, only add revisions to the map if we actually managed to load them.
Test Plan: Page no longer fatals.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13040
Differential Revision: https://secure.phabricator.com/D18884
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".
If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.
Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.
Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.
Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13031
Differential Revision: https://secure.phabricator.com/D18850
Summary: Depends on D18827. Ref T7789. See PHI204. See PHI131. This button got accidentally removed in Diffusion refactoring (`$data` is no longer used).
Test Plan: {F5321459}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18828
Summary: Ref T13001, URLs that return multiple commits should show a list of those commits. Not sure if the actual list looks very pretty this way, but was wondering if this approach was vaguely correct.
Test Plan:
- Navigate to `install/rPbd3c23`
- User should see a list view providing links to `install/rPbd3c2355e8e2b220ae5e3cbfe4a057c8088c6a38` and `install/rPbd3c239d5aada68a31db5742bbb8ec099074a561`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13001
Differential Revision: https://secure.phabricator.com/D18816
Summary: Ref T13019, adds build status back to Diffusion commits
Test Plan: Open a Diffusion commit that has a build status, property list view should show the build status, but not Subscriptions, Projects, or Tokens.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13019
Differential Revision: https://secure.phabricator.com/D18813
Summary: See PHI234. In T12931 we improved the behavior of Diffusion when a repository's default branch is set to a branch that does not exist, but in T11823 the way refcursors work changed, and we can now get a cursor (just with no positions) back for a deleted branch. When we did, we didn't handle things gracefully.
Test Plan:
- Set default branch to a deleted branch, saw nice error instead of fatal.
- Set default branch to a nonexistent branch which never existed, saw nice error.
- Set default branch to existing "master", saw repository normally.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18811
Summary: Give profile images a little more space, fix "/" spacing, add a tooltip.
Test Plan: {F5251205}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18749
Summary: Depends on D18746. See PHI174. Adds small author portraits next to each blame line (this is similar to GitHub).
Test Plan:
My local test data isn't that great since I don't have commits from a lot of accounts, but looks functional:
{F5251056}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18747
Summary:
Ref PHI174. This reverts most of these changes:
- 37843127e9 / D18481
- 94cad30ac3 / D18474
- 12ae08b6b1 / D18473
- 0a01334172 / D18462
- ac91ab1ef9 / D18452
These changes made the Diffusion blame view very similar to GitHub's blame view. See D18452 for a before/after of the bulk of these changes; the other revisions are bugfixes.
I think this was generally a step backward, and not motivated by solving a specific problem. I've found the new UI less usable than the old one, and at least one install (see PHI174) also has.
In particular, the revision/commit titles are very bulky and not terribly useful; the date column also isn't terribly useful; the "age" color actually IS pretty useful and was heavily de-emphasized.
I've kept one bugfix here (missing `'a'` tag type) and kept the upgraded icon for "Skip Past This Commit".
I'm going to follow this up with some additional changes:
- Show a small author profile icon, similar to GitHub, to address PHI174 more directly.
- Try a zebra-stripe on blocks of rows to make it more clear where changes affected by a particular commit begin and end.
- Try a hue shift, not just a brightness/saturation shift, to make the "age" color more distinct.
- Try computing colors as even steps, not based purely on age. Currently, if a file has one long-distant commit and several recent commits, all the recent ones show up as very bright green. I think this would probably be more useful if they were distributed more evenly across the available color bands.
Test Plan:
Viewed blame views in Diffusion, saw a more compact UI similar to the old UI.
{F5251019}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18746
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
See PHI112. The install presumably wants to generate links to Diffusion commits from an external tool, but only knows the short name of the repository.
Provide a `/source/phabricator/commit/abcdef908273` URI which redirects to the canonical URI for the commit.
Test Plan:
- Visited `/source/` URI for a commit, got a redirect.
- Visited normal URI for a commit, got a commit page.
- Visited `/branches/` and `/tags/` for a `/source/` repository, got proper pages.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18676
Summary:
Ref PHI101. It looks like this was maybe copy/pasted by mistake in recent design refactoring.
We need to pass the full path, not the `basename()` of the path, to the search form.
Test Plan: Searched inside `scripts/test/`, found results inside `scripts/test/`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18664
Summary:
See <https://discourse.phabricator-community.org/t/unable-to-use-current-mercurial-on-debian-stretch/391>.
The Mercurial commit is helpful in particular: <https://www.mercurial-scm.org/repo/hg/rev/77eaf9539499>
We weren't vulnerable to the security issue (users can not control any part of the command) but pass the working directory explicitly to get past the new safety check.
I left `setCWD()` in place (a few lines below) just because it can't hurt, and in some other contexts it sometimes matter (for example, if commit hooks execute, they might inherit the parent CWD here or in other VCSes).
Test Plan:
- Cloned from a Mercurial repo locally over HTTP.
- Verified that SSH cloning already uses `-R` (it does, see `DiffusionMercurialServeSSHWorkflow`).
- Did not actually upgrade to Mercurial 4.0/4.1.3 to completely verify this, but a user in the Discourse thread asserted that a substantially similar fix worked correctly.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18611
Summary: This should have a border
Test Plan: Reload page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18549
Summary: Simplifies the Repository Management pages to the new fixed column layout. I've also moved "Status" into the Basics page, which feels better, and moved "Documentation" as a nav item to a button in the header. This removed "action list" and "curtain view" from the management panels and uses the new bits from Config/Phacility. Undecided if the icons should stay or go for the nav. Left them in for Diffusion. I want to update the EditEngine pages to display in this UI and not leave the portal, but I haven't dug into that this page. I'm a bit worried it will not easily be possible.
Test Plan:
Generate a svn, git, hg repository, test each of the new pages and each of the new buttons. Activate, deactivate, etc.
{F5164674}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18523
Summary: Implements a new mobile view thats more fullscreen, not boxed, so more space. Fixes issues with mobile tables when scrolling overflowed content.
Test Plan: Test home, branch, tags, code, file browse, graph, compare, history, readme, open revisions, owners.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18505
Summary: Visually selects the button if blame is on.
Test Plan: Turn blame on and off in Diffusion on a file.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18504
Summary: 50% more line, no additional cost! Order Now! Operators are standing by.
Test Plan: Blame a file
Reviewers: epriestley, avivey
Reviewed By: avivey
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18481
Summary: My fake data was 100%, and not all tables have full revision history. This leads to a broken table. Instead check if we have //any// revisions at all, then always show the column, with or without a link inside.
Test Plan: going on a limb this is the correct fix and test on secure... again ...
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18474
Summary: There is still some layout issues with revisions, so I've tested it better and moved it to it's own column
Test Plan: Fake in some revision data, test various sizes and shapes.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18473
Summary: I missed an anchor tag here, adds it back
Test Plan: View blame, click a previous version of the file, click Back to HEAD link.
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18451
Summary: Ref T12824, adds more information to the blame view, exposes date, commit summary, lighter colors.
Test Plan:
Review many diffs with and without blame on.
{F5111758}
{F5111759}
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18452
Summary: Moves browseFile to single column, implements Owners as a list under the file (and now directory as well), improved information listed in Owners, and moves actions into the Diffusion action bar instead of the header.
Test Plan:
Test browsing directories, files, text, images, binaries, enabling blame. Mobile and desktop.
{F5111045}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18448
Summary: This is in the crumbs, but a little hidden. Puts branch name at the top of the browse table header.
Test Plan: Review a few branchs, change branch, see new name.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18441
Summary: Moves the method up to DiffusionController, so it can be more universally used. Also now center aligns tabs on mobile. Still todo, get search nicely toggled on mobile
Test Plan: Test mobile, desktop. Test search from home, from browse, and browsing a specific path.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18432
Summary: Moving this down the the "bar" to allow pattern search on home. Rebuilds the mobile layout a little.
Test Plan:
Test actions on mobile, desktop, tablet.
{F5100460}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18431
