git-codereview: add sync-branch -merge-back-to-parent
Dev branches come to an end.
Making sync-branch help that process instead of forcing
people to follow a playbook will help avoid mistakes.
The flag name was chosen to be very unlikely to be used
accidentally, and the commit subject and message both
are distinct to make clear to reviewers what they are being
asked to +2.
The Merge List is also included in full and is likely to be
quite large, yet another signal for everyone involved about
the magnitude and weight of the change.
Change-Id: I91cdda2b85cd3811711a339f4f3290fee109022e
diff --git a/git-codereview/sync.go b/git-codereview/sync.go
index c38215c..b079c84 100644
--- a/git-codereview/sync.go
+++ b/git-codereview/sync.go
@@ -127,8 +127,9 @@
func cmdSyncBranch(args []string) {
os.Setenv("GIT_EDITOR", ":") // do not bring up editor during merge, commit
- var cont bool
+ var cont, mergeBackToParent bool
flags.BoolVar(&cont, "continue", false, "continue after merge conflicts")
+ flags.BoolVar(&mergeBackToParent, "merge-back-to-parent", false, "for shutting down the dev branch")
flags.Parse(args)
if len(flag.Args()) > 0 {
fmt.Fprintf(stderr(), "Usage: %s sync-branch %s [-continue]\n", progName, globalFlags)
@@ -155,10 +156,17 @@
}
if cont {
+ // Note: There is no -merge-back-to-parent -continue
+ // because -merge-back-to-parent never has merge conflicts.
+ // (It requires that the parent be fully merged into the
+ // dev branch or it won't even attempt the reverse merge.)
+ if mergeBackToParent {
+ dief("cannot use -continue with -merge-back-to-parent")
+ }
if _, err := os.Stat(syncBranchStatusFile()); err != nil {
dief("cannot sync-branch -continue: no pending sync-branch status file found")
}
- syncBranchContinue(" -continue", b, readSyncBranchStatus())
+ syncBranchContinue(syncBranchContinueFlag, b, readSyncBranchStatus())
return
}
@@ -192,11 +200,59 @@
}
writeSyncBranchStatus(status)
- // Start the merge.
- _, err := cmdOutputErr("git", "merge", "origin/"+parent)
+ parentHash, err := cmdOutputErr("git", "rev-parse", "origin/"+parent)
+ if err != nil {
+ dief("cannot sync-branch: cannot resolve origin/%s: %v\n%s", parent, err, parentHash)
+ }
+ branchHash, err := cmdOutputErr("git", "rev-parse", "origin/"+branch)
+ if err != nil {
+ dief("cannot sync-branch: cannot resolve origin/%s: %v\n%s", branch, err, branchHash)
+ }
+ parentHash = trim(parentHash)
+ branchHash = trim(branchHash)
- // Resolve codereview.cfg the right way (never take it from the merge).
- cmdOutputDir(repoRoot(), "git", "checkout", "HEAD", "--", "codereview.cfg")
+ // Only --merge-back-to-parent when there's nothing waiting
+ // to be merged in from parent. If a non-trivial merge needs
+ // to be done, it should be done first on the dev branch,
+ // not the parent branch.
+ if mergeBackToParent {
+ other := cmdOutput("git", "log", "--format=format:+ %cd %h %s", "--date=short", "origin/"+branch+"..origin/"+parent)
+ if other != "" {
+ dief("cannot sync-branch --merge-back-to-parent: parent has new commits.\n"+
+ "\trun 'git sync-branch' to bring them into this branch first:\n%s",
+ other)
+ }
+ }
+
+ // Start the merge.
+ if mergeBackToParent {
+ // Change HEAD back to "parent" and merge "branch" into it,
+ // even though we could instead merge "parent" into "branch".
+ // This way the parent-branch lineage ends up the first parent
+ // of the merge, the same as it would when we are doing it by hand
+ // with a plain "git merge". This may help the display of the
+ // merge graph in some tools more closely reflect what we did.
+ run("git", "reset", "--hard", "origin/"+parent)
+ _, err = cmdOutputErr("git", "merge", "--no-ff", "origin/"+branch)
+ } else {
+ _, err = cmdOutputErr("git", "merge", "origin/"+parent)
+ }
+
+ // Resolve codereview.cfg the right way - never take it from the merge.
+ // For a regular sync-branch we keep the branch's.
+ // For a merge-back-to-parent we take the parent's.
+ // The codereview.cfg contains the branch config and we don't want
+ // it to change.
+ what := branchHash
+ if mergeBackToParent {
+ what = parentHash
+ }
+ cmdOutputDir(repoRoot(), "git", "checkout", what, "--", "codereview.cfg")
+
+ if mergeBackToParent {
+ syncBranchContinue(syncBranchMergeBackFlag, b, status)
+ return
+ }
if err != nil {
// Check whether the only listed file is codereview.cfg and try again if so.
@@ -224,8 +280,10 @@
if err != nil {
if len(status.Conflicts) == 0 {
- dief("cannot sync-branch: git merge failed but no conflicts found\n" +
- "(unexpected error, please ask for help!)")
+ dief("cannot sync-branch: git merge failed but no conflicts found\n"+
+ "(unexpected error, please ask for help!)\n\ngit status:\n%s\ngit status -b --porcelain:\n%s",
+ cmdOutputDir(repoRoot(), "git", "status"),
+ cmdOutputDir(repoRoot(), "git", "status", "-b", "--porcelain"))
}
dief("sync-branch: merge conflicts in:\n\t- %s\n\n"+
"Please fix them (use 'git status' to see the list again),\n"+
@@ -246,6 +304,18 @@
cmd)
}
+func prefixFor(branch string) string {
+ if strings.HasPrefix(branch, "dev.") || strings.HasPrefix(branch, "release-branch.") {
+ return "[" + branch + "] "
+ }
+ return ""
+}
+
+const (
+ syncBranchContinueFlag = " -continue"
+ syncBranchMergeBackFlag = " -merge-back-to-parent"
+)
+
func syncBranchContinue(flag string, b *Branch, status *syncBranchStatus) {
if h := gitHash("origin/" + status.Parent); h != status.ParentHash {
dief("cannot sync-branch%s: parent hash changed: %.7s -> %.7s", flag, status.ParentHash, h)
@@ -257,34 +327,43 @@
dief("cannot sync-branch%s: branch changed underfoot: %s -> %s", flag, status.Local, b.Name)
}
- branch := status.Branch
- parent := status.Parent
- branchHash := status.BranchHash
- parentHash := status.ParentHash
-
- prefix := ""
- if strings.HasPrefix(branch, "dev.") || strings.HasPrefix(branch, "release-branch.") {
- prefix = "[" + branch + "] "
+ var (
+ dst = status.Branch
+ dstHash = status.BranchHash
+ src = status.Parent
+ srcHash = status.ParentHash
+ )
+ if flag == syncBranchMergeBackFlag {
+ // This is a reverse merge: commits are flowing
+ // in the opposite direction from normal.
+ dst, src = src, dst
+ dstHash, srcHash = srcHash, dstHash
}
- msg := fmt.Sprintf("%sall: merge %s (%.7s) into %s", prefix, parent, parentHash, branch)
- if flag != "" {
+ prefix := prefixFor(dst)
+ op := "merge"
+ if flag == syncBranchMergeBackFlag {
+ op = "REVERSE MERGE"
+ }
+ msg := fmt.Sprintf("%sall: %s %s (%.7s) into %s", prefix, op, src, srcHash, dst)
+
+ if flag == syncBranchContinueFlag {
// Need to commit the merge.
// Check that the state of the client is the way we left it before any merge conflicts.
mergeHead, err := cmdOutputErr("git", "rev-parse", "MERGE_HEAD")
if err != nil {
dief("cannot sync-branch%s: no pending merge\n"+
- "If you accidentally ran 'git merge --continue',\n"+
+ "If you accidentally ran 'git merge --continue' or 'git commit',\n"+
"then use 'git reset --hard HEAD^' to undo.\n", flag)
}
mergeHead = trim(mergeHead)
- if mergeHead != parentHash {
- dief("cannot sync-branch%s: MERGE_HEAD is %.7s, but origin/%s is %.7s", flag, mergeHead, parent, parentHash)
+ if mergeHead != srcHash {
+ dief("cannot sync-branch%s: MERGE_HEAD is %.7s, but origin/%s is %.7s", flag, mergeHead, src, srcHash)
}
head := gitHash("HEAD")
- if head != branchHash {
- dief("cannot sync-branch%s: HEAD is %.7s, but origin/%s is %.7s", flag, head, branch, branchHash)
+ if head != dstHash {
+ dief("cannot sync-branch%s: HEAD is %.7s, but origin/%s is %.7s", flag, head, dst, dstHash)
}
if HasUnstagedChanges() {
@@ -301,15 +380,24 @@
// to use our standard format and list the incorporated CLs.
// Merge must never sync codereview.cfg,
- // because it contains the parent and branch config.
- // Force the on-branch copy back while amending the commit.
- cmdOutputDir(repoRoot(), "git", "checkout", "origin/"+branch, "--", "codereview.cfg")
+ // because it contains the src and dst config.
+ // Force the on-dst copy back while amending the commit.
+ cmdOutputDir(repoRoot(), "git", "checkout", "origin/"+dst, "--", "codereview.cfg")
conflictMsg := ""
if len(status.Conflicts) > 0 {
conflictMsg = "Conflicts:\n\n- " + strings.Join(status.Conflicts, "\n- ") + "\n\n"
}
- msg = fmt.Sprintf("%s\n\n%sMerge List:\n\n%s", msg, conflictMsg,
+
+ if flag == syncBranchMergeBackFlag {
+ msg += fmt.Sprintf("\n\n"+
+ "This commit is a REVERSE MERGE.\n"+
+ "It merges %s back into its parent branch, %s.\n"+
+ "This marks the end of development on %s.\n",
+ status.Branch, status.Parent, status.Branch)
+ }
+
+ msg += fmt.Sprintf("\n\n%sMerge List:\n\n%s", conflictMsg,
cmdOutput("git", "log", "--format=format:+ %cd %h %s", "--date=short", "HEAD^1..HEAD^2"))
run("git", "commit", "--amend", "-m", msg)
@@ -317,4 +405,6 @@
cmdPending([]string{"-c", "-l"})
fmt.Fprintf(stderr(), "\n* Merge commit created.\nRun 'git codereview mail' to send for review.\n")
+
+ os.Remove(syncBranchStatusFile())
}
diff --git a/git-codereview/sync_test.go b/git-codereview/sync_test.go
index d8733c8..0d3978d 100644
--- a/git-codereview/sync_test.go
+++ b/git-codereview/sync_test.go
@@ -5,6 +5,9 @@
package main
import (
+ "bytes"
+ "io/ioutil"
+ "path/filepath"
"strings"
"testing"
)
@@ -79,7 +82,7 @@
// submit first two CLs - gt.serverWork does same thing gt.work does, but on server
gt.serverWork(t)
- gt.serverWorkUnrelated(t) // wedge in unrelated work to get different hashes
+ gt.serverWorkUnrelated(t, "") // wedge in unrelated work to get different hashes
gt.serverWork(t)
testMain(t, "sync")
@@ -215,9 +218,9 @@
gt.serverWork(t)
gt.serverWork(t)
trun(t, gt.server, "git", "checkout", "dev.branch")
- gt.serverWorkUnrelated(t)
- gt.serverWorkUnrelated(t)
- gt.serverWorkUnrelated(t)
+ gt.serverWorkUnrelated(t, "")
+ gt.serverWorkUnrelated(t, "")
+ gt.serverWorkUnrelated(t, "")
trun(t, gt.server, "git", "checkout", "main")
testMain(t, "change", "dev.branch")
@@ -232,6 +235,90 @@
"Run 'git codereview mail' to send for review.")
}
+func TestSyncBranchMergeBack(t *testing.T) {
+ gt := newGitTest(t)
+ defer gt.done()
+
+ // server does not default to having a codereview.cfg on main,
+ // but sync-branch requires one.
+ var mainCfg = []byte("branch: main\n")
+ ioutil.WriteFile(filepath.Join(gt.server, "codereview.cfg"), mainCfg, 0666)
+ trun(t, gt.server, "git", "add", "codereview.cfg")
+ trun(t, gt.server, "git", "commit", "-m", "config for main", "codereview.cfg")
+
+ gt.serverWork(t)
+ gt.serverWork(t)
+ trun(t, gt.server, "git", "checkout", "dev.branch")
+ gt.serverWorkUnrelated(t, "work on dev.branch#1")
+ gt.serverWorkUnrelated(t, "work on dev.branch#2")
+ gt.serverWorkUnrelated(t, "work on dev.branch#3")
+ trun(t, gt.server, "git", "checkout", "main")
+ testMain(t, "change", "dev.branch")
+
+ // Merge back should fail because there are
+ // commits in the parent that have not been merged here yet.
+ testMainDied(t, "sync-branch", "--merge-back-to-parent")
+ testNoStdout(t)
+ testPrintedStderr(t, "parent has new commits")
+
+ // Bring them in, creating a merge commit.
+ testMain(t, "sync-branch")
+
+ // Merge back should still fail - merge commit not submitted yet.
+ testMainDied(t, "sync-branch", "--merge-back-to-parent")
+ testNoStdout(t)
+ testPrintedStderr(t, "pending changes exist")
+
+ // Push the changes back to the server.
+ // (In a real system this would happen through Gerrit.)
+ trun(t, gt.client, "git", "push", "origin")
+
+ devHash := trim(trun(t, gt.client, "git", "rev-parse", "HEAD"))
+
+ // Nothing should be pending.
+ testMain(t, "pending", "-c")
+ testPrintedStdout(t, "!+")
+
+ // This time should work.
+ testMain(t, "sync-branch", "--merge-back-to-parent")
+ testPrintedStdout(t,
+ "\n\tall: REVERSE MERGE dev.branch ("+devHash[:7]+") into main",
+ "This commit is a REVERSE MERGE.",
+ "It merges dev.branch back into its parent branch, main.",
+ "This marks the end of development on dev.branch.",
+ "Merge List:",
+ "msg #2",
+ "!config for main",
+ )
+ testPrintedStderr(t, "Merge commit created.")
+
+ data, err := ioutil.ReadFile(filepath.Join(gt.client, "codereview.cfg"))
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !bytes.Equal(data, mainCfg) {
+ t.Fatalf("codereview.cfg not restored:\n%s", data)
+ }
+
+ // Check pending knows what branch it is on.
+ testMain(t, "pending", "-c")
+ testHideRevHashes(t)
+ testPrintedStdout(t,
+ "dev.branch REVHASH..REVHASH (current branch)", // no "tracking dev.branch" anymore
+ "REVHASH (merge=REVHASH)",
+ "Merge List:",
+ "!config for main",
+ )
+
+ // Check that mail sends the merge to the right place!
+ testMain(t, "mail", "-n")
+ testNoStdout(t)
+ testPrintedStderr(t,
+ "git push -q origin HEAD:refs/for/main",
+ "git tag -f dev.branch.mailed",
+ )
+}
+
func TestSyncBranchConflict(t *testing.T) {
gt := newGitTest(t)
defer gt.done()
diff --git a/git-codereview/util_test.go b/git-codereview/util_test.go
index a4f0b51..775b244 100644
--- a/git-codereview/util_test.go
+++ b/git-codereview/util_test.go
@@ -69,7 +69,7 @@
}
// doWork simulates commit 'n' touching 'file' in 'dir'
-func doWork(t *testing.T, n int, dir, file, changeid string) {
+func doWork(t *testing.T, n int, dir, file, changeid string, msg string) {
t.Helper()
write(t, dir+"/"+file, fmt.Sprintf("new content %d", n), 0644)
trun(t, dir, "git", "add", file)
@@ -77,8 +77,11 @@
if n > 1 {
suffix = fmt.Sprintf(" #%d", n)
}
- msg := fmt.Sprintf("msg%s\n\nChange-Id: I%d%s\n", suffix, n, changeid)
- trun(t, dir, "git", "commit", "-m", msg)
+ if msg != "" {
+ msg += "\n\n"
+ }
+ cmsg := fmt.Sprintf("%smsg%s\n\nChange-Id: I%d%s\n", msg, suffix, n, changeid)
+ trun(t, dir, "git", "commit", "-m", cmsg)
}
func (gt *gitTest) work(t *testing.T) {
@@ -91,14 +94,14 @@
// make local change on client
gt.nwork++
- doWork(t, gt.nwork, gt.client, "file", "23456789")
+ doWork(t, gt.nwork, gt.client, "file", "23456789", "")
}
func (gt *gitTest) workFile(t *testing.T, file string) {
t.Helper()
// make local change on client in the specific file
gt.nwork++
- doWork(t, gt.nwork, gt.client, file, "23456789")
+ doWork(t, gt.nwork, gt.client, file, "23456789", "")
}
func (gt *gitTest) serverWork(t *testing.T) {
@@ -108,15 +111,15 @@
// having gone through Gerrit and submitted with possibly
// different commit hashes but the same content.
gt.nworkServer++
- doWork(t, gt.nworkServer, gt.server, "file", "23456789")
+ doWork(t, gt.nworkServer, gt.server, "file", "23456789", "")
}
-func (gt *gitTest) serverWorkUnrelated(t *testing.T) {
+func (gt *gitTest) serverWorkUnrelated(t *testing.T, msg string) {
t.Helper()
// make unrelated change on server
// this makes history different on client and server
gt.nworkOther++
- doWork(t, gt.nworkOther, gt.server, "otherfile", "9999")
+ doWork(t, gt.nworkOther, gt.server, "otherfile", "9999", msg)
}
func newGitTest(t *testing.T) (gt *gitTest) {
