Merge branch 'ac-fix-diverged-pull-mirroring' into 'master'

Fix diverged from upstream detection with pull_mirror_branch_prefix

See merge request gitlab-org/gitlab!18069

ee/app/models/ee/repository.rb

@@ -15,6 +15,7 @@ module EE
       include Elastic::RepositoriesSearch
 
       delegate :checksum, :find_remote_root_ref, to: :raw_repository
+      delegate :pull_mirror_branch_prefix, to: :project
     end
 
     # Transiently sets a configuration variable
@@ -33,6 +34,19 @@ module EE
       expire_content_cache
     end
 
+    def upstream_branch_name(branch_name)
+      return branch_name unless ::Feature.enabled?(:pull_mirror_branch_prefix, project)
+      return branch_name unless pull_mirror_branch_prefix
+
+      # when pull_mirror_branch_prefix is set, a branch not starting with it
+      # is a local branch that doesn't tracking upstream
+      if branch_name.start_with?(pull_mirror_branch_prefix)
+        branch_name.delete_prefix(pull_mirror_branch_prefix)
+      else
+        nil
+      end
+    end
+
     def fetch_upstream(url, forced: false)
       add_remote(MIRROR_REMOTE, url)
       fetch_remote(MIRROR_REMOTE, ssh_auth: project&.import_data, forced: forced)
@@ -43,35 +57,26 @@ module EE
     end
 
     def diverged_from_upstream?(branch_name)
-      branch_commit = commit("refs/heads/#{branch_name}")
-      upstream_commit = commit("refs/remotes/#{MIRROR_REMOTE}/#{branch_name}")
+      upstream_branch = upstream_branch_name(branch_name)
+      return false unless upstream_branch
 
-      if branch_commit && upstream_commit
+      diverged?(branch_name, MIRROR_REMOTE, upstream_branch_name: upstream_branch) do |branch_commit, upstream_commit|
         !raw_repository.ancestor?(branch_commit.id, upstream_commit.id)
-      else
-        false
       end
     end
 
     def upstream_has_diverged?(branch_name, remote_ref)
-      branch_commit = commit("refs/heads/#{branch_name}")
-      upstream_commit = commit("refs/remotes/#{remote_ref}/#{branch_name}")
-
-      if branch_commit && upstream_commit
+      diverged?(branch_name, remote_ref) do |branch_commit, upstream_commit|
         !raw_repository.ancestor?(upstream_commit.id, branch_commit.id)
-      else
-        false
       end
     end
 
     def up_to_date_with_upstream?(branch_name)
-      branch_commit = commit("refs/heads/#{branch_name}")
-      upstream_commit = commit("refs/remotes/#{MIRROR_REMOTE}/#{branch_name}")
+      upstream_branch = upstream_branch_name(branch_name)
+      return false unless upstream_branch
 
-      if branch_commit && upstream_commit
+      diverged?(branch_name, MIRROR_REMOTE, upstream_branch_name: upstream_branch) do |branch_commit, upstream_commit|
         ancestor?(branch_commit.id, upstream_commit.id)
-      else
-        false
       end
     end
 
@@ -103,5 +108,18 @@ module EE
     def insights_config_for(sha)
       blob_data_at(sha, ::Gitlab::Insights::CONFIG_FILE_PATH)
     end
+
+    private
+
+    def diverged?(branch_name, remote_ref, upstream_branch_name: branch_name)
+      branch_commit = commit("refs/heads/#{branch_name}")
+      upstream_commit = commit("refs/remotes/#{remote_ref}/#{upstream_branch_name}")
+
+      if branch_commit && upstream_commit
+        yield branch_commit, upstream_commit
+      else
+        false
+      end
+    end
   end
 end

ee/spec/models/repository_spec.rb

@@ -20,6 +20,14 @@ describe Repository do
     rugged.references.create("refs/remotes/#{remote_name}/#{branch_name}", target.id)
   end
 
+  describe 'delegated methods' do
+    subject { repository }
+
+    it { is_expected.to delegate_method(:checksum).to(:raw_repository) }
+    it { is_expected.to delegate_method(:find_remote_root_ref).to(:raw_repository) }
+    it { is_expected.to delegate_method(:pull_mirror_branch_prefix).to(:project) }
+  end
+
   describe '#after_sync' do
     it 'expires repository cache' do
       expect(repository).to receive(:expire_all_method_caches)
@@ -208,4 +216,61 @@ describe Repository do
       expect(project.repository.insights_config_for(project.repository.root_ref)).to eq("monthlyBugsCreated:\n  title: My chart")
     end
   end
