Page MenuHomePhabricator

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

Authored by cspeckmim on Oct 6 2015, 5:40 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Aug 3, 6:13 AM
Unknown Object (File)
Tue, Aug 2, 8:21 PM
Unknown Object (File)
Tue, Aug 2, 3:37 PM
Unknown Object (File)
Sun, Jul 24, 12:59 PM
Unknown Object (File)
Mon, Jul 18, 10:10 AM
Unknown Object (File)
Fri, Jul 15, 3:29 AM
Unknown Object (File)
Thu, Jul 14, 8:22 PM
Unknown Object (File)
Thu, Jul 14, 4:10 AM
Subscribers

Details

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
Unit
Tests Passed
Build Status
Buildable 8221
Build 9403: arc lint + arc unit

Event Timeline

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 edited edge metadata.
  • Fixed spelling mistake and other lint issues except for the really long lines

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

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
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

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'

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 edited edge metadata.
  • Updated filter method to be more succinct and formatted test cases appropriately
cspeckmim edited edge metadata.
  • Changed all array items to terminate with comma. I thought the lint had applied this change during last diff update.

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 edited edge metadata.

Awesome, thanks!

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

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

This revision was automatically updated to reflect the committed changes.