vis: refactored code to reflect using vis/unvis for file names

Added some more test cases for `vis`ing and `unvis`ing
strings, and a test case that walks/checks a directory with
filenames that require encoding. Had to change Path() to account
for possible errors Unvis() could return. Refactored Vis()/Unvis() into
go-mtree tar functionality as well.

Signed-off-by: Stephen Chung <schung@redhat.com>

check.go

@@ -53,8 +53,11 @@ func Check(root string, dh *DirectoryHierarchy, keywords []string) (*Result, err
 				creator.curSet = nil
 			}
 		case RelativeType, FullType:
-			filename := e.Path()
-			info, err := os.Lstat(filename)
+			pathname, err := e.Path()
+			if err != nil {
+				return nil, err
+			}
+			info, err := os.Lstat(pathname)
 			if err != nil {
 				return nil, err
 			}
@@ -77,23 +80,23 @@ func Check(root string, dh *DirectoryHierarchy, keywords []string) (*Result, err
 
 				keywordFunc, ok := KeywordFuncs[kw]
 				if !ok {
-					return nil, fmt.Errorf("Unknown keyword %q for file %q", kv.Keyword(), e.Path())
+					return nil, fmt.Errorf("Unknown keyword %q for file %q", kv.Keyword(), pathname)
 				}
 				if keywords != nil && !inSlice(kv.Keyword(), keywords) {
 					continue
 				}
-				fh, err := os.Open(filename)
+				fh, err := os.Open(pathname)
 				if err != nil {
 					return nil, err
 				}
-				curKeyVal, err := keywordFunc(filename, info, fh)
+				curKeyVal, err := keywordFunc(pathname, info, fh)
 				if err != nil {
 					fh.Close()
 					return nil, err
 				}
 				fh.Close()
 				if string(kv) != curKeyVal {
-					failure := Failure{Path: e.Path(), Keyword: kv.Keyword(), Expected: kv.Value(), Got: KeyVal(curKeyVal).Value()}
+					failure := Failure{Path: pathname, Keyword: kv.Keyword(), Expected: kv.Value(), Got: KeyVal(curKeyVal).Value()}
 					result.Failures = append(result.Failures, failure)
 				}
 			}
@@ -133,8 +136,12 @@ func TarCheck(tarDH, dh *DirectoryHierarchy, keywords []string) (*Result, error)
 				creator.curSet = nil
 			}
 		case RelativeType, FullType:
+			pathname, err := e.Path()
+			if err != nil {
+				return nil, err
+			}
 			if outOfTree {
-				return &result, fmt.Errorf("No parent node from %s", e.Path())
+				return &result, fmt.Errorf("No parent node from %s", pathname)
 			}
 			// TODO: handle the case where "." is not the first Entry to be found
 			tarEntry := curDir.Descend(e.Name)
@@ -165,15 +172,20 @@ func TarCheck(tarDH, dh *DirectoryHierarchy, keywords []string) (*Result, error)
 			}
 
 			for _, kv := range kvs {
+
 				if _, ok := KeywordFuncs[kv.Keyword()]; !ok {
-					return nil, fmt.Errorf("Unknown keyword %q for file %q", kv.Keyword(), e.Path())
+					return nil, fmt.Errorf("Unknown keyword %q for file %q", kv.Keyword(), pathname)
 				}
 				if keywords != nil && !inSlice(kv.Keyword(), keywords) {
 					continue
 				}
+				tarpath, err := tarEntry.Path()
+				if err != nil {
+					return nil, err
+				}
 				if tarkv := tarkvs.Has(kv.Keyword()); tarkv != emptyKV {
 					if string(tarkv) != string(kv) {
-						failure := Failure{Path: tarEntry.Path(), Keyword: kv.Keyword(), Expected: kv.Value(), Got: tarkv.Value()}
+						failure := Failure{Path: tarpath, Keyword: kv.Keyword(), Expected: kv.Value(), Got: tarkv.Value()}
 						result.Failures = append(result.Failures, failure)
 					}
 				}

check_test.go

@@ -260,3 +260,38 @@ func TestIgnoreComments(t *testing.T) {
 		t.Fatal(res.Failures)
 	}
 }
