From a04ea6cc6c2019af3f9dad7f286f7a08cfdda4a5 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 16 Dec 2021 17:36:40 -0500 Subject: [PATCH 1/4] spike: setup failing tests --- commands/internal/arguments/arguments.go | 40 +++++++++----- commands/internal/arguments/arguments_test.go | 53 +++++++++++++++++++ commands/internal/arguments/testdata/a.txt | 1 + commands/internal/arguments/testdata/b.txt | 1 + features/command_expand.feature | 29 ++++++++-- 5 files changed, 106 insertions(+), 18 deletions(-) create mode 100644 commands/internal/arguments/testdata/a.txt create mode 100644 commands/internal/arguments/testdata/b.txt diff --git a/commands/internal/arguments/arguments.go b/commands/internal/arguments/arguments.go index 465bd73..f79c857 100644 --- a/commands/internal/arguments/arguments.go +++ b/commands/internal/arguments/arguments.go @@ -12,28 +12,42 @@ import ( var expandArgDigitMatcher = regexp.MustCompile("^[0-9]{0,4}$") var expandArgRangeMatcher = regexp.MustCompile("^([0-9]+)-([0-9]+)$") -// EvaluateEnvironment evaluates a string of arguments and expands environment variables. +// EvaluateEnvironment evaluates a single arguments and expands environment +// variables. +// +// TODO: For scmpuff-managed position variables only (e.g. $e1, etc), the +// variable is expanded into a locatable file path, and expandRelative is true, +// it will be converted into a relative path when possible. func EvaluateEnvironment(arg string, expandRelative bool) string { - expandedArg := os.ExpandEnv(arg) - if expandRelative { - return convertToRelativeIfFilePath(expandedArg) + expanded := os.ExpandEnv(arg) + wasChanged := (expanded != arg) + if wasChanged && expandRelative { + relPath, err := convertToRelativeIfFilePath(expanded) + if err == nil { + return relPath + } } - return expandedArg + return expanded } // For a given arg, try to determine if it represents a file, and if so, convert // it to a relative filepath. // // Otherwise (or if any error conditions occur) return it unmolested. -func convertToRelativeIfFilePath(arg string) string { - if _, err := os.Stat(arg); err == nil { - wd, err1 := os.Getwd() - relPath, err2 := filepath.Rel(wd, arg) - if err1 == nil && err2 == nil { - return relPath - } +func convertToRelativeIfFilePath(arg string) (string, error) { + _, err := os.Stat(arg) + if err != nil { + return arg, err + } + wd, err := os.Getwd() + if err != nil { + return arg, err + } + relPath, err := filepath.Rel(wd, arg) + if err != nil { + return arg, err } - return arg + return relPath, nil } // Expand takes the list of arguments received from the command line and expands diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index 4ca8570..64820f5 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -1,6 +1,8 @@ package arguments import ( + "os" + "path/filepath" "reflect" "strings" "testing" @@ -46,3 +48,54 @@ func TestExpandArg(t *testing.T) { } } } + +func TestEvaluateEnvironment(t *testing.T) { + // It would be wonderful to use fstest.MapFS here and have the function rely + // upon fs.StatFS, however MapFS currently does not work with absolute paths + // at all, which makes it useless for our testing here. Bummer. So we use a + // testdata fixture instead. + /* var mockFS = fstest.MapFS{ + "/foo/a.txt": &fstest.MapFile{Mode: 0644}, + "/foo/b.txt": &fstest.MapFile{Mode: 0644}, + "/foo/bar/c.txt": &fstest.MapFile{Mode: 0644}, + "/usr/local/bin/git": &fstest.MapFile{Mode: 0777}, + } */ + wd, err := os.Getwd() + if err != nil { + t.Skip("failed to get wd, cannot test") + } + + fakegitAbsPath := filepath.Join(wd, "/testdata/bin/fakegit") + t.Setenv("SCMPUFF_GIT_CMD", fakegitAbsPath) + t.Setenv("e1", filepath.Join(wd, "/testdata/a.txt")) + t.Setenv("e2", filepath.Join(wd, "/testdata/b.txt")) + t.Setenv("FOO_USER", "not_a_file") + + t.Logf("$CWD=%v", wd) + t.Logf("$SCMPUFF_GIT_CMD=%v", os.Getenv("SCMPUFF_GIT_CMD")) + t.Logf("$e1=%v", os.Getenv("e1")) + t.Logf("$e2=%v", os.Getenv("e2")) + + tests := []struct { + name string + arg string + expandRelative bool + want string + }{ + {name: "not an env var", arg: "eee", expandRelative: false, want: "eee"}, + {name: "not file absolute", arg: "$FOO_USER", expandRelative: false, want: "not_a_file"}, + {name: "not file relative", arg: "$FOO_USER", expandRelative: true, want: "not_a_file"}, + {name: "absolute file", arg: "$e1", expandRelative: false, want: filepath.Join(wd, "/testdata/a.txt")}, + {name: "relative file", arg: "$e1", expandRelative: true, want: "testdata/a.txt"}, + {name: "path binary dont convert relative - abs", arg: "$SCMPUFF_GIT_CMD", expandRelative: false, want: fakegitAbsPath}, + {name: "path binary dont convert relative - rel", arg: "$SCMPUFF_GIT_CMD", expandRelative: true, want: fakegitAbsPath}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := EvaluateEnvironment(tt.arg, tt.expandRelative); got != tt.want { + t.Errorf("EvaluateEnvironment(%v, %v) = %v, want %v", tt.arg, tt.expandRelative, got, tt.want) + } + }) + } +} diff --git a/commands/internal/arguments/testdata/a.txt b/commands/internal/arguments/testdata/a.txt new file mode 100644 index 0000000..7898192 --- /dev/null +++ b/commands/internal/arguments/testdata/a.txt @@ -0,0 +1 @@ +a diff --git a/commands/internal/arguments/testdata/b.txt b/commands/internal/arguments/testdata/b.txt new file mode 100644 index 0000000..6178079 --- /dev/null +++ b/commands/internal/arguments/testdata/b.txt @@ -0,0 +1 @@ +b diff --git a/features/command_expand.feature b/features/command_expand.feature index 5ee0a99..3868a49 100644 --- a/features/command_expand.feature +++ b/features/command_expand.feature @@ -69,16 +69,35 @@ Feature: command expansion at command line Then the stderr should not contain anything And the output should match /git\tcommit\t-m\tfoo\\;\\ bar/ - Scenario: Allow user to specify --relative paths + @wip + Scenario Outline: Allow user to specify --relative paths Given a directory named "foo" And a directory named "foo/bar" And an empty file named "xxx.jpg" And I cd to "foo/bar" Given I override environment variable "e1" to the absolute path of "xxx.jpg" - When I successfully run `scmpuff expand 1` - Then the stdout from "scmpuff expand 1" should contain "/tmp/aruba/xxx.jpg" - When I successfully run `scmpuff expand -r -- 1` - Then the stdout from "scmpuff expand -r -- 1" should contain "../../xxx.jpg" + When I successfully run `` + Then the output should match + Examples: + | cmd | output_regex | + | scmpuff expand 1 | %r<^\S*/tmp/aruba/xxx\.jpg$> | + | scmpuff expand -r -- 1 | %r<\.\./\.\./xxx\.jpg> | + + @wip + Scenario Outline: Do not convert anything other than special variables to --relative paths + Given a directory named "foo" + And a directory named "foo/bar" + And an empty file named "fakegit" + And an empty file named "xxx.jpg" + And I cd to "foo/bar" + Given I override environment variable "SCMPUFF_GIT_CMD" to the absolute path of "fakegit" + And I override environment variable "e1" to the absolute path of "xxx.jpg" + When I successfully run `` + Then the output should match + Examples: + | cmd | output_regex | + | scmpuff expand -- $SCMPUFF_GIT_CMD checkout 1 | %r<^\S*/tmp/aruba/fakegit\tcheckout\t\S*/tmp/aruba/xxx\.jpg$> | + | scmpuff expand -r -- $SCMPUFF_GIT_CMD checkout 1 | %r<^\S*/tmp/aruba/fakegit\tcheckout\t\.\./\.\./xxx\.jpg$> | Scenario: Don't trim empty string args when expanding command There are certain situations where someone would want to actually pass an From 00361b132ced3d3050de11267eab60f62db8606a Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 16 Dec 2021 17:58:50 -0500 Subject: [PATCH 2/4] fix(expand): only convert to rel path on special vars --- commands/internal/arguments/arguments.go | 15 +++++++++------ features/command_expand.feature | 2 -- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/commands/internal/arguments/arguments.go b/commands/internal/arguments/arguments.go index f79c857..fdddb01 100644 --- a/commands/internal/arguments/arguments.go +++ b/commands/internal/arguments/arguments.go @@ -9,19 +9,22 @@ import ( "strconv" ) -var expandArgDigitMatcher = regexp.MustCompile("^[0-9]{0,4}$") -var expandArgRangeMatcher = regexp.MustCompile("^([0-9]+)-([0-9]+)$") +var ( + expandArgDigitMatcher = regexp.MustCompile("^[0-9]{0,4}$") + expandArgRangeMatcher = regexp.MustCompile("^([0-9]+)-([0-9]+)$") + managedEnvVar = regexp.MustCompile(`^\$e\d+$`) +) // EvaluateEnvironment evaluates a single arguments and expands environment // variables. // -// TODO: For scmpuff-managed position variables only (e.g. $e1, etc), the -// variable is expanded into a locatable file path, and expandRelative is true, -// it will be converted into a relative path when possible. +// For scmpuff-managed position variables only (e.g. $e1, etc), the variable is +// expanded into a locatable file path, and expandRelative is true, it will be +// converted into a relative path when possible. func EvaluateEnvironment(arg string, expandRelative bool) string { expanded := os.ExpandEnv(arg) wasChanged := (expanded != arg) - if wasChanged && expandRelative { + if wasChanged && expandRelative && managedEnvVar.MatchString(arg) { relPath, err := convertToRelativeIfFilePath(expanded) if err == nil { return relPath diff --git a/features/command_expand.feature b/features/command_expand.feature index 3868a49..8435ed7 100644 --- a/features/command_expand.feature +++ b/features/command_expand.feature @@ -69,7 +69,6 @@ Feature: command expansion at command line Then the stderr should not contain anything And the output should match /git\tcommit\t-m\tfoo\\;\\ bar/ - @wip Scenario Outline: Allow user to specify --relative paths Given a directory named "foo" And a directory named "foo/bar" @@ -83,7 +82,6 @@ Feature: command expansion at command line | scmpuff expand 1 | %r<^\S*/tmp/aruba/xxx\.jpg$> | | scmpuff expand -r -- 1 | %r<\.\./\.\./xxx\.jpg> | - @wip Scenario Outline: Do not convert anything other than special variables to --relative paths Given a directory named "foo" And a directory named "foo/bar" From 3f4adaa0aa5cb1e6e4b595e047d448f76df43dfc Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 16 Dec 2021 18:05:29 -0500 Subject: [PATCH 3/4] fix: windows compatible path in testcases --- commands/internal/arguments/arguments_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index 64820f5..5b30fb6 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -93,7 +93,8 @@ func TestEvaluateEnvironment(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - if got := EvaluateEnvironment(tt.arg, tt.expandRelative); got != tt.want { + // filepath.ToSlash for windows compatibility since our test case wants assume output on unix like systems + if got := filepath.ToSlash(EvaluateEnvironment(tt.arg, tt.expandRelative)); got != tt.want { t.Errorf("EvaluateEnvironment(%v, %v) = %v, want %v", tt.arg, tt.expandRelative, got, tt.want) } }) From 64f2adda82a3ab334f66c91a9ff4ab591208e28a Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Thu, 16 Dec 2021 18:17:23 -0500 Subject: [PATCH 4/4] fix: windows compatible test paths, take two --- commands/internal/arguments/arguments_test.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index 5b30fb6..f8a5df9 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -65,10 +65,10 @@ func TestEvaluateEnvironment(t *testing.T) { t.Skip("failed to get wd, cannot test") } - fakegitAbsPath := filepath.Join(wd, "/testdata/bin/fakegit") + fakegitAbsPath := filepath.Join(wd, "testdata", "bin", "fakegit") t.Setenv("SCMPUFF_GIT_CMD", fakegitAbsPath) - t.Setenv("e1", filepath.Join(wd, "/testdata/a.txt")) - t.Setenv("e2", filepath.Join(wd, "/testdata/b.txt")) + t.Setenv("e1", filepath.Join(wd, "testdata", "a.txt")) + t.Setenv("e2", filepath.Join(wd, "testdata", "b.txt")) t.Setenv("FOO_USER", "not_a_file") t.Logf("$CWD=%v", wd) @@ -85,16 +85,15 @@ func TestEvaluateEnvironment(t *testing.T) { {name: "not an env var", arg: "eee", expandRelative: false, want: "eee"}, {name: "not file absolute", arg: "$FOO_USER", expandRelative: false, want: "not_a_file"}, {name: "not file relative", arg: "$FOO_USER", expandRelative: true, want: "not_a_file"}, - {name: "absolute file", arg: "$e1", expandRelative: false, want: filepath.Join(wd, "/testdata/a.txt")}, - {name: "relative file", arg: "$e1", expandRelative: true, want: "testdata/a.txt"}, + {name: "absolute file", arg: "$e1", expandRelative: false, want: filepath.Join(wd, "testdata", "a.txt")}, + {name: "relative file", arg: "$e1", expandRelative: true, want: filepath.FromSlash("testdata/a.txt")}, {name: "path binary dont convert relative - abs", arg: "$SCMPUFF_GIT_CMD", expandRelative: false, want: fakegitAbsPath}, {name: "path binary dont convert relative - rel", arg: "$SCMPUFF_GIT_CMD", expandRelative: true, want: fakegitAbsPath}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // filepath.ToSlash for windows compatibility since our test case wants assume output on unix like systems - if got := filepath.ToSlash(EvaluateEnvironment(tt.arg, tt.expandRelative)); got != tt.want { + if got := EvaluateEnvironment(tt.arg, tt.expandRelative); got != tt.want { t.Errorf("EvaluateEnvironment(%v, %v) = %v, want %v", tt.arg, tt.expandRelative, got, tt.want) } })