From fe9c0598288b8aab763e870dd206fd68b5354418 Mon Sep 17 00:00:00 2001
From: barney-s <6457279+barney-s@users.noreply.github.com>
Date: Mon, 21 Jun 2021 14:55:12 -0700
Subject: [PATCH] Cleanup worktree defensively

This is to avoid wedge cases where the worktree was created but this function error'd without cleaning the worktree.
Next timearound, the sync loop fails to create the worktree and bails out.

We observed a case where due to #412, the next sync loop failed with this error:
" Run(git worktree add /repo/root/rev-nnnn origin/develop): exit status 128: { stdout: \"Preparing worktree (detached HEAD nnnn)\\n\", stderr: \"fatal: '/repo/root/rev-nnnn' already exists\\n\" }"
---
 cmd/git-sync/main.go | 30 ++++++++++++++++++++++++------
 test_e2e.sh          | 35 +++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
index e581c301a..23e8dadc2 100644
--- a/cmd/git-sync/main.go
+++ b/cmd/git-sync/main.go
@@ -673,6 +673,18 @@ func setRepoReady() {
 	repoReady = true
 }
 
+// cleanupWorkTree() is used to remove a worktree and its folder
+func cleanupWorkTree(ctx context.Context, gitRoot, worktree string) error {
+	// Clean up worktree(s)
+	log.V(1).Info("removing worktree", "path", worktree)
+	if err := os.RemoveAll(worktree); err != nil {
+		return fmt.Errorf("error removing directory: %v", err)
+	} else if _, err := runCommand(ctx, gitRoot, *flGitCmd, "worktree", "prune"); err != nil {
+		return err
+	}
+	return nil
+}
+
 // addWorktreeAndSwap creates a new worktree and calls updateSymlink to swap the symlink to point to the new worktree
 func addWorktreeAndSwap(ctx context.Context, gitRoot, dest, branch, rev string, depth int, hash string, submoduleMode string) error {
 	log.V(0).Info("syncing git", "rev", rev, "hash", hash)
@@ -695,6 +707,17 @@ func addWorktreeAndSwap(ctx context.Context, gitRoot, dest, branch, rev string,
 
 	// Make a worktree for this exact git hash.
 	worktreePath := filepath.Join(gitRoot, hash)
+
+	// Avoid wedge cases where the worktree was created but this function error'd without cleaning the worktree.
+	// Next timearound, the sync loop fails to create the worktree and bails out.
+	// Error observed:
+	//   " Run(git worktree add /repo/root/rev-nnnn origin/develop):
+	//     exit status 128: { stdout: \"Preparing worktree (detached HEAD nnnn)\\n\", stderr: \"fatal: '/repo/root/rev-nnnn' already exists\\n\" }"
+	//.  "
+	if err := cleanupWorkTree(ctx, gitRoot, worktreePath); err != nil {
+		return err
+	}
+
 	_, err := runCommand(ctx, gitRoot, *flGitCmd, "worktree", "add", worktreePath, "origin/"+branch, "--no-checkout")
 	log.V(0).Info("adding worktree", "path", worktreePath, "branch", fmt.Sprintf("origin/%s", branch))
 	if err != nil {
@@ -808,12 +831,7 @@ func addWorktreeAndSwap(ctx context.Context, gitRoot, dest, branch, rev string,
 	// Clean up previous worktree(s).
 	var cleanupErr error
 	if oldWorktree != "" {
-		log.V(1).Info("removing old worktree", "path", oldWorktree)
-		if err := os.RemoveAll(oldWorktree); err != nil {
-			cleanupErr = fmt.Errorf("error removing directory: %v", err)
-		} else if _, err := runCommand(ctx, gitRoot, *flGitCmd, "worktree", "prune"); err != nil {
-			cleanupErr = err
-		}
+		cleanupErr = cleanupWorkTree(ctx, gitRoot, oldWorktree)
 	}
 
 	if cleanupErr != nil {
diff --git a/test_e2e.sh b/test_e2e.sh
index af953b6b3..32b0e0f1d 100755
--- a/test_e2e.sh
+++ b/test_e2e.sh
@@ -310,6 +310,41 @@ assert_file_eq "$ROOT"/link/file "$TESTCASE 1"
 # Wrap up
 pass
 
+##############################################
+# Test worktree-cleanup
+##############################################
+testcase "worktree-cleanup"
+
+echo "$TESTCASE" > "$REPO"/file
+git -C "$REPO" commit -qam "$TESTCASE"
+GIT_SYNC \
+    --wait=10 \
+    --repo="file://$REPO" \
+    --branch=e2e-branch \
+    --rev=HEAD \
+    --root="$ROOT" \
+    --dest="link" \
+    > "$DIR"/log."$TESTCASE" 2>&1 &
+
+# wait for first sync
+sleep 4
+assert_link_exists "$ROOT"/link
+assert_file_exists "$ROOT"/link/file
+assert_file_eq "$ROOT"/link/file "$TESTCASE"
+
+# second commit
+echo "$TESTCASE-ok" > "$REPO"/file2
+git -C "$REPO" add file2
+git -C "$REPO" commit -qam "$TESTCASE new file"
+REV=$(git -C "$REPO" rev-list -n1 HEAD)
+git -C "$REPO" worktree add -q "$ROOT"/"$REV" -b e2e --no-checkout
+sleep 10
+
+assert_file_exists "$ROOT"/link/file2
+assert_file_eq "$ROOT"/link/file2 "$TESTCASE-ok"
+# Wrap up
+pass
+
 ##############################################
 # Test readlink
 ##############################################