From 904dbf0db66618f33f61874a9256adcc3c6faa0b Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 16 Apr 2019 09:08:39 -0700 Subject: [PATCH] Make the "git upload-pack" proxy more robust Summary: Depends on D20381. Ref T8093. This makes minor improvements to the protocol proxy to handle cases where we add, remove, or replace refs and may need to move the "capabilities" section. Rather than invoking a callback on every ref: parse the whole ref list into a data structure, mutate it if necessary (in a future diff), then dump it back into wire format. This allows us to shift the capabilities data (which needs to be coupled with the first ref) around if we modify the first ref, and reorder the reflist alphabetically like git does. When the server has no refs, Git sends no capabilities data. This is easy to emulate, just surprising. Test Plan: Tested the cases not covered by D20381: - Fetching where the fetch actually fetches data. - `ls-remote` when we hide the first ref (capabilities data properly moves to the first visible ref). - `ls-remote` when the remote is empty (we just drop the capabilities frame completely). Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T8093 Differential Revision: https://secure.phabricator.com/D20436 --- src/__phutil_library_map__.php | 6 + .../DiffusionGitUploadPackWireProtocol.php | 124 ++++++++---------- .../DiffusionGitWireProtocolCapabilities.php | 18 +++ .../protocol/DiffusionGitWireProtocolRef.php | 32 +++++ .../DiffusionGitWireProtocolRefList.php | 28 ++++ 5 files changed, 141 insertions(+), 67 deletions(-) create mode 100644 src/applications/diffusion/protocol/DiffusionGitWireProtocolCapabilities.php create mode 100644 src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php create mode 100644 src/applications/diffusion/protocol/DiffusionGitWireProtocolRefList.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b171d6b072..9dfec9c718 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -815,6 +815,9 @@ phutil_register_library_map(array( 'DiffusionGitUploadPackSSHWorkflow' => 'applications/diffusion/ssh/DiffusionGitUploadPackSSHWorkflow.php', 'DiffusionGitUploadPackWireProtocol' => 'applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php', 'DiffusionGitWireProtocol' => 'applications/diffusion/protocol/DiffusionGitWireProtocol.php', + 'DiffusionGitWireProtocolCapabilities' => 'applications/diffusion/protocol/DiffusionGitWireProtocolCapabilities.php', + 'DiffusionGitWireProtocolRef' => 'applications/diffusion/protocol/DiffusionGitWireProtocolRef.php', + 'DiffusionGitWireProtocolRefList' => 'applications/diffusion/protocol/DiffusionGitWireProtocolRefList.php', 'DiffusionGraphController' => 'applications/diffusion/controller/DiffusionGraphController.php', 'DiffusionHistoryController' => 'applications/diffusion/controller/DiffusionHistoryController.php', 'DiffusionHistoryListView' => 'applications/diffusion/view/DiffusionHistoryListView.php', @@ -6464,6 +6467,9 @@ phutil_register_library_map(array( 'DiffusionGitUploadPackSSHWorkflow' => 'DiffusionGitSSHWorkflow', 'DiffusionGitUploadPackWireProtocol' => 'DiffusionGitWireProtocol', 'DiffusionGitWireProtocol' => 'Phobject', + 'DiffusionGitWireProtocolCapabilities' => 'Phobject', + 'DiffusionGitWireProtocolRef' => 'Phobject', + 'DiffusionGitWireProtocolRefList' => 'Phobject', 'DiffusionGraphController' => 'DiffusionController', 'DiffusionHistoryController' => 'DiffusionController', 'DiffusionHistoryListView' => 'DiffusionHistoryView', diff --git a/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php b/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php index 5371586439..451ad74c9e 100644 --- a/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php +++ b/src/applications/diffusion/protocol/DiffusionGitUploadPackWireProtocol.php @@ -166,13 +166,17 @@ final class DiffusionGitUploadPackWireProtocol $head_key = head_key($frames); $last_key = last_key($frames); - $output = array(); + $capabilities = null; + $last_frame = null; + + $refs = array(); foreach ($frames as $key => $frame) { $is_last = ($key === $last_key); if ($is_last) { - $output[] = $frame; // This is a "0000" frame at the end of the list of refs, so we pass - // it through unmodified. + // it through unmodified after we figure out what the rest of the + // frames should look like, below. + $last_frame = $frame; continue; } @@ -230,42 +234,69 @@ final class DiffusionGitUploadPackWireProtocol $hash = $matches['hash']; $name = $matches['name']; - $capabilities = idx($matches, 'capabilities'); - $ref = array( - 'hash' => $hash, - 'name' => $name, - 'capabilities' => $capabilities, - ); - - $old_ref = $ref; - - $ref = $this->willReadRef($ref); - - $new_ref = $ref; - - $this->didRewriteRef($old_ref, $new_ref); - - if ($ref === null) { - continue; + if ($is_first) { + $capabilities = $matches['capabilities']; } - if (isset($ref['capabilities'])) { + $refs[] = array( + 'hash' => $hash, + 'name' => $name, + ); + } + + $capabilities = DiffusionGitWireProtocolCapabilities::newFromWireFormat( + $capabilities); + + $ref_list = id(new DiffusionGitWireProtocolRefList()) + ->setCapabilities($capabilities); + + foreach ($refs as $ref) { + $ref_list->addRef( + id(new DiffusionGitWireProtocolRef()) + ->setName($ref['name']) + ->setHash($ref['hash'])); + } + + // TODO: Here, we have a structured list of refs. In a future change, + // we are free to mutate the structure before flattening it back into + // wire format. + + $refs = $ref_list->getRefs(); + + // Before we write the ref list, sort it for consistency with native + // Git output. We may have added, removed, or renamed refs and ended up + // with an out-of-order list. + + $refs = msortv($refs, 'newSortVector'); + + // The first ref we send back includes the capabilities data. Note that if + // we send back no refs, we also don't send back capabilities! This is + // a little surprising, but is consistent with the native behavior of the + // protocol. + + $output = array(); + $is_first = true; + foreach ($refs as $ref) { + if ($is_first) { $result = sprintf( "%s %s\0%s\n", - $ref['hash'], - $ref['name'], - $ref['capabilities']); + $ref->getHash(), + $ref->getName(), + $ref_list->getCapabilities()->toWireFormat()); } else { $result = sprintf( "%s %s\n", - $ref['hash'], - $ref['name']); + $ref->getHash(), + $ref->getName()); } $output[] = $this->newProtocolFrame('data', $result); + $is_first = false; } + $output[] = $last_frame; + return $this->newProtocolDataMessage($output); } @@ -291,45 +322,4 @@ final class DiffusionGitUploadPackWireProtocol return $message; } - private function willReadRef(array $ref) { - return $ref; - } - - private function didRewriteRef($old_ref, $new_ref) { - $log = $this->getProtocolLog(); - if (!$log) { - return; - } - - if (!$old_ref) { - $old_name = null; - } else { - $old_name = $old_ref['name']; - } - - if (!$new_ref) { - $new_name = null; - } else { - $new_name = $new_ref['name']; - } - - if ($old_name === $new_name) { - return; - } - - if ($old_name === null) { - $old_name = ''; - } - - if ($new_name === null) { - $new_name = ''; - } - - $log->didWriteFrame( - pht( - 'Rewrite Ref: %s -> %s', - $old_name, - $new_name)); - } - } diff --git a/src/applications/diffusion/protocol/DiffusionGitWireProtocolCapabilities.php b/src/applications/diffusion/protocol/DiffusionGitWireProtocolCapabilities.php new file mode 100644 index 0000000000..865a061484 --- /dev/null +++ b/src/applications/diffusion/protocol/DiffusionGitWireProtocolCapabilities.php @@ -0,0 +1,18 @@ +raw = $raw; + return $capabilities; + } + + public function toWireFormat() { + return $this->raw; + } + +} diff --git a/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php new file mode 100644 index 0000000000..bf5238c219 --- /dev/null +++ b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRef.php @@ -0,0 +1,32 @@ +name = $name; + return $this; + } + + public function getName() { + return $this->name; + } + + public function setHash($hash) { + $this->hash = $hash; + return $this; + } + + public function getHash() { + return $this->hash; + } + + public function newSortVector() { + return id(new PhutilSortVector()) + ->addString($this->getName()); + } + +} diff --git a/src/applications/diffusion/protocol/DiffusionGitWireProtocolRefList.php b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRefList.php new file mode 100644 index 0000000000..f4b2b70000 --- /dev/null +++ b/src/applications/diffusion/protocol/DiffusionGitWireProtocolRefList.php @@ -0,0 +1,28 @@ +capabilities = $capabilities; + return $this; + } + + public function getCapabilities() { + return $this->capabilities; + } + + public function addRef(DiffusionGitWireProtocolRef $ref) { + $this->refs[] = $ref; + return $this; + } + + public function getRefs() { + return $this->refs; + } + +}