Merge branch '51083-fix-move-api' into 'master'

Commits API: Preserve file content in move operations if unspecified

Closes #52974 et #51083

See merge request gitlab-org/gitlab-ce!23387

9 files changed, 55 insertions, 17 deletions

diff --git a/GITALY_SERVER_VERSION b/GITALY_SERVER_VERSION
index 9084fa2f716..26aaba0e866 100644
--- a/GITALY_SERVER_VERSION
+++ b/GITALY_SERVER_VERSION
@@ -1 +1 @@
-1.1.0
+1.2.0
diff --git a/Gemfile b/Gemfile
index c463c46a639..32e319f4d69 100644
--- a/Gemfile
+++ b/Gemfile
@@ -432,7 +432,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC client
-gem 'gitaly-proto', '~> 1.1.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 1.2.0', require: 'gitaly'
 gem 'grpc', '~> 1.15.0'
 
 gem 'google-protobuf', '~> 3.6'
diff --git a/Gemfile.lock b/Gemfile.lock
index 96b453344a1..d7ef3b114d3 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -273,7 +273,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (1.1.0)
+    gitaly-proto (1.2.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-default_value_for (3.1.1)
@@ -1006,7 +1006,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 1.1.0)
+  gitaly-proto (~> 1.2.0)
   github-markup (~> 1.7.0)
   gitlab-default_value_for (~> 3.1.1)
   gitlab-markup (~> 1.6.5)
diff --git a/Gemfile.rails4.lock b/Gemfile.rails4.lock
index 1289a28b719..ea248dfdabe 100644
--- a/Gemfile.rails4.lock
+++ b/Gemfile.rails4.lock
@@ -272,7 +272,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
-    gitaly-proto (1.1.0)
+    gitaly-proto (1.2.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-markup (1.6.5)
@@ -998,7 +998,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly-proto (~> 1.1.0)
+  gitaly-proto (~> 1.2.0)
   github-markup (~> 1.7.0)
   gitlab-markup (~> 1.6.5)
   gitlab-sidekiq-fetcher
diff --git a/app/services/files/multi_service.rb b/app/services/files/multi_service.rb
index c9d3ee31d82..927634c2159 100644
--- a/app/services/files/multi_service.rb
+++ b/app/services/files/multi_service.rb
@@ -8,6 +8,7 @@ module Files
       transformer = Lfs::FileTransformer.new(project, @branch_name)
 
       actions = actions_after_lfs_transformation(transformer, params[:actions])
+      actions = transform_move_actions(actions)
 
       commit_actions!(actions)
     end
@@ -26,6 +27,16 @@ module Files
       end
     end
 
+    # When moving a file, `content: nil` means "use the contents of the previous
+    # file", while `content: ''` means "move the file and set it to empty"
+    def transform_move_actions(actions)
+      actions.map do |action|
+        action[:infer_content] = true if action[:content].nil?
+
+        action
+      end
+    end
+
     def commit_actions!(actions)
       repository.multi_action(
         current_user,
diff --git a/changelogs/unreleased/51083-fix-move-api.yml b/changelogs/unreleased/51083-fix-move-api.yml
new file mode 100644
index 00000000000..8838f6f267e
--- /dev/null
+++ b/changelogs/unreleased/51083-fix-move-api.yml
@@ -0,0 +1,5 @@
+---
+title: 'Commits API: Preserve file content in move operations if unspecified'
+merge_request: 23387
+author:
+type: fixed
diff --git a/doc/api/commits.md b/doc/api/commits.md
index 7d9b52ec24f..6c16216429d 100644
--- a/doc/api/commits.md
+++ b/doc/api/commits.md
@@ -87,7 +87,7 @@ POST /projects/:id/repository/commits
 | `action` | string | yes | The action to perform, `create`, `delete`, `move`, `update`, `chmod`|
 | `file_path` | string | yes | Full path to the file. Ex. `lib/class.rb` |
 | `previous_path` | string | no | Original full path to the file being moved. Ex. `lib/class1.rb`. Only considered for `move` action. |
-| `content` | string | no | File content, required for all except `delete` and `chmod`. Optional for `move` |
+| `content` | string | no | File content, required for all except `delete`, `chmod`, and `move`. Move actions that do not specify `content` will preserve the existing file content, and any other value of `content` will overwrite the file content. |
 | `encoding` | string | no | `text` or `base64`. `text` is default. |
 | `last_commit_id` | string | no | Last known file commit id. Will be only considered in update, move and delete actions. |
 | `execute_filemode` | boolean | no | When `true/false` enables/disables the execute flag on the file. Only considered for `chmod` action. |
diff --git a/lib/gitlab/gitaly_client/operation_service.rb b/lib/gitlab/gitaly_client/operation_service.rb
index c32c2c0b2fb..22d2d149e65 100644
--- a/lib/gitlab/gitaly_client/operation_service.rb
+++ b/lib/gitlab/gitaly_client/operation_service.rb
@@ -385,7 +385,8 @@ module Gitlab
           file_path: encode_binary(action[:file_path]),
           previous_path: encode_binary(action[:previous_path]),
           base64_content: action[:encoding] == 'base64',
-          execute_filemode: !!action[:execute_filemode]
+          execute_filemode: !!action[:execute_filemode],
+          infer_content: !!action[:infer_content]
         )
       rescue RangeError
         raise ArgumentError, "Unknown action '#{action[:action]}'"
diff --git a/spec/services/files/multi_service_spec.rb b/spec/services/files/multi_service_spec.rb
index 5f3c8e82715..84c48d63c64 100644
--- a/spec/services/files/multi_service_spec.rb
+++ b/spec/services/files/multi_service_spec.rb
@@ -122,26 +122,47 @@ describe Files::MultiService do
       let(:action) { 'move' }
       let(:new_file_path) { 'files/ruby/new_popen.rb' }
 
+      let(:result) { subject.execute }
+      let(:blob) { repository.blob_at_branch(branch_name, new_file_path) }
+
       context 'when original file has been updated' do
         before do
           update_file(original_file_path)
         end
 
         it 'rejects the commit' do
-          results = subject.execute
-
-          expect(results[:status]).to eq(:error)
-          expect(results[:message]).to match(original_file_path)
+          expect(result[:status]).to eq(:error)
+          expect(result[:message]).to match(original_file_path)
         end
       end
 
-      context 'when original file have not been updated' do
+      context 'when original file has not been updated' do
         it 'moves the file' do
-          results = subject.execute
-          blob = project.repository.blob_at_branch(branch_name, new_file_path)
-
-          expect(results[:status]).to eq(:success)
+          expect(result[:status]).to eq(:success)
           expect(blob).to be_present
+          expect(blob.data).to eq(file_content)
+        end
+
+        context 'when content is nil' do
+          let(:file_content) { nil }
+
+          it 'moves the existing content untouched' do
+            original_content = repository.blob_at_branch(branch_name, original_file_path).data
+
+            expect(result[:status]).to eq(:success)
+            expect(blob).to be_present
+            expect(blob.data).to eq(original_content)
+          end
+        end
+
+        context 'when content is an empty string' do
+          let(:file_content) { '' }
+
+          it 'moves the file and empties it' do
+            expect(result[:status]).to eq(:success)
+            expect(blob).not_to be_nil
+            expect(blob.data).to eq('')
+          end
         end
       end
     end