From d1afc12e67dddb64a3710ffd34d5665c55939512 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Wed, 23 Jun 2021 12:51:15 -0700 Subject: [PATCH] Handle a race between ls-remote and fetch This is a port of PR #413. --- cmd/git-sync/main.go | 36 +++++++++++++++++++++++++---------- slow_git_fetch.sh | 24 +++++++++++++++++++++++ test_e2e.sh | 45 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 95 insertions(+), 10 deletions(-) create mode 100755 slow_git_fetch.sh diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go index e2ab25e..eb8284f 100644 --- a/cmd/git-sync/main.go +++ b/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) { - output, err := runCommand(ctx, git.root, git.cmd, "rev-parse", git.rev) +func (git *repoSync) LocalHashForRev(ctx context.Context, rev string) (string, error) { + 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. - output, err := runCommand(ctx, git.root, git.cmd, "cat-file", "-t", git.rev) + hash, err := git.ResolveRef(ctx, 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 } diff --git a/slow_git_fetch.sh b/slow_git_fetch.sh new file mode 100755 index 0000000..e4d2c37 --- /dev/null +++ b/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 "$@" diff --git a/test_e2e.sh b/test_e2e.sh index 37e5b79..d9c1a01 100755 --- a/test_e2e.sh +++ b/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 ##############################################