Merge lp:~niemeyer/pyjuju/go-charm-bits into lp:pyjuju/go

[Gustavo Niemeyer]

Improvements and fixes in charm directory packing

- Ignore hidden files as the Python version.
- Use the new filepath.Walk interface.
- Bundle dirs as well, so that empty dirs are included.
- Pack unix file mode into the charm bundle.

The last change requires this Go CL to be merged,
so the branch can't be merged before it goes in:

 http://codereview.appspot.com/5124044/

[lp:~niemeyer/pyjuju/go-charm-bits]

13. By Gustavo Niemeyer on 2011-09-25

Bundle directories into the zip as well, so that empty directories are
handled properly.

[Gustavo Niemeyer (niemeyer)]

The upstream CL went in already, so one will just need a tip branch to
test this for the moment.

[William Reade (fwereade)]

[0]

+// join builds a path rooted at the charm's expended directory

s/expended/expanded

Otherwise all looks very clean, +1

review: Approve

[Roger Peppe (rogpeppe)]

general +1

[1]
charm/bundle.go
 for _, header := range zipr.File {

Seems to me like the body of this loop would be better
in a separate function. Then you could just defer
zf.Close and things would be generally cleaner.

[2]
charm/dir.go
 parts = append([]string{dir.Path}, parts...)

appending to parts directly is bad behaviour because
it could interfere with any callers that call join
with a slice. i know you don't do that, but it sets
a dubious example.

[3]
charm/dir.go
 hidden := len(relpath) > 1 && relpath[0] == '.'

I hope this behaviour is well documented. I've provided
config files as part of a charm before, and it's only
luck that they didn't start with a ".".
If the object is to exclude source-code control files
(e.g. .hg and .bzr) perhaps it would be better to
name the exclusions specifically, rather than excluding
all files starting with ".".

[4]
charm/reltest.go
  got, err := filepath_Rel(test.root, test.path)

i thought Rel had now been accepted.

[Roger Peppe (rogpeppe)]

review: Needs Fixing

[Roger Peppe (rogpeppe)]

review: Approve

[Gustavo Niemeyer (niemeyer)]

[1]

That's done. Thanks.

[2]

As we talked online, that's not the case.

[3]

It's not well documented to be honest, but it's easy to go from
more restrictive to more liberal than otherwise. In general depending
on . files at runtime is a bad idea, though, so let's way for someone
to complain so that we can see a real use case.

[4]

It was, but this work was were filepath.Rel was born, so it wasn't
around by the time this branch was pushed. I'll remove the internal
copy now.

Thanks for the review.

[Gustavo Niemeyer (niemeyer)]

William, that's done as well, thanks.

=== modified file 'charm/Makefile'
--- charm/Makefile	2011-09-24 15:21:23 +0000
+++ charm/Makefile	2011-09-25 21:15:26 +0000
@@ -10,7 +10,6 @@
 	dir.go\
 	charm.go\
 	meta.go\
-	walk.go\
 	rel.go\
 
 GOFMT=gofmt

=== modified file 'charm/bundle.go'
--- charm/bundle.go	2011-09-24 15:21:23 +0000
+++ charm/bundle.go	2011-09-25 21:15:26 +0000
@@ -5,6 +5,7 @@
 	"io"
 	"os"
 	"path/filepath"
+	"strings"
 )
 
 // ReadBundle returns a Bundle for the charm in path.
@@ -129,6 +130,14 @@
 			continue
 		}
 		path := filepath.Join(dir, filepath.Clean(header.Name))
