From 96ad402ede0867d71abf92aa93b04197c81cbbf0 Mon Sep 17 00:00:00 2001 From: Gabriel Sanches Date: Fri, 1 May 2020 20:49:15 -0300 Subject: [PATCH] cmd/plg: fix slice overwriting on expansion --- cmd/plg/check_test.go | 225 ++++++++++++++++++ .../TestCheck/bug_expand/combined.golden | 9 + .../TestCheck/bug_expand/stdout.golden | 9 + linker/linker.go | 6 +- linker/linker_test.go | 179 +++++++++++++- parser/parser.go | 6 +- 6 files changed, 427 insertions(+), 7 deletions(-) create mode 100644 cmd/plg/testdata/TestCheck/bug_expand/combined.golden create mode 100644 cmd/plg/testdata/TestCheck/bug_expand/stdout.golden diff --git a/cmd/plg/check_test.go b/cmd/plg/check_test.go index 6e59ddf..c0839b3 100644 --- a/cmd/plg/check_test.go +++ b/cmd/plg/check_test.go @@ -973,6 +973,231 @@ func TestCheck(t *testing.T) { conflicts: true, err: nil, }, + { + // NOTE(gbrlsnchs): Bug caught on my own dotfiles. + name: "bug expand", + drv: fstest.InMemoryDriver{ + CurrentDir: "home/dotfiles", + Files: map[string]fstest.File{ + "home": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "dotfiles": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "root": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "dir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "subdir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "target_1": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "foo": {Data: []byte("foo")}, + "bar": {Data: []byte("bar")}, + }, + }, + "target_2": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "baz": {Data: []byte("baz")}, + }, + }, + }, + }, + }, + }, + }, + }, + "bug_expand.yml": { + Linkname: "", + Perm: os.ModePerm, + Data: yamlData(config.Config{ + Targets: []string{"root"}, + Options: map[string]*config.Config{ + "root": { + BaseDir: fstest.AbsPath("etc"), + Targets: []string{"dir"}, + Options: map[string]*config.Config{ + "dir": { + Targets: []string{"subdir"}, + Flatten: true, + }, + }, + Flatten: true, + }, + }, + }), + Children: nil, + }, + }, + }, + }, + }, + "etc": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "subdir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "target_1": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "foo": {Data: []byte("foo")}, + "bar": {Data: []byte("bar")}, + }, + }, + "target_2": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "qux": {Data: []byte("qux")}, + }, + }, + }, + }, + }, + }, + }, + }, + cmd: checkCmd{}, + want: fstest.InMemoryDriver{ + CurrentDir: "home/dotfiles", + Files: map[string]fstest.File{ + "home": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "dotfiles": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "root": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "dir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "subdir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "target_1": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "foo": {Data: []byte("foo")}, + "bar": {Data: []byte("bar")}, + }, + }, + "target_2": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "baz": {Data: []byte("baz")}, + }, + }, + }, + }, + }, + }, + }, + }, + "bug_expand.yml": { + Linkname: "", + Perm: os.ModePerm, + Data: yamlData(config.Config{ + Targets: []string{"root"}, + Options: map[string]*config.Config{ + "root": { + BaseDir: fstest.AbsPath("etc"), + Targets: []string{"dir"}, + Options: map[string]*config.Config{ + "dir": { + Targets: []string{"subdir"}, + Flatten: true, + }, + }, + Flatten: true, + }, + }, + }), + Children: nil, + }, + }, + }, + }, + }, + "etc": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "subdir": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "target_1": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "foo": {Data: []byte("foo")}, + "bar": {Data: []byte("bar")}, + }, + }, + "target_2": { + Linkname: "", + Perm: os.ModePerm, + Data: nil, + Children: map[string]fstest.File{ + "qux": {Data: []byte("qux")}, + }, + }, + }, + }, + }, + }, + }, + }, + conflicts: false, + err: nil, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { diff --git a/cmd/plg/testdata/TestCheck/bug_expand/combined.golden b/cmd/plg/testdata/TestCheck/bug_expand/combined.golden new file mode 100644 index 0000000..ef2360d --- /dev/null +++ b/cmd/plg/testdata/TestCheck/bug_expand/combined.golden @@ -0,0 +1,9 @@ +. +└── root (SKIP) + └── dir (SKIP) + └── subdir (EXPAND) + ├── target_1 (EXPAND) + │ ├── bar <- ~etc/subdir/target_1/bar (CONFLICT) + │ └── foo <- ~etc/subdir/target_1/foo (CONFLICT) + └── target_2 (EXPAND) + └── baz <- ~etc/subdir/target_2/baz (READY) diff --git a/cmd/plg/testdata/TestCheck/bug_expand/stdout.golden b/cmd/plg/testdata/TestCheck/bug_expand/stdout.golden new file mode 100644 index 0000000..ef2360d --- /dev/null +++ b/cmd/plg/testdata/TestCheck/bug_expand/stdout.golden @@ -0,0 +1,9 @@ +. +└── root (SKIP) + └── dir (SKIP) + └── subdir (EXPAND) + ├── target_1 (EXPAND) + │ ├── bar <- ~etc/subdir/target_1/bar (CONFLICT) + │ └── foo <- ~etc/subdir/target_1/foo (CONFLICT) + └── target_2 (EXPAND) + └── baz <- ~etc/subdir/target_2/baz (READY) diff --git a/linker/linker.go b/linker/linker.go index 7c65fca..c8e921f 100644 --- a/linker/linker.go +++ b/linker/linker.go @@ -145,14 +145,16 @@ func expand(n *parser.Node, children []fs.FileInfo) { } n.Children = make([]*parser.Node, len(children)) for i, c := range children { + tg := append(make([]string, 0, len(n.Target.Path) + 1), n.Target.Path...) + ln := append(make([]string, 0, len(n.Link.Path) + 1), n.Link.Path...) n.Children[i] = &parser.Node{ Target: parser.File{ BaseDir: n.Target.BaseDir, - Path: append(n.Target.Path, c.Name()), + Path: append(tg, c.Name()), }, Link: parser.File{ BaseDir: n.Link.BaseDir, - Path: append(n.Link.Path, c.Name()), + Path: append(ln, c.Name()), }, Children: nil, } diff --git a/linker/linker_test.go b/linker/linker_test.go index 0028cb7..bb28a7e 100644 --- a/linker/linker_test.go +++ b/linker/linker_test.go @@ -678,9 +678,6 @@ func testResolve(t *testing.T) { filepath.Join("test", "foo", "bar"): fstest.StubFile{ ExistsReturn: false, }, - filepath.Join("test", "foo", "bar"): fstest.StubFile{ - ExistsReturn: false, - }, filepath.Join("test", "foo", "baz"): fstest.StubFile{ ExistsReturn: false, }, @@ -868,6 +865,181 @@ func testResolve(t *testing.T) { Status: parser.StatusSkip, }, }, + { + drv: fstest.SpyDriver{ + StatReturn: map[string]fs.FileInfo{ + // targets + "foo": fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("foo", "bar"): fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("foo", "bar", "test"): fstest.StubFile{ + ExistsReturn: true, + }, + filepath.Join("foo", "qux"): fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("foo", "qux", "test"): fstest.StubFile{ + ExistsReturn: true, + }, + // links + filepath.Join("test", "foo"): fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("test", "foo", "bar"): fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("test", "foo", "bar", "test"): fstest.StubFile{ + ExistsReturn: false, + }, + filepath.Join("test", "foo", "qux"): fstest.StubFile{ + ExistsReturn: true, + IsDirReturn: true, + }, + filepath.Join("test", "foo", "qux", "test"): fstest.StubFile{ + ExistsReturn: false, + }, + }, + StatErr: map[string]error{ + "foo": nil, + filepath.Join("test", "foo"): nil, + }, + ReadDirReturn: map[string][]fs.FileInfo{ + "foo": { + fstest.StubFile{NameReturn: "bar"}, + fstest.StubFile{NameReturn: "qux"}, + }, + filepath.Join("foo", "bar"): { + fstest.StubFile{NameReturn: "test"}, + }, + filepath.Join("foo", "qux"): { + fstest.StubFile{NameReturn: "test"}, + }, + filepath.Join("test", "foo"): { + fstest.StubFile{NameReturn: "bar", IsDirReturn: true}, + fstest.StubFile{NameReturn: "qux", IsDirReturn: true}, + }, + filepath.Join("test", "foo", "bar"): { + fstest.StubFile{NameReturn: "test"}, + }, + filepath.Join("test", "foo", "qux"): { + fstest.StubFile{NameReturn: "test"}, + }, + }, + ReadDirErr: map[string]error{ + "foo": nil, + filepath.Join("test", "foo"): nil, + }, + }, + n: &parser.Node{ + Target: parser.File{ + BaseDir: "", + Path: []string{"foo"}, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{"foo"}, + }, + Children: nil, + }, + err: nil, + want: &parser.Node{ + Target: parser.File{ + BaseDir: "", + Path: []string{"foo"}, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{"foo"}, + }, + Children: []*parser.Node{ + { + Target: parser.File{ + BaseDir: "", + Path: []string{ + "foo", + "bar", + }, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{ + "foo", + "bar", + }, + }, + Children: []*parser.Node{ + { + Target: parser.File{ + BaseDir: "", + Path: []string{ + "foo", + "bar", + "test", + }, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{ + "foo", + "bar", + "test", + }, + }, + Status: parser.StatusReady, + }, + }, + Status: parser.StatusExpand, + }, + { + Target: parser.File{ + BaseDir: "", + Path: []string{ + "foo", + "qux", + }, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{ + "foo", + "qux", + }, + }, + Children: []*parser.Node{ + { + Target: parser.File{ + BaseDir: "", + Path: []string{ + "foo", + "qux", + "test", + }, + }, + Link: parser.File{ + BaseDir: "test", + Path: []string{ + "foo", + "qux", + "test", + }, + }, + Status: parser.StatusReady, + }, + }, + Status: parser.StatusExpand, + }, + }, + Status: parser.StatusExpand, + }, + }, } for _, tc := range testCases { t.Run("", func(t *testing.T) { @@ -900,6 +1072,7 @@ func testResolve(t *testing.T) { cmp.Diff(want, got), ) } + t.Logf("\n%s", tr.String()) }) } } diff --git a/parser/parser.go b/parser/parser.go index 4659e5f..04eca47 100644 --- a/parser/parser.go +++ b/parser/parser.go @@ -72,10 +72,12 @@ func (p *Parser) parseChildren(c *config.Config, ptargets, plinks []string) []*N if cc.BaseDir == "" { cc.BaseDir = c.BaseDir } + tgs := append(make([]string, 0, len(ptargets)+1), ptargets...) + lns := append(make([]string, 0, len(plinks)+1), plinks...) cc.BaseDir = p.expandVar(cc.BaseDir) children = append(children, p.parseTarget(cc, - append(ptargets, tg), - append(plinks, tg))) + append(tgs, tg), + append(lns, tg))) } } return children