clean refactor

app/models/communication/website/git_file.rb

@@ -31,6 +31,10 @@ class Communication::Website::GitFile < ApplicationRecord
     object.before_git_sync
     git_file = where(website: website, about: object).first_or_create
     git_file.will_be_destroyed = destroy
+    # It is very important to go through this specific instance of the website, 
+    # and not through each git_file.website, which would be different instances.
+    # Otherwise, we get 1 instance of git_repository per git_file, 
+    # and it causes a huge amount of useless queries.
     website.git_repository.add_git_file git_file
   end
 

app/services/git/analyzer.rb

+class Git::Analyzer
+  attr_accessor :git_file
+  attr_reader :repository
+
+  def initialize(repository)
+    @repository = repository
+  end
+
+  def should_create?
+    !should_destroy? &&
+    !exists_on_git? &&
+    (
+      !synchronized_with_git? ||
+      previous_path.nil? ||
+      previous_sha.nil?
+    )
+  end
+
+  def should_update?
+    !should_destroy? &&
+    (
+      previous_path != path ||
+      previous_sha != computed_sha
+    )
+  end
+
+  def should_destroy?
+    will_be_destroyed ||
+    path.nil?
+  end
+
+  protected
+
+  def path
+    git_file.path
+  end
+
+  def sha
+    git_file.sha
+  end
+
+  def computed_sha
+    repository.computed_sha(git_file.to_s)
+  end
+
+  def will_be_destroyed
+    git_file.will_be_destroyed
+  end
+
+  def previous_path
+    git_file.previous_path
+  end
+
+  def previous_sha
+    git_file.previous_sha
+  end
+
+  def exists_on_git?
+    repository.git_sha(git_file.previous_path).present? ||  # The file exists where it was last time
+    (
+      git_file.previous_path.nil? &&                        # Never saved in the database
+      repository.git_sha(git_file.path).present?            # but it exists in the git repo
+    )
+  end
+
+  def synchronized_with_git?
+    exists_on_git? &&                                           # File exists
+    git_file.previous_path == git_file.path &&                  # at the same place
+    repository.git_sha(git_file.path) == git_file.previous_sha  # with the same content
+  end
+  
+end
\ No newline at end of file

app/services/git/providers/github.rb

@@ -9,7 +9,7 @@ class Git::Providers::Github < Git::Providers::Abstract
   end
 
   def update_file(path, previous_path, content)
