Page MenuHomePhabricator

Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway
ClosedPublic

Authored by epriestley on Jan 4 2018, 8:26 PM.
Tags
None
Referenced Files
F14054330: D18855.id45235.diff
Sat, Nov 16, 12:38 AM
F14054007: D18855.id45235.diff
Fri, Nov 15, 9:41 PM
F14016434: D18855.diff
Mon, Nov 4, 9:38 AM
F14014018: D18855.diff
Sat, Nov 2, 8:57 PM
F14013804: D18855.diff
Sat, Nov 2, 3:14 PM
F13997981: D18855.id45235.diff
Thu, Oct 24, 7:05 AM
F13994844: D18855.diff
Wed, Oct 23, 8:49 AM
F13967358: D18855.id45230.diff
Oct 16 2024, 1:05 PM
Subscribers

Details

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.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

This revision is now accepted and ready to land.Jan 4 2018, 8:29 PM
cspeckmim added inline comments.
src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php
114

From what I remember this method and an associated unit test could probably also be removed

There's still a call to it in DiffusionServeController.php which I think does have an effect (although I haven't confirmed this).

This revision was automatically updated to reflect the committed changes.