Fix cases of syncing different SHAs back to back

Prior to this, it would fail if the 2nd SHA wasn't in the local repo. Now it doesn't care what the local SHA for rev is, it only cares what is checked out at HEAD.
thockin · Dec 25, 2022 · 2f73358 · 2f73358
1 parent a761413
commit 2f73358
Show file tree

Hide file tree

Showing 2 changed files with 143 additions and 26 deletions.
diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
@@ -1094,30 +1094,35 @@ func (git *repoSync) CleanupWorkTree(ctx context.Context, gitRoot, worktree stri
 func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) (bool, string, error) {
 	git.log.V(0).Info("syncing git", "rev", git.rev, "hash", hash)
 
-	args := []string{"fetch", "-f", "--tags"}
-	if git.depth != 0 {
-		args = append(args, "--depth", strconv.Itoa(git.depth))
-	}
-	fetch := "HEAD"
-	if git.branch != "" {
-		fetch = git.branch
-	}
-	args = append(args, git.repo, fetch)
+	// If we don't have this hash, we need to fetch it.
+	if _, err := git.ResolveRef(ctx, hash); err != nil {
+		git.log.V(2).Info("can't resolve commit, will try fetch", "rev", git.rev, "hash", hash)
 
-	// Update from the remote.
-	if _, err := git.run.Run(ctx, git.root, nil, git.cmd, args...); err != nil {
-		return false, "", err
-	}
+		args := []string{"fetch", "-f", "--tags"}
+		if git.depth != 0 {
+			args = append(args, "--depth", strconv.Itoa(git.depth))
+		}
+		fetch := "HEAD"
+		if git.branch != "" {
+			fetch = git.branch
+		}
+		args = append(args, git.repo, fetch)
 
-	// With shallow fetches, it's possible to race with the upstream repo and
-	// end up NOT fetching the hash we wanted. If we can't resolve that hash
-	// to a commit we can just end early and leave it for the next sync period.
-	// This probably SHOULD be an error, but it can be a problem for startup
-	// and first-sync-must-succeed semantics.  If/when we fix that this can be
-	// fixed.
-	if _, err := git.ResolveRef(ctx, hash); err != nil {
-		git.log.Error(err, "can't resolve commit, will retry", "rev", git.rev, "hash", hash)
-		return false, "", nil
+		// Update from the remote.
+		if _, err := git.run.Run(ctx, git.root, nil, git.cmd, args...); err != nil {
+			return false, "", err
+		}
+
+		// With shallow fetches, it's possible to race with the upstream repo and
+		// end up NOT fetching the hash we wanted. If we can't resolve that hash
+		// to a commit we can just end early and leave it for the next sync period.
+		// This probably SHOULD be an error, but it can be a problem for startup
+		// and first-sync-must-succeed semantics.  If/when we fix that this can be
+		// fixed.
+		if _, err := git.ResolveRef(ctx, hash); err != nil {
+			git.log.Error(err, "can't resolve commit, will retry", "rev", git.rev, "hash", hash)
+			return false, "", nil
+		}
 	}
 
 	// Make a worktree for this exact git hash.
@@ -1373,6 +1378,16 @@ func (git *repoSync) LocalHashForRev(ctx context.Context, rev string) (string, e
 	if err != nil {
 		return "", err
 	}
+	result := strings.Trim(string(output), "\n")
+	if result == rev {
+		// It appears to be a SHA, so we need to verify that we have it.  We
+		// don't care what cat-file says, just whether it succeeds or fails.
+		_, err := git.run.Run(ctx, git.root, nil, git.cmd, "cat-file", "-t", rev)
+		if err != nil {
+			// Indicate that we do not have a local hash for this hash.
+			return "", err
+		}
+	}
 	return strings.Trim(string(output), "\n"), nil
 }
 
@@ -1494,16 +1509,20 @@ func stringContainsOneOf(s string, matches ...string) bool {
 	return false
 }
 
-// GetRevs returns the local and upstream hashes for rev.
+// GetRevs returns the current HEAD and upstream hash for rev.  Normally the
+// current HEAD is a previous or current version of git.rev, but if the app was
+// started with one rev and then restarted with a different one, HEAD could be
+// anything.
 func (git *repoSync) GetRevs(ctx context.Context) (string, string, error) {
-	// Ask git what the exact hash is for rev.
-	local, err := git.LocalHashForRev(ctx, git.rev)
+	// Find the currently synced HEAD.
+	local, err := git.LocalHashForRev(ctx, "HEAD")
 	if err != nil {
 		return "", "", err
 	}
 
 	// Build a ref string, depending on whether the user asked to track HEAD or
-	// a tag.
+	// a tag.  We can't really tell if it is a SHA yet, so we will catch that
+	// case later.
 	ref := "HEAD"
 	if git.rev != "HEAD" {
 		ref = "refs/tags/" + git.rev
@@ -1517,6 +1536,20 @@ func (git *repoSync) GetRevs(ctx context.Context) (string, string, error) {
 		return "", "", err
 	}
 
+	// If we couldn't find a remote hash, it might have been a SHA literal.
+	if remote == "" {
+		// If git thinks it tastes like a SHA, we just return that and if it
+		// is wrong, we will fail later.
+		output, err := git.run.Run(ctx, git.root, nil, git.cmd, "rev-parse", git.rev)
+		if err != nil {
+			return "", "", err
+		}
+		result := strings.Trim(string(output), "\n")
+		if result == git.rev {
+			remote = git.rev
+		}
+	}
+
 	return local, remote, nil
 }
 

diff --git a/test_e2e.sh b/test_e2e.sh
@@ -853,6 +853,90 @@ function e2e::sync_sha_once() {
     assert_file_eq "$ROOT"/link/file "$FUNCNAME"
 }
 
+##############################################
+# Test rev-sync on a different rev we already have
+##############################################
+function e2e::sync_sha_once_sync_different_sha_known() {
+    # All revs will be known because we check out the branch
+    echo "$FUNCNAME 1" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 1"
+    REV1=$(git -C "$REPO" rev-list -n1 HEAD)
+    echo "$FUNCNAME 2" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 2"
+    REV2=$(git -C "$REPO" rev-list -n1 HEAD)
+    echo "$FUNCNAME 3" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 3"
+
+    # Sync REV1
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev="$REV1" \
+        --root="$ROOT" \
+        --link="link" \
+        >> "$1" 2>&1
+    assert_link_exists "$ROOT"/link
+    assert_file_exists "$ROOT"/link/file
+    assert_file_eq "$ROOT"/link/file "$FUNCNAME 1"
+
+    # Sync REV2
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev="$REV2" \
+        --root="$ROOT" \
+        --link="link" \
+        >> "$1" 2>&1
+    assert_link_exists "$ROOT"/link
+    assert_file_exists "$ROOT"/link/file
+    assert_file_eq "$ROOT"/link/file "$FUNCNAME 2"
+}
+
+##############################################
+# Test rev-sync on a different rev we do not have
+##############################################
+function e2e::sync_sha_once_sync_different_sha_unknown() {
+    echo "$FUNCNAME 1" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 1"
+    REV1=$(git -C "$REPO" rev-list -n1 HEAD)
+
+    # Sync REV1
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev="$REV1" \
+        --root="$ROOT" \
+        --link="link" \
+        >> "$1" 2>&1
+    assert_link_exists "$ROOT"/link
+    assert_file_exists "$ROOT"/link/file
+    assert_file_eq "$ROOT"/link/file "$FUNCNAME 1"
+
+    # The locally synced repo does not know this new SHA.
+    echo "$FUNCNAME 2" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 2"
+    REV2=$(git -C "$REPO" rev-list -n1 HEAD)
+    # Make sure the SHA is not at HEAD, to prevent things that only work in
+    # that case.
+    echo "$FUNCNAME 3" > "$REPO"/file
+    git -C "$REPO" commit -qam "$FUNCNAME 3"
+
+    # Sync REV2
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev="$REV2" \
+        --root="$ROOT" \
+        --link="link" \
+        >> "$1" 2>&1
+    assert_link_exists "$ROOT"/link
+    assert_file_exists "$ROOT"/link/file
+    assert_file_eq "$ROOT"/link/file "$FUNCNAME 2"
+}
 ##############################################
 # Test syncing after a crash
 ##############################################