Follow-up from "fix: Include default_branch in project export/import"

Everyone can contribute. Help move this issue forward while earning points, leveling up and collecting rewards.

• Work on this issue

The following discussions from !214849 should be addressed:

• @jnutt started a discussion:

  suggestion (non-blocking): Is it better to use an instance variable here, or to define attr_accessor :desired_default_branch in the same file? This seems to achieve the same thing in a way that doesn't displease Rubocop, but it seems like a roundabout way of doing the same thing.

  It would make the tests a little easier to follow, since IMO instance_variable_get/instance_variable_set feel like a bit of an escape hatch.

• @jnutt started a discussion: (+1 comment)

  question: What do you mean by "required" in this context? That we were required to provide a reason, or that there's no effective way to do this without disabling the cop? I think we ought to expand on this.

• @jnutt started a discussion: (+4 comments)

  question: I was wondering if we could write these specs by actually updating the underlying repo, rather than stubbing, and I'm struggling.

  diff --git a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb
  index c94118af3e16..13aaf2d565de 100644
  --- a/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb
  +++ b/spec/support/shared_examples/models/concerns/has_repository_shared_examples.rb
  @@ -253,14 +253,13 @@
   
           context 'when desired branch exists' do
             before do
  -            allow(repository).to receive(:root_ref).and_return('main')
  -            allow(repository).to receive(:branch_exists?).with(new_branch).and_return(true)
  +            repository.create_branch(new_branch, repository.root_ref)
             end
   
             it 'changes HEAD to the desired branch' do
  -            expect(repository).to receive(:change_head).with(new_branch)
  -
  -            container.apply_desired_default_branch
  +            expect { container.apply_desired_default_branch }
  +              .to change { container.reload_default_branch }
  +              .from('master').to(new_branch)
             end
   
             it 'reloads the default branch' do
  

  This change causes the spec to fail, but I would have expected it to pass. Manually testing your change, it looks good, but this is making me worry that I don't understand the underlying mechanism well enough to approve this. Could you point me towards what I might be missing? 🙏

• @SamWord started a discussion:

  Suggestion (non-blocking): I think you could get away without using an instance variable at all here and just pass it to apply_desired_default_branch as an argument. apply_desired_default_branch is only called when setting branch_name on a model with HasRespository, and it's just whatever branch_name is anyway, so I don't think it'd result in any performance gains.