Replace the the "force" flag with an error

The force flag was an ugly way of doing this, anyway. By returning an
error, CouldNotCreateLockError, that clearly exposes the path to the
lock file, we give the calling code the power to resolve the situation
much more sanely than we can within the library.

manager_test.go

@@ -44,7 +44,7 @@ func mkNaiveSM(t *testing.T) (*SourceMgr, func()) {
 		t.FailNow()
 	}
 
-	sm, err := NewSourceManager(naiveAnalyzer{}, cpath, false)
+	sm, err := NewSourceManager(naiveAnalyzer{}, cpath)
 	if err != nil {
 		t.Errorf("Unexpected error on SourceManager creation: %s", err)
 		t.FailNow()
@@ -69,37 +69,44 @@ func TestSourceManagerInit(t *testing.T) {
 	if err != nil {
 		t.Errorf("Failed to create temp dir: %s", err)
 	}
-	sm, err := NewSourceManager(naiveAnalyzer{}, cpath, false)
+	sm, err := NewSourceManager(naiveAnalyzer{}, cpath)
 
 	if err != nil {
 		t.Errorf("Unexpected error on SourceManager creation: %s", err)
 	}
-	defer func() {
-		sm.Release()
-		err := removeAll(cpath)
-		if err != nil {
-			t.Errorf("removeAll failed: %s", err)
-		}
-	}()
 
-	_, err = NewSourceManager(naiveAnalyzer{}, cpath, false)
+	_, err = NewSourceManager(naiveAnalyzer{}, cpath)
 	if err == nil {
 		t.Errorf("Creating second SourceManager should have failed due to file lock contention")
-	}
-
-	sm, err = NewSourceManager(naiveAnalyzer{}, cpath, true)
-	if err != nil {
-		t.Errorf("Creating second SourceManager should have succeeded when force flag was passed, but failed with err %s", err)
+	} else if te, ok := err.(CouldNotCreateLockError); !ok {
+		t.Errorf("Should have gotten CouldNotCreateLockError error type, but got %T", te)
 	}
 
 	if _, err = os.Stat(path.Join(cpath, "sm.lock")); err != nil {
 		t.Errorf("Global cache lock file not created correctly")
-		t.FailNow()
 	}
 
 	sm.Release()
+	err = removeAll(cpath)
+	if err != nil {
+		t.Errorf("removeAll failed: %s", err)
+	}
+
 	if _, err = os.Stat(path.Join(cpath, "sm.lock")); !os.IsNotExist(err) {
 		t.Errorf("Global cache lock file not cleared correctly on Release()")
+		t.FailNow()
+	}
+
+	// Set another one up at the same spot now, just to be sure
+	sm, err = NewSourceManager(naiveAnalyzer{}, cpath)
+	if err != nil {
+		t.Errorf("Creating a second SourceManager should have succeeded when the first was released, but failed with err %s", err)
+	}
+
+	sm.Release()
+	err = removeAll(cpath)
+	if err != nil {
+		t.Errorf("removeAll failed: %s", err)
 	}
 }
 
@@ -115,7 +122,7 @@ func TestSourceInit(t *testing.T) {
 		t.FailNow()
 	}
 
-	sm, err := NewSourceManager(naiveAnalyzer{}, cpath, false)
+	sm, err := NewSourceManager(naiveAnalyzer{}, cpath)
 	if err != nil {
 		t.Errorf("Unexpected error on SourceManager creation: %s", err)
 		t.FailNow()

result_test.go

@@ -73,7 +73,7 @@ func BenchmarkCreateVendorTree(b *testing.B) {
 	tmp := path.Join(os.TempDir(), "vsolvtest")
 
 	clean := true
-	sm, err := NewSourceManager(naiveAnalyzer{}, path.Join(tmp, "cache"), true)
+	sm, err := NewSourceManager(naiveAnalyzer{}, path.Join(tmp, "cache"))
 	if err != nil {
 		b.Errorf("NewSourceManager errored unexpectedly: %q", err)
 		clean = false
@@ -81,7 +81,7 @@ func BenchmarkCreateVendorTree(b *testing.B) {
 
 	// Prefetch the projects before timer starts
 	for _, lp := range r.p {
-		_, _, err := sm.GetManifestAndLock(lp.Ident(), lp.Version())
+		err := sm.SyncSourceFor(lp.Ident())
 		if err != nil {
 			b.Errorf("failed getting project info during prefetch: %s", err)
 			clean = false

source_manager.go

@@ -95,22 +95,19 @@ var _ SourceManager = &SourceMgr{}
 
 // NewSourceManager produces an instance of gps's built-in SourceManager. It
 // takes a cache directory (where local instances of upstream repositories are
-// stored), a vendor directory for the project currently being worked on, and a
-// force flag indicating whether to overwrite the global cache lock file (if
-// present).
+// stored), and a ProjectAnalyzer that is used to extract manifest and lock
+// information from source trees.
 //
 // The returned SourceManager aggressively caches information wherever possible.
-// It is recommended that, if tools need to do preliminary, work involving
-// upstream repository analysis prior to invoking a solve run, that they create
-// this SourceManager as early as possible and use it to their ends. That way,
-// the solver can benefit from any caches that may have already been warmed.
+// If tools need to do preliminary work involving upstream repository analysis
+// prior to invoking a solve run, it is recommended that they create this
+// SourceManager as early as possible and use it to their ends. That way, the
+// solver can benefit from any caches that may have already been warmed.
 //
-// gps's SourceManager is intended to be threadsafe (if it's not, please
-// file a bug!). It should certainly be safe to reuse from one solving run to
-// the next; however, the fact that it takes a basedir as an argument makes it
-// much less useful for simultaneous use by separate solvers operating on
-// different root projects. This architecture may change in the future.
-func NewSourceManager(an ProjectAnalyzer, cachedir string, force bool) (*SourceMgr, error) {
+// gps's SourceManager is intended to be threadsafe (if it's not, please file a
+// bug!). It should be safe to reuse across concurrent solving runs, even on
+// unrelated projects.
+func NewSourceManager(an ProjectAnalyzer, cachedir string) (*SourceMgr, error) {
 	if an == nil {
 		return nil, fmt.Errorf("a ProjectAnalyzer must be provided to the SourceManager")
 	}
@@ -122,13 +119,19 @@ func NewSourceManager(an ProjectAnalyzer, cachedir string, force bool) (*SourceM
 
 	glpath := filepath.Join(cachedir, "sm.lock")
 	_, err = os.Stat(glpath)
-	if err == nil && !force {
-		return nil, fmt.Errorf("cache lock file %s exists - another process crashed or is still running?", glpath)
+	if err == nil {
+		return nil, CouldNotCreateLockError{
+			Path: glpath,
+			Err:  fmt.Errorf("cache lock file %s exists - another process crashed or is still running?", glpath),
+		}
 	}
 
-	fi, err := os.OpenFile(glpath, os.O_CREATE, 0600) // is 0600 sane for this purpose?
+	fi, err := os.OpenFile(glpath, os.O_CREATE|os.O_EXCL, 0600) // is 0600 sane for this purpose?
 	if err != nil {
-		return nil, fmt.Errorf("failed to create global cache lock file at %s with err %s", glpath, err)
+		return nil, CouldNotCreateLockError{
+			Path: glpath,
+			Err:  fmt.Errorf("err on attempting to create global cache lock: %s", err),
+		}
 	}
 
 	return &SourceMgr{
@@ -141,6 +144,15 @@ func NewSourceManager(an ProjectAnalyzer, cachedir string, force bool) (*SourceM
 	}, nil
 }
 
+type CouldNotCreateLockError struct {
+	Path string
+	Err  error
+}
+
+func (e CouldNotCreateLockError) Error() string {
+	return e.Err.Error()
+}
+
 // Release lets go of any locks held by the SourceManager.
 func (sm *SourceMgr) Release() {
 	sm.lf.Close()