Skip to content

Commit 9fba65f

Browse files
roodboiroodboi
committed
fix: address review regressions in adopt and compose
1 parent 07ba51e commit 9fba65f

3 files changed

Lines changed: 228 additions & 6 deletions

File tree

‎internal/cmd/root.go‎

Lines changed: 49 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -328,22 +328,35 @@ stack adopt pr 354 --parent pr/353 --yes
328328
return err
329329
}
330330

331-
needsFetch := !runtime.Git.BranchExists(runtime.Context, branch)
331+
branchExists := runtime.Git.BranchExists(runtime.Context, branch)
332+
localHeadOID := ""
333+
if branchExists {
334+
localHeadOID, err = runtime.Git.ResolveRef(runtime.Context, branch)
335+
if err != nil {
336+
return err
337+
}
338+
localHeadOID = strings.TrimSpace(localHeadOID)
339+
}
340+
prHeadOID := strings.TrimSpace(pr.LastSeenHeadOID)
341+
needsFetch := !branchExists
342+
refreshHead := branchExists && localHeadOID != "" && prHeadOID != "" && localHeadOID != prHeadOID
332343
preview := []string{
333344
fmt.Sprintf("pr: #%d", pr.Number),
334345
fmt.Sprintf("branch: %s", branch),
335346
fmt.Sprintf("parent: %s", parent),
336347
}
337348
if needsFetch {
338349
preview = append(preview, fmt.Sprintf("fetch: %s/%s -> %s", state.DefaultRemote, branch, branch))
350+
} else if refreshHead {
351+
preview = append(preview, fmt.Sprintf("refresh: local head %s -> PR head %s", shortOID(localHeadOID), shortOID(prHeadOID)))
339352
}
340353
if pr.BaseRefName != "" && pr.BaseRefName != parent {
341354
preview = append(preview, fmt.Sprintf("note: PR #%d currently targets %s; `stack submit %s` will retarget it later", pr.Number, pr.BaseRefName, branch))
342355
}
343356
_, _ = fmt.Fprintln(cmd.OutOrStdout(), ui.RenderPreview("Adopt PR preview", preview))
344357

345358
if !yes {
346-
confirmed, err := forms.Confirm("Adopt pull request", "This may fetch a remote branch head and will update local stack metadata.")
359+
confirmed, err := forms.Confirm("Adopt pull request", "This may fetch or refresh a remote branch head and will update local stack metadata.")
347360
if err != nil {
348361
return err
349362
}
@@ -356,6 +369,34 @@ stack adopt pr 354 --parent pr/353 --yes
356369
if err := runtime.Git.FetchBranch(runtime.Context, state.DefaultRemote, branch, branch); err != nil {
357370
return fmt.Errorf("fetch pull request #%d head %q from %s: %w", pr.Number, branch, state.DefaultRemote, err)
358371
}
372+
if prHeadOID != "" {
373+
fetchedHeadOID, err := runtime.Git.ResolveRef(runtime.Context, branch)
374+
if err != nil {
375+
return err
376+
}
377+
if strings.TrimSpace(fetchedHeadOID) != prHeadOID {
378+
return fmt.Errorf("fetched pull request #%d head %q from %s, but local branch %q now points to %s instead of expected PR head %s", pr.Number, branch, state.DefaultRemote, branch, shortOID(strings.TrimSpace(fetchedHeadOID)), shortOID(prHeadOID))
379+
}
380+
}
381+
}
382+
if refreshHead {
383+
currentBranch, err := runtime.Git.CurrentBranch(runtime.Context)
384+
if err != nil {
385+
return err
386+
}
387+
if strings.TrimSpace(currentBranch) == branch {
388+
return fmt.Errorf("local branch %q points to %s but pull request #%d is at %s; switch away from %s or delete the local branch, then rerun `stack adopt pr %d --parent %s`", branch, shortOID(localHeadOID), pr.Number, shortOID(prHeadOID), branch, pr.Number, parent)
389+
}
390+
if err := runtime.Git.FetchBranchForce(runtime.Context, state.DefaultRemote, branch, branch); err != nil {
391+
return fmt.Errorf("refresh pull request #%d head %q from %s: %w", pr.Number, branch, state.DefaultRemote, err)
392+
}
393+
refreshedHeadOID, err := runtime.Git.ResolveRef(runtime.Context, branch)
394+
if err != nil {
395+
return err
396+
}
397+
if strings.TrimSpace(refreshedHeadOID) != prHeadOID {
398+
return fmt.Errorf("refreshed pull request #%d head %q from %s, but local branch %q now points to %s instead of expected PR head %s", pr.Number, branch, state.DefaultRemote, branch, shortOID(strings.TrimSpace(refreshedHeadOID)), shortOID(prHeadOID))
399+
}
359400
}
360401

