From b7d740ac6db03127516e5b386aa7a6a69d08494b Mon Sep 17 00:00:00 2001 From: Herman Schaaf Date: Thu, 28 Apr 2016 20:11:24 +0800 Subject: [PATCH] Fix edge case where we can not go get the repo This fixes an edge case where the repo being tested has a dependency that has had its git history rebased, thus causing go get to fail, and a subsequent delete of that repo and another go get to fail as well. Now, if go get fails the first time, we read the error message and clear the directory that caused the error. Then we try again. If we get the same error two times in a row, something is wrong, and we return failure. If not, we keep going and deleting cached repos until no error is returned. --- handlers/checks.go | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-) diff --git a/handlers/checks.go b/handlers/checks.go index fdc35fc..df80136 100644 --- a/handlers/checks.go +++ b/handlers/checks.go @@ -9,6 +9,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "sort" "strings" "time" @@ -18,6 +19,8 @@ import ( "github.com/gojp/goreportcard/check" ) +var reBadRepo = regexp.MustCompile(`package\s([\w\/\.]+)\: exit status \d+`) + func dirName(repo string) string { return fmt.Sprintf("repos/src/%s", repo) } @@ -77,7 +80,7 @@ type checksResp struct { HumanizedLastRefresh string `json:"humanized_last_refresh"` } -func goGet(repo string, firstAttempt bool) error { +func goGet(repo string, prevError string) error { log.Printf("Go getting %q...", repo) if err := os.Mkdir("repos", 0755); err != nil && !os.IsExist(err) { return fmt.Errorf("could not create dir: %v", err) @@ -106,17 +109,41 @@ func goGet(repo string, firstAttempt bool) error { } err = cmd.Wait() + errStr := string(b) + // we don't care if there are no buildable Go source files, we just need the source on disk - if err != nil && !strings.Contains(string(b), "no buildable Go source files") { + hadError := err != nil && !strings.Contains(errStr, "no buildable Go source files") + + if hadError { log.Println("Go get error log:", string(b)) - if firstAttempt { - // try one more time, this time deleting the cached directory first, - // in case our cache is stale (remote repository was force-pushed, replaced, etc) + if errStr != prevError { + // try again, this time deleting the cached directory, and also the + // package that caused the error in case our cache is stale + // (remote repository or one of its dependencices was force-pushed, + // replaced, etc) err = os.RemoveAll(filepath.Join(d, "src", repo)) if err != nil { return fmt.Errorf("could not delete repo: %v", err) } - return goGet(repo, false) + + packageNames := reBadRepo.FindStringSubmatch(errStr) + if len(packageNames) >= 2 { + pkg := packageNames[1] + fp := filepath.Clean(filepath.Join(d, "src", pkg)) + if strings.HasPrefix(fp, filepath.Join(d, "src")) { + // if the path is prefixed with the path to our + // cached repos, then it's safe to delete it. + // These precautions are here so that someone can't + // craft a malicious package name with .. in it + // and cause us to delete our server's root directory. + log.Println("Cleaning out rebased dependency:", fp) + err = os.RemoveAll(fp) + if err != nil { + return err + } + } + } + return goGet(repo, errStr) } return fmt.Errorf("could not run go get: %v", err) @@ -137,7 +164,7 @@ func newChecksResp(repo string, forceRefresh bool) (checksResp, error) { } // fetch the repo and grade it - err := goGet(repo, true) + err := goGet(repo, "") if err != nil { return checksResp{}, fmt.Errorf("could not clone repo: %v", err) }