Page MenuHomePhabricator
Differential D14241

Added an intercept to Mercurial's `capabilities` command to remove bundle2.
ClosedPublic
Actions

Authored by cspeckmim on Oct 6 2015, 5:40 AM.
Tags
None
Referenced Files
F14036077: D14241.diff
Sun, Nov 10, 8:28 AM
F14013692: D14241.diff
Sat, Nov 2, 10:01 AM
F14012667: D14241.diff
Fri, Nov 1, 2:55 PM
F13994897: D14241.diff
Wed, Oct 23, 9:14 AM
F13994029: D14241.id34404.diff
Wed, Oct 23, 3:15 AM
F13989972: D14241.diff
Tue, Oct 22, 12:13 AM
F13989798: D14241.id.diff
Mon, Oct 21, 11:11 PM
F13989732: D14241.diff
Mon, Oct 21, 10:39 PM
View All Files
Subscribers
Korvin

Details

Reviewers
epriestley
Group Reviewers
Blessed Reviewers
Maniphest Tasks
T9450: "abort: not a Mercurial bundle" with Mercurial 3.5>=
Commits
Restricted Diffusion Commit
rP32d4ae8cb2cc: Added an intercept to Mercurial's `capabilities` command to remove bundle2.
Summary

If Mercurial 3.4+ is used to host repositories in Phabricator, any clients using 3.5+ will receive an exception after the bundle is pushed up. Clients will also fail to update phases for changesets pushed up.

Before directly responding to mercurial clients with all capabilities, this change filters out the 'bundle2' capability so the client negotiates using a legacy bundle wire format instead.

Test Plan

Server: Mercurial 3.5
Client: Mercurial 3.4

Test with both HTTP and SSH protocols:

  1. Create a local commit on client
  2. Push commit to server
  3. Verify the client emits something like:
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files

Closes T9450

Diff Detail

Repository
rP Phabricator
Branch
master
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php:37XHP48Array Separator
Unit
Tests Passed
Build Status
Buildable 8220
Build 9402: arc lint + arc unit

Event Timeline

cspeckmim updated this revision to Diff 34385.Oct 6 2015, 5:40 AM
cspeckmim retitled this revision from to Added an intercept to Mercurial's `capabilities` command to remove bundle2..
cspeckmim updated this object.
cspeckmim edited the test plan for this revision. (Show Details)
cspeckmim added a reviewer: epriestley.
cspeckmim added a task: T9450: "abort: not a Mercurial bundle" with Mercurial 3.5>=.
Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptOct 6 2015, 5:40 AM
Herald added a subscriber: Korvin. · View Herald Transcript
cspeckmim updated this revision to Diff 34386.Oct 6 2015, 5:49 AM
cspeckmim edited edge metadata.
  • Fixed spelling mistake and other lint issues except for the really long lines
cspeckmim planned changes to this revision.Oct 6 2015, 1:57 PM

Currently this change only affects HTTP traffic. For SSH I think DiffusionMercurialServeSSHWorkflow.willWriteMessageCallback() will need updated to intercept the capabilities response.

cspeckmim updated this revision to Diff 34404.Oct 9 2015, 4:43 AM

Slight refactor and updated SSH wire protocol to filter bundle2

  1. Renamed removeMercurial... to filterBundle2Capability and moved into DiffusionMercurialWireProtocol
  2. Moved test into /diffusion/protocol/__tests__/
  3. Updated DiffusionServeController to refer to new filter method
  4. Updated DiffusionMercuralServeSSHWorkflow to apply same capabilities filtering on the SSH wire protocol
cspeckmim edited the test plan for this revision. (Show Details)Oct 9 2015, 4:50 AM
epriestley requested changes to this revision.Oct 9 2015, 11:08 AM
epriestley edited edge metadata.

Two minor nitpicks about code style, take 'em or leave 'em -- I think this is reasonable overall and worth upstreaming until we can implement bundle2 properly.

src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php
122–138

I think you can do this a little more easily as:

$parts = explode(' ', $capabilities);
foreach ($parts as $key => $part) {
  if (preg_match('/^bundle2=/', $part)) {
    unset($parts[$key]);
    break;
  }
}
return implode(' ', $parts);
src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php
16

Consider moving the strings into this method, since it's unlikely that other tests will need to access them. The most standard format in this codebase would be something like:

