Merge branch 'rs-cherry-pick-revert-dry-run' into 'master'

Support dry-run cherry-picks and reverts

Closes #231032

See merge request gitlab-org/gitlab!37240

Gemfile

@@ -462,7 +462,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.2.0.pre.rc2'
+gem 'gitaly', '~> 13.3.0-rc1'
 
 gem 'grpc', '~> 1.24.0'
 

Gemfile.lock

@@ -400,7 +400,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     git (1.5.0)
-    gitaly (13.2.0.pre.rc2)
+    gitaly (13.3.0.pre.rc1)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
@@ -1262,7 +1262,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 13.2.0.pre.rc2)
+  gitaly (~> 13.3.0.pre.rc1)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-labkit (= 0.12.1)

app/models/repository.rb

@@ -852,7 +852,7 @@ class Repository
 
   def revert(
     user, commit, branch_name, message,
-    start_branch_name: nil, start_project: project)
+    start_branch_name: nil, start_project: project, dry_run: false)
 
     with_cache_hooks do
       raw_repository.revert(
@@ -861,14 +861,15 @@ class Repository
         branch_name: branch_name,
         message: message,
         start_branch_name: start_branch_name,
-        start_repository: start_project.repository.raw_repository
+        start_repository: start_project.repository.raw_repository,
+        dry_run: dry_run
       )
     end
   end
 
   def cherry_pick(
     user, commit, branch_name, message,
-    start_branch_name: nil, start_project: project)
+    start_branch_name: nil, start_project: project, dry_run: false)
 
     with_cache_hooks do
       raw_repository.cherry_pick(
@@ -877,7 +878,8 @@ class Repository
         branch_name: branch_name,
         message: message,
         start_branch_name: start_branch_name,
-        start_repository: start_project.repository.raw_repository
+        start_repository: start_project.repository.raw_repository,
+        dry_run: dry_run
       )
     end
   end

app/services/commits/change_service.rb

@@ -22,7 +22,9 @@ module Commits
         @branch_name,
         message,
         start_project: @start_project,
-        start_branch_name: @start_branch)
+        start_branch_name: @start_branch,
+        dry_run: @dry_run
+      )
     rescue Gitlab::Git::Repository::CreateTreeError => ex
       act = action.to_s.dasherize
       type = @commit.change_type_title(current_user)

app/services/commits/create_service.rb

@@ -21,6 +21,7 @@ module Commits
       @start_sha = params[:start_sha]
       @branch_name = params[:branch_name]
       @force = params[:force] || false
+      @dry_run = params[:dry_run] || false
     end
 
     def execute

changelogs/unreleased/rs-cherry-pick-revert-dry-run.yml

+---
+title: Support dry-run cherry-picks and reverts via API
+merge_request: 37240
+author:
+type: added

doc/api/commits.md

@@ -299,9 +299,10 @@ Parameters:
 
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
-| `id`      | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
+| `id`      | integer/string | yes | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user |
 | `sha` | string | yes | The commit hash  |
 | `branch` | string | yes | The name of the branch  |
+| `dry_run` | boolean | no | Does not commit any changes. Default is false. [Introduced in GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/231032) |
 
 ```shell
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" --form "branch=master" "https://gitlab.example.com/api/v4/projects/5/repository/commits/master/cherry_pick"
@@ -345,6 +346,19 @@ indicates that the commit already exists in the target branch. The other
 possible error code is `conflict`, which indicates that there was a merge
 conflict.
 
+When `dry_run` is enabled, the server will attempt to apply the cherry-pick _but
+not actually commit any resulting changes_. If the cherry-pick applies cleanly,
+the API will respond with `200 OK`:
+
+```json
+{
+  "dry_run": "success"
+}
+```
+
+In the event of a failure, you'll see an error identical to a failure without
+dry run.
+
 ## Revert a commit
 
 > [Introduced](https://gitlab.com/gitlab-org/gitlab-foss/-/merge_requests/22919) in GitLab 11.5.
@@ -362,6 +376,7 @@ Parameters:
 | `id`      | integer/string | yes      | The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) |
 | `sha`     | string         | yes      | Commit SHA to revert                                                            |
 | `branch`  | string         | yes      | Target branch name                                                              |
+| `dry_run` | boolean        | no       | Does not commit any changes. Default is false. [Introduced in GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/231032) |
 
 ```shell
 curl --request POST --header "PRIVATE-TOKEN: <your_access_token>" --form "branch=master" "https://gitlab.example.com/api/v4/projects/5/repository/commits/a738f717824ff53aebad8b090c1b79a14f2bd9e8/revert"
