git-codereview: refactor getOutput/getLines into cmdOutput, trim, lines, nonBlankLines
This CL separates the "run the command and gather output" from
the "process output" operations. Doing so makes the desired
processing clearer, makes the processing available for use on
other text sources.
Also, add cmdOutputDir, for running a command in a different
directory. This is needed by the gofmt command to work around
a bug in Git, and it's made much easier by the above separation.
Change-Id: I2addd8b79dab2d4aa8a24c94a8d7b3a16a8e3484
Reviewed-on: https://go-review.googlesource.com/2906
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/git-codereview/api.go b/git-codereview/api.go
index ac61cba..6ab65b1 100644
--- a/git-codereview/api.go
+++ b/git-codereview/api.go
@@ -41,7 +41,7 @@
}
// Gerrit must be set as Git's origin remote.
- origin := getOutput("git", "config", "remote.origin.url")
+ origin := trim(cmdOutput("git", "config", "remote.origin.url"))
if strings.Contains(origin, "//github.com/") {
dief("git origin must be a Gerrit host, not GitHub: %s", origin)
@@ -84,10 +84,10 @@
// First look in Git's http.cookiefile, which is where Gerrit
// now tells users to store this information.
- if cookieFile, _ := getOutputErr("git", "config", "http.cookiefile"); cookieFile != "" {
+ if cookieFile, _ := trimErr(cmdOutputErr("git", "config", "http.cookiefile")); cookieFile != "" {
data, _ := ioutil.ReadFile(cookieFile)
maxMatch := -1
- for _, line := range strings.Split(string(data), "\n") {
+ for _, line := range lines(string(data)) {
f := strings.Split(line, "\t")
if len(f) >= 7 && (f[0] == auth.host || strings.HasPrefix(f[0], ".") && strings.HasSuffix(auth.host, f[0])) {
if len(f[0]) > maxMatch {
@@ -106,7 +106,7 @@
// used to tell users to store the information, until the passwords
// got so long that old versions of curl couldn't handle them.
data, _ := ioutil.ReadFile(os.Getenv("HOME") + "/.netrc")
- for _, line := range strings.Split(string(data), "\n") {
+ for _, line := range lines(string(data)) {
if i := strings.Index(line, "#"); i >= 0 {
line = line[:i]
}
diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 2a69dcd..1193da9 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -41,7 +41,7 @@
// CurrentBranch returns the current branch.
func CurrentBranch() *Branch {
- name := getOutput("git", "rev-parse", "--abbrev-ref", "HEAD")
+ name := trim(cmdOutput("git", "rev-parse", "--abbrev-ref", "HEAD"))
return &Branch{Name: name}
}
@@ -113,7 +113,7 @@
b.loadedPending = true
// In case of early return.
- b.branchpoint = getOutput("git", "rev-parse", "HEAD")
+ b.branchpoint = trim(cmdOutput("git", "rev-parse", "HEAD"))
if b.DetachedHead() {
return
@@ -122,7 +122,7 @@
// Note: --topo-order means child first, then parent.
origin := b.OriginBranch()
const numField = 5
- all := getOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", origin+".."+b.Name, "--")
+ all := trim(cmdOutput("git", "log", "--topo-order", "--format=format:%H%x00%h%x00%P%x00%B%x00%s%x00", origin+".."+b.Name, "--"))
fields := strings.Split(all, "\x00")
if len(fields) < numField {
return // nothing pending
@@ -149,16 +149,16 @@
// even if we later see additional commits on a different branch leading down to
// a lower location on the same origin branch.
// Check c.Merge (the second parent) too, so we don't depend on the parent order.
- if strings.Contains(getOutput("git", "branch", "-a", "--contains", c.Parent), " "+origin+"\n") {
+ if strings.Contains(cmdOutput("git", "branch", "-a", "--contains", c.Parent), " "+origin+"\n") {
foundMergeBranchpoint = true
b.branchpoint = c.Parent
}
- if strings.Contains(getOutput("git", "branch", "-a", "--contains", c.Merge), " "+origin+"\n") {
+ if strings.Contains(cmdOutput("git", "branch", "-a", "--contains", c.Merge), " "+origin+"\n") {
foundMergeBranchpoint = true
b.branchpoint = c.Merge
}
}
- for _, line := range strings.Split(c.Message, "\n") {
+ for _, line := range lines(c.Message) {
// Note: Keep going even if we find one, so that
// we take the last Change-Id line, just in case
// there is a commit message quoting another
@@ -175,7 +175,7 @@
}
}
b.commitsAhead = len(b.pending)
- b.commitsBehind = len(getOutput("git", "log", "--format=format:x", b.Name+".."+b.OriginBranch(), "--"))
+ b.commitsBehind = len(trim(cmdOutput("git", "log", "--format=format:x", b.Name+".."+b.OriginBranch(), "--")))
}
// Submitted reports whether some form of b's pending commit
@@ -185,7 +185,7 @@
return false
}
line := "Change-Id: " + id
- out := getOutput("git", "log", "-n", "1", "-F", "--grep", line, b.Name+".."+b.OriginBranch(), "--")
+ out := cmdOutput("git", "log", "-n", "1", "-F", "--grep", line, b.Name+".."+b.OriginBranch(), "--")
return strings.Contains(out, line)
}
@@ -193,7 +193,7 @@
// HasStagedChanges reports whether the working directory contains staged changes.
func HasStagedChanges() bool {
- for _, s := range getLines("git", "status", "-b", "--porcelain") {
+ for _, s := range nonBlankLines(cmdOutput("git", "status", "-b", "--porcelain")) {
if stagedRE.MatchString(s) {
return true
}
@@ -205,7 +205,7 @@
// HasUnstagedChanges reports whether the working directory contains unstaged changes.
func HasUnstagedChanges() bool {
- for _, s := range getLines("git", "status", "-b", "--porcelain") {
+ for _, s := range nonBlankLines(cmdOutput("git", "status", "-b", "--porcelain")) {
if unstagedRE.MatchString(s) {
return true
}
@@ -220,8 +220,7 @@
// the files (or the from, to fields of a rename or copy) are quoted C strings.
// For now, we expect the caller only shows these to the user, so these exceptions are okay.
func LocalChanges() (staged, unstaged, untracked []string) {
- // NOTE: Cannot use getLines, because it throws away leading spaces.
- for _, s := range strings.Split(getOutput("git", "status", "-b", "--porcelain"), "\n") {
+ for _, s := range lines(cmdOutput("git", "status", "-b", "--porcelain")) {
if len(s) < 4 || s[2] != ' ' {
continue
}
@@ -245,7 +244,7 @@
func LocalBranches() []*Branch {
var branches []*Branch
current := CurrentBranch()
- for _, s := range getLines("git", "branch", "-q") {
+ for _, s := range nonBlankLines(cmdOutput("git", "branch", "-q")) {
s = strings.TrimSpace(s)
if strings.HasPrefix(s, "* ") {
// * marks current branch in output.
@@ -265,7 +264,7 @@
func OriginBranches() []string {
var branches []string
- for _, line := range getLines("git", "branch", "-a", "-q") {
+ for _, line := range nonBlankLines(cmdOutput("git", "branch", "-a", "-q")) {
line = strings.TrimSpace(line)
if i := strings.Index(line, " -> "); i >= 0 {
line = line[:i]
diff --git a/git-codereview/change.go b/git-codereview/change.go
index 2a66533..ae761cf 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -162,7 +162,7 @@
)
func commitMessageOK() bool {
- body := getOutput("git", "log", "--format=format:%B", "-n", "1")
+ body := cmdOutput("git", "log", "--format=format:%B", "-n", "1")
ok := true
if !messageRE.MatchString(body) {
fmt.Print(commitMessageWarning)
diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index 28edf5d..937fbd6 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -120,15 +120,15 @@
// Find files modified in the index compared to the branchpoint.
branchpt := b.Branchpoint()
- if strings.Contains(getOutput("git", "branch", "-r", "--contains", b.Name), "origin/") {
+ if strings.Contains(cmdOutput("git", "branch", "-r", "--contains", b.Name), "origin/") {
// This is a branch tag move, not an actual change.
// Use HEAD as branch point, so nothing will appear changed.
// We don't want to think about gofmt on already published
// commits.
branchpt = "HEAD"
}
- indexFiles := addRoot(repo, filter(gofmtRequired, getLines("git", "diff", "--name-only", "--diff-filter=ACM", "--cached", branchpt, "--")))
- localFiles := addRoot(repo, filter(gofmtRequired, getLines("git", "diff", "--name-only", "--diff-filter=ACM")))
+ indexFiles := addRoot(repo, filter(gofmtRequired, nonBlankLines(cmdOutput("git", "diff", "--name-only", "--diff-filter=ACM", "--cached", branchpt, "--"))))
+ localFiles := addRoot(repo, filter(gofmtRequired, nonBlankLines(cmdOutput("git", "diff", "--name-only", "--diff-filter=ACM"))))
localFilesMap := stringMap(localFiles)
isUnstaged := func(file string) bool {
return localFilesMap[file]
@@ -203,7 +203,7 @@
}
args := []string{"checkout-index", "--temp", "--"}
args = append(args, needTemp[:n]...)
- for _, line := range getLines("git", args...) {
+ for _, line := range nonBlankLines(cmdOutput("git", args...)) {
i := strings.Index(line, "\t")
if i < 0 {
continue
@@ -266,10 +266,7 @@
}
// Build file list.
- files = strings.Split(stdout.String(), "\n")
- if len(files) > 0 && files[len(files)-1] == "" {
- files = files[:len(files)-1]
- }
+ files = lines(stdout.String())
// Restage files that need to be restaged.
if flags&gofmtWrite != 0 {
@@ -288,7 +285,7 @@
run("git", add...)
}
if len(updateIndex) > 0 {
- hashes := getLines("git", write...)
+ hashes := nonBlankLines(cmdOutput("git", write...))
if len(hashes) != len(write)-3 {
dief("git hash-object -w did not write expected number of objects")
}
diff --git a/git-codereview/mail.go b/git-codereview/mail.go
index c5ec7ce..a68cdea 100644
--- a/git-codereview/mail.go
+++ b/git-codereview/mail.go
@@ -172,7 +172,7 @@
return
}
countByAddr := map[string]int{}
- for _, line := range getLines("git", "log", "--format=format:%B") {
+ for _, line := range nonBlankLines(cmdOutput("git", "log", "--format=format:%B")) {
if strings.HasPrefix(line, "Reviewed-by:") {
f := strings.Fields(line)
addr := f[len(f)-1]
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 150d91f..1a58fe2 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -41,7 +41,7 @@
b.staged, b.unstaged, b.untracked = LocalChanges()
}
for _, c := range b.Pending() {
- c.committed = getLines("git", "diff", "--name-only", c.Parent, c.Hash, "--")
+ c.committed = nonBlankLines(cmdOutput("git", "diff", "--name-only", c.Parent, c.Hash, "--"))
if !pendingLocal {
c.g, c.gerr = b.GerritChange(c, "DETAILED_LABELS", "CURRENT_REVISION", "MESSAGES", "DETAILED_ACCOUNTS")
}
diff --git a/git-codereview/review.go b/git-codereview/review.go
index 5f4b890..b99aee5 100644
--- a/git-codereview/review.go
+++ b/git-codereview/review.go
@@ -191,13 +191,26 @@
return cmd.Run()
}
-// getOutput runs the specified command and returns its combined standard
-// output and standard error outputs.
-// It dies on command errors.
-// NOTE: It should only be used to run commands that return information,
-// **not** commands that make any actual changes.
-func getOutput(command string, args ...string) string {
- s, err := getOutputErr(command, args...)
+// cmdOutput runs the command line, returning its output.
+// If the command cannot be run or does not exit successfully,
+// cmdOutput dies.
+//
+// NOTE: cmdOutput must be used only to run commands that read state,
+// not for commands that make changes. Commands that make changes
+// should be run using runDirErr so that the -v and -n flags apply to them.
+func cmdOutput(command string, args ...string) string {
+ return cmdOutputDir(".", command, args...)
+}
+
+// cmdOutputDir runs the command line in dir, returning its output.
+// If the command cannot be run or does not exit successfully,
+// cmdOutput dies.
+//
+// NOTE: cmdOutput must be used only to run commands that read state,
+// not for commands that make changes. Commands that make changes
+// should be run using runDirErr so that the -v and -n flags apply to them.
+func cmdOutputDir(dir, command string, args ...string) string {
+ s, err := cmdOutputDirErr(dir, command, args...)
if err != nil {
fmt.Fprintf(stderr(), "%v\n%s\n", commandString(command, args), s)
dief("%v", err)
@@ -205,9 +218,23 @@
return s
}
-// Given a command and its arguments, getOutputErr returns the same
-// trimmed output as getOutput, but it returns any error instead of exiting.
-func getOutputErr(command string, args ...string) (string, error) {
+// cmdOutputErr runs the command line in dir, returning its output
+// and any error results.
+//
+// NOTE: cmdOutputErr must be used only to run commands that read state,
+// not for commands that make changes. Commands that make changes
+// should be run using runDirErr so that the -v and -n flags apply to them.
+func cmdOutputErr(command string, args ...string) (string, error) {
+ return cmdOutputDirErr(".", command, args...)
+}
+
+// cmdOutputDirErr runs the command line in dir, returning its output
+// and any error results.
+//
+// NOTE: cmdOutputDirErr must be used only to run commands that read state,
+// not for commands that make changes. Commands that make changes
+// should be run using runDirErr so that the -v and -n flags apply to them.
+func cmdOutputDirErr(dir, command string, args ...string) (string, error) {
// NOTE: We only show these non-state-modifying commands with -v -v.
// Otherwise things like 'git sync -v' show all our internal "find out about
// the git repo" commands, which is confusing if you are just trying to find
@@ -215,22 +242,44 @@
if *verbose > 1 {
fmt.Fprintln(stderr(), commandString(command, args))
}
- b, err := exec.Command(command, args...).CombinedOutput()
- return string(bytes.TrimSpace(b)), err
+ cmd := exec.Command(command, args...)
+ if dir != "." {
+ cmd.Dir = dir
+ }
+ b, err := cmd.CombinedOutput()
+ return string(b), err
}
-// getLines is like getOutput but it returns only non-empty output lines,
-// with leading and trailing spaces removed.
-// NOTE: It should only be used to run commands that return information,
-// **not** commands that make any actual changes.
-func getLines(command string, args ...string) []string {
- var s []string
- for _, l := range strings.Split(getOutput(command, args...), "\n") {
- if len(strings.TrimSpace(l)) > 0 {
- s = append(s, l)
+// trim is shorthand for strings.TrimSpace.
+func trim(text string) string {
+ return strings.TrimSpace(text)
+}
+
+// trimErr applies strings.TrimSpace to the result of cmdOutput(Dir)Err,
+// passing the error along unmodified.
+func trimErr(text string, err error) (string, error) {
+ return strings.TrimSpace(text), err
+}
+
+// lines returns the lines in text.
+func lines(text string) []string {
+ out := strings.Split(text, "\n")
+ // Split will include a "" after the last line. Remove it.
+ if n := len(out) - 1; n >= 0 && out[n] == "" {
+ out = out[:n]
+ }
+ return out
+}
+
+// nonBlankLines returns the non-blank lines in text.
+func nonBlankLines(text string) []string {
+ var out []string
+ for _, s := range lines(text) {
+ if strings.TrimSpace(s) != "" {
+ out = append(out, s)
}
}
- return s
+ return out
}
func commandString(command string, args []string) string {
