Merge branch 'rs-change-service-failure-reasons' into 'master'

Provide context about why a cherry pick or revert failed

Closes #33178

See merge request gitlab-org/gitlab!19518

Gemfile

@@ -449,7 +449,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 1.65.0'
+gem 'gitaly', '~> 1.70.0'
 
 gem 'grpc', '~> 1.24.0'
 

Gemfile.lock

@@ -358,7 +358,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     git (1.5.0)
-    gitaly (1.65.0)
+    gitaly (1.70.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-labkit (0.7.0)
@@ -1167,7 +1167,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 1.65.0)
+  gitaly (~> 1.70.0)
   github-markup (~> 1.7.0)
   gitlab-labkit (~> 0.5)
   gitlab-license (~> 1.0)

app/services/base_service.rb

@@ -50,16 +50,24 @@ class BaseService
 
   private
 
-  def error(message, http_status = nil)
+  # Return a Hash with an `error` status
+  #
+  # message     - Error message to include in the Hash
+  # http_status - Optional HTTP status code override (default: nil)
+  # pass_back   - Additional attributes to be included in the resulting Hash
+  def error(message, http_status = nil, pass_back: {})
     result = {
       message: message,
       status: :error
-    }
+    }.reverse_merge(pass_back)
 
     result[:http_status] = http_status if http_status
     result
   end
 
+  # Return a Hash with a `success` status
+  #
+  # pass_back - Additional attributes to be included in the resulting Hash
   def success(pass_back = {})
     pass_back[:status] = :success
     pass_back

app/services/commits/change_service.rb

@@ -23,14 +23,15 @@ module Commits
         message,
         start_project: @start_project,
         start_branch_name: @start_branch)
-    rescue Gitlab::Git::Repository::CreateTreeError
+    rescue Gitlab::Git::Repository::CreateTreeError => ex
       act = action.to_s.dasherize
       type = @commit.change_type_title(current_user)
 
       error_msg = "Sorry, we cannot #{act} this #{type} automatically. " \
         "This #{type} may already have been #{act}ed, or a more recent " \
         "commit may have updated some of its content."
-      raise ChangeError, error_msg
+
+      raise ChangeError.new(error_msg, ex.error_code)
     end
   end
 end

app/services/commits/create_service.rb

@@ -3,7 +3,15 @@
 module Commits
   class CreateService < ::BaseService
     ValidationError = Class.new(StandardError)
-    ChangeError = Class.new(StandardError)
+    class ChangeError < StandardError
+      attr_reader :error_code
+
+      def initialize(message, error_code = nil)
+        super(message)
+
+        @error_code = error_code
+      end
+    end
 
     def initialize(*args)
       super
@@ -21,8 +29,9 @@ module Commits
       new_commit = create_commit!
 
       success(result: new_commit)
+    rescue ChangeError => ex
+      error(ex.message, pass_back: { error_code: ex.error_code })
     rescue ValidationError,
-           ChangeError,
            Gitlab::Git::Index::IndexError,
            Gitlab::Git::CommitError,
            Gitlab::Git::PreReceiveError,

changelogs/unreleased/rs-change-service-failure-reasons.yml

+---
+title: 'Add an `error_code` attribute to the API response when a cherry-pick or revert fails.'
+merge_request: 19518
+author:
+type: added

doc/api/commits.md

@@ -317,6 +317,21 @@ Example response:
 }
 ```
 
+In the event of a failed cherry-pick, the response will provide context about
+why:
+
+```json
+{
+  "message": "Sorry, we cannot cherry-pick this commit automatically. This commit may already have been cherry-picked, or a more recent commit may have updated some of its content.",
+  "error_code": "empty"
+}
+```
+
+In this case, the cherry-pick failed because the changeset was empty and likely
+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.
+
 ## Revert a commit
 
 > [Introduced][ce-22919] in GitLab 11.5.
@@ -358,6 +373,19 @@ Example response:
 }
 ```
 
