From 93b8a38a924e7bb19beb0655196914567505d16c Mon Sep 17 00:00:00 2001
From: Karl Isenberg <karlisenberg@google.com>
Date: Thu, 20 Jul 2023 17:23:38 -0700
Subject: [PATCH] Handle errors from credential refresh (v3)

Previously, errors from askpass and credential storage were being
ignored, causing git clone/fetch to later error with hard-to-read
errors.

Now the error indicates the credential refresh as the problem, and
either does not try to sync (if no creds) or tries to use previous creds
(if they were fetched at some point).
---
 cmd/git-sync/main.go |   4 +-
 test_e2e.sh          | 110 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 111 insertions(+), 3 deletions(-)

diff --git a/cmd/git-sync/main.go b/cmd/git-sync/main.go
index dced83357..3f1cf3d12 100644
--- a/cmd/git-sync/main.go
+++ b/cmd/git-sync/main.go
@@ -1083,7 +1083,9 @@ func revIsHash(ctx context.Context, rev, gitRoot string) (bool, error) {
 // syncRepo syncs the branch of a given repository to the destination at the given rev.
 // returns (1) whether a change occured, (2) the new hash, and (3) an error if one happened
 func syncRepo(ctx context.Context, repo, branch, rev string, depth int, gitRoot, dest string, refreshCreds func(context.Context) error, submoduleMode string) (bool, string, error) {
-	refreshCreds(ctx)
+	if err := refreshCreds(ctx); err != nil {
+		return false, "", fmt.Errorf("credential refresh failed: %w", err)
+	}
 
 	currentWorktreeGit := filepath.Join(dest, ".git")
 	var hash string
diff --git a/test_e2e.sh b/test_e2e.sh
index f86a9123d..8b9ef7c8f 100755
--- a/test_e2e.sh
+++ b/test_e2e.sh
@@ -1093,9 +1093,9 @@ function e2e::auth_askpass_url_correct_password() {
 }
 
 ##############################################
-# Test askpass-url where the URL is flaky
+# Test askpass-url where the URL is sometimes wrong
 ##############################################
-function e2e::auth_askpass_url_flaky() {
+function e2e::auth_askpass_url_sometimes_wrong() {
     # run with askpass_url service which alternates good/bad replies.
     HITLOG="$WORK/hitlog"
     cat /dev/null > "$HITLOG"
@@ -1154,6 +1154,112 @@ function e2e::auth_askpass_url_flaky() {
     assert_file_eq "$ROOT"/link/file "$FUNCNAME 1"
 }
 
+##############################################
+# Test askpass-url where the URL is flaky
+##############################################
+function e2e::auth_askpass_url_flaky() {
+    # run with askpass_url service which alternates good/bad replies.
+    HITLOG="$WORK/hitlog"
+    cat /dev/null > "$HITLOG"
+    CTR=$(docker_run \
+        -v "$HITLOG":/var/log/hits \
+        e2e/test/ncsvr \
+        80 'read X
+            if [ -f /tmp/flag ]; then
+                echo "HTTP/1.1 200 OK"
+                echo
+                echo "username=my-username"
+                echo "password=my-password"
+                rm /tmp/flag
+            else
+                echo "HTTP/1.1 503 Service Unavailable"
+                echo
+                touch /tmp/flag
+            fi
+            ')
+    IP=$(docker_ip "$CTR")
+
+    # First sync
+    echo "$FUNCNAME 1" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME 1"
+
+    GIT_SYNC \
+        --git="/$ASKPASS_GIT" \
+        --askpass-url="http://$IP/git_askpass" \
+        --max-sync-failures=2 \
+        --wait=0.1 \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev=HEAD \
+        --root="$ROOT" \
+        --dest="link" \
+        >> "$1" 2>&1 &
+    sleep 3
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 1"
+
+    # Move HEAD forward
+    echo "$FUNCNAME 2" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME 2"
+    sleep 3
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 2"
+
+    # Move HEAD backward
+    git -C "$REPO" reset -q --hard HEAD^
+    sleep 3
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME 1"
+}
+
+##############################################
+# Test askpass-url where the URL fails at startup
+##############################################
+function e2e::auth_askpass_url_slow_start() {
+    # run with askpass_url service which takes a while to come up
+    HITLOG="$WORK/hitlog"
+    cat /dev/null > "$HITLOG"
+    CTR=$(docker_run \
+        -v "$HITLOG":/var/log/hits \
+        --entrypoint sh \
+        e2e/test/ncsvr \
+        -c "sleep 4;
+            /ncsvr.sh 80 'read X
+                echo \"HTTP/1.1 200 OK\"
+                echo
+                echo \"username=my-username\"
+                echo \"password=my-password\"
+                '")
+    IP=$(docker_ip "$CTR")
+
+    # Sync
+    echo "$FUNCNAME" > "$REPO/file"
+    git -C "$REPO" commit -qam "$FUNCNAME"
+
+    GIT_SYNC \
+        --git="/$ASKPASS_GIT" \
+        --askpass-url="http://$IP/git_askpass" \
+        --max-sync-failures=5 \
+        --wait=1 \
+        --repo="file://$REPO" \
+        --branch="$MAIN_BRANCH" \
+        --rev=HEAD \
+        --root="$ROOT" \
+        --dest="link" \
+        >> "$1" 2>&1 &
+    sleep 1
+    assert_file_absent "$ROOT/link"
+
+    sleep 5
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "$FUNCNAME"
+}
+
+
 ##############################################
 # Test exechook-success
 ##############################################