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

GITALY_SERVER_VERSION

@@ -1 +1 @@
-1.1.0
+1.2.0

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'

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)

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

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,

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

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. |

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]}'"

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(result[:status]).to eq(:error)
-
+          expect(result[:message]).to match(original_file_path)
-          expect(results[:status]).to eq(:error)
-          expect(results[: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
+          expect(result[:status]).to eq(:success)
-          blob = project.repository.blob_at_branch(branch_name, new_file_path)
-
-          expect(results[: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