Upadte archive.ReplaceFileTarWrapper() to not expect a sorted archive

Improve test coverage of ReplaceFileTarWrapper()

Signed-off-by: Daniel Nephin <dnephin@docker.com>

cli/command/image/build.go

@@ -218,13 +218,14 @@ func runBuild(dockerCli *command.DockerCli, options buildOptions) error {
 			return errors.Errorf("Error checking context: '%s'.", err)
 		}
 
-		// If .dockerignore mentions .dockerignore or the Dockerfile
-		// then make sure we send both files over to the daemon
-		// because Dockerfile is, obviously, needed no matter what, and
-		// .dockerignore is needed to know if either one needs to be
-		// removed. The daemon will remove them for us, if needed, after it
-		// parses the Dockerfile. Ignore errors here, as they will have been
-		// caught by validateContextDirectory above.
+		// If .dockerignore mentions .dockerignore or the Dockerfile then make
+		// sure we send both files over to the daemon because Dockerfile is,
+		// obviously, needed no matter what, and .dockerignore is needed to know
+		// if either one needs to be removed. The daemon will remove them
+		// if necessary, after it parses the Dockerfile. Ignore errors here, as
+		// they will have been caught by validateContextDirectory above.
+		// Excludes are used instead of includes to maintain the order of files
+		// in the archive.
 		if keep, _ := fileutils.Matches(".dockerignore", excludes); keep {
 			excludes = append(excludes, "!.dockerignore")
 		}
@@ -384,17 +385,16 @@ func addDockerfileToBuildContext(dockerfileCtx io.ReadCloser, buildCtx io.ReadCl
 			if h == nil {
 				h = hdrTmpl
 			}
-			extraIgnore := randomName + "\n"
+
 			b := &bytes.Buffer{}
 			if content != nil {
-				_, err := b.ReadFrom(content)
-				if err != nil {
+				if _, err := b.ReadFrom(content); err != nil {
 					return nil, nil, err
 				}
 			} else {
-				extraIgnore += ".dockerignore\n"
+				b.WriteString(".dockerignore")
 			}
-			b.Write([]byte("\n" + extraIgnore))
+			b.WriteString("\n" + randomName + "\n")
 			return h, b.Bytes(), nil
 		},
 	})

integration-cli/docker_cli_build_test.go