361402
parentOID, usedMergeBase, err := trackRestackAnchorDetail(runtime, branch, parent)
@@ -388,6 +429,9 @@ stack adopt pr 354 --parent pr/353 --yes
388429
if needsFetch {
389430
lines = append(lines, fmt.Sprintf("fetched: %s/%s", state.DefaultRemote, branch))
390431
}
432+
if refreshHead {
433+
lines = append(lines, fmt.Sprintf("refreshed: %s/%s -> %s", state.DefaultRemote, branch, shortOID(prHeadOID)))
434+
}
391435
if usedMergeBase {
392436
lines = append(lines, "restack anchor: merge-base with the selected parent")
393437
} else {
@@ -504,6 +548,9 @@ stack compose discovery-core --branches feature/a --branches feature/b --yes
504548
CreatedAt: time.Now().UTC().Format(time.RFC3339),
505549
}
506550
state.Landings[plan.Destination] = landing
551+
if err := runtime.Store.WriteState(runtime.Context, state); err != nil {
552+
return err
553+
}
507554

508555
if openPR {
509556
metadata := resolveComposePRMetadata(plan, tickets, title, body)
@@ -548,10 +595,6 @@ stack compose discovery-core --branches feature/a --branches feature/b --yes
548595
return nil
549596
}
550597

551-
if err := runtime.Store.WriteState(runtime.Context, state); err != nil {
552-
return err
553-
}
554-
555598
lines := []string{
556599
fmt.Sprintf("branch: %s", plan.Destination),
557600
fmt.Sprintf("base: %s", plan.Base),

‎internal/cmd/root_test.go‎

Lines changed: 173 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1230,6 +1230,97 @@ func TestAdoptPRUsesMergeBaseAnchorForStaleLocalBranch(t *testing.T) {
12301230
}
12311231
}
12321232

