Summary:
Ref T13110. Currently, build status is published the same way for every Buildable by the BuildEngine.
I want to change this to delegate publishing to each Buildable, particularly so that Differential may use more detailed rules for handling builds and drafts.
Rather than add additional methods to the existing `BuildableInterface`, add an engine generator method instead. This is a pattern which has seen more use recently (e.g., in Ferret) and lets us pay a little more upfront to pull complex pieces of logic out of the main class and let them use inheritence more easily. If we had Traits that might cover this to some degree.
I'd expect to eventually reduce the size of `BuildableInterface` and move the `CircleCI` and `BuildKite` interfaces so that the `BuildableEngine` implements them instead of the main object.
Here, this new engine does nothing and is never instantiated. In upcoming changes, publishing logic will move into it so that Differential can handle publishing differently.
Test Plan: Ran `arc liberate`, loaded pages, grepped for `BuildableInterface`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13110
Differential Revision: https://secure.phabricator.com/D19278
Summary: See PHI514. Ref T13114. Ref T8951. When a push is an "initial import" (a push of at least 7 commits to an empty repository) don't run Herald or enormous change protection.
Test Plan: Pushed some non-initial changes to a repository, and some initial changes.
Maniphest Tasks: T13114, T8951
Differential Revision: https://secure.phabricator.com/D19265
Summary:
Depends on D19249. Ref T13109. Add timing information to the `PushEvent`:
- `writeWait`: Time spent waiting for a write lock.
- `readWait`: Time spent waiting for a read lock.
- `hostWait`: Roughly, total time spent on the leaf node.
The primary goal here is to see if `readWait` is meaningful in the wild. If it is, that motivates smarter routing, and the value of smarter routing can be demonstrated by looking for a reduction in read wait times.
Test Plan: Pushed some stuff, saw reasonable timing values in the table. Saw timing information in "Export Data".
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19250
Summary:
Depends on D19247. Ref T13109. When we receive an SSH request, generate a random unique ID for the request. Then thread it down through the process tree.
The immediate goal is to let the `ssh-exec` process coordinate with `commit-hook` process and log information about read and write lock wait times. Today, there's no way for `ssh-exec` to interact with the `PushEvent`, but this is the most helpful place to store this data for users.
Test Plan: Made pushes, saw the `PushEvent` table populate with a random request ID. Exported data and saw the ID preserved in the export.
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19249
Summary:
Ref T13109. Make it slightly more clear what the scope of the write and read locks are, and slightly more clear that we're actively acquiring locks, not just sitting around waiting.
While waiting on another writer, show who we're waiting on so you can walk over to their desk and glare at them.
Test Plan:
Added `sleep(15)` after `willWrite()`. Pushed in two windows. Saw new, more informative messages. In the second window, saw the new guidance:
> # Waiting for hector to finish writing (on device "repo1.local.phacility.net" for 11s)...
Reviewers: asherkin
Reviewed By: asherkin
Subscribers: asherkin
Maniphest Tasks: T13109
Differential Revision: https://secure.phabricator.com/D19247
Summary:
Ref T13108. See PHI364. See the task and issue for discussion.
If a `git fetch` during synchronization hangs, the whole node currently hangs. While the causes of a `git fetch` hang aren't clear, we don't expect synchronization to ever reasonably take more than 15 minutes, so add a default timeout.
Test Plan: Will deploy and observe; this is difficult to reproduce or test directly.
Maniphest Tasks: T13108
Differential Revision: https://secure.phabricator.com/D19235
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 T13093. Depends on D19145. See PHI398. Previously, see D18933. This provides the current viewer to `ConduitCall` so that we don't try to use device credentials from unprivileged web hosts.
Test Plan: Evaluated the "Branches" field locally, saw an appropriate field value.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19146
Summary:
Ref T13093. See PHI396. These are possibly somewhat niche, but reasonable to support and consistent with the existing "Pusher's projects".
Also relabel "Pusher's projects" and "Project tags" for consistency and, hopefully, clarity.
Test Plan:
- Created new "commit" and "hook: commit content" Herald rules which run against "Author's projects" and "Committer's projects".
- Test console'd the "Commit" rules.
- Pushed through the "Hook" rule.
- In all cases, saw fields populate appropriately.
Maniphest Tasks: T13093
Differential Revision: https://secure.phabricator.com/D19145
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:
Ref T13083. Facts has a fair amount of weird hardcoding and duplication of responsibilities. Reduce this somewhat: no more hard-coded fact aggregates, no more database-driven list of available facts, etc. Generally, derive all objective truth from FactEngines. This is more similar to how most other modern applications work.
For clarity, hopefully: rename "FactSpec" to "Fact". Rename "RawFact" to "Datapoint".
Split the fairly optimistic "RawFact" table into an "IntDatapoint" table with less stuff in it, then dimension tables for the object PHIDs and key names. This is primarily aimed at reducing the row size of each datapoint. At the time I originally wrote this code we hadn't experimented much with storing similar data in multiple tables, but this is now more common and has worked well elsewhere (CustomFields, Edges, Ferret) so I don't anticipate this causing issues. If we need more complex or multidimension/multivalue tables later we can accommodate them. The queries a single table supports (like "all facts of all kinds in some time window") don't make any sense as far as I can tell and could likely be UNION ALL'd anyway.
Remove all the aggregation stuff for now, it's not really clear to me what this should look like.
Test Plan: Ran `bin/fact analyze` and viewed web UI. Nothing exploded too violently.
Subscribers: yelirekim
Maniphest Tasks: T13083
Differential Revision: https://secure.phabricator.com/D19119
Summary:
See PHI370. Support the "Affected packages" and "Affected package owners" Herald fields in pre-commit hooks.
I believe there's no technical reason these fields aren't supported and this was just overlooked.
Test Plan: Wrote a rule which makes use of the new fields, pushed commits through it. Checked transcripts and saw sensible-looking values.
Differential Revision: https://secure.phabricator.com/D19104
Summary: Depends on D19087. Ref T13079. This still doesn't feel like the most clean, general system in the world, but is a step forward from hard-coded `switch()` stuff.
Test Plan:
- Jumped to `r`.
- Jumped to `a`.
- Jumped to `r poe` (multiple results).
- Jumped to `r poetry` (one result).
- Jumped to `r syzygy` (no results).
- Jumped to `p`.
- Jumped to `p robot` (multiple results); `p assessment` (one result).
- The behavior for `p <string>` has changed slightly but should be more powerful now (it's consistent with `r <string>`).
- Jumped to `s <symbol>` and `s <context>-><symbol>`.
- Jumped to `d`.
- Jumped to `f`.
- Jumped to `t`.
- Jumped to `T123`, `D123`, `@dog`, `PHID-DREV-abcd`, etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19088
Summary: Ref T13079. This recently-introduced Engine/EngineExtension are a good fit for adding more datasource functions in general, but we didn't think quite big enough in naming them.
Test Plan: Used quick search typeahead, hit applications/users/monograms/symbols/etc.
Maniphest Tasks: T13079
Differential Revision: https://secure.phabricator.com/D19087
Summary: Depends on D19063. Ref T13054. Prepare for the addition of a new `PREPARING` status by getting rid of the "scattered mess of switch statements" pattern of status management.
Test Plan: Searched/browsed buildables. Viewed buildables. Viewed revisions. Grepped for all affected symbols.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13054
Differential Revision: https://secure.phabricator.com/D19064
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:
Ref T13053. Fixes T7804. Adds "Acting user" so you can have "always email me" stuff skip things you did or keep an eye on suspicious interns.
For the test console, the current user is the acting user.
For pushes, the pusher is the acting user.
Test Plan: Wrote acting user rules, triggered them via test console and via multiple actors on real objects.
Maniphest Tasks: T13053, T7804
Differential Revision: https://secure.phabricator.com/D19031
Summary:
This command also needs a "." instead of an empty string now.
(This powers the file browser typeahead in Diffusion.)
Test Plan: Will test in production since there's still no easy 2.16 installer for macOS.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19010
Summary:
Ref T13057. This makes "reverts" syntax more visible and useful. In particular, you can now `Reverts Dxx` in a revision or commit, and `Reverts <hash>` from a revision.
When you do, the corresponding object will get a more-visible cross-reference marker in its timeline:
{F5405517}
From here, we can look at surfacing revert information more heavily, since we can now query it on revision/commit pages via edges.
Test Plan: Used "reverts <hash>" and "reverts <revision>" in Differential and Diffusion, got sensible results in the timeline.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13057
Differential Revision: https://secure.phabricator.com/D18978
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 D18970. Ref T13049. Currently, the policy for viewing remote addresses is:
- In activity logs: administrators.
- In push and pull logs: users who can edit the corresponding repository.
This sort of makes sense, but is also sort of weird. Particularly, I think it's kind of hard to understand and predict, and hard to guess that this is the behavior we implement. The actual implementation is complex, too.
Instead, just use the rule "administrators can see remote addresses" consistently across all applications. This should generally be more strict than the old rule, because administrators could usually have seen everyone's address in the activity logs anyway. It's also simpler and more expected, and I don't really know of any legit use cases for the "repository editor" rule.
Test Plan: Viewed pull/push/activity logs as non-admin. Saw remote addresses as an admin, and none as a non-admin.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18971
Summary: Ref T13049. This is just a general nice-to-have so you don't have to export a 300MB file if you want to check the last month of data or whatever.
Test Plan: Applied filters to all three logs, got appropriate date-range result sets.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18970
Summary:
Ref T13049. All exportable objects should always have these fields, so make them builtins.
This also sets things up for extensions (like custom fields).
Test Plan: Exported user data, got the same export as before.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18951
Summary: See PHI276. Ref T13048. The fix in D18933 got one callsite, but missed the one in the `callConduit()` method, so the issue isn't fully fixed in production. Convert this adapter to use a real viewer (if one is available) more thoroughly.
Test Plan: Ran rules in test console, saw field values. Will test in production again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18950
Summary: Ref T13050. Oh boy. Both of them run `grep`!
Test Plan: Will push again.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13050
Differential Revision: https://secure.phabricator.com/D18945
A recent version of Git has changed some piece of behavior here and we
now get "fatal: ssh variant 'simple' does not support setting port"
when using a port. Explicitly setting GIT_SSH_VARIANT to `ssh` likely
fixes this.
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 D18918. Ref T13046. Ref T5954. Pull logs can currently be browsed in the web UI, but this isn't very powerful, especially if you have thousands of them.
Allow SearchEngine implementations to define exportable fields so that users can "Use Results > Export Data" on any query. In particular, they can use this workflow to download a file with pull logs.
In the future, this can replace the existing "Export to Excel" feature in Maniphest.
For now, we hard-code JSON as the only supported datatype and don't actually make any effort to format the data properly, but this leaves room to add more exporters (CSV, Excel) and data type awareness (integer casting, date formatting, etc) in the future.
For sufficiently large result sets, this will probably time out. At some point, I'll make this use the job queue (like bulk editing) when the export is "large" (affects more than 1K rows?).
Test Plan: Downloaded pull logs in JSON format.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046, T5954
Differential Revision: https://secure.phabricator.com/D18919
Summary:
Depends on D18932. Ref T13048. See PHI276. In the cluster, we don't have device keys on `web` nodes. This is generally good, since they don't need them, and it means that we aren't putting more credentials than we need on those hosts.
However, it means that when we pull diff content to test "Commit" rules via the Herald test console, we use the omnipotent user and try to use device credentials, and this fails since we don't have any.
Instead, pass the real viewer in this case so we just sign the request as them, like we do for normal Diffusion requests.
Test Plan:
Wrote and ran a commit content rule locally, no issues.
This isn't completely convincing since my local setup does have device credentials, but I'll double-check in production once this deploys.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048
Differential Revision: https://secure.phabricator.com/D18933
Summary:
Depends on D18931. Ref T13048. Ref T13041. This field means "the first accepting reviewer, where order is mostly arbitrary". Modern rules should almost certainly use "Accepting Reviewers" instead.
Getting rid of this completely is a pain, but we can at least reduce confusion by marking it as not-the-new-hotness. Add a "Deprecated" group, move it there, and mark it for exile.
Test Plan:
Edited a commit rule, saw it in "Deprecated" group at the bottom of the list:
{F5395001}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13048, T13041
Differential Revision: https://secure.phabricator.com/D18932
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:
See PHI305. Ref T13046.
The SSH workflows currently extend `PhabricatorManagementWorkflow` to benefit from sharing all the standard argument parsing code. Sharing the parsing code is good, but it also means they inherit a `getViewer()` method which returns the ommnipotent viewer.
This is appropriate for everything else which extends `ManagementWorkflow` (like `bin/storage`, `bin/auth`, etc.) but not appropriate for SSH workflows, which have a real user.
This caused a bug with the pull logs where `pullerPHID` was not recorded properly. We used `$this->getViewer()->getPHID()` but the correct code was `$this->getUser()->getPHID()`.
To harden this against future mistakes:
- Don't extend `ManagementWorkflow`. Extend `PhutilArgumentWorkflow` instead. We **only** want the argument parsing code.
- Rename `get/setUser()` to `get/setSSHUser()` to make them explicit.
Then, fix the pull log bug by calling `getSSHUser()` instead of `getViewer()`.
Test Plan:
- Pulled and pushed to a repository over SSH.
- Grepped all the SSH stuff for the altered symbols.
- Saw pulls record a valid `pullerPHID` in the pull log.
- Used `echo {} | ssh ... conduit conduit.ping` to test conduit over SSH.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13046
Differential Revision: https://secure.phabricator.com/D18912
Summary:
Ref T13043. We have ~4 copies of this logic (registration, lost password recovery, set password, set VCS password).
Currently it varies a bit from case to case, but since it's all going to be basically identical once account passwords swap to the new infrastructure, bring it into the Engine so it can live in one place.
This also fixes VCS passwords not being affected by `account.minimum-password-length`.
Test Plan: Hit all errors in "VCS Password" panel. Successfully changed password.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13043
Differential Revision: https://secure.phabricator.com/D18902
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:
See <https://discourse.phabricator-community.org/t/files-created-from-repository-contents-slightly-over-one-chunk-in-size-are-truncated-to-exactly-one-chunk-in-size/988/1>. Three issues here:
- When we finish reading `git cat-file ...` or whatever, we can end up with more than one chunk worth of bytes left in the internal buffer if the read is fast. Use `while` instead of `if` to make sure we write the whole buffer.
- Limiting output with `setStdoutSizeLimit()` isn't really a reliable way to limit the size if we're also reading from the buffer. It's also pretty indirect and confusing. Instead, just let the `FileUploadSource` explicitly implement a byte limit in a straightforward way.
- We weren't setting the time limit correctly on the main path.
Overall, this could cause >4MB files to "write" as 4MB files, with the rest of the file left in the UploadSource buffer. Since these files were technically under the limit, they could return as valid. This was intermittent.
Test Plan:
- Pushed a ~4.2MB file.
- Reloaded Diffusion a bunch, sometimes saw the `while/if` buffer race and produce a 4MB file with a prompt to download it. (Other times, the buffer worked right and the page just says "this file is too big, sorry").
- Applied patches.
- Reloaded Diffusion a bunch, no longer saw bad behavior or truncated files.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18885
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: See D18857. Ref T13036. See PHI275. Explain what's going on here a little better since it isn't entirely obvious and debugging these stream parsers is a gigantic pain.
Test Plan: Read text.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18859
Summary:
Depends on D18856. Ref T13036. See PHI275. When we receive a length frame but the buffer doesn't have any data yet, we currently emit a pointless 0-length data frame on the channel.
For normal chatter this is harmless/valid, but it causes problems when a channel has transitioned into bundle2 mode (probably it indicates "end of stream")?
In any case, it's never helpful, so if we're about to read a data block and don't have any data, just bail out until we see some more data.
Note that we can't end up here //expecting// a 0-length data block: both the `data-length` and `data-bytes` states already handle that properly.
Test Plan: Pushed 4MB changes to a Mercurial repository with Mercurial 4.1.1, was no longer able to hit channel errors.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18857
Summary:
Depends on D18855. Ref T13036. This comment no longer seems to be accurate: anything we send over `stderr` is faithfully shown to the user with recent clients.
From [[ https://www.mercurial-scm.org/repo/hg/file/default/mercurial/help/internals/wireprotocol.txt | this document ]], the missing sauce may have been:
```
A generic error response type is also supported. It consists of a an error
message written to ``stderr`` followed by ``\n-\n``. In addition, ``\n`` is
written to ``stdout``.
```
That is, writing "\n" to stdout in addition to writing the error to stderr. However, this no longer appears to be necessary.
I think the modern client behavior is generally sensible (and consistent with the behavior of Git and Subversion) so this //probably// isn't a bug or me making a mistake.
Test Plan: With a modern client, threw some arbitrary exception during execution. Observed a helpful message on the client with no additional steps.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18856
Summary:
Ref T13036. This code attempts to filter the "capabilities" message to remove "bundle2", but I think this has never worked.
Specifically, the //write// pathway is hooked, and "write" here means "client is writing a message to the server". However, the "capabilities" frame is part of the response, not part of the request. Thus, this code never fires, at least on recent versions of Mercurial.
Since I plan to support bundle2 and don't want to decode response frames, just get rid of this, assuming we'll achieve those goals.
I think this was just overlooked in D14241, which probably focused on the HTTP version. This code does (at least, potentially) do something for HTTP.
I'm leaving the actual "strip stuff" code in place for now since I think it's still used on the HTTP pathway.
Test Plan:
- Added debug logging, saw this code never hit even though `hg push --debug` shows the client believing bundle2 is supported.
- Logged both halves of the wire protocol and saw this come from the server, not the client.
- Ran the failing `hg push` of a 4MB file under hg 4.4.1, got the same error as before.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: cspeckmim
Maniphest Tasks: T13036
Differential Revision: https://secure.phabricator.com/D18855
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:
See PHI262. Fixes T12578. Although this is a bit niche and probably better accomplished through advisory/soft measures ("Add blocking reviewers") in most cases, it isn't difficult to implement and doesn't create any technical or product tension.
If installs write a rule that blocks commits, that will probably also naturally lead them to an "add reviewers" rule anyway.
Also, allow packages to be hit with the typeahead. They're valid reviewers but previously you couldn't write rules against them, for no actual reason.
Test Plan: Used test console to run this against commits, got sensible results for the field value.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12578
Differential Revision: https://secure.phabricator.com/D18839
Summary: Ref T13030. See PHI254. This behavior could be cleaner than I've made it, but it fixes the "this is totally broken" issue, replacing a fatal/exception with an informative (just not terribly useful) page.
Test Plan:
- Added a submodule to a repository.
- In Diffusion, clicked some other file next to the submodule, then edited the URI to the submodule path instead.
- Before patch: fatal.
- After patch: relatively useful message about this being a submodule.
Note that it's normally hard to hit this URI directly. In the browse view, submodules are marked up as directories and linked to a separate submodule resolution flow.
{F5321524}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13030
Differential Revision: https://secure.phabricator.com/D18831
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: See PHI131. Ref T7789. Although this probably isn't 100% complete, there don't seem to be any actual, known, practical blocking issues remaining (everything is either heresay or not reproducible).
Test Plan: Tried to push LFS locally, got blocked with a helpful message. Enabled setting, tried to push LFS locally, got a successful push.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18825
Summary:
See <https://discourse.phabricator-community.org/t/diffusion-observed-mercurial-repository-history-broken/825>.
In D18769, I rewrote this from using the `--branch` flag (which is unsafe and does not function on branches named `--config=x.y` and such).
However, this rewrite accidentally changed the result order, which impacted Mercurial commit hisotry lists and graphs. Swap the order of the constraints so we get newest-to-oldest again, as expected.
Test Plan: Viewed a Mercurial repository's history graph, saw sensible chronology after the patch.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18817
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:
See PHI234. Several issues here:
- The warning about observing a repository in Read/Write mode checks the raw I/O type, not the effective I/O type. That means we can fail to warn if other URIs are set to "Default", and "Default" is "Read/Write" in practice.
- There's just an actual typo which prevents the "Observe" version of this error from triggering properly.
Additionally, add more forceful warnings that "Observe" and "Mirror" mean that you want to //replace// a repository with another one, not that we somehow merge branches selectively. It isn't necessarily obvious that "Observe" doesn't mean "merge/union", since the reasons it can't in the general case are somewhat subtle (conflicts between refs with the same names, detecting ref deletion).
Test Plan:
Read documentation. Hit the error locally by trying to "Observe" while in Read/Write mode:
{F5302655}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18810
Summary:
Use ClassQuery to find datasources for the quick-search.
Mostly, this allows extensions to add quicksearches.
Test Plan:
using `/typeahead/class/`, tested several search terms that make sense.
Removed the tag interface from a datasource, which removed it from results.
Reviewers: epriestley, amckinley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18760
Summary:
Ref T13012. These flags can be exploited by attackers to execute code remotely. See T13012 for discussion and context.
Additionally, harden some Mercurial commands where possible (by using additional quoting or embedding arguments in other constructs) so they resist these flags and behave properly when passed arguments with these values.
Test Plan:
- Added unit tests.
- Verified "--config" and "--debugger" commands are rejected.
- Verified more commands now work properly even with branches and files named `--debugger`, although not all of them do.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13012
Differential Revision: https://secure.phabricator.com/D18769
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:
Ref T12680. See PHI167. See that task for discussion.
Rewrite `DiffusionCommitQuery` to work more like `DifferentialRevisionQuery`, and use a UNION to find "all revisions you need to audit OR respond to".
I tried to get this working a little more cleanly than RevisionQuery does, and can probably simplify that now.
Test Plan: Poked at the UI locally without hitting any apparent issues, but my local data is pretty garbage at this point. I'll take a look at how the query plans work on `secure`.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T12680
Differential Revision: https://secure.phabricator.com/D18722
Summary:
See PHI158. In the RefEngine, we test if any old branch positions have been removed from the repository. This is uncommon (but not impossible) in Mercurial, and corresponds to users deleting branches in Git.
Currently, we end up running `hg log` for each position, in parallel. Because of Python's large startup overhead, this can be resource intensive for repositories with a large number of branches.
We have to do this in the general case because the caller may be asking us to resolve `tip`, `newfeature`, `tip~3`, `9`, etc. However, in the specific case where the refs are 40-digit hashes, we can bulk resolve them if they exist, like this:
```
hg log ... --rev (abcd or def0 or ab12 or ...)
```
In the general case, we could probably do less of this than we currently do (instead of testing all old heads, we could prune the list by removing commits which we know are still pointed to by current heads) but that's a slightly more involved change and the effect here is already dramatic.
Test Plan:
Verified that CPU usage drops from ~110s -> ~0.9s:
Before:
```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.
real 0m14.676s
user 1m24.714s
sys 0m21.645s
```
After:
```
epriestley@orbital ~/dev/phabricator $ time ./bin/repository refs nss
Updating refs in "nss"...
Done.
real 0m0.861s
user 0m0.882s
sys 0m0.213s
```
- Manually resolved `blue`, `tip`, `9`, etc., got expected results.
- Tried to resolve invalid hashes, got expected result (no resolution).
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18717
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:
Ref PHI109. Ref T11786. We currently test elapsed time every 64 iterations (since iterations are normally very fast), but at least one install is seeing the page timeout after 30 seconds.
One reason could be that cache fills may occur, and are likely to be much slower than normal iterations. In an extreme case, we could do 64 cache fills before checking the time. Tweak thing so that we always check the time after doing a cache fill, regardless of how many iterations have elapsed since the last attempt.
Additionally, this API method currently accepts an arbitrary number of paths, but implicitly limits each cache query to 500ms. If more than 60 paths are passed, this may exceed 30s. Only let the cache churn for a maximum of 10s across all paths.
If this is more the latter issue than the former, this might replace the GraphCache timeouts with `git` timeouts, but at least our understanding of what's going on here will improve.
Test Plan: This is difficult to test convincingly locally, since I can't reproduce the original issue. It still works after these changes, but it worked fine before these changes too.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11786
Differential Revision: https://secure.phabricator.com/D18692
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:
Ref T11823. This is the meaty part of the change, and updates `RefEngine` to use separate RefCursor (for names) and RefPosition (for actual commit positions) tables.
I'll hold this whole series until after the release cut so it has some time to bake on `secure` to look for issues. It's also not a huge problem if there are bugs here since these tables are just caches anyway, although they do feed into some other things, and obviously it's never good to have bugs.
Test Plan:
- This logic can be invoked directly with `bin/repository refs <repository> --trace --verbose`.
- Ran that on unchanged repositories, new branches, removed branches, and modified branches. Saw appropriate output and cursor positions.
- Ran on a mercurial repository to test the close/open logic, saw it correct open/closed state of incorrect positions.
- Browed around Diffusion in various repositories.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T11823
Differential Revision: https://secure.phabricator.com/D18614
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: Miss this with earlier pass, updates the VCS password page.
Test Plan: Try to set a vcs password
Reviewers: epriestley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18574
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: Adds a `MenuName` method to applications that `ProfileMenuItem` uses instead of the application name if set. This improves the home/menu/new user experience at little cost. Also renamed the label from Applications to Favorites, since this menu gets altered to provide more than just applications. This also allows instances to set back to Maniphest if they so choose. Overall I think this direction resolves 95% of my concerns, with maybe a small potential downside which I don't really anticipate. We already name Dashboard panels by their object, and that hasn't really caused confusion. I think these links are similar. I click 'Tasks' and get presented a list of my tasks from Maniphest.
Test Plan: Review each of the name changes as a default new install and a modified install.
Reviewers: epriestley, amckinley
Reviewed By: epriestley
Spies: Korvin
Differential Revision: https://secure.phabricator.com/D18524
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: Adds some basic UI for open / closed state when viewing a list of branches in Mercurial. Fixes T12838
Test Plan: Close and open branches, view list.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T12838
Differential Revision: https://secure.phabricator.com/D18447
Summary: Better table layouts here for branches view
Test Plan: Test git, hg repositories. See column go away.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18444
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: Adds an icon for default branch, status for branch status
Test Plan: Review `hg` and `git` repositories, change default branch, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18443
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
Summary: Roughs this in a little, kinda basic. Allows for grouping results by page. A bit better on mobile. Would like more content return from conduit though.
Test Plan:
Test `CMS`, `cms`, and `OMGLOLWTFBBQ`, desktop and mobile
{F5099081}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18429
Summary: This is only on browse pages, but I think could be global (home) also. Moves it from a button, field, to just a field.
Test Plan:
Review search on desktop, mobile.
{F5098886}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18428
Summary: Removing this cleanly in event we want to put it back later. 99% of these cases are likely workable either by command line or the typeahead. Will gauge feedback if users notice.
Test Plan: Reload page, perform file grep search.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18425
Summary: Getting to the straight browse view went away, this adds a link back. I'll look at more long term solution for getting to grep search.
Test Plan: Click on header, get take to browse view.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18421
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
