Merge branch 'fix-collapse-lines' into diff-possible-fixdiff-possible-fix

15 files changed, 178 insertions, 78 deletions

diff --git a/_support/vendor-gitlab-git b/_support/vendor-gitlab-git
index 19f9a6666..7d0a5e78f 100755
--- a/_support/vendor-gitlab-git
+++ b/_support/vendor-gitlab-git
@@ -24,6 +24,7 @@ EXCLUDE = %w[
   lib/gitlab/git/index.rb
   lib/gitlab/git/lfs_changes.rb
   lib/gitlab/git/lfs_pointer_file.rb
+  lib/gitlab/git/merge_base.rb
   lib/gitlab/git/rev_list.rb
   lib/gitlab/git/storage/
   lib/gitlab/git/storage.rb
diff --git a/changelogs/unreleased/fix-collapse-lines.yml b/changelogs/unreleased/fix-collapse-lines.yml
new file mode 100644
index 000000000..87cb3e491
--- /dev/null
+++ b/changelogs/unreleased/fix-collapse-lines.yml
@@ -0,0 +1,5 @@
+---
+title: Fix patch size calculations to not include headers
+merge_request: 859
+author:
+type: fixed
diff --git a/changelogs/unreleased/sh-bump-gitaly-proto-ruby.yml b/changelogs/unreleased/sh-bump-gitaly-proto-ruby.yml
new file mode 100644
index 000000000..f402303a4
--- /dev/null
+++ b/changelogs/unreleased/sh-bump-gitaly-proto-ruby.yml
@@ -0,0 +1,5 @@
+---
+title: Bump gitaly-proto to 0.112.0
+merge_request: 857
+author:
+type: other
diff --git a/changelogs/unreleased/zj-head-lokc.yml b/changelogs/unreleased/zj-head-lokc.yml
new file mode 100644
index 000000000..d2078fadf
--- /dev/null
+++ b/changelogs/unreleased/zj-head-lokc.yml
@@ -0,0 +1,5 @@
+---
+title: Remove stale HEAD.lock if it exists
+merge_request: 861
+author:
+type: fixed
diff --git a/changelogs/unreleased/zj-vendor-gitlab-git-20180815.yml b/changelogs/unreleased/zj-vendor-gitlab-git-20180815.yml
new file mode 100644
index 000000000..a91917fd0
--- /dev/null
+++ b/changelogs/unreleased/zj-vendor-gitlab-git-20180815.yml
@@ -0,0 +1,5 @@
+---
+title: Vendor Gitlab::Git at c87ca832263
+merge_request: 860
+author:
+type: other
diff --git a/internal/diff/diff.go b/internal/diff/diff.go
index 9401fe72e..2334d091e 100644
--- a/internal/diff/diff.go
+++ b/internal/diff/diff.go
@@ -128,7 +128,7 @@ func (parser *Parser) Parse() bool {
 			}
 
 			if len(line) > 0 && len(line) < 10 {
-				parser.consumeChunkLine()
+				parser.consumeChunkLine(true)
 			}
 
 			break
@@ -140,11 +140,11 @@ func (parser *Parser) Parse() bool {
 		if bytes.HasPrefix(line, []byte("diff --git")) {
 			break
 		} else if bytes.HasPrefix(line, []byte("@@")) {
-			parser.consumeChunkLine()
+			parser.consumeChunkLine(false)
 		} else if helper.ByteSliceHasAnyPrefix(line, "---", "+++") && !parser.isParsingChunkLines() {
-			parser.consumeLine(true)
+			parser.consumeLine(false)
 		} else if helper.ByteSliceHasAnyPrefix(line, "-", "+", " ", "\\", "Binary") {
-			parser.consumeChunkLine()
+			parser.consumeChunkLine(true)
 		} else {
 			parser.consumeLine(false)
 		}
@@ -331,7 +331,7 @@ func parseRawLine(line []byte, diff *Diff) error {
 	return nil
 }
 
-func (parser *Parser) consumeChunkLine() {
+func (parser *Parser) consumeChunkLine(updateLineStats bool) {
 	var line []byte
 	var err error
 
@@ -351,9 +351,12 @@ func (parser *Parser) consumeChunkLine() {
 	}
 
 	parser.currentDiff.Patch = append(parser.currentDiff.Patch, line...)
-	parser.currentDiff.lineCount++
-	parser.linesProcessed++
-	parser.bytesProcessed += len(line)
+
+	if updateLineStats {
+		parser.bytesProcessed += len(line)
+		parser.currentDiff.lineCount++
+		parser.linesProcessed++
+	}
 }
 
 func (parser *Parser) consumeLine(updateStats bool) {
diff --git a/internal/diff/diff_test.go b/internal/diff/diff_test.go
index 79928ecb5..a5db182ce 100644
--- a/internal/diff/diff_test.go
+++ b/internal/diff/diff_test.go
@@ -38,12 +38,7 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 		MaxLines:      10000000,
 		CollapseDiffs: true,
 	}
-	diffParser := NewDiffParser(strings.NewReader(rawDiff), limits)
-
-	diffs := []*Diff{}
-	for diffParser.Parse() {
-		diffs = append(diffs, diffParser.Diff())
-	}
+	diffs := getDiffs(rawDiff, limits)
 
 	expectedDiffs := []*Diff{
 		&Diff{
@@ -55,7 +50,7 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 			ToPath:    []byte("big.txt"),
 			Status:    'A',
 			Collapsed: true,
-			lineCount: 100003,
+			lineCount: 100000,
 		},
 		&Diff{
 			OldMode:   0,
@@ -134,9 +129,93 @@ index 0000000000000000000000000000000000000000..3be11c69355948412925fa5e073d76d5
 			Status:    'A',
 			Collapsed: false,
 			Patch:     []byte("@@ -0,0 +1 @@\n+Lorem ipsum\n"),
-			lineCount: 4,
+			lineCount: 1,
+		},
+	}
+
+	require.Equal(t, expectedDiffs, diffs)
+}
+
+func TestDiffParserWithLargeDiffOfSmallPatches(t *testing.T) {
+	patch := "@@ -0,0 +1,5 @@\n" + strings.Repeat("+Lorem\n", 5)
+	rawDiff := `:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-0.txt
+:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-1.txt
+:000000 100644 0000000000000000000000000000000000000000 b6507e5b5ce18077e3ec8aaa2291404e5051d45d A	expand-collapse/file-2.txt
+
+`
+
+	// Create 3 files of 5 lines. The first two files added together surpass
+	// the limits, which should cause the last one to be collpased.
+	for i := 0; i < 3; i++ {
+		rawDiff += fmt.Sprintf(`diff --git a/expand-collapse/file-%d.txt b/expand-collapse/file-%d.txt
+new file mode 100644
+index 0000000000000000000000000000000000000000..b6507e5b5ce18077e3ec8aaa2291404e5051d45d
+--- /dev/null
++++ b/expand-collapse/file-%d.txt
+%s`, i, i, i, patch)
+	}
+
+	limits := Limits{
+		EnforceLimits: true,
+		SafeMaxLines:  10, // This is the one we care about here
+		SafeMaxFiles:  10000000,
+		SafeMaxBytes:  10000000,
+		MaxFiles:      10000000,
+		MaxBytes:      10000000,
+		MaxLines:      10000000,
+		CollapseDiffs: true,
+	}
+	diffs := getDiffs(rawDiff, limits)
+
+	expectedDiffs := []*Diff{
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-0.txt"),
+			ToPath:    []byte("expand-collapse/file-0.txt"),
+			Status:    65,
+			Collapsed: false,
+			Patch:     []byte(patch),
+			lineCount: 5,
+		},
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-1.txt"),
+			ToPath:    []byte("expand-collapse/file-1.txt"),
+			Status:    65,
+			Collapsed: false,
+			Patch:     []byte(patch),
+			lineCount: 5,
+		},
+		&Diff{
+			OldMode:   0,
+			NewMode:   0100644,
+			FromID:    "0000000000000000000000000000000000000000",
+			ToID:      "b6507e5b5ce18077e3ec8aaa2291404e5051d45d",
+			FromPath:  []byte("expand-collapse/file-2.txt"),
+			ToPath:    []byte("expand-collapse/file-2.txt"),
+			Status:    65,
+			Collapsed: true,
+			Patch:     nil,
+			lineCount: 5,
 		},
 	}
 
 	require.Equal(t, expectedDiffs, diffs)
 }
