settings-sync: rewrite procedure of logging and merging changes

GitOrigin-RevId: 394fbd7cbfa53f28abd74cdb55129c327583ddd0

plugins/settings-sync/resources/messages/SettingsSyncBundle.properties

@@ -6,6 +6,7 @@ notification.title.push.error=Couldn't synchronize settings
 notification.title.update.error=Couldn't get settings from server
 notification.title.update.no.such.file=No settings file on the server
 progress.title.updating.settings.from.server=Updating settings from server...
+notification.title.apply.error=Couldn't apply settings
 
 title.settings.sync=Settings Sync
 configurable.what.to.sync.label=Configure what to sync:

plugins/settings-sync/src/com/intellij/settingsSync/GitSettingsLog.kt

@@ -4,11 +4,15 @@ import com.intellij.openapi.Disposable
 import com.intellij.openapi.diagnostic.logger
 import com.intellij.openapi.util.Disposer
 import com.intellij.openapi.util.io.NioFiles
-import com.intellij.util.concurrency.annotations.RequiresBackgroundThread
 import com.intellij.util.io.*
 import org.eclipse.jgit.api.Git
-import org.eclipse.jgit.api.MergeResult.MergeStatus.*
+import org.eclipse.jgit.api.MergeResult.MergeStatus.CONFLICTING
+import org.eclipse.jgit.api.MergeResult.MergeStatus.FAST_FORWARD
+import org.eclipse.jgit.api.errors.EmptyCommitException
 import org.eclipse.jgit.lib.Constants
+import org.eclipse.jgit.lib.Constants.R_HEADS
+import org.eclipse.jgit.lib.ObjectId
+import org.eclipse.jgit.lib.Ref
 import org.eclipse.jgit.lib.Repository
 import org.eclipse.jgit.revwalk.RevCommit
 import org.eclipse.jgit.storage.file.FileRepositoryBuilder
@@ -23,7 +27,11 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
 ) : SettingsLog, Disposable {
 
   private lateinit var repository: Repository
-  private lateinit var remoteBranch: RevCommit
+  private lateinit var git: Git
+
+  private val master: Ref get() = repository.findRef(MASTER_REF_NAME)!!
+  private val ide: Ref get() = repository.findRef(IDE_REF_NAME)!!
+  private val cloud: Ref get() = repository.findRef(CLOUD_REF_NAME)!!
 
   init {
     Disposer.register(parentDisposable, this)
@@ -32,20 +40,36 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
   override fun initialize(): Boolean {
     val dotGit = settingsSyncStorage.resolve(".git")
     repository = FileRepositoryBuilder.create(dotGit.toFile())
+    git = Git(repository)
 
     val newRepository = !dotGit.exists()
     if (newRepository) {
+      LOG.info("Initializing new Git repository for Settings Sync at $settingsSyncStorage")
       repository.create()
       initRepository(repository)
       copyExistingSettings(repository)
     }
 
-    remoteBranch = repository.headCommit()
+    createBranchIfNeeded(MASTER_REF_NAME, newRepository)
+    createBranchIfNeeded(CLOUD_REF_NAME, newRepository)
+    createBranchIfNeeded(IDE_REF_NAME, newRepository)
 
     return newRepository
   }
 
+  private fun createBranchIfNeeded(name: String, newRepository: Boolean) {
+    val ref = repository.findRef(name)
+    if (ref == null) {
+      val head = repository.headCommit()
+      if (!newRepository) {
+        LOG.warn("Ref with name $name not found in existing repository. Recreating at position of HEAD@${head.toObjectId().short}")
+      }
+      git.branchCreate().setName(name).setStartPoint(head).call()
+    }
+  }
+
   private fun copyExistingSettings(repository: Repository) {
+    LOG.info("Copying existing settings from $rootConfigPath to $settingsSyncStorage")
     val copiedFileSpecs = mutableListOf<String>()
 
     val filesToExport = collectFilesToExportFromSettings()
@@ -71,7 +95,7 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
         addCommand.addFilepattern(fileSpec)
       }
       addCommand.call()
-      git.commit().setMessage("copy existing configs").call()
+      git.commit().setMessage("Copy existing configs").call()
     }
   }
 
@@ -100,80 +124,44 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
 
     val git = Git(repository)
     git.add().addFilepattern(".gitignore").call()
-    git.commit().setMessage("initial").call()
-    // todo repo should be pushed after initialization (or be initialized by data received from the server)
+    git.commit().setMessage("Initial").call()
   }
 
