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

diff --git a/src/cmd/pack/doc.go b/src/cmd/pack/doc.go
index 1529e07e906b66d79fd982320f98b7633de190c9..a702594e23de5486813dd4ef1c0420a967f7e957 100644 (file)
--- 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 594433712d960d49af1f76ca5dbc7755dff90812..ffb2d617ae4172473d3fc640341c4c9ba8befb2a 100644 (file)
--- 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 e41cf3ce42d004d5c238cd923dbf13a296c7397c..cf6121fcc1149557200e7cb12d051de4082a05d0 100644 (file)
--- 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) {