cmd/pack: fix c command for existing file

There were at least two bugs:
1) It would overwrite a non-archive.
2) It would truncate a non-archive and then fail.
In general the file handling was too clever to be correct.
Make it more straightforward, doing the creation
separately from archive management.

Fixes #8369.

LGTM=adg, iant
R=golang-codereviews, adg, iant
CC=golang-codereviews
https://golang.org/cl/147010046

3 files changed, 57 insertions, 20 deletions

diff --git a/src/cmd/pack/doc.go b/src/cmd/pack/doc.go
index 1529e07e90..a702594e23 100644
--- a/src/cmd/pack/doc.go
+++ b/src/cmd/pack/doc.go
@@ -20,6 +20,10 @@ The operation op is given by one of these letters:
 	t	list files from the archive
 	x	extract files from the archive
 
+The archive argument to the c command must be non-existent or a
+valid archive file, which will be cleared before adding new entries. It
+is an error if the file exists but is not an archive.
+
 For the p, t, and x commands, listing no names on the command line
 causes the operation to apply to all files in the archive.
 
diff --git a/src/cmd/pack/pack.go b/src/cmd/pack/pack.go
index 594433712d..ffb2d617ae 100644
--- a/src/cmd/pack/pack.go
+++ b/src/cmd/pack/pack.go
@@ -142,16 +142,19 @@ type Archive struct {
 	matchAll bool     // match all files in archive
 }
 
-// archive opens (or if necessary creates) the named archive.
+// archive opens (and if necessary creates) the named archive.
 func archive(name string, mode int, files []string) *Archive {
-	fd, err := os.OpenFile(name, mode, 0)
-	if err != nil && mode&^os.O_TRUNC == os.O_RDWR && os.IsNotExist(err) {
-		fd, err = create(name)
+	// If the file exists, it must be an archive. If it doesn't exist, or if
+	// we're doing the c command, indicated by O_TRUNC, truncate the archive.
+	if !existingArchive(name) || mode&os.O_TRUNC != 0 {
+		create(name)
+		mode &^= os.O_TRUNC
 	}
+	fd, err := os.OpenFile(name, mode, 0)
 	if err != nil {
 		log.Fatal(err)
 	}
-	mustBeArchive(fd)
+	checkHeader(fd)
 	return &Archive{
 		fd:       fd,
 		files:    files,
@@ -160,23 +163,40 @@ func archive(name string, mode int, files []string) *Archive {
 }
 
 // create creates and initializes an archive that does not exist.
-func create(name string) (*os.File, error) {
-	fd, err := os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_TRUNC, 0644)
+func create(name string) {
+	fd, err := os.Create(name)
 	if err != nil {
-		return nil, err
+		log.Fatal(err)
+	}
+	_, err = fmt.Fprint(fd, arHeader)
+	if err != nil {
+		log.Fatal(err)
+	}
+	fd.Close()
+}
+
+// existingArchive reports whether the file exists and is a valid archive.
+// If it exists but is not an archive, existingArchive will exit.
+func existingArchive(name string) bool {
+	fd, err := os.Open(name)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false
+		}
+		log.Fatal("cannot open file: %s", err)
 	}
-	fmt.Fprint(fd, arHeader)
-	fd.Seek(0, 0)
-	return fd, nil
+	checkHeader(fd)
+	fd.Close()
+	return true
 }
 
-// mustBeArchive verifies the header of the file. It assumes the file offset
-// is 0 coming in, and leaves it positioned immediately after the header.
-func mustBeArchive(fd *os.File) {
+// checkHeader verifies the header of the file. It assumes the file
+// is positioned at 0 and leaves it positioned at the end of the header.
+func checkHeader(fd *os.File) {
 	buf := make([]byte, len(arHeader))
 	_, err := io.ReadFull(fd, buf)
 	if err != nil || string(buf) != arHeader {
-		log.Fatal("file is not an archive: bad header")
+		log.Fatal("%s is not an archive: bad header", fd.Name())
 	}
 }
 
diff --git a/src/cmd/pack/pack_test.go b/src/cmd/pack/pack_test.go
index e41cf3ce42..cf6121fcc1 100644
--- a/src/cmd/pack/pack_test.go
+++ b/src/cmd/pack/pack_test.go
@@ -56,11 +56,8 @@ func tmpDir(t *testing.T) string {
 	return name
 }
 
-// Test that we can create an archive, write to it, and get the same contents back.
-// Tests the rv and then the pv command on a new archive.
-func TestCreate(t *testing.T) {
-	dir := tmpDir(t)
-	defer os.RemoveAll(dir)
+// testCreate creates an archive in the specified directory.
+func testCreate(t *testing.T, dir string) {
 	name := filepath.Join(dir, "pack.a")
 	ar := archive(name, os.O_RDWR, nil)
 	// Add an entry by hand.
@@ -85,6 +82,22 @@ func TestCreate(t *testing.T) {
 	}
 }
 
+// Test that we can create an archive, write to it, and get the same contents back.
+// Tests the rv and then the pv command on a new archive.
+func TestCreate(t *testing.T) {
+	dir := tmpDir(t)
+	defer os.RemoveAll(dir)
+	testCreate(t, dir)
+}
+
+// Test that we can create an archive twice with the same name (Issue 8369).
+func TestCreateTwice(t *testing.T) {
+	dir := tmpDir(t)
+	defer os.RemoveAll(dir)
+	testCreate(t, dir)
+	testCreate(t, dir)
+}
+
 // Test that we can create an archive, put some files in it, and get back a correct listing.
 // Tests the tv command.
 func TestTableOfContents(t *testing.T) {