$cases = array(
  array(
    'name' => pht('Do test 1'),
    'input' => 'lookup ...',
    'expect' => 'lookup ...',
  ),
  ...
);

Then:

foreach ($cases as $case) {
  $actual = DiffusionMercurialWireProtocol...
  $this->assertEqual($case['expect'], $actual, $case['name']);
}

This makes it a little easier to add more cases later, too.

This revision now requires changes to proceed.Oct 9 2015, 11:08 AM
epriestley added a comment.Oct 9 2015, 11:13 AM

Oh, to break the lines you just have to do this kinda junk:

'lookup changegroupsubset branchmap pushkey '.
'known getbundle unbundlehash batch stream '.
'unbundle=HG10GZ,HG10BZ,HG10UN httpheader=1024 '.
'largefiles=serve'
cspeckmim added a comment.Oct 9 2015, 2:35 PM

For the test strings, I had also considered putting them in files, similar to the .cow test, allowing for the filenames to also indicate which version of Mercurial they were from.

src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php
122–138

Ok I'll fix that. I was initially afraid to try and modify the array while iterating over it, as that typically causes runtime errors in Java. Also I forgot about preg_match.

src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php
16

I'll restructure the test in that fashion, it definitely makes more sense. I wasn't sure how best to place the strings - I think I was trying to do something similar to static constants.

cspeckmim updated this revision to Diff 34408.Oct 10 2015, 8:17 AM
cspeckmim edited edge metadata.
  • Updated filter method to be more succinct and formatted test cases appropriately
cspeckmim updated this revision to Diff 34409.Oct 10 2015, 8:21 AM
cspeckmim edited edge metadata.
  • Changed all array items to terminate with comma. I thought the lint had applied this change during last diff update.
cspeckmim added a comment.Oct 10 2015, 8:27 AM

I ran verification again after these latest updates with 3.5.1 on client and 3.5.1 on server. Pushing commits via HTTP and SSH both result in success:

cspeckrun@specktop ~/P/n/hgtest> nano README 
cspeckrun@specktop ~/P/n/hgtest> hg ci -m "test hg................."
cspeckrun@specktop ~/P/n/hgtest> hg pus http
pushing to http://Administrator:***@192.168.0.133/diffusion/HGTEST/hgtest
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
cspeckrun@specktop ~/P/n/hgtest> nano README 
cspeckrun@specktop ~/P/n/hgtest> hg ci -m "test hg.................."
cspeckrun@specktop ~/P/n/hgtest> hg pus ssh
pushing to ssh://phab-devel.code/diffusion/HGTEST/hgtest/
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
cspeckrun@specktop ~/P/n/hgtest>
epriestley accepted this revision.Oct 10 2015, 11:06 AM
epriestley edited edge metadata.

Awesome, thanks!

This revision is now accepted and ready to land.Oct 10 2015, 11:06 AM
epriestley added a comment.Oct 10 2015, 11:06 AM

(I'll pull this after I cut the release, shortly.)

Closed by commit rP32d4ae8cb2cc: Added an intercept to Mercurial's `capabilities` command to remove bundle2. (authored by cspeckmim, committed by epriestley). · Explain WhyOct 10 2015, 2:14 PM
This revision was automatically updated to reflect the committed changes.
urzds mentioned this in T10382: Transaction abort when pushing via SSH to a Mercurial repository.Feb 18 2016, 3:58 PM
epriestley mentioned this in T13036: Fix issues with recent Mercurial wire protocol versions.Jan 4 2018, 7:59 PM
epriestley mentioned this in D18855: Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway.Jan 4 2018, 8:27 PM
epriestley mentioned this in rP0f02d79ffa9d: Remove nonfunctional Mercurial "bundle2" capability filtering from SSH pathway.Jan 4 2018, 10:05 PM

Revision Contents
Changeset List

Diff 34408

View Options

src/__phutil_library_map__.php

Loading...
View Options

src/applications/diffusion/controller/DiffusionServeController.php

Loading...
View Options

src/applications/diffusion/protocol/DiffusionMercurialWireProtocol.php

Loading...
View Options

src/applications/diffusion/protocol/__tests__/DiffusionMercurialWireProtocolTests.php

Loading...
View Options

src/applications/diffusion/ssh/DiffusionMercurialServeSSHWorkflow.php

Loading...
Phabricator · Built by Phacility