Skip to content

Commit

Permalink
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix: local commit detection not thread safe
Browse files Browse the repository at this point in the history
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.
tracemeyers committed Nov 8, 2023
1 parent 7d19206 commit a75dd65
Showing 1 changed file with 19 additions and 21 deletions.
Original file line number Diff line number Diff line change
@@ -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,18 +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)
fun hasLocalCommits(): Boolean {
return _hasLocalCommits.get()
}

private val git = initGit(gitConfig)

override fun instancesDirectory(): File {
@@ -105,8 +92,6 @@ class GitHandlerService(
override fun commitAllChanges(commitMessage: String) {
addAllChanges()
commitAsOsbApi(commitMessage)

_hasLocalCommits.set(true)
}

override fun synchronizeWithRemoteRepository() {
@@ -115,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
@@ -126,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) {
@@ -207,6 +189,22 @@ class GitHandlerService(
return false
}

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." }

@@ -372,4 +370,4 @@ class GitHandlerService(
}
}
}
}
}

0 comments on commit a75dd65

Please sign in to comment.