Summary:
Ref T2543. These are the last `ArcanistDifferentialRevisionStatus` callsites.
This removes the very old legacy `precommitRevisionStatus` field, which has no other readers. This was obsoleted by the `CLOSED_FROM_ACCEPTED` stuff, but retained for compatibility.
Test Plan:
- Poked these with the test console, although they're a little tricky to be sure about.
- Grepped for `ArcanistDifferentialRevisionStatus`, no more hits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18416
Summary: Ref T2543. Several queries want only open revisions. Provide a tailored, non-legacy way to issue that query.
Test Plan: Viewed some of these callsites (e.g., "Similar open revisions affecting these files"), saw only open revisions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18395
Summary:
Ref T12961. In Mercurial, it's possible to have "subrepos" which may use a different protocol than the main repository.
By putting an SSH repository inside an HTTP repository, an attacker can theoretically get us to execute `hg` without overriding `ui.ssh`, then execute code via the SSH hostname attack.
As an immediate mitigation to this attack, specify `ui.ssh` unconditionally. Normally, this will have no effect (it will just be ignored). In the specific case of an SSH repo inside an HTTP repo, it will defuse the `ssh` protocol.
For good measure and consistency, do the same for Subversion and Git. However, we don't normally maintain working copies for either Subversion or Git so it's unlikely that similar attacks exist there.
Test Plan:
- Put an SSH subrepo with an attack URI inside an HTTP outer repo in Mercurial.
- Ran `hg up` with and without `ui.ssh` specified.
- Got dangerous badness without `ui.ssh` and safe `ssh` subprocesses with `ui.ssh`.
I'm not yet able to confirm that `hg pull -u -- <uri>` can actually trigger this, but this can't hurt and our SSH wrapper is safer than the native behavior for all Subversion, Git and Mercurial versions released prior to today.
Reviewers: chad
Reviewed By: chad
Subscribers: cspeckmim
Maniphest Tasks: T12961
Differential Revision: https://secure.phabricator.com/D18389
Summary: Fixes T12832. Adds a basic table (not paginated?) to view tracking and autoclose status.
Test Plan:
Review a large repository (Krita) with setting various states of tracking and autoclose.
{F5092117}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12832
Differential Revision: https://secure.phabricator.com/D18386
Summary:
Ref T2543. Currently, Differential uses a set of hard-coded query filters (like "open" and "closed") to query revisions by status (for example, "open" means any of "review, revision, changes planned, accepted [usually]").
In other applications, like Maniphest, we've replaced this with a low level list of the actual statuses, plus higher level convenience UI through tokenizer functions. This basically has all of the benefits of the hard-coded filters with none of the drawbacks, and is generally more flexible.
I'd like to do that in Differential, too, although we'll need to keep the legacy maps around for a while because they're used by `differential.find` and `differential.getrevision`. To prepare for this, pull all the legacy stuff out into a separate class. Then I'll modernize where I can, and we can get rid of this junk some day.
Test Plan: Grepped for `RevisionQuery::STATUS`. Ran queries via Differential UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18343
Summary:
Ref T2543. These are currently numeric values, like "0" and "3". I want to replace them with strings, like "accepted", and move definitions from Arcanist to Phabricator.
To set the stage for this, reduce the number of callsites where Phabricator invokes `ArcanistDifferentialRevisionStatus`.
This is just the easy ones. I'll hold this until the release cut.
Test Plan:
- Called `differential.find`.
- Called `differential.getrevision`.
- Called `differential.query`.
- Removed all reviewers from a revision, saw warning.
- Abandoned the no-reviewers revision, no more warning.
- Attached a revision to a task to get it to show the state icon with the status on a tooltip.
- Viewed revision bucketing on dashboard.
- Used `bin/search index` to reindex a revision.
- Hit the "Land Revision" endpoint.
I didn't explicitly test these cases:
- Doorkeeper Asana integration, since setup takes a thousand years.
- Disambiguation logic when multiple hashes match, since setup is also very involved.
- Releeph because it's Releeph.
Reviewers: chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18339
Summary: Just a few more.
Test Plan: Edit Picture, see new image, choose image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18370
Summary:
Fixes T12942.
- Adds binary version and path information to {nav Config > Version Information}.
- Replaces old code all over the place with new consolidated code.
Test Plan:
{F5073531}
Also faked some cases of missing binaries, bad versions, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12942
Differential Revision: https://secure.phabricator.com/D18306
Summary:
In Diffusion, the "Tags" view may read commits which haven't imported or parsed yet, and thus don't have loadable objects.
Most of this logic tests for `if ($commit)`, but the author part did not. Instead, don't render author information if `$commit` is not present.
Test Plan:
- Loaded tags view with commits present.
- Faked `$commit = null;`, loaded tag view, got this instead of a fatal:
{F5068432}
Reviewers: chad, amckinley
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18290
Summary:
Fixes T12893. See also PHI15. This is complicated but:
- In the documentation, we say "register your web devices with Almanac". We do this ourselves on `secure` and in the production Phacility cluster.
- We don't actually require you to do this, don't detect that you didn't, and there's no actual reason you need to.
- If you don't register your "web" devices, the only bad thing that really happens is that creating repositories skips version initialization, creating the bug in T12893. This process does not actually require the devices be registered, but the code currently just kind of fails silently if they aren't.
Instead, just move forward on these init/resync phases even if the device isn't registered. These steps are safe to run from unregistered hosts since they just wipe the whole table and don't affect specific devices.
If this sticks, I'll probably update the docs to not tell you to register `web` devices, or at least add "Optionally, ...". I don't think there's any future reason we'd need them to be registered.
Test Plan:
This is a bit tough to test without multiple hosts, but I added this piece of code to `AlmanacKeys` so we'd pretend to be a nameless "web" device when creating a repository:
```
if ($_REQUEST['__path__'] == '/diffusion/edit/form/default/') {
return null;
}
```
Then I created some Git repositories. Before the patch, they came up with `-` versions (no version information). After the patch, they came up with `0` versions (correctly initialized).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12893
Differential Revision: https://secure.phabricator.com/D18273
Summary: Fixes T12931. Adds a branch selector that's always visible if the repo has commits.
Test Plan:
Test a plain hg, svn, git repository. Test setting a bad default branch. Test a good default branch. Test on desktop, mobile layouts.
{F5058061}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12931
Differential Revision: https://secure.phabricator.com/D18267
Summary: This spelling can definitely feel a little overplayed at times, but I still think it's a gold standard in spellings of "capabilities".
Test Plan: Felt old and uncool.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18215
Summary: Just some cleanup. Make sure action-bar has consistent space if locate is there or not, hide tabs if repository has no content. Use clone or checkout language depending on SCM. Fixes T12915.
Test Plan:
Test git, hg, svn blank states.
{F5042707}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12915
Differential Revision: https://secure.phabricator.com/D18208
Summary: This moves the clone details on the Repository Home to a button / dialog. Functionally this is to pull content on the page way up, while giving full space to all the clone options. I think we can build this into some FancyJS if needed, but this seems to clean ui the UI dramatically with little overhead. I don't want to attempt the JS dropdown unless we're sure that's the best path (it exposes the most common URI by default, saving a click).
Test Plan: Tested hg, svn, git repositories and the raw URL page. Test close button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18203
Summary: This moves actions into the Diffusion main header, removes the locate file box, and widens description and cloning details. Projects are not currently in this layout, but will follow up in another diff. Trying to keep these changes small and iterative.
Test Plan:
Locate some files, test actions dropdown, repository with and without description. Also tablet, mobile layouts.
{F5040026}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18193
Summary: Cleans up colors, removes commit hash and links the text instead. Also unsure how valuable "lint" column is here, but left it. I'd maybe like to understand that workflow since it just seems like clutter overall. Also Fixes T12905
Test Plan:
Review Phabricator, hg, and a few other test repositories locally. Holler if anything here seems bad, but this feels easier to read and use to me.
{F5038425}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12905
Differential Revision: https://secure.phabricator.com/D18189
Summary:
Adds a responsive tab bar navigation to Diffusion. Working through the new design here in pieces, so keep in mind M1477 is the target. Notably:
- Removes "branches" and "tags" from RevisionView, now on tabs
- Keeps "browse", "history", "readme" on RevisionView
- Adds tabs for all main views, including Graph... unless how that feels, so let me know.
Test Plan: Browse all pages, desktop and mobile. Test hg, svn, git repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18161
Summary: Fixes T12894. See that task for discussion.
Test Plan:
- Created repositories `abcdef`, then `abcdef-a` through `abcdef-f`.
- Before patch, awkward sort order.
- After patch, query for `abcdef` hits `abcdef` first.
- See T12894 for details and screenshots.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12894
Differential Revision: https://secure.phabricator.com/D18179
Summary: The main change here is moving (compare, search, history) into buttons in the header bar on all browse views. This allows Directory Browsing to be full width, since there is no other curtain information. File, Image, LFS, Binary all stay in TwoColumn layouts with the same buttons in the header.
Test Plan: Test viewing a directory, file, image, binary file, readme, and fake a gitlfs.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17766
Summary:
Fixes T12884. In cases other than this UI, applications access URIs through the Repository they're part of. This means that applications interact with URIs which have gone through the correction/adjustment logic in `PhabricatorRepository->attachURIs()`, which fixes up "builtin" URIs to have the right values based on configuration.
In this case (and, as far as I can tell, only this case) we load the URI directly //and// act on its properties which depend on configuration and repository state.
This can mean we're using a different view of the URI than we should be.
To fix this: after loading the URI, reload it through the repository so the relevant adjustments are applied.
I think this is the most reasonable fix. We could try to make `RepositoryURIQuery` somehow enforce this, but the cost of this error is small (mild confusion about display state), the other things which do direct loads don't depend on this state (editing), and everything else loads via a repository and is likely to continue doing that forever.
Test Plan: {F5026633}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12884
Differential Revision: https://secure.phabricator.com/D18176
Summary: Fixes T12840. This adds a parallel "graph" button next to history on home and on the history list page. I'll think more about better placement of how to get to this page with the upcoming redesign that's still sitting in Pholio.
Test Plan: View History, View Graph, Try pager, go to a file, click view history, see no graph button.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12840
Differential Revision: https://secure.phabricator.com/D18131
Summary: Moves DiffusionTagsListView to uhhh, list. Separates out table view which is still in use now, implements mobile friendly UI for tags.
Test Plan:
Review KDE's Krita repository locally with lots of tags, desktop and mobile.
{F4997708}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18115
Summary: Adds a new DiffusionBranchListView which replaces the BranchTable when browsing all branches in Diffusion. Has all the same capabilities, but is easier to read, adds a Compare button, and plays nicely on mobile. It does take up more space, but I think that's generally OK here since we expect our branches to not be heaping piles of intern revert branches.
Test Plan:
Follow a few repositories with branches, like Phabricator and KDE's Krita. View layouts on mobile, tablet, desktop. Try out new compare button.
{F4996207}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: avivey, Korvin
Maniphest Tasks: T12824
Differential Revision: https://secure.phabricator.com/D18113
Summary: Builds out some images to use to identify repositories. Fixes T12825.
Test Plan:
Try setting custom, built in, and null images.
{F4998175}
{F4998192}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12825
Differential Revision: https://secure.phabricator.com/D18116
Summary:
Fixes T12807. Some shells may apparently mangle/strip UTF8 characters? Just dodge this whole problem by sending the pattern over stdin rather than actually figuring out the particulars.
Related tasks, like T7339 and T5554, discuss finding broader fixes for this class of issue, and this definitely isn't exactly a fully legitimate fix, but in many cases (as here) we can reasonably just avoid the problem rather than actually fixing it, at least for a long time.
Test Plan: Searched for emoji and non-emoji locally, but this worked fine (on OSX) for me before the patch too.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12807
Differential Revision: https://secure.phabricator.com/D18105
Summary: Porting over a fix that we could miss the tail end of commits. Also use the new tag borderless option.
Test Plan: Review various commit pages in profile.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18086
Summary: Little nits and spacing changes to viewing diffusion commit history on phones.
Test Plan:
Review in Chrome, iOS Simulator.
{F4990749}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18085
Summary: Formally support borderless tags in PHUITagView.
Test Plan: Used in Diffusion History List
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18074
Summary:
Commits in the list are grouped by the date they occurred in server time. This may not be the date they occurred in client time.
Use client time, not server time, to group commits.
Test Plan:
- Set server timezone to "Asia/Famagusta".
- Set client timezone to "America/Los_Angeles".
- Viewed Phabricator repository history.
Here's what it looks like before the change:
{F4987094}
Note that the headers of the first two groups both say "Yesterday".
This is because the first commits in each group occurred on June 1 and June 2, respectively, in Famagusta, but both occurred on June 1 in Los Angeles.
Here's what it looks like after the change:
{F4987095}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18067
Summary:
Currently, the last group of commits is not shown in the list view because the final `$list` is never added to `$view`.
For example, if the first page would contain commits from "April 7", "April 6", and "April 5", commits from "April 5" are not shown.
(If a repository has 100 commits in a single day, nothing is shown.)
On this server, here's the bottom of page 1:
{F4987087}
Here's the top of page 2:
{F4987088}
However, here's `git log` between those commits:
```
$ git log --oneline 7e46^..5f49f
5f49f9c793 Add sound to logged out Conpherence
1644b45050 Disperse task subpriorities in blocks
c6a7bcfe89 Make Pholio description behave as a remarkup field (e.g., subscribe mentioned users)
bbc5f79227 Make membership lock/unlock feed stories read more naturally
789d57522b Make editing project images redirect to "Manage" more consistently
10b3879232 Make Project slug/hashtag transactions render a little more nicely
abd791889c Update Maniphest title transaction again
5a34b299e4 Update Maniphest title language
601622013d Clarify milestone/subproject creation language
c9889e3d55 Fix an issue in Phriction where moving a document just copied it instead
fdf00f6df4 Clean up some minor UI behaviors in Differential
6c46f27d98 Add quest objectives to the minimap
d783299a19 Fix Phriction status not set property on new document
93e28da76e Add more "disabled" UI to PHUIObjectItemView
7e46d7ab6a Migrate Project color to modular transactions
```
This group of commits does not currently appear anywhere in the list.
Test Plan: Viewed a page of commits, saw 100 commits.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18066
Summary: Ref rPf2fcafb40dde94ddf4ee22716fea74fca0334a64#38208, I think this is a more usable layout. Gets rid of clippy, audit. Adds back Differential link as tag, Build Status as button.
Test Plan: Faked data on this for Differential, Builds, should all work though. Test on real and fake repositories.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18061
Summary: Ref T12780. Makes the button do something useful, like link to the history at the right spot in the graph.
Test Plan: Click on various browse buttons, get correct url.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12780
Differential Revision: https://secure.phabricator.com/D18054
Summary: This moves Diffusion History to use an easier to parse list view for commits and their (diff, audit, build) status. I left TableView around, which is used on a repositories home, and we can maybe add a "graph view" history back as another controller. Not sure what the real use is for that kind of feature though. I don't have Harbormaster set up locally so I could use another install to give this a run. I also expect to maybe not live with this UI as final, I like the UX, but the icons for indicating status don't really feel great to me, just OK.
Test Plan:
pull various repositories, check various history displays.
{F4980356}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18039
Summary: I think this name is more accurate, also add proper links to author image.
Test Plan: Review commits in sandbox, see new URL on image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18026
Summary: Gives the ability to hide a big long block of text in an ObjectListItem without cluttering the UI.
Test Plan:
Added a test case to UIExamples. Click on icon, see content. Click again, content go away.
{F4974153}
{F4974311}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18006
Summary: Going to play a bit with this layout (diffusion sans audit) and see how it feels on profile. Uses a user image, moves the commit hash (easily selectible) and separates commits by date.
Test Plan:
Review profiles with and without commits.
{F4973987}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18005
Summary: This moves the navigation to a standard sidebar, and moves all actions to the curtain. Also pulled out info view when available for cleaner UI.
Test Plan:
Create a git, svn, hg test repository and verify each page in the sidebar renders as expected.
{F4973792}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18003
Summary: Grep for phui-tag-shade and verify we're no longer calling shade-color directly.
Test Plan: Search, workboard, story points, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17993
Summary: Ref T12616. This moves most keyboard shortcuts into DiffChangesetList. It breaks some shortcuts that I plan to restore later, noted in T12616 (toggle file, edit inline, reply to inline), since I think ripping them out now and rebuilding them in a little bit will make things much simpler.
Test Plan:
- Used j, k, n, p, J, K shortcuts to navigate a revision.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17859
Summary:
Ref T12613. Currently, the SVNTEST and HGTEST repositories are improperly configured on `secure`. These repositories use VCS systems which do not support synchronization, so they can not be served from cluster services with multiple hosts.
However, I've incorrectly configured them the same way as all the Git repositories, which support synchronization. This causes about 50% of requests to randomly fail (when they reach the wrong host).
Detect this issue and warn the user that the configuration is not valid.
It should be exceptionally difficult for normal installs to run into this.
Test Plan:
- Mostly faked these conditions locally, verified that `secure` really has this configuration.
- I'll push this, verify that the issue is detected correctly in production, then fix the config which should resolve the intermittent issues with SVNTEST.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12613
Differential Revision: https://secure.phabricator.com/D17774
Summary: Uses more standard objects and more padding for reading. Removes the ToC, which is visually broken anyways.
Test Plan: Review a README.md in a local repository.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17752
Summary: Adds the ability to set a pager onto an object box directly and pick up appropriate styles.
Test Plan: grep for renderTablePagerBox, test layouts with and without a pager.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12604
Differential Revision: https://secure.phabricator.com/D17754
Summary: Ref T12509. This encourages code to move away from HMAC+SHA1 by making the method name more obviously undesirable.
Test Plan: `grep`, browsed around.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12509
Differential Revision: https://secure.phabricator.com/D17632
Summary:
Ref T12298. The PullLocal daemon has had hibernation code for a little while, but it never actually activated because we don't sleep for more than 15 seconds in any case.
Add a maximum sleep instead and use that to control the longest sleep we'll do for hibernation purposes.
Also, when a repository or repository URI is edited, write a NEEDS_UPDATE event into the message table to make sure the daemons de-hibernate.
Test Plan: Used `bin/phd debug pull`, saw the daemon actually hibernate instead of just sleeping for 15 seconds.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12298
Differential Revision: https://secure.phabricator.com/D17635
Summary:
Ref T11357. When creating a file, callers can currently specify a `ttl`. However, it isn't unambiguous what you're supposed to pass, and some callers get it wrong.
For example, to mean "this file expires in 60 minutes", you might pass either of these:
- `time() + phutil_units('60 minutes in seconds')`
- `phutil_units('60 minutes in seconds')`
The former means "60 minutes from now". The latter means "1 AM, January 1, 1970". In practice, because the GC normally runs only once every four hours (at least, until recently), and all the bad TTLs are cases where files are normally accessed immediately, these 1970 TTLs didn't cause any real problems.
Split `ttl` into `ttl.relative` and `ttl.absolute`, and make sure the values are sane. Then correct all callers, and simplify out the `time()` calls where possible to make switching to `PhabricatorTime` easier.
Test Plan:
- Generated an SSH keypair.
- Viewed a changeset.
- Viewed a raw diff.
- Viewed a commit's file data.
- Viewed a temporary file's details, saw expiration date and relative time.
- Ran unit tests.
- (Didn't really test Phragment.)
Reviewers: chad
Reviewed By: chad
Subscribers: hach-que
Maniphest Tasks: T11357
Differential Revision: https://secure.phabricator.com/D17616
