Merge branch 'pks-housekeeping-fix-config-cleanup' into 'master'

housekeeping: Improve cleanup of the gitconfig

See merge request gitlab-org/gitaly!4284

2 files changed, 315 insertions, 34 deletions

diff --git a/internal/git/housekeeping/housekeeping.go b/internal/git/housekeeping/housekeeping.go
index aedc6cd5e..d157fb08d 100644
--- a/internal/git/housekeeping/housekeeping.go
+++ b/internal/git/housekeeping/housekeeping.go
@@ -1,6 +1,7 @@
 package housekeeping
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"fmt"
@@ -16,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/safe"
 	"google.golang.org/grpc/codes"
 )
 
@@ -107,16 +109,121 @@ func Perform(ctx context.Context, repo *localrepo.Repo, txManager transaction.Ma
 
 	// TODO: https://gitlab.com/gitlab-org/gitaly/-/issues/3987
 	// This is a temporary code and needs to be removed once it will be run on all repositories at least once.
-	unnecessaryConfigRegex := "^(http\\..+\\.extraheader|remote\\..+\\.(fetch|mirror|prunes|url)|core\\.(commitgraph|sparsecheckout|splitindex))$"
+	unnecessaryConfigRegex := "^(http\\..+\\.extraheader|remote\\..+\\.(fetch|mirror|prune|url)|core\\.(commitgraph|sparsecheckout|splitindex))$"
 	if err := repo.UnsetMatchingConfig(ctx, unnecessaryConfigRegex, txManager); err != nil {
 		if !errors.Is(err, git.ErrNotFound) {
 			return fmt.Errorf("housekeeping could not unset unnecessary config lines: %w", err)
 		}
 	}
 
+	if err := pruneEmptyConfigSections(ctx, repo); err != nil {
+		return fmt.Errorf("failed pruning empty sections: %w", err)
+	}
+
 	return nil
 }
 
