Merge branch 'remove-local-git-diff' into 'master'

Remove local git diff handling

See merge request gitlab-org/gitlab-workhorse!345

gitaly_integration_test.go

@@ -225,3 +225,30 @@ func TestAllowedGetGitArchive(t *testing.T) {
 
 	assert.True(t, foundEntry, "Couldn't find %v directory entry", archivePrefix)
 }
+
+func TestAllowedGetGitDiff(t *testing.T) {
+	skipUnlessRealGitaly(t)
+
+	// Create the repository in the Gitaly server
+	apiResponse := realGitalyOkBody(t)
+	require.NoError(t, ensureGitalyRepository(t, apiResponse))
+
+	leftCommit := "8a0f2ee90d940bfb0ba1e14e8214b0649056e4ab"
+	rightCommit := "e395f646b1499e8e0279445fc99a0596a65fab7e"
+	expectedBody := "diff --git a/README.md b/README.md"
+
+	msg := serializedMessage("RawDiffRequest", &pb.RawDiffRequest{
+		Repository:    &apiResponse.Repository,
+		LeftCommitId:  leftCommit,
+		RightCommitId: rightCommit,
+	})
+	jsonParams := buildGitalyRPCParams(gitalyAddress, msg)
+
+	resp, body, err := doSendDataRequest("/something", "git-diff", jsonParams)
+	require.NoError(t, err)
+	shortBody := string(body[:len(expectedBody)])
+
+	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resp.Request.URL)
+	assert.Equal(t, expectedBody, shortBody, "GET %q: response body", resp.Request.URL)
+	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
+}

internal/git/diff.go

@@ -2,7 +2,6 @@ package git
 
 import (
 	"fmt"
-	"io"
 	"net/http"
 
 	"github.com/golang/protobuf/jsonpb"
@@ -10,15 +9,11 @@ import (
 
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/gitaly"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/helper"
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/log"
 	"gitlab.com/gitlab-org/gitlab-workhorse/internal/senddata"
 )
 
 type diff struct{ senddata.Prefix }
 type diffParams struct {
-	RepoPath       string
-	ShaFrom        string
-	ShaTo          string
 	GitalyServer   gitaly.Server
 	RawDiffRequest string
 }
@@ -32,22 +27,16 @@ func (d *diff) Inject(w http.ResponseWriter, r *http.Request, sendData string) {
 		return
 	}
 
-	if params.GitalyServer.Address != "" {
-		handleSendDiffWithGitaly(w, r, &params)
-	} else {
-		handleSendDiffLocally(w, r, &params)
-	}
-}
-
-func handleSendDiffWithGitaly(w http.ResponseWriter, r *http.Request, params *diffParams) {
 	request := &pb.RawDiffRequest{}
 	if err := jsonpb.UnmarshalString(params.RawDiffRequest, request); err != nil {
 		helper.Fail500(w, r, fmt.Errorf("diff.RawDiff: %v", err))
+		return
 	}
 
 	diffClient, err := gitaly.NewDiffClient(params.GitalyServer)
 	if err != nil {
 		helper.Fail500(w, r, fmt.Errorf("diff.RawDiff: %v", err))
+		return
 	}
 
 	if err := diffClient.SendRawDiff(r.Context(), w, request); err != nil {
@@ -55,39 +44,6 @@ func handleSendDiffWithGitaly(w http.ResponseWriter, r *http.Request, params *di
 			r,
 			&copyError{fmt.Errorf("diff.RawDiff: request=%v, err=%v", request, err)},
 		)
-	}
-}
-
-func handleSendDiffLocally(w http.ResponseWriter, r *http.Request, params *diffParams) {
-	log.WithFields(r.Context(), log.Fields{
-		"shaFrom": params.ShaFrom,
-		"shaTo":   params.ShaTo,
-		"path":    r.URL.Path,
-	}).Print("SendDiff: sending diff")
-
-	gitDiffCmd := gitCommand("git", "--git-dir="+params.RepoPath, "diff", params.ShaFrom, params.ShaTo)
-	stdout, err := gitDiffCmd.StdoutPipe()
-	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendDiff: create stdout pipe: %v", err))
-		return
-	}
-
-	if err := gitDiffCmd.Start(); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendDiff: start %v: %v", gitDiffCmd.Args, err))
-		return
-	}
-	defer helper.CleanUpProcessGroup(gitDiffCmd)
-
-	w.Header().Del("Content-Length")
-	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(
-			r,
-			&copyError{fmt.Errorf("SendDiff: copy %v stdout: %v", gitDiffCmd.Args, err)},
-		)
-		return
-	}
-	if err := gitDiffCmd.Wait(); err != nil {
-		helper.LogError(r, fmt.Errorf("SendDiff: wait for %v: %v", gitDiffCmd.Args, err))
 		return
 	}
 }

main_test.go

@@ -380,21 +380,6 @@ func TestSendURLForArtifacts(t *testing.T) {
 	assert.Equal(t, fileContents, string(body), "GET %q: response body", resourcePath)
 }
 
-func TestGetGitDiff(t *testing.T) {
-	fromSha := "be93687618e4b132087f430a4d8fc3a609c9b77c"
-	toSha := "54fcc214b94e78d7a41a9a8fe6d87a5e59500e51"
-	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
-	expectedBody := "diff --git a/README b/README"
-
-	resp, body, err := doSendDataRequest("/something", "git-diff", jsonParams)
-	require.NoError(t, err)
-
-	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resp.Request.URL)
-	assert.Equal(t, expectedBody, string(body[:len(expectedBody)]), "GET %q: response body", resp.Request.URL)
-	assert.Equal(t, 155, len(body), "GET %q: body size", resp.Request.URL)
-	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
-}
-
 func TestGetGitPatch(t *testing.T) {
 	// HEAD of master branch against HEAD of fix branch
 	fromSha := "6907208d755b60ebeacb2e9dfea74c92c3449a1f"