Merge branch 'sh-fix-gitaly-bug-2857-for-dirs' into 'master'

Fix directory and last commit not loading for some filenames

See merge request gitlab-org/gitlab!34985

Gemfile

@@ -454,7 +454,7 @@ group :ed25519 do
 end
 
 # Gitaly GRPC protocol definitions
-gem 'gitaly', '~> 13.2.0.pre.rc1'
+gem 'gitaly', '~> 13.2.0.pre.rc2'
 
 gem 'grpc', '~> 1.24.0'
 

Gemfile.lock

@@ -377,7 +377,7 @@ GEM
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
     git (1.5.0)
-    gitaly (13.2.0.pre.rc1)
+    gitaly (13.2.0.pre.rc2)
       grpc (~> 1.0)
     github-markup (1.7.0)
     gitlab-chronic (0.10.5)
@@ -1239,7 +1239,7 @@ DEPENDENCIES
   gettext (~> 3.2.2)
   gettext_i18n_rails (~> 1.8.0)
   gettext_i18n_rails_js (~> 1.3)
-  gitaly (~> 13.2.0.pre.rc1)
+  gitaly (~> 13.2.0.pre.rc2)
   github-markup (~> 1.7.0)
   gitlab-chronic (~> 0.10.5)
   gitlab-labkit (= 0.12.0)

app/controllers/projects/blob_controller.rb

@@ -207,7 +207,7 @@ class Projects::BlobController < Projects::ApplicationController
 
   def set_last_commit_sha
     @last_commit_sha = Gitlab::Git::Commit
-      .last_for_path(@repository, @ref, @path).sha
+      .last_for_path(@repository, @ref, @path, literal_pathspec: true).sha
   end
 
   def show_html

app/graphql/resolvers/last_commit_resolver.rb

@@ -9,7 +9,7 @@ module Resolvers
     def resolve(**args)
       # Ensure merge commits can be returned by sending nil to Gitaly instead of '/'
       path = tree.path == '/' ? nil : tree.path
-      commit = Gitlab::Git::Commit.last_for_path(tree.repository, tree.sha, path)
+      commit = Gitlab::Git::Commit.last_for_path(tree.repository, tree.sha, path, literal_pathspec: true)
 
       ::Commit.new(commit, tree.repository.project) if commit
     end

app/models/repository.rb

@@ -149,7 +149,8 @@ class Repository
       before: opts[:before],
       all: !!opts[:all],
       first_parent: !!opts[:first_parent],
-      order: opts[:order]
+      order: opts[:order],
+      literal_pathspec: opts.fetch(:literal_pathspec, true)
     }
 
     commits = Gitlab::Git::Commit.where(options)
@@ -676,8 +677,8 @@ class Repository
     end
   end
 
-  def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
-    commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
+  def list_last_commits_for_tree(sha, path, offset: 0, limit: 25, literal_pathspec: false)
+    commits = raw_repository.list_last_commits_for_tree(sha, path, offset: offset, limit: limit, literal_pathspec: literal_pathspec)
 
     commits.each do |path, commit|
       commits[path] = ::Commit.new(commit, container)

app/services/files/base_service.rb

@@ -25,7 +25,7 @@ module Files
       return false unless commit_id
 
       last_commit = Gitlab::Git::Commit
-        .last_for_path(@start_project.repository, @start_branch, path)
+        .last_for_path(@start_project.repository, @start_branch, path, literal_pathspec: true)
 
       return false unless last_commit
 

changelogs/unreleased/sh-fix-gitaly-bug-2857-for-dirs.yml

+---
+title: Fix directory and last commit not loading for some filenames
+merge_request: 34985
+author:
+type: fixed

lib/gitlab/git/commit.rb

@@ -90,14 +90,15 @@ module Gitlab
         #
         #   Commit.last_for_path(repo, 'master', 'Gemfile')
         #
-        def last_for_path(repo, ref, path = nil)
+        def last_for_path(repo, ref, path = nil, literal_pathspec: false)
           # rubocop: disable Rails/FindBy
           # This is not where..first from ActiveRecord
           where(
             repo: repo,
             ref: ref,
             path: path,
-            limit: 1
+            limit: 1,
+            literal_pathspec: literal_pathspec
           ).first
           # rubocop: enable Rails/FindBy
         end

lib/gitlab/git/repository.rb

@@ -1002,9 +1002,9 @@ module Gitlab
         end
       end
 
-      def list_last_commits_for_tree(sha, path, offset: 0, limit: 25)
+      def list_last_commits_for_tree(sha, path, offset: 0, limit: 25, literal_pathspec: false)
         wrapped_gitaly_errors do
