From 90d658a96d5f6a7f54a81a91078270978f3426de Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 21 Aug 2020 16:50:35 +0100
Subject: [PATCH 01/41] Sync LFS objects when push mirroring

---
 .../projects/update_remote_mirror_service.rb  | 70 ++++++++++++++++++-
 .../unreleased/16525-push-mirror-lfs-sync.yml |  5 ++
 .../development/push_mirror_syncs_lfs.yml     |  7 ++
 spec/features/projects/remote_mirror_spec.rb  | 20 ++++++
 4 files changed, 100 insertions(+), 2 deletions(-)
 create mode 100644 changelogs/unreleased/16525-push-mirror-lfs-sync.yml
 create mode 100644 config/feature_flags/development/push_mirror_syncs_lfs.yml

diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 7961f689259597..746809078dbcd7 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -38,8 +38,74 @@ def update_mirror(remote_mirror)
         message += "\n\n#{response.divergent_refs.join("\n")}"
 
         remote_mirror.mark_as_failed!(message)
-      else
-        remote_mirror.update_finish!
+        return
+      end
+
+      send_lfs_objects!(remote_mirror)
+
+      remote_mirror.update_finish!
+    end
+
+    # Minimal implementation of a git-lfs client, based on the docs here:
+    # https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
+    #
+    # The object is to send all the project's LFS objects to the remote
+    def send_lfs_objects!(remote_mirror)
+      return unless Feature.enabled?(:push_mirror_syncs_lfs, project)
+      return unless project.lfs_enabled?
+      return if project.lfs_objects.count == 0
+
+      # TODO: LFS sync should be configurable per remote mirror
+
+      # TODO: LFS sync over SSH
+      return unless remote_mirror.url =~ /\Ahttps?:\/\//i
+
+      # FIXME: do we need .git on the URL?
+      url = remote_mirror.url + "/info/lfs/objects/batch"
+      objects = project.lfs_objects.index_by(&:oid)
+
+      # TODO: we can use body_stream if we want to reduce overhead here
+      body = {
+        operation: 'upload',
+        transfers: ['basic'],
+        # We don't know `ref`, so can't send it
+        objects: objects.map { |oid, object| { oid: oid, size: object.size } }
+      }
+
+      rsp = Gitlab::HTTP.post(url, format: 'application/vnd.git-lfs+json', body: body)
+      transfer = rsp.fetch('transfer', 'basic')
+
+      raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
+
+      # TODO: we could parallelize this
+      rsp['objects'].each do |spec|
+        actions = spec.dig('actions')
+        upload = spec.dig('actions', 'upload')
+        verify = spec.dig('actions', 'verify')
+        object = objects[spec['oid']]
+
+        # The server already has this object, or we don't need to upload it
+        next unless actions && upload
+
+        # The server wants us to upload the object but something is wrong
+        unless object && object.size == spec['size']
+          logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
+          next
+        end
+
+        # TODO: we need to discover credentials in some cases. These would come
+        # from the remote mirror's credentials
+        Gitlab::HTTP.post(
+          upload['href'],
+          body_stream: object.file,
+          headers: upload['header'],
+          format: 'application/octet-stream'
+        )
+
+        # TODO: Now we've uploaded, verify the upload if requested
+        if verify
+          logger.warn("Was asked to verify #{spec['oid']} but didn't: #{verify}")
+        end
       end
     end
 
diff --git a/changelogs/unreleased/16525-push-mirror-lfs-sync.yml b/changelogs/unreleased/16525-push-mirror-lfs-sync.yml
new file mode 100644
index 00000000000000..60801c747aba1b
--- /dev/null
+++ b/changelogs/unreleased/16525-push-mirror-lfs-sync.yml
@@ -0,0 +1,5 @@
+---
+title: Sync LFS objects when push mirroring
+merge_request: 40137
+author:
+type: added
diff --git a/config/feature_flags/development/push_mirror_syncs_lfs.yml b/config/feature_flags/development/push_mirror_syncs_lfs.yml
new file mode 100644
index 00000000000000..d78fe679baab1b
--- /dev/null
+++ b/config/feature_flags/development/push_mirror_syncs_lfs.yml
@@ -0,0 +1,7 @@
+---
+name: push_mirror_syncs_lfs
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40137
+rollout_issue_url: 
+group: group::source code
+type: development
+default_enabled: false
diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 26d27c914cc79c..1a2bc9cf9b4730 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -33,6 +33,26 @@
       end
     end
 
+    context 'pushing to a remote' do
+      let(:remote_project) { create(:project, :empty_repo) }
+
+      before do
+        remote_project.add_maintainer(user)
+
+        # FIXME: currently blocked
+        remote_mirror.update!(url: remote_project.http_url_to_repo)
+      end
+
+      it 'transfers code and LFS objects' do
+        lfs_object = create(:lfs_objects_project, project: project).lfs_object
+
+        Projects::UpdateRemoveMirrorService.new(project, user).execute(remote_mirror)
+
+        expect(remote_project.lfs_objects.reload.count).to eq(1)
+        expect(remote_project.commit).to eq(project.commit)
+      end
+    end
+
     def expect_mirror_to_have_error_and_timeago(timeago)
       row = first('.js-mirrors-table-body tr')
       expect(row).to have_content('Error')
-- 
GitLab


From 1e47392183ed52ec90185f8f3f85ae183e897c29 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 25 Aug 2020 10:11:17 -0700
Subject: [PATCH 02/41] Move lfs_object into a let for rubocop

---
 spec/features/projects/remote_mirror_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 1a2bc9cf9b4730..41e6eeb55122b0 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -35,6 +35,7 @@
 
     context 'pushing to a remote' do
       let(:remote_project) { create(:project, :empty_repo) }
+      let!(:lfs_object) { create(:lfs_objects_project, project: project).lfs_object }
 
       before do
         remote_project.add_maintainer(user)
@@ -44,8 +45,6 @@
       end
 
       it 'transfers code and LFS objects' do
-        lfs_object = create(:lfs_objects_project, project: project).lfs_object
-
         Projects::UpdateRemoveMirrorService.new(project, user).execute(remote_mirror)
 
         expect(remote_project.lfs_objects.reload.count).to eq(1)
-- 
GitLab


From 2534ce23b4092b8eb75498e8f9fd7fb1212f2a10 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 25 Aug 2020 10:11:31 -0700
Subject: [PATCH 03/41] Fix typo in class name

---
 spec/features/projects/remote_mirror_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 41e6eeb55122b0..7ec623700b87de 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -45,7 +45,7 @@
       end
 
       it 'transfers code and LFS objects' do
-        Projects::UpdateRemoveMirrorService.new(project, user).execute(remote_mirror)
+        Projects::UpdateRemoteMirrorService.new(project, user).execute(remote_mirror)
 
         expect(remote_project.lfs_objects.reload.count).to eq(1)
         expect(remote_project.commit).to eq(project.commit)
-- 
GitLab


From c28e1e97bef68ffc941614ed12889fdfc3049740 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 25 Aug 2020 12:33:07 -0700
Subject: [PATCH 04/41] Additional test setup to get past localhost restriction

---
 spec/features/projects/remote_mirror_spec.rb | 27 +++++++++++++++++---
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 7ec623700b87de..7bd9320f3118b1 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -34,18 +34,37 @@
     end
 
     context 'pushing to a remote' do
-      let(:remote_project) { create(:project, :empty_repo) }
+      let(:remote_project) { create(:forked_project_with_submodules) }
       let!(:lfs_object) { create(:lfs_objects_project, project: project).lfs_object }