+		if strings.HasSuffix(header.Name, "/") {
+			err = os.MkdirAll(path, 0755)
+			if err != nil {
+				zf.Close()
+				lasterr = err
+			}
+			continue
+		}
 		base, _ := filepath.Split(path)
 		err = os.MkdirAll(base, 0755)
 		if err != nil {

=== modified file 'charm/dir.go'
--- charm/dir.go	2011-09-24 15:21:23 +0000
+++ charm/dir.go	2011-09-25 21:15:26 +0000
@@ -43,6 +43,13 @@
 // Trick to ensure *Dir implements the Charm interface.
 var _ Charm = (*Dir)(nil)
 
+// join builds a path rooted at the charm's expended directory
+// path and the extra path components provided.
+func (dir *Dir) join(parts ...string) string {
+	parts = append([]string{dir.Path}, parts...)
+	return filepath.Join(parts...)
+}
+
 // Meta returns the Meta representing the metadata.yaml file
 // for the charm expanded in dir.
 func (dir *Dir) Meta() *Meta {
@@ -58,63 +65,56 @@
 // BundleTo creates a charm file from the charm expanded in dir.
 func (dir *Dir) BundleTo(w io.Writer) (err os.Error) {
 	zipw := zip.NewWriter(w)
-	defer func() {
-		zipw.Close()
-		handleZipError(&err)
-	}()
-	visitor := zipVisitor{zipw, dir.Path}
-	walk(dir.Path, &visitor)
-	return nil
+	defer zipw.Close()
+	zp := zipPacker{zipw, dir.Path}
+	return filepath.Walk(dir.Path, zp.WalkFunc())
 }
 
-type zipVisitor struct {
+type zipPacker struct {
 	*zip.Writer
 	root string
 }
 
-func (zipw *zipVisitor) VisitDir(path string, f *os.FileInfo) bool {
-	relpath, err := filepath_Rel(zipw.root, path)
-	zipw.Error(path, err)
-	return relpath != "build"
+func (zp *zipPacker) WalkFunc() filepath.WalkFunc {
+	return func(path string, fi *os.FileInfo, err os.Error) os.Error {
+		return zp.Visit(path, fi, err)
+	}
 }
 
-func (zipw *zipVisitor) VisitFile(path string, f *os.FileInfo) {
-	relpath, err := filepath_Rel(zipw.root, path)
-	zipw.Error(path, err)
-	w, err := zipw.Create(relpath)
-	zipw.Error(path, err)
+func (zp *zipPacker) Visit(path string, fi *os.FileInfo, err os.Error) os.Error {
+	if err != nil {
+		return err
+	}
+	relpath, err := filepath_Rel(zp.root, path)
+	if err != nil {
+		return err
+	}
+	hidden := len(relpath) > 1 && relpath[0] == '.'
+	if fi.IsDirectory() {
+		if relpath == "build" {
+			return filepath.SkipDir
+		}
+		if hidden {
+			return filepath.SkipDir
+		}
+		relpath += "/"
+	}
+	if hidden {
+		return nil
+	}
+	h := &zip.FileHeader{
+		Name: relpath,
+		Method: zip.Deflate,
+	}
+	h.SetMode(fi.Mode)
+	w, err := zp.CreateHeader(h)
+	if err != nil || fi.IsDirectory() {
+		return err
+	}
 	data, err := ioutil.ReadFile(path)
-	zipw.Error(path, err)
+	if err != nil {
+		return err
+	}
 	_, err = w.Write(data)
-	zipw.Error(path, err)
-}
-
-type zipError os.Error
-
-func (zipw *zipVisitor) Error(path string, err os.Error) {
-	if err != nil {
-		panic(zipError(err))
-	}
-}
-
-func handleZipError(err *os.Error) {
-	if *err != nil {
-		return // Do not override a previous problem
-	}
-	panicv := recover()
-	if panicv == nil {
-		return
-	}
-	if e, ok := panicv.(zipError); ok {
-		*err = (os.Error)(e)
-		return
-	}
-	panic(panicv) // Something else
-}
-
-// join builds a path rooted at the charm's expended directory
-// path and the extra path components provided.
-func (dir *Dir) join(parts ...string) string {
-	parts = append([]string{dir.Path}, parts...)
-	return filepath.Join(parts...)
+	return err
 }

=== modified file 'charm/dir_test.go'
--- charm/dir_test.go	2011-09-24 15:21:23 +0000
+++ charm/dir_test.go	2011-09-25 21:15:26 +0000
@@ -6,6 +6,7 @@
 	"launchpad.net/juju/go/charm"
 	"os"
 	"path/filepath"
+	"syscall"
 )
 
 func repoDir(name string) (path string) {
@@ -34,14 +35,20 @@
 	c.Assert(err, IsNil)
 	defer zipr.Close()
 
-	var metaf *zip.File
+	var metaf, instf, emptyf *zip.File
 	for _, f := range zipr.File {
 		c.Logf("Bundled file: %s", f.Name)
 		switch f.Name {
 		case "metadata.yaml":
 			metaf = f
+		case "hooks/install":
+			instf = f
+		case "empty/":
+			emptyf = f
 		case "build/ignored":
-			c.Fatal("bundle includes build/*")
+			c.Errorf("bundle includes build/*: %s", f.Name)
+		case ".ignored", ".dir/ignored":
+			c.Errorf("bundle includes .* entries: %s", f.Name)
 		}
 	}
 
@@ -52,4 +59,14 @@
 	meta, err := charm.ReadMeta(reader)
 	c.Assert(err, IsNil)
 	c.Assert(meta.Name, Equals, "dummy")
+
+	c.Assert(instf, NotNil)
+	mode, err := instf.Mode()
+	c.Assert(err, IsNil)
+	c.Assert(mode & 0777, Equals, uint32(0755))
+
+	c.Assert(emptyf, NotNil)
+	mode, err = emptyf.Mode()
+	c.Assert(err, IsNil)
+	c.Assert(mode & syscall.S_IFMT, Equals, uint32(syscall.S_IFDIR))
 }

=== modified file 'charm/rel_test.go'
--- charm/rel_test.go	2011-09-24 15:21:23 +0000
+++ charm/rel_test.go	2011-09-25 21:15:26 +0000
@@ -1,7 +1,6 @@
 package charm
 
 import (
-	"path/filepath"
 	"runtime"
 	"testing"
 )
@@ -64,7 +63,7 @@
 		tests = append(tests, winreltests...)
 	}
 	for _, test := range tests {
-		got, err := filepath.Rel(test.root, test.path)
+		got, err := filepath_Rel(test.root, test.path)
 		if test.want == "err" {
 			if err == nil {
 				t.Errorf("Rel(%q, %q)=%q, want error", test.root, test.path, got)

=== added directory 'charm/testrepo/dummy/.dir'
=== added file 'charm/testrepo/dummy/.dir/ignored'
=== added directory 'charm/testrepo/dummy/hooks'
=== added file 'charm/testrepo/dummy/hooks/install'
--- charm/testrepo/dummy/hooks/install	1970-01-01 00:00:00 +0000
+++ charm/testrepo/dummy/hooks/install	2011-09-25 21:15:26 +0000
@@ -0,0 +1,2 @@
+#!/bin/bash
+echo "Done!"

=== removed file 'charm/walk.go'
--- charm/walk.go	2011-09-24 15:21:23 +0000
+++ charm/walk.go	1970-01-01 00:00:00 +0000
@@ -1,78 +0,0 @@
-// Copyright 2009 The Go Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-package charm
-
-import (
-	"os"
-	"path/filepath"
-	"sort"
-)
-
-// Drop in replacement for filepath.Walk while issue 2237 is not solved:
-//
-//    http://code.google.com/p/go/issues/detail?id=2237
-
-type walkErrorHandler interface {
-	Error(path string, err os.Error)
-}
-
-// walk walks the file tree rooted at root, calling v.VisitDir or
-// v.VisitFile for each directory or file in the tree, including root.
-// If v.VisitDir returns false, Walk skips the directory's entries;
-// otherwise it invokes itself for each directory entry in sorted order.
-// If the visitor implements the walkErrorHandler interface, any errors
-// found will be dispatched to it.
-func walk(root string, v filepath.Visitor) {
-	f, err := os.Lstat(root)
-	if err != nil {
-		if eh, ok := v.(walkErrorHandler); ok {
-			eh.Error(root, err)
-		}
-		return // can't progress
-	}
-	walk_(root, f, v)
-}
-
-func walk_(path string, f *os.FileInfo, v filepath.Visitor) {
-	if !f.IsDirectory() {
-		v.VisitFile(path, f)
-		return
-	}
-	if !v.VisitDir(path, f) {
-		return // skip directory entries
-	}
-	list, err := readDir(path)
-	if err != nil {
-		if eh, ok := v.(walkErrorHandler); ok {
-			eh.Error(path, err)
-		}
-	}
-	for _, e := range list {
-		walk_(filepath.Join(path, e.Name), e, v)
-	}
-}
-
-func readDir(dirname string) ([]*os.FileInfo, os.Error) {
-	f, err := os.Open(dirname)
-	if err != nil {
-		return nil, err
-	}
-	list, err := f.Readdir(-1)
-	f.Close()
-	if err != nil {
-		return nil, err
-	}
-	fi := make(fileInfoList, len(list))
-	for i := range list {
-		fi[i] = &list[i]
-	}
-	sort.Sort(fi)
-	return fi, nil
-}
-
-type fileInfoList []*os.FileInfo
-
-func (f fileInfoList) Len() int           { return len(f) }
-func (f fileInfoList) Less(i, j int) bool { return f[i].Name < f[j].Name }
-func (f fileInfoList) Swap(i, j int)      { f[i], f[j] = f[j], f[i] }