Refactor git client and add tests (cli#6525)

git/command.go

@@ -0,0 +1,100 @@
+package git
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"io"
+	"os/exec"
+
+	"github.com/cli/cli/v2/internal/run"
+)
+
+type commandCtx = func(ctx context.Context, name string, args ...string) *exec.Cmd
+
+type gitCommand struct {
+	*exec.Cmd
+}
+
+func (gc *gitCommand) Run() error {
+	stderr := &bytes.Buffer{}
+	if gc.Cmd.Stderr == nil {
+		gc.Cmd.Stderr = stderr
+	}
+	// This is a hack in order to not break the hundreds of
+	// existing tests that rely on `run.PrepareCmd` to be invoked.
+	err := run.PrepareCmd(gc.Cmd).Run()
+	if err != nil {
+		ge := GitError{err: err, Stderr: stderr.String()}
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			ge.ExitCode = exitError.ExitCode()
+		}
+		return &ge
+	}
+	return nil
+}
+
+func (gc *gitCommand) Output() ([]byte, error) {
+	gc.Stdout = nil
+	gc.Stderr = nil
+	// This is a hack in order to not break the hundreds of
+	// existing tests that rely on `run.PrepareCmd` to be invoked.
+	out, err := run.PrepareCmd(gc.Cmd).Output()
+	if err != nil {
+		ge := GitError{err: err}
+		var exitError *exec.ExitError
+		if errors.As(err, &exitError) {
+			ge.Stderr = string(exitError.Stderr)
+			ge.ExitCode = exitError.ExitCode()
+		}
+		err = &ge
+	}
+	return out, err
+}
+
+func (gc *gitCommand) setRepoDir(repoDir string) {
+	for i, arg := range gc.Args {
+		if arg == "-C" {
+			gc.Args[i+1] = repoDir
+			return
+		}
+	}
+	// Handle "--" invocations for testing purposes.
+	var index int
+	for i, arg := range gc.Args {
+		if arg == "--" {
+			index = i + 1
+		}
+	}
+	gc.Args = append(gc.Args[:index+3], gc.Args[index+1:]...)
+	gc.Args[index+1] = "-C"
+	gc.Args[index+2] = repoDir
+}
+
+// Allow individual commands to be modified from the default client options.
+type CommandModifier func(*gitCommand)
+
+func WithStderr(stderr io.Writer) CommandModifier {
+	return func(gc *gitCommand) {
+		gc.Stderr = stderr
+	}
+}
+
+func WithStdout(stdout io.Writer) CommandModifier {
+	return func(gc *gitCommand) {
+		gc.Stdout = stdout
+	}
+}
+
+func WithStdin(stdin io.Reader) CommandModifier {
+	return func(gc *gitCommand) {
+		gc.Stdin = stdin
+	}
+}
+
+func WithRepoDir(repoDir string) CommandModifier {
+	return func(gc *gitCommand) {
+		gc.setRepoDir(repoDir)
+	}
+}

git/errors.go

@@ -0,0 +1,39 @@
+package git
+
+import (
+	"errors"
+	"fmt"
+)
+
+// ErrNotOnAnyBranch indicates that the user is in detached HEAD state.
+var ErrNotOnAnyBranch = errors.New("git: not on any branch")
+
+type NotInstalled struct {
+	message string
+	err     error
+}
+
+func (e *NotInstalled) Error() string {
+	return e.message
+}
+
+func (e *NotInstalled) Unwrap() error {
+	return e.err
+}
+
+type GitError struct {
+	ExitCode int
+	Stderr   string
+	err      error
+}
+
+func (ge *GitError) Error() string {
+	if ge.Stderr == "" {
+		return fmt.Sprintf("failed to run git: %v", ge.err)
+	}
+	return fmt.Sprintf("failed to run git: %s", ge.Stderr)
+}
+
+func (ge *GitError) Unwrap() error {
+	return ge.err
+}

internal/run/run.go

