From a328242e07cf825301af2bd428f757462c41b3c4 Mon Sep 17 00:00:00 2001 From: Vasilii Iakliushin Date: Wed, 30 Aug 2023 18:20:34 +0200 Subject: [PATCH] Add an option to use `unidiff` format for diff API responses Contriubutes to https://gitlab.com/gitlab-org/gitlab/-/issues/23284 **Problem** `diff` field doesn't return diff headers compatible with Unified format. **Solution** Use Unified format for diffs when unidiff option is provided. Changelog: added --- doc/api/commits.md | 1 + doc/api/merge_requests.md | 3 + doc/api/repositories.md | 1 + lib/api/commits.rb | 4 +- lib/api/entities/diff.rb | 6 +- lib/api/helpers/unidiff.rb | 17 +++ lib/api/merge_request_diffs.rb | 4 +- lib/api/merge_requests.rb | 10 +- lib/api/repositories.rb | 4 +- lib/gitlab/diff/file.rb | 2 +- lib/gitlab/git/diff.rb | 12 +- spec/lib/api/entities/diff_spec.rb | 46 +++++++ spec/lib/gitlab/git/diff_spec.rb | 122 +++++++++++++++++- spec/requests/api/commits_spec.rb | 10 ++ spec/requests/api/merge_request_diffs_spec.rb | 9 ++ spec/requests/api/merge_requests_spec.rb | 18 +++ spec/requests/api/repositories_spec.rb | 12 ++ 17 files changed, 271 insertions(+), 10 deletions(-) create mode 100644 lib/api/helpers/unidiff.rb create mode 100644 spec/lib/api/entities/diff_spec.rb diff --git a/doc/api/commits.md b/doc/api/commits.md index 68c453aa3178e8..94cdaaa191de00 100644 --- a/doc/api/commits.md +++ b/doc/api/commits.md @@ -452,6 +452,7 @@ Parameters: | --------- | ---- | -------- | ----------- | | `id` | integer/string | yes | The ID or [URL-encoded path of the project](rest/index.md#namespaced-path-encoding) owned by the authenticated user | `sha` | string | yes | The commit hash or name of a repository branch or tag | +| `unidiff` | boolean | no | Present diffs in the [unified diff](https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html) format. Default is false. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130610) in GitLab 16.5. | ```shell curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/5/repository/commits/main/diff" diff --git a/doc/api/merge_requests.md b/doc/api/merge_requests.md index 53a605c56f0955..d9700b807d8c89 100644 --- a/doc/api/merge_requests.md +++ b/doc/api/merge_requests.md @@ -986,6 +986,7 @@ Supported attributes: | `id` | integer or string | Yes | The ID or [URL-encoded path of the project](rest/index.md#namespaced-path-encoding) owned by the authenticated user. | | `merge_request_iid` | integer | Yes | The internal ID of the merge request. | | `access_raw_diffs` | boolean | No | Retrieve change diffs via Gitaly. | +| `unidiff` | boolean | No | Present change diffs in the [unified diff](https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html) format. Default is false. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130610) in GitLab 16.5. | ```json { @@ -1110,6 +1111,7 @@ Supported attributes: | `merge_request_iid` | integer | Yes | The internal ID of the merge request. | | `page` | integer | no | The page of results to return. Defaults to 1. | | `per_page` | integer | no | The number of results per page. Defaults to 20. | +| `unidiff` | boolean | no | Present diffs in the [unified diff](https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html) format. Default is false. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130610) in GitLab 16.5. | If successful, returns [`200 OK`](rest/index.md#status-codes) and the following response attributes: @@ -2605,6 +2607,7 @@ GET /projects/:id/merge_requests/:merge_request_iid/versions/:version_id | `id` | String | Yes | The ID of the project. | | `merge_request_iid` | integer | Yes | The internal ID of the merge request. | | `version_id` | integer | Yes | The ID of the merge request diff version. | +| `unidiff` | boolean | No | Present diffs in the [unified diff](https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html) format. Default is false. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130610) in GitLab 16.5. | ```shell curl --header "PRIVATE-TOKEN: " "https://gitlab.example.com/api/v4/projects/1/merge_requests/1/versions/1" diff --git a/doc/api/repositories.md b/doc/api/repositories.md index aae475a0356bdb..e17044f1c383ee 100644 --- a/doc/api/repositories.md +++ b/doc/api/repositories.md @@ -187,6 +187,7 @@ Supported attributes: | `to` | string | yes | The commit SHA or branch name. | | `from_project_id` | integer | no | The ID to compare from. | | `straight` | boolean | no | Comparison method: `true` for direct comparison between `from` and `to` (`from`..`to`), `false` to compare using merge base (`from`...`to`)'. Default is `false`. | +| `unidiff` | boolean | No | Present diffs in the [unified diff](https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html) format. Default is false. [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/130610) in GitLab 16.5. | ```plaintext GET /projects/:id/repository/compare?from=main&to=feature diff --git a/lib/api/commits.rb b/lib/api/commits.rb index 069d117db17442..7256d9c6d93761 100644 --- a/lib/api/commits.rb +++ b/lib/api/commits.rb @@ -4,6 +4,7 @@ module API class Commits < ::API::Base include PaginationParams + include Helpers::Unidiff feature_category :source_code_management @@ -274,6 +275,7 @@ def track_commit_events params do requires :sha, type: String, desc: 'A commit sha, or the name of a branch or tag' use :pagination + use :with_unidiff end get ':id/repository/commits/:sha/diff', requirements: API::COMMIT_ENDPOINT_REQUIREMENTS, urgency: :low do commit = user_project.commit(params[:sha]) @@ -282,7 +284,7 @@ def track_commit_events raw_diffs = ::Kaminari.paginate_array(commit.diffs(expanded: true).diffs.to_a) - present paginate(raw_diffs), with: Entities::Diff + present paginate(raw_diffs), with: Entities::Diff, enable_unidiff: declared_params[:unidiff] end desc "Get a commit's comments" do diff --git a/lib/api/entities/diff.rb b/lib/api/entities/diff.rb index b9538893d3205a..cc53736a5b1ed1 100644 --- a/lib/api/entities/diff.rb +++ b/lib/api/entities/diff.rb @@ -3,10 +3,12 @@ module API module Entities class Diff < Grape::Entity - expose :json_safe_diff, as: :diff, documentation: { + expose :diff, documentation: { type: 'string', example: '@@ -71,6 +71,8 @@\n...' - } + } do |instance, options| + options[:enable_unidiff] == true ? instance.unidiff : instance.json_safe_diff + end expose :new_path, documentation: { type: 'string', example: 'doc/update/5.4-to-6.0.md' } expose :old_path, documentation: { type: 'string', example: 'doc/update/5.4-to-6.0.md' } expose :a_mode, documentation: { type: 'string', example: '100755' } diff --git a/lib/api/helpers/unidiff.rb b/lib/api/helpers/unidiff.rb new file mode 100644 index 00000000000000..aabc0acd454014 --- /dev/null +++ b/lib/api/helpers/unidiff.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module API + module Helpers + module Unidiff + extend ActiveSupport::Concern + + included do + helpers do + params :with_unidiff do + optional :unidiff, type: ::Grape::API::Boolean, default: false, desc: 'A diff in a Unified diff format' + end + end + end + end + end +end diff --git a/lib/api/merge_request_diffs.rb b/lib/api/merge_request_diffs.rb index e7193035ce0ba8..9b8468b6efba17 100644 --- a/lib/api/merge_request_diffs.rb +++ b/lib/api/merge_request_diffs.rb @@ -4,6 +4,7 @@ module API # MergeRequestDiff API class MergeRequestDiffs < ::API::Base include PaginationParams + include Helpers::Unidiff before { authenticate! } @@ -39,12 +40,13 @@ class MergeRequestDiffs < ::API::Base params do requires :merge_request_iid, type: Integer, desc: 'The internal ID of the merge request' requires :version_id, type: Integer, desc: 'The ID of the merge request diff version' + use :with_unidiff end get ":id/merge_requests/:merge_request_iid/versions/:version_id", urgency: :low do merge_request = find_merge_request_with_access(params[:merge_request_iid]) - present_cached merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull, cache_context: nil + present_cached merge_request.merge_request_diffs.find(params[:version_id]), with: Entities::MergeRequestDiffFull, cache_context: nil, enable_unidiff: declared_params[:unidiff] end end end diff --git a/lib/api/merge_requests.rb b/lib/api/merge_requests.rb index 1c0b9c56aa7498..6b67e3aba194a0 100644 --- a/lib/api/merge_requests.rb +++ b/lib/api/merge_requests.rb @@ -3,6 +3,7 @@ module API class MergeRequests < ::API::Base include PaginationParams + include Helpers::Unidiff CONTEXT_COMMITS_POST_LIMIT = 20 @@ -505,6 +506,9 @@ def batch_process_mergeability_checks(merge_requests) ] tags %w[merge_requests] end + params do + use :with_unidiff + end get ':id/merge_requests/:merge_request_iid/changes', feature_category: :code_review_workflow, urgency: :low do merge_request = find_merge_request_with_access(params[:merge_request_iid]) @@ -512,7 +516,8 @@ def batch_process_mergeability_checks(merge_requests) with: Entities::MergeRequestChanges, current_user: current_user, project: user_project, - access_raw_diffs: to_boolean(params.fetch(:access_raw_diffs, false)) + access_raw_diffs: to_boolean(params.fetch(:access_raw_diffs, false)), + enable_unidiff: declared_params[:unidiff] end desc 'Get the merge request diffs' do @@ -526,11 +531,12 @@ def batch_process_mergeability_checks(merge_requests) end params do use :pagination + use :with_unidiff end get ':id/merge_requests/:merge_request_iid/diffs', feature_category: :code_review_workflow, urgency: :low do merge_request = find_merge_request_with_access(params[:merge_request_iid]) - present paginate(merge_request.merge_request_diff.paginated_diffs(params[:page], params[:per_page])).diffs, with: Entities::Diff + present paginate(merge_request.merge_request_diff.paginated_diffs(params[:page], params[:per_page])).diffs, with: Entities::Diff, enable_unidiff: declared_params[:unidiff] end desc 'Get single merge request pipelines' do diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb index 98316bf1d4ba58..0f1426dde99396 100644 --- a/lib/api/repositories.rb +++ b/lib/api/repositories.rb @@ -5,6 +5,7 @@ module API class Repositories < ::API::Base include PaginationParams + include Helpers::Unidiff content_type :txt, 'text/plain' @@ -202,6 +203,7 @@ def rescue_not_found? documentation: { example: 'feature' } optional :from_project_id, type: Integer, desc: 'The project to compare from', documentation: { example: 1 } optional :straight, type: Boolean, desc: 'Comparison method, `true` for direct comparison between `from` and `to` (`from`..`to`), `false` to compare using merge base (`from`...`to`)', default: false + use :with_unidiff end get ':id/repository/compare', urgency: :low do target_project = fetch_target_project(current_user, user_project, params) @@ -220,7 +222,7 @@ def rescue_not_found? compare = CompareService.new(user_project, params[:to]).execute(target_project, params[:from], straight: params[:straight]) if compare - present compare, with: Entities::Compare, current_user: current_user + present compare, with: Entities::Compare, current_user: current_user, enable_unidiff: declared_params[:unidiff] else not_found!("Ref") end diff --git a/lib/gitlab/diff/file.rb b/lib/gitlab/diff/file.rb index d5c0b187f924c0..de7be6efd72190 100644 --- a/lib/gitlab/diff/file.rb +++ b/lib/gitlab/diff/file.rb @@ -7,7 +7,7 @@ class File attr_reader :diff, :repository, :diff_refs, :fallback_diff_refs, :unique_identifier - delegate :new_file?, :deleted_file?, :renamed_file?, + delegate :new_file?, :deleted_file?, :renamed_file?, :unidiff, :old_path, :new_path, :a_mode, :b_mode, :mode_changed?, :submodule?, :expanded?, :too_large?, :collapsed?, :line_count, :has_binary_notice?, to: :diff, prefix: false diff --git a/lib/gitlab/git/diff.rb b/lib/gitlab/git/diff.rb index 0694e4d0f78575..743bac62764ef1 100644 --- a/lib/gitlab/git/diff.rb +++ b/lib/gitlab/git/diff.rb @@ -33,7 +33,7 @@ class Diff SERIALIZE_KEYS = %i[diff new_path old_path a_mode b_mode new_file renamed_file deleted_file too_large].freeze - BINARY_NOTICE_PATTERN = %r{Binary files a\/(.*) and b\/(.*) differ} + BINARY_NOTICE_PATTERN = %r{Binary files (.*) and (.*) differ} class << self def between(repo, head, base, options = {}, *paths) @@ -183,6 +183,16 @@ def submodule? a_mode == '160000' || b_mode == '160000' end + def unidiff + return diff if diff.blank? + return json_safe_diff if detect_binary?(@diff) || has_binary_notice? + + old_path_header = new_file? ? '/dev/null' : "a/#{old_path}" + new_path_header = deleted_file? ? '/dev/null' : "b/#{new_path}" + + "--- #{old_path_header}\n+++ #{new_path_header}\n" + diff + end + def line_count @line_count ||= Util.count_lines(@diff) end diff --git a/spec/lib/api/entities/diff_spec.rb b/spec/lib/api/entities/diff_spec.rb new file mode 100644 index 00000000000000..27d9ed44c98925 --- /dev/null +++ b/spec/lib/api/entities/diff_spec.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::API::Entities::Diff, feature_category: :source_code_management do + subject(:json) { entity.as_json } + + let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :repository) } + let_it_be(:repository) { project.repository } + let_it_be(:diff) { repository.diff('HEAD~1', 'HEAD').first } + + let(:entity) { described_class.new(diff, options) } + let(:options) { {} } + + it 'returns expected data' do + expect(entity.as_json).to eq( + { + diff: diff.diff, + new_path: diff.new_path, + old_path: diff.old_path, + a_mode: diff.a_mode, + b_mode: diff.b_mode, + new_file: diff.new_file?, + renamed_file: diff.renamed_file?, + deleted_file: diff.deleted_file? + } + ) + end + + context 'when enable_unidiff option is set' do + let(:options) { { enable_unidiff: true } } + + it 'returns expected data' do + expect(entity.as_json).to include(diff: diff.unidiff) + end + end + + context 'when enable_unidiff option is false' do + let(:options) { { enable_unidiff: false } } + + it 'returns expected data' do + expect(entity.as_json).to include(diff: diff.diff) + end + end +end diff --git a/spec/lib/gitlab/git/diff_spec.rb b/spec/lib/gitlab/git/diff_spec.rb index 4d78e194da87a0..6b3630d7a1f6e6 100644 --- a/spec/lib/gitlab/git/diff_spec.rb +++ b/spec/lib/gitlab/git/diff_spec.rb @@ -2,7 +2,7 @@ require "spec_helper" -RSpec.describe Gitlab::Git::Diff do +RSpec.describe Gitlab::Git::Diff, feature_category: :source_code_management do let_it_be(:project) { create(:project, :repository) } let_it_be(:repository) { project.repository } @@ -336,6 +336,121 @@ end end + describe '#unidiff' do + let_it_be(:project) { create(:project, :empty_repo) } + let_it_be(:repository) { project.repository } + let_it_be(:user) { project.first_owner } + + let(:commits) { repository.commits('master', limit: 10) } + let(:diffs) { commits.map(&:diffs).map(&:diffs).flat_map(&:to_a).reverse } + + before_all do + create_commit( + project, + user, + commit_message: "Create file", + actions: [{ action: 'create', content: 'foo', file_path: 'a.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Update file", + actions: [{ action: 'update', content: 'foo2', file_path: 'a.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Rename file without change", + actions: [{ action: 'move', previous_path: 'a.txt', file_path: 'b.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Rename file with change", + actions: [{ action: 'move', content: 'foo3', previous_path: 'b.txt', file_path: 'c.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Delete file", + actions: [{ action: 'delete', file_path: 'c.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Create empty file", + actions: [{ action: 'create', file_path: 'empty.txt' }] + ) + + create_commit( + project, + user, + commit_message: "Create binary file", + actions: [{ action: 'create', content: 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQMAAAAl21bKAAAAA1BMVEUAAACnej3aAAAAAXRSTlMAQObYZgAAAApJREFUCNdjYAAAAAIAAeIhvDMAAAAASUVORK5CYII=', file_path: 'test%2Ebin', encoding: 'base64' }] + ) + end + + context 'when file was created' do + it 'returns a correct header' do + diff = diffs[0] + + expect(diff.unidiff).to start_with("--- /dev/null\n+++ b/a.txt\n") + end + end + + context 'when file was changed' do + it 'returns a correct header' do + diff = diffs[1] + + expect(diff.unidiff).to start_with("--- a/a.txt\n+++ b/a.txt\n") + end + end + + context 'when file was moved without content change' do + it 'returns an empty header' do + diff = diffs[2] + + expect(diff.unidiff).to eq('') + end + end + + context 'when file was moved with content change' do + it 'returns a correct header' do + expect(diffs[3].unidiff).to start_with("--- /dev/null\n+++ b/c.txt\n") + expect(diffs[4].unidiff).to start_with("--- a/b.txt\n+++ /dev/null\n") + end + end + + context 'when file was deleted' do + it 'returns a correct header' do + diff = diffs[5] + + expect(diff.unidiff).to start_with("--- a/c.txt\n+++ /dev/null\n") + end + end + + context 'when empty file was created' do + it 'returns an empty header' do + diff = diffs[6] + + expect(diff.unidiff).to eq('') + end + end + + context 'when file is binary' do + it 'returns a binary files message' do + diff = diffs[7] + + expect(diff.unidiff).to eq("Binary files /dev/null and b/test%2Ebin differ\n") + end + end + end + describe '#submodule?' do let(:gitaly_submodule_diff) do Gitlab::GitalyClient::Diff.new( @@ -445,4 +560,9 @@ def binary_diff(project) # rugged will not detect this as binary, but we can fake it described_class.between(project.repository, 'add-pdf-text-binary', 'add-pdf-text-binary^').first end + + def create_commit(project, user, params) + params = { start_branch: 'master', branch_name: 'master' }.merge(params) + Files::MultiService.new(project, user, params).execute.fetch(:result) + end end diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb index 8b9ac7cd58864e..7843a971a45ff3 100644 --- a/spec/requests/api/commits_spec.rb +++ b/spec/requests/api/commits_spec.rb @@ -1640,6 +1640,16 @@ def push_params(branch_name) it_behaves_like 'ref diff' end + + context 'when unidiff format is requested' do + it 'returns the diff in Unified format' do + get api(route, current_user), params: { unidiff: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(response).to include_limited_pagination_headers + expect(json_response.dig(0, 'diff')).to eq(commit.diffs.diffs.first.unidiff) + end + end end end diff --git a/spec/requests/api/merge_request_diffs_spec.rb b/spec/requests/api/merge_request_diffs_spec.rb index 4f812e5d8ebbb3..53cef226ad8bde 100644 --- a/spec/requests/api/merge_request_diffs_spec.rb +++ b/spec/requests/api/merge_request_diffs_spec.rb @@ -55,6 +55,15 @@ expect(json_response['diffs'].size).to eq(merge_request_diff.diffs.size) end + context 'when unidiff format is requested' do + it 'returns a diff in Unified format' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/versions/#{merge_request_diff.id}", user), params: { unidiff: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.dig('diffs', 0, 'diff')).to eq(merge_request_diff.diffs.diffs.first.unidiff) + end + end + it 'returns a 404 when merge_request id is used instead of the iid' do get api("/projects/#{project.id}/merge_requests/#{merge_request.id}/versions/#{merge_request_diff.id}", user) expect(response).to have_gitlab_http_status(:not_found) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index d3f8aeb3e76ac3..4a11cddc79f1a5 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -1829,6 +1829,15 @@ expect(json_response['overflow']).to be_falsy end + context 'when unidiff format is requested' do + it 'returns the diff in Unified format' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/changes", user), params: { unidiff: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.dig('changes', 0, 'diff')).to eq(merge_request.diffs.diffs.first.unidiff) + end + end + context 'when using DB-backed diffs' do it_behaves_like 'find an existing merge request' @@ -1902,6 +1911,15 @@ expect(json_response.size).to eq(merge_request.diffs.size) end + context 'when unidiff format is requested' do + it 'returns the diff in Unified format' do + get api("/projects/#{project.id}/merge_requests/#{merge_request.iid}/diffs", user), params: { unidiff: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.dig(0, 'diff')).to eq(merge_request.diffs.diffs.first.unidiff) + end + end + context 'when pagination params are present' do it 'returns limited diffs' do get( diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb index a94ed63bf47678..22239f1d23fd00 100644 --- a/spec/requests/api/repositories_spec.rb +++ b/spec/requests/api/repositories_spec.rb @@ -538,6 +538,18 @@ expect(json_response['compare_same_ref']).to be_truthy end + context 'when unidiff format is requested' do + let(:commit) { project.repository.commit('feature') } + let(:diff) { commit.diffs.diffs.first } + + it 'returns a diff in Unified format' do + get api(route, current_user), params: { from: 'master', to: 'feature', unidiff: true } + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response.dig('diffs', 0, 'diff')).to eq(diff.unidiff) + end + end + it "returns an empty string when the diff overflows" do allow(Gitlab::Git::DiffCollection) .to receive(:default_limits) -- GitLab