From c04147328fa3006c75b6a669d6c0e6ecb335be01 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2020 13:03:34 -0700
Subject: [PATCH] Fix isValidGitShallowCloneResponse

Summary:
Changes the heuristic method by which non-zero exit statuses from git-http-backend are found to be due to packfile negotiation during shallow fetches, etc.

Instead of checking git-http-backend stderr for a generic "hung up" error message, see if the pack-result response contains a terminating flush packet ("0000"). This should give a greater assurance that the request was handled correctly and the response is complete.

Test Plan: Run `GIT_CURL_VERBOSE=1 git fetch --depth 1 https://host.example/source/repo.git HEAD` to ensure it completes and includes two successful POST requests during packfile negotiation (the last one actually receives the packfile).

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: Korvin, dzduvall

Tags: #diffusion

Differential Revision: https://secure.phabricator.com/D21484
---
 .../controller/DiffusionServeController.php   | 20 +++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/applications/diffusion/controller/DiffusionServeController.php b/src/applications/diffusion/controller/DiffusionServeController.php
index 60d5c1578d..1a1383368f 100644
--- a/src/applications/diffusion/controller/DiffusionServeController.php
+++ b/src/applications/diffusion/controller/DiffusionServeController.php
@@ -922,18 +922,26 @@ final class DiffusionServeController extends DiffusionController {
     // This is a pretty funky fix: it would be nice to more precisely detect
     // that a request is a `--depth N` clone request, but we don't have any code
     // to decode protocol frames yet. Instead, look for reasonable evidence
-    // in the error and output that we're looking at a `--depth` clone.
+    // in the output that we're looking at a `--depth` clone.
 
-    // For evidence this isn't completely crazy, see:
-    // https://github.com/schacon/grack/pull/7
+    // A valid x-git-upload-pack-result response during packfile negotiation
+    // should end with a flush packet ("0000"). As long as that packet
+    // terminates the response body in the response, we'll assume the response
+    // is correct and complete.
+
+    // See https://git-scm.com/docs/pack-protocol#_packfile_negotiation
 
     $stdout_regexp = '(^Content-Type: application/x-git-upload-pack-result)m';
-    $stderr_regexp = '(The remote end hung up unexpectedly)';
 
     $has_pack = preg_match($stdout_regexp, $stdout);
-    $is_hangup = preg_match($stderr_regexp, $stderr);
 
-    return $has_pack && $is_hangup;
+    if (strlen($stdout) >= 4) {
+      $has_flush_packet = (substr($stdout, -4) === "0000");
+    } else {
+      $has_flush_packet = false;
+    }
+
+    return ($has_pack && $has_flush_packet);
   }
 
   private function getCommonEnvironment(PhabricatorUser $viewer) {