@@ -2,6 +2,7 @@ package run
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -33,16 +34,19 @@ func (c cmdWithStderr) Output() ([]byte, error) {
 	if isVerbose, _ := utils.IsDebugEnabled(); isVerbose {
 		_ = printArgs(os.Stderr, c.Cmd.Args)
 	}
-	if c.Cmd.Stderr != nil {
-		return c.Cmd.Output()
-	}
-	errStream := &bytes.Buffer{}
-	c.Cmd.Stderr = errStream
 	out, err := c.Cmd.Output()
-	if err != nil {
-		err = &CmdError{errStream, c.Cmd.Args, err}
+	if c.Cmd.Stderr != nil || err == nil {
+		return out, err
 	}
-	return out, err
+	cmdErr := &CmdError{
+		Args: c.Cmd.Args,
+		Err:  err,
+	}
+	var exitError *exec.ExitError
+	if errors.As(err, &exitError) {
+		cmdErr.Stderr = bytes.NewBuffer(exitError.Stderr)
+	}
+	return out, cmdErr
 }
 
 func (c cmdWithStderr) Run() error {
@@ -56,16 +60,20 @@ func (c cmdWithStderr) Run() error {
 	c.Cmd.Stderr = errStream
 	err := c.Cmd.Run()
 	if err != nil {
-		err = &CmdError{errStream, c.Cmd.Args, err}
+		err = &CmdError{
+			Args:   c.Cmd.Args,
+			Err:    err,
+			Stderr: errStream,
+		}
 	}
 	return err
 }
 
 // CmdError provides more visibility into why an exec.Cmd had failed
 type CmdError struct {
-	Stderr *bytes.Buffer
 	Args   []string
 	Err    error
+	Stderr *bytes.Buffer
 }
 
 func (e CmdError) Error() string {

pkg/cmd/issue/develop/develop.go

@@ -245,7 +245,6 @@ func developRunList(opts *DevelopOptions) (err error) {
 }
 
 func checkoutBranch(opts *DevelopOptions, baseRepo ghrepo.Interface, checkoutBranch string) (err error) {
-
 	remotes, err := opts.Remotes()
 	if err != nil {
 		return err
@@ -261,18 +260,11 @@ func checkoutBranch(opts *DevelopOptions, baseRepo ghrepo.Interface, checkoutBra
 			return err
 		}
 	} else {
-		gitFetch, err := opts.GitClient.Command(ctx.Background(), "fetch", "origin", fmt.Sprintf("+refs/heads/%[1]s:refs/remotes/origin/%[1]s", checkoutBranch))
-
+		err := opts.GitClient.Fetch(ctx.Background(), "origin", fmt.Sprintf("+refs/heads/%[1]s:refs/remotes/origin/%[1]s", checkoutBranch))
 		if err != nil {
 			return err
 		}
 
-		gitFetch.Stdout = opts.IO.Out
-		gitFetch.Stderr = opts.IO.ErrOut
-		err = gitFetch.Run()
-		if err != nil {
-			return err
-		}
 		if err := opts.GitClient.CheckoutNewBranch(ctx.Background(), baseRemote.Name, checkoutBranch); err != nil {
 			return err
 		}

pkg/cmd/pr/checkout/checkout.go

@@ -236,13 +236,13 @@ func missingMergeConfigForBranch(client *git.Client, b string) bool {
 }
 
 func localBranchExists(client *git.Client, b string) bool {
-	_, err := client.ShowRefs(context.Background(), "refs/heads/"+b)
+	_, err := client.ShowRefs(context.Background(), []string{"refs/heads/" + b})
 	return err == nil
 }
 
 func executeCmds(client *git.Client, cmdQueue [][]string) error {
 	for _, args := range cmdQueue {
-		//TODO: Use AuthenticatedCommand
+		// TODO: Use AuthenticatedCommand
 		cmd, err := client.Command(context.Background(), args...)
 		if err != nil {
 			return err

pkg/cmd/pr/create/create.go

@@ -423,7 +423,7 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h
 		refsForLookup = append(refsForLookup, tr.String())
 	}
 
-	resolvedRefs, _ := gitClient.ShowRefs(context.Background(), refsForLookup...)
+	resolvedRefs, _ := gitClient.ShowRefs(context.Background(), refsForLookup)
 	if len(resolvedRefs) > 1 {
 		for _, r := range resolvedRefs[1:] {
 			if r.Hash != resolvedRefs[0].Hash {

pkg/cmd/repo/create/create.go

@@ -551,15 +551,10 @@ func createFromLocal(opts *CreateOptions) error {
 	}
 
 	if opts.Push {
-		repoPush, err := opts.GitClient.Command(context.Background(), "push", "-u", baseRemote, "HEAD")
+		err := opts.GitClient.Push(context.Background(), baseRemote, "HEAD")
 		if err != nil {
 			return err
 		}
-		err = repoPush.Run()
-		if err != nil {
-			return err
-		}
-
 		if isTTY {
 			fmt.Fprintf(stdout, "%s Pushed commits to %s\n", cs.SuccessIcon(), remoteURL)
 		}
@@ -569,20 +564,16 @@ func createFromLocal(opts *CreateOptions) error {
 
 func sourceInit(gitClient *git.Client, io *iostreams.IOStreams, remoteURL, baseRemote string) error {
 	cs := io.ColorScheme()
-	isTTY := io.IsStdoutTTY()
-	stdout := io.Out
-
 	remoteAdd, err := gitClient.Command(context.Background(), "remote", "add", baseRemote, remoteURL)
 	if err != nil {
 		return err
 	}
-
 	_, err = remoteAdd.Output()
 	if err != nil {
 		return fmt.Errorf("%s Unable to add remote %q", cs.FailureIcon(), baseRemote)
 	}
-	if isTTY {
-		fmt.Fprintf(stdout, "%s Added remote %s\n", cs.SuccessIcon(), remoteURL)
+	if io.IsStdoutTTY() {
+		fmt.Fprintf(io.Out, "%s Added remote %s\n", cs.SuccessIcon(), remoteURL)
 	}
 	return nil
 }
@@ -656,21 +647,13 @@ func localInit(gitClient *git.Client, remoteURL, path, checkoutBranch string) er
 		return nil
 	}
 
-	gitFetch, err := gc.Command(ctx, "fetch", "origin", fmt.Sprintf("+refs/heads/%[1]s:refs/remotes/origin/%[1]s", checkoutBranch))
-	if err != nil {
-		return err
-	}
-	err = gitFetch.Run()
+	refspec := fmt.Sprintf("+refs/heads/%[1]s:refs/remotes/origin/%[1]s", checkoutBranch)
+	err = gc.Fetch(ctx, "origin", refspec)
 	if err != nil {
 		return err
 	}
 
-	gitCheckout, err := gc.Command(ctx, "checkout", checkoutBranch)
-	if err != nil {
-		return err
-	}
-	_, err = gitCheckout.Output()
-	return err
+	return gc.CheckoutBranch(ctx, checkoutBranch)
 }
 
 func interactiveGitIgnore(client *http.Client, hostname string, prompter iprompter) (string, error) {

pkg/cmd/repo/create/create_test.go

@@ -410,7 +410,7 @@ func Test_createRun(t *testing.T) {
 				cs.Register(`git -C . rev-parse --git-dir`, 0, ".git")
 				cs.Register(`git -C . rev-parse HEAD`, 0, "commithash")
 				cs.Register(`git -C . remote add origin https://github.com/OWNER/REPO`, 0, "")
-				cs.Register(`git -C . push -u origin HEAD`, 0, "")
+				cs.Register(`git -C . push --set-upstream origin HEAD`, 0, "")
 			},
 			wantStdout: "✓ Created repository OWNER/REPO on GitHub\n✓ Added remote https://github.com/OWNER/REPO.git\n✓ Pushed commits to https://github.com/OWNER/REPO.git\n",
 		},

pkg/cmd/repo/sync/git.go

@@ -93,18 +93,11 @@ func (g *gitExecuter) IsAncestor(ancestor, progeny string) (bool, error) {
 }
 
 func (g *gitExecuter) IsDirty() (bool, error) {
-	cmd, err := g.client.Command(context.Background(), "status", "--untracked-files=no", "--porcelain")
+	changeCount, err := g.client.UncommittedChangeCount(context.Background())
 	if err != nil {
 		return false, err
 	}
-	output, err := cmd.Output()
-	if err != nil {
-		return true, err
-	}
-	if len(output) > 0 {
-		return true, nil
-	}
-	return false, nil
+	return changeCount != 0, nil
 }
 
 func (g *gitExecuter) MergeFastForward(ref string) error {