Commit b2de3273 authored by Sean McGivern's avatar Sean McGivern

Optimise ExtractsRef when there is no slash

ExtractsRef attempts to take a string containing slashes and figure out
which part's a reference to a tree-ish object, and which part's a path
in that tree-ish. For instance, `foo/bar/baz` might mean:

1. A ref of `foo`, a path of `bar/baz`
2. A ref of `foo/bar`, a path of `baz`
3. A ref of `foo/bar/baz`, and no path (the root)

To do this, it needs to load the full list of references (branches and
tags) for the repository, which can be slow and put pressure on Redis as
it uses SMEMBERS, which is an O(n) operation.

When there is no slash in the string, there is no ambiguity: it must be
a reference and no path (the third case above). In that case we can skip
loading the references entirely and just detect that by looking at the
string in question.
parent 19fe2d0e
---
title: Remove some unnecessary Redis calls on commit lists
merge_request: 38343
author:
type: performance
...@@ -47,6 +47,10 @@ module ExtractsRef ...@@ -47,6 +47,10 @@ module ExtractsRef
if id =~ /^(\h{40})(.+)/ if id =~ /^(\h{40})(.+)/
# If the ref appears to be a SHA, we're done, just split the string # If the ref appears to be a SHA, we're done, just split the string
pair = $~.captures pair = $~.captures
elsif id.exclude?('/')
# If the ID contains no slash, we must have a ref and no path, so
# we can skip the Redis calls below
pair = [id, '']
else else
# Otherwise, attempt to detect the ref using a list of the repository_container's # Otherwise, attempt to detect the ref using a list of the repository_container's
# branches and tags # branches and tags
...@@ -71,12 +75,10 @@ module ExtractsRef ...@@ -71,12 +75,10 @@ module ExtractsRef
end end
end end
pair[0] = pair[0].strip [
pair[0].strip,
# Remove ending slashes from path pair[1].gsub(%r{^/|/$}, '') # Remove leading and trailing slashes from path
pair[1].gsub!(%r{^/|/$}, '') ]
pair
end end
# Assigns common instance variables for views working with Git tree-ish objects # Assigns common instance variables for views working with Git tree-ish objects
......
...@@ -88,9 +88,16 @@ RSpec.shared_examples 'extracts refs' do ...@@ -88,9 +88,16 @@ RSpec.shared_examples 'extracts refs' do
expect(extract_ref('stable')).to eq(['stable', '']) expect(extract_ref('stable')).to eq(['stable', ''])
end end
it 'extracts the longest matching ref' do it 'does not fetch ref names when there is no slash' do
expect(extract_ref('release/app/v1.0.0/README.md')).to eq( expect(self).not_to receive(:ref_names)
['release/app/v1.0.0', 'README.md'])
extract_ref('master')
end
it 'fetches ref names when there is a slash' do
expect(self).to receive(:ref_names).and_call_original
extract_ref('release/app/v1.0.0')
end end
end end
...@@ -113,6 +120,11 @@ RSpec.shared_examples 'extracts refs' do ...@@ -113,6 +120,11 @@ RSpec.shared_examples 'extracts refs' do
it 'falls back to a primitive split for an invalid ref' do it 'falls back to a primitive split for an invalid ref' do
expect(extract_ref('stable/CHANGELOG')).to eq(%w(stable CHANGELOG)) expect(extract_ref('stable/CHANGELOG')).to eq(%w(stable CHANGELOG))
end end
it 'extracts the longest matching ref' do
expect(extract_ref('release/app/v1.0.0/README.md')).to eq(
['release/app/v1.0.0', 'README.md'])
end
end end
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment