Skip to content

Commit 4ea9076

Browse files
BagToadBagToad
authored
Merge pull request cli#12651 from gunadhya/fix-repeating-cmd-gh-issue-develop
`issue develop`: prevent .git/config corruption on repeated `--name` invocation
2 parents 39874b7 + ee8014a commit 4ea9076

2 files changed

Lines changed: 283 additions & 18 deletions

File tree

‎pkg/cmd/issue/develop/develop.go‎

Lines changed: 94 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@ import (
44
ctx "context"
55
"fmt"
66
"net/http"
7+
"net/url"
8+
"strings"
79

810
"github.com/MakeNowJust/heredoc"
911
"github.com/cli/cli/v2/api"
@@ -150,21 +152,22 @@ func developRun(opts *DevelopOptions) error {
150152
return err
151153
}
152154

153-
opts.IO.StartProgressIndicator()
155+
opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching issue #%d", opts.IssueNumber))
156+
defer opts.IO.StopProgressIndicator()
157+
154158
issue, err := shared.FindIssueOrPR(httpClient, baseRepo, opts.IssueNumber, []string{"id", "number"})
155-
opts.IO.StopProgressIndicator()
156159
if err != nil {
157160
return err
158161
}
159162

160163
apiClient := api.NewClientFromHTTP(httpClient)
161164

162-
opts.IO.StartProgressIndicator()
165+
opts.IO.StartProgressIndicatorWithLabel("Checking linked branch support")
163166
err = api.CheckLinkedBranchFeature(apiClient, baseRepo.RepoHost())
164-
opts.IO.StopProgressIndicator()
165167
if err != nil {
166168
return err
167169
}
170+
opts.IO.StopProgressIndicator()
168171

169172
if opts.List {
170173
return developRunList(opts, apiClient, baseRepo, issue)
@@ -174,7 +177,6 @@ func developRun(opts *DevelopOptions) error {
174177

175178
func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghrepo.Interface, issue *api.Issue) error {
176179
branchRepo := issueRepo
177-
var repoID string
178180
if opts.BranchRepo != "" {
179181
var err error
180182
branchRepo, err = ghrepo.FromFullName(opts.BranchRepo)
@@ -183,24 +185,67 @@ func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghr
183185
}
184186
}
185187

186-
opts.IO.StartProgressIndicator()
187-
repoID, branchID, err := api.FindRepoBranchID(apiClient, branchRepo, opts.BaseBranch)
188-
opts.IO.StopProgressIndicator()
189-
if err != nil {
190-
return err
188+
opts.IO.StartProgressIndicatorWithLabel("Preparing linked branch")
189+
defer opts.IO.StopProgressIndicator()
190+
191+
branchName := ""
192+
reusedExisting := false
193+
if opts.Name != "" {
194+
opts.IO.StartProgressIndicatorWithLabel("Checking existing linked branches")
195+
branches, err := api.ListLinkedBranches(apiClient, issueRepo, issue.Number)
196+
if err != nil {
197+
return err
198+
}
199+
branchName = findExistingLinkedBranchName(branches, branchRepo, opts.Name)
200+
reusedExisting = branchName != ""
201+
}
202+
203+
repoID := ""
204+
branchID := ""
205+
baseValidated := false
206+
if opts.BaseBranch != "" {
207+
opts.IO.StartProgressIndicatorWithLabel(fmt.Sprintf("Validating base branch %q", opts.BaseBranch))
208+
foundRepoID, foundBranchID, err := api.FindRepoBranchID(apiClient, branchRepo, opts.BaseBranch)
209+
if err != nil {
210+
return err
211+
}
212+
repoID = foundRepoID
213+
branchID = foundBranchID
214+
baseValidated = true
215+
}
216+
217+
if branchName == "" {
218+
if !baseValidated {
219+
opts.IO.StartProgressIndicatorWithLabel("Resolving base branch")
220+
foundRepoID, foundBranchID, err := api.FindRepoBranchID(apiClient, branchRepo, opts.BaseBranch)
221+
if err != nil {
222+
return err
223+
}
224+
repoID = foundRepoID
225+
branchID = foundBranchID
226+
}
227+
228+
opts.IO.StartProgressIndicatorWithLabel("Creating linked branch")
229+
createdBranchName, err := api.CreateLinkedBranch(apiClient, branchRepo.RepoHost(), repoID, issue.ID, branchID, opts.Name)
230+
if err != nil {
231+
return err
232+
}
233+
branchName = createdBranchName
234+
}
235+
236+
if branchName == "" {
237+
return fmt.Errorf("failed to create linked branch: API returned empty branch name")
191238
}
192239

193-
opts.IO.StartProgressIndicator()
194-
branchName, err := api.CreateLinkedBranch(apiClient, branchRepo.RepoHost(), repoID, issue.ID, branchID, opts.Name)
195240
opts.IO.StopProgressIndicator()
196-
if err != nil {
197-
return err
241+
242+
if reusedExisting && opts.IO.IsStdoutTTY() {
243+
fmt.Fprintf(opts.IO.ErrOut, "Using existing linked branch %q\n", branchName)
198244
}
199245

200246
// Remember which branch to target when creating a PR.
201247
if opts.BaseBranch != "" {
202-
err = opts.GitClient.SetBranchConfig(ctx.Background(), branchName, git.MergeBaseConfig, opts.BaseBranch)
203-
if err != nil {
248+
if err := opts.GitClient.SetBranchConfig(ctx.Background(), branchName, git.MergeBaseConfig, opts.BaseBranch); err != nil {
204249
return err
205250
}
206251
}
@@ -210,13 +255,44 @@ func developRunCreate(opts *DevelopOptions, apiClient *api.Client, issueRepo ghr
210255
return checkoutBranch(opts, branchRepo, branchName)
211256
}
212257

258+
func findExistingLinkedBranchName(branches []api.LinkedBranch, branchRepo ghrepo.Interface, branchName string) string {
259+
for _, branch := range branches {
260+
if branch.BranchName != branchName {
261+
continue
262+
}
263+
linkedRepo, err := linkedBranchRepoFromURL(branch.URL)
264+
if err != nil {
265+
continue
266+
}
267+
if ghrepo.IsSame(linkedRepo, branchRepo) {
268+
return branch.BranchName
269+
}
270+
}
271+
return ""
272+
}
273+
274+
func linkedBranchRepoFromURL(branchURL string) (ghrepo.Interface, error) {
275+
u, err := url.Parse(branchURL)
276+
if err != nil {
277+
return nil, err
278+
}
279+
pathParts := strings.SplitN(strings.Trim(u.Path, "/"), "/", 3)
280+
if len(pathParts) < 2 {
281+
return nil, fmt.Errorf("invalid linked branch URL: %q", branchURL)
282+
}
283+
u.Path = "/" + strings.Join(pathParts[0:2], "/")
284+
return ghrepo.FromURL(u)
285+
}
286+
213287
func developRunList(opts *DevelopOptions, apiClient *api.Client, issueRepo ghrepo.Interface, issue *api.Issue) error {
214-
opts.IO.StartProgressIndicator()
288+
opts.IO.StartProgressIndicatorWithLabel("Fetching linked branches")
289+
defer opts.IO.StopProgressIndicator()
290+
215291
branches, err := api.ListLinkedBranches(apiClient, issueRepo, issue.Number)
216-
opts.IO.StopProgressIndicator()
217292
if err != nil {
218293
return err
219294
}
295+
opts.IO.StopProgressIndicator()
220296

221297
if len(branches) == 0 {
222298
return cmdutil.NewNoResultsError(fmt.Sprintf("no linked branches found for %s#%d", ghrepo.FullName(issueRepo), issue.Number))

‎pkg/cmd/issue/develop/develop_test.go‎

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,16 @@ func TestDevelopRun(t *testing.T) {
353353
reg.Register(
354354
httpmock.GraphQL(`query FindRepoBranchID\b`),
355355
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`))
356+
reg.Register(
357+
httpmock.GraphQL(`query ListLinkedBranches\b`),
358+
httpmock.GraphQLQuery(`
359+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[]}}}}}
360+
`, func(query string, inputs map[string]interface{}) {
361+
assert.Equal(t, float64(123), inputs["number"])
362+
assert.Equal(t, "OWNER", inputs["owner"])
363+
assert.Equal(t, "REPO", inputs["name"])
364+
}),
365+
)
356366
reg.Register(
357367
httpmock.GraphQL(`mutation CreateLinkedBranch\b`),
358368
httpmock.GraphQLMutation(`{"data":{"createLinkedBranch":{"linkedBranch":{"id":"2","ref":{"name":"my-branch"}}}}}`,
@@ -370,6 +380,165 @@ func TestDevelopRun(t *testing.T) {
370380
},
371381
expectedOut: "github.com/OWNER/REPO/tree/my-branch\n",
372382
},
383+
{
384+
name: "develop existing linked branch with name and checkout",
385+
opts: &DevelopOptions{
386+
Name: "my-branch",
387+
BaseBranch: "main",
388+
IssueNumber: 123,
389+
Checkout: true,
390+
},
391+
remotes: map[string]string{
392+
"origin": "OWNER/REPO",
393+
},
394+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
395+
reg.Register(
396+
httpmock.GraphQL(`query LinkedBranchFeature\b`),
397+
httpmock.StringResponse(featureEnabledPayload),
398+
)
399+
reg.Register(
400+
httpmock.GraphQL(`query IssueByNumber\b`),
401+
httpmock.StringResponse(`{"data":{"repository":{ "hasIssuesEnabled":true,"issue":{"id":"SOMEID","number":123,"title":"my issue"}}}}`),
402+
)
403+
reg.Register(
404+
httpmock.GraphQL(`query ListLinkedBranches\b`),
405+
httpmock.GraphQLQuery(`
406+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[{"ref":{"name":"my-branch","repository":{"url":"https://github.com/OWNER/REPO"}}}]}}}}}
407+
`, func(query string, inputs map[string]interface{}) {
408+
assert.Equal(t, float64(123), inputs["number"])
409+
assert.Equal(t, "OWNER", inputs["owner"])
410+
assert.Equal(t, "REPO", inputs["name"])
411+
}),
412+
)
413+
reg.Register(
414+
httpmock.GraphQL(`query FindRepoBranchID\b`),
415+
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`))
416+
},
417+
runStubs: func(cs *run.CommandStubber) {
418+
cs.Register(`git config branch\.my-branch\.gh-merge-base main`, 0, "")
419+
cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "")
420+
cs.Register(`git rev-parse --verify refs/heads/my-branch`, 0, "")
421+
cs.Register(`git checkout my-branch`, 0, "")
422+
cs.Register(`git pull --ff-only origin my-branch`, 0, "")
423+
},
424+
expectedOut: "github.com/OWNER/REPO/tree/my-branch\n",
425+
},
426+
{
427+
name: "develop existing linked branch with name in tty shows reuse message",
428+
opts: &DevelopOptions{
429+
Name: "my-branch",
430+
BaseBranch: "main",
431+
IssueNumber: 123,
432+
},
433+
tty: true,
434+
remotes: map[string]string{
435+
"origin": "OWNER/REPO",
436+
},
437+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
438+
reg.Register(
439+
httpmock.GraphQL(`query LinkedBranchFeature\b`),
440+
httpmock.StringResponse(featureEnabledPayload),
441+
)
442+
reg.Register(
443+
httpmock.GraphQL(`query IssueByNumber\b`),
444+
httpmock.StringResponse(`{"data":{"repository":{ "hasIssuesEnabled":true,"issue":{"id":"SOMEID","number":123,"title":"my issue"}}}}`),
445+
)
446+
reg.Register(
447+
httpmock.GraphQL(`query ListLinkedBranches\b`),
448+
httpmock.GraphQLQuery(`
449+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[{"ref":{"name":"my-branch","repository":{"url":"https://github.com/OWNER/REPO"}}}]}}}}}
450+
`, func(query string, inputs map[string]interface{}) {
451+
assert.Equal(t, float64(123), inputs["number"])
452+
assert.Equal(t, "OWNER", inputs["owner"])
453+
assert.Equal(t, "REPO", inputs["name"])
454+
}),
455+
)
456+
reg.Register(
457+
httpmock.GraphQL(`query FindRepoBranchID\b`),
458+
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`))
459+
},
460+
runStubs: func(cs *run.CommandStubber) {
461+
cs.Register(`git config branch\.my-branch\.gh-merge-base main`, 0, "")
462+
cs.Register(`git fetch origin \+refs/heads/my-branch:refs/remotes/origin/my-branch`, 0, "")
463+
},
464+
expectedOut: "github.com/OWNER/REPO/tree/my-branch\n",
465+
expectedErrOut: "Using existing linked branch \"my-branch\"\n",
466+
},
467+
{
468+
name: "develop existing linked branch with invalid base branch returns an error",
469+
opts: &DevelopOptions{
470+
Name: "my-branch",
471+
BaseBranch: "does-not-exist-branch",
472+
IssueNumber: 123,
473+
},
474+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
475+
reg.Register(
476+
httpmock.GraphQL(`query LinkedBranchFeature\b`),
477+
httpmock.StringResponse(featureEnabledPayload),
478+
)
479+
reg.Register(
480+
httpmock.GraphQL(`query IssueByNumber\b`),
481+
httpmock.StringResponse(`{"data":{"repository":{ "hasIssuesEnabled":true,"issue":{"id":"SOMEID","number":123,"title":"my issue"}}}}`),
482+
)
483+
reg.Register(
484+
httpmock.GraphQL(`query ListLinkedBranches\b`),
485+
httpmock.GraphQLQuery(`
486+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[{"ref":{"name":"my-branch","repository":{"url":"https://github.com/OWNER/REPO"}}}]}}}}}
487+
`, func(query string, inputs map[string]interface{}) {
488+
assert.Equal(t, float64(123), inputs["number"])
489+
assert.Equal(t, "OWNER", inputs["owner"])
490+
assert.Equal(t, "REPO", inputs["name"])
491+
}),
492+
)
493+
reg.Register(
494+
httpmock.GraphQL(`query FindRepoBranchID\b`),
495+
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","defaultBranchRef":{"target":{"oid":"DEFAULTOID"}},"ref":null}}}`),
496+
)
497+
},
498+
wantErr: `could not find branch "does-not-exist-branch" in OWNER/REPO`,
499+
},
500+
{
501+
name: "develop with empty linked branch name response returns an error",
502+
opts: &DevelopOptions{
503+
Name: "my-branch",
504+
BaseBranch: "main",
505+
IssueNumber: 123,
506+
},
507+
httpStubs: func(reg *httpmock.Registry, t *testing.T) {
508+
reg.Register(
509+
httpmock.GraphQL(`query LinkedBranchFeature\b`),
510+
httpmock.StringResponse(featureEnabledPayload),
511+
)
512+
reg.Register(
513+
httpmock.GraphQL(`query IssueByNumber\b`),
514+
httpmock.StringResponse(`{"data":{"repository":{ "hasIssuesEnabled":true,"issue":{"id":"SOMEID","number":123,"title":"my issue"}}}}`),
515+
)
516+
reg.Register(
517+
httpmock.GraphQL(`query ListLinkedBranches\b`),
518+
httpmock.GraphQLQuery(`
519+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[]}}}}}
520+
`, func(query string, inputs map[string]interface{}) {
521+
assert.Equal(t, float64(123), inputs["number"])
522+
assert.Equal(t, "OWNER", inputs["owner"])
523+
assert.Equal(t, "REPO", inputs["name"])
524+
}),
525+
)
526+
reg.Register(
527+
httpmock.GraphQL(`query FindRepoBranchID\b`),
528+
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`))
529+
reg.Register(
530+
httpmock.GraphQL(`mutation CreateLinkedBranch\b`),
531+
httpmock.GraphQLMutation(`{"data":{"createLinkedBranch":{"linkedBranch":{"id":"2","ref":{"name":""}}}}}`,
532+
func(inputs map[string]interface{}) {
533+
assert.Equal(t, "REPOID", inputs["repositoryId"])
534+
assert.Equal(t, "SOMEID", inputs["issueId"])
535+
assert.Equal(t, "OID", inputs["oid"])
536+
assert.Equal(t, "my-branch", inputs["name"])
537+
}),
538+
)
539+
},
540+
wantErr: "failed to create linked branch: API returned empty branch name",
541+
},
373542
{
374543
name: "develop new branch outside of local git repo",
375544
opts: &DevelopOptions{
@@ -426,6 +595,16 @@ func TestDevelopRun(t *testing.T) {
426595
httpmock.GraphQL(`query FindRepoBranchID\b`),
427596
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`),
428597
)
598+
reg.Register(
599+
httpmock.GraphQL(`query ListLinkedBranches\b`),
600+
httpmock.GraphQLQuery(`
601+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[]}}}}}
602+
`, func(query string, inputs map[string]interface{}) {
603+
assert.Equal(t, float64(123), inputs["number"])
604+
assert.Equal(t, "OWNER", inputs["owner"])
605+
assert.Equal(t, "REPO", inputs["name"])
606+
}),
607+
)
429608
reg.Register(
430609
httpmock.GraphQL(`mutation CreateLinkedBranch\b`),
431610
httpmock.GraphQLMutation(`{"data":{"createLinkedBranch":{"linkedBranch":{"id":"2","ref":{"name":"my-branch"}}}}}`,
@@ -468,6 +647,16 @@ func TestDevelopRun(t *testing.T) {
468647
httpmock.GraphQL(`query FindRepoBranchID\b`),
469648
httpmock.StringResponse(`{"data":{"repository":{"id":"REPOID","ref":{"target":{"oid":"OID"}}}}}`),
470649
)
650+
reg.Register(
651+
httpmock.GraphQL(`query ListLinkedBranches\b`),
652+
httpmock.GraphQLQuery(`
653+
{"data":{"repository":{"issue":{"linkedBranches":{"nodes":[]}}}}}
654+
`, func(query string, inputs map[string]interface{}) {
655+
assert.Equal(t, float64(123), inputs["number"])
656+
assert.Equal(t, "OWNER", inputs["owner"])
657+
assert.Equal(t, "REPO", inputs["name"])
658+
}),
659+
)
471660
reg.Register(
472661
httpmock.GraphQL(`mutation CreateLinkedBranch\b`),
473662
httpmock.GraphQLMutation(`{"data":{"createLinkedBranch":{"linkedBranch":{"id":"2","ref":{"name":"my-branch"}}}}}`,

0 commit comments

Comments
 (0)