Merge branch 'api-empty-commit' into 'master'

Improve error messages when file editing fails

Give more specific errors in API responses and web UI flash messages when a file update fails.  See #1479.

Instead of returning false from `Gitlab::Satellite::Files::EditFileAction#commit!` when a `Grit::Git::CommandFailed` error is raised, now `#commit!` raises a different error depending on whether the failure happened during checkout, commit, or push.

@dzaporozhets Please let me know if you want to change the HTTP status codes or the error messages in `Files::UpdateService`

cc @sytse

See merge request !1569

CHANGELOG

 v 7.9.0 (unreleased)
   - Move labels/milestones tabs to sidebar
   - Upgrade Rails gem to version 4.1.9.
+  - Improve error messages for file edit failures
   - Improve UI for commits, issues and merge request lists
   - Fix commit comments on first line of diff not rendering in Merge Request Discussion view.
 

app/services/base_service.rb

@@ -37,11 +37,14 @@ class BaseService
 
   private
 
-  def error(message)
-    {
+  def error(message, http_status = nil)
+    result = {
       message: message,
       status: :error
     }
+
+    result[:http_status] = http_status if http_status
+    result
   end
 
   def success

app/services/files/update_service.rb

@@ -20,17 +20,19 @@ module Files
       end
 
       edit_file_action = Gitlab::Satellite::EditFileAction.new(current_user, project, ref, path)
-      created_successfully = edit_file_action.commit!(
+      edit_file_action.commit!(
         params[:content],
         params[:commit_message],
         params[:encoding]
       )
 
-      if created_successfully
-        success
-      else
-        error("Your changes could not be committed. Maybe the file was changed by another process or there was nothing to commit?")
-      end
+      success
+    rescue Gitlab::Satellite::CheckoutFailed => ex
+      error("Your changes could not be committed because ref '#{ref}' could not be checked out", 400)
+    rescue Gitlab::Satellite::CommitFailed => ex
+      error("Your changes could not be committed. Maybe there was nothing to commit?", 409)
+    rescue Gitlab::Satellite::PushFailed => ex
+      error("Your changes could not be committed. Maybe the file was changed by another process?", 409)
     end
   end
 end

lib/api/files.rb

@@ -117,7 +117,8 @@ module API
             branch_name: branch_name
           }
         else
-          render_api_error!(result[:message], 400)
+          http_status = result[:http_status] || 400
+          render_api_error!(result[:message], http_status)
         end
       end
 

lib/gitlab/satellite/files/edit_file_action.rb

@@ -15,7 +15,11 @@ module Gitlab
           prepare_satellite!(repo)
 
           # create target branch in satellite at the corresponding commit from bare repo
-          repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}")
+          begin
+            repo.git.checkout({ raise: true, timeout: true, b: true }, ref, "origin/#{ref}")
+          rescue Grit::Git::CommandFailed => ex
+            log_and_raise(CheckoutFailed, ex.message)
+          end
 
           # update the file in the satellite's working dir
           file_path_in_satellite = File.join(repo.working_dir, file_path)
@@ -31,19 +35,31 @@ module Gitlab
 
           # commit the changes
           # will raise CommandFailed when commit fails
-          repo.git.commit(raise: true, timeout: true, a: true, m: commit_message)
+          begin
+            repo.git.commit(raise: true, timeout: true, a: true, m: commit_message)
+          rescue Grit::Git::CommandFailed => ex
+            log_and_raise(CommitFailed, ex.message)
+          end
 
 
           # push commit back to bare repo
           # will raise CommandFailed when push fails
-          repo.git.push({ raise: true, timeout: true }, :origin, ref)
+          begin
+            repo.git.push({ raise: true, timeout: true }, :origin, ref)
+          rescue Grit::Git::CommandFailed => ex
+            log_and_raise(PushFailed, ex.message)
+          end
 
           # everything worked
           true
         end
-      rescue Grit::Git::CommandFailed => ex
-        Gitlab::GitLogger.error(ex.message)
-        false
+      end
+
+      private
+
+      def log_and_raise(errorClass, message)
+        Gitlab::GitLogger.error(message)
+        raise(errorClass, message)
       end
     end
   end

lib/gitlab/satellite/satellite.rb

 module Gitlab
   module Satellite
+    class CheckoutFailed < StandardError; end
+    class CommitFailed < StandardError; end
+    class PushFailed < StandardError; end
+
     class Satellite
       include Gitlab::Popen
 

spec/requests/api/files_spec.rb

@@ -98,13 +98,33 @@ describe API::API, api: true  do
       expect(response.status).to eq(400)
     end
 
-    it "should return a 400 if satellite fails to create file" do
-      Gitlab::Satellite::EditFileAction.any_instance.stub(
-        commit!: false,
-      )
+    it 'should return a 400 if the checkout fails' do
+      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
+        .and_raise(Gitlab::Satellite::CheckoutFailed)
 
       put api("/projects/#{project.id}/repository/files", user), valid_params
       expect(response.status).to eq(400)
+
+      ref = valid_params[:branch_name]
+      expect(response.body).to match("ref '#{ref}' could not be checked out")
+    end
+
+    it 'should return a 409 if the file was not modified' do
+      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
+        .and_raise(Gitlab::Satellite::CommitFailed)
+
+      put api("/projects/#{project.id}/repository/files", user), valid_params
+      expect(response.status).to eq(409)
+      expect(response.body).to match("Maybe there was nothing to commit?")
+    end
+
+    it 'should return a 409 if the push fails' do
+      Gitlab::Satellite::EditFileAction.any_instance.stub(:commit!)
+        .and_raise(Gitlab::Satellite::PushFailed)
+
+      put api("/projects/#{project.id}/repository/files", user), valid_params
+      expect(response.status).to eq(409)
+      expect(response.body).to match("Maybe the file was changed by another process?")
     end
   end