Fully-qualify branch refspecs when pushing

gh-stack builds `git push` arguments from stack branch names in `internal/git`.
The force path passed `<branch>:refs/heads/<branch>`, and the non-force path
passed bare branch names. A git refspec treats a leading `+` as "force update",
and Git allows branch names that begin with `+`, so a branch named `+feature`
was parsed as refspec syntax for `feature`: the force path pushed local
`feature` into remote `+feature`, and the non-force path force-updated remote
`feature`.

Build fully-qualified refspecs for both the source and destination of every
push: `refs/heads/<branch>:refs/heads/<branch>`. A branch name can no longer be
reinterpreted as a refspec modifier. Force updates are still requested via the
existing `--force-with-lease` flags, whose ref names were already
fully-qualified. `DeleteRemoteBranch` is fully-qualified the same way.

The `Push` signature and every call site are unchanged. Add real-git
integration tests covering the force and non-force paths with a `+`-prefixed
branch.

Author: skarim

internal/git/gitops.go

@@ -188,12 +188,19 @@ func (d *defaultOps) Push(remote string, branches []string, force, atomic bool)
 		args = append(args, "--atomic")
 	}
 	if force {
-		// Explicit refspecs: <local-branch>:refs/heads/<remote-branch>.
+		// Fully-qualified refspecs: refs/heads/<local>:refs/heads/<remote>.
+		// Qualifying the source (not a bare branch name) ensures a branch
+		// name is never reinterpreted as refspec syntax — e.g. a leading
+		// "+" is part of the ref, not a force modifier. Force is supplied
+		// out-of-band by the --force-with-lease flags above.
 		for _, b := range branches {
-			args = append(args, fmt.Sprintf("%s:refs/heads/%s", b, b))
+			args = append(args, fmt.Sprintf("refs/heads/%s:refs/heads/%s", b, b))
 		}
 	} else {
-		args = append(args, branches...)
+		// Fully-qualified refspecs here too, for the same reason.
+		for _, b := range branches {
+			args = append(args, fmt.Sprintf("refs/heads/%s:refs/heads/%s", b, b))
+		}
 	}
 	return runSilent(args...)
 }
