Merge branch 'issue-24093-compare-link-for-submod-changes' into 'master'

Add link to compare changes intoduced by a git submodule update

See merge request gitlab-org/gitlab!37740

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>

app/helpers/diff_helper.rb

@@ -100,20 +100,43 @@ module DiffHelper
   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

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 @@ module SubmoduleHelper
       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 @@ module SubmoduleHelper
     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 @@ module SubmoduleHelper
     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 @@ module SubmoduleHelper
     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
 

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
 

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

changelogs/unreleased/compare-link-git-submodule-update.yml

+---
+title: Add link to compare changes intoduced by a git submodule update
+merge_request: 37740
+author: Daniel Seemer @Phaiax
+type: added

lib/gitlab/graphql/representation/submodule_tree_entry.rb

@@ -24,11 +24,11 @@ module Gitlab
         end
 
         def web_url
-          @submodule_links.first
+          @submodule_links&.web
         end
 
         def tree_url
-          @submodule_links.last
+          @submodule_links&.tree
         end
       end
     end

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 @@ module Gitlab
       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

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 ""
 

spec/lib/gitlab/submodule_links_spec.rb

@@ -18,7 +18,7 @@ RSpec.describe Gitlab::SubmoduleLinks do
       end
 
       it 'returns no links' do
-        expect(subject).to eq([nil, nil])
+        expect(subject).to be_nil
       end
     end
 
@@ -28,17 +28,28 @@ RSpec.describe Gitlab::SubmoduleLinks do
       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 @@ RSpec.describe Gitlab::SubmoduleLinks do
         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

spec/serializers/diff_file_base_entity_spec.rb

@@ -7,21 +7,50 @@ RSpec.describe DiffFileBaseEntity do
   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