From 5617b8658c4ff7af5665cf0fd4bc5448d4ada7b9 Mon Sep 17 00:00:00 2001 From: Russ Cox Date: Thu, 7 Jan 2021 12:26:06 -0500 Subject: 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 Reviewed-by: Matthew Dempsky Reviewed-by: Austin Clements --- git-codereview/branch.go | 5 --- git-codereview/branch_test.go | 21 ++++++------ git-codereview/change.go | 34 ++++++++++---------- git-codereview/change_test.go | 23 +++++--------- git-codereview/hook.go | 8 ----- git-codereview/mail_test.go | 2 +- git-codereview/pending.go | 19 ----------- git-codereview/pending_test.go | 72 ++++++++++++------------------------------ git-codereview/sync.go | 20 ++++++++++++ 9 files changed, 76 insertions(+), 128 deletions(-) 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 @@ func (b *Branch) FullName() string { 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 @@ func TestCurrentBranch(t *testing.T) { 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 @@ func checkCurrentBranch(t *testing.T, name, origin string, isLocal, hasPending b 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 @@ func cmdChange(args []string) { if target != "" { checkoutOrCreate(target) b := CurrentBranch() - if HasStagedChanges() && b.IsLocalOnly() && !b.HasPendingCommit() { + if HasStagedChanges() && !b.HasPendingCommit() { commitChanges(false) } b.check() @@ -40,10 +40,6 @@ func cmdChange(args []string) { // 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 @@ func (b *Branch) check() { 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 @@ func checkoutOrCreate(target string) { // 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 @@ func TestChange(t *testing.T) { 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 @@ func TestChange(t *testing.T) { 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 @@ func TestChangeHEAD(t *testing.T) { 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 @@ func hookPreCommit(args []string) { } */ - // 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 @@ func TestMailEmpty(t *testing.T) { }() 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 @@ func cmdPending(args []string) { } 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 @@ func allSubmitted(work []*Commit) bool { 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 @@ func TestPendingBasic(t *testing.T) { work REVHASH..REVHASH (current branch) + REVHASH msg - + Change-Id: I123456789 - + Files in this change: file @@ -109,9 +109,9 @@ func TestPendingComplex(t *testing.T) { work REVHASH..REVHASH (3 behind) + REVHASH msg - + Change-Id: I123456789 - + Files in this change: file @@ -131,11 +131,11 @@ func TestPendingComplex(t *testing.T) { + REVHASH some changes - + Files in this change: file file1 - + `) testPendingArgs(t, []string{"-c", "-s"}, ` @@ -150,37 +150,6 @@ func TestPendingComplex(t *testing.T) { 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 @@ func TestPendingMultiChange(t *testing.T) { 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 @@ func TestPendingGerrit(t *testing.T) { work REVHASH..REVHASH (current branch) + REVHASH msg - + Change-Id: I123456789 - + Files in this change: file @@ -273,7 +242,7 @@ func TestPendingGerrit(t *testing.T) { work REVHASH..REVHASH (current branch) + REVHASH msg - + Change-Id: I123456789 Files in this change: @@ -292,9 +261,9 @@ func TestPendingGerrit(t *testing.T) { 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 @@ func TestPendingGerrit(t *testing.T) { work REVHASH..REVHASH (current branch, 1 behind) + REVHASH msg - + Change-Id: I123456789 Files in this change: @@ -363,10 +332,10 @@ func TestPendingGerritMultiChange(t *testing.T) { file Files staged: file - + + REVHASH http://127.0.0.1:PORT/1234 (mailed) v2 - + Change-Id: I2345 Code-Review: @@ -379,9 +348,9 @@ func TestPendingGerritMultiChange(t *testing.T) { + 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 @@ func testPendingArgs(t *testing.T, args []string, want string) { 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 @@ func cmdSync(args []string) { 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") -- cgit v1.3