@@ -2069,34 +2069,40 @@ func (s *DockerSuite) testBuildDockerfileStdinNoExtraFiles(c *check.C, hasDocker
 	name := "stdindockerfilenoextra"
 	tmpDir, err := ioutil.TempDir("", "fake-context")
 	c.Assert(err, check.IsNil)
-	err = ioutil.WriteFile(filepath.Join(tmpDir, "foo"), []byte("bar"), 0600)
-	c.Assert(err, check.IsNil)
-	if hasDockerignore {
-		// test that this file is removed
-		err = ioutil.WriteFile(filepath.Join(tmpDir, "Dockerfile"), []byte(""), 0600)
+	defer os.RemoveAll(tmpDir)
+
+	writeFile := func(filename, content string) {
+		err = ioutil.WriteFile(filepath.Join(tmpDir, filename), []byte(content), 0600)
 		c.Assert(err, check.IsNil)
+	}
+
+	writeFile("foo", "bar")
+
+	if hasDockerignore {
+		// Add an empty Dockerfile to verify that it is not added to the image
+		writeFile("Dockerfile", "")
+
 		ignores := "Dockerfile\n"
 		if ignoreDockerignore {
 			ignores += ".dockerignore\n"
 		}
-		err = ioutil.WriteFile(filepath.Join(tmpDir, ".dockerignore"), []byte(ignores), 0600)
-		c.Assert(err, check.IsNil)
+		writeFile(".dockerignore", ignores)
 	}
 
-	icmd.RunCmd(icmd.Cmd{
+	result := icmd.RunCmd(icmd.Cmd{
 		Command: []string{dockerBinary, "build", "-t", name, "-f", "-", tmpDir},
 		Stdin: strings.NewReader(
 			`FROM busybox
 COPY . /baz`),
-	}).Assert(c, icmd.Success)
+	})
+	result.Assert(c, icmd.Success)
 
-	out, _ := dockerCmd(c, "run", "--rm", name, "ls", "-A", "/baz")
+	result = cli.DockerCmd(c, "run", "--rm", name, "ls", "-A", "/baz")
 	if hasDockerignore && !ignoreDockerignore {
-		c.Assert(strings.TrimSpace(string(out)), checker.Equals, ".dockerignore\nfoo")
+		c.Assert(result.Stdout(), checker.Equals, ".dockerignore\nfoo\n")
 	} else {
-		c.Assert(strings.TrimSpace(string(out)), checker.Equals, "foo")
+		c.Assert(result.Stdout(), checker.Equals, "foo\n")
 	}
-
 }
 
 func (s *DockerSuite) TestBuildWithVolumeOwnership(c *check.C) {

pkg/archive/archive.go

@@ -14,7 +14,6 @@ import (
 	"os/exec"
 	"path/filepath"
 	"runtime"
-	"sort"
 	"strings"
 	"syscall"
 
@@ -227,7 +226,10 @@ func CompressStream(dest io.Writer, compression Compression) (io.WriteCloser, er
 }
 
 // TarModifierFunc is a function that can be passed to ReplaceFileTarWrapper to
-// define a modification step for a single path
+// modify the contents or header of an entry in the archive. If the file already
+// exists in the archive the TarModifierFunc will be called with the Header and
+// a reader which will return the files content. If the file does not exist both
+// header and content will be nil.
 type TarModifierFunc func(path string, header *tar.Header, content io.Reader) (*tar.Header, []byte, error)
 
 // ReplaceFileTarWrapper converts inputTarStream to a new tar stream. Files in the
@@ -235,76 +237,77 @@ type TarModifierFunc func(path string, header *tar.Header, content io.Reader) (*
 func ReplaceFileTarWrapper(inputTarStream io.ReadCloser, mods map[string]TarModifierFunc) io.ReadCloser {
 	pipeReader, pipeWriter := io.Pipe()
 
-	modKeys := make([]string, 0, len(mods))
-	for key := range mods {
-		modKeys = append(modKeys, key)
-	}
-	sort.Strings(modKeys)
-
 	go func() {
 		tarReader := tar.NewReader(inputTarStream)
 		tarWriter := tar.NewWriter(pipeWriter)
-
 		defer inputTarStream.Close()
+		defer tarWriter.Close()
 
-	loop0:
+		modify := func(name string, original *tar.Header, modifier TarModifierFunc, tarReader io.Reader) error {
+			header, data, err := modifier(name, original, tarReader)
+			switch {
+			case err != nil:
+				return err
+			case header == nil:
+				return nil
+			}
+
+			header.Name = name
+			header.Size = int64(len(data))
+			if err := tarWriter.WriteHeader(header); err != nil {
+				return err
+			}
+			if len(data) != 0 {
+				if _, err := tarWriter.Write(data); err != nil {
+					return err
+				}
+			}
+			return nil
+		}
+
+		var err error
+		var originalHeader *tar.Header
 		for {
-			hdr, err := tarReader.Next()
-			for len(modKeys) > 0 && (err == io.EOF || err == nil && hdr.Name >= modKeys[0]) {
-				var h *tar.Header
-				var rdr io.Reader
-				if err == nil && hdr != nil && hdr.Name == modKeys[0] {
-					h = hdr
-					rdr = tarReader
-				}
-
-				h2, dt, err := mods[modKeys[0]](modKeys[0], h, rdr)
-				if err != nil {
-					pipeWriter.CloseWithError(err)
-					return
-				}
-				if h2 != nil {
-					h2.Name = modKeys[0]
-					h2.Size = int64(len(dt))
-					if err := tarWriter.WriteHeader(h2); err != nil {
-						pipeWriter.CloseWithError(err)
-						return
-					}
-					if len(dt) != 0 {
-						if _, err := tarWriter.Write(dt); err != nil {
-							pipeWriter.CloseWithError(err)
-							return
-						}
-					}
-				}
-				modKeys = modKeys[1:]
-				if h != nil {
-					continue loop0
-				}
-			}
-
+			originalHeader, err = tarReader.Next()
 			if err == io.EOF {
-				tarWriter.Close()
-				pipeWriter.Close()
-				return
+				break
 			}
-
 			if err != nil {
 				pipeWriter.CloseWithError(err)
 				return
 			}
 
-			if err := tarWriter.WriteHeader(hdr); err != nil {
+			modifier, ok := mods[originalHeader.Name]
+			if !ok {
+				// No modifiers for this file, copy the header and data
+				if err := tarWriter.WriteHeader(originalHeader); err != nil {
+					pipeWriter.CloseWithError(err)
+					return
+				}
+				if _, err := pools.Copy(tarWriter, tarReader); err != nil {
+					pipeWriter.CloseWithError(err)
+					return
+				}
+				continue
+			}
+			delete(mods, originalHeader.Name)
+
+			if err := modify(originalHeader.Name, originalHeader, modifier, tarReader); err != nil {
 				pipeWriter.CloseWithError(err)
 				return
 			}
-
-			if _, err := pools.Copy(tarWriter, tarReader); err != nil {
-				pipeWriter.CloseWithError(err)
-				return
-			}
-
 		}
+
+		// Apply the modifiers that haven't matched any files in the archive
+		for name, modifier := range mods {
+			if err := modify(name, nil, modifier, nil); err != nil {
+				pipeWriter.CloseWithError(err)
+				return
+			}
+		}
+
+		pipeWriter.Close()
+
 	}()
 	return pipeReader
 }

pkg/archive/archive_test.go

@@ -4,6 +4,7 @@ import (
 	"archive/tar"
 	"bytes"
 	"fmt"
+	"github.com/docker/docker/pkg/testutil/assert"
 	"io"
 	"io/ioutil"
 	"os"
@@ -1161,58 +1162,110 @@ func TestTempArchiveCloseMultipleTimes(t *testing.T) {
 	}
 }
 
-func testReplaceFileTarWrapper(t *testing.T, name string) {
-	srcDir, err := ioutil.TempDir("", "docker-test-srcDir")
-	if err != nil {
-		t.Fatal(err)
+func TestReplaceFileTarWrapper(t *testing.T) {
+	filesInArchive := 20
+	testcases := []struct {
+		doc       string
+		filename  string
+		modifier  TarModifierFunc
+		expected  string
+		fileCount int
+	}{
+		{
+			doc:       "Modifier creates a new file",
+			filename:  "newfile",
+			modifier:  createModifier(t),
+			expected:  "the new content",
+			fileCount: filesInArchive + 1,
+		},
+		{
+			doc:       "Modifier replaces a file",
+			filename:  "file-2",
+			modifier:  createOrReplaceModifier,
+			expected:  "the new content",
+			fileCount: filesInArchive,
+		},
+		{
+			doc:       "Modifier replaces the last file",
+			filename:  fmt.Sprintf("file-%d", filesInArchive-1),
+			modifier:  createOrReplaceModifier,
+			expected:  "the new content",
+			fileCount: filesInArchive,
+		},
+		{
+			doc:       "Modifier appends to a file",
+			filename:  "file-3",
+			modifier:  appendModifier,
+			expected:  "fooo\nnext line",
+			fileCount: filesInArchive,
+		},
 	}
-	defer os.RemoveAll(srcDir)
 
-	destDir, err := ioutil.TempDir("", "docker-test-destDir")
-	if err != nil {
-		t.Fatal(err)
+	for _, testcase := range testcases {
+		sourceArchive, cleanup := buildSourceArchive(t, filesInArchive)
+		defer cleanup()
+
+		resultArchive := ReplaceFileTarWrapper(
+			sourceArchive,
+			map[string]TarModifierFunc{testcase.filename: testcase.modifier})
+
+		actual := readFileFromArchive(t, resultArchive, testcase.filename, testcase.fileCount, testcase.doc)
+		assert.Equal(t, actual, testcase.expected, testcase.doc)
 	}
+}
+
+func buildSourceArchive(t *testing.T, numberOfFiles int) (io.ReadCloser, func()) {
+	srcDir, err := ioutil.TempDir("", "docker-test-srcDir")
+	assert.NilError(t, err)
+
+	_, err = prepareUntarSourceDirectory(numberOfFiles, srcDir, false)
+	assert.NilError(t, err)
+
+	sourceArchive, err := TarWithOptions(srcDir, &TarOptions{})
+	assert.NilError(t, err)
+	return sourceArchive, func() {
+		os.RemoveAll(srcDir)
+		sourceArchive.Close()
+	}
+}
+
+func createOrReplaceModifier(path string, header *tar.Header, content io.Reader) (*tar.Header, []byte, error) {
+	return &tar.Header{
+		Mode:     0600,
+		Typeflag: tar.TypeReg,
+	}, []byte("the new content"), nil
+}
+
+func createModifier(t *testing.T) TarModifierFunc {
+	return func(path string, header *tar.Header, content io.Reader) (*tar.Header, []byte, error) {
+		assert.Nil(t, content)
+		return createOrReplaceModifier(path, header, content)
+	}
+}
+
+func appendModifier(path string, header *tar.Header, content io.Reader) (*tar.Header, []byte, error) {
+	buffer := bytes.Buffer{}
+	if content != nil {
+		if _, err := buffer.ReadFrom(content); err != nil {
+			return nil, nil, err
+		}
+	}
+	buffer.WriteString("\nnext line")
+	return &tar.Header{Mode: 0600, Typeflag: tar.TypeReg}, buffer.Bytes(), nil
+}
+
+func readFileFromArchive(t *testing.T, archive io.ReadCloser, name string, expectedCount int, doc string) string {
+	destDir, err := ioutil.TempDir("", "docker-test-destDir")
+	assert.NilError(t, err)
 	defer os.RemoveAll(destDir)
 
-	_, err = prepareUntarSourceDirectory(20, srcDir, false)
-	if err != nil {
-		t.Fatal(err)
-	}
+	err = Untar(archive, destDir, nil)
+	assert.NilError(t, err)
 
-	archive, err := TarWithOptions(srcDir, &TarOptions{})
-	if err != nil {
-		t.Fatal(err)
-	}
-	defer archive.Close()
+	files, _ := ioutil.ReadDir(destDir)
+	assert.Equal(t, len(files), expectedCount, doc)
 
-	archive2 := ReplaceFileTarWrapper(archive, map[string]TarModifierFunc{name: func(path string, header *tar.Header, content io.Reader) (*tar.Header, []byte, error) {
-		return &tar.Header{
-			Mode:     0600,
-			Typeflag: tar.TypeReg,
-		}, []byte("foobar"), nil
-	}})
-
-	if err := Untar(archive2, destDir, nil); err != nil {
-		t.Fatal(err)
-	}
-
-	dt, err := ioutil.ReadFile(filepath.Join(destDir, name))
-	if err != nil {
-		t.Fatal(err)
-	}
-	if expected, actual := "foobar", string(dt); actual != expected {
-		t.Fatalf("file contents mismatch, expected: %q, got %q", expected, actual)
-	}
-}
-
-func TestReplaceFileTarWrapperNewFile(t *testing.T) {
-	testReplaceFileTarWrapper(t, "abc")
-}
-
-func TestReplaceFileTarWrapperReplaceFile(t *testing.T) {
-	testReplaceFileTarWrapper(t, "file-2")
-}
-
-func TestReplaceFileTarWrapperLastFile(t *testing.T) {
-	testReplaceFileTarWrapper(t, "file-999")
+	content, err := ioutil.ReadFile(filepath.Join(destDir, name))
+	assert.NilError(t, err)
+	return string(content)
 }

pkg/testutil/assert/assert.go

@@ -20,9 +20,9 @@ type TestingT interface {
 
 // Equal compare the actual value to the expected value and fails the test if
 // they are not equal.
-func Equal(t TestingT, actual, expected interface{}) {
+func Equal(t TestingT, actual, expected interface{}, extra ...string) {
 	if expected != actual {
-		fatal(t, "Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)
+		fatalWithExtra(t, extra, "Expected '%v' (%T) got '%v' (%T)", expected, expected, actual, actual)
 	}
 }
 
@@ -103,10 +103,25 @@ func NotNil(t TestingT, obj interface{}) {
 	}
 }
 
+// Nil fails the test if the object is not nil
+func Nil(t TestingT, obj interface{}) {
+	if obj != nil {
+		fatal(t, "Expected nil value, got (%T) %s", obj, obj)
+	}
+}
+
 func fatal(t TestingT, format string, args ...interface{}) {
 	t.Fatalf(errorSource()+format, args...)
 }
 
+func fatalWithExtra(t TestingT, extra []string, format string, args ...interface{}) {
+	msg := fmt.Sprintf(errorSource()+format, args...)
+	if len(extra) > 0 {
+		msg += ": " + strings.Join(extra, ", ")
+	}
+	t.Fatalf(msg)
+}
+
 // See testing.decorate()
 func errorSource() string {
 	_, filename, line, ok := runtime.Caller(3)