make CLI commands use the cache to lock the repo properly

Michael Muré created

Change summary

bug/bug_actions.go            |   1 
cache/bug_cache.go            |  10 ++
cache/cache.go                | 105 ------------------------------------
cache/repo_cache.go           | 100 ++++++++++++++++++++++++++++++++++
commands/close.go             |  17 +++--
commands/comment.go           |  19 +++--
commands/label.go             |  21 +++---
commands/ls.go                |   7 ++
commands/new.go               |  26 +++-----
commands/open.go              |  17 +++--
commands/pull.go              |  10 ++
commands/push.go              |  10 ++
commands/root.go              |  13 ----
commands/show.go              |  12 +++-
commands/termui.go            |   9 ++
graphql/resolvers/mutation.go |   2 
termui/show_bug.go            |   4 
termui/termui.go              |  17 +----
18 files changed, 208 insertions(+), 192 deletions(-)

Detailed changes

bug/bug_actions.go 🔗

@@ -25,6 +25,7 @@ func Push(repo repository.Repo, remote string) (string, error) {
 	return repo.PushRefs(remote, bugsRefPattern+"*")
 }
 
+// TODO: return a chan of changes for the cache to be updated properly
 func Pull(repo repository.Repo, out io.Writer, remote string) error {
 	if out == nil {
 		out = ioutil.Discard

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
 	}

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)
-}

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
+}

commands/close.go 🔗

