fix(mr): stop retrying branch creation for an issue on every error

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/cli:jmrp-mr-for-branch-retries into main

Description

glab mr for and glab mr create --related-issue (without --source-branch) create a branch named after the issue. When that first branch creation failed, both commands tried numbered names until one call succeeded, whatever the error was, so a 403, an invalid name or a network error made them loop forever. The numbered names also skipped the sanitization of the first name: for a title such as Fix: crash in ~/.config? whose branch already exists, every numbered name was invalid. And since neither the loop nor its requests looked at the command's context, Ctrl+C did not stop it. The issue has a run of each command.

This MR moves the branch creation of both commands into one helper, mrutils.CreateUniqueBranch, which:

  • tries <name>-1, <name>-2 and so on only while GitLab answers 400 with already exists or, when a branch <name>/... exists, conflicts with existing branch hierarchy (create_service.rb#L89-94). An invalid name is refused with the same status (validate_new_service.rb#L10-13, branches.rb#L240), so the message is what tells them apart. The check reads the ErrorResponse status and message, as isAlreadyExists in glab variable import does (import.go#L263-274);
  • numbers the sanitized name, so the second try for that title is 7-fix-crash-in-config--1;
  • stops after 100 numbered names, so a server that keeps answering "already exists" cannot hang the command;
  • passes the command's context with gitlab.WithContext(ctx), so the request stops when the command is interrupted;
  • returns any other error as could not create branch "<name>": <error>, after one request.

The stray fmt.Println of glab mr for, which printed the error of every retry and a <nil> once a retry worked, goes with the loop.

There are two commits, and each passes the tests of the packages it touches: the first adds the helper and uses it in glab mr for, the second uses it in glab mr create --related-issue, the command that glab mr for's deprecation message points to.

Behavior changes:

  • When the branch cannot be created for any reason other than an existing branch taking the name, both commands now exit with the API's error. Before, they kept retrying with numbered names, so an error that lasted never let them return.
  • When the branch exists, the numbered name now has the form of the first one. For a title whose characters are valid in a ref but removed by the sanitization, such as Fix (urgent) bug, the second try used to be 7-fix-(urgent)-bug-1 and is now 7-fix-urgent-bug-1.

Related Issues

Closes #8680 (closed)

#7657 is about the same --related-issue code path (the current branch is ignored and a new branch is always created). This MR does not change that.

How has this been tested?

I added internal/commands/mr/mrutils/branch_test.go for the helper, internal/commands/mr/for/mr_for_test.go (the package had no tests) and TestNewCmdCreate_RelatedIssueBranchCreation in mr_create_test.go, all with the gitlabtesting mocks:

  • the helper: the branch is created; the name is numbered while the branch, or a branch below it, exists; an invalid name, a 403, a 409 whose message says "already exists" and an error without a response are each returned after one request; it stops after 100 numbered names;
  • each command: a 403 is returned and no merge request is created; when the branch exists, the sanitized -1 name is created and becomes the merge request's source branch.

On main at a6fd50ce the four command tests fail: the mock reports a second CreateBranch call after the 403, and the retry name is 7-fix:-crash-in-~/.config?-1 instead of 7-fix-crash-in-config--1. The helper tests need the helper, so for them I removed each condition in turn (the existing-branch check, each of the two messages it accepts, the status check, the limit, numbering from the first name, and the error return in each command) and a test failed each time.

With the change, on Go 1.27.3:

  • go test -race ./..., as tests:unit runs it, passes;
  • golangci-lint run (2.14.0, as .tool-versions pins) reports 0 issues, and golangci-lint fmt --diff shows nothing on the changed packages;
  • go run ./scripts/comment-overlap -threshold 1.0 -max-findings 0 ./cmd/... ./internal/... reports 0 findings;
  • make gen-docs and make generate leave the tree unchanged, and make check-embed and make check-args pass;
  • both commit messages and this title pass commitlint with the rules of scripts/commit-lint/lint.js.

I also built glab from main and from this branch and ran both commands against a local stand-in for the API routes they call, sending SIGINT (what Ctrl+C sends) 15 seconds after start and SIGKILL 5 seconds later:

Command Branch creation answer main This branch
glab mr for 7 403 1567 requests, killed 1 request, exit 1 with the error
glab mr create --related-issue 7 403 1682 requests, killed 1 request, exit 1 with the error
glab mr for 7, title Fix: crash in ~/.config? exists, then invalid name 887 requests, killed 7-fix-crash-in-config--1 created, merge request created
glab mr create --related-issue 7, same title exists, then invalid name 797 requests, killed 7-fix-crash-in-config--1 created, merge request created
glab mr for 7, title Fix bug exists, then created 7-fix-bug-1 created, <nil> printed 7-fix-bug-1 created, nothing extra printed
glab mr for 7 and glab mr create --related-issue 7, title Fix bug, while 7-fix-bug/wip exists branch hierarchy conflict, then created 7-fix-bug-1 created, merge request created 7-fix-bug-1 created, merge request created
glab mr for 7 and glab mr create --related-issue 7 no answer for 60 seconds still waiting after SIGINT, killed ends at SIGINT with the command execution has been interrupted.
Test output on main and with the change

On main at a6fd50ce, with only the new command tests added:

--- FAIL: TestNewCmdFor_ExistingBranchGetsSanitizedNumberedName (0.00s)
    mr_for.go:94: Unexpected call to *testing.MockBranchesServiceInterface.CreateBranch(...) because:
        expected call at .../mr_for_test.go:51 doesn't match the argument at index 1.
--- FAIL: TestNewCmdFor_BranchCreationErrorIsReturned (0.00s)
    mr_for.go:94: Unexpected call to *testing.MockBranchesServiceInterface.CreateBranch(...) because:
        expected call at .../mr_for_test.go:27 has already been called the max number of times
FAIL	gitlab.com/gitlab-org/cli/internal/commands/mr/for
--- FAIL: TestNewCmdCreate_RelatedIssueBranchCreation (0.00s)
    --- FAIL: .../returns_an_error_that_is_not_about_an_existing_branch (0.00s)
        mr_create.go:550: Unexpected call to *testing.MockBranchesServiceInterface.CreateBranch(...) because:
            expected call at .../mr_create_test.go:330 has already been called the max number of times
    --- FAIL: .../uses_a_sanitized_numbered_branch_when_the_branch_exists (0.00s)
            expected: []string{"1-fix-crash-in-config-", "1-fix-crash-in-config--1"}
            actual  : []string{"1-fix-crash-in-config-", "1-fix:-crash-in-~/.config?-1"}
FAIL	gitlab.com/gitlab-org/cli/internal/commands/mr/create

With the change:

--- PASS: TestNewCmdFor_BranchCreationErrorIsReturned (0.00s)
--- PASS: TestNewCmdFor_ExistingBranchGetsSanitizedNumberedName (0.00s)
ok  	gitlab.com/gitlab-org/cli/internal/commands/mr/for
--- PASS: TestCreateUniqueBranch (0.00s)
    --- PASS: TestCreateUniqueBranch/creates_the_branch (0.00s)
    --- PASS: TestCreateUniqueBranch/numbers_the_name_while_the_branch_exists (0.00s)
    --- PASS: TestCreateUniqueBranch/numbers_the_name_while_a_branch_below_it_exists (0.00s)
    --- PASS: TestCreateUniqueBranch/returns_an_invalid_name_error (0.00s)
    --- PASS: TestCreateUniqueBranch/returns_an_existing_branch_error_with_another_status (0.00s)
    --- PASS: TestCreateUniqueBranch/returns_a_permission_error (0.00s)
    --- PASS: TestCreateUniqueBranch/returns_an_error_without_a_response (0.00s)
--- PASS: TestCreateUniqueBranch_StopsAfterMaxBranchSuffix (0.00s)
ok  	gitlab.com/gitlab-org/cli/internal/commands/mr/mrutils
--- PASS: TestNewCmdCreate_RelatedIssueBranchCreation (0.02s)
    --- PASS: .../returns_an_error_that_is_not_about_an_existing_branch (0.02s)
    --- PASS: .../uses_a_sanitized_numbered_branch_when_the_branch_exists (0.00s)
ok  	gitlab.com/gitlab-org/cli/internal/commands/mr/create
The run against the stand-in

The stand-in answers GET .../issues/7, GET of the project, POST .../repository/branches and POST .../merge_requests as GitLab does: 403 Forbidden for a user who cannot push, 400 Branch already exists for the issue's branch, and 400 Branch name is invalid for a name containing :, ~ or ?. For the row with 7-fix-bug/wip it answers the issue's branch with 400 Failed to create branch '7-fix-bug': Branch name conflicts with existing branch hierarchy., and for the last row it holds every branch creation for 60 seconds instead. glab ran with GITLAB_HOST=127.0.0.1:<port>, GLAB_API_PROTOCOL=http, a dummy token, -R OWNER/REPO and --target-branch main (and --title and --yes for mr create), outside any repository.

On main, glab mr for 7 with the Fix: crash in ~/.config? title sent these branch names first:

7-fix-crash-in-config-
7-fix:-crash-in-~/.config?-1
7-fix:-crash-in-~/.config?-2

and printed one line per numbered retry, until SIGKILL:

POST http://127.0.0.1:18745/api/v4/projects/OWNER%2FREPO/repository/branches: 400 {message: Branch name is invalid}
POST http://127.0.0.1:18745/api/v4/projects/OWNER%2FREPO/repository/branches: 400 {message: Branch name is invalid}

With this branch, glab mr for 7 against the 403 answer exits 1 after one request with:

   ERROR

  could not create branch "7-fix-bug": POST http://127.0.0.1:18743/api/v4/projects/OWNER%2FREPO/repository/branches:
  403 {message: 403 Forbidden}.

Screenshots (if appropriate):

Not applicable: the change is in the requests the two commands send and the error they return.

I found this while maintaining gitlab-mcp-server; its upstream-bugs.md records everything done on its dependencies and sibling projects.

Merge request reports