Keep only the changes that are known to work well

Also, improved a bit the method names in
Github::Representation::PullRequest.

Last but not least, ensure temp branch names doesn't contain a `/` as
this would create the ref in a subfolder in `refs/heads` (e.g.
`refs/heads/gh-123/456/rymai/foo`), and would leave empty directories
upon branch deletion (e.g. `refs/heads/gh-123/456/rymai/`.

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/models/concerns/repository_mirroring.rb

@@ -1,36 +1,39 @@
 module RepositoryMirroring
-  IMPORT_REFS = %w[+refs/pull/*/head:refs/merge-requests/*/head +refs/heads/*:refs/heads/* +refs/tags/*:refs/tags/*].freeze
+  IMPORT_REFS = %w[
+    +refs/heads/*:refs/heads/*
+    +refs/tags/*:refs/tags/*
+  ].freeze
 
   def set_remote_as_mirror(name)
     # This is used to define repository as equivalent as "git clone --mirror"
-    config["remote.#{name}.fetch"] = 'refs/*:refs/*'
+    raw_repository.rugged.config["remote.#{name}.fetch"] = 'refs/*:refs/*'
-    config["remote.#{name}.mirror"] = true
+    raw_repository.rugged.config["remote.#{name}.mirror"] = true
-    config["remote.#{name}.prune"] = true
+    raw_repository.rugged.config["remote.#{name}.prune"] = true
   end
 
-  def set_import_remote_as_mirror(name)
+  def set_import_remote_as_mirror(remote_name)
     # Add first fetch with Rugged so it does not create its own.
-    config["remote.#{name}.fetch"] = IMPORT_REFS.first
+    raw_repository.rugged.config["remote.#{remote_name}.fetch"] = IMPORT_REFS.first
 
-    IMPORT_REFS.drop(1).each do |ref|
+    IMPORT_REFS.drop(1).each do |refspec|
-      run_git(%W[config --add remote.#{name}.fetch #{ref}])
+      add_remote_fetch_config(remote_name, refspec)
     end
 
-    config["remote.#{name}.mirror"] = true
+    raw_repository.rugged.config["remote.#{remote_name}.mirror"] = true
-    config["remote.#{name}.prune"] = true
+    raw_repository.rugged.config["remote.#{remote_name}.prune"] = true
   rescue Rugged::ConfigError
     # Ignore multivar errors when the config already exist
     # TODO: refactor/fix this
   end
 
+  def add_remote_fetch_config(remote_name, refspec)
+    run_git(%W[config --add remote.#{remote_name}.fetch #{refspec}])
+  end
+
   def fetch_mirror(remote, url)
     add_remote(remote, url)
     set_remote_as_mirror(remote)
     fetch_remote(remote, forced: true)
     remove_remote(remote)
   end
-
-  def config
-    raw_repository.rugged.config
-  end
 end

app/models/merge_request_diff.rb

@@ -55,7 +55,7 @@ class MergeRequestDiff < ActiveRecord::Base
   end
 
   def ensure_commit_shas
-    merge_request.fetch_ref unless merge_request.source_branch_head
+    merge_request.fetch_ref
 
     self.start_commit_sha ||= merge_request.target_branch_sha
     self.head_commit_sha  ||= merge_request.source_branch_sha

lib/github/import.rb

@@ -57,6 +57,7 @@ module Github
         project.ensure_repository
         project.repository.add_remote('github', repo_url)
         project.repository.set_import_remote_as_mirror('github')
+        project.repository.add_remote_fetch_config('github', '+refs/pull/*/head:refs/merge-requests/*/head')
         project.repository.fetch_remote('github', forced: true)
       rescue Gitlab::Git::Repository::NoRepository, Gitlab::Shell::Error => e
         error(:project, repo_url, e.message)
@@ -140,7 +141,7 @@ module Github
         response.body.each do |raw|
           pull_request  = Github::Representation::PullRequest.new(raw, options.merge(project: project))
           merge_request = MergeRequest.find_or_initialize_by(iid: pull_request.iid, source_project_id: project.id)
-          next unless merge_request.new_record? && pull_request.valid? && merge_request.source_branch_head
+          next unless merge_request.new_record? && pull_request.valid?
 
           begin
             pull_request.restore_branches!
@@ -173,6 +174,8 @@ module Github
             fetch_comments(merge_request, :review_comment, review_comments_url, LegacyDiffNote)
           rescue => e
             error(:pull_request, pull_request.url, e.message)
+          ensure
+            pull_request.remove_tmp_branches!
           end
         end
 

lib/github/representation/pull_request.rb

@@ -9,20 +9,13 @@ module Github
       end
 
       def source_branch_name
-        @source_branch_name ||=
+        @source_branch_name ||= source_branch_exists? ? source_branch_ref : tmp_source_branch
-          if opened? && project.repository.branch_exists?(source_branch.ref)
-            source_branch.ref
-          elsif cross_project? && opened?
-            source_branch_name_prefixed
-          else
-            source_branch_ref
-          end
       end
 
       def source_branch_exists?
         return @source_branch_exists if defined?(@source_branch_exists)
 
-        @source_branch_exists = project.repository.branch_exists?(source_branch_name)
+        @source_branch_exists = !cross_project? && source_branch.exists?
       end
 
       def target_project
@@ -30,7 +23,7 @@ module Github
       end
 
       def target_branch_name
-        @target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed
+        @target_branch_name ||= target_branch_exists? ? target_branch_ref : tmp_target_branch
       end
 
       def target_branch_exists?
@@ -57,6 +50,13 @@ module Github
         restore_target_branch!
       end
 
+      def remove_tmp_branches!
+        return if opened?
+
+        remove_tmp_source_branch!
+        remove_tmp_target_branch!
+      end
+
       private
 
       def project
@@ -67,22 +67,10 @@ module Github
         @source_branch ||= Representation::Branch.new(raw['head'], repository: project.repository)
       end
 
-      def source_branch_ref
-        "refs/merge-requests/#{iid}/head"
-      end
-
-      def source_branch_name_prefixed
-        "gh-#{target_branch_short_sha}/#{iid}/#{source_branch_user}/#{source_branch.ref}"
-      end
-
       def target_branch
         @target_branch ||= Representation::Branch.new(raw['base'], repository: project.repository)
       end
 
-      def target_branch_name_prefixed
-        "gl-#{target_branch_short_sha}/#{iid}/#{target_branch_user}/#{target_branch_ref}"
-      end
-
       def cross_project?
         return true if source_branch_repo.nil?
 
@@ -100,6 +88,20 @@ module Github
 
         target_branch.restore!(target_branch_name)
       end
+
+      def remove_tmp_source_branch!
+        # We should remove the source/target branches only if they were
+        # restored. Otherwise, we'll remove branches like 'master' that
+        # target_branch_exists? returns true. In other words, we need
+        # to clean up only the restored branches that (source|target)_branch_exists?
+        # returns false for the first time it has been called, because of
+        # this that is important to memoize these values.
+        source_branch.remove!(source_branch_name) unless source_branch_exists?
+      end
+
+      def remove_tmp_target_branch!
+        target_branch.remove!(target_branch_name) unless target_branch_exists?
+      end
     end
   end
 end