added separator after ref to remove git reset --soft ambiguity

[AlanZhang2002]

Fixes #944

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: AlanZhang2002 / name: Alan Zhang (21fce7f, 7d9699e)

[thockin]

Thanks!

This SEEMS like an appropriate change, but can you help me with a repro case in the e2e (./test_e2e.sh)

Clearly it doesn't fail the current e2e test, so we need to encode whatever scenario you hit into a test case.

[AlanZhang2002]

Sure, I'll try to reproduce in testing, though I'm honestly not completely sure how I hit it.
Here's a little snippet of part of the setup (besides auth):

        - name: *******
          image: registry.k8s.io/git-sync/git-sync:v4.4.0
          imagePullPolicy: Always
          env:
            - name: GITSYNC_REPO
              value: {{ .Values.kubernetes.git.repo }}
            - name: GITSYNC_REF
              value: {{ .Values.kubernetes.git.ref }} # for this case, just HEAD
            - name: GITSYNC_ONE_TIME
              value: "true"
            - name: GITSYNC_ROOT
              value: "/git/src/"
            - name: GITSYNC_LINK
              value: "repo"

One thing to note is that when this happens, I have an existing k8 git-sync deployment that doesn't run with --one-time, and when I run this version with the --one-time flag as a k8 job on the same repo and ref that it is already synced on, the error happens. Running in an environment where there is only the --one-time version doing the updating does not make this error happen, when syncing on a repo that was already synced. Not sure exactly why, or how to put this in a test case.

[thockin]

Are they sharing a volume on disk?

[AlanZhang2002]

Yes, so effectively same local repo being synced with two different instances, two different ways. The deployment was also on v3.6.2, not sure if that may change things. Here's the deployment setup:

        - name: git-sync-clone-dags
          image: registry.k8s.io/git-sync/git-sync:v3.6.2
          imagePullPolicy: Always
          env:
            - name: GIT_SYNC_REPO
              value: {{ .Values.kubernetes.git.repo }}
            - name: GIT_SYNC_BRANCH
              value: {{ .Values.kubernetes.git.branch }}
            - name: GIT_SYNC_ROOT
              value: "/git/src/"
            - name: GIT_SYNC_DEST
              value: "repo"
            - name: GIT_SYNC_WAIT
              value: "300"
            - name: GIT_SYNC_DEPTH
              value: "1"

[thockin]

Oh. That is not at all something I test for, and I am not too surprised it breaks. The default settings are pretty aggressive about pruning inactive data (as determined by the sync being performed).

Can you explain more about why you have it set up this way?

[AlanZhang2002]

It was just temporarily how it was set up while I was testing, not a setup we actually stick with. I'm sure no actual git-sync setup would have something like this also, but it's probably a good change to add -- into the command anyways.

[thockin]

Yeah, I think the -- is likely correct, but I want to have a test.

[thockin]

I think I have a test (it fails without the -- and passes with it). Can you patch this in, and if it passes e2e (run ./test_e2e.sh sync_sha_shafile_exists) add it to this PR?

diff --git a/test_e2e.sh b/test_e2e.sh
index 142d7c1..855e87c 100755
--- a/test_e2e.sh
+++ b/test_e2e.sh
@@ -1491,6 +1491,40 @@ function e2e::sync_crash_no_worktree_cleanup_retry() {
     assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]}"
 }
 
+##############################################
+# Test syncing if a file named for the SHA exists
+##############################################
+function e2e::sync_sha_shafile_exists() {
+    echo "${FUNCNAME[0]} 1" > "$REPO/file"
+    git -C "$REPO" commit -qam "${FUNCNAME[0]} 1"
+    SHA1=$(git -C "$REPO" rev-list -n1 HEAD)
+    echo "${FUNCNAME[0]} 2" > "$REPO/file"
+    git -C "$REPO" commit -qam "${FUNCNAME[0]} 2"
+    SHA2=$(git -C "$REPO" rev-list -n1 HEAD)
+
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --ref="$SHA1" \
+        --root="$ROOT" \
+        --link="link"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]} 1"
+
+    touch "$ROOT/$SHA2"
+
+    GIT_SYNC \
+        --one-time \
+        --repo="file://$REPO" \
+        --ref="$SHA2" \
+        --root="$ROOT" \
+        --link="link"
+    assert_link_exists "$ROOT/link"
+    assert_file_exists "$ROOT/link/file"
+    assert_file_eq "$ROOT/link/file" "${FUNCNAME[0]} 2"
+}
+
 ##############################################
 # Test changing repos with storage intact
 ##############################################

[AlanZhang2002]

I think I'm unable to test locally for some reason, my docker setup is a bit scuffed atm probably. I added the test to the pr though, I'll see if I can get it running locally.

[thockin]

OK. I ran it locally and passes. Putting it in the PR means we don't get half of it.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlanZhang2002, thockin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment