From 77734205b82e409224858eb44aba458df143a5cc Mon Sep 17 00:00:00 2001 From: Brett Randall Date: Wed, 27 Apr 2016 09:24:00 +1000 Subject: [PATCH] Stop SimpleExec swallowing errors. Fixed #1183. ExitError conditions now return a plain error with message like: Error running git [config --system filter.lfs.clean git-lfs clean -- %f]: 'error: could not lock config file /etc/gitconfig: Permission denied' 'exit status 255' Also: * Made Set/Unset Config commands return errors when they occur. * Made install attribute set fail properly with an error message e.g. if permission-denied. Previously this failed silently and reported success. ***NOTE*** this commit contains Go 1.6-specific APIs. The following commit will remove those, and that following commit can be reverted when Go >= 1.6 is required. --- git/git.go | 32 ++++++++++++++++---------------- lfs/attribute.go | 13 ++++++++----- subprocess/subprocess.go | 18 ++++++++++++------ 3 files changed, 36 insertions(+), 27 deletions(-) diff --git a/git/git.go b/git/git.go index a37967fb..e32e1ff6 100644 --- a/git/git.go +++ b/git/git.go @@ -304,55 +304,55 @@ func (c *gitConfig) FindLocal(val string) string { } // SetGlobal sets the git config value for the key in the global config -func (c *gitConfig) SetGlobal(key, val string) { - subprocess.SimpleExec("git", "config", "--global", key, val) +func (c *gitConfig) SetGlobal(key, val string) (string, error) { + return subprocess.SimpleExec("git", "config", "--global", key, val) } // SetSystem sets the git config value for the key in the system config -func (c *gitConfig) SetSystem(key, val string) { - subprocess.SimpleExec("git", "config", "--system", key, val) +func (c *gitConfig) SetSystem(key, val string) (string, error) { + return subprocess.SimpleExec("git", "config", "--system", key, val) } // UnsetGlobal removes the git config value for the key from the global config -func (c *gitConfig) UnsetGlobal(key string) { - subprocess.SimpleExec("git", "config", "--global", "--unset", key) +func (c *gitConfig) UnsetGlobal(key string) (string, error) { + return subprocess.SimpleExec("git", "config", "--global", "--unset", key) } // UnsetSystem removes the git config value for the key from the system config -func (c *gitConfig) UnsetSystem(key string) { - subprocess.SimpleExec("git", "config", "--system", "--unset", key) +func (c *gitConfig) UnsetSystem(key string) (string, error) { + return subprocess.SimpleExec("git", "config", "--system", "--unset", key) } // UnsetGlobalSection removes the entire named section from the global config -func (c *gitConfig) UnsetGlobalSection(key string) { - subprocess.SimpleExec("git", "config", "--global", "--remove-section", key) +func (c *gitConfig) UnsetGlobalSection(key string) (string, error) { + return subprocess.SimpleExec("git", "config", "--global", "--remove-section", key) } // UnsetGlobalSection removes the entire named section from the system config -func (c *gitConfig) UnsetSystemSection(key string) { - subprocess.SimpleExec("git", "config", "--system", "--remove-section", key) +func (c *gitConfig) UnsetSystemSection(key string) (string, error) { + return subprocess.SimpleExec("git", "config", "--system", "--remove-section", key) } // SetLocal sets the git config value for the key in the specified config file -func (c *gitConfig) SetLocal(file, key, val string) { +func (c *gitConfig) SetLocal(file, key, val string) (string, error) { args := make([]string, 1, 5) args[0] = "config" if len(file) > 0 { args = append(args, "--file", file) } args = append(args, key, val) - subprocess.SimpleExec("git", args...) + return subprocess.SimpleExec("git", args...) } // UnsetLocalKey removes the git config value for the key from the specified config file -func (c *gitConfig) UnsetLocalKey(file, key string) { +func (c *gitConfig) UnsetLocalKey(file, key string) (string, error) { args := make([]string, 1, 5) args[0] = "config" if len(file) > 0 { args = append(args, "--file", file) } args = append(args, "--unset", key) - subprocess.SimpleExec("git", args...) + return subprocess.SimpleExec("git", args...) } // List lists all of the git config values diff --git a/lfs/attribute.go b/lfs/attribute.go index 5a589c40..cef8d946 100644 --- a/lfs/attribute.go +++ b/lfs/attribute.go @@ -74,18 +74,21 @@ func (a *Attribute) set(key, value string, opt InstallOptions) error { } if opt.Force || shouldReset(currentValue) { + var err error if opt.Local { + // ignore error for unset, git returns non-zero if missing git.Config.UnsetLocalKey("", key) - git.Config.SetLocal("", key, value) + _, err = git.Config.SetLocal("", key, value) } else if opt.System { + // ignore error for unset, git returns non-zero if missing git.Config.UnsetSystem(key) - git.Config.SetSystem(key, value) + _, err = git.Config.SetSystem(key, value) } else { + // ignore error for unset, git returns non-zero if missing git.Config.UnsetGlobal(key) - git.Config.SetGlobal(key, value) + _, err = git.Config.SetGlobal(key, value) } - - return nil + return err } else if currentValue != value { return fmt.Errorf("The %s attribute should be \"%s\" but is \"%s\"", key, value, currentValue) diff --git a/subprocess/subprocess.go b/subprocess/subprocess.go index 826a3471..1f1e508a 100644 --- a/subprocess/subprocess.go +++ b/subprocess/subprocess.go @@ -17,14 +17,20 @@ func SimpleExec(name string, args ...string) (string, error) { cmd := ExecCommand(name, args...) output, err := cmd.Output() - if _, ok := err.(*exec.ExitError); ok { - return "", nil - } - if err != nil { - return fmt.Sprintf("Error running %s %s", name, args), err + if exitError, ok := err.(*exec.ExitError); ok { + errorOutput := strings.TrimSpace(string(exitError.Stderr)) + if errorOutput == "" { + // some commands might write nothing to stderr but something to stdout in error-conditions, in which case, we'll use that + // in the error string + errorOutput = strings.TrimSpace(string(output)) + } + formattedErr := fmt.Errorf("Error running %s %s: '%s' '%s'", name, args, errorOutput, strings.TrimSpace(exitError.Error())) + + // return "" as output in error case, for callers that don't care about errors but rely on "" returned, in-case stdout != "" + return "", formattedErr } - return strings.Trim(string(output), " \n"), nil + return strings.Trim(string(output), " \n"), err } // An env for an exec.Command without GIT_TRACE