Submitting review efcba5b

Better support for rebasing reviews

This change improves the support for rebasing reviews in multiple ways:

1. Adding a new `refs/archives/devtools` ref that can be used to make sure that commits which have been rebased do not get garbage collected.
2. Adding a new `alias` field to the request schema that enables tracking review history across rebases.
3. Adding a new `rebase` subcommand that enables us to update the history for a change prior to submitting it.

Author: ojarjur

commands/commands.go

@@ -22,6 +22,7 @@ import (
 )
 
 const notesRefPattern = "refs/notes/devtools/*"
+const archiveRefPattern = "refs/devtools/archives/*"
 const commentFilename = "APPRAISE_COMMENT_EDITMSG"
 
 // Command represents the definition of a single command.
@@ -45,6 +46,7 @@ var CommandMap = map[string]*Command{
 	"list":    listCmd,
 	"pull":    pullCmd,
 	"push":    pushCmd,
+	"rebase":  rebaseCmd,
 	"reject":  rejectCmd,
 	"request": requestCmd,
 	"show":    showCmd,

commands/pull.go

@@ -33,7 +33,9 @@ func pull(repo repository.Repo, args []string) error {
 		remote = args[0]
 	}
 
-	repo.PullNotes(remote, notesRefPattern)
+	if err := repo.PullNotesAndArchive(remote, notesRefPattern, archiveRefPattern); err != nil {
+		return err
+	}
 	return nil
 }
 

commands/push.go

@@ -33,7 +33,10 @@ func push(repo repository.Repo, args []string) error {
 		remote = args[0]
 	}
 
