Merge branch 'remove-local-git-format-patch' into 'master'

Remove local git format-patch handling

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

.gitlab-ci.yml

@@ -13,7 +13,6 @@ verify:
   variables:
     GITALY_ADDRESS: "tcp://gitaly:8075"
   script:
-  - apt update -qq && apt install -y unzip bzip2
   - go version
   - make test
 

gitaly_integration_test.go

@@ -252,3 +252,36 @@ func TestAllowedGetGitDiff(t *testing.T) {
 	assert.Equal(t, expectedBody, shortBody, "GET %q: response body", resp.Request.URL)
 	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
 }
+
+func TestAllowedGetGitFormatPatch(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"
+	msg := serializedMessage("RawPatchRequest", &pb.RawPatchRequest{
+		Repository:    &apiResponse.Repository,
+		LeftCommitId:  leftCommit,
+		RightCommitId: rightCommit,
+	})
+	jsonParams := buildGitalyRPCParams(gitalyAddress, msg)
+
+	resp, body, err := doSendDataRequest("/something", "git-format-patch", jsonParams)
+	require.NoError(t, err)
+
+	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resp.Request.URL)
+	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
+
+	testhelper.AssertPatchSeries(
+		t,
+		body,
+		"372ab6950519549b14d220271ee2322caa44d4eb",
+		"57290e673a4c87f51294f5216672cbc58d485d25",
+		"41ae11ba5d091d73d5de671f6fa7d1a4539e979e",
+		"742518b2be68fc750bb4c357c0df821a88113286",
+		rightCommit,
+	)
+}

gitaly_test.go

@@ -36,7 +36,6 @@ func TestFailedCloneNoGitaly(t *testing.T) {
 	authBody := &api.Response{
 		GL_ID:       "user-123",
 		GL_USERNAME: "username",
-		RepoPath:    repoPath(t),
 		// This will create a failure to connect to Gitaly
 		GitalyServer: gitaly.Server{Address: "unix:/nonexistent"},
 	}

internal/api/api.go

@@ -106,9 +106,6 @@ type Response struct {
 	// GL_REPOSITORY is an environment variable used by gitlab-shell hooks during
 	// 'git push' and 'git pull'
 	GL_REPOSITORY string
-	// RepoPath is the full path on disk to the Git repository the request is
-	// about
-	RepoPath string
 	// GitConfigOptions holds the custom options that we want to pass to the git command
 	GitConfigOptions []string
 	// StoreLFSPath is provided by the GitLab Rails application to mark where the tmp file should be placed.
@@ -132,8 +129,7 @@ type Response struct {
 	Terminal *TerminalSettings
 	// GitalyServer specifies an address and authentication token for a gitaly server we should connect to.
 	GitalyServer gitaly.Server
-	// Repository object for making gRPC requests to Gitaly. This will
-	// eventually replace the RepoPath field.
+	// Repository object for making gRPC requests to Gitaly.
 	Repository pb.Repository
 	// For git-http, does the requestor have the right to view all refs?
 	ShowAllRefs bool

internal/git/command.go

-package git
-
-import (
-	"fmt"
-	"os"
-	"os/exec"
-	"syscall"
-
-	"gitlab.com/gitlab-org/gitlab-workhorse/internal/api"
-)
-
-var execCommand = exec.Command
-
-// Git subprocess helpers
-func gitCommandApi(a *api.Response, name string, args ...string) *exec.Cmd {
-	cmd := execCommand(name, args...)
-	// Start the command in its own process group (nice for signalling)
-	cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true}
-	// Explicitly set the environment for the Git command
-	cmd.Env = []string{
-		fmt.Sprintf("HOME=%s", os.Getenv("HOME")),
-		fmt.Sprintf("PATH=%s", os.Getenv("PATH")),
-		fmt.Sprintf("LD_LIBRARY_PATH=%s", os.Getenv("LD_LIBRARY_PATH")),
-		fmt.Sprintf("GL_PROTOCOL=http"),
-	}
-	if a != nil {
-		cmd.Env = append(cmd.Env, fmt.Sprintf("GL_ID=%s", a.GL_ID))
-		cmd.Env = append(cmd.Env, fmt.Sprintf("GL_USERNAME=%s", a.GL_USERNAME))
-		if a.GL_REPOSITORY != "" {
-			cmd.Env = append(cmd.Env, fmt.Sprintf("GL_REPOSITORY=%s", a.GL_REPOSITORY))
-		}
-	}
-	// If we don't do something with cmd.Stderr, Git errors will be lost
-	cmd.Stderr = os.Stderr
-	return cmd
-}
-
-func gitCommand(name string, args ...string) *exec.Cmd {
-	return gitCommandApi(nil, name, args...)
-}

internal/git/format-patch.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 patch struct{ senddata.Prefix }
 type patchParams struct {
-	RepoPath        string
-	ShaFrom         string
-	ShaTo           string
 	GitalyServer    gitaly.Server
 	RawPatchRequest string
 }
@@ -31,22 +26,17 @@ func (p *patch) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 		helper.Fail500(w, r, fmt.Errorf("SendPatch: unpack sendData: %v", err))
 		return
 	}
-	if params.GitalyServer.Address != "" {
-		handleSendPatchWithGitaly(w, r, &params)
-	} else {
-		handleSendPatchLocally(w, r, &params)
-	}
-}
 
