Commit 573ce182 authored by Alejandro Rodríguez's avatar Alejandro Rodríguez

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

parents 7043617f 14caa5ca
Pipeline #28081017 failed with stages
in 9 minutes and 25 seconds
......@@ -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
......
---
title: Fix patch size calculations to not include headers
merge_request: 859
author:
type: fixed
---
title: Bump gitaly-proto to 0.112.0
merge_request: 857
author:
type: other
---
title: Remove stale HEAD.lock if it exists
merge_request: 861
author:
type: fixed
---
title: Vendor Gitlab::Git at c87ca832263
merge_request: 860
author:
type: other
......@@ -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) {
......
......@@ -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
}
......@@ -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")
}
......
......@@ -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
}
}
}
......
......@@ -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)
}
}
......@@ -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
......
......@@ -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)
......
2ca8219a20f16636b7a0ffa899a1a04ab8e84782
c87ca8322636db69b6f097336f163d0373bb415e
......@@ -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
......
......@@ -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
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment