From f554a07f74dac9943537b2a299bf87e5ebc74095 Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Mon, 13 Jan 2025 16:05:41 -0600 Subject: [PATCH 1/8] Update Gitaly gem file - Modifies the raw_blame method signature to accept ignore_revisions_blob - Updates the Gitaly::RawBlameRequest creation to include the new parameter - Ensures compatibility with the updated Gitaly gem --- Gemfile.lock | 6 +++--- lib/gitlab/gitaly_client/commit_service.rb | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 6be8e7dfb4103c..5c920041f83712 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -732,7 +732,7 @@ GEM git (1.18.0) addressable (~> 2.8) rchardet (~> 1.8) - gitaly (17.7.0) + gitaly (17.7.1) grpc (~> 1.0) gitlab (4.19.0) httparty (~> 0.20) @@ -902,8 +902,8 @@ GEM google-protobuf (~> 3.14) googleapis-common-protos-types (~> 1.2) grpc (~> 1.27) - googleapis-common-protos-types (1.5.0) - google-protobuf (~> 3.14) + googleapis-common-protos-types (1.17.0) + google-protobuf (>= 3.18, < 5.a) googleauth (1.8.1) faraday (>= 0.17.3, < 3.a) jwt (>= 1.4, < 3.0) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 6e55f0345c7365..b324eefb388385 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -403,12 +403,13 @@ def languages(ref = nil) response.languages.map { |l| { value: l.share.round(2), label: l.name, color: l.color, highlight: l.color } } end - def raw_blame(revision, path, range:) + def raw_blame(revision, path, range:,ignore_revisions_blob) request = Gitaly::RawBlameRequest.new( repository: @gitaly_repo, revision: encode_binary(revision), path: encode_binary(path), - range: (encode_binary(range) if range) + range: (encode_binary(range) if range), + ignore_revisions_blob: (encode_binary(ignore_revisions_blob) if ignore_revisions_blob) ) response = gitaly_client_call(@repository.storage, :commit_service, :raw_blame, request, timeout: GitalyClient.medium_timeout) -- GitLab From 68b8ab4ff34f1d07f77a19f85eef03ec834c24ac Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Tue, 14 Jan 2025 17:20:46 -0600 Subject: [PATCH 2/8] Add new rspec test for ignore_revisions_blob --- Gemfile.lock | 6 ++-- lib/gitlab/gitaly_client/commit_service.rb | 2 +- .../gitaly_client/commit_service_spec.rb | 31 +++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 5c920041f83712..ea1518c8ecc83f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -732,7 +732,7 @@ GEM git (1.18.0) addressable (~> 2.8) rchardet (~> 1.8) - gitaly (17.7.1) + gitaly (17.7.0) grpc (~> 1.0) gitlab (4.19.0) httparty (~> 0.20) @@ -902,8 +902,8 @@ GEM google-protobuf (~> 3.14) googleapis-common-protos-types (~> 1.2) grpc (~> 1.27) - googleapis-common-protos-types (1.17.0) - google-protobuf (>= 3.18, < 5.a) + googleapis-common-protos-types (1.5.0) + google-protobuf (~> 3.14) googleauth (1.8.1) faraday (>= 0.17.3, < 3.a) jwt (>= 1.4, < 3.0) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index b324eefb388385..d4f59cb22f8814 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -403,7 +403,7 @@ def languages(ref = nil) response.languages.map { |l| { value: l.share.round(2), label: l.name, color: l.color, highlight: l.color } } end - def raw_blame(revision, path, range:,ignore_revisions_blob) + def raw_blame(revision, path, range:, ignore_revisions_blob: nil) request = Gitaly::RawBlameRequest.new( repository: @gitaly_repo, revision: encode_binary(revision), diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index bf8ac409603eae..0f06d7eee12c40 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1146,6 +1146,37 @@ def wrap_commits(commits) end end + context 'with ignore_revisions_blob' do + let(:file_name) { '.git-ignore-revs-file' } + let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } + let(:ignore_revisions_blob) { "refs/heads/master:#{file_name}" } + + before do + project.repository.create_file(project.owner, file_name, file_content, message: "add file", branch_name: "master") + end + + subject(:blame) { client.raw_blame(revision, path, range: range, ignore_revisions_blob: ignore_revisions_blob).split("\n") } + + it "excludes the specified revision from blame" do + original_blame = client.raw_blame(revision, path, range: range).split("\n") + expect(blame).not_to eq(original_blame) + expect(blame).not_to include(file_content) + end + + context 'with invalid ignore file content' do + let(:file_name) { '.git-ignore-revs-invalid' } + let(:file_content) { 'invalid_content' } + + it "raises a GRPC::NotFound error" do + expect { blame }.to raise_error(GRPC::NotFound) do |error| + expect(error.message).to include('invalid object name') + expect(error.debug_error_string).to include('grpc_status:5') + expect(error.debug_error_string).to include('grpc_message:"invalid object name"') + end + end + end + end + context 'with a range' do let(:range) { '3,4' } -- GitLab From 401b4b154a638688796894bde09dc4d33bf2d7a7 Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Tue, 14 Jan 2025 17:47:12 -0600 Subject: [PATCH 3/8] Revert gem version to previous version --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index ea1518c8ecc83f..6be8e7dfb4103c 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -903,7 +903,7 @@ GEM googleapis-common-protos-types (~> 1.2) grpc (~> 1.27) googleapis-common-protos-types (1.5.0) - google-protobuf (~> 3.14) + google-protobuf (~> 3.14) googleauth (1.8.1) faraday (>= 0.17.3, < 3.a) jwt (>= 1.4, < 3.0) -- GitLab From 51689514a8ee2101e3daf4e0a0a76768211c8c7c Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Wed, 15 Jan 2025 15:19:52 -0600 Subject: [PATCH 4/8] Add additional test for ignore_revision_blob --- spec/lib/gitlab/gitaly_client/commit_service_spec.rb | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 0f06d7eee12c40..04247554760f6f 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1175,6 +1175,14 @@ def wrap_commits(commits) end end end + + context 'with invalid ignore revision' do + let(:ignore_revisions_blob) { "refs/heads/invalid" } + + it "raises a GRPC::NotFound error" do + blame + end + end end context 'with a range' do -- GitLab From 3748b77b4e32b4f07d3c9622961bfd4e2a0144a3 Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Thu, 16 Jan 2025 14:47:58 -0600 Subject: [PATCH 5/8] Add additional rspec tests for ignore_revisions_blob -Refactor the test using shared examples --- .../gitaly_client/commit_service_spec.rb | 65 +++++++++++++------ 1 file changed, 45 insertions(+), 20 deletions(-) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 04247554760f6f..9ee63117b9b472 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1149,39 +1149,64 @@ def wrap_commits(commits) context 'with ignore_revisions_blob' do let(:file_name) { '.git-ignore-revs-file' } let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } - let(:ignore_revisions_blob) { "refs/heads/master:#{file_name}" } + let(:ignore_revisions_blob) { "refs/heads/#{branch_name}:#{file_name}" } + let(:branch_name) { generate :branch } - before do - project.repository.create_file(project.owner, file_name, file_content, message: "add file", branch_name: "master") + subject(:blame) do + client.raw_blame(revision, path, range: range, ignore_revisions_blob: ignore_revisions_blob).split("\n") end - subject(:blame) { client.raw_blame(revision, path, range: range, ignore_revisions_blob: ignore_revisions_blob).split("\n") } - - it "excludes the specified revision from blame" do - original_blame = client.raw_blame(revision, path, range: range).split("\n") - expect(blame).not_to eq(original_blame) - expect(blame).not_to include(file_content) + shared_examples 'raises error with message' do |error_class, message| + it "raises #{error_class} with correct message" do + expect { blame }.to raise_error(error_class) do |error| + expect(error.message).to include(message) + end + end end - context 'with invalid ignore file content' do - let(:file_name) { '.git-ignore-revs-invalid' } - let(:file_content) { 'invalid_content' } + context 'when ignore file exists' do + before do + project.repository.create_file( + project.owner, + file_name, + file_content, + message: "add file", + branch_name: branch_name + ) + end - it "raises a GRPC::NotFound error" do - expect { blame }.to raise_error(GRPC::NotFound) do |error| - expect(error.message).to include('invalid object name') - expect(error.debug_error_string).to include('grpc_status:5') - expect(error.debug_error_string).to include('grpc_message:"invalid object name"') + context "with valid ignore file" do + it 'excludes the specified revision from blame' do + original_blame = client.raw_blame(revision, path, range: range).split("\n") + expect(blame).not_to eq(original_blame) + expect(blame).not_to include(file_content) end end + + context 'with invalid ignore file content' do + let(:file_name) { '.git-ignore-revs-invalid' } + let(:file_content) { 'invalid_content' } + + include_examples 'raises error with message', + GRPC::NotFound, + 'invalid object name' + end end context 'with invalid ignore revision' do let(:ignore_revisions_blob) { "refs/heads/invalid" } - it "raises a GRPC::NotFound error" do - blame - end + include_examples 'raises error with message', + GRPC::NotFound, + '' + end + + context 'when ignore_blob is a directory' do + let(:ignore_revisions_blob) { "refs/heads/#{revision}:files" } + + include_examples 'raises error with message', + GRPC::InvalidArgument, + 'ignore revision is not a blob' end end -- GitLab From 79fe98277d7df00c4ba157e4a09406d2bc127e06 Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Fri, 17 Jan 2025 16:21:45 -0600 Subject: [PATCH 6/8] Apply suggestions requested by reviewer --- spec/lib/gitlab/gitaly_client/commit_service_spec.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 9ee63117b9b472..4f641f8a252608 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1147,8 +1147,6 @@ def wrap_commits(commits) end context 'with ignore_revisions_blob' do - let(:file_name) { '.git-ignore-revs-file' } - let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } let(:ignore_revisions_blob) { "refs/heads/#{branch_name}:#{file_name}" } let(:branch_name) { generate :branch } @@ -1176,6 +1174,9 @@ def wrap_commits(commits) end context "with valid ignore file" do + let(:file_name) { '.git-ignore-revs-file' } + let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } + it 'excludes the specified revision from blame' do original_blame = client.raw_blame(revision, path, range: range).split("\n") expect(blame).not_to eq(original_blame) @@ -1198,10 +1199,10 @@ def wrap_commits(commits) include_examples 'raises error with message', GRPC::NotFound, - '' + 'cannot resolve ignore-revs blob' end - context 'when ignore_blob is a directory' do + context 'when ignore_revision_blob is a directory' do let(:ignore_revisions_blob) { "refs/heads/#{revision}:files" } include_examples 'raises error with message', -- GitLab From 1217867de172b35d6621255ceff14125fc2f1a6f Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Fri, 24 Jan 2025 14:15:22 -0600 Subject: [PATCH 7/8] Apply suggestions recommended by reviewer --- spec/lib/gitlab/gitaly_client/commit_service_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index 4f641f8a252608..cde14f6012d234 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1157,7 +1157,7 @@ def wrap_commits(commits) shared_examples 'raises error with message' do |error_class, message| it "raises #{error_class} with correct message" do expect { blame }.to raise_error(error_class) do |error| - expect(error.message).to include(message) + expect(error.details).to eq(message) end end end @@ -1178,8 +1178,7 @@ def wrap_commits(commits) let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } it 'excludes the specified revision from blame' do - original_blame = client.raw_blame(revision, path, range: range).split("\n") - expect(blame).not_to eq(original_blame) + expect(blame).to include(*blame_headers[0..2], blame_headers[4]) expect(blame).not_to include(file_content) end end -- GitLab From 55c97c8f7abad6b88d6d260a45dc17bcf6827b59 Mon Sep 17 00:00:00 2001 From: Pranav Jain Date: Wed, 29 Jan 2025 12:16:56 -0600 Subject: [PATCH 8/8] Apply suggestions requested by reviewer --- spec/lib/gitlab/gitaly_client/commit_service_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb index cde14f6012d234..fdc8913b2009cf 100644 --- a/spec/lib/gitlab/gitaly_client/commit_service_spec.rb +++ b/spec/lib/gitlab/gitaly_client/commit_service_spec.rb @@ -1173,7 +1173,7 @@ def wrap_commits(commits) ) end - context "with valid ignore file" do + context "with valid ignore file content" do let(:file_name) { '.git-ignore-revs-file' } let(:file_content) { '3685515c40444faf92774e72835e1f9c0e809672' } -- GitLab