Skip to content

Improve performance of searching branches by name by using a memoized branches array

username-removed-590447 requested to merge fbernier/gitlab-ce:18988-memoize-branches-on-find-by-name into master

What does this MR do?

!4859 (closed) memoized the find_tags method to improve performance. It was discussed in this MR that maybe the find_branches should also be memoized, and then #18988 (moved) was opened for this.

Are there points in the code the reviewer needs to double check?

Simply changing the find_branches method to used the memoized array caused some specs to fail.

 1) Projects::CommitController POST revert when the revert failed should redirect to the commit page
     Failure/Error: expect(response).to redirect_to namespace_project_commit_path(project.namespace, project, commit.id)

       Expected response to be a redirect to <http://test.host/namespace21/gitlabhq/commit/5937ac0a7beb003549fc5fd26fc247adbce4a52e> but was a redirect to <ht
tp://test.host/namespace21/gitlabhq/commits/master>.
       Expected "http://test.host/namespace21/gitlabhq/commit/5937ac0a7beb003549fc5fd26fc247adbce4a52e" to be === "http://test.host/namespace21/gitlabhq/commi
ts/master".
     # ./spec/controllers/projects/commit_controller_spec.rb:191:in `block (4 levels) in <top (required)>'

I tracked it down to the check_revert_content and check_cherry_pick_content methods where I did not have the time to find why it should not be memoized there but simply enabled find_branches to use a non-memoized version.

I haven't used GitLab much and just took an hour to set my environment and take a stab at this issue so sorry for my lack of knowledge about the process and the internals.

Why was this MR needed?

Because it's probably good to keep some consistency with find_tags if it makes sense to keep doing so. Also, performance.

What are the relevant issue numbers?

#18988 (moved)

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

Merge request reports