From 030939953813fdca1ca356e40e495ac039cd3c62 Mon Sep 17 00:00:00 2001 From: Kirill Likhodedov Date: Fri, 2 Jan 2015 14:47:46 +0300 Subject: [PATCH] [git] Respect branch default setting for update-when-rejected-push + remove the unneeded extra UpdateMethod enum Relates to IDEA-134326 --- .../src/git4idea/push/GitPushOperation.java | 13 ++- .../push/GitRejectedPushUpdateDialog.java | 6 + .../git4idea/update/GitUpdateEnvironment.java | 2 +- .../src/git4idea/update/GitUpdateProcess.java | 18 +-- .../src/git4idea/update/GitUpdater.java | 50 ++++----- .../push/GitPushOperationSingleRepoTest.java | 105 ++++++++++++++++++ 6 files changed, 144 insertions(+), 50 deletions(-) diff --git a/plugins/git4idea/src/git4idea/push/GitPushOperation.java b/plugins/git4idea/src/git4idea/push/GitPushOperation.java index f67a807bfbaf..8973cd2b7b4b 100644 --- a/plugins/git4idea/src/git4idea/push/GitPushOperation.java +++ b/plugins/git4idea/src/git4idea/push/GitPushOperation.java @@ -52,6 +52,7 @@ import git4idea.repo.GitRepositoryManager; import git4idea.update.GitRebaseOverMergeProblem; import git4idea.update.GitUpdateProcess; import git4idea.update.GitUpdateResult; +import git4idea.update.GitUpdater; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -383,7 +384,8 @@ public class GitPushOperation { private void savePushUpdateSettings(@NotNull PushUpdateSettings settings, boolean rebaseOverMergeDetected) { UpdateMethod updateMethod = settings.getUpdateMethod(); mySettings.setUpdateAllRootsIfPushRejected(settings.shouldUpdateAllRoots()); - if (!rebaseOverMergeDetected) { // don't overwrite explicit "rebase" with temporary "merge" caused by merge commits + if (!rebaseOverMergeDetected // don't overwrite explicit "rebase" with temporary "merge" caused by merge commits + && mySettings.getUpdateType() != updateMethod && mySettings.getUpdateType() != UpdateMethod.BRANCH_DEFAULT) { // don't overwrite "branch default" setting mySettings.setUpdateType(updateMethod); } } @@ -392,6 +394,10 @@ public class GitPushOperation { private PushUpdateSettings readPushUpdateSettings() { boolean updateAllRoots = mySettings.shouldUpdateAllRootsIfPushRejected(); UpdateMethod updateMethod = mySettings.getUpdateType(); + if (updateMethod == UpdateMethod.BRANCH_DEFAULT) { + // deliberate limitation: we have only 2 buttons => choose method from the 1st repo if different + updateMethod = GitUpdater.resolveUpdateMethod(myProject, myPushSpecs.keySet().iterator().next().getRoot()); + } return new PushUpdateSettings(updateAllRoots, updateMethod); } @@ -429,12 +435,9 @@ public class GitPushOperation { protected GitUpdateResult update(@NotNull Collection rootsToUpdate, @NotNull UpdateMethod updateMethod, boolean checkForRebaseOverMergeProblem) { - GitUpdateProcess.UpdateMethod um = updateMethod == UpdateMethod.MERGE ? - GitUpdateProcess.UpdateMethod.MERGE : - GitUpdateProcess.UpdateMethod.REBASE; GitUpdateResult updateResult = new GitUpdateProcess(myProject, myPlatformFacade, myProgressIndicator, new HashSet(rootsToUpdate), UpdatedFiles.create(), - checkForRebaseOverMergeProblem).update(um); + checkForRebaseOverMergeProblem).update(updateMethod); for (GitRepository repository : rootsToUpdate) { repository.getRoot().refresh(true, true); repository.update(); diff --git a/plugins/git4idea/src/git4idea/push/GitRejectedPushUpdateDialog.java b/plugins/git4idea/src/git4idea/push/GitRejectedPushUpdateDialog.java index fdbdf229d277..a24da32017ab 100644 --- a/plugins/git4idea/src/git4idea/push/GitRejectedPushUpdateDialog.java +++ b/plugins/git4idea/src/git4idea/push/GitRejectedPushUpdateDialog.java @@ -220,6 +220,12 @@ class GitRejectedPushUpdateDialog extends DialogWrapper { return myRebaseOverMergeProblemDetected; } + @TestOnly + @NotNull + Action getDefaultAction() { + return Boolean.TRUE.equals(myMergeAction.getValue(DEFAULT_ACTION)) ? myMergeAction : myRebaseAction; + } + private class MergeAction extends AbstractAction { MergeAction() { super("&Merge"); diff --git a/plugins/git4idea/src/git4idea/update/GitUpdateEnvironment.java b/plugins/git4idea/src/git4idea/update/GitUpdateEnvironment.java index 3e9c48abbbbb..b7903cc2b719 100644 --- a/plugins/git4idea/src/git4idea/update/GitUpdateEnvironment.java +++ b/plugins/git4idea/src/git4idea/update/GitUpdateEnvironment.java @@ -61,7 +61,7 @@ public class GitUpdateEnvironment implements UpdateEnvironment { final GitUpdateProcess gitUpdateProcess = new GitUpdateProcess(myProject, myPlatformFacade, progressIndicator, getRepositoriesFromRoots(repositoryManager, roots), updatedFiles, true); - boolean result = gitUpdateProcess.update(GitUpdateProcess.UpdateMethod.READ_FROM_SETTINGS).isSuccess(); + boolean result = gitUpdateProcess.update(mySettings.getUpdateType()).isSuccess(); return new GitUpdateSession(result); } diff --git a/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java b/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java index 33909cd9fa31..6c610575a00f 100644 --- a/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java +++ b/plugins/git4idea/src/git4idea/update/GitUpdateProcess.java @@ -45,6 +45,7 @@ import git4idea.GitUtil; import git4idea.branch.GitBranchPair; import git4idea.branch.GitBranchUtil; import git4idea.commands.Git; +import git4idea.config.UpdateMethod; import git4idea.merge.GitConflictResolver; import git4idea.merge.GitMergeCommittingConflictResolver; import git4idea.merge.GitMerger; @@ -83,12 +84,6 @@ public class GitUpdateProcess { private GitUpdateResult myResult; private final Collection myRootsToSave; - public enum UpdateMethod { - MERGE, - REBASE, - READ_FROM_SETTINGS - } - public GitUpdateProcess(@NotNull Project project, @NotNull GitPlatformFacade platformFacade, @Nullable ProgressIndicator progressIndicator, @@ -298,15 +293,8 @@ public class GitUpdateProcess { LOG.info("updateImpl: defining updaters..."); for (GitRepository repository : myRepositories) { VirtualFile root = repository.getRoot(); - final GitUpdater updater; - if (updateMethod == UpdateMethod.MERGE) { - updater = new GitMergeUpdater(myProject, myGit, root, myTrackedBranches, myProgressIndicator, myUpdatedFiles); - } else if (updateMethod == UpdateMethod.REBASE) { - updater = new GitRebaseUpdater(myProject, myGit, root, myTrackedBranches, myProgressIndicator, myUpdatedFiles); - } else { - updater = GitUpdater.getUpdater(myProject, myGit, myTrackedBranches, root, myProgressIndicator, myUpdatedFiles); - } - + GitUpdater updater = GitUpdater.getUpdater(myProject, myGit, myTrackedBranches, root, myProgressIndicator, myUpdatedFiles, + updateMethod); if (updater.isUpdateNeeded()) { updaters.put(root, updater); } diff --git a/plugins/git4idea/src/git4idea/update/GitUpdater.java b/plugins/git4idea/src/git4idea/update/GitUpdater.java index a093aafba89b..42c44df2cf06 100644 --- a/plugins/git4idea/src/git4idea/update/GitUpdater.java +++ b/plugins/git4idea/src/git4idea/update/GitUpdater.java @@ -29,7 +29,7 @@ import git4idea.commands.Git; import git4idea.commands.GitCommand; import git4idea.commands.GitSimpleHandler; import git4idea.config.GitConfigUtil; -import git4idea.config.GitVcsSettings; +import git4idea.config.UpdateMethod; import git4idea.merge.MergeChangeCollector; import git4idea.repo.GitRepositoryManager; import org.jetbrains.annotations.NotNull; @@ -76,43 +76,35 @@ public abstract class GitUpdater { * @return {@link GitMergeUpdater} or {@link GitRebaseUpdater}. */ @NotNull - public static GitUpdater getUpdater(@NotNull Project project, @NotNull Git git, @NotNull Map trackedBranches, - @NotNull VirtualFile root, @NotNull ProgressIndicator progressIndicator, - @NotNull UpdatedFiles updatedFiles) { - final GitVcsSettings settings = GitVcsSettings.getInstance(project); - if (settings == null) { - return getDefaultUpdaterForBranch(project, git, root, trackedBranches, progressIndicator, updatedFiles); + public static GitUpdater getUpdater(@NotNull Project project, + @NotNull Git git, + @NotNull Map trackedBranches, + @NotNull VirtualFile root, + @NotNull ProgressIndicator progressIndicator, + @NotNull UpdatedFiles updatedFiles, + @NotNull UpdateMethod updateMethod) { + if (updateMethod == UpdateMethod.BRANCH_DEFAULT) { + updateMethod = resolveUpdateMethod(project, root); } - switch (settings.getUpdateType()) { - case REBASE: - return new GitRebaseUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles); - case MERGE: - return new GitMergeUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles); - case BRANCH_DEFAULT: - // use default for the branch - return getDefaultUpdaterForBranch(project, git, root, trackedBranches, progressIndicator, updatedFiles); - } - return getDefaultUpdaterForBranch(project, git, root, trackedBranches, progressIndicator, updatedFiles); + return updateMethod == UpdateMethod.REBASE ? + new GitRebaseUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles): + new GitMergeUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles); } @NotNull - private static GitUpdater getDefaultUpdaterForBranch(@NotNull Project project, @NotNull Git git, @NotNull VirtualFile root, - @NotNull Map trackedBranches, - @NotNull ProgressIndicator progressIndicator, @NotNull UpdatedFiles updatedFiles) { - try { - GitLocalBranch branch = GitBranchUtil.getCurrentBranch(project, root); - boolean rebase = false; - if (branch != null) { + public static UpdateMethod resolveUpdateMethod(@NotNull Project project, @NotNull VirtualFile root) { + GitLocalBranch branch = GitBranchUtil.getCurrentBranch(project, root); + boolean rebase = false; + if (branch != null) { + try { String rebaseValue = GitConfigUtil.getValue(project, root, "branch." + branch.getName() + ".rebase"); rebase = rebaseValue != null && rebaseValue.equalsIgnoreCase("true"); } - if (rebase) { - return new GitRebaseUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles); + catch (VcsException e) { + LOG.warn("Couldn't get git config branch." + branch.getName() + ".rebase", e); } - } catch (VcsException e) { - LOG.info("getDefaultUpdaterForBranch branch", e); } - return new GitMergeUpdater(project, git, root, trackedBranches, progressIndicator, updatedFiles); + return rebase ? UpdateMethod.REBASE : UpdateMethod.MERGE; } @NotNull diff --git a/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.java b/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.java index 8f1882e09269..dc6f65b76177 100644 --- a/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.java +++ b/plugins/git4idea/tests/git4idea/push/GitPushOperationSingleRepoTest.java @@ -18,6 +18,7 @@ package git4idea.push; import com.intellij.dvcs.push.PushSpec; import com.intellij.openapi.ui.DialogWrapper; import com.intellij.openapi.ui.Messages; +import com.intellij.openapi.util.Condition; import com.intellij.openapi.util.Ref; import com.intellij.openapi.util.Trinity; import com.intellij.openapi.util.io.FileUtil; @@ -36,6 +37,7 @@ import git4idea.update.GitUpdateResult; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import javax.swing.*; import java.io.File; import java.io.IOException; import java.util.Collection; @@ -47,6 +49,7 @@ import static git4idea.test.GitExecutor.*; import static git4idea.test.GitTestUtil.makeCommit; import static java.util.Collections.singletonMap; +@SuppressWarnings("StringToUpperCaseOrToLowerCaseWithoutLocale") public class GitPushOperationSingleRepoTest extends GitPushOperationBaseTest { protected GitRepository myRepository; @@ -188,6 +191,50 @@ public class GitPushOperationSingleRepoTest extends GitPushOperationBaseTest { assertFalse("The commit shouldn't be pushed", history.contains(hash)); } + public void test_use_selected_update_method_for_all_consecutive_updates() throws IOException { + pushCommitFromBro(); + cd(myRepository); + makeCommit("afile.txt"); + + agreeToUpdate(GitRejectedPushUpdateDialog.REBASE_EXIT_CODE); + + refresh(); + PushSpec pushSpec = makePushSpec(myRepository, "master", "origin/master"); + + GitPushResult result = new GitPushOperation(myProject, myPushSupport, singletonMap(myRepository, pushSpec), null, false) { + boolean updateHappened; + + @NotNull + @Override + protected GitUpdateResult update(@NotNull Collection rootsToUpdate, + @NotNull UpdateMethod updateMethod, + boolean checkForRebaseOverMergeProblem) { + GitUpdateResult updateResult = super.update(rootsToUpdate, updateMethod, checkForRebaseOverMergeProblem); + try { + if (!updateHappened) { + updateHappened = true; + pushCommitFromBro(); + } + } + catch (IOException e) { + throw new RuntimeException(e); + } + return updateResult; + } + }.execute(); + + assertResult(SUCCESS, 1, "master", "origin/master", GitUpdateResult.SUCCESS, result.getResults().get(myRepository)); + cd(myRepository); + String[] commitMessages = StringUtil.splitByLines(log("--pretty=%s")); + boolean mergeCommitsInTheLog = ContainerUtil.exists(commitMessages, new Condition() { + @Override + public boolean value(String s) { + return s.toLowerCase().contains("merge"); + } + }); + assertFalse("Unexpected merge commits when rebase method is selected", mergeCommitsInTheLog); + } + public void test_force_push() throws IOException { String lostHash = pushCommitFromBro(); cd(myRepository); @@ -305,6 +352,64 @@ public class GitPushOperationSingleRepoTest extends GitPushOperationBaseTest { UpdateMethod.REBASE, myGitSettings.getUpdateType()); } + public void test_respect_branch_default_setting_for_rejected_push_dialog() throws IOException { + generateUpdateNeeded(); + myGitSettings.setUpdateType(UpdateMethod.BRANCH_DEFAULT); + git("config branch.master.rebase true"); + + final Ref defaultActionName = Ref.create(); + myDialogManager.registerDialogHandler(GitRejectedPushUpdateDialog.class, new TestDialogHandler() { + @Override + public int handleDialog(@NotNull GitRejectedPushUpdateDialog dialog) { + defaultActionName.set((String)dialog.getDefaultAction().getValue(Action.NAME)); + return DialogWrapper.CANCEL_EXIT_CODE; + } + }); + + push("master", "origin/master"); + assertTrue("Default action in rejected-push dialog is incorrect: " + defaultActionName.get(), + defaultActionName.get().toLowerCase().contains("rebase")); + + git("config branch.master.rebase false"); + push("master", "origin/master"); + assertTrue("Default action in rejected-push dialog is incorrect: " + defaultActionName.get(), + defaultActionName.get().toLowerCase().contains("merge")); + } + + public void test_respect_branch_default_setting_for_silent_update_when_rejected_push() throws IOException { + generateUpdateNeeded(); + myGitSettings.setUpdateType(UpdateMethod.BRANCH_DEFAULT); + git("config branch.master.rebase true"); + myGitSettings.setAutoUpdateIfPushRejected(true); + + push("master", "origin/master"); + assertFalse("Unexpected merge commit: rebase should have happened", log("-1 --pretty=%s").toLowerCase().startsWith("merge")); + } + + // there is no "branch default" choice in the rejected push dialog + // => simply don't rewrite the setting if the same value is chosen, as was default value initially + public void test_dont_overwrite_branch_default_setting_when_agree_in_rejected_push_dialog() throws IOException { + generateUpdateNeeded(); + myGitSettings.setUpdateType(UpdateMethod.BRANCH_DEFAULT); + git("config branch.master.rebase true"); + + myDialogManager.registerDialogHandler(GitRejectedPushUpdateDialog.class, new TestDialogHandler() { + @Override + public int handleDialog(@NotNull GitRejectedPushUpdateDialog dialog) { + return GitRejectedPushUpdateDialog.REBASE_EXIT_CODE; + } + }); + + push("master", "origin/master"); + assertEquals(UpdateMethod.BRANCH_DEFAULT, myGitSettings.getUpdateType()); + } + + private void generateUpdateNeeded() throws IOException { + pushCommitFromBro(); + cd(myRepository); + makeCommit("file.txt"); + } + private void generateUnpushedMergedCommitProblem() throws IOException { pushCommitFromBro(); cd(myRepository);