From eb5bd9b8e6e1495affa6cee08faafa8291cd0915 Mon Sep 17 00:00:00 2001 From: Justin Tobler Date: Tue, 9 Aug 2022 18:20:19 -0500 Subject: config: Set autocrlf to false Gitaly should be agnostic as a datastore and not transform line endings. Currently by default `core.autocrlf` is set to `input` which can transform line endings on commit. This change updates the default global configuration of `core.autocrlf` to `false` which stops Gitaly from transforming line endings. --- internal/git/command_factory.go | 9 +++++++- internal/git/command_factory_test.go | 25 ++++++++++++++++++---- internal/git/localrepo/objects_test.go | 4 ++-- .../gitaly/service/operations/commit_files_test.go | 12 +++++------ .../metadata/featureflag/ff_autocrlf_config.go | 8 +++++++ internal/testhelper/testhelper.go | 2 ++ 6 files changed, 47 insertions(+), 13 deletions(-) create mode 100644 internal/metadata/featureflag/ff_autocrlf_config.go diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go index 77ff1976c..ce23c76b0 100644 --- a/internal/git/command_factory.go +++ b/internal/git/command_factory.go @@ -17,6 +17,7 @@ import ( "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config" "gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/storage" "gitlab.com/gitlab-org/gitaly/v15/internal/log" + "gitlab.com/gitlab-org/gitaly/v15/internal/metadata/featureflag" ) // CommandFactory is designed to create and run git commands in a protected and fully managed manner. @@ -497,6 +498,12 @@ func (cf *ExecCommandFactory) globalConfiguration(ctx context.Context) ([]Global return nil, fmt.Errorf("determining Git version: %w", err) } + // Feature flag to change the default global configuration of autocrlf. + autocrlf := "input" + if featureflag.AutocrlfConfig.IsEnabled(ctx) { + autocrlf = "false" + } + // As global options may cancel out each other, we have a clearly defined order in which // globals get applied. The order is similar to how git handles configuration options from // most general to most specific. This allows callsites to override options which would @@ -518,7 +525,7 @@ func (cf *ExecCommandFactory) globalConfiguration(ctx context.Context) ([]Global // when writing blobs to the object database. No conversion is // done when reading blobs from the object database. This is // required for the web editor. - ConfigPair{Key: "core.autocrlf", Value: "input"}, + ConfigPair{Key: "core.autocrlf", Value: autocrlf}, // Git allows the use of replace refs, where a given object ID can be replaced with a // different one. The result is that Git commands would use the new object instead of the diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go index 76dccbb7d..7414f3bbf 100644 --- a/internal/git/command_factory_test.go +++ b/internal/git/command_factory_test.go @@ -4,6 +4,7 @@ package git_test import ( "bytes" + "context" "errors" "fmt" "io" @@ -471,8 +472,10 @@ func TestExecCommandFactory_GitVersion(t *testing.T) { func TestExecCommandFactory_config(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.AutocrlfConfig).Run(t, testExecCommandFactoryConfig) +} - ctx := testhelper.Context(t) +func testExecCommandFactoryConfig(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) // Create a repository and remove its gitconfig to bring us into a known state where there @@ -482,9 +485,15 @@ func TestExecCommandFactory_config(t *testing.T) { }) require.NoError(t, os.Remove(filepath.Join(repoDir, "config"))) + // Feature flag to change the default global configuration of autocrlf. + autocrlf := "input" + if featureflag.AutocrlfConfig.IsEnabled(ctx) { + autocrlf = "false" + } + commonEnv := []string{ "gc.auto=0", - "core.autocrlf=input", + "core.autocrlf=" + autocrlf, "core.usereplacerefs=false", "commitgraph.generationversion=1", } @@ -535,17 +544,25 @@ func TestExecCommandFactory_config(t *testing.T) { func TestExecCommandFactory_SidecarGitConfiguration(t *testing.T) { t.Parallel() + testhelper.NewFeatureSets(featureflag.AutocrlfConfig).Run(t, testExecCommandFactorySidecarGitConfiguration) +} - ctx := testhelper.Context(t) +func testExecCommandFactorySidecarGitConfiguration(t *testing.T, ctx context.Context) { cfg := testcfg.Build(t) cfg.Git.Config = []config.GitConfig{ {Key: "custom.key", Value: "injected"}, } + // Feature flag to change the default global configuration of autocrlf. + autocrlf := "input" + if featureflag.AutocrlfConfig.IsEnabled(ctx) { + autocrlf = "false" + } + commonHead := []git.ConfigPair{ {Key: "gc.auto", Value: "0"}, - {Key: "core.autocrlf", Value: "input"}, + {Key: "core.autocrlf", Value: autocrlf}, {Key: "core.useReplaceRefs", Value: "false"}, {Key: "commitGraph.generationVersion", Value: "1"}, } diff --git a/internal/git/localrepo/objects_test.go b/internal/git/localrepo/objects_test.go index 6bc6056b1..f33a4cf22 100644 --- a/internal/git/localrepo/objects_test.go +++ b/internal/git/localrepo/objects_test.go @@ -56,9 +56,9 @@ func TestRepo_WriteBlob(t *testing.T) { content: "\n", }, { - desc: "CRLF converted to LF due to global git config", + desc: "CRLF not converted to LF due to global git config", input: strings.NewReader("\r\n"), - content: "\n", + content: "\r\n", }, { desc: "line endings preserved in binary files", diff --git a/internal/gitaly/service/operations/commit_files_test.go b/internal/gitaly/service/operations/commit_files_test.go index 1c23a7646..c275a8764 100644 --- a/internal/gitaly/service/operations/commit_files_test.go +++ b/internal/gitaly/service/operations/commit_files_test.go @@ -265,7 +265,7 @@ func TestUserCommitFiles(t *testing.T) { }, }, { - desc: "create file normalizes line endings", + desc: "create file does not normalize line endings", steps: []step{ { actions: []*gitalypb.UserCommitFilesRequest{ @@ -276,7 +276,7 @@ func TestUserCommitFiles(t *testing.T) { repoCreated: true, branchCreated: true, treeEntries: []gittest.TreeEntry{ - {Mode: DefaultMode, Path: "file-1", Content: "content-1\n content-2\n"}, + {Mode: DefaultMode, Path: "file-1", Content: "content-1\r\n content-2\r\n"}, }, }, }, @@ -330,7 +330,7 @@ func TestUserCommitFiles(t *testing.T) { }, }, { - desc: "update file normalizes line endings", + desc: "update file does not normalize line endings", steps: []step{ { actions: []*gitalypb.UserCommitFilesRequest{ @@ -342,7 +342,7 @@ func TestUserCommitFiles(t *testing.T) { repoCreated: true, branchCreated: true, treeEntries: []gittest.TreeEntry{ - {Mode: DefaultMode, Path: "file-1", Content: "content-2\n"}, + {Mode: DefaultMode, Path: "file-1", Content: "content-2\r\n"}, }, }, }, @@ -601,7 +601,7 @@ func TestUserCommitFiles(t *testing.T) { }, }, { - desc: "move file normalizes line endings", + desc: "move file does not normalize line endings", steps: []step{ { actions: []*gitalypb.UserCommitFilesRequest{ @@ -620,7 +620,7 @@ func TestUserCommitFiles(t *testing.T) { actionContentRequest("new-content\r\n"), }, treeEntries: []gittest.TreeEntry{ - {Mode: DefaultMode, Path: "moved-file", Content: "new-content\n"}, + {Mode: DefaultMode, Path: "moved-file", Content: "new-content\r\n"}, }, }, }, diff --git a/internal/metadata/featureflag/ff_autocrlf_config.go b/internal/metadata/featureflag/ff_autocrlf_config.go new file mode 100644 index 000000000..3f464a6c4 --- /dev/null +++ b/internal/metadata/featureflag/ff_autocrlf_config.go @@ -0,0 +1,8 @@ +package featureflag + +var AutocrlfConfig = NewFeatureFlag( + "autocrlf", + "v15.3.0", + "https://gitlab.com/gitlab-org/gitaly/-/issues/4425", + false, +) diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go index f1db352f5..081b4e90a 100644 --- a/internal/testhelper/testhelper.go +++ b/internal/testhelper/testhelper.go @@ -179,6 +179,8 @@ func ContextWithoutCancel(opts ...ContextOpt) context.Context { // PraefectGeneratedReplicaPaths affects many tests as it changes the repository creation logic. // Randomly enable the flag to exercise both paths to some extent. ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.PraefectGeneratedReplicaPaths, rnd.Int()%2 == 0) + // AutocrlfConfig affects the global options configuration. + ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.AutocrlfConfig, true) for _, opt := range opts { ctx = opt(ctx) -- cgit v1.2.3