-	return repo.PushNotes(remote, notesRefPattern)
+	if err := repo.PushNotesAndArchive(remote, notesRefPattern, archiveRefPattern); err != nil {
+		return err
+	}
+	return nil
 }
 
 var pushCmd = &Command{

commands/rebase.go

@@ -0,0 +1,90 @@
+/*
+Copyright 2016 Google Inc. All rights reserved.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package commands
+
+import (
+	"errors"
+	"flag"
+	"fmt"
+	"github.com/google/git-appraise/repository"
+	"github.com/google/git-appraise/review"
+)
+
+var rebaseFlagSet = flag.NewFlagSet("rebase", flag.ExitOnError)
+
+var (
+	rebaseArchive = rebaseFlagSet.Bool("archive", true, "Prevent the original commit from being garbage collected.")
+)
+
+// Validate that the user's request to rebase a review makes sense.
+//
+// This checks both that the request is well formed, and that the
+// corresponding review is in a state where rebasing is appropriate.
+func validateRebaseRequest(repo repository.Repo, args []string) (*review.Review, error) {
+	var r *review.Review
+	var err error
+	if len(args) > 1 {
+		return nil, errors.New("Only rebasing a single review is supported.")
+	}
+	if len(args) == 1 {
+		r, err = review.Get(repo, args[0])
+	} else {
+		r, err = review.GetCurrent(repo)
+	}
+	if err != nil {
+		return nil, fmt.Errorf("Failed to load the review: %v\n", err)
+	}
+	if r == nil {
+		return nil, errors.New("There is no matching review.")
+	}
+
+	if r.Submitted {
+		return nil, errors.New("The review has already been submitted.")
+	}
+
+	target := r.Request.TargetRef
+	if err := repo.VerifyGitRef(target); err != nil {
+		return nil, err
+	}
+
+	return r, nil
+}
+
+// Rebase the current code review.
+//
+// The "args" parameter contains all of the command line arguments that followed the subcommand.
+func rebaseReview(repo repository.Repo, args []string) error {
+	rebaseFlagSet.Parse(args)
+	args = rebaseFlagSet.Args()
+
+	r, err := validateRebaseRequest(repo, args)
+	if err != nil {
+		return err
+	}
+	return r.Rebase(*rebaseArchive)
+}
+
+// rebaseCmd defines the "rebase" subcommand.
+var rebaseCmd = &Command{
+	Usage: func(arg0 string) {
+		fmt.Printf("Usage: %s rebase [<option>...] [<review-hash>]\n\nOptions:\n", arg0)
+		rebaseFlagSet.PrintDefaults()
+	},
+	RunMethod: func(repo repository.Repo, args []string) error {
+		return rebaseReview(repo, args)
+	},
+}

commands/request.go

@@ -65,11 +65,39 @@ func buildRequestFromFlags(requester string) (request.Request, error) {
 	return request.New(requester, reviewers, *requestSource, *requestTarget, *requestMessage), nil
 }
 
+// Get the commit at which the review request should be anchored.
+func getReviewCommit(repo repository.Repo, r request.Request, args []string) (string, string, error) {
+	if len(args) > 1 {
+		return "", "", errors.New("Only updating a single review is supported.")
+	}
+	if len(args) == 1 {
+		base, err := repo.MergeBase(r.TargetRef, args[0])
+		if err != nil {
+			return "", "", err
+		}
+		return args[0], base, nil
+	}
+
+	base, err := repo.MergeBase(r.TargetRef, r.ReviewRef)
+	if err != nil {
+		return "", "", err
+	}
+	reviewCommits, err := repo.ListCommitsBetween(base, r.ReviewRef)
+	if err != nil {
+		return "", "", err
+	}
+	if reviewCommits == nil {
+		return "", "", errors.New("There are no commits included in the review request")
+	}
+	return reviewCommits[0], base, nil
+}
+
 // Create a new code review request.
 //
 // The "args" parameter is all of the command line arguments that followed the subcommand.
 func requestReview(repo repository.Repo, args []string) error {
 	requestFlagSet.Parse(args)
+	args = requestFlagSet.Args()
 
 	if !*requestAllowUncommitted {
 		// Requesting a code review with uncommited local changes is usually a mistake, so
@@ -104,22 +132,14 @@ func requestReview(repo repository.Repo, args []string) error {
 	if err := repo.VerifyGitRef(r.ReviewRef); err != nil {
 		return err
 	}
-	base, err := repo.MergeBase(r.TargetRef, r.ReviewRef)
-	if err != nil {
-		return err
-	}
-	r.BaseCommit = base
 
-	reviewCommits, err := repo.ListCommitsBetween(base, r.ReviewRef)
+	reviewCommit, baseCommit, err := getReviewCommit(repo, r, args)
 	if err != nil {
 		return err
 	}
-	if reviewCommits == nil {
-		return errors.New("There are no commits included in the review request")
-	}
-
+	r.BaseCommit = baseCommit
 	if r.Description == "" {
-		description, err := repo.GetCommitMessage(reviewCommits[0])
+		description, err := repo.GetCommitMessage(reviewCommit)
 		if err != nil {
 			return err
 		}
@@ -130,17 +150,17 @@ func requestReview(repo repository.Repo, args []string) error {
 	if err != nil {
 		return err
 	}
-	repo.AppendNote(request.Ref, reviewCommits[0], note)
+	repo.AppendNote(request.Ref, reviewCommit, note)
 	if !*requestQuiet {
-		fmt.Printf(requestSummaryTemplate, reviewCommits[0], r.TargetRef, r.ReviewRef, r.Description)
+		fmt.Printf(requestSummaryTemplate, reviewCommit, r.TargetRef, r.ReviewRef, r.Description)
 	}
 	return nil
 }
 
 // requestCmd defines the "request" subcommand.
 var requestCmd = &Command{
 	Usage: func(arg0 string) {
-		fmt.Printf("Usage: %s request [<option>...]\n\nOptions:\n", arg0)
+		fmt.Printf("Usage: %s request [<option>...] [<review-hash>]\n\nOptions:\n", arg0)
 		requestFlagSet.PrintDefaults()
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {

commands/submit.go

@@ -31,6 +31,7 @@ var (
 	submitRebase      = submitFlagSet.Bool("rebase", false, "Rebase the source ref onto the target ref.")
 	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.")
 )
 
 // Submit the current code review request.
@@ -87,10 +88,6 @@ func submitReview(repo repository.Repo, args []string) error {
 		return errors.New("Refusing to submit a non-fast-forward review. First merge the target ref.")
 	}
 
-	if err := repo.SwitchToRef(target); err != nil {
-		return err
-	}
-
 	if !(*submitRebase || *submitMerge || *submitFastForward) {
 		submitStrategy, err := repo.GetSubmitStrategy()
 		if err != nil {
@@ -107,11 +104,22 @@ func submitReview(repo repository.Repo, args []string) error {
 		}
 	}
 
+	if *submitRebase {
+		if err := r.Rebase(*submitArchive); err != nil {
+			return err
+		}
+		source, err = r.GetHeadCommit()
+		if err != nil {
+			return err
+		}
+	}
+
+	if err := repo.SwitchToRef(target); err != nil {
+		return err
+	}
 	if *submitMerge {
 		submitMessage := fmt.Sprintf("Submitting review %.12s", r.Revision)
 		return repo.MergeRef(source, false, submitMessage, r.Request.Description)
-	} else if *submitRebase {
-		return repo.RebaseRef(source)
 	} else {
 		return repo.MergeRef(source, true)
 	}
@@ -120,7 +128,7 @@ func submitReview(repo repository.Repo, args []string) error {
 // submitCmd defines the "submit" subcommand.
 var submitCmd = &Command{
 	Usage: func(arg0 string) {
-		fmt.Printf("Usage: %s submit [<option>...]\n\nOptions:\n", arg0)
+		fmt.Printf("Usage: %s submit [<option>...] [<review-hash>]\n\nOptions:\n", arg0)
 		submitFlagSet.PrintDefaults()
 	},
 	RunMethod: func(repo repository.Repo, args []string) error {