From d4ee735ff9a2c320fdffff2a08eba280013b1a62 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Tue, 16 Jan 2024 23:34:00 +0100 Subject: [PATCH 01/11] escape paths --- go.mod | 5 ++++- go.sum | 2 ++ pathlist/list.go | 21 ++++++++++++++------- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/go.mod b/go.mod index 43622bb..16a1aa6 100644 --- a/go.mod +++ b/go.mod @@ -2,7 +2,10 @@ module al.essio.dev/pkg/tools go 1.21 -require github.com/stretchr/testify v1.8.4 +require ( + github.com/alessio/shellescape v1.4.2 + github.com/stretchr/testify v1.8.4 +) require ( github.com/davecgh/go-spew v1.1.1 // indirect diff --git a/go.sum b/go.sum index fa4b6e6..841b2bb 100644 --- a/go.sum +++ b/go.sum @@ -1,3 +1,5 @@ +github.com/alessio/shellescape v1.4.2 h1:MHPfaU+ddJ0/bYWpgIeUnQUqKrlJ1S7BfEYPM4uEoM0= +github.com/alessio/shellescape v1.4.2/go.mod h1:PZAiSCk0LJaZkiCSkPv8qIobYglO3FPpyFjDCtHLS30= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= diff --git a/pathlist/list.go b/pathlist/list.go index 0a5eeb4..26f8de1 100644 --- a/pathlist/list.go +++ b/pathlist/list.go @@ -7,6 +7,8 @@ import ( "path/filepath" "slices" "strings" + + "github.com/alessio/shellescape" ) // List builds a list of directories by parsing PATH-like variables @@ -107,7 +109,7 @@ func (d *dirList) load() { } func (d *dirList) Append(path string) { - p := filepath.Clean(path) + p := quoteAndClean(path) if d.Nil() { d.lst = []string{p} return @@ -122,7 +124,7 @@ func (d *dirList) Drop(path string) { if d.Nil() { return } - p := filepath.Clean(path) + p := quoteAndClean(path) if idx := slices.Index(d.lst, p); idx != -1 { d.lst = slices.Delete(d.lst, idx, idx+1) @@ -130,7 +132,7 @@ func (d *dirList) Drop(path string) { } func (d *dirList) Prepend(path string) { - p := filepath.Clean(path) + p := quoteAndClean(path) if d.Nil() { d.lst = []string{p} return @@ -167,9 +169,9 @@ func (d *dirList) clone(o *dirList) *dirList { return o } -func removeDups[T comparable](col []T, applyFn func(T) (T, bool)) []T { - var uniq = make([]T, 0) - ks := make(map[T]interface{}) +func removeDups(col []string, applyFn func(string) (string, bool)) []string { + var uniq = make([]string, 0) + ks := make(map[string]interface{}) for _, el := range col { vv, ok := applyFn(el) @@ -178,7 +180,8 @@ func removeDups[T comparable](col []T, applyFn func(T) (T, bool)) []T { } if _, ok := ks[vv]; !ok { - uniq = append(uniq, vv) + quoted := shellescape.Quote(string(vv)) + uniq = append(uniq, quoted) ks[vv] = struct{}{} } } @@ -194,3 +197,7 @@ var filterEmptyStrings = func(s string) (string, bool) { return clean, false } + +func quoteAndClean(s string) string { + return shellescape.Quote(filepath.Clean(s)) +} From 0f3ef7446c1c2afddbe31a7800b14327ba8cdd49 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 18 Jan 2024 03:33:19 +0100 Subject: [PATCH 02/11] remove redundant type conversion --- pathlist/list.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pathlist/list.go b/pathlist/list.go index 26f8de1..3a0730e 100644 --- a/pathlist/list.go +++ b/pathlist/list.go @@ -180,7 +180,7 @@ func removeDups(col []string, applyFn func(string) (string, bool)) []string { } if _, ok := ks[vv]; !ok { - quoted := shellescape.Quote(string(vv)) + quoted := shellescape.Quote(vv) uniq = append(uniq, quoted) ks[vv] = struct{}{} } From faaadb90e30c6e19ca46b5d1b3735bb6cb5db951 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Thu, 18 Jan 2024 04:31:15 +0100 Subject: [PATCH 03/11] create a package-scoped instance refactoring --- cmd/pathctl/main.go | 18 ++--- dirlist/dirlist.go | 43 ++++++++++++ dirlist/dirlist_test.go | 97 ++++++++++++++++++++++++++ {pathlist => dirlist}/list.go | 77 +++++++++++++-------- dirlist/list_internal_test.go | 34 ++++++++++ dirlist/list_test.go | 124 ++++++++++++++++++++++++++++++++++ pathlist/list_test.go | 77 --------------------- 7 files changed, 354 insertions(+), 116 deletions(-) create mode 100644 dirlist/dirlist.go create mode 100644 dirlist/dirlist_test.go rename {pathlist => dirlist}/list.go (71%) create mode 100644 dirlist/list_internal_test.go create mode 100644 dirlist/list_test.go delete mode 100644 pathlist/list_test.go diff --git a/cmd/pathctl/main.go b/cmd/pathctl/main.go index 6ca8c15..4d561a5 100644 --- a/cmd/pathctl/main.go +++ b/cmd/pathctl/main.go @@ -7,8 +7,8 @@ import ( "os" "strings" + "al.essio.dev/pkg/tools/dirlist" "al.essio.dev/pkg/tools/internal/version" - "al.essio.dev/pkg/tools/pathlist" ) const ( @@ -27,7 +27,7 @@ var ( envVar string ) -var cmdHandlers map[string]func(d pathlist.List) +var cmdHandlers map[string]func(d dirlist.List) func init() { flag.BoolVar(&helpMode, "help", false, "display this help and exit.") @@ -39,22 +39,22 @@ func init() { flag.Usage = usage flag.CommandLine.SetOutput(os.Stderr) - cmdHandlers = func() map[string]func(pathlist.List) { - hAppend := func(d pathlist.List) { + cmdHandlers = func() map[string]func(dirlist.List) { + hAppend := func(d dirlist.List) { if dropMode { d.Drop(flag.Arg(1)) } d.Append(flag.Arg(1)) } - hDrop := func(d pathlist.List) { d.Drop(flag.Arg(1)) } - hPrepend := func(d pathlist.List) { + hDrop := func(d dirlist.List) { d.Drop(flag.Arg(1)) } + hPrepend := func(d dirlist.List) { if dropMode { d.Drop(flag.Arg(1)) } d.Prepend(flag.Arg(1)) } - return map[string]func(pathlist.List){ + return map[string]func(dirlist.List){ "append": hAppend, "drop": hDrop, "prepend": hPrepend, @@ -75,7 +75,7 @@ func main() { handleHelpAndVersionModes() - dirs := pathlist.New() + dirs := dirlist.New() dirs.LoadEnv(envVar) if flag.NArg() < 1 { @@ -91,7 +91,7 @@ func main() { } } -func printPathList(d pathlist.List) { +func printPathList(d dirlist.List) { var sb = strings.Builder{} sb.Reset() diff --git a/dirlist/dirlist.go b/dirlist/dirlist.go new file mode 100644 index 0000000..d728a91 --- /dev/null +++ b/dirlist/dirlist.go @@ -0,0 +1,43 @@ +package dirlist + +var ( + dList List +) + +func init() { + dList = New() +} + +func Reset() { dList.Reset() } + +func Contains(p string) bool { + return dList.Contains(quoteAndClean(p)) +} + +func Load(s string) { + dList.Load(s) +} + +func LoadEnv(s string) { + dList.LoadEnv(s) +} + +func Prepend(p string) { + dList.Prepend(p) +} + +func Append(p string) { + dList.Append(p) +} + +func Drop(p string) { + dList.Drop(p) +} + +func Slice() []string { + return dList.Slice() +} + +func String() string { + return dList.String() +} diff --git a/dirlist/dirlist_test.go b/dirlist/dirlist_test.go new file mode 100644 index 0000000..47fd3ae --- /dev/null +++ b/dirlist/dirlist_test.go @@ -0,0 +1,97 @@ +package dirlist + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestAppend(t *testing.T) { + Reset() + require.Equal(t, "", String()) + for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { + Append(p) + } + + require.Equal(t, "/var:/bin", String()) + Prepend("/bin///") + require.Equal(t, "/var:/bin", String()) +} + +func TestContains(t *testing.T) { + Reset() + Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") + require.False(t, Contains("/ur/local/sbin")) + require.False(t, Contains("/ur/local////sbin/")) + require.True(t, Contains("/sbin")) + require.True(t, Contains("///sbin//")) + +} + +func TestDrop(t *testing.T) { + Reset() + Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") + require.Equal(t, Slice(), []string{"/opt/local/bin", "/usr/local/bin", "/sbin", "/bin", "/var"}) + Drop("/opt/local/bin") + Drop("/opt/local/bin") + Drop("/opt/local/bin") + Drop("/usr/local/bin") + Drop("/var") + require.NotEqual(t, "", String()) + Drop("/sbin") + Drop("/bin") + require.False(t, Contains("/bin")) + require.Equal(t, "", String()) + + Reset() + require.NotPanics(t, func() { Drop("") }) +} + +func TestLoadEnv(t *testing.T) { + tests := []struct { + name string + val string + want string + }{ + {"empty", "", ""}, + } + for i, tt := range tests { + tt2 := tt + t.Run(tt2.name, func(t *testing.T) { + envvar := fmt.Sprintf("%s_%d_VAR", t.Name(), i) + Reset() + t.Setenv(envvar, tt.val) + LoadEnv(envvar) + require.Equal(t, tt2.want, String()) + }) + } +} + +func TestPrepend(t *testing.T) { + Reset() + require.Equal(t, "", String()) + for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { + Append(p) + } + + require.Equal(t, "/var:/bin", String()) + Prepend("/bin///") + require.Equal(t, "/var:/bin", String()) +} + +func TestSlice(t *testing.T) { + Reset() + require.Equal(t, 0, len(Slice())) + Prepend("/usr/bin") + Append("/bin") + require.Equal(t, []string{"/usr/bin", "/bin"}, Slice()) +} + +func TestString(t *testing.T) { + Reset() + require.Equal(t, "", String()) + Prepend("/usr/bin") + Append("/bin") + require.Equal(t, "/usr/bin:/bin", String()) +} diff --git a/pathlist/list.go b/dirlist/list.go similarity index 71% rename from pathlist/list.go rename to dirlist/list.go index 3a0730e..02b28af 100644 --- a/pathlist/list.go +++ b/dirlist/list.go @@ -1,8 +1,9 @@ -// Package pathlist implements functions to manipulate PATH-like +// Package dirlist implements functions to manipulate PATH-like // environment variables. -package pathlist +package dirlist import ( + "fmt" "os" "path/filepath" "slices" @@ -22,7 +23,7 @@ type List interface { Contains(string) bool // Nil returns true if the list is emppty. - Nil() bool + // Nil() bool // Load reads the list of directories from a string. Load(string) @@ -62,16 +63,16 @@ func New() List { } func (d *dirList) Contains(p string) bool { - return slices.Contains(d.lst, p) + return slices.Contains(d.lst, quoteAndClean(p)) } func (d *dirList) Reset() { d.init() } -func (d *dirList) Nil() bool { - return d.lst == nil || len(d.lst) == 0 -} +// func (d *dirList) Nil() bool { +// return d.l == nil || len(d.l) == 0 +// } func (d *dirList) Load(s string) { d.src = s @@ -82,26 +83,25 @@ func (d *dirList) LoadEnv(s string) { d.Load(os.Getenv(s)) } -func (d *dirList) Slice() []string { - if d.Nil() { - return []string{} +func (d *dirList) Slice() (dst []string) { + if len(d.lst) == 0 { + return } - dst := make([]string, len(d.lst)) - n := copy(dst, d.lst) - if n != len(d.lst) { - panic("couldn't copy the list") + dst = make([]string, len(d.lst)) + if n := copy(dst, d.lst); n == len(d.lst) { + return dst } - return dst + panic("couldn't copy the list") } func (d *dirList) String() string { - if !d.Nil() { - return strings.Join(d.lst, string(filepath.ListSeparator)) + if len(d.lst) == 0 { + return "" } - return "" + return strings.Join(d.lst, string(filepath.ListSeparator)) } func (d *dirList) load() { @@ -110,7 +110,7 @@ func (d *dirList) load() { func (d *dirList) Append(path string) { p := quoteAndClean(path) - if d.Nil() { + if len(d.lst) == 0 { d.lst = []string{p} return } @@ -121,9 +121,10 @@ func (d *dirList) Append(path string) { } func (d *dirList) Drop(path string) { - if d.Nil() { + if len(d.lst) == 0 { return } + p := quoteAndClean(path) if idx := slices.Index(d.lst, p); idx != -1 { @@ -133,7 +134,7 @@ func (d *dirList) Drop(path string) { func (d *dirList) Prepend(path string) { p := quoteAndClean(path) - if d.Nil() { + if len(d.lst) == 0 { d.lst = []string{p} return } @@ -149,12 +150,16 @@ func (d *dirList) init() { } func (d *dirList) cleanPathVar() []string { - if d.src == "" { + return cleanPathVar(d.src) +} + +func cleanPathVar(src string) []string { + if src == "" { return nil } - pthSlice := filepath.SplitList(d.src) - if pthSlice == nil { + pthSlice := filepath.SplitList(src) + if len(pthSlice) == 0 { return nil } @@ -163,8 +168,13 @@ func (d *dirList) cleanPathVar() []string { func (d *dirList) clone(o *dirList) *dirList { o.src = d.src - o.lst = make([]string, len(d.lst)) - copy(o.lst, d.lst) + + n := len(d.lst) + o.lst = make([]string, n) + + if m := copy(o.lst, d.lst); n != m { + panic(fmt.Sprintf("copy: expected %d items, got %d", n, m)) + } return o } @@ -190,12 +200,19 @@ func removeDups(col []string, applyFn func(string) (string, bool)) []string { } var filterEmptyStrings = func(s string) (string, bool) { - clean := filepath.Clean(s) - if clean != "" { - return clean, true + if strings.TrimSpace(s) == "" { + return s, false } - return clean, false + // I removed the following because filepath.Clean() + // never returns "". + // + // clean := filepath.Clean(s) + // if clean == "" { + // return clean, false + // } + + return filepath.Clean(s), true } func quoteAndClean(s string) string { diff --git a/dirlist/list_internal_test.go b/dirlist/list_internal_test.go new file mode 100644 index 0000000..d7b9b5d --- /dev/null +++ b/dirlist/list_internal_test.go @@ -0,0 +1,34 @@ +package dirlist + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_newDirList(t *testing.T) { + d := new(dirList) + d.Reset() + require.NotNil(t, d) + require.NotNil(t, d) + require.Equal(t, "", d.String()) + + d.Append("/ciao") + require.Equal(t, "/ciao", d.String()) + + d1 := new(dirList) + d1.Reset() + d1 = d.clone(d1) + d.Append("/var") + require.NotEqual(t, &d, &d1) +} + +func Test_removeDups(t *testing.T) { + require.Equal(t, + []string{"alpha", "bravo", "charlie", "."}, + removeDups( + []string{"alpha", "bravo", "charlie", "bravo", " ", "."}, + filterEmptyStrings, + ), + ) +} diff --git a/dirlist/list_test.go b/dirlist/list_test.go new file mode 100644 index 0000000..11ffe2c --- /dev/null +++ b/dirlist/list_test.go @@ -0,0 +1,124 @@ +package dirlist_test + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + "al.essio.dev/pkg/tools/dirlist" +) + +func TestList_Append(t *testing.T) { + d := dirlist.New() + require.Equal(t, "", d.String()) + for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { + d.Append(p) + } + + require.Equal(t, "/var:/bin", d.String()) + d.Prepend("/bin///") + require.Equal(t, "/var:/bin", d.String()) + + // require.Equal(t, 2, d.Append("/var"), ("/usr/local/bin", "/opt/local/bin")) + // require.Equal(t, "/var:/bin:/usr/local/bin:/opt/local/bin", d.String()) +} + +func TestList_Prepend(t *testing.T) { + d := dirlist.New() + dirs := []string{ + "/var", "/var", "/bin", "/bin/", + } + + for _, dir := range dirs { + d.Prepend(dir) + } + + d.Append("/bin/") + + require.Equal(t, "/bin:/var", d.String()) + d.Prepend("/sbin") + require.Equal(t, d.Slice(), []string{"/sbin", "/bin", "/var"}) + require.Equal(t, d.String(), "/sbin:/bin:/var") + d.Prepend("/var") + d.Prepend("/usr/local/bin") + d.Prepend("/opt/local/bin") + require.Equal(t, d.String(), "/opt/local/bin:/usr/local/bin:/sbin:/bin:/var") +} + +func TestList_Drop(t *testing.T) { + d := dirlist.New() + d.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") + require.Equal(t, d.Slice(), []string{"/opt/local/bin", "/usr/local/bin", "/sbin", "/bin", "/var"}) + d.Drop("/opt/local/bin") + d.Drop("/opt/local/bin") + d.Drop("/opt/local/bin") + d.Drop("/usr/local/bin") + d.Drop("/var") + require.NotEqual(t, "", d.String()) + d.Drop("/sbin") + d.Drop("/bin") + require.Equal(t, "", d.String()) + + require.NotPanics(t, func() { dirlist.New().Drop("") }) +} + +func TestList_Reset(t *testing.T) { + d1 := dirlist.New() + require.Equal(t, "", d1.String()) + d1.Reset() + require.Equal(t, "", d1.String()) + + d2 := dirlist.New() + d2.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") + require.Equal(t, 5, len(d2.Slice())) + d2.Reset() + require.Equal(t, 0, len(d2.Slice())) + require.Equal(t, "", d2.String()) +} + +func TestList_Contains(t *testing.T) { + d := dirlist.New() + d.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") + require.False(t, d.Contains("/ur/local/sbin")) + require.False(t, d.Contains("/ur/local////sbin/")) + require.True(t, d.Contains("/sbin")) + require.True(t, d.Contains("///sbin//")) + +} + +func TestList_LoadEnv(t *testing.T) { + tests := []struct { + name string + val string + want string + }{ + {"empty", "", ""}, + } + for i, tt := range tests { + tt2 := tt + t.Run(tt2.name, func(t *testing.T) { + envvar := fmt.Sprintf("%s_%d_VAR", t.Name(), i) + d := dirlist.New() + t.Setenv(envvar, tt.val) + d.LoadEnv(envvar) + require.Equal(t, tt2.want, d.String()) + }) + } +} + +func TestList_Slice(t *testing.T) { + d := dirlist.New() + require.Equal(t, 0, len(d.Slice())) + d.Prepend("/usr/bin") + d.Append("/bin") + require.Equal(t, []string{"/usr/bin", "/bin"}, d.Slice()) +} + +func TestList_String(t *testing.T) { + d := dirlist.New() + require.Equal(t, "", d.String()) + d.Prepend("/usr/bin") + d.Append("/bin") + require.Equal(t, "/usr/bin:/bin", d.String()) +} diff --git a/pathlist/list_test.go b/pathlist/list_test.go deleted file mode 100644 index f2c8576..0000000 --- a/pathlist/list_test.go +++ /dev/null @@ -1,77 +0,0 @@ -package pathlist - -import ( - "testing" - - "github.com/stretchr/testify/require" -) - -func Test_newDirList(t *testing.T) { - d := new(dirList) - d.Reset() - require.NotNil(t, d) - require.True(t, d.Nil()) - require.Equal(t, "", d.String()) - - d.Append("/ciao") - require.Equal(t, "/ciao", d.String()) - - d1 := new(dirList) - d1.Reset() - d1 = d.clone(d1) - d.Append("/var") - require.NotEqual(t, &d, &d1) -} - -func Test_DirList_Append(t *testing.T) { - d := New() - require.True(t, d.Nil()) - for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { - d.Append(p) - } - - require.Equal(t, "/var:/bin", d.String()) - d.Prepend("/bin///") - require.Equal(t, "/var:/bin", d.String()) - - // require.Equal(t, 2, d.Append("/var"), ("/usr/local/bin", "/opt/local/bin")) - // require.Equal(t, "/var:/bin:/usr/local/bin:/opt/local/bin", d.String()) -} - -func Test_DirList_Prepend(t *testing.T) { - d := New() - dirs := []string{ - "/var", "/var", "/bin", "/bin/", - } - - for _, dir := range dirs { - d.Prepend(dir) - } - - d.Append("/bin/") - - require.Equal(t, "/bin:/var", d.String()) - d.Prepend("/sbin") - require.Equal(t, d.Slice(), []string{"/sbin", "/bin", "/var"}) - require.Equal(t, d.String(), "/sbin:/bin:/var") - d.Prepend("/var") - d.Prepend("/usr/local/bin") - d.Prepend("/opt/local/bin") - require.Equal(t, d.String(), "/opt/local/bin:/usr/local/bin:/sbin:/bin:/var") -} - -func Test_DirList_Drop(t *testing.T) { - d := New() - d.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") - require.Equal(t, d.Slice(), []string{"/opt/local/bin", "/usr/local/bin", "/sbin", "/bin", "/var"}) - d.Drop("/opt/local/bin") - d.Drop("/opt/local/bin") - d.Drop("/opt/local/bin") - d.Drop("/usr/local/bin") - d.Drop("/var") - require.False(t, d.Nil()) - d.Drop("/sbin") - d.Drop("/bin") - require.Equal(t, "", d.String()) - require.True(t, d.Nil()) -} From 57af39128fcb98cd567009cf272de73bb878a1e9 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 09:50:29 +0100 Subject: [PATCH 04/11] fix quoting and cleaning --- dirlist/dirlist.go | 2 +- dirlist/list.go | 18 ++++++------------ dirlist/list_test.go | 7 +++++++ 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/dirlist/dirlist.go b/dirlist/dirlist.go index d728a91..0ea1a1d 100644 --- a/dirlist/dirlist.go +++ b/dirlist/dirlist.go @@ -11,7 +11,7 @@ func init() { func Reset() { dList.Reset() } func Contains(p string) bool { - return dList.Contains(quoteAndClean(p)) + return dList.Contains(p) } func Load(s string) { diff --git a/dirlist/list.go b/dirlist/list.go index 02b28af..d4ae17f 100644 --- a/dirlist/list.go +++ b/dirlist/list.go @@ -8,8 +8,6 @@ import ( "path/filepath" "slices" "strings" - - "github.com/alessio/shellescape" ) // List builds a list of directories by parsing PATH-like variables @@ -63,7 +61,7 @@ func New() List { } func (d *dirList) Contains(p string) bool { - return slices.Contains(d.lst, quoteAndClean(p)) + return slices.Contains(d.lst, filepath.Clean(p)) } func (d *dirList) Reset() { @@ -109,7 +107,7 @@ func (d *dirList) load() { } func (d *dirList) Append(path string) { - p := quoteAndClean(path) + p := filepath.Clean(path) if len(d.lst) == 0 { d.lst = []string{p} return @@ -125,7 +123,7 @@ func (d *dirList) Drop(path string) { return } - p := quoteAndClean(path) + p := filepath.Clean(path) if idx := slices.Index(d.lst, p); idx != -1 { d.lst = slices.Delete(d.lst, idx, idx+1) @@ -133,7 +131,7 @@ func (d *dirList) Drop(path string) { } func (d *dirList) Prepend(path string) { - p := quoteAndClean(path) + p := filepath.Clean(path) if len(d.lst) == 0 { d.lst = []string{p} return @@ -189,9 +187,9 @@ func removeDups(col []string, applyFn func(string) (string, bool)) []string { continue } + vv = filepath.Join(filepath.Split(vv)) if _, ok := ks[vv]; !ok { - quoted := shellescape.Quote(vv) - uniq = append(uniq, quoted) + uniq = append(uniq, vv) ks[vv] = struct{}{} } } @@ -214,7 +212,3 @@ var filterEmptyStrings = func(s string) (string, bool) { return filepath.Clean(s), true } - -func quoteAndClean(s string) string { - return shellescape.Quote(filepath.Clean(s)) -} diff --git a/dirlist/list_test.go b/dirlist/list_test.go index 11ffe2c..d5347c8 100644 --- a/dirlist/list_test.go +++ b/dirlist/list_test.go @@ -61,6 +61,13 @@ func TestList_Drop(t *testing.T) { require.Equal(t, "", d.String()) require.NotPanics(t, func() { dirlist.New().Drop("") }) + + d1 := dirlist.New() + d1.Load(`/Library/Application Support:/Library/Application Support/`) + require.Equal(t, []string{"/Library/Application Support"}, d1.Slice()) + require.True(t, d1.Contains("/Library/Application Support")) + d1.Drop("/Library/Application Support") + require.False(t, d1.Contains("/Library/Application Support")) } func TestList_Reset(t *testing.T) { From 3d54ccca3849e62e6f4c52a783077eb988651068 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 09:55:16 +0100 Subject: [PATCH 05/11] improve comment --- dirlist/list.go | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/dirlist/list.go b/dirlist/list.go index d4ae17f..f92ef9e 100644 --- a/dirlist/list.go +++ b/dirlist/list.go @@ -202,13 +202,7 @@ var filterEmptyStrings = func(s string) (string, bool) { return s, false } - // I removed the following because filepath.Clean() - // never returns "". - // - // clean := filepath.Clean(s) - // if clean == "" { - // return clean, false - // } - + // It'd pointless to check filepath.Clean()'s return + // value's nil-ness as it would never be "". return filepath.Clean(s), true } From bc1ce02dff825d9b62d0765528e2cf6adfd4296a Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 10:07:20 +0100 Subject: [PATCH 06/11] cosmetic fixes --- cmd/pathctl/main.go | 56 ++++++++++++++++++++++++--------------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/cmd/pathctl/main.go b/cmd/pathctl/main.go index 4d561a5..0c25a39 100644 --- a/cmd/pathctl/main.go +++ b/cmd/pathctl/main.go @@ -12,14 +12,14 @@ import ( ) const ( - programme = "pathctl" + program = "pathctl" ) var ( helpMode bool versionMode bool listMode bool - noprefixMode bool + noPrefixMode bool dropMode bool ) @@ -33,43 +33,29 @@ func init() { flag.BoolVar(&helpMode, "help", false, "display this help and exit.") flag.BoolVar(&versionMode, "version", false, "output version information and exit.") flag.BoolVar(&dropMode, "D", false, "drop the path before adding it again to the list.") - flag.BoolVar(&noprefixMode, "noprefix", false, "output the variable contents only.") + flag.BoolVar(&noPrefixMode, "noprefix", false, "output the variable contents only.") flag.BoolVar(&listMode, "L", false, "use a newline character as path list separator.") flag.StringVar(&envVar, "E", "PATH", "input environment variable.") flag.Usage = usage flag.CommandLine.SetOutput(os.Stderr) cmdHandlers = func() map[string]func(dirlist.List) { - hAppend := func(d dirlist.List) { - if dropMode { - d.Drop(flag.Arg(1)) - } - d.Append(flag.Arg(1)) - } - hDrop := func(d dirlist.List) { d.Drop(flag.Arg(1)) } - hPrepend := func(d dirlist.List) { - if dropMode { - d.Drop(flag.Arg(1)) - } - d.Prepend(flag.Arg(1)) - } - return map[string]func(dirlist.List){ - "append": hAppend, - "drop": hDrop, - "prepend": hPrepend, + "append": cmdHandlerAppend, + "drop": cmdHandlerDrop, + "prepend": cmdHandlerPrepend, // aliases - "a": hAppend, - "d": hDrop, - "p": hPrepend, + "a": cmdHandlerAppend, + "d": cmdHandlerDrop, + "p": cmdHandlerPrepend, } }() } func main() { log.SetFlags(0) - log.SetPrefix(fmt.Sprintf("%s: ", programme)) + log.SetPrefix(fmt.Sprintf("%s: ", program)) log.SetOutput(os.Stderr) flag.Parse() @@ -95,7 +81,7 @@ func printPathList(d dirlist.List) { var sb = strings.Builder{} sb.Reset() - printPrefix := !noprefixMode + printPrefix := !noPrefixMode switch { case listMode: @@ -138,7 +124,7 @@ Commands: prepend, p prepend a path to the list. Options: -`, programme) +`, program) _, _ = fmt.Fprintln(os.Stderr, s) flag.PrintDefaults() @@ -152,3 +138,21 @@ element of the path list. If COMMAND is not provided, it prints the contents of the PATH environment variable.`) } + +func cmdHandlerAppend(d dirlist.List) { + if dropMode { + d.Drop(flag.Arg(1)) + } + d.Append(flag.Arg(1)) +} + +func cmdHandlerDrop(d dirlist.List) { + d.Drop(flag.Arg(1)) +} + +func cmdHandlerPrepend(d dirlist.List) { + if dropMode { + d.Drop(flag.Arg(1)) + } + d.Prepend(flag.Arg(1)) +} From 0dbdbe3287e13a431c7b409b3ad907fe3bdc40e5 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 10:12:36 +0100 Subject: [PATCH 07/11] Improve tests --- dirlist/list.go | 4 ---- dirlist/list_internal_test.go | 8 +++++--- dirlist/list_test.go | 3 --- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/dirlist/list.go b/dirlist/list.go index f92ef9e..7836a10 100644 --- a/dirlist/list.go +++ b/dirlist/list.go @@ -68,10 +68,6 @@ func (d *dirList) Reset() { d.init() } -// func (d *dirList) Nil() bool { -// return d.l == nil || len(d.l) == 0 -// } - func (d *dirList) Load(s string) { d.src = s d.load() diff --git a/dirlist/list_internal_test.go b/dirlist/list_internal_test.go index d7b9b5d..603d406 100644 --- a/dirlist/list_internal_test.go +++ b/dirlist/list_internal_test.go @@ -10,17 +10,19 @@ func Test_newDirList(t *testing.T) { d := new(dirList) d.Reset() require.NotNil(t, d) - require.NotNil(t, d) require.Equal(t, "", d.String()) - d.Append("/ciao") - require.Equal(t, "/ciao", d.String()) + d.Append("/sbin") + require.Equal(t, "/sbin", d.String()) d1 := new(dirList) d1.Reset() d1 = d.clone(d1) d.Append("/var") require.NotEqual(t, &d, &d1) + d1.Prepend("/usr/bin") + d1.Append("/usr/local/bin") + require.Equal(t, "/usr/bin:/sbin:/usr/local/bin", d1.String()) } func Test_removeDups(t *testing.T) { diff --git a/dirlist/list_test.go b/dirlist/list_test.go index d5347c8..4038996 100644 --- a/dirlist/list_test.go +++ b/dirlist/list_test.go @@ -19,9 +19,6 @@ func TestList_Append(t *testing.T) { require.Equal(t, "/var:/bin", d.String()) d.Prepend("/bin///") require.Equal(t, "/var:/bin", d.String()) - - // require.Equal(t, 2, d.Append("/var"), ("/usr/local/bin", "/opt/local/bin")) - // require.Equal(t, "/var:/bin:/usr/local/bin:/opt/local/bin", d.String()) } func TestList_Prepend(t *testing.T) { From dad28ef4d93284b0754c48930caf7da77d0aacee Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 10:14:28 +0100 Subject: [PATCH 08/11] remove package-scoped stuff --- dirlist/dirlist.go | 43 ------------------ dirlist/dirlist_test.go | 97 ----------------------------------------- 2 files changed, 140 deletions(-) delete mode 100644 dirlist/dirlist.go delete mode 100644 dirlist/dirlist_test.go diff --git a/dirlist/dirlist.go b/dirlist/dirlist.go deleted file mode 100644 index 0ea1a1d..0000000 --- a/dirlist/dirlist.go +++ /dev/null @@ -1,43 +0,0 @@ -package dirlist - -var ( - dList List -) - -func init() { - dList = New() -} - -func Reset() { dList.Reset() } - -func Contains(p string) bool { - return dList.Contains(p) -} - -func Load(s string) { - dList.Load(s) -} - -func LoadEnv(s string) { - dList.LoadEnv(s) -} - -func Prepend(p string) { - dList.Prepend(p) -} - -func Append(p string) { - dList.Append(p) -} - -func Drop(p string) { - dList.Drop(p) -} - -func Slice() []string { - return dList.Slice() -} - -func String() string { - return dList.String() -} diff --git a/dirlist/dirlist_test.go b/dirlist/dirlist_test.go deleted file mode 100644 index 47fd3ae..0000000 --- a/dirlist/dirlist_test.go +++ /dev/null @@ -1,97 +0,0 @@ -package dirlist - -import ( - "fmt" - "testing" - - "github.com/stretchr/testify/require" -) - -func TestAppend(t *testing.T) { - Reset() - require.Equal(t, "", String()) - for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { - Append(p) - } - - require.Equal(t, "/var:/bin", String()) - Prepend("/bin///") - require.Equal(t, "/var:/bin", String()) -} - -func TestContains(t *testing.T) { - Reset() - Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") - require.False(t, Contains("/ur/local/sbin")) - require.False(t, Contains("/ur/local////sbin/")) - require.True(t, Contains("/sbin")) - require.True(t, Contains("///sbin//")) - -} - -func TestDrop(t *testing.T) { - Reset() - Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") - require.Equal(t, Slice(), []string{"/opt/local/bin", "/usr/local/bin", "/sbin", "/bin", "/var"}) - Drop("/opt/local/bin") - Drop("/opt/local/bin") - Drop("/opt/local/bin") - Drop("/usr/local/bin") - Drop("/var") - require.NotEqual(t, "", String()) - Drop("/sbin") - Drop("/bin") - require.False(t, Contains("/bin")) - require.Equal(t, "", String()) - - Reset() - require.NotPanics(t, func() { Drop("") }) -} - -func TestLoadEnv(t *testing.T) { - tests := []struct { - name string - val string - want string - }{ - {"empty", "", ""}, - } - for i, tt := range tests { - tt2 := tt - t.Run(tt2.name, func(t *testing.T) { - envvar := fmt.Sprintf("%s_%d_VAR", t.Name(), i) - Reset() - t.Setenv(envvar, tt.val) - LoadEnv(envvar) - require.Equal(t, tt2.want, String()) - }) - } -} - -func TestPrepend(t *testing.T) { - Reset() - require.Equal(t, "", String()) - for _, p := range []string{"/var", "/var", "/bin", "/bin/", "/bin///"} { - Append(p) - } - - require.Equal(t, "/var:/bin", String()) - Prepend("/bin///") - require.Equal(t, "/var:/bin", String()) -} - -func TestSlice(t *testing.T) { - Reset() - require.Equal(t, 0, len(Slice())) - Prepend("/usr/bin") - Append("/bin") - require.Equal(t, []string{"/usr/bin", "/bin"}, Slice()) -} - -func TestString(t *testing.T) { - Reset() - require.Equal(t, "", String()) - Prepend("/usr/bin") - Append("/bin") - require.Equal(t, "/usr/bin:/bin", String()) -} From 700cab8a1d55b3e2073f94b295a08b188b9e613d Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 12:06:03 +0100 Subject: [PATCH 09/11] Remove commented out function --- dirlist/list.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/dirlist/list.go b/dirlist/list.go index 7836a10..cb47469 100644 --- a/dirlist/list.go +++ b/dirlist/list.go @@ -20,9 +20,6 @@ type List interface { // Contains returns true if the list contains the path. Contains(string) bool - // Nil returns true if the list is emppty. - // Nil() bool - // Load reads the list of directories from a string. Load(string) From dcc487657679f9f6989a0ae4eb5c0801996153bd Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 12:11:46 +0100 Subject: [PATCH 10/11] refactor tests --- dirlist/list_test.go | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/dirlist/list_test.go b/dirlist/list_test.go index 4038996..0aa1f70 100644 --- a/dirlist/list_test.go +++ b/dirlist/list_test.go @@ -46,7 +46,6 @@ func TestList_Prepend(t *testing.T) { func TestList_Drop(t *testing.T) { d := dirlist.New() d.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") - require.Equal(t, d.Slice(), []string{"/opt/local/bin", "/usr/local/bin", "/sbin", "/bin", "/var"}) d.Drop("/opt/local/bin") d.Drop("/opt/local/bin") d.Drop("/opt/local/bin") @@ -61,21 +60,16 @@ func TestList_Drop(t *testing.T) { d1 := dirlist.New() d1.Load(`/Library/Application Support:/Library/Application Support/`) - require.Equal(t, []string{"/Library/Application Support"}, d1.Slice()) - require.True(t, d1.Contains("/Library/Application Support")) d1.Drop("/Library/Application Support") require.False(t, d1.Contains("/Library/Application Support")) } func TestList_Reset(t *testing.T) { d1 := dirlist.New() - require.Equal(t, "", d1.String()) d1.Reset() - require.Equal(t, "", d1.String()) d2 := dirlist.New() d2.Load("/opt/local/bin:/usr/local/bin:/sbin:/bin:/var:/bin") - require.Equal(t, 5, len(d2.Slice())) d2.Reset() require.Equal(t, 0, len(d2.Slice())) require.Equal(t, "", d2.String()) @@ -88,7 +82,6 @@ func TestList_Contains(t *testing.T) { require.False(t, d.Contains("/ur/local////sbin/")) require.True(t, d.Contains("/sbin")) require.True(t, d.Contains("///sbin//")) - } func TestList_LoadEnv(t *testing.T) { @@ -114,15 +107,12 @@ func TestList_LoadEnv(t *testing.T) { func TestList_Slice(t *testing.T) { d := dirlist.New() require.Equal(t, 0, len(d.Slice())) - d.Prepend("/usr/bin") - d.Append("/bin") + d.Load("/usr/bin:/bin") require.Equal(t, []string{"/usr/bin", "/bin"}, d.Slice()) } func TestList_String(t *testing.T) { d := dirlist.New() - require.Equal(t, "", d.String()) - d.Prepend("/usr/bin") - d.Append("/bin") + d.Load("/usr/bin:/bin") require.Equal(t, "/usr/bin:/bin", d.String()) } From 227532311d10582e27efc5a6279f0c2bb847dee6 Mon Sep 17 00:00:00 2001 From: Alessio Treglia Date: Mon, 22 Jan 2024 12:16:34 +0100 Subject: [PATCH 11/11] refactor tests --- dirlist/list_test.go | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/dirlist/list_test.go b/dirlist/list_test.go index 0aa1f70..5bff6d8 100644 --- a/dirlist/list_test.go +++ b/dirlist/list_test.go @@ -31,16 +31,11 @@ func TestList_Prepend(t *testing.T) { d.Prepend(dir) } - d.Append("/bin/") - - require.Equal(t, "/bin:/var", d.String()) d.Prepend("/sbin") - require.Equal(t, d.Slice(), []string{"/sbin", "/bin", "/var"}) - require.Equal(t, d.String(), "/sbin:/bin:/var") d.Prepend("/var") d.Prepend("/usr/local/bin") d.Prepend("/opt/local/bin") - require.Equal(t, d.String(), "/opt/local/bin:/usr/local/bin:/sbin:/bin:/var") + require.Equal(t, "/opt/local/bin:/usr/local/bin:/sbin:/bin:/var", d.String()) } func TestList_Drop(t *testing.T) {