Refactor repository related code

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2021-04-21 12:35:40 +0200 using RuboCop version 0.50.0.
+# on 2021-04-21 19:04:03 +0200 using RuboCop version 0.50.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -11,42 +11,35 @@ Lint/UselessAssignment:
   Exclude:
     - 'lib/modulesync.rb'
 
-# Offense count: 11
+# Offense count: 10
 Metrics/AbcSize:
-  Max: 48
+  Max: 53
 
-# Offense count: 1
+# Offense count: 2
 # Configuration parameters: CountComments.
 Metrics/ClassLength:
-  Max: 106
+  Max: 158
 
 # Offense count: 3
 Metrics/CyclomaticComplexity:
   Max: 12
 
-# Offense count: 2
-# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
-# URISchemes: http, https
-Metrics/LineLength:
-  Max: 132
-
 # Offense count: 13
 # Configuration parameters: CountComments.
 Metrics/MethodLength:
-  Max: 43
+  Max: 40
 
 # Offense count: 3
 Metrics/PerceivedComplexity:
   Max: 15
 
-# Offense count: 9
+# Offense count: 8
 Style/Documentation:
   Exclude:
     - 'spec/**/*'
     - 'test/**/*'
     - 'lib/modulesync.rb'
     - 'lib/modulesync/cli.rb'
-    - 'lib/modulesync/git.rb'
     - 'lib/modulesync/hook.rb'
     - 'lib/modulesync/renderer.rb'
     - 'lib/modulesync/util.rb'

features/update.feature

@@ -461,6 +461,7 @@ Feature: update
     And a directory named "moduleroot"
     When I run `msync update --message "Running without changes"`
     Then the exit status should be 0
+    And the stdout should contain "There were no changes in 'modules/fakenamespace/puppet-test'. Not committing."
     And the puppet module "puppet-test" from "fakenamespace" should have no commits made by "Aruba"
 
   Scenario: When specifying configurations in managed_modules.yml

lib/modulesync.rb

@@ -3,7 +3,7 @@
 
 require 'modulesync/cli'
 require 'modulesync/constants'
-require 'modulesync/git'
+require 'modulesync/repository'
 require 'modulesync/hook'
 require 'modulesync/puppet_module'
 require 'modulesync/renderer'
@@ -111,7 +111,9 @@ def self.manage_file(puppet_module, filename, settings, options)
   end
 
   def self.manage_module(puppet_module, module_files, defaults)
