Skip to content

Commit

Permalink
fix(internal/gapicgen): exec Stdout already set (#4509)
Browse files Browse the repository at this point in the history
This error was occuring because stdout was being set by some
calling locations which violates Command.Output usage. This
refactors code to never set stdout. I also added some more debug
info of logging the command output which can be useful in the case
of failures.
  • Loading branch information
codyoss authored Jul 28, 2021
1 parent d8ec92b commit 41246e9
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 31 deletions.
6 changes: 4 additions & 2 deletions internal/gapicgen/execv/command.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,15 +33,17 @@ type CmdWrapper struct {
// The commands stdout/stderr default to os.Stdout/os.Stderr respectfully.
func Command(name string, arg ...string) *CmdWrapper {
c := &CmdWrapper{exec.Command(name, arg...)}
c.Stdout = os.Stdout
c.Stderr = os.Stderr
c.Stdin = os.Stdin
return &CmdWrapper{exec.Command(name, arg...)}
}

// Run a command.
func (c *CmdWrapper) Run() error {
_, err := c.Output()
b, err := c.Output()
if len(b) > 0 {
log.Printf("Command Output: %s", b)
}
return err
}

Expand Down
41 changes: 15 additions & 26 deletions internal/gapicgen/git/git.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ package git
import (
"bytes"
"fmt"
"io"
"log"
"os"
"os/exec"
Expand Down Expand Up @@ -87,17 +86,16 @@ func ParseChangeInfo(googleapisDir string, hashes []string, gapicPkgs map[string
var changes []*ChangeInfo
for _, hash := range hashes {
// Get commit title and body
rawBody := bytes.NewBuffer(nil)
c := execv.Command("git", "show", "--pretty=format:%s~~%b", "-s", hash)
c.Stdout = rawBody
c.Dir = googleapisDir
if err := c.Run(); err != nil {
b, err := c.Output()
if err != nil {
return nil, err
}

ss := strings.Split(rawBody.String(), "~~")
ss := strings.Split(string(b), "~~")
if len(ss) != 2 {
return nil, fmt.Errorf("expected two segments for commit, got %d: %q", len(ss), rawBody.String())
return nil, fmt.Errorf("expected two segments for commit, got %d: %s", len(ss), b)
}
title, body := strings.TrimSpace(ss[0]), strings.TrimSpace(ss[1])

Expand Down Expand Up @@ -154,46 +152,38 @@ func CommitsSinceHash(gitDir, hash string, inclusive bool) ([]string, error) {
commitRange = fmt.Sprintf("%s..", hash)
}

out := bytes.NewBuffer(nil)
c := execv.Command("git", "rev-list", commitRange)
c.Stdout = out
c.Dir = gitDir
if err := c.Run(); err != nil {
b, err := c.Output()
if err != nil {
return nil, err
}
return strings.Split(strings.TrimSpace(out.String()), "\n"), nil
return strings.Split(strings.TrimSpace(string(b)), "\n"), nil
}

// UpdateFilesSinceHash returns a listed of files updated since the provided
// hash for the given gitDir.
func UpdateFilesSinceHash(gitDir, hash string) ([]string, error) {
out := bytes.NewBuffer(nil)
// The provided diff-filter flags restricts to files that have been:
// - (A) Added
// - (C) Copied
// - (M) Modified
// - (R) Renamed
c := execv.Command("git", "diff-tree", "--no-commit-id", "--name-only", "--diff-filter=ACMR", "-r", fmt.Sprintf("%s..HEAD", hash))
c.Stdout = out
c.Dir = gitDir
if err := c.Run(); err != nil {
b, err := c.Output()
if err != nil {
return nil, err
}
return strings.Split(out.String(), "\n"), nil
return strings.Split(string(b), "\n"), nil
}

// HasChanges reports whether the given directory has uncommitted git changes.
func HasChanges(dir string) (bool, error) {
// Write command output to both os.Stderr and local, so that we can check
// whether there are modified files.
inmem := &bytes.Buffer{}
w := io.MultiWriter(os.Stderr, inmem)

c := execv.Command("bash", "-c", "git status --short")
c.Dir = dir
c.Stdout = w
err := c.Run()
return inmem.Len() > 0, err
b, err := c.Output()
return len(b) > 0, err
}

// DeepClone clones a repository in the given directory.
Expand Down Expand Up @@ -249,12 +239,11 @@ func FileDiff(dir, filename string) (string, error) {
// filesChanged returns a list of files changed in a commit for the provdied
// hash in the given gitDir.
func filesChanged(gitDir, hash string) ([]string, error) {
out := bytes.NewBuffer(nil)
c := execv.Command("git", "show", "--pretty=format:", "--name-only", hash)
c.Stdout = out
c.Dir = gitDir
if err := c.Run(); err != nil {
b, err := c.Output()
if err != nil {
return nil, err
}
return strings.Split(out.String(), "\n"), nil
return strings.Split(string(b), "\n"), nil
}
6 changes: 3 additions & 3 deletions internal/gapicgen/tools.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ package gapicgen
import (
"fmt"
"os"
"os/exec"

"cloud.google.com/go/internal/gapicgen/execv"
)

// VerifyAllToolsExist ensures that all required tools exist on the system.
func VerifyAllToolsExist(toolsNeeded []string) error {
for _, t := range toolsNeeded {
c := exec.Command("which", t)
c.Stdout = os.Stdout
c := execv.Command("which", t)
c.Stderr = os.Stderr
if c.Run() != nil {
return fmt.Errorf("%s does not appear to be installed. please install it. all tools needed: %v", t, toolsNeeded)
Expand Down

0 comments on commit 41246e9

Please sign in to comment.