submit: Include merged CRs in navigation comments (#508)

This change adds merged CRs to navigation comments.
When a CR is merged, its upstacks will still report it in their nav comment
even when they're rebased and resubmitted.


To do that git-spice now keeps track of merged PRs in the per-branch state
of the bottom branch in each stack.
This then gets bubbled up to all branches directly above it when that branch
is merged.

The state is currently propagated from `repo sync`.
Two special-cases are added:

- `stack edit` and `downstack edit` will transfer
  the merged history  to the new bottom-most branch
- `branch create --below` from the bottom-most branch
  will transfer merge history to the new bottom-most branch

In the future, we may need to introduce a shared space
for tracking branch and CR history instead of in dependents.
(e.g. a place where all branches, current and former are tracked,
maybe with some garbage collection.)


Resolves #331

---------

Co-authored-by: Abhinav Gupta <[email protected]>

.changes/unreleased/Added-20241201-205446.yaml

@@ -0,0 +1,5 @@
+kind: Added
+body: >-
+  submit: Include merged changes in navigation comments
+  when restacking and resubmitting changes based on them.
+time: 2024-12-01T20:54:46.755566Z

branch_create.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 
 	"github.com/charmbracelet/log"
@@ -110,6 +111,11 @@ func (cmd *branchCreateCmd) Run(ctx context.Context, log *log.Logger, view ui.Vi
 	var (
 		baseHash       git.Hash
 		restackOntoNew []string // branches to restack onto the new branch
+
+		// Downstack history for the new branch
+		// and for those restacked on top of it.
+		newMergedDownstack       *[]json.RawMessage
+		restackedMergedDownstack *[]json.RawMessage
 	)
 	if cmd.Below {
 		if cmd.Target == trunk {
@@ -128,6 +134,16 @@ func (cmd *branchCreateCmd) Run(ctx context.Context, log *log.Logger, view ui.Vi
 		restackOntoNew = append(restackOntoNew, cmd.Target)
 		baseName = b.Base
 		baseHash = b.BaseHash
+
+		// If the branch is at the bottom of the stack
+		// and has a merged downstack history,
+		// transfer it to the new branch.
+		if len(b.MergedDownstack) > 0 {
+			newMergedDownstack = &b.MergedDownstack
+			restackedMergedDownstack = new([]json.RawMessage)
+		}
+
+		// TODO: Maybe this transfer should take place at submit time?
 	} else if cmd.Insert {
 		// If inserting, above the target branch,
 		// restack all its upstack branches on top of the new branch.
@@ -207,9 +223,10 @@ func (cmd *branchCreateCmd) Run(ctx context.Context, log *log.Logger, view ui.Vi
 	// and rollback to the original branch and staged changes.
 	branchTx := store.BeginBranchTx()
 	if err := branchTx.Upsert(ctx, state.UpsertRequest{
-		Name:     cmd.Name,
-		Base:     baseName,
-		BaseHash: baseHash,
+		Name:            cmd.Name,
+		Base:            baseName,
+		BaseHash:        baseHash,
+		MergedDownstack: newMergedDownstack,
 	}); err != nil {
 		return fmt.Errorf("add branch %v with base %v: %w", cmd.Name, baseName, err)
 	}
@@ -220,8 +237,9 @@ func (cmd *branchCreateCmd) Run(ctx context.Context, log *log.Logger, view ui.Vi
 		//
 		// We'll run a restack command after this to update the state.
 		if err := branchTx.Upsert(ctx, state.UpsertRequest{
-			Name: branch,
-			Base: cmd.Name,
+			Name:            branch,
+			Base:            cmd.Name,
+			MergedDownstack: restackedMergedDownstack,
 		}); err != nil {
 			return fmt.Errorf("update base branch of %v: %w", branch, err)
 		}

doc/src/guide/cr.md

@@ -79,6 +79,16 @@ and the position of the current branch in it.
 This behavior may be changed with the $$spice.submit.navigationComment$$
 configuration key.
 
+
+!!! info "Stack history in navigation comments"
+
+    <!-- gs:version unreleased -->
+    When possible, git-spice will remember CRs as they're merged into trunk,
+    and continue to list them in navigation comments of branches
+    based on those changes.
+    However, it is unable to do this following complex stack manipulation
+    operations.
+
 ### Non-interactive submission
 
 Use the `--fill` flag (or `-c` since <!-- gs:version v0.3.0 -->)

internal/forge/shamhub/cli.go

@@ -245,8 +245,21 @@ func (c *Cmd) Run(ts *testscript.TestScript, neg bool, args []string) {
 			give = changes
 
 		case "comments":
+			want := func(*ChangeComment) bool { return true }
 			if len(args) != 0 {
-				ts.Fatalf("usage: shamhub dump comments")
+				changeIDs := make(map[int]struct{})
+				for _, arg := range args {
+					n, err := strconv.Atoi(arg)
+					if err != nil {
+						ts.Fatalf("invalid change number: %s", err)
+					}
+					changeIDs[n] = struct{}{}
+				}
+
+				want = func(c *ChangeComment) bool {
+					_, ok := changeIDs[c.Change]
+					return ok
+				}
 			}
 
 			// Actual change comments have non-deterministic IDs.
@@ -264,6 +277,10 @@ func (c *Cmd) Run(ts *testscript.TestScript, neg bool, args []string) {
 
 			var comments []changeComment
 			for _, c := range shamComments {
+				if !want(c) {
+					continue
+				}
+
 				comments = append(comments, changeComment{
 					Change: c.Change,
 					Body:   c.Body,

internal/spice/branch.go

@@ -76,6 +76,12 @@ type LookupBranchResponse struct {
 
 	// Head is the commit at the head of the branch.
 	Head git.Hash
+
+	// MergedDownstack is a list of branches that were previously
+	// downstack from this branch and have since been merged into trunk.
+	//
+	// This is used to correctly display the history of the branch.
+	MergedDownstack []json.RawMessage
 }
 
 // DeletedBranchError is returned when a branch was deleted out of band.
@@ -112,10 +118,11 @@ func (s *Service) LookupBranch(ctx context.Context, name string) (*LookupBranchR
 	// !nil     | !nil   | Branch is not known to the repository
 	if storeErr == nil && gitErr == nil {
 		out := &LookupBranchResponse{
-			Base:           resp.Base,
-			BaseHash:       resp.BaseHash,
-			UpstreamBranch: resp.UpstreamBranch,
-			Head:           head,
+			Base:            resp.Base,
+			BaseHash:        resp.BaseHash,
+			UpstreamBranch:  resp.UpstreamBranch,
+			Head:            head,
+			MergedDownstack: resp.MergedDownstack,
 		}
 
 		if resp.ChangeMetadata != nil {
@@ -340,6 +347,10 @@ type LoadBranchItem struct {
 	// UpstreamBranch is the name under which this branch
 	// was pushed to the upstream repository.
 	UpstreamBranch string
+
+	// MergedDownstack contains information about any branches,
+	// which this one was based on, that have already been merged into trunk.
+	MergedDownstack []json.RawMessage
 }
 
 // LoadBranches loads all tracked branches
@@ -370,12 +381,13 @@ func (s *Service) LoadBranches(ctx context.Context) ([]LoadBranchItem, error) {
 		}
 
 		items = append(items, LoadBranchItem{
-			Name:           name,
-			Head:           resp.Head,
-			Base:           resp.Base,
-			BaseHash:       resp.BaseHash,
-			UpstreamBranch: resp.UpstreamBranch,
-			Change:         resp.Change,
+			Name:            name,
+			Head:            resp.Head,
+			Base:            resp.Base,
+			BaseHash:        resp.BaseHash,
+			UpstreamBranch:  resp.UpstreamBranch,
+			Change:          resp.Change,
+			MergedDownstack: resp.MergedDownstack,
 		})
 	}
 

internal/spice/onto.go

@@ -2,6 +2,7 @@ package spice
 
 import (
 	"context"
+	"encoding/json"
 	"fmt"
 
 	"go.abhg.dev/gs/internal/git"
@@ -18,6 +19,9 @@ type BranchOntoRequest struct {
 	// Onto is the target branch to move the branch onto.
 	// Onto may be the trunk branch.
 	Onto string
+
+	// MergedDownstack for [Branch], if any.
+	MergedDownstack *[]json.RawMessage
 }
 
 // BranchOnto moves the commits of a branch onto a different base branch,
@@ -50,9 +54,10 @@ func (s *Service) BranchOnto(ctx context.Context, req *BranchOntoRequest) error
 
 	branchTx := s.store.BeginBranchTx()
 	if err := branchTx.Upsert(ctx, state.UpsertRequest{
-		Name:     req.Branch,
-		Base:     req.Onto,
-		BaseHash: ontoHash,
+		Name:            req.Branch,
+		Base:            req.Onto,
+		BaseHash:        ontoHash,
+		MergedDownstack: req.MergedDownstack,
 	}); err != nil {
 		return fmt.Errorf("set base of branch %s to %s: %w", req.Branch, req.Onto, err)
 	}

internal/spice/stack_edit.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -44,17 +45,41 @@ func (s *Service) StackEdit(ctx context.Context, req *StackEditRequest) (*StackE
 	must.NotContainf(req.Stack, s.store.Trunk(), "cannot edit trunk")
 	must.NotBeBlankf(req.Editor, "editor is required")
 
+	// TODO: assert that req.Stack[0] has trunk as its base.
+	bottomName := req.Stack[0]
+	bottom, err := s.LookupBranch(ctx, req.Stack[0])
+	if err != nil {
+		return nil, fmt.Errorf("look up lowest branch (%q): %w", req.Stack[0], err)
+	}
+
 	branches, err := editStackFile(req.Editor, req.Stack)
 	if err != nil {
 		return nil, err
 	}
 
-	base := s.store.Trunk()
-	for _, branch := range branches {
-		if err := s.BranchOnto(ctx, &BranchOntoRequest{
+	base := bottom.Base
+	for idx, branch := range branches {
+		req := BranchOntoRequest{
 			Branch: branch,
 			Onto:   base,
-		}); err != nil {
+		}
+
+		if len(bottom.MergedDownstack) > 0 {
+			// If the bottom-most branch is changing,
+			// copy the merged downstack over to it.
+			if idx == 0 && branch != bottomName {
+				req.MergedDownstack = &bottom.MergedDownstack
+			}
+
+			// Also in that case, make sure to clear it
+			// from the new position of the original bottom branch.
+			if idx > 0 && branch == bottomName {
+				var newHistory []json.RawMessage
+				req.MergedDownstack = &newHistory
+			}
+		}
+
+		if err := s.BranchOnto(ctx, &req); err != nil {
 			return nil, fmt.Errorf("branch %v onto %v: %w", branch, base, err)
 		}
 		base = branch

internal/spice/state/branch.go

@@ -67,6 +67,8 @@ type branchState struct {
 	Base     branchStateBase      `json:"base"`
 	Upstream *branchUpstreamState `json:"upstream,omitempty"`
 	Change   *branchChangeState   `json:"change,omitempty"`
+
+	MergedDownstack []json.RawMessage `json:"merged,omitempty"`
 }
 
 // branchKey returns the path to the JSON file for the given branch
@@ -98,6 +100,15 @@ type LookupResponse struct {
 	// UpstreamBranch is the name of the upstream branch
 	// or an empty string if the branch is not tracking an upstream branch.
 	UpstreamBranch string
+
+	// MergedDownstack holds information about branches
+	// that were previously downstack from this branch
+	// that have since been merged into trunk.
+	//
+	// MergedDownstack is in the order that the branches were merged.
+	// For example, if the stack was main -> A -> B -> C,
+	// where C is this branch, MergedDownstack will be [A, B].
+	MergedDownstack []json.RawMessage
 }
 
 // LookupBranch returns information about a tracked branch.
@@ -109,8 +120,9 @@ func (s *Store) LookupBranch(ctx context.Context, name string) (*LookupResponse,
 	}
 
 	res := &LookupResponse{
-		Base:     state.Base.Name,
-		BaseHash: git.Hash(state.Base.Hash),
+		Base:            state.Base.Name,
+		BaseHash:        git.Hash(state.Base.Hash),
+		MergedDownstack: state.MergedDownstack,
 	}
 
 	if change := state.Change; change != nil {
@@ -220,6 +232,10 @@ type UpsertRequest struct {
 	// UpstreamBranch is the name of the upstream branch to track.
 	// Leave nil to leave it unchanged, or set to an empty string to clear it.
 	UpstreamBranch *string
+
+	// MergedDownstack is a list of branches that were previously
+	// downstack from this branch that have since been merged into trunk.
+	MergedDownstack *[]json.RawMessage
 }
 
 // Upsert adds or updates information about a branch.
@@ -298,6 +314,10 @@ func (tx *BranchTx) Upsert(ctx context.Context, req UpsertRequest) error {
 		}
 	}
 
+	if req.MergedDownstack != nil {
+		state.MergedDownstack = *req.MergedDownstack
+	}
+
 	tx.states[req.Name] = state
 	tx.sets[req.Name] = struct{}{}
 	delete(tx.dels, req.Name)