make CLI commands use the cache to lock the repo properly

3 files changed, 108 insertions, 107 deletions

diff --git a/cache/bug_cache.go b/cache/bug_cache.go
index 7df76efe..c9e1f2f5 100644
--- a/cache/bug_cache.go
+++ b/cache/bug_cache.go
@@ -1,6 +1,8 @@
 package cache
 
 import (
+	"io"
+
 	"github.com/MichaelMure/git-bug/bug"
 	"github.com/MichaelMure/git-bug/bug/operations"
 	"github.com/MichaelMure/git-bug/util"
@@ -22,6 +24,10 @@ func (c *BugCache) Snapshot() *bug.Snapshot {
 	return c.bug.Snapshot()
 }
 
+func (c *BugCache) HumanId() string {
+	return c.bug.HumanId()
+}
+
 func (c *BugCache) notifyUpdated() error {
 	return c.repoCache.bugUpdated(c.bug.Id())
 }
@@ -45,13 +51,13 @@ func (c *BugCache) AddCommentWithFiles(message string, files []util.Hash) error
 	return c.notifyUpdated()
 }
 
-func (c *BugCache) ChangeLabels(added []string, removed []string) error {
+func (c *BugCache) ChangeLabels(out io.Writer, added []string, removed []string) error {
 	author, err := bug.GetUser(c.repoCache.repo)
 	if err != nil {
 		return err
 	}
 
-	err = operations.ChangeLabels(nil, c.bug, author, added, removed)
+	err = operations.ChangeLabels(out, c.bug, author, added, removed)
 	if err != nil {
 		return err
 	}
diff --git a/cache/cache.go b/cache/cache.go
index 6dbafba1..751467b2 100644
--- a/cache/cache.go
+++ b/cache/cache.go
@@ -2,14 +2,8 @@ package cache
 
 import (
 	"fmt"
-	"io"
-	"io/ioutil"
-	"os"
-	"path"
-	"strconv"
 
 	"github.com/MichaelMure/git-bug/repository"
-	"github.com/MichaelMure/git-bug/util"
 )
 
 const lockfile = "lock"
@@ -27,11 +21,6 @@ func NewCache() RootCache {
 
 // RegisterRepository register a named repository. Use this for multi-repo setup
 func (c *RootCache) RegisterRepository(ref string, repo repository.Repo) error {
-	err := c.lockRepository(repo)
-	if err != nil {
-		return err
-	}
-
 	r, err := NewRepoCache(repo)
 	if err != nil {
 		return err
@@ -43,11 +32,6 @@ func (c *RootCache) RegisterRepository(ref string, repo repository.Repo) error {
 
 // RegisterDefaultRepository register a unnamed repository. Use this for mono-repo setup
 func (c *RootCache) RegisterDefaultRepository(repo repository.Repo) error {
-	err := c.lockRepository(repo)
-	if err != nil {
-		return err
-	}
-
 	r, err := NewRepoCache(repo)
 	if err != nil {
 		return err
@@ -57,28 +41,6 @@ func (c *RootCache) RegisterDefaultRepository(repo repository.Repo) error {
 	return nil
 }
 
-func (c *RootCache) lockRepository(repo repository.Repo) error {
-	lockPath := repoLockFilePath(repo)
-
-	err := RepoIsAvailable(repo)
-	if err != nil {
-		return err
-	}
-
-	f, err := os.Create(lockPath)
-	if err != nil {
-		return err
-	}
-
-	pid := fmt.Sprintf("%d", os.Getpid())
-	_, err = f.WriteString(pid)
-	if err != nil {
-		return err
-	}
-
-	return f.Close()
-}
-
 // ResolveRepo retrieve a repository by name
 func (c *RootCache) DefaultRepo() (*RepoCache, error) {
 	if len(c.repos) != 1 {
@@ -104,75 +66,10 @@ func (c *RootCache) ResolveRepo(ref string) (*RepoCache, error) {
 // Close will do anything that is needed to close the cache properly
 func (c *RootCache) Close() error {
 	for _, cachedRepo := range c.repos {
-		lockPath := repoLockFilePath(cachedRepo.repo)
-		err := os.Remove(lockPath)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-// RepoIsAvailable check is the given repository is locked by a Cache.
-// Note: this is a smart function that will cleanup the lock file if the
-// corresponding process is not there anymore.
-// If no error is returned, the repo is free to edit.
-func RepoIsAvailable(repo repository.Repo) error {
-	lockPath := repoLockFilePath(repo)
-
-	// Todo: this leave way for a racey access to the repo between the test
-	// if the file exist and the actual write. It's probably not a problem in
-	// practice because using a repository will be done from user interaction
-	// or in a context where a single instance of git-bug is already guaranteed
-	// (say, a server with the web UI running). But still, that might be nice to
-	// have a mutex or something to guard that.
-
-	// Todo: this will fail if somehow the filesystem is shared with another
-	// computer. Should add a configuration that prevent the cleaning of the
-	// lock file
-
-	f, err := os.Open(lockPath)
-
-	if err != nil && !os.IsNotExist(err) {
-		return err
-	}
-
-	if err == nil {
-		// lock file already exist
-		buf, err := ioutil.ReadAll(io.LimitReader(f, 10))
-		if err != nil {
-			return err
-		}
-		if len(buf) == 10 {
-			return fmt.Errorf("The lock file should be < 10 bytes")
-		}
-
-		pid, err := strconv.Atoi(string(buf))
-		if err != nil {
-			return err
-		}
-
-		if util.ProcessIsRunning(pid) {
-			return fmt.Errorf("The repository you want to access is already locked by the process pid %d", pid)
-		}
-
-		// The lock file is just laying there after a crash, clean it
-
-		fmt.Println("A lock file is present but the corresponding process is not, removing it.")
-		err = f.Close()
-		if err != nil {
-			return err
-		}
-
-		os.Remove(lockPath)
+		err := cachedRepo.Close()
 		if err != nil {
 			return err
 		}
 	}
-
 	return nil
 }
-
-func repoLockFilePath(repo repository.Repo) string {
-	return path.Join(repo.GetPath(), ".git", "git-bug", lockfile)
-}
diff --git a/cache/repo_cache.go b/cache/repo_cache.go
index c73dbe9f..3d98806c 100644
--- a/cache/repo_cache.go
+++ b/cache/repo_cache.go
@@ -5,8 +5,10 @@ import (
 	"encoding/gob"
 	"fmt"
 	"io"
+	"io/ioutil"
 	"os"
 	"path"
+	"strconv"
 	"strings"
 
 	"github.com/MichaelMure/git-bug/bug"
@@ -27,8 +29,12 @@ func NewRepoCache(r repository.Repo) (*RepoCache, error) {
 		bugs: make(map[string]*BugCache),
 	}
 
-	err := c.loadExcerpts()
+	err := c.lock()
+	if err != nil {
+		return &RepoCache{}, err
+	}
 
+	err = c.loadExcerpts()
 	if err == nil {
 		return c, nil
 	}
@@ -42,6 +48,33 @@ func (c *RepoCache) Repository() repository.Repo {
 	return c.repo
 }
 
+func (c *RepoCache) lock() error {
+	lockPath := repoLockFilePath(c.repo)
+
+	err := repoIsAvailable(c.repo)
+	if err != nil {
+		return err
+	}
+
+	f, err := os.Create(lockPath)
+	if err != nil {
+		return err
+	}
+
+	pid := fmt.Sprintf("%d", os.Getpid())
+	_, err = f.WriteString(pid)
+	if err != nil {
+		return err
+	}
+
+	return f.Close()
+}
+
+func (c *RepoCache) Close() error {
+	lockPath := repoLockFilePath(c.repo)
+	return os.Remove(lockPath)
+}
+
 // bugUpdated is a callback to trigger when the excerpt of a bug changed,
 // that is each time a bug is updated
 func (c *RepoCache) bugUpdated(id string) error {
@@ -217,3 +250,68 @@ func (c *RepoCache) Pull(remote string, out io.Writer) error {
 func (c *RepoCache) Push(remote string) (string, error) {
 	return bug.Push(c.repo, remote)
 }
+
+func repoLockFilePath(repo repository.Repo) string {
+	return path.Join(repo.GetPath(), ".git", "git-bug", lockfile)
+}
+
+// repoIsAvailable check is the given repository is locked by a Cache.
+// Note: this is a smart function that will cleanup the lock file if the
+// corresponding process is not there anymore.
+// If no error is returned, the repo is free to edit.
+// @Deprecated
+func repoIsAvailable(repo repository.Repo) error {
+	lockPath := repoLockFilePath(repo)
+
+	// Todo: this leave way for a racey access to the repo between the test
+	// if the file exist and the actual write. It's probably not a problem in
+	// practice because using a repository will be done from user interaction
+	// or in a context where a single instance of git-bug is already guaranteed
+	// (say, a server with the web UI running). But still, that might be nice to
+	// have a mutex or something to guard that.
+
+	// Todo: this will fail if somehow the filesystem is shared with another
+	// computer. Should add a configuration that prevent the cleaning of the
+	// lock file
+
+	f, err := os.Open(lockPath)
+
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+
+	if err == nil {
+		// lock file already exist
+		buf, err := ioutil.ReadAll(io.LimitReader(f, 10))
+		if err != nil {
+			return err
+		}
+		if len(buf) == 10 {
+			return fmt.Errorf("The lock file should be < 10 bytes")
+		}
+
+		pid, err := strconv.Atoi(string(buf))
+		if err != nil {
+			return err
+		}
+
+		if util.ProcessIsRunning(pid) {
+			return fmt.Errorf("The repository you want to access is already locked by the process pid %d", pid)
+		}
+
+		// The lock file is just laying there after a crash, clean it
+
+		fmt.Println("A lock file is present but the corresponding process is not, removing it.")
+		err = f.Close()
+		if err != nil {
+			return err
+		}
+
+		os.Remove(lockPath)
+		if err != nil {
+			return err
+		}
+	}
+
+	return nil
+}