Skip to content

Commit 2a31ea5

Browse files
abhinavclaude
abhinav
and
claude
authored
fix(submit): Validate fetch refspecs and fix upstream tracking (#963)
Fixes a panic when updating CRs without upstream tracking and prevents branches from being left in an untrackable state. When git is configured with minimal fetch refspecs (e.g., only fetching main), pushing a new branch would succeed, but the branch could not be fetched back locally. This left the branch without upstream tracking, causing a panic on subsequent submit attempts. This change adds validation before pushing to ensure the remote's fetch refspecs will actually fetch the pushed branch. If not, it provides clear error messages with options to fix the configuration. Additionally, replaces the panic when updating CRs without upstream tracking with a helpful error message guiding users to set the upstream manually. LLM assistance: An LLM was used to reproduce the original panic, to identify in Git source code where refspec matching is implemented, to transform that logic into Go code, and to draft the original commit message. Fixes #962 Co-authored-by: Claude <noreply@anthropic.com>
1 parent 6fa18ab commit 2a31ea5

File tree

7 files changed

+428
-1
lines changed

7 files changed

+428
-1
lines changed

‎.changes/unreleased/Fixed-20251204-211214.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
kind: Fixed
2+
body: 'branch submit: Check if git configuration prevents pushed branches from being tracked, which could leave branches in a state where follow-up submits are not possible'
3+
time: 2025-12-04T21:12:14.799178-08:00

‎.changes/unreleased/Fixed-20251204-211302.yaml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
kind: Fixed
2+
body: 'branch submit: Fix panic when updating CRs that have no upstream branch configured'
3+
time: 2025-12-04T21:13:02.794912-08:00

‎internal/git/ref.go‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package git
22

33
import (
44
"context"
5+
"strings"
56

67
"go.abhg.dev/gs/internal/silog"
78
)
@@ -14,6 +15,45 @@ func (r Refspec) String() string {
1415
return string(r)
1516
}
1617

18+
// Matches checks if a branch name matches a fetch refspec pattern.
19+
//
20+
// The refspec should be in the format used by git fetch, e.g.:
21+
// - "refs/heads/foo/*" (pattern)
22+
// - "+refs/heads/foo/*:refs/remotes/origin/foo/*" (full refspec with destination)
23+
// - "refs/heads/master" (exact match)
24+
//
25+
// For fetch refspecs with a destination (containing ':'),
26+
// only the source side (left of ':') is used for matching.
27+
//
28+
// This implements the same algorithm as Git:
29+
// https://github.com/git/git/blob/f0ef5b6d9bcc258e4cbef93839d1b7465d5212b9/refspec.c#L298-L333
30+
func (r Refspec) Matches(ref string) bool {
31+
pattern := strings.TrimPrefix(string(r), "+") // "+" prefix is optional
32+
33+
// For refspecs with destination, extract source side (left of ':').
34+
// E.g.
35+
// "+refs/heads/foo/*:refs/remotes/origin/foo/*" -> "refs/heads/foo/*"
36+
pattern, _, _ = strings.Cut(pattern, ":")
37+
38+
prefix, suffix, ok := strings.Cut(pattern, "*")
39+
if !ok {
40+
// If this is not a pattern refspec (no '*'), do exact match
41+
return pattern == ref
42+
}
43+
44+
prefixLen := len(prefix)
45+
suffixLen := len(suffix)
46+
refLen := len(ref)
47+
48+
// Match if:
49+
// 1. branchName starts with prefix
50+
// 2. branchName is long enough for prefix + suffix
51+
// 3. branchName ends with suffix
52+
return refLen >= prefixLen+suffixLen &&
53+
strings.HasPrefix(ref, prefix) &&
54+
strings.HasSuffix(ref, suffix)
55+
}
56+
1757
// SetRefRequest is a request to set a ref to a new hash.
1858
type SetRefRequest struct {
1959
// Ref is the name of the ref to set.

‎internal/git/ref_test.go‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,198 @@ import (
1212
"go.abhg.dev/gs/internal/text"
1313
)
1414

15+
func TestRefspec_Matches(t *testing.T) {
16+
tests := []struct {
17+
name string
18+
refspec git.Refspec
19+
ref string
20+
want bool
21+
}{
22+
// Exact match cases
23+
{
24+
name: "ExactMatch",
25+
refspec: "refs/heads/main",
26+
ref: "refs/heads/main",
27+
want: true,
28+
},
29+
{
30+
name: "ExactMatchNoMatch",
31+
refspec: "refs/heads/main",
32+
ref: "refs/heads/feature",
33+
want: false,
34+
},
35+
{
36+
name: "ExactMatchCaseSensitive",
37+
refspec: "refs/heads/Main",
38+
ref: "refs/heads/main",
39+
want: false,
40+
},
41+
42+
// Wildcard pattern cases - prefix only
43+
{
44+
name: "WildcardPrefixMatch",
45+
refspec: "refs/heads/*",
46+
ref: "refs/heads/feature",
47+
want: true,
48+
},
49+
{
50+
name: "WildcardPrefixMatchNested",
51+
refspec: "refs/heads/*",
52+
ref: "refs/heads/feature/foo",
53+
want: true,
54+
},
55+
{
56+
name: "WildcardPrefixNoMatch",
57+
refspec: "refs/heads/*",
58+
ref: "refs/tags/v1.0",
59+
want: false,
60+
},
61+
{
62+
name: "WildcardPrefixTooShort",
63+
refspec: "refs/heads/*",
64+
ref: "refs/heads/",
65+
want: true, // empty suffix, so this matches
66+
},
67+
68+
// Wildcard pattern cases - prefix and suffix
69+
{
70+
name: "WildcardPrefixAndSuffixMatch",
71+
refspec: "refs/heads/*/main",
72+
ref: "refs/heads/feature/main",
73+
want: true,
74+
},
75+
{
76+
name: "WildcardPrefixAndSuffixMatchLonger",
77+
refspec: "refs/heads/*/main",
78+
ref: "refs/heads/team/feature/main",
79+
want: true,
80+
},
81+
{
82+
name: "WildcardPrefixAndSuffixNoMatch",
83+
refspec: "refs/heads/*/main",
84+
ref: "refs/heads/feature/develop",
85+
want: false,
86+
},
87+
{
88+
name: "WildcardPrefixAndSuffixTooShort",
89+
refspec: "refs/heads/*/main",
90+
ref: "refs/heads/main",
91+
want: false, // not long enough for prefix + suffix
92+
},
93+
94+
// Wildcard at start
95+
{
96+
name: "WildcardAtStartMatch",
97+
refspec: "*/main",
98+
ref: "refs/heads/main",
99+
want: true,
100+
},
101+
{
102+
name: "WildcardAtStartNoMatch",
103+
refspec: "*/main",
104+
ref: "refs/heads/feature",
105+
want: false,
106+
},
107+
108+
// Refspec format handling - with '+' prefix
109+
{
110+
name: "ForcePushPrefixExact",
111+
refspec: "+refs/heads/main",
112+
ref: "refs/heads/main",
113+
want: true,
114+
},
115+
{
116+
name: "ForcePushPrefixWildcard",
117+
refspec: "+refs/heads/*",
118+
ref: "refs/heads/feature",
119+
want: true,
120+
},
121+
122+
// Refspec format handling - with destination
123+
{
124+
name: "WithDestinationExact",
125+
refspec: "refs/heads/main:refs/remotes/origin/main",
126+
ref: "refs/heads/main",
127+
want: true,
128+
},
129+
{
130+
name: "WithDestinationWildcard",
131+
refspec: "refs/heads/*:refs/remotes/origin/*",
132+
ref: "refs/heads/feature",
133+
want: true,
134+
},
135+
{
136+
name: "WithDestinationNoMatch",
137+
refspec: "refs/heads/main:refs/remotes/origin/main",
138+
ref: "refs/heads/feature",
139+
want: false,
140+
},
141+
142+
// Refspec format handling - with both '+' and destination
143+
{
144+
name: "ForcePushWithDestination",
145+
refspec: "+refs/heads/*:refs/remotes/origin/*",
146+
ref: "refs/heads/feature",
147+
want: true,
148+
},
149+
150+
// Real-world examples from Issue #962
151+
{
152+
name: "Issue962MinimalRefspecMatch",
153+
refspec: "+refs/heads/main:refs/remotes/origin/main",
154+
ref: "refs/heads/main",
155+
want: true,
156+
},
157+
{
158+
name: "Issue962MinimalRefspecNoMatch",
159+
refspec: "+refs/heads/main:refs/remotes/origin/main",
160+
ref: "refs/heads/feature1",
161+
want: false,
162+
},
163+
{
164+
name: "Issue962StandardRefspec",
165+
refspec: "+refs/heads/*:refs/remotes/origin/*",
166+
ref: "refs/heads/feature1",
167+
want: true,
168+
},
169+
170+
// Edge cases
171+
{
172+
name: "EmptyRefspec",
173+
refspec: "",
174+
ref: "refs/heads/main",
175+
want: false,
176+
},
177+
{
178+
name: "EmptyRef",
179+
refspec: "refs/heads/*",
180+
ref: "",
181+
want: false,
182+
},
183+
{
184+
name: "OnlyWildcard",
185+
refspec: "*",
186+
ref: "anything",
187+
want: true,
188+
},
189+
{
190+
name: "OnlyWildcardWithColon",
191+
refspec: "*:refs/remotes/origin/*",
192+
ref: "refs/heads/main",
193+
want: true,
194+
},
195+
}
196+
197+
for _, tt := range tests {
198+
t.Run(tt.name, func(t *testing.T) {
199+
got := tt.refspec.Matches(tt.ref)
200+
assert.Equal(t, tt.want, got,
201+
"Refspec(%q).Matches(%q) = %v, want %v",
202+
tt.refspec, tt.ref, got, tt.want)
203+
})
204+
}
205+
}
206+
15207
func TestSetRef(t *testing.T) {
16208
fixture, err := gittest.LoadFixtureScript([]byte(text.Dedent(`
17209
as 'Test <test@example.com>'

‎internal/git/remote.go‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,32 @@ func (r *Repository) RemoteDefaultBranch(ctx context.Context, remote string) (st
6060
return ref, nil
6161
}
6262

63+
// RemoteFetchRefspecs returns the fetch refspecs for a remote.
64+
// That is, the refspecs used when fetching from the remote.
65+
// Example:
66+
//
67+
// +refs/heads/*:refs/remotes/origin/*
68+
func (r *Repository) RemoteFetchRefspecs(ctx context.Context, remote string) ([]Refspec, error) {
69+
output, err := r.gitCmd(
70+
ctx, "config", "--get-all", "remote."+remote+".fetch").
71+
OutputString(r.exec)
72+
if err != nil {
73+
return nil, fmt.Errorf("config get-all remote.%s.fetch: %w", remote, err)
74+
}
75+
76+
var refspecs []Refspec
77+
for line := range strings.SplitSeq(output, "\n") {
78+
line = strings.TrimSpace(line)
79+
if line == "" {
80+
continue
81+
}
82+
83+
refspecs = append(refspecs, Refspec(line))
84+
}
85+
86+
return refspecs, nil
87+
}
88+
6389
// RemoteRef is a reference in a remote Git repository.
6490
type RemoteRef struct {
6591
// Name is the full name of the reference.

‎internal/handler/submit/handler.go‎

Lines changed: 53 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"encoding"
99
"errors"
1010
"fmt"
11+
"os"
1112
"slices"
1213
"sort"
1314
"strconv"
@@ -35,6 +36,7 @@ type GitRepository interface {
3536
SetBranchUpstream(ctx context.Context, branch string, upstream string) error
3637
Var(ctx context.Context, name string) (string, error)
3738
CommitMessageRange(ctx context.Context, start string, stop string) ([]git.CommitMessage, error)
39+
RemoteFetchRefspecs(ctx context.Context, remote string) ([]git.Refspec, error)
3840
}
3941

4042
var _ GitRepository = (*git.Repository)(nil)
@@ -648,6 +650,49 @@ func (h *Handler) submitBranch(
648650
return status, nil
649651
}
650652

653+
// Sanity check:
654+
// If we're going to push the branch,
655+
// make sure that the fetch refspec will actually fetch it.
656+
// Otherwise, we will push to origin/feature,
657+
// but won't have a local refs/remotes/origin/feature
658+
// to track it after a 'git fetch'.
659+
if refspecs, err := h.Repository.RemoteFetchRefspecs(ctx, remote); err != nil {
660+
log.Warn("Unable to verify remote's fetch refspecs",
661+
"remote", remote,
662+
"error", err)
663+
} else {
664+
wantMatch := "refs/heads/" + upstreamBranch
665+
var hasMatch bool
666+
for _, refspec := range refspecs {
667+
if refspec.Matches(wantMatch) {
668+
hasMatch = true
669+
break
670+
}
671+
}
672+
673+
if !hasMatch && !opts.Force {
674+
log.Errorf("Remote '%v' has refspecs:", remote)
675+
for _, refspec := range refspecs {
676+
log.Errorf(" - %v", refspec)
677+
}
678+
user := cmp.Or(os.Getenv("USER"), "yourname")
679+
log.Errorf("None of these will fetch branch '%v' after pushing.", upstreamBranch)
680+
log.Error("This will make follow up changes on them impossible.")
681+
log.Error("To fix this, you can do one of the following:")
682+
log.Errorf("1. Manually add a fetch refspec for just this branch:")
683+
log.Errorf(" git config --add remote.%v.fetch +refs/heads/%v:refs/remotes/%v/%v",
684+
remote, upstreamBranch, remote, upstreamBranch)
685+
log.Errorf("2. Prefix all your branches with your username (e.g. '%v/%v'),", user, upstreamBranch)
686+
log.Errorf(" and add a fetch refspec to fetch all branches under that prefix:")
687+
log.Errorf(" git config --add remote.%v.fetch '+refs/heads/%v/*:refs/remotes/%v/%v/*'",
688+
remote, user, remote, user)
689+
log.Errorf(" You can configure git-spice to automatically add this prefix for future branches with:")
690+
log.Errorf(" git config --global spice.branchCreate.prefix %v/", user)
691+
log.Errorf("3. Use the --force flag to push anyway (not recommended).")
692+
return status, errors.New("remote cannot fetch pushed branch")
693+
}
694+
}
695+
651696
var prepared *preparedBranch
652697
if opts.Publish {
653698
needsNavComment()
@@ -752,7 +797,14 @@ func (h *Handler) submitBranch(
752797
}
753798
} else {
754799
needsNavComment()
755-
must.NotBeBlankf(upstreamBranch, "upstream branch must be set if branch has a CR")
800+
if upstreamBranch == "" {
801+
log.Error("No upstream branch was found for branch %v with existing CR %v", branchToSubmit, existingChange.ID)
802+
log.Error("We cannot update the CR without an upstream branch name.")
803+
log.Error("To fix this, identify the correct upstream branch name and set it with, e.g.:")
804+
log.Error(" git branch --set-upstream-to=<remote>/<branch> %v", branchToSubmit)
805+
log.Error("Then, try again.")
806+
return status, errors.New("upstream branch not set")
807+
}
756808

757809
if !opts.Publish {
758810
log.Warnf("Ignoring --no-publish: %s was already published: %s", branchToSubmit, existingChange.URL)

0 commit comments

Comments
 (0)