From f22320a478bec242fd8350480ac0d91420f5704e Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Tue, 22 Mar 2022 01:57:37 +0000 Subject: [PATCH] Parse scp args --- internal/codespaces/ssh.go | 50 ++++++++++---- internal/codespaces/ssh_test.go | 116 ++++++++++++++++++++++---------- pkg/cmd/codespace/ssh.go | 2 +- 3 files changed, 121 insertions(+), 47 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 1096014e7..1a23b1112 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -38,18 +38,35 @@ func Shell(ctx context.Context, p printer, sshArgs []string, port int, destinati func Copy(ctx context.Context, scpArgs []string, port int, destination string) error { // Beware: invalid syntax causes scp to exit 1 with // no error message, so don't let that happen. - cmd := exec.CommandContext(ctx, "scp", + connArgs := []string{ "-P", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes", "-C", // compression - ) - for _, arg := range scpArgs { - // Replace "remote:" prefix with (e.g.) "root@localhost:". - if rest := strings.TrimPrefix(arg, "remote:"); rest != arg { - arg = destination + ":" + rest - } - cmd.Args = append(cmd.Args, arg) } + + cmdArgs, command, err := parseSCPArgs(scpArgs) + if err != nil { + return err + } + + cmdArgs = append(cmdArgs, connArgs...) + + if len(command) > 0 { + cmdArgs = append(cmdArgs, "--") + + for _, arg := range command { + // Replace "remote:" prefix with (e.g.) "root@localhost:". + if rest := strings.TrimPrefix(arg, "remote:"); rest != arg { + arg = destination + ":" + rest + } + cmdArgs = append(cmdArgs, arg) + } + } + + fmt.Println(cmdArgs) + + cmd := exec.CommandContext(ctx, "scp", cmdArgs...) + cmd.Stdin = nil cmd.Stdout = os.Stderr cmd.Stderr = os.Stderr @@ -95,9 +112,18 @@ func newSSHCommand(ctx context.Context, port int, dst string, cmdArgs []string) return cmd, connArgs, nil } -// parseSSHArgs parses SSH arguments into two distinct slices of flags and command. -// It returns an error if a unary flag is provided without an argument. func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { + return parseArgs(args, "bcDeFIiLlmOopRSWw") +} + +func parseSCPArgs(args []string) (cmdArgs, command []string, err error) { + return parseArgs(args, "cFiJloPS") +} + +// parseArgs parses arguments into two distinct slices of flags and command. Parsing stops +// as soon as a non-flag argument is found assuming the remaining arguments are the command. +// It returns an error if a unary flag is provided without an argument. +func parseArgs(args []string, unaryFlags string) (cmdArgs, command []string, err error) { for i := 0; i < len(args); i++ { arg := args[i] @@ -108,9 +134,9 @@ func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { } cmdArgs = append(cmdArgs, arg) - if len(arg) == 2 && strings.Contains("bcDeFIiLlmOopRSWw", arg[1:2]) { + if len(arg) == 2 && strings.Contains(unaryFlags, arg[1:2]) { if i++; i == len(args) { - return nil, nil, fmt.Errorf("ssh flag: %s requires an argument", arg) + return nil, nil, fmt.Errorf("flag: %s requires an argument", arg) } cmdArgs = append(cmdArgs, args[i]) diff --git a/internal/codespaces/ssh_test.go b/internal/codespaces/ssh_test.go index c804f6000..7389bd2f9 100644 --- a/internal/codespaces/ssh_test.go +++ b/internal/codespaces/ssh_test.go @@ -5,15 +5,15 @@ import ( "testing" ) -func TestParseSSHArgs(t *testing.T) { - type testCase struct { - Args []string - ParsedArgs []string - Command []string - Error string - } +type parseTestCase struct { + Args []string + ParsedArgs []string + Command []string + Error string +} - testCases := []testCase{ +func TestParseSSHArgs(t *testing.T) { + testCases := []parseTestCase{ {}, // empty test case { Args: []string{"-X", "-Y"}, @@ -69,37 +69,85 @@ func TestParseSSHArgs(t *testing.T) { Args: []string{"-b"}, ParsedArgs: nil, Command: nil, - Error: "ssh flag: -b requires an argument", + Error: "flag: -b requires an argument", }, } for _, tcase := range testCases { args, command, err := parseSSHArgs(tcase.Args) - if tcase.Error != "" { - if err == nil { - t.Errorf("expected error and got nil: %#v", tcase) - } - if err.Error() != tcase.Error { - t.Errorf("error does not match expected error, got: '%s', expected: '%s'", err.Error(), tcase.Error) - } - - continue - } - - if err != nil { - t.Errorf("unexpected error: %v on test case: %#v", err, tcase) - continue - } - - argsStr, parsedArgsStr := fmt.Sprintf("%s", args), fmt.Sprintf("%s", tcase.ParsedArgs) - if argsStr != parsedArgsStr { - t.Errorf("args do not match parsed args. got: '%s', expected: '%s'", argsStr, parsedArgsStr) - } - - commandStr, parsedCommandStr := fmt.Sprintf("%s", command), fmt.Sprintf("%s", tcase.Command) - if commandStr != parsedCommandStr { - t.Errorf("command does not match parsed command. got: '%s', expected: '%s'", commandStr, parsedCommandStr) - } + checkParseResult(t, tcase, args, command, err) + } +} + +func TestParseSCPArgs(t *testing.T) { + testCases := []parseTestCase{ + {}, // empty test case + { + Args: []string{"-X", "-Y"}, + ParsedArgs: []string{"-X", "-Y"}, + Command: nil, + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: nil, + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test", "local/file", "remote:file"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: []string{"local/file", "remote:file"}, + }, + { + Args: []string{"-X", "-Y", "-o", "someoption=test", "local/file", "remote:file"}, + ParsedArgs: []string{"-X", "-Y", "-o", "someoption=test"}, + Command: []string{"local/file", "remote:file"}, + }, + { + Args: []string{"local/file", "remote:file"}, + ParsedArgs: []string{}, + Command: []string{"local/file", "remote:file"}, + }, + { + Args: []string{"-c"}, + ParsedArgs: nil, + Command: nil, + Error: "flag: -c requires an argument", + }, + } + + for _, tcase := range testCases { + args, command, err := parseSCPArgs(tcase.Args) + + checkParseResult(t, tcase, args, command, err) + } +} + +func checkParseResult(t *testing.T, tcase parseTestCase, gotArgs, gotCmd []string, gotErr error) { + if tcase.Error != "" { + if gotErr == nil { + t.Errorf("expected error and got nil: %#v", tcase) + } + + if gotErr.Error() != tcase.Error { + t.Errorf("error does not match expected error, got: '%s', expected: '%s'", gotErr.Error(), tcase.Error) + } + + return + } + + if gotErr != nil { + t.Errorf("unexpected error: %v on test case: %#v", gotErr, tcase) + return + } + + argsStr, parsedArgsStr := fmt.Sprintf("%s", gotArgs), fmt.Sprintf("%s", tcase.ParsedArgs) + if argsStr != parsedArgsStr { + t.Errorf("args do not match parsed args. got: '%s', expected: '%s'", argsStr, parsedArgsStr) + } + + commandStr, parsedCommandStr := fmt.Sprintf("%s", gotCmd), fmt.Sprintf("%s", tcase.Command) + if commandStr != parsedCommandStr { + t.Errorf("command does not match parsed command. got: '%s', expected: '%s'", commandStr, parsedCommandStr) } } diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 69c9c9ee1..462b31a7b 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -392,7 +392,7 @@ func (a *App) Copy(ctx context.Context, args []string, opts cpOptions) error { if opts.recursive { opts.scpArgs = append(opts.scpArgs, "-r") } - opts.scpArgs = append(opts.scpArgs, "--") + hasRemote := false for _, arg := range args { if rest := strings.TrimPrefix(arg, "remote:"); rest != arg {