housekeeping: Report pruned empty config sections

We do not report any empty config sections, which makes us blind to
whether this code is still in use. Add this reporting so that we can
tell whether this section is needed at all anymore.

2 files changed, 48 insertions, 29 deletions

diff --git a/internal/git/housekeeping/clean_stale_data.go b/internal/git/housekeeping/clean_stale_data.go
index 7c1dc08ac..ee1699151 100644
--- a/internal/git/housekeeping/clean_stale_data.go
+++ b/internal/git/housekeeping/clean_stale_data.go
@@ -115,7 +115,9 @@ func (m *RepositoryManager) CleanStaleData(ctx context.Context, repo *localrepo.
 		staleDataByType["configkeys"] = 1
 	}
 
-	if err := pruneEmptyConfigSections(ctx, repo); err != nil {
+	skippedSections, err := pruneEmptyConfigSections(ctx, repo)
+	staleDataByType["configsections"] = skippedSections
+	if err != nil {
 		return fmt.Errorf("failed pruning empty sections: %w", err)
 	}
 
@@ -123,10 +125,10 @@ func (m *RepositoryManager) CleanStaleData(ctx context.Context, repo *localrepo.
 }
 
 // pruneEmptyConfigSections prunes all empty sections from the repo's config.
-func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (returnedErr error) {
+func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (_ int, returnedErr error) {
 	repoPath, err := repo.Path()
 	if err != nil {
-		return fmt.Errorf("getting repo path: %w", err)
+		return 0, fmt.Errorf("getting repo path: %w", err)
 	}
 	configPath := filepath.Join(repoPath, "config")
 
@@ -134,7 +136,7 @@ func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (return
 	// 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)
+		return 0, fmt.Errorf("reading config: %w", err)
 	}
 	configLines := strings.SplitAfter(string(configContents), "\n")
 	if configLines[len(configLines)-1] == "" {
@@ -142,6 +144,8 @@ func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (return
 		configLines = configLines[:len(configLines)-1]
 	}
 
+	skippedSections := 0
+
 	// 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
@@ -151,24 +155,26 @@ func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (return
 	for i := 0; i < len(configLines)-1; i++ {
 		// Skip if we have two consecutive section headers.
 		if isSectionHeader(configLines[i]) && isSectionHeader(configLines[i+1]) {
+			skippedSections++
 			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]) {
+		skippedSections++
 		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
+		return 0, nil
 	}
 
 	// 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)
+		return 0, fmt.Errorf("creating config configWriter: %w", err)
 	}
 	defer func() {
 		if err := configWriter.Close(); err != nil && returnedErr == nil {
@@ -178,7 +184,7 @@ func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (return
 
 	for _, filteredLine := range filteredLines {
 		if _, err := configWriter.Write([]byte(filteredLine)); err != nil {
-			return fmt.Errorf("writing filtered config: %w", err)
+			return 0, fmt.Errorf("writing filtered config: %w", err)
 		}
 	}
 
@@ -197,25 +203,25 @@ func pruneEmptyConfigSections(ctx context.Context, repo *localrepo.Repo) (return
 				git.Flag{Name: "-l"},
 			},
 		}, git.WithStdout(&configOutput)); err != nil {
-			return fmt.Errorf("listing config: %w", err)
+			return 0, 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")
+		return 0, 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)
+		return 0, fmt.Errorf("failed locking config: %w", err)
 	}
 	if err := configWriter.Commit(); err != nil {
-		return fmt.Errorf("failed committing pruned config: %w", err)
+		return 0, fmt.Errorf("failed committing pruned config: %w", err)
 	}
 
-	return nil
+	return skippedSections, nil
 }
 
 func isSectionHeader(line string) bool {
diff --git a/internal/git/housekeeping/clean_stale_data_test.go b/internal/git/housekeeping/clean_stale_data_test.go
index 758d87b41..44a0edc1f 100644
--- a/internal/git/housekeeping/clean_stale_data_test.go
+++ b/internal/git/housekeeping/clean_stale_data_test.go
@@ -133,6 +133,7 @@ func d(name string, mode os.FileMode, age time.Duration, finalState entryFinalSt
 
 type cleanStaleDataMetrics struct {
 	configkeys     int
+	configsections int
 	objects        int
 	locks          int
 	refs           int
@@ -155,6 +156,7 @@ func requireCleanStaleDataMetrics(t *testing.T, m *RepositoryManager, metrics cl
 
 	for metric, expectedValue := range map[string]int{
 		"configkeys":     metrics.configkeys,
+		"configsections": metrics.configsections,
 		"objects":        metrics.objects,
 		"locks":          metrics.locks,
 		"refs":           metrics.refs,
@@ -977,7 +979,8 @@ func TestRepositoryManager_CleanStaleData_pruneEmptyConfigSections(t *testing.T)
 `, string(testhelper.MustReadFile(t, configPath)))
 
 	requireCleanStaleDataMetrics(t, mgr, cleanStaleDataMetrics{
-		configkeys: 1,
+		configkeys:     1,
+		configsections: 7,
 	})
 }
 
@@ -992,9 +995,10 @@ func TestPruneEmptyConfigSections(t *testing.T) {
 	configPath := filepath.Join(repoPath, "config")
 
 	for _, tc := range []struct {
-		desc         string
-		configData   string
-		expectedData string
+		desc                    string
+		configData              string
+		expectedData            string
+		expectedSkippedSections int
 	}{
 		{
 			desc:         "empty",
@@ -1035,9 +1039,10 @@ func TestPruneEmptyConfigSections(t *testing.T) {
 			// 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 with comment",
+			configData:              "[foo]\n# comment\n[bar]\n[baz]\n",
+			expectedData:            "[foo]\n# comment\n",
+			expectedSkippedSections: 1,
 		},
 		{
 			desc:         "empty section",
@@ -1045,14 +1050,16 @@ func TestPruneEmptyConfigSections(t *testing.T) {
 			expectedData: "",
 		},
 		{
-			desc:         "empty sections",
-			configData:   "[foo]\n[bar]\n[baz]\n",
-			expectedData: "",
+			desc:                    "empty sections",
+			configData:              "[foo]\n[bar]\n[baz]\n",
+			expectedData:            "",
+			expectedSkippedSections: 2,
 		},
 		{
-			desc:         "empty sections with missing newline",
-			configData:   "[foo]\n[bar]\n[baz]",
-			expectedData: "",
+			desc:                    "empty sections with missing newline",
+			configData:              "[foo]\n[bar]\n[baz]",
+			expectedData:            "",
+			expectedSkippedSections: 2,
 		},
 		{
 			desc:         "trailing empty section",
@@ -1060,9 +1067,10 @@ func TestPruneEmptyConfigSections(t *testing.T) {
 			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:                    "mixed keys and sections",
+			configData:              "[empty]\n[nonempty]\nbar = baz\nbar = baz\n[empty]\n",
+			expectedData:            "[nonempty]\nbar = baz\nbar = baz\n",
+			expectedSkippedSections: 1,
 		},
 		{
 			desc: "real world example",
@@ -1099,11 +1107,16 @@ func TestPruneEmptyConfigSections(t *testing.T) {
 [remote "tmp-f7a91ec9415f984d3747cf608b0a7e9c"]
         prune = true
 `,
+			expectedSkippedSections: 15,
 		},
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			require.NoError(t, os.WriteFile(configPath, []byte(tc.configData), 0o644))
-			require.NoError(t, pruneEmptyConfigSections(ctx, repo))
+
+			skippedSections, err := pruneEmptyConfigSections(ctx, repo)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedSkippedSections, skippedSections)
+
 			require.Equal(t, tc.expectedData, string(testhelper.MustReadFile(t, configPath)))
 		})
 	}