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