-  @RequiresBackgroundThread
-  override fun applyLocalState(snapshot: SettingsSnapshot) {
-    if (snapshot.fileStates.isEmpty()) { // todo move upwards?
-      LOG.error("Don't record empty settings snapshot")
+  override fun applyIdeState(snapshot: SettingsSnapshot) {
+    applyState(IDE_REF_NAME, snapshot)
+  }
+
+  override fun applyCloudState(snapshot: SettingsSnapshot) {
+    applyState(CLOUD_REF_NAME, snapshot)
+  }
+
+  private fun applyState(refName: String, snapshot: SettingsSnapshot) {
+    if (snapshot.isEmpty()) {
+      LOG.error("Empty snapshot")
+      return
     }
 
-    applySnapshotAndCommit(snapshot)
+    git.checkout().setName(refName).call()
+    applySnapshotAndCommit(refName, snapshot)
   }
 
-  private fun applySnapshotAndCommit(snapshot: SettingsSnapshot): RevCommit {
+  private fun applySnapshotAndCommit(refName: String, snapshot: SettingsSnapshot) {
     // todo check repository consistency before each operation: that we're on master, that rb is deleted, that there're no uncommitted changes
 
-    // todo recording local operation should also be merged instead of applying,
-    // because of a race: local changes can happen after receiving changes from server but before and applying them.
-
-    val git = Git(repository)
+    LOG.info("Applying settings changes to $refName")
     val addCommand = git.add()
-    val message = StringBuilder()
+    val message = "Apply changes received from $refName"
     for (fileState in snapshot.fileStates) {
       settingsSyncStorage.resolve(fileState.file).write(fileState.content, 0, fileState.size)
       addCommand.addFilepattern(fileState.file)
-      message.appendLine(fileState.file)
     }
     addCommand.call()
-    return git.commit().setMessage("committing $message").call()
-  }
-
-  override fun pushedSuccessfully() {
-    remoteBranch = repository.headCommit()
-  }
-
-  override fun applyRemoteState(snapshot: SettingsSnapshot): Boolean { // todo improve return result API
-    // todo check repository consistency before each operation: that we're on master, than rb is deleted, that there're no uncommitted changes
-
-    val git = Git(repository)
-
-    if (repository.resolve("rb") != null) {
-      LOG.warn("rb wasn't deleted") // todo maybe don't need a warning here: it can happen after abnormal termination
-      git.checkout().setName("master").call() // todo check if not already checked out
-      git.branchDelete().setBranchNames("rb").setForce(true).call()
-    }
-
+    // Don't allow empty commit: sometimes the stream provider can notify about changes but there are no actual changes on disk
     try {
-      var rb = git.checkout().setCreateBranch(true).setName("rb").setStartPoint(remoteBranch).call().objectId
-      rb = applySnapshotAndCommit(snapshot)
-      git.checkout().setName("master").call()
-      val mergeResult = git.merge().include(rb).call()
-
-      // todo handle merge conflicts
-
-      when (mergeResult.mergeStatus) {
-        FAST_FORWARD, ALREADY_UP_TO_DATE -> {
-          return false
-        }
-        MERGED -> {
-          return true
-        }
-        else -> {
-          throw IllegalStateException("Unexpected merge result status: " + mergeResult.mergeStatus) //todo
-        }
-      }
+      git.commit().setMessage(message).setAllowEmpty(false).call()
     }
-    finally {
-      try {
-        git.checkout().setName("master").call() // todo extract to "returnToConsistency"
-        git.branchDelete().setBranchNames("rb").setForce(true).call()
-      }
-      catch (e: Throwable) {
-        LOG.error("Couldn't return to consistent state", e)
-      }
+    catch (e: EmptyCommitException) {
+      LOG.info("No actual changes in the settings")
     }
   }
 
@@ -181,7 +169,9 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
     repository.close()   // todo synchronize
   }
 
-  override fun collectCurrentSnapshot(): SettingsSnapshot {   // todo check if there are uncommitted changes (should be none)
+  override fun collectCurrentSnapshot(): SettingsSnapshot {
+    // todo check repository consistency, e.g. there should be no uncommitted changes
+
     val files = settingsSyncStorage.toFile().walkTopDown()
       .onEnter { it.name != ".git" }
       .filter { it.isFile && it.name != ".gitignore" }
@@ -192,12 +182,94 @@ internal class GitSettingsLog(private val settingsSyncStorage: Path,
     return SettingsSnapshot(files)
   }
 
+  override fun getIdePosition(): SettingsLog.Position {
+    return getPosition(ide)
+  }
+
+  override fun getCloudPosition(): SettingsLog.Position {
+    return getPosition(cloud)
+  }
+
+  override fun getMasterPosition(): SettingsLog.Position {
+    return getPosition(master)
+  }
+
+  private fun getPosition(ref: Ref) = BranchPosition(ref.objectId.name())
+
+  override fun setIdePosition(position: SettingsLog.Position) {
+    updateBranchPosition(IDE_REF_NAME, position)
+  }
+
+  override fun setCloudPosition(position: SettingsLog.Position) {
+    updateBranchPosition(CLOUD_REF_NAME, position)
+  }
+
+  private fun updateBranchPosition(refName: String, targetPosition: SettingsLog.Position): Ref {
+    val ref = repository.findRef(refName)!!
+    val previousObjectId = ref.objectId
+    val targetObjectId = ObjectId.fromString(targetPosition.id)
+    val refUpdate = repository.updateRef(ref.name)
+    refUpdate.setNewObjectId(targetObjectId)
+    val result = refUpdate.update()
+    LOG.info("Updated position of ${ref.short} from ${previousObjectId.short} to $targetPosition: $result")
+    return repository.findRef(ref.name)!!
+  }
+
+  private fun fastForwardMaster(branchOnSamePosition: Ref, targetBranch: Ref): BranchPosition {
+      LOG.info("Advancing master. Its position is equal to ${branchOnSamePosition.short}: ${master.objectId.short}. " +
+               "Expecting fast-forward merge to ${targetBranch.short} ${targetBranch.objectId.short}")
+      val mergeResult = git.merge().include(targetBranch).call()
+      if (mergeResult.mergeStatus != FAST_FORWARD) {
+        LOG.warn("Non-fast-forward result: $mergeResult")
+        // todo check consistency here
+      }
+      return getPosition(master)
+  }
+
+  override fun advanceMaster(): SettingsLog.Position {
+    git.checkout().setName(MASTER_REF_NAME).call()
+
+    if (master.objectId == ide.objectId) {
+      return fastForwardMaster(ide, cloud)
+    }
+
+    if (master.objectId == cloud.objectId) {
+      return fastForwardMaster(cloud, ide)
+    }
+
+    LOG.info("Advancing master@${master.objectId.short}. Need merge of ide@${ide.objectId.short} and cloud@${cloud.objectId.short}")
+    // 1. move master to ide
+    val refUpdate = repository.updateRef(ide.name)
+    refUpdate.setNewObjectId(ide.objectId)
+    refUpdate.update()
+
+    // 2. merge with cloud
+    val mergeResult = git.merge().include(cloud).call()
+    LOG.info("Merge of master&ide@${master.objectId.short} with cloud@${cloud.objectId.short}: $mergeResult")
+    if (mergeResult.mergeStatus == CONFLICTING) {
+      // todo redo merge with "last modified" strategy
+    }
+    return getPosition(master)
+  }
+
   private fun Repository.headCommit(): RevCommit {
-    val ref = this.exactRef(Constants.HEAD)
+    val ref = this.findRef(Constants.HEAD)
     return this.parseCommit(ref.objectId)
   }
 
-  companion object {
+  private val Ref.short get() = this.name.removePrefix(R_HEADS)
+
+  private val ObjectId.short get() = this.name.substring(0, 8)
+
+  private data class BranchPosition(override val id: String): SettingsLog.Position {
+    override fun toString(): String = id.substring(0, 8)
+  }
+
+  private companion object {
     val LOG = logger<GitSettingsLog>()
+
+    const val MASTER_REF_NAME = "master"
+    const val IDE_REF_NAME = "ide"
+    const val CLOUD_REF_NAME = "cloud"
   }
 }

plugins/settings-sync/src/com/intellij/settingsSync/ManualUpdateAction.kt

@@ -14,7 +14,20 @@ class ManualUpdateAction : DumbAwareAction() {
     // todo don't allow to start several tasks at once, including the automated ones
     object: Task.Backgroundable(e.project, SettingsSyncBundle.message("progress.title.updating.settings.from.server"), false) {
       override fun run(indicator: ProgressIndicator) {
-        facade.updateChecker.updateFromServer()
+        val updateResult = facade.updateChecker.scheduleUpdateFromServer()
+        when (updateResult) {
+          is UpdateResult.Success -> Unit // received the update pack successfully, it will be applied async via the SettingsSyncBridge
+          is UpdateResult.Error -> {
+            // todo remove the error notification after next successful update
+            notifySettingsSyncError(SettingsSyncBundle.message("notification.title.update.error"), updateResult.message)
+            return
+          }
+          is UpdateResult.NoFileOnServer -> {
+            notifySettingsSyncError(SettingsSyncBundle.message("notification.title.update.error"),
+                                    SettingsSyncBundle.message("notification.title.update.no.such.file"))
+          }
+        }
+
       }
     }.queue()
   }

plugins/settings-sync/src/com/intellij/settingsSync/SettingSyncNotifications.kt

@@ -4,9 +4,9 @@ import com.intellij.notification.NotificationGroupManager
 import com.intellij.notification.NotificationType
 import com.intellij.openapi.util.NlsContexts
 
-private const val NOTIFICATION_GROUP = "settingsSync.errors"
+internal const val NOTIFICATION_GROUP = "settingsSync.errors"
 
-internal fun notifyError(@NlsContexts.NotificationTitle title: String, @NlsContexts.NotificationContent   message: String) {
+internal fun notifySettingsSyncError(@NlsContexts.NotificationTitle title: String, @NlsContexts.NotificationContent message: String) {
   NotificationGroupManager.getInstance().getNotificationGroup(NOTIFICATION_GROUP)
     .createNotification(title, message, NotificationType.ERROR)
     .notify(null)

plugins/settings-sync/src/com/intellij/settingsSync/SettingsChangeListener.kt

@@ -4,20 +4,14 @@ import java.nio.charset.StandardCharsets.UTF_8
 
 internal interface SettingsChangeListener {
 
-  fun settingChanged(event: SettingsChangeEvent)
+  fun settingChanged(event: SyncSettingsEvent)
 
 }
 
-internal data class SettingsChangeEvent(val source: ChangeSource, val snapshot: SettingsSnapshot)
-
-internal enum class ChangeSource {
-  FROM_LOCAL,
-  FROM_SERVER
-}
-
-internal interface SettingsLoggedListener {
-
-  fun settingsLogged(event: SettingsLoggedEvent)
+internal sealed class SyncSettingsEvent {
+  class IdeChange(val snapshot: SettingsSnapshot) : SyncSettingsEvent()
+  class CloudChange(val snapshot: SettingsSnapshot) : SyncSettingsEvent()
+  class PushRequest(): SyncSettingsEvent()
 }
 
 internal data class SettingsSnapshot(val fileStates: Set<FileState>) {

plugins/settings-sync/src/com/intellij/settingsSync/SettingsLog.kt

@@ -6,6 +6,9 @@ import com.intellij.util.concurrency.annotations.RequiresBackgroundThread
  * Records changes in the settings, merges changes made locally and remotely.
  */
 internal interface SettingsLog {
+  interface Position {
+    val id: String
+  }
 
   /**
    * Initializes the log, either from scratch (if the settings sync was not enabled for this IDE yet),
@@ -15,13 +18,14 @@ internal interface SettingsLog {
    * @return true if the repository was created from scratch;
    * false if the data was already there, and we've just created internal structures.
    */
+  @RequiresBackgroundThread
   fun initialize(): Boolean
 
   /**
    * Records the current local state of the settings.
    */
   @RequiresBackgroundThread
-  fun applyLocalState(snapshot: SettingsSnapshot)
+  fun applyIdeState(snapshot: SettingsSnapshot)
 
   /**
    * Records the state of the settings received from the server.
@@ -29,7 +33,7 @@ internal interface SettingsLog {
    * returns true if merge has happened, false in case of fast-forward
    */
   @RequiresBackgroundThread
-  fun applyRemoteState(snapshot: SettingsSnapshot): Boolean //todo improve the return value type API
+  fun applyCloudState(snapshot: SettingsSnapshot)
 
   /**
    * Returns the current state of the settings as it is now from the SettingsLog point of view,
@@ -38,9 +42,23 @@ internal interface SettingsLog {
   @RequiresBackgroundThread
   fun collectCurrentSnapshot(): SettingsSnapshot
 
+  fun getIdePosition(): Position
+  fun getCloudPosition(): Position
+  fun getMasterPosition(): Position
+
+  fun setIdePosition(position: Position)
+  fun setCloudPosition(position: Position)
+
   /**
-   * Tells the SettingsLog that the settings have been pushed successfully, to let the SettingsLog update its state accordingly.
+   * Moves the master branch to the actual position, which is defined as following:
+   * * If the ide branch has advanced further, but the cloud branch didn't, then just move the master branch to the position of the 'ide'
+   * (fast-forward merge in Git terminology).
+   * * Same if the cloud branch has advanced, but the ide stayed intact.
+   * * If both ide and cloud branches have advanced (which means that settings changes were applied both in ide and in cloud more or less
+   * simultaneously), then this method merges branches and moves the 'master' label to the new merge position.
+   *
+   * @return New position of 'master'.
    */
-  fun pushedSuccessfully()
+  fun advanceMaster(): Position
 
 }

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncBridge.kt

@@ -2,6 +2,7 @@ package com.intellij.settingsSync
 
 import com.intellij.openapi.Disposable
 import com.intellij.openapi.application.Application
+import com.intellij.openapi.diagnostic.logger
 import com.intellij.util.Alarm
 import com.intellij.util.concurrency.annotations.RequiresBackgroundThread
 import com.intellij.util.containers.ContainerUtil
@@ -15,12 +16,15 @@ import java.util.concurrent.TimeUnit
  * Handles events about settings change both from the current IDE, and from the server, merges the settings, logs them,
  * and provides the combined data to clients: both to the IDE and to the server.
  */
-internal class SettingsSyncBridge(private val application: Application,
+internal class SettingsSyncBridge(application: Application,
                                   parentDisposable: Disposable,
                                   private val settingsLog: SettingsLog,
-                                  private val pusher: SettingsSyncPusher) {
+                                  private val ideUpdater: SettingsSyncIdeUpdater,
+                                  private val remoteCommunicator: SettingsSyncRemoteCommunicator,
+                                  private val updateChecker: SettingsSyncUpdateChecker
+) {
 
-  private val pendingEvents = ContainerUtil.createConcurrentList<SettingsChangeEvent>()
+  private val pendingEvents = ContainerUtil.createConcurrentList<SyncSettingsEvent>()
 
   private val queue = MergingUpdateQueue("SettingsSyncBridge", 1000, true, null, parentDisposable, null, Alarm.ThreadToUse.POOLED_THREAD).
     apply {
@@ -42,7 +46,7 @@ internal class SettingsSyncBridge(private val application: Application,
     })
 
     application.messageBus.connect(parentDisposable).subscribe(SETTINGS_CHANGED_TOPIC, object : SettingsChangeListener {
-      override fun settingChanged(event: SettingsChangeEvent) {
+      override fun settingChanged(event: SyncSettingsEvent) {
         pendingEvents.add(event)
         queue.queue(updateObject)
       }
@@ -52,41 +56,91 @@ internal class SettingsSyncBridge(private val application: Application,
   private fun initializeLog() {
     val newRepository = settingsLog.initialize()
     if (newRepository) {
-      push()
+      remoteCommunicator.push(settingsLog.collectCurrentSnapshot()) // todo handle non-successful result
     }
   }
 
   @RequiresBackgroundThread
   private fun processPendingEvents() {
+    val previousIdePosition = settingsLog.getIdePosition()
+    val previousCloudPosition = settingsLog.getCloudPosition()
+
+    var pushToCloudRequired = false
     while (pendingEvents.isNotEmpty()) {
       val event = pendingEvents.removeAt(0)
-      processSettingsChangeEvent(event)
-    }
-    val snap = settingsLog.collectCurrentSnapshot()
-    // todo send event only after changes from server or merges
-    application.messageBus.syncPublisher(SETTINGS_LOGGED_TOPIC).settingsLogged(SettingsLoggedEvent(snap, true, true, emptySet()))
-  }
-
-  @RequiresBackgroundThread
-  private fun processSettingsChangeEvent(event: SettingsChangeEvent) {
-    if (event.snapshot.isEmpty()) {
-      return
+      if (event is SyncSettingsEvent.IdeChange) {
+        settingsLog.applyIdeState(event.snapshot)
+      }
+      else if (event is SyncSettingsEvent.CloudChange) {
+        settingsLog.applyCloudState(event.snapshot)
+      }
+      else if (event is SyncSettingsEvent.PushRequest) {
+        pushToCloudRequired = true
+      }
     }
 
-    if (event.source == ChangeSource.FROM_LOCAL) {
-      settingsLog.applyLocalState(event.snapshot)
-      push() // todo push only after processing all events, to avoid extra pushes
+    val newIdePosition = settingsLog.getIdePosition()
+    val newCloudPosition = settingsLog.getCloudPosition()
+    val masterPosition: SettingsLog.Position
+    if (newIdePosition != previousIdePosition || newCloudPosition != previousCloudPosition) {
+      // move master to the actual position. It can be a fast-forward to either ide, or cloud changes, or it can be a merge
+      masterPosition = settingsLog.advanceMaster()
     }
-    else if (event.source == ChangeSource.FROM_SERVER) {
-      val merged = settingsLog.applyRemoteState(event.snapshot)
-      if (merged) {
-        push()
+    else {
+      // there were only fake events without actual changes to the repository => master doesn't need to be changed either
+      masterPosition = settingsLog.getMasterPosition()
+    }
+
+    if (newIdePosition != masterPosition) { // master has advanced further that ide => the ide needs to be updated
+      val pushResult: SettingsSyncPushResult = pushToIde(settingsLog.collectCurrentSnapshot())
+      LOG.info("Result of pushing settings to the cloud: $pushResult")
+      when (pushResult) {
+        SettingsSyncPushResult.Success -> settingsLog.setIdePosition(masterPosition)
+        is SettingsSyncPushResult.Error -> {
+          // todo notify only in case of explicit sync invocation, otherwise update some SettingsSyncStatus
+          notifySettingsSyncError(title = SettingsSyncBundle.message("notification.title.apply.error"),
+                                  message = pushResult.message)
+        }
+        SettingsSyncPushResult.Rejected -> {
+          // In the case of reject we'll just "wait" for the next update event:
+          // it will be processed in the next session anyway
+          if (pendingEvents.none { it is SyncSettingsEvent.IdeChange }) {
+            // todo schedule update
+          }
+        }
+      }
+    }
+
+    if (pushToCloudRequired || newCloudPosition != masterPosition) {
+      val pushResult: SettingsSyncPushResult = pushToCloud(settingsLog.collectCurrentSnapshot())
+      when (pushResult) {
+        SettingsSyncPushResult.Success -> settingsLog.setCloudPosition(masterPosition)
+        is SettingsSyncPushResult.Error -> {
+          // todo notify only in case of explicit sync invocation, otherwise update some SettingsSyncStatus
+          notifySettingsSyncError(SettingsSyncBundle.message("notification.title.push.error"), pushResult.message)
+        }
+        SettingsSyncPushResult.Rejected -> {
+          // todo add protection against potential infinite reject-update-reject cycle
+          //  (it would indicate some problem, but still shouldn't cycle forever)
+
+          // In the case of reject we'll just "wait" for the next update event:
+          // it will be processed in the next session anyway
+          if (pendingEvents.none { it is SyncSettingsEvent.CloudChange }) {
+            // not to wait for too long, schedule an update right away unless it has already been scheduled
+            updateChecker.scheduleUpdateFromServer()
+          }
+        }
       }
     }
   }
 
-  private fun push() {
-    pusher.push()
+  private fun pushToCloud(settingsSnapshot: SettingsSnapshot): SettingsSyncPushResult {
+    return remoteCommunicator.push(settingsSnapshot)
+  }
+
+  private fun pushToIde(settingsSnapshot: SettingsSnapshot): SettingsSyncPushResult {
+    ideUpdater.settingsLogged(settingsSnapshot)
+    return SettingsSyncPushResult.Success // todo
   }
 
   @TestOnly
@@ -103,4 +157,8 @@ internal class SettingsSyncBridge(private val application: Application,
   internal fun resumeEventProcessing() {
     queue.resume()
   }
+
+  companion object {
+    val LOG = logger<SettingsSyncBridge>()
+  }
 }

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncIdeUpdater.kt

@@ -27,22 +27,13 @@ import java.nio.file.Path
 
 internal class SettingsSyncIdeUpdater(application: Application,
                                       private val componentStore: ComponentStoreImpl,
-                                      private val rootConfig: Path) : SettingsLoggedListener {
+                                      private val rootConfig: Path) {
 
   companion object {
     val LOG = logger<SettingsSyncIdeUpdater>()
   }
 
-  init {
-    application.messageBus.connect().subscribe(SETTINGS_LOGGED_TOPIC, this)
-  }
-
-  override fun settingsLogged(event: SettingsLoggedEvent) {
-    if (!event.hasRemote) {
-      return
-    }
-
-    val snapshot = event.snapshot
+  fun settingsLogged(snapshot: SettingsSnapshot) {
     // todo race between this code and SettingsSyncStreamProvider.write which can write other user settings at the same time
 
     // todo update only that has really changed

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncMain.kt

@@ -6,6 +6,7 @@ import com.intellij.configurationStore.getExportableItemsFromLocalStorage
 import com.intellij.ide.ApplicationLoadListener
 import com.intellij.openapi.Disposable
 import com.intellij.openapi.application.Application
+import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.components.stateStore
 import com.intellij.util.SystemProperties
 import com.intellij.util.messages.Topic
@@ -14,9 +15,6 @@ import java.nio.file.Path
 @Topic.AppLevel
 internal val SETTINGS_CHANGED_TOPIC: Topic<SettingsChangeListener> = Topic(SettingsChangeListener::class.java)
 
-@Topic.AppLevel
-internal val SETTINGS_LOGGED_TOPIC: Topic<SettingsLoggedListener> = Topic(SettingsLoggedListener::class.java)
-
 private const val SETTINGS_SYNC_ENABLED_PROPERTY = "idea.settings.sync.enabled"
 
 internal fun isSettingsSyncEnabled() : Boolean =
@@ -26,7 +24,7 @@ internal class SettingsSyncFacade {
   internal val updateChecker: SettingsSyncUpdateChecker get() = getMain().controls.updateChecker
 
   internal fun pushSettingsToServer() {
-    getMain().controls.pusher.push()
+    ApplicationManager.getApplication().messageBus.syncPublisher(SETTINGS_CHANGED_TOPIC).settingChanged(SyncSettingsEvent.PushRequest())
   }
 
   private fun getMain(): SettingsSyncMain {
@@ -66,20 +64,18 @@ internal class SettingsSyncMain : ApplicationLoadListener {
       val settingsLog = GitSettingsLog(settingsSyncStorage, appConfigPath, parentDisposable) {
         getExportableItemsFromLocalStorage(getExportableComponentsMap(false), componentStore.storageManager).keys
       }
-      val pusher = SettingsSyncPusher(settingsLog, remoteCommunicator)
-      val bridge = SettingsSyncBridge(application, parentDisposable, settingsLog, pusher)
-      SettingsSyncIdeUpdater(application, componentStore, appConfigPath)
+      val ideUpdater = SettingsSyncIdeUpdater(application, componentStore, appConfigPath)
+      val updateChecker = SettingsSyncUpdateChecker(application, remoteCommunicator)
+      val bridge = SettingsSyncBridge(application, parentDisposable, settingsLog, ideUpdater, remoteCommunicator, updateChecker)
 
       val streamProvider = SettingsSyncStreamProvider(application, appConfigPath)
       componentStore.storageManager.addStreamProvider(streamProvider)
 
-      val updateChecker = SettingsSyncUpdateChecker(application, remoteCommunicator)
 
-      return SettingsSyncControls(updateChecker, pusher, bridge)
+      return SettingsSyncControls(updateChecker, bridge)
     }
   }
 
   internal class SettingsSyncControls(val updateChecker: SettingsSyncUpdateChecker,
-                                      val pusher: SettingsSyncPusher,
                                       val bridge: SettingsSyncBridge)
 }

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncPushResult.kt

@@ -3,8 +3,16 @@ package com.intellij.settingsSync
 import com.intellij.openapi.util.NlsSafe
 
 internal sealed class SettingsSyncPushResult {
-  object Success: SettingsSyncPushResult()
-  object Rejected: SettingsSyncPushResult()
-  class Error(@NlsSafe val message: String): SettingsSyncPushResult()
+  object Success : SettingsSyncPushResult() {
+    override fun toString(): String = "SUCCESS"
+  }
+
+  object Rejected: SettingsSyncPushResult() {
+    override fun toString(): String = "REJECTED"
+  }
+
+  class Error(@NlsSafe val message: String): SettingsSyncPushResult() {
+    override fun toString(): String = "ERROR[$message]"
+  }
 }
 

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncPusher.kt

@@ -1,43 +0,0 @@
-package com.intellij.settingsSync
-
-import com.intellij.openapi.diagnostic.logger
-
-internal class SettingsSyncPusher(private val settingsLog: GitSettingsLog,
-                                  private val remoteCommunicator: SettingsSyncRemoteCommunicator) {
-
-  // todo notify error only when called explicitly, otherwise just set the status
-  internal fun push() {
-    val pushResult = remoteCommunicator.push(settingsLog.collectCurrentSnapshot())
-    when (pushResult) {
-      is SettingsSyncPushResult.Success -> {
-        settingsLog.pushedSuccessfully()
-      }
-      is SettingsSyncPushResult.Rejected -> {
-        val updateResult = remoteCommunicator.receiveUpdates()
-        when (updateResult) {
-          is UpdateResult.Success -> {
-            settingsLog.applyRemoteState(updateResult.settingsSnapshot) // todo handle conflicts and another merge
-            push() // todo push only if pull succeeded
-          }
-          is UpdateResult.Error -> {
-            notifyError(SettingsSyncBundle.message("notification.title.push.error"), updateResult.message)
-            return
-          }
-          is UpdateResult.NoFileOnServer -> {
-            LOG.error("No settings file on the server, but push has been rejected")
-            notifyError(SettingsSyncBundle.message("notification.title.update.error"),
-                        SettingsSyncBundle.message("notification.title.update.no.such.file"))
-            return
-          }
-        }
-      }
-      is SettingsSyncPushResult.Error -> {
-        notifyError(SettingsSyncBundle.message("notification.title.push.error"), pushResult.message)
-      }
-    }
-  }
-
-  companion object {
-    val LOG = logger<SettingsSyncPusher>()
-  }
-}

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncStreamProvider.kt

@@ -41,8 +41,8 @@ internal class SettingsSyncStreamProvider(private val application: Application,
       return
     }
 
-    val snap = SettingsSnapshot(setOf(FileState(file, content, size)))
-    application.messageBus.syncPublisher(SETTINGS_CHANGED_TOPIC).settingChanged(SettingsChangeEvent(ChangeSource.FROM_LOCAL, snap))
+    val snapshot = SettingsSnapshot(setOf(FileState(file, content, size)))
+    application.messageBus.syncPublisher(SETTINGS_CHANGED_TOPIC).settingChanged(SyncSettingsEvent.IdeChange(snapshot))
   }
 
   override fun read(fileSpec: String, roamingType: RoamingType, consumer: (InputStream?) -> Unit): Boolean {

plugins/settings-sync/src/com/intellij/settingsSync/SettingsSyncUpdateChecker.kt

@@ -7,24 +7,13 @@ internal class SettingsSyncUpdateChecker(private val application: Application,
                                          private val remoteCommunicator: SettingsSyncRemoteCommunicator) {
 
   @RequiresBackgroundThread
-  fun updateFromServer() {
+  fun scheduleUpdateFromServer() : UpdateResult {
     val updateResult = remoteCommunicator.receiveUpdates()
-    when (updateResult) {
-      is UpdateResult.Success -> {
-        val snapshot = updateResult.settingsSnapshot
-        val event = SettingsChangeEvent(ChangeSource.FROM_SERVER, snapshot)
-        application.messageBus.syncPublisher(SETTINGS_CHANGED_TOPIC).settingChanged(event)
-      }
-      is UpdateResult.Error -> {
-        // todo remove the error notification after next successful update
-        notifyError(SettingsSyncBundle.message("notification.title.update.error"), updateResult.message)
-        return
-      }
-      is UpdateResult.NoFileOnServer -> {
-        notifyError(SettingsSyncBundle.message("notification.title.update.error"),
-                    SettingsSyncBundle.message("notification.title.update.no.such.file"))
-      }
+    if (updateResult is UpdateResult.Success) {
+      val snapshot = updateResult.settingsSnapshot
+      application.messageBus.syncPublisher(SETTINGS_CHANGED_TOPIC).settingChanged(SyncSettingsEvent.CloudChange(snapshot))
     }
+    return updateResult
   }
 
   // todo update by app focus receive & by timer

plugins/settings-sync/tests/com/intellij/settingsSync/SettingsSyncTest.kt

@@ -7,7 +7,6 @@ import com.intellij.ide.ui.UISettings
 import com.intellij.openapi.application.ApplicationManager
 import com.intellij.openapi.application.impl.ApplicationImpl
 import com.intellij.openapi.components.PersistentStateComponent
-import com.intellij.openapi.components.State
 import com.intellij.openapi.components.StateStorage
 import com.intellij.openapi.components.impl.stores.IComponentStore
 import com.intellij.openapi.editor.ex.EditorSettingsExternalizable
@@ -169,7 +168,7 @@ internal class SettingsSyncTest {
     }.toFileState()
     remoteCommunicator.updateResult = UpdateResult.Success(SettingsSnapshot(setOf(fileState)))
 
-    updateChecker.updateFromServer()
+    updateChecker.scheduleUpdateFromServer()
 
     waitForSettingsToBeApplied(generalSettings)
     assertFalse(generalSettings.isSaveOnFrameDeactivation)
@@ -204,7 +203,7 @@ internal class SettingsSyncTest {
     remoteCommunicator.updateResult = UpdateResult.Success(SettingsSnapshot(setOf(fileState)))
     remoteCommunicator.offline = false
 
-    updateChecker.updateFromServer() // merge will happen here
+    updateChecker.scheduleUpdateFromServer() // merge will happen here
 
     assertSettingsPushed {
       fileState {