gitserver: Simplify clone and update

Those two used to have separate mechanisms for locking and for making sure the operation keeps going in the background.

This PR unifies those two where possible.

Test plan:

The clone test covers this pretty well, and I made a few adjustments there to cover the new behavior.
Also, E2E tests will cover clone failures.

Author: eseliger

cmd/gitserver/internal/integration_tests/clone_test.go

@@ -200,8 +200,8 @@ func TestClone_Fail(t *testing.T) {
 	require.Contains(t, err.Error(), "error cloning repo: repo github.com/test/repo not cloneable:")
 	require.Contains(t, err.Error(), "exit status 128")
 
-	// No lock should have been acquired.
-	mockassert.NotCalled(t, locker.TryAcquireFunc)
+	mockassert.CalledOnce(t, locker.TryAcquireFunc)
+	mockassert.CalledOnce(t, lock.ReleaseFunc)
 
 	// Check we reported an error.
 	// Check that it was called exactly once total.
@@ -237,13 +237,13 @@ func TestClone_Fail(t *testing.T) {
 	require.Contains(t, err.Error(), "failed to clone github.com/test/repo: clone failed. Output: Creating bare repo\nCreated bare repo at")
 
 	// Should have acquired a lock.
-	mockassert.CalledOnce(t, locker.TryAcquireFunc)
+	mockassert.CalledN(t, locker.TryAcquireFunc, 2)
 	// Should have reported status. 7 lines is the output git currently produces.
 	// This number might need to be adjusted over time, but before doing so please
 	// check that the calls actually use the args you would expect them to use.
 	mockassert.CalledN(t, lock.SetStatusFunc, 7)
 	// Should have released the lock.
-	mockassert.CalledOnce(t, lock.ReleaseFunc)
+	mockassert.CalledN(t, lock.ReleaseFunc, 2)
 
 	// Check it was set to cloning first, then uncloned again (since clone failed).
 	mockassert.CalledN(t, gsStore.SetCloneStatusFunc, 2)

cmd/gitserver/internal/lock.go

@@ -20,8 +20,7 @@ type RepositoryLock interface {
 // When a repository is locked, only the owner of the lock is allowed to perform
 // writing operations against it.
 //
-// The main use of RepositoryLocker is to prevent concurrent clones. However,
-// it is also used during maintenance tasks such as recloning/migrating/etc.
+// The main use of RepositoryLocker is to prevent concurrent fetches.
 type RepositoryLocker interface {
 	// TryAcquire acquires the lock for repo. If it is already held, ok is false
 	// and lock is nil. Otherwise a non-nil lock is returned and true. When

cmd/gitserver/internal/main_test.go

@@ -39,7 +39,7 @@ func runCmd(t *testing.T, dir string, cmd string, arg ...string) string {
 		"GIT_AUTHOR_NAME=a",
 		"[email protected]",
 	}
-	b, err := c.CombinedOutput()
+	b, err := c.Output()
 	if err != nil {
 		t.Fatalf("%s %s failed: %s\nOutput: %s", cmd, strings.Join(arg, " "), err, b)
 	}