From 41255588935d1eca6a48f6366f5c0f9078b175d3 Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Thu, 13 Apr 2023 12:16:28 +0800 Subject: [PATCH] improve --- modules/git/command.go | 54 +++++++++++++++++++++---------------- modules/git/command_test.go | 8 ++++++ modules/git/repo.go | 45 +++++++------------------------ 3 files changed, 48 insertions(+), 59 deletions(-) diff --git a/modules/git/command.go b/modules/git/command.go index 9a65279a8cb5f..a42d859f55f7b 100644 --- a/modules/git/command.go +++ b/modules/git/command.go @@ -40,7 +40,7 @@ const DefaultLocale = "C" // Command represents a command with its subcommands or arguments. type Command struct { - name string + prog string args []string parentContext context.Context desc string @@ -49,10 +49,28 @@ type Command struct { } func (c *Command) String() string { - if len(c.args) == 0 { - return c.name + return c.toString(false) +} + +func (c *Command) toString(sanitizing bool) string { + // WARNING: this function is for debugging purposes only. It's much better than old code (which only joins args with space), + // It's impossible to make a simple and 100% correct implementation of argument quoting for different platforms. + debugQuote := func(s string) string { + if strings.ContainsAny(s, " `'\"\t\r\n") { + return fmt.Sprintf("%q", s) + } + return s + } + a := make([]string, 0, len(c.args)+1) + a = append(a, debugQuote(c.prog)) + for _, arg := range c.args { + if sanitizing && (strings.Contains(arg, "://") && strings.Contains(arg, "@")) { + a = append(a, debugQuote(util.SanitizeCredentialURLs(arg))) + } else { + a = append(a, debugQuote(arg)) + } } - return fmt.Sprintf("%s %s", c.name, strings.Join(c.args, " ")) + return strings.Join(a, " ") } // NewCommand creates and returns a new Git Command based on given command and arguments. @@ -67,7 +85,7 @@ func NewCommand(ctx context.Context, args ...internal.CmdArg) *Command { cargs = append(cargs, string(arg)) } return &Command{ - name: GitExecutable, + prog: GitExecutable, args: cargs, parentContext: ctx, globalArgsLength: len(globalCommandArgs), @@ -82,7 +100,7 @@ func NewCommandContextNoGlobals(ctx context.Context, args ...internal.CmdArg) *C cargs = append(cargs, string(arg)) } return &Command{ - name: GitExecutable, + prog: GitExecutable, args: cargs, parentContext: ctx, } @@ -250,28 +268,18 @@ func (c *Command) Run(opts *RunOpts) error { } if len(opts.Dir) == 0 { - log.Debug("%s", c) + log.Debug("git.Command.Run: %s", c) } else { - log.Debug("%s: %v", opts.Dir, c) + log.Debug("git.Command.RunDir(%s): %s", opts.Dir, c) } desc := c.desc if desc == "" { - args := c.args[c.globalArgsLength:] - var argSensitiveURLIndexes []int - for i, arg := range c.args { - if strings.Contains(arg, "://") && strings.Contains(arg, "@") { - argSensitiveURLIndexes = append(argSensitiveURLIndexes, i) - } - } - if len(argSensitiveURLIndexes) > 0 { - args = make([]string, len(c.args)) - copy(args, c.args) - for _, urlArgIndex := range argSensitiveURLIndexes { - args[urlArgIndex] = util.SanitizeCredentialURLs(args[urlArgIndex]) - } + if opts.Dir == "" { + desc = fmt.Sprintf("git: %s", c.toString(true)) + } else { + desc = fmt.Sprintf("git(dir:%s): %s", opts.Dir, c.toString(true)) } - desc = fmt.Sprintf("%s %s [repo_path: %s]", c.name, strings.Join(args, " "), opts.Dir) } var ctx context.Context @@ -285,7 +293,7 @@ func (c *Command) Run(opts *RunOpts) error { } defer finished() - cmd := exec.CommandContext(ctx, c.name, c.args...) + cmd := exec.CommandContext(ctx, c.prog, c.args...) if opts.Env == nil { cmd.Env = os.Environ() } else { diff --git a/modules/git/command_test.go b/modules/git/command_test.go index 4e5f991d31125..9a6228c9ad05f 100644 --- a/modules/git/command_test.go +++ b/modules/git/command_test.go @@ -52,3 +52,11 @@ func TestGitArgument(t *testing.T) { assert.True(t, isSafeArgumentValue("x")) assert.False(t, isSafeArgumentValue("-x")) } + +func TestCommandString(t *testing.T) { + cmd := NewCommandContextNoGlobals(context.Background(), "a", "-m msg", "it's a test", `say "hello"`) + assert.EqualValues(t, cmd.prog+` a "-m msg" "it's a test" "say \"hello\""`, cmd.String()) + + cmd = NewCommandContextNoGlobals(context.Background(), "url: https://a:b@c/") + assert.EqualValues(t, cmd.prog+` "url: https://sanitized-credential@c/"`, cmd.toString(true)) +} diff --git a/modules/git/repo.go b/modules/git/repo.go index 233f7f20cfc2f..d29ec40ae2ab0 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -209,49 +209,22 @@ func Push(ctx context.Context, repoPath string, opts PushOptions) error { } else { cmd.SetDescription(fmt.Sprintf("push branch %s to %s (force: %t, mirror: %t)", opts.Branch, opts.Remote, opts.Force, opts.Mirror)) } - var outbuf, errbuf strings.Builder - if opts.Timeout == 0 { - opts.Timeout = -1 - } - - err := cmd.Run(&RunOpts{ - Env: opts.Env, - Timeout: opts.Timeout, - Dir: repoPath, - Stdout: &outbuf, - Stderr: &errbuf, - }) + stdout, stderr, err := cmd.RunStdString(&RunOpts{Env: opts.Env, Timeout: opts.Timeout, Dir: repoPath}) if err != nil { - if strings.Contains(errbuf.String(), "non-fast-forward") { - return &ErrPushOutOfDate{ - StdOut: outbuf.String(), - StdErr: errbuf.String(), - Err: err, - } - } else if strings.Contains(errbuf.String(), "! [remote rejected]") { - err := &ErrPushRejected{ - StdOut: outbuf.String(), - StdErr: errbuf.String(), - Err: err, - } + if strings.Contains(stderr, "non-fast-forward") { + return &ErrPushOutOfDate{StdOut: stdout, StdErr: stderr, Err: err} + } else if strings.Contains(stderr, "! [remote rejected]") { + err := &ErrPushRejected{StdOut: stdout, StdErr: stderr, Err: err} err.GenerateMessage() return err - } else if strings.Contains(errbuf.String(), "matches more than one") { - err := &ErrMoreThanOne{ - StdOut: outbuf.String(), - StdErr: errbuf.String(), - Err: err, - } - return err + } else if strings.Contains(stderr, "matches more than one") { + return &ErrMoreThanOne{StdOut: stdout, StdErr: stderr, Err: err} } + return fmt.Errorf("push failed: %w - %s\n%s", err, stderr, stdout) } - if errbuf.Len() > 0 && err != nil { - return fmt.Errorf("%w - %s", err, errbuf.String()) - } - - return err + return nil } // GetLatestCommitTime returns time for latest commit in repository (across all branches)