branch submit: Handle CR already closed or merged (#381)

If a submitted CR is already closed or merged server-side,
pretend that the CR does not exist and create a new CR instead.

This is a more user-friendly behavior
than permanently tying the branch name to that CR
(or until the user runs untrack-retrack).

In the future, we can offer to re-open closed CRs with a prompt too.

Resolves #374

Author: abhinav

.changes/unreleased/Changed-20240904-054549.yaml

@@ -0,0 +1,7 @@
+kind: Changed
+body: >-
+  submit: If a CR for a branch is closed or merged,
+  and the branch is submitted again,
+  git-spice will now create a new CR for that branch
+  instead of failing to update the existing CR.
+time: 2024-09-04T05:45:49.967536-07:00

branch_submit.go

@@ -263,8 +263,22 @@ func (cmd *branchSubmitCmd) run(
 			return fmt.Errorf("find change: %w", err)
 		}
 
-		// TODO: If the CR is closed, we should treat it as non-existent.
-		existingChange = change
+		// Consider the PR only if it's open.
+		if change.State == forge.ChangeOpen {
+			existingChange = change
+		} else {
+			var state string
+			if change.State == forge.ChangeMerged {
+				state = "merged"
+			} else {
+				state = "closed"
+			}
+
+			log.Infof("%v: Ignoring CR %v as it was %s: %v", cmd.Branch, change.ID, state, change.URL)
+			// TODO:
+			// We could offer to reopen the CR if it was closed,
+			// but not if it was merged.
+		}
 	}
 
 	// At this point, existingChange is nil only if we need to create a new CR.

internal/forge/forge.go