-    file = tree_item_for_path(previous_path)
+    file = tree_item_at_path(previous_path)
     return if file.nil?
     batch << {
       path: previous_path,
@@ -26,7 +26,7 @@ class Git::Providers::Github < Git::Providers::Abstract
   end
 
   def destroy_file(path)
-    file = tree_item_for_path(path)
+    file = tree_item_at_path(path)
     return if file.nil?
     batch << {
       path: path,
@@ -41,6 +41,10 @@ class Git::Providers::Github < Git::Providers::Abstract
     new_tree = client.create_tree repository, batch, base_tree: tree[:sha]
     commit = client.create_commit repository, commit_message, new_tree[:sha], branch_sha
     client.update_branch repository, default_branch, commit[:sha]
+    # The repo changed, invalidate the tree
+    @tree = nil
+    @tree_items_by_path = nil
+    #
     true
   end
 
@@ -53,15 +57,14 @@ class Git::Providers::Github < Git::Providers::Abstract
   def git_sha(path)
     return if path.nil?
     # Try to find in stored tree to avoid multiple queries
-    return tree_item_for_path(path)&.dig(:sha)
-    # This is still generating too many requests, so we try based only on the tree
-    # begin
-    #   # The fast way, with no query, does not work.
-    #   # Let's query the API.
-    #   content = client.content repository, path: path
-    #   return content[:sha]
-    # rescue
-    # end
+    return tree_item_at_path(path)&.dig(:sha)
+    begin
+      # The fast way, with no query, does not work.
+      # Let's query the API.
+      content = client.content repository, path: path
+      return content[:sha]
+    rescue
+    end
     nil
   end
 
@@ -79,6 +82,10 @@ class Git::Providers::Github < Git::Providers::Abstract
     @branch_sha ||= client.branch(repository, default_branch)[:commit][:sha]
   end
 
+  def tree_item_at_path(path)
+    tree_items_by_path[path] if tree_items_by_path.has_key? path
+  end
+
   def tree_items_by_path
     unless @tree_items_by_path
       @tree_items_by_path = {}
@@ -98,7 +105,4 @@ class Git::Providers::Github < Git::Providers::Abstract
     @tree ||= client.tree repository, branch_sha, recursive: true
   end
 
-  def tree_item_for_path(path)
-    tree_items_by_path[path] if tree_items_by_path.has_key? path
-  end
 end

app/services/git/repository.rb

@@ -8,7 +8,8 @@ class Git::Repository
   def add_git_file(git_file)
     puts "Adding #{git_file.path}"
     if git_files.empty?
-      action = should_destroy_file?(git_file) ? "Destroy" : "Save"
+      analyzer.git_file = git_file
+      action = analyzer.should_destroy? ? "Destroy" : "Save"
       @commit_message = "[#{ git_file.about.class.name }] #{ action } #{ git_file.about }"
     end
     git_files << git_file
@@ -50,71 +51,37 @@ class Git::Repository
     @git_files ||= []
   end
 
+  def analyzer
+    @analyzer ||= Git::Analyzer.new self
+  end
+
   def sync_git_files
-    git_files.each do |file|
-      if should_create_file?(file)
-        puts "Syncing - Creating #{file.path}"
-        provider.create_file file.path, file.to_s
-      elsif should_update_file?(file)
-        puts "Syncing - Updating #{file.path}"
-        provider.update_file file.path, file.previous_path, file.to_s
-      elsif should_destroy_file?(file)
-        puts "Syncing - Destroying #{file.previous_path}"
-        provider.destroy_file file.previous_path
+    git_files.each do |git_file|
+      analyzer.git_file = git_file
+      if analyzer.should_create?
+        puts "Syncing - Creating #{git_file.path}"
+        provider.create_file git_file.path, git_file.to_s
+      elsif analyzer.should_update?
+        puts "Syncing - Updating #{git_file.path}"
+        provider.update_file git_file.path, git_file.previous_path, git_file.to_s
+      elsif analyzer.should_destroy?
+        puts "Syncing - Destroying #{git_file.previous_path}"
+        provider.destroy_file git_file.previous_path
       else
-        puts "Syncing - Nothing to do with #{file.path}"
+        puts "Syncing - Nothing to do with #{git_file.path}"
       end
     end
   end
 
-  # TODO Arnaud : Nettoyer / Rendre + élégant
-
-  def should_create_file?(file)
-    !should_destroy_file?(file) &&
-    !file_exists_on_git?(file) &&
-    (
-      !file_synchronized_with_git?(file) ||
-      file.previous_path.nil? ||
-      file.previous_sha.nil?
-    )
-  end
-
-  def should_update_file?(file)
-    !should_destroy_file?(file) &&
-    (
-      file.previous_path != file.path ||
-      file.previous_sha != computed_sha(file.to_s)
-    )
-  end
-
-  def should_destroy_file?(file)
-    file.will_be_destroyed ||
-    file.path.nil?
-  end
-
-  def file_exists_on_git?(file)
-    git_sha(file.previous_path).present? || # The file exists where it was last time
-    (
-      file.previous_path.nil? &&  # Never saved in the database
-      git_sha(file.path).present? # but it exists in the git repo
-    )
-  end
-
-  def file_synchronized_with_git?(file)
-    file_exists_on_git?(file) && # File exists
-    file.previous_path == file.path && # at the same place
-    git_sha(file.path) == file.previous_sha # with the same content
-  end
-
   def mark_as_synced
     puts "Marking as synced"
     git_files.each do |git_file|
-      if should_destroy_file?(git_file)
+      analyzer.git_file = git_file
+      if analyzer.should_destroy?
         path = nil
         sha = nil
       else
         path = git_file.path
-        # TODO Arnaud : Invalider le cache des tree et tree_items_by_path pour GitHub pour faire un appel API au lieu de N calculs de SHA
         sha = computed_sha(git_file.to_s)
       end
       puts "Marking #{path}"