-    Git.pull(puppet_module, options[:branch]) unless options[:offline]
+    repository = Repository.new directory: puppet_module.working_directory, remote: puppet_module.repository_remote
+    puts "Syncing '#{puppet_module.given_name}'"
+    repository.prepare_workspace(options[:branch]) unless options[:offline]
 
     module_configs = Util.parse_config(module_file(puppet_module, MODULE_CONF_FILE))
     settings = Settings.new(defaults[GLOBAL_DEFAULTS_KEY] || {},
@@ -130,11 +132,12 @@ def self.manage_module(puppet_module, module_files, defaults)
     files_to_manage.each { |filename| manage_file(puppet_module, filename, settings, options) }
 
     if options[:noop]
-      Git.update_noop(puppet_module.repository_path, options)
+      puts "Using no-op. Files in '#{puppet_module.given_name}' may be changed but will not be committed."
+      repository.show_changes(options)
       options[:pr] && \
         pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
     elsif !options[:offline]
-      pushed = Git.update(puppet_module.repository_path, files_to_manage, options)
+      pushed = repository.submit_changes(files_to_manage, options)
       pushed && options[:pr] && \
         pr(puppet_module).manage(puppet_module.repository_namespace, puppet_module.repository_name, options)
     end

lib/modulesync/git.rb → lib/modulesync/repository.rb

@@ -2,39 +2,52 @@
 require 'puppet_blacksmith'
 
 module ModuleSync
-  module Git # rubocop:disable Metrics/ModuleLength
-    include Constants
+  # Wrapper for Git in ModuleSync context
+  class Repository
+    def initialize(directory:, remote:)
+      @directory = directory
+      @remote = remote
+    end
+
+    def git
+      @git ||= Git.open @directory
+    end
 
-    def self.remote_branch_exists?(repo, branch)
+    # This is an alias to minimize code alteration
+    def repo
+      git
+    end
+
+    def remote_branch_exists?(branch)
       repo.branches.remote.collect(&:name).include?(branch)
     end
 
-    def self.local_branch_exists?(repo, branch)
+    def local_branch_exists?(branch)
       repo.branches.local.collect(&:name).include?(branch)
     end
 
-    def self.remote_branch_differ?(repo, local_branch, remote_branch)
-      !remote_branch_exists?(repo, remote_branch) ||
+    def remote_branch_differ?(local_branch, remote_branch)
+      !remote_branch_exists?(remote_branch) ||
         repo.diff("#{local_branch}..origin/#{remote_branch}").any?
     end
 
-    def self.default_branch(repo)
+    def default_branch
       symbolic_ref = repo.branches.find { |b| b.full =~ %r{remotes/origin/HEAD} }
       return unless symbolic_ref
       %r{remotes/origin/HEAD\s+->\s+origin/(?<branch>.+?)$}.match(symbolic_ref.full)[:branch]
     end
 
-    def self.switch_branch(repo, branch)
+    def switch_branch(branch)
       unless branch
-        branch = default_branch(repo)
+        branch = default_branch
         puts "Using repository's default branch: #{branch}"
       end
       return if repo.current_branch == branch
 
-      if local_branch_exists?(repo, branch)
+      if local_branch_exists?(branch)
         puts "Switching to branch #{branch}"
         repo.checkout(branch)
-      elsif remote_branch_exists?(repo, branch)
+      elsif remote_branch_exists?(branch)
         puts "Creating local branch #{branch} from origin/#{branch}"
         repo.checkout("origin/#{branch}")
         repo.branch(branch).checkout
@@ -45,33 +58,30 @@ def self.switch_branch(repo, branch)
       end
     end
 
-    def self.pull(puppet_module, branch)
-      puts "Syncing '#{puppet_module.given_name}'"
-
+    def prepare_workspace(branch)
       # Repo needs to be cloned in the cwd
-      if !Dir.exist?("#{puppet_module.working_directory}/.git")
+      if !Dir.exist?("#{@directory}/.git")
         puts 'Cloning repository fresh'
-        remote = puppet_module.repository_remote
-        local = puppet_module.working_directory
-        puts "Cloning from '#{remote}'"
-        repo = ::Git.clone(remote, local)
-        switch_branch(repo, branch)
+        puts "Cloning from '#{@remote}'"
+        @git = Git.clone(@remote, @directory)
+        switch_branch(branch)
       # Repo already cloned, check out master and override local changes
       else
         # Some versions of git can't properly handle managing a repo from outside the repo directory
-        Dir.chdir(puppet_module.working_directory) do
-          puts "Overriding any local changes to repositories in '#{puppet_module.working_directory}'"
-          repo = ::Git.open('.')
+        Dir.chdir(@directory) do
+          puts "Overriding any local changes to repository in '#{@directory}'"
+          @git = Git.open('.')
           repo.fetch
           repo.reset_hard
-          switch_branch(repo, branch)
-          repo.pull('origin', branch) if remote_branch_exists?(repo, branch)
+          switch_branch(branch)
+          git.pull('origin', branch) if remote_branch_exists?(branch)
         end
       end
     end
 
-    def self.update_changelog(repo, version, message, module_root)
-      changelog = "#{module_root}/CHANGELOG.md"
+    # PuppetModule, is it used?
+    def update_changelog(version, message)
+      changelog = "#{@directory}/CHANGELOG.md"
       if File.exist?(changelog)
         puts "Updating #{changelog} for version #{version}"
         changes = File.readlines(changelog)
@@ -88,40 +98,39 @@ def self.update_changelog(repo, version, message, module_root)
       end
     end
 
-    def self.bump(repo, m, message, module_root, changelog = false)
+    # PuppetModule
+    def bump(message, changelog = false)
+      m = Blacksmith::Modulefile.new("#{@directory}/metadata.json")
       new = m.bump!
       puts "Bumped to version #{new}"
       repo.add('metadata.json')
-      update_changelog(repo, new, message, module_root) if changelog
+      update_changelog(new, message) if changelog
       repo.commit("Release version #{new}")
       repo.push
       new
     end
 
-    def self.tag(repo, version, tag_pattern)
+    def tag(version, tag_pattern)
       tag = tag_pattern % version
       puts "Tagging with #{tag}"
       repo.add_tag(tag)
       repo.push('origin', tag)
     end
 
-    def self.checkout_branch(repo, branch)
+    def checkout_branch(branch)
       selected_branch = branch || repo.current_branch || 'master'
       repo.branch(selected_branch).checkout
       selected_branch
     end
-    private_class_method :checkout_branch
 
     # Git add/rm, git commit, git push
-    def self.update(name, files, options)
-      module_root = "#{options[:project_root]}/#{name}"
+    def submit_changes(files, options)
       message = options[:message]
-      repo = ::Git.open(module_root)
-      branch = checkout_branch(repo, options[:branch])
+      branch = checkout_branch(options[:branch])
       files.each do |file|
         if repo.status.deleted.include?(file)
           repo.remove(file)
-        elsif File.exist?("#{module_root}/#{file}")
+        elsif File.exist?("#{@directory}/#{file}")
           repo.add(file)
         end
       end
@@ -132,28 +141,27 @@ def self.update(name, files, options)
         opts_push = { :force => true } if options[:force]
         if options[:pre_commit_script]
           script = "#{File.dirname(File.dirname(__FILE__))}/../contrib/#{options[:pre_commit_script]}"
-          `#{script} #{module_root}`
+          `#{script} #{@directory}`
         end
         repo.commit(message, opts_commit)
         if options[:remote_branch]
-          if remote_branch_differ?(repo, branch, options[:remote_branch])
+          if remote_branch_differ?(branch, options[:remote_branch])
             repo.push('origin', "#{branch}:#{options[:remote_branch]}", opts_push)
           end
         else
           repo.push('origin', branch, opts_push)
         end
         # Only bump/tag if pushing didn't fail (i.e. there were changes)
-        m = Blacksmith::Modulefile.new("#{module_root}/metadata.json")
         if options[:bump]
-          new = bump(repo, m, message, module_root, options[:changelog])
-          tag(repo, new, options[:tag_pattern]) if options[:tag]
+          new = bump(message, options[:changelog])
+          tag(new, options[:tag_pattern]) if options[:tag]
         end
-      rescue ::Git::GitExecuteError => git_error
-        if git_error.message.match?(/working (directory|tree) clean/)
-          puts "There were no files to update in #{name}. Not committing."
+      rescue Git::GitExecuteError => e
+        if e.message.match?(/working (directory|tree) clean/)
+          puts "There were no changes in '#{@directory}'. Not committing."
           return false
         else
-          puts git_error
+          puts e
           raise
         end
       end
@@ -164,25 +172,22 @@ def self.update(name, files, options)
     # Needed because of a bug in the git gem that lists ignored files as
     # untracked under some circumstances
     # https://github.com/schacon/ruby-git/issues/130
-    def self.untracked_unignored_files(repo)
-      ignore_path = "#{repo.dir.path}/.gitignore"
+    def untracked_unignored_files
+      ignore_path = "#{@directory}/.gitignore"
       ignored = File.exist?(ignore_path) ? File.read(ignore_path).split : []
       repo.status.untracked.keep_if { |f, _| ignored.none? { |i| File.fnmatch(i, f) } }
     end
 
-    def self.update_noop(name, options)
-      puts "Using no-op. Files in #{name} may be changed but will not be committed."
-
-      repo = ::Git.open("#{options[:project_root]}/#{name}")
-      checkout_branch(repo, options[:branch])
+    def show_changes(options)
+      checkout_branch(options[:branch])
 
       puts 'Files changed:'
       repo.diff('HEAD', '--').each do |diff|
         puts diff.patch
       end
 
       puts 'Files added:'
-      untracked_unignored_files(repo).each_key do |file|
+      untracked_unignored_files.each_key do |file|
         puts file
       end