Unify anchor link format for MR diff files !7298

Right now, the following naming scheme for diff files is used: diff-1, diff-2, ... and also we have "internal" format which is file-path-HASH, where HASH is sha1 of file path.
Besides, we have HASH_lineA_lineB format to link exact line number in MR diff. It makes sence to unify the way we link diff from outside, while leave "file-path-HASH" format for internal (js) usage.
Changes in this commit  allow to link diff just by HASH, if we don't want specify exact lines, also it changes "file-path-HASH" and "diff-NUMBER" links in code to this unified format.

Inspired by #24010 and !7298

app/controllers/projects/blob_controller.rb

@@ -42,7 +42,7 @@ class Projects::BlobController < Projects::ApplicationController
     after_edit_path =
       if from_merge_request && @target_branch == @ref
         diffs_namespace_project_merge_request_path(from_merge_request.target_project.namespace, from_merge_request.target_project, from_merge_request) +
-          "#file-path-#{hexdigest(@path)}"
+          "##{hexdigest(@path)}"
       else
         namespace_project_blob_path(@project.namespace, @project, File.join(@target_branch, @path))
       end

app/views/notify/repository_push_email.html.haml

@@ -27,9 +27,9 @@
   %h4 #{pluralize @message.diffs_count, "changed file"}:
 
   %ul
-    - @message.diffs.each_with_index do |diff, i|
+    - @message.diffs.each do |diff|
       %li.file-stats
-        %a{href: "#{@message.target_url if @message.disable_diffs?}#diff-#{i}" }
+        %a{href: "#{@message.target_url if @message.disable_diffs?}##{hexdigest(diff.file_path)}" }
           - if diff.deleted_file
             %span.deleted-file
               −
@@ -52,9 +52,10 @@
       %h5 The diff was not included because it is too large.
     - else
       %h4 Changes:
-      - diff_files.each_with_index do |diff_file, i|
-        %li{id: "diff-#{i}"}
-          %a{href: @message.target_url + "#diff-#{i}"}<
+      - diff_files.each do |diff_file|
+        - file_hash = hexdigest(diff_file.file_path)
+        %li{id: file_hash}
+          %a{href: @message.target_url + "##{file_hash}"}<
             - if diff_file.deleted_file
               %strong<
                 = diff_file.old_path

app/views/projects/diffs/_diffs.html.haml

@@ -22,11 +22,12 @@
   = render 'projects/diffs/warning', diff_files: diff_files
 
 .files{ data: { can_create_note: can_create_note } }
-  - diff_files.each_with_index do |diff_file, index|
+  - diff_files.each_with_index do |diff_file|
     - diff_commit = commit_for_diff(diff_file)
     - blob = diff_file.blob(diff_commit)
     - next unless blob
     - blob.load_all_data!(diffs.project.repository) unless blob.only_display_raw?
+    - file_hash = hexdigest(diff_file.file_path)
 
-    = render 'projects/diffs/file', index: index, project: diffs.project,
+    = render 'projects/diffs/file', file_hash: file_hash, project: diffs.project,
       diff_file: diff_file, diff_commit: diff_commit, blob: blob

app/views/projects/diffs/_file.html.haml

-.diff-file.file-holder{id: "diff-#{index}", data: diff_file_html_data(project, diff_file.file_path, diff_commit.id)}
+.diff-file.file-holder{id: file_hash, data: diff_file_html_data(project, diff_file.file_path, diff_commit.id)}
   .file-title{id: "file-path-#{hexdigest(diff_file.file_path)}"}
-    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "#diff-#{index}"
+    = render "projects/diffs/file_header", diff_file: diff_file, blob: blob, diff_commit: diff_commit, project: project, url: "##{file_hash}"
 
     - unless diff_file.submodule?
       .file-actions.hidden-xs

app/views/projects/diffs/_stats.html.haml

@@ -9,28 +9,29 @@
     %strong.cred #{diff_files.sum(&:removed_lines)} deletions
   .file-stats.js-toggle-content.hide
     %ul