+      let(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
 
       before do
         remote_project.add_maintainer(user)
 
-        # FIXME: currently blocked
-        remote_mirror.update!(url: remote_project.http_url_to_repo)
+        project.update_attribute(:lfs_enabled, true)
+
+        allow(remote_mirror)
+          .to receive(:update_repository)
+          .and_return(double(divergent_refs: []))
+        allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+
+        # TODO: This needs to return a response as if it were our remote LFS
+        #   server.
+        #
+        stub_request(:post, "http://test.com/info/lfs/objects/batch")
+          .with(
+            body: "operation=upload&transfers[]=basic&objects[][oid]=b68143e6463773b1b6c6fd009a76c32aeec041faff32ba2ed42fd7f708a00001&objects[][size]=499013",
+            headers: {
+              'Accept'          => '*/*',
+              'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
+              'Authorization'   => 'Basic Zm9vOmJhcg==',
+              'User-Agent'      => 'Ruby'
+            }
+          ).to_return(status: 200, body: "", headers: {})
       end
 
       it 'transfers code and LFS objects' do
-        Projects::UpdateRemoteMirrorService.new(project, user).execute(remote_mirror)
+        Projects::UpdateRemoteMirrorService.new(project, user).execute(remote_mirror, 0)
 
         expect(remote_project.lfs_objects.reload.count).to eq(1)
         expect(remote_project.commit).to eq(project.commit)
-- 
GitLab


From dc901f536e5e421fca369718184f3d884ded737f Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Tue, 25 Aug 2020 15:53:28 -0700
Subject: [PATCH 05/41] Extract RemoteMirrorLfsObjectUploaderWorker

This will allow us to parallelize the work involved in uploading an
arbitrary number of LFS objects to the remote mirror.
---
 .../projects/update_remote_mirror_service.rb  |  29 +----
 ...emote_mirror_lfs_object_uploader_worker.rb |  41 +++++++
 ..._mirror_lfs_object_uploader_worker_spec.rb | 109 ++++++++++++++++++
 3 files changed, 151 insertions(+), 28 deletions(-)
 create mode 100644 app/workers/remote_mirror_lfs_object_uploader_worker.rb
 create mode 100644 spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb

diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 746809078dbcd7..97c8eaf969f02d 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -77,35 +77,8 @@ def send_lfs_objects!(remote_mirror)
 
       raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
 
-      # TODO: we could parallelize this
       rsp['objects'].each do |spec|
-        actions = spec.dig('actions')
-        upload = spec.dig('actions', 'upload')
-        verify = spec.dig('actions', 'verify')
-        object = objects[spec['oid']]
-
-        # The server already has this object, or we don't need to upload it
-        next unless actions && upload
-
-        # The server wants us to upload the object but something is wrong
-        unless object && object.size == spec['size']
-          logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
-          next
-        end
-
-        # TODO: we need to discover credentials in some cases. These would come
-        # from the remote mirror's credentials
-        Gitlab::HTTP.post(
-          upload['href'],
-          body_stream: object.file,
-          headers: upload['header'],
-          format: 'application/octet-stream'
-        )
-
-        # TODO: Now we've uploaded, verify the upload if requested
-        if verify
-          logger.warn("Was asked to verify #{spec['oid']} but didn't: #{verify}")
-        end
+        RemoteMirrorLfsObjectUploaderWorker.perform_async(spec, objects[spec['oid']])
       end
     end
 
diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
new file mode 100644
index 00000000000000..85df35c62e418f
--- /dev/null
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+class RemoteMirrorLfsObjectUploaderWorker # rubocop:disable Scalability/IdempotentWorker
+  include ApplicationWorker
+
+  feature_category :source_code_management
+  weight 2
+
+  def perform(spec, object)
+    actions = spec.dig('actions')
+    upload = spec.dig('actions', 'upload')
+    verify = spec.dig('actions', 'verify')
+
+    # The server already has this object, or we don't need to upload it
+    #
+    return unless actions && upload
+
+    # The server wants us to upload the object but something is wrong
+    #
+    unless object && object.size == spec['size']
+      logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
+      return
+    end
+
+    # TODO: we need to discover credentials in some cases. These would come from
+    #   the remote mirror's credentials
+    #
+    Gitlab::HTTP.post(
+      upload['href'],
+      body_stream: object.file,
+      headers: upload['header'],
+      format: 'application/octet-stream'
+    )
+
+    # TODO: Now we've uploaded, verify the upload if requested
+    #
+    if verify
+      logger.warn("Was asked to verify #{spec['oid']} but didn't: #{verify}")
+    end
+  end
+end
diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
new file mode 100644
index 00000000000000..664fd027baaff3
--- /dev/null
+++ b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
@@ -0,0 +1,109 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe RemoteMirrorLfsObjectUploaderWorker do
+  # let_it_be(:project) { create(:project, :repository, :remote_mirror) }
+  # let_it_be(:mirror) { project.remote_mirrors.first }
+  let(:logger) { subject.send(:logger) }
+
+  let(:object) { create(:lfs_object) }
+
+  let(:spec) do
+    {
+      "oid" => object.oid,
+      "size" => object.size,
+      "actions" => {
+        "upload" => {
+          "href" => "https://example.com/some/file",
+          "header" => {
+            "Key" => "value"
+          }
+        },
+        "verify" => {
+          "href" => "https://example.com/some/file/verify",
+          "header" => {
+            "Key" => "value"
+          }
+        }
+      }
+    }
+  end
+
+  shared_examples "returns early without attempting upload" do
+    it "returns early without attempting upload" do
+      expect(Gitlab::HTTP).not_to receive(:post)
+
+      expect(subject.perform(spec, object)).to be_nil
+    end
+  end
+
+  describe "#perform" do
+    context "upload to remote server is requested" do
+      it "attempts to upload the LFS object" do
+        expect(Gitlab::HTTP).to receive(:post)
+
+        subject.perform(spec, object)
+      end
+
+      context "when 'verify' is requested" do
+        before do
+          expect(Gitlab::HTTP).to receive(:post)
+        end
+
+        it "logs a warning about the lack of a verify routine" do
+          expect(logger)
+            .to receive(:warn)
+            .with("Was asked to verify #{spec['oid']} but didn't: #{spec['actions']['verify']}")
+
+          subject.perform(spec, object)
+        end
+      end
+
+      context "when 'verify' is missing" do
+        before do
+          expect(Gitlab::HTTP).to receive(:post)
+          spec['actions'].delete('verify')
+        end
+
+        it "logs a warning about the lack of a verify routine" do
+          expect(Gitlab::HTTP).not_to receive(:post)
+
+          subject.perform(spec, object)
+        end
+      end
+    end
+
+    context "missing 'actions'" do
+      before do
+        spec.delete('actions')
+      end
+
+      it_behaves_like "returns early without attempting upload"
+    end
+
+    context "missing 'upload' action" do
+      before do
+        spec['actions'].delete('upload')
+      end
+
+      it_behaves_like "returns early without attempting upload"
+    end
+
+    context "object and and spec size do not match" do
+      before do
+        spec['size'] = object.size + 1
+      end
+
+      it_behaves_like "returns early without attempting upload"
+
+      it "logs a warning about the size mismatch" do
+        expect(logger)
+          .to receive(:warn)
+          .with("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
+
+        subject.perform(spec, object)
+      end
+    end
+  end
+end
-- 
GitLab


From 819addd469597bd86efa29871d6767c53c541aa4 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 10:29:40 -0700
Subject: [PATCH 06/41] Add Gitlab::Lfs::Client to hold HTTP actions

The beginnings of a basic implementation of an LFS client for uploading
and verifying files to remote servers.
---
 lib/gitlab/lfs/client.rb           | 24 ++++++++++++++
 spec/lib/gitlab/lfs/client_spec.rb | 50 ++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)
 create mode 100644 lib/gitlab/lfs/client.rb
 create mode 100644 spec/lib/gitlab/lfs/client_spec.rb

diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
new file mode 100644
index 00000000000000..3277d519d54d19
--- /dev/null
+++ b/lib/gitlab/lfs/client.rb
@@ -0,0 +1,24 @@
+# frozen_string_literal: true
+module Gitlab
+  module Lfs
+    class Client
+      def upload(object, upload_action)
+        # TODO: we need to discover credentials in some cases. These would come
+        #   from the remote mirror's credentials
+        #
+        Gitlab::HTTP.post(
+          upload_action['href'],
+          body_stream: object.file,
+          headers: upload_action['header'],
+          format: 'application/octet-stream'
+        )
+      end
+
+      # TODO: verify the file
+      #
+      def verify(object, verify_action)
+        # noop
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
new file mode 100644
index 00000000000000..599f4a2e495397
--- /dev/null
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -0,0 +1,50 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Gitlab::Lfs::Client do
+  let(:object) { create(:lfs_object) }
+
+  let(:upload_action) do
+    {
+      "upload" => {
+        "href" => "https://example.com/some/file",
+        "header" => {
+          "Key" => "value"
+        }
+      }
+    }
+  end
+
+  let(:verify_action) do
+    {
+      "verify" => {
+        "href" => "https://example.com/some/file/verify",
+        "header" => {
+          "Key" => "value"
+        }
+      }
+    }
+  end
+
+  describe "#upload" do
+    it "makes an HTTP post with expected parameters" do
+      expect(Gitlab::HTTP)
+        .to receive(:post)
+        .with(
+          upload_action['href'],
+          body_stream: object.file,
+          headers:     upload_action['header'],
+          format:      'application/octet-stream'
+        )
+
+      subject.upload(object, upload_action)
+    end
+  end
+
+  describe "#verify" do
+    it "does nothing" do
+      expect(subject.verify(object, verify_action)).to be_nil
+    end
+  end
+end
-- 
GitLab


From 87d2a2475e69f6835633756d2accd8dac4233bf5 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 10:31:04 -0700
Subject: [PATCH 07/41] Replace HTTP actions with calls to Gitlab::Lfs::Client

---
 ...emote_mirror_lfs_object_uploader_worker.rb | 22 +++++++--------
 ..._mirror_lfs_object_uploader_worker_spec.rb | 27 +++++++++----------
 2 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
index 85df35c62e418f..f0e5ee174b390e 100644
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -22,20 +22,16 @@ def perform(spec, object)
       return
     end
 
-    # TODO: we need to discover credentials in some cases. These would come from
-    #   the remote mirror's credentials
-    #
-    Gitlab::HTTP.post(
-      upload['href'],
-      body_stream: object.file,
-      headers: upload['header'],
-      format: 'application/octet-stream'
-    )
-
-    # TODO: Now we've uploaded, verify the upload if requested
-    #
+    lfs_client.upload(object, upload)
+
     if verify
-      logger.warn("Was asked to verify #{spec['oid']} but didn't: #{verify}")
+      lfs_client.verify(object, verify)
     end
   end
+
+  private
+
+  def lfs_client
+    @_lfs_client ||= Gitlab::Lfs::Client.new
+  end
 end
diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
index 664fd027baaff3..5c08e466a40536 100644
--- a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
+++ b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
@@ -32,7 +32,7 @@
 
   shared_examples "returns early without attempting upload" do
     it "returns early without attempting upload" do
-      expect(Gitlab::HTTP).not_to receive(:post)
+      expect(Gitlab::Lfs::Client).not_to receive(:upload)
 
       expect(subject.perform(spec, object)).to be_nil
     end
@@ -40,35 +40,34 @@
 
   describe "#perform" do
     context "upload to remote server is requested" do
-      it "attempts to upload the LFS object" do
-        expect(Gitlab::HTTP).to receive(:post)
+      before do
+        expect_next_instance_of(Gitlab::Lfs::Client) do |instance|
+          expect(instance).to receive(:upload)
+        end
+      end
 
+      it "attempts to upload the LFS object" do
         subject.perform(spec, object)
       end
 
-      context "when 'verify' is requested" do
+      context "when 'verify' action is present" do
         before do
-          expect(Gitlab::HTTP).to receive(:post)
+          expect_any_instance_of(Gitlab::Lfs::Client).to receive(:verify).and_call_original
         end
 
         it "logs a warning about the lack of a verify routine" do
-          expect(logger)
-            .to receive(:warn)
-            .with("Was asked to verify #{spec['oid']} but didn't: #{spec['actions']['verify']}")
-
           subject.perform(spec, object)
         end
       end
 
-      context "when 'verify' is missing" do
+      context "when 'verify' action is missing" do
         before do
-          expect(Gitlab::HTTP).to receive(:post)
           spec['actions'].delete('verify')
-        end
 
-        it "logs a warning about the lack of a verify routine" do
-          expect(Gitlab::HTTP).not_to receive(:post)
+          expect_any_instance_of(Gitlab::Lfs::Client).not_to receive(:verify).and_call_original
+        end
 
+        it "does not attempt to verify the object" do
           subject.perform(spec, object)
         end
       end
-- 
GitLab


From d796232108500319e48c09fc4aed447f9385552e Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 10:31:19 -0700
Subject: [PATCH 08/41] Remove commented out test setup

---
 spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
index 5c08e466a40536..80d350c3302fef 100644
--- a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
+++ b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
@@ -3,8 +3,6 @@
 require 'spec_helper'
 
 RSpec.describe RemoteMirrorLfsObjectUploaderWorker do
-  # let_it_be(:project) { create(:project, :repository, :remote_mirror) }
-  # let_it_be(:mirror) { project.remote_mirrors.first }
   let(:logger) { subject.send(:logger) }
 
   let(:object) { create(:lfs_object) }
-- 
GitLab


From bb44dca70cef9fc5f86073131e8462a48c5e3d18 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 14:05:06 -0700
Subject: [PATCH 09/41] Update sidekiq queue metadata

---
 app/workers/all_queues.yml | 8 ++++++++
 config/sidekiq_queues.yml  | 2 ++
 2 files changed, 10 insertions(+)

diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 451decce9fb470..1a5a77df162152 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -1724,6 +1724,14 @@
   :weight: 2
   :idempotent:
   :tags: []
+- :name: remote_mirror_lfs_object_uploader
+  :feature_category: :source_code_management
+  :has_external_dependencies:
+  :urgency: :low
+  :resource_boundary: :unknown
+  :weight: 2
+  :idempotent:
+  :tags: []
 - :name: remote_mirror_notification
   :feature_category: :source_code_management
   :has_external_dependencies:
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index cb85b5b6d1e190..99413d0c8c9384 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -238,6 +238,8 @@
   - 2
 - - refresh_license_compliance_checks
   - 2
+- - remote_mirror_lfs_object_uploader
+  - 2
 - - remote_mirror_notification
   - 2
 - - repository_check
-- 
GitLab


From 3dc60e181a1e526d1e259b09434703557b079da6 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 14:12:30 -0700
Subject: [PATCH 10/41] Use oid and size from the lfs_object directly

---
 spec/features/projects/remote_mirror_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 7bd9320f3118b1..8a4220c92bff5a 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -53,7 +53,7 @@
         #
         stub_request(:post, "http://test.com/info/lfs/objects/batch")
           .with(
-            body: "operation=upload&transfers[]=basic&objects[][oid]=b68143e6463773b1b6c6fd009a76c32aeec041faff32ba2ed42fd7f708a00001&objects[][size]=499013",
+            body: "operation=upload&transfers[]=basic&objects[][oid]=#{lfs_object.oid}&objects[][size]=#{lfs_object.size}",
             headers: {
               'Accept'          => '*/*',
               'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
-- 
GitLab


From 09ab63a609a88a2b4672bb1b5b8aed0a4f625aa7 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 26 Aug 2020 14:12:46 -0700
Subject: [PATCH 11/41] Mark failing test as pending for now

---
 spec/features/projects/remote_mirror_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index 8a4220c92bff5a..bf34dcf16ff327 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -63,7 +63,7 @@
           ).to_return(status: 200, body: "", headers: {})
       end
 
-      it 'transfers code and LFS objects' do
+      xit 'transfers code and LFS objects' do
         Projects::UpdateRemoteMirrorService.new(project, user).execute(remote_mirror, 0)
 
         expect(remote_project.lfs_objects.reload.count).to eq(1)
-- 
GitLab


From 5bcc4d0e436417070ed27b13f1e935d200e1772a Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Thu, 27 Aug 2020 21:35:01 -0700
Subject: [PATCH 12/41] Avoid queueing jobs when we would be unable to upload

---
 .../projects/update_remote_mirror_service.rb  | 17 ++++-
 ...emote_mirror_lfs_object_uploader_worker.rb | 12 ----
 .../update_remote_mirror_service_spec.rb      | 72 +++++++++++++++++++
 ..._mirror_lfs_object_uploader_worker_spec.rb | 40 -----------
 4 files changed, 88 insertions(+), 53 deletions(-)

diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 97c8eaf969f02d..8e65e385394b86 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -78,7 +78,22 @@ def send_lfs_objects!(remote_mirror)
       raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
 
       rsp['objects'].each do |spec|
-        RemoteMirrorLfsObjectUploaderWorker.perform_async(spec, objects[spec['oid']])
+        actions = spec.dig('actions')
+        upload = spec.dig('actions', 'upload')
+        object = objects[spec['oid']]
+
+        # The server already has this object, or we don't need to upload it
+        #
+        next unless actions && upload
+
+        # The server wants us to upload the object but something is wrong
+        #
+        unless object && object.size == spec['size']
+          Rails.logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object") # rubocop:disable Gitlab/RailsLogger
+          next
+        end
+
+        RemoteMirrorLfsObjectUploaderWorker.perform_async(spec, object)
       end
     end
 
diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
index f0e5ee174b390e..5f71bc2033b3a6 100644
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -7,21 +7,9 @@ class RemoteMirrorLfsObjectUploaderWorker # rubocop:disable Scalability/Idempote
   weight 2
 
   def perform(spec, object)
-    actions = spec.dig('actions')
     upload = spec.dig('actions', 'upload')
     verify = spec.dig('actions', 'verify')
 
-    # The server already has this object, or we don't need to upload it
-    #
-    return unless actions && upload
-
-    # The server wants us to upload the object but something is wrong
-    #
-    unless object && object.size == spec['size']
-      logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
-      return
-    end
-
     lfs_client.upload(object, upload)
 
     if verify
diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb
index a452f88c75f7aa..df4ec75d304f05 100644
--- a/spec/services/projects/update_remote_mirror_service_spec.rb
+++ b/spec/services/projects/update_remote_mirror_service_spec.rb
@@ -127,5 +127,77 @@
         expect(remote_mirror.last_error).to include("refs/heads/develop")
       end
     end
+
+    context "sending lfs objects" do
+      let!(:lfs_object) { create(:lfs_objects_project, project: project).lfs_object }
+      let(:sample_lfs_object) { project.lfs_objects.first }
+      let(:spec) do
+        {
+          "objects" => [{
+            "oid" => sample_lfs_object.oid,
+            "size" => sample_lfs_object.size,
+            "actions" => {
+              "upload" => {
+                "href" => "https://example.com/some/file",
+                "header" => {
+                  "Key" => "value"
+                }
+              },
+              "verify" => {
+                "href" => "https://example.com/some/file/verify",
+                "header" => {
+                  "Key" => "value"
+                }
+              }
+            }
+          }]
+        }
+      end
+
+      shared_examples "returns early without attempting upload" do
+        it "returns early without attempting upload" do
+          expect(Gitlab::Lfs::Client).not_to receive(:upload)
+
+          execute!
+        end
+      end
+
+      context "object and and spec size do not match" do
+        before do
+          project.update_attribute(:lfs_enabled, true)
+          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+
+          spec['objects'].first['size'] = sample_lfs_object.size + 1
+
+          expect(Gitlab::HTTP).to receive(:post).and_return(spec)
+        end
+
+        it_behaves_like "returns early without attempting upload"
+
+        it "logs a warning about the size mismatch" do
+          expect(Rails.logger)
+            .to receive(:warn)
+            .with("Couldn't match #{spec['objects'].first['oid']} at size #{spec['objects'].first['size']} with an LFS object")
+
+          execute!
+        end
+      end
+
+      context "missing 'actions'" do
+        before do
+          spec['objects'].first.delete('actions')
+        end
+
+        it_behaves_like "returns early without attempting upload"
+      end
+
+      context "missing 'upload' action" do
+        before do
+          spec['objects'].first['actions'].delete('upload')
+        end
+
+        it_behaves_like "returns early without attempting upload"
+      end
+    end
   end
 end
diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
index 80d350c3302fef..74c916ad856633 100644
--- a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
+++ b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
@@ -28,14 +28,6 @@
     }
   end
 
-  shared_examples "returns early without attempting upload" do
-    it "returns early without attempting upload" do
-      expect(Gitlab::Lfs::Client).not_to receive(:upload)
-
-      expect(subject.perform(spec, object)).to be_nil
-    end
-  end
-
   describe "#perform" do
     context "upload to remote server is requested" do
       before do
@@ -70,37 +62,5 @@
         end
       end
     end
-
-    context "missing 'actions'" do
-      before do
-        spec.delete('actions')
-      end
-
-      it_behaves_like "returns early without attempting upload"
-    end
-
-    context "missing 'upload' action" do
-      before do
-        spec['actions'].delete('upload')
-      end
-
-      it_behaves_like "returns early without attempting upload"
-    end
-
-    context "object and and spec size do not match" do
-      before do
-        spec['size'] = object.size + 1
-      end
-
-      it_behaves_like "returns early without attempting upload"
-
-      it "logs a warning about the size mismatch" do
-        expect(logger)
-          .to receive(:warn)
-          .with("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object")
-
-        subject.perform(spec, object)
-      end
-    end
   end
 end
-- 
GitLab


From ad7a28570d2806b4b506a42a11f2e77d4cbdd0a5 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Wed, 2 Sep 2020 14:56:28 +0100
Subject: [PATCH 13/41] Batch the LFS batch endpoint; resolve a data dependency

---
 .../projects/update_remote_mirror_service.rb  | 57 ++++++++-----------
 ...emote_mirror_lfs_object_uploader_worker.rb |  7 ++-
 lib/gitlab/lfs/client.rb                      | 32 +++++++++++
 3 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 8e65e385394b86..5b63b9ce0f3e3d 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -60,40 +60,31 @@ def send_lfs_objects!(remote_mirror)
       # TODO: LFS sync over SSH
       return unless remote_mirror.url =~ /\Ahttps?:\/\//i
 
-      # FIXME: do we need .git on the URL?
-      url = remote_mirror.url + "/info/lfs/objects/batch"
-      objects = project.lfs_objects.index_by(&:oid)
-
-      # TODO: we can use body_stream if we want to reduce overhead here
-      body = {
-        operation: 'upload',
-        transfers: ['basic'],
-        # We don't know `ref`, so can't send it
-        objects: objects.map { |oid, object| { oid: oid, size: object.size } }
-      }
-
-      rsp = Gitlab::HTTP.post(url, format: 'application/vnd.git-lfs+json', body: body)
-      transfer = rsp.fetch('transfer', 'basic')
-
-      raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
-
-      rsp['objects'].each do |spec|
-        actions = spec.dig('actions')
-        upload = spec.dig('actions', 'upload')
-        object = objects[spec['oid']]
-
-        # The server already has this object, or we don't need to upload it
-        #
-        next unless actions && upload
-
-        # The server wants us to upload the object but something is wrong
-        #
-        unless object && object.size == spec['size']
-          Rails.logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object") # rubocop:disable Gitlab/RailsLogger
-          next
-        end
+      lfs_client = Gitlab::Lfs::Client.new(remote_mirror.url) # FIXME: do we need .git on the URL?
+
+      project.lfs_objects.each_batch do |objects|
+        rsp = lfs_client.batch('upload', objects)
+        objects = objects.index_by(&:oid)
+
+        rsp['objects'].each do |spec|
+          actions = spec.dig('actions')
+          upload = spec.dig('actions', 'upload')
+
+          # The server already has this object, or we don't need to upload it
+          #
+          next unless actions && upload
 
-        RemoteMirrorLfsObjectUploaderWorker.perform_async(spec, object)
+          object = objects[spec['oid']]
+
+          # The server wants us to upload the object but something is wrong
+          #
+          unless object && object.size == spec['size']
+            Rails.logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object") # rubocop:disable Gitlab/RailsLogger
+            next
+          end
+
+          RemoteMirrorLfsObjectUploaderWorker.perform_async(remote_mirror.id, spec, object)
+        end
       end
     end
 
diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
index 5f71bc2033b3a6..468a0bb0e85f6e 100644
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -6,7 +6,10 @@ class RemoteMirrorLfsObjectUploaderWorker # rubocop:disable Scalability/Idempote
   feature_category :source_code_management
   weight 2
 
-  def perform(spec, object)
+  def perform(remote_mirror_id, spec, object)
+    remote_mirror = RemoteMirror.find_by_id(remote_mirror_id)
+    return unless remote_mirror&.enabled?
+
     upload = spec.dig('actions', 'upload')
     verify = spec.dig('actions', 'verify')
 
@@ -20,6 +23,6 @@ def perform(spec, object)
   private
 
   def lfs_client
-    @_lfs_client ||= Gitlab::Lfs::Client.new
+    @_lfs_client ||= Gitlab::Lfs::Client.new(remote_mirror.url)
   end
 end
diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index 3277d519d54d19..0b7a84a96501db 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -2,6 +2,32 @@
 module Gitlab
   module Lfs
     class Client
+      attr_reader :base_url
+
+      def initialize(base_url)
+        @base_url = base_url
+      end
+
+      def batch(operation, objects)
+        body = {
+          operation: operation,
+          transfers: ['basic'],
+          # We don't know `ref`, so can't send it
+          objects: objects.map { |object| { oid: object.oid, size: object.size } }
+        }
+
+        rsp = Gitlab::HTTP.post(
+          batch_url,
+          format: 'application/vnd.git-lfs+json',
+          body: body
+        )
+
+        transfer = rsp.fetch('transfer', 'basic')
+        raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
+
+        rsp
+      end
+
       def upload(object, upload_action)
         # TODO: we need to discover credentials in some cases. These would come
         #   from the remote mirror's credentials
@@ -19,6 +45,12 @@ def upload(object, upload_action)
       def verify(object, verify_action)
         # noop
       end
+
+      private
+
+      def batch_url
+        base_url + '/info/lfs/objects/batch'
+      end
     end
   end
 end
-- 
GitLab


From b5c9eb6c5b22245e11734c0a5ea68ebf99a62d34 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 2 Sep 2020 15:53:25 -0600
Subject: [PATCH 14/41] Add base_url to subject instantiation

---
 spec/lib/gitlab/lfs/client_spec.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index 599f4a2e495397..f173dceeaba85e 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -4,11 +4,12 @@
 
 RSpec.describe Gitlab::Lfs::Client do
   let(:object) { create(:lfs_object) }
+  let(:base_url) { "https://example.com" }
 
   let(:upload_action) do
     {
       "upload" => {
-        "href" => "https://example.com/some/file",
+        "href" => "#{base_url}/some/file",
         "header" => {
           "Key" => "value"
         }
@@ -19,7 +20,7 @@
   let(:verify_action) do
     {
       "verify" => {
-        "href" => "https://example.com/some/file/verify",
+        "href" => "#{base_url}/some/file/verify",
         "header" => {
           "Key" => "value"
         }
@@ -27,6 +28,8 @@
     }
   end
 
+  subject { described_class.new(base_url) }
+
   describe "#upload" do
     it "makes an HTTP post with expected parameters" do
       expect(Gitlab::HTTP)
-- 
GitLab


From 123b697cccf5f1791250768874feb0e00c619aa1 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 2 Sep 2020 17:21:18 -0600
Subject: [PATCH 15/41] Make remote_mirror an ivar

---
 app/workers/remote_mirror_lfs_object_uploader_worker.rb | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
index 468a0bb0e85f6e..8c975f5aaabaee 100644
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -7,8 +7,8 @@ class RemoteMirrorLfsObjectUploaderWorker # rubocop:disable Scalability/Idempote
   weight 2
 
   def perform(remote_mirror_id, spec, object)
-    remote_mirror = RemoteMirror.find_by_id(remote_mirror_id)
-    return unless remote_mirror&.enabled?
+    @remote_mirror = RemoteMirror.find_by_id(remote_mirror_id)
+    return unless @remote_mirror&.enabled?
 
     upload = spec.dig('actions', 'upload')
     verify = spec.dig('actions', 'verify')
@@ -23,6 +23,6 @@ def perform(remote_mirror_id, spec, object)
   private
 
   def lfs_client
-    @_lfs_client ||= Gitlab::Lfs::Client.new(remote_mirror.url)
+    @_lfs_client ||= Gitlab::Lfs::Client.new(@remote_mirror.url)
   end
 end
-- 
GitLab


From 830b1256cddabf4d17cf53577f5eddcfc9716b03 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Wed, 2 Sep 2020 17:21:48 -0600
Subject: [PATCH 16/41] Update #perform calls to pass remote_mirror.id

---
 .../remote_mirror_lfs_object_uploader_worker_spec.rb     | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
index 74c916ad856633..d1bf3f36f4b29b 100644
--- a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
+++ b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
@@ -28,6 +28,9 @@
     }
   end
 
+  let_it_be(:project) { create(:forked_project_with_submodules) }
+  let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
+
   describe "#perform" do
     context "upload to remote server is requested" do
       before do
@@ -37,7 +40,7 @@
       end
 
       it "attempts to upload the LFS object" do
-        subject.perform(spec, object)
+        subject.perform(remote_mirror.id, spec, object)
       end
 
       context "when 'verify' action is present" do
@@ -46,7 +49,7 @@
         end
 
         it "logs a warning about the lack of a verify routine" do
-          subject.perform(spec, object)
+          subject.perform(remote_mirror.id, spec, object)
         end
       end
 
@@ -58,7 +61,7 @@
         end
 
         it "does not attempt to verify the object" do
-          subject.perform(spec, object)
+          subject.perform(remote_mirror.id, spec, object)
         end
       end
     end
-- 
GitLab


From 62534ccf4d1a1f978cf8c48ba35d6cd6fd470f11 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 8 Sep 2020 14:27:56 +0100
Subject: [PATCH 17/41] Initial push to get it working between GitLab instances

---
 app/models/remote_mirror.rb                   |  5 ++++
 .../projects/update_remote_mirror_service.rb  | 17 +++++++----
 ...emote_mirror_lfs_object_uploader_worker.rb |  2 +-
 lib/gitlab/lfs/client.rb                      | 28 +++++++++++++++----
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index 8b15d481c1b75f..74648755692cbb 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -210,6 +210,11 @@ def safe_url
     super(usernames_whitelist: %w[git])
   end
 
+  def bare_url
+    original = read_attribute(:url)
+    Gitlab::UrlSanitizer.new(original).full_url if original
+  end
+
   def ensure_remote!
     return unless project
     return unless remote_name && remote_url
diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 5b63b9ce0f3e3d..0fe297196cc4b8 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -31,6 +31,9 @@ def update_mirror(remote_mirror)
       remote_mirror.update_start!
       remote_mirror.ensure_remote!
 
+      # LFS objects must be sent first, or the push has dangling pointers
+      send_lfs_objects!(remote_mirror)
+
       response = remote_mirror.update_repository
 
       if response.divergent_refs.any?
@@ -41,8 +44,6 @@ def update_mirror(remote_mirror)
         return
       end
 
-      send_lfs_objects!(remote_mirror)
-
       remote_mirror.update_finish!
     end
 
@@ -59,11 +60,15 @@ def send_lfs_objects!(remote_mirror)
 
       # TODO: LFS sync over SSH
       return unless remote_mirror.url =~ /\Ahttps?:\/\//i
+      return unless remote_mirror.password_auth?
 
-      lfs_client = Gitlab::Lfs::Client.new(remote_mirror.url) # FIXME: do we need .git on the URL?
+      lfs_client = Gitlab::Lfs::Client.new(
+        remote_mirror.bare_url, # FIXME: do we need .git on the URL?
+        credentials: remote_mirror.credentials
+      )
 
       project.lfs_objects.each_batch do |objects|
-        rsp = lfs_client.batch('upload', objects)
+        rsp = Gitlab::JSON.parse(lfs_client.batch('upload', objects))
         objects = objects.index_by(&:oid)
 
         rsp['objects'].each do |spec|
@@ -78,12 +83,12 @@ def send_lfs_objects!(remote_mirror)
 
           # The server wants us to upload the object but something is wrong
           #
-          unless object && object.size == spec['size']
+          unless object && object.size == spec['size'].to_i
             Rails.logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object") # rubocop:disable Gitlab/RailsLogger
             next
           end
 
-          RemoteMirrorLfsObjectUploaderWorker.perform_async(remote_mirror.id, spec, object)
+          RemoteMirrorLfsObjectUploaderWorker.new.perform(remote_mirror.id, spec, object)
         end
       end
     end
diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
index 8c975f5aaabaee..caf5f862cd074e 100644
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
@@ -23,6 +23,6 @@ def perform(remote_mirror_id, spec, object)
   private
 
   def lfs_client
-    @_lfs_client ||= Gitlab::Lfs::Client.new(@remote_mirror.url)
+    @_lfs_client ||= Gitlab::Lfs::Client.new(@remote_mirror.bare_url, credentials: @remote_mirror.credentials)
   end
 end
diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index 0b7a84a96501db..afaa20d22e32f4 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -1,11 +1,16 @@
 # frozen_string_literal: true
 module Gitlab
   module Lfs
+    # Gitlab::Lfs::Client implements a simple LFS client, designed to talk to
+    # LFS servers as described in these documents:
+    #   * https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
+    #   * https://github.com/git-lfs/git-lfs/blob/master/docs/api/basic-transfers.md
     class Client
       attr_reader :base_url
 
-      def initialize(base_url)
+      def initialize(base_url, credentials:)
         @base_url = base_url
+        @credentials = credentials
       end
 
       def batch(operation, objects)
@@ -18,8 +23,9 @@ def batch(operation, objects)
 
         rsp = Gitlab::HTTP.post(
           batch_url,
-          format: 'application/vnd.git-lfs+json',
-          body: body
+          basic_auth: basic_auth,
+          body: body,
+          format: 'application/vnd.git-lfs+json'
         )
 
         transfer = rsp.fetch('transfer', 'basic')
@@ -32,10 +38,12 @@ def upload(object, upload_action)
         # TODO: we need to discover credentials in some cases. These would come
         #   from the remote mirror's credentials
         #
-        Gitlab::HTTP.post(
+        Gitlab::HTTP.put(
           upload_action['href'],
-          body_stream: object.file,
-          headers: upload_action['header'],
+          body_stream: object.file.file.to_file,
+          headers: {
+            'Content-Length' => object.size.to_s
+          }.merge(upload_action['header'] || {}),
           format: 'application/octet-stream'
         )
       end
@@ -48,9 +56,17 @@ def verify(object, verify_action)
 
       private
 
+      attr_reader :credentials
+
       def batch_url
         base_url + '/info/lfs/objects/batch'
       end
+
+      def basic_auth
+        return unless credentials[:auth_method] == "password"
+
+        { username: credentials[:user], password: credentials[:password] }
+      end
     end
   end
 end
-- 
GitLab


From c0da40dad30e0a3a663111a8f4c0b6bdf49f9c9f Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Wed, 9 Sep 2020 20:07:19 +0100
Subject: [PATCH 18/41] Refactor a worker into a service

Since we need all the LFS objects to upload *before* we push to the
remote, we can't easily benefit from parallelised uploads. Rework like
this for now.
---
 app/models/project.rb                         |  6 ++
 app/services/lfs/push_service.rb              | 60 ++++++++++++
 .../projects/update_remote_mirror_service.rb  | 44 ++-------
 app/workers/all_queues.yml                    |  8 --
 ...emote_mirror_lfs_object_uploader_worker.rb | 28 ------
 config/sidekiq_queues.yml                     |  2 -
 spec/services/lfs/push_service_spec.rb        | 91 +++++++++++++++++++
 ..._mirror_lfs_object_uploader_worker_spec.rb | 69 --------------
 8 files changed, 164 insertions(+), 144 deletions(-)
 create mode 100644 app/services/lfs/push_service.rb
 delete mode 100644 app/workers/remote_mirror_lfs_object_uploader_worker.rb
 create mode 100644 spec/services/lfs/push_service_spec.rb
 delete mode 100644 spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb

diff --git a/app/models/project.rb b/app/models/project.rb
index b72bf55f879705..f3ae8a44019524 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1469,6 +1469,12 @@ def fork_source
     forked_from_project || fork_network&.root_project
   end
 
+  def lfs_objects_of_type(type)
+    LfsObject
+      .joins(:lfs_objects_projects)
+      .where(lfs_objects_projects: { project: self, repository_type: type })
+  end
+
   # TODO: Remove this method once all LfsObjectsProject records are backfilled
   # for forks.
   #
diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
new file mode 100644
index 00000000000000..9cfa8ba4da283e
--- /dev/null
+++ b/app/services/lfs/push_service.rb
@@ -0,0 +1,60 @@
+# frozen_string_literal: true
+
+module Lfs
+  # Lfs::PushService pushes the LFS objects associated with a project to a
+  # remote URL
+  class PushService < BaseService
+    include Gitlab::Utils::StrongMemoize
+
+    def execute
+      project.lfs_objects_of_type([nil, :project]).each_batch { |objects| push_objects(objects) }
+
+      success
+    rescue => err
+      error(err.message)
+    end
+
+    private
+
+    def push_objects(objects)
+      rsp = lfs_client.batch('upload', objects)
+      objects = objects.index_by(&:oid)
+
+      rsp.fetch('objects', []).each do |spec|
+        actions = spec.dig('actions')
+        upload = spec.dig('actions', 'upload')
+        verify = spec.dig('actions', 'verify')
+
+        # The server already has this object, or we don't need to upload it
+        next unless actions && upload
+
+        object = objects[spec['oid']]
+        # The server wants us to upload the object but something is wrong
+        unless object && object.size == spec['size'].to_i
+          log_error("Couldn't match object #{spec['oid']}/#{spec['size']}")
+          next
+        end
+
+        lfs_client.upload(object, upload)
+
+        if verify
+          log_error("LFS upload verification requested, but not supported for #{object.oid}")
+        end
+      end
+    end
+
+    def url
+      params.fetch(:url)
+    end
+
+    def credentials
+      params.fetch(:credentials)
+    end
+
+    def lfs_client
+      strong_memoize(:lfs_client) do
+        Gitlab::Lfs::Client.new(url, credentials: credentials)
+      end
+    end
+  end
+end
diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 0fe297196cc4b8..9bfde0928cbf85 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -41,56 +41,26 @@ def update_mirror(remote_mirror)
         message += "\n\n#{response.divergent_refs.join("\n")}"
 
         remote_mirror.mark_as_failed!(message)
-        return
+      else
+        remote_mirror.update_finish!
       end
-
-      remote_mirror.update_finish!
     end
 
-    # Minimal implementation of a git-lfs client, based on the docs here:
-    # https://github.com/git-lfs/git-lfs/blob/master/docs/api/batch.md
-    #
-    # The object is to send all the project's LFS objects to the remote
     def send_lfs_objects!(remote_mirror)
       return unless Feature.enabled?(:push_mirror_syncs_lfs, project)
       return unless project.lfs_enabled?
       return if project.lfs_objects.count == 0
 
-      # TODO: LFS sync should be configurable per remote mirror
-
       # TODO: LFS sync over SSH
       return unless remote_mirror.url =~ /\Ahttps?:\/\//i
       return unless remote_mirror.password_auth?
 
-      lfs_client = Gitlab::Lfs::Client.new(
-        remote_mirror.bare_url, # FIXME: do we need .git on the URL?
+      Lfs::PushService.new(
+        project,
+        current_user,
+        url: remote_mirror.bare_url,
         credentials: remote_mirror.credentials
-      )
-
-      project.lfs_objects.each_batch do |objects|
-        rsp = Gitlab::JSON.parse(lfs_client.batch('upload', objects))
-        objects = objects.index_by(&:oid)
-
-        rsp['objects'].each do |spec|
-          actions = spec.dig('actions')
-          upload = spec.dig('actions', 'upload')
-
-          # The server already has this object, or we don't need to upload it
-          #
-          next unless actions && upload
-
-          object = objects[spec['oid']]
-
-          # The server wants us to upload the object but something is wrong
-          #
-          unless object && object.size == spec['size'].to_i
-            Rails.logger.warn("Couldn't match #{spec['oid']} at size #{spec['size']} with an LFS object") # rubocop:disable Gitlab/RailsLogger
-            next
-          end
-
-          RemoteMirrorLfsObjectUploaderWorker.new.perform(remote_mirror.id, spec, object)
-        end
-      end
+      ).execute
     end
 
     def retry_or_fail(mirror, message, tries)
diff --git a/app/workers/all_queues.yml b/app/workers/all_queues.yml
index 1a5a77df162152..451decce9fb470 100644
--- a/app/workers/all_queues.yml
+++ b/app/workers/all_queues.yml
@@ -1724,14 +1724,6 @@
   :weight: 2
   :idempotent:
   :tags: []
-- :name: remote_mirror_lfs_object_uploader
-  :feature_category: :source_code_management
-  :has_external_dependencies:
-  :urgency: :low
-  :resource_boundary: :unknown
-  :weight: 2
-  :idempotent:
-  :tags: []
 - :name: remote_mirror_notification
   :feature_category: :source_code_management
   :has_external_dependencies:
diff --git a/app/workers/remote_mirror_lfs_object_uploader_worker.rb b/app/workers/remote_mirror_lfs_object_uploader_worker.rb
deleted file mode 100644
index caf5f862cd074e..00000000000000
--- a/app/workers/remote_mirror_lfs_object_uploader_worker.rb
+++ /dev/null
@@ -1,28 +0,0 @@
-# frozen_string_literal: true
-
-class RemoteMirrorLfsObjectUploaderWorker # rubocop:disable Scalability/IdempotentWorker
-  include ApplicationWorker
-
-  feature_category :source_code_management
-  weight 2
-
-  def perform(remote_mirror_id, spec, object)
-    @remote_mirror = RemoteMirror.find_by_id(remote_mirror_id)
-    return unless @remote_mirror&.enabled?
-
-    upload = spec.dig('actions', 'upload')
-    verify = spec.dig('actions', 'verify')
-
-    lfs_client.upload(object, upload)
-
-    if verify
-      lfs_client.verify(object, verify)
-    end
-  end
-
-  private
-
-  def lfs_client
-    @_lfs_client ||= Gitlab::Lfs::Client.new(@remote_mirror.bare_url, credentials: @remote_mirror.credentials)
-  end
-end
diff --git a/config/sidekiq_queues.yml b/config/sidekiq_queues.yml
index 99413d0c8c9384..cb85b5b6d1e190 100644
--- a/config/sidekiq_queues.yml
+++ b/config/sidekiq_queues.yml
@@ -238,8 +238,6 @@
   - 2
 - - refresh_license_compliance_checks
   - 2
-- - remote_mirror_lfs_object_uploader
-  - 2
 - - remote_mirror_notification
   - 2
 - - repository_check
diff --git a/spec/services/lfs/push_service_spec.rb b/spec/services/lfs/push_service_spec.rb
new file mode 100644
index 00000000000000..b9338c7e14a6ee
--- /dev/null
+++ b/spec/services/lfs/push_service_spec.rb
@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Lfs::PushService do
+  let(:logger) { service.send(:logger) }
+  let(:lfs_client) { service.send(:lfs_client) }
+
+  let_it_be(:project) { create(:forked_project_with_submodules) }
+  let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
+  let_it_be(:lfs_object) { create_linked_lfs_object(project, :project) }
+
+  let(:params) { { url: remote_mirror.bare_url, credentials: remote_mirror.credentials } }
+
+  subject(:service) { described_class.new(project, nil, params) }
+
+  describe "#execute" do
+    context 'upload is requested' do
+      it 'uploads the object' do
+        stub_lfs_batch(lfs_object)
+        expect(lfs_client).to receive(:upload).with(lfs_object, upload_action_spec(lfs_object))
+
+        expect(service.execute).to eq(status: :success)
+      end
+    end
+
+    context 'upload is not requested' do
+      it 'does not upload the object' do
+        stub_lfs_batch(lfs_object, upload: false)
+
+        expect(lfs_client).not_to receive(:upload)
+
+        expect(service.execute).to eq(status: :success)
+      end
+    end
+
+    context 'non-project-repository LFS objects' do
+      let_it_be(:nil_lfs_object) { create_linked_lfs_object(project, nil) }
+      let_it_be(:design_lfs_object) { create_linked_lfs_object(project, :design) }
+      let_it_be(:wiki_lfs_object)   { create_linked_lfs_object(project, :wiki) }
+
+      it 'only tries to upload the project-repository LFS object' do
+        stub_lfs_batch(nil_lfs_object, lfs_object, upload: false)
+
+        expect(service.execute).to eq(status: :success)
+      end
+    end
+
+    context 'submitting a batch fails' do
+      it 'returns a failure' do
+        expect(lfs_client).to receive(:batch) { raise 'failed' }
+
+        expect(service.execute).to eq(status: :error, message: 'failed')
+      end
+    end
+
+    context 'submitting an upload fails' do
+      it 'returns a failure' do
+        stub_lfs_batch(lfs_object)
+        expect(lfs_client).to receive(:upload) { raise 'failed' }
+
+        expect(service.execute).to eq(status: :error, message: 'failed')
+      end
+    end
+  end
+
+  def create_linked_lfs_object(project, type)
+    create(:lfs_objects_project, project: project, repository_type: type).lfs_object
+  end
+
+  def stub_lfs_batch(*objects, upload: true)
+    expect(lfs_client)
+      .to receive(:batch).with('upload', containing_exactly(*objects))
+      .and_return('transfer' => 'basic', 'objects' => objects.map { |o| object_spec(o, upload: upload) })
+  end
+
+  def batch_spec(*objects, upload: true)
+    { 'transfer' => 'basic', 'objects' => objects.map {|o| object_spec(o, upload: upload) } }
+  end
+
+  def object_spec(object, upload: true)
+    out = { 'oid' => object.oid, 'size' => object.size }
+    out['actions'] = { 'upload' => upload_action_spec(object) } if upload
+
+    out
+  end
+
+  def upload_action_spec(object)
+    { 'href' => "https://example.com/#{object.oid}/#{object.size}", 'header' => { 'Key' => 'value' } }
+  end
+end
diff --git a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb b/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
deleted file mode 100644
index d1bf3f36f4b29b..00000000000000
--- a/spec/workers/remote_mirror_lfs_object_uploader_worker_spec.rb
+++ /dev/null
@@ -1,69 +0,0 @@
-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe RemoteMirrorLfsObjectUploaderWorker do
-  let(:logger) { subject.send(:logger) }
-
-  let(:object) { create(:lfs_object) }
-
-  let(:spec) do
-    {
-      "oid" => object.oid,
-      "size" => object.size,
-      "actions" => {
-        "upload" => {
-          "href" => "https://example.com/some/file",
-          "header" => {
-            "Key" => "value"
-          }
-        },
-        "verify" => {
-          "href" => "https://example.com/some/file/verify",
-          "header" => {
-            "Key" => "value"
-          }
-        }
-      }
-    }
-  end
-
-  let_it_be(:project) { create(:forked_project_with_submodules) }
-  let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
-
-  describe "#perform" do
-    context "upload to remote server is requested" do
-      before do
-        expect_next_instance_of(Gitlab::Lfs::Client) do |instance|
-          expect(instance).to receive(:upload)
-        end
-      end
-
-      it "attempts to upload the LFS object" do
-        subject.perform(remote_mirror.id, spec, object)
-      end
-
-      context "when 'verify' action is present" do
-        before do
-          expect_any_instance_of(Gitlab::Lfs::Client).to receive(:verify).and_call_original
-        end
-
-        it "logs a warning about the lack of a verify routine" do
-          subject.perform(remote_mirror.id, spec, object)
-        end
-      end
-
-      context "when 'verify' action is missing" do
-        before do
-          spec['actions'].delete('verify')
-
-          expect_any_instance_of(Gitlab::Lfs::Client).not_to receive(:verify).and_call_original
-        end
-
-        it "does not attempt to verify the object" do
-          subject.perform(remote_mirror.id, spec, object)
-        end
-      end
-    end
-  end
-end
-- 
GitLab


From 66b26c0a0877f69cd9c462c3b4f9f7c66a33bbd3 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Fri, 11 Sep 2020 20:02:22 +0100
Subject: [PATCH 19/41] Refine LFS client behaviour

---
 app/services/lfs/push_service.rb   |   3 +-
 lib/gitlab/lfs/client.rb           |  40 ++++-----
 spec/lib/gitlab/lfs/client_spec.rb | 133 ++++++++++++++++++++++-------
 3 files changed, 124 insertions(+), 52 deletions(-)

diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index 9cfa8ba4da283e..aa45a7918581e4 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -21,6 +21,7 @@ def push_objects(objects)
       objects = objects.index_by(&:oid)
 
       rsp.fetch('objects', []).each do |spec|
+        authenticated = spec.dig('authenticated')
         actions = spec.dig('actions')
         upload = spec.dig('actions', 'upload')
         verify = spec.dig('actions', 'verify')
@@ -35,7 +36,7 @@ def push_objects(objects)
           next
         end
 
-        lfs_client.upload(object, upload)
+        lfs_client.upload(object, upload, authenticated: authenticated)
 
         if verify
           log_error("LFS upload verification requested, but not supported for #{object.oid}")
diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index afaa20d22e32f4..fff6f3e8f7200d 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -24,34 +24,36 @@ def batch(operation, objects)
         rsp = Gitlab::HTTP.post(
           batch_url,
           basic_auth: basic_auth,
-          body: body,
+          body: body.to_json,
           format: 'application/vnd.git-lfs+json'
         )
 
-        transfer = rsp.fetch('transfer', 'basic')
+        raise "Failed to submit batch" unless rsp.success?
+
+        body = Gitlab::Json.parse(rsp.parsed_response)
+
+        transfer = body.fetch('transfer', 'basic')
         raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
 
-        rsp
+        body
       end
 
-      def upload(object, upload_action)
-        # TODO: we need to discover credentials in some cases. These would come
-        #   from the remote mirror's credentials
-        #
-        Gitlab::HTTP.put(
-          upload_action['href'],
-          body_stream: object.file.file.to_file,
-          headers: {
-            'Content-Length' => object.size.to_s
-          }.merge(upload_action['header'] || {}),
+      def upload(object, upload_action, authenticated:)
+        file = object.file.open
+
+        params = {
+          body_stream: file,
+          headers: { 'Content-Length' => object.size.to_s }.merge(upload_action['header'] || {}),
           format: 'application/octet-stream'
-        )
-      end
+        }
+
+        params[:basic_auth] = basic_auth unless authenticated
+
+        rsp = Gitlab::HTTP.put(upload_action['href'], params)
 
-      # TODO: verify the file
-      #
-      def verify(object, verify_action)
-        # noop
+        raise "Failed to upload object" unless rsp.success?
+      ensure
+        file&.close
       end
 
       private
diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index f173dceeaba85e..f0852db5b3423a 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -3,51 +3,120 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Lfs::Client do
-  let(:object) { create(:lfs_object) }
   let(:base_url) { "https://example.com" }
+  let(:username) { 'user' }
+  let(:password) { 'password' }
+  let(:credentials) { { user: username, password: password, auth_method: 'password' } }
 
-  let(:upload_action) do
-    {
-      "upload" => {
-        "href" => "#{base_url}/some/file",
-        "header" => {
-          "Key" => "value"
-        }
-      }
-    }
+  let(:basic_auth_headers) do
+    { 'Authorization' => "Basic #{Base64.strict_encode64("#{username}:#{password}")}" }
   end
 
-  let(:verify_action) do
+  let(:upload_action) do
     {
-      "verify" => {
-        "href" => "#{base_url}/some/file/verify",
-        "header" => {
-          "Key" => "value"
-        }
+      "href" => "#{base_url}/some/file",
+      "header" => {
+        "Key" => "value"
       }
     }
   end
 
-  subject { described_class.new(base_url) }
+  subject(:lfs_client) { described_class.new(base_url, credentials: credentials) }
 
-  describe "#upload" do
-    it "makes an HTTP post with expected parameters" do
-      expect(Gitlab::HTTP)
-        .to receive(:post)
-        .with(
-          upload_action['href'],
-          body_stream: object.file,
-          headers:     upload_action['header'],
-          format:      'application/octet-stream'
-        )
-
-      subject.upload(object, upload_action)
+  describe '#batch' do
+    let_it_be(:objects) { create_list(:lfs_object, 3) }
+
+    context 'server returns 200 OK' do
+      it 'makes a successful batch request' do
+        stub_batch(
+          objects: objects,
+          headers: basic_auth_headers
+        ).to_return(status: 200, body: { 'objects' => 'anything', 'transfer' => 'basic' }.to_json)
+
+        result = lfs_client.batch('upload', objects)
+
+        expect(result).to eq('objects' => 'anything', 'transfer' => 'basic')
+      end
+    end
+
+    context 'server returns 400 error' do
+      it 'raises an error' do
+        stub_batch(objects: objects, headers: basic_auth_headers).to_return(status: 400)
+
+        expect { lfs_client.batch('upload', objects) }.to raise_error(/Failed/)
+      end
+    end
+
+    context 'server returns 500 error' do
+      it 'raises an error' do
+        stub_batch(objects: objects, headers: basic_auth_headers).to_return(status: 400)
+
+        expect { lfs_client.batch('upload', objects) }.to raise_error(/Failed/)
+      end
+    end
+
+    context 'server returns an exotic transfer method' do
+      it 'raises an error' do
+        stub_batch(
+          objects: objects,
+          headers: basic_auth_headers
+        ).to_return(status: 200, body: { 'transfer' => 'carrier-pigeon' }.to_json)
+
+        expect { lfs_client.batch('upload', objects) }.to raise_error(/Unsupported transfer/)
+      end
+    end
+
+    def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
+      objects = objects.map { |o| { oid: o.oid, size: o.size } }
+      body = { operation: operation, 'transfers': [transfer], objects: objects }.to_json
+
+      stub_request(:post, base_url + '/info/lfs/objects/batch').with(body: body, headers: headers)
     end
   end
 
-  describe "#verify" do
-    it "does nothing" do
-      expect(subject.verify(object, verify_action)).to be_nil
+  describe "#upload" do
+    let_it_be(:object) { create(:lfs_object) }
+
+    context 'server returns 200 OK to an authenticated request' do
+      it "makes an HTTP PUT with expected parameters" do
+        stub_upload(object: object, headers: upload_action['header']).to_return(status: 200)
+
+        lfs_client.upload(object, upload_action, authenticated: true)
+      end
+    end
+
+    context 'server returns 200 OK to an unauthenticated request' do
+      it "makes an HTTP PUT with expected parameters" do
+        stub_upload(
+          object: object,
+          headers: basic_auth_headers.merge(upload_action['header'])
+        ).to_return(status: 200)
+
+        lfs_client.upload(object, upload_action, authenticated: false)
+      end
+    end
+
+    context 'server returns 400 error' do
+      it 'raises an error' do
+        stub_upload(object: object, headers: upload_action['header']).to_return(status: 400)
+
+        expect { lfs_client.upload(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
+      end
+    end
+
+    context 'server returns 500 error' do
+      it 'raises an error' do
+        stub_upload(object: object, headers: upload_action['header']).to_return(status: 500)
+
+        expect { lfs_client.upload(object, upload_action, authenticated: true) }.to raise_error(/Failed/)
+      end
+    end
+
+    def stub_upload(object:, headers:)
+      stub_request(:put, upload_action['href']).with(
+        body: object.file.read,
+        headers: headers.merge('Content-Length' => object.size.to_s)
+      )
     end
   end
 end
-- 
GitLab


From dabe64a33a9e2bbca893949c25c00ae07577be60 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 15:42:15 +0100
Subject: [PATCH 20/41] Test for Project#lfs_objects_of_type

---
 spec/models/project_spec.rb | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 77b8b22e3f3567..0068e536482a68 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2901,6 +2901,20 @@
     end
   end
 
+  describe '#lfs_objects_of_type' do
+    let(:project) { create(:project) }
+
+    it 'returns LFS objects of the specified type only' do
+      none, design, wiki = *[nil, :design, :wiki].map do |type|
+        create(:lfs_objects_project, project: project, repository_type: type).lfs_object
+      end
+
+      expect(project.lfs_objects_of_type(nil)).to contain_exactly(none)
+      expect(project.lfs_objects_of_type([nil, :wiki])).to contain_exactly(none, wiki)
+      expect(project.lfs_objects_of_type(:design)).to contain_exactly(design)
+    end
+  end
+
   context 'forks' do
     include ProjectForksHelper
 
-- 
GitLab


From be44005050f4b01398a68b4fc7ace559d703bad0 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 16:05:09 +0100
Subject: [PATCH 21/41] Add more tests

---
 spec/services/lfs/push_service_spec.rb        | 14 +--
 .../update_remote_mirror_service_spec.rb      | 93 +++++++++----------
 2 files changed, 52 insertions(+), 55 deletions(-)

diff --git a/spec/services/lfs/push_service_spec.rb b/spec/services/lfs/push_service_spec.rb
index b9338c7e14a6ee..df6a8e5a5ce5c0 100644
--- a/spec/services/lfs/push_service_spec.rb
+++ b/spec/services/lfs/push_service_spec.rb
@@ -18,7 +18,10 @@
     context 'upload is requested' do
       it 'uploads the object' do
         stub_lfs_batch(lfs_object)
-        expect(lfs_client).to receive(:upload).with(lfs_object, upload_action_spec(lfs_object))
+
+        expect(lfs_client)
+          .to receive(:upload)
+          .with(lfs_object, upload_action_spec(lfs_object), authenticated: true)
 
         expect(service.execute).to eq(status: :success)
       end
@@ -36,8 +39,8 @@
 
     context 'non-project-repository LFS objects' do
       let_it_be(:nil_lfs_object) { create_linked_lfs_object(project, nil) }
+      let_it_be(:wiki_lfs_object) { create_linked_lfs_object(project, :wiki) }
       let_it_be(:design_lfs_object) { create_linked_lfs_object(project, :design) }
-      let_it_be(:wiki_lfs_object)   { create_linked_lfs_object(project, :wiki) }
 
       it 'only tries to upload the project-repository LFS object' do
         stub_lfs_batch(nil_lfs_object, lfs_object, upload: false)
@@ -79,10 +82,9 @@ def batch_spec(*objects, upload: true)
   end
 
   def object_spec(object, upload: true)
-    out = { 'oid' => object.oid, 'size' => object.size }
-    out['actions'] = { 'upload' => upload_action_spec(object) } if upload
-
-    out
+    { 'oid' => object.oid, 'size' => object.size, 'authenticated' => true }.tap do |spec|
+      spec['actions'] = { 'upload' => upload_action_spec(object) } if upload
+    end
   end
 
   def upload_action_spec(object)
diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb
index df4ec75d304f05..9684c4b046b8a5 100644
--- a/spec/services/projects/update_remote_mirror_service_spec.rb
+++ b/spec/services/projects/update_remote_mirror_service_spec.rb
@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Projects::UpdateRemoteMirrorService do
-  let(:project) { create(:project, :repository) }
-  let(:remote_project) { create(:forked_project_with_submodules) }
-  let(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
+  let_it_be(:project) { create(:project, :repository, lfs_enabled: true) }
+  let_it_be(:remote_project) { create(:forked_project_with_submodules) }
+  let_it_be(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
+
   let(:remote_name) { remote_mirror.remote_name }
 
   subject(:service) { described_class.new(project, project.creator) }
@@ -129,74 +130,68 @@
     end
 
     context "sending lfs objects" do
-      let!(:lfs_object) { create(:lfs_objects_project, project: project).lfs_object }
-      let(:sample_lfs_object) { project.lfs_objects.first }
-      let(:spec) do
-        {
-          "objects" => [{
-            "oid" => sample_lfs_object.oid,
-            "size" => sample_lfs_object.size,
-            "actions" => {
-              "upload" => {
-                "href" => "https://example.com/some/file",
-                "header" => {
-                  "Key" => "value"
-                }
-              },
-              "verify" => {
-                "href" => "https://example.com/some/file/verify",
-                "header" => {
-                  "Key" => "value"
-                }
-              }
-            }
-          }]
-        }
+      let_it_be(:lfs_pointer) { create(:lfs_objects_project, project: project) }
+
+      before do
+        stub_lfs_setting(enabled: true)
       end
 
-      shared_examples "returns early without attempting upload" do
-        it "returns early without attempting upload" do
-          expect(Gitlab::Lfs::Client).not_to receive(:upload)
+      context 'feature flag enabled' do
+        before do
+          stub_feature_flags(push_mirror_syncs_lfs: true)
+        end
+
+        it 'pushes LFS objects to a HTTP repository' do
+          expect_next_instance_of(Lfs::PushService) do |service|
+            expect(service).to receive(:execute)
+          end
 
           execute!
         end
-      end
 
-      context "object and and spec size do not match" do
-        before do
-          project.update_attribute(:lfs_enabled, true)
-          allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
+        it 'does nothing to an SSH repository' do
+          remote_mirror.update!(url: 'ssh://example.com')
 
-          spec['objects'].first['size'] = sample_lfs_object.size + 1
+          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
 
-          expect(Gitlab::HTTP).to receive(:post).and_return(spec)
+          execute!
         end
 
-        it_behaves_like "returns early without attempting upload"
+        it 'does nothing if LFS is disabled' do
+          expect(project).to receive(:lfs_enabled?) { false }
 
-        it "logs a warning about the size mismatch" do
-          expect(Rails.logger)
-            .to receive(:warn)
-            .with("Couldn't match #{spec['objects'].first['oid']} at size #{spec['objects'].first['size']} with an LFS object")
+          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
 
           execute!
         end
-      end
 
-      context "missing 'actions'" do
-        before do
-          spec['objects'].first.delete('actions')
+        it 'does nothing with no LFS objects' do
+          lfs_pointer.destroy!
+
+          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
+
+          execute!
         end
 
-        it_behaves_like "returns early without attempting upload"
+        it 'does nothing if non-password auth is specified' do
+          remote_mirror.update!(auth_method: 'ssh_public_key')
+
+          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
+
+          execute!
+        end
       end
 
-      context "missing 'upload' action" do
+      context 'feature flag disabled' do
         before do
-          spec['objects'].first['actions'].delete('upload')
+          stub_feature_flags(push_mirror_syncs_lfs: false)
         end
 
-        it_behaves_like "returns early without attempting upload"
+        it 'does nothing' do
+          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
+
+          execute!
+        end
       end
     end
   end
-- 
GitLab


From fd847c421a8a353eb04093ca77c9f5199f0f6f07 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 16:17:41 +0100
Subject: [PATCH 22/41] Enable LFS in the push mirror QA spec

---
 .../3_create/repository/push_mirroring_over_http_spec.rb       | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
index 00d4acbd1e1053..2e18058fada21c 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
@@ -17,7 +17,10 @@ module QA
           push.file_name = 'README.md'
           push.file_content = '# This is a test project'
           push.commit_message = 'Add README.md'
+          push.use_lfs = true
         end
+        
+        
         source_project_push.project.visit!
 
         Page::Project::Menu.perform(&:go_to_repository_settings)
-- 
GitLab


From 802a2dd09708e91618ee92df67d8d31841514da4 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 16:28:53 +0100
Subject: [PATCH 23/41] Enable the push_mirror_syncs LFS feature flag in QA

---
 .../3_create/repository/push_mirroring_over_http_spec.rb         | 1 +
 1 file changed, 1 insertion(+)

diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
index 2e18058fada21c..30cc9a6ee471c8 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
@@ -4,6 +4,7 @@ module QA
   RSpec.describe 'Create' do
     describe 'Push mirror a repository over HTTP' do
       it 'configures and syncs a (push) mirrored repository', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/414' do
+        Runtime::Feature.enable_and_verify('push_mirror_syncs_lfs')
         Runtime::Browser.visit(:gitlab, Page::Main::Login)
         Page::Main::Login.perform(&:sign_in_using_credentials)
 
-- 
GitLab


From 1c78b21bb05a890bcd21e730e7ab9402278c74a4 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 16:34:18 +0100
Subject: [PATCH 24/41] Remove unnecessary whitespace

---
 .../3_create/repository/push_mirroring_over_http_spec.rb        | 2 --
 1 file changed, 2 deletions(-)

diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
index 30cc9a6ee471c8..6eb161ecafb50b 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
@@ -20,8 +20,6 @@ module QA
           push.commit_message = 'Add README.md'
           push.use_lfs = true
         end
-        
-        
         source_project_push.project.visit!
 
         Page::Project::Menu.perform(&:go_to_repository_settings)
-- 
GitLab


From ad2d691f8140ab18b527ee0bc4148774d9bf6cdd Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 16:39:50 +0100
Subject: [PATCH 25/41] Remove an unneeded test

---
 spec/features/projects/remote_mirror_spec.rb | 38 --------------------
 1 file changed, 38 deletions(-)

diff --git a/spec/features/projects/remote_mirror_spec.rb b/spec/features/projects/remote_mirror_spec.rb
index bf34dcf16ff327..26d27c914cc79c 100644
--- a/spec/features/projects/remote_mirror_spec.rb
+++ b/spec/features/projects/remote_mirror_spec.rb
@@ -33,44 +33,6 @@
       end
     end
 
-    context 'pushing to a remote' do
-      let(:remote_project) { create(:forked_project_with_submodules) }
-      let!(:lfs_object) { create(:lfs_objects_project, project: project).lfs_object }
-      let(:remote_mirror) { create(:remote_mirror, project: project, enabled: true) }
-
-      before do
-        remote_project.add_maintainer(user)
-
-        project.update_attribute(:lfs_enabled, true)
-
-        allow(remote_mirror)
-          .to receive(:update_repository)
-          .and_return(double(divergent_refs: []))
-        allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
-
-        # TODO: This needs to return a response as if it were our remote LFS
-        #   server.
-        #
-        stub_request(:post, "http://test.com/info/lfs/objects/batch")
-          .with(
-            body: "operation=upload&transfers[]=basic&objects[][oid]=#{lfs_object.oid}&objects[][size]=#{lfs_object.size}",
-            headers: {
-              'Accept'          => '*/*',
-              'Accept-Encoding' => 'gzip;q=1.0,deflate;q=0.6,identity;q=0.3',
-              'Authorization'   => 'Basic Zm9vOmJhcg==',
-              'User-Agent'      => 'Ruby'
-            }
-          ).to_return(status: 200, body: "", headers: {})
-      end
-
-      xit 'transfers code and LFS objects' do
-        Projects::UpdateRemoteMirrorService.new(project, user).execute(remote_mirror, 0)
-
-        expect(remote_project.lfs_objects.reload.count).to eq(1)
-        expect(remote_project.commit).to eq(project.commit)
-      end
-    end
-
     def expect_mirror_to_have_error_and_timeago(timeago)
       row = first('.js-mirrors-table-body tr')
       expect(row).to have_content('Error')
-- 
GitLab


From 8cd53c47bfc7a2628a8b20097b249fb2ef91fd53 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Mon, 14 Sep 2020 18:21:02 +0100
Subject: [PATCH 26/41] Fix parsing batch responses as JSON

---
 lib/gitlab/lfs/client.rb           | 12 +++++++-----
 spec/lib/gitlab/lfs/client_spec.rb | 12 ++++++++++--
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index fff6f3e8f7200d..b6f9ae966c12e8 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -25,14 +25,14 @@ def batch(operation, objects)
           batch_url,
           basic_auth: basic_auth,
           body: body.to_json,
-          format: 'application/vnd.git-lfs+json'
+          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
         )
 
         raise "Failed to submit batch" unless rsp.success?
 
-        body = Gitlab::Json.parse(rsp.parsed_response)
-
+        body = Gitlab::Json.parse(rsp.body)
         transfer = body.fetch('transfer', 'basic')
+
         raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
 
         body
@@ -43,8 +43,10 @@ def upload(object, upload_action, authenticated:)
 
         params = {
           body_stream: file,
-          headers: { 'Content-Length' => object.size.to_s }.merge(upload_action['header'] || {}),
-          format: 'application/octet-stream'
+          headers: {
+            'Content-Length' => object.size.to_s,
+            'Content-Type' => 'application/octet-stream'
+          }.merge(upload_action['header'] || {})
         }
 
         params[:basic_auth] = basic_auth unless authenticated
diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index f0852db5b3423a..1fcef80b0b0ca6 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -31,7 +31,11 @@
         stub_batch(
           objects: objects,
           headers: basic_auth_headers
-        ).to_return(status: 200, body: { 'objects' => 'anything', 'transfer' => 'basic' }.to_json)
+        ).to_return(
+          status: 200,
+          body: { 'objects' => 'anything', 'transfer' => 'basic' }.to_json,
+          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
+        )
 
         result = lfs_client.batch('upload', objects)
 
@@ -60,7 +64,11 @@
         stub_batch(
           objects: objects,
           headers: basic_auth_headers
-        ).to_return(status: 200, body: { 'transfer' => 'carrier-pigeon' }.to_json)
+        ).to_return(
+          status: 200,
+          body: { 'transfer' => 'carrier-pigeon' }.to_json,
+          headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
+        )
 
         expect { lfs_client.batch('upload', objects) }.to raise_error(/Unsupported transfer/)
       end
-- 
GitLab


From 1373ad9bd81bf0a54126ea43611b45746d329ee9 Mon Sep 17 00:00:00 2001
From: Kerri Miller <kerrizor@kerrizor.com>
Date: Mon, 14 Sep 2020 12:41:56 -0700
Subject: [PATCH 27/41] Add custom exception classes to client

---
 lib/gitlab/lfs/client.rb | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index b6f9ae966c12e8..384fbbd1676ff6 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -28,12 +28,12 @@ def batch(operation, objects)
           headers: { 'Content-Type' => 'application/vnd.git-lfs+json' }
         )
 
-        raise "Failed to submit batch" unless rsp.success?
+        raise BatchSubmitError unless rsp.success?
 
         body = Gitlab::Json.parse(rsp.body)
         transfer = body.fetch('transfer', 'basic')
 
-        raise "Unsupported transfer: #{transfer.inspect}" unless transfer == 'basic'
+        raise UnsupportedTransferError.new(transfer.inspect) unless transfer == 'basic'
 
         body
       end
@@ -53,7 +53,7 @@ def upload(object, upload_action, authenticated:)
 
         rsp = Gitlab::HTTP.put(upload_action['href'], params)
 
-        raise "Failed to upload object" unless rsp.success?
+        raise ObjectUploadError unless rsp.success?
       ensure
         file&.close
       end
@@ -71,6 +71,29 @@ def basic_auth
 
         { username: credentials[:user], password: credentials[:password] }
       end
+
+      class BatchSubmitError < StandardError
+        def message
+          "Failed to submit batch"
+        end
+      end
+
+      class UnsupportedTransferError < StandardError
+        def initialize(transfer = nil)
+          super
+          @transfer = transfer
+        end
+
+        def message
+          "Unsupported transfer: #{@transfer}"
+        end
+      end
+
+      class ObjectUploadError < StandardError
+        def message
+          "Failed to upload object"
+        end
+      end
     end
   end
 end
-- 
GitLab


From fb595bf390cf0b7ce0ddb2e54bd639ab09a1ba84 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 13:42:49 +0100
Subject: [PATCH 28/41] Rename the lfs_objects_of_type method

---
 app/models/project.rb            | 2 +-
 app/services/lfs/push_service.rb | 4 +++-
 spec/models/project_spec.rb      | 8 ++++----
 3 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index 43dbe57c33db2f..b559d4e3d6ba96 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1469,7 +1469,7 @@ def fork_source
     forked_from_project || fork_network&.root_project
   end
 
-  def lfs_objects_of_type(type)
+  def lfs_objects_for_repository_type(type)
     LfsObject
       .joins(:lfs_objects_projects)
       .where(lfs_objects_projects: { project: self, repository_type: type })
diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index aa45a7918581e4..941c9ec747df73 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -7,7 +7,9 @@ class PushService < BaseService
     include Gitlab::Utils::StrongMemoize
 
     def execute
-      project.lfs_objects_of_type([nil, :project]).each_batch { |objects| push_objects(objects) }
+      project
+        .lfs_objects_for_repository_type([nil, :project])
+        .each_batch { |objects| push_objects(objects) }
 
       success
     rescue => err
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9cc497906d1235..f4d33a3af49927 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2901,7 +2901,7 @@
     end
   end
 
-  describe '#lfs_objects_of_type' do
+  describe '#lfs_objects_for_repository_type' do
     let(:project) { create(:project) }
 
     it 'returns LFS objects of the specified type only' do
@@ -2909,9 +2909,9 @@
         create(:lfs_objects_project, project: project, repository_type: type).lfs_object
       end
 
-      expect(project.lfs_objects_of_type(nil)).to contain_exactly(none)
-      expect(project.lfs_objects_of_type([nil, :wiki])).to contain_exactly(none, wiki)
-      expect(project.lfs_objects_of_type(:design)).to contain_exactly(design)
+      expect(project.lfs_objects_for_repository_type(nil)).to contain_exactly(none)
+      expect(project.lfs_objects_for_repository_type([nil, :wiki])).to contain_exactly(none, wiki)
+      expect(project.lfs_objects_for_repository_type(:design)).to contain_exactly(design)
     end
   end
 
-- 
GitLab


From 4e49b18b0e9b513ddf26a691db0910d1250f9135 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 13:55:19 +0100
Subject: [PATCH 29/41] Rework Lfs::PushService code layout

---
 app/services/lfs/push_service.rb | 40 ++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 17 deletions(-)

diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index 941c9ec747df73..80dcb89eb15403 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -23,27 +23,33 @@ def push_objects(objects)
       objects = objects.index_by(&:oid)
 
       rsp.fetch('objects', []).each do |spec|
-        authenticated = spec.dig('authenticated')
-        actions = spec.dig('actions')
-        upload = spec.dig('actions', 'upload')
-        verify = spec.dig('actions', 'verify')
-
-        # The server already has this object, or we don't need to upload it
-        next unless actions && upload
-
+        actions = spec['actions']
         object = objects[spec['oid']]
-        # The server wants us to upload the object but something is wrong
-        unless object && object.size == spec['size'].to_i
-          log_error("Couldn't match object #{spec['oid']}/#{spec['size']}")
-          next
-        end
 
-        lfs_client.upload(object, upload, authenticated: authenticated)
+        upload_object!(object, spec) if actions&.key?('upload')
+        verify_object!(object, spec) if actions&.key?('verify')
+      end
+    end
+
+    def upload_object!(object, spec)
+      size = spec['size'].to_i
+      authenticated = spec['authenticated']
+      upload = spec.dig('actions', 'upload')
 
-        if verify
-          log_error("LFS upload verification requested, but not supported for #{object.oid}")
-        end
+      # The server wants us to upload the object but something is wrong
+      unless object && object.size == spec['size'].to_i
+        log_error("Couldn't match object #{spec['oid']}/#{spec['size']}")
+        return
       end
+
+      lfs_client.upload(object, upload, authenticated: authenticated)
+    end
+
+    def verify_object!(object, spec)
+      verify = spec.dig('actions', 'verify')
+      return unless verify
+
+      log_error("LFS upload verification requested, but not supported for #{object.oid}")
     end
 
     def url
-- 
GitLab


From ea2ab0d736156b5d7c399e66ad911547b0a66d0d Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 13:56:53 +0100
Subject: [PATCH 30/41] Add a comment explaining why we parse JSON

---
 lib/gitlab/lfs/client.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/gitlab/lfs/client.rb b/lib/gitlab/lfs/client.rb
index 384fbbd1676ff6..e4d600694c2269 100644
--- a/lib/gitlab/lfs/client.rb
+++ b/lib/gitlab/lfs/client.rb
@@ -30,6 +30,8 @@ def batch(operation, objects)
 
         raise BatchSubmitError unless rsp.success?
 
+        # HTTParty provides rsp.parsed_response, but it only kicks in for the
+        # application/json content type in the response, which we can't rely on
         body = Gitlab::Json.parse(rsp.body)
         transfer = body.fetch('transfer', 'basic')
 
-- 
GitLab


From 351058dc2991c0885b275feb9130332267bdce78 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 13:58:39 +0100
Subject: [PATCH 31/41] Duplicate the existing push mirroring QA test for LFS

Modifying it might leave a minor test gap
---
 .../push_mirroring_lfs_over_http_spec.rb      | 45 +++++++++++++++++++
 .../push_mirroring_over_http_spec.rb          |  2 -
 2 files changed, 45 insertions(+), 2 deletions(-)
 create mode 100644 qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_lfs_over_http_spec.rb

diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_lfs_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_lfs_over_http_spec.rb
new file mode 100644
index 00000000000000..f96b424d233b4c
--- /dev/null
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_lfs_over_http_spec.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+module QA
+  RSpec.describe 'Create' do
+    describe 'Push mirror a repository over HTTP' do
+      it 'configures and syncs LFS objects for a (push) mirrored repository', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/414' do
+        Runtime::Feature.enable_and_verify('push_mirror_syncs_lfs')
+        Runtime::Browser.visit(:gitlab, Page::Main::Login)
+        Page::Main::Login.perform(&:sign_in_using_credentials)
+
+        target_project = Resource::Project.fabricate_via_api! do |project|
+          project.name = 'push-mirror-target-project'
+        end
+        target_project_uri = target_project.repository_http_location.uri
+        target_project_uri.user = Runtime::User.username
+
+        source_project_push = Resource::Repository::ProjectPush.fabricate! do |push|
+          push.file_name = 'README.md'
+          push.file_content = '# This is a test project'
+          push.commit_message = 'Add README.md'
+          push.use_lfs = true
+        end
+        source_project_push.project.visit!
+
+        Page::Project::Menu.perform(&:go_to_repository_settings)
+        Page::Project::Settings::Repository.perform do |settings|
+          settings.expand_mirroring_repositories do |mirror_settings|
+            # Configure the source project to push to the target project
+            mirror_settings.repository_url = target_project_uri
+            mirror_settings.mirror_direction = 'Push'
+            mirror_settings.authentication_method = 'Password'
+            mirror_settings.password = Runtime::User.password
+            mirror_settings.mirror_repository
+            mirror_settings.update target_project_uri
+          end
+        end
+
+        # Check that the target project has the commit from the source
+        target_project.visit!
+        expect(page).to have_content('README.md')
+        expect(page).to have_content('The rendered file could not be displayed because it is stored in LFS')
+      end
+    end
+  end
+end
diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
index 6eb161ecafb50b..00d4acbd1e1053 100644
--- a/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
+++ b/qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb
@@ -4,7 +4,6 @@ module QA
   RSpec.describe 'Create' do
     describe 'Push mirror a repository over HTTP' do
       it 'configures and syncs a (push) mirrored repository', testcase: 'https://gitlab.com/gitlab-org/quality/testcases/-/issues/414' do
-        Runtime::Feature.enable_and_verify('push_mirror_syncs_lfs')
         Runtime::Browser.visit(:gitlab, Page::Main::Login)
         Page::Main::Login.perform(&:sign_in_using_credentials)
 
@@ -18,7 +17,6 @@ module QA
           push.file_name = 'README.md'
           push.file_content = '# This is a test project'
           push.commit_message = 'Add README.md'
-          push.use_lfs = true
         end
         source_project_push.project.visit!
 
-- 
GitLab


From 82243a36abec7e777fe1d294738e11cd76ed42c6 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 15:53:50 +0100
Subject: [PATCH 32/41] Robustify a couple of specs

---
 spec/lib/gitlab/lfs/client_spec.rb | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index 1fcef80b0b0ca6..0ac03e43c68ea9 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -28,7 +28,7 @@
 
     context 'server returns 200 OK' do
       it 'makes a successful batch request' do
-        stub_batch(
+        stub = stub_batch(
           objects: objects,
           headers: basic_auth_headers
         ).to_return(
@@ -39,6 +39,7 @@
 
         result = lfs_client.batch('upload', objects)
 
+        expect(stub).to have_been_requested
         expect(result).to eq('objects' => 'anything', 'transfer' => 'basic')
       end
     end
@@ -95,12 +96,14 @@ def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
 
     context 'server returns 200 OK to an unauthenticated request' do
       it "makes an HTTP PUT with expected parameters" do
-        stub_upload(
+        stub = stub_upload(
           object: object,
           headers: basic_auth_headers.merge(upload_action['header'])
         ).to_return(status: 200)
 
         lfs_client.upload(object, upload_action, authenticated: false)
+
+        expect(stub).to have_been_requested
       end
     end
 
-- 
GitLab


From 5af59cdc58bb00a692dcc9b70d9c49ac3901a55b Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:06:01 +0100
Subject: [PATCH 33/41] Add a spec for the LfsObject with no file case

---
 spec/lib/gitlab/lfs/client_spec.rb | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index 0ac03e43c68ea9..b4f0b24d013368 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -107,6 +107,22 @@ def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
       end
     end
 
+    context 'LFS object has no file' do
+      let(:object) { LfsObject.new }
+
+      it 'makes an HJTT PUT with expected parameters' do
+        stub = stub_upload(
+          object: object,
+          headers: upload_action['header']
+        ).to_return(status: 200)
+
+        lfs_client.upload(object, upload_action, authenticated: true)
+
+        expect(stub).to have_been_requested
+      end
+    end
+
+
     context 'server returns 400 error' do
       it 'raises an error' do
         stub_upload(object: object, headers: upload_action['header']).to_return(status: 400)
-- 
GitLab


From 33d283878770e352b27583a887808a6527775f49 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:10:20 +0100
Subject: [PATCH 34/41] Refine the SSH LFS support TODO

---
 app/services/projects/update_remote_mirror_service.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index 9bfde0928cbf85..fa1b91f237e9e2 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -51,7 +51,8 @@ def send_lfs_objects!(remote_mirror)
       return unless project.lfs_enabled?
       return if project.lfs_objects.count == 0
 
-      # TODO: LFS sync over SSH
+      # TODO: Support LFS sync over SSH
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/249587
       return unless remote_mirror.url =~ /\Ahttps?:\/\//i
       return unless remote_mirror.password_auth?
 
-- 
GitLab


From 14480c2529ad30e97284d54149ea5774305014d5 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:22:27 +0100
Subject: [PATCH 35/41] Pluralise a method name

---
 app/models/project.rb            | 4 ++--
 app/services/lfs/push_service.rb | 2 +-
 spec/models/project_spec.rb      | 6 +++---
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index b559d4e3d6ba96..c7ef8db5801eb9 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -1469,10 +1469,10 @@ def fork_source
     forked_from_project || fork_network&.root_project
   end
 
-  def lfs_objects_for_repository_type(type)
+  def lfs_objects_for_repository_types(*types)
     LfsObject
       .joins(:lfs_objects_projects)
-      .where(lfs_objects_projects: { project: self, repository_type: type })
+      .where(lfs_objects_projects: { project: self, repository_type: types })
   end
 
   def lfs_objects_oids(oids: [])
diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index 80dcb89eb15403..d6e4f78c6e5962 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -8,7 +8,7 @@ class PushService < BaseService
 
     def execute
       project
-        .lfs_objects_for_repository_type([nil, :project])
+        .lfs_objects_for_repository_types(nil, :project)
         .each_batch { |objects| push_objects(objects) }
 
       success
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index f4d33a3af49927..d008b8c06d4775 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2909,9 +2909,9 @@
         create(:lfs_objects_project, project: project, repository_type: type).lfs_object
       end
 
-      expect(project.lfs_objects_for_repository_type(nil)).to contain_exactly(none)
-      expect(project.lfs_objects_for_repository_type([nil, :wiki])).to contain_exactly(none, wiki)
-      expect(project.lfs_objects_for_repository_type(:design)).to contain_exactly(design)
+      expect(project.lfs_objects_for_repository_types(nil)).to contain_exactly(none)
+      expect(project.lfs_objects_for_repository_types(nil, :wiki)).to contain_exactly(none, wiki)
+      expect(project.lfs_objects_for_repository_types(:design)).to contain_exactly(design)
     end
   end
 
-- 
GitLab


From 0359c7ff754067246ff3433f92d9713e78f0fb38 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:22:43 +0100
Subject: [PATCH 36/41] Handle the nil case for RemoteMirror#bare_url

---
 app/models/remote_mirror.rb       |  3 +--
 spec/models/remote_mirror_spec.rb | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/app/models/remote_mirror.rb b/app/models/remote_mirror.rb
index 74648755692cbb..6b8b34ce4d2492 100644
--- a/app/models/remote_mirror.rb
+++ b/app/models/remote_mirror.rb
@@ -211,8 +211,7 @@ def safe_url
   end
 
   def bare_url
-    original = read_attribute(:url)
-    Gitlab::UrlSanitizer.new(original).full_url if original
+    Gitlab::UrlSanitizer.new(read_attribute(:url)).full_url
   end
 
   def ensure_remote!
diff --git a/spec/models/remote_mirror_spec.rb b/spec/models/remote_mirror_spec.rb
index 6e18002a190d37..4c3151f431c007 100644
--- a/spec/models/remote_mirror_spec.rb
+++ b/spec/models/remote_mirror_spec.rb
@@ -142,6 +142,20 @@
     end
   end
 
+  describe '#bare_url' do
+    it 'returns the URL without any credentials' do
+      remote_mirror = build(:remote_mirror, url: 'http://user:pass@example.com/foo')
+
+      expect(remote_mirror.bare_url).to eq('http://example.com/foo')
+    end
+
+    it 'returns an empty string when the URL is nil' do
+      remote_mirror = build(:remote_mirror, url: nil)
+
+      expect(remote_mirror.bare_url).to eq('')
+    end
+  end
+
   describe '#update_repository' do
     it 'performs update including options' do
       git_remote_mirror = stub_const('Gitlab::Git::RemoteMirror', spy)
-- 
GitLab


From c7cfb102e8fd7fff493e880cfdb70cb018850014 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:23:12 +0100
Subject: [PATCH 37/41] Fix a couple of lint errors

---
 .rubocop_todo.yml                | 1 +
 app/services/lfs/push_service.rb | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml
index 7522230c418ef3..32c281dbb4e7ca 100644
--- a/.rubocop_todo.yml
+++ b/.rubocop_todo.yml
@@ -815,6 +815,7 @@ Rails/SaveBang:
     - 'ee/spec/workers/repository_import_worker_spec.rb'
     - 'ee/spec/workers/update_all_mirrors_worker_spec.rb'
     - 'qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_over_http_spec.rb'
+    - 'qa/qa/specs/features/browser_ui/3_create/repository/push_mirroring_lfs_over_http_spec.rb'
     - 'qa/qa/specs/features/ee/browser_ui/3_create/repository/pull_mirroring_over_http_spec.rb'
     - 'qa/qa/specs/features/ee/browser_ui/3_create/repository/pull_mirroring_over_ssh_with_key_spec.rb'
     - 'spec/controllers/abuse_reports_controller_spec.rb'
diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index d6e4f78c6e5962..bb9c2165171aa4 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -32,7 +32,6 @@ def push_objects(objects)
     end
 
     def upload_object!(object, spec)
-      size = spec['size'].to_i
       authenticated = spec['authenticated']
       upload = spec.dig('actions', 'upload')
 
-- 
GitLab


From 04e4d0aa982715ea56d09cf29c28cb22a3fed103 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 16:38:04 +0100
Subject: [PATCH 38/41] Fix a describe block name

---
 spec/models/project_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index d008b8c06d4775..d121a0057c8035 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -2901,7 +2901,7 @@
     end
   end
 
-  describe '#lfs_objects_for_repository_type' do
+  describe '#lfs_objects_for_repository_types' do
     let(:project) { create(:project) }
 
     it 'returns LFS objects of the specified type only' do
-- 
GitLab


From 47300a85f71e9b4d41e850821fb081236fec276f Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Tue, 15 Sep 2020 17:17:19 +0100
Subject: [PATCH 39/41] Remove a stray newline

---
 spec/lib/gitlab/lfs/client_spec.rb | 1 -
 1 file changed, 1 deletion(-)

diff --git a/spec/lib/gitlab/lfs/client_spec.rb b/spec/lib/gitlab/lfs/client_spec.rb
index b4f0b24d013368..03563a632d69fa 100644
--- a/spec/lib/gitlab/lfs/client_spec.rb
+++ b/spec/lib/gitlab/lfs/client_spec.rb
@@ -122,7 +122,6 @@ def stub_batch(objects:, headers:, operation: 'upload', transfer: 'basic')
       end
     end
 
-
     context 'server returns 400 error' do
       it 'raises an error' do
         stub_upload(object: object, headers: upload_action['header']).to_return(status: 400)
-- 
GitLab


From 2d28442fb23bd8dfa6044a61ac0e6211b771ef0e Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Wed, 16 Sep 2020 16:49:41 +0100
Subject: [PATCH 40/41] Address minor review feedback

---
 app/services/lfs/push_service.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index bb9c2165171aa4..b800517e59e1e8 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -7,6 +7,10 @@ class PushService < BaseService
     include Gitlab::Utils::StrongMemoize
 
     def execute
+      # Currently we only set repository_type for design repository objects, so
+      # push mirroring must send objects with a `nil` repository type - but if
+      # the wiki repository uses LFS, its objects will also be sent. This will
+      # be addressed by https://gitlab.com/gitlab-org/gitlab/-/issues/250346
       project
         .lfs_objects_for_repository_types(nil, :project)
         .each_batch { |objects| push_objects(objects) }
@@ -45,9 +49,6 @@ def upload_object!(object, spec)
     end
 
     def verify_object!(object, spec)
-      verify = spec.dig('actions', 'verify')
-      return unless verify
-
       log_error("LFS upload verification requested, but not supported for #{object.oid}")
     end
 
-- 
GitLab


From db71c218173efaa0d2236a327d56a68525708316 Mon Sep 17 00:00:00 2001
From: Nick Thomas <nick@gitlab.com>
Date: Thu, 17 Sep 2020 12:56:10 +0100
Subject: [PATCH 41/41] Address maintainer review comments

---
 app/services/lfs/push_service.rb              | 25 +++++---
 .../projects/update_remote_mirror_service.rb  |  1 -
 spec/services/lfs/push_service_spec.rb        | 62 +++++++++----------
 .../update_remote_mirror_service_spec.rb      |  8 ---
 4 files changed, 49 insertions(+), 47 deletions(-)

diff --git a/app/services/lfs/push_service.rb b/app/services/lfs/push_service.rb
index b800517e59e1e8..6e1a11ebff8304 100644
--- a/app/services/lfs/push_service.rb
+++ b/app/services/lfs/push_service.rb
@@ -6,14 +6,14 @@ module Lfs
   class PushService < BaseService
     include Gitlab::Utils::StrongMemoize
 
+    # Match the canonical LFS client's batch size:
+    # https://github.com/git-lfs/git-lfs/blob/master/tq/transfer_queue.go#L19
+    BATCH_SIZE = 100
+
     def execute
-      # Currently we only set repository_type for design repository objects, so
-      # push mirroring must send objects with a `nil` repository type - but if
-      # the wiki repository uses LFS, its objects will also be sent. This will
-      # be addressed by https://gitlab.com/gitlab-org/gitlab/-/issues/250346
-      project
-        .lfs_objects_for_repository_types(nil, :project)
-        .each_batch { |objects| push_objects(objects) }
+      lfs_objects_relation.each_batch(of: BATCH_SIZE) do |objects|
+        push_objects(objects)
+      end
 
       success
     rescue => err
@@ -22,6 +22,14 @@ def execute
 
     private
 
+    # Currently we only set repository_type for design repository objects, so
+    # push mirroring must send objects with a `nil` repository type - but if the
+    # wiki repository uses LFS, its objects will also be sent. This will be
+    # addressed by https://gitlab.com/gitlab-org/gitlab/-/issues/250346
+    def lfs_objects_relation
+      project.lfs_objects_for_repository_types(nil, :project)
+    end
+
     def push_objects(objects)
       rsp = lfs_client.batch('upload', objects)
       objects = objects.index_by(&:oid)
@@ -49,6 +57,9 @@ def upload_object!(object, spec)
     end
 
     def verify_object!(object, spec)
+      # TODO: the remote has requested that we make another call to verify that
+      # the object has been sent correctly.
+      # https://gitlab.com/gitlab-org/gitlab/-/issues/250654
       log_error("LFS upload verification requested, but not supported for #{object.oid}")
     end
 
diff --git a/app/services/projects/update_remote_mirror_service.rb b/app/services/projects/update_remote_mirror_service.rb
index fa1b91f237e9e2..5c41f00aac2211 100644
--- a/app/services/projects/update_remote_mirror_service.rb
+++ b/app/services/projects/update_remote_mirror_service.rb
@@ -49,7 +49,6 @@ def update_mirror(remote_mirror)
     def send_lfs_objects!(remote_mirror)
       return unless Feature.enabled?(:push_mirror_syncs_lfs, project)
       return unless project.lfs_enabled?
-      return if project.lfs_objects.count == 0
 
       # TODO: Support LFS sync over SSH
       # https://gitlab.com/gitlab-org/gitlab/-/issues/249587
diff --git a/spec/services/lfs/push_service_spec.rb b/spec/services/lfs/push_service_spec.rb
index df6a8e5a5ce5c0..8e5b98fdc9ce6d 100644
--- a/spec/services/lfs/push_service_spec.rb
+++ b/spec/services/lfs/push_service_spec.rb
@@ -15,26 +15,43 @@
   subject(:service) { described_class.new(project, nil, params) }
 
   describe "#execute" do
-    context 'upload is requested' do
-      it 'uploads the object' do
-        stub_lfs_batch(lfs_object)
+    it 'uploads the object when upload is requested' do
+      stub_lfs_batch(lfs_object)
 
-        expect(lfs_client)
-          .to receive(:upload)
-          .with(lfs_object, upload_action_spec(lfs_object), authenticated: true)
+      expect(lfs_client)
+        .to receive(:upload)
+        .with(lfs_object, upload_action_spec(lfs_object), authenticated: true)
 
-        expect(service.execute).to eq(status: :success)
-      end
+      expect(service.execute).to eq(status: :success)
     end
 
-    context 'upload is not requested' do
-      it 'does not upload the object' do
-        stub_lfs_batch(lfs_object, upload: false)
+    it 'does nothing if there are no LFS objects' do
+      lfs_object.destroy!
 
-        expect(lfs_client).not_to receive(:upload)
+      expect(lfs_client).not_to receive(:upload)
 
-        expect(service.execute).to eq(status: :success)
-      end
+      expect(service.execute).to eq(status: :success)
+    end
+
+    it 'does not upload the object when upload is not requested' do
+      stub_lfs_batch(lfs_object, upload: false)
+
+      expect(lfs_client).not_to receive(:upload)
+
+      expect(service.execute).to eq(status: :success)
+    end
+
+    it 'returns a failure when submitting a batch fails' do
+      expect(lfs_client).to receive(:batch) { raise 'failed' }
+
+      expect(service.execute).to eq(status: :error, message: 'failed')
+    end
+
+    it 'returns a failure when submitting an upload fails' do
+      stub_lfs_batch(lfs_object)
+      expect(lfs_client).to receive(:upload) { raise 'failed' }
+
+      expect(service.execute).to eq(status: :error, message: 'failed')
     end
 
     context 'non-project-repository LFS objects' do
@@ -48,23 +65,6 @@
         expect(service.execute).to eq(status: :success)
       end
     end
-
-    context 'submitting a batch fails' do
-      it 'returns a failure' do
-        expect(lfs_client).to receive(:batch) { raise 'failed' }
-
-        expect(service.execute).to eq(status: :error, message: 'failed')
-      end
-    end
-
-    context 'submitting an upload fails' do
-      it 'returns a failure' do
-        stub_lfs_batch(lfs_object)
-        expect(lfs_client).to receive(:upload) { raise 'failed' }
-
-        expect(service.execute).to eq(status: :error, message: 'failed')
-      end
-    end
   end
 
   def create_linked_lfs_object(project, type)
diff --git a/spec/services/projects/update_remote_mirror_service_spec.rb b/spec/services/projects/update_remote_mirror_service_spec.rb
index 9684c4b046b8a5..1de04888e0aff6 100644
--- a/spec/services/projects/update_remote_mirror_service_spec.rb
+++ b/spec/services/projects/update_remote_mirror_service_spec.rb
@@ -165,14 +165,6 @@
           execute!
         end
 
-        it 'does nothing with no LFS objects' do
-          lfs_pointer.destroy!
-
-          expect_any_instance_of(Lfs::PushService).not_to receive(:execute)
-
-          execute!
-        end
-
         it 'does nothing if non-password auth is specified' do
           remote_mirror.update!(auth_method: 'ssh_public_key')
 
-- 
GitLab