-          gitaly_commit_client.list_last_commits_for_tree(sha, path, offset: offset, limit: limit)
+          gitaly_commit_client.list_last_commits_for_tree(sha, path, offset: offset, limit: limit, literal_pathspec: literal_pathspec)
         end
       end
 

lib/gitlab/gitaly_client/commit_service.rb

@@ -162,13 +162,14 @@ module Gitlab
         [response.left_count, response.right_count]
       end
 
-      def list_last_commits_for_tree(revision, path, offset: 0, limit: 25)
+      def list_last_commits_for_tree(revision, path, offset: 0, limit: 25, literal_pathspec: false)
         request = Gitaly::ListLastCommitsForTreeRequest.new(
           repository: @gitaly_repo,
           revision: encode_binary(revision),
           path: encode_binary(path.to_s),
           offset: offset,
-          limit: limit
+          limit: limit,
+          global_options: parse_global_options!(literal_pathspec: literal_pathspec)
         )
 
         response = GitalyClient.call(@repository.storage, :commit_service, :list_last_commits_for_tree, request, timeout: GitalyClient.medium_timeout)
@@ -185,7 +186,7 @@ module Gitlab
           repository: @gitaly_repo,
           revision: encode_binary(revision),
           path: encode_binary(path.to_s),
-          literal_pathspec: literal_pathspec
+          global_options: parse_global_options!(literal_pathspec: literal_pathspec)
         )
 
         gitaly_commit = GitalyClient.call(@repository.storage, :commit_service, :last_commit_for_path, request, timeout: GitalyClient.fast_timeout).commit
@@ -244,14 +245,15 @@ module Gitlab
         []
       end
 
-      def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0)
+      def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0, literal_pathspec: true)
         request = Gitaly::CommitsByMessageRequest.new(
           repository: @gitaly_repo,
           query: query,
           revision: encode_binary(revision),
           path: encode_binary(path),
           limit: limit.to_i,
-          offset: offset.to_i
+          offset: offset.to_i,
+          global_options: parse_global_options!(literal_pathspec: literal_pathspec)
         )
 
         GitalyClient.streaming_call(@repository.storage, :commit_service, :commits_by_message, request, timeout: GitalyClient.medium_timeout) do |response|
@@ -321,6 +323,7 @@ module Gitlab
           skip_merges:  options[:skip_merges],
           all:          !!options[:all],
           first_parent: !!options[:first_parent],
+          global_options: parse_global_options!(options),
           disable_walk: true # This option is deprecated. The 'walk' implementation is being removed.
         )
         request.after    = GitalyClient.timestamp(options[:after]) if options[:after]
@@ -408,6 +411,11 @@ module Gitlab
 
       private
 
+      def parse_global_options!(options)
+        literal_pathspec = options.delete(:literal_pathspec)
+        Gitaly::GlobalOptions.new(literal_pathspecs: literal_pathspec)
+      end
+
       def call_commit_diff(request_params, options = {})
         request_params[:ignore_whitespace_change] = options.fetch(:ignore_whitespace_change, false)
         request_params[:enforce_limits] = options.fetch(:limits, true)

lib/gitlab/tree_summary.rb

@@ -97,7 +97,7 @@ module Gitlab
           File.join(*[path, ""])
         end
 
-      commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit)
+      commits_hsh = repository.list_last_commits_for_tree(commit.id, ensured_path, offset: offset, limit: limit, literal_pathspec: true)
       prerender_commit_full_titles!(commits_hsh.values)
 
       entries.each do |entry|

spec/features/projects/commits/user_browses_commits_spec.rb

@@ -137,6 +137,33 @@ RSpec.describe 'User browses commits' do
         .and have_selector('entry summary', text: commit.description[0..10].delete("\r\n"))
     end
 
+    context "when commit has a filename with pathspec characters" do
+      let(:path) { ':wq' }
+      let(:filename) { File.join(path, 'test.txt') }
+      let(:ref) { project.repository.root_ref }
+      let(:newrev) { project.repository.commit('master').sha }
+      let(:short_newrev) { project.repository.commit('master').short_id }
+      let(:message) { 'Glob characters'}
+
+      before do
+        create_file_in_repo(project, ref, ref, filename, 'Test file', commit_message: message)
+        visit project_commits_path(project, "#{ref}/#{path}", limit: 1)
+        wait_for_requests
+      end
+
+      it 'searches commit', :js do
+        expect(page).to have_content(message)
+
+        fill_in 'commits-search', with: 'bogus12345'
+
+        expect(page).to have_content "Your search didn't match any commits"
+
+        fill_in 'commits-search', with: 'Glob'
+
+        expect(page).to have_content message
+      end
+    end
+
     context 'when a commit links to a confidential issue' do
       let(:confidential_issue) { create(:issue, confidential: true, title: 'Secret issue!', project: project) }
 