@@ -400,6 +415,19 @@ In this case, the revert failed because the attempted revert generated a merge
 conflict. The other possible error code is `empty`, which indicates that the
 changeset was empty, likely due to the change having already been reverted.
 
+When `dry_run` is enabled, the server will attempt to apply the revert _but not
+actually commit any resulting changes_. If the revert applies cleanly, the API
+will respond with `200 OK`:
+
+```json
+{
+  "dry_run": "success"
+}
+```
+
+In the event of a failure, you'll see an error identical to a failure without
+dry run.
+
 ## Get the diff of a commit
 
 Get the diff of a commit in a project.

lib/api/commits.rb

@@ -203,6 +203,7 @@ module API
       params do
         requires :sha, type: String, desc: 'A commit sha, or the name of a branch or tag to be cherry picked'
         requires :branch, type: String, desc: 'The name of the branch', allow_blank: false
+        optional :dry_run, type: Boolean, default: false, desc: "Does not commit any changes"
       end
       post ':id/repository/commits/:sha/cherry_pick', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
         authorize_push_to_branch!(params[:branch])
@@ -215,7 +216,8 @@ module API
         commit_params = {
           commit: commit,
           start_branch: params[:branch],
-          branch_name: params[:branch]
+          branch_name: params[:branch],
+          dry_run: params[:dry_run]
         }
 
         result = ::Commits::CherryPickService
@@ -223,10 +225,18 @@ module API
           .execute
 
         if result[:status] == :success
+          if params[:dry_run]
+            present dry_run: :success
+            status :ok
+          else
             present user_project.repository.commit(result[:result]),
               with: Entities::Commit
+          end
         else
-          error!(result.slice(:message, :error_code), 400, header)
+          response = result.slice(:message, :error_code)
+          response[:dry_run] = :error if params[:dry_run]
+
+          error!(response, 400, header)
         end
       end
 
@@ -237,6 +247,7 @@ module API
       params do
         requires :sha, type: String, desc: 'Commit SHA to revert'
         requires :branch, type: String, desc: 'Target branch name', allow_blank: false
+        optional :dry_run, type: Boolean, default: false, desc: "Does not commit any changes"
       end
       post ':id/repository/commits/:sha/revert', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS do
         authorize_push_to_branch!(params[:branch])
@@ -249,7 +260,8 @@ module API
         commit_params = {
           commit: commit,
           start_branch: params[:branch],
-          branch_name: params[:branch]
+          branch_name: params[:branch],
+          dry_run: params[:dry_run]
         }
 
         result = ::Commits::RevertService
@@ -257,10 +269,18 @@ module API
           .execute
 
         if result[:status] == :success
+          if params[:dry_run]
+            present dry_run: :success
+            status :ok
+          else
             present user_project.repository.commit(result[:result]),
               with: Entities::Commit
+          end
         else
-          error!(result.slice(:message, :error_code), 400, header)
+          response = result.slice(:message, :error_code)
+          response[:dry_run] = :error if params[:dry_run]
+
+          error!(response, 400, header)
         end
       end
 

lib/gitlab/git/repository.rb

@@ -598,14 +598,15 @@ module Gitlab
         end
       end
 
-      def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+      def revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:, dry_run: false)
         args = {
           user: user,
           commit: commit,
           branch_name: branch_name,
           message: message,
           start_branch_name: start_branch_name,
-          start_repository: start_repository
+          start_repository: start_repository,
+          dry_run: dry_run
         }
 
         wrapped_gitaly_errors do
@@ -613,14 +614,15 @@ module Gitlab
         end
       end
 
-      def cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+      def cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:, dry_run: false)
         args = {
           user: user,
           commit: commit,
           branch_name: branch_name,
           message: message,
           start_branch_name: start_branch_name,
-          start_repository: start_repository
+          start_repository: start_repository,
+          dry_run: dry_run
         }
 
         wrapped_gitaly_errors do

