Merge branch 'change-diff-and-patch-specs' into 'master'

Change the revspec for `git format-patch`

For merge requests, GitLab currently sends:

* ShaFrom == tip of target branch
* ShaTo == tip of source branch

This MR adds tests for this scenario and the alternative:

* ShaFrom == base SHA
* ShaTo == tip of source branch

It then makes the test for the first case pass by altering the revspec. 

What to do with diffs is still an open question, so we leave those untouched for the moment.

See https://gitlab.com/gitlab-org/gitlab-ce/issues/22229

See merge request !68

internal/git/format-patch.go

@@ -28,7 +28,7 @@ func (p *patch) Inject(w http.ResponseWriter, r *http.Request, sendData string)
 
 	log.Printf("SendPatch: sending patch between %q and %q for %q", params.ShaFrom, params.ShaTo, r.URL.Path)
 
-	gitRange := fmt.Sprintf("%v...%v", params.ShaFrom, params.ShaTo)
+	gitRange := fmt.Sprintf("%s..%s", params.ShaFrom, params.ShaTo)
 	gitPatchCmd := gitCommand("", "git", "--git-dir="+params.RepoPath, "format-patch", gitRange, "--stdout")
 
 	stdout, err := gitPatchCmd.StdoutPipe()

internal/testhelper/testhelper.go

 package testhelper
 
 import (
+	"bufio"
+	"bytes"
 	"errors"
 	"fmt"
 	"io/ioutil"
@@ -12,6 +14,7 @@ import (
 	"path"
 	"regexp"
 	"runtime"
+	"strings"
 	"testing"
 )
 
@@ -19,6 +22,35 @@ func SecretPath() string {
 	return path.Join(RootDir(), "testdata/test-secret")
 }
 
+var extractPatchSeriesMatcher = regexp.MustCompile(`^From (\w+)`)
+
+// AssertPatchSeries takes a `git format-patch` blob, extracts the From xxxxx
+// lines and compares the SHAs to expected list.
+func AssertPatchSeries(t *testing.T, blob []byte, expected ...string) {
+	var actual []string
+	footer := make([]string, 3)
+
+	scanner := bufio.NewScanner(bytes.NewReader(blob))
+
+	for scanner.Scan() {
+		line := scanner.Text()
+		if matches := extractPatchSeriesMatcher.FindStringSubmatch(line); len(matches) == 2 {
+			actual = append(actual, matches[1])
+		}
+		footer = []string{footer[1], footer[2], line}
+	}
+
+	if strings.Join(actual, "\n") != strings.Join(expected, "\n") {
+		t.Fatalf("Patch series differs. Expected: %v. Got: %v", expected, actual)
+	}
+
+	// Check the last returned patch is complete
+	// Don't assert on the final line, it is a git version
+	if footer[0] != "-- " {
+		t.Fatalf("Expected end of patch, found: \n\t%q", strings.Join(footer, "\n\t"))
+	}
+}
+
 func AssertResponseCode(t *testing.T, response *httptest.ResponseRecorder, expectedCode int) {
 	if response.Code != expectedCode {
 		t.Fatalf("for HTTP request expected to get %d, got %d instead", expectedCode, response.Code)

main_test.go

@@ -577,35 +577,68 @@ func TestArtifactsUpload(t *testing.T) {
 	}
 }
 
-func TestArtifactsGetSingleFile(t *testing.T) {
-	// We manually created this zip file in the gitlab-workhorse Git repository
-	archivePath := `testdata/artifacts-archive.zip`
-	fileName := "myfile"
-	fileContents := "MY FILE"
-	resourcePath := `/namespace/project/builds/123/artifacts/file/` + fileName
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`\A`+resourcePath+`\z`), func(w http.ResponseWriter, r *http.Request) {
-		encodedFilename := base64.StdEncoding.EncodeToString([]byte(fileName))
-		jsonParams := fmt.Sprintf(`{"Archive":"%s","Entry":"%s"}`, archivePath, encodedFilename)
-		data := base64.URLEncoding.EncodeToString([]byte(jsonParams))
-		w.Header().Set("Gitlab-Workhorse-Send-Data", "artifacts-entry:"+data)
+var sendDataHeader = "Gitlab-Workhorse-Send-Data"
+
+func sendDataResponder(command string, literalJSON string) *httptest.Server {
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		data := base64.URLEncoding.EncodeToString([]byte(literalJSON))
+		w.Header().Set(sendDataHeader, fmt.Sprintf("%s:%s", command, data))
+
+		// This should never be returned
+		if _, err := fmt.Fprintf(w, "gibberish"); err != nil {
+			panic(err)
+		}
+
 		return
-	})
+	}
+
+	return testhelper.TestServerWithHandler(regexp.MustCompile(`.`), handler)
+}
+
+func doSendDataRequest(path string, command, literalJSON string) (*http.Response, []byte, error) {
+	ts := sendDataResponder(command, literalJSON)
 	defer ts.Close()
+
 	ws := startWorkhorseServer(ts.URL)
 	defer ws.Close()
 
-	resp, err := http.Get(ws.URL + resourcePath)
+	resp, err := http.Get(ws.URL + path)
 	if err != nil {
-		t.Error(err)
+		return nil, nil, err
 	}
 	defer resp.Body.Close()
-	if resp.StatusCode != 200 {
-		t.Errorf("GET %q: expected 200, got %d", resourcePath, resp.StatusCode)
+
+	bodyData, err := ioutil.ReadAll(resp.Body)
+	if err != nil {
+		return resp, nil, err
 	}
-	body, err := ioutil.ReadAll(resp.Body)
+
+	headerValue := resp.Header.Get(sendDataHeader)
+	if headerValue != "" {
+		return resp, bodyData, fmt.Errorf("%s header should not be present, but has value %q", sendDataHeader, headerValue)
+	}
+
+	return resp, bodyData, nil
+}
+
+func TestArtifactsGetSingleFile(t *testing.T) {
+	// We manually created this zip file in the gitlab-workhorse Git repository
+	archivePath := `testdata/artifacts-archive.zip`
+	fileName := "myfile"
+	fileContents := "MY FILE"
+	resourcePath := `/namespace/project/builds/123/artifacts/file/` + fileName
+	encodedFilename := base64.StdEncoding.EncodeToString([]byte(fileName))
+	jsonParams := fmt.Sprintf(`{"Archive":"%s","Entry":"%s"}`, archivePath, encodedFilename)
+
+	resp, body, err := doSendDataRequest(resourcePath, "artifacts-entry", jsonParams)
 	if err != nil {
-		t.Fatal(err)
+		t.Error(err)
 	}
+
+	if resp.StatusCode != http.StatusOK {
+		t.Errorf("GET %q: expected HTTP 200, got %d", resp.Request.URL, resp.StatusCode)
+	}
+
 	if string(body) != fileContents {
 		t.Fatalf("Expected file contents %q, got %q", fileContents, body)
 	}
@@ -614,41 +647,25 @@ func TestArtifactsGetSingleFile(t *testing.T) {
 func TestGetGitBlob(t *testing.T) {
 	blobId := "50b27c6518be44c42c4d87966ae2481ce895624c" // the LICENSE file in the test repository
 	blobLength := 1075
-	headerKey := http.CanonicalHeaderKey("Gitlab-Workhorse-Send-Data")
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		responseJSON := fmt.Sprintf(`{"RepoPath":"%s","BlobId":"%s"}`, path.Join(testRepoRoot, testRepo), blobId)
-		encodedJSON := base64.URLEncoding.EncodeToString([]byte(responseJSON))
-		w.Header().Set(headerKey, "git-blob:"+encodedJSON)
-		if _, err := fmt.Fprintf(w, "GNU General Public License"); err != nil {
-			t.Fatal(err)
-		}
-		return
-	})
-	defer ts.Close()
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-	resourcePath := "/something"
-	resp, err := http.Get(ws.URL + resourcePath)
+	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","BlobId":"%s"}`, path.Join(testRepoRoot, testRepo), blobId)
+
+	resp, body, err := doSendDataRequest("/something", "git-blob", jsonParams)
 	if err != nil {
 		t.Error(err)
 	}
-	defer resp.Body.Close()
-	if resp.StatusCode != 200 {
-		t.Errorf("GET %q: expected 200, got %d", resourcePath, resp.StatusCode)
-	}
-	if len(resp.Header[headerKey]) != 0 {
-		t.Fatalf("Unexpected response header: %s: %q", headerKey, resp.Header.Get(headerKey))
-	}
-	body, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		t.Fatal(err)
+
+	if resp.StatusCode != http.StatusOK {
+		t.Errorf("GET %q: expected HTTP 200, got %d", resp.Request.URL, resp.StatusCode)
 	}
+
 	if len(body) != blobLength {
 		t.Fatalf("Expected body of %d bytes, got %d", blobLength, len(body))
 	}
+
 	if cl := resp.Header.Get("Content-Length"); cl != fmt.Sprintf("%d", blobLength) {
 		t.Fatalf("Expected Content-Length %v, got %q", blobLength, cl)
 	}
+
 	if !strings.HasPrefix(string(body), "The MIT License (MIT)") {
 		t.Fatalf("Expected MIT license, got %q", body)
 	}
@@ -657,35 +674,15 @@ func TestGetGitBlob(t *testing.T) {
 func TestGetGitDiff(t *testing.T) {
 	fromSha := "be93687618e4b132087f430a4d8fc3a609c9b77c"
 	toSha := "54fcc214b94e78d7a41a9a8fe6d87a5e59500e51"
-	headerKey := http.CanonicalHeaderKey("Gitlab-Workhorse-Send-Data")
+	jsonParams := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
 
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		responseJSON := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
-		encodedJSON := base64.URLEncoding.EncodeToString([]byte(responseJSON))
-		w.Header().Set(headerKey, "git-diff:"+encodedJSON)
-		return
-	})
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resourcePath := "/something"
-	resp, err := http.Get(ws.URL + resourcePath)
+	resp, body, err := doSendDataRequest("/something", "git-diff", jsonParams)
 	if err != nil {
 		t.Error(err)
 	}
-	defer resp.Body.Close()
-	if resp.StatusCode != 200 {
-		t.Errorf("GET %q: expected 200, got %d", resourcePath, resp.StatusCode)
-	}
-	if len(resp.Header[headerKey]) != 0 {
-		t.Fatalf("Unexpected response header: %s: %q", headerKey, resp.Header.Get(headerKey))
-	}
 
-	body, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		t.Fatal(err)
+	if resp.StatusCode != http.StatusOK {
+		t.Errorf("GET %q: expected HTTP 200, got %d", resp.Request.URL, resp.StatusCode)
 	}
 
 	if !strings.HasPrefix(string(body), "diff --git a/README b/README") {
@@ -699,52 +696,22 @@ func TestGetGitDiff(t *testing.T) {
 }
 
 func TestGetGitPatch(t *testing.T) {
-	fromSha := "be93687618e4b132087f430a4d8fc3a609c9b77c"
-	toSha := "54fcc214b94e78d7a41a9a8fe6d87a5e59500e51"
-	headerKey := http.CanonicalHeaderKey("Gitlab-Workhorse-Send-Data")
+	// 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)
 
-	ts := testhelper.TestServerWithHandler(regexp.MustCompile(`.`), func(w http.ResponseWriter, r *http.Request) {
-		responseJSON := fmt.Sprintf(`{"RepoPath":"%s","ShaFrom":"%s","ShaTo":"%s"}`, path.Join(testRepoRoot, testRepo), fromSha, toSha)
-		encodedJSON := base64.URLEncoding.EncodeToString([]byte(responseJSON))
-		w.Header().Set(headerKey, "git-format-patch:"+encodedJSON)
-		return
-	})
-	defer ts.Close()
-
-	ws := startWorkhorseServer(ts.URL)
-	defer ws.Close()
-
-	resourcePath := "/something"
-	resp, err := http.Get(ws.URL + resourcePath)
+	resp, body, err := doSendDataRequest("/something", "git-format-patch", jsonParams)
 	if err != nil {
 		t.Error(err)
 	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode != 200 {
-		t.Errorf("GET %q: expected 200, got %d", resourcePath, resp.StatusCode)
-	}
-	if len(resp.Header[headerKey]) != 0 {
-		t.Fatalf("Unexpected response header: %s: %q", headerKey, resp.Header.Get(headerKey))
-	}
 
-	body, err := ioutil.ReadAll(resp.Body)
-	if err != nil {
-		t.Fatal(err)
+	if resp.StatusCode != http.StatusOK {
+		t.Errorf("GET %q: expected HTTP 200, got %d", resp.Request.URL, resp.StatusCode)
 	}
 
-	if !strings.HasPrefix(string(body), "From 54fcc214b94e78d7a41a9a8fe6d87a5e59500e51 Mon Sep 17 00:00:00 2001") {
-		t.Fatalf("Expected: From 54fcc214b94e78d7a41a9a8fe6d87a5e59500e51 Mon Sep 17 00:00:00 2001, got: %v", body)
-	}
-
-	// The contents of the last line of the patch depend on the version of
-	// Git used during the test run. Ignore that data and look at the line
-	// that should terminate the diff.
-	lastPatchIndex := 442
-	endOfPatch := string(body[lastPatchIndex-5 : lastPatchIndex])
-	if endOfPatch != "\n-- \n" {
-		t.Fatalf("Expected end of patch at %d, found %q", lastPatchIndex, endOfPatch)
-	}
+	// Only the two commits on the fix branch should be included
+	testhelper.AssertPatchSeries(t, body, "12d65c8dd2b2676fa3ac47d955accc085a37a9c1", toSha)
 }
 
 func TestApiContentTypeBlock(t *testing.T) {