rickub
rickub/clipublic Fork
⑂ main
Branches
feat/10-repo-rename-transfer feat/7-edit-title-and-body feat/8-run-logs-job-filter feat/staleness-notice main test/create-body-dash-stdin
Tags
v0.1.0 v0.1.0-rc.11 v0.1.0-rc.14 v0.1.0-rc.16 v0.1.0-rc.20 v0.1.0-rc.26 v0.1.0-rc.36 v0.1.0-rc.43 v0.1.0-rc.50 v0.1.0-rc.64 v0.2.0 View all branches → View all tags →
Commits
⬇ Clone ▾
git clone https://git.rickub.com/rickub/cli.git
git clone ssh://git@rickub.com/rickub/cli.git

Host key fingerprint (ed25519): SHA256:iycHnxEyq0Q7uyVpB7JlznP0G7JrTPXLYRcAU5CSLhc — verify it before your first connect.

Download ZIP

mr edit --title and a new issue edit: catch the CLI up to the PATCH routes (#7) #1

Merged@ogirardot wants to merge feat/7-edit-title-and-body into main
Merged as 8e1b39cbe92c.
11 files changed
modified README.md +6 -2
@@ -163,8 +163,10 @@ rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for r
163163 rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only]
164164 rickub pr close <number> [-R owner/repo]
165165 rickub pr reopen <number> [-R owner/repo]
166-rickub pr edit <number> [-R owner/repo] -b "text" | -b -
167- # replace the description (author or repo writer)
166+rickub pr edit <number> [-R owner/repo] [-t "title"] [-b "text" | -b -]
167+ # correct the title and/or the description in one call (author or
168+ # repo writer); a field you do not name is left untouched, and
169+ # passing neither flag is refused without sending a request
168170 rickub pr comment <number> [-R owner/repo] -b "text"
169171 # whole-MR conversation comment (any reader)
170172 rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N
@@ -191,6 +193,8 @@ rickub run watch <number> [-R owner/repo] [--interval 2s] [--timeout 30m] [--
191193 rickub issue list [-R owner/repo] [--state open|closed|all] [--page N]
192194 rickub issue view <number> [-R owner/repo]
193195 rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file]
196+rickub issue edit <number> [-R owner/repo] [-t "title"] [-b "body" | -b - < file]
197+ # same rules as `pr edit`: name a field to change it, omit it to keep it
194198 rickub issue close <number> [-R owner/repo]
195199 rickub issue reopen <number> [-R owner/repo]
196200 rickub issue comment <number> [-R owner/repo] -b "text"
@@ -163,8 +163,10 @@ rickub pr ready <number> [-R owner/repo] [--undo] # draft → ready for r
163 rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only]163 rickub pr merge <number> [-R owner/repo] [--method merge|squash|ff-only]
164 rickub pr close <number> [-R owner/repo]164 rickub pr close <number> [-R owner/repo]
165 rickub pr reopen <number> [-R owner/repo]165 rickub pr reopen <number> [-R owner/repo]
166-rickub pr edit <number> [-R owner/repo] -b "text" | -b -166+rickub pr edit <number> [-R owner/repo] [-t "title"] [-b "text" | -b -]
167- # replace the description (author or repo writer)167+ # correct the title and/or the description in one call (author or
168+ # repo writer); a field you do not name is left untouched, and
169+ # passing neither flag is refused without sending a request
168 rickub pr comment <number> [-R owner/repo] -b "text"170 rickub pr comment <number> [-R owner/repo] -b "text"
169 # whole-MR conversation comment (any reader)171 # whole-MR conversation comment (any reader)
170 rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N172 rickub pr comment <number> [-R owner/repo] -b "text" --file PATH --line N
@@ -191,6 +193,8 @@ rickub run watch <number> [-R owner/repo] [--interval 2s] [--timeout 30m] [--
191 rickub issue list [-R owner/repo] [--state open|closed|all] [--page N]193 rickub issue list [-R owner/repo] [--state open|closed|all] [--page N]
192 rickub issue view <number> [-R owner/repo]194 rickub issue view <number> [-R owner/repo]
193 rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file]195 rickub issue create [-R owner/repo] -t "title" [-b "body" | -b - < file]
196+rickub issue edit <number> [-R owner/repo] [-t "title"] [-b "body" | -b - < file]
197+ # same rules as `pr edit`: name a field to change it, omit it to keep it
194 rickub issue close <number> [-R owner/repo]198 rickub issue close <number> [-R owner/repo]
195 rickub issue reopen <number> [-R owner/repo]199 rickub issue reopen <number> [-R owner/repo]
196 rickub issue comment <number> [-R owner/repo] -b "text"200 rickub issue comment <number> [-R owner/repo] -b "text"
added cmd/edit.go +94 -0
new file mode 100644
@@ -0,0 +1,94 @@
1+package cmd
2+
3+import (
4+ "fmt"
5+ "strings"
6+
7+ "github.com/spf13/cobra"
8+)
9+
10+// --- --title/--body resolution for the `edit` commands -----------------------
11+//
12+// `mr edit` and `issue edit` both drive a PATCH whose two fields are optional
13+// and independent: a field the user did not name must be ABSENT from the request
14+// so the server leaves it alone. Sending "" instead would be a change — a blank
15+// title is refused by the API (title_required) and a blank body would silently
16+// erase a description nobody asked to erase.
17+//
18+// Both commands resolve their flags here, for the same reason cmd/body.go exists:
19+// a second interpretation of "the user didn't pass --title" is how the two halves
20+// drift apart.
21+
22+// resolveEditFields turns an edit command's --title/--body flags into the two
23+// optional halves of a patch. A nil result means "not given, leave it alone".
24+//
25+// The rules, all enforced BEFORE any request is made:
26+// - neither flag given → error, and nothing is sent (the server would answer
27+// 400 empty_update; a local refusal is faster and says what to do instead)
28+// - --title given but blank → error; a title is the one field that cannot be
29+// empty, and a CLI that forwards it just to collect the server's 400 is worse
30+// - --body given but blank → error; issue #7's criterion, inherited from #94 —
31+// an edit must never silently blank a description. The stdin PARSING is the
32+// shared resolveBody (that shared parser is what #94 was really about); only
33+// the refusal message is written here, because `mr edit`'s is a different
34+// sentence from `mr comment`'s: on an edit, the way to leave a description
35+// alone is to omit --body, not to pass an empty one.
36+func resolveEditFields(cmd *cobra.Command, title, body string) (*string, *string, error) {
37+ titleSet := cmd.Flags().Changed("title")
38+ bodySet := cmd.Flags().Changed("body")
39+ if !titleSet && !bodySet {
40+ return nil, nil, fmt.Errorf("nothing to update: pass --title and/or --body")
41+ }
42+
43+ var titleOut, bodyOut *string
44+ if titleSet {
45+ t := strings.TrimSpace(title)
46+ if t == "" {
47+ return nil, nil, fmt.Errorf("--title must not be blank: a title cannot be empty (omit --title to leave it as it is)")
48+ }
49+ titleOut = &t
50+ }
51+ if bodySet {
52+ b, err := resolveBody(cmd, body)
53+ if err != nil {
54+ return nil, nil, err
55+ }
56+ if strings.TrimSpace(b) == "" {
57+ if body == "-" {
58+ return nil, nil, fmt.Errorf("--body - read nothing from stdin: refusing to blank the description (omit --body to leave it as it is)")
59+ }
60+ return nil, nil, fmt.Errorf("--body must not be blank: refusing to blank the description (omit --body to leave it as it is)")
61+ }
62+ bodyOut = &b
63+ }
64+ return titleOut, bodyOut, nil
65+}
66+
67+// editSummary renders the success line from the state the SERVER echoed back,
68+// not from what was sent: a success line that does not prove the bytes landed is
69+// exactly what made issue #94 invisible. Only the fields the user actually
70+// patched are reported — sent and stored are the same pointers the patch carried.
71+//
72+// The echo is preferred but not trusted blindly: a response that arrives empty
73+// (Client.do returns without unmarshalling when the body is blank) or without the
74+// field would otherwise turn a successful rename into `title now ""`, which reads
75+// as "your title is now empty" — the opposite of what happened. When the echo has
76+// nothing to say, fall back to the value that was sent and mark it as such.
77+func editSummary(sentTitle, sentBody *string, storedTitle, storedBody string) string {
78+ parts := make([]string, 0, 2)
79+ if sentTitle != nil {
80+ if storedTitle != "" {
81+ parts = append(parts, fmt.Sprintf("title now %q", clean(storedTitle)))
82+ } else {
83+ parts = append(parts, fmt.Sprintf("title set to %q (not echoed back)", clean(*sentTitle)))
84+ }
85+ }
86+ if sentBody != nil {
87+ if storedBody != "" {
88+ parts = append(parts, fmt.Sprintf("description now %d bytes", len(storedBody)))
89+ } else {
90+ parts = append(parts, fmt.Sprintf("description set to %d bytes (not echoed back)", len(*sentBody)))
91+ }
92+ }
93+ return strings.Join(parts, ", ")
94+}
new file mode 100644
@@ -0,0 +1,94 @@
1+package cmd
2+
3+import (
4+ "fmt"
5+ "strings"
6+
7+ "github.com/spf13/cobra"
8+)
9+
10+// --- --title/--body resolution for the `edit` commands -----------------------
11+//
12+// `mr edit` and `issue edit` both drive a PATCH whose two fields are optional
13+// and independent: a field the user did not name must be ABSENT from the request
14+// so the server leaves it alone. Sending "" instead would be a change — a blank
15+// title is refused by the API (title_required) and a blank body would silently
16+// erase a description nobody asked to erase.
17+//
18+// Both commands resolve their flags here, for the same reason cmd/body.go exists:
19+// a second interpretation of "the user didn't pass --title" is how the two halves
20+// drift apart.
21+
22+// resolveEditFields turns an edit command's --title/--body flags into the two
23+// optional halves of a patch. A nil result means "not given, leave it alone".
24+//
25+// The rules, all enforced BEFORE any request is made:
26+// - neither flag given → error, and nothing is sent (the server would answer
27+// 400 empty_update; a local refusal is faster and says what to do instead)
28+// - --title given but blank → error; a title is the one field that cannot be
29+// empty, and a CLI that forwards it just to collect the server's 400 is worse
30+// - --body given but blank → error; issue #7's criterion, inherited from #94 —
31+// an edit must never silently blank a description. The stdin PARSING is the
32+// shared resolveBody (that shared parser is what #94 was really about); only
33+// the refusal message is written here, because `mr edit`'s is a different
34+// sentence from `mr comment`'s: on an edit, the way to leave a description
35+// alone is to omit --body, not to pass an empty one.
36+func resolveEditFields(cmd *cobra.Command, title, body string) (*string, *string, error) {
37+ titleSet := cmd.Flags().Changed("title")
38+ bodySet := cmd.Flags().Changed("body")
39+ if !titleSet && !bodySet {
40+ return nil, nil, fmt.Errorf("nothing to update: pass --title and/or --body")
41+ }
42+
43+ var titleOut, bodyOut *string
44+ if titleSet {
45+ t := strings.TrimSpace(title)
46+ if t == "" {
47+ return nil, nil, fmt.Errorf("--title must not be blank: a title cannot be empty (omit --title to leave it as it is)")
48+ }
49+ titleOut = &t
50+ }
51+ if bodySet {
52+ b, err := resolveBody(cmd, body)
53+ if err != nil {
54+ return nil, nil, err
55+ }
56+ if strings.TrimSpace(b) == "" {
57+ if body == "-" {
58+ return nil, nil, fmt.Errorf("--body - read nothing from stdin: refusing to blank the description (omit --body to leave it as it is)")
59+ }
60+ return nil, nil, fmt.Errorf("--body must not be blank: refusing to blank the description (omit --body to leave it as it is)")
61+ }
62+ bodyOut = &b
63+ }
64+ return titleOut, bodyOut, nil
65+}
66+
67+// editSummary renders the success line from the state the SERVER echoed back,
68+// not from what was sent: a success line that does not prove the bytes landed is
69+// exactly what made issue #94 invisible. Only the fields the user actually
70+// patched are reported — sent and stored are the same pointers the patch carried.
71+//
72+// The echo is preferred but not trusted blindly: a response that arrives empty
73+// (Client.do returns without unmarshalling when the body is blank) or without the
74+// field would otherwise turn a successful rename into `title now ""`, which reads
75+// as "your title is now empty" — the opposite of what happened. When the echo has
76+// nothing to say, fall back to the value that was sent and mark it as such.
77+func editSummary(sentTitle, sentBody *string, storedTitle, storedBody string) string {
78+ parts := make([]string, 0, 2)
79+ if sentTitle != nil {
80+ if storedTitle != "" {
81+ parts = append(parts, fmt.Sprintf("title now %q", clean(storedTitle)))
82+ } else {
83+ parts = append(parts, fmt.Sprintf("title set to %q (not echoed back)", clean(*sentTitle)))
84+ }
85+ }
86+ if sentBody != nil {
87+ if storedBody != "" {
88+ parts = append(parts, fmt.Sprintf("description now %d bytes", len(storedBody)))
89+ } else {
90+ parts = append(parts, fmt.Sprintf("description set to %d bytes (not echoed back)", len(*sentBody)))
91+ }
92+ }
93+ return strings.Join(parts, ", ")
94+}
added cmd/edit_test.go +277 -0
new file mode 100644
@@ -0,0 +1,277 @@
1+package cmd
2+
3+import (
4+ "encoding/json"
5+ "io"
6+ "net/http"
7+ "net/http/httptest"
8+ "strings"
9+ "testing"
10+)
11+
12+// editServer records every request it sees and answers a PATCH with the merged
13+// result, so a test can assert both what went on the wire and what the CLI
14+// printed from the response.
15+type editCall struct {
16+ Method string
17+ Path string
18+ Body string
19+}
20+
21+func editServer(t *testing.T, reply string) (*httptest.Server, *[]editCall) {
22+ t.Helper()
23+ var calls []editCall
24+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
25+ b, _ := io.ReadAll(r.Body)
26+ calls = append(calls, editCall{Method: r.Method, Path: r.URL.Path, Body: string(b)})
27+ w.Header().Set("Content-Type", "application/json")
28+ _, _ = w.Write([]byte(reply))
29+ }))
30+ t.Cleanup(srv.Close)
31+ return srv, &calls
32+}
33+
34+func editEnv(t *testing.T, srv *httptest.Server) {
35+ t.Helper()
36+ t.Setenv("XDG_CONFIG_HOME", t.TempDir())
37+ t.Setenv("RICKUB_HOST", srv.URL)
38+ t.Setenv("RICKUB_TOKEN", "rickub_pat_test")
39+}
40+
41+// TestPREditTitleOnlyLeavesBodyAlone is issue #7's first acceptance criterion:
42+// renaming a merge request must not touch its description, which on the wire
43+// means the payload carries no "body" key at all.
44+func TestPREditTitleOnlyLeavesBodyAlone(t *testing.T) {
45+ srv, calls := editServer(t, `{"number":12,"title":"new title","body":"untouched","state":"open"}`)
46+ editEnv(t, srv)
47+
48+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "new title")
49+ if err != nil {
50+ t.Fatalf("execute: %v", err)
51+ }
52+ if len(*calls) != 1 {
53+ t.Fatalf("expected exactly one request, got %d", len(*calls))
54+ }
55+ c := (*calls)[0]
56+ if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/merge-requests/12" {
57+ t.Errorf("wrong route: %s %s", c.Method, c.Path)
58+ }
59+ var payload map[string]any
60+ if err := json.Unmarshal([]byte(c.Body), &payload); err != nil {
61+ t.Fatalf("payload is not JSON: %v (%s)", err, c.Body)
62+ }
63+ if payload["title"] != "new title" {
64+ t.Errorf("title not sent: %s", c.Body)
65+ }
66+ if _, ok := payload["body"]; ok {
67+ t.Errorf("body must be absent when --body was not passed: %s", c.Body)
68+ }
69+ if !strings.Contains(out, `title now "new title"`) {
70+ t.Errorf("success line should echo the stored title:\n%s", out)
71+ }
72+ if strings.Contains(out, "description") {
73+ t.Errorf("success line should not claim the description changed:\n%s", out)
74+ }
75+}
76+
77+// TestPREditBodyFromStdin keeps `-b -` working through the shared resolver and
78+// checks the byte count comes from the server's echo (issue #94's lesson).
79+func TestPREditBodyFromStdin(t *testing.T) {
80+ srv, calls := editServer(t, `{"number":12,"title":"kept","body":"piped in","state":"open"}`)
81+ editEnv(t, srv)
82+
83+ rootCmd.SetIn(strings.NewReader("piped in\n"))
84+ defer rootCmd.SetIn(nil)
85+
86+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "-")
87+ if err != nil {
88+ t.Fatalf("execute: %v", err)
89+ }
90+ c := (*calls)[0]
91+ var payload map[string]any
92+ _ = json.Unmarshal([]byte(c.Body), &payload)
93+ if payload["body"] != "piped in" {
94+ t.Errorf("stdin body not sent verbatim: %s", c.Body)
95+ }
96+ if _, ok := payload["title"]; ok {
97+ t.Errorf("title must be absent when --title was not passed: %s", c.Body)
98+ }
99+ if !strings.Contains(out, "description now 8 bytes") {
100+ t.Errorf("success line should report the stored byte count:\n%s", out)
101+ }
102+}
103+
104+// TestPREditBothFieldsOnePatch: naming both flags corrects both in ONE call.
105+func TestPREditBothFieldsOnePatch(t *testing.T) {
106+ srv, calls := editServer(t, `{"number":12,"title":"t","body":"bb","state":"open"}`)
107+ editEnv(t, srv)
108+
109+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "t", "--body", "bb")
110+ if err != nil {
111+ t.Fatalf("execute: %v", err)
112+ }
113+ if len(*calls) != 1 {
114+ t.Fatalf("expected one PATCH, got %d requests", len(*calls))
115+ }
116+ var payload map[string]any
117+ _ = json.Unmarshal([]byte((*calls)[0].Body), &payload)
118+ if payload["title"] != "t" || payload["body"] != "bb" {
119+ t.Errorf("both fields expected: %s", (*calls)[0].Body)
120+ }
121+ if !strings.Contains(out, `title now "t"`) || !strings.Contains(out, "description now 2 bytes") {
122+ t.Errorf("success line should report both:\n%s", out)
123+ }
124+}
125+
126+// TestEditWithNoFieldFlagsSendsNothing: the refusal is LOCAL. A command that
127+// forwarded an empty patch just to collect the server's 400 empty_update would
128+// still be wrong here — the assertion that matters is zero requests.
129+func TestEditWithNoFieldFlagsSendsNothing(t *testing.T) {
130+ for _, args := range [][]string{
131+ {"mr", "edit", "12", "-R", "o/r"},
132+ {"issue", "edit", "7", "-R", "o/r"},
133+ } {
134+ srv, calls := editServer(t, `{}`)
135+ editEnv(t, srv)
136+ _, _, err := execute(t, args...)
137+ if err == nil {
138+ t.Fatalf("%v: expected an error when no field flag is given", args)
139+ }
140+ if !strings.Contains(err.Error(), "nothing to update") {
141+ t.Errorf("%v: unhelpful error: %v", args, err)
142+ }
143+ if len(*calls) != 0 {
144+ t.Errorf("%v: %d request(s) sent; nothing should be sent", args, len(*calls))
145+ }
146+ }
147+}
148+
149+// TestEditRefusesBlankFields: a blank title is refused (it is the one field that
150+// cannot be empty) and so is a blank body (issue #94) — both before any request.
151+func TestEditRefusesBlankFields(t *testing.T) {
152+ cases := [][]string{
153+ {"mr", "edit", "12", "-R", "o/r", "--title", " "},
154+ {"mr", "edit", "12", "-R", "o/r", "--body", ""},
155+ {"issue", "edit", "7", "-R", "o/r", "--title", ""},
156+ {"issue", "edit", "7", "-R", "o/r", "--body", " "},
157+ }
158+ for _, args := range cases {
159+ srv, calls := editServer(t, `{}`)
160+ editEnv(t, srv)
161+ if _, _, err := execute(t, args...); err == nil {
162+ t.Errorf("%v: expected a refusal", args)
163+ }
164+ if len(*calls) != 0 {
165+ t.Errorf("%v: %d request(s) sent; a blank field must not reach the server", args, len(*calls))
166+ }
167+ }
168+}
169+
170+// TestIssueEditPatchesIssueRoute is the issue half: same rules, the issues route.
171+func TestIssueEditPatchesIssueRoute(t *testing.T) {
172+ srv, calls := editServer(t, `{"number":7,"title":"renamed","body":"kept","state":"open"}`)
173+ editEnv(t, srv)
174+
175+ out, _, err := execute(t, "issue", "edit", "7", "-R", "o/r", "--title", "renamed")
176+ if err != nil {
177+ t.Fatalf("execute: %v", err)
178+ }
179+ c := (*calls)[0]
180+ if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/issues/7" {
181+ t.Errorf("wrong route: %s %s", c.Method, c.Path)
182+ }
183+ if !strings.Contains(out, `Updated issue #7: title now "renamed"`) {
184+ t.Errorf("unexpected success line:\n%s", out)
185+ }
186+}
187+
188+// TestEditSurfacesServerRefusalCleanly: a caller who may not touch the resource
189+// gets the server's uniform 404 as a plain CLI error.
190+func TestEditSurfacesServerRefusalCleanly(t *testing.T) {
191+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
192+ w.WriteHeader(http.StatusNotFound)
193+ _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"merge request not found"}}`))
194+ }))
195+ defer srv.Close()
196+ editEnv(t, srv)
197+
198+ _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "x")
199+ if err == nil {
200+ t.Fatal("expected an error")
201+ }
202+ if !strings.Contains(err.Error(), "merge request not found") || !strings.Contains(err.Error(), "not_found") {
203+ t.Errorf("error should surface the server's message and code: %v", err)
204+ }
205+}
206+
207+// TestEditValidatesNumberBeforeReadingStdin: a bad number must be rejected before
208+// anything drains stdin. `mr edit abc -b - <<'EOF' … EOF` used to swallow the
209+// heredoc and then report "invalid merge-request number", which is issue #94's
210+// failure mode — a body that vanishes with a plausible-looking error — reached by
211+// a different route.
212+func TestEditValidatesNumberBeforeReadingStdin(t *testing.T) {
213+ for _, args := range [][]string{
214+ {"mr", "edit", "abc", "-R", "o/r", "-b", "-"},
215+ {"issue", "edit", "0", "-R", "o/r", "-b", "-"},
216+ } {
217+ srv, calls := editServer(t, `{}`)
218+ editEnv(t, srv)
219+
220+ const piped = "a body nobody should lose\n"
221+ stdin := strings.NewReader(piped)
222+ rootCmd.SetIn(stdin)
223+
224+ _, _, err := execute(t, args...)
225+ rootCmd.SetIn(nil)
226+
227+ if err == nil || !strings.Contains(err.Error(), "invalid") {
228+ t.Errorf("%v: want an invalid-number error, got %v", args, err)
229+ }
230+ if len(*calls) != 0 {
231+ t.Errorf("%v: %d request(s) sent for an invalid number", args, len(*calls))
232+ }
233+ // The heredoc is still there to be read: nothing consumed it.
234+ rest, _ := io.ReadAll(stdin)
235+ if string(rest) != piped {
236+ t.Errorf("%v: stdin was consumed before the number was validated (%q left)", args, rest)
237+ }
238+ }
239+}
240+
241+// TestEditSummaryFallsBackToTheSentValue: an echo that arrives empty must not turn
242+// a successful rename into `title now ""`, which reads as "your title is now
243+// empty" — the opposite of what happened.
244+func TestEditSummaryFallsBackToTheSentValue(t *testing.T) {
245+ title, body := "the new title", "the new body"
246+ if got := editSummary(&title, nil, "stored title", ""); got != `title now "stored title"` {
247+ t.Errorf("echo present: got %q", got)
248+ }
249+ if got := editSummary(&title, nil, "", ""); got != `title set to "the new title" (not echoed back)` {
250+ t.Errorf("echo absent: got %q", got)
251+ }
252+ if got := editSummary(nil, &body, "", ""); got != "description set to 12 bytes (not echoed back)" {
253+ t.Errorf("body echo absent: got %q", got)
254+ }
255+ if got := editSummary(nil, nil, "x", "y"); got != "" {
256+ t.Errorf("nothing patched should report nothing: got %q", got)
257+ }
258+}
259+
260+// TestEditBlankBodyMessageTellsYouWhatToDo: `edit`'s refusal is not `comment`'s.
261+// On an edit the way to leave a description alone is to OMIT --body, and the
262+// message must say so rather than "--body is required".
263+func TestEditBlankBodyMessageTellsYouWhatToDo(t *testing.T) {
264+ srv, _ := editServer(t, `{}`)
265+ editEnv(t, srv)
266+
267+ _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "")
268+ if err == nil {
269+ t.Fatal("a blank description was accepted")
270+ }
271+ if !strings.Contains(err.Error(), "omit --body") {
272+ t.Errorf("message should point at omitting the flag: %v", err)
273+ }
274+ if strings.Contains(err.Error(), "comment") {
275+ t.Errorf("message is `comment`'s, not `edit`'s: %v", err)
276+ }
277+}
new file mode 100644
@@ -0,0 +1,277 @@
1+package cmd
2+
3+import (
4+ "encoding/json"
5+ "io"
6+ "net/http"
7+ "net/http/httptest"
8+ "strings"
9+ "testing"
10+)
11+
12+// editServer records every request it sees and answers a PATCH with the merged
13+// result, so a test can assert both what went on the wire and what the CLI
14+// printed from the response.
15+type editCall struct {
16+ Method string
17+ Path string
18+ Body string
19+}
20+
21+func editServer(t *testing.T, reply string) (*httptest.Server, *[]editCall) {
22+ t.Helper()
23+ var calls []editCall
24+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
25+ b, _ := io.ReadAll(r.Body)
26+ calls = append(calls, editCall{Method: r.Method, Path: r.URL.Path, Body: string(b)})
27+ w.Header().Set("Content-Type", "application/json")
28+ _, _ = w.Write([]byte(reply))
29+ }))
30+ t.Cleanup(srv.Close)
31+ return srv, &calls
32+}
33+
34+func editEnv(t *testing.T, srv *httptest.Server) {
35+ t.Helper()
36+ t.Setenv("XDG_CONFIG_HOME", t.TempDir())
37+ t.Setenv("RICKUB_HOST", srv.URL)
38+ t.Setenv("RICKUB_TOKEN", "rickub_pat_test")
39+}
40+
41+// TestPREditTitleOnlyLeavesBodyAlone is issue #7's first acceptance criterion:
42+// renaming a merge request must not touch its description, which on the wire
43+// means the payload carries no "body" key at all.
44+func TestPREditTitleOnlyLeavesBodyAlone(t *testing.T) {
45+ srv, calls := editServer(t, `{"number":12,"title":"new title","body":"untouched","state":"open"}`)
46+ editEnv(t, srv)
47+
48+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "new title")
49+ if err != nil {
50+ t.Fatalf("execute: %v", err)
51+ }
52+ if len(*calls) != 1 {
53+ t.Fatalf("expected exactly one request, got %d", len(*calls))
54+ }
55+ c := (*calls)[0]
56+ if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/merge-requests/12" {
57+ t.Errorf("wrong route: %s %s", c.Method, c.Path)
58+ }
59+ var payload map[string]any
60+ if err := json.Unmarshal([]byte(c.Body), &payload); err != nil {
61+ t.Fatalf("payload is not JSON: %v (%s)", err, c.Body)
62+ }
63+ if payload["title"] != "new title" {
64+ t.Errorf("title not sent: %s", c.Body)
65+ }
66+ if _, ok := payload["body"]; ok {
67+ t.Errorf("body must be absent when --body was not passed: %s", c.Body)
68+ }
69+ if !strings.Contains(out, `title now "new title"`) {
70+ t.Errorf("success line should echo the stored title:\n%s", out)
71+ }
72+ if strings.Contains(out, "description") {
73+ t.Errorf("success line should not claim the description changed:\n%s", out)
74+ }
75+}
76+
77+// TestPREditBodyFromStdin keeps `-b -` working through the shared resolver and
78+// checks the byte count comes from the server's echo (issue #94's lesson).
79+func TestPREditBodyFromStdin(t *testing.T) {
80+ srv, calls := editServer(t, `{"number":12,"title":"kept","body":"piped in","state":"open"}`)
81+ editEnv(t, srv)
82+
83+ rootCmd.SetIn(strings.NewReader("piped in\n"))
84+ defer rootCmd.SetIn(nil)
85+
86+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "-")
87+ if err != nil {
88+ t.Fatalf("execute: %v", err)
89+ }
90+ c := (*calls)[0]
91+ var payload map[string]any
92+ _ = json.Unmarshal([]byte(c.Body), &payload)
93+ if payload["body"] != "piped in" {
94+ t.Errorf("stdin body not sent verbatim: %s", c.Body)
95+ }
96+ if _, ok := payload["title"]; ok {
97+ t.Errorf("title must be absent when --title was not passed: %s", c.Body)
98+ }
99+ if !strings.Contains(out, "description now 8 bytes") {
100+ t.Errorf("success line should report the stored byte count:\n%s", out)
101+ }
102+}
103+
104+// TestPREditBothFieldsOnePatch: naming both flags corrects both in ONE call.
105+func TestPREditBothFieldsOnePatch(t *testing.T) {
106+ srv, calls := editServer(t, `{"number":12,"title":"t","body":"bb","state":"open"}`)
107+ editEnv(t, srv)
108+
109+ out, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "t", "--body", "bb")
110+ if err != nil {
111+ t.Fatalf("execute: %v", err)
112+ }
113+ if len(*calls) != 1 {
114+ t.Fatalf("expected one PATCH, got %d requests", len(*calls))
115+ }
116+ var payload map[string]any
117+ _ = json.Unmarshal([]byte((*calls)[0].Body), &payload)
118+ if payload["title"] != "t" || payload["body"] != "bb" {
119+ t.Errorf("both fields expected: %s", (*calls)[0].Body)
120+ }
121+ if !strings.Contains(out, `title now "t"`) || !strings.Contains(out, "description now 2 bytes") {
122+ t.Errorf("success line should report both:\n%s", out)
123+ }
124+}
125+
126+// TestEditWithNoFieldFlagsSendsNothing: the refusal is LOCAL. A command that
127+// forwarded an empty patch just to collect the server's 400 empty_update would
128+// still be wrong here — the assertion that matters is zero requests.
129+func TestEditWithNoFieldFlagsSendsNothing(t *testing.T) {
130+ for _, args := range [][]string{
131+ {"mr", "edit", "12", "-R", "o/r"},
132+ {"issue", "edit", "7", "-R", "o/r"},
133+ } {
134+ srv, calls := editServer(t, `{}`)
135+ editEnv(t, srv)
136+ _, _, err := execute(t, args...)
137+ if err == nil {
138+ t.Fatalf("%v: expected an error when no field flag is given", args)
139+ }
140+ if !strings.Contains(err.Error(), "nothing to update") {
141+ t.Errorf("%v: unhelpful error: %v", args, err)
142+ }
143+ if len(*calls) != 0 {
144+ t.Errorf("%v: %d request(s) sent; nothing should be sent", args, len(*calls))
145+ }
146+ }
147+}
148+
149+// TestEditRefusesBlankFields: a blank title is refused (it is the one field that
150+// cannot be empty) and so is a blank body (issue #94) — both before any request.
151+func TestEditRefusesBlankFields(t *testing.T) {
152+ cases := [][]string{
153+ {"mr", "edit", "12", "-R", "o/r", "--title", " "},
154+ {"mr", "edit", "12", "-R", "o/r", "--body", ""},
155+ {"issue", "edit", "7", "-R", "o/r", "--title", ""},
156+ {"issue", "edit", "7", "-R", "o/r", "--body", " "},
157+ }
158+ for _, args := range cases {
159+ srv, calls := editServer(t, `{}`)
160+ editEnv(t, srv)
161+ if _, _, err := execute(t, args...); err == nil {
162+ t.Errorf("%v: expected a refusal", args)
163+ }
164+ if len(*calls) != 0 {
165+ t.Errorf("%v: %d request(s) sent; a blank field must not reach the server", args, len(*calls))
166+ }
167+ }
168+}
169+
170+// TestIssueEditPatchesIssueRoute is the issue half: same rules, the issues route.
171+func TestIssueEditPatchesIssueRoute(t *testing.T) {
172+ srv, calls := editServer(t, `{"number":7,"title":"renamed","body":"kept","state":"open"}`)
173+ editEnv(t, srv)
174+
175+ out, _, err := execute(t, "issue", "edit", "7", "-R", "o/r", "--title", "renamed")
176+ if err != nil {
177+ t.Fatalf("execute: %v", err)
178+ }
179+ c := (*calls)[0]
180+ if c.Method != http.MethodPatch || c.Path != "/api/v1/repos/o/r/issues/7" {
181+ t.Errorf("wrong route: %s %s", c.Method, c.Path)
182+ }
183+ if !strings.Contains(out, `Updated issue #7: title now "renamed"`) {
184+ t.Errorf("unexpected success line:\n%s", out)
185+ }
186+}
187+
188+// TestEditSurfacesServerRefusalCleanly: a caller who may not touch the resource
189+// gets the server's uniform 404 as a plain CLI error.
190+func TestEditSurfacesServerRefusalCleanly(t *testing.T) {
191+ srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
192+ w.WriteHeader(http.StatusNotFound)
193+ _, _ = w.Write([]byte(`{"error":{"code":"not_found","message":"merge request not found"}}`))
194+ }))
195+ defer srv.Close()
196+ editEnv(t, srv)
197+
198+ _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "--title", "x")
199+ if err == nil {
200+ t.Fatal("expected an error")
201+ }
202+ if !strings.Contains(err.Error(), "merge request not found") || !strings.Contains(err.Error(), "not_found") {
203+ t.Errorf("error should surface the server's message and code: %v", err)
204+ }
205+}
206+
207+// TestEditValidatesNumberBeforeReadingStdin: a bad number must be rejected before
208+// anything drains stdin. `mr edit abc -b - <<'EOF' … EOF` used to swallow the
209+// heredoc and then report "invalid merge-request number", which is issue #94's
210+// failure mode — a body that vanishes with a plausible-looking error — reached by
211+// a different route.
212+func TestEditValidatesNumberBeforeReadingStdin(t *testing.T) {
213+ for _, args := range [][]string{
214+ {"mr", "edit", "abc", "-R", "o/r", "-b", "-"},
215+ {"issue", "edit", "0", "-R", "o/r", "-b", "-"},
216+ } {
217+ srv, calls := editServer(t, `{}`)
218+ editEnv(t, srv)
219+
220+ const piped = "a body nobody should lose\n"
221+ stdin := strings.NewReader(piped)
222+ rootCmd.SetIn(stdin)
223+
224+ _, _, err := execute(t, args...)
225+ rootCmd.SetIn(nil)
226+
227+ if err == nil || !strings.Contains(err.Error(), "invalid") {
228+ t.Errorf("%v: want an invalid-number error, got %v", args, err)
229+ }
230+ if len(*calls) != 0 {
231+ t.Errorf("%v: %d request(s) sent for an invalid number", args, len(*calls))
232+ }
233+ // The heredoc is still there to be read: nothing consumed it.
234+ rest, _ := io.ReadAll(stdin)
235+ if string(rest) != piped {
236+ t.Errorf("%v: stdin was consumed before the number was validated (%q left)", args, rest)
237+ }
238+ }
239+}
240+
241+// TestEditSummaryFallsBackToTheSentValue: an echo that arrives empty must not turn
242+// a successful rename into `title now ""`, which reads as "your title is now
243+// empty" — the opposite of what happened.
244+func TestEditSummaryFallsBackToTheSentValue(t *testing.T) {
245+ title, body := "the new title", "the new body"
246+ if got := editSummary(&title, nil, "stored title", ""); got != `title now "stored title"` {
247+ t.Errorf("echo present: got %q", got)
248+ }
249+ if got := editSummary(&title, nil, "", ""); got != `title set to "the new title" (not echoed back)` {
250+ t.Errorf("echo absent: got %q", got)
251+ }
252+ if got := editSummary(nil, &body, "", ""); got != "description set to 12 bytes (not echoed back)" {
253+ t.Errorf("body echo absent: got %q", got)
254+ }
255+ if got := editSummary(nil, nil, "x", "y"); got != "" {
256+ t.Errorf("nothing patched should report nothing: got %q", got)
257+ }
258+}
259+
260+// TestEditBlankBodyMessageTellsYouWhatToDo: `edit`'s refusal is not `comment`'s.
261+// On an edit the way to leave a description alone is to OMIT --body, and the
262+// message must say so rather than "--body is required".
263+func TestEditBlankBodyMessageTellsYouWhatToDo(t *testing.T) {
264+ srv, _ := editServer(t, `{}`)
265+ editEnv(t, srv)
266+
267+ _, _, err := execute(t, "mr", "edit", "12", "-R", "o/r", "-b", "")
268+ if err == nil {
269+ t.Fatal("a blank description was accepted")
270+ }
271+ if !strings.Contains(err.Error(), "omit --body") {
272+ t.Errorf("message should point at omitting the flag: %v", err)
273+ }
274+ if strings.Contains(err.Error(), "comment") {
275+ t.Errorf("message is `comment`'s, not `edit`'s: %v", err)
276+ }
277+}
modified cmd/issue.go +50 -1
@@ -111,6 +111,24 @@ func init() {
111111 RunE: runIssueLabelsList,
112112 }
113113
114+ editCmd := &cobra.Command{
115+ Use: "edit <number>",
116+ Short: "Correct an issue's title and/or description",
117+ Long: "Correct an issue's title (--title) and/or description (--body, or -b - to read it\n" +
118+ "from stdin).\n\n" +
119+ "Both flags are optional but at least one is required: a field you do not name is\n" +
120+ "left exactly as it was, and passing neither is refused without sending a request.\n" +
121+ "Naming both corrects both in a single call.\n\n" +
122+ "The issue's author, or anyone with write access to the repository, may edit it.",
123+ Example: " rickub issue edit 7 --title \"run logs has no --job filter\"\n" +
124+ " rickub issue edit 7 -b - <<'EOF'\n the new description\n EOF\n" +
125+ " rickub issue edit 7 --title \"…\" --body \"…\"",
126+ Args: cobra.ExactArgs(1),
127+ RunE: runIssueEdit,
128+ }
129+ editCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "new title")
130+ editCmd.Flags().StringVarP(&issueBody, "body", "b", "", "new description "+bodyFlagHelp)
131+
114132 deleteCommentCmd := &cobra.Command{
115133 Use: "delete-comment <number> <comment-id>",
116134 Aliases: []string{"retract"},
@@ -123,7 +141,7 @@ func init() {
123141 RunE: runIssueDeleteComment,
124142 }
125143
126- issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd,
144+ issueCmd.AddCommand(listCmd, viewCmd, createCmd, editCmd, closeCmd, reopenCmd, commentCmd, labelCmd,
127145 milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd)
128146 rootCmd.AddCommand(issueCmd)
129147 }
@@ -274,6 +292,37 @@ func runIssueCreate(cmd *cobra.Command, _ []string) error {
274292 return nil
275293 }
276294
295+func runIssueEdit(cmd *cobra.Command, args []string) error {
296+ // Number first, before anything reads stdin — see runPREdit.
297+ n, err := issueNumber(args[0])
298+ if err != nil {
299+ return err
300+ }
301+ // Same resolver as `mr edit`: a flag the user did not pass stays nil and is
302+ // left out of the PATCH, so the other field survives untouched.
303+ title, body, err := resolveEditFields(cmd, issueTitle, issueBody)
304+ if err != nil {
305+ return err
306+ }
307+ client, err := newClient()
308+ if err != nil {
309+ return err
310+ }
311+ owner, repo, err := resolveRepo(issueRepo)
312+ if err != nil {
313+ return err
314+ }
315+ i, err := client.UpdateIssue(cmd.Context(), owner, repo, n, api.IssuePatch{Title: title, Body: body})
316+ if err != nil {
317+ return err
318+ }
319+ if flagJSON {
320+ return printJSON(cmd.OutOrStdout(), i)
321+ }
322+ fmt.Fprintf(cmd.OutOrStdout(), "Updated issue #%d: %s\n", n, editSummary(title, body, i.Title, i.Body))
323+ return nil
324+}
325+
277326 func runIssueState(cmd *cobra.Command, args []string, state string) error {
278327 client, err := newClient()
279328 if err != nil {
@@ -111,6 +111,24 @@ func init() {
111 RunE: runIssueLabelsList,111 RunE: runIssueLabelsList,
112 }112 }
113 113
114+ editCmd := &cobra.Command{
115+ Use: "edit <number>",
116+ Short: "Correct an issue's title and/or description",
117+ Long: "Correct an issue's title (--title) and/or description (--body, or -b - to read it\n" +
118+ "from stdin).\n\n" +
119+ "Both flags are optional but at least one is required: a field you do not name is\n" +
120+ "left exactly as it was, and passing neither is refused without sending a request.\n" +
121+ "Naming both corrects both in a single call.\n\n" +
122+ "The issue's author, or anyone with write access to the repository, may edit it.",
123+ Example: " rickub issue edit 7 --title \"run logs has no --job filter\"\n" +
124+ " rickub issue edit 7 -b - <<'EOF'\n the new description\n EOF\n" +
125+ " rickub issue edit 7 --title \"…\" --body \"…\"",
126+ Args: cobra.ExactArgs(1),
127+ RunE: runIssueEdit,
128+ }
129+ editCmd.Flags().StringVarP(&issueTitle, "title", "t", "", "new title")
130+ editCmd.Flags().StringVarP(&issueBody, "body", "b", "", "new description "+bodyFlagHelp)
131+
114 deleteCommentCmd := &cobra.Command{132 deleteCommentCmd := &cobra.Command{
115 Use: "delete-comment <number> <comment-id>",133 Use: "delete-comment <number> <comment-id>",
116 Aliases: []string{"retract"},134 Aliases: []string{"retract"},
@@ -123,7 +141,7 @@ func init() {
123 RunE: runIssueDeleteComment,141 RunE: runIssueDeleteComment,
124 }142 }
125 143
126- issueCmd.AddCommand(listCmd, viewCmd, createCmd, closeCmd, reopenCmd, commentCmd, labelCmd,144+ issueCmd.AddCommand(listCmd, viewCmd, createCmd, editCmd, closeCmd, reopenCmd, commentCmd, labelCmd,
127 milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd)145 milestoneCmd, assignCmd, labelsCmd, deleteCommentCmd)
128 rootCmd.AddCommand(issueCmd)146 rootCmd.AddCommand(issueCmd)
129 }147 }
@@ -274,6 +292,37 @@ func runIssueCreate(cmd *cobra.Command, _ []string) error {
274 return nil292 return nil
275 }293 }
276 294
295+func runIssueEdit(cmd *cobra.Command, args []string) error {
296+ // Number first, before anything reads stdin — see runPREdit.
297+ n, err := issueNumber(args[0])
298+ if err != nil {
299+ return err
300+ }
301+ // Same resolver as `mr edit`: a flag the user did not pass stays nil and is
302+ // left out of the PATCH, so the other field survives untouched.
303+ title, body, err := resolveEditFields(cmd, issueTitle, issueBody)
304+ if err != nil {
305+ return err
306+ }
307+ client, err := newClient()
308+ if err != nil {
309+ return err
310+ }
311+ owner, repo, err := resolveRepo(issueRepo)
312+ if err != nil {
313+ return err
314+ }
315+ i, err := client.UpdateIssue(cmd.Context(), owner, repo, n, api.IssuePatch{Title: title, Body: body})
316+ if err != nil {
317+ return err
318+ }
319+ if flagJSON {
320+ return printJSON(cmd.OutOrStdout(), i)
321+ }
322+ fmt.Fprintf(cmd.OutOrStdout(), "Updated issue #%d: %s\n", n, editSummary(title, body, i.Title, i.Body))
323+ return nil
324+}
325+
277 func runIssueState(cmd *cobra.Command, args []string, state string) error {326 func runIssueState(cmd *cobra.Command, args []string, state string) error {
278 client, err := newClient()327 client, err := newClient()
279 if err != nil {328 if err != nil {
modified cmd/pr.go +24 -13
@@ -158,13 +158,21 @@ func init() {
158158
159159 editCmd := &cobra.Command{
160160 Use: "edit <number>",
161- Short: "Replace a merge request's description",
162- Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" +
161+ Short: "Correct a merge request's title and/or description",
162+ Long: "Correct a merge request's title (--title) and/or description (--body, or -b - to\n" +
163+ "read it from stdin).\n\n" +
164+ "Both flags are optional but at least one is required: a field you do not name is\n" +
165+ "left exactly as it was, and passing neither is refused without sending a request.\n" +
166+ "Naming both corrects both in a single call.\n\n" +
163167 "The merge request's author, or anyone with write access to the repository, may edit it.",
168+ Example: " rickub mr edit 12 --title \"fix the retry budget, not the timeout\"\n" +
169+ " rickub mr edit 12 -b - <<'EOF'\n the new description\n EOF\n" +
170+ " rickub mr edit 12 --title \"…\" --body \"…\"",
164171 Args: cobra.ExactArgs(1),
165172 RunE: runPREdit,
166173 }
167- editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required")
174+ editCmd.Flags().StringVarP(&prTitle, "title", "t", "", "new title")
175+ editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp)
168176
169177 prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd,
170178 lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd)
@@ -274,34 +282,37 @@ func prStateLabel(mr api.MergeRequest) string {
274282 }
275283
276284 func runPREdit(cmd *cobra.Command, args []string) error {
277- // Same resolver as every other --body: `-b -` reads stdin, and a blank result
278- // is refused rather than silently blanking the description.
279- body, err := requireBody(cmd, prBody)
285+ // The number is validated FIRST, before anything reads stdin: `mr edit abc -b -`
286+ // must not swallow a heredoc on its way to "invalid merge-request number". A
287+ // piped body that vanishes into a rejected command is issue #94's failure mode
288+ // with a different trigger.
289+ n, err := prNumber(args[0])
280290 if err != nil {
281291 return err
282292 }
283- client, err := newClient()
293+ // Shared with `issue edit`: a flag the user did not pass stays nil and is
294+ // left out of the PATCH, `-b -` still reads stdin, and a blank title or body
295+ // is refused here rather than after a round trip.
296+ title, body, err := resolveEditFields(cmd, prTitle, prBody)
284297 if err != nil {
285298 return err
286299 }
287- owner, repo, err := resolveRepo(prRepo)
300+ client, err := newClient()
288301 if err != nil {
289302 return err
290303 }
291- n, err := prNumber(args[0])
304+ owner, repo, err := resolveRepo(prRepo)
292305 if err != nil {
293306 return err
294307 }
295- mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body)
308+ mr, err := client.UpdateMergeRequest(cmd.Context(), owner, repo, n, api.MergeRequestPatch{Title: title, Body: body})
296309 if err != nil {
297310 return err
298311 }
299312 if flagJSON {
300313 return printJSON(cmd.OutOrStdout(), mr)
301314 }
302- // Report the stored length, not just "ok": a success line that does not prove
303- // the bytes landed is exactly what made issue #94 invisible.
304- fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body))
315+ fmt.Fprintf(cmd.OutOrStdout(), "Updated #%d: %s\n", n, editSummary(title, body, mr.Title, mr.Body))
305316 return nil
306317 }
307318
@@ -158,13 +158,21 @@ func init() {
158 158
159 editCmd := &cobra.Command{159 editCmd := &cobra.Command{
160 Use: "edit <number>",160 Use: "edit <number>",
161- Short: "Replace a merge request's description",161+ Short: "Correct a merge request's title and/or description",
162- Long: "Replace a merge request's description with --body (or -b - to read it from stdin).\n\n" +162+ Long: "Correct a merge request's title (--title) and/or description (--body, or -b - to\n" +
163+ "read it from stdin).\n\n" +
164+ "Both flags are optional but at least one is required: a field you do not name is\n" +
165+ "left exactly as it was, and passing neither is refused without sending a request.\n" +
166+ "Naming both corrects both in a single call.\n\n" +
163 "The merge request's author, or anyone with write access to the repository, may edit it.",167 "The merge request's author, or anyone with write access to the repository, may edit it.",
168+ Example: " rickub mr edit 12 --title \"fix the retry budget, not the timeout\"\n" +
169+ " rickub mr edit 12 -b - <<'EOF'\n the new description\n EOF\n" +
170+ " rickub mr edit 12 --title \"…\" --body \"…\"",
164 Args: cobra.ExactArgs(1),171 Args: cobra.ExactArgs(1),
165 RunE: runPREdit,172 RunE: runPREdit,
166 }173 }
167- editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp+"; required")174+ editCmd.Flags().StringVarP(&prTitle, "title", "t", "", "new title")
175+ editCmd.Flags().StringVarP(&prBody, "body", "b", "", "new description "+bodyFlagHelp)
168 176
169 prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd,177 prCmd.AddCommand(listCmd, viewCmd, createCmd, readyCmd, mergeCmd, closeCmd, reopenCmd, commentCmd,
170 lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd)178 lineCommentsCmd, resolveCmd, deleteCommentCmd, editCmd)
@@ -274,34 +282,37 @@ func prStateLabel(mr api.MergeRequest) string {
274 }282 }
275 283
276 func runPREdit(cmd *cobra.Command, args []string) error {284 func runPREdit(cmd *cobra.Command, args []string) error {
277- // Same resolver as every other --body: `-b -` reads stdin, and a blank result285+ // The number is validated FIRST, before anything reads stdin: `mr edit abc -b -`
278- // is refused rather than silently blanking the description.286+ // must not swallow a heredoc on its way to "invalid merge-request number". A
279- body, err := requireBody(cmd, prBody)287+ // piped body that vanishes into a rejected command is issue #94's failure mode
288+ // with a different trigger.
289+ n, err := prNumber(args[0])
280 if err != nil {290 if err != nil {
281 return err291 return err
282 }292 }
283- client, err := newClient()293+ // Shared with `issue edit`: a flag the user did not pass stays nil and is
294+ // left out of the PATCH, `-b -` still reads stdin, and a blank title or body
295+ // is refused here rather than after a round trip.
296+ title, body, err := resolveEditFields(cmd, prTitle, prBody)
284 if err != nil {297 if err != nil {
285 return err298 return err
286 }299 }
287- owner, repo, err := resolveRepo(prRepo)300+ client, err := newClient()
288 if err != nil {301 if err != nil {
289 return err302 return err
290 }303 }
291- n, err := prNumber(args[0])304+ owner, repo, err := resolveRepo(prRepo)
292 if err != nil {305 if err != nil {
293 return err306 return err
294 }307 }
295- mr, err := client.UpdateMergeRequestBody(cmd.Context(), owner, repo, n, body)308+ mr, err := client.UpdateMergeRequest(cmd.Context(), owner, repo, n, api.MergeRequestPatch{Title: title, Body: body})
296 if err != nil {309 if err != nil {
297 return err310 return err
298 }311 }
299 if flagJSON {312 if flagJSON {
300 return printJSON(cmd.OutOrStdout(), mr)313 return printJSON(cmd.OutOrStdout(), mr)
301 }314 }
302- // Report the stored length, not just "ok": a success line that does not prove315+ fmt.Fprintf(cmd.OutOrStdout(), "Updated #%d: %s\n", n, editSummary(title, body, mr.Title, mr.Body))
303- // the bytes landed is exactly what made issue #94 invisible.
304- fmt.Fprintf(cmd.OutOrStdout(), "Updated the description of #%d (%d bytes)\n", n, len(mr.Body))
305 return nil316 return nil
306 }317 }
307 318
modified cmd/pr_line_comments_test.go +5 -6
@@ -294,10 +294,9 @@ func TestPRViewShowsCommentIDs(t *testing.T) {
294294 }
295295 }
296296
297-// TestPREditSendsBodyAndReportsLength: `mr edit` posts the description to the
298-// merge-request body route (which did not exist on /api/v1 before this branch),
299-// resolves `-b -` from stdin through the shared resolver, and reports the STORED
300-// byte count rather than a bare success line.
297+// TestPREditSendsBodyAndReportsLength: `mr edit` PATCHes the description onto the
298+// merge-request route, resolves `-b -` from stdin through the shared resolver, and
299+// reports the STORED byte count rather than a bare success line.
301300 func TestPREditSendsBodyAndReportsLength(t *testing.T) {
302301 var seen captured
303302 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -325,8 +324,8 @@ func TestPREditSendsBodyAndReportsLength(t *testing.T) {
325324 if err != nil {
326325 t.Fatalf("execute: %v", err)
327326 }
328- if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" {
329- t.Fatalf("path = %q", seen.Path)
327+ if seen.Method != http.MethodPatch || seen.Path != "/api/v1/repos/rick/portal/merge-requests/12" {
328+ t.Fatalf("route = %s %q", seen.Method, seen.Path)
330329 }
331330 if seen.Body["body"] != desc {
332331 t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"])
@@ -294,10 +294,9 @@ func TestPRViewShowsCommentIDs(t *testing.T) {
294 }294 }
295 }295 }
296 296
297-// TestPREditSendsBodyAndReportsLength: `mr edit` posts the description to the297+// TestPREditSendsBodyAndReportsLength: `mr edit` PATCHes the description onto the
298-// merge-request body route (which did not exist on /api/v1 before this branch),298+// merge-request route, resolves `-b -` from stdin through the shared resolver, and
299-// resolves `-b -` from stdin through the shared resolver, and reports the STORED299+// reports the STORED byte count rather than a bare success line.
300-// byte count rather than a bare success line.
301 func TestPREditSendsBodyAndReportsLength(t *testing.T) {300 func TestPREditSendsBodyAndReportsLength(t *testing.T) {
302 var seen captured301 var seen captured
303 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {302 srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -325,8 +324,8 @@ func TestPREditSendsBodyAndReportsLength(t *testing.T) {
325 if err != nil {324 if err != nil {
326 t.Fatalf("execute: %v", err)325 t.Fatalf("execute: %v", err)
327 }326 }
328- if seen.Path != "/api/v1/repos/rick/portal/merge-requests/12/body" {327+ if seen.Method != http.MethodPatch || seen.Path != "/api/v1/repos/rick/portal/merge-requests/12" {
329- t.Fatalf("path = %q", seen.Path)328+ t.Fatalf("route = %s %q", seen.Method, seen.Path)
330 }329 }
331 if seen.Body["body"] != desc {330 if seen.Body["body"] != desc {
332 t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"])331 t.Fatalf("sent body = %q, want the stdin text", seen.Body["body"])
modified cmd/repo_test.go +18 -0
@@ -7,6 +7,9 @@ import (
77 "net/http/httptest"
88 "strings"
99 "testing"
10+
11+ "github.com/spf13/cobra"
12+ "github.com/spf13/pflag"
1013 )
1114
1215 // execute runs the root command with args against a fresh output buffer, after
@@ -39,6 +42,7 @@ func execute(t *testing.T, args ...string) (string, string, error) {
3942 authLoginScope = "all"
4043 authLoginNoBrowser = false
4144 prRepo = ""
45+ prTitle = ""
4246 prBody = ""
4347 prFile = ""
4448 prLine = 0
@@ -53,6 +57,12 @@ func execute(t *testing.T, args ...string) (string, string, error) {
5357 labelColor = ""
5458 labelNewName = ""
5559
60+ // Cobra records "the user passed this flag" on the flag object itself, and the
61+ // command tree is built once per process by init(). `edit` asks exactly that
62+ // question to decide which fields go into its PATCH, so a stale Changed from an
63+ // earlier test would make it send a field nobody passed. Clear it everywhere.
64+ resetFlagsChanged(rootCmd)
65+
5666 var out, errBuf bytes.Buffer
5767 rootCmd.SetOut(&out)
5868 rootCmd.SetErr(&errBuf)
@@ -61,6 +71,14 @@ func execute(t *testing.T, args ...string) (string, string, error) {
6171 return out.String(), errBuf.String(), err
6272 }
6373
74+// resetFlagsChanged clears the Changed bit on every flag in a command tree.
75+func resetFlagsChanged(c *cobra.Command) {
76+ c.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false })
77+ for _, sub := range c.Commands() {
78+ resetFlagsChanged(sub)
79+ }
80+}
81+
6482 func repoListServer(t *testing.T) *httptest.Server {
6583 return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
6684 if r.URL.Path != "/api/v1/users/ricktester/repos" {
@@ -7,6 +7,9 @@ import (
7 "net/http/httptest"7 "net/http/httptest"
8 "strings"8 "strings"
9 "testing"9 "testing"
10+
11+ "github.com/spf13/cobra"
12+ "github.com/spf13/pflag"
10 )13 )
11 14
12 // execute runs the root command with args against a fresh output buffer, after15 // execute runs the root command with args against a fresh output buffer, after
@@ -39,6 +42,7 @@ func execute(t *testing.T, args ...string) (string, string, error) {
39 authLoginScope = "all"42 authLoginScope = "all"
40 authLoginNoBrowser = false43 authLoginNoBrowser = false
41 prRepo = ""44 prRepo = ""
45+ prTitle = ""
42 prBody = ""46 prBody = ""
43 prFile = ""47 prFile = ""
44 prLine = 048 prLine = 0
@@ -53,6 +57,12 @@ func execute(t *testing.T, args ...string) (string, string, error) {
53 labelColor = ""57 labelColor = ""
54 labelNewName = ""58 labelNewName = ""
55 59
60+ // Cobra records "the user passed this flag" on the flag object itself, and the
61+ // command tree is built once per process by init(). `edit` asks exactly that
62+ // question to decide which fields go into its PATCH, so a stale Changed from an
63+ // earlier test would make it send a field nobody passed. Clear it everywhere.
64+ resetFlagsChanged(rootCmd)
65+
56 var out, errBuf bytes.Buffer66 var out, errBuf bytes.Buffer
57 rootCmd.SetOut(&out)67 rootCmd.SetOut(&out)
58 rootCmd.SetErr(&errBuf)68 rootCmd.SetErr(&errBuf)
@@ -61,6 +71,14 @@ func execute(t *testing.T, args ...string) (string, string, error) {
61 return out.String(), errBuf.String(), err71 return out.String(), errBuf.String(), err
62 }72 }
63 73
74+// resetFlagsChanged clears the Changed bit on every flag in a command tree.
75+func resetFlagsChanged(c *cobra.Command) {
76+ c.Flags().VisitAll(func(f *pflag.Flag) { f.Changed = false })
77+ for _, sub := range c.Commands() {
78+ resetFlagsChanged(sub)
79+ }
80+}
81+
64 func repoListServer(t *testing.T) *httptest.Server {82 func repoListServer(t *testing.T) *httptest.Server {
65 return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {83 return httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
66 if r.URL.Path != "/api/v1/users/ricktester/repos" {84 if r.URL.Path != "/api/v1/users/ricktester/repos" {
modified go.mod +2 -4
@@ -6,10 +6,8 @@ toolchain go1.26.8
66
77 require (
88 github.com/spf13/cobra v1.10.2
9+ github.com/spf13/pflag v1.0.9
910 gopkg.in/yaml.v3 v3.0.1
1011 )
1112
12-require (
13- github.com/inconshreveable/mousetrap v1.1.0 // indirect
14- github.com/spf13/pflag v1.0.9 // indirect
15-)
13+require github.com/inconshreveable/mousetrap v1.1.0 // indirect
@@ -6,10 +6,8 @@ toolchain go1.26.8
6 6
7 require (7 require (
8 github.com/spf13/cobra v1.10.28 github.com/spf13/cobra v1.10.2
9+ github.com/spf13/pflag v1.0.9
9 gopkg.in/yaml.v3 v3.0.110 gopkg.in/yaml.v3 v3.0.1
10 )11 )
11 12
12-require (13+require github.com/inconshreveable/mousetrap v1.1.0 // indirect
13- github.com/inconshreveable/mousetrap v1.1.0 // indirect
14- github.com/spf13/pflag v1.0.9 // indirect
15-)
modified internal/api/endpoints.go +21 -6
@@ -207,14 +207,16 @@ func lineCommentsPath(owner, repo string, number int) string {
207207 return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments"
208208 }
209209
210-// UpdateMergeRequestBody replaces a merge request's description. The MR's author
211-// or any write-access user may do it.
212-func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) {
210+// UpdateMergeRequest patches a merge request's title and/or description in one
211+// call. Fields left nil in the patch are not sent, so the server keeps them; a
212+// patch with neither field set is refused by the server as 400 empty_update.
213+// The MR's author or any write-access user may do it.
214+func (c *Client) UpdateMergeRequest(ctx context.Context, owner, repo string, number int, in MergeRequestPatch) (*MergeRequest, error) {
213215 var mr MergeRequest
214216 err := c.do(ctx, Request{
215- Method: http.MethodPost,
216- Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body",
217- Body: map[string]string{"body": body},
217+ Method: http.MethodPatch,
218+ Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number),
219+ Body: in,
218220 }, &mr)
219221 return &mr, err
220222 }
@@ -367,6 +369,19 @@ func (c *Client) CreateIssue(ctx context.Context, owner, repo, title, body strin
367369 return &i, err
368370 }
369371
372+// UpdateIssue patches an issue's title and/or description in one call, with the
373+// same nil-means-untouched rule as UpdateMergeRequest. The issue's author or any
374+// write-access user may do it.
375+func (c *Client) UpdateIssue(ctx context.Context, owner, repo string, number int, in IssuePatch) (*IssueDetail, error) {
376+ var i IssueDetail
377+ err := c.do(ctx, Request{
378+ Method: http.MethodPatch,
379+ Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number),
380+ Body: in,
381+ }, &i)
382+ return &i, err
383+}
384+
370385 // SetIssueState closes ("closed") or reopens ("open") an issue.
371386 func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) {
372387 var i IssueDetail
@@ -207,14 +207,16 @@ func lineCommentsPath(owner, repo string, number int) string {
207 return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments"207 return "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/line-comments"
208 }208 }
209 209
210-// UpdateMergeRequestBody replaces a merge request's description. The MR's author210+// UpdateMergeRequest patches a merge request's title and/or description in one
211-// or any write-access user may do it.211+// call. Fields left nil in the patch are not sent, so the server keeps them; a
212-func (c *Client) UpdateMergeRequestBody(ctx context.Context, owner, repo string, number int, body string) (*MergeRequest, error) {212+// patch with neither field set is refused by the server as 400 empty_update.
213+// The MR's author or any write-access user may do it.
214+func (c *Client) UpdateMergeRequest(ctx context.Context, owner, repo string, number int, in MergeRequestPatch) (*MergeRequest, error) {
213 var mr MergeRequest215 var mr MergeRequest
214 err := c.do(ctx, Request{216 err := c.do(ctx, Request{
215- Method: http.MethodPost,217+ Method: http.MethodPatch,
216- Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number) + "/body",218+ Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/merge-requests/" + itoa(number),
217- Body: map[string]string{"body": body},219+ Body: in,
218 }, &mr)220 }, &mr)
219 return &mr, err221 return &mr, err
220 }222 }
@@ -367,6 +369,19 @@ func (c *Client) CreateIssue(ctx context.Context, owner, repo, title, body strin
367 return &i, err369 return &i, err
368 }370 }
369 371
372+// UpdateIssue patches an issue's title and/or description in one call, with the
373+// same nil-means-untouched rule as UpdateMergeRequest. The issue's author or any
374+// write-access user may do it.
375+func (c *Client) UpdateIssue(ctx context.Context, owner, repo string, number int, in IssuePatch) (*IssueDetail, error) {
376+ var i IssueDetail
377+ err := c.do(ctx, Request{
378+ Method: http.MethodPatch,
379+ Path: "/repos/" + seg(owner) + "/" + seg(repo) + "/issues/" + itoa(number),
380+ Body: in,
381+ }, &i)
382+ return &i, err
383+}
384+
370 // SetIssueState closes ("closed") or reopens ("open") an issue.385 // SetIssueState closes ("closed") or reopens ("open") an issue.
371 func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) {386 func (c *Client) SetIssueState(ctx context.Context, owner, repo string, number int, state string) (*IssueDetail, error) {
372 var i IssueDetail387 var i IssueDetail
added internal/api/endpoints_patch_test.go +89 -0
new file mode 100644
@@ -0,0 +1,89 @@
1+package api
2+
3+import (
4+ "context"
5+ "errors"
6+ "strings"
7+ "testing"
8+)
9+
10+// TestUpdateMergeRequestAndIssuePatchWire pins the wire contract of the two PATCH
11+// helpers: the method, the path, and — the load-bearing part — that a field the
12+// caller left nil is ABSENT from the JSON rather than sent as "". Sending an empty
13+// title would trip the API's title_required; sending an empty body would blank a
14+// description nobody asked to blank.
15+func TestUpdateMergeRequestAndIssuePatchWire(t *testing.T) {
16+ srv, calls, responses := newIssuesServer(t)
17+ responses["PATCH /api/v1/repos/o/r/merge-requests/12"] = `{"number":12,"title":"corrected","body":"kept","state":"open"}`
18+ responses["PATCH /api/v1/repos/o/r/issues/7"] = `{"number":7,"title":"kept","body":"corrected","state":"open"}`
19+ c := New(srv.URL, "rickub_pat_x")
20+ ctx := context.Background()
21+
22+ title := "corrected"
23+ body := "new body"
24+
25+ // Title only: the JSON must carry "title" and NOT "body".
26+ mr, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title})
27+ if err != nil || mr.Title != "corrected" {
28+ t.Fatalf("UpdateMergeRequest: %+v err=%v", mr, err)
29+ }
30+ last := (*calls)[len(*calls)-1]
31+ if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/merge-requests/12" {
32+ t.Errorf("wrong route: %s %s", last.Method, last.Path)
33+ }
34+ if !strings.Contains(last.Body, `"title":"corrected"`) {
35+ t.Errorf("title missing from payload: %s", last.Body)
36+ }
37+ if strings.Contains(last.Body, `"body"`) {
38+ t.Errorf("body must not appear when it was not set: %s", last.Body)
39+ }
40+
41+ // Body only: the mirror image.
42+ iss, err := c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &body})
43+ if err != nil || iss.Body != "corrected" {
44+ t.Fatalf("UpdateIssue: %+v err=%v", iss, err)
45+ }
46+ last = (*calls)[len(*calls)-1]
47+ if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/issues/7" {
48+ t.Errorf("wrong route: %s %s", last.Method, last.Path)
49+ }
50+ if !strings.Contains(last.Body, `"body":"new body"`) {
51+ t.Errorf("body missing from payload: %s", last.Body)
52+ }
53+ if strings.Contains(last.Body, `"title"`) {
54+ t.Errorf("title must not appear when it was not set: %s", last.Body)
55+ }
56+
57+ // Both: one PATCH carries both fields.
58+ if _, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title, Body: &body}); err != nil {
59+ t.Fatalf("UpdateMergeRequest both: %v", err)
60+ }
61+ last = (*calls)[len(*calls)-1]
62+ if !strings.Contains(last.Body, `"title":"corrected"`) || !strings.Contains(last.Body, `"body":"new body"`) {
63+ t.Errorf("both fields expected in one payload: %s", last.Body)
64+ }
65+
66+ // An empty string is a deliberate value, not an omission: a caller that
67+ // really passes "" must see it on the wire (the server then answers
68+ // title_required). This guards the pointer semantics from being "fixed"
69+ // into omitempty-on-value.
70+ empty := ""
71+ _, _ = c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &empty})
72+ last = (*calls)[len(*calls)-1]
73+ if !strings.Contains(last.Body, `"body":""`) {
74+ t.Errorf("an explicit empty body must be sent, not dropped: %s", last.Body)
75+ }
76+}
77+
78+// TestUpdateSurfacesAPIError checks the server's uniform 404 (not the author, no
79+// write access) arrives as a clean *APIError rather than something opaque.
80+func TestUpdateSurfacesAPIError(t *testing.T) {
81+ srv, _, _ := newIssuesServer(t) // every unscripted route answers 404 not_found
82+ c := New(srv.URL, "rickub_pat_x")
83+ title := "nope"
84+ _, err := c.UpdateMergeRequest(context.Background(), "o", "r", 99, MergeRequestPatch{Title: &title})
85+ var apiErr *APIError
86+ if err == nil || !errors.As(err, &apiErr) || apiErr.Code != "not_found" {
87+ t.Fatalf("expected an *APIError not_found, got %v", err)
88+ }
89+}
new file mode 100644
@@ -0,0 +1,89 @@
1+package api
2+
3+import (
4+ "context"
5+ "errors"
6+ "strings"
7+ "testing"
8+)
9+
10+// TestUpdateMergeRequestAndIssuePatchWire pins the wire contract of the two PATCH
11+// helpers: the method, the path, and — the load-bearing part — that a field the
12+// caller left nil is ABSENT from the JSON rather than sent as "". Sending an empty
13+// title would trip the API's title_required; sending an empty body would blank a
14+// description nobody asked to blank.
15+func TestUpdateMergeRequestAndIssuePatchWire(t *testing.T) {
16+ srv, calls, responses := newIssuesServer(t)
17+ responses["PATCH /api/v1/repos/o/r/merge-requests/12"] = `{"number":12,"title":"corrected","body":"kept","state":"open"}`
18+ responses["PATCH /api/v1/repos/o/r/issues/7"] = `{"number":7,"title":"kept","body":"corrected","state":"open"}`
19+ c := New(srv.URL, "rickub_pat_x")
20+ ctx := context.Background()
21+
22+ title := "corrected"
23+ body := "new body"
24+
25+ // Title only: the JSON must carry "title" and NOT "body".
26+ mr, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title})
27+ if err != nil || mr.Title != "corrected" {
28+ t.Fatalf("UpdateMergeRequest: %+v err=%v", mr, err)
29+ }
30+ last := (*calls)[len(*calls)-1]
31+ if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/merge-requests/12" {
32+ t.Errorf("wrong route: %s %s", last.Method, last.Path)
33+ }
34+ if !strings.Contains(last.Body, `"title":"corrected"`) {
35+ t.Errorf("title missing from payload: %s", last.Body)
36+ }
37+ if strings.Contains(last.Body, `"body"`) {
38+ t.Errorf("body must not appear when it was not set: %s", last.Body)
39+ }
40+
41+ // Body only: the mirror image.
42+ iss, err := c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &body})
43+ if err != nil || iss.Body != "corrected" {
44+ t.Fatalf("UpdateIssue: %+v err=%v", iss, err)
45+ }
46+ last = (*calls)[len(*calls)-1]
47+ if last.Method != "PATCH" || last.Path != "/api/v1/repos/o/r/issues/7" {
48+ t.Errorf("wrong route: %s %s", last.Method, last.Path)
49+ }
50+ if !strings.Contains(last.Body, `"body":"new body"`) {
51+ t.Errorf("body missing from payload: %s", last.Body)
52+ }
53+ if strings.Contains(last.Body, `"title"`) {
54+ t.Errorf("title must not appear when it was not set: %s", last.Body)
55+ }
56+
57+ // Both: one PATCH carries both fields.
58+ if _, err := c.UpdateMergeRequest(ctx, "o", "r", 12, MergeRequestPatch{Title: &title, Body: &body}); err != nil {
59+ t.Fatalf("UpdateMergeRequest both: %v", err)
60+ }
61+ last = (*calls)[len(*calls)-1]
62+ if !strings.Contains(last.Body, `"title":"corrected"`) || !strings.Contains(last.Body, `"body":"new body"`) {
63+ t.Errorf("both fields expected in one payload: %s", last.Body)
64+ }
65+
66+ // An empty string is a deliberate value, not an omission: a caller that
67+ // really passes "" must see it on the wire (the server then answers
68+ // title_required). This guards the pointer semantics from being "fixed"
69+ // into omitempty-on-value.
70+ empty := ""
71+ _, _ = c.UpdateIssue(ctx, "o", "r", 7, IssuePatch{Body: &empty})
72+ last = (*calls)[len(*calls)-1]
73+ if !strings.Contains(last.Body, `"body":""`) {
74+ t.Errorf("an explicit empty body must be sent, not dropped: %s", last.Body)
75+ }
76+}
77+
78+// TestUpdateSurfacesAPIError checks the server's uniform 404 (not the author, no
79+// write access) arrives as a clean *APIError rather than something opaque.
80+func TestUpdateSurfacesAPIError(t *testing.T) {
81+ srv, _, _ := newIssuesServer(t) // every unscripted route answers 404 not_found
82+ c := New(srv.URL, "rickub_pat_x")
83+ title := "nope"
84+ _, err := c.UpdateMergeRequest(context.Background(), "o", "r", 99, MergeRequestPatch{Title: &title})
85+ var apiErr *APIError
86+ if err == nil || !errors.As(err, &apiErr) || apiErr.Code != "not_found" {
87+ t.Fatalf("expected an *APIError not_found, got %v", err)
88+ }
89+}
modified internal/api/models.go +20 -0
@@ -91,6 +91,26 @@ type MergeRequestCreate struct {
9191 Draft *bool `json:"draft,omitempty"`
9292 }
9393
94+// MergeRequestPatch is the PATCH-a-merge-request request body (title and/or
95+// description). Both fields are POINTERS, and that is the load-bearing detail:
96+// a field the caller did not set is absent from the JSON entirely, so the server
97+// leaves it as it was. Sending "" instead would be a *change* — an empty title
98+// trips the API's title_required, and an empty body would blank a description
99+// the user never meant to touch.
100+//
101+// The server refuses a patch carrying neither field (400 empty_update); the CLI
102+// refuses it earlier, before a request is made.
103+type MergeRequestPatch struct {
104+ Title *string `json:"title,omitempty"`
105+ Body *string `json:"body,omitempty"`
106+}
107+
108+// IssuePatch is MergeRequestPatch's twin for an issue, with the same rules.
109+type IssuePatch struct {
110+ Title *string `json:"title,omitempty"`
111+ Body *string `json:"body,omitempty"`
112+}
113+
94114 // Comment is a conversation comment on an MR or an issue.
95115 //
96116 // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment`
@@ -91,6 +91,26 @@ type MergeRequestCreate struct {
91 Draft *bool `json:"draft,omitempty"`91 Draft *bool `json:"draft,omitempty"`
92 }92 }
93 93
94+// MergeRequestPatch is the PATCH-a-merge-request request body (title and/or
95+// description). Both fields are POINTERS, and that is the load-bearing detail:
96+// a field the caller did not set is absent from the JSON entirely, so the server
97+// leaves it as it was. Sending "" instead would be a *change* — an empty title
98+// trips the API's title_required, and an empty body would blank a description
99+// the user never meant to touch.
100+//
101+// The server refuses a patch carrying neither field (400 empty_update); the CLI
102+// refuses it earlier, before a request is made.
103+type MergeRequestPatch struct {
104+ Title *string `json:"title,omitempty"`
105+ Body *string `json:"body,omitempty"`
106+}
107+
108+// IssuePatch is MergeRequestPatch's twin for an issue, with the same rules.
109+type IssuePatch struct {
110+ Title *string `json:"title,omitempty"`
111+ Body *string `json:"body,omitempty"`
112+}
113+
94 // Comment is a conversation comment on an MR or an issue.114 // Comment is a conversation comment on an MR or an issue.
95 //115 //
96 // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment`116 // ID is what makes it addressable — `mr delete-comment` / `issue delete-comment`