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 1/3] 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 { From a03051f295de33800700798194ca5faaeded2a4b Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Tue, 22 Mar 2022 14:49:58 +0000 Subject: [PATCH 2/3] Refactoring and comments --- internal/codespaces/ssh.go | 77 +++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 34 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 1a23b1112..8e26ef545 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -29,47 +29,18 @@ func Shell(ctx context.Context, p printer, sshArgs []string, port int, destinati return cmd.Run() } -// Copy runs an scp command over the specified port. The arguments may -// include flags and non-flags, optionally separated by "--". +// Copy runs an scp command over the specified port. scpArgs should contain both scp flags +// as well as the list of files to copy, with the flags first. // // Remote files indicated by a "remote:" prefix are resolved relative // to the remote user's home directory, and are subject to shell expansion // on the remote host; see https://lwn.net/Articles/835962/. 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. - connArgs := []string{ - "-P", strconv.Itoa(port), - "-o", "NoHostAuthenticationForLocalhost=yes", - "-C", // compression - } - - cmdArgs, command, err := parseSCPArgs(scpArgs) + cmd, err := newSCPCommand(ctx, port, destination, scpArgs) if err != nil { - return err + return fmt.Errorf("failed to create scp command: %w", 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 return cmd.Run() } @@ -116,11 +87,49 @@ func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { return parseArgs(args, "bcDeFIiLlmOopRSWw") } +// newSCPCommand populates an exec.Cmd to run an scp command for the files specified in cmdArgs. +// cmdArgs is parsed such that scp flags and the files to copy are separated by a "--" in the command. +// For example: scp -F ./config -- local/file remote:file +func newSCPCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, error) { + // Beware: invalid syntax causes scp to exit 1 with + // no error message, so don't let that happen. + connArgs := []string{ + "-P", strconv.Itoa(port), + "-o", "NoHostAuthenticationForLocalhost=yes", + "-C", // compression + } + + cmdArgs, command, err := parseSCPArgs(cmdArgs) + if err != nil { + return nil, err + } + + cmdArgs = append(cmdArgs, connArgs...) + + cmdArgs = append(cmdArgs, "--") + + for _, arg := range command { + // Replace "remote:" prefix with (e.g.) "root@localhost:". + if rest := strings.TrimPrefix(arg, "remote:"); rest != arg { + arg = dst + ":" + rest + } + cmdArgs = append(cmdArgs, arg) + } + + cmd := exec.CommandContext(ctx, "scp", cmdArgs...) + + cmd.Stdin = nil + cmd.Stdout = os.Stderr + cmd.Stderr = os.Stderr + + return cmd, nil +} + 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 +// 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) { From 623e67f6b1f46fcb6d9cb00f984a94537f453f39 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Wed, 23 Mar 2022 16:09:07 +0000 Subject: [PATCH 3/3] Update comments/usage and remove -- --- internal/codespaces/ssh.go | 10 ++++------ pkg/cmd/codespace/ssh.go | 3 ++- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/internal/codespaces/ssh.go b/internal/codespaces/ssh.go index 8e26ef545..29bf1e68c 100644 --- a/internal/codespaces/ssh.go +++ b/internal/codespaces/ssh.go @@ -88,11 +88,9 @@ func parseSSHArgs(args []string) (cmdArgs, command []string, err error) { } // newSCPCommand populates an exec.Cmd to run an scp command for the files specified in cmdArgs. -// cmdArgs is parsed such that scp flags and the files to copy are separated by a "--" in the command. -// For example: scp -F ./config -- local/file remote:file +// cmdArgs is parsed such that scp flags precede the files to copy in the command. +// For example: scp -F ./config local/file remote:file func newSCPCommand(ctx context.Context, port int, dst string, cmdArgs []string) (*exec.Cmd, error) { - // Beware: invalid syntax causes scp to exit 1 with - // no error message, so don't let that happen. connArgs := []string{ "-P", strconv.Itoa(port), "-o", "NoHostAuthenticationForLocalhost=yes", @@ -106,8 +104,6 @@ func newSCPCommand(ctx context.Context, port int, dst string, cmdArgs []string) cmdArgs = append(cmdArgs, connArgs...) - cmdArgs = append(cmdArgs, "--") - for _, arg := range command { // Replace "remote:" prefix with (e.g.) "root@localhost:". if rest := strings.TrimPrefix(arg, "remote:"); rest != arg { @@ -116,6 +112,8 @@ func newSCPCommand(ctx context.Context, port int, dst string, cmdArgs []string) cmdArgs = append(cmdArgs, arg) } + // Beware: invalid syntax causes scp to exit 1 with + // no error message, so don't let that happen. cmd := exec.CommandContext(ctx, "scp", cmdArgs...) cmd.Stdin = nil diff --git a/pkg/cmd/codespace/ssh.go b/pkg/cmd/codespace/ssh.go index 462b31a7b..97fc1974e 100644 --- a/pkg/cmd/codespace/ssh.go +++ b/pkg/cmd/codespace/ssh.go @@ -343,7 +343,7 @@ func newCpCmd(app *App) *cobra.Command { var opts cpOptions cpCmd := &cobra.Command{ - Use: "cp [-e] [-r] ... ", + Use: "cp [-e] [-r] [-- [...]] ... ", Short: "Copy files between local and remote file systems", Long: heredoc.Docf(` The cp command copies files between the local and remote file systems. @@ -368,6 +368,7 @@ func newCpCmd(app *App) *cobra.Command { $ gh codespace cp -e README.md 'remote:/workspaces/$RepositoryName/' $ gh codespace cp -e 'remote:~/*.go' ./gofiles/ $ gh codespace cp -e 'remote:/workspaces/myproj/go.{mod,sum}' ./gofiles/ + $ gh codespace cp -e -- -F ~/.ssh/codespaces_config 'remote:~/*.go' ./gofiles/ `), RunE: func(cmd *cobra.Command, args []string) error { return app.Copy(cmd.Context(), args, opts)