-      - diff_files.each_with_index do |diff_file, i|
+      - diff_files.each do |diff_file|
+        - file_hash = hexdigest(diff_file.file_path)
         %li
           - if diff_file.deleted_file
             %span.deleted-file
-              %a{href: "#diff-#{i}"}
+              %a{href: "##{file_hash}"}
                 %i.fa.fa-minus
                 = diff_file.old_path
           - elsif diff_file.renamed_file
             %span.renamed-file
-              %a{href: "#diff-#{i}"}
+              %a{href: "##{file_hash}"}
                 %i.fa.fa-minus
                 = diff_file.old_path
                 →
                 = diff_file.new_path
           - elsif diff_file.new_file
             %span.new-file
-              %a{href: "#diff-#{i}"}
+              %a{href: "##{file_hash}"}
                 %i.fa.fa-plus
                 = diff_file.new_path
           - else
             %span.edit-file
-              %a{href: "#diff-#{i}"}
+              %a{href: "##{file_hash}"}
                 %i.fa.fa-adjust
                 = diff_file.new_path
 

changelogs/unreleased/24010-change-anchor-link-to-mr-diff.yml

+---
+title: Unify anchor link format for MR diff files
+merge_request: 7298
+author: YarNayar

features/steps/project/merge_requests.rb

@@ -413,37 +413,37 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I click link "Hide inline discussion" of the third file' do
-    page.within '.files [id^=diff]:nth-child(3)' do
+    page.within '.files>div:nth-child(3)' do
       find('.js-toggle-diff-comments').trigger('click')
     end
   end
 
   step 'I click link "Show inline discussion" of the third file' do
-    page.within '.files [id^=diff]:nth-child(3)' do
+    page.within '.files>div:nth-child(3)' do
       find('.js-toggle-diff-comments').trigger('click')
     end
   end
 
   step 'I should not see a comment like "Line is wrong" in the third file' do
-    page.within '.files [id^=diff]:nth-child(3)' do
+    page.within '.files>div:nth-child(3)' do
       expect(page).not_to have_visible_content "Line is wrong"
     end
   end
 
   step 'I should see a comment like "Line is wrong" in the third file' do
-    page.within '.files [id^=diff]:nth-child(3) .note-body > .note-text' do
+    page.within '.files>div:nth-child(3) .note-body > .note-text' do
       expect(page).to have_visible_content "Line is wrong"
     end
   end
 
   step 'I should not see a comment like "Line is wrong here" in the third file' do
-    page.within '.files [id^=diff]:nth-child(3)' do
+    page.within '.files>div:nth-child(3)' do
       expect(page).not_to have_visible_content "Line is wrong here"
     end
   end
 
   step 'I should see a comment like "Line is wrong here" in the third file' do
-    page.within '.files [id^=diff]:nth-child(3) .note-body > .note-text' do
+    page.within '.files>div:nth-child(3) .note-body > .note-text' do
       expect(page).to have_visible_content "Line is wrong here"
     end
   end
@@ -456,7 +456,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
       click_button "Comment"
     end
 
-    page.within ".files [id^=diff]:nth-child(2) .note-body > .note-text" do
+    page.within ".files>div:nth-child(2) .note-body > .note-text" do
       expect(page).to have_content "Line is correct"
     end
   end
@@ -471,7 +471,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I should still see a comment like "Line is correct" in the second file' do
-    page.within '.files [id^=diff]:nth-child(2) .note-body > .note-text' do
+    page.within '.files>div:nth-child(2) .note-body > .note-text' do
       expect(page).to have_visible_content "Line is correct"
     end
   end
@@ -494,7 +494,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I should see comments on the side-by-side diff page' do
-    page.within '.files [id^=diff]:nth-child(2) .parallel .note-body > .note-text' do
+    page.within '.files>div:nth-child(2) .parallel .note-body > .note-text' do
       expect(page).to have_visible_content "Line is correct"
     end
   end