From 31938b20b7038c078581aea0f8d5000c4daa933b Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Sat, 26 Mar 2022 11:48:13 -0400 Subject: [PATCH 1/4] failing test case for issue #69 --- commands/internal/arguments/arguments_test.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index f8a5df9..3593462 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -15,6 +15,10 @@ var testExpandCases = []struct { {"1 3 7", "$e1 $e3 $e7"}, {"1-3 6", "$e1 $e2 $e3 $e6"}, {"seven 2-5 1", "seven $e2 $e3 $e4 $e5 $e1"}, + // Test cases for https://github.com/mroth/scmpuff/issues/69 + {"log -1 1", "log -1 $e1"}, + {"log -n1 2", "log -n1 $e2"}, + {"log -n 1 2", "log -n 1 $e2"}, } func TestExpand(t *testing.T) { @@ -25,7 +29,7 @@ func TestExpand(t *testing.T) { expected := strings.Split(tc.expected, " ") actual := Expand(args) if !reflect.DeepEqual(actual, expected) { - t.Fatalf("ExpandArgs(%v): expected %v, actual %v", tc.args, expected, actual) + t.Errorf("ExpandArgs(%v): expected %v, actual %v", tc.args, expected, actual) } } } @@ -44,7 +48,7 @@ func TestExpandArg(t *testing.T) { for _, tc := range testExpandArgCases { actual := expandArg(tc.arg) if !reflect.DeepEqual(actual, tc.expected) { - t.Fatalf("ExpandArg(%v): expected %v, actual %v", tc.arg, tc.expected, actual) + t.Errorf("ExpandArg(%v): expected %v, actual %v", tc.arg, tc.expected, actual) } } } From 9d607349ec6d7a5075304fc388da0cf1029ee468 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Sat, 26 Mar 2022 12:41:32 -0400 Subject: [PATCH 2/4] test more git log edge cases --- commands/internal/arguments/arguments_test.go | 37 +++++++++++++------ 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index 3593462..6ba208d 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -15,22 +15,35 @@ var testExpandCases = []struct { {"1 3 7", "$e1 $e3 $e7"}, {"1-3 6", "$e1 $e2 $e3 $e6"}, {"seven 2-5 1", "seven $e2 $e3 $e4 $e5 $e1"}, - // Test cases for https://github.com/mroth/scmpuff/issues/69 - {"log -1 1", "log -1 $e1"}, - {"log -n1 2", "log -n1 $e2"}, - {"log -n 1 2", "log -n 1 $e2"}, + /* + Testcases for https://github.com/mroth/scmpuff/issues/69, git subcommand + that can accept a dangling numeric argument. This is annoying because it + looks like a scmpuff numeric ref, so we'll have to special case those if + we want to allow this git porcelain CLI parsing behavior. + */ + {"log -1 1", "log -1 $e1"}, // was not an issue to begin with + {"log -n1 2", "log -n1 $e2"}, // was not an issue to begin with + {"log -n 1", "log -n 1"}, // simple case + {"log -n 1 2", "log -n 1 $e2"}, // simple case + {"log --max-count 1 2", "log --max-count 1 $e2"}, // other log instances + {"log --skip 1 2", "log --skip 1 $e2"}, // other log instances + {"log --min-parents 2 --max-parents 5 1-3", "log --min-parents 2 --max-parents 5 $e1 $e2 $e3"}, + {"log -g -n 1 -i 1", "log -g -n 1 -i $e1"}, // mixed in + {"rm -n 1", "rm -n $e1"}, // don't just check for -n, its subcommand specific } func TestExpand(t *testing.T) { for _, tc := range testExpandCases { - // split here to emulate what Cobra will pass us but still write tests with - // normal looking strings - args := strings.Split(tc.args, " ") - expected := strings.Split(tc.expected, " ") - actual := Expand(args) - if !reflect.DeepEqual(actual, expected) { - t.Errorf("ExpandArgs(%v): expected %v, actual %v", tc.args, expected, actual) - } + t.Run(tc.args, func(t *testing.T) { + // split here to emulate what Cobra will pass us but still write tests with + // normal looking strings + args := strings.Split(tc.args, " ") + expected := strings.Split(tc.expected, " ") + actual := Expand(args) + if !reflect.DeepEqual(actual, expected) { + t.Errorf("ExpandArgs(%v): expected %v, actual %v", tc.args, expected, actual) + } + }) } } From e2c11945321b0c59756fa4c3e67775edac0bee84 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Sat, 26 Mar 2022 13:15:22 -0400 Subject: [PATCH 3/4] special case git log numeric flags --- commands/internal/arguments/arguments.go | 27 +++++++++++++++++-- commands/internal/arguments/arguments_test.go | 19 ++++++------- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/commands/internal/arguments/arguments.go b/commands/internal/arguments/arguments.go index fdddb01..9523b46 100644 --- a/commands/internal/arguments/arguments.go +++ b/commands/internal/arguments/arguments.go @@ -60,12 +60,35 @@ func convertToRelativeIfFilePath(arg string) (string, error) { // environment variable symbolic representation, func Expand(args []string) []string { var results []string - for _, arg := range args { - results = append(results, expandArg(arg)...) + for i, arg := range args { + // for special casing, try to understand if we are expanding a full git + // command invocation, so we can apply edge case rules + if refuseExpandArg(args, i) { + results = append(results, arg) + } else { + results = append(results, expandArg(arg)...) + } } return results } +func refuseExpandArg(args []string, pos int) bool { + // at least two args, first being the registered git cmd, and we're past those + if len(args) <= 2 || args[0] != os.Getenv("SCMPUFF_GIT_CMD") || pos < 2 { + return false + } + + switch args[1] { + case "log": + switch args[pos-1] { //check previous arg + case "-n", "--max-count", "--skip", "--min-parents", "--max-parents": + return true + } + } + + return false +} + // expandArg "expands" a single argument we received on the command line. // // It's possible that argument represents a numeric file placeholder, in which diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index 6ba208d..bcf0e1b 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -21,18 +21,19 @@ var testExpandCases = []struct { looks like a scmpuff numeric ref, so we'll have to special case those if we want to allow this git porcelain CLI parsing behavior. */ - {"log -1 1", "log -1 $e1"}, // was not an issue to begin with - {"log -n1 2", "log -n1 $e2"}, // was not an issue to begin with - {"log -n 1", "log -n 1"}, // simple case - {"log -n 1 2", "log -n 1 $e2"}, // simple case - {"log --max-count 1 2", "log --max-count 1 $e2"}, // other log instances - {"log --skip 1 2", "log --skip 1 $e2"}, // other log instances - {"log --min-parents 2 --max-parents 5 1-3", "log --min-parents 2 --max-parents 5 $e1 $e2 $e3"}, - {"log -g -n 1 -i 1", "log -g -n 1 -i $e1"}, // mixed in - {"rm -n 1", "rm -n $e1"}, // don't just check for -n, its subcommand specific + {"git log -1 1", "git log -1 $e1"}, // was not an issue to begin with + {"git log -n1 2", "git log -n1 $e2"}, // was not an issue to begin with + {"git log -n 1", "git log -n 1"}, // simple case + {"git log -n 1 2", "git log -n 1 $e2"}, // simple case + {"git log --max-count 1 2", "git log --max-count 1 $e2"}, // other log instances + {"git log --skip 1 2", "git log --skip 1 $e2"}, // other log instances + {"git log --min-parents 2 --max-parents 5 1-3", "git log --min-parents 2 --max-parents 5 $e1 $e2 $e3"}, + {"git log -g -n 1 -i 1", "git log -g -n 1 -i $e1"}, // mixed in + {"git rm -n 1", "git rm -n $e1"}, // don't just check for -n, its subcommand specific } func TestExpand(t *testing.T) { + t.Setenv("SCMPUFF_GIT_CMD", "git") for _, tc := range testExpandCases { t.Run(tc.args, func(t *testing.T) { // split here to emulate what Cobra will pass us but still write tests with From 5ba104eb2f101a62adaf917e9c705d1944674cc5 Mon Sep 17 00:00:00 2001 From: Matthew Rothenberg Date: Mon, 28 Mar 2022 19:12:29 -0400 Subject: [PATCH 4/4] add more edge cases --- commands/internal/arguments/arguments.go | 7 ++++++- commands/internal/arguments/arguments_test.go | 6 ++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/commands/internal/arguments/arguments.go b/commands/internal/arguments/arguments.go index 9523b46..ed611ca 100644 --- a/commands/internal/arguments/arguments.go +++ b/commands/internal/arguments/arguments.go @@ -80,10 +80,15 @@ func refuseExpandArg(args []string, pos int) bool { switch args[1] { case "log": - switch args[pos-1] { //check previous arg + switch args[pos-1] { case "-n", "--max-count", "--skip", "--min-parents", "--max-parents": return true } + case "blame": + switch args[pos-1] { + case "-L": + return true + } } return false diff --git a/commands/internal/arguments/arguments_test.go b/commands/internal/arguments/arguments_test.go index bcf0e1b..1f1ab2c 100644 --- a/commands/internal/arguments/arguments_test.go +++ b/commands/internal/arguments/arguments_test.go @@ -30,6 +30,12 @@ var testExpandCases = []struct { {"git log --min-parents 2 --max-parents 5 1-3", "git log --min-parents 2 --max-parents 5 $e1 $e2 $e3"}, {"git log -g -n 1 -i 1", "git log -g -n 1 -i $e1"}, // mixed in {"git rm -n 1", "git rm -n $e1"}, // don't just check for -n, its subcommand specific + // more evaluated edge cases... + // git diff -U does not allow space + // git blame -M and -C take optional integers, but no space + // git blame -L (not ,) is an edge case to handle + {"git blame -L 1 1", "git blame -L 1 $e1"}, + // TODO: git rebase -C is not documented with a space, but needs to be tested to see if it accepts one } func TestExpand(t *testing.T) {