git-codereview: allow work on main branches
The only reason not to allow work on branches named for the
origin branches is to preserve them for "git change main; git change new"
to make a new branch tracking main. But we can still do that and
allow commits on main - we just have to use the branchpoint
as the root of the new branch.
Now people can work on "main" (or "dev.regabi") if that suits them.
In particular, if you're doing merges, it's nice to be on "dev.regabi"
and know for sure that's the branch you're working on.
Change-Id: I8e9458793c30857a5c00e6bfd4f1cb41adbbe637
Reviewed-on: https://go-review.googlesource.com/c/review/+/279874
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Austin Clements <austin@google.com>
diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 2bd9fe9..7c9b282 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -116,11 +116,6 @@
return b.Name
}
-// IsLocalOnly reports whether b is a local work branch (only local, not known to remote server).
-func (b *Branch) IsLocalOnly() bool {
- return "origin/"+b.Name != b.OriginBranch()
-}
-
// HasPendingCommit reports whether b has any pending commits.
func (b *Branch) HasPendingCommit() bool {
b.loadPending()
diff --git a/git-codereview/branch_test.go b/git-codereview/branch_test.go
index 438ff39..e4530eb 100644
--- a/git-codereview/branch_test.go
+++ b/git-codereview/branch_test.go
@@ -15,36 +15,36 @@
defer gt.done()
t.Logf("on main")
- checkCurrentBranch(t, "main", "origin/main", false, false, "", "")
+ checkCurrentBranch(t, "main", "origin/main", false, "", "")
t.Logf("on newbranch")
trun(t, gt.client, "git", "checkout", "--no-track", "-b", "newbranch")
- checkCurrentBranch(t, "newbranch", "origin/main", true, false, "", "")
+ checkCurrentBranch(t, "newbranch", "origin/main", false, "", "")
t.Logf("making change")
write(t, gt.client+"/file", "i made a change", 0644)
trun(t, gt.client, "git", "commit", "-a", "-m", "My change line.\n\nChange-Id: I0123456789abcdef0123456789abcdef\n")
- checkCurrentBranch(t, "newbranch", "origin/main", true, true, "I0123456789abcdef0123456789abcdef", "My change line.")
+ checkCurrentBranch(t, "newbranch", "origin/main", true, "I0123456789abcdef0123456789abcdef", "My change line.")
t.Logf("on dev.branch")
trun(t, gt.client, "git", "checkout", "-t", "-b", "dev.branch", "origin/dev.branch")
- checkCurrentBranch(t, "dev.branch", "origin/dev.branch", false, false, "", "")
+ checkCurrentBranch(t, "dev.branch", "origin/dev.branch", false, "", "")
t.Logf("on newdev")
trun(t, gt.client, "git", "checkout", "-t", "-b", "newdev", "origin/dev.branch")
- checkCurrentBranch(t, "newdev", "origin/dev.branch", true, false, "", "")
+ checkCurrentBranch(t, "newdev", "origin/dev.branch", false, "", "")
t.Logf("making change")
write(t, gt.client+"/file", "i made another change", 0644)
trun(t, gt.client, "git", "commit", "-a", "-m", "My other change line.\n\nChange-Id: I1123456789abcdef0123456789abcdef\n")
- checkCurrentBranch(t, "newdev", "origin/dev.branch", true, true, "I1123456789abcdef0123456789abcdef", "My other change line.")
+ checkCurrentBranch(t, "newdev", "origin/dev.branch", true, "I1123456789abcdef0123456789abcdef", "My other change line.")
t.Logf("detached head mode")
trun(t, gt.client, "git", "checkout", "HEAD^0")
- checkCurrentBranch(t, "HEAD", "origin/HEAD", false, false, "", "")
+ checkCurrentBranch(t, "HEAD", "origin/HEAD", false, "", "")
}
-func checkCurrentBranch(t *testing.T, name, origin string, isLocal, hasPending bool, changeID, subject string) {
+func checkCurrentBranch(t *testing.T, name, origin string, hasPending bool, changeID, subject string) {
b := CurrentBranch()
if b.Name != name {
t.Errorf("b.Name = %q, want %q", b.Name, name)
@@ -52,11 +52,8 @@
if x := b.OriginBranch(); x != origin {
t.Errorf("b.OriginBranch() = %q, want %q", x, origin)
}
- if x := b.IsLocalOnly(); x != isLocal {
- t.Errorf("b.IsLocalOnly() = %v, want %v", x, isLocal)
- }
if x := b.HasPendingCommit(); x != hasPending {
- t.Errorf("b.HasPendingCommit() = %v, want %v", x, isLocal)
+ t.Errorf("b.HasPendingCommit() = %v, want %v", x, hasPending)
}
if work := b.Pending(); len(work) > 0 {
c := work[0]
diff --git a/git-codereview/change.go b/git-codereview/change.go
index 991707d..6f76df1 100644
--- a/git-codereview/change.go
+++ b/git-codereview/change.go
@@ -31,7 +31,7 @@
if target != "" {
checkoutOrCreate(target)
b := CurrentBranch()
- if HasStagedChanges() && b.IsLocalOnly() && !b.HasPendingCommit() {
+ if HasStagedChanges() && !b.HasPendingCommit() {
commitChanges(false)
}
b.check()
@@ -40,10 +40,6 @@
// Create or amend change commit.
b := CurrentBranch()
- if !b.IsLocalOnly() {
- dief("can't commit to %s branch (use '%s change branchname').", b.Name, progName)
- }
-
amend := b.HasPendingCommit()
if amend {
// Dies if there is not exactly one commit.
@@ -65,11 +61,6 @@
printf("warning: %d commit%s behind %s; run 'git codereview sync' to update.", n, suffix(n, "s"), b.OriginBranch())
}
}
-
- // TODO(rsc): Test
- if text := b.errors(); text != "" {
- printf("error: %s\n", text)
- }
}
var testCommitMsg string
@@ -158,19 +149,28 @@
// If the current branch has a pending commit, building
// on top of it will not help. Don't allow that.
- // Otherwise, inherit HEAD and upstream from the current branch.
+ // Otherwise, inherit branchpoint and upstream from the current branch.
b := CurrentBranch()
+ branchpoint := "HEAD"
if b.HasPendingCommit() {
- if !b.IsLocalOnly() {
- dief("bad repo state: branch %s is ahead of origin/%s", b.Name, b.Name)
- }
- dief("cannot branch from work branch; change back to %v first.", strings.TrimPrefix(b.OriginBranch(), "origin/"))
+ fmt.Fprintf(stderr(), "warning: pending changes on %s are not copied to new branch %s\n", b.Name, target)
+ branchpoint = b.Branchpoint()
}
origin := b.OriginBranch()
- // NOTE: This is different from git checkout -q -t -b branch. It does not move HEAD.
- run("git", "checkout", "-q", "-b", target)
+ // NOTE: This is different from git checkout -q -t -b origin,
+ // because the -t wold use the origin directly, and that may be
+ // ahead of the current directory. The goal of this command is
+ // to create a new branch for work on the current directory,
+ // not to incorporate new commits at the same time (use 'git sync' for that).
+ // The ideal is that HEAD doesn't change at all.
+ // In the absence of pending commits, that ideal is achieved.
+ // But if there are pending commits, it'd be too confusing to have them
+ // on two different work branches, so we drop them and use the
+ // branchpoint they started from (after warning above), moving HEAD
+ // as little as possible.
+ run("git", "checkout", "-q", "-b", target, branchpoint)
run("git", "branch", "-q", "--set-upstream-to", origin)
printf("created branch %v tracking %s.", target, origin)
}
diff --git a/git-codereview/change_test.go b/git-codereview/change_test.go
index 8a3960d..d3b37b0 100644
--- a/git-codereview/change_test.go
+++ b/git-codereview/change_test.go
@@ -17,11 +17,12 @@
t.Logf("main -> main")
testMain(t, "change", "main")
testRan(t, "git checkout -q main")
+ branchpoint := strings.TrimSpace(trun(t, gt.client, "git", "rev-parse", "HEAD"))
testCommitMsg = "foo: my commit msg"
t.Logf("main -> work")
testMain(t, "change", "work")
- testRan(t, "git checkout -q -b work",
+ testRan(t, "git checkout -q -b work HEAD",
"git branch -q --set-upstream-to origin/main")
t.Logf("work -> main")
@@ -35,7 +36,12 @@
testRan(t, "git checkout -q work",
"git commit -q --allow-empty -m foo: my commit msg")
- t.Logf("main -> dev.branch")
+ t.Logf("work -> work2")
+ testMain(t, "change", "work2")
+ testRan(t, "git checkout -q -b work2 "+branchpoint,
+ "git branch -q --set-upstream-to origin/main")
+
+ t.Logf("work2 -> dev.branch")
testMain(t, "change", "dev.branch")
testRan(t, "git checkout -q -t -b dev.branch origin/dev.branch")
@@ -62,19 +68,6 @@
testPrintedStderr(t, "invalid branch name \"HeAd\": ref name HEAD is reserved for git")
}
-func TestChangeAhead(t *testing.T) {
- gt := newGitTest(t)
- defer gt.done()
-
- // commit to main (mistake)
- write(t, gt.client+"/file", "new content", 0644)
- trun(t, gt.client, "git", "add", "file")
- trun(t, gt.client, "git", "commit", "-m", "msg")
-
- testMainDied(t, "change", "work")
- testPrintedStderr(t, "bad repo state: branch main is ahead of origin/main")
-}
-
func TestMessageRE(t *testing.T) {
for _, c := range []struct {
in string
diff --git a/git-codereview/hook.go b/git-codereview/hook.go
index 09bed8b..1913666 100644
--- a/git-codereview/hook.go
+++ b/git-codereview/hook.go
@@ -279,14 +279,6 @@
}
*/
- // Prevent commits to master branches, but only if we're here for code review.
- if haveGerrit() {
- b := CurrentBranch()
- if !b.IsLocalOnly() && b.Name != "HEAD" {
- dief("cannot commit on %s branch", b.Name)
- }
- }
-
hookGofmt()
}
diff --git a/git-codereview/mail_test.go b/git-codereview/mail_test.go
index b754d3b..93b89f4 100644
--- a/git-codereview/mail_test.go
+++ b/git-codereview/mail_test.go
@@ -298,7 +298,7 @@
}()
testMain(t, "change", "work")
- testRan(t, "git checkout -q -b work",
+ testRan(t, "git checkout -q -b work HEAD",
"git branch -q --set-upstream-to origin/main")
t.Logf("creating empty change")
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 5808764..c1594b5 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -254,13 +254,6 @@
}
fmt.Fprintf(&buf, "\n")
printed := false
- if text := b.errors(); text != "" {
- fmt.Fprintf(&buf, "\tERROR: %s\n", strings.Replace(strings.TrimSpace(text), "\n", "\n\t", -1))
- if !pendingShort {
- printed = true
- fmt.Fprintf(&buf, "\n")
- }
- }
if b.current && len(b.staged)+len(b.unstaged)+len(b.untracked) > 0 {
printed = true
@@ -419,18 +412,6 @@
return true
}
-// errors returns any errors that should be displayed
-// about the state of the current branch, diagnosing common mistakes.
-func (b *Branch) errors() string {
- b.loadPending()
- var buf bytes.Buffer
- if haveGerrit() && !b.IsLocalOnly() && b.commitsAhead > 0 {
- fmt.Fprintf(&buf, "Branch contains %d commit%s not on origin/%s.\n", b.commitsAhead, suffix(b.commitsAhead, "s"), b.Name)
- fmt.Fprintf(&buf, "\tDo not commit directly to %s branch.\n", b.Name)
- }
- return buf.String()
-}
-
// suffix returns an empty string if n == 1, s otherwise.
func suffix(n int, s string) string {
if n == 1 {
diff --git a/git-codereview/pending_test.go b/git-codereview/pending_test.go
index 1124e4d..6899213 100644
--- a/git-codereview/pending_test.go
+++ b/git-codereview/pending_test.go
@@ -48,9 +48,9 @@
work REVHASH..REVHASH (current branch)
+ REVHASH
msg
-
+
Change-Id: I123456789
-
+
Files in this change:
file
@@ -109,9 +109,9 @@
work REVHASH..REVHASH (3 behind)
+ REVHASH
msg
-
+
Change-Id: I123456789
-
+
Files in this change:
file
@@ -131,11 +131,11 @@
+ REVHASH
some changes
-
+
Files in this change:
file
file1
-
+
`)
testPendingArgs(t, []string{"-c", "-s"}, `
@@ -150,37 +150,6 @@
afile1
file1
+ REVHASH some changes
-
- `)
-}
-
-func TestPendingErrors(t *testing.T) {
- gt := newGitTest(t)
- gt.enableGerrit(t)
- defer gt.done()
-
- trun(t, gt.client, "git", "checkout", "main")
- write(t, gt.client+"/file", "v3", 0644)
- trun(t, gt.client, "git", "commit", "-a", "-m", "v3")
-
- testPending(t, `
- main REVHASH..REVHASH (current branch)
- ERROR: Branch contains 1 commit not on origin/main.
- Do not commit directly to main branch.
-
- + REVHASH
- v3
-
- Files in this change:
- file
-
- `)
-
- testPendingArgs(t, []string{"-s"}, `
- main REVHASH..REVHASH (current branch)
- ERROR: Branch contains 1 commit not on origin/main.
- Do not commit directly to main branch.
- + REVHASH v3
`)
}
@@ -208,18 +177,18 @@
file
Files staged:
file
-
+
+ REVHASH
v2
-
+
Files in this change:
file
+ REVHASH
msg
-
+
Change-Id: I123456789
-
+
Files in this change:
file
@@ -253,9 +222,9 @@
work REVHASH..REVHASH (current branch)
+ REVHASH
msg
-
+
Change-Id: I123456789
-
+
Files in this change:
file
@@ -273,7 +242,7 @@
work REVHASH..REVHASH (current branch)
+ REVHASH
msg
-
+
Change-Id: I123456789
Files in this change:
@@ -292,9 +261,9 @@
work REVHASH..REVHASH (current branch, all mailed, all submitted, 1 behind)
+ REVHASH http://127.0.0.1:PORT/1234 (mailed, submitted)
msg
-
+
Change-Id: I123456789
-
+
Code-Review:
+1 Grace Emlin
-2 George Opher
@@ -316,7 +285,7 @@
work REVHASH..REVHASH (current branch, 1 behind)
+ REVHASH
msg
-
+
Change-Id: I123456789
Files in this change:
@@ -363,10 +332,10 @@
file
Files staged:
file
-
+
+ REVHASH http://127.0.0.1:PORT/1234 (mailed)
v2
-
+
Change-Id: I2345
Code-Review:
@@ -379,9 +348,9 @@
+ REVHASH http://127.0.0.1:PORT/1234 (mailed, submitted)
msg
-
+
Change-Id: I123456789
-
+
Code-Review:
+1 Grace Emlin
-2 George Opher
@@ -525,6 +494,7 @@
out = regexp.MustCompile(`\b[0-9a-f]{7}\b`).ReplaceAllLiteral(out, []byte("REVHASH"))
out = regexp.MustCompile(`\b127\.0\.0\.1:\d+\b`).ReplaceAllLiteral(out, []byte("127.0.0.1:PORT"))
+ out = regexp.MustCompile(`(?m)[ \t]+$`).ReplaceAllLiteral(out, nil) // ignore trailing space differences
if string(out) != want {
t.Errorf("invalid pending output:\n%s\nwant:\n%s", out, want)
diff --git a/git-codereview/sync.go b/git-codereview/sync.go
index f518a3f..9ea3e67 100644
--- a/git-codereview/sync.go
+++ b/git-codereview/sync.go
@@ -16,6 +16,26 @@
id = work[0].ChangeID
}
+ // If this is a Gerrit repo, disable the status advice that
+ // tells users to run 'git push' and so on, like the marked (<<<) lines:
+ //
+ // % git status
+ // On branch master
+ // Your branch is ahead of 'origin/master' by 3 commits. <<<
+ // (use "git push" to publish your local commits) <<<
+ // ...
+ //
+ // (This advice is inappropriate when using Gerrit.)
+ if len(b.Pending()) > 0 && haveGerrit() {
+ // Only disable if statusHints is unset in the local config.
+ // This allows users who really want them to put them back
+ // in the .git/config for the Gerrit-cloned repo.
+ _, err := cmdOutputErr("git", "config", "--local", "advice.statusHints")
+ if err != nil {
+ run("git", "config", "--local", "advice.statusHints", "false")
+ }
+ }
+
// Don't sync with staged or unstaged changes.
// rebase is going to complain if we don't, and we can give a nicer error.
checkStaged("sync")
