From 365b1fec6e5e6945574bc095b6dab60ee9c52d6f Mon Sep 17 00:00:00 2001
From: Arnaud Levy <contact@arnaudlevy.com>
Date: Fri, 29 Jul 2022 13:59:47 +0200
Subject: [PATCH] clean refactor
---
app/models/communication/website/git_file.rb | 4 ++
app/services/git/analyzer.rb | 72 +++++++++++++++++++
app/services/git/providers/github.rb | 32 +++++----
app/services/git/repository.rb | 73 ++++++--------------
4 files changed, 114 insertions(+), 67 deletions(-)
create mode 100644 app/services/git/analyzer.rb
diff --git a/app/models/communication/website/git_file.rb b/app/models/communication/website/git_file.rb
index 87795be36..71e0f33ff 100644
--- a/app/models/communication/website/git_file.rb
+++ b/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
diff --git a/app/services/git/analyzer.rb b/app/services/git/analyzer.rb
new file mode 100644
index 000000000..cbc3eb234
--- /dev/null
+++ b/app/services/git/analyzer.rb
@@ -0,0 +1,72 @@
+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
diff --git a/app/services/git/providers/github.rb b/app/services/git/providers/github.rb
index 949d14ef3..f42b8b01f 100644
--- a/app/services/git/providers/github.rb
+++ b/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
diff --git a/app/services/git/repository.rb b/app/services/git/repository.rb
index f36e0ad9d..6609f0fd5 100644
--- a/app/services/git/repository.rb
+++ b/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}"
--
GitLab