1233+
func TestAdoptPRRefreshesStaleLocalBranchToMatchPRHead(t *testing.T) {
1234+
repo := testutil.SetupGitRepo(t)
1235+
remote := filepath.Join(t.TempDir(), "remote.git")
1236+
testutil.Run(t, repo, "git", "init", "--bare", remote)
1237+
testutil.Run(t, repo, "git", "remote", "add", "origin", remote)
1238+
testutil.Run(t, repo, "git", "push", "-u", "origin", "main")
1239+
1240+
testutil.Run(t, repo, "git", "switch", "-c", "feature/a")
1241+
testutil.WriteFile(t, filepath.Join(repo, "feature-a.txt"), "feature a\n")
1242+
testutil.Run(t, repo, "git", "add", "feature-a.txt")
1243+
testutil.Run(t, repo, "git", "commit", "-m", "add feature a")
1244+
testutil.Run(t, repo, "git", "push", "-u", "origin", "feature/a")
1245+
staleHead := strings.TrimSpace(testutil.Run(t, repo, "git", "rev-parse", "feature/a"))
1246+
testutil.Run(t, repo, "git", "switch", "main")
1247+
1248+
otherClone := filepath.Join(t.TempDir(), "other-clone")
1249+
testutil.Run(t, "", "git", "clone", remote, otherClone)
1250+
testutil.Run(t, otherClone, "git", "switch", "feature/a")
1251+
testutil.WriteFile(t, filepath.Join(otherClone, "feature-a.txt"), "feature a advanced\n")
1252+
testutil.Run(t, otherClone, "git", "add", "feature-a.txt")
1253+
testutil.Run(t, otherClone, "git", "commit", "-m", "advance feature a")
1254+
testutil.Run(t, otherClone, "git", "push", "origin", "feature/a")
1255+
freshHead := strings.TrimSpace(testutil.Run(t, otherClone, "git", "rev-parse", "feature/a"))
1256+
1257+
runtime := newTestRuntime(repo)
1258+
mainHead, err := runtime.Git.ResolveRef(runtime.Context, "main")
1259+
if err != nil {
1260+
t.Fatalf("resolve main head: %v", err)
1261+
}
1262+
state := store.RepoState{
1263+
Version: 1,
1264+
Repo: "hack-dance/stack",
1265+
DefaultRemote: "origin",
1266+
Trunk: "main",
1267+
Branches: map[string]store.BranchRecord{},
1268+
}
1269+
if err := runtime.Store.WriteState(runtime.Context, state); err != nil {
1270+
t.Fatalf("write state: %v", err)
1271+
}
1272+
1273+
ghStub := testutil.SetupGHStub(t, "hack-dance/stack", "main")
1274+
t.Setenv("STACK_TEST_GH_STATE", ghStub.StatePath)
1275+
t.Setenv("STACK_TEST_GH_LOG", ghStub.LogPath)
1276+
t.Setenv("PATH", ghStub.Dir+string(os.PathListSeparator)+os.Getenv("PATH"))
1277+
1278+
writeGHState(t, ghStub.StatePath, `{
1279+
"repo": {
1280+
"nameWithOwner": "hack-dance/stack",
1281+
"url": "https://github.com/hack-dance/stack",
1282+
"defaultBranchRef": { "name": "main" }
1283+
},
1284+
"prs": {
1285+
"7": {
1286+
"id": "PR_7",
1287+
"number": 7,
1288+
"url": "https://example.com/hack-dance/stack/pull/7",
1289+
"repo": "hack-dance/stack",
1290+
"headRefName": "feature/a",
1291+
"baseRefName": "main",
1292+
"headRefOid": "`+freshHead+`",
1293+
"baseRefOid": "`+mainHead+`",
1294+
"state": "OPEN",
1295+
"isDraft": false
1296+
}
1297+
},
1298+
"next_number": 8
1299+
}`)
1300+
1301+
output := executeCommand(t, runtime, "adopt", "pr", "7", "--parent", "main", "--yes")
1302+
if !strings.Contains(output, "refreshed: origin/feature/a") {
1303+
t.Fatalf("expected refresh output, got %q", output)
1304+
}
1305+
1306+
currentHead := strings.TrimSpace(testutil.Run(t, repo, "git", "rev-parse", "feature/a"))
1307+
if currentHead != freshHead {
1308+
t.Fatalf("expected local feature/a head %q after adopt refresh, got %q (stale was %q)", freshHead, currentHead, staleHead)
1309+
}
1310+
1311+
state, err = runtime.Store.ReadState(runtime.Context)
1312+
if err != nil {
1313+
t.Fatalf("read state: %v", err)
1314+
}
1315+
record := state.Branches["feature/a"]
1316+
if record.PR.Number != 7 {
1317+
t.Fatalf("expected tracked PR #7, got %+v", record.PR)
1318+
}
1319+
if record.Restack.LastParentHeadOID != mainHead {
1320+
t.Fatalf("expected restack anchor %q, got %q", mainHead, record.Restack.LastParentHeadOID)
1321+
}
1322+
}
1323+
12331324
func TestComposeCreatesLandingBranchFromTrackedRange(t *testing.T) {
12341325
repo := testutil.SetupGitRepo(t)
12351326

@@ -1443,6 +1534,88 @@ func TestComposeOpenPRCreatesLandingPRAndStoresTickets(t *testing.T) {
14431534
}
14441535
}
14451536

