repository: Refactor test to test GarbageCollect with locks

Refactor the test that verifies behaviour of the GarbageCollect RPC when
there are lockfiles around to match modern best practices.

Note that this refactoring surfaces an error when trying to garbage
collect a repo with a locked commit-graph. This is surfaces only now
because previously, we were reusing the same repository across all of
the tests. As a consequence, the commit-graph was getting generated by
the first subtest that called GarbageCollect without the lockfile having
been written yet. Subsequent calls thus wouldn't have to update it
anymore and succeeded even with the lockfile existing.

Now that we use a separate repository per test we get an error. This
error is in fact expected.

1 files changed, 61 insertions, 25 deletions

diff --git a/internal/gitaly/service/repository/gc_test.go b/internal/gitaly/service/repository/gc_test.go
index 3bd186a8c..5d303ba73 100644
--- a/internal/gitaly/service/repository/gc_test.go
+++ b/internal/gitaly/service/repository/gc_test.go
@@ -267,36 +267,72 @@ func TestGarbageCollectDeletesFileLocks(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	_, repo, repoPath, client := setupRepositoryService(ctx, t)
-
-	req := &gitalypb.GarbageCollectRequest{Repository: repo}
+	cfg, client := setupRepositoryServiceWithoutRepo(t)
 
-	for _, tc := range []string{
-		"config.lock",
-		"HEAD.lock",
-		"objects/info/commit-graphs/commit-graph-chain.lock",
+	for _, tc := range []struct {
+		desc                string
+		lockfile            string
+		expectedErrContains string
+	}{
+		{
+			desc:     "locked gitconfig",
+			lockfile: "config.lock",
+		},
+		{
+			desc:     "locked HEAD",
+			lockfile: "HEAD.lock",
+		},
+		{
+			desc:     "locked commit-graph",
+			lockfile: "objects/info/commit-graphs/commit-graph-chain.lock",
+			// Writing commit-graphs fails if there is another, concurrent process that
+			// has locked the commit-graph chain.
+			expectedErrContains: "Another git process seems to be running in this repository",
+		},
 	} {
-		lockPath := filepath.Join(repoPath, tc)
-		// No file on the lock path
-		//nolint:staticcheck
-		_, err := client.GarbageCollect(ctx, req)
-		assert.NoError(t, err)
-
-		// Fresh lock should remain
-		mustCreateFileWithTimes(t, lockPath, freshTime)
-		//nolint:staticcheck
-		_, err = client.GarbageCollect(ctx, req)
+		t.Run(tc.desc, func(t *testing.T) {
+			// Create a lockfile and run GarbageCollect. Because the lock has been
+			// freshly created GarbageCollect shouldn't remove the not-yet-stale
+			// lockfile.
+			t.Run("with recent lockfile", func(t *testing.T) {
+				repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
+					Seed: gittest.SeedGitLabTest,
+				})
+
+				lockPath := filepath.Join(repoPath, tc.lockfile)
+				mustCreateFileWithTimes(t, lockPath, freshTime)
+
+				//nolint:staticcheck
+				_, err := client.GarbageCollect(ctx, &gitalypb.GarbageCollectRequest{
+					Repository: repo,
+				})
+				if tc.expectedErrContains == "" {
+					require.NoError(t, err)
+				} else {
+					require.Error(t, err)
+					require.Contains(t, err.Error(), tc.expectedErrContains)
+				}
+				require.FileExists(t, lockPath)
+			})
 
-		assert.NoError(t, err)
+			// Redo the same test, but this time we create the lockfile so that it is
+			// considered stale. GarbageCollect should know to remove it.
+			t.Run("with stale lockfile", func(t *testing.T) {
+				repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
+					Seed: gittest.SeedGitLabTest,
+				})
 
-		assert.FileExists(t, lockPath)
+				lockPath := filepath.Join(repoPath, tc.lockfile)
+				mustCreateFileWithTimes(t, lockPath, oldTime)
 
-		// Old lock should be removed
-		mustCreateFileWithTimes(t, lockPath, oldTime)
-		//nolint:staticcheck
-		_, err = client.GarbageCollect(ctx, req)
-		assert.NoError(t, err)
-		require.NoFileExists(t, lockPath)
+				//nolint:staticcheck
+				_, err := client.GarbageCollect(ctx, &gitalypb.GarbageCollectRequest{
+					Repository: repo,
+				})
+				require.NoError(t, err)
+				require.NoFileExists(t, lockPath)
+			})
+		})
 	}
 }