@@ -272,26 +272,26 @@ type EditChangeOptions struct {
 // repository.
 type FindChangeItem struct {
 	// ID is a unique identifier for the change.
-	ID ChangeID
+	ID ChangeID // required
 
 	// URL is the web URL at which the change can be viewed.
-	URL string
+	URL string // required
 
 	// State is the current state of the change.
-	State ChangeState
+	State ChangeState // required
 
 	// Subject is the title of the change.
-	Subject string
+	Subject string // required
 
 	// HeadHash is the hash of the commit at the top of the change.
-	HeadHash git.Hash
+	HeadHash git.Hash // required
 
 	// BaseName is the name of the base branch
 	// that this change is proposed against.
-	BaseName string
+	BaseName string // required
 
 	// Draft is true if the change is not yet ready to be reviewed.
-	Draft bool
+	Draft bool // required
 }
 
 // ChangeTemplate is a template for a new change proposal.

internal/forge/shamhub/change.go

@@ -56,14 +56,25 @@ type shamChange struct {
 	Owner string
 	Repo  string
 
+	// State is the current state of the change.
+	// It can be open, closed, or merged.
+	State shamChangeState
+
+	// Number is the numeric identifier of the change.
+	// These increment monotonically.
 	Number int
-	Draft  bool
-	State  shamChangeState
+
+	// Draft indicates that the change is not yet ready to be reviewed.
+	Draft bool
 
 	Subject string
 	Body    string
 
+	// Name of the base branch.
 	Base string
+
+	// Name of the head branch.
+	// This is the branch that contains the changes to be merged.
 	Head string
 }
 

internal/forge/shamhub/cli.go

@@ -157,6 +157,31 @@ func (c *Cmd) Run(ts *testscript.TestScript, neg bool, args []string) {
 
 		ts.Check(sh.MergeChange(req))
 
+	case "reject":
+		if len(args) != 2 {
+			ts.Fatalf("usage: shamhub reject <owner/repo> <pr>")
+		}
+		if sh == nil {
+			ts.Fatalf("ShamHub not initialized")
+		}
+
+		ownerRepo, prStr := args[0], args[1]
+		owner, repo, ok := strings.Cut(ownerRepo, "/")
+		if !ok {
+			ts.Fatalf("invalid owner/repo: %s", ownerRepo)
+		}
+		pr, err := strconv.Atoi(prStr)
+		if err != nil {
+			ts.Fatalf("invalid PR number: %s", err)
+		}
+
+		req := RejectChangeRequest{
+			Owner:  owner,
+			Repo:   repo,
+			Number: pr,
+		}
+		ts.Check(sh.RejectChange(req))
+
 	case "register":
 		if len(args) != 1 {
 			ts.Fatalf("usage: shamhub register <username>")

internal/forge/shamhub/find.go

@@ -142,13 +142,26 @@ func (f *forgeRepository) FindChangeByID(ctx context.Context, fid forge.ChangeID
 		return nil, fmt.Errorf("find change by ID: %w", err)
 	}
 
+	var state forge.ChangeState
+	switch res.State {
+	case "open":
+		state = forge.ChangeOpen
+	case "closed":
+		if res.Merged {
+			state = forge.ChangeMerged
+		} else {
+			state = forge.ChangeClosed
+		}
+	}
+
 	return &forge.FindChangeItem{
 		ID:       ChangeID(res.Number),
 		URL:      res.URL,
 		Subject:  res.Subject,
 		HeadHash: git.Hash(res.Head.Hash),
 		BaseName: res.Base.Name,
 		Draft:    res.Draft,
+		State:    state,
 	}, nil
 }
 

internal/forge/shamhub/reject.go

@@ -0,0 +1,35 @@
+package shamhub
+
+import (
+	"fmt"
+)
+
+// RejectChangeRequest is a request to reject a change.
+type RejectChangeRequest struct {
+	Owner, Repo string
+	Number      int
+}
+
+// RejectChange closes a CR without merging it.
+func (sh *ShamHub) RejectChange(req RejectChangeRequest) error {
+	if req.Owner == "" || req.Repo == "" || req.Number == 0 {
+		return fmt.Errorf("owner, repo, and number are required")
+	}
+	sh.mu.Lock()
+	defer sh.mu.Unlock()
+
+	var changeIdx int
+	for idx, change := range sh.changes {
+		if change.Owner == req.Owner && change.Repo == req.Repo && change.Number == req.Number {
+			changeIdx = idx
+			break
+		}
+	}
+
+	if sh.changes[changeIdx].State != shamChangeOpen {
+		return fmt.Errorf("change %d is not open", req.Number)
+	}
+
+	sh.changes[changeIdx].State = shamChangeClosed
+	return nil
+}

testdata/script/README.md

@@ -132,14 +132,25 @@ shamhub merge <owner/repo> <num>
 Merges Change Request `<num>` made in the given repository
 into its default branch.
 
+### shamhub reject
+
+```
+shamhub reject <owner/repo> <num>
+```
+
+Rejects Change Request `<num>` made in the given repository.
+Closes the CR without merging.
+
 #### shamhub dump
 
 ```
 shamhub dump changes
 shamhub dump change <num>
+shamhub dump comments
 ```
 
-Dumps a JSON blob of all changes or a single change made against ShamHub.
+Dumps information about all changes, a single change, or all comments
+to stdout.
 
 #### shamhub register
 

testdata/script/branch_submit_update_pr_is_closed.txt

@@ -0,0 +1,94 @@
+# 'branch submit' gracefully handles the case
+# where a PR being updated is already closed.
+#
+# https://github.com/abhinav/git-spice/issues/374
+
+as 'Test <[email protected]>'
+at '2024-09-04T05:06:07Z'
+
+# setup
+cd repo
+git init
+git commit --allow-empty -m 'Initial commit'
+
+# set up a fake GitHub remote
+shamhub init
+shamhub new origin alice/example.git
+shamhub register alice
+git push origin main
+
+env SHAMHUB_USERNAME=alice
+gs auth login
+
+git add feat1.txt
+gs bc -m feat1
+gs bs -c
+stderr 'Created #'
+
+gs ll -a
+cmp stderr $WORK/golden/open.txt
+
+# Close the PR
+shamhub reject alice/example.git 1
+
+mv $WORK/extra/feat1-new.txt feat1.txt
+git add feat1.txt
+gs cc -m 'Update feature 1'
+
+gs branch submit --fill
+stderr 'Ignoring CR #1 as it was closed'
+
+gs ll -a
+cmp stderr $WORK/golden/resubmit.txt
+
+shamhub dump changes
+cmpenvJSON stdout $WORK/golden/changes.json
+
+-- repo/feat1.txt --
+Contents of feature 1
+
+-- extra/feat1-new.txt --
+New contents of feature 1
+
+-- golden/open.txt --
+┏━■ feat1 (#1) ◀
+┃   fabf34f feat1 (now)
+main
+-- golden/resubmit.txt --
+┏━■ feat1 (#2) ◀
+┃   03fced6 Update feature 1 (now)
+┃   fabf34f feat1 (now)
+main
+-- golden/changes.json --
+[
+  {
+    "number": 1,
+    "html_url": "$SHAMHUB_URL/alice/example/change/1",
+    "state": "closed",
+    "title": "feat1",
+    "body": "",
+    "base": {
+      "ref": "main",
+      "sha": "c649f748239dea65932c853775c6121d7cc79029"
+    },
+    "head": {
+      "ref": "feat1",
+      "sha": "03fced68a13af09f59f08474c47452dd15f3042c"
+    }
+  },
+  {
+    "number": 2,
+    "html_url": "$SHAMHUB_URL/alice/example/change/2",
+    "state": "open",
+    "title": "feat1",
+    "body": "feat1\n\nUpdate feature 1",
+    "base": {
+      "ref": "main",
+      "sha": "c649f748239dea65932c853775c6121d7cc79029"
+    },
+    "head": {
+      "ref": "feat1",
+      "sha": "03fced68a13af09f59f08474c47452dd15f3042c"
+    }
+  }
+]