Repository: twpayne/chezmoi
Pull request #27 (2018-11-27 00:55:21)

Refactor source name parsing and generation
This PR factors out a few structs to avoid returning a large number of values from source name parsing functions.


Patch:
diff --git a/README.md b/README.md
index 2bb3d42f8..be16c4972 100644
--- a/README.md
+++ b/README.md
@@ -113,7 +113,7 @@ what might change. For your home machine:
       email: john@home.org
 
 If you intend to store private data (e.g. access tokens) in `~/.chezmoi.yaml`,
-make sure it has mode 0600. See "Keeping data private" below for more
+make sure it has permissions 0600. See "Keeping data private" below for more
 discussion on this.
 
 If you prefer, you can use any format supported by
@@ -209,7 +209,7 @@ My `~/.chezmoi/private_dot_gitconfig.tmpl` then contains:
     {{- end }}
 
 Note that any config files containing tokens in plain text should be private
-(mode 0600).
+(permissions 0600).
 
 ### Using encrypted config files
 
diff --git a/lib/chezmoi/any_actuator.go b/lib/chezmoi/any_actuator.go
index 67c549a62..c5c84d953 100644
--- a/lib/chezmoi/any_actuator.go
+++ b/lib/chezmoi/any_actuator.go
@@ -29,9 +29,9 @@ func (a *AnyActuator) Chmod(name string, mode os.FileMode) error {
 }
 
 // Mkdir implements Actuator.Mkdir.