@@ -3,8 +3,7 @@ package commands
 import (
 	"errors"
 
-	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/bug/operations"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
 
@@ -17,21 +16,25 @@ func runCloseBug(cmd *cobra.Command, args []string) error {
 		return errors.New("You must provide a bug id")
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	prefix := args[0]
 
-	b, err := bug.FindLocalBug(repo, prefix)
+	b, err := backend.ResolveBugPrefix(prefix)
 	if err != nil {
 		return err
 	}
 
-	author, err := bug.GetUser(repo)
+	err = b.Close()
 	if err != nil {
 		return err
 	}
 
-	operations.Close(b, author)
-
-	return b.Commit(repo)
+	return b.Commit()
 }
 
 var closeCmd = &cobra.Command{

commands/comment.go 🔗

@@ -4,8 +4,7 @@ import (
 	"errors"
 	"fmt"
 
-	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/bug/operations"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/input"
 	"github.com/spf13/cobra"
 )
@@ -26,6 +25,12 @@ func runComment(cmd *cobra.Command, args []string) error {
 		return errors.New("You must provide a bug id")
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	prefix := args[0]
 
 	if commentMessageFile != "" && commentMessage == "" {
@@ -36,7 +41,7 @@ func runComment(cmd *cobra.Command, args []string) error {
 	}
 
 	if commentMessage == "" {
-		commentMessage, err = input.BugCommentEditorInput(repo)
+		commentMessage, err = input.BugCommentEditorInput(backend.Repository())
 		if err == input.ErrEmptyMessage {
 			fmt.Println("Empty message, aborting.")
 			return nil
@@ -46,19 +51,17 @@ func runComment(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	author, err := bug.GetUser(repo)
+	b, err := backend.ResolveBugPrefix(prefix)
 	if err != nil {
 		return err
 	}
 
-	b, err := bug.FindLocalBug(repo, prefix)
+	err = b.AddComment(commentMessage)
 	if err != nil {
 		return err
 	}
 
-	operations.Comment(b, author, commentMessage)
-
-	return b.Commit(repo)
+	return b.Commit()
 }
 
 var commentCmd = &cobra.Command{

commands/label.go 🔗

@@ -4,8 +4,7 @@ import (
 	"errors"
 	"os"
 
-	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/bug/operations"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
 
@@ -20,6 +19,12 @@ func runLabel(cmd *cobra.Command, args []string) error {
 		return errors.New("You must provide a label")
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	prefix := args[0]
 
 	var add, remove []string
@@ -30,23 +35,17 @@ func runLabel(cmd *cobra.Command, args []string) error {
 		add = args[1:]
 	}
 
-	b, err := bug.FindLocalBug(repo, prefix)
+	b, err := backend.ResolveBugPrefix(prefix)
 	if err != nil {
 		return err
 	}
 
-	author, err := bug.GetUser(repo)
-	if err != nil {
-		return err
-	}
-
-	err = operations.ChangeLabels(os.Stdout, b, author, add, remove)
-
+	err = b.ChangeLabels(os.Stdout, add, remove)
 	if err != nil {
 		return err
 	}
 
-	return b.Commit(repo)
+	return b.Commit()
 }
 
 var labelCmd = &cobra.Command{

commands/ls.go 🔗

@@ -9,6 +9,13 @@ import (
 )
 
 func runLsBug(cmd *cobra.Command, args []string) error {
+	//backend, err := cache.NewRepoCache(repo)
+	//if err != nil {
+	//	return err
+	//}
+
+	// Todo: read bugs from backend
+
 	bugs := bug.ReadAllLocalBugs(repo)
 
 	for b := range bugs {

commands/new.go 🔗

@@ -3,8 +3,7 @@ package commands
 import (
 	"fmt"
 
-	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/bug/operations"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/input"
 	"github.com/spf13/cobra"
 )
@@ -25,8 +24,14 @@ func runNewBug(cmd *cobra.Command, args []string) error {
 		}
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	if newMessage == "" || newTitle == "" {
-		newTitle, newMessage, err = input.BugCreateEditorInput(repo, newTitle, newMessage)
+		newTitle, newMessage, err = input.BugCreateEditorInput(backend.Repository(), newTitle, newMessage)
 
 		if err == input.ErrEmptyTitle {
 			fmt.Println("Empty title, aborting.")
@@ -37,23 +42,12 @@ func runNewBug(cmd *cobra.Command, args []string) error {
 		}
 	}
 
-	author, err := bug.GetUser(repo)
-	if err != nil {
-		return err
-	}
-
-	newBug, err := operations.Create(author, newTitle, newMessage)
-	if err != nil {
-		return err
-	}
-
-	err = newBug.Commit(repo)
-
+	b, err := backend.NewBug(newTitle, newMessage)
 	if err != nil {
 		return err
 	}
 
-	fmt.Printf("%s created\n", newBug.HumanId())
+	fmt.Printf("%s created\n", b.HumanId())
 
 	return nil
 }

commands/open.go 🔗

@@ -3,8 +3,7 @@ package commands
 import (
 	"errors"
 
-	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/bug/operations"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
 
@@ -17,21 +16,25 @@ func runOpenBug(cmd *cobra.Command, args []string) error {
 		return errors.New("You must provide a bug id")
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	prefix := args[0]
 
-	b, err := bug.FindLocalBug(repo, prefix)
+	b, err := backend.ResolveBugPrefix(prefix)
 	if err != nil {
 		return err
 	}
 
-	author, err := bug.GetUser(repo)
+	err = b.Open()
 	if err != nil {
 		return err
 	}
 
-	operations.Open(b, author)
-
-	return b.Commit(repo)
+	return b.Commit()
 }
 
 var openCmd = &cobra.Command{

commands/pull.go 🔗

@@ -4,7 +4,7 @@ import (
 	"errors"
 	"os"
 
-	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
 
@@ -18,7 +18,13 @@ func runPull(cmd *cobra.Command, args []string) error {
 		remote = args[0]
 	}
 
-	return bug.Pull(repo, os.Stdout, remote)
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
+	return backend.Pull(remote, os.Stdout)
 }
 
 // showCmd defines the "push" subcommand.

commands/push.go 🔗

@@ -4,7 +4,7 @@ import (
 	"errors"
 	"fmt"
 
-	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/spf13/cobra"
 )
 
@@ -18,7 +18,13 @@ func runPush(cmd *cobra.Command, args []string) error {
 		remote = args[0]
 	}
 
-	stdout, err := bug.Push(repo, remote)
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
+	stdout, err := backend.Push(remote)
 	if err != nil {
 		return err
 	}

commands/root.go 🔗

@@ -5,15 +5,10 @@ import (
 	"os"
 
 	"github.com/MichaelMure/git-bug/bug"
-	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/repository"
 	"github.com/spf13/cobra"
 )
 
-// Will display "git bug"
-// \u00A0 is a non-breaking space
-// It's used to avoid cobra to split the Use string at the first space to get the root command name
-//const rootCommandName = "git\u00A0bug"
 const rootCommandName = "git-bug"
 
 // package scoped var to hold the repo after the PreRun execution
@@ -69,13 +64,5 @@ func loadRepo(cmd *cobra.Command, args []string) error {
 		return err
 	}
 
-	// Prevent the command from running when the cache has locked the repo
-	// Todo: make it more fine-grained at first
-	// Todo: make the running cache available for other processes
-	err = cache.RepoIsAvailable(repo)
-	if err != nil {
-		return err
-	}
-
 	return nil
 }

commands/show.go 🔗

@@ -5,7 +5,7 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/MichaelMure/git-bug/bug"
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/util"
 	"github.com/spf13/cobra"
 )
@@ -19,14 +19,20 @@ func runShowBug(cmd *cobra.Command, args []string) error {
 		return errors.New("You must provide a bug id")
 	}
 
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
 	prefix := args[0]
 
-	b, err := bug.FindLocalBug(repo, prefix)
+	b, err := backend.ResolveBugPrefix(prefix)
 	if err != nil {
 		return err
 	}
 
-	snapshot := b.Compile()
+	snapshot := b.Snapshot()
 
 	if len(snapshot.Comments) == 0 {
 		return errors.New("Invalid bug: no comment")

commands/termui.go 🔗

@@ -1,12 +1,19 @@
 package commands
 
 import (
+	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/termui"
 	"github.com/spf13/cobra"
 )
 
 func runTermUI(cmd *cobra.Command, args []string) error {
-	return termui.Run(repo)
+	backend, err := cache.NewRepoCache(repo)
+	if err != nil {
+		return err
+	}
+	defer backend.Close()
+
+	return termui.Run(backend)
 }
 
 var termUICmd = &cobra.Command{

graphql/resolvers/mutation.go 🔗

@@ -89,7 +89,7 @@ func (r mutationResolver) ChangeLabels(ctx context.Context, repoRef *string, pre
 		return bug.Snapshot{}, err
 	}
 
-	err = b.ChangeLabels(added, removed)
+	err = b.ChangeLabels(nil, added, removed)
 	if err != nil {
 		return bug.Snapshot{}, err
 	}

termui/show_bug.go 🔗

@@ -599,7 +599,7 @@ func (sb *showBug) addLabel(g *gocui.Gui, v *gocui.View) error {
 			return r == ' ' || r == ','
 		})
 
-		err := sb.bug.ChangeLabels(trimLabels(labels), nil)
+		err := sb.bug.ChangeLabels(nil, trimLabels(labels), nil)
 		if err != nil {
 			ui.msgPopup.Activate(msgPopupErrorTitle, err.Error())
 		}
@@ -622,7 +622,7 @@ func (sb *showBug) removeLabel(g *gocui.Gui, v *gocui.View) error {
 			return r == ' ' || r == ','
 		})
 
-		err := sb.bug.ChangeLabels(nil, trimLabels(labels))
+		err := sb.bug.ChangeLabels(nil, nil, trimLabels(labels))
 		if err != nil {
 			ui.msgPopup.Activate(msgPopupErrorTitle, err.Error())
 		}

termui/termui.go 🔗

@@ -3,7 +3,6 @@ package termui
 import (
 	"github.com/MichaelMure/git-bug/cache"
 	"github.com/MichaelMure/git-bug/input"
-	"github.com/MichaelMure/git-bug/repository"
 	"github.com/jroimartin/gocui"
 	"github.com/pkg/errors"
 )
@@ -42,18 +41,12 @@ type window interface {
 }
 
 // Run will launch the termUI in the terminal
-func Run(repo repository.Repo) error {
-	c, err := cache.NewRepoCache(repo)
-
-	if err != nil {
-		return err
-	}
-
+func Run(cache *cache.RepoCache) error {
 	ui = &termUI{
 		gError:     make(chan error, 1),
-		cache:      c,
-		bugTable:   newBugTable(c),
-		showBug:    newShowBug(c),
+		cache:      cache,
+		bugTable:   newBugTable(cache),
+		showBug:    newShowBug(cache),
 		msgPopup:   newMsgPopup(),
 		inputPopup: newInputPopup(),
 	}
@@ -62,7 +55,7 @@ func Run(repo repository.Repo) error {
 
 	initGui(nil)
 
-	err = <-ui.gError
+	err := <-ui.gError
 
 	if err != nil && err != gocui.ErrQuit {
 		return err