+// pruneEmptyConfigSections prunes all empty sections from the repo's config.
+func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (returnedErr error) {
+	repoPath, err := repo.Path()
+	if err != nil {
+		return fmt.Errorf("getting repo path: %w", err)
+	}
+	configPath := filepath.Join(repoPath, "config")
+
+	// The gitconfig shouldn't ever be big given that we nowadays don't write any unbounded
+	// values into it anymore. Slurping it into memory should thus be fine.
+	configContents, err := os.ReadFile(configPath)
+	if err != nil {
+		return fmt.Errorf("reading config: %w", err)
+	}
+	configLines := strings.SplitAfter(string(configContents), "\n")
+	if configLines[len(configLines)-1] == "" {
+		// Strip the last line if it's empty.
+		configLines = configLines[:len(configLines)-1]
+	}
+
+	// We now filter out any empty sections. A section is empty if the next line is a section
+	// header as well, or if it is the last line in the gitconfig. This isn't quite the whole
+	// story given that a section can also be empty if it just ain't got any keys, but only
+	// comments or whitespace. But we only ever write the gitconfig programmatically, so we
+	// shouldn't typically see any such cases at all.
+	filteredLines := make([]string, 0, len(configLines))
+	for i := 0; i < len(configLines)-1; i++ {
+		// Skip if we have two consecutive section headers.
+		if isSectionHeader(configLines[i]) && isSectionHeader(configLines[i+1]) {
+			continue
+		}
+		filteredLines = append(filteredLines, configLines[i])
+	}
+	// The final line is always stripped in case it is a section header.
+	if len(configLines) > 0 && !isSectionHeader(configLines[len(configLines)-1]) {
+		filteredLines = append(filteredLines, configLines[len(configLines)-1])
+	}
+
+	// If we haven't filtered out anything then there is no need to update the target file.
+	if len(configLines) == len(filteredLines) {
+		return
+	}
+
+	// Otherwise, we need to update the repository's configuration.
+	configWriter, err := safe.NewLockingFileWriter(configPath)
+	if err != nil {
+		return fmt.Errorf("creating config configWriter: %w", err)
+	}
+	defer func() {
+		if err := configWriter.Close(); err != nil && returnedErr == nil {
+			returnedErr = fmt.Errorf("closing config writer: %w", err)
+		}
+	}()
+
+	for _, filteredLine := range filteredLines {
+		if _, err := configWriter.Write([]byte(filteredLine)); err != nil {
+			return fmt.Errorf("writing filtered config: %w", err)
+		}
+	}
+
+	// This is a sanity check to assert that we really didn't change anything as seen by
+	// Git. We run `git config -l` on both old and new file and assert that they report
+	// the same config entries. Because empty sections are never reported we shouldn't
+	// see those, and as a result any difference in output is a difference we need to
+	// worry about.
+	var configOutputs []string
+	for _, path := range []string{configPath, configWriter.Path()} {
+		var configOutput bytes.Buffer
+		if err := repo.ExecAndWait(ctx, git.SubCmd{
+			Name: "config",
+			Flags: []git.Option{
+				git.ValueFlag{Name: "-f", Value: path},
+				git.Flag{Name: "-l"},
+			},
+		}, git.WithStdout(&configOutput)); err != nil {
+			return fmt.Errorf("listing config: %w", err)
+		}
+
+		configOutputs = append(configOutputs, configOutput.String())
+	}
+	if configOutputs[0] != configOutputs[1] {
+		return fmt.Errorf("section pruning has caused config change")
+	}
+
+	// We don't use transactional voting but commit the file directly -- we have asserted that
+	// the change is idempotent anyway.
+	if err := configWriter.Lock(); err != nil {
+		return fmt.Errorf("failed locking config: %w", err)
+	}
+	if err := configWriter.Commit(); err != nil {
+		return fmt.Errorf("failed committing pruned config: %w", err)
+	}
+
+	return nil
+}
+
+func isSectionHeader(line string) bool {
+	line = strings.TrimSpace(line)
+	return strings.HasPrefix(line, "[") && strings.HasSuffix(line, "]")
+}
+
 // findStaleFiles determines whether any of the given files rooted at repoPath
 // are stale or not. A file is considered stale if it exists and if it has not
 // been modified during the gracePeriod. A nonexistent file is not considered
diff --git a/internal/git/housekeeping/housekeeping_test.go b/internal/git/housekeeping/housekeeping_test.go
index 88b00ef7e..00c794bf1 100644
--- a/internal/git/housekeeping/housekeeping_test.go
+++ b/internal/git/housekeeping/housekeeping_test.go
@@ -6,7 +6,6 @@ import (
 	"os"
 	"path/filepath"
 	"runtime"
-	"strings"
 	"testing"
 	"time"
 
@@ -657,42 +656,48 @@ func TestPerform_UnsetConfiguration(t *testing.T) {
 	cfg := testcfg.Build(t)
 	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
-	ctx := testhelper.Context(t)
+	configPath := filepath.Join(repoPath, "config")
 
-	var expectedEntries []string
-	for key, value := range map[string]string{
-		"core.commitgraph":        "true",
-		"core.sparsecheckout":     "true",
-		"core.splitindex":         "false",
-		"remote.first.fetch":      "baz",
-		"remote.first.mirror":     "baz",
-		"remote.first.prune":      "baz",
-		"remote.first.url":        "baz",
-		"http.first.extraHeader":  "barfoo",
-		"http.second.extraHeader": "barfoo",
-		"http.extraHeader":        "untouched",
-		"http.something.else":     "untouched",
-		"totally.unrelated":       "untouched",
-	} {
-		gittest.Exec(t, cfg, "-C", repoPath, "config", key, value)
-		expectedEntries = append(expectedEntries, strings.ToLower(key)+"="+value)
-	}
+	ctx := testhelper.Context(t)
 
-	preimageConfig := gittest.Exec(t, cfg, "-C", repoPath, "config", "--local", "--list")
-	require.Subset(t, strings.Split(string(preimageConfig), "\n"), expectedEntries)
+	require.NoError(t, os.WriteFile(configPath, []byte(
+		`[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = true
+	commitGraph = true
+	sparseCheckout = true
+	splitIndex = false
+[remote "first"]
+	fetch = baz
+	mirror = baz
+	prune = baz
+	url = baz
+[http "first"]
+	extraHeader = barfoo
+[http "second"]
+	extraHeader = barfoo
+[http]
+	extraHeader = untouched
+[http "something"]
+	else = untouched
+[totally]
+	unrelated = untouched
+`), 0o644))
 
 	require.NoError(t, Perform(ctx, repo, nil))
-
-	postimageConfig := gittest.Exec(t, cfg, "-C", repoPath, "config", "--local", "--list")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "core.commitgraph")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "core.sparsecheckout")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "core.splitindex")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "remote.first.fetch")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "remote.first.mirror")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "remote.first.prune")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "remote.first.url")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "http.first.extraheader")
-	require.NotContains(t, strings.Split(string(postimageConfig), "\n"), "http.second.extraheader")
+	require.Equal(t,
+		`[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = true
+[http]
+	extraHeader = untouched
+[http "something"]
+	else = untouched
+[totally]
+	unrelated = untouched
+`, string(testhelper.MustReadFile(t, configPath)))
 }
 
 func TestPerform_UnsetConfiguration_transactional(t *testing.T) {
@@ -725,3 +730,172 @@ func TestPerform_UnsetConfiguration_transactional(t *testing.T) {
 	}
 	require.Equal(t, expectedConfig, string(configKeys))
 }
