From 8a2966d777e7f6efffa7dfb02353058b952be5f7 Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Mon, 22 Oct 2018 15:40:38 -0700
Subject: [PATCH 1/6] adds gpg package

---
 review/gpg/signable.go | 124 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 124 insertions(+)
 create mode 100644 review/gpg/signable.go

diff --git a/review/gpg/signable.go b/review/gpg/signable.go
new file mode 100644
index 00000000..a7eaf2f2
--- /dev/null
+++ b/review/gpg/signable.go
@@ -0,0 +1,124 @@
+// Package gpg provides an interface and an abstraction with which to sign and
+// verify review requests and comments.
+package gpg
+
+import (
+	"bytes"
+	"encoding/json"
+	"io/ioutil"
+	"os"
+	"os/exec"
+)
+
+const placeholder = "gpgsig"
+
+// Sig provides an abstraction around shelling out to GPG to sign the
+// content it's given.
+type Sig struct {
+	// Sig holds an object's content's signature.
+	Sig string `json:"signature,omitempty"`
+}
+
+// Signable is an interfaces which provides the pointer to the signable
+// object's stringified signature.
+//
+// This pointer is used by `Sign` and `Verify` to replace its contents with
+// `placeholder` or the signature itself for the purposes of signing or
+// verifying.
+type Signable interface {
+	Signature() *string
+}
+
+// Signature is `Sig`'s implementation of `Signable`. Through this function, an
+// object which needs to implement `Signable` need only embed `Sig`
+// anonymously. See, e.g., review/equest.go.
+func (s *Sig) Signature() *string {
+	return &s.Sig
+}
+
+// Sign uses gpg to sign the contents of a request and deposit it into the
+// signature key of the request.
+func Sign(key string, s Signable) error {
+	// First we retrieve the pointer and write `placeholder` as its value.
+	sigPtr := s.Signature()
+	*sigPtr = placeholder
+
+	// Marshal the content and sign it.
+	content, err := json.Marshal(s)
+	if err != nil {
+		return err
+	}
+	sig, err := signContent(key, content)
+	if err != nil {
+		return err
+	}
+
+	// Write the signature as the new value at the pointer.
+	*sigPtr = sig.String()
+	return nil
+}
+
+func signContent(key string, content []byte) (*bytes.Buffer,
+	error) {
+	var stdout, stderr bytes.Buffer
+	cmd := exec.Command("gpg", "-u", key, "--detach-sign", "--armor")
+	cmd.Stdin = bytes.NewReader(content)
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	err := cmd.Run()
+	return &stdout, err
+}
+
+// Verify verifies the signatures on the request and its comments with the
+// given key.
+func Verify(key string, s Signable) error {
+	// Retrieve the pointer.
+	sigPtr := s.Signature()
+	// Copy its contents.
+	sig := *sigPtr
+	// Overwrite the value with the placeholder.
+	*sigPtr = placeholder
+
+	// 1. Marshal the content into JSON.
+	// 2. Write the signature and the content to temp files.
+	// 3. Use gpg to verify the signature.
+	content, err := json.Marshal(s)
+	if err != nil {
+		return err
+	}
+	sigFile, err := ioutil.TempFile("", "sig")
+	if err != nil {
+		return err
+	}
+	defer os.Remove(sigFile.Name())
+	_, err = sigFile.Write([]byte(sig))
+	if err != nil {
+		return err
+	}
+	err = sigFile.Close()
+	if err != nil {
+		return err
+	}
+
+	contentFile, err := ioutil.TempFile("", "content")
+	if err != nil {
+		return err
+	}
+	defer os.Remove(contentFile.Name())
+	_, err = contentFile.Write(content)
+	if err != nil {
+		return err
+	}
+	err = contentFile.Close()
+	if err != nil {
+		return err
+	}
+
+	var stdout, stderr bytes.Buffer
+	cmd := exec.Command("gpg", "-u", key, "--verify", sigFile.Name(),
+		contentFile.Name())
+	cmd.Stdout = &stdout
+	cmd.Stderr = &stderr
+	err = cmd.Run()
+	return err
+}

From 431ccab3e75b00103eaa92a437760ac29eb63259 Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Mon, 22 Oct 2018 15:43:00 -0700
Subject: [PATCH 2/6] implements signing for requests acceptances

---
 commands/accept.go        | 14 ++++++++++++++
 commands/request.go       | 17 +++++++++++++++--
 repository/git.go         |  6 ++++++
 repository/mock_repo.go   |  6 ++++++
 repository/repo.go        |  4 ++++
 review/comment/comment.go |  3 +++
 review/request/request.go |  2 ++
 7 files changed, 50 insertions(+), 2 deletions(-)

diff --git a/commands/accept.go b/commands/accept.go
index 6207b7b9..b50f424c 100644
--- a/commands/accept.go
+++ b/commands/accept.go
@@ -24,6 +24,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var acceptFlagSet = flag.NewFlagSet("accept", flag.ExitOnError)
@@ -31,6 +32,9 @@ var acceptFlagSet = flag.NewFlagSet("accept", flag.ExitOnError)
 var (
 	acceptMessageFile = acceptFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	acceptMessage     = acceptFlagSet.String("m", "", "Message to attach to the review")
+
+	acceptSign = acceptFlagSet.Bool("S", false,
+		"sign the contents of the acceptance")
 )
 
 // acceptReview adds an LGTM comment to the current code review.
@@ -80,6 +84,16 @@ func acceptReview(repo repository.Repo, args []string) error {
 	c := comment.New(userEmail, *acceptMessage)
 	c.Location = &location
 	c.Resolved = &resolved
+	if *acceptSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 	return r.AddComment(c)
 }
 
diff --git a/commands/request.go b/commands/request.go
index ff3ff7fd..9a9854c3 100644
--- a/commands/request.go
+++ b/commands/request.go
@@ -20,10 +20,12 @@ import (
 	"errors"
 	"flag"
 	"fmt"
+	"strings"
+
 	"github.com/google/git-appraise/commands/input"
 	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review/gpg"
 	"github.com/google/git-appraise/review/request"
-	"strings"
 )
 
 // Template for the "request" subcommand's output.
@@ -44,6 +46,8 @@ var (
 	requestTarget           = requestFlagSet.String("target", "refs/heads/master", "Revision against which to review")
 	requestQuiet            = requestFlagSet.Bool("quiet", false, "Suppress review summary output")
 	requestAllowUncommitted = requestFlagSet.Bool("allow-uncommitted", false, "Allow uncommitted local changes.")
+	requestSign             = requestFlagSet.Bool("S", false,
+		"GPG sign the content of the request")
 )
 
 // Build the template review request based solely on the parsed flag values.
@@ -145,7 +149,16 @@ func requestReview(repo repository.Repo, args []string) error {
 		}
 		r.Description = description
 	}