@@ -544,7 +551,9 @@ func (d *defaultOps) DeleteBranch(name string, force bool) error {
 }
 
 func (d *defaultOps) DeleteRemoteBranch(remote, branch string) error {
-	return runSilent("push", remote, "--delete", branch)
+	// Fully-qualify the ref so a branch name is never reinterpreted as
+	// refspec syntax.
+	return runSilent("push", remote, "--delete", "refs/heads/"+branch)
 }
 
 func (d *defaultOps) DeleteTrackingRef(remote, branch string) error {

internal/git/gitops_test.go

@@ -361,6 +361,100 @@ func TestIntegration_Push_MixedStack(t *testing.T) {
 	assert.Equal(t, localB2, remoteBranchSHA(t, bareDir, "b2"))
 }
 
+// A stack branch whose name begins with "+" must push its own ref, not a
+// similarly named sibling. A leading "+" in a git refspec means "force update",
+// so passing a bare "+feature" (or "+feature:...") lets git treat it as a
+// refspec modifier for "feature". Fully-qualified refspecs keep the "+" part of
+// the branch name. Force path (used by push/submit).
+func TestIntegration_Push_PlusPrefixedBranch_Force(t *testing.T) {
+	bareDir, cloneDir := setupBareAndClone(t)
+	restore := withGitDir(t, cloneDir)
+	defer restore()
+
+	d := &defaultOps{}
+
+	// Create and push a normal "feature" branch (content A).
+	gitExec(t, cloneDir, "checkout", "-b", "feature")
+	writeFile(t, cloneDir, "feature.txt", "A")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "feature A")
+	gitExec(t, cloneDir, "push", "origin", "feature")
+	remoteFeatureBefore := remoteBranchSHA(t, bareDir, "feature")
+
+	// Create a local "+feature" branch off main with different content (B).
+	gitExec(t, cloneDir, "checkout", "main")
+	gitExec(t, cloneDir, "checkout", "-b", "+feature")
+	writeFile(t, cloneDir, "plus.txt", "B")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "plus B")
+	localPlus := gitExec(t, cloneDir, "rev-parse", "refs/heads/+feature")
+
+	// Advance local "feature" (content C) but do NOT push it. If the push
+	// followed refspec syntax, "+feature" would push this ref instead.
+	gitExec(t, cloneDir, "checkout", "feature")
+	writeFile(t, cloneDir, "feature.txt", "C")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "feature C")
+	localFeature := gitExec(t, cloneDir, "rev-parse", "refs/heads/feature")
+	require.NotEqual(t, localPlus, localFeature, "test setup: +feature and feature must differ")
+
+	// Push the "+feature" stack branch via the force path.
+	gitExec(t, cloneDir, "checkout", "+feature")
+	require.NoError(t, d.FetchBranches("origin", []string{"+feature"}))
+	require.NoError(t, d.Push("origin", []string{"+feature"}, true, false))
+
+	// Remote "+feature" must point at local "+feature" (B), and remote
+	// "feature" must be untouched (still A).
+	assert.Equal(t, localPlus, remoteBranchSHA(t, bareDir, "+feature"),
+		"remote +feature should hold the +feature commit, not feature's")
+	assert.Equal(t, remoteFeatureBefore, remoteBranchSHA(t, bareDir, "feature"),
+		"remote feature must not be force-updated")
+}
+
+// Same as above for the non-force, atomic path (used by link and by sync when
+// no rebase happened). A bare "+feature" operand would force-update remote
+// "feature"; a fully-qualified refspec creates remote "+feature" instead.
+func TestIntegration_Push_PlusPrefixedBranch_NonForce(t *testing.T) {
+	bareDir, cloneDir := setupBareAndClone(t)
+	restore := withGitDir(t, cloneDir)
+	defer restore()
+
+	d := &defaultOps{}
+
+	// Create and push a normal "feature" branch (content A).
+	gitExec(t, cloneDir, "checkout", "-b", "feature")
+	writeFile(t, cloneDir, "feature.txt", "A")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "feature A")
+	gitExec(t, cloneDir, "push", "origin", "feature")
+	remoteFeatureBefore := remoteBranchSHA(t, bareDir, "feature")
+
+	// Create a local "+feature" branch off main with different content (B).
+	gitExec(t, cloneDir, "checkout", "main")
+	gitExec(t, cloneDir, "checkout", "-b", "+feature")
+	writeFile(t, cloneDir, "plus.txt", "B")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "plus B")
+	localPlus := gitExec(t, cloneDir, "rev-parse", "refs/heads/+feature")
+
+	// Advance local "feature" (content C) but do NOT push it.
+	gitExec(t, cloneDir, "checkout", "feature")
+	writeFile(t, cloneDir, "feature.txt", "C")
+	gitExec(t, cloneDir, "add", ".")
+	gitExec(t, cloneDir, "commit", "-m", "feature C")
+
+	// Push "+feature" via the non-force, atomic path.
+	gitExec(t, cloneDir, "checkout", "+feature")
+	require.NoError(t, d.Push("origin", []string{"+feature"}, false, true))
+
+	// Remote "+feature" must be created from local "+feature" (B), and remote
+	// "feature" must be untouched (still A).
+	assert.Equal(t, localPlus, remoteBranchSHA(t, bareDir, "+feature"),
+		"remote +feature should be created from the +feature commit")
+	assert.Equal(t, remoteFeatureBefore, remoteBranchSHA(t, bareDir, "feature"),
+		"remote feature must not be force-updated")
+}
+
 func TestSplitCommitMessage(t *testing.T) {
 	tests := []struct {
 		name        string