Address PR feedback

archive/archive.go

@@ -114,7 +114,6 @@ func copyBuffer(dst Writer, src Reader) (err error) {
 
 		// Write the buffered file
 		if err := dst.NextFile(name, fi); err != nil {
-			fmt.Println("Failed to prep writer for next file")
 			return err
 		}
 		if _, err := io.Copy(dst, buf); err != nil {

archive/archive_test.go

@@ -2,6 +2,7 @@ package archive_test
 
 import (
 	"bytes"
+	"fmt"
 	"io"
 	"testing"
 
@@ -13,11 +14,13 @@ import (
 )
 
 const (
-	gzipType = iota
-	tarType
-	zipType
+	gzipType = "gzip"
+	tarType  = "tar"
+	zipType  = "zip"
 )
 
+var types = []string{gzipType, tarType, zipType}
+
 type file struct {
 	name     string
 	contents []byte
@@ -42,60 +45,37 @@ var (
 	}
 )
 
-var copyTests = map[string]struct {
-	srcType, dstType int
+type testCase struct {
+	srcType, dstType string
 	in               []file
-}{
-	"zip to zip": {
-		srcType: zipType,
-		dstType: zipType,
-		in:      twoFiles,
-	},
-	"zip to tar": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      twoFiles,
-	},
-	"zip to gzip": {
-		srcType: zipType,
-		dstType: gzipType,
-		in:      oneFile,
-	},
-	"tar to tar": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      twoFiles,
-	},
-	"tar to gzip": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      oneFile,
-	},
-	"tar to zip": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      twoFiles,
-	},
-	"gzip to gzip": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      oneFile,
-	},
-	"gzip to tar": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      oneFile,
-	},
-	"gzip to zip": {
-		srcType: zipType,
-		dstType: tarType,
-		in:      oneFile,
-	},
+}
+
+func copyTests() map[string]testCase {
+	// types X types X files
+	tests := make(map[string]testCase, len(types)*len(types)*2)
+	for _, srct := range types {
+		for _, dstt := range types {
+			tests[fmt.Sprintf("%s to %s: one file", srct, dstt)] = testCase{
+				srcType: srct,
+				dstType: dstt,
+				in:      oneFile,
+			}
+			// gzip does not handle more than one file
+			if srct != gzipType && dstt != gzipType {
+				tests[fmt.Sprintf("%s to %s: two files", srct, dstt)] = testCase{
+					srcType: srct,
+					dstType: dstt,
+					in:      twoFiles,
+				}
+			}
+		}
+	}
+	return tests
 }
 
 func TestCopy(t *testing.T) {
-	var srcb, dstb bytes.Buffer
-	for tname, tt := range copyTests {
+	srcb, dstb := new(bytes.Buffer), new(bytes.Buffer)
+	for tname, tt := range copyTests() {
 		// Reset buffers between tests
 		srcb.Reset()
 		dstb.Reset()
@@ -104,7 +84,7 @@ func TestCopy(t *testing.T) {
 		var fname string
 
 		// Seed the src
-		srcw := writer(t, tname, &srcb, tt.srcType)
+		srcw := writer(t, tname, srcb, tt.srcType)
 		for _, f := range tt.in {
 			srcw.NextFile(f.name, iotest.FileInfo(t, f.contents))
 			mustCopy(t, tname, srcw, bytes.NewReader(f.contents))
@@ -113,24 +93,27 @@ func TestCopy(t *testing.T) {
 		mustClose(t, tname, srcw)
 
 		// Perform the copy
-		srcr := reader(t, tname, &srcb, tt.srcType, fname)
-		dstw := writer(t, tname, &dstb, tt.dstType)
+		srcr := reader(t, tname, srcb, tt.srcType, fname)
+		dstw := writer(t, tname, dstb, tt.dstType)
 		if err := archive.Copy(dstw, srcr); err != nil {
 			t.Fatalf("%s: %v", tname, err)
 		}
 		srcr.Close() // Read-only
 		mustClose(t, tname, dstw)
 
 		// Read back dst to confirm our expectations
-		dstr := reader(t, tname, &dstb, tt.dstType, fname)
+		dstr := reader(t, tname, dstb, tt.dstType, fname)
 		for _, want := range tt.in {
-			var (
-				got file
-				buf bytes.Buffer
-			)
-			got.name, _ = dstr.NextFile()
-			mustCopy(t, tname, &buf, dstr)
-			got.contents = buf.Bytes()
+			buf := new(bytes.Buffer)
+			name, err := dstr.NextFile()
+			if err != nil {
+				t.Fatalf("%s: %v", tname, err)
+			}
+			mustCopy(t, tname, buf, dstr)
+			got := file{
+				name:     name,
+				contents: buf.Bytes(),
+			}
 
 			switch {
 			case got.name != want.name:
@@ -146,7 +129,7 @@ func TestCopy(t *testing.T) {
 	}
 }
 
-func writer(t *testing.T, tname string, w io.Writer, atype int) archive.WriteCloser {
+func writer(t *testing.T, tname string, w io.Writer, atype string) archive.WriteCloser {
 	switch atype {
 	case gzipType:
 		return gzip.NewWriter(w)
@@ -155,11 +138,11 @@ func writer(t *testing.T, tname string, w io.Writer, atype int) archive.WriteClo
 	case zipType:
 		return zip.NewWriter(w)
 	}
-	t.Fatalf("%s: unrecognized archive type: %d", tname, atype)
+	t.Fatalf("%s: unrecognized archive type: %s", tname, atype)
 	panic("execution continued after (*testing.T).Fatalf")
 }
 
-func reader(t *testing.T, tname string, buf *bytes.Buffer, atype int, fname string) archive.ReadCloser {
+func reader(t *testing.T, tname string, buf *bytes.Buffer, atype string, fname string) archive.ReadCloser {
 	switch atype {
 	case gzipType:
 		gr, err := gzip.NewReader(buf, fname)
@@ -178,7 +161,7 @@ func reader(t *testing.T, tname string, buf *bytes.Buffer, atype int, fname stri
 		}
 		return archive.NopCloser(zr)
 	}
-	t.Fatalf("%s: unrecognized archive type: %d", tname, atype)
+	t.Fatalf("%s: unrecognized archive type: %s", tname, atype)
 	panic("execution continued after (*testing.T).Fatalf")
 }
 

writers_rollingfilewriter.go

@@ -29,7 +29,6 @@ import (
 	"io"
 	"io/ioutil"
 	"os"
-	"path"
 	"path/filepath"
 	"sort"
 	"strconv"
@@ -361,7 +360,8 @@ func (rw *rollingFileWriter) archiveExplodedLogs(logFilename string, compression
 	defer src.Close() // Read-only
 
 	// Buffer to a temporary file on the same partition
-	dst, err := rw.tempArchiveFile()
+	// Note: archivePath is a path to a directory when handling exploded logs
+	dst, err := rw.tempArchiveFile(rw.archivePath)
 	if err != nil {
 		return err
 	}
@@ -373,8 +373,8 @@ func (rw *rollingFileWriter) archiveExplodedLogs(logFilename string, compression
 		}
 
 		// Finalize archive by swapping the buffered archive into place
-		archiveFile := path.Clean(rw.archivePath + "/" + compressionType.rollingArchiveTypeName(logFilename, true))
-		err = os.Rename(dst.Name(), archiveFile)
+		err = os.Rename(dst.Name(), filepath.Join(rw.archivePath,
+			compressionType.rollingArchiveTypeName(logFilename, true)))
 	}()
 
 	// archive entry
@@ -399,7 +399,8 @@ func (rw *rollingFileWriter) archiveUnexplodedLogs(compressionType compressionTy
 	}
 
 	// Buffer to a temporary file on the same partition
-	dst, err := rw.tempArchiveFile()
+	// Note: archivePath is a path to a file when handling unexploded logs
+	dst, err := rw.tempArchiveFile(filepath.Dir(rw.archivePath))
 	if err != nil {
 		return err
 	}
@@ -479,7 +480,7 @@ func (rw *rollingFileWriter) deleteOldRolls(history []string) error {
 				rw.archiveExplodedLogs(history[i], compressionTypes[rw.archiveType])
 			}
 		} else {
-			os.MkdirAll(path.Dir(rw.archivePath), defaultDirectoryPermissions)
+			os.MkdirAll(filepath.Dir(rw.archivePath), defaultDirectoryPermissions)
 
 			rw.archiveUnexplodedLogs(compressionTypes[rw.archiveType], rollsToDelete, history)
 		}
@@ -612,8 +613,8 @@ func (rw *rollingFileWriter) Close() error {
 	return nil
 }
 
-func (rw *rollingFileWriter) tempArchiveFile() (*os.File, error) {
-	tmp := path.Join(path.Dir(rw.archivePath), ".seelog_tmp")
+func (rw *rollingFileWriter) tempArchiveFile(archiveDir string) (*os.File, error) {
+	tmp := filepath.Join(archiveDir, ".seelog_tmp")
 	if err := os.MkdirAll(tmp, defaultDirectoryPermissions); err != nil {
 		return nil, err
 	}