use different timeouts for all vcs commands

both `runFromRepoDir` and `runFromCwd` accept now a timeout to control
the timeouts of the underlying `monitoredCmd`, instead of having a
hardcoded timeout of 2 minutes for all commands.
This will allow better tuning of the timeouts on a per-command basis.

Signed-off-by: Miguel Molina <[email protected]>

Author: erizocosmico

internal/gps/cmd.go

@@ -136,12 +136,22 @@ func (e killCmdError) Error() string {
 	return fmt.Sprintf("error killing command: %s", e.err)
 }
 
-func runFromCwd(ctx context.Context, cmd string, args ...string) ([]byte, error) {
-	c := newMonitoredCmd(exec.Command(cmd, args...), 2*time.Minute)
+func runFromCwd(ctx context.Context, timeout time.Duration, cmd string, args ...string) ([]byte, error) {
+	c := newMonitoredCmd(exec.Command(cmd, args...), timeout)
 	return c.combinedOutput(ctx)
 }
 
-func runFromRepoDir(ctx context.Context, repo vcs.Repo, cmd string, args ...string) ([]byte, error) {
-	c := newMonitoredCmd(repo.CmdFromDir(cmd, args...), 2*time.Minute)
+func runFromRepoDir(ctx context.Context, repo vcs.Repo, timeout time.Duration, cmd string, args ...string) ([]byte, error) {
+	c := newMonitoredCmd(repo.CmdFromDir(cmd, args...), timeout)
 	return c.combinedOutput(ctx)
 }
+
+const (
+	// expensiveCmdTimeout is meant to be used in a command that is expensive
+	// in terms of computation and we know it will take long or one that uses
+	// the network, such as clones, updates, ....
+	expensiveCmdTimeout = 2 * time.Minute
+	// defaultCmdTimeout is just an umbrella value for all other commands that
+	// should not take much.
+	defaultCmdTimeout = 10 * time.Second
+)

internal/gps/vcs_repo.go

@@ -80,7 +80,7 @@ func newVcsLocalErrorOr(msg string, err error, out string) error {
 }
 
 func (r *gitRepo) get(ctx context.Context) error {
-	out, err := runFromCwd(ctx, "git", "clone", "--recursive", "-v", "--progress", r.Remote(), r.LocalPath())
+	out, err := runFromCwd(ctx, expensiveCmdTimeout, "git", "clone", "--recursive", "-v", "--progress", r.Remote(), r.LocalPath())
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to get repository", err, string(out))
 	}
@@ -90,15 +90,15 @@ func (r *gitRepo) get(ctx context.Context) error {
 
 func (r *gitRepo) fetch(ctx context.Context) error {
 	// Perform a fetch to make sure everything is up to date.
-	out, err := runFromRepoDir(ctx, r, "git", "fetch", "--tags", "--prune", r.RemoteLocation)
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "fetch", "--tags", "--prune", r.RemoteLocation)
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update repository", err, string(out))
 	}
 	return nil
 }
 
 func (r *gitRepo) updateVersion(ctx context.Context, v string) error {
-	out, err := runFromRepoDir(ctx, r, "git", "checkout", v)
+	out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "checkout", v)
 	if err != nil {
 		return newVcsLocalErrorOr("Unable to update checked out version", err, string(out))
 	}
@@ -110,20 +110,20 @@ func (r *gitRepo) updateVersion(ctx context.Context, v string) error {
 // submodules. Or nested submodules. What a great idea, submodules.
 func (r *gitRepo) defendAgainstSubmodules(ctx context.Context) error {
 	// First, update them to whatever they should be, if there should happen to be any.
-	out, err := runFromRepoDir(ctx, r, "git", "submodule", "update", "--init", "--recursive")
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "git", "submodule", "update", "--init", "--recursive")
 	if err != nil {
 		return newVcsLocalErrorOr("unexpected error while defensively updating submodules", err, string(out))
 	}
 
 	// Now, do a special extra-aggressive clean in case changing versions caused
 	// one or more submodules to go away.
-	out, err = runFromRepoDir(ctx, r, "git", "clean", "-x", "-d", "-f", "-f")
+	out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "clean", "-x", "-d", "-f", "-f")
 	if err != nil {
 		return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict submodule directories", err, string(out))
 	}
 
 	// Then, repeat just in case there are any nested submodules that went away.
-	out, err = runFromRepoDir(ctx, r, "git", "submodule", "foreach", "--recursive", "git", "clean", "-x", "-d", "-f", "-f")
+	out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "submodule", "foreach", "--recursive", "git", "clean", "-x", "-d", "-f", "-f")
 	if err != nil {
 		return newVcsLocalErrorOr("unexpected error while defensively cleaning up after possible derelict nested submodule directories", err, string(out))
 	}
