Remove fsrepo.At, make Open a constructor function

Nobody calls At without immediately calling Open.

First step, a mechanical transformation. Cleanups will follow.

cmd/ipfs/daemon.go

@@ -118,8 +118,8 @@ func daemonFunc(req cmds.Request, res cmds.Response) {
 
 	// acquire the repo lock _before_ constructing a node. we need to make
 	// sure we are permitted to access the resources (datastore, etc.)
-	repo := fsrepo.At(req.Context().ConfigRoot)
+	repo, err := fsrepo.Open(req.Context().ConfigRoot)
-	if err := repo.Open(); err != nil {
+	if err != nil {
 		res.SetError(debugerror.Errorf("Couldn't obtain lock. Is another daemon already running?"), cmds.ErrNormal)
 		return
 	}

cmd/ipfs/init.go

@@ -110,8 +110,8 @@ func addDefaultAssets(out io.Writer, repoRoot string) error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	r := fsrepo.At(repoRoot)
+	r, err := fsrepo.Open(repoRoot)
-	if err := r.Open(); err != nil { // NB: repo is owned by the node
+	if err != nil { // NB: repo is owned by the node
 		return err
 	}
 
@@ -163,8 +163,8 @@ func initializeIpnsKeyspace(repoRoot string) error {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	r := fsrepo.At(repoRoot)
+	r, err := fsrepo.Open(repoRoot)
-	if err := r.Open(); err != nil { // NB: repo is owned by the node
+	if err != nil { // NB: repo is owned by the node
 		return err
 	}
 

cmd/ipfs/main.go

@@ -193,8 +193,8 @@ func (i *cmdInvocation) constructNodeFunc(ctx context.Context) func() (*core.Ipf
 			return nil, errors.New("constructing node without a request context")
 		}
 
-		r := fsrepo.At(i.req.Context().ConfigRoot)
+		r, err := fsrepo.Open(i.req.Context().ConfigRoot)
-		if err := r.Open(); err != nil { // repo is owned by the node
+		if err != nil { // repo is owned by the node
 			return nil, err
 		}
 

cmd/ipfs/tour.go

@@ -193,8 +193,8 @@ func tourGet(id tour.ID) (*tour.Topic, error) {
 // TODO share func
 func writeConfig(path string, cfg *config.Config) error {
 	// NB: This needs to run on the daemon.
-	r := fsrepo.At(path)
+	r, err := fsrepo.Open(path)
-	if err := r.Open(); err != nil {
+	if err != nil {
 		return err
 	}
 	defer r.Close()

cmd/ipfs_bootstrapd/main.go

@@ -57,8 +57,8 @@ func run() error {
 		}
 	}
 
-	repo := fsrepo.At(repoPath)
+	repo, err := fsrepo.Open(repoPath)
-	if err := repo.Open(); err != nil { // owned by node
+	if err != nil { // owned by node
 		return err
 	}
 

cmd/ipfs_routingd/main.go

@@ -57,8 +57,8 @@ func run() error {
 			return err
 		}
 	}
-	repo := fsrepo.At(repoPath)
+	repo, err := fsrepo.Open(repoPath)
-	if err := repo.Open(); err != nil { // owned by node
+	if err != nil { // owned by node
 		return err
 	}
 

cmd/ipfswatch/main.go

@@ -65,8 +65,8 @@ func run(ipfsPath, watchPath string) error {
 		return err
 	}
 
-	r := fsrepo.At(ipfsPath)
+	r, err := fsrepo.Open(ipfsPath)
-	if err := r.Open(); err != nil {
+	if err != nil {
 		// TODO handle case: daemon running
 		// TODO handle case: repo doesn't exist or isn't initialized
 		return err

core/commands/bootstrap.go

@@ -66,8 +66,8 @@ in the bootstrap list).
 			return
 		}
 
-		r := fsrepo.At(req.Context().ConfigRoot)
+		r, err := fsrepo.Open(req.Context().ConfigRoot)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
@@ -143,8 +143,8 @@ var bootstrapRemoveCmd = &cmds.Command{
 			return
 		}
 
-		r := fsrepo.At(req.Context().ConfigRoot)
+		r, err := fsrepo.Open(req.Context().ConfigRoot)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
@@ -192,8 +192,8 @@ var bootstrapListCmd = &cmds.Command{
 	},
 
 	Run: func(req cmds.Request, res cmds.Response) {
-		r := fsrepo.At(req.Context().ConfigRoot)
+		r, err := fsrepo.Open(req.Context().ConfigRoot)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}

core/commands/config.go

@@ -64,14 +64,13 @@ Set the value of the 'datastore.path' key:
 		args := req.Arguments()
 		key := args[0]
 
-		r := fsrepo.At(req.Context().ConfigRoot)
+		r, err := fsrepo.Open(req.Context().ConfigRoot)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}
 		defer r.Close()
 
-		var err error
 		var output *ConfigField
 		if len(args) == 2 {
 			value := args[1]
@@ -182,8 +181,8 @@ can't be undone.
 		cmds.FileArg("file", true, false, "The file to use as the new config"),
 	},
 	Run: func(req cmds.Request, res cmds.Response) {
-		r := fsrepo.At(req.Context().ConfigRoot)
+		r, err := fsrepo.Open(req.Context().ConfigRoot)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			res.SetError(err, cmds.ErrNormal)
 			return
 		}

repo/fsrepo/fsrepo.go

@@ -75,13 +75,51 @@ type FSRepo struct {
 
 var _ repo.Repo = (*FSRepo)(nil)
 
-// At returns a handle to an FSRepo at the provided |path|.
+// Open the FSRepo at path. Returns an error if the repo is not
-func At(repoPath string) *FSRepo {
+// initialized.
-	// This method must not have side-effects.
+func Open(repoPath string) (*FSRepo, error) {
-	return &FSRepo{
+	packageLock.Lock()
+	defer packageLock.Unlock()
+
+	r := &FSRepo{
 		path:  path.Clean(repoPath),
 		state: unopened, // explicitly set for clarity
 	}
+
+	expPath, err := u.TildeExpansion(r.path)
+	if err != nil {
+		return nil, err
+	}
+	r.path = expPath
+
+	if r.state != unopened {
+		return nil, debugerror.Errorf("repo is %s", r.state)
+	}
+	if !isInitializedUnsynced(r.path) {
+		return nil, debugerror.New("ipfs not initialized, please run 'ipfs init'")
+	}
+	// check repo path, then check all constituent parts.
+	// TODO acquire repo lock
+	// TODO if err := initCheckDir(logpath); err != nil { // }
+	if err := dir.Writable(r.path); err != nil {
+		return nil, err
+	}
+
+	if err := r.openConfig(); err != nil {
+		return nil, err
+	}
+
+	if err := r.openDatastore(); err != nil {
+		return nil, err
+	}
+
+	// log.Debugf("writing eventlogs to ...", c.path)
+	configureEventLoggerAtRepoPath(r.config, r.path)
+
+	if err := r.transitionToOpened(); err != nil {
+		return nil, err
+	}
+	return r, nil
 }
 
 // ConfigAt returns an error if the FSRepo at the given path is not
@@ -236,45 +274,6 @@ func configureEventLoggerAtRepoPath(c *config.Config, repoPath string) {
 	eventlog.Configure(eventlog.OutputRotatingLogFile(rotateConf))
 }
 
-// Open returns an error if the repo is not initialized.
-func (r *FSRepo) Open() error {
-
-	packageLock.Lock()
-	defer packageLock.Unlock()
-
-	expPath, err := u.TildeExpansion(r.path)
-	if err != nil {
-		return err
-	}
-	r.path = expPath
-
-	if r.state != unopened {
-		return debugerror.Errorf("repo is %s", r.state)
-	}
-	if !isInitializedUnsynced(r.path) {
-		return debugerror.New("ipfs not initialized, please run 'ipfs init'")
-	}
-	// check repo path, then check all constituent parts.
-	// TODO acquire repo lock
-	// TODO if err := initCheckDir(logpath); err != nil { // }
-	if err := dir.Writable(r.path); err != nil {
-		return err
-	}
-
-	if err := r.openConfig(); err != nil {
-		return err
-	}
-
-	if err := r.openDatastore(); err != nil {
-		return err
-	}
-
-	// log.Debugf("writing eventlogs to ...", c.path)
-	configureEventLoggerAtRepoPath(r.config, r.path)
-
-	return r.transitionToOpened()
-}
-
 func (r *FSRepo) closeDatastore() error {
 	dsLock.Lock()
 	defer dsLock.Unlock()

repo/fsrepo/fsrepo_test.go

@@ -33,19 +33,6 @@ func TestRemove(t *testing.T) {
 	assert.Nil(Remove(path), t, "can remove a repository")
 }
 
-func TestCannotBeReopened(t *testing.T) {
-	t.Parallel()
-	path := testRepoPath("", t)
-	assert.Nil(Init(path, &config.Config{}), t)
-	r := At(path)
-	assert.Nil(r.Open(), t)
-	assert.Nil(r.Close(), t)
-	assert.Err(r.Open(), t, "shouldn't be possible to re-open the repo")
-
-	// mutable state is the enemy. Take Close() as an opportunity to reduce
-	// entropy. Callers ought to start fresh with a new handle by calling `At`.
-}
-
 func TestCanManageReposIndependently(t *testing.T) {
 	t.Parallel()
 	pathA := testRepoPath("a", t)
@@ -60,10 +47,10 @@ func TestCanManageReposIndependently(t *testing.T) {
 	assert.True(IsInitialized(pathB), t, "b should be initialized")
 
 	t.Log("open the two repos")
-	repoA := At(pathA)
+	repoA, err := Open(pathA)
-	repoB := At(pathB)
+	assert.Nil(err, t, "a")
-	assert.Nil(repoA.Open(), t, "a")
+	repoB, err := Open(pathB)
-	assert.Nil(repoB.Open(), t, "b")
+	assert.Nil(err, t, "b")
 
 	t.Log("close and remove b while a is open")
 	assert.Nil(repoB.Close(), t, "close b")
@@ -80,15 +67,15 @@ func TestDatastoreGetNotAllowedAfterClose(t *testing.T) {
 
 	assert.True(!IsInitialized(path), t, "should NOT be initialized")
 	assert.Nil(Init(path, &config.Config{}), t, "should initialize successfully")
-	r := At(path)
+	r, err := Open(path)
-	assert.Nil(r.Open(), t, "should open successfully")
+	assert.Nil(err, t, "should open successfully")
 
 	k := "key"
 	data := []byte(k)
 	assert.Nil(r.Datastore().Put(datastore.NewKey(k), data), t, "Put should be successful")
 
 	assert.Nil(r.Close(), t)
-	_, err := r.Datastore().Get(datastore.NewKey(k))
+	_, err = r.Datastore().Get(datastore.NewKey(k))
 	assert.Err(err, t, "after closer, Get should be fail")
 }
 
@@ -97,16 +84,16 @@ func TestDatastorePersistsFromRepoToRepo(t *testing.T) {
 	path := testRepoPath("test", t)
 
 	assert.Nil(Init(path, &config.Config{}), t)
-	r1 := At(path)
+	r1, err := Open(path)
-	assert.Nil(r1.Open(), t)
+	assert.Nil(err, t)
 
 	k := "key"
 	expected := []byte(k)
 	assert.Nil(r1.Datastore().Put(datastore.NewKey(k), expected), t, "using first repo, Put should be successful")
 	assert.Nil(r1.Close(), t)
 
-	r2 := At(path)
+	r2, err := Open(path)
-	assert.Nil(r2.Open(), t)
+	assert.Nil(err, t)
 	v, err := r2.Datastore().Get(datastore.NewKey(k))
 	assert.Nil(err, t, "using second repo, Get should be successful")
 	actual, ok := v.([]byte)
@@ -120,10 +107,10 @@ func TestOpenMoreThanOnceInSameProcess(t *testing.T) {
 	path := testRepoPath("", t)
 	assert.Nil(Init(path, &config.Config{}), t)
 
-	r1 := At(path)
+	r1, err := Open(path)
-	r2 := At(path)
+	assert.Nil(err, t, "first repo should open successfully")
-	assert.Nil(r1.Open(), t, "first repo should open successfully")
+	r2, err := Open(path)
-	assert.Nil(r2.Open(), t, "second repo should open successfully")
+	assert.Nil(err, t, "second repo should open successfully")
 	assert.True(r1.ds == r2.ds, t, "repos should share the datastore")
 
 	assert.Nil(r1.Close(), t)

test/supernode_client/main.go

@@ -66,8 +66,8 @@ func run() error {
 	repoPath := gopath.Join(cwd, ".go-ipfs")
 	if err := ensureRepoInitialized(repoPath); err != nil {
 	}
-	repo := fsrepo.At(repoPath)
+	repo, err := fsrepo.Open(repoPath)
-	if err := repo.Open(); err != nil { // owned by node
+	if err != nil { // owned by node
 		return err
 	}
 	cfg := repo.Config()

updates/updates.go

@@ -212,8 +212,8 @@ func CliCheckForUpdates(cfg *config.Config, repoPath string) error {
 	// if we checked successfully.
 	if err == ErrNoUpdateAvailable {
 		log.Noticef("No update available, checked on %s", time.Now())
-		r := fsrepo.At(repoPath)
+		r, err := fsrepo.Open(repoPath)
-		if err := r.Open(); err != nil {
+		if err != nil {
 			return err
 		}
 		if err := recordUpdateCheck(cfg); err != nil {