+In the event of a failed revert, the response will provide context about why:
+
+```json
+{
+  "message": "Sorry, we cannot revert this commit automatically. This commit may already have been reverted, or a more recent commit may have updated some of its content.",
+  "error_code": "conflict"
+}
+```
+
+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.
+
 ## Get the diff of a commit
 
 Get the diff of a commit in a project.

lib/api/commits.rb

@@ -223,7 +223,7 @@ module API
           present user_project.repository.commit(result[:result]),
             with: Entities::Commit
         else
-          render_api_error!(result[:message], 400)
+          error!(result.slice(:message, :error_code), 400, header)
         end
       end
 
@@ -257,7 +257,7 @@ module API
           present user_project.repository.commit(result[:result]),
             with: Entities::Commit
         else
-          render_api_error!(result[:message], 400)
+          error!(result.slice(:message, :error_code), 400, header)
         end
       end
 

lib/gitlab/git/repository.rb

@@ -25,9 +25,18 @@ module Gitlab
       InvalidRef = Class.new(StandardError)
       GitError = Class.new(StandardError)
       DeleteBranchError = Class.new(StandardError)
-      CreateTreeError = Class.new(StandardError)
       TagExistsError = Class.new(StandardError)
       ChecksumError = Class.new(StandardError)
+      class CreateTreeError < StandardError
+        attr_reader :error_code
+
+        def initialize(error_code)
+          super(self.class.name)
+
+          # The value coming from Gitaly is an uppercase String (e.g., "EMPTY")
+          @error_code = error_code.downcase.to_sym
+        end
+      end
 
       # Directory name of repo
       attr_reader :name

lib/gitlab/gitaly_client/operation_service.rb

@@ -447,7 +447,7 @@ module Gitlab
         elsif response.commit_error.presence
           raise Gitlab::Git::CommitError, response.commit_error
         elsif response.create_tree_error.presence
-          raise Gitlab::Git::Repository::CreateTreeError, response.create_tree_error
+          raise Gitlab::Git::Repository::CreateTreeError, response.create_tree_error_code
         end
 
         Gitlab::Git::OperationService::BranchUpdate.from_gitaly(response.branch_update)

spec/lib/gitlab/gitaly_client/operation_service_spec.rb

@@ -211,10 +211,12 @@ describe Gitlab::GitalyClient::OperationService do
     end
 
     context 'when a create_tree_error is present' do
-      let(:response) { response_class.new(create_tree_error: "something failed") }
+      let(:response) { response_class.new(create_tree_error: "something failed", create_tree_error_code: 'EMPTY') }
 
       it 'raises a CreateTreeError' do
-        expect { subject }.to raise_error(Gitlab::Git::Repository::CreateTreeError, "something failed")
+        expect { subject }.to raise_error(Gitlab::Git::Repository::CreateTreeError) do |error|
+          expect(error.error_code).to eq(:empty)
+        end
       end
     end
 

spec/requests/api/commits_spec.rb

@@ -1376,6 +1376,12 @@ describe API::Commits do
         it_behaves_like '400 response' do
           let(:request) { post api(route, current_user), params: { branch: 'markdown' } }
         end
+
+        it 'includes an error_code in the response' do
+          post api(route, current_user), params: { branch: 'markdown' }
+
+          expect(json_response['error_code']).to eq 'empty'
+        end
       end
 
       context 'when ref contains a dot' do
@@ -1535,6 +1541,19 @@ describe API::Commits do
           let(:request) { post api(route, current_user) }
         end
       end
+
+      context 'when commit is already reverted in the target branch' do
+        it 'includes an error_code in the response' 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' }
+
+          expect(response).to have_gitlab_http_status(400)
+          expect(json_response['error_code']).to eq 'empty'
+        end
+      end
     end
 
     context 'when authenticated', 'as a developer' do