1537+
func TestComposeOpenPRPersistsLandingMetadataBeforePROperations(t *testing.T) {
1538+
repo := testutil.SetupGitRepo(t)
1539+
remote := filepath.Join(t.TempDir(), "remote.git")
1540+
testutil.Run(t, repo, "git", "init", "--bare", remote)
1541+
testutil.Run(t, repo, "git", "remote", "add", "origin", remote)
1542+
testutil.Run(t, repo, "git", "push", "-u", "origin", "main")
1543+
1544+
testutil.Run(t, repo, "git", "switch", "-c", "feature/a")
1545+
testutil.WriteFile(t, filepath.Join(repo, "feature-a.txt"), "feature a\n")
1546+
testutil.Run(t, repo, "git", "add", "feature-a.txt")
1547+
testutil.Run(t, repo, "git", "commit", "-m", "add feature a")
1548+
featureAHead := strings.TrimSpace(testutil.Run(t, repo, "git", "rev-parse", "feature/a"))
1549+
1550+
testutil.Run(t, repo, "git", "switch", "-c", "feature/b")
1551+
testutil.WriteFile(t, filepath.Join(repo, "feature-b.txt"), "feature b\n")
1552+
testutil.Run(t, repo, "git", "add", "feature-b.txt")
1553+
testutil.Run(t, repo, "git", "commit", "-m", "add feature b")
1554+
1555+
runtime := newTestRuntime(repo)
1556+
mainHead, err := runtime.Git.ResolveRef(runtime.Context, "main")
1557+
if err != nil {
1558+
t.Fatalf("resolve main head: %v", err)
1559+
}
1560+
state := store.RepoState{
1561+
Version: 1,
1562+
Repo: "hack-dance/stack",
1563+
DefaultRemote: "origin",
1564+
Trunk: "main",
1565+
Branches: map[string]store.BranchRecord{
1566+
"feature/a": {
1567+
ParentBranch: "main",
1568+
RemoteName: "origin",
1569+
Restack: store.RestackMetadata{
1570+
LastParentHeadOID: mainHead,
1571+
},
1572+
},
1573+
"feature/b": {
1574+
ParentBranch: "feature/a",
1575+
RemoteName: "origin",
1576+
Restack: store.RestackMetadata{
1577+
LastParentHeadOID: featureAHead,
1578+
},
1579+
},
1580+
},
1581+
}
1582+
if err := runtime.Store.WriteState(runtime.Context, state); err != nil {
1583+
t.Fatalf("write state: %v", err)
1584+
}
1585+
1586+
failDir := t.TempDir()
1587+
failGH := filepath.Join(failDir, "gh")
1588+
testutil.WriteFile(t, failGH, "#!/bin/sh\necho \"forced gh failure\" >&2\nexit 1\n")
1589+
testutil.Run(t, "", "chmod", "+x", failGH)
1590+
t.Setenv("PATH", failDir+string(os.PathListSeparator)+os.Getenv("PATH"))
1591+
1592+
err = executeCommandExpectError(runtime, "compose", "discovery-core", "--from", "feature/a", "--to", "feature/b", "--ticket", "LNHACK-66", "--open-pr", "--yes")
1593+
if err == nil || !strings.Contains(err.Error(), "forced gh failure") {
1594+
t.Fatalf("expected gh failure during compose --open-pr, got %v", err)
1595+
}
1596+
1597+
state, err = runtime.Store.ReadState(runtime.Context)
1598+
if err != nil {
1599+
t.Fatalf("read state after failed compose: %v", err)
1600+
}
1601+
landing, ok := state.Landings["stack/discovery-core"]
1602+
if !ok {
1603+
t.Fatalf("expected landing metadata to be recorded before PR operations fail")
1604+
}
1605+
if got := strings.Join(landing.Tickets, ","); got != "LNHACK-66" {
1606+
t.Fatalf("unexpected landing tickets %q", got)
1607+
}
1608+
if landing.LandingPRNumber != 0 {
1609+
t.Fatalf("expected landing PR number to remain unset after failure, got %+v", landing)
1610+
}
1611+
if !runtime.Git.BranchExists(runtime.Context, "stack/discovery-core") {
1612+
t.Fatalf("expected landing branch to exist locally after failed PR open")
1613+
}
1614+
if !runtime.Git.RemoteBranchExists(runtime.Context, "origin", "stack/discovery-core") {
1615+
t.Fatalf("expected landing branch to be pushed before gh failure")
1616+
}
1617+
}
1618+
14461619
func TestCloseoutClassifiesSupersededPRsAndDeployGatedTickets(t *testing.T) {
14471620
repo := testutil.SetupGitRepo(t)
14481621
remote := filepath.Join(t.TempDir(), "remote.git")

‎internal/git/client.go‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,12 @@ func (c *Client) FetchBranch(ctx context.Context, remote string, sourceBranch st
170170
return err
171171
}
172172

173+
func (c *Client) FetchBranchForce(ctx context.Context, remote string, sourceBranch string, localBranch string) error {
174+
refspec := fmt.Sprintf("+refs/heads/%s:refs/heads/%s", sourceBranch, localBranch)
175+
_, err := c.run(ctx, "fetch", remote, refspec)
176+
return err
177+
}
178+
173179
func (c *Client) PushBranch(ctx context.Context, remote string, branch string, expectedRemoteOID string) error {
174180
args := []string{"push"}
175181
if expectedRemoteOID != "" {

0 commit comments

Comments
 (0)