From 9d9c015d951a74d001e89bd2cd7a86dadd7f0da6 Mon Sep 17 00:00:00 2001 From: Kirill Likhodedov Date: Mon, 14 May 2018 11:01:31 +0300 Subject: [PATCH] git: stash-unstash just once during the push-with-update IDEA-76774 --- .../src/git4idea/push/GitPushOperation.java | 41 ++++++++-- .../src/git4idea/update/GitUpdateProcess.java | 82 +++++++++++-------- .../push/GitPushOperationSingleRepoTest.kt | 63 ++++++++++++-- 3 files changed, 137 insertions(+), 49 deletions(-) diff --git a/plugins/git4idea/src/git4idea/push/GitPushOperation.java b/plugins/git4idea/src/git4idea/push/GitPushOperation.java index 9655119a16ae..e24e642decc3 100644 --- a/plugins/git4idea/src/git4idea/push/GitPushOperation.java +++ b/plugins/git4idea/src/git4idea/push/GitPushOperation.java @@ -28,6 +28,7 @@ import com.intellij.openapi.progress.ProgressManager; import com.intellij.openapi.progress.util.BackgroundTaskUtil; import com.intellij.openapi.project.Project; import com.intellij.openapi.ui.DialogWrapper; +import com.intellij.openapi.util.Pair; import com.intellij.openapi.util.Ref; import com.intellij.openapi.vcs.VcsException; import com.intellij.openapi.vcs.update.UpdatedFiles; @@ -55,6 +56,7 @@ import git4idea.update.GitRebaseOverMergeProblem; import git4idea.update.GitUpdateProcess; import git4idea.update.GitUpdateResult; import git4idea.update.GitUpdater; +import git4idea.util.GitPreservingProcess; import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -137,6 +139,7 @@ public class GitPushOperation { final Map results = ContainerUtil.newHashMap(); Map updatedRoots = ContainerUtil.newHashMap(); + GitPreservingProcess preservingProcess = null; try { Collection remainingRoots = myPushSpecs.keySet(); @@ -182,7 +185,13 @@ public class GitPushOperation { beforePushLabel = LocalHistory.getInstance().putSystemLabel(myProject, "Before push"); } Collection rootsToUpdate = getRootsToUpdate(updateSettings, result.rejected.keySet()); - GitUpdateResult updateResult = update(rootsToUpdate, updateSettings.getUpdateMethod(), rebaseOverMergeProblemDetected == null); + Pair pair = update(rootsToUpdate, updateSettings.getUpdateMethod(), + rebaseOverMergeProblemDetected == null, + preservingProcess == null); + if (preservingProcess == null) { // multiple updates => multiple preserving processes, but all are fake ones except the first + preservingProcess = pair.second; + } + GitUpdateResult updateResult = pair.first; for (GitRepository repository : rootsToUpdate) { updatedRoots.put(repository, updateResult); // TODO update result in GitUpdateProcess is a single for several roots } @@ -195,6 +204,9 @@ public class GitPushOperation { if (myPushProcessCustomization != null) myPushProcessCustomization.executeAfterPush(results); } finally { + if (preservingProcess != null) { + preservingProcess.load(); + } if (beforePushLabel != null) { afterPushLabel = LocalHistory.getInstance().putSystemLabel(myProject, "After push"); } @@ -453,17 +465,30 @@ public class GitPushOperation { } @NotNull - protected GitUpdateResult update(@NotNull Collection rootsToUpdate, - @NotNull UpdateMethod updateMethod, - boolean checkForRebaseOverMergeProblem) { - GitUpdateResult updateResult = new GitUpdateProcess(myProject, myProgressIndicator, - new HashSet<>(rootsToUpdate), UpdatedFiles.create(), - checkForRebaseOverMergeProblem, false).update(updateMethod); + protected Pair update(@NotNull Collection rootsToUpdate, + @NotNull UpdateMethod updateMethod, + boolean checkForRebaseOverMergeProblem, + boolean stashBeforeUpdate) { + GitUpdateProcess updateProcess = new GitUpdateProcess(myProject, myProgressIndicator, + new HashSet<>(rootsToUpdate), UpdatedFiles.create(), + checkForRebaseOverMergeProblem, false) { + @Override + protected boolean stashBeforeUpdate() { + return stashBeforeUpdate; + } + + @Override + protected boolean unstashAfterUpdate() { + return false; + } + }; + GitUpdateResult updateResult = updateProcess.update(updateMethod); + GitPreservingProcess preservingProcess = stashBeforeUpdate ? updateProcess.getPreservingProcess() : null; for (GitRepository repository : rootsToUpdate) { repository.getRoot().refresh(true, true); repository.update(); } - return updateResult; + return Pair.create(updateResult, preservingProcess); } private static class ResultWithOutput { diff --git a/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java b/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java index 358c167588ee..4b2df75d76fb 100644 --- a/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java +++ b/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java @@ -62,8 +62,6 @@ import static git4idea.util.GitUIUtil.*; /** * Handles update process (pull via merge or rebase) for several roots. - * - * @author Kirill Likhodedov */ public class GitUpdateProcess { private static final Logger LOG = Logger.getInstance(GitUpdateProcess.class); @@ -80,6 +78,8 @@ public class GitUpdateProcess { @NotNull private final ProgressIndicator myProgressIndicator; @NotNull private final GitMerger myMerger; + @Nullable private GitPreservingProcess myPreservingProcess; + public GitUpdateProcess(@NotNull Project project, @Nullable ProgressIndicator progressIndicator, @NotNull Collection repositories, @@ -143,6 +143,14 @@ public class GitUpdateProcess { return result; } + protected boolean unstashAfterUpdate() { + return true; + } + + protected boolean stashBeforeUpdate() { + return true; + } + @NotNull private GitUpdateResult updateImpl(@NotNull UpdateMethod updateMethod) { Map trackedBranches = checkTrackedBranchesConfiguration(); @@ -195,12 +203,14 @@ public class GitUpdateProcess { // save local changes if needed (update via merge may perform without saving). final Collection myRootsToSave = ContainerUtil.newArrayList(); LOG.info("updateImpl: identifying if save is needed..."); - for (Map.Entry entry : updaters.entrySet()) { - GitRepository repo = entry.getKey(); - GitUpdater updater = entry.getValue(); - if (updater.isSaveNeeded()) { - myRootsToSave.add(repo.getRoot()); - LOG.info("update| root " + repo + " needs save"); + if (stashBeforeUpdate()) { + for (Map.Entry entry : updaters.entrySet()) { + GitRepository repo = entry.getKey(); + GitUpdater updater = entry.getValue(); + if (updater.isSaveNeeded()) { + myRootsToSave.add(repo.getRoot()); + LOG.info("update| root " + repo + " needs save"); + } } } @@ -208,30 +218,33 @@ public class GitUpdateProcess { final Ref incomplete = Ref.create(false); final Ref compoundResult = Ref.create(); final Map finalUpdaters = updaters; - new GitPreservingProcess(myProject, myGit, myRootsToSave, "Update", "Remote", - GitVcsSettings.getInstance(myProject).updateChangesPolicy(), myProgressIndicator, () -> { - LOG.info("updateImpl: updating..."); - GitRepository currentlyUpdatedRoot = null; - try { - for (GitRepository repo : myRepositories) { - GitUpdater updater = finalUpdaters.get(repo); - if (updater == null) continue; - currentlyUpdatedRoot = repo; - GitUpdateResult res = updater.update(); - LOG.info("updating root " + currentlyUpdatedRoot + " finished: " + res); - if (res == GitUpdateResult.INCOMPLETE) { - incomplete.set(true); - } - compoundResult.set(joinResults(compoundResult.get(), res)); - } - } - catch (VcsException e) { - String rootName = (currentlyUpdatedRoot == null) ? "" : getShortRepositoryName(currentlyUpdatedRoot); - LOG.info("Error updating changes for root " + currentlyUpdatedRoot, e); - notifyImportantError(myProject, "Error updating " + rootName, - "Updating " + rootName + " failed with an error: " + e.getLocalizedMessage()); - } - }).execute(() -> { + myPreservingProcess = new GitPreservingProcess(myProject, myGit, myRootsToSave, "Update", "Remote", + GitVcsSettings.getInstance(myProject).updateChangesPolicy(), myProgressIndicator, () -> { + LOG.info("updateImpl: updating..."); + GitRepository currentlyUpdatedRoot = null; + try { + for (GitRepository repo : myRepositories) { + GitUpdater updater = finalUpdaters.get(repo); + if (updater == null) continue; + currentlyUpdatedRoot = repo; + GitUpdateResult res = updater.update(); + LOG.info("updating root " + currentlyUpdatedRoot + " finished: " + res); + if (res == GitUpdateResult.INCOMPLETE) { + incomplete.set(true); + } + compoundResult.set(joinResults(compoundResult.get(), res)); + } + } + catch (VcsException e) { + String rootName = (currentlyUpdatedRoot == null) ? "" : getShortRepositoryName(currentlyUpdatedRoot); + LOG.info("Error updating changes for root " + currentlyUpdatedRoot, e); + notifyImportantError(myProject, "Error updating " + rootName, + "Updating " + rootName + " failed with an error: " + e.getLocalizedMessage()); + } + }); + myPreservingProcess.execute(() -> { + if (!unstashAfterUpdate()) return false; + // Note: compoundResult normally should not be null, because the updaters map was checked for non-emptiness. // But if updater.update() fails with exception for the first root, then the value would not be assigned. // In this case we don't restore local changes either, because update failed. @@ -295,6 +308,11 @@ public class GitUpdateProcess { return updaters; } + @Nullable + public GitPreservingProcess getPreservingProcess() { + return myPreservingProcess; + } + @NotNull private static GitUpdateResult joinResults(@Nullable GitUpdateResult compoundResult, GitUpdateResult result) { if (compoundResult == null) { diff --git a/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.kt b/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.kt index 89b9f8cf3bc7..cdd310e40292 100644 --- a/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.kt +++ b/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.kt @@ -23,7 +23,6 @@ import com.intellij.openapi.util.text.StringUtil import com.intellij.openapi.vcs.Executor import com.intellij.openapi.vcs.update.FileGroup import com.intellij.openapi.vcs.update.UpdatedFiles -import com.intellij.testFramework.UsefulTestCase import com.intellij.util.containers.ContainerUtil import git4idea.branch.GitBranchUtil import git4idea.config.GitVersionSpecialty @@ -33,6 +32,7 @@ import git4idea.repo.GitRepository import git4idea.test.* import git4idea.update.GitRebaseOverMergeProblem import git4idea.update.GitUpdateResult +import git4idea.util.GitPreservingProcess import org.junit.Assume.assumeTrue import java.io.File import java.util.Collections.singletonMap @@ -141,10 +141,11 @@ class GitPushOperationSingleRepoTest : GitPushOperationBaseTest() { val pushSpec = makePushSpec(repository, "master", "origin/master") val result = object : GitPushOperation(project, pushSupport, singletonMap(repository, pushSpec), null, false, false) { - override fun update(rootsToUpdate: Collection, + override fun update(rootsToUpdate: MutableCollection, updateMethod: UpdateMethod, - checkForRebaseOverMergeProblem: Boolean): GitUpdateResult { - val updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem) + checkForRebaseOverMergeProblem: Boolean, + stashBeforeUpdate: Boolean): Pair { + val updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem, stashBeforeUpdate) pushCommitFromBro() return updateResult } @@ -169,10 +170,11 @@ class GitPushOperationSingleRepoTest : GitPushOperationBaseTest() { val result = object : GitPushOperation(project, pushSupport, singletonMap(repository, pushSpec), null, false, false) { internal var updateHappened: Boolean = false - override fun update(rootsToUpdate: Collection, + override fun update(rootsToUpdate: MutableCollection, updateMethod: UpdateMethod, - checkForRebaseOverMergeProblem: Boolean): GitUpdateResult { - val updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem) + checkForRebaseOverMergeProblem: Boolean, + stashBeforeUpdate: Boolean): Pair { + val updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem, stashBeforeUpdate) if (!updateHappened) { updateHappened = true pushCommitFromBro() @@ -426,6 +428,49 @@ class GitPushOperationSingleRepoTest : GitPushOperationBaseTest() { assertEquals(UpdateMethod.BRANCH_DEFAULT, settings.updateType) } + // IDEA-76774 + fun `test local changes are stashed-unstashed only once for several push rejections`() { + pushCommitFromBro() + cd(repository) + makeCommit("afile.txt") + val localFile = repository.file("local-changes.txt") + localFile.create("Local changes").add() + + updateRepositories() + refresh() + updateChangeListManager() + + agreeToUpdate(GitRejectedPushUpdateDialog.REBASE_EXIT_CODE) + + val pushSpec = makePushSpec(repository, "master", "origin/master") + + var stashCalled = 0 + git.stashListener = { + stashCalled++ + } + + var pushRejected = 0 + + val result = object : GitPushOperation(project, pushSupport, singletonMap(repository, pushSpec), null, false, false) { + override fun update(rootsToUpdate: MutableCollection, + updateMethod: UpdateMethod, + checkForRebaseOverMergeProblem: Boolean, + stashBeforeUpdate: Boolean): Pair { + val updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem, stashBeforeUpdate) + if (pushRejected < 2) { + pushCommitFromBro() + pushRejected++ + } + return updateResult + } + }.execute() + + assertResult(SUCCESS, 1, "master", "origin/master", result) + assertTrue("Locally changed file wasn't unstashed", localFile.exists()) + repository.assertStatus(localFile.file, 'A') + assertEquals("Stash should have been called only once", 1, stashCalled) + } + private fun generateUpdateNeeded() { pushCommitFromBro() cd(repository) @@ -464,8 +509,8 @@ class GitPushOperationSingleRepoTest : GitPushOperationBaseTest() { updatedFiles: List?, actualResult: GitPushResult) { assertResult(type, pushedCommits, from, to, updateResult, actualResult.results[repository]!!) - UsefulTestCase.assertSameElements("Updated files set is incorrect", - getUpdatedFiles(actualResult.updatedFiles), ContainerUtil.notNullize(updatedFiles)) + if (updatedFiles != null) + assertSameElements("Updated files set is incorrect", getUpdatedFiles(actualResult.updatedFiles), updatedFiles) } private fun getUpdatedFiles(updatedFiles: UpdatedFiles): Collection {