+
+func getDiffs(rawDiff string, limits Limits) []*Diff {
+	diffParser := NewDiffParser(strings.NewReader(rawDiff), limits)
+
+	diffs := []*Diff{}
+	for diffParser.Parse() {
+		diffs = append(diffs, diffParser.Diff())
+	}
+
+	return diffs
+}
diff --git a/internal/service/diff/commit_test.go b/internal/service/diff/commit_test.go
index 551f4e151..17baf59c9 100644
--- a/internal/service/diff/commit_test.go
+++ b/internal/service/diff/commit_test.go
@@ -478,7 +478,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 			request: pb.CommitDiffRequest{
 				EnforceLimits: true,
 				MaxFiles:      5,
-				MaxLines:      100,
+				MaxLines:      90,
 				MaxBytes:      5 * 5 * 1024,
 			},
 			result: []diffAttributes{
@@ -493,7 +493,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				EnforceLimits: true,
 				MaxFiles:      5,
 				MaxLines:      1000,
-				MaxBytes:      7650,
+				MaxBytes:      6900,
 			},
 			result: []diffAttributes{
 				{path: "CHANGELOG"},
@@ -550,7 +550,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				MaxLines:      100,
 				MaxBytes:      5 * 5 * 1024,
 				SafeMaxFiles:  5,
-				SafeMaxLines:  45,
+				SafeMaxLines:  40,
 				SafeMaxBytes:  5 * 5 * 1024,
 			},
 			result: []diffAttributes{
@@ -610,7 +610,7 @@ func TestSuccessfulCommitDiffRequestWithLimits(t *testing.T) {
 				require.Equal(t, requestAndResult.result[i].path, string(diff.FromPath), "path")
 
 				collapsed := requestAndResult.result[i].collapsed
-				require.Equal(t, collapsed, diff.Collapsed, "collapsed")
+				require.Equal(t, collapsed, diff.Collapsed, "%s collapsed", requestAndResult.result[i].path)
 				if collapsed {
 					require.Empty(t, diff.Patch, "patch")
 				}
diff --git a/internal/service/repository/cleanup.go b/internal/service/repository/cleanup.go
index 571cf2059..9f076316c 100644
--- a/internal/service/repository/cleanup.go
+++ b/internal/service/repository/cleanup.go
@@ -15,6 +15,8 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 )
 
+var lockFiles = []string{"config.lock", "HEAD.lock"}
+
 func (server) Cleanup(_ctx context.Context, in *pb.CleanupRequest) (*pb.CleanupResponse, error) {
 	repoPath, err := helper.GetRepoPath(in.GetRepository())
 	if err != nil {
@@ -43,7 +45,7 @@ func cleanupRepo(repoPath string) error {
 	}
 
 	configLockThreshod := time.Now().Add(-15 * time.Minute)
-	if err := cleanupConfigLock(repoPath, configLockThreshod); err != nil {
+	if err := cleanFileLocks(repoPath, configLockThreshod); err != nil {
 		return status.Errorf(codes.Internal, "Cleanup: cleanupConfigLock: %v", err)
 	}
 
@@ -127,20 +129,22 @@ func cleanStaleWorktrees(repoPath string, threshold time.Time) error {
 	return nil
 }
 
-func cleanupConfigLock(repoPath string, threshold time.Time) error {
-	configLockPath := filepath.Join(repoPath, "config.lock")
+func cleanFileLocks(repoPath string, threshold time.Time) error {
+	for _, fileName := range lockFiles {
+		lockPath := filepath.Join(repoPath, fileName)
 
-	fi, err := os.Stat(configLockPath)
-	if err != nil {
-		if os.IsNotExist(err) {
-			return nil
+		fi, err := os.Stat(lockPath)
+		if err != nil {
+			if os.IsNotExist(err) {
+				continue
+			}
+			return err
 		}
-		return err
-	}
 
-	if fi.ModTime().Before(threshold) {
-		if err := os.Remove(configLockPath); err != nil && !os.IsNotExist(err) {
-			return err
+		if fi.ModTime().Before(threshold) {
+			if err := os.Remove(lockPath); err != nil && !os.IsNotExist(err) {
+				return err
+			}
 		}
 	}
 
diff --git a/internal/service/repository/cleanup_test.go b/internal/service/repository/cleanup_test.go
index 6db127de5..0a24cf767 100644
--- a/internal/service/repository/cleanup_test.go
+++ b/internal/service/repository/cleanup_test.go
@@ -186,7 +186,7 @@ func TestCleanupDeletesStaleWorktrees(t *testing.T) {
 	}
 }
 
-func TestCleanupConfigLocks(t *testing.T) {
+func TestCleanupFileLocks(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
 
@@ -200,21 +200,23 @@ func TestCleanupConfigLocks(t *testing.T) {
 	defer cancel()
 
 	req := &pb.CleanupRequest{Repository: testRepo}
-	lockPath := filepath.Join(testRepoPath, "config.lock")
 
-	// No file on the lock path
-	_, err := client.Cleanup(ctx, req)
-	assert.NoError(t, err)
-
-	// Fresh lock should remain
-	createFileWithTimes(lockPath, freshTime)
-	_, err = client.Cleanup(ctx, req)
-	assert.NoError(t, err)
-	assert.FileExists(t, lockPath)
-
-	// Old lock should be removed
-	createFileWithTimes(lockPath, oldTime)
-	_, err = client.Cleanup(ctx, req)
-	assert.NoError(t, err)
-	testhelper.AssertFileNotExists(t, lockPath)
+	for _, fileName := range lockFiles {
+		lockPath := filepath.Join(testRepoPath, fileName)
+		// No file on the lock path
+		_, err := client.Cleanup(ctx, req)
+		assert.NoError(t, err)
+
+		// Fresh lock should remain
+		createFileWithTimes(lockPath, freshTime)
+		_, err = client.Cleanup(ctx, req)
+		assert.NoError(t, err)
+		assert.FileExists(t, lockPath)
+
+		// Old lock should be removed
+		createFileWithTimes(lockPath, oldTime)
+		_, err = client.Cleanup(ctx, req)
+		assert.NoError(t, err)
+		testhelper.AssertFileNotExists(t, lockPath)
+	}
 }
diff --git a/ruby/Gemfile b/ruby/Gemfile
index ad3a4c171..454bade3c 100644
--- a/ruby/Gemfile
+++ b/ruby/Gemfile
@@ -3,7 +3,7 @@ source 'https://rubygems.org'
 gem 'rugged', '~> 0.27.4'
 gem 'github-linguist', '~> 6.1', require: 'linguist'
 gem 'gitlab-markup', '~> 1.6.4'
-gem 'gitaly-proto', '~> 0.107.0', require: 'gitaly'
+gem 'gitaly-proto', '~> 0.112.0', require: 'gitaly'
 gem 'activesupport', '~> 5.0.2'
 gem 'rdoc', '~> 4.2'
 gem 'gitlab-gollum-lib', '~> 4.2', require: false
diff --git a/ruby/Gemfile.lock b/ruby/Gemfile.lock
index 7ee0c4548..540a4cee6 100644
--- a/ruby/Gemfile.lock
+++ b/ruby/Gemfile.lock
@@ -18,7 +18,7 @@ GEM
       multipart-post (>= 1.2, < 3)
     gemojione (3.3.0)
       json
-    gitaly-proto (0.107.0)
+    gitaly-proto (0.112.0)
       google-protobuf (~> 3.1)
       grpc (~> 1.10)
     github-linguist (6.2.0)
@@ -147,7 +147,7 @@ PLATFORMS
 DEPENDENCIES
   activesupport (~> 5.0.2)
   faraday (~> 0.12)
-  gitaly-proto (~> 0.107.0)
+  gitaly-proto (~> 0.112.0)
   github-linguist (~> 6.1)
   gitlab-gollum-lib (~> 4.2)
   gitlab-gollum-rugged_adapter (~> 0.4.4)
diff --git a/ruby/vendor/gitlab_git/REVISION b/ruby/vendor/gitlab_git/REVISION
index 48e55c453..85c7be410 100644
--- a/ruby/vendor/gitlab_git/REVISION
+++ b/ruby/vendor/gitlab_git/REVISION
@@ -1 +1 @@
-2ca8219a20f16636b7a0ffa899a1a04ab8e84782
+c87ca8322636db69b6f097336f163d0373bb415e
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git.rb b/ruby/vendor/gitlab_git/lib/gitlab/git.rb
index 55236a112..2913a3e41 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git.rb
@@ -10,9 +10,11 @@ module Gitlab
     TAG_REF_PREFIX = "refs/tags/".freeze
     BRANCH_REF_PREFIX = "refs/heads/".freeze
 
-    CommandError = Class.new(StandardError)
-    CommitError = Class.new(StandardError)
-    OSError = Class.new(StandardError)
+    BaseError = Class.new(StandardError)
+    CommandError = Class.new(BaseError)
+    CommitError = Class.new(BaseError)
+    OSError = Class.new(BaseError)
+    UnknownRef = Class.new(BaseError)
 
     class << self
       include Gitlab::EncodingHelper
diff --git a/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb b/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
index eb02c7ac8..e9c901f85 100644
--- a/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
+++ b/ruby/vendor/gitlab_git/lib/gitlab/git/repository.rb
@@ -19,6 +19,7 @@ module Gitlab
         GIT_ALTERNATE_OBJECT_DIRECTORIES_RELATIVE
       ].freeze
       SEARCH_CONTEXT_LINES = 3
+      REV_LIST_COMMIT_LIMIT = 2_000
       # In https://gitlab.com/gitlab-org/gitaly/merge_requests/698
       # We copied these two prefixes into gitaly-go, so don't change these
       # or things will break! (REBASE_WORKTREE_PREFIX and SQUASH_WORKTREE_PREFIX)
@@ -39,19 +40,6 @@ module Gitlab
       ChecksumError = Class.new(StandardError)
 
       class << self
-        # Unlike `new`, `create` takes the repository path
-        def create(repo_path, bare: true, symlink_hooks_to: nil)
-          FileUtils.mkdir_p(repo_path, mode: 0770)
-
-          # Equivalent to `git --git-path=#{repo_path} init [--bare]`
-          repo = Rugged::Repository.init_at(repo_path, bare)
-          repo.close
-
-          create_hooks(repo_path, symlink_hooks_to) if symlink_hooks_to.present?
-
-          true
-        end
-
         def create_hooks(repo_path, global_hooks_path)
           local_hooks_path = File.join(repo_path, 'hooks')
           real_local_hooks_path = :not_found
@@ -378,17 +366,18 @@ module Gitlab
         end
       end
 
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/1233
       def new_commits(newrev)
-        gitaly_migrate(:new_commits) do |is_enabled|
-          if is_enabled
-            gitaly_ref_client.list_new_commits(newrev)
-          else
-            refs = Gitlab::GitalyClient::StorageSettings.allow_disk_access do
-              rev_list(including: newrev, excluding: :all).split("\n").map(&:strip)
-            end
+        wrapped_gitaly_errors do
+          gitaly_ref_client.list_new_commits(newrev)
+        end
+      end
+
+      def new_blobs(newrev)
+        return [] if newrev.blank? || newrev == ::Gitlab::Git::BLANK_SHA
 
-            Gitlab::Git::Commit.batch_by_oid(self, refs)
+        strong_memoize("new_blobs_#{newrev}") do
+          wrapped_gitaly_errors do
+            gitaly_ref_client.list_new_blobs(newrev, REV_LIST_COMMIT_LIMIT)
           end
         end
       end