spec/features/projects/files/user_browses_files_spec.rb

@@ -341,7 +341,7 @@ RSpec.describe "User browses files" do
     end
   end
 
-  context "when browsing a file with glob characters" do
+  context "when browsing a file with pathspec characters" do
     let(:filename) { ':wq' }
     let(:newrev) { project.repository.commit('master').sha }
 

spec/features/projects/tree/tree_show_spec.rb

@@ -49,10 +49,11 @@ RSpec.describe 'Projects tree', :js do
     expect(page).not_to have_selector('.flash-alert')
   end
 
-  context "with a tree that contains glob characters" do
+  context "with a tree that contains pathspec characters" do
     let(:path) { ':wq' }
     let(:filename) { File.join(path, 'test.txt') }
     let(:newrev) { project.repository.commit('master').sha }
+    let(:short_newrev) { project.repository.commit('master').short_id }
     let(:message) { 'Glob characters'}
 
     before do
@@ -61,11 +62,14 @@ RSpec.describe 'Projects tree', :js do
       wait_for_requests
     end
 
-    # Disabled until https://gitlab.com/gitlab-org/gitaly/-/issues/2888 is resolved
-    xit "renders tree table without errors" do
+    it "renders tree table without errors" do
       expect(page).to have_selector('.tree-item')
       expect(page).to have_content('test.txt')
       expect(page).to have_content(message)
+
+      # Check last commit
+      expect(find('.commit-content').text).to include(message)
+      expect(find('.commit-sha-group').text).to eq(short_newrev)
     end
   end
 

spec/graphql/resolvers/last_commit_resolver_spec.rb

@@ -4,8 +4,10 @@ require 'spec_helper'
 
 RSpec.describe Resolvers::LastCommitResolver do
   include GraphqlHelpers
+  include RepoHelpers
 
-  let(:repository) { create(:project, :repository).repository }
+  let(:project) { create(:project, :repository) }
+  let(:repository) { project.repository }
   let(:tree) { repository.tree(ref, path) }
 
   let(:commit) { resolve(described_class, obj: tree) }
@@ -29,6 +31,28 @@ RSpec.describe Resolvers::LastCommitResolver do
       end
     end
 
+    context 'last commit for a wildcard pathspec' do
+      let(:ref) { 'fix' }
+      let(:path) { 'files/*' }
+
+      it 'returns nil' do
+        expect(commit).to be_nil
+      end
+    end
+
+    context 'last commit with pathspec characters' do
+      let(:ref) { 'fix' }
+      let(:path) { ':wq' }
+
+      before do
+        create_file_in_repo(project, ref, ref, path, 'Test file')
+      end
+
+      it 'resolves commit' do
+        expect(commit).to eq(repository.commits(ref, path: path, limit: 1).last)
+      end
+    end
+
     context 'last commit does not exist' do
       let(:ref) { 'master' }
       let(:path) { 'does-not-exist' }

spec/lib/gitlab/git/commit_spec.rb

@@ -227,6 +227,34 @@ RSpec.describe Gitlab::Git::Commit, :seed_helper do
         end
       end
 
+      context 'pathspec' do
+        let(:pathspec) { 'files/ruby/*' }
+
+        context 'with default literal_pathspec value' do
+          it 'finds the seed commit' do
+            commit = described_class.last_for_path(repository, 'master', pathspec)
+
+            expect(commit.id).to eq(SeedRepo::Commit::ID)
+          end
+        end
+
+        context 'with literal_pathspec set to false' do
+          it 'finds the seed commit' do
+            commit = described_class.last_for_path(repository, 'master', pathspec, literal_pathspec: false)
+
+            expect(commit.id).to eq(SeedRepo::Commit::ID)
+          end
+        end
+
+        context 'with literal_pathspec set to true' do
+          it 'does not find the seed commit' do
+            commit = described_class.last_for_path(repository, 'master', pathspec, literal_pathspec: true)
+
+            expect(commit).to be_nil
+          end
+        end
+      end
+
       context 'ref + path' do
         subject { described_class.last_for_path(repository, SeedRepo::Commit::ID, 'encoding') }
 

spec/lib/gitlab/gitaly_client/commit_service_spec.rb

@@ -290,7 +290,8 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
       request = Gitaly::FindCommitsRequest.new(
         repository: repository_message,
         disable_walk: true,
-        order: 'NONE'
+        order: 'NONE',
+        global_options: Gitaly::GlobalOptions.new(literal_pathspecs: false)
       )
 
       expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commits)