+
+func TestCheckNeedsEncoding(t *testing.T) {
+	dir, err := ioutil.TempDir("", "test-needs-encoding")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer os.RemoveAll(dir)
+
+	fh, err := os.Create(filepath.Join(dir, "file[ "))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := fh.Close(); err != nil {
+		t.Error(err)
+	}
+	fh, err = os.Create(filepath.Join(dir, "    , should work"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := fh.Close(); err != nil {
+		t.Error(err)
+	}
+
+	dh, err := Walk(dir, nil, DefaultKeywords)
+	if err != nil {
+		t.Fatal(err)
+	}
+	res, err := Check(dir, dh, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(res.Failures) > 0 {
+		t.Fatal(res.Failures)
+	}
+}

cmd/gomtree/main.go

@@ -202,13 +202,25 @@ func main() {
 			if len(res.Extra) > 0 {
 				defer os.Exit(1)
 				for _, extra := range res.Extra {
-					fmt.Printf("%s extra\n", extra.Path())
+					extrapath, err := extra.Path()
+					if err != nil {
+						log.Println(err)
+						isErr = true
+						return
+					}
+					fmt.Printf("%s extra\n", extrapath)
 				}
 			}
 			if len(res.Missing) > 0 {
 				defer os.Exit(1)
 				for _, missing := range res.Missing {
-					fmt.Printf("%s missing\n", missing.Path())
+					missingpath, err := missing.Path()
+					if err != nil {
+						log.Println(err)
+						isErr = true
+						return
+					}
+					fmt.Printf("%s missing\n", missingpath)
 				}
 			}
 		} else {

entry.go

@@ -48,14 +48,25 @@ func (e Entry) Ascend() *Entry {
 	return e.Parent
 }
 
-// Path provides the full path of the file, despite RelativeType or FullType
-func (e Entry) Path() string {
-	if e.Parent == nil || e.Type == FullType {
-		return filepath.Clean(e.Name)
+// Path provides the full path of the file, despite RelativeType or FullType. It
+// will be in Unvis'd form.
+func (e Entry) Path() (string, error) {
+	decodedName, err := Unvis(e.Name)
+	if err != nil {
+		return "", err
 	}
-	return filepath.Clean(filepath.Join(e.Parent.Path(), e.Name))
+	if e.Parent == nil || e.Type == FullType {
+		return filepath.Clean(decodedName), nil
+	}
+	parentName, err := e.Parent.Path()
+	if err != nil {
+		return "", err
+	}
+	return filepath.Clean(filepath.Join(parentName, decodedName)), nil
 }
 
+// String joins a file with its associated keywords. The file name will be the
+// Vis'd encoded version so that it can be parsed appropriately when Check'd.
 func (e Entry) String() string {
 	if e.Raw != "" {
 		return e.Raw

hierarchy.go

@@ -16,7 +16,8 @@ func (dh DirectoryHierarchy) WriteTo(w io.Writer) (n int64, err error) {
 	sort.Sort(byPos(dh.Entries))
 	var sum int64
 	for _, e := range dh.Entries {
-		i, err := io.WriteString(w, e.String()+"\n")
+		str := e.String()
+		i, err := io.WriteString(w, str+"\n")
 		if err != nil {
 			return sum, err
 		}

tar.go

@@ -97,8 +97,15 @@ func (ts *tarStream) readHeaders() {
 		defer os.Remove(tmpFile.Name())
 
 		// Alright, it's either file or directory
+		encodedName, err := Vis(filepath.Base(hdr.Name))
+		if err != nil {
+			tmpFile.Close()
+			os.Remove(tmpFile.Name())
+			ts.pipeReader.CloseWithError(err)
+			return
+		}
 		e := Entry{
-			Name: filepath.Base(hdr.Name),
+			Name: encodedName,
 			Type: RelativeType,
 		}
 
@@ -213,8 +220,13 @@ func populateTree(root, e *Entry, hdr *tar.Header, ts *tarStream) {
 			if isDir {
 				newEntry = e
 			} else {
+				encodedName, err := Vis(name)
+				if err != nil {
+					ts.setErr(err)
+					return
+				}
 				newEntry = &Entry{
-					Name: name,
+					Name: encodedName,
 					Type: RelativeType,
 				}
 			}
@@ -230,8 +242,13 @@ func populateTree(root, e *Entry, hdr *tar.Header, ts *tarStream) {
 		parent.Children = append([]*Entry{e}, parent.Children...)
 		e.Parent = parent
 	} else {
+		commentpath, err := e.Path()
+		if err != nil {
+			ts.setErr(err)
+			return
+		}
 		commentEntry := Entry{
-			Raw:  "# " + e.Path(),
+			Raw:  "# " + commentpath,
 			Type: CommentType,
 		}
 		e.Prev = &commentEntry

tar_test.go

@@ -119,12 +119,20 @@ func TestTar(t *testing.T) {
 			errors += "Keyword validation errors\n"
 		case len(res.Missing) > 0:
 			for _, m := range res.Missing {
-				t.Errorf("Missing file: %s\n", m.Path())
+				missingpath, err := m.Path()
+				if err != nil {
+					t.Fatal(err)
+				}
+				t.Errorf("Missing file: %s\n", missingpath)
 			}
 			errors += "Missing files not expected for this test\n"
 		case len(res.Extra) > 0:
 			for _, e := range res.Extra {
-				t.Errorf("Extra file: %s\n", e.Path())
+				extrapath, err := e.Path()
+				if err != nil {
+					t.Fatal(err)
+				}
+				t.Errorf("Extra file: %s\n", extrapath)
 			}
 			errors += "Extra files not expected for this test\n"
 		}

unvis.go

@@ -1,15 +1,22 @@
 package mtree
 
 // #include "vis.h"
+// #include <stdlib.h>
 import "C"
-import "fmt"
+import (
+	"fmt"
+	"unsafe"
+)
 
-func Unvis(str string) (string, error) {
-	dst := new(C.char)
-	ret := C.strunvis(dst, C.CString(str))
-	if ret == 0 {
-		return "", fmt.Errorf("failed to encode string")
+// Unvis is a wrapper for the C implementation of unvis, which decodes a string
+// that potentially has characters that are encoded with Vis
+func Unvis(src string) (string, error) {
+	cDst, cSrc := C.CString(string(make([]byte, len(src)+1))), C.CString(src)
+	defer C.free(unsafe.Pointer(cDst))
+	defer C.free(unsafe.Pointer(cSrc))
+	ret := C.strunvis(cDst, cSrc)
+	if ret == -1 {
+		return "", fmt.Errorf("failed to decode: %q", src)
 	}
-
-	return C.GoString(dst), nil
+	return C.GoString(cDst), nil
 }

vis.go

@@ -1,14 +1,26 @@
 package mtree
 
 // #include "vis.h"
+// #include <stdlib.h>
 import "C"
-import "fmt"
+import (
+	"fmt"
+	"math"
+	"unsafe"
+)
 
-func Vis(str string) (string, error) {
-	dst := new(C.char)
-	ret := C.strvis(dst, C.CString(str), C.VIS_WHITE|C.VIS_OCTAL|C.VIS_GLOB)
-	if ret == 0 {
-		return "", fmt.Errorf("failed to encode string")
+// Vis is a wrapper of the C implementation of the function vis, which encodes
+// a character with a particular format/style
+func Vis(src string) (string, error) {
+	// dst needs to be 4 times the length of str, must check appropriate size
+	if uint32(len(src)*4+1) >= math.MaxUint32/4 {
+		return "", fmt.Errorf("failed to encode: %q", src)
 	}
-	return C.GoString(dst), nil
+	dst := string(make([]byte, 4*len(src)+1))
+	cDst, cSrc := C.CString(dst), C.CString(src)
+	defer C.free(unsafe.Pointer(cDst))
+	defer C.free(unsafe.Pointer(cSrc))
+	C.strvis(cDst, cSrc, C.VIS_WHITE|C.VIS_OCTAL|C.VIS_GLOB)
+
+	return C.GoString(cDst), nil
 }

vis_test.go

@@ -9,13 +9,17 @@ func TestVis(t *testing.T) {
 		{"[", "\\133"},
 		{" ", "\\040"},
 		{"	", "\\011"},
+		{"dir with space", "dir\\040with\\040space"},
+		{"consec   spaces", "consec\\040\\040\\040spaces"},
+		{"trailingsymbol[", "trailingsymbol\\133"},
+		{" [ leadingsymbols", "\\040\\133\\040leadingsymbols"},
+		{"no_need_for_encoding", "no_need_for_encoding"},
 	}
 
 	for i := range testset {
 		got, err := Vis(testset[i].Src)
 		if err != nil {
 			t.Errorf("working with %q: %s", testset[i].Src, err)
-			continue
 		}
 		if got != testset[i].Dest {
 			t.Errorf("expected %#v; got %#v", testset[i].Dest, got)
@@ -33,3 +37,13 @@ func TestVis(t *testing.T) {
 		}
 	}
 }
+
+// The resulting string of Vis output could potentially be four times longer than
+// the original. Vis must handle this possibility.
+func TestVisLength(t *testing.T) {
+	testString := "All work and no play makes Jack a dull boy\n"
+	for i := 0; i < 20; i++ {
+		Vis(testString)
+		testString = testString + testString
+	}
+}

walk.go

@@ -47,9 +47,13 @@ func Walk(root string, exlcudes []ExcludeFunc, keywords []string) (*DirectoryHie
 
 			// Insert a comment of the full path of the directory's name
 			if creator.curDir != nil {
+				dirname, err := creator.curDir.Path()
+				if err != nil {
+					return err
+				}
 				creator.DH.Entries = append(creator.DH.Entries, Entry{
 					Pos:  len(creator.DH.Entries),
-					Raw:  "# " + filepath.Join(creator.curDir.Path(), entryPathName),
+					Raw:  "# " + filepath.Join(dirname, entryPathName),
 					Type: CommentType,
 				})
 			} else {
@@ -147,9 +151,12 @@ func Walk(root string, exlcudes []ExcludeFunc, keywords []string) (*DirectoryHie
 				}
 			}
 		}
-
+		encodedEntryName, err := Vis(entryPathName)
+		if err != nil {
+			return err
+		}
 		e := Entry{
-			Name:   entryPathName,
+			Name:   encodedEntryName,
 			Pos:    len(creator.DH.Entries),
 			Type:   RelativeType,
 			Set:    creator.curSet,