@@ -144,7 +144,7 @@ func (r *bzrRepo) get(ctx context.Context) error {
 		}
 	}
 
-	out, err := runFromCwd(ctx, "bzr", "branch", r.Remote(), r.LocalPath())
+	out, err := runFromCwd(ctx, expensiveCmdTimeout, "bzr", "branch", r.Remote(), r.LocalPath())
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to get repository", err, string(out))
 	}
@@ -153,15 +153,15 @@ func (r *bzrRepo) get(ctx context.Context) error {
 }
 
 func (r *bzrRepo) fetch(ctx context.Context) error {
-	out, err := runFromRepoDir(ctx, r, "bzr", "pull")
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "pull")
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update repository", err, string(out))
 	}
 	return nil
 }
 
 func (r *bzrRepo) updateVersion(ctx context.Context, version string) error {
-	out, err := runFromRepoDir(ctx, r, "bzr", "update", "-r", version)
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "bzr", "update", "-r", version)
 	if err != nil {
 		return newVcsLocalErrorOr("unable to update checked out version", err, string(out))
 	}
@@ -173,7 +173,7 @@ type hgRepo struct {
 }
 
 func (r *hgRepo) get(ctx context.Context) error {
-	out, err := runFromCwd(ctx, "hg", "clone", r.Remote(), r.LocalPath())
+	out, err := runFromCwd(ctx, expensiveCmdTimeout, "hg", "clone", r.Remote(), r.LocalPath())
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to get repository", err, string(out))
 	}
@@ -182,15 +182,15 @@ func (r *hgRepo) get(ctx context.Context) error {
 }
 
 func (r *hgRepo) fetch(ctx context.Context) error {
-	out, err := runFromRepoDir(ctx, r, "hg", "pull")
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "pull")
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to fetch latest changes", err, string(out))
 	}
 	return nil
 }
 
 func (r *hgRepo) updateVersion(ctx context.Context, version string) error {
-	out, err := runFromRepoDir(ctx, r, "hg", "update", version)
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "hg", "update", version)
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update checked out version", err, string(out))
 	}
@@ -210,7 +210,7 @@ func (r *svnRepo) get(ctx context.Context) error {
 		remote = "file:///" + remote
 	}
 
-	out, err := runFromCwd(ctx, "svn", "checkout", remote, r.LocalPath())
+	out, err := runFromCwd(ctx, expensiveCmdTimeout, "svn", "checkout", remote, r.LocalPath())
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to get repository", err, string(out))
 	}
@@ -219,7 +219,7 @@ func (r *svnRepo) get(ctx context.Context) error {
 }
 
 func (r *svnRepo) update(ctx context.Context) error {
-	out, err := runFromRepoDir(ctx, r, "svn", "update")
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update")
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update repository", err, string(out))
 	}
@@ -228,7 +228,7 @@ func (r *svnRepo) update(ctx context.Context) error {
 }
 
 func (r *svnRepo) updateVersion(ctx context.Context, version string) error {
-	out, err := runFromRepoDir(ctx, r, "svn", "update", "-r", version)
+	out, err := runFromRepoDir(ctx, r, expensiveCmdTimeout, "svn", "update", "-r", version)
 	if err != nil {
 		return newVcsRemoteErrorOr("unable to update checked out version", err, string(out))
 	}
@@ -250,7 +250,7 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) {
 			Commit commit `xml:"entry>commit"`
 		}
 
