feat(mr): delete the source branch on merge from the web and the CLI
- commit
- 4ea27ba3c2ec2048a29b656d03affce6e70dcb45
- author
- brice · · 2026-10-11 05:39 UTC
- parent
- d205ffd
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
25 files · +688 -254
M README.md +1 -1
| @@ -41,7 +41,7 @@ ermite token create <label> [--preset cli|agent|ci] [--scope repo:read …] [--r | |||
| 41 | 41 | ermite token list | revoke <id> | |
| 42 | 42 | ermite mr create <repo> --source <branch> --title … | list <repo> | show <repo> <n> [--diff] | |
| 43 | 43 | ermite mr comment <repo> <n> -m … [--path … --line … --side old|new] | resolve <repo> <n> <comment> | |
| 44 | - | ermite mr merge <repo> <n> --head <id> [--squash] | close <repo> <n> | |
| 44 | + | ermite mr merge <repo> <n> --head <id> [--squash] [--delete-branch] | close <repo> <n> | |
| 45 | 45 | ermite web login # prints a one-time sign-in link (60 s) | |
| 46 | 46 | ermite web logout --all # signs every browser out | |
| 47 | 47 | ermite signing-key show # public key and allowed_signers line of squash commits | |
| @@ -41,7 +41,7 @@ ermite token create <label> [--preset cli|agent|ci] [--scope repo:read …] [--r | |||||
| 41 | ermite token list | revoke <id> | 41 | ermite token list | revoke <id> | ||
| 42 | ermite mr create <repo> --source <branch> --title … | list <repo> | show <repo> <n> [--diff] | 42 | ermite mr create <repo> --source <branch> --title … | list <repo> | show <repo> <n> [--diff] | ||
| 43 | ermite mr comment <repo> <n> -m … [--path … --line … --side old|new] | resolve <repo> <n> <comment> | 43 | ermite mr comment <repo> <n> -m … [--path … --line … --side old|new] | resolve <repo> <n> <comment> | ||
| 44 | - | ermite mr merge <repo> <n> --head <id> [--squash] | close <repo> <n> | 44 | + | ermite mr merge <repo> <n> --head <id> [--squash] [--delete-branch] | close <repo> <n> |
| 45 | ermite web login # prints a one-time sign-in link (60 s) | 45 | ermite web login # prints a one-time sign-in link (60 s) | ||
| 46 | ermite web logout --all # signs every browser out | 46 | ermite web logout --all # signs every browser out | ||
| 47 | ermite signing-key show # public key and allowed_signers line of squash commits | 47 | ermite signing-key show # public key and allowed_signers line of squash commits | ||
M cmd/ermite/api_test.go +1 -1
| @@ -158,7 +158,7 @@ func TestAC2ReadAccess(t *testing.T) { | |||
| 158 | 158 | for _, who := range allPrincipals { | |
| 159 | 159 | resp, err := r.who[who].server.GetVersion(ctx, connect.NewRequest(&pb.GetVersionRequest{})) | |
| 160 | 160 | require.NoError(t, err, who) | |
| 161 | - | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"}, resp.Msg.GetFeatures(), who) | |
| 161 | + | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"}, resp.Msg.GetFeatures(), who) | |
| 162 | 162 | assert.NotEmpty(t, resp.Msg.GetVersion()) | |
| 163 | 163 | } | |
| 164 | 164 | }) | |
| @@ -158,7 +158,7 @@ func TestAC2ReadAccess(t *testing.T) { | |||||
| 158 | for _, who := range allPrincipals { | 158 | for _, who := range allPrincipals { | ||
| 159 | resp, err := r.who[who].server.GetVersion(ctx, connect.NewRequest(&pb.GetVersionRequest{})) | 159 | resp, err := r.who[who].server.GetVersion(ctx, connect.NewRequest(&pb.GetVersionRequest{})) | ||
| 160 | require.NoError(t, err, who) | 160 | require.NoError(t, err, who) | ||
| 161 | - | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"}, resp.Msg.GetFeatures(), who) | 161 | + | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"}, resp.Msg.GetFeatures(), who) |
| 162 | assert.NotEmpty(t, resp.Msg.GetVersion()) | 162 | assert.NotEmpty(t, resp.Msg.GetVersion()) | ||
| 163 | } | 163 | } | ||
| 164 | }) | 164 | }) | ||
M cmd/ermite/cli_e2e_test.go +1 -1
| @@ -324,7 +324,7 @@ func TestAC6CLIEndToEnd(t *testing.T) { | |||
| 324 | 324 | ver := decode[versionOut](t, res.stdout) | |
| 325 | 325 | assert.Equal(t, c.url, ver.Server.URL) | |
| 326 | 326 | assert.NotEmpty(t, ver.Server.Version) | |
| 327 | - | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"}, ver.Server.Features) | |
| 327 | + | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"}, ver.Server.Features) | |
| 328 | 328 | assert.Equal(t, []string{}, ver.Server.MissingFeatures) | |
| 329 | 329 | stdout, stderr, code = c.bin(t, secret, "version") | |
| 330 | 330 | require.Equal(t, 0, code, stderr) | |
| @@ -324,7 +324,7 @@ func TestAC6CLIEndToEnd(t *testing.T) { | |||||
| 324 | ver := decode[versionOut](t, res.stdout) | 324 | ver := decode[versionOut](t, res.stdout) | ||
| 325 | assert.Equal(t, c.url, ver.Server.URL) | 325 | assert.Equal(t, c.url, ver.Server.URL) | ||
| 326 | assert.NotEmpty(t, ver.Server.Version) | 326 | assert.NotEmpty(t, ver.Server.Version) | ||
| 327 | - | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"}, ver.Server.Features) | 327 | + | assert.ElementsMatch(t, []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"}, ver.Server.Features) |
| 328 | assert.Equal(t, []string{}, ver.Server.MissingFeatures) | 328 | assert.Equal(t, []string{}, ver.Server.MissingFeatures) | ||
| 329 | stdout, stderr, code = c.bin(t, secret, "version") | 329 | stdout, stderr, code = c.bin(t, secret, "version") | ||
| 330 | require.Equal(t, 0, code, stderr) | 330 | require.Equal(t, 0, code, stderr) | ||
M cmd/ermite/cli_mr.go +23 -4
| @@ -115,6 +115,12 @@ type jsonMergeRequest struct { | |||
| 115 | 115 | MergeMode string `json:"merge_mode"` | |
| 116 | 116 | } | |
| 117 | 117 | ||
| 118 | + | // jsonMerged is the output of `mr merge --json`. | |
| 119 | + | type jsonMerged struct { | |
| 120 | + | jsonMergeRequest | |
| 121 | + | SourceBranchDeleted bool `json:"source_branch_deleted"` | |
| 122 | + | } | |
| 123 | + | ||
| 118 | 124 | func toJSONMergeRequest(m *ermite.MergeRequest) jsonMergeRequest { | |
| 119 | 125 | return jsonMergeRequest{ | |
| 120 | 126 | Repo: m.GetRepo(), Number: m.GetNumber(), Title: m.GetTitle(), Description: m.GetDescription(), | |
| @@ -740,7 +746,7 @@ func newMRResolveCmd(env *cliEnv) *cobra.Command { | |||
| 740 | 746 | } | |
| 741 | 747 | ||
| 742 | 748 | func newMRMergeCmd(env *cliEnv) *cobra.Command { | |
| 743 | - | var squash bool | |
| 749 | + | var squash, deleteBranch bool | |
| 744 | 750 | var head string | |
| 745 | 751 | cmd := &cobra.Command{ | |
| 746 | 752 | Use: "merge <repo> <number>", | |
| @@ -753,12 +759,17 @@ func newMRMergeCmd(env *cliEnv) *cobra.Command { | |||
| 753 | 759 | "The server merges only if the source branch still points to the head you reviewed, so --head is\n" + | |
| 754 | 760 | "required: give the full id of the head that `ermite mr show <repo> <number>` printed when you\n" + | |
| 755 | 761 | "reviewed the change. If the branch moved since, the merge is refused: review again. Needs the\n" + | |
| 756 | - | "mr:merge scope." + jsonMRHelp, | |
| 762 | + | "mr:merge scope.\n\n" + | |
| 763 | + | "--delete-branch deletes the source branch after the merge. The merge is refused when the branch\n" + | |
| 764 | + | "is the default branch or used by another open merge request. If the deletion fails after the\n" + | |
| 765 | + | "merge, the merge stands, the branch is kept and a warning goes to standard error." + jsonMRHelp + | |
| 766 | + | "\n--json adds source_branch_deleted (true when the branch was deleted).", | |
| 757 | 767 | Args: cobra.ExactArgs(2), | |
| 758 | 768 | ValidArgsFunction: completeMRRepoThenNumber(env), | |
| 759 | 769 | } | |
| 760 | 770 | cmd.Flags().BoolVar(&squash, "squash", false, "squash the branch into one signed commit instead of a fast-forward") | |
| 761 | 771 | cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed, from `ermite mr show` (required)") | |
| 772 | + | cmd.Flags().BoolVar(&deleteBranch, "delete-branch", false, "delete the source branch after the merge") | |
| 762 | 773 | asJSON := addJSONFlag(cmd) | |
| 763 | 774 | cmd.RunE = func(cmd *cobra.Command, args []string) error { | |
| 764 | 775 | n, err := parseNumber(args[1]) | |
| @@ -780,17 +791,25 @@ func newMRMergeCmd(env *cliEnv) *cobra.Command { | |||
| 780 | 791 | mode = ermite.MergeMode_MERGE_MODE_SQUASH | |
| 781 | 792 | } | |
| 782 | 793 | res, err := c.MergeRequests().MergeMergeRequest(ctx, connect.NewRequest(&ermite.MergeMergeRequestRequest{ | |
| 783 | - | Repo: args[0], Number: n, Mode: mode, HeadId: head, | |
| 794 | + | Repo: args[0], Number: n, Mode: mode, HeadId: head, DeleteSourceBranch: deleteBranch, | |
| 784 | 795 | })) | |
| 785 | 796 | if err != nil { | |
| 786 | 797 | return c.Describe(err) | |
| 787 | 798 | } | |
| 788 | 799 | m := res.Msg.GetMergeRequest() | |
| 800 | + | deleted := res.Msg.GetSourceBranchDeleted() | |
| 801 | + | if deleteBranch && !deleted { | |
| 802 | + | _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "ermite: warning: the branch %s was kept (it moved after the merge, or the server cannot delete it)\n", | |
| 803 | + | clean(m.GetSourceBranch())) | |
| 804 | + | } | |
| 789 | 805 | if *asJSON { | |
| 790 | - | return writeJSON(cmd.OutOrStdout(), toJSONMergeRequest(m)) | |
| 806 | + | return writeJSON(cmd.OutOrStdout(), jsonMerged{jsonMergeRequest: toJSONMergeRequest(m), SourceBranchDeleted: deleted}) | |
| 791 | 807 | } | |
| 792 | 808 | _, err = fmt.Fprintf(cmd.OutOrStdout(), "merged %s!%d into %s (%s) at %s\n", clean(m.GetRepo()), m.GetNumber(), | |
| 793 | 809 | clean(m.GetTargetBranch()), modeName(m.GetMergeMode()), shortID(m.GetMergedCommitId())) | |
| 810 | + | if err == nil && deleted { | |
| 811 | + | _, err = fmt.Fprintf(cmd.OutOrStdout(), "deleted branch %s\n", clean(m.GetSourceBranch())) | |
| 812 | + | } | |
| 794 | 813 | return err | |
| 795 | 814 | } | |
| 796 | 815 | return cmd | |
| @@ -115,6 +115,12 @@ type jsonMergeRequest struct { | |||||
| 115 | MergeMode string `json:"merge_mode"` | 115 | MergeMode string `json:"merge_mode"` | ||
| 116 | } | 116 | } | ||
| 117 | 117 | ||||
| 118 | + | // jsonMerged is the output of `mr merge --json`. | |||
| 119 | + | type jsonMerged struct { | |||
| 120 | + | jsonMergeRequest | |||
| 121 | + | SourceBranchDeleted bool `json:"source_branch_deleted"` | |||
| 122 | + | } | |||
| 123 | + | ||||
| 118 | func toJSONMergeRequest(m *ermite.MergeRequest) jsonMergeRequest { | 124 | func toJSONMergeRequest(m *ermite.MergeRequest) jsonMergeRequest { | ||
| 119 | return jsonMergeRequest{ | 125 | return jsonMergeRequest{ | ||
| 120 | Repo: m.GetRepo(), Number: m.GetNumber(), Title: m.GetTitle(), Description: m.GetDescription(), | 126 | Repo: m.GetRepo(), Number: m.GetNumber(), Title: m.GetTitle(), Description: m.GetDescription(), | ||
| @@ -740,7 +746,7 @@ func newMRResolveCmd(env *cliEnv) *cobra.Command { | |||||
| 740 | } | 746 | } | ||
| 741 | 747 | ||||
| 742 | func newMRMergeCmd(env *cliEnv) *cobra.Command { | 748 | func newMRMergeCmd(env *cliEnv) *cobra.Command { | ||
| 743 | - | var squash bool | 749 | + | var squash, deleteBranch bool |
| 744 | var head string | 750 | var head string | ||
| 745 | cmd := &cobra.Command{ | 751 | cmd := &cobra.Command{ | ||
| 746 | Use: "merge <repo> <number>", | 752 | Use: "merge <repo> <number>", | ||
| @@ -753,12 +759,17 @@ func newMRMergeCmd(env *cliEnv) *cobra.Command { | |||||
| 753 | "The server merges only if the source branch still points to the head you reviewed, so --head is\n" + | 759 | "The server merges only if the source branch still points to the head you reviewed, so --head is\n" + | ||
| 754 | "required: give the full id of the head that `ermite mr show <repo> <number>` printed when you\n" + | 760 | "required: give the full id of the head that `ermite mr show <repo> <number>` printed when you\n" + | ||
| 755 | "reviewed the change. If the branch moved since, the merge is refused: review again. Needs the\n" + | 761 | "reviewed the change. If the branch moved since, the merge is refused: review again. Needs the\n" + | ||
| 756 | - | "mr:merge scope." + jsonMRHelp, | 762 | + | "mr:merge scope.\n\n" + |
| 763 | + | "--delete-branch deletes the source branch after the merge. The merge is refused when the branch\n" + | |||
| 764 | + | "is the default branch or used by another open merge request. If the deletion fails after the\n" + | |||
| 765 | + | "merge, the merge stands, the branch is kept and a warning goes to standard error." + jsonMRHelp + | |||
| 766 | + | "\n--json adds source_branch_deleted (true when the branch was deleted).", | |||
| 757 | Args: cobra.ExactArgs(2), | 767 | Args: cobra.ExactArgs(2), | ||
| 758 | ValidArgsFunction: completeMRRepoThenNumber(env), | 768 | ValidArgsFunction: completeMRRepoThenNumber(env), | ||
| 759 | } | 769 | } | ||
| 760 | cmd.Flags().BoolVar(&squash, "squash", false, "squash the branch into one signed commit instead of a fast-forward") | 770 | cmd.Flags().BoolVar(&squash, "squash", false, "squash the branch into one signed commit instead of a fast-forward") | ||
| 761 | cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed, from `ermite mr show` (required)") | 771 | cmd.Flags().StringVar(&head, "head", "", "full id of the head you reviewed, from `ermite mr show` (required)") | ||
| 772 | + | cmd.Flags().BoolVar(&deleteBranch, "delete-branch", false, "delete the source branch after the merge") | |||
| 762 | asJSON := addJSONFlag(cmd) | 773 | asJSON := addJSONFlag(cmd) | ||
| 763 | cmd.RunE = func(cmd *cobra.Command, args []string) error { | 774 | cmd.RunE = func(cmd *cobra.Command, args []string) error { | ||
| 764 | n, err := parseNumber(args[1]) | 775 | n, err := parseNumber(args[1]) | ||
| @@ -780,17 +791,25 @@ func newMRMergeCmd(env *cliEnv) *cobra.Command { | |||||
| 780 | mode = ermite.MergeMode_MERGE_MODE_SQUASH | 791 | mode = ermite.MergeMode_MERGE_MODE_SQUASH | ||
| 781 | } | 792 | } | ||
| 782 | res, err := c.MergeRequests().MergeMergeRequest(ctx, connect.NewRequest(&ermite.MergeMergeRequestRequest{ | 793 | res, err := c.MergeRequests().MergeMergeRequest(ctx, connect.NewRequest(&ermite.MergeMergeRequestRequest{ | ||
| 783 | - | Repo: args[0], Number: n, Mode: mode, HeadId: head, | 794 | + | Repo: args[0], Number: n, Mode: mode, HeadId: head, DeleteSourceBranch: deleteBranch, |
| 784 | })) | 795 | })) | ||
| 785 | if err != nil { | 796 | if err != nil { | ||
| 786 | return c.Describe(err) | 797 | return c.Describe(err) | ||
| 787 | } | 798 | } | ||
| 788 | m := res.Msg.GetMergeRequest() | 799 | m := res.Msg.GetMergeRequest() | ||
| 800 | + | deleted := res.Msg.GetSourceBranchDeleted() | |||
| 801 | + | if deleteBranch && !deleted { | |||
| 802 | + | _, _ = fmt.Fprintf(cmd.ErrOrStderr(), "ermite: warning: the branch %s was kept (it moved after the merge, or the server cannot delete it)\n", | |||
| 803 | + | clean(m.GetSourceBranch())) | |||
| 804 | + | } | |||
| 789 | if *asJSON { | 805 | if *asJSON { | ||
| 790 | - | return writeJSON(cmd.OutOrStdout(), toJSONMergeRequest(m)) | 806 | + | return writeJSON(cmd.OutOrStdout(), jsonMerged{jsonMergeRequest: toJSONMergeRequest(m), SourceBranchDeleted: deleted}) |
| 791 | } | 807 | } | ||
| 792 | _, err = fmt.Fprintf(cmd.OutOrStdout(), "merged %s!%d into %s (%s) at %s\n", clean(m.GetRepo()), m.GetNumber(), | 808 | _, err = fmt.Fprintf(cmd.OutOrStdout(), "merged %s!%d into %s (%s) at %s\n", clean(m.GetRepo()), m.GetNumber(), | ||
| 793 | clean(m.GetTargetBranch()), modeName(m.GetMergeMode()), shortID(m.GetMergedCommitId())) | 809 | clean(m.GetTargetBranch()), modeName(m.GetMergeMode()), shortID(m.GetMergedCommitId())) | ||
| 810 | + | if err == nil && deleted { | |||
| 811 | + | _, err = fmt.Fprintf(cmd.OutOrStdout(), "deleted branch %s\n", clean(m.GetSourceBranch())) | |||
| 812 | + | } | |||
| 794 | return err | 813 | return err | ||
| 795 | } | 814 | } | ||
| 796 | return cmd | 815 | return cmd | ||
M cmd/ermite/cli_mr_test.go +32 -0
| @@ -309,6 +309,38 @@ func TestMRMergeNeedsTheHead(t *testing.T) { | |||
| 309 | 309 | assert.NotContains(t, res.stdout, "default: the current head") | |
| 310 | 310 | } | |
| 311 | 311 | ||
| 312 | + | func TestMRMergeDeleteBranch(t *testing.T) { | |
| 313 | + | merged := proto.Clone(mrOne).(*ermite.MergeRequest) | |
| 314 | + | merged.State = ermite.MergeRequestState_MERGE_REQUEST_STATE_MERGED | |
| 315 | + | merged.MergedCommitId, merged.MergeMode = headA, ermite.MergeMode_MERGE_MODE_FAST_FORWARD | |
| 316 | + | deleted := true | |
| 317 | + | f := &fakeAPI{mergeMR: func(*ermite.MergeMergeRequestRequest) (*ermite.MergeMergeRequestResponse, error) { | |
| 318 | + | return &ermite.MergeMergeRequestResponse{MergeRequest: merged, SourceBranchDeleted: deleted}, nil | |
| 319 | + | }} | |
| 320 | + | url := f.serve(t) | |
| 321 | + | ||
| 322 | + | res := runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch").ok(t) | |
| 323 | + | assert.Equal(t, "merged alpha!1 into main (fast-forward) at "+headA[:10]+"\ndeleted branch feat/x\n", res.stdout) | |
| 324 | + | assert.Empty(t, res.stderr) | |
| 325 | + | _, reqs := f.snapshot() | |
| 326 | + | assert.True(t, reqs[0].(*ermite.MergeMergeRequestRequest).GetDeleteSourceBranch()) | |
| 327 | + | ||
| 328 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch", "--json").ok(t) | |
| 329 | + | assert.Contains(t, res.stdout, `"source_branch_deleted": true`) | |
| 330 | + | ||
| 331 | + | // The merge stands when the branch is kept: a warning, no failure. | |
| 332 | + | deleted = false | |
| 333 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch").ok(t) | |
| 334 | + | assert.Equal(t, "merged alpha!1 into main (fast-forward) at "+headA[:10]+"\n", res.stdout) | |
| 335 | + | assert.Contains(t, res.stderr, "ermite: warning: the branch feat/x was kept") | |
| 336 | + | ||
| 337 | + | // Without the flag nothing is asked and nothing is said. | |
| 338 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA).ok(t) | |
| 339 | + | assert.Empty(t, res.stderr) | |
| 340 | + | _, reqs = f.snapshot() | |
| 341 | + | assert.False(t, reqs[len(reqs)-1].(*ermite.MergeMergeRequestRequest).GetDeleteSourceBranch()) | |
| 342 | + | } | |
| 343 | + | ||
| 312 | 344 | func TestMRErrorsAreFriendly(t *testing.T) { | |
| 313 | 345 | f := &fakeAPI{ | |
| 314 | 346 | listMR: func(*ermite.ListMergeRequestsRequest) (*ermite.ListMergeRequestsResponse, error) { | |
| @@ -309,6 +309,38 @@ func TestMRMergeNeedsTheHead(t *testing.T) { | |||||
| 309 | assert.NotContains(t, res.stdout, "default: the current head") | 309 | assert.NotContains(t, res.stdout, "default: the current head") | ||
| 310 | } | 310 | } | ||
| 311 | 311 | ||||
| 312 | + | func TestMRMergeDeleteBranch(t *testing.T) { | |||
| 313 | + | merged := proto.Clone(mrOne).(*ermite.MergeRequest) | |||
| 314 | + | merged.State = ermite.MergeRequestState_MERGE_REQUEST_STATE_MERGED | |||
| 315 | + | merged.MergedCommitId, merged.MergeMode = headA, ermite.MergeMode_MERGE_MODE_FAST_FORWARD | |||
| 316 | + | deleted := true | |||
| 317 | + | f := &fakeAPI{mergeMR: func(*ermite.MergeMergeRequestRequest) (*ermite.MergeMergeRequestResponse, error) { | |||
| 318 | + | return &ermite.MergeMergeRequestResponse{MergeRequest: merged, SourceBranchDeleted: deleted}, nil | |||
| 319 | + | }} | |||
| 320 | + | url := f.serve(t) | |||
| 321 | + | ||||
| 322 | + | res := runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch").ok(t) | |||
| 323 | + | assert.Equal(t, "merged alpha!1 into main (fast-forward) at "+headA[:10]+"\ndeleted branch feat/x\n", res.stdout) | |||
| 324 | + | assert.Empty(t, res.stderr) | |||
| 325 | + | _, reqs := f.snapshot() | |||
| 326 | + | assert.True(t, reqs[0].(*ermite.MergeMergeRequestRequest).GetDeleteSourceBranch()) | |||
| 327 | + | ||||
| 328 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch", "--json").ok(t) | |||
| 329 | + | assert.Contains(t, res.stdout, `"source_branch_deleted": true`) | |||
| 330 | + | ||||
| 331 | + | // The merge stands when the branch is kept: a warning, no failure. | |||
| 332 | + | deleted = false | |||
| 333 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA, "--delete-branch").ok(t) | |||
| 334 | + | assert.Equal(t, "merged alpha!1 into main (fast-forward) at "+headA[:10]+"\n", res.stdout) | |||
| 335 | + | assert.Contains(t, res.stderr, "ermite: warning: the branch feat/x was kept") | |||
| 336 | + | ||||
| 337 | + | // Without the flag nothing is asked and nothing is said. | |||
| 338 | + | res = runCLI(t, envFor(url), "", "mr", "merge", "alpha", "1", "--head", headA).ok(t) | |||
| 339 | + | assert.Empty(t, res.stderr) | |||
| 340 | + | _, reqs = f.snapshot() | |||
| 341 | + | assert.False(t, reqs[len(reqs)-1].(*ermite.MergeMergeRequestRequest).GetDeleteSourceBranch()) | |||
| 342 | + | } | |||
| 343 | + | ||||
| 312 | func TestMRErrorsAreFriendly(t *testing.T) { | 344 | func TestMRErrorsAreFriendly(t *testing.T) { | ||
| 313 | f := &fakeAPI{ | 345 | f := &fakeAPI{ | ||
| 314 | listMR: func(*ermite.ListMergeRequestsRequest) (*ermite.ListMergeRequestsResponse, error) { | 346 | listMR: func(*ermite.ListMergeRequestsRequest) (*ermite.ListMergeRequestsResponse, error) { | ||
M cmd/ermite/cli_version.go +6 -5
| @@ -100,9 +100,10 @@ func newVersionCmd(env *cliEnv) *cobra.Command { | |||
| 100 | 100 | ||
| 101 | 101 | // featureUse says what breaks without a feature, for the version warning. | |
| 102 | 102 | var featureUse = map[string]string{ | |
| 103 | - | client.FeatureRepoUpdate: "`ermite repo edit` needs: the command will fail on this server", | |
| 104 | - | client.FeatureEventsResume: "`ermite watch` uses to resume after a reconnection: events during an outage may be missed", | |
| 105 | - | client.FeatureMergeRequests: "`ermite mr` needs: the commands will fail on this server", | |
| 106 | - | client.FeatureSigningKey: "`ermite signing-key show` needs: the command will fail on this server", | |
| 107 | - | client.FeatureSession: "`ermite web login` needs: the command will fail on this server", | |
| 103 | + | client.FeatureRepoUpdate: "`ermite repo edit` needs: the command will fail on this server", | |
| 104 | + | client.FeatureEventsResume: "`ermite watch` uses to resume after a reconnection: events during an outage may be missed", | |
| 105 | + | client.FeatureMergeRequests: "`ermite mr` needs: the commands will fail on this server", | |
| 106 | + | client.FeatureMergeDeleteBranch: "`ermite mr merge --delete-branch` needs: the merge works but the branch is kept", | |
| 107 | + | client.FeatureSigningKey: "`ermite signing-key show` needs: the command will fail on this server", | |
| 108 | + | client.FeatureSession: "`ermite web login` needs: the command will fail on this server", | |
| 108 | 109 | } | |
| @@ -100,9 +100,10 @@ func newVersionCmd(env *cliEnv) *cobra.Command { | |||||
| 100 | 100 | ||||
| 101 | // featureUse says what breaks without a feature, for the version warning. | 101 | // featureUse says what breaks without a feature, for the version warning. | ||
| 102 | var featureUse = map[string]string{ | 102 | var featureUse = map[string]string{ | ||
| 103 | - | client.FeatureRepoUpdate: "`ermite repo edit` needs: the command will fail on this server", | 103 | + | client.FeatureRepoUpdate: "`ermite repo edit` needs: the command will fail on this server", |
| 104 | - | client.FeatureEventsResume: "`ermite watch` uses to resume after a reconnection: events during an outage may be missed", | 104 | + | client.FeatureEventsResume: "`ermite watch` uses to resume after a reconnection: events during an outage may be missed", |
| 105 | - | client.FeatureMergeRequests: "`ermite mr` needs: the commands will fail on this server", | 105 | + | client.FeatureMergeRequests: "`ermite mr` needs: the commands will fail on this server", |
| 106 | - | client.FeatureSigningKey: "`ermite signing-key show` needs: the command will fail on this server", | 106 | + | client.FeatureMergeDeleteBranch: "`ermite mr merge --delete-branch` needs: the merge works but the branch is kept", |
| 107 | - | client.FeatureSession: "`ermite web login` needs: the command will fail on this server", | 107 | + | client.FeatureSigningKey: "`ermite signing-key show` needs: the command will fail on this server", |
| 108 | + | client.FeatureSession: "`ermite web login` needs: the command will fail on this server", | |||
| 108 | } | 109 | } | ||
M cmd/ermite/cli_version_test.go +5 -5
| @@ -14,7 +14,7 @@ import ( | |||
| 14 | 14 | ) | |
| 15 | 15 | ||
| 16 | 16 | // allFeatures are the features this client uses. | |
| 17 | - | var allFeatures = []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"} | |
| 17 | + | var allFeatures = []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"} | |
| 18 | 18 | ||
| 19 | 19 | func versionFake(features ...string) *fakeAPI { | |
| 20 | 20 | return &fakeAPI{getVersion: func(*ermite.GetVersionRequest) (*ermite.GetVersionResponse, error) { | |
| @@ -35,15 +35,15 @@ func TestVersionWithServer(t *testing.T) { | |||
| 35 | 35 | f := versionFake(allFeatures...) | |
| 36 | 36 | url := f.serve(t) | |
| 37 | 37 | res := runCLI(t, envFor(url), "", "version").ok(t) | |
| 38 | - | assert.Equal(t, "client ermite v0.0.1\nserver v0.4.0 at "+url+"\nfeatures repo.update, events.resume, merge_requests, signing_key, session\n", res.stdout) | |
| 38 | + | assert.Equal(t, "client ermite v0.0.1\nserver v0.4.0 at "+url+"\nfeatures repo.update, events.resume, merge_requests, merge_requests.delete_branch, signing_key, session\n", res.stdout) | |
| 39 | 39 | assert.Empty(t, res.stderr, "no warning when every feature is there") | |
| 40 | 40 | ||
| 41 | 41 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | |
| 42 | - | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","events.resume","merge_requests","signing_key","session"],"missing_features":[]}}`, res.stdout) | |
| 42 | + | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","events.resume","merge_requests","merge_requests.delete_branch","signing_key","session"],"missing_features":[]}}`, res.stdout) | |
| 43 | 43 | } | |
| 44 | 44 | ||
| 45 | 45 | func TestVersionWarnsAboutMissingFeatures(t *testing.T) { | |
| 46 | - | f := versionFake("repo.update", "merge_requests", "signing_key", "session") | |
| 46 | + | f := versionFake("repo.update", "merge_requests", "merge_requests.delete_branch", "signing_key", "session") | |
| 47 | 47 | url := f.serve(t) | |
| 48 | 48 | res := runCLI(t, envFor(url), "", "version").ok(t) | |
| 49 | 49 | assert.Contains(t, res.stdout, "v0.4.0") | |
| @@ -51,7 +51,7 @@ func TestVersionWarnsAboutMissingFeatures(t *testing.T) { | |||
| 51 | 51 | assert.NotContains(t, res.stderr, "repo.update") | |
| 52 | 52 | ||
| 53 | 53 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | |
| 54 | - | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","merge_requests","signing_key","session"],"missing_features":["events.resume"]}}`, res.stdout) | |
| 54 | + | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","merge_requests","merge_requests.delete_branch","signing_key","session"],"missing_features":["events.resume"]}}`, res.stdout) | |
| 55 | 55 | assert.Contains(t, res.stderr, "events.resume", "the warning also goes to stderr with --json") | |
| 56 | 56 | ||
| 57 | 57 | f = versionFake() | |
| @@ -14,7 +14,7 @@ import ( | |||||
| 14 | ) | 14 | ) | ||
| 15 | 15 | ||||
| 16 | // allFeatures are the features this client uses. | 16 | // allFeatures are the features this client uses. | ||
| 17 | - | var allFeatures = []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"} | 17 | + | var allFeatures = []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"} |
| 18 | 18 | ||||
| 19 | func versionFake(features ...string) *fakeAPI { | 19 | func versionFake(features ...string) *fakeAPI { | ||
| 20 | return &fakeAPI{getVersion: func(*ermite.GetVersionRequest) (*ermite.GetVersionResponse, error) { | 20 | return &fakeAPI{getVersion: func(*ermite.GetVersionRequest) (*ermite.GetVersionResponse, error) { | ||
| @@ -35,15 +35,15 @@ func TestVersionWithServer(t *testing.T) { | |||||
| 35 | f := versionFake(allFeatures...) | 35 | f := versionFake(allFeatures...) | ||
| 36 | url := f.serve(t) | 36 | url := f.serve(t) | ||
| 37 | res := runCLI(t, envFor(url), "", "version").ok(t) | 37 | res := runCLI(t, envFor(url), "", "version").ok(t) | ||
| 38 | - | assert.Equal(t, "client ermite v0.0.1\nserver v0.4.0 at "+url+"\nfeatures repo.update, events.resume, merge_requests, signing_key, session\n", res.stdout) | 38 | + | assert.Equal(t, "client ermite v0.0.1\nserver v0.4.0 at "+url+"\nfeatures repo.update, events.resume, merge_requests, merge_requests.delete_branch, signing_key, session\n", res.stdout) |
| 39 | assert.Empty(t, res.stderr, "no warning when every feature is there") | 39 | assert.Empty(t, res.stderr, "no warning when every feature is there") | ||
| 40 | 40 | ||||
| 41 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | 41 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | ||
| 42 | - | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","events.resume","merge_requests","signing_key","session"],"missing_features":[]}}`, res.stdout) | 42 | + | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","events.resume","merge_requests","merge_requests.delete_branch","signing_key","session"],"missing_features":[]}}`, res.stdout) |
| 43 | } | 43 | } | ||
| 44 | 44 | ||||
| 45 | func TestVersionWarnsAboutMissingFeatures(t *testing.T) { | 45 | func TestVersionWarnsAboutMissingFeatures(t *testing.T) { | ||
| 46 | - | f := versionFake("repo.update", "merge_requests", "signing_key", "session") | 46 | + | f := versionFake("repo.update", "merge_requests", "merge_requests.delete_branch", "signing_key", "session") |
| 47 | url := f.serve(t) | 47 | url := f.serve(t) | ||
| 48 | res := runCLI(t, envFor(url), "", "version").ok(t) | 48 | res := runCLI(t, envFor(url), "", "version").ok(t) | ||
| 49 | assert.Contains(t, res.stdout, "v0.4.0") | 49 | assert.Contains(t, res.stdout, "v0.4.0") | ||
| @@ -51,7 +51,7 @@ func TestVersionWarnsAboutMissingFeatures(t *testing.T) { | |||||
| 51 | assert.NotContains(t, res.stderr, "repo.update") | 51 | assert.NotContains(t, res.stderr, "repo.update") | ||
| 52 | 52 | ||||
| 53 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | 53 | res = runCLI(t, envFor(url), "", "version", "--json").ok(t) | ||
| 54 | - | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","merge_requests","signing_key","session"],"missing_features":["events.resume"]}}`, res.stdout) | 54 | + | assert.JSONEq(t, `{"client":{"version":"v0.0.1"},"server":{"url":"`+url+`","version":"v0.4.0","features":["repo.update","merge_requests","merge_requests.delete_branch","signing_key","session"],"missing_features":["events.resume"]}}`, res.stdout) |
| 55 | assert.Contains(t, res.stderr, "events.resume", "the warning also goes to stderr with --json") | 55 | assert.Contains(t, res.stderr, "events.resume", "the warning also goes to stderr with --json") | ||
| 56 | 56 | ||||
| 57 | f = versionFake() | 57 | f = versionFake() | ||
M cmd/ermite/web_mr_test.go +5 -1
| @@ -160,6 +160,8 @@ func TestWebMergeRequestPagesRender(t *testing.T) { | |||
| 160 | 160 | assert.Contains(t, own.body, `<option value="fast-forward">`) | |
| 161 | 161 | assert.Contains(t, own.body, `<option value="squash">`) | |
| 162 | 162 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head) | |
| 163 | + | assert.Contains(t, own.body, `name="delete_branch" value="1"> delete feat/search after the merge`) | |
| 164 | + | assert.NotContains(t, own.body, `name="delete_branch" value="1" checked>`, "unchecked by default") | |
| 163 | 165 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head+" --squash") | |
| 164 | 166 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/comments"`) | |
| 165 | 167 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/close"`) | |
| @@ -377,8 +379,10 @@ func TestWebMergeRequestSquashCloseAndList(t *testing.T) { | |||
| 377 | 379 | "source": {"feat/search"}, "target": {"main"}, "title": {"Squash me"}, "description": {""}, | |
| 378 | 380 | }) | |
| 379 | 381 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | |
| 380 | - | res = b.form("/proj/merge-requests/1/merge", token, url.Values{"mode": {"squash"}, "head_id": {head}}) | |
| 382 | + | assert.Contains(t, b.get("/proj/merge-requests/1", as(token)).body, `name="delete_branch" value="1"`, "the box offers to delete the branch") | |
| 383 | + | res = b.form("/proj/merge-requests/1/merge", token, url.Values{"mode": {"squash"}, "head_id": {head}, "delete_branch": {"1"}}) | |
| 381 | 384 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | |
| 385 | + | assert.Empty(t, r.bareGit("proj", "for-each-ref", "refs/heads/feat/search"), "the source branch is deleted") | |
| 382 | 386 | merged := r.headOf("main") | |
| 383 | 387 | assert.NotEqual(t, head, merged, "squash makes a new commit") | |
| 384 | 388 | assert.Equal(t, main0, r.bareGit("proj", "rev-parse", "main^"), "on top of the old target") | |
| @@ -160,6 +160,8 @@ func TestWebMergeRequestPagesRender(t *testing.T) { | |||||
| 160 | assert.Contains(t, own.body, `<option value="fast-forward">`) | 160 | assert.Contains(t, own.body, `<option value="fast-forward">`) | ||
| 161 | assert.Contains(t, own.body, `<option value="squash">`) | 161 | assert.Contains(t, own.body, `<option value="squash">`) | ||
| 162 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head) | 162 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head) | ||
| 163 | + | assert.Contains(t, own.body, `name="delete_branch" value="1"> delete feat/search after the merge`) | |||
| 164 | + | assert.NotContains(t, own.body, `name="delete_branch" value="1" checked>`, "unchecked by default") | |||
| 163 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head+" --squash") | 165 | assert.Contains(t, own.body, "ermite mr merge proj 1 --head "+head+" --squash") | ||
| 164 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/comments"`) | 166 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/comments"`) | ||
| 165 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/close"`) | 167 | assert.Contains(t, own.body, `action="/proj/merge-requests/1/close"`) | ||
| @@ -377,8 +379,10 @@ func TestWebMergeRequestSquashCloseAndList(t *testing.T) { | |||||
| 377 | "source": {"feat/search"}, "target": {"main"}, "title": {"Squash me"}, "description": {""}, | 379 | "source": {"feat/search"}, "target": {"main"}, "title": {"Squash me"}, "description": {""}, | ||
| 378 | }) | 380 | }) | ||
| 379 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | 381 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | ||
| 380 | - | res = b.form("/proj/merge-requests/1/merge", token, url.Values{"mode": {"squash"}, "head_id": {head}}) | 382 | + | assert.Contains(t, b.get("/proj/merge-requests/1", as(token)).body, `name="delete_branch" value="1"`, "the box offers to delete the branch") |
| 383 | + | res = b.form("/proj/merge-requests/1/merge", token, url.Values{"mode": {"squash"}, "head_id": {head}, "delete_branch": {"1"}}) | |||
| 381 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | 384 | require.Equal(t, http.StatusSeeOther, res.status, res.body) | ||
| 385 | + | assert.Empty(t, r.bareGit("proj", "for-each-ref", "refs/heads/feat/search"), "the source branch is deleted") | |||
| 382 | merged := r.headOf("main") | 386 | merged := r.headOf("main") | ||
| 383 | assert.NotEqual(t, head, merged, "squash makes a new commit") | 387 | assert.NotEqual(t, head, merged, "squash makes a new commit") | ||
| 384 | assert.Equal(t, main0, r.bareGit("proj", "rev-parse", "main^"), "on top of the old target") | 388 | assert.Equal(t, main0, r.bareGit("proj", "rev-parse", "main^"), "on top of the old target") | ||
M internal/adapters/client/client_test.go +2 -2
| @@ -139,10 +139,10 @@ func TestDescribeBoundsLongMessages(t *testing.T) { | |||
| 139 | 139 | func TestMissingFeatures(t *testing.T) { | |
| 140 | 140 | assert.Empty(t, client.MissingFeatures(client.UsedFeatures())) | |
| 141 | 141 | assert.Equal(t, | |
| 142 | - | []string{client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureSigningKey}, | |
| 142 | + | []string{client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureMergeDeleteBranch, client.FeatureSigningKey}, | |
| 143 | 143 | client.MissingFeatures([]string{client.FeatureRepoUpdate, client.FeatureSession, "other"})) | |
| 144 | 144 | assert.Equal(t, []string{client.FeatureSigningKey}, client.MissingFeatures([]string{ | |
| 145 | - | client.FeatureRepoUpdate, client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureSession, | |
| 145 | + | client.FeatureRepoUpdate, client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureMergeDeleteBranch, client.FeatureSession, | |
| 146 | 146 | })) | |
| 147 | 147 | assert.Equal(t, client.UsedFeatures(), client.MissingFeatures(nil)) | |
| 148 | 148 | } | |
| @@ -139,10 +139,10 @@ func TestDescribeBoundsLongMessages(t *testing.T) { | |||||
| 139 | func TestMissingFeatures(t *testing.T) { | 139 | func TestMissingFeatures(t *testing.T) { | ||
| 140 | assert.Empty(t, client.MissingFeatures(client.UsedFeatures())) | 140 | assert.Empty(t, client.MissingFeatures(client.UsedFeatures())) | ||
| 141 | assert.Equal(t, | 141 | assert.Equal(t, | ||
| 142 | - | []string{client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureSigningKey}, | 142 | + | []string{client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureMergeDeleteBranch, client.FeatureSigningKey}, |
| 143 | client.MissingFeatures([]string{client.FeatureRepoUpdate, client.FeatureSession, "other"})) | 143 | client.MissingFeatures([]string{client.FeatureRepoUpdate, client.FeatureSession, "other"})) | ||
| 144 | assert.Equal(t, []string{client.FeatureSigningKey}, client.MissingFeatures([]string{ | 144 | assert.Equal(t, []string{client.FeatureSigningKey}, client.MissingFeatures([]string{ | ||
| 145 | - | client.FeatureRepoUpdate, client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureSession, | 145 | + | client.FeatureRepoUpdate, client.FeatureEventsResume, client.FeatureMergeRequests, client.FeatureMergeDeleteBranch, client.FeatureSession, |
| 146 | })) | 146 | })) | ||
| 147 | assert.Equal(t, client.UsedFeatures(), client.MissingFeatures(nil)) | 147 | assert.Equal(t, client.UsedFeatures(), client.MissingFeatures(nil)) | ||
| 148 | } | 148 | } | ||
M internal/adapters/client/features.go +4 -1
| @@ -15,6 +15,9 @@ const ( | |||
| 15 | 15 | FeatureEventsResume = "events.resume" | |
| 16 | 16 | // FeatureMergeRequests is MergeRequestService (`ermite mr`). | |
| 17 | 17 | FeatureMergeRequests = "merge_requests" | |
| 18 | + | // FeatureMergeDeleteBranch is MergeMergeRequest with delete_source_branch | |
| 19 | + | // (`ermite mr merge --delete-branch`). | |
| 20 | + | FeatureMergeDeleteBranch = "merge_requests.delete_branch" | |
| 18 | 21 | // FeatureSigningKey is ServerService.GetSigningKey (`ermite signing-key`). | |
| 19 | 22 | FeatureSigningKey = "signing_key" | |
| 20 | 23 | // FeatureSession is SessionService (`ermite web login`, `web logout`). | |
| @@ -23,7 +26,7 @@ const ( | |||
| 23 | 26 | ||
| 24 | 27 | // UsedFeatures lists the features the CLI uses, in the order they are reported. | |
| 25 | 28 | func UsedFeatures() []string { | |
| 26 | - | return []string{FeatureRepoUpdate, FeatureEventsResume, FeatureMergeRequests, FeatureSigningKey, FeatureSession} | |
| 29 | + | return []string{FeatureRepoUpdate, FeatureEventsResume, FeatureMergeRequests, FeatureMergeDeleteBranch, FeatureSigningKey, FeatureSession} | |
| 27 | 30 | } | |
| 28 | 31 | ||
| 29 | 32 | // MissingFeatures returns the features the CLI uses that have is lacking. | |
| @@ -15,6 +15,9 @@ const ( | |||||
| 15 | FeatureEventsResume = "events.resume" | 15 | FeatureEventsResume = "events.resume" | ||
| 16 | // FeatureMergeRequests is MergeRequestService (`ermite mr`). | 16 | // FeatureMergeRequests is MergeRequestService (`ermite mr`). | ||
| 17 | FeatureMergeRequests = "merge_requests" | 17 | FeatureMergeRequests = "merge_requests" | ||
| 18 | + | // FeatureMergeDeleteBranch is MergeMergeRequest with delete_source_branch | |||
| 19 | + | // (`ermite mr merge --delete-branch`). | |||
| 20 | + | FeatureMergeDeleteBranch = "merge_requests.delete_branch" | |||
| 18 | // FeatureSigningKey is ServerService.GetSigningKey (`ermite signing-key`). | 21 | // FeatureSigningKey is ServerService.GetSigningKey (`ermite signing-key`). | ||
| 19 | FeatureSigningKey = "signing_key" | 22 | FeatureSigningKey = "signing_key" | ||
| 20 | // FeatureSession is SessionService (`ermite web login`, `web logout`). | 23 | // FeatureSession is SessionService (`ermite web login`, `web logout`). | ||
| @@ -23,7 +26,7 @@ const ( | |||||
| 23 | 26 | ||||
| 24 | // UsedFeatures lists the features the CLI uses, in the order they are reported. | 27 | // UsedFeatures lists the features the CLI uses, in the order they are reported. | ||
| 25 | func UsedFeatures() []string { | 28 | func UsedFeatures() []string { | ||
| 26 | - | return []string{FeatureRepoUpdate, FeatureEventsResume, FeatureMergeRequests, FeatureSigningKey, FeatureSession} | 29 | + | return []string{FeatureRepoUpdate, FeatureEventsResume, FeatureMergeRequests, FeatureMergeDeleteBranch, FeatureSigningKey, FeatureSession} |
| 27 | } | 30 | } | ||
| 28 | 31 | ||||
| 29 | // MissingFeatures returns the features the CLI uses that have is lacking. | 32 | // MissingFeatures returns the features the CLI uses that have is lacking. | ||
M internal/adapters/gitcli/merge.go +26 -0
| @@ -235,3 +235,29 @@ func (g *Git) UpdateBranch(ctx context.Context, repo domain.RepoName, branch str | |||
| 235 | 235 | } | |
| 236 | 236 | return fmt.Errorf("update branch %q of %q: %w", branch, repo, err) | |
| 237 | 237 | } | |
| 238 | + | ||
| 239 | + | // DeleteBranch runs `git update-ref -d refs/heads/<branch> <old>`, which takes | |
| 240 | + | // the ref lock and refuses when the ref is not at old. A failure is told from | |
| 241 | + | // a moved or deleted branch by reading the branch again. | |
| 242 | + | // | |
| 243 | + | //hexcheck:ignore suspicious-business-logic-in-adapter maps a refused update-ref to the conflict error of the port | |
| 244 | + | func (g *Git) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |
| 245 | + | dir, err := g.repoDir(repo) | |
| 246 | + | if err != nil { | |
| 247 | + | return err | |
| 248 | + | } | |
| 249 | + | if _, err := domain.ParseBranchName(branch); err != nil { | |
| 250 | + | return fmt.Errorf("delete branch of %q: %w", repo, err) | |
| 251 | + | } | |
| 252 | + | _, err = g.run(ctx, "--git-dir="+dir, "update-ref", "-d", headsRefPrefix+branch, old.String()) | |
| 253 | + | if err == nil { | |
| 254 | + | return nil | |
| 255 | + | } | |
| 256 | + | switch cur, herr := g.BranchHead(ctx, repo, branch); { | |
| 257 | + | case errors.Is(herr, domain.ErrNotFound): | |
| 258 | + | return fmt.Errorf("branch %q of %q is gone: %w", branch, repo, domain.ErrConflict) | |
| 259 | + | case herr == nil && cur != old: | |
| 260 | + | return fmt.Errorf("branch %q of %q is at %s, not %s: %w", branch, repo, cur, old, domain.ErrConflict) | |
| 261 | + | } | |
| 262 | + | return fmt.Errorf("delete branch %q of %q: %w", branch, repo, err) | |
| 263 | + | } | |
| @@ -235,3 +235,29 @@ func (g *Git) UpdateBranch(ctx context.Context, repo domain.RepoName, branch str | |||||
| 235 | } | 235 | } | ||
| 236 | return fmt.Errorf("update branch %q of %q: %w", branch, repo, err) | 236 | return fmt.Errorf("update branch %q of %q: %w", branch, repo, err) | ||
| 237 | } | 237 | } | ||
| 238 | + | ||||
| 239 | + | // DeleteBranch runs `git update-ref -d refs/heads/<branch> <old>`, which takes | |||
| 240 | + | // the ref lock and refuses when the ref is not at old. A failure is told from | |||
| 241 | + | // a moved or deleted branch by reading the branch again. | |||
| 242 | + | // | |||
| 243 | + | //hexcheck:ignore suspicious-business-logic-in-adapter maps a refused update-ref to the conflict error of the port | |||
| 244 | + | func (g *Git) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |||
| 245 | + | dir, err := g.repoDir(repo) | |||
| 246 | + | if err != nil { | |||
| 247 | + | return err | |||
| 248 | + | } | |||
| 249 | + | if _, err := domain.ParseBranchName(branch); err != nil { | |||
| 250 | + | return fmt.Errorf("delete branch of %q: %w", repo, err) | |||
| 251 | + | } | |||
| 252 | + | _, err = g.run(ctx, "--git-dir="+dir, "update-ref", "-d", headsRefPrefix+branch, old.String()) | |||
| 253 | + | if err == nil { | |||
| 254 | + | return nil | |||
| 255 | + | } | |||
| 256 | + | switch cur, herr := g.BranchHead(ctx, repo, branch); { | |||
| 257 | + | case errors.Is(herr, domain.ErrNotFound): | |||
| 258 | + | return fmt.Errorf("branch %q of %q is gone: %w", branch, repo, domain.ErrConflict) | |||
| 259 | + | case herr == nil && cur != old: | |||
| 260 | + | return fmt.Errorf("branch %q of %q is at %s, not %s: %w", branch, repo, cur, old, domain.ErrConflict) | |||
| 261 | + | } | |||
| 262 | + | return fmt.Errorf("delete branch %q of %q: %w", branch, repo, err) | |||
| 263 | + | } | |||
M internal/adapters/gitcli/merge_test.go +25 -0
| @@ -206,3 +206,28 @@ func TestWriteCommitAndUpdateBranch(t *testing.T) { | |||
| 206 | 206 | err = g.UpdateBranch(ctx, name(t, "missing"), "main", id, m.mainTip) | |
| 207 | 207 | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 208 | 208 | } | |
| 209 | + | ||
| 210 | + | func TestDeleteBranch(t *testing.T) { | |
| 211 | + | g, data := newGit(t) | |
| 212 | + | ctx := context.Background() | |
| 213 | + | m := newMergeFixture(t, data) | |
| 214 | + | m.f.Git("pack-refs", "--all") // the ref is in packed-refs too | |
| 215 | + | ||
| 216 | + | // The old value is a precondition. | |
| 217 | + | err := g.DeleteBranch(ctx, m.f.Name, "feat", m.mainTip) | |
| 218 | + | assert.ErrorIs(t, err, domain.ErrConflict, "feat is not at main") | |
| 219 | + | assert.Equal(t, m.featTip.String(), m.f.Git("rev-parse", "refs/heads/feat")) | |
| 220 | + | ||
| 221 | + | require.NoError(t, g.DeleteBranch(ctx, m.f.Name, "feat", m.featTip)) | |
| 222 | + | _, err = g.BranchHead(ctx, m.f.Name, "feat") | |
| 223 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 224 | + | assert.Empty(t, m.f.Git("for-each-ref", "refs/heads/feat"), "gone from packed-refs too") | |
| 225 | + | ||
| 226 | + | err = g.DeleteBranch(ctx, m.f.Name, "feat", m.featTip) | |
| 227 | + | assert.ErrorIs(t, err, domain.ErrConflict, "already gone") | |
| 228 | + | err = g.DeleteBranch(ctx, m.f.Name, "refs/heads/main", m.mainTip) | |
| 229 | + | assert.Error(t, err, "only short branch names") | |
| 230 | + | assert.Equal(t, m.mainTip.String(), m.f.Git("rev-parse", "refs/heads/main")) | |
| 231 | + | err = g.DeleteBranch(ctx, name(t, "missing"), "main", m.mainTip) | |
| 232 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |
| 233 | + | } | |
| @@ -206,3 +206,28 @@ func TestWriteCommitAndUpdateBranch(t *testing.T) { | |||||
| 206 | err = g.UpdateBranch(ctx, name(t, "missing"), "main", id, m.mainTip) | 206 | err = g.UpdateBranch(ctx, name(t, "missing"), "main", id, m.mainTip) | ||
| 207 | assert.ErrorIs(t, err, domain.ErrNotFound) | 207 | assert.ErrorIs(t, err, domain.ErrNotFound) | ||
| 208 | } | 208 | } | ||
| 209 | + | ||||
| 210 | + | func TestDeleteBranch(t *testing.T) { | |||
| 211 | + | g, data := newGit(t) | |||
| 212 | + | ctx := context.Background() | |||
| 213 | + | m := newMergeFixture(t, data) | |||
| 214 | + | m.f.Git("pack-refs", "--all") // the ref is in packed-refs too | |||
| 215 | + | ||||
| 216 | + | // The old value is a precondition. | |||
| 217 | + | err := g.DeleteBranch(ctx, m.f.Name, "feat", m.mainTip) | |||
| 218 | + | assert.ErrorIs(t, err, domain.ErrConflict, "feat is not at main") | |||
| 219 | + | assert.Equal(t, m.featTip.String(), m.f.Git("rev-parse", "refs/heads/feat")) | |||
| 220 | + | ||||
| 221 | + | require.NoError(t, g.DeleteBranch(ctx, m.f.Name, "feat", m.featTip)) | |||
| 222 | + | _, err = g.BranchHead(ctx, m.f.Name, "feat") | |||
| 223 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 224 | + | assert.Empty(t, m.f.Git("for-each-ref", "refs/heads/feat"), "gone from packed-refs too") | |||
| 225 | + | ||||
| 226 | + | err = g.DeleteBranch(ctx, m.f.Name, "feat", m.featTip) | |||
| 227 | + | assert.ErrorIs(t, err, domain.ErrConflict, "already gone") | |||
| 228 | + | err = g.DeleteBranch(ctx, m.f.Name, "refs/heads/main", m.mainTip) | |||
| 229 | + | assert.Error(t, err, "only short branch names") | |||
| 230 | + | assert.Equal(t, m.mainTip.String(), m.f.Git("rev-parse", "refs/heads/main")) | |||
| 231 | + | err = g.DeleteBranch(ctx, name(t, "missing"), "main", m.mainTip) | |||
| 232 | + | assert.ErrorIs(t, err, domain.ErrNotFound) | |||
| 233 | + | } | |||
M internal/adapters/rpc/api.go +1 -1
| @@ -27,7 +27,7 @@ const MaxRequestBytes = 1 << 20 | |||
| 27 | 27 | ||
| 28 | 28 | // Features are the optional capabilities of the contract that this server | |
| 29 | 29 | // implements, as GetVersion lists them. | |
| 30 | - | var Features = []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"} | |
| 30 | + | var Features = []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"} | |
| 31 | 31 | ||
| 32 | 32 | // Authenticator turns a token secret into a principal, or | |
| 33 | 33 | // domain.ErrUnauthenticated. app.Auth fits. | |
| @@ -27,7 +27,7 @@ const MaxRequestBytes = 1 << 20 | |||||
| 27 | 27 | ||||
| 28 | // Features are the optional capabilities of the contract that this server | 28 | // Features are the optional capabilities of the contract that this server | ||
| 29 | // implements, as GetVersion lists them. | 29 | // implements, as GetVersion lists them. | ||
| 30 | - | var Features = []string{"repo.update", "events.resume", "merge_requests", "signing_key", "session"} | 30 | + | var Features = []string{"repo.update", "events.resume", "merge_requests", "merge_requests.delete_branch", "signing_key", "session"} |
| 31 | 31 | ||||
| 32 | // Authenticator turns a token secret into a principal, or | 32 | // Authenticator turns a token secret into a principal, or | ||
| 33 | // domain.ErrUnauthenticated. app.Auth fits. | 33 | // domain.ErrUnauthenticated. app.Auth fits. | ||
M internal/adapters/rpc/mergerequest.go +6 -2
| @@ -245,11 +245,15 @@ func (s *mergeRequestService) MergeMergeRequest(ctx context.Context, req *connec | |||
| 245 | 245 | if err != nil { | |
| 246 | 246 | return nil, connectError(ctx, err) | |
| 247 | 247 | } | |
| 248 | - | mr, err := s.mrs.Merge(ctx, caller(ctx), name, req.Msg.GetNumber(), app.MergeParams{Mode: mode, HeadID: head}) | |
| 248 | + | res, err := s.mrs.Merge(ctx, caller(ctx), name, req.Msg.GetNumber(), app.MergeParams{ | |
| 249 | + | Mode: mode, HeadID: head, DeleteSource: req.Msg.GetDeleteSourceBranch(), | |
| 250 | + | }) | |
| 249 | 251 | if err != nil { | |
| 250 | 252 | return nil, connectError(ctx, err) | |
| 251 | 253 | } | |
| 252 | - | return connect.NewResponse(&pb.MergeMergeRequestResponse{MergeRequest: mergeRequestToPB(mr)}), nil | |
| 254 | + | return connect.NewResponse(&pb.MergeMergeRequestResponse{ | |
| 255 | + | MergeRequest: mergeRequestToPB(res.MergeRequest), SourceBranchDeleted: res.SourceDeleted, | |
| 256 | + | }), nil | |
| 253 | 257 | } | |
| 254 | 258 | ||
| 255 | 259 | func (s *mergeRequestService) MarkRead(ctx context.Context, req *connect.Request[pb.MarkReadRequest]) (*connect.Response[pb.MarkReadResponse], error) { | |
| @@ -245,11 +245,15 @@ func (s *mergeRequestService) MergeMergeRequest(ctx context.Context, req *connec | |||||
| 245 | if err != nil { | 245 | if err != nil { | ||
| 246 | return nil, connectError(ctx, err) | 246 | return nil, connectError(ctx, err) | ||
| 247 | } | 247 | } | ||
| 248 | - | mr, err := s.mrs.Merge(ctx, caller(ctx), name, req.Msg.GetNumber(), app.MergeParams{Mode: mode, HeadID: head}) | 248 | + | res, err := s.mrs.Merge(ctx, caller(ctx), name, req.Msg.GetNumber(), app.MergeParams{ |
| 249 | + | Mode: mode, HeadID: head, DeleteSource: req.Msg.GetDeleteSourceBranch(), | |||
| 250 | + | }) | |||
| 249 | if err != nil { | 251 | if err != nil { | ||
| 250 | return nil, connectError(ctx, err) | 252 | return nil, connectError(ctx, err) | ||
| 251 | } | 253 | } | ||
| 252 | - | return connect.NewResponse(&pb.MergeMergeRequestResponse{MergeRequest: mergeRequestToPB(mr)}), nil | 254 | + | return connect.NewResponse(&pb.MergeMergeRequestResponse{ |
| 255 | + | MergeRequest: mergeRequestToPB(res.MergeRequest), SourceBranchDeleted: res.SourceDeleted, | |||
| 256 | + | }), nil | |||
| 253 | } | 257 | } | ||
| 254 | 258 | ||||
| 255 | func (s *mergeRequestService) MarkRead(ctx context.Context, req *connect.Request[pb.MarkReadRequest]) (*connect.Response[pb.MarkReadResponse], error) { | 259 | func (s *mergeRequestService) MarkRead(ctx context.Context, req *connect.Request[pb.MarkReadRequest]) (*connect.Response[pb.MarkReadResponse], error) { | ||
M internal/adapters/web/mr.go +7 -3
| @@ -147,6 +147,9 @@ type MergeBox struct { | |||
| 147 | 147 | CLI Clone | |
| 148 | 148 | // CanMarkRead is true when the head differs from the one last read. | |
| 149 | 149 | CanMarkRead bool | |
| 150 | + | // CanDeleteBranch offers to delete the source after the merge: it is not | |
| 151 | + | // the default branch. The server checks again, with the open requests. | |
| 152 | + | CanDeleteBranch bool | |
| 150 | 153 | } | |
| 151 | 154 | ||
| 152 | 155 | // ModeOption is a merge mode the repository allows. | |
| @@ -528,7 +531,7 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||
| 528 | 531 | p.Invisible = invisible | |
| 529 | 532 | p.DiffView = newDiffView(r, lines, self+"?view=unified", self+"?view=split") | |
| 530 | 533 | if signed && open { | |
| 531 | - | p.Box = s.mergeBox(d, n, head, lastRead) | |
| 534 | + | p.Box = s.mergeBox(d, n, m.GetSourceBranch(), head, lastRead) | |
| 532 | 535 | } | |
| 533 | 536 | if canWrite { | |
| 534 | 537 | p.GeneralForm = &CommentForm{ | |
| @@ -573,11 +576,12 @@ func (d *repoData) hasBranch(name string) bool { | |||
| 573 | 576 | } | |
| 574 | 577 | ||
| 575 | 578 | // mergeBox is what the owner can do with an open request. | |
| 576 | - | func (s *Server) mergeBox(d *repoData, n int64, head, lastRead string) *MergeBox { | |
| 579 | + | func (s *Server) mergeBox(d *repoData, n int64, source, head, lastRead string) *MergeBox { | |
| 577 | 580 | b := &MergeBox{ | |
| 578 | 581 | MergeAction: mrHref(d.name(), n, "merge"), CloseAction: mrHref(d.name(), n, "close"), | |
| 579 | 582 | ReadAction: mrHref(d.name(), n, "read"), Head: head, HeadGone: head == "", | |
| 580 | - | CanMarkRead: head != "" && head != lastRead, | |
| 583 | + | CanMarkRead: head != "" && head != lastRead, | |
| 584 | + | CanDeleteBranch: source != d.repo.GetDefaultBranch(), | |
| 581 | 585 | } | |
| 582 | 586 | ff, sq := false, false | |
| 583 | 587 | if st := d.repo.GetSettings(); st != nil { | |
| @@ -147,6 +147,9 @@ type MergeBox struct { | |||||
| 147 | CLI Clone | 147 | CLI Clone | ||
| 148 | // CanMarkRead is true when the head differs from the one last read. | 148 | // CanMarkRead is true when the head differs from the one last read. | ||
| 149 | CanMarkRead bool | 149 | CanMarkRead bool | ||
| 150 | + | // CanDeleteBranch offers to delete the source after the merge: it is not | |||
| 151 | + | // the default branch. The server checks again, with the open requests. | |||
| 152 | + | CanDeleteBranch bool | |||
| 150 | } | 153 | } | ||
| 151 | 154 | ||||
| 152 | // ModeOption is a merge mode the repository allows. | 155 | // ModeOption is a merge mode the repository allows. | ||
| @@ -528,7 +531,7 @@ func (s *Server) mrPage(ctx context.Context, r *http.Request, since bool) (templ | |||||
| 528 | p.Invisible = invisible | 531 | p.Invisible = invisible | ||
| 529 | p.DiffView = newDiffView(r, lines, self+"?view=unified", self+"?view=split") | 532 | p.DiffView = newDiffView(r, lines, self+"?view=unified", self+"?view=split") | ||
| 530 | if signed && open { | 533 | if signed && open { | ||
| 531 | - | p.Box = s.mergeBox(d, n, head, lastRead) | 534 | + | p.Box = s.mergeBox(d, n, m.GetSourceBranch(), head, lastRead) |
| 532 | } | 535 | } | ||
| 533 | if canWrite { | 536 | if canWrite { | ||
| 534 | p.GeneralForm = &CommentForm{ | 537 | p.GeneralForm = &CommentForm{ | ||
| @@ -573,11 +576,12 @@ func (d *repoData) hasBranch(name string) bool { | |||||
| 573 | } | 576 | } | ||
| 574 | 577 | ||||
| 575 | // mergeBox is what the owner can do with an open request. | 578 | // mergeBox is what the owner can do with an open request. | ||
| 576 | - | func (s *Server) mergeBox(d *repoData, n int64, head, lastRead string) *MergeBox { | 579 | + | func (s *Server) mergeBox(d *repoData, n int64, source, head, lastRead string) *MergeBox { |
| 577 | b := &MergeBox{ | 580 | b := &MergeBox{ | ||
| 578 | MergeAction: mrHref(d.name(), n, "merge"), CloseAction: mrHref(d.name(), n, "close"), | 581 | MergeAction: mrHref(d.name(), n, "merge"), CloseAction: mrHref(d.name(), n, "close"), | ||
| 579 | ReadAction: mrHref(d.name(), n, "read"), Head: head, HeadGone: head == "", | 582 | ReadAction: mrHref(d.name(), n, "read"), Head: head, HeadGone: head == "", | ||
| 580 | - | CanMarkRead: head != "" && head != lastRead, | 583 | + | CanMarkRead: head != "" && head != lastRead, |
| 584 | + | CanDeleteBranch: source != d.repo.GetDefaultBranch(), | |||
| 581 | } | 585 | } | ||
| 582 | ff, sq := false, false | 586 | ff, sq := false, false | ||
| 583 | if st := d.repo.GetSettings(); st != nil { | 587 | if st := d.repo.GetSettings(); st != nil { | ||
M internal/adapters/web/mr.templ +3 -0
| @@ -382,6 +382,9 @@ templ mergeBox(p MRPage) { | |||
| 382 | 382 | <option value={ m.Value }>{ m.Label }</option> | |
| 383 | 383 | } | |
| 384 | 384 | </select> | |
| 385 | + | if p.Box.CanDeleteBranch { | |
| 386 | + | <label class="d-check"><input type="checkbox" name="delete_branch" value="1"/> delete { p.Source } after the merge</label> | |
| 387 | + | } | |
| 385 | 388 | <div class="d-btns"> | |
| 386 | 389 | <button type="submit" class="d-main">[merge]</button> | |
| 387 | 390 | </div> | |
| @@ -382,6 +382,9 @@ templ mergeBox(p MRPage) { | |||||
| 382 | <option value={ m.Value }>{ m.Label }</option> | 382 | <option value={ m.Value }>{ m.Label }</option> | ||
| 383 | } | 383 | } | ||
| 384 | </select> | 384 | </select> | ||
| 385 | + | if p.Box.CanDeleteBranch { | |||
| 386 | + | <label class="d-check"><input type="checkbox" name="delete_branch" value="1"/> delete { p.Source } after the merge</label> | |||
| 387 | + | } | |||
| 385 | <div class="d-btns"> | 388 | <div class="d-btns"> | ||
| 386 | <button type="submit" class="d-main">[merge]</button> | 389 | <button type="submit" class="d-main">[merge]</button> | ||
| 387 | </div> | 390 | </div> | ||
M internal/adapters/web/mr_post.go +5 -3
| @@ -205,9 +205,10 @@ func (s *Server) mrClose(ctx context.Context, r *http.Request) (string, string, | |||
| 205 | 205 | return back, back, nil | |
| 206 | 206 | } | |
| 207 | 207 | ||
| 208 | - | // mrMerge is POST /<repo>/merge-requests/<n>/merge. The form carries the mode | |
| 209 | - | // and the head the owner was shown (head_id): the server merges only if the | |
| 210 | - | // branch still points to it. | |
| 208 | + | // mrMerge is POST /<repo>/merge-requests/<n>/merge. The form carries the mode, | |
| 209 | + | // the head the owner was shown (head_id): the server merges only if the | |
| 210 | + | // branch still points to it, and delete_branch when the owner asked to delete | |
| 211 | + | // the source branch after the merge. | |
| 211 | 212 | func (s *Server) mrMerge(ctx context.Context, r *http.Request) (string, string, error) { | |
| 212 | 213 | repo := r.PathValue("repo") | |
| 213 | 214 | n, err := number(r) | |
| @@ -226,6 +227,7 @@ func (s *Server) mrMerge(ctx context.Context, r *http.Request) (string, string, | |||
| 226 | 227 | } | |
| 227 | 228 | if _, err := s.cfg.MergeRequests.MergeMergeRequest(ctx, connect.NewRequest(&pb.MergeMergeRequestRequest{ | |
| 228 | 229 | Repo: repo, Number: n, Mode: mode, HeadId: r.PostFormValue("head_id"), | |
| 230 | + | DeleteSourceBranch: r.PostFormValue("delete_branch") == "1", | |
| 229 | 231 | })); err != nil { | |
| 230 | 232 | return "", back, err | |
| 231 | 233 | } | |
| @@ -205,9 +205,10 @@ func (s *Server) mrClose(ctx context.Context, r *http.Request) (string, string, | |||||
| 205 | return back, back, nil | 205 | return back, back, nil | ||
| 206 | } | 206 | } | ||
| 207 | 207 | ||||
| 208 | - | // mrMerge is POST /<repo>/merge-requests/<n>/merge. The form carries the mode | 208 | + | // mrMerge is POST /<repo>/merge-requests/<n>/merge. The form carries the mode, |
| 209 | - | // and the head the owner was shown (head_id): the server merges only if the | 209 | + | // the head the owner was shown (head_id): the server merges only if the |
| 210 | - | // branch still points to it. | 210 | + | // branch still points to it, and delete_branch when the owner asked to delete |
| 211 | + | // the source branch after the merge. | |||
| 211 | func (s *Server) mrMerge(ctx context.Context, r *http.Request) (string, string, error) { | 212 | func (s *Server) mrMerge(ctx context.Context, r *http.Request) (string, string, error) { | ||
| 212 | repo := r.PathValue("repo") | 213 | repo := r.PathValue("repo") | ||
| 213 | n, err := number(r) | 214 | n, err := number(r) | ||
| @@ -226,6 +227,7 @@ func (s *Server) mrMerge(ctx context.Context, r *http.Request) (string, string, | |||||
| 226 | } | 227 | } | ||
| 227 | if _, err := s.cfg.MergeRequests.MergeMergeRequest(ctx, connect.NewRequest(&pb.MergeMergeRequestRequest{ | 228 | if _, err := s.cfg.MergeRequests.MergeMergeRequest(ctx, connect.NewRequest(&pb.MergeMergeRequestRequest{ | ||
| 228 | Repo: repo, Number: n, Mode: mode, HeadId: r.PostFormValue("head_id"), | 229 | Repo: repo, Number: n, Mode: mode, HeadId: r.PostFormValue("head_id"), | ||
| 230 | + | DeleteSourceBranch: r.PostFormValue("delete_branch") == "1", | |||
| 229 | })); err != nil { | 231 | })); err != nil { | ||
| 230 | return "", back, err | 232 | return "", back, err | ||
| 231 | } | 233 | } | ||
M internal/adapters/web/mr_templ.go +223 -200 generated file: collapsed
M internal/adapters/web/static/css/60-mr.css +2 -0
| @@ -24,6 +24,8 @@ | |||
| 24 | 24 | display: block; width: 100%; font: inherit; color: inherit; background: var(--paper-alt); | |
| 25 | 25 | border: 1px solid var(--rule); border-radius: 0; padding: 0 1ch; | |
| 26 | 26 | } | |
| 27 | + | & .d-check { display: block; margin-top: 4px; color: var(--ink-dim); cursor: pointer; } | |
| 28 | + | & .d-check input { accent-color: var(--ink); margin: 0 1ch 0 0; vertical-align: middle; } | |
| 27 | 29 | & .d-btns { display: flex; gap: 2ch; margin-top: 4px; flex-wrap: wrap; } | |
| 28 | 30 | & .d-btns button { color: var(--ink-dim); } | |
| 29 | 31 | & .d-btns button:hover { color: var(--ink); text-decoration: underline; } | |
| @@ -24,6 +24,8 @@ | |||||
| 24 | display: block; width: 100%; font: inherit; color: inherit; background: var(--paper-alt); | 24 | display: block; width: 100%; font: inherit; color: inherit; background: var(--paper-alt); | ||
| 25 | border: 1px solid var(--rule); border-radius: 0; padding: 0 1ch; | 25 | border: 1px solid var(--rule); border-radius: 0; padding: 0 1ch; | ||
| 26 | } | 26 | } | ||
| 27 | + | & .d-check { display: block; margin-top: 4px; color: var(--ink-dim); cursor: pointer; } | |||
| 28 | + | & .d-check input { accent-color: var(--ink); margin: 0 1ch 0 0; vertical-align: middle; } | |||
| 27 | & .d-btns { display: flex; gap: 2ch; margin-top: 4px; flex-wrap: wrap; } | 29 | & .d-btns { display: flex; gap: 2ch; margin-top: 4px; flex-wrap: wrap; } | ||
| 28 | & .d-btns button { color: var(--ink-dim); } | 30 | & .d-btns button { color: var(--ink-dim); } | ||
| 29 | & .d-btns button:hover { color: var(--ink); text-decoration: underline; } | 31 | & .d-btns button:hover { color: var(--ink); text-decoration: underline; } | ||
M internal/app/mergerequests.go +85 -22
| @@ -703,6 +703,16 @@ type MergeParams struct { | |||
| 703 | 703 | // HeadID is the commit the caller reviewed; the source branch must still | |
| 704 | 704 | // point to it. | |
| 705 | 705 | HeadID domain.ObjectID | |
| 706 | + | // DeleteSource deletes the source branch after the merge. | |
| 707 | + | DeleteSource bool | |
| 708 | + | } | |
| 709 | + | ||
| 710 | + | // MergeResult is a merged request and what happened to its source branch. | |
| 711 | + | type MergeResult struct { | |
| 712 | + | domain.MergeRequest | |
| 713 | + | // SourceDeleted is true when DeleteSource was asked and the branch is | |
| 714 | + | // gone. | |
| 715 | + | SourceDeleted bool | |
| 706 | 716 | } | |
| 707 | 717 | ||
| 708 | 718 | // Merge merges an open merge request, publishes MergeRequestMerged then a | |
| @@ -722,10 +732,21 @@ type MergeParams struct { | |||
| 722 | 732 | // precondition. A squash writes one commit with the tree of the head, the | |
| 723 | 733 | // target as parent and the author and message of domain.NewSquashCommit, | |
| 724 | 734 | // signed by the instance key, and moves the target to it the same way. | |
| 725 | - | func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (domain.MergeRequest, error) { | |
| 735 | + | // | |
| 736 | + | // With DeleteSource the merge is also refused, before anything changes, when | |
| 737 | + | // the source is the default branch or the source or target of another open | |
| 738 | + | // request. After the merge the source is deleted if it still points to the | |
| 739 | + | // head; the PushEvent then carries the deletion too. A failed deletion is | |
| 740 | + | // logged and leaves the branch: the merge stands and SourceDeleted is false. | |
| 741 | + | func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (MergeResult, error) { | |
| 742 | + | mr, deleted, err := m.merge(ctx, p, name, number, params) | |
| 743 | + | return MergeResult{MergeRequest: mr, SourceDeleted: deleted}, err | |
| 744 | + | } | |
| 745 | + | ||
| 746 | + | func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (domain.MergeRequest, bool, error) { | |
| 726 | 747 | repo, err := m.writable(ctx, p, name, domain.ScopeMRMerge) | |
| 727 | 748 | if err != nil { | |
| 728 | - | return domain.MergeRequest{}, err | |
| 749 | + | return domain.MergeRequest{}, false, err | |
| 729 | 750 | } | |
| 730 | 751 | mode := params.Mode | |
| 731 | 752 | if mode == "" { | |
| @@ -739,83 +760,125 @@ func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name doma | |||
| 739 | 760 | v.Add("head_id", "is required") | |
| 740 | 761 | } | |
| 741 | 762 | if err := v.Err(); err != nil { | |
| 742 | - | return domain.MergeRequest{}, err | |
| 763 | + | return domain.MergeRequest{}, false, err | |
| 743 | 764 | } | |
| 744 | 765 | ||
| 745 | 766 | unlock, err := m.lockRepo(ctx, name) | |
| 746 | 767 | if err != nil { | |
| 747 | - | return domain.MergeRequest{}, err | |
| 768 | + | return domain.MergeRequest{}, false, err | |
| 748 | 769 | } | |
| 749 | 770 | defer unlock() | |
| 750 | 771 | ||
| 751 | 772 | mr, err := m.store.Get(ctx, name, number) | |
| 752 | 773 | if err != nil { | |
| 753 | - | return domain.MergeRequest{}, err | |
| 774 | + | return domain.MergeRequest{}, false, err | |
| 754 | 775 | } | |
| 755 | 776 | if !mr.IsOpen() { | |
| 756 | - | return domain.MergeRequest{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State) | |
| 777 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State) | |
| 757 | 778 | } | |
| 758 | 779 | // The settings are read again under the lock: a change applies at once. | |
| 759 | 780 | repo, err = m.meta.Get(ctx, name) | |
| 760 | 781 | if err != nil { | |
| 761 | 782 | if errors.Is(err, domain.ErrNotFound) { | |
| 762 | - | return domain.MergeRequest{}, err // deleted meanwhile | |
| 783 | + | return domain.MergeRequest{}, false, err // deleted meanwhile | |
| 763 | 784 | } | |
| 764 | - | return domain.MergeRequest{}, fmt.Errorf("get repository %q: %w", name, err) | |
| 785 | + | return domain.MergeRequest{}, false, fmt.Errorf("get repository %q: %w", name, err) | |
| 765 | 786 | } | |
| 766 | 787 | settings := repo.Settings | |
| 767 | 788 | if settings.IsZero() { | |
| 768 | 789 | settings = domain.DefaultRepoSettings() | |
| 769 | 790 | } | |
| 770 | 791 | if !settings.Allows(mode) { | |
| 771 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the %s merge mode is not allowed in %s", domain.ErrPreconditionFailed, mode, name) | |
| 792 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the %s merge mode is not allowed in %s", domain.ErrPreconditionFailed, mode, name) | |
| 772 | 793 | } | |
| 773 | 794 | ||
| 774 | 795 | head, err := m.branchOrZero(ctx, name, mr.Source) | |
| 775 | 796 | switch { | |
| 776 | 797 | case err != nil: | |
| 777 | - | return domain.MergeRequest{}, err | |
| 798 | + | return domain.MergeRequest{}, false, err | |
| 778 | 799 | case head.IsZero(): | |
| 779 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the source branch %q no longer exists", domain.ErrPreconditionFailed, mr.Source) | |
| 800 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the source branch %q no longer exists", domain.ErrPreconditionFailed, mr.Source) | |
| 780 | 801 | case head != params.HeadID: | |
| 781 | - | return domain.MergeRequest{}, fmt.Errorf("%w: branch moved, read again: %q is at %s, not %s", domain.ErrPreconditionFailed, mr.Source, head, params.HeadID) | |
| 802 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: branch moved, read again: %q is at %s, not %s", domain.ErrPreconditionFailed, mr.Source, head, params.HeadID) | |
| 782 | 803 | } | |
| 783 | 804 | tip, err := m.branchOrZero(ctx, name, mr.Target) | |
| 784 | 805 | switch { | |
| 785 | 806 | case err != nil: | |
| 786 | - | return domain.MergeRequest{}, err | |
| 807 | + | return domain.MergeRequest{}, false, err | |
| 787 | 808 | case tip.IsZero(): | |
| 788 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the target branch %q no longer exists", domain.ErrPreconditionFailed, mr.Target) | |
| 809 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the target branch %q no longer exists", domain.ErrPreconditionFailed, mr.Target) | |
| 789 | 810 | case tip == head: | |
| 790 | - | return domain.MergeRequest{}, fmt.Errorf("%w: nothing to merge, %q already is at %s", domain.ErrPreconditionFailed, mr.Target, head) | |
| 811 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: nothing to merge, %q already is at %s", domain.ErrPreconditionFailed, mr.Target, head) | |
| 791 | 812 | } | |
| 792 | 813 | switch ok, err := m.git.IsAncestor(ctx, name, tip, head); { | |
| 793 | 814 | case err != nil: | |
| 794 | - | return domain.MergeRequest{}, err | |
| 815 | + | return domain.MergeRequest{}, false, err | |
| 795 | 816 | case !ok: | |
| 796 | - | return domain.MergeRequest{}, fmt.Errorf("%w: %q is not an ancestor of %q: rebase it on %q and push again", domain.ErrPreconditionFailed, mr.Target, mr.Source, mr.Target) | |
| 817 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: %q is not an ancestor of %q: rebase it on %q and push again", domain.ErrPreconditionFailed, mr.Target, mr.Source, mr.Target) | |
| 818 | + | } | |
| 819 | + | if params.DeleteSource { | |
| 820 | + | if err := m.checkDeletable(ctx, name, mr); err != nil { | |
| 821 | + | return domain.MergeRequest{}, false, err | |
| 822 | + | } | |
| 797 | 823 | } | |
| 798 | 824 | ||
| 799 | 825 | merged, err := m.moveTarget(ctx, mr, mode, tip, head) | |
| 800 | 826 | if err != nil { | |
| 801 | - | return domain.MergeRequest{}, err | |
| 827 | + | return domain.MergeRequest{}, false, err | |
| 802 | 828 | } | |
| 803 | 829 | m.recordPush(ctx, name) // the target moved, whatever happens next | |
| 804 | 830 | mr, err = m.markMerged(ctx, name, number, mode, tip, head, merged) | |
| 805 | 831 | if err != nil { | |
| 806 | - | return domain.MergeRequest{}, err | |
| 832 | + | return domain.MergeRequest{}, false, err | |
| 807 | 833 | } | |
| 808 | 834 | log := zerowrap.FromCtx(ctx) | |
| 809 | 835 | log.Info().Str("repo", name.String()).Int64("number", number).Str("mode", mode.String()). | |
| 810 | 836 | Str("target", mr.Target).Str("commit", merged.String()). | |
| 811 | 837 | Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("merge request merged") | |
| 838 | + | updates := []domain.RefUpdate{{Old: tip.String(), New: merged.String(), Ref: "refs/heads/" + mr.Target}} | |
| 839 | + | deleted := false | |
| 840 | + | if params.DeleteSource { | |
| 841 | + | // The merge is done and recorded: a failure here keeps the branch. | |
| 842 | + | switch err := m.git.DeleteBranch(context.WithoutCancel(ctx), name, mr.Source, head); { | |
| 843 | + | case err != nil: | |
| 844 | + | log.Warn().Err(err).Str("repo", name.String()).Int64("number", number).Str("branch", mr.Source). | |
| 845 | + | Msg("source branch kept after the merge") | |
| 846 | + | default: | |
| 847 | + | deleted = true | |
| 848 | + | updates = append(updates, domain.RefUpdate{Old: head.String(), New: domain.ZeroOID, Ref: "refs/heads/" + mr.Source}) | |
| 849 | + | log.Info().Str("repo", name.String()).Int64("number", number).Str("branch", mr.Source). | |
| 850 | + | Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("source branch deleted after the merge") | |
| 851 | + | } | |
| 852 | + | } | |
| 812 | 853 | at := mr.UpdatedAt | |
| 813 | 854 | m.bus.Publish(ctx, domain.MergeRequestMerged{MergeRequest: mr, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt}) | |
| 814 | 855 | m.bus.Publish(ctx, domain.PushEvent{ | |
| 815 | - | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, | |
| 816 | - | Updates: []domain.RefUpdate{{Old: tip.String(), New: merged.String(), Ref: "refs/heads/" + mr.Target}}, | |
| 856 | + | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, | |
| 817 | 857 | }) | |
| 818 | - | return mr, nil | |
| 858 | + | return mr, deleted, nil | |
| 859 | + | } | |
| 860 | + | ||
| 861 | + | // checkDeletable refuses to delete the source branch of mr when it is the | |
| 862 | + | // default branch or the source or target of another open request. It runs | |
| 863 | + | // under the merge lock, which also guards the creation of requests. | |
| 864 | + | func (m *MergeRequests) checkDeletable(ctx context.Context, name domain.RepoName, mr domain.MergeRequest) error { | |
| 865 | + | def, _, err := m.repos.DefaultBranch(ctx, name) | |
| 866 | + | if err != nil { | |
| 867 | + | return fmt.Errorf("default branch of %q: %w", name, err) | |
| 868 | + | } | |
| 869 | + | if mr.Source == def { | |
| 870 | + | return fmt.Errorf("%w: %q is the default branch of %s: it cannot be deleted", domain.ErrPreconditionFailed, mr.Source, name) | |
| 871 | + | } | |
| 872 | + | all, err := m.store.List(ctx, name) | |
| 873 | + | if err != nil { | |
| 874 | + | return fmt.Errorf("list merge requests of %q: %w", name, err) | |
| 875 | + | } | |
| 876 | + | for _, o := range all { | |
| 877 | + | if o.Number != mr.Number && o.IsOpen() && (o.Source == mr.Source || o.Target == mr.Source) { | |
| 878 | + | return fmt.Errorf("%w: !%d still uses %q: it cannot be deleted", domain.ErrPreconditionFailed, o.Number, mr.Source) | |
| 879 | + | } | |
| 880 | + | } | |
| 881 | + | return nil | |
| 819 | 882 | } | |
| 820 | 883 | ||
| 821 | 884 | // recordPush does for a merge what Pushes.Record does for a push, as the merge | |
| @@ -703,6 +703,16 @@ type MergeParams struct { | |||||
| 703 | // HeadID is the commit the caller reviewed; the source branch must still | 703 | // HeadID is the commit the caller reviewed; the source branch must still | ||
| 704 | // point to it. | 704 | // point to it. | ||
| 705 | HeadID domain.ObjectID | 705 | HeadID domain.ObjectID | ||
| 706 | + | // DeleteSource deletes the source branch after the merge. | |||
| 707 | + | DeleteSource bool | |||
| 708 | + | } | |||
| 709 | + | ||||
| 710 | + | // MergeResult is a merged request and what happened to its source branch. | |||
| 711 | + | type MergeResult struct { | |||
| 712 | + | domain.MergeRequest | |||
| 713 | + | // SourceDeleted is true when DeleteSource was asked and the branch is | |||
| 714 | + | // gone. | |||
| 715 | + | SourceDeleted bool | |||
| 706 | } | 716 | } | ||
| 707 | 717 | ||||
| 708 | // Merge merges an open merge request, publishes MergeRequestMerged then a | 718 | // Merge merges an open merge request, publishes MergeRequestMerged then a | ||
| @@ -722,10 +732,21 @@ type MergeParams struct { | |||||
| 722 | // precondition. A squash writes one commit with the tree of the head, the | 732 | // precondition. A squash writes one commit with the tree of the head, the | ||
| 723 | // target as parent and the author and message of domain.NewSquashCommit, | 733 | // target as parent and the author and message of domain.NewSquashCommit, | ||
| 724 | // signed by the instance key, and moves the target to it the same way. | 734 | // signed by the instance key, and moves the target to it the same way. | ||
| 725 | - | func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (domain.MergeRequest, error) { | 735 | + | // |
| 736 | + | // With DeleteSource the merge is also refused, before anything changes, when | |||
| 737 | + | // the source is the default branch or the source or target of another open | |||
| 738 | + | // request. After the merge the source is deleted if it still points to the | |||
| 739 | + | // head; the PushEvent then carries the deletion too. A failed deletion is | |||
| 740 | + | // logged and leaves the branch: the merge stands and SourceDeleted is false. | |||
| 741 | + | func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (MergeResult, error) { | |||
| 742 | + | mr, deleted, err := m.merge(ctx, p, name, number, params) | |||
| 743 | + | return MergeResult{MergeRequest: mr, SourceDeleted: deleted}, err | |||
| 744 | + | } | |||
| 745 | + | ||||
| 746 | + | func (m *MergeRequests) merge(ctx context.Context, p domain.Principal, name domain.RepoName, number int64, params MergeParams) (domain.MergeRequest, bool, error) { | |||
| 726 | repo, err := m.writable(ctx, p, name, domain.ScopeMRMerge) | 747 | repo, err := m.writable(ctx, p, name, domain.ScopeMRMerge) | ||
| 727 | if err != nil { | 748 | if err != nil { | ||
| 728 | - | return domain.MergeRequest{}, err | 749 | + | return domain.MergeRequest{}, false, err |
| 729 | } | 750 | } | ||
| 730 | mode := params.Mode | 751 | mode := params.Mode | ||
| 731 | if mode == "" { | 752 | if mode == "" { | ||
| @@ -739,83 +760,125 @@ func (m *MergeRequests) Merge(ctx context.Context, p domain.Principal, name doma | |||||
| 739 | v.Add("head_id", "is required") | 760 | v.Add("head_id", "is required") | ||
| 740 | } | 761 | } | ||
| 741 | if err := v.Err(); err != nil { | 762 | if err := v.Err(); err != nil { | ||
| 742 | - | return domain.MergeRequest{}, err | 763 | + | return domain.MergeRequest{}, false, err |
| 743 | } | 764 | } | ||
| 744 | 765 | ||||
| 745 | unlock, err := m.lockRepo(ctx, name) | 766 | unlock, err := m.lockRepo(ctx, name) | ||
| 746 | if err != nil { | 767 | if err != nil { | ||
| 747 | - | return domain.MergeRequest{}, err | 768 | + | return domain.MergeRequest{}, false, err |
| 748 | } | 769 | } | ||
| 749 | defer unlock() | 770 | defer unlock() | ||
| 750 | 771 | ||||
| 751 | mr, err := m.store.Get(ctx, name, number) | 772 | mr, err := m.store.Get(ctx, name, number) | ||
| 752 | if err != nil { | 773 | if err != nil { | ||
| 753 | - | return domain.MergeRequest{}, err | 774 | + | return domain.MergeRequest{}, false, err |
| 754 | } | 775 | } | ||
| 755 | if !mr.IsOpen() { | 776 | if !mr.IsOpen() { | ||
| 756 | - | return domain.MergeRequest{}, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State) | 777 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: !%d is %s", domain.ErrPreconditionFailed, number, mr.State) |
| 757 | } | 778 | } | ||
| 758 | // The settings are read again under the lock: a change applies at once. | 779 | // The settings are read again under the lock: a change applies at once. | ||
| 759 | repo, err = m.meta.Get(ctx, name) | 780 | repo, err = m.meta.Get(ctx, name) | ||
| 760 | if err != nil { | 781 | if err != nil { | ||
| 761 | if errors.Is(err, domain.ErrNotFound) { | 782 | if errors.Is(err, domain.ErrNotFound) { | ||
| 762 | - | return domain.MergeRequest{}, err // deleted meanwhile | 783 | + | return domain.MergeRequest{}, false, err // deleted meanwhile |
| 763 | } | 784 | } | ||
| 764 | - | return domain.MergeRequest{}, fmt.Errorf("get repository %q: %w", name, err) | 785 | + | return domain.MergeRequest{}, false, fmt.Errorf("get repository %q: %w", name, err) |
| 765 | } | 786 | } | ||
| 766 | settings := repo.Settings | 787 | settings := repo.Settings | ||
| 767 | if settings.IsZero() { | 788 | if settings.IsZero() { | ||
| 768 | settings = domain.DefaultRepoSettings() | 789 | settings = domain.DefaultRepoSettings() | ||
| 769 | } | 790 | } | ||
| 770 | if !settings.Allows(mode) { | 791 | if !settings.Allows(mode) { | ||
| 771 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the %s merge mode is not allowed in %s", domain.ErrPreconditionFailed, mode, name) | 792 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the %s merge mode is not allowed in %s", domain.ErrPreconditionFailed, mode, name) |
| 772 | } | 793 | } | ||
| 773 | 794 | ||||
| 774 | head, err := m.branchOrZero(ctx, name, mr.Source) | 795 | head, err := m.branchOrZero(ctx, name, mr.Source) | ||
| 775 | switch { | 796 | switch { | ||
| 776 | case err != nil: | 797 | case err != nil: | ||
| 777 | - | return domain.MergeRequest{}, err | 798 | + | return domain.MergeRequest{}, false, err |
| 778 | case head.IsZero(): | 799 | case head.IsZero(): | ||
| 779 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the source branch %q no longer exists", domain.ErrPreconditionFailed, mr.Source) | 800 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the source branch %q no longer exists", domain.ErrPreconditionFailed, mr.Source) |
| 780 | case head != params.HeadID: | 801 | case head != params.HeadID: | ||
| 781 | - | return domain.MergeRequest{}, fmt.Errorf("%w: branch moved, read again: %q is at %s, not %s", domain.ErrPreconditionFailed, mr.Source, head, params.HeadID) | 802 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: branch moved, read again: %q is at %s, not %s", domain.ErrPreconditionFailed, mr.Source, head, params.HeadID) |
| 782 | } | 803 | } | ||
| 783 | tip, err := m.branchOrZero(ctx, name, mr.Target) | 804 | tip, err := m.branchOrZero(ctx, name, mr.Target) | ||
| 784 | switch { | 805 | switch { | ||
| 785 | case err != nil: | 806 | case err != nil: | ||
| 786 | - | return domain.MergeRequest{}, err | 807 | + | return domain.MergeRequest{}, false, err |
| 787 | case tip.IsZero(): | 808 | case tip.IsZero(): | ||
| 788 | - | return domain.MergeRequest{}, fmt.Errorf("%w: the target branch %q no longer exists", domain.ErrPreconditionFailed, mr.Target) | 809 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: the target branch %q no longer exists", domain.ErrPreconditionFailed, mr.Target) |
| 789 | case tip == head: | 810 | case tip == head: | ||
| 790 | - | return domain.MergeRequest{}, fmt.Errorf("%w: nothing to merge, %q already is at %s", domain.ErrPreconditionFailed, mr.Target, head) | 811 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: nothing to merge, %q already is at %s", domain.ErrPreconditionFailed, mr.Target, head) |
| 791 | } | 812 | } | ||
| 792 | switch ok, err := m.git.IsAncestor(ctx, name, tip, head); { | 813 | switch ok, err := m.git.IsAncestor(ctx, name, tip, head); { | ||
| 793 | case err != nil: | 814 | case err != nil: | ||
| 794 | - | return domain.MergeRequest{}, err | 815 | + | return domain.MergeRequest{}, false, err |
| 795 | case !ok: | 816 | case !ok: | ||
| 796 | - | return domain.MergeRequest{}, fmt.Errorf("%w: %q is not an ancestor of %q: rebase it on %q and push again", domain.ErrPreconditionFailed, mr.Target, mr.Source, mr.Target) | 817 | + | return domain.MergeRequest{}, false, fmt.Errorf("%w: %q is not an ancestor of %q: rebase it on %q and push again", domain.ErrPreconditionFailed, mr.Target, mr.Source, mr.Target) |
| 818 | + | } | |||
| 819 | + | if params.DeleteSource { | |||
| 820 | + | if err := m.checkDeletable(ctx, name, mr); err != nil { | |||
| 821 | + | return domain.MergeRequest{}, false, err | |||
| 822 | + | } | |||
| 797 | } | 823 | } | ||
| 798 | 824 | ||||
| 799 | merged, err := m.moveTarget(ctx, mr, mode, tip, head) | 825 | merged, err := m.moveTarget(ctx, mr, mode, tip, head) | ||
| 800 | if err != nil { | 826 | if err != nil { | ||
| 801 | - | return domain.MergeRequest{}, err | 827 | + | return domain.MergeRequest{}, false, err |
| 802 | } | 828 | } | ||
| 803 | m.recordPush(ctx, name) // the target moved, whatever happens next | 829 | m.recordPush(ctx, name) // the target moved, whatever happens next | ||
| 804 | mr, err = m.markMerged(ctx, name, number, mode, tip, head, merged) | 830 | mr, err = m.markMerged(ctx, name, number, mode, tip, head, merged) | ||
| 805 | if err != nil { | 831 | if err != nil { | ||
| 806 | - | return domain.MergeRequest{}, err | 832 | + | return domain.MergeRequest{}, false, err |
| 807 | } | 833 | } | ||
| 808 | log := zerowrap.FromCtx(ctx) | 834 | log := zerowrap.FromCtx(ctx) | ||
| 809 | log.Info().Str("repo", name.String()).Int64("number", number).Str("mode", mode.String()). | 835 | log.Info().Str("repo", name.String()).Int64("number", number).Str("mode", mode.String()). | ||
| 810 | Str("target", mr.Target).Str("commit", merged.String()). | 836 | Str("target", mr.Target).Str("commit", merged.String()). | ||
| 811 | Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("merge request merged") | 837 | Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("merge request merged") | ||
| 838 | + | updates := []domain.RefUpdate{{Old: tip.String(), New: merged.String(), Ref: "refs/heads/" + mr.Target}} | |||
| 839 | + | deleted := false | |||
| 840 | + | if params.DeleteSource { | |||
| 841 | + | // The merge is done and recorded: a failure here keeps the branch. | |||
| 842 | + | switch err := m.git.DeleteBranch(context.WithoutCancel(ctx), name, mr.Source, head); { | |||
| 843 | + | case err != nil: | |||
| 844 | + | log.Warn().Err(err).Str("repo", name.String()).Int64("number", number).Str("branch", mr.Source). | |||
| 845 | + | Msg("source branch kept after the merge") | |||
| 846 | + | default: | |||
| 847 | + | deleted = true | |||
| 848 | + | updates = append(updates, domain.RefUpdate{Old: head.String(), New: domain.ZeroOID, Ref: "refs/heads/" + mr.Source}) | |||
| 849 | + | log.Info().Str("repo", name.String()).Int64("number", number).Str("branch", mr.Source). | |||
| 850 | + | Str("principal", p.Label()).Str("token_id", p.TokenID()).Msg("source branch deleted after the merge") | |||
| 851 | + | } | |||
| 852 | + | } | |||
| 812 | at := mr.UpdatedAt | 853 | at := mr.UpdatedAt | ||
| 813 | m.bus.Publish(ctx, domain.MergeRequestMerged{MergeRequest: mr, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt}) | 854 | m.bus.Publish(ctx, domain.MergeRequestMerged{MergeRequest: mr, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt}) | ||
| 814 | m.bus.Publish(ctx, domain.PushEvent{ | 855 | m.bus.Publish(ctx, domain.PushEvent{ | ||
| 815 | - | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, | 856 | + | Repo: name, At: at, Visibility: repo.Visibility, CreatedAt: repo.CreatedAt, Updates: updates, |
| 816 | - | Updates: []domain.RefUpdate{{Old: tip.String(), New: merged.String(), Ref: "refs/heads/" + mr.Target}}, | |||
| 817 | }) | 857 | }) | ||
| 818 | - | return mr, nil | 858 | + | return mr, deleted, nil |
| 859 | + | } | |||
| 860 | + | ||||
| 861 | + | // checkDeletable refuses to delete the source branch of mr when it is the | |||
| 862 | + | // default branch or the source or target of another open request. It runs | |||
| 863 | + | // under the merge lock, which also guards the creation of requests. | |||
| 864 | + | func (m *MergeRequests) checkDeletable(ctx context.Context, name domain.RepoName, mr domain.MergeRequest) error { | |||
| 865 | + | def, _, err := m.repos.DefaultBranch(ctx, name) | |||
| 866 | + | if err != nil { | |||
| 867 | + | return fmt.Errorf("default branch of %q: %w", name, err) | |||
| 868 | + | } | |||
| 869 | + | if mr.Source == def { | |||
| 870 | + | return fmt.Errorf("%w: %q is the default branch of %s: it cannot be deleted", domain.ErrPreconditionFailed, mr.Source, name) | |||
| 871 | + | } | |||
| 872 | + | all, err := m.store.List(ctx, name) | |||
| 873 | + | if err != nil { | |||
| 874 | + | return fmt.Errorf("list merge requests of %q: %w", name, err) | |||
| 875 | + | } | |||
| 876 | + | for _, o := range all { | |||
| 877 | + | if o.Number != mr.Number && o.IsOpen() && (o.Source == mr.Source || o.Target == mr.Source) { | |||
| 878 | + | return fmt.Errorf("%w: !%d still uses %q: it cannot be deleted", domain.ErrPreconditionFailed, o.Number, mr.Source) | |||
| 879 | + | } | |||
| 880 | + | } | |||
| 881 | + | return nil | |||
| 819 | } | 882 | } | ||
| 820 | 883 | ||||
| 821 | // recordPush does for a merge what Pushes.Record does for a push, as the merge | 884 | // recordPush does for a merge what Pushes.Record does for a push, as the merge | ||
A internal/app/mergerequests_delete_test.go +151 -0
| @@ -0,0 +1,151 @@ | |||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |
| 2 | + | ||
| 3 | + | package app_test | |
| 4 | + | ||
| 5 | + | import ( | |
| 6 | + | "context" | |
| 7 | + | "testing" | |
| 8 | + | ||
| 9 | + | "github.com/stretchr/testify/assert" | |
| 10 | + | "github.com/stretchr/testify/require" | |
| 11 | + | ||
| 12 | + | "bnema.dev/ermite/internal/adapters/gitcli" | |
| 13 | + | "bnema.dev/ermite/internal/app" | |
| 14 | + | "bnema.dev/ermite/internal/domain" | |
| 15 | + | ) | |
| 16 | + | ||
| 17 | + | // A merge can delete its source branch; the push event carries both refs. | |
| 18 | + | func TestMergeDeletesTheSourceBranch(t *testing.T) { | |
| 19 | + | for _, mode := range []domain.MergeMode{domain.MergeModeFastForward, domain.MergeModeSquash} { | |
| 20 | + | t.Run(string(mode), func(t *testing.T) { | |
| 21 | + | r := newMRRig(t) | |
| 22 | + | main := r.seed() | |
| 23 | + | head := r.feature("feat") | |
| 24 | + | mr := r.open("feat") | |
| 25 | + | r.drain() | |
| 26 | + | ||
| 27 | + | res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{Mode: mode, HeadID: head, DeleteSource: true}) | |
| 28 | + | require.NoError(t, err) | |
| 29 | + | assert.True(t, res.SourceDeleted) | |
| 30 | + | assert.Equal(t, domain.MRStateMerged, res.State) | |
| 31 | + | _, err = r.git.BranchHead(r.ctx, r.repo, "feat") | |
| 32 | + | assert.ErrorIs(t, err, domain.ErrNotFound, "the branch is gone") | |
| 33 | + | ||
| 34 | + | evs := r.drain() | |
| 35 | + | require.Len(t, evs, 2) | |
| 36 | + | assert.Equal(t, []domain.RefUpdate{ | |
| 37 | + | {Old: main.String(), New: res.MergedCommitID.String(), Ref: "refs/heads/main"}, | |
| 38 | + | {Old: head.String(), New: domain.ZeroOID, Ref: "refs/heads/feat"}, | |
| 39 | + | }, evs[1].(domain.PushEvent).Updates) | |
| 40 | + | ||
| 41 | + | // The merged request still reads, with its commits and diff. | |
| 42 | + | d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false) | |
| 43 | + | require.NoError(t, err) | |
| 44 | + | assert.Len(t, d.Commits, 2) | |
| 45 | + | assert.Len(t, d.Diff.Files, 2) | |
| 46 | + | }) | |
| 47 | + | } | |
| 48 | + | ||
| 49 | + | t.Run("without the option the branch stays", func(t *testing.T) { | |
| 50 | + | r := newMRRig(t) | |
| 51 | + | r.seed() | |
| 52 | + | head := r.feature("feat") | |
| 53 | + | mr := r.open("feat") | |
| 54 | + | res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head}) | |
| 55 | + | require.NoError(t, err) | |
| 56 | + | assert.False(t, res.SourceDeleted) | |
| 57 | + | assert.Equal(t, head, r.branchHead("feat")) | |
| 58 | + | }) | |
| 59 | + | } | |
| 60 | + | ||
| 61 | + | // Deleting the source is refused before the merge when the branch is still | |
| 62 | + | // needed: nothing changes. | |
| 63 | + | func TestMergeRefusesToDeleteANeededBranch(t *testing.T) { | |
| 64 | + | t.Run("another open request uses it as its source", func(t *testing.T) { | |
| 65 | + | r := newMRRig(t) | |
| 66 | + | main := r.seed() | |
| 67 | + | head := r.feature("feat") | |
| 68 | + | r.sh("checkout", "-q", "-B", "release", "main") | |
| 69 | + | r.push("release") | |
| 70 | + | mr := r.open("feat") | |
| 71 | + | _, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "to release", Source: "feat", Target: "release"}) | |
| 72 | + | require.NoError(t, err) | |
| 73 | + | ||
| 74 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |
| 75 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |
| 76 | + | assert.Contains(t, err.Error(), "!2 still uses") | |
| 77 | + | assert.Equal(t, main, r.branchHead("main"), "not merged") | |
| 78 | + | assert.Equal(t, head, r.branchHead("feat")) | |
| 79 | + | d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false) | |
| 80 | + | require.NoError(t, err) | |
| 81 | + | assert.Equal(t, domain.MRStateOpen, d.MergeRequest.State) | |
| 82 | + | }) | |
| 83 | + | ||
| 84 | + | t.Run("another open request targets it", func(t *testing.T) { | |
| 85 | + | r := newMRRig(t) | |
| 86 | + | r.seed() | |
| 87 | + | head := r.feature("feat") | |
| 88 | + | mr := r.open("feat") | |
| 89 | + | r.sh("checkout", "-q", "-B", "sub", "feat") | |
| 90 | + | r.commit(author, "d.txt", "dee\n", "add d") | |
| 91 | + | r.push("sub") | |
| 92 | + | _, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "stacked", Source: "sub", Target: "feat"}) | |
| 93 | + | require.NoError(t, err) | |
| 94 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |
| 95 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |
| 96 | + | assert.Equal(t, head, r.branchHead("feat")) | |
| 97 | + | }) | |
| 98 | + | ||
| 99 | + | t.Run("the default branch", func(t *testing.T) { | |
| 100 | + | r := newMRRig(t) | |
| 101 | + | main := r.seed() | |
| 102 | + | r.sh("checkout", "-q", "-B", "old", "main~1") | |
| 103 | + | r.push("old") | |
| 104 | + | old := r.branchHead("old") | |
| 105 | + | mr, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "back", Source: "main", Target: "old"}) | |
| 106 | + | require.NoError(t, err) | |
| 107 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: main, DeleteSource: true}) | |
| 108 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |
| 109 | + | assert.Contains(t, err.Error(), "default branch") | |
| 110 | + | assert.Equal(t, old, r.branchHead("old"), "not merged") | |
| 111 | + | assert.Equal(t, main, r.branchHead("main")) | |
| 112 | + | }) | |
| 113 | + | } | |
| 114 | + | ||
| 115 | + | // movingGit pushes to the source branch between the merge and the deletion. | |
| 116 | + | type movingGit struct { | |
| 117 | + | *gitcli.Git | |
| 118 | + | move func() | |
| 119 | + | } | |
| 120 | + | ||
| 121 | + | func (g movingGit) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |
| 122 | + | g.move() | |
| 123 | + | return g.Git.DeleteBranch(ctx, repo, branch, old) | |
| 124 | + | } | |
| 125 | + | ||
| 126 | + | // A branch that moves after the merge is kept: the merge stands. | |
| 127 | + | func TestMergeKeepsASourceThatMovedAfterTheMerge(t *testing.T) { | |
| 128 | + | r := newMRRig(t) | |
| 129 | + | r.seed() | |
| 130 | + | head := r.feature("feat") | |
| 131 | + | mr := r.open("feat") | |
| 132 | + | var moved domain.ObjectID | |
| 133 | + | mrs := app.NewMergeRequests(app.MergeRequestsConfig{ | |
| 134 | + | Meta: r.store, Store: r.store.MergeRequests(), Repos: r.git, Bus: r.bus, Clock: fixedClock{}, Maint: r.sched, | |
| 135 | + | Git: movingGit{Git: r.git, move: func() { | |
| 136 | + | moved = r.commit(author, "late.txt", "late\n", "pushed during the merge") | |
| 137 | + | r.push("feat") | |
| 138 | + | }}, | |
| 139 | + | }) | |
| 140 | + | r.drain() | |
| 141 | + | ||
| 142 | + | res, err := mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |
| 143 | + | require.NoError(t, err, "the merge stands") | |
| 144 | + | assert.False(t, res.SourceDeleted) | |
| 145 | + | assert.Equal(t, domain.MRStateMerged, res.State) | |
| 146 | + | assert.Equal(t, head, r.branchHead("main")) | |
| 147 | + | assert.Equal(t, moved, r.branchHead("feat"), "the late commit is kept") | |
| 148 | + | evs := r.drain() | |
| 149 | + | require.Len(t, evs, 2) | |
| 150 | + | assert.Len(t, evs[1].(domain.PushEvent).Updates, 1, "only the target moved") | |
| 151 | + | } | |
| @@ -0,0 +1,151 @@ | |||||
| 1 | + | // SPDX-License-Identifier: AGPL-3.0-or-later | |||
| 2 | + | ||||
| 3 | + | package app_test | |||
| 4 | + | ||||
| 5 | + | import ( | |||
| 6 | + | "context" | |||
| 7 | + | "testing" | |||
| 8 | + | ||||
| 9 | + | "github.com/stretchr/testify/assert" | |||
| 10 | + | "github.com/stretchr/testify/require" | |||
| 11 | + | ||||
| 12 | + | "bnema.dev/ermite/internal/adapters/gitcli" | |||
| 13 | + | "bnema.dev/ermite/internal/app" | |||
| 14 | + | "bnema.dev/ermite/internal/domain" | |||
| 15 | + | ) | |||
| 16 | + | ||||
| 17 | + | // A merge can delete its source branch; the push event carries both refs. | |||
| 18 | + | func TestMergeDeletesTheSourceBranch(t *testing.T) { | |||
| 19 | + | for _, mode := range []domain.MergeMode{domain.MergeModeFastForward, domain.MergeModeSquash} { | |||
| 20 | + | t.Run(string(mode), func(t *testing.T) { | |||
| 21 | + | r := newMRRig(t) | |||
| 22 | + | main := r.seed() | |||
| 23 | + | head := r.feature("feat") | |||
| 24 | + | mr := r.open("feat") | |||
| 25 | + | r.drain() | |||
| 26 | + | ||||
| 27 | + | res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{Mode: mode, HeadID: head, DeleteSource: true}) | |||
| 28 | + | require.NoError(t, err) | |||
| 29 | + | assert.True(t, res.SourceDeleted) | |||
| 30 | + | assert.Equal(t, domain.MRStateMerged, res.State) | |||
| 31 | + | _, err = r.git.BranchHead(r.ctx, r.repo, "feat") | |||
| 32 | + | assert.ErrorIs(t, err, domain.ErrNotFound, "the branch is gone") | |||
| 33 | + | ||||
| 34 | + | evs := r.drain() | |||
| 35 | + | require.Len(t, evs, 2) | |||
| 36 | + | assert.Equal(t, []domain.RefUpdate{ | |||
| 37 | + | {Old: main.String(), New: res.MergedCommitID.String(), Ref: "refs/heads/main"}, | |||
| 38 | + | {Old: head.String(), New: domain.ZeroOID, Ref: "refs/heads/feat"}, | |||
| 39 | + | }, evs[1].(domain.PushEvent).Updates) | |||
| 40 | + | ||||
| 41 | + | // The merged request still reads, with its commits and diff. | |||
| 42 | + | d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false) | |||
| 43 | + | require.NoError(t, err) | |||
| 44 | + | assert.Len(t, d.Commits, 2) | |||
| 45 | + | assert.Len(t, d.Diff.Files, 2) | |||
| 46 | + | }) | |||
| 47 | + | } | |||
| 48 | + | ||||
| 49 | + | t.Run("without the option the branch stays", func(t *testing.T) { | |||
| 50 | + | r := newMRRig(t) | |||
| 51 | + | r.seed() | |||
| 52 | + | head := r.feature("feat") | |||
| 53 | + | mr := r.open("feat") | |||
| 54 | + | res, err := r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head}) | |||
| 55 | + | require.NoError(t, err) | |||
| 56 | + | assert.False(t, res.SourceDeleted) | |||
| 57 | + | assert.Equal(t, head, r.branchHead("feat")) | |||
| 58 | + | }) | |||
| 59 | + | } | |||
| 60 | + | ||||
| 61 | + | // Deleting the source is refused before the merge when the branch is still | |||
| 62 | + | // needed: nothing changes. | |||
| 63 | + | func TestMergeRefusesToDeleteANeededBranch(t *testing.T) { | |||
| 64 | + | t.Run("another open request uses it as its source", func(t *testing.T) { | |||
| 65 | + | r := newMRRig(t) | |||
| 66 | + | main := r.seed() | |||
| 67 | + | head := r.feature("feat") | |||
| 68 | + | r.sh("checkout", "-q", "-B", "release", "main") | |||
| 69 | + | r.push("release") | |||
| 70 | + | mr := r.open("feat") | |||
| 71 | + | _, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "to release", Source: "feat", Target: "release"}) | |||
| 72 | + | require.NoError(t, err) | |||
| 73 | + | ||||
| 74 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |||
| 75 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |||
| 76 | + | assert.Contains(t, err.Error(), "!2 still uses") | |||
| 77 | + | assert.Equal(t, main, r.branchHead("main"), "not merged") | |||
| 78 | + | assert.Equal(t, head, r.branchHead("feat")) | |||
| 79 | + | d, err := r.mrs.Get(r.ctx, reader, r.repo, mr.Number, false) | |||
| 80 | + | require.NoError(t, err) | |||
| 81 | + | assert.Equal(t, domain.MRStateOpen, d.MergeRequest.State) | |||
| 82 | + | }) | |||
| 83 | + | ||||
| 84 | + | t.Run("another open request targets it", func(t *testing.T) { | |||
| 85 | + | r := newMRRig(t) | |||
| 86 | + | r.seed() | |||
| 87 | + | head := r.feature("feat") | |||
| 88 | + | mr := r.open("feat") | |||
| 89 | + | r.sh("checkout", "-q", "-B", "sub", "feat") | |||
| 90 | + | r.commit(author, "d.txt", "dee\n", "add d") | |||
| 91 | + | r.push("sub") | |||
| 92 | + | _, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "stacked", Source: "sub", Target: "feat"}) | |||
| 93 | + | require.NoError(t, err) | |||
| 94 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |||
| 95 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |||
| 96 | + | assert.Equal(t, head, r.branchHead("feat")) | |||
| 97 | + | }) | |||
| 98 | + | ||||
| 99 | + | t.Run("the default branch", func(t *testing.T) { | |||
| 100 | + | r := newMRRig(t) | |||
| 101 | + | main := r.seed() | |||
| 102 | + | r.sh("checkout", "-q", "-B", "old", "main~1") | |||
| 103 | + | r.push("old") | |||
| 104 | + | old := r.branchHead("old") | |||
| 105 | + | mr, err := r.mrs.Create(r.ctx, root, r.repo, app.CreateMergeRequestParams{Title: "back", Source: "main", Target: "old"}) | |||
| 106 | + | require.NoError(t, err) | |||
| 107 | + | _, err = r.mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: main, DeleteSource: true}) | |||
| 108 | + | require.ErrorIs(t, err, domain.ErrPreconditionFailed) | |||
| 109 | + | assert.Contains(t, err.Error(), "default branch") | |||
| 110 | + | assert.Equal(t, old, r.branchHead("old"), "not merged") | |||
| 111 | + | assert.Equal(t, main, r.branchHead("main")) | |||
| 112 | + | }) | |||
| 113 | + | } | |||
| 114 | + | ||||
| 115 | + | // movingGit pushes to the source branch between the merge and the deletion. | |||
| 116 | + | type movingGit struct { | |||
| 117 | + | *gitcli.Git | |||
| 118 | + | move func() | |||
| 119 | + | } | |||
| 120 | + | ||||
| 121 | + | func (g movingGit) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |||
| 122 | + | g.move() | |||
| 123 | + | return g.Git.DeleteBranch(ctx, repo, branch, old) | |||
| 124 | + | } | |||
| 125 | + | ||||
| 126 | + | // A branch that moves after the merge is kept: the merge stands. | |||
| 127 | + | func TestMergeKeepsASourceThatMovedAfterTheMerge(t *testing.T) { | |||
| 128 | + | r := newMRRig(t) | |||
| 129 | + | r.seed() | |||
| 130 | + | head := r.feature("feat") | |||
| 131 | + | mr := r.open("feat") | |||
| 132 | + | var moved domain.ObjectID | |||
| 133 | + | mrs := app.NewMergeRequests(app.MergeRequestsConfig{ | |||
| 134 | + | Meta: r.store, Store: r.store.MergeRequests(), Repos: r.git, Bus: r.bus, Clock: fixedClock{}, Maint: r.sched, | |||
| 135 | + | Git: movingGit{Git: r.git, move: func() { | |||
| 136 | + | moved = r.commit(author, "late.txt", "late\n", "pushed during the merge") | |||
| 137 | + | r.push("feat") | |||
| 138 | + | }}, | |||
| 139 | + | }) | |||
| 140 | + | r.drain() | |||
| 141 | + | ||||
| 142 | + | res, err := mrs.Merge(r.ctx, mrM, r.repo, mr.Number, app.MergeParams{HeadID: head, DeleteSource: true}) | |||
| 143 | + | require.NoError(t, err, "the merge stands") | |||
| 144 | + | assert.False(t, res.SourceDeleted) | |||
| 145 | + | assert.Equal(t, domain.MRStateMerged, res.State) | |||
| 146 | + | assert.Equal(t, head, r.branchHead("main")) | |||
| 147 | + | assert.Equal(t, moved, r.branchHead("feat"), "the late commit is kept") | |||
| 148 | + | evs := r.drain() | |||
| 149 | + | require.Len(t, evs, 2) | |||
| 150 | + | assert.Len(t, evs[1].(domain.PushEvent).Updates, 1, "only the target moved") | |||
| 151 | + | } | |||
M internal/ports/mocks/MergeGit.go +69 -0
| @@ -191,6 +191,75 @@ func (_c *MergeGit_CommitTree_Call) RunAndReturn(run func(ctx context.Context, r | |||
| 191 | 191 | return _c | |
| 192 | 192 | } | |
| 193 | 193 | ||
| 194 | + | // DeleteBranch provides a mock function for the type MergeGit | |
| 195 | + | func (_mock *MergeGit) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |
| 196 | + | ret := _mock.Called(ctx, repo, branch, old) | |
| 197 | + | ||
| 198 | + | if len(ret) == 0 { | |
| 199 | + | panic("no return value specified for DeleteBranch") | |
| 200 | + | } | |
| 201 | + | ||
| 202 | + | var r0 error | |
| 203 | + | if returnFunc, ok := ret.Get(0).(func(context.Context, domain.RepoName, string, domain.ObjectID) error); ok { | |
| 204 | + | r0 = returnFunc(ctx, repo, branch, old) | |
| 205 | + | } else { | |
| 206 | + | r0 = ret.Error(0) | |
| 207 | + | } | |
| 208 | + | return r0 | |
| 209 | + | } | |
| 210 | + | ||
| 211 | + | // MergeGit_DeleteBranch_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteBranch' | |
| 212 | + | type MergeGit_DeleteBranch_Call struct { | |
| 213 | + | *mock.Call | |
| 214 | + | } | |
| 215 | + | ||
| 216 | + | // DeleteBranch is a helper method to define mock.On call | |
| 217 | + | // - ctx context.Context | |
| 218 | + | // - repo domain.RepoName | |
| 219 | + | // - branch string | |
| 220 | + | // - old domain.ObjectID | |
| 221 | + | func (_e *MergeGit_Expecter) DeleteBranch(ctx any, repo any, branch any, old any) *MergeGit_DeleteBranch_Call { | |
| 222 | + | return &MergeGit_DeleteBranch_Call{Call: _e.mock.On("DeleteBranch", ctx, repo, branch, old)} | |
| 223 | + | } | |
| 224 | + | ||
| 225 | + | func (_c *MergeGit_DeleteBranch_Call) Run(run func(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID)) *MergeGit_DeleteBranch_Call { | |
| 226 | + | _c.Call.Run(func(args mock.Arguments) { | |
| 227 | + | var arg0 context.Context | |
| 228 | + | if args[0] != nil { | |
| 229 | + | arg0 = args[0].(context.Context) | |
| 230 | + | } | |
| 231 | + | var arg1 domain.RepoName | |
| 232 | + | if args[1] != nil { | |
| 233 | + | arg1 = args[1].(domain.RepoName) | |
| 234 | + | } | |
| 235 | + | var arg2 string | |
| 236 | + | if args[2] != nil { | |
| 237 | + | arg2 = args[2].(string) | |
| 238 | + | } | |
| 239 | + | var arg3 domain.ObjectID | |
| 240 | + | if args[3] != nil { | |
| 241 | + | arg3 = args[3].(domain.ObjectID) | |
| 242 | + | } | |
| 243 | + | run( | |
| 244 | + | arg0, | |
| 245 | + | arg1, | |
| 246 | + | arg2, | |
| 247 | + | arg3, | |
| 248 | + | ) | |
| 249 | + | }) | |
| 250 | + | return _c | |
| 251 | + | } | |
| 252 | + | ||
| 253 | + | func (_c *MergeGit_DeleteBranch_Call) Return(err error) *MergeGit_DeleteBranch_Call { | |
| 254 | + | _c.Call.Return(err) | |
| 255 | + | return _c | |
| 256 | + | } | |
| 257 | + | ||
| 258 | + | func (_c *MergeGit_DeleteBranch_Call) RunAndReturn(run func(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error) *MergeGit_DeleteBranch_Call { | |
| 259 | + | _c.Call.Return(run) | |
| 260 | + | return _c | |
| 261 | + | } | |
| 262 | + | ||
| 194 | 263 | // DiffCommits provides a mock function for the type MergeGit | |
| 195 | 264 | func (_mock *MergeGit) DiffCommits(ctx context.Context, repo domain.RepoName, base domain.ObjectID, head domain.ObjectID, limits domain.DiffLimits) (domain.DiffStats, domain.Diff, error) { | |
| 196 | 265 | ret := _mock.Called(ctx, repo, base, head, limits) | |
| @@ -191,6 +191,75 @@ func (_c *MergeGit_CommitTree_Call) RunAndReturn(run func(ctx context.Context, r | |||||
| 191 | return _c | 191 | return _c | ||
| 192 | } | 192 | } | ||
| 193 | 193 | ||||
| 194 | + | // DeleteBranch provides a mock function for the type MergeGit | |||
| 195 | + | func (_mock *MergeGit) DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error { | |||
| 196 | + | ret := _mock.Called(ctx, repo, branch, old) | |||
| 197 | + | ||||
| 198 | + | if len(ret) == 0 { | |||
| 199 | + | panic("no return value specified for DeleteBranch") | |||
| 200 | + | } | |||
| 201 | + | ||||
| 202 | + | var r0 error | |||
| 203 | + | if returnFunc, ok := ret.Get(0).(func(context.Context, domain.RepoName, string, domain.ObjectID) error); ok { | |||
| 204 | + | r0 = returnFunc(ctx, repo, branch, old) | |||
| 205 | + | } else { | |||
| 206 | + | r0 = ret.Error(0) | |||
| 207 | + | } | |||
| 208 | + | return r0 | |||
| 209 | + | } | |||
| 210 | + | ||||
| 211 | + | // MergeGit_DeleteBranch_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'DeleteBranch' | |||
| 212 | + | type MergeGit_DeleteBranch_Call struct { | |||
| 213 | + | *mock.Call | |||
| 214 | + | } | |||
| 215 | + | ||||
| 216 | + | // DeleteBranch is a helper method to define mock.On call | |||
| 217 | + | // - ctx context.Context | |||
| 218 | + | // - repo domain.RepoName | |||
| 219 | + | // - branch string | |||
| 220 | + | // - old domain.ObjectID | |||
| 221 | + | func (_e *MergeGit_Expecter) DeleteBranch(ctx any, repo any, branch any, old any) *MergeGit_DeleteBranch_Call { | |||
| 222 | + | return &MergeGit_DeleteBranch_Call{Call: _e.mock.On("DeleteBranch", ctx, repo, branch, old)} | |||
| 223 | + | } | |||
| 224 | + | ||||
| 225 | + | func (_c *MergeGit_DeleteBranch_Call) Run(run func(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID)) *MergeGit_DeleteBranch_Call { | |||
| 226 | + | _c.Call.Run(func(args mock.Arguments) { | |||
| 227 | + | var arg0 context.Context | |||
| 228 | + | if args[0] != nil { | |||
| 229 | + | arg0 = args[0].(context.Context) | |||
| 230 | + | } | |||
| 231 | + | var arg1 domain.RepoName | |||
| 232 | + | if args[1] != nil { | |||
| 233 | + | arg1 = args[1].(domain.RepoName) | |||
| 234 | + | } | |||
| 235 | + | var arg2 string | |||
| 236 | + | if args[2] != nil { | |||
| 237 | + | arg2 = args[2].(string) | |||
| 238 | + | } | |||
| 239 | + | var arg3 domain.ObjectID | |||
| 240 | + | if args[3] != nil { | |||
| 241 | + | arg3 = args[3].(domain.ObjectID) | |||
| 242 | + | } | |||
| 243 | + | run( | |||
| 244 | + | arg0, | |||
| 245 | + | arg1, | |||
| 246 | + | arg2, | |||
| 247 | + | arg3, | |||
| 248 | + | ) | |||
| 249 | + | }) | |||
| 250 | + | return _c | |||
| 251 | + | } | |||
| 252 | + | ||||
| 253 | + | func (_c *MergeGit_DeleteBranch_Call) Return(err error) *MergeGit_DeleteBranch_Call { | |||
| 254 | + | _c.Call.Return(err) | |||
| 255 | + | return _c | |||
| 256 | + | } | |||
| 257 | + | ||||
| 258 | + | func (_c *MergeGit_DeleteBranch_Call) RunAndReturn(run func(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error) *MergeGit_DeleteBranch_Call { | |||
| 259 | + | _c.Call.Return(run) | |||
| 260 | + | return _c | |||
| 261 | + | } | |||
| 262 | + | ||||
| 194 | // DiffCommits provides a mock function for the type MergeGit | 263 | // DiffCommits provides a mock function for the type MergeGit | ||
| 195 | func (_mock *MergeGit) DiffCommits(ctx context.Context, repo domain.RepoName, base domain.ObjectID, head domain.ObjectID, limits domain.DiffLimits) (domain.DiffStats, domain.Diff, error) { | 264 | func (_mock *MergeGit) DiffCommits(ctx context.Context, repo domain.RepoName, base domain.ObjectID, head domain.ObjectID, limits domain.DiffLimits) (domain.DiffStats, domain.Diff, error) { | ||
| 196 | ret := _mock.Called(ctx, repo, base, head, limits) | 265 | ret := _mock.Called(ctx, repo, base, head, limits) | ||
M internal/ports/ports.go +3 -0
| @@ -277,6 +277,9 @@ type MergeGit interface { | |||
| 277 | 277 | // UpdateBranch moves refs/heads/<branch> from old to new. It returns | |
| 278 | 278 | // domain.ErrConflict when the branch no longer points to old. | |
| 279 | 279 | UpdateBranch(ctx context.Context, repo domain.RepoName, branch string, old, new domain.ObjectID) error | |
| 280 | + | // DeleteBranch deletes refs/heads/<branch> if it still points to old. It | |
| 281 | + | // returns domain.ErrConflict when the branch moved or is gone. | |
| 282 | + | DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error | |
| 280 | 283 | } | |
| 281 | 284 | ||
| 282 | 285 | // CommitSigner signs the commits ermite creates (ADR 0022). The private key | |
| @@ -277,6 +277,9 @@ type MergeGit interface { | |||||
| 277 | // UpdateBranch moves refs/heads/<branch> from old to new. It returns | 277 | // UpdateBranch moves refs/heads/<branch> from old to new. It returns | ||
| 278 | // domain.ErrConflict when the branch no longer points to old. | 278 | // domain.ErrConflict when the branch no longer points to old. | ||
| 279 | UpdateBranch(ctx context.Context, repo domain.RepoName, branch string, old, new domain.ObjectID) error | 279 | UpdateBranch(ctx context.Context, repo domain.RepoName, branch string, old, new domain.ObjectID) error | ||
| 280 | + | // DeleteBranch deletes refs/heads/<branch> if it still points to old. It | |||
| 281 | + | // returns domain.ErrConflict when the branch moved or is gone. | |||
| 282 | + | DeleteBranch(ctx context.Context, repo domain.RepoName, branch string, old domain.ObjectID) error | |||
| 280 | } | 283 | } | ||
| 281 | 284 | ||||
| 282 | // CommitSigner signs the commits ermite creates (ADR 0022). The private key | 285 | // CommitSigner signs the commits ermite creates (ADR 0022). The private key | ||
M skills/ermite/CLI.md +1 -1
| @@ -55,7 +55,7 @@ A token can never grant more than the token that creates it. `--repo` (repeatabl | |||
| 55 | 55 | | `mr show <repo> <n> [--diff] [--since-last-read] [--mark-read] [--json]` | `mr:write` for `--mark-read` | commits, changed files and comments; `--diff` adds hunks with line numbers | | |
| 56 | 56 | | `mr comment <repo> <n> -m TEXT\|- [--path P --line N --side old\|new]` | `mr:write` | the line must be in the current diff; a deleted file uses its old path | | |
| 57 | 57 | | `mr resolve <repo> <n> <comment-id> [--undo]` | `mr:write` | doing it twice is not an error | | |
| 58 | - | | `mr merge <repo> <n> --head <full-id> [--squash]` | `mr:merge` | refused if the branch moved or the target is not an ancestor of the head | | |
| 58 | + | | `mr merge <repo> <n> --head <full-id> [--squash] [--delete-branch]` | `mr:merge` | refused if the branch moved or the target is not an ancestor of the head; `--delete-branch` deletes the source after the merge (refused for the default branch or a branch another open request uses; JSON adds `source_branch_deleted`) | | |
| 59 | 59 | | `mr close <repo> <n>` | `mr:write` | a closed merge request cannot be reopened | | |
| 60 | 60 | ||
| 61 | 61 | Merge request JSON: `repo`, `number`, `title`, `description`, `source_branch`, `target_branch`, `state`, `author` (token label), `created_at`, `updated_at`, `head_id`, `merged_commit_id`, `merge_mode`. `mr show --json` adds `commits`, `diff_base_id`, `stats`, `files` (with `hunks` under `--diff`: `kind`, `old_number`, `new_number`, `text`), `comments`, `last_read_id`, `last_read_lost`. | |
| @@ -55,7 +55,7 @@ A token can never grant more than the token that creates it. `--repo` (repeatabl | |||||
| 55 | | `mr show <repo> <n> [--diff] [--since-last-read] [--mark-read] [--json]` | `mr:write` for `--mark-read` | commits, changed files and comments; `--diff` adds hunks with line numbers | | 55 | | `mr show <repo> <n> [--diff] [--since-last-read] [--mark-read] [--json]` | `mr:write` for `--mark-read` | commits, changed files and comments; `--diff` adds hunks with line numbers | | ||
| 56 | | `mr comment <repo> <n> -m TEXT\|- [--path P --line N --side old\|new]` | `mr:write` | the line must be in the current diff; a deleted file uses its old path | | 56 | | `mr comment <repo> <n> -m TEXT\|- [--path P --line N --side old\|new]` | `mr:write` | the line must be in the current diff; a deleted file uses its old path | | ||
| 57 | | `mr resolve <repo> <n> <comment-id> [--undo]` | `mr:write` | doing it twice is not an error | | 57 | | `mr resolve <repo> <n> <comment-id> [--undo]` | `mr:write` | doing it twice is not an error | | ||
| 58 | - | | `mr merge <repo> <n> --head <full-id> [--squash]` | `mr:merge` | refused if the branch moved or the target is not an ancestor of the head | | 58 | + | | `mr merge <repo> <n> --head <full-id> [--squash] [--delete-branch]` | `mr:merge` | refused if the branch moved or the target is not an ancestor of the head; `--delete-branch` deletes the source after the merge (refused for the default branch or a branch another open request uses; JSON adds `source_branch_deleted`) | |
| 59 | | `mr close <repo> <n>` | `mr:write` | a closed merge request cannot be reopened | | 59 | | `mr close <repo> <n>` | `mr:write` | a closed merge request cannot be reopened | | ||
| 60 | 60 | ||||
| 61 | Merge request JSON: `repo`, `number`, `title`, `description`, `source_branch`, `target_branch`, `state`, `author` (token label), `created_at`, `updated_at`, `head_id`, `merged_commit_id`, `merge_mode`. `mr show --json` adds `commits`, `diff_base_id`, `stats`, `files` (with `hunks` under `--diff`: `kind`, `old_number`, `new_number`, `text`), `comments`, `last_read_id`, `last_read_lost`. | 61 | Merge request JSON: `repo`, `number`, `title`, `description`, `source_branch`, `target_branch`, `state`, `author` (token label), `created_at`, `updated_at`, `head_id`, `merged_commit_id`, `merge_mode`. `mr show --json` adds `commits`, `diff_base_id`, `stats`, `files` (with `hunks` under `--diff`: `kind`, `old_number`, `new_number`, `text`), `comments`, `last_read_id`, `last_read_lost`. | ||
M skills/ermite/SKILL.md +1 -1
| @@ -34,7 +34,7 @@ git push https://git.example.com/<repo>.git main # password = token, | |||
| 34 | 34 | 1. List: `ermite mr list <repo> --json`. | |
| 35 | 35 | 2. Read: `ermite mr show <repo> <n> --diff --json`. Keep `head_id`; take line numbers from the hunks. | |
| 36 | 36 | 3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`. Without `--path` the comment is general. The line must appear in the diff. | |
| 37 | - | 4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server. | |
| 37 | + | 4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards. | |
| 38 | 38 | ||
| 39 | 39 | The merge is refused when the branch moved since `head_id` (review again) or when the target is not an ancestor of the head (rebase and push again). The repository may allow only one merge mode. | |
| 40 | 40 | ||
| @@ -34,7 +34,7 @@ git push https://git.example.com/<repo>.git main # password = token, | |||||
| 34 | 1. List: `ermite mr list <repo> --json`. | 34 | 1. List: `ermite mr list <repo> --json`. | ||
| 35 | 2. Read: `ermite mr show <repo> <n> --diff --json`. Keep `head_id`; take line numbers from the hunks. | 35 | 2. Read: `ermite mr show <repo> <n> --diff --json`. Keep `head_id`; take line numbers from the hunks. | ||
| 36 | 3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`. Without `--path` the comment is general. The line must appear in the diff. | 36 | 3. Comment on a line: `ermite mr comment <repo> <n> -m "text" --path <file> --line <n> --side new`. Without `--path` the comment is general. The line must appear in the diff. | ||
| 37 | - | 4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server. | 37 | + | 4. Merge only when asked: `ermite mr merge <repo> <n> --head <head_id>`, with `--squash` for one commit signed by the server and `--delete-branch` to delete the source branch afterwards. |
| 38 | 38 | ||||
| 39 | The merge is refused when the branch moved since `head_id` (review again) or when the target is not an ancestor of the head (rebase and push again). The repository may allow only one merge mode. | 39 | The merge is refused when the branch moved since `head_id` (review again) or when the target is not an ancestor of the head (rebase and push again). The repository may allow only one merge mode. | ||
| 40 | 40 | ||||