-
+	if *requestSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &r)
+		if err != nil {
+			return err
+		}
+	}
 	note, err := r.Write()
 	if err != nil {
 		return err
diff --git a/repository/git.go b/repository/git.go
index a7a6e1a1..3aea6435 100644
--- a/repository/git.go
+++ b/repository/git.go
@@ -102,6 +102,12 @@ func (repo *GitRepo) GetUserEmail() (string, error) {
 	return repo.runGitCommand("config", "user.email")
 }
 
+// GetUserSigningKey returns the key id the user has configured for
+// sigining git artifacts.
+func (repo *GitRepo) GetUserSigningKey() (string, error) {
+	return repo.runGitCommand("config", "user.signingKey")
+}
+
 // GetCoreEditor returns the name of the editor that the user has used to configure git.
 func (repo *GitRepo) GetCoreEditor() (string, error) {
 	return repo.runGitCommand("var", "GIT_EDITOR")
diff --git a/repository/mock_repo.go b/repository/mock_repo.go
index c9fd105b..cc45f944 100644
--- a/repository/mock_repo.go
+++ b/repository/mock_repo.go
@@ -194,6 +194,12 @@ func (r *mockRepoForTest) GetRepoStateHash() (string, error) {
 // GetUserEmail returns the email address that the user has used to configure git.
 func (r *mockRepoForTest) GetUserEmail() (string, error) { return "user@example.com", nil }
 
+// GetUserSigningKey returns the key id the user has configured for
+// sigining git artifacts.
+func (r *mockRepoForTest) GetUserSigningKey() (string, error) {
+	return "gpgsig", nil
+}
+
 // GetCoreEditor returns the name of the editor that the user has used to configure git.
 func (r *mockRepoForTest) GetCoreEditor() (string, error) { return "vi", nil }
 
diff --git a/repository/repo.go b/repository/repo.go
index 03413e30..744c7929 100644
--- a/repository/repo.go
+++ b/repository/repo.go
@@ -41,6 +41,10 @@ type Repo interface {
 	// GetUserEmail returns the email address that the user has used to configure git.
 	GetUserEmail() (string, error)
 
+	// GetUserSigningKey returns the key id the user has configured for
+	// sigining git artifacts.
+	GetUserSigningKey() (string, error)
+
 	// GetCoreEditor returns the name of the editor that the user has used to configure git.
 	GetCoreEditor() (string, error)
 
diff --git a/review/comment/comment.go b/review/comment/comment.go
index c083fb1f..b1dea49c 100644
--- a/review/comment/comment.go
+++ b/review/comment/comment.go
@@ -27,6 +27,7 @@ import (
 	"time"
 
 	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 // Ref defines the git-notes ref that we expect to contain review comments.
@@ -118,6 +119,8 @@ type Comment struct {
 	Resolved *bool `json:"resolved,omitempty"`
 	// Version represents the version of the metadata format.
 	Version int `json:"v,omitempty"`
+
+	gpg.Sig
 }
 
 // New returns a new comment with the given description message.
diff --git a/review/request/request.go b/review/request/request.go
index f332e300..18477d08 100644
--- a/review/request/request.go
+++ b/review/request/request.go
@@ -53,6 +53,8 @@ type Request struct {
 	// Alias stores a post-rebase commit ID for the review. This allows the tool
 	// to track the history of a review even if the commit history changes.
 	Alias string `json:"alias,omitempty"`
+
+	gpg.Sig
 }
 
 // New returns a new request.

From a632af3b751f954b23525a7ea6bfd82ac80a6e6d Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Mon, 22 Oct 2018 15:43:40 -0700
Subject: [PATCH 3/6] adds verification for pulling

---
 commands/pull.go          | 60 +++++++++++++++++++++++++----
 repository/git.go         | 81 +++++++++++++++++++++++++++++++++++----
 repository/mock_repo.go   | 17 ++++++++
 repository/repo.go        | 14 +++++++
 review/request/request.go |  4 +-
 review/review.go          | 49 ++++++++++++++++++++---
 6 files changed, 204 insertions(+), 21 deletions(-)

diff --git a/commands/pull.go b/commands/pull.go
index 48dfe5ba..bbbad6a3 100644
--- a/commands/pull.go
+++ b/commands/pull.go
@@ -18,30 +18,74 @@ package commands
 
 import (
 	"errors"
+	"flag"
 	"fmt"
+
 	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review"
+)
+
+var (
+	pullFlagSet = flag.NewFlagSet("pull", flag.ExitOnError)
+	pullVerify  = pullFlagSet.Bool("verify-signatures", false,
+		"verify the signatures of pulled reviews")
 )
 
-// pull updates the local git-notes used for reviews with those from a remote repo.
+// pull updates the local git-notes used for reviews with those from a remote
+// repo.
 func pull(repo repository.Repo, args []string) error {
-	if len(args) > 1 {
-		return errors.New("Only pulling from one remote at a time is supported.")
+	pullFlagSet.Parse(args)
+	pullArgs := pullFlagSet.Args()
+
+	if len(pullArgs) > 1 {
+		return errors.New(
+			"Only pulling from one remote at a time is supported.")
 	}
 
 	remote := "origin"
-	if len(args) == 1 {
-		remote = args[0]
+	if len(pullArgs) == 1 {
+		remote = pullArgs[0]
+	}
+	// This is the easy case. We're not checking signatures so just go the
+	// normal route.
+	if !*pullVerify {
+		return repo.PullNotesAndArchive(remote, notesRefPattern,
+			archiveRefPattern)
 	}
 
-	if err := repo.PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern); err != nil {
+	// Otherwise, we collect the fetched reviewed revisions (their hashes), get
+	// their reviews, and then one by one, verify them. If we make it through
+	// the set, _then_ we merge the remote reference into the local branch.
+	revisions, err := repo.FetchAndReturnNewReviewHashes(remote,
+		notesRefPattern, archiveRefPattern)
+	if err != nil {
 		return err
 	}
-	return nil
+	key, err := repo.GetUserSigningKey()
+	if err != nil {
+		return err
+	}
+	for _, revision := range revisions {
+		rvw, err := review.GetSummaryViaRefs(repo,
+			"refs/notes/"+remote+"/devtools/reviews",
+			"refs/notes/"+remote+"/devtools/discuss", revision)
+		if err != nil {
+			return err
+		}
+		err = rvw.Verify(key)
+		if err != nil {
+			return err
+		}
+		fmt.Println("verified review:", revision)
+	}
+
+	return repo.MergeNotesAndArchive(remote, notesRefPattern,
+		archiveRefPattern)
 }
 
 var pullCmd = &Command{
 	Usage: func(arg0 string) {
-		fmt.Printf("Usage: %s pull [<remote>]\n", arg0)
+		fmt.Printf("Usage: %s pull [<option>] [<remote>]\n", arg0)
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {
 		return pull(repo, args)
diff --git a/repository/git.go b/repository/git.go
index 3aea6435..e62e7f1f 100644
--- a/repository/git.go
+++ b/repository/git.go
@@ -800,6 +800,18 @@ func (repo *GitRepo) mergeRemoteArchives(remote, archiveRefPattern string) error
 	return nil
 }
 
+func (repo *GitRepo) fetchNotes(remote, notesRefPattern,
+	archiveRefPattern string) error {
+
+	remoteArchiveRef := getRemoteArchiveRef(remote, archiveRefPattern)
+	archiveFetchRefSpec := fmt.Sprintf("+%s:%s", archiveRefPattern, remoteArchiveRef)
+
+	remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
+	notesFetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
+
+	return repo.runGitCommandInline("fetch", remote, notesFetchRefSpec, archiveFetchRefSpec)
+}
+
 // PullNotesAndArchive fetches the contents of the notes and archives refs from
 // a remote repo, and merges them with the corresponding local refs.
 //
@@ -813,17 +825,20 @@ func (repo *GitRepo) mergeRemoteArchives(remote, archiveRefPattern string) error
 // we merely ensure that their history graph includes every commit that we
 // intend to keep.
 func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error {
-	remoteArchiveRef := getRemoteArchiveRef(remote, archiveRefPattern)
-	archiveFetchRefSpec := fmt.Sprintf("+%s:%s", archiveRefPattern, remoteArchiveRef)
-
-	remoteNotesRefPattern := getRemoteNotesRef(remote, notesRefPattern)
-	notesFetchRefSpec := fmt.Sprintf("+%s:%s", notesRefPattern, remoteNotesRefPattern)
-
-	err := repo.runGitCommandInline("fetch", remote, notesFetchRefSpec, archiveFetchRefSpec)
+	err := repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
 	if err != nil {
 		return err
 	}
 
+	return repo.MergeNotesAndArchive(remote, notesRefPattern,
+		archiveRefPattern)
+}
+
+// MergeNotesAndArchive merges the notes and archives from `remote` into the
+// local branch.
+func (repo *GitRepo) MergeNotesAndArchive(remote, notesRefPattern,
+	archiveRefPattern string) error {
+
 	if err := repo.mergeRemoteNotes(remote, notesRefPattern); err != nil {
 		return err
 	}
@@ -832,3 +847,55 @@ func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPatt
 	}
 	return nil
 }
+
+// FetchAndReturnNewReviewHashes fetches the notes "branches" and then susses
+// out the IDs (the revision the review points to) of any new reviews, then
+// returns that list of IDs.
+//
+// This is accomplished by determining which files in the notes tree have
+// changed because the _names_ of these files correspond to the revisions they
+// point to.
+func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
+	archiveRefPattern string) ([]string, error) {
+
+	beforeHash, err := repo.GetCommitHash(
+		"notes/" + remote + "/devtools/reviews")
+	if err != nil {
+		// We don't have a reference for our remote yet. This is the first
+		// fetch of notes. We need to return _all_ hashes, which is to say,
+		// all the files present at where the reference
+		// `notes/<remote>/reviews' points.
+		err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
+		if err != nil {
+			return nil, err
+		}
+		hash, err := repo.GetCommitHash(
+			"notes/" + remote + "/devtools/reviews")
+		if err != nil {
+			return nil, err
+		}
+		rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only", hash)
+		if err != nil {
+			return nil, err
+		}
+		return strings.Split(rvws, "\n"), nil
+	}
+
+	afterHash, err := repo.GetCommitHash(
+		"notes/" + remote + "/devtools/reviews")
+	if err != nil {
+		return nil, err
+	}
+
+	// Nothing new here.
+	if beforeHash == afterHash {
+		return []string{}, nil
+	}
+
+	newReviews, err := repo.runGitCommand("diff", "--name-only", beforeHash,
+		afterHash)
+	if err != nil {
+		return nil, err
+	}
+	return strings.Split(newReviews, "\n"), nil
+}
diff --git a/repository/mock_repo.go b/repository/mock_repo.go
index cc45f944..72ed6087 100644
--- a/repository/mock_repo.go
+++ b/repository/mock_repo.go
@@ -572,3 +572,20 @@ func (r *mockRepoForTest) PushNotesAndArchive(remote, notesRefPattern, archiveRe
 func (r *mockRepoForTest) PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error {
 	return nil
 }
+
+func (repo *mockRepoForTest) MergeNotesAndArchive(remote, notesRefPattern,
+	archiveRefPattern string) error {
+	return nil
+}
+
+// FetchAndReturnNewReviewHashes fetches the notes "branches" and then susses
+// out the IDs (the revision the review points to) of any new reviews, then
+// returns that list of IDs.
+//
+// This is accomplished by determining which files in the notes tree have
+// changed because the _names_ of these files correspond to the revisions they
+// point to.
+func (repo *mockRepoForTest) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
+	archiveRefPattern string) ([]string, error) {
+	return nil, nil
+}
diff --git a/repository/repo.go b/repository/repo.go
index 744c7929..9566fec6 100644
--- a/repository/repo.go
+++ b/repository/repo.go
@@ -189,4 +189,18 @@ type Repo interface {
 	// we merely ensure that their history graph includes every commit that we
 	// intend to keep.
 	PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error
+
+	// MergeNotesAndArchive merges the notes and archives from `remote` into
+	// the local branch.
+	MergeNotesAndArchive(remote, notesRefPattern,
+		archiveRefPattern string) error
+
+	// FetchAndReturnNewReviewHashes fetches the notes "branches" and then
+	// susses out the IDs (the revision the review points to) of any new
+	// reviews, then returns that list of IDs.
+	//
+	// This is accomplished by determining which files in the notes tree have
+	// changed because the _names_ of these files correspond to the revisions
+	// they point to.
+	FetchAndReturnNewReviewHashes(remote, notesRefPattern, archiveRefPattern string) ([]string, error)
 }
diff --git a/review/request/request.go b/review/request/request.go
index 18477d08..c23fd427 100644
--- a/review/request/request.go
+++ b/review/request/request.go
@@ -19,9 +19,11 @@ package request
 
 import (
 	"encoding/json"
-	"github.com/google/git-appraise/repository"
 	"strconv"
 	"time"
+
+	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 // Ref defines the git-notes ref that we expect to contain review requests.
diff --git a/review/review.go b/review/review.go
index 68434c6c..ade41355 100644
--- a/review/review.go
+++ b/review/review.go
@@ -21,12 +21,14 @@ import (
 	"bytes"
 	"encoding/json"
 	"fmt"
+	"sort"
+
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review/analyses"
 	"github.com/google/git-appraise/review/ci"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 	"github.com/google/git-appraise/review/request"
-	"sort"
 )
 
 const archiveRef = "refs/devtools/archives/reviews"
@@ -163,6 +165,21 @@ func (thread *CommentThread) updateResolvedStatus() {
 	thread.Resolved = resolved
 }
 
+// Verify verifies the signature on a comment.
+func (thread *CommentThread) Verify(key string) error {
+	err := gpg.Verify(key, &thread.Comment)
+	if err != nil {
+		return err
+	}
+	for _, child := range thread.Children {
+		err = child.Verify(key)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // mutableThread is an internal-only data structure used to store partially constructed comment threads.
 type mutableThread struct {
 	Hash     string
@@ -263,15 +280,18 @@ func getSummaryFromNotes(repo repository.Repo, revision string, requestNotes, co
 	return &reviewSummary, nil
 }
 
-// GetSummary returns the summary of the specified code review.
+// GetSummary returns the summary of the code review specified by its revision
+// and the references which contain that reviews summary and comments.
 //
 // If no review request exists, the returned review summary is nil.
-func GetSummary(repo repository.Repo, revision string) (*Summary, error) {
+func GetSummaryViaRefs(repo repository.Repo, requestRef, commentRef,
+	revision string) (*Summary, error) {
+
 	if err := repo.VerifyCommit(revision); err != nil {
 		return nil, fmt.Errorf("Could not find a commit named %q", revision)
 	}
-	requestNotes := repo.GetNotes(request.Ref, revision)
-	commentNotes := repo.GetNotes(comment.Ref, revision)
+	requestNotes := repo.GetNotes(requestRef, revision)
+	commentNotes := repo.GetNotes(commentRef, revision)
 	summary, err := getSummaryFromNotes(repo, revision, requestNotes, commentNotes)
 	if err != nil {
 		return nil, err
@@ -291,6 +311,13 @@ func GetSummary(repo repository.Repo, revision string) (*Summary, error) {
 	return summary, nil
 }
 
+// GetSummary returns the summary of the specified code review.
+//
+// If no review request exists, the returned review summary is nil.
+func GetSummary(repo repository.Repo, revision string) (*Summary, error) {
+	return GetSummaryViaRefs(repo, request.Ref, comment.Ref, revision)
+}
+
 // Details returns the detailed review for the given summary.
 func (r *Summary) Details() (*Review, error) {
 	review := Review{
@@ -314,6 +341,18 @@ func (r *Summary) IsOpen() bool {
 	return !r.Submitted && !r.IsAbandoned()
 }
 
+// Verify returns whether or not a summary's comments are a) signed, and b)
+/// that those signatures are verifiable.
+func (r *Summary) Verify(key string) error {
+	for _, thread := range r.Comments {
+		err := thread.Verify(key)
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // Get returns the specified code review.
 //
 // If no review request exists, the returned review is nil.

From 8abc1c7583cc6b7b5138ef55ddee4875d29d87cd Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Thu, 1 Nov 2018 10:40:27 -0700
Subject: [PATCH 4/6] implements signing for remaining subcommands

---
 commands/abandon.go     | 24 +++++++++++++++++++++++-
 commands/comment.go     | 15 +++++++++++++++
 commands/rebase.go      |  4 +++-
 commands/reject.go      | 14 ++++++++++++++
 commands/submit.go      | 10 +++++++---
 repository/git.go       | 10 ++++++++--
 repository/mock_repo.go | 14 +++++++++++---
 review/review.go        | 14 ++++++++++++--
 review/review_test.go   |  8 ++++----
 9 files changed, 97 insertions(+), 16 deletions(-)

diff --git a/commands/abandon.go b/commands/abandon.go
index b9ab6ebe..6f408e16 100644
--- a/commands/abandon.go
+++ b/commands/abandon.go
@@ -25,6 +25,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 	"github.com/google/git-appraise/review/request"
 )
 
@@ -33,6 +34,9 @@ var abandonFlagSet = flag.NewFlagSet("abandon", flag.ExitOnError)
 var (
 	abandonMessageFile = abandonFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	abandonMessage     = abandonFlagSet.String("m", "", "Message to attach to the review")
+
+	abandonSign = abandonFlagSet.Bool("S", false,
+		"Sign the contents of the abandonment")
 )
 
 // abandonReview adds an NMW comment to the current code review.
@@ -88,14 +92,32 @@ func abandonReview(repo repository.Repo, args []string) error {
 	c.Location = &location
 	c.Resolved = &resolved
 
-	err = r.AddComment(c)
+	var key string
+	if *abandonSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 
+	err = r.AddComment(c)
 	if err != nil {
 		return err
 	}
 
 	// Empty target ref indicates that request was abandoned
 	r.Request.TargetRef = ""
+	// (re)sign the request after clearing out `TargetRef'.
+	if *abandonSign {
+		err = gpg.Sign(key, &r.Request)
+		if err != nil {
+			return err
+		}
+	}
 
 	note, err := r.Request.Write()
 	if err != nil {
diff --git a/commands/comment.go b/commands/comment.go
index 28867499..cfb0cf6a 100644
--- a/commands/comment.go
+++ b/commands/comment.go
@@ -25,6 +25,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var commentFlagSet = flag.NewFlagSet("comment", flag.ExitOnError)
@@ -37,6 +38,8 @@ var (
 	commentFile        = commentFlagSet.String("f", "", "File being commented upon")
 	commentLgtm        = commentFlagSet.Bool("lgtm", false, "'Looks Good To Me'. Set this to express your approval. This cannot be combined with nmw")
 	commentNmw         = commentFlagSet.Bool("nmw", false, "'Needs More Work'. Set this to express your disapproval. This cannot be combined with lgtm")
+	commentSign        = commentFlagSet.Bool("S", false,
+		"Sign the contents of the comment")
 )
 
 func init() {
@@ -129,6 +132,18 @@ func commentOnReview(repo repository.Repo, args []string) error {
 		resolved := *commentLgtm
 		c.Resolved = &resolved
 	}
+
+	if *commentSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
+
 	return r.AddComment(c)
 }
 
diff --git a/commands/rebase.go b/commands/rebase.go
index 26c4d3b8..5664cabf 100644
--- a/commands/rebase.go
+++ b/commands/rebase.go
@@ -29,6 +29,8 @@ var rebaseFlagSet = flag.NewFlagSet("rebase", flag.ExitOnError)
 
 var (
 	rebaseArchive = rebaseFlagSet.Bool("archive", true, "Prevent the original commit from being garbage collected.")
+	rebaseSign    = rebaseFlagSet.Bool("S", false,
+		"Sign the contents of the request after the rebase")
 )
 
 // Validate that the user's request to rebase a review makes sense.
@@ -80,7 +82,7 @@ func rebaseReview(repo repository.Repo, args []string) error {
 	if err != nil {
 		return err
 	}
-	return r.Rebase(*rebaseArchive)
+	return r.Rebase(*rebaseArchive, *rebaseSign)
 }
 
 // rebaseCmd defines the "rebase" subcommand.
diff --git a/commands/reject.go b/commands/reject.go
index 500a6515..e0e45bab 100644
--- a/commands/reject.go
+++ b/commands/reject.go
@@ -25,6 +25,7 @@ import (
 	"github.com/google/git-appraise/repository"
 	"github.com/google/git-appraise/review"
 	"github.com/google/git-appraise/review/comment"
+	"github.com/google/git-appraise/review/gpg"
 )
 
 var rejectFlagSet = flag.NewFlagSet("reject", flag.ExitOnError)
@@ -32,6 +33,9 @@ var rejectFlagSet = flag.NewFlagSet("reject", flag.ExitOnError)
 var (
 	rejectMessageFile = rejectFlagSet.String("F", "", "Take the comment from the given file. Use - to read the message from the standard input")
 	rejectMessage     = rejectFlagSet.String("m", "", "Message to attach to the review")
+
+	rejectSign = rejectFlagSet.Bool("S", false,
+		"Sign the contents of the rejection")
 )
 
 // rejectReview adds an NMW comment to the current code review.
@@ -90,6 +94,16 @@ func rejectReview(repo repository.Repo, args []string) error {
 	c := comment.New(userEmail, *rejectMessage)
 	c.Location = &location
 	c.Resolved = &resolved
+	if *rejectSign {
+		key, err := repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &c)
+		if err != nil {
+			return err
+		}
+	}
 	return r.AddComment(c)
 }
 
diff --git a/commands/submit.go b/commands/submit.go
index 9eb2e15f..761723bd 100644
--- a/commands/submit.go
+++ b/commands/submit.go
@@ -32,6 +32,9 @@ var (
 	submitFastForward = submitFlagSet.Bool("fast-forward", false, "Create a merge using the default fast-forward mode.")
 	submitTBR         = submitFlagSet.Bool("tbr", false, "(To be reviewed) Force the submission of a review that has not been accepted.")
 	submitArchive     = submitFlagSet.Bool("archive", true, "Prevent the original commit from being garbage collected; only affects rebased submits.")
+
+	submitSign = submitFlagSet.Bool("S", false,
+		"Sign the contents of the submission")
 )
 
 // Submit the current code review request.
@@ -105,7 +108,7 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 
 	if *submitRebase {
-		if err := r.Rebase(*submitArchive); err != nil {
+		if err := r.Rebase(*submitArchive, *submitSign); err != nil {
 			return err
 		}
 		source, err = r.GetHeadCommit()
@@ -119,9 +122,10 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 	if *submitMerge {
 		submitMessage := fmt.Sprintf("Submitting review %.12s", r.Revision)
-		return repo.MergeRef(source, false, submitMessage, r.Request.Description)
+		return repo.MergeRef(source, false, *submitSign, submitMessage,
+			r.Request.Description)
 	} else {
-		return repo.MergeRef(source, true)
+		return repo.MergeRef(source, true, *submitSign)
 	}
 }
 
diff --git a/repository/git.go b/repository/git.go
index e62e7f1f..4e7c3543 100644
--- a/repository/git.go
+++ b/repository/git.go
@@ -369,13 +369,16 @@ func (repo *GitRepo) ArchiveRef(ref, archive string) error {
 // current ref should only move forward, as opposed to creating a bubble merge.
 // The messages argument(s) provide text that should be included in the default
 // merge commit message (separated by blank lines).
-func (repo *GitRepo) MergeRef(ref string, fastForward bool, messages ...string) error {
+func (repo *GitRepo) MergeRef(ref string, fastForward, sign bool, messages ...string) error {
 	args := []string{"merge"}
 	if fastForward {
 		args = append(args, "--ff", "--ff-only")
 	} else {
 		args = append(args, "--no-ff")
 	}
+	if sign {
+		args = append(args, "-S")
+	}
 	if len(messages) > 0 {
 		commitMessage := strings.Join(messages, "\n\n")
 		args = append(args, "-e", "-m", commitMessage)
@@ -385,7 +388,10 @@ func (repo *GitRepo) MergeRef(ref string, fastForward bool, messages ...string)
 }
 
 // RebaseRef rebases the current ref onto the given one.
-func (repo *GitRepo) RebaseRef(ref string) error {
+func (repo *GitRepo) RebaseRef(ref string, sign bool) error {
+	if sign {
+		return repo.runGitCommandInline("rebase", "-S", "-i", ref)
+	}
 	return repo.runGitCommandInline("rebase", "-i", ref)
 }
 
diff --git a/repository/mock_repo.go b/repository/mock_repo.go
index 72ed6087..e2708265 100644
--- a/repository/mock_repo.go
+++ b/repository/mock_repo.go
@@ -419,7 +419,7 @@ func (r *mockRepoForTest) ArchiveRef(ref, archive string) error {
 //
 // The ref argument is the ref to merge, and fastForward indicates that the
 // current ref should only move forward, as opposed to creating a bubble merge.
-func (r *mockRepoForTest) MergeRef(ref string, fastForward bool, messages ...string) error {
+func (r *mockRepoForTest) MergeRef(ref string, fastForward, sign bool, messages ...string) error {
 	newCommitHash, err := r.resolveLocalRef(ref)
 	if err != nil {
 		return err
@@ -446,7 +446,7 @@ func (r *mockRepoForTest) MergeRef(ref string, fastForward bool, messages ...str
 }
 
 // RebaseRef rebases the current ref onto the given one.
-func (r *mockRepoForTest) RebaseRef(ref string) error {
+func (r *mockRepoForTest) RebaseRef(ref string, sign bool) error {
 	parentHash := r.Refs[ref]
 	origCommit, err := r.getCommit(r.Head)
 	if err != nil {
@@ -573,7 +573,15 @@ func (r *mockRepoForTest) PullNotesAndArchive(remote, notesRefPattern, archiveRe
 	return nil
 }
 
-func (repo *mockRepoForTest) MergeNotesAndArchive(remote, notesRefPattern,
+// MergeNotes merges in the remote's state of the archives reference into
+// the local repository's.
+func (repo *mockRepoForTest) MergeNotes(remote, notesRefPattern string) error {
+	return nil
+}
+
+// MergeArchives merges in the remote's state of the archives reference into
+// the local repository's.
+func (repo *mockRepoForTest) MergeArchives(remote,
 	archiveRefPattern string) error {
 	return nil
 }
diff --git a/review/review.go b/review/review.go
index ade41355..f1b612fc 100644
--- a/review/review.go
+++ b/review/review.go
@@ -685,7 +685,7 @@ func (r *Review) AddComment(c comment.Comment) error {
 // review will be added to the 'refs/devtools/archives/reviews' ref prior
 // to being rewritten. That ensures the review history is kept from being
 // garbage collected.
-func (r *Review) Rebase(archivePrevious bool) error {
+func (r *Review) Rebase(archivePrevious, sign bool) error {
 	if archivePrevious {
 		orig, err := r.GetHeadCommit()
 		if err != nil {
@@ -698,7 +698,7 @@ func (r *Review) Rebase(archivePrevious bool) error {
 	if err := r.Repo.SwitchToRef(r.Request.ReviewRef); err != nil {
 		return err
 	}
-	if err := r.Repo.RebaseRef(r.Request.TargetRef); err != nil {
+	if err := r.Repo.RebaseRef(r.Request.TargetRef, sign); err != nil {
 		return err
 	}
 	alias, err := r.Repo.GetCommitHash("HEAD")
@@ -706,6 +706,16 @@ func (r *Review) Rebase(archivePrevious bool) error {
 		return err
 	}
 	r.Request.Alias = alias
+	if sign {
+		key, err := r.Repo.GetUserSigningKey()
+		if err != nil {
+			return err
+		}
+		err = gpg.Sign(key, &r.Request)
+		if err != nil {
+			return err
+		}
+	}
 	newNote, err := r.Request.Write()
 	if err != nil {
 		return err
diff --git a/review/review_test.go b/review/review_test.go
index af699afd..eb836900 100644
--- a/review/review_test.go
+++ b/review/review_test.go
@@ -751,7 +751,7 @@ func TestRebase(t *testing.T) {
 	}
 
 	// Rebase the review and then confirm that it has been updated correctly.
-	if err := pendingReview.Rebase(true); err != nil {
+	if err := pendingReview.Rebase(true, false); err != nil {
 		t.Fatal(err)
 	}
 	reviewJSON, err := pendingReview.GetJSON()
@@ -785,7 +785,7 @@ func TestRebase(t *testing.T) {
 	if err := repo.SwitchToRef(pendingReview.Request.TargetRef); err != nil {
 		t.Fatal(err)
 	}
-	if err := repo.MergeRef(pendingReview.Request.ReviewRef, true); err != nil {
+	if err := repo.MergeRef(pendingReview.Request.ReviewRef, true, false); err != nil {
 		t.Fatal(err)
 	}
 
@@ -825,7 +825,7 @@ func TestRebaseDetachedHead(t *testing.T) {
 	}
 
 	// Rebase the review and then confirm that it has been updated correctly.
-	if err := pendingReview.Rebase(true); err != nil {
+	if err := pendingReview.Rebase(true, false); err != nil {
 		t.Fatal(err)
 	}
 	headRef, err := repo.GetHeadRef()
@@ -851,7 +851,7 @@ func TestRebaseDetachedHead(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := repo.MergeRef(reviewHead, true); err != nil {
+	if err := repo.MergeRef(reviewHead, true, false); err != nil {
 		t.Fatal(err)
 	}
 

From fe1cb670a7c65902a658a60f86a709d6cd2f5733 Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Thu, 1 Nov 2018 10:40:56 -0700
Subject: [PATCH 5/6] addresses review comments

---
 commands/pull.go       |  7 +++++--
 repository/git.go      | 31 ++++++++++++++-----------------
 repository/repo.go     | 14 ++++++++------
 review/gpg/signable.go |  4 +++-
 4 files changed, 30 insertions(+), 26 deletions(-)

diff --git a/commands/pull.go b/commands/pull.go
index bbbad6a3..e8d87a27 100644
--- a/commands/pull.go
+++ b/commands/pull.go
@@ -79,8 +79,11 @@ func pull(repo repository.Repo, args []string) error {
 		fmt.Println("verified review:", revision)
 	}
 
-	return repo.MergeNotesAndArchive(remote, notesRefPattern,
-		archiveRefPattern)
+	err = repo.MergeNotes(remote, notesRefPattern)
+	if err != nil {
+		return err
+	}
+	return repo.MergeArchives(remote, archiveRefPattern)
 }
 
 var pullCmd = &Command{
diff --git a/repository/git.go b/repository/git.go
index 4e7c3543..3cdc4d16 100644
--- a/repository/git.go
+++ b/repository/git.go
@@ -750,7 +750,9 @@ func getRemoteNotesRef(remote, localNotesRef string) string {
 	return "refs/notes/" + remote + "/" + relativeNotesRef
 }
 
-func (repo *GitRepo) mergeRemoteNotes(remote, notesRefPattern string) error {
+// MergeNotes merges in the remote's state of the notes reference into the
+// local repository's.
+func (repo *GitRepo) MergeNotes(remote, notesRefPattern string) error {
 	remoteRefs, err := repo.runGitCommand("ls-remote", remote, notesRefPattern)
 	if err != nil {
 		return err
@@ -780,7 +782,7 @@ func (repo *GitRepo) PullNotes(remote, notesRefPattern string) error {
 		return err
 	}
 
-	return repo.mergeRemoteNotes(remote, notesRefPattern)
+	return repo.MergeNotes(remote, notesRefPattern)
 }
 
 func getRemoteArchiveRef(remote, archiveRefPattern string) string {
@@ -788,7 +790,9 @@ func getRemoteArchiveRef(remote, archiveRefPattern string) string {
 	return "refs/devtools/remoteArchives/" + remote + "/" + relativeArchiveRef
 }
 
-func (repo *GitRepo) mergeRemoteArchives(remote, archiveRefPattern string) error {
+// MergeArchives merges in the remote's state of the archives reference into
+// the local repository's.
+func (repo *GitRepo) MergeArchives(remote, archiveRefPattern string) error {
 	remoteRefs, err := repo.runGitCommand("ls-remote", remote, archiveRefPattern)
 	if err != nil {
 		return err
@@ -836,22 +840,11 @@ func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPatt
 		return err
 	}
 
-	return repo.MergeNotesAndArchive(remote, notesRefPattern,
-		archiveRefPattern)
-}
-
-// MergeNotesAndArchive merges the notes and archives from `remote` into the
-// local branch.
-func (repo *GitRepo) MergeNotesAndArchive(remote, notesRefPattern,
-	archiveRefPattern string) error {
-
-	if err := repo.mergeRemoteNotes(remote, notesRefPattern); err != nil {
-		return err
-	}
-	if err := repo.mergeRemoteArchives(remote, archiveRefPattern); err != nil {
+	err = repo.MergeNotes(remote, notesRefPattern)
+	if err != nil {
 		return err
 	}
-	return nil
+	return repo.MergeArchives(remote, archiveRefPattern)
 }
 
 // FetchAndReturnNewReviewHashes fetches the notes "branches" and then susses
@@ -887,6 +880,10 @@ func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
 		return strings.Split(rvws, "\n"), nil
 	}
 
+	err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
+	if err != nil {
+		return nil, err
+	}
 	afterHash, err := repo.GetCommitHash(
 		"notes/" + remote + "/devtools/reviews")
 	if err != nil {
diff --git a/repository/repo.go b/repository/repo.go
index 9566fec6..703e7cc7 100644
--- a/repository/repo.go
+++ b/repository/repo.go
@@ -121,10 +121,10 @@ type Repo interface {
 	// current ref should only move forward, as opposed to creating a bubble merge.
 	// The messages argument(s) provide text that should be included in the default
 	// merge commit message (separated by blank lines).
-	MergeRef(ref string, fastForward bool, messages ...string) error
+	MergeRef(ref string, fastForward, sign bool, messages ...string) error
 
 	// RebaseRef rebases the current ref onto the given one.
-	RebaseRef(ref string) error
+	RebaseRef(ref string, sign bool) error
 
 	// ListCommits returns the list of commits reachable from the given ref.
 	//
@@ -190,10 +190,12 @@ type Repo interface {
 	// intend to keep.
 	PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern string) error
 
-	// MergeNotesAndArchive merges the notes and archives from `remote` into
-	// the local branch.
-	MergeNotesAndArchive(remote, notesRefPattern,
-		archiveRefPattern string) error
+	// MergeNotes merges in the remote's state of the archives reference into
+	// the local repository's.
+	MergeNotes(remote, notesRefPattern string) error
+	// MergeArchives merges in the remote's state of the archives reference
+	// into the local repository's.
+	MergeArchives(remote, archiveRefPattern string) error
 
 	// FetchAndReturnNewReviewHashes fetches the notes "branches" and then
 	// susses out the IDs (the revision the review points to) of any new
diff --git a/review/gpg/signable.go b/review/gpg/signable.go
index a7eaf2f2..e8e62fea 100644
--- a/review/gpg/signable.go
+++ b/review/gpg/signable.go
@@ -31,7 +31,7 @@ type Signable interface {
 
 // Signature is `Sig`'s implementation of `Signable`. Through this function, an
 // object which needs to implement `Signable` need only embed `Sig`
-// anonymously. See, e.g., review/equest.go.
+// anonymously. See, e.g., review/request.go.
 func (s *Sig) Signature() *string {
 	return &s.Sig
 }
@@ -79,6 +79,8 @@ func Verify(key string, s Signable) error {
 	// Overwrite the value with the placeholder.
 	*sigPtr = placeholder
 
+	defer func() { *sigPtr = sig }()
+
 	// 1. Marshal the content into JSON.
 	// 2. Write the signature and the content to temp files.
 	// 3. Use gpg to verify the signature.

From 19e9212616a9c06b0eb5986817f67f2a073ef68f Mon Sep 17 00:00:00 2001
From: dan pittman <dan@dpitt.me>
Date: Mon, 5 Nov 2018 14:48:48 -0800
Subject: [PATCH 6/6] addesses second round of review comments

---
 commands/pull.go        |   6 +-
 commands/rebase.go      |   5 +-
 commands/submit.go      |  24 +++++--
 repository/git.go       | 153 +++++++++++++++++++++++++++++++---------
 repository/mock_repo.go |  18 ++++-
 repository/repo.go      |  17 ++++-
 review/gpg/signable.go  |  11 +--
 review/review.go        |  72 +++++++++++++++----
 review/review_test.go   |   8 +--
 9 files changed, 245 insertions(+), 69 deletions(-)

diff --git a/commands/pull.go b/commands/pull.go
index e8d87a27..63fd2399 100644
--- a/commands/pull.go
+++ b/commands/pull.go
@@ -61,10 +61,6 @@ func pull(repo repository.Repo, args []string) error {
 	if err != nil {
 		return err
 	}
-	key, err := repo.GetUserSigningKey()
-	if err != nil {
-		return err
-	}
 	for _, revision := range revisions {
 		rvw, err := review.GetSummaryViaRefs(repo,
 			"refs/notes/"+remote+"/devtools/reviews",
@@ -72,7 +68,7 @@ func pull(repo repository.Repo, args []string) error {
 		if err != nil {
 			return err
 		}
-		err = rvw.Verify(key)
+		err = rvw.Verify()
 		if err != nil {
 			return err
 		}
diff --git a/commands/rebase.go b/commands/rebase.go
index 5664cabf..2c4595a5 100644
--- a/commands/rebase.go
+++ b/commands/rebase.go
@@ -82,7 +82,10 @@ func rebaseReview(repo repository.Repo, args []string) error {
 	if err != nil {
 		return err
 	}
-	return r.Rebase(*rebaseArchive, *rebaseSign)
+	if *rebaseSign {
+		return r.RebaseAndSign(*rebaseArchive)
+	}
+	return r.Rebase(*rebaseArchive)
 }
 
 // rebaseCmd defines the "rebase" subcommand.
diff --git a/commands/submit.go b/commands/submit.go
index 761723bd..58fa0023 100644
--- a/commands/submit.go
+++ b/commands/submit.go
@@ -108,9 +108,16 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 
 	if *submitRebase {
-		if err := r.Rebase(*submitArchive, *submitSign); err != nil {
+		var err error
+		if *submitSign {
+			err = r.RebaseAndSign(*submitArchive)
+		} else {
+			err = r.Rebase(*submitArchive)
+		}
+		if err != nil {
 			return err
 		}
+
 		source, err = r.GetHeadCommit()
 		if err != nil {
 			return err
@@ -122,10 +129,19 @@ func submitReview(repo repository.Repo, args []string) error {
 	}
 	if *submitMerge {
 		submitMessage := fmt.Sprintf("Submitting review %.12s", r.Revision)
-		return repo.MergeRef(source, false, *submitSign, submitMessage,
-			r.Request.Description)
+		if *submitSign {
+			return repo.MergeAndSignRef(source, false, submitMessage,
+				r.Request.Description)
+		} else {
+			return repo.MergeRef(source, false, submitMessage,
+				r.Request.Description)
+		}
 	} else {
-		return repo.MergeRef(source, true, *submitSign)
+		if *submitSign {
+			return repo.MergeAndSignRef(source, true)
+		} else {
+			return repo.MergeRef(source, true)
+		}
 	}
 }
 
diff --git a/repository/git.go b/repository/git.go
index 3cdc4d16..31d27ea6 100644
--- a/repository/git.go
+++ b/repository/git.go
@@ -369,15 +369,36 @@ func (repo *GitRepo) ArchiveRef(ref, archive string) error {
 // current ref should only move forward, as opposed to creating a bubble merge.
 // The messages argument(s) provide text that should be included in the default
 // merge commit message (separated by blank lines).
-func (repo *GitRepo) MergeRef(ref string, fastForward, sign bool, messages ...string) error {
+func (repo *GitRepo) MergeRef(ref string, fastForward bool, messages ...string) error {
 	args := []string{"merge"}
 	if fastForward {
 		args = append(args, "--ff", "--ff-only")
 	} else {
 		args = append(args, "--no-ff")
 	}
-	if sign {
-		args = append(args, "-S")
+	if len(messages) > 0 {
+		commitMessage := strings.Join(messages, "\n\n")
+		args = append(args, "-e", "-m", commitMessage)
+	}
+	args = append(args, ref)
+	return repo.runGitCommandInline(args...)
+}
+
+// MergeAndSignRef merges the given ref into the current one and signs the
+// merge.
+//
+// The ref argument is the ref to merge, and fastForward indicates that the
+// current ref should only move forward, as opposed to creating a bubble merge.
+// The messages argument(s) provide text that should be included in the default
+// merge commit message (separated by blank lines).
+func (repo *GitRepo) MergeAndSignRef(ref string, fastForward bool,
+	messages ...string) error {
+
+	args := []string{"merge"}
+	if fastForward {
+		args = append(args, "--ff", "--ff-only", "-S")
+	} else {
+		args = append(args, "--no-ff", "-S")
 	}
 	if len(messages) > 0 {
 		commitMessage := strings.Join(messages, "\n\n")
@@ -388,13 +409,16 @@ func (repo *GitRepo) MergeRef(ref string, fastForward, sign bool, messages ...st
 }
 
 // RebaseRef rebases the current ref onto the given one.
-func (repo *GitRepo) RebaseRef(ref string, sign bool) error {
-	if sign {
-		return repo.runGitCommandInline("rebase", "-S", "-i", ref)
-	}
+func (repo *GitRepo) RebaseRef(ref string) error {
 	return repo.runGitCommandInline("rebase", "-i", ref)
 }
 
+// RebaseAndSignRef rebases the current ref onto the given one and signs the
+// result.
+func (repo *GitRepo) RebaseAndSignRef(ref string) error {
+	return repo.runGitCommandInline("rebase", "-S", "-i", ref)
+}
+
 // ListCommits returns the list of commits reachable from the given ref.
 //
 // The generated list is in chronological order (with the oldest commit first).
@@ -857,48 +881,107 @@ func (repo *GitRepo) PullNotesAndArchive(remote, notesRefPattern, archiveRefPatt
 func (repo *GitRepo) FetchAndReturnNewReviewHashes(remote, notesRefPattern,
 	archiveRefPattern string) ([]string, error) {
 
-	beforeHash, err := repo.GetCommitHash(
+	// Record the current state of the reviews and comments refs.
+	var (
+		getAllRevs, getAllComs    bool
+		reviewsList, commentsList []string
+	)
+	reviewBeforeHash, err := repo.GetCommitHash(
 		"notes/" + remote + "/devtools/reviews")
+	getAllRevs = err != nil
+
+	commentBeforeHash, err := repo.GetCommitHash(
+		"notes/" + remote + "/devtools/discuss")
+	getAllComs = err != nil
+
+	// Update them from the remote.
+	err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
 	if err != nil {
-		// We don't have a reference for our remote yet. This is the first
-		// fetch of notes. We need to return _all_ hashes, which is to say,
-		// all the files present at where the reference
-		// `notes/<remote>/reviews' points.
-		err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
-		if err != nil {
-			return nil, err
-		}
+		return nil, err
+	}
+
+	// Now, if either of these are new refs, we just use the whole tree at that
+	// new ref. Otherwise we see which reviews or comments changed and collect
+	// them into a list.
+	if getAllRevs {
 		hash, err := repo.GetCommitHash(
 			"notes/" + remote + "/devtools/reviews")
+		// It is possible that even after we've pulled that this ref still
+		// isn't present (because there are no reviews yet).
+		if err == nil {
+			rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only",
+				hash)
+			if err != nil {
+				return nil, err
+			}
+			reviewsList = strings.Split(strings.Replace(rvws, "/", "", -1),
+				"\n")
+		}
+	} else {
+		reviewAfterHash, err := repo.GetCommitHash(
+			"notes/" + remote + "/devtools/reviews")
 		if err != nil {
 			return nil, err
 		}
-		rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only", hash)
+
+		// Only run through this if the fetch fetched new revisions.
+		// Otherwise leave reviewsList as its default value, an empty slice
+		// of strings.
+		if reviewBeforeHash != reviewAfterHash {
+			newReviewsRaw, err := repo.runGitCommand("diff", "--name-only",
+				reviewBeforeHash, reviewAfterHash)
+			if err != nil {
+				return nil, err
+			}
+			reviewsList = strings.Split(strings.Replace(newReviewsRaw,
+				"/", "", -1), "\n")
+		}
+	}
+
+	if getAllComs {
+		hash, err := repo.GetCommitHash(
+			"notes/" + remote + "/devtools/discuss")
+		// It is possible that even after we've pulled that this ref still
+		// isn't present (because there are no comments yet).
+		if err == nil {
+			rvws, err := repo.runGitCommand("ls-tree", "-r", "--name-only",
+				hash)
+			if err != nil {
+				return nil, err
+			}
+			commentsList = strings.Split(strings.Replace(rvws, "/", "", -1),
+				"\n")
+		}
+	} else {
+		commentAfterHash, err := repo.GetCommitHash(
+			"notes/" + remote + "/devtools/discuss")
 		if err != nil {
 			return nil, err
 		}
-		return strings.Split(rvws, "\n"), nil
-	}
 
-	err = repo.fetchNotes(remote, notesRefPattern, archiveRefPattern)
-	if err != nil {
-		return nil, err
-	}
-	afterHash, err := repo.GetCommitHash(
-		"notes/" + remote + "/devtools/reviews")
-	if err != nil {
-		return nil, err
+		// Only run through this if the fetch fetched new revisions.
+		// Otherwise leave commentsList as its default value, an empty slice
+		// of strings.
+		if commentBeforeHash != commentAfterHash {
+			newCommentsRaw, err := repo.runGitCommand("diff", "--name-only",
+				commentBeforeHash, commentAfterHash)
+			if err != nil {
+				return nil, err
+			}
+			commentsList = strings.Split(strings.Replace(newCommentsRaw,
+				"/", "", -1), "\n")
+		}
 	}
 
-	// Nothing new here.
-	if beforeHash == afterHash {
-		return []string{}, nil
+	// Now that we have our two lists, we need to merge them.
+	updatedReviewSet := make(map[string]struct{})
+	for _, hash := range append(reviewsList, commentsList...) {
+		updatedReviewSet[hash] = struct{}{}
 	}
 
-	newReviews, err := repo.runGitCommand("diff", "--name-only", beforeHash,
-		afterHash)
-	if err != nil {
-		return nil, err
+	updatedReviews := make([]string, 0, len(updatedReviewSet))
+	for key, _ := range updatedReviewSet {
+		updatedReviews = append(updatedReviews, key)
 	}
-	return strings.Split(newReviews, "\n"), nil
+	return updatedReviews, nil
 }
diff --git a/repository/mock_repo.go b/repository/mock_repo.go
index e2708265..2d8debe4 100644
--- a/repository/mock_repo.go
+++ b/repository/mock_repo.go
@@ -419,7 +419,7 @@ func (r *mockRepoForTest) ArchiveRef(ref, archive string) error {
 //
 // The ref argument is the ref to merge, and fastForward indicates that the
 // current ref should only move forward, as opposed to creating a bubble merge.
-func (r *mockRepoForTest) MergeRef(ref string, fastForward, sign bool, messages ...string) error {
+func (r *mockRepoForTest) MergeRef(ref string, fastForward bool, messages ...string) error {
 	newCommitHash, err := r.resolveLocalRef(ref)
 	if err != nil {
 		return err
@@ -445,8 +445,18 @@ func (r *mockRepoForTest) MergeRef(ref string, fastForward, sign bool, messages
 	return nil
 }
 
+// MergeAndSignRef merges the given ref into the current one and signs the
+// merge.
+//
+// The ref argument is the ref to merge, and fastForward indicates that the
+// current ref should only move forward, as opposed to creating a bubble merge.
+func (r *mockRepoForTest) MergeAndSignRef(ref string, fastForward bool,
+	messages ...string) error {
+	return nil
+}
+
 // RebaseRef rebases the current ref onto the given one.
-func (r *mockRepoForTest) RebaseRef(ref string, sign bool) error {
+func (r *mockRepoForTest) RebaseRef(ref string) error {
 	parentHash := r.Refs[ref]
 	origCommit, err := r.getCommit(r.Head)
 	if err != nil {
@@ -466,6 +476,10 @@ func (r *mockRepoForTest) RebaseRef(ref string, sign bool) error {
 	return nil
 }
 
+// RebaseAndSignRef rebases the current ref onto the given one and signs the
+// result.
+func (r *mockRepoForTest) RebaseAndSignRef(ref string) error { return nil }
+
 // ListCommits returns the list of commits reachable from the given ref.
 //
 // The generated list is in chronological order (with the oldest commit first).
diff --git a/repository/repo.go b/repository/repo.go
index 703e7cc7..91acd177 100644
--- a/repository/repo.go
+++ b/repository/repo.go
@@ -121,10 +121,23 @@ type Repo interface {
 	// current ref should only move forward, as opposed to creating a bubble merge.
 	// The messages argument(s) provide text that should be included in the default
 	// merge commit message (separated by blank lines).
-	MergeRef(ref string, fastForward, sign bool, messages ...string) error
+	MergeRef(ref string, fastForward bool, messages ...string) error
+
+	// MergeAndSignRef merges the given ref into the current one and signs the
+	// merge.
+	//
+	// The ref argument is the ref to merge, and fastForward indicates that the
+	// current ref should only move forward, as opposed to creating a bubble merge.
+	// The messages argument(s) provide text that should be included in the default
+	// merge commit message (separated by blank lines).
+	MergeAndSignRef(ref string, fastForward bool, messages ...string) error
 
 	// RebaseRef rebases the current ref onto the given one.
-	RebaseRef(ref string, sign bool) error
+	RebaseRef(ref string) error
+
+	// RebaseAndSignRef rebases the current ref onto the given one and signs
+	// the result.
+	RebaseAndSignRef(ref string) error
 
 	// ListCommits returns the list of commits reachable from the given ref.
 	//
diff --git a/review/gpg/signable.go b/review/gpg/signable.go
index e8e62fea..776764c6 100644
--- a/review/gpg/signable.go
+++ b/review/gpg/signable.go
@@ -5,6 +5,7 @@ package gpg
 import (
 	"bytes"
 	"encoding/json"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"os/exec"
@@ -71,7 +72,7 @@ func signContent(key string, content []byte) (*bytes.Buffer,
 
 // Verify verifies the signatures on the request and its comments with the
 // given key.
-func Verify(key string, s Signable) error {
+func Verify(s Signable) error {
 	// Retrieve the pointer.
 	sigPtr := s.Signature()
 	// Copy its contents.
@@ -117,10 +118,12 @@ func Verify(key string, s Signable) error {
 	}
 
 	var stdout, stderr bytes.Buffer
-	cmd := exec.Command("gpg", "-u", key, "--verify", sigFile.Name(),
-		contentFile.Name())
+	cmd := exec.Command("gpg", "--verify", sigFile.Name(), contentFile.Name())
 	cmd.Stdout = &stdout
 	cmd.Stderr = &stderr
 	err = cmd.Run()
-	return err
+	if err != nil {
+		return fmt.Errorf("%s", stderr.String())
+	}
+	return nil
 }
diff --git a/review/review.go b/review/review.go
index f1b612fc..a23dd17b 100644
--- a/review/review.go
+++ b/review/review.go
@@ -166,13 +166,14 @@ func (thread *CommentThread) updateResolvedStatus() {
 }
 
 // Verify verifies the signature on a comment.
-func (thread *CommentThread) Verify(key string) error {
-	err := gpg.Verify(key, &thread.Comment)
+func (thread *CommentThread) Verify() error {
+	err := gpg.Verify(&thread.Comment)
 	if err != nil {
-		return err
+		hash, _ := thread.Comment.Hash()
+		return fmt.Errorf("verification of comment [%s] failed: %s", hash, err)
 	}
 	for _, child := range thread.Children {
-		err = child.Verify(key)
+		err = child.Verify()
 		if err != nil {
 			return err
 		}
@@ -343,9 +344,14 @@ func (r *Summary) IsOpen() bool {
 
 // Verify returns whether or not a summary's comments are a) signed, and b)
 /// that those signatures are verifiable.
-func (r *Summary) Verify(key string) error {
+func (r *Summary) Verify() error {
+	err := gpg.Verify(&r.Request)
+	if err != nil {
+		return fmt.Errorf("couldn't verify request targeting: %q: %s",
+			r.Request.TargetRef, err)
+	}
 	for _, thread := range r.Comments {
-		err := thread.Verify(key)
+		err := thread.Verify()
 		if err != nil {
 			return err
 		}
@@ -685,7 +691,7 @@ func (r *Review) AddComment(c comment.Comment) error {
 // review will be added to the 'refs/devtools/archives/reviews' ref prior
 // to being rewritten. That ensures the review history is kept from being
 // garbage collected.
-func (r *Review) Rebase(archivePrevious, sign bool) error {
+func (r *Review) Rebase(archivePrevious bool) error {
 	if archivePrevious {
 		orig, err := r.GetHeadCommit()
 		if err != nil {
@@ -698,24 +704,66 @@ func (r *Review) Rebase(archivePrevious, sign bool) error {
 	if err := r.Repo.SwitchToRef(r.Request.ReviewRef); err != nil {
 		return err
 	}
-	if err := r.Repo.RebaseRef(r.Request.TargetRef, sign); err != nil {
+
+	err := r.Repo.RebaseRef(r.Request.TargetRef)
+	if err != nil {
 		return err
 	}
+
 	alias, err := r.Repo.GetCommitHash("HEAD")
 	if err != nil {
 		return err
 	}
 	r.Request.Alias = alias
-	if sign {
-		key, err := r.Repo.GetUserSigningKey()
+	newNote, err := r.Request.Write()
+	if err != nil {
+		return err
+	}
+	return r.Repo.AppendNote(request.Ref, r.Revision, newNote)
+}
+
+// RebaseAndSign performs an interactive rebase of the review onto its
+// target ref. It signs the result of the rebase as well as (re)signs
+// the review request itself.
+//
+// If the 'archivePrevious' argument is true, then the previous head of the
+// review will be added to the 'refs/devtools/archives/reviews' ref prior
+// to being rewritten. That ensures the review history is kept from being
+// garbage collected.
+func (r *Review) RebaseAndSign(archivePrevious bool) error {
+	if archivePrevious {
+		orig, err := r.GetHeadCommit()
 		if err != nil {
 			return err
 		}
-		err = gpg.Sign(key, &r.Request)
-		if err != nil {
+		if err := r.Repo.ArchiveRef(orig, archiveRef); err != nil {
 			return err
 		}
 	}
+	if err := r.Repo.SwitchToRef(r.Request.ReviewRef); err != nil {
+		return err
+	}
+
+	err := r.Repo.RebaseAndSignRef(r.Request.TargetRef)
+	if err != nil {
+		return err
+	}
+
+	alias, err := r.Repo.GetCommitHash("HEAD")
+	if err != nil {
+		return err
+	}
+	r.Request.Alias = alias
+
+	key, err := r.Repo.GetUserSigningKey()
+	if err != nil {
+		return err
+	}
+	err = gpg.Sign(key, &r.Request)
+	if err != nil {
+		return err
+	}
+
 	newNote, err := r.Request.Write()
 	if err != nil {
 		return err
diff --git a/review/review_test.go b/review/review_test.go
index eb836900..af699afd 100644
--- a/review/review_test.go
+++ b/review/review_test.go
@@ -751,7 +751,7 @@ func TestRebase(t *testing.T) {
 	}
 
 	// Rebase the review and then confirm that it has been updated correctly.
-	if err := pendingReview.Rebase(true, false); err != nil {
+	if err := pendingReview.Rebase(true); err != nil {
 		t.Fatal(err)
 	}
 	reviewJSON, err := pendingReview.GetJSON()
@@ -785,7 +785,7 @@ func TestRebase(t *testing.T) {
 	if err := repo.SwitchToRef(pendingReview.Request.TargetRef); err != nil {
 		t.Fatal(err)
 	}
-	if err := repo.MergeRef(pendingReview.Request.ReviewRef, true, false); err != nil {
+	if err := repo.MergeRef(pendingReview.Request.ReviewRef, true); err != nil {
 		t.Fatal(err)
 	}
 
@@ -825,7 +825,7 @@ func TestRebaseDetachedHead(t *testing.T) {
 	}
 
 	// Rebase the review and then confirm that it has been updated correctly.
-	if err := pendingReview.Rebase(true, false); err != nil {
+	if err := pendingReview.Rebase(true); err != nil {
 		t.Fatal(err)
 	}
 	headRef, err := repo.GetHeadRef()
@@ -851,7 +851,7 @@ func TestRebaseDetachedHead(t *testing.T) {
 	if err != nil {
 		t.Fatal(err)
 	}
-	if err := repo.MergeRef(reviewHead, true, false); err != nil {
+	if err := repo.MergeRef(reviewHead, true); err != nil {
 		t.Fatal(err)
 	}