-		out, err := runFromRepoDir(ctx, r, "svn", "info", "-r", id, "--xml")
+		out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "info", "-r", id, "--xml")
 		if err != nil {
 			return nil, newVcsLocalErrorOr("unable to retrieve commit information", err, string(out))
 		}
@@ -267,7 +267,7 @@ func (r *svnRepo) CommitInfo(id string) (*vcs.CommitInfo, error) {
 		}
 	}
 
-	out, err := runFromRepoDir(ctx, r, "svn", "log", "-r", id, "--xml")
+	out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "svn", "log", "-r", id, "--xml")
 	if err != nil {
 		return nil, newVcsRemoteErrorOr("unable to retrieve commit information", err, string(out))
 	}

internal/gps/vcs_source.go

@@ -136,7 +136,7 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin
 	// could have an err here...but it's hard to imagine how?
 	defer fs.RenameWithFallback(bak, idx)
 
-	out, err := runFromRepoDir(ctx, r, "git", "read-tree", rev.String())
+	out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "read-tree", rev.String())
 	if err != nil {
 		return fmt.Errorf("%s: %s", out, err)
 	}
@@ -152,7 +152,7 @@ func (s *gitSource) exportRevisionTo(ctx context.Context, rev Revision, to strin
 	// though we have a bunch of housekeeping to do to set up, then tear
 	// down, the sparse checkout controls, as well as restore the original
 	// index and HEAD.
-	out, err = runFromRepoDir(ctx, r, "git", "checkout-index", "-a", "--prefix="+to)
+	out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "git", "checkout-index", "-a", "--prefix="+to)
 	if err != nil {
 		return fmt.Errorf("%s: %s", out, err)
 	}
@@ -365,15 +365,15 @@ func (s *bzrSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 	}
 
 	// Now, list all the tags
-	out, err := runFromRepoDir(ctx, r, "bzr", "tags", "--show-ids", "-v")
+	out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "tags", "--show-ids", "-v")
 	if err != nil {
 		return nil, fmt.Errorf("%s: %s", err, string(out))
 	}
 
 	all := bytes.Split(bytes.TrimSpace(out), []byte("\n"))
 
 	var branchrev []byte
-	branchrev, err = runFromRepoDir(ctx, r, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.")
+	branchrev, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "bzr", "version-info", "--custom", "--template={revision_id}", "--revision=branch:.")
 	br := string(branchrev)
 	if err != nil {
 		return nil, fmt.Errorf("%s: %s", err, br)
@@ -416,7 +416,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 	}
 
 	// Now, list all the tags
-	out, err := runFromRepoDir(ctx, r, "hg", "tags", "--debug", "--verbose")
+	out, err := runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "tags", "--debug", "--verbose")
 	if err != nil {
 		return nil, fmt.Errorf("%s: %s", err, string(out))
 	}
@@ -450,7 +450,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 	// bookmarks next, because the presence of the magic @ bookmark has to
 	// determine how we handle the branches
 	var magicAt bool
-	out, err = runFromRepoDir(ctx, r, "hg", "bookmarks", "--debug")
+	out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "bookmarks", "--debug")
 	if err != nil {
 		// better nothing than partial and misleading
 		return nil, fmt.Errorf("%s: %s", err, string(out))
@@ -483,7 +483,7 @@ func (s *hgSource) listVersions(ctx context.Context) ([]PairedVersion, error) {
 		}
 	}
 
-	out, err = runFromRepoDir(ctx, r, "hg", "branches", "-c", "--debug")
+	out, err = runFromRepoDir(ctx, r, defaultCmdTimeout, "hg", "branches", "-c", "--debug")
 	if err != nil {
 		// better nothing than partial and misleading
 		return nil, fmt.Errorf("%s: %s", err, string(out))