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

3 files changed, 28 insertions, 7 deletions

diff --git a/lib/api/files.rb b/lib/api/files.rb
index e6e71bac367..3176ef0e256 100644
--- a/lib/api/files.rb
+++ b/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
 
diff --git a/lib/gitlab/satellite/files/edit_file_action.rb b/lib/gitlab/satellite/files/edit_file_action.rb
index 2834b722b27..82d71ab9906 100644
--- a/lib/gitlab/satellite/files/edit_file_action.rb
+++ b/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
diff --git a/lib/gitlab/satellite/satellite.rb b/lib/gitlab/satellite/satellite.rb
index 62d1bb364d3..70125d539da 100644
--- a/lib/gitlab/satellite/satellite.rb
+++ b/lib/gitlab/satellite/satellite.rb
@@ -1,5 +1,9 @@
 module Gitlab
   module Satellite
+    class CheckoutFailed < StandardError; end
+    class CommitFailed < StandardError; end
+    class PushFailed < StandardError; end
+
     class Satellite
       include Gitlab::Popen