-func (a *AnyActuator) Mkdir(name string, mode os.FileMode) error {
+func (a *AnyActuator) Mkdir(name string, perm os.FileMode) error {
 	a.actuated = true
-	return a.a.Mkdir(name, mode)
+	return a.a.Mkdir(name, perm)
 }
 
 // RemoveAll implements Actuator.RemoveAll.
@@ -41,7 +41,7 @@ func (a *AnyActuator) RemoveAll(name string) error {
 }
 
 // WriteFile implements Actuator.WriteFile.
-func (a *AnyActuator) WriteFile(name string, contents []byte, mode os.FileMode, currentContents []byte) error {
+func (a *AnyActuator) WriteFile(name string, contents []byte, perm os.FileMode, currentContents []byte) error {
 	a.actuated = true
-	return a.a.WriteFile(name, contents, mode, currentContents)
+	return a.a.WriteFile(name, contents, perm, currentContents)
 }
diff --git a/lib/chezmoi/chezmoi.go b/lib/chezmoi/chezmoi.go
index 31c262875..d30082af6 100644
--- a/lib/chezmoi/chezmoi.go
+++ b/lib/chezmoi/chezmoi.go
@@ -59,11 +59,28 @@ type TargetState struct {
 	Entries   map[string]Entry
 }
 
+type parsedSourceDirName struct {
+	dirName string
+	perm    os.FileMode
+}
+
+type parsedSourceFileName struct {
+	fileName string
+	perm     os.FileMode
+	empty    bool
+	template bool
+}
+
+type parsedSourceFilePath struct {
+	parsedSourceFileName
+	dirNames []string
+}
+
 // newDir returns a new directory state.
-func newDir(sourceName string, mode os.FileMode) *Dir {
+func newDir(sourceName string, perm os.FileMode) *Dir {
 	return &Dir{
 		sourceName: sourceName,
-		Perm:       mode,
+		Perm:       perm,
 		Entries:    make(map[string]Entry),
 	}
 }
@@ -255,7 +272,12 @@ func (ts *TargetState) Add(fs vfs.FS, target string, info os.FileInfo, addEmpty,
 		if info.Size() == 0 && !addEmpty {
 			return nil
 		}
-		sourceName := makeFileName(name, info.Mode(), info.Size() == 0, addTemplate)
+		sourceName := parsedSourceFileName{
+			fileName: name,
+			perm:     info.Mode() & os.ModePerm,
+			empty:    info.Size() == 0,
+			template: addTemplate,
+		}.SourceFileName()
 		if dirSourceName != "" {
 			sourceName = filepath.Join(dirSourceName, sourceName)
 		}
@@ -282,7 +304,10 @@ func (ts *TargetState) Add(fs vfs.FS, target string, info os.FileInfo, addEmpty,
 			}
 			return nil // entry already exists
 		}
-		sourceName := makeDirName(name, info.Mode())
+		sourceName := parsedSourceDirName{
+			dirName: name,
+			perm:    info.Mode() & os.ModePerm,
+		}.SourceDirName()
 		if dirSourceName != "" {
 			sourceName = filepath.Join(dirSourceName, sourceName)
 		}
@@ -390,8 +415,8 @@ func (ts *TargetState) Populate(fs vfs.FS) error {
 		}
 		switch {
 		case info.Mode().IsRegular():
-			dirNames, fileName, mode, isEmpty, isTemplate := parseFilePath(relPath)
-			entries, err := ts.findEntries(dirNames)
+			psfp := parseSourceFilePath(relPath)
+			entries, err := ts.findEntries(psfp.dirNames)
 			if err != nil {
 				return err
 			}
@@ -399,7 +424,7 @@ func (ts *TargetState) Populate(fs vfs.FS) error {
 			if err != nil {
 				return err
 			}
-			if isTemplate {
+			if psfp.template {
 				tmpl, err := template.New(path).Parse(string(contents))
 				if err != nil {
 					return fmt.Errorf("%s: %v", path, err)
@@ -410,22 +435,22 @@ func (ts *TargetState) Populate(fs vfs.FS) error {
 				}
 				contents = output.Bytes()
 			}
-			entries[fileName] = &File{
+			entries[psfp.fileName] = &File{
 				sourceName: relPath,
-				Empty:      isEmpty,
-				Perm:       mode,
+				Empty:      psfp.empty,
+				Perm:       psfp.perm,
 				Contents:   contents,
 			}
 		case info.Mode().IsDir():
 			components := splitPathList(relPath)
-			dirNames, modes := parseDirNameComponents(components)
+			dirNames, perms := parseDirNameComponents(components)
 			entries, err := ts.findEntries(dirNames[:len(dirNames)-1])
 			if err != nil {
 				return err
 			}
 			dirName := dirNames[len(dirNames)-1]
-			mode := modes[len(modes)-1]
-			entries[dirName] = newDir(relPath, mode)
+			perm := perms[len(perms)-1]
+			entries[dirName] = newDir(relPath, perm)
 		default:
 			return fmt.Errorf("unsupported file type: %s", path)
 		}
@@ -456,110 +481,117 @@ func (ts *TargetState) findEntry(name string) (Entry, error) {
 	return entries[names[len(names)-1]], nil
 }
 
-func makeDirName(name string, mode os.FileMode) string {
-	dirName := ""
-	if mode&os.FileMode(077) == os.FileMode(0) {
-		dirName = privatePrefix
+// parseSourceDirName parses a single directory name.
+func parseSourceDirName(dirName string) parsedSourceDirName {
+	perm := os.FileMode(0777)
+	if strings.HasPrefix(dirName, privatePrefix) {
+		dirName = strings.TrimPrefix(dirName, privatePrefix)
+		perm &= 0700
+	}
+	if strings.HasPrefix(dirName, dotPrefix) {
+		dirName = "." + strings.TrimPrefix(dirName, dotPrefix)
+	}
+	return parsedSourceDirName{
+		dirName: dirName,
+		perm:    perm,
 	}
-	if strings.HasPrefix(name, ".") {
-		dirName += dotPrefix + strings.TrimPrefix(name, ".")
+}
+
+// SourceDirName returns psdn's source dir name.
+func (psdn parsedSourceDirName) SourceDirName() string {
+	sourceDirName := ""
+	if psdn.perm&os.FileMode(077) == os.FileMode(0) {
+		sourceDirName = privatePrefix
+	}
+	if strings.HasPrefix(psdn.dirName, ".") {
+		sourceDirName += dotPrefix + strings.TrimPrefix(psdn.dirName, ".")
 	} else {
-		dirName += name
+		sourceDirName += psdn.dirName
 	}
-	return dirName
+	return sourceDirName
 }
 
-func makeFileName(name string, mode os.FileMode, isEmpty bool, isTemplate bool) string {
-	fileName := ""
-	if mode&os.FileMode(077) == os.FileMode(0) {
-		fileName = privatePrefix
+// parseSourceFileName parses a source file name.
+func parseSourceFileName(fileName string) parsedSourceFileName {
+	perm := os.FileMode(0666)
+	private := false
+	empty := false
+	template := false
+	if strings.HasPrefix(fileName, privatePrefix) {
+		fileName = strings.TrimPrefix(fileName, privatePrefix)
+		private = true
 	}
-	if isEmpty {
-		fileName += emptyPrefix
+	if strings.HasPrefix(fileName, emptyPrefix) {
+		fileName = strings.TrimPrefix(fileName, emptyPrefix)
+		empty = true
 	}
-	if mode&os.FileMode(0111) != os.FileMode(0) {
-		fileName += executablePrefix
+	if strings.HasPrefix(fileName, executablePrefix) {
+		fileName = strings.TrimPrefix(fileName, executablePrefix)
+		perm |= 0111
 	}
-	if strings.HasPrefix(name, ".") {
-		fileName += dotPrefix + strings.TrimPrefix(name, ".")
-	} else {
-		fileName += name
+	if strings.HasPrefix(fileName, dotPrefix) {
+		fileName = "." + strings.TrimPrefix(fileName, dotPrefix)
 	}
-	if isTemplate {
-		fileName += templateSuffix
+	if strings.HasSuffix(fileName, templateSuffix) {
+		fileName = strings.TrimSuffix(fileName, templateSuffix)
+		template = true
 	}
-	return fileName
-}
-
-// parseDirName parses a single directory name. It returns the target name,
-// mode.
-func parseDirName(dirName string) (string, os.FileMode) {
-	name := dirName
-	mode := os.FileMode(0777)
-	if strings.HasPrefix(name, privatePrefix) {
-		name = strings.TrimPrefix(name, privatePrefix)
-		mode &= 0700
+	if private {
+		perm &= 0700
 	}
-	if strings.HasPrefix(name, dotPrefix) {
-		name = "." + strings.TrimPrefix(name, dotPrefix)
+	return parsedSourceFileName{
+		fileName: fileName,
+		perm:     perm,
+		empty:    empty,
+		template: template,
 	}
-	return name, mode
 }
 
-// parseFileName parses a single file name. It returns the target name, mode,
-// whether the contents should be interpreted as a template, and any error.
-func parseFileName(fileName string) (string, os.FileMode, bool, bool) {
-	name := fileName
-	mode := os.FileMode(0666)
-	isPrivate := false
-	isEmpty := false
-	isTemplate := false
-	if strings.HasPrefix(name, privatePrefix) {
-		name = strings.TrimPrefix(name, privatePrefix)
-		isPrivate = true
-	}
-	if strings.HasPrefix(name, emptyPrefix) {
-		name = strings.TrimPrefix(name, emptyPrefix)
-		isEmpty = true
+// SourceFileName returns psfn's source file name.
+func (psfn parsedSourceFileName) SourceFileName() string {
+	fileName := ""
+	if psfn.perm&os.FileMode(077) == os.FileMode(0) {
+		fileName = privatePrefix
 	}
-	if strings.HasPrefix(name, executablePrefix) {
-		name = strings.TrimPrefix(name, executablePrefix)
-		mode |= 0111
+	if psfn.empty {
+		fileName += emptyPrefix
 	}
-	if strings.HasPrefix(name, dotPrefix) {
-		name = "." + strings.TrimPrefix(name, dotPrefix)
+	if psfn.perm&os.FileMode(0111) != os.FileMode(0) {
+		fileName += executablePrefix
 	}
-	if strings.HasSuffix(name, templateSuffix) {
-		name = strings.TrimSuffix(name, templateSuffix)
-		isTemplate = true
+	if strings.HasPrefix(psfn.fileName, ".") {
+		fileName += dotPrefix + strings.TrimPrefix(psfn.fileName, ".")
+	} else {
+		fileName += psfn.fileName
 	}
-	if isPrivate {
-		mode &= 0700
+	if psfn.template {
+		fileName += templateSuffix
 	}
-	return name, mode, isEmpty, isTemplate
+	return fileName
 }
 
 // parseDirNameComponents parses multiple directory name components. It returns
-// the target directory names, target modes, and any error.
+// the target directory names, target permissions, and any error.
 func parseDirNameComponents(components []string) ([]string, []os.FileMode) {
 	dirNames := []string{}
-	modes := []os.FileMode{}
+	perms := []os.FileMode{}
 	for _, component := range components {
-		dirName, mode := parseDirName(component)
-		dirNames = append(dirNames, dirName)
-		modes = append(modes, mode)
+		psdn := parseSourceDirName(component)
+		dirNames = append(dirNames, psdn.dirName)
+		perms = append(perms, psdn.perm)
 	}
-	return dirNames, modes
+	return dirNames, perms
 }
 
-// parseFilePath parses a single file path. It returns the target directory
-// names, the target filename, the target mode, whether the contents should be
-// interpreted as a template, and any error.
-func parseFilePath(path string) ([]string, string, os.FileMode, bool, bool) {
+// parseSourceFilePath parses a single source file path.
+func parseSourceFilePath(path string) parsedSourceFilePath {
 	components := splitPathList(path)
 	dirNames, _ := parseDirNameComponents(components[0 : len(components)-1])
-	fileName, mode, isEmpty, isTemplate := parseFileName(components[len(components)-1])
-	return dirNames, fileName, mode, isEmpty, isTemplate
+	psfn := parseSourceFileName(components[len(components)-1])
+	return parsedSourceFilePath{
+		parsedSourceFileName: psfn,
+		dirNames:             dirNames,
+	}
 }
 
 // sortedEntryNames returns a sorted slice of all entry names.
diff --git a/lib/chezmoi/fs_actuator.go b/lib/chezmoi/fs_actuator.go
index f97061806..b3d8e1d52 100644
--- a/lib/chezmoi/fs_actuator.go
+++ b/lib/chezmoi/fs_actuator.go
@@ -27,7 +27,7 @@ func NewFSActuator(fs vfs.FS, targetDir string) *FSActuator {
 }
 
 // WriteFile implements Actuator.WriteFile.
-func (a *FSActuator) WriteFile(name string, contents []byte, mode os.FileMode, currentContents []byte) error {
+func (a *FSActuator) WriteFile(name string, contents []byte, perm os.FileMode, currentContents []byte) error {
 	// Special case: if writing to the real filesystem, use github.com/google/renameio
 	if a.FS == vfs.OSFS {
 		t, err := renameio.TempFile(a.dir, name)
@@ -35,7 +35,7 @@ func (a *FSActuator) WriteFile(name string, contents []byte, mode os.FileMode, c
 			return err
 		}
 		defer t.Cleanup()
-		if err := t.Chmod(mode); err != nil {
+		if err := t.Chmod(perm); err != nil {
 			return err
 		}
 		if _, err := t.Write(contents); err != nil {
@@ -43,5 +43,5 @@ func (a *FSActuator) WriteFile(name string, contents []byte, mode os.FileMode, c
 		}
 		return t.CloseAtomicallyReplace()
 	}
-	return a.FS.WriteFile(name, contents, mode)
+	return a.FS.WriteFile(name, contents, perm)
 }
diff --git a/lib/chezmoi/logging_actuator.go b/lib/chezmoi/logging_actuator.go
index 2e304c065..e657749db 100644
--- a/lib/chezmoi/logging_actuator.go
+++ b/lib/chezmoi/logging_actuator.go
@@ -35,9 +35,9 @@ func (a *LoggingActuator) Chmod(name string, mode os.FileMode) error {
 }
 
 // Mkdir implements Actuator.Mkdir.
-func (a *LoggingActuator) Mkdir(name string, mode os.FileMode) error {
-	action := fmt.Sprintf("mkdir -m %o %s", mode, name)
-	err := a.a.Mkdir(name, mode)
+func (a *LoggingActuator) Mkdir(name string, perm os.FileMode) error {
+	action := fmt.Sprintf("mkdir -m %o %s", perm, name)
+	err := a.a.Mkdir(name, perm)
 	if err == nil {
 		fmt.Fprintln(a.w, action)
 	} else {
@@ -59,9 +59,9 @@ func (a *LoggingActuator) RemoveAll(name string) error {
 }
 
 // WriteFile implements Actuator.WriteFile.
-func (a *LoggingActuator) WriteFile(name string, contents []byte, mode os.FileMode, currentContents []byte) error {
-	action := fmt.Sprintf("install -m %o /dev/null %s", mode, name)
-	err := a.a.WriteFile(name, contents, mode, currentContents)
+func (a *LoggingActuator) WriteFile(name string, contents []byte, perm os.FileMode, currentContents []byte) error {
+	action := fmt.Sprintf("install -m %o /dev/null %s", perm, name)
+	err := a.a.WriteFile(name, contents, perm, currentContents)
 	if err == nil {
 		fmt.Fprintln(a.w, action)
 		for _, section := range diff(splitLines(currentContents), splitLines(contents)) {


Test patch:
diff --git a/lib/chezmoi/chezmoi_test.go b/lib/chezmoi/chezmoi_test.go
index 76c54179e..96ddc2f5c 100644
--- a/lib/chezmoi/chezmoi_test.go
+++ b/lib/chezmoi/chezmoi_test.go
@@ -8,51 +8,137 @@ import (
 	"github.com/twpayne/go-vfs/vfstest"
 )
 
-func TestDirName(t *testing.T) {
+func TestSourceDirName(t *testing.T) {
 	for _, tc := range []struct {
-		dirName string
-		name    string
-		mode    os.FileMode
+		sourceDirName string
+		psdn          parsedSourceDirName
 	}{
-		{dirName: "foo", name: "foo", mode: 0777},
-		{dirName: "dot_foo", name: ".foo", mode: 0777},
-		{dirName: "private_foo", name: "foo", mode: 0700},
-		{dirName: "private_dot_foo", name: ".foo", mode: 0700},
+		{
+			sourceDirName: "foo",
+			psdn: parsedSourceDirName{
+				dirName: "foo",
+				perm:    0777,
+			},
+		},
+		{
+			sourceDirName: "dot_foo",
+			psdn: parsedSourceDirName{
+				dirName: ".foo",
+				perm:    0777,
+			},
+		},
+		{
+			sourceDirName: "private_foo",
+			psdn: parsedSourceDirName{
+				dirName: "foo",
+				perm:    0700,
+			},
+		},
+		{
+			sourceDirName: "private_dot_foo",
+			psdn: parsedSourceDirName{
+				dirName: ".foo",
+				perm:    0700,
+			},
+		},
 	} {
-		t.Run(tc.dirName, func(t *testing.T) {
-			if gotName, gotMode := parseDirName(tc.dirName); gotName != tc.name || gotMode != tc.mode {
-				t.Errorf("parseDirName(%q) == %q, %v, want %q, %v", tc.dirName, gotName, gotMode, tc.name, tc.mode)
+		t.Run(tc.sourceDirName, func(t *testing.T) {
+			gotPSDN := parseSourceDirName(tc.sourceDirName)
+			if diff, equal := messagediff.PrettyDiff(tc.psdn, gotPSDN); !equal {
+				t.Errorf("parseSourceDirName(%q) == %+v, want %+v, diff:\n%s", tc.sourceDirName, gotPSDN, tc.psdn, diff)
 			}
-			if gotDirName := makeDirName(tc.name, tc.mode); gotDirName != tc.dirName {
-				t.Errorf("makeDirName(%q, %v) == %q, want %q", tc.name, tc.mode, gotDirName, tc.dirName)
+			if gotSourceDirName := tc.psdn.SourceDirName(); gotSourceDirName != tc.sourceDirName {
+				t.Errorf("%+v.SourceDirName() == %q, want %q", tc.psdn, gotSourceDirName, tc.sourceDirName)
 			}
 		})
 	}
 }
 
-func TestFileName(t *testing.T) {
+func TestSourceFileName(t *testing.T) {
 	for _, tc := range []struct {
-		fileName   string
-		name       string
-		mode       os.FileMode
-		isEmpty    bool
-		isTemplate bool
+		sourceFileName string
+		psfn           parsedSourceFileName
 	}{
-		{fileName: "foo", name: "foo", mode: 0666, isEmpty: false, isTemplate: false},
-		{fileName: "dot_foo", name: ".foo", mode: 0666, isEmpty: false, isTemplate: false},
-		{fileName: "private_foo", name: "foo", mode: 0600, isEmpty: false, isTemplate: false},
-		{fileName: "private_dot_foo", name: ".foo", mode: 0600, isEmpty: false, isTemplate: false},
-		{fileName: "empty_foo", name: "foo", mode: 0666, isEmpty: true, isTemplate: false},
-		{fileName: "executable_foo", name: "foo", mode: 0777, isEmpty: false, isTemplate: false},
-		{fileName: "foo.tmpl", name: "foo", mode: 0666, isEmpty: false, isTemplate: true},
-		{fileName: "private_executable_dot_foo.tmpl", name: ".foo", mode: 0700, isEmpty: false, isTemplate: true},
+		{
+			sourceFileName: "foo",
+			psfn: parsedSourceFileName{
+				fileName: "foo",
+				perm:     0666,
+				empty:    false,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "dot_foo",
+			psfn: parsedSourceFileName{
+				fileName: ".foo",
+				perm:     0666,
+				empty:    false,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "private_foo",
+			psfn: parsedSourceFileName{
+				fileName: "foo",
+				perm:     0600,
+				empty:    false,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "private_dot_foo",
+			psfn: parsedSourceFileName{
+				fileName: ".foo",
+				perm:     0600,
+				empty:    false,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "empty_foo",
+			psfn: parsedSourceFileName{
+				fileName: "foo",
+				perm:     0666,
+				empty:    true,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "executable_foo",
+			psfn: parsedSourceFileName{
+				fileName: "foo",
+				perm:     0777,
+				empty:    false,
+				template: false,
+			},
+		},
+		{
+			sourceFileName: "foo.tmpl",
+			psfn: parsedSourceFileName{
+				fileName: "foo",
+				perm:     0666,
+				empty:    false,
+				template: true,
+			},
+		},
+		{
+			sourceFileName: "private_executable_dot_foo.tmpl",
+			psfn: parsedSourceFileName{
+				fileName: ".foo",
+				perm:     0700,
+				empty:    false,
+				template: true,
+			},
+		},
 	} {
-		t.Run(tc.fileName, func(t *testing.T) {
-			if gotName, gotMode, gotIsEmpty, gotIsTemplate := parseFileName(tc.fileName); gotName != tc.name || gotMode != tc.mode || gotIsEmpty != tc.isEmpty || gotIsTemplate != tc.isTemplate {
-				t.Errorf("parseFileName(%q) == %q, %v, %v, %v want %q, %v, %v, %v", tc.fileName, gotName, gotMode, gotIsEmpty, gotIsTemplate, tc.name, tc.mode, tc.isEmpty, tc.isTemplate)
+		t.Run(tc.sourceFileName, func(t *testing.T) {
+			gotPSFN := parseSourceFileName(tc.sourceFileName)
+			if diff, equal := messagediff.PrettyDiff(tc.psfn, gotPSFN); !equal {
+				t.Errorf("parseSourceFileName(%q) == %+v, want %+v, diff:\n%s", tc.sourceFileName, gotPSFN, tc.psfn, diff)
 			}
-			if gotFileName := makeFileName(tc.name, tc.mode, tc.isEmpty, tc.isTemplate); gotFileName != tc.fileName {
-				t.Errorf("makeFileName(%q, %v, %v, %v) == %q, want %q", tc.name, tc.mode, tc.isEmpty, tc.isTemplate, gotFileName, tc.fileName)
+			if gotSourceFileName := tc.psfn.SourceFileName(); gotSourceFileName != tc.sourceFileName {
+				t.Errorf("%+v.SourceFileName() == %q, want %q", tc.psfn, gotSourceFileName, tc.sourceFileName)
 			}
 		})
 	}