+
+func TestPerform_cleanupConfig(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg := testcfg.Build(t)
+	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	configPath := filepath.Join(repoPath, "config")
+
+	require.NoError(t, os.WriteFile(configPath, []byte(
+		`[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = true
+[uploadpack]
+	allowAnySHA1InWant = true
+[remote "tmp-8be1695862b62390d1f873f9164122e4"]
+[remote "tmp-d97f78c39fde4b55e0d0771dfc0501ef"]
+[remote "tmp-23a2471e7084e1548ef47bbc9d6afff6"]
+[remote "tmp-d76633a16d61f6681de396ec9ecfd7b5"]
+	prune = true
+[remote "tmp-8fbf8d5e7585d48668f1791284a912ef"]
+[remote "tmp-f539c59068f291e52f1140e39830f9ca"]
+[remote "tmp-17b67d28909768db3213917255c72af2"]
+	prune = true
+[remote "tmp-03b5e8c765135b343214d471843a062a"]
+[remote "tmp-f57338181aca1d599669dbb71ce9ce57"]
+[remote "tmp-8c948ca94832c2725733e48cb2902287"]
+`), 0o644))
+
+	require.NoError(t, Perform(ctx, repo, nil))
+	require.Equal(t, `[core]
+	repositoryformatversion = 0
+	filemode = true
+	bare = true
+[uploadpack]
+	allowAnySHA1InWant = true
+`, string(testhelper.MustReadFile(t, configPath)))
+}
+
+func TestPruneEmptyConfigSections(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+
+	cfg := testcfg.Build(t)
+	repoProto, repoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	repo := localrepo.NewTestRepo(t, cfg, repoProto)
+	configPath := filepath.Join(repoPath, "config")
+
+	for _, tc := range []struct {
+		desc         string
+		configData   string
+		expectedData string
+	}{
+		{
+			desc:         "empty",
+			configData:   "",
+			expectedData: "",
+		},
+		{
+			desc:         "newline only",
+			configData:   "\n",
+			expectedData: "\n",
+		},
+		{
+			desc:         "no stripping",
+			configData:   "[foo]\nbar = baz\n",
+			expectedData: "[foo]\nbar = baz\n",
+		},
+		{
+			desc:         "no stripping with missing newline",
+			configData:   "[foo]\nbar = baz",
+			expectedData: "[foo]\nbar = baz",
+		},
+		{
+			desc:         "multiple sections",
+			configData:   "[foo]\nbar = baz\n[bar]\nfoo = baz\n",
+			expectedData: "[foo]\nbar = baz\n[bar]\nfoo = baz\n",
+		},
+		{
+			desc:         "missing newline",
+			configData:   "[foo]\nbar = baz",
+			expectedData: "[foo]\nbar = baz",
+		},
+		{
+			desc:         "single comment",
+			configData:   "# foobar\n",
+			expectedData: "# foobar\n",
+		},
+		{
+			// This is not correct, but we really don't want to start parsing
+			// the config format completely. So we err on the side of caution
+			// and just say this is fine.
+			desc:         "empty section with comment",
+			configData:   "[foo]\n# comment\n[bar]\n[baz]\n",
+			expectedData: "[foo]\n# comment\n",
+		},
+		{
+			desc:         "empty section",
+			configData:   "[foo]\n",
+			expectedData: "",
+		},
+		{
+			desc:         "empty sections",
+			configData:   "[foo]\n[bar]\n[baz]\n",
+			expectedData: "",
+		},
+		{
+			desc:         "empty sections with missing newline",
+			configData:   "[foo]\n[bar]\n[baz]",
+			expectedData: "",
+		},
+		{
+			desc:         "trailing empty section",
+			configData:   "[foo]\nbar = baz\n[foo]\n",
+			expectedData: "[foo]\nbar = baz\n",
+		},
+		{
+			desc:         "mixed keys and sections",
+			configData:   "[empty]\n[nonempty]\nbar = baz\nbar = baz\n[empty]\n",
+			expectedData: "[nonempty]\nbar = baz\nbar = baz\n",
+		},
+		{
+			desc: "real world example",
+			configData: `[core]
+        repositoryformatversion = 0
+        filemode = true
+        bare = true
+[uploadpack]
+        allowAnySHA1InWant = true
+[remote "tmp-8be1695862b62390d1f873f9164122e4"]
+[remote "tmp-d97f78c39fde4b55e0d0771dfc0501ef"]
+[remote "tmp-23a2471e7084e1548ef47bbc9d6afff6"]
+[remote "tmp-6ef9759bb14db34ca67de4681f0a812a"]
+[remote "tmp-992cb6a0ea428a511cc2de3cde051227"]
+[remote "tmp-a720c2b6794fdbad50f36f0a4e9501ff"]
+[remote "tmp-4b4f6d68031aa1288613f40b1a433278"]
+[remote "tmp-fc12da796c907e8ea5faed134806acfb"]
+[remote "tmp-49e1fbb6eccdb89059a7231eef785d03"]
+[remote "tmp-e504bbbed5d828cd96b228abdef4b055"]
+[remote "tmp-36e856371fdacb7b4909240ba6bc0b34"]
+[remote "tmp-9a1bc23bb2200b9426340a5ba934f5ba"]
+[remote "tmp-49ead30f732995498e0585b569917c31"]
+[remote "tmp-8419f1e1445ccd6e1c60aa421573447c"]
+[remote "tmp-f7a91ec9415f984d3747cf608b0a7e9c"]
+        prune = true
+[remote "tmp-ea77d1e5348d07d693aa2bf8a2c98637"]
+[remote "tmp-3f190ab463b804612cb007487e0cbb4d"]`,
+			expectedData: `[core]
+        repositoryformatversion = 0
+        filemode = true
+        bare = true
+[uploadpack]
+        allowAnySHA1InWant = true
+[remote "tmp-f7a91ec9415f984d3747cf608b0a7e9c"]
+        prune = true
+`,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			require.NoError(t, os.WriteFile(configPath, []byte(tc.configData), 0o644))
+			require.NoError(t, pruneEmptyConfigSections(ctx, repo))
+			require.Equal(t, tc.expectedData, string(testhelper.MustReadFile(t, configPath)))
+		})
+	}
+}