Merge pull request #656 from thockin/fix-back-to-back-rev-syncs

Fix cases of syncing different SHAs back to back

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 we don't have this hash, we need to fetch it.
-	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)
-
-	// 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)
+		git.log.V(2).Info("can't resolve commit, will try fetch", "rev", git.rev, "hash", hash)
-		return false, "", nil
+
+		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)
+
+		// 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.
+	// Find the currently synced HEAD.
-	local, err := git.LocalHashForRev(ctx, git.rev)
+	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
 }
 

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
 ##############################################