Commit 8969a823 authored by Stan Hu's avatar Stan Hu

Merge branch 'fix-multiple-ref-prefix' into 'master'

Extract the longest-matching ref from a commit path when multiple matches occur

### What does this MR do?

This MR extracts the longest-matching ref from a commit path. In cases when there are multiple refs that prefix the path, the ref name is ambiguous. Using the heuristic that the longest-matching ref seems like a sensible default.

### Why was this MR needed?

Suppose there is a branch named `release/app` and a tag named `release/app/v1.0.0`. Suppose `README.md` exists in the root directory.

Let's suppose the path passed in is `release/app/v1.0.0/README.md`. There are two possible ways to interpret the ref and path:

1. ref = `release/app`, path = `v1.0.0/README.md`
2. ref = `release/app/v1.0.0`, path = `README.md`

The crux of the issue is that there is ambiguity which one is correct; both could be real possibilities. In the current implementation of `extract_ref`, GitLab gets confused and tries neither: it uses ref = `release` and path = `app/v1.0.0/README.md`. Since the file does not exist, it returns 404.

### What are the relevant issue numbers?

Closes #1839

See merge request !859
parents 3603edcf 9add3e6e
...@@ -10,6 +10,7 @@ v 7.13.0 (unreleased) ...@@ -10,6 +10,7 @@ v 7.13.0 (unreleased)
- Fix downloading of patches on public merge requests when user logged out (Stan Hu) - Fix downloading of patches on public merge requests when user logged out (Stan Hu)
- The password for the default administrator (root) account has been changed from "5iveL!fe" to "password". - The password for the default administrator (root) account has been changed from "5iveL!fe" to "password".
- Fix Error 500 when relative submodule resolves to a namespace that has a different name from its path (Stan Hu) - Fix Error 500 when relative submodule resolves to a namespace that has a different name from its path (Stan Hu)
- Extract the longest-matching ref from a commit path when multiple matches occur (Stan Hu)
- Update maintenance documentation to explain no need to recompile asssets for omnibus installations (Stan Hu) - Update maintenance documentation to explain no need to recompile asssets for omnibus installations (Stan Hu)
- Support commenting on diffs in side-by-side mode (Stan Hu) - Support commenting on diffs in side-by-side mode (Stan Hu)
- Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu) - Fix JavaScript error when clicking on the comment button on a diff line that has a comment already (Stan Hu)
......
...@@ -55,12 +55,16 @@ module ExtractsPath ...@@ -55,12 +55,16 @@ module ExtractsPath
valid_refs = @project.repository.ref_names valid_refs = @project.repository.ref_names
valid_refs.select! { |v| id.start_with?("#{v}/") } valid_refs.select! { |v| id.start_with?("#{v}/") }
if valid_refs.length != 1 if valid_refs.length == 0
# No exact ref match, so just try our best # No exact ref match, so just try our best
pair = id.match(/([^\/]+)(.*)/).captures pair = id.match(/([^\/]+)(.*)/).captures
else else
# There is a distinct possibility that multiple refs prefix the ID.
# Use the longest match to maximize the chance that we have the
# right ref.
best_match = valid_refs.max_by(&:length)
# Partition the string into the ref and the path, ignoring the empty first value # Partition the string into the ref and the path, ignoring the empty first value
pair = id.partition(valid_refs.first)[1..-1] pair = id.partition(best_match)[1..-1]
end end
end end
......
...@@ -10,7 +10,8 @@ describe ExtractsPath do ...@@ -10,7 +10,8 @@ describe ExtractsPath do
before do before do
@project = project @project = project
repo = double(ref_names: ['master', 'foo/bar/baz', 'v1.0.0', 'v2.0.0']) repo = double(ref_names: ['master', 'foo/bar/baz', 'v1.0.0', 'v2.0.0',
'release/app', 'release/app/v1.0.0'])
allow(project).to receive(:repository).and_return(repo) allow(project).to receive(:repository).and_return(repo)
allow(project).to receive(:path_with_namespace). allow(project).to receive(:path_with_namespace).
and_return('gitlab/gitlab-ci') and_return('gitlab/gitlab-ci')
...@@ -54,11 +55,17 @@ describe ExtractsPath do ...@@ -54,11 +55,17 @@ describe ExtractsPath 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')).to eq(['stable', '']) expect(extract_ref('stable')).to eq(['stable', ''])
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
context "with a path" do context "with a path" do
it "extracts a valid branch" do it "extracts a valid branch" do
expect(extract_ref('foo/bar/baz/CHANGELOG')).to eq(['foo/bar/baz', 'CHANGELOG']) expect(extract_ref('foo/bar/baz/CHANGELOG')).to eq(
['foo/bar/baz', 'CHANGELOG'])
end end
it "extracts a valid tag" do it "extracts a valid tag" do
......
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