diff options
author | Toon Claes <toon@gitlab.com> | 2022-08-12 08:49:54 +0300 |
---|---|---|
committer | Toon Claes <toon@gitlab.com> | 2022-08-12 08:49:54 +0300 |
commit | 45cad1b3720c6099e8d8fcc207c9f6fc26580796 (patch) | |
tree | 033462bb48997bd02356e19f89e8b5ba3aeb6778 | |
parent | 3e9cc60933881ce8c33c379c72dad3907755aac2 (diff) | |
parent | b55885d92c1ea09b0ac7599d1333ab095ecbc257 (diff) |
Merge branch 'jt-update-git-fsck-config' into 'master'
fsck: Update fsck ignore rules configuration
Closes #4265 and #4404
See merge request gitlab-org/gitaly!4777
-rw-r--r-- | internal/git/command_description.go | 50 | ||||
-rw-r--r-- | internal/git/command_description_test.go | 45 | ||||
-rw-r--r-- | internal/git/command_factory_test.go | 71 |
3 files changed, 151 insertions, 15 deletions
diff --git a/internal/git/command_description.go b/internal/git/command_description.go index a1d2aa90e..1000cd918 100644 --- a/internal/git/command_description.go +++ b/internal/git/command_description.go @@ -58,7 +58,7 @@ var commandDescriptions = map[string]commandDescription{ ConfigPair{Key: "init.templateDir", Value: ""}, // See "fetch" for why we disable following redirects. ConfigPair{Key: "http.followRedirects", Value: "false"}, - }, packConfiguration()...), fsckConfiguration("fetch")...), + }, packConfiguration()...), fetchFsckConfiguration()...), }, "commit": { flags: 0, @@ -118,7 +118,7 @@ var commandDescriptions = map[string]commandDescription{ // of time though because we never populate submodules at all. We thus // disable recursion into submodules. ConfigPair{Key: "fetch.recurseSubmodules", Value: "no"}, - }, fsckConfiguration("fetch")...), packConfiguration()...), + }, fetchFsckConfiguration()...), packConfiguration()...), }, "for-each-ref": { flags: scNoRefUpdates, @@ -128,6 +128,7 @@ var commandDescriptions = map[string]commandDescription{ }, "fsck": { flags: scNoRefUpdates, + opts: fsckConfiguration(), }, "gc": { flags: scNoRefUpdates, @@ -225,7 +226,7 @@ var commandDescriptions = map[string]commandDescription{ // Make git-receive-pack(1) advertise the push options // capability to clients. ConfigPair{Key: "receive.advertisePushOptions", Value: "true"}, - }, hiddenReceivePackRefPrefixes()...), fsckConfiguration("receive")...), packConfiguration()...), + }, hiddenReceivePackRefPrefixes()...), receiveFsckConfiguration()...), packConfiguration()...), }, "remote": { // While git-remote(1)'s `add` subcommand does support `--end-of-options`, @@ -409,38 +410,57 @@ func hiddenUploadPackRefPrefixes() []GlobalOption { return config } -// fsckConfiguration generates our fsck configuration, including ignored checks. The prefix must -// either be "receive" or "fetch" and indicates whether it should apply to git-receive-pack(1) or to -// git-fetch-pack(1). -func fsckConfiguration(prefix string) []GlobalOption { - var configPairs []GlobalOption +// fsckConfiguration generates default fsck options used by git-fsck(1). +func fsckConfiguration() []GlobalOption { + return templateFsckConfiguration("fsck") +} + +// fetchFsckConfiguration generates default fsck options used by git-fetch-pack(1). +func fetchFsckConfiguration() []GlobalOption { + return templateFsckConfiguration("fetch.fsck") +} + +// receiveFsckConfiguration generates default fsck options used by git-receive-pack(1). +func receiveFsckConfiguration() []GlobalOption { + return templateFsckConfiguration("receive.fsck") +} + +// templateFsckConfiguration generates our fsck configuration, including ignored checks. +// The prefix must either be "fsck", "receive.fsck" or "fetch.fsck" and indicates whether +// it should apply to git-fsck(1), git-receive-pack(1) or to git-fetch-pack(1). +func templateFsckConfiguration(prefix string) []GlobalOption { + configPairs := []GlobalOption{ + // When receiving objects from an untrusted source, we want to always assert that + // all objects are valid. When fetch.fsckObjects or receive.fsckObjects are not set, + // the value of transfer.fsckObjects is used instead. Since the fsck configuration + // of git-fetch-pack(1) and git-receive-pack(1) is coupled, transfer.fsckObjects can + // be used for both. + ConfigPair{Key: "transfer.fsckObjects", Value: "true"}, + } + for _, config := range []struct { key string value string }{ - // When receiving objects from an untrusted source, we want to always assert that - // all objects are valid. - {key: "fsckObjects", value: "true"}, - // In the past, there was a bug in git that caused users to create commits with // invalid timezones. As a result, some histories contain commits that do not match // the spec. As we fsck received packfiles by default, any push containing such // a commit will be rejected. As this is a mostly harmless issue, we add the // following flag to ignore this check. - {key: "fsck.badTimezone", value: "ignore"}, + {key: "badTimezone", value: "ignore"}, // git-fsck(1) complains in case a signature does not have a space // between mail and date. The most common case where this can be hit // is in case the date is missing completely. This error is harmless // enough and we cope just fine parsing such signatures, so we can // ignore this error. - {key: "fsck.missingSpaceBeforeDate", value: "ignore"}, + {key: "missingSpaceBeforeDate", value: "ignore"}, // Oldish Git versions used to zero-pad some filemodes, e.g. instead of a // file mode of 40000 the tree object would have encoded the filemode as // 04000. This doesn't cause any and Git can cope with it alright, so let's // ignore it. - {key: "fsck.zeroPaddedFilemode", value: "ignore"}, + {key: "zeroPaddedFilemode", value: "ignore"}, } { configPairs = append(configPairs, ConfigPair{ Key: fmt.Sprintf("%s.%s", prefix, config.key), diff --git a/internal/git/command_description_test.go b/internal/git/command_description_test.go index 574fa5592..f5895492b 100644 --- a/internal/git/command_description_test.go +++ b/internal/git/command_description_test.go @@ -69,3 +69,48 @@ func TestThreadsConfigValue(t *testing.T) { require.Equal(t, tt.threads, actualThreads) } } + +func TestFsckConfiguration_prefix(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + prefix string + expected []ConfigPair + }{ + { + prefix: "fsck", + expected: []ConfigPair{ + {Key: "transfer.fsckObjects", Value: "true"}, + {Key: "fsck.badTimezone", Value: "ignore"}, + {Key: "fsck.missingSpaceBeforeDate", Value: "ignore"}, + {Key: "fsck.zeroPaddedFilemode", Value: "ignore"}, + }, + }, + { + prefix: "fetch.fsck", + expected: []ConfigPair{ + {Key: "transfer.fsckObjects", Value: "true"}, + {Key: "fetch.fsck.badTimezone", Value: "ignore"}, + {Key: "fetch.fsck.missingSpaceBeforeDate", Value: "ignore"}, + {Key: "fetch.fsck.zeroPaddedFilemode", Value: "ignore"}, + }, + }, + { + prefix: "receive.fsck", + expected: []ConfigPair{ + {Key: "transfer.fsckObjects", Value: "true"}, + {Key: "receive.fsck.badTimezone", Value: "ignore"}, + {Key: "receive.fsck.missingSpaceBeforeDate", Value: "ignore"}, + {Key: "receive.fsck.zeroPaddedFilemode", Value: "ignore"}, + }, + }, + } { + t.Run(tc.prefix, func(t *testing.T) { + opts := templateFsckConfiguration(tc.prefix) + + for _, config := range tc.expected { + require.Containsf(t, opts, config, fmt.Sprintf("missing %s", config.Key)) + } + }) + } +} diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go index 5cff2406f..76dccbb7d 100644 --- a/internal/git/command_factory_test.go +++ b/internal/git/command_factory_test.go @@ -589,3 +589,74 @@ func TestExecCommandFactory_SidecarGitConfiguration(t *testing.T) { }) } } + +// TestFsckConfiguration tests the hardcoded configuration of the +// git fsck subcommand generated through the command factory. +func TestFsckConfiguration(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + desc string + data string + }{ + { + desc: "with valid commit", + data: strings.Join([]string{ + "tree " + gittest.DefaultObjectHash.EmptyTreeOID.String(), + "author " + gittest.DefaultCommitterSignature, + "committer " + gittest.DefaultCommitterSignature, + "", + "message", + }, "\n"), + }, + { + desc: "with missing space", + data: strings.Join([]string{ + "tree " + gittest.DefaultObjectHash.EmptyTreeOID.String(), + "author Scrooge McDuck <scrooge@mcduck.com>1659043074 -0500", + "committer Scrooge McDuck <scrooge@mcduck.com>1659975573 -0500", + "", + "message", + }, "\n"), + }, + { + desc: "with bad timezone", + data: strings.Join([]string{ + "tree " + gittest.DefaultObjectHash.EmptyTreeOID.String(), + "author Scrooge McDuck <scrooge@mcduck.com> 1659043074 -0500BAD", + "committer Scrooge McDuck <scrooge@mcduck.com> 1659975573 -0500BAD", + "", + "message", + }, "\n"), + }, + } { + t.Run(tc.desc, func(t *testing.T) { + ctx := testhelper.Context(t) + cfg := testcfg.Build(t) + repoProto, repoPath := gittest.CreateRepository(ctx, t, cfg, + gittest.CreateRepositoryConfig{SkipCreationViaService: true}, + ) + + // Create commit object. + commitOut := gittest.ExecOpts(t, cfg, gittest.ExecConfig{Stdin: bytes.NewBufferString(tc.data)}, + "-C", repoPath, "hash-object", "-w", "-t", "commit", "--stdin", "--literally", + ) + _, err := gittest.DefaultObjectHash.FromHex(text.ChompBytes(commitOut)) + require.NoError(t, err) + + gitCmdFactory, cleanup, err := git.NewExecCommandFactory(cfg) + require.NoError(t, err) + defer cleanup() + + // Create fsck command with configured ignore rules options. + cmd, err := gitCmdFactory.New(ctx, repoProto, + git.SubCmd{Name: "fsck"}, + ) + require.NoError(t, err) + + // Execute git fsck command. + err = cmd.Wait() + require.NoError(t, err) + }) + } +} |