Page MenuHomePhabricator

Fix isValidGitShallowCloneResponse
ClosedPublic

Authored by dduvall on Oct 23 2020, 11:26 PM.

Details

Maniphest Tasks
T240862: Can't do shallow clone from phabricator
Reviewers
mmodell
thcipriani
Commits
rPHAB620fa0816bfc: Fix isValidGitShallowCloneResponse
Patch without arc
git checkout -b D1185 && curl -L https://phabricator.wikimedia.org/D1185?download=true | git apply
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.

Submitted upstream as https://secure.phabricator.com/D21484

Bug: T240862

Test Plan

The following request should respond with a flush packet ("0000"), not close the connection unexpectedly.

echo -ne "0091want 09a632607a03e09fa750884a819b91b400504619 multi_ack_detailed no-done side-band-64k thin-pack no-progress ofs-delta agent=git/2.6.1.dirty\n0034shallow 09a632607a03e09fa750884a819b91b400504619000cdeepen 10000" | curl -v --http1.1 https://phabricator.wikimedia.org/diffusion/ANAB/analytics-blog.git/git-upload-pack -H 'Content-Type: application/x-git-upload-pack-request' -H 'Accept: application/x-git-upload-pack-result' --data-binary @-

Diff Detail

Repository
rPHAB Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

dduvall created this revision.

+1

The output of the shallow clone from the "git-http-backend" is:

thcipriani@phab1001:~$ sudo -n -u phd ./fake-git-request.sh                                                                                            
Expires: Fri, 01 Jan 1980 00:00:00 GMT                                                                                                                 
Pragma: no-cache                                                                                                                                       
Cache-Control: no-cache, max-age=0, must-revalidate                                                                                                    
Content-Type: application/x-git-upload-pack-result                                                                                                     
                                                                                                                                                       
0000fatal: the remote end hung up unexpectedly

Note the lowercase t in the. Darn regexes.

I was reading a bit more about packfile negotiation and it seems like a better heuristic approach than matching a regex against the stderr for this function's implementation might be to:

  1. Look for a Content-Type: application/x-git-upload/pack-result header as is already the case.
  2. In addition, inspect whether the final line of git-http-backend output is a flush command ("0000"). This should indicate that the server is done sending a set of packet lines but not blindly assume that a broken pipe is not a real failure.

It also seems like upstream is suffering from this bug as well, so maybe I should submit a fit there?

$ git fetch --depth 1 https://secure.phabricator.com/source/phabricator.git HEAD
error: RPC failed; result=22, HTTP code = 500
fatal: The remote end hung up unexpectedly
dduvall retitled this revision from Fix regex in isValidGitShallowCloneResponse to Fix isValidGitShallowCloneResponse.
dduvall edited the summary of this revision. (Show Details)
dduvall edited the test plan for this revision. (Show Details)

I've updated the patch to match what I submitted upstream.

This revision is now accepted and ready to land.Oct 28 2020, 3:21 PM