diff --git a/app/assets/javascripts/diffs/components/diff_file_header.vue b/app/assets/javascripts/diffs/components/diff_file_header.vue
index e3e140ea35ec0f209ff42bd3380a68db892a689a..fded391cc849b799a6aff7065ef610bab165d3f8 100644
--- a/app/assets/javascripts/diffs/components/diff_file_header.vue
+++ b/app/assets/javascripts/diffs/components/diff_file_header.vue
@@ -2,7 +2,14 @@
/* eslint-disable vue/no-v-html */
import { escape } from 'lodash';
import { mapActions, mapGetters } from 'vuex';
-import { GlDeprecatedButton, GlTooltipDirective, GlLoadingIcon, GlIcon } from '@gitlab/ui';
+import {
+ GlDeprecatedButton,
+ GlTooltipDirective,
+ GlSafeHtmlDirective,
+ GlLoadingIcon,
+ GlIcon,
+ GlButton,
+} from '@gitlab/ui';
import ClipboardButton from '~/vue_shared/components/clipboard_button.vue';
import FileIcon from '~/vue_shared/components/file_icon.vue';
import { truncateSha } from '~/lib/utils/text_utility';
@@ -21,9 +28,11 @@ export default {
GlIcon,
FileIcon,
DiffStats,
+ GlButton,
},
directives: {
GlTooltip: GlTooltipDirective,
+ SafeHtml: GlSafeHtmlDirective,
},
props: {
discussionPath: {
@@ -77,6 +86,21 @@ export default {
return this.discussionPath;
},
+ submoduleDiffCompareLinkText() {
+ if (this.diffFile.submodule_compare) {
+ const truncatedOldSha = escape(truncateSha(this.diffFile.submodule_compare.old_sha));
+ const truncatedNewSha = escape(truncateSha(this.diffFile.submodule_compare.new_sha));
+ return sprintf(
+ s__('Compare %{oldCommitId}...%{newCommitId}'),
+ {
+ oldCommitId: `<span class="commit-sha">${truncatedOldSha}</span>`,
+ newCommitId: `<span class="commit-sha">${truncatedNewSha}</span>`,
+ },
+ false,
+ );
+ }
+ return null;
+ },
filePath() {
if (this.diffFile.submodule) {
return `${this.diffFile.file_path} @ ${truncateSha(this.diffFile.blob.id)}`;
@@ -311,5 +335,18 @@ export default {
</a>
</div>
</div>
+
+ <div
+ v-if="diffFile.submodule_compare"
+ class="file-actions d-none d-sm-flex align-items-center flex-wrap"
+ >
+ <gl-button
+ v-gl-tooltip.hover
+ v-safe-html="submoduleDiffCompareLinkText"
+ class="submodule-compare"
+ :title="s__('Compare submodule commit revisions')"
+ :href="diffFile.submodule_compare.url"
+ />
+ </div>
</div>
</template>
diff --git a/app/helpers/diff_helper.rb b/app/helpers/diff_helper.rb
index 3b25de521d023a170acf2ffc331c8207bda63541..7c254e069f67f38b26fc1a5f249ee5da78c07724 100644
--- a/app/helpers/diff_helper.rb
+++ b/app/helpers/diff_helper.rb
@@ -100,20 +100,43 @@ def parallel_diff_btn
end
def submodule_link(blob, ref, repository = @repository)
- project_url, tree_url = submodule_links(blob, ref, repository)
- commit_id = if tree_url.nil?
- Commit.truncate_sha(blob.id)
- else
- link_to Commit.truncate_sha(blob.id), tree_url
- end
+ urls = submodule_links(blob, ref, repository)
+
+ folder_name = truncate(blob.name, length: 40)
+ folder_name = link_to(folder_name, urls.web) if urls&.web
+
+ commit_id = Commit.truncate_sha(blob.id)
+ commit_id = link_to(commit_id, urls.tree) if urls&.tree
[
- content_tag(:span, link_to(truncate(blob.name, length: 40), project_url)),
+ content_tag(:span, folder_name),
'@',
content_tag(:span, commit_id, class: 'commit-sha')
].join(' ').html_safe
end
+ def submodule_diff_compare_link(diff_file)
+ compare_url = submodule_links(diff_file.blob, diff_file.content_sha, diff_file.repository, diff_file)&.compare
+
+ link = ""
+
+ if compare_url
+
+ link_text = [
+ _('Compare'),
+ ' ',
+ content_tag(:span, Commit.truncate_sha(diff_file.old_blob.id), class: 'commit-sha'),
+ '...',
+ content_tag(:span, Commit.truncate_sha(diff_file.blob.id), class: 'commit-sha')
+ ].join('').html_safe
+
+ tooltip = _('Compare submodule commit revisions')
+ link = content_tag(:span, link_to(link_text, compare_url, class: 'btn has-tooltip', title: tooltip), class: 'submodule-compare')
+ end
+
+ link
+ end
+
def diff_file_blob_raw_url(diff_file, only_path: false)
project_raw_url(@project, tree_join(diff_file.content_sha, diff_file.file_path), only_path: only_path)
end
diff --git a/app/helpers/submodule_helper.rb b/app/helpers/submodule_helper.rb
index 2e7146eaa43c871f7251208cb54951c4eecb7665..959178c47d7c8050fcc062f03378c0e17a284076 100644
--- a/app/helpers/submodule_helper.rb
+++ b/app/helpers/submodule_helper.rb
@@ -6,12 +6,12 @@ module SubmoduleHelper
VALID_SUBMODULE_PROTOCOLS = %w[http https git ssh].freeze
# links to files listing for submodule if submodule is a project on this server
- def submodule_links(submodule_item, ref = nil, repository = @repository)
- repository.submodule_links.for(submodule_item, ref)
+ def submodule_links(submodule_item, ref = nil, repository = @repository, diff_file = nil)
+ repository.submodule_links.for(submodule_item, ref, diff_file)
end
- def submodule_links_for_url(submodule_item_id, url, repository)
- return [nil, nil] unless url
+ def submodule_links_for_url(submodule_item_id, url, repository, old_submodule_item_id = nil)
+ return [nil, nil, nil] unless url
if url == '.' || url == './'
url = File.join(Gitlab.config.gitlab.url, repository.project.full_path)
@@ -34,21 +34,24 @@ def submodule_links_for_url(submodule_item_id, url, repository)
project.sub!(/\.git\z/, '')
if self_url?(url, namespace, project)
- [url_helpers.namespace_project_path(namespace, project),
- url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id)]
+ [
+ url_helpers.namespace_project_path(namespace, project),
+ url_helpers.namespace_project_tree_path(namespace, project, submodule_item_id),
+ (url_helpers.namespace_project_compare_path(namespace, project, to: submodule_item_id, from: old_submodule_item_id) if old_submodule_item_id)
+ ]
elsif relative_self_url?(url)
- relative_self_links(url, submodule_item_id, repository.project)
+ relative_self_links(url, submodule_item_id, old_submodule_item_id, repository.project)
elsif gist_github_dot_com_url?(url)
gist_github_com_tree_links(namespace, project, submodule_item_id)
elsif github_dot_com_url?(url)
- github_com_tree_links(namespace, project, submodule_item_id)
+ github_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id)
elsif gitlab_dot_com_url?(url)
- gitlab_com_tree_links(namespace, project, submodule_item_id)
+ gitlab_com_tree_links(namespace, project, submodule_item_id, old_submodule_item_id)
else
- [sanitize_submodule_url(url), nil]
+ [sanitize_submodule_url(url), nil, nil]
end
else
- [sanitize_submodule_url(url), nil]
+ [sanitize_submodule_url(url), nil, nil]
end
end
@@ -79,22 +82,30 @@ def relative_self_url?(url)
url.start_with?('../', './')
end
- def gitlab_com_tree_links(namespace, project, commit)
+ def gitlab_com_tree_links(namespace, project, commit, old_commit)
base = ['https://gitlab.com/', namespace, '/', project].join('')
- [base, [base, '/-/tree/', commit].join('')]
+ [
+ base,
+ [base, '/-/tree/', commit].join(''),
+ ([base, '/-/compare/', old_commit, '...', commit].join('') if old_commit)
+ ]
end
def gist_github_com_tree_links(namespace, project, commit)
base = ['https://gist.github.com/', namespace, '/', project].join('')
- [base, [base, commit].join('/')]
+ [base, [base, commit].join('/'), nil]
end
- def github_com_tree_links(namespace, project, commit)
+ def github_com_tree_links(namespace, project, commit, old_commit)
base = ['https://github.com/', namespace, '/', project].join('')
- [base, [base, '/tree/', commit].join('')]
+ [
+ base,
+ [base, '/tree/', commit].join(''),
+ ([base, '/compare/', old_commit, '...', commit].join('') if old_commit)
+ ]
end
- def relative_self_links(relative_path, commit, project)
+ def relative_self_links(relative_path, commit, old_commit, project)
relative_path = relative_path.rstrip
absolute_project_path = "/" + project.full_path
@@ -107,7 +118,7 @@ def relative_self_links(relative_path, commit, project)
target_namespace_path = File.dirname(submodule_project_path)
if target_namespace_path == '/' || target_namespace_path.start_with?(absolute_project_path)
- return [nil, nil]
+ return [nil, nil, nil]
end
target_namespace_path.sub!(%r{^/}, '')
@@ -116,10 +127,11 @@ def relative_self_links(relative_path, commit, project)
begin
[
url_helpers.namespace_project_path(target_namespace_path, submodule_base),
- url_helpers.namespace_project_tree_path(target_namespace_path, submodule_base, commit)
+ url_helpers.namespace_project_tree_path(target_namespace_path, submodule_base, commit),
+ (url_helpers.namespace_project_compare_path(target_namespace_path, submodule_base, to: commit, from: old_commit) if old_commit)
]
rescue ActionController::UrlGenerationError
- [nil, nil]
+ [nil, nil, nil]
end
end
diff --git a/app/serializers/diff_file_base_entity.rb b/app/serializers/diff_file_base_entity.rb
index 2af14f1eb82abf4ddc668b0e855e1a3c01491c5b..9f27191c3c8bca4353c379439ec36b3da19c4229 100644
--- a/app/serializers/diff_file_base_entity.rb
+++ b/app/serializers/diff_file_base_entity.rb
@@ -12,11 +12,23 @@ class DiffFileBaseEntity < Grape::Entity
expose :submodule?, as: :submodule
expose :submodule_link do |diff_file, options|
- memoized_submodule_links(diff_file, options).first
+ memoized_submodule_links(diff_file, options)&.web
end
expose :submodule_tree_url do |diff_file|
- memoized_submodule_links(diff_file, options).last
+ memoized_submodule_links(diff_file, options)&.tree
+ end
+
+ expose :submodule_compare do |diff_file|
+ url = memoized_submodule_links(diff_file, options)&.compare
+
+ next unless url
+
+ {
+ url: url,
+ old_sha: diff_file.old_blob&.id,
+ new_sha: diff_file.blob&.id
+ }
end
expose :edit_path, if: -> (_, options) { options[:merge_request] } do |diff_file|
@@ -96,11 +108,9 @@ class DiffFileBaseEntity < Grape::Entity
def memoized_submodule_links(diff_file, options)
strong_memoize(:submodule_links) do
- if diff_file.submodule?
- options[:submodule_links].for(diff_file.blob, diff_file.content_sha)
- else
- []
- end
+ next unless diff_file.submodule?
+
+ options[:submodule_links].for(diff_file.blob, diff_file.content_sha, diff_file)
end
end
diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml
index bd023e0442c13224f8ad4c4e746fed89e80d3824..187ebcb739cf5593c10d42364f3ed0e1dcc9a1af 100644
--- a/app/views/projects/diffs/_file.html.haml
+++ b/app/views/projects/diffs/_file.html.haml
@@ -9,6 +9,10 @@
.file-header-content
= render "projects/diffs/file_header", diff_file: diff_file, url: "##{file_hash}"
+ - if diff_file.submodule?
+ .file-actions.d-none.d-sm-block
+ = submodule_diff_compare_link(diff_file)
+
- unless diff_file.submodule?
- blob = diff_file.blob
.file-actions.d-none.d-sm-block
diff --git a/changelogs/unreleased/compare-link-git-submodule-update.yml b/changelogs/unreleased/compare-link-git-submodule-update.yml
new file mode 100644
index 0000000000000000000000000000000000000000..8632c095c5fe8cf101e66a57a9e6b105d45f22e5
--- /dev/null
+++ b/changelogs/unreleased/compare-link-git-submodule-update.yml
@@ -0,0 +1,5 @@
+---
+title: Add link to compare changes intoduced by a git submodule update
+merge_request: 37740
+author: Daniel Seemer @Phaiax
+type: added
diff --git a/lib/gitlab/graphql/representation/submodule_tree_entry.rb b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
index 8d17cb9eecc4c4122915928dae97fc03cd6a3d97..aa5e74cc837c9ce90028bebf1dc0074865a63dc5 100644
--- a/lib/gitlab/graphql/representation/submodule_tree_entry.rb
+++ b/lib/gitlab/graphql/representation/submodule_tree_entry.rb
@@ -24,11 +24,11 @@ def initialize(submodule, submodule_links)
end
def web_url
- @submodule_links.first
+ @submodule_links&.web
end
def tree_url
- @submodule_links.last
+ @submodule_links&.tree
end
end
end
diff --git a/lib/gitlab/submodule_links.rb b/lib/gitlab/submodule_links.rb
index b0ee0877f304a6f2b7f81e9dc339fadc2b69cb7e..38b10c5892da635ff5774b501ab58a7a6fa87a4b 100644
--- a/lib/gitlab/submodule_links.rb
+++ b/lib/gitlab/submodule_links.rb
@@ -4,14 +4,18 @@ module Gitlab
class SubmoduleLinks
include Gitlab::Utils::StrongMemoize
+ Urls = Struct.new(:web, :tree, :compare)
+
def initialize(repository)
@repository = repository
@cache_store = {}
end
- def for(submodule, sha)
+ def for(submodule, sha, diff_file = nil)
submodule_url = submodule_url_for(sha, submodule.path)
- SubmoduleHelper.submodule_links_for_url(submodule.id, submodule_url, repository)
+ old_submodule_id = old_submodule_id(submodule_url, diff_file)
+ urls = SubmoduleHelper.submodule_links_for_url(submodule.id, submodule_url, repository, old_submodule_id)
+ Urls.new(*urls) if urls.any?
end
private
@@ -29,5 +33,15 @@ def submodule_url_for(sha, path)
urls = submodule_urls_for(sha)
urls && urls[path]
end
+
+ def old_submodule_id(submodule_url, diff_file)
+ return unless diff_file&.old_blob && diff_file&.old_content_sha
+
+ # if the submodule url has changed from old_sha to sha, a compare link does not make sense
+ #
+ old_submodule_url = submodule_url_for(diff_file.old_content_sha, diff_file.old_blob.path)
+
+ diff_file.old_blob.id if old_submodule_url == submodule_url
+ end
end
end
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 25268ad1792870c6c1494d858973ccc72e34b8ef..b805f1426328c4f62ace25e5440245ac055eaa3f 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6346,6 +6346,9 @@ msgstr ""
msgid "Compare"
msgstr ""
+msgid "Compare %{oldCommitId}...%{newCommitId}"
+msgstr ""
+
msgid "Compare Git revisions"
msgstr ""
@@ -6361,6 +6364,9 @@ msgstr ""
msgid "Compare changes with the merge request target branch"
msgstr ""
+msgid "Compare submodule commit revisions"
+msgstr ""
+
msgid "Compare with previous version"
msgstr ""
diff --git a/spec/helpers/submodule_helper_spec.rb b/spec/helpers/submodule_helper_spec.rb
index 426bca2ced2d03995ac922f291db094003891b5d..a419b6b9c8422275b271d2a67aea6b39dc9c89a9 100644
--- a/spec/helpers/submodule_helper_spec.rb
+++ b/spec/helpers/submodule_helper_spec.rb
@@ -24,7 +24,11 @@
allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(22) # set this just to be sure
allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
stub_url([config.ssh_user, '@', config.host, ':gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'detects ssh on standard port without a username' do
@@ -32,14 +36,22 @@
allow(Gitlab.config.gitlab_shell).to receive(:ssh_user).and_return('')
allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
stub_url([config.host, ':gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'detects ssh on non-standard port' do
allow(Gitlab.config.gitlab_shell).to receive(:ssh_port).and_return(2222)
allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
stub_url(['ssh://', config.ssh_user, '@', config.host, ':2222/gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'detects ssh on non-standard port without a username' do
@@ -47,21 +59,33 @@
allow(Gitlab.config.gitlab_shell).to receive(:ssh_user).and_return('')
allow(Gitlab.config.gitlab_shell).to receive(:ssh_path_prefix).and_return(Settings.send(:build_gitlab_shell_ssh_path_prefix))
stub_url(['ssh://', config.host, ':2222/gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'detects http on standard port' do
allow(Gitlab.config.gitlab).to receive(:port).and_return(80)
allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
stub_url(['http://', config.host, '/gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'detects http on non-standard port' do
allow(Gitlab.config.gitlab).to receive(:port).and_return(3000)
allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
stub_url(['http://', config.host, ':3000/gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'works with relative_url_root' do
@@ -69,7 +93,11 @@
allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root')
allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
stub_url(['http://', config.host, '/gitlab/root/gitlab-org/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org', 'gitlab-foss'), namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
it 'works with subgroups' do
@@ -77,61 +105,105 @@
allow(Gitlab.config.gitlab).to receive(:relative_url_root).and_return('/gitlab/root')
allow(Gitlab.config.gitlab).to receive(:url).and_return(Settings.send(:build_gitlab_url))
stub_url(['http://', config.host, '/gitlab/root/gitlab-org/sub/gitlab-foss.git'].join(''))
- expect(subject).to eq([namespace_project_path('gitlab-org/sub', 'gitlab-foss'), namespace_project_tree_path('gitlab-org/sub', 'gitlab-foss', 'hash')])
+ aggregate_failures do
+ expect(subject.web).to eq(namespace_project_path('gitlab-org/sub', 'gitlab-foss'))
+ expect(subject.tree).to eq(namespace_project_tree_path('gitlab-org/sub', 'gitlab-foss', 'hash'))
+ expect(subject.compare).to be_nil
+ end
end
end
context 'submodule on gist.github.com' do
it 'detects ssh' do
stub_url('git@gist.github.com:gitlab-org/gitlab-foss.git')
- is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects http' do
stub_url('http://gist.github.com/gitlab-org/gitlab-foss.git')
- is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects https' do
stub_url('https://gist.github.com/gitlab-org/gitlab-foss.git')
- is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'handles urls with no .git on the end' do
stub_url('http://gist.github.com/gitlab-org/gitlab-foss')
- is_expected.to eq(['https://gist.github.com/gitlab-org/gitlab-foss', 'https://gist.github.com/gitlab-org/gitlab-foss/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gist.github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gist.github.com/gitlab-org/gitlab-foss/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'returns original with non-standard url' do
stub_url('http://gist.github.com/another/gitlab-org/gitlab-foss.git')
- is_expected.to eq([repo.submodule_url_for, nil])
+ aggregate_failures do
+ expect(subject.web).to eq(repo.submodule_url_for)
+ expect(subject.tree).to be_nil
+ expect(subject.compare).to be_nil
+ end
end
end
context 'submodule on github.com' do
it 'detects ssh' do
stub_url('git@github.com:gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects http' do
stub_url('http://github.com/gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects https' do
stub_url('https://github.com/gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'handles urls with no .git on the end' do
stub_url('http://github.com/gitlab-org/gitlab-foss')
- expect(subject).to eq(['https://github.com/gitlab-org/gitlab-foss', 'https://github.com/gitlab-org/gitlab-foss/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://github.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://github.com/gitlab-org/gitlab-foss/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'returns original with non-standard url' do
stub_url('http://github.com/another/gitlab-org/gitlab-foss.git')
- expect(subject).to eq([repo.submodule_url_for, nil])
+ aggregate_failures do
+ expect(subject.web).to eq(repo.submodule_url_for)
+ expect(subject.tree).to be_nil
+ expect(subject.compare).to be_nil
+ end
end
end
@@ -143,39 +215,67 @@
allow(repo).to receive(:project).and_return(project)
stub_url('./')
- expect(subject).to eq(["/master-project/#{project.path}", "/master-project/#{project.path}/-/tree/hash"])
+ aggregate_failures do
+ expect(subject.web).to eq("/master-project/#{project.path}")
+ expect(subject.tree).to eq("/master-project/#{project.path}/-/tree/hash")
+ expect(subject.compare).to be_nil
+ end
end
end
context 'submodule on gitlab.com' do
it 'detects ssh' do
stub_url('git@gitlab.com:gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects http' do
stub_url('http://gitlab.com/gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'detects https' do
stub_url('https://gitlab.com/gitlab-org/gitlab-foss.git')
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'handles urls with no .git on the end' do
stub_url('http://gitlab.com/gitlab-org/gitlab-foss')
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'handles urls with trailing whitespace' do
stub_url('http://gitlab.com/gitlab-org/gitlab-foss.git ')
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
end
it 'returns original with non-standard url' do
stub_url('http://gitlab.com/another/gitlab-org/gitlab-foss.git')
- expect(subject).to eq([repo.submodule_url_for, nil])
+ aggregate_failures do
+ expect(subject.web).to eq(repo.submodule_url_for)
+ expect(subject.tree).to be_nil
+ expect(subject.compare).to be_nil
+ end
end
end
@@ -183,25 +283,29 @@
it 'sanitizes unsupported protocols' do
stub_url('javascript:alert("XSS");')
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
it 'sanitizes unsupported protocols disguised as a repository URL' do
stub_url('javascript:alert("XSS");foo/bar.git')
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
it 'sanitizes invalid URL with extended ASCII' do
stub_url('é')
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
it 'returns original' do
stub_url('http://mygitserver.com/gitlab-org/gitlab-foss')
- expect(subject).to eq([repo.submodule_url_for, nil])
+ aggregate_failures do
+ expect(subject.web).to eq(repo.submodule_url_for)
+ expect(subject.tree).to be_nil
+ expect(subject.compare).to be_nil
+ end
end
end
@@ -214,7 +318,11 @@ def expect_relative_link_to_resolve_to(relative_path, expected_path)
stub_url(relative_path)
result = subject
- expect(result).to eq([expected_path, "#{expected_path}/-/tree/#{submodule_item.id}"])
+ aggregate_failures do
+ expect(result.web).to eq(expected_path)
+ expect(result.tree).to eq("#{expected_path}/-/tree/#{submodule_item.id}")
+ expect(result.compare).to be_nil
+ end
end
it 'handles project under same group' do
@@ -235,7 +343,7 @@ def expect_relative_link_to_resolve_to(relative_path, expected_path)
result = subject
- expect(result).to eq([nil, nil])
+ expect(result).to be_nil
end
end
@@ -245,7 +353,7 @@ def expect_relative_link_to_resolve_to(relative_path, expected_path)
result = subject
- expect(result).to eq([nil, nil])
+ expect(result).to be_nil
end
end
@@ -289,7 +397,7 @@ def expect_relative_link_to_resolve_to(relative_path, expected_path)
end
it 'returns no links' do
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
end
end
diff --git a/spec/lib/gitlab/submodule_links_spec.rb b/spec/lib/gitlab/submodule_links_spec.rb
index c69326e12bea6ec1db7b8c91b93d369cabd41975..e2bbda81780024f2b62f57dc4228de0f55aa9509 100644
--- a/spec/lib/gitlab/submodule_links_spec.rb
+++ b/spec/lib/gitlab/submodule_links_spec.rb
@@ -18,7 +18,7 @@
end
it 'returns no links' do
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
end
@@ -28,17 +28,28 @@
end
it 'returns no links' do
- expect(subject).to eq([nil, nil])
+ expect(subject).to be_nil
end
end
context 'when the submodule is known' do
before do
- stub_urls({ 'gitlab-foss' => 'git@gitlab.com:gitlab-org/gitlab-foss.git' })
+ gitlab_foss = 'git@gitlab.com:gitlab-org/gitlab-foss.git'
+
+ stub_urls({
+ 'ref' => { 'gitlab-foss' => gitlab_foss },
+ 'other_ref' => { 'gitlab-foss' => gitlab_foss },
+ 'signed-commits' => { 'gitlab-foss' => gitlab_foss },
+ 'special_ref' => { 'gitlab-foss' => 'git@OTHER.com:gitlab-org/gitlab-foss.git' }
+ })
end
it 'returns links and caches the by ref' do
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
cache_store = links.instance_variable_get("@cache_store")
@@ -49,13 +60,46 @@
let(:ref) { 'signed-commits' }
it 'returns links' do
- expect(subject).to eq(['https://gitlab.com/gitlab-org/gitlab-foss', 'https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash'])
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
+ end
+ end
+
+ context 'and the diff information is available' do
+ let(:old_ref) { 'other_ref' }
+ let(:diff_file) { double(old_blob: double(id: 'old-hash', path: 'gitlab-foss'), old_content_sha: old_ref) }
+
+ subject { links.for(submodule_item, ref, diff_file) }
+
+ it 'the returned links include the compare link' do
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/compare/old-hash...hash')
+ end
+ end
+
+ context 'but the submodule url has changed' do
+ let(:old_ref) { 'special_ref' }
+
+ it 'the returned links do not include the compare link' do
+ aggregate_failures do
+ expect(subject.web).to eq('https://gitlab.com/gitlab-org/gitlab-foss')
+ expect(subject.tree).to eq('https://gitlab.com/gitlab-org/gitlab-foss/-/tree/hash')
+ expect(subject.compare).to be_nil
+ end
+ end
end
end
end
end
- def stub_urls(urls)
- allow(repo).to receive(:submodule_urls_for).and_return(urls)
+ def stub_urls(urls_by_ref)
+ allow(repo).to receive(:submodule_urls_for) do |ref|
+ urls_by_ref[ref] if urls_by_ref
+ end
end
end
diff --git a/spec/serializers/diff_file_base_entity_spec.rb b/spec/serializers/diff_file_base_entity_spec.rb
index bf69a50a072b771f8aceb6c94043e24301a7a81c..94c39e117903fc463f3dfb1fe90d04bde6691dcc 100644
--- a/spec/serializers/diff_file_base_entity_spec.rb
+++ b/spec/serializers/diff_file_base_entity_spec.rb
@@ -7,21 +7,50 @@
let(:repository) { project.repository }
let(:entity) { described_class.new(diff_file, options).as_json }
- context 'diff for a changed submodule' do
- let(:commit_sha_with_changed_submodule) do
- "cfe32cf61b73a0d5e9f13e774abde7ff789b1660"
- end
-
- let(:commit) { project.commit(commit_sha_with_changed_submodule) }
+ context 'submodule information for a' do
+ let(:commit_sha) { "" }
+ let(:commit) { project.commit(commit_sha) }
let(:options) { { request: {}, submodule_links: Gitlab::SubmoduleLinks.new(repository) } }
let(:diff_file) { commit.diffs.diff_files.to_a.last }
- it do
- expect(entity[:submodule]).to eq(true)
- expect(entity[:submodule_link]).to eq("https://github.com/randx/six")
- expect(entity[:submodule_tree_url]).to eq(
- "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d"
- )
+ context 'newly added submodule' do
+ let(:commit_sha) { "cfe32cf61b73a0d5e9f13e774abde7ff789b1660" }
+
+ it 'says it is a submodule and contains links' do
+ expect(entity[:submodule]).to eq(true)
+ expect(entity[:submodule_link]).to eq("https://github.com/randx/six")
+ expect(entity[:submodule_tree_url]).to eq(
+ "https://github.com/randx/six/tree/409f37c4f05865e4fb208c771485f211a22c4c2d"
+ )
+ end
+
+ it 'has no compare url because the submodule was newly added' do
+ expect(entity[:submodule_compare]).to eq(nil)
+ end
+ end
+
+ context 'changed submodule' do
+ # Test repo does not contain a commit that changes a submodule, so we have create such a commit here
+ let(:commit_sha) { repository.update_submodule(project.members[0].user, "six", "c6bc3aa2ee76cadaf0cbd325067c55450997632e", message: "Go one commit back", branch: "master") }
+
+ it 'contains a link to compare the changes' do
+ expect(entity[:submodule_compare]).to eq(
+ {
+ url: "https://github.com/randx/six/compare/409f37c4f05865e4fb208c771485f211a22c4c2d...c6bc3aa2ee76cadaf0cbd325067c55450997632e",
+ old_sha: "409f37c4f05865e4fb208c771485f211a22c4c2d",
+ new_sha: "c6bc3aa2ee76cadaf0cbd325067c55450997632e"
+ }
+ )
+ end
+ end
+
+ context 'normal file (no submodule)' do
+ let(:commit_sha) { '570e7b2abdd848b95f2f578043fc23bd6f6fd24d' }
+
+ it 'sets submodule to false' do
+ expect(entity[:submodule]).to eq(false)
+ expect(entity[:submodule_compare]).to eq(nil)
+ end
end
end