lib/gitlab/gitaly_client/operation_service.rb

@@ -187,24 +187,26 @@ module Gitlab
         raise Gitlab::Git::CommitError, e
       end
 
-      def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+      def user_cherry_pick(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:, dry_run: false)
         call_cherry_pick_or_revert(:cherry_pick,
                                    user: user,
                                    commit: commit,
                                    branch_name: branch_name,
                                    message: message,
                                    start_branch_name: start_branch_name,
-                                   start_repository: start_repository)
+                                   start_repository: start_repository,
+                                   dry_run: dry_run)
       end
 
-      def user_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+      def user_revert(user:, commit:, branch_name:, message:, start_branch_name:, start_repository:, dry_run: false)
         call_cherry_pick_or_revert(:revert,
                                    user: user,
                                    commit: commit,
                                    branch_name: branch_name,
                                    message: message,
                                    start_branch_name: start_branch_name,
-                                   start_repository: start_repository)
+                                   start_repository: start_repository,
+                                   dry_run: dry_run)
       end
 
       def rebase(user, rebase_id, branch:, branch_sha:, remote_repository:, remote_branch:, push_options: [])
@@ -390,7 +392,7 @@ module Gitlab
         response
       end
 
-      def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:)
+      def call_cherry_pick_or_revert(rpc, user:, commit:, branch_name:, message:, start_branch_name:, start_repository:, dry_run:)
         request_class = "Gitaly::User#{rpc.to_s.camelcase}Request".constantize
 
         request = request_class.new(
@@ -400,7 +402,8 @@ module Gitlab
           branch_name: encode_binary(branch_name),
           message: encode_binary(message),
           start_branch_name: encode_binary(start_branch_name.to_s),
-          start_repository: start_repository.gitaly_repository
+          start_repository: start_repository.gitaly_repository,
+          dry_run: dry_run
         )
 
         response = GitalyClient.call(

spec/requests/api/commits_spec.rb

@@ -1462,6 +1462,16 @@ RSpec.describe API::Commits do
           expect(json_response['author_name']).to eq(commit.author_name)
           expect(json_response['committer_name']).to eq(user.name)
         end
+
+        it 'supports dry-run without applying changes' do
+          head = project.commit(branch)
+
+          post api(route, current_user), params: { branch: branch, dry_run: true }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response).to eq("dry_run" => "success")
+          expect(project.commit(branch)).to eq(head)
+        end
       end
 
       context 'when repository is disabled' do
@@ -1533,6 +1543,14 @@ RSpec.describe API::Commits do
 
           expect(json_response['error_code']).to eq 'empty'
         end
+
+        it 'includes an additional dry_run error field when enabled' do
+          post api(route, current_user), params: { branch: 'markdown', dry_run: true }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['error_code']).to eq 'empty'
+          expect(json_response['dry_run']).to eq 'error'
+        end
       end
 
       context 'when ref contains a dot' do
@@ -1623,6 +1641,16 @@ RSpec.describe API::Commits do
           expect(json_response['committer_name']).to eq(user.name)
           expect(json_response['parent_ids']).to contain_exactly(commit_id)
         end
+
+        it 'supports dry-run without applying changes' do
+          head = project.commit(branch)
+
+          post api(route, current_user), params: { branch: branch, dry_run: true }
+
+          expect(response).to have_gitlab_http_status(:ok)
+          expect(json_response).to eq("dry_run" => "success")
+          expect(project.commit(branch)).to eq(head)
+        end
       end
 
       context 'when repository is disabled' do
@@ -1704,6 +1732,18 @@ RSpec.describe API::Commits do
           expect(response).to have_gitlab_http_status(:bad_request)
           expect(json_response['error_code']).to eq 'empty'
         end
+
+        it 'includes an additional dry_run error field when enabled' do
+          # First one actually reverts
+          post api(route, current_user), params: { branch: 'markdown' }
+
+          # Second one is redundant and should be empty
+          post api(route, current_user), params: { branch: 'markdown', dry_run: true }
+
+          expect(response).to have_gitlab_http_status(:bad_request)
+          expect(json_response['error_code']).to eq 'empty'
+          expect(json_response['dry_run']).to eq 'error'
+        end
       end
     end