From 7aea4ab5ec1fcedc4ea7789836b503fedf642a71 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 9 Jul 2026 00:28:01 -0400 Subject: [PATCH 1/2] Fully-qualify branch refspecs when pushing gh-stack builds `git push` arguments from stack branch names in `internal/git`. The force path passed `:refs/heads/`, 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/:refs/heads/`. 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. --- internal/git/gitops.go | 17 +++++-- internal/git/gitops_test.go | 94 +++++++++++++++++++++++++++++++++++++ 2 files changed, 107 insertions(+), 4 deletions(-) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index bd4fe3e..f901269 100644 --- a/internal/git/gitops.go +++ b/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: :refs/heads/. + // Fully-qualified refspecs: refs/heads/:refs/heads/. + // 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 { diff --git a/internal/git/gitops_test.go b/internal/git/gitops_test.go index 24ca680..5d9ddd1 100644 --- a/internal/git/gitops_test.go +++ b/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 From 45472f0c3d1d429a7e2aa61a5dc329b95fc7f130 Mon Sep 17 00:00:00 2001 From: Sameen Karim Date: Thu, 9 Jul 2026 00:40:03 -0400 Subject: [PATCH 2/2] rm redundant if and simplify --- internal/git/gitops.go | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/internal/git/gitops.go b/internal/git/gitops.go index f901269..53d844e 100644 --- a/internal/git/gitops.go +++ b/internal/git/gitops.go @@ -187,20 +187,14 @@ func (d *defaultOps) Push(remote string, branches []string, force, atomic bool) if atomic { args = append(args, "--atomic") } - if force { - // Fully-qualified refspecs: refs/heads/:refs/heads/. - // 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("refs/heads/%s:refs/heads/%s", b, b)) - } - } else { - // 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)) - } + // Fully-qualified refspecs: refs/heads/:refs/heads/. + // 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. This form is identical whether or not the push + // is forced; force is supplied out-of-band by the --force-with-lease flags + // built above. + for _, b := range branches { + args = append(args, fmt.Sprintf("refs/heads/%s:refs/heads/%s", b, b)) } return runSilent(args...) }