@@ -303,7 +304,8 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
       request = Gitaly::FindCommitsRequest.new(
         repository: repository_message,
         disable_walk: true,
-        order: 'TOPO'
+        order: 'TOPO',
+        global_options: Gitaly::GlobalOptions.new(literal_pathspecs: false)
       )
 
       expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commits)
@@ -317,7 +319,8 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
         repository: repository_message,
         disable_walk: true,
         order: 'NONE',
-        author: "Billy Baggins <bilbo@shire.com>"
+        author: "Billy Baggins <bilbo@shire.com>",
+        global_options: Gitaly::GlobalOptions.new(literal_pathspecs: false)
       )
 
       expect_any_instance_of(Gitaly::CommitService::Stub).to receive(:find_commits)
@@ -338,7 +341,8 @@ RSpec.describe Gitlab::GitalyClient::CommitService do
           revision: (options[:revision] || '').dup.force_encoding(Encoding::ASCII_8BIT),
           path: (options[:path] || '').dup.force_encoding(Encoding::ASCII_8BIT),
           limit: (options[:limit] || 1000).to_i,
-          offset: (options[:offset] || 0).to_i
+          offset: (options[:offset] || 0).to_i,
+          global_options: Gitaly::GlobalOptions.new(literal_pathspecs: true)
         )
 
         allow_any_instance_of(Gitaly::CommitService::Stub)

spec/lib/gitlab/tree_summary_spec.rb

@@ -47,15 +47,17 @@ RSpec.describe Gitlab::TreeSummary do
   end
 
   describe '#summarize (entries)' do
-    let(:limit) { 2 }
+    let(:limit) { 4 }
 
     custom_files = {
       'a.txt' => '',
       'b.txt' => '',
-      'directory/c.txt' => ''
+      'directory/c.txt' => '',
+      ':dir/test.txt' => '',
+      ':file' => ''
     }
 
-    let(:project) { create(:project, :custom_repo, files: custom_files) }
+    let!(:project) { create(:project, :custom_repo, files: custom_files) }
     let(:commit) { repo.head_commit }
 
     subject(:entries) { summary.summarize.first }
@@ -63,13 +65,16 @@ RSpec.describe Gitlab::TreeSummary do
     it 'summarizes the entries within the window' do
       is_expected.to contain_exactly(
         a_hash_including(type: :tree, file_name: 'directory'),
-        a_hash_including(type: :blob, file_name: 'a.txt')
+        a_hash_including(type: :blob, file_name: 'a.txt'),
+        a_hash_including(type: :blob, file_name: ':file'),
+        a_hash_including(type: :tree, file_name: ':dir')
         # b.txt is excluded by the limit
       )
     end
 
     it 'references the commit and commit path in entries' do
-      entry = entries.first
+      # There are 2 trees and the summary is not ordered
+      entry = entries.find { |entry| entry[:commit].id == commit.id }
       expected_commit_path = Gitlab::Routing.url_helpers.project_commit_path(project, commit)
 
       expect(entry[:commit]).to be_a(::Commit)
@@ -85,6 +90,14 @@ RSpec.describe Gitlab::TreeSummary do
       end
     end
 
+    context 'in a subdirectory with a pathspec character' do
+      let(:path) { ':dir' }
+
+      it 'summarizes the entries in the subdirectory' do
+        is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'test.txt'))
+      end
+    end
+
     context 'in a non-existent subdirectory' do
       let(:path) { 'tmp' }
 
@@ -92,7 +105,7 @@ RSpec.describe Gitlab::TreeSummary do
     end
 
     context 'custom offset and limit' do
-      let(:offset) { 2 }
+      let(:offset) { 4 }
 
       it 'returns entries from the offset' do
         is_expected.to contain_exactly(a_hash_including(type: :blob, file_name: 'b.txt'))

spec/models/repository_spec.rb

@@ -253,7 +253,7 @@ RSpec.describe Repository do
       end
     end
 
-    context 'with filename with glob characters' do
+    context 'with filename with pathspec characters' do
       let(:filename) { ':wq' }
       let(:newrev) { project.repository.commit('master').sha }
 
@@ -292,7 +292,7 @@ RSpec.describe Repository do
       end
     end
 
-    context 'with filename with glob characters' do
+    context 'with filename with pathspec characters' do
       let(:filename) { ':wq' }
       let(:newrev) { project.repository.commit('master').sha }
 

spec/requests/api/files_spec.rb

@@ -185,7 +185,7 @@ RSpec.describe API::Files do
         expect(response.content_type).to eq('application/json')
       end
 
-      context 'with filename with glob characters' do
+      context 'with filename with pathspec characters' do
         let(:file_path) { ':wq' }
         let(:newrev) { project.repository.commit('master').sha }