From 2f7335868eec846f12ea90342d67f1ba870caa07 Mon Sep 17 00:00:00 2001 From: yoyehan Date: Thu, 22 Dec 2022 21:33:14 +0900 Subject: [PATCH] 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. --- cmd/git-sync/main.go | 85 ++++++++++++++++++++++++++++++-------------- test_e2e.sh | 84 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 143 insertions(+), 26 deletions(-) diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index c5e509994..65f13bb0d 100644 --- 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 index 9f774af0f..83bad51da 100755 --- 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 ##############################################