-func handleSendPatchWithGitaly(w http.ResponseWriter, r *http.Request, params *patchParams) {
 	request := &pb.RawPatchRequest{}
 	if err := jsonpb.UnmarshalString(params.RawPatchRequest, request); err != nil {
 		helper.Fail500(w, r, fmt.Errorf("diff.RawPatch: %v", err))
+		return
 	}
 
 	diffClient, err := gitaly.NewDiffClient(params.GitalyServer)
 	if err != nil {
 		helper.Fail500(w, r, fmt.Errorf("diff.RawPatch: %v", err))
+		return
 	}
 
 	if err := diffClient.SendRawPatch(r.Context(), w, request); err != nil {
@@ -54,38 +44,6 @@ func handleSendPatchWithGitaly(w http.ResponseWriter, r *http.Request, params *p
 			r,
 			&copyError{fmt.Errorf("diff.RawPatch: request=%v, err=%v", request, err)},
 		)
-	}
-}
-
-func handleSendPatchLocally(w http.ResponseWriter, r *http.Request, params *patchParams) {
-	log.WithFields(r.Context(), log.Fields{
-		"shaFrom": params.ShaFrom,
-		"shaTo":   params.ShaTo,
-		"path":    r.URL.Path,
-	}).Print("SendPatch: sending patch")
-
-	gitRange := fmt.Sprintf("%s..%s", params.ShaFrom, params.ShaTo)
-	gitPatchCmd := gitCommand("git", "--git-dir="+params.RepoPath, "format-patch", gitRange, "--stdout")
-
-	stdout, err := gitPatchCmd.StdoutPipe()
-	if err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendPatch: create stdout pipe: %v", err))
-		return
-	}
-
-	if err := gitPatchCmd.Start(); err != nil {
-		helper.Fail500(w, r, fmt.Errorf("SendPatch: start %v: %v", gitPatchCmd.Args, err))
-		return
-	}
-	defer helper.CleanUpProcessGroup(gitPatchCmd)
-
-	w.Header().Del("Content-Length")
-	if _, err := io.Copy(w, stdout); err != nil {
-		helper.LogError(r, &copyError{fmt.Errorf("SendPatch: copy %v stdout: %v", gitPatchCmd.Args, err)})
-		return
-	}
-	if err := gitPatchCmd.Wait(); err != nil {
-		helper.LogError(r, fmt.Errorf("SendPatch: wait for %v: %v", gitPatchCmd.Args, err))
 		return
 	}
 }

main_test.go

@@ -380,22 +380,6 @@ func TestSendURLForArtifacts(t *testing.T) {
 	assert.Equal(t, fileContents, string(body), "GET %q: response body", resourcePath)
 }
 
-func TestGetGitPatch(t *testing.T) {
-	// HEAD of master branch against HEAD of fix branch
-	fromSha := "6907208d755b60ebeacb2e9dfea74c92c3449a1f"
-	toSha := "48f0be4bd10c1decee6fae52f9ae6d10f77b60f4"
-	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
-
-	resp, body, err := doSendDataRequest("/something", "git-format-patch", jsonParams)
-	require.NoError(t, err)
-
-	assert.Equal(t, 200, resp.StatusCode, "GET %q: status code", resp.Request.URL)
-	assertNginxResponseBuffering(t, "no", resp, "GET %q: nginx response buffering", resp.Request.URL)
-
-	// Only the two commits on the fix branch should be included
-	testhelper.AssertPatchSeries(t, body, "12d65c8dd2b2676fa3ac47d955accc085a37a9c1", toSha)
-}
-
 func TestApiContentTypeBlock(t *testing.T) {
 	wrongResponse := `{"hello":"world"}`
 	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, _ *http.Request) {
@@ -538,11 +522,9 @@ func runOrFail(t *testing.T, cmd *exec.Cmd) {
 }
 
 func gitOkBody(t *testing.T) *api.Response {
-	repoPath := repoPath(t)
 	return &api.Response{
 		GL_ID:       "user-123",
 		GL_USERNAME: "username",
-		RepoPath:    repoPath,
 		Repository: pb.Repository{
 			StorageName:  "default",
 			RelativePath: "foo/bar.git",
@@ -550,13 +532,6 @@ func gitOkBody(t *testing.T) *api.Response {
 	}
 }
 
-func repoPath(t *testing.T) string {
-	cwd, err := os.Getwd()
-	require.NoError(t, err)
-
-	return path.Join(cwd, testRepoRoot, testRepo)
-}
-
 func httpGet(t *testing.T, url string, headers map[string]string) (*http.Response, string) {
 	req, err := http.NewRequest("GET", url, nil)
 	require.NoError(t, err)