git-codereview: clean up detached HEAD mode

Long ago I decided to return origin/HEAD from b.OriginBranch
in detached HEAD mode, thinking it would cause obvious failures.
But the joke was on me - origin/HEAD is a real thing in git,
and HEAD tracking origin/HEAD is not the right answer on dev branches.

Now that each branch's codereview.cfg typically has the branch info
we need, we can use that in detached HEAD mode to be able to provide
useful displays in commands like "git pending". And we can be careful
not to do that when we don't know the actual branch.

This commit cleans all that up.

Change-Id: I0e59bcb6f9b61e0cdce7a27299b7f29fef8e7048
Reviewed-on: https://go-review.googlesource.com/c/review/+/282616
Trust: Russ Cox <rsc@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

6 files changed, 96 insertions, 42 deletions

diff --git a/git-codereview/branch.go b/git-codereview/branch.go
index 5d277c8..d25c4c9 100644
--- a/git-codereview/branch.go
+++ b/git-codereview/branch.go
@@ -98,18 +98,26 @@ func (b *Branch) DetachedHead() bool {
 	return b.Name == "HEAD"
 }
 
+// NeedOriginBranch exits with an error message
+// if the origin branch is unknown.
+// The cmd is the user command reported in the failure message.
+func (b *Branch) NeedOriginBranch(cmd string) {
+	if b.OriginBranch() == "" {
+		why := ""
+		if b.DetachedHead() {
+			why = " (in detached HEAD mode)"
+		}
+		if cmd == "<internal branchpoint>" && exitTrap != nil {
+			panic("NeedOriginBranch")
+		}
+		dief("cannot %s: no origin branch%s", cmd, why)
+	}
+}
+
 // OriginBranch returns the name of the origin branch that branch b tracks.
 // The returned name is like "origin/master" or "origin/dev.garbage" or
 // "origin/release-branch.go1.4".
 func (b *Branch) OriginBranch() string {
-	if b.DetachedHead() {
-		// Detached head mode.
-		// "origin/HEAD" is clearly false, but it should be easy to find when it
-		// appears in other commands. Really any caller of OriginBranch
-		// should check for detached head mode.
-		return "origin/HEAD"
-	}
-
 	if b.originBranch != "" {
 		return b.originBranch
 	}
@@ -119,28 +127,33 @@ func (b *Branch) OriginBranch() string {
 	if cfg != "" {
 		upstream = "origin/" + cfg
 	}
-	gitUpstream := b.gitOriginBranch()
-	if upstream == "" {
-		upstream = gitUpstream
-	}
-	if upstream == "" {
-		// Assume branch was created before we set upstream correctly.
-		// See if origin/main exists; if so, use it.
-		// Otherwise, fall back to origin/master.
-		argv := []string{"git", "rev-parse", "--abbrev-ref", "origin/main"}
-		cmd := exec.Command(argv[0], argv[1:]...)
-		setEnglishLocale(cmd)
-		if err := cmd.Run(); err == nil {
-			upstream = "origin/main"
-		} else {
-			upstream = "origin/master"
-		}
-	}
 
-	if gitUpstream != upstream && b.Current {
-		// Best effort attempt to correct setting for next time,
-		// and for "git status".
-		exec.Command("git", "branch", "-u", upstream).Run()
+	// Consult and possibly update git,
+	// but only if we are actually on a real branch,
+	// not in detached HEAD mode.
+	if !b.DetachedHead() {
+		gitUpstream := b.gitOriginBranch()
+		if upstream == "" {
+			upstream = gitUpstream
+		}
+		if upstream == "" {
+			// Assume branch was created before we set upstream correctly.
+			// See if origin/main exists; if so, use it.
+			// Otherwise, fall back to origin/master.
+			argv := []string{"git", "rev-parse", "--abbrev-ref", "origin/main"}
+			cmd := exec.Command(argv[0], argv[1:]...)
+			setEnglishLocale(cmd)
+			if err := cmd.Run(); err == nil {
+				upstream = "origin/main"
+			} else {
+				upstream = "origin/master"
+			}
+		}
+		if gitUpstream != upstream && b.Current {
+			// Best effort attempt to correct setting for next time,
+			// and for "git status".
+			exec.Command("git", "branch", "-u", upstream).Run()
+		}
 	}
 
 	b.originBranch = upstream
@@ -197,6 +210,7 @@ func (b *Branch) Pending() []*Commit {
 // Branchpoint returns an identifier for the latest revision
 // common to both this branch and its upstream branch.
 func (b *Branch) Branchpoint() string {
+	b.NeedOriginBranch("<internal branchpoint>")
 	b.loadPending()
 	return b.branchpoint
 }
@@ -515,8 +529,10 @@ func ListFiles(c *Commit) []string {
 }
 
 func cmdBranchpoint(args []string) {
-	expectZeroArgs(args, "sync")
-	fmt.Fprintf(stdout(), "%s\n", CurrentBranch().Branchpoint())
+	expectZeroArgs(args, "branchpoint")
+	b := CurrentBranch()
+	b.NeedOriginBranch("branchpoint")
+	fmt.Fprintf(stdout(), "%s\n", b.Branchpoint())
 }
 
 func cmdRebaseWork(args []string) {
diff --git a/git-codereview/branch_test.go b/git-codereview/branch_test.go
index 9e999b7..d7db6ee 100644
--- a/git-codereview/branch_test.go
+++ b/git-codereview/branch_test.go
@@ -40,11 +40,14 @@ func TestCurrentBranch(t *testing.T) {
 	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, "", "")
+	trun(t, gt.client, "git", "checkout", "main^0")
+	checkCurrentBranch(t, "HEAD", "", false, "", "")
+	trun(t, gt.client, "git", "checkout", "dev.branch^0")
+	checkCurrentBranch(t, "HEAD", "origin/dev.branch", false, "", "")
 }
 
 func checkCurrentBranch(t *testing.T, name, origin string, hasPending bool, changeID, subject string) {
+	t.Helper()
 	b := CurrentBranch()
 	if b.Name != name {
 		t.Errorf("b.Name = %q, want %q", b.Name, name)
diff --git a/git-codereview/gofmt.go b/git-codereview/gofmt.go
index 7986d07..ca5f798 100644
--- a/git-codereview/gofmt.go
+++ b/git-codereview/gofmt.go
@@ -120,13 +120,15 @@ func runGofmt(flags int) (files []string, stderrText string) {
 	}
 
 	// Find files modified in the index compared to the branchpoint.
-	branchpt := b.Branchpoint()
-	if strings.Contains(cmdOutput("git", "branch", "-r", "--contains", b.FullName()), "origin/") {
-		// This is a branch tag move, not an actual change.
-		// Use HEAD as branch point, so nothing will appear changed.
-		// We don't want to think about gofmt on already published
-		// commits.
-		branchpt = "HEAD"
+	// The default of HEAD will only compare against the most recent commit.
+	// But if we know the origin branch, and this isn't a branch tag move,
+	// then check all the pending commits.
+	branchpt := "HEAD"
+	if b.OriginBranch() != "" {
+		isBranchTagMove := strings.Contains(cmdOutput("git", "branch", "-r", "--contains", b.FullName()), "origin/")
+		if !isBranchTagMove {
+			branchpt = b.Branchpoint()
+		}
 	}
 	indexFiles := addRoot(repo, filter(gofmtRequired, nonBlankLines(cmdOutput("git", "diff", "--name-only", "--diff-filter=ACM", "--cached", branchpt, "--"))))
 	localFiles := addRoot(repo, filter(gofmtRequired, nonBlankLines(cmdOutput("git", "diff", "--name-only", "--diff-filter=ACM"))))
diff --git a/git-codereview/pending.go b/git-codereview/pending.go
index 0f61208..3ed50ec 100644
--- a/git-codereview/pending.go
+++ b/git-codereview/pending.go
@@ -234,7 +234,9 @@ func cmdPending(args []string) {
 			fmt.Fprintf(&buf, " %.7s..%s", b.branchpoint, work[0].ShortHash)
 		}
 		var tags []string
-		if b.current {
+		if b.DetachedHead() {
+			tags = append(tags, "detached")
+		} else if b.current {
 			tags = append(tags, "current branch")
 		}
 		if allMailed(work) && len(work) > 0 {
@@ -246,7 +248,9 @@ func cmdPending(args []string) {
 		if n := b.CommitsBehind(); n > 0 {
 			tags = append(tags, fmt.Sprintf("%d behind", n))
 		}
-		if br := b.OriginBranch(); br != "origin/master" && br != "origin/main" {
+		if br := b.OriginBranch(); br == "" {
+			tags = append(tags, "remote branch unknown")
+		} else if br != "origin/master" && br != "origin/main" {
 			tags = append(tags, "tracking "+strings.TrimPrefix(b.OriginBranch(), "origin/"))
 		}
 		if len(tags) > 0 {
diff --git a/git-codereview/sync.go b/git-codereview/sync.go
index 0334341..c38215c 100644
--- a/git-codereview/sync.go
+++ b/git-codereview/sync.go
@@ -19,6 +19,7 @@ func cmdSync(args []string) {
 
 	// Get current branch and commit ID for fixup after pull.
 	b := CurrentBranch()
+	b.NeedOriginBranch("sync")
 	var id string
 	if work := b.Pending(); len(work) > 0 {
 		id = work[0].ChangeID
diff --git a/git-codereview/sync_test.go b/git-codereview/sync_test.go
index 1498098..d8733c8 100644
--- a/git-codereview/sync_test.go
+++ b/git-codereview/sync_test.go
@@ -180,6 +180,34 @@ func TestBranchConfig(t *testing.T) {
 		"work REVHASH..REVHASH\n") // the \n checks for not having a (tracking main)
 }
 
+func TestDetachedHead(t *testing.T) {
+	gt := newGitTest(t)
+	defer gt.done()
+	gt.work(t) // do the main-branch work setup now to avoid unwanted change below
+
+	trun(t, gt.client, "git", "checkout", "HEAD^0") // enter detached HEAD mode with one pending commit
+
+	// Pending should succeed and just print very little.
+	testMain(t, "pending", "-c", "-l")
+	testPrintedStdout(t, "HEAD (detached, remote branch unknown)", "!+")
+	testNoStderr(t)
+
+	// Sync, branchpoint should fail - branch unknown
+	// (there is no "upstream" coming from git, and there's no branch line
+	// in codereview.cfg on main in the test setup).
+	for _, cmd := range []string{"sync", "branchpoint"} {
+		testMainDied(t, cmd)
+		testNoStdout(t)
+		testPrintedStderr(t, "cannot "+cmd+": no origin branch (in detached HEAD mode)")
+	}
+
+	// If we switch to dev.branch, which does have a branch line,
+	// detached HEAD mode should be able to find the branchpoint.
+	trun(t, gt.client, "git", "checkout", "dev.branch")
+	gt.work(t)
+	trun(t, gt.client, "git", "checkout", "HEAD^0")
+}
+
 func TestSyncBranch(t *testing.T) {
 	gt := newGitTest(t)
 	defer gt.done()