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

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)
+		})
+	}
+}