Refactor, no change in behavior

lib/gitlab/background_migration/populate_untracked_uploads.rb

@@ -55,9 +55,7 @@ module Gitlab
         def ensure_tracked!
           return if persisted? && tracked?
 
-          unless in_uploads?
-            add_to_uploads
-          end
+          add_to_uploads unless in_uploads?
 
           mark_as_tracked
         end
@@ -82,8 +80,7 @@ module Gitlab
         end
 
         def mark_as_tracked
-          self.tracked = true
-          self.save!
+          update!(tracked: true)
         end
 
         def upload_path
@@ -121,7 +118,8 @@ module Gitlab
 
         # Not including a leading slash
         def path_relative_to_upload_dir
-          @path_relative_to_upload_dir ||= path.sub(%r{\A#{Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR}/}, '')
+          base = %r{\A#{Regexp.escape(Gitlab::BackgroundMigration::PrepareUnhashedUploads::UPLOAD_DIR)}/}
+          @path_relative_to_upload_dir ||= path.sub(base, '')
         end
 
         # Not including a leading slash

lib/gitlab/background_migration/prepare_unhashed_uploads.rb

@@ -35,29 +35,36 @@ module Gitlab
       def store_unhashed_upload_file_paths
         return unless Dir.exist?(UPLOAD_DIR)
 
-        file_paths = []
-        each_file_path(UPLOAD_DIR) do |file_path|
-          file_paths << file_path
-
-          if file_paths.size >= FILE_PATH_BATCH_SIZE
-            insert_file_paths(file_paths)
-            file_paths = []
-          end
+        each_file_batch(UPLOAD_DIR, FILE_PATH_BATCH_SIZE) do |file_paths|
+          insert_file_paths(file_paths)
         end
-
-        insert_file_paths(file_paths) if file_paths.any?
       end
 
-      def each_file_path(search_dir, &block)
+      def each_file_batch(search_dir, batch_size, &block)
         cmd = build_find_command(search_dir)
+
         Open3.popen2(*cmd) do |stdin, stdout, status_thread|
-          stdout.each_line("\0") do |line|
-            yield(line.chomp("\0"))
-          end
+          yield_paths_in_batches(stdout, batch_size, &block)
+
           raise "Find command failed" unless status_thread.value.success?
         end
       end
 
+      def yield_paths_in_batches(stdout, batch_size, &block)
+        paths = []
+
+        stdout.each_line("\0") do |line|
+          paths << line.chomp("\0")
+
+          if paths.size >= batch_size
+            yield(paths)
+            paths = []
+          end
+        end
+
+        yield(paths)
+      end
+
       def build_find_command(search_dir)
         hashed_path = "#{UPLOAD_DIR}/@hashed/*"
         tmp_path = "#{UPLOAD_DIR}/tmp/*"

spec/lib/gitlab/background_migration/populate_untracked_uploads_spec.rb

@@ -114,6 +114,8 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads, :migration, :sid
 end
 
 describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFile do
+  include TrackUntrackedUploadsHelpers
+
   let(:upload_class) { Gitlab::BackgroundMigration::PopulateUntrackedUploads::Upload }
 
   describe '#ensure_tracked!' do
@@ -596,11 +598,4 @@ describe Gitlab::BackgroundMigration::PopulateUntrackedUploads::UnhashedUploadFi
       end
     end
   end
-
-  def rails_sample_jpg_attrs
-    {
-      "size"       => 35255,
-      "checksum"   => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844'
-    }
-  end
 end

spec/migrations/track_untracked_uploads_spec.rb

@@ -2,6 +2,8 @@ require 'spec_helper'
 require Rails.root.join('db', 'post_migrate', '20171103140253_track_untracked_uploads')
 
 describe TrackUntrackedUploads, :migration, :sidekiq do
+  include TrackUntrackedUploadsHelpers
+
   class UnhashedUploadFile < ActiveRecord::Base
     self.table_name = 'unhashed_upload_files'
   end
@@ -36,11 +38,18 @@ describe TrackUntrackedUploads, :migration, :sidekiq do
   it 'has a path field long enough for really long paths' do
     migrate!
 
-    max_length_namespace_path = max_length_project_path = max_length_filename = 'a' * 255
-    long_path = "./uploads#{"/#{max_length_namespace_path}" * Namespace::NUMBER_OF_ANCESTORS_ALLOWED}/#{max_length_project_path}/#{max_length_filename}"
-    unhashed_upload_file = UnhashedUploadFile.new(path: long_path)
-    unhashed_upload_file.save!
-    expect(UnhashedUploadFile.first.path.size).to eq(5641)
+    component = 'a'*255
+
+    long_path = [
+      CarrierWave.root,
+      'uploads',
+      [component] * Namespace::NUMBER_OF_ANCESTORS_ALLOWED, # namespaces
+      component, # project
+      component  # filename
+    ].flatten.join('/')
+
+    record = UnhashedUploadFile.create!(path: long_path)
+    expect(record.reload.path.size).to eq(5711)
   end
 
   context 'with tracked and untracked uploads' do
@@ -132,11 +141,4 @@ describe TrackUntrackedUploads, :migration, :sidekiq do
       end
     end
   end
-
-  def rails_sample_jpg_attrs
-    {
-      "size"       => 35255,
-      "checksum"   => 'f2d1fd9d8d8a3368d468fa067888605d74a66f41c16f55979ceaf2af77375844'
-    }
-  end
 end

spec/support/track_untracked_uploads_helpers.rb

@@ -0,0 +1,12 @@
+module TrackUntrackedUploadsHelpers
+  def rails_sample_jpg_attrs
+    @rails_sample_jpg_attrs ||= {
+      "size"       => File.size(rails_sample_file_path),
+      "checksum"   => Digest::SHA256.file(rails_sample_file_path).hexdigest
+    }
+  end
+
+  def rails_sample_file_path
+    Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')
+  end
+end