From 46d676dbdae186f4475b7cda1d5b91580679934d Mon Sep 17 00:00:00 2001
From: Trace Meyers <1397338+tracemeyers@users.noreply.github.com>
Date: Mon, 15 May 2023 15:31:25 -0500
Subject: [PATCH] fix: local commit detection not thread safe

synchronizeWithRemoteRepository did not handle the scenario where other
threads may modify the local repository between the merge operation and
when hasLocalCommits is set to false. This can cause the local view of
the repo to become out of sync with the remote. At best it is resolved
when another OSB operation, e.g. a provision request, creates a new
commit with the side effect that it signals there are local changes. At
worst the commits are lost if the repo storage is ephemeral and
is destroyed before the commits are pushed.
---
 .../persistence/GitHandlerService.kt          | 37 ++++++++++---------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/src/main/kotlin/io/meshcloud/dockerosb/persistence/GitHandlerService.kt b/src/main/kotlin/io/meshcloud/dockerosb/persistence/GitHandlerService.kt
index ae3fd8e..4d8d4cd 100644
--- a/src/main/kotlin/io/meshcloud/dockerosb/persistence/GitHandlerService.kt
+++ b/src/main/kotlin/io/meshcloud/dockerosb/persistence/GitHandlerService.kt
@@ -12,7 +12,6 @@ import org.eclipse.jgit.transport.URIish
 import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider
 import org.springframework.stereotype.Service
 import java.io.File
-import java.util.concurrent.atomic.AtomicBoolean
 
 private val log = KotlinLogging.logger {}
 
@@ -24,15 +23,6 @@ class GitHandlerService(
     private val gitConfig: GitConfig
 ) : GitHandler {
 
-  /**
-   * Signal if we have local commits that we want to sync or not.
-   *
-   * At startup we just assume there are new commits in git.
-   * This prevents commits "laying around" at start time until a new
-   * commit would set this flag.
-   */
-  private val hasLocalCommits = AtomicBoolean(true)
-
   private val git = initGit(gitConfig)
 
   override fun instancesDirectory(): File {
@@ -102,8 +92,6 @@ class GitHandlerService(
   override fun commitAllChanges(commitMessage: String) {
     addAllChanges()
     commitAsOsbApi(commitMessage)
-
-    hasLocalCommits.set(true)
   }
 
   override fun synchronizeWithRemoteRepository() {
@@ -112,7 +100,7 @@ class GitHandlerService(
       return
     }
 
-    if (!hasLocalCommits.get()) {
+    if (!hasLocalCommits()) {
       // note: we do also not execute a fetch in this case because if the unipipe-osb is just idling
       // there's no sense in us fetching from the remote all the time. Consumers can explicitly call
       // pullFastForwardOnly if they want an up to date copy
@@ -123,10 +111,7 @@ class GitHandlerService(
     log.info { "Starting synchronizeWithRemoteRepository" }
 
     try {
-      val pushedSuccessfully = fetchMergePush()
-      if (pushedSuccessfully) {
-        hasLocalCommits.set(false)
-      }
+      fetchMergePush()
 
       log.info { "Completed synchronizeWithRemoteRepository" }
     } catch (ex: Exception) {
@@ -204,6 +189,22 @@ class GitHandlerService(
     return false
   }
 
+  private fun hasLocalCommits(): Boolean {
+    val origin = git.repository.resolve("origin/${gitConfig.remoteBranch}")
+    val head = git.getRepository().resolve("HEAD")
+
+    var count = 0
+    for (entry in git.log().addRange(origin, head).call()) {
+      ++count
+    }
+
+    if (count > 0) {
+      log.info { "Your branch is ahead of 'origin/${gitConfig.remoteBranch}' by $count commit(s)." }
+    }
+
+    return count > 0
+  }
+
   private fun resolveMergeConflictsUsingOurs(files: List<String>) {
     log.warn { "Encountered conflicts in files $files. Will attempt to auto-resolve conflicts, preferring local changes." }
 
@@ -369,4 +370,4 @@ class GitHandlerService(
       }
     }
   }
-}
\ No newline at end of file
+}