+
+  describe '#upstream_branch_name' do
+    let(:pull_mirror_branch_prefix) { 'upstream/' }
+    let(:branch_name) { 'upstream/master' }
+
+    subject { repository.upstream_branch_name(branch_name) }
+
+    before do
+      project.update(pull_mirror_branch_prefix: pull_mirror_branch_prefix)
+    end
+
+    it { is_expected.to eq('master') }
+
+    context 'when the branch is local (not mirrored)' do
+      let(:branch_name) { 'a-local-branch' }
+
+      it { is_expected.to be_nil }
+    end
+
+    context 'when pull_mirror_branch_prefix is nil' do
+      let(:pull_mirror_branch_prefix) { nil }
+
+      it { is_expected.to eq(branch_name) }
+    end
+
+    context 'when pull_mirror_branch_prefix is empty' do
+      let(:pull_mirror_branch_prefix) { '' }
+
+      it { is_expected.to eq(branch_name) }
+    end
+
+    context 'when pull_mirror_branch_prefix feature flag is disabled' do
+      before do
+        stub_feature_flags(pull_mirror_branch_prefix: false)
+      end
+
+      it { is_expected.to eq(branch_name) }
+
+      context 'when the branch is local (not mirrored)' do
+        let(:branch_name) { 'a-local-branch' }
+
+        it { is_expected.to eq(branch_name) }
+      end
+
+      context 'when pull_mirror_branch_prefix is nil' do
+        let(:pull_mirror_branch_prefix) { nil }
+
+        it { is_expected.to eq(branch_name) }
+      end
+
+      context 'when pull_mirror_branch_prefix is empty' do
+        let(:pull_mirror_branch_prefix) { '' }
+
+        it { is_expected.to eq(branch_name) }
+      end
+    end
+  end
 end

ee/spec/services/projects/update_mirror_service_spec.rb

@@ -139,42 +139,31 @@ describe Projects::UpdateMirrorService do
       end
     end
 
-    context "updating branches" do
-      context 'when pull_mirror_branch_prefix is set' do
-        let(:pull_mirror_branch_prefix) { 'upstream/' }
+    context 'updating branches' do
+      shared_examples 'a working pull mirror' do |branch_prefix|
+        context 'when the mirror has a repository' do
+          let(:master) { "#{branch_prefix}master"}
 
           before do
-          project.update(pull_mirror_branch_prefix: pull_mirror_branch_prefix)
-        end
-
-        it "creates new branches" do
             stub_fetch_mirror(project)
+          end
 
+          it 'creates new branches' do
             service.execute
 
-          expect(project.repository.branch_names).to include("#{pull_mirror_branch_prefix}new-branch")
-          expect(project.repository.branch_names).not_to include('new-branch')
-        end
-
-        context 'when pull_mirror_branch_prefix feature flag is disabled' do
-          before do
-            stub_feature_flags(pull_mirror_branch_prefix: false)
+            expect(project.repository.branch_names).to include("#{branch_prefix}new-branch")
           end
 
-          it "creates new branches" do
-            stub_fetch_mirror(project)
-
+          it 'updates existing branches' do
             service.execute
 
-            expect(project.repository.branch_names).not_to include("#{pull_mirror_branch_prefix}new-branch")
-            expect(project.repository.branch_names).to include('new-branch')
-          end
-        end
+            expect(project.repository.find_branch("#{branch_prefix}existing-branch").dereferenced_target)
+              .to eq(project.repository.find_branch(master).dereferenced_target)
           end
 
           context 'when mirror only protected branches option is set' do
-        let(:new_protected_branch_name) { 'new-branch' }
-        let(:protected_branch_name) { 'existing-branch' }
+            let(:new_protected_branch_name) { "#{branch_prefix}new-branch" }
+            let(:protected_branch_name) { "#{branch_prefix}existing-branch" }
 
             before do
               project.update(only_mirror_protected_branches: true)
@@ -184,16 +173,12 @@ describe Projects::UpdateMirrorService do
               create(:protected_branch, project: project, name: new_protected_branch_name)
               project.reload
 
-          stub_fetch_mirror(project)
-
               service.execute
 
               expect(project.repository.branch_names).to include(new_protected_branch_name)
             end
 
             it 'does not create an unprotected branch' do
-          stub_fetch_mirror(project)
-
               service.execute
 
               expect(project.repository.branch_names).not_to include(new_protected_branch_name)
@@ -203,45 +188,22 @@ describe Projects::UpdateMirrorService do
               create(:protected_branch, project: project, name: protected_branch_name)
               project.reload
 
-          stub_fetch_mirror(project)
-
               service.execute
 
               expect(project.repository.find_branch(protected_branch_name).dereferenced_target)
-            .to eq(project.repository.find_branch('master').dereferenced_target)
+                .to eq(project.repository.find_branch(master).dereferenced_target)
             end
 
-        it "does not update unprotected branches" do
-          stub_fetch_mirror(project)
-
+            it 'does not update unprotected branches' do
               service.execute
 
               expect(project.repository.find_branch(protected_branch_name).dereferenced_target)
-            .not_to eq(project.repository.find_branch('master').dereferenced_target)
+                .not_to eq(project.repository.find_branch(master).dereferenced_target)
             end
           end
 
