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() and String()
to account for possible errors Vis() and Unvis() could return.

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

check.go

@@ -50,7 +50,11 @@ func Check(root string, dh *DirectoryHierarchy, keywords []string) (*Result, err
 				creator.curSet = nil
 			}
 		case RelativeType, FullType:
-			info, err := os.Lstat(e.Path())
+			pathname, err := e.Path()
+			if err != nil {
+				return nil, err
+			}
+			info, err := os.Lstat(pathname)
 			if err != nil {
 				return nil, err
 			}
@@ -65,17 +69,17 @@ func Check(root string, dh *DirectoryHierarchy, keywords []string) (*Result, err
 			for _, kv := range kvs {
 				keywordFunc, ok := KeywordFuncs[kv.Keyword()]
 				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
 				}
-				curKeyVal, err := keywordFunc(filepath.Join(root, e.Path()), info)
+				curKeyVal, err := keywordFunc(filepath.Join(root, pathname), info)
 				if err != nil {
 					return nil, err
 				}
 				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)
 				}
 			}

check_test.go

@@ -208,3 +208,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)
+	}
+}

hierarchy.go

@@ -19,7 +19,11 @@ 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, err := e.String()
+		if err != nil {
+			return sum, err
+		}
+		i, err := io.WriteString(w, str+"\n")
 		if err != nil {
 			return sum, err
 		}
@@ -47,28 +51,39 @@ type Entry struct {
 }
 
 // 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)
+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
 }
 
-func (e Entry) String() string {
+func (e Entry) String() (string, error) {
 	if e.Raw != "" {
-		return e.Raw
+		return e.Raw, nil
 	}
 	if e.Type == BlankType {
-		return ""
+		return "", nil
 	}
 	if e.Type == DotDotType {
-		return e.Name
+		return e.Name, nil
+	}
+	decodedName, err := Unvis(e.Name)
+	if err != nil {
+		return "", err
 	}
-	// TODO(vbatts) if type is RelativeType and a keyword of not type=dir
 	if e.Type == SpecialType || e.Type == FullType || inSlice("type=dir", e.Keywords) {
-		return fmt.Sprintf("%s %s", e.Name, strings.Join(e.Keywords, " "))
+		return fmt.Sprintf("%s %s", decodedName, strings.Join(e.Keywords, " ")), nil
 	}
-	return fmt.Sprintf("    %s %s", e.Name, strings.Join(e.Keywords, " "))
+	return fmt.Sprintf("    %s %s", decodedName, strings.Join(e.Keywords, " ")), nil
 }
 
 // EntryType are the formats of lines in an mtree spec file

unvis.go

@@ -1,15 +1,23 @@
 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

@@ -53,9 +53,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 {
@@ -113,9 +117,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,