Handle a race between ls-remote and fetch

This is a port of PR #413.

cmd/git-sync/main.go

@@ -908,6 +908,14 @@ func (git *repoSync) AddWorktreeAndSwap(ctx context.Context, hash string) error
 		return 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.
+	if _, err := git.ResolveRef(ctx, hash); err != nil {
+		log.Error(err, "can't resolve commit, will retry", "rev", git.rev, "hash", hash)
+		return nil
+	}
+
 	// GC clone
 	if _, err := runCommand(ctx, git.root, git.cmd, "gc", "--prune=all"); err != nil {
 		return err
@@ -1134,8 +1142,8 @@ func (git *repoSync) CloneRepo(ctx context.Context) error {
 }
 
 // LocalHashForRev returns the locally known hash for a given rev.
-func (git *repoSync) LocalHashForRev(ctx context.Context) (string, error) {
+func (git *repoSync) LocalHashForRev(ctx context.Context, rev string) (string, error) {
-	output, err := runCommand(ctx, git.root, git.cmd, "rev-parse", git.rev)
+	output, err := runCommand(ctx, git.root, git.cmd, "rev-parse", rev)
 	if err != nil {
 		return "", err
 	}
@@ -1153,25 +1161,33 @@ func (git *repoSync) RemoteHashForRef(ctx context.Context, ref string) (string,
 }
 
 func (git *repoSync) RevIsHash(ctx context.Context) (bool, error) {
-	// If git doesn't identify rev as a commit, we're done.
+	hash, err := git.ResolveRef(ctx, git.rev)
-	output, err := runCommand(ctx, git.root, git.cmd, "cat-file", "-t", git.rev)
 	if err != nil {
 		return false, err
 	}
+	return strings.HasPrefix(hash, git.rev), nil
+}
+
+func (git *repoSync) ResolveRef(ctx context.Context, ref string) (string, error) {
+	// If git doesn't identify rev as a commit, we're done.
+	output, err := runCommand(ctx, git.root, git.cmd, "cat-file", "-t", ref)
+	if err != nil {
+		return "", err
+	}
 	o := strings.Trim(string(output), "\n")
 	if o != "commit" {
-		return false, nil
+		return "", nil
 	}
 
 	// `git cat-file -t` also returns "commit" for tags. If rev is already a git
 	// hash, the output will be the same hash as the input.  Of course, a user
 	// could specify "abc" and match "abcdef12345678", so we just do a prefix
 	// match.
-	output, err = git.LocalHashForRev(ctx)
+	output, err = git.LocalHashForRev(ctx, ref)
 	if err != nil {
-		return false, err
+		return "", err
 	}
-	return strings.HasPrefix(output, git.rev), nil
+	return output, nil
 }
 
 // SyncRepo syncs the branch of a given repository to the link at the given rev.
@@ -1198,7 +1214,7 @@ func (git *repoSync) SyncRepo(ctx context.Context) (bool, string, error) {
 		if err != nil {
 			return false, "", err
 		}
-		hash, err = git.LocalHashForRev(ctx)
+		hash, err = git.LocalHashForRev(ctx, git.rev)
 		if err != nil {
 			return false, "", err
 		}
@@ -1224,7 +1240,7 @@ func (git *repoSync) SyncRepo(ctx context.Context) (bool, string, error) {
 // GetRevs returns the local and upstream hashes for rev.
 func (git *repoSync) GetRevs(ctx context.Context) (string, string, error) {
 	// Ask git what the exact hash is for rev.
-	local, err := git.LocalHashForRev(ctx)
+	local, err := git.LocalHashForRev(ctx, git.rev)
 	if err != nil {
 		return "", "", err
 	}

slow_git_fetch.sh

@@ -0,0 +1,24 @@
+#!/bin/sh
+#
+# Copyright 2020 The Kubernetes Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+
+if [ "$1" != "fetch" ]; then
+  git "$@"
+  exit $?
+fi
+
+sleep 5
+git "$@"

test_e2e.sh

@@ -162,6 +162,7 @@ function finish() {
 trap finish INT EXIT
 
 SLOW_GIT_CLONE=/slow_git_clone.sh
+SLOW_GIT_FETCH=/slow_git_fetch.sh
 ASKPASS_GIT=/askpass_git.sh
 SYNC_HOOK_COMMAND=/test_sync_hook_command.sh
 
@@ -175,6 +176,7 @@ function GIT_SYNC() {
         -u $(id -u):$(id -g) \
         -v "$DIR":"$DIR":rw \
         -v "$(pwd)/slow_git_clone.sh":"$SLOW_GIT_CLONE":ro \
+        -v "$(pwd)/slow_git_fetch.sh":"$SLOW_GIT_FETCH":ro \
         -v "$(pwd)/askpass_git.sh":"$ASKPASS_GIT":ro \
         -v "$(pwd)/test_sync_hook_command.sh":"$SYNC_HOOK_COMMAND":ro \
         -v "$DOT_SSH/id_test":"/etc/git-secret/ssh":ro \
@@ -818,6 +820,49 @@ fi
 # Wrap up
 pass
 
+##############################################
+# Test fetch skipping commit
+##############################################
+testcase "fetch-skip-depth-1"
+echo "$TESTCASE" > "$REPO"/file
+git -C "$REPO" commit -qam "$TESTCASE"
+GIT_SYNC \
+    --git="$SLOW_GIT_FETCH" \
+    --period=100ms \
+    --depth=1 \
+    --repo="file://$REPO" \
+    --branch=e2e-branch \
+    --rev=HEAD \
+    --root="$ROOT" \
+    --link="link" \
+    > "$DIR"/log."$TESTCASE" 2>&1 &
+
+# wait for first sync which does a clone followed by an artifically slowed fetch
+sleep 8
+assert_link_exists "$ROOT"/link
+assert_file_exists "$ROOT"/link/file
+assert_file_eq "$ROOT"/link/file "$TESTCASE"
+
+# make a second commit to trigger a sync with shallow fetch
+echo "$TESTCASE-ok" > "$REPO"/file2
+git -C "$REPO" add file2
+git -C "$REPO" commit -qam "$TESTCASE new file"
+
+# Give time for ls-remote to detect the commit and slowed fetch to start
+sleep 2
+
+# make a third commit to insert the commit between ls-remote and fetch
+echo "$TESTCASE-ok" > "$REPO"/file3
+git -C "$REPO" add file3
+git -C "$REPO" commit -qam "$TESTCASE third file"
+sleep 10
+assert_link_exists "$ROOT"/link
+assert_file_exists "$ROOT"/link/file3
+assert_file_eq "$ROOT"/link/file3 "$TESTCASE-ok"
+
+# Wrap up
+pass
+
 ##############################################
 # Test password
 ##############################################