-      it "creates new branches" do
-        stub_fetch_mirror(project)
-
-        service.execute
-
-        expect(project.repository.branch_names).to include('new-branch')
-      end
-
-      it "updates existing branches" do
-        stub_fetch_mirror(project)
-
-        service.execute
-
-        expect(project.repository.find_branch('existing-branch').dereferenced_target)
-          .to eq(project.repository.find_branch('master').dereferenced_target)
-      end
-
           context 'with diverged branches' do
-        before do
-          stub_fetch_mirror(project)
-        end
+            let(:diverged_branch) { "#{branch_prefix}markdown"}
 
             context 'when mirror_overwrites_diverged_branches is true' do
               it 'update diverged branches' do
@@ -249,8 +211,8 @@ describe Projects::UpdateMirrorService do
 
                 service.execute
 
-            expect(project.repository.find_branch('markdown').dereferenced_target)
-                .to eq(project.repository.find_branch('master').dereferenced_target)
+                expect(project.repository.find_branch(diverged_branch).dereferenced_target)
+                  .to eq(project.repository.find_branch(master).dereferenced_target)
               end
             end
 
@@ -260,8 +222,8 @@ describe Projects::UpdateMirrorService do
 
                 service.execute
 
-            expect(project.repository.find_branch('markdown').dereferenced_target)
-                .not_to eq(project.repository.find_branch('master').dereferenced_target)
+                expect(project.repository.find_branch(diverged_branch).dereferenced_target)
+                  .not_to eq(project.repository.find_branch(master).dereferenced_target)
               end
             end
 
@@ -271,8 +233,9 @@ describe Projects::UpdateMirrorService do
 
                 service.execute
 
-            expect(project.repository.find_branch('markdown').dereferenced_target)
-                .not_to eq(project.repository.find_branch('master').dereferenced_target)
+                expect(project.repository.find_branch(diverged_branch).dereferenced_target)
+                  .not_to eq(project.repository.find_branch(master).dereferenced_target)
+              end
             end
           end
         end
@@ -290,6 +253,43 @@ describe Projects::UpdateMirrorService do
             expect(repository.branch_names).not_to include('master')
           end
         end
+      end
+
+      context 'when pull_mirror_branch_prefix is set' do
+        let(:pull_mirror_branch_prefix) { 'upstream/' }
+
+        before do
+          project.update(pull_mirror_branch_prefix: pull_mirror_branch_prefix)
+        end
+
+        it "doesn't create unprefixed branches" do
+          stub_fetch_mirror(project)
+
+          service.execute
+
+          expect(project.repository.branch_names).not_to include('new-branch')
+        end
+
+        it_behaves_like 'a working pull mirror', 'upstream/'
+
+        context 'when pull_mirror_branch_prefix feature flag is disabled' do
+          before do
+            stub_feature_flags(pull_mirror_branch_prefix: false)
+          end
+
+          it_behaves_like 'a working pull mirror'
+
+          it "doesn't create prefixed branches" do
+            stub_fetch_mirror(project)
+
+            service.execute
+
+            expect(project.repository.branch_names).not_to include("#{pull_mirror_branch_prefix}new-branch")
+          end
+        end
+      end
+
+      it_behaves_like 'a working pull mirror'
 
       def create_file(repository)
         repository.create_file(
@@ -385,16 +385,42 @@ describe Projects::UpdateMirrorService do
     end
   end
 
+  def rewrite_refs_as_pull_mirror(project)
+    return unless project.pull_mirror_branch_prefix
+    return unless Feature.enabled?(:pull_mirror_branch_prefix)
+
+    repository = project.repository
+    old_branches = repository.branches.each_with_object({}) do |branch, branches|
+      branches[branch.name] = branch.dereferenced_target.id
+    end
+
+    rugged = rugged_repo(repository)
+    old_branches.each do |name, target|
+      mirrored_branch_ref = "refs/heads/#{project.pull_mirror_branch_prefix}#{name}"
+      rugged.references.create(mirrored_branch_ref, target)
+      rugged.head = mirrored_branch_ref if name == 'master'
+      rugged.branches.delete(name)
+    end
+
+    repository.expire_branches_cache
+    repository.branches
+  end
+
   def stub_fetch_mirror(project, repository: project.repository)
-    allow(project).to receive(:fetch_mirror) { fetch_mirror(repository) }
+    branch_prefix = project.pull_mirror_branch_prefix
+    branch_prefix = '' unless Feature.enabled?(:pull_mirror_branch_prefix)
+
+    rewrite_refs_as_pull_mirror(project)
+
+    allow(project).to receive(:fetch_mirror) { fetch_mirror(repository, branch_prefix: branch_prefix) }
   end
 
-  def fetch_mirror(repository)
+  def fetch_mirror(repository, branch_prefix: '')
     rugged = rugged_repo(repository)
-    masterrev = repository.find_branch('master').dereferenced_target.id
+    masterrev = repository.find_branch("#{branch_prefix}master").dereferenced_target.id
 
     parentrev = repository.commit(masterrev).parent_id
-    rugged.references.create('refs/heads/existing-branch', parentrev)
+    rugged.references.create("refs/heads/#{branch_prefix}existing-branch", parentrev)
 
     repository.expire_branches_cache
     repository.branches