From c7163ad6cbdf1a124fb9ad8b8f0b69f91ecc2d59 Mon Sep 17 00:00:00 2001 From: Aleksey Pivovarov Date: Mon, 11 Mar 2019 00:20:41 +0300 Subject: [PATCH] git: fix GitRebaseEditorService handler registration * Remove state from GitInteractiveRebaseEditorHandler (as its Closeable was ignored by everyone) * Remove dangling Disposable from `GitRebaseEditorService.registerHandler` (fix another instance of IDEA-208453) * Use try-with-resources to pass editor handlers to GitHandler follow-up: 509fead23a6650c3481b2ca987e96880e9a19024 --- plugins/git4idea/src/git4idea/GitUtil.java | 17 ++++++ .../GitHandlerAuthenticationManager.java | 19 +------ .../src/git4idea/commands/GitImpl.java | 14 +---- .../rebase/GitAutomaticRebaseEditor.kt | 2 +- .../rebase/GitHandlerRebaseEditorManager.java | 50 ++++++++++++++++ .../GitInteractiveRebaseEditorHandler.java | 34 +---------- .../rebase/GitRebaseEditorHandler.java | 8 --- .../rebase/GitRebaseEditorService.java | 24 +------- .../src/git4idea/rebase/GitRebaser.java | 57 +++++++------------ .../git4idea/tests/git4idea/test/TestGit.kt | 7 +-- 10 files changed, 97 insertions(+), 135 deletions(-) create mode 100644 plugins/git4idea/src/git4idea/rebase/GitHandlerRebaseEditorManager.java diff --git a/plugins/git4idea/src/git4idea/GitUtil.java b/plugins/git4idea/src/git4idea/GitUtil.java index 457ccc2fb8db..bd3710bce340 100644 --- a/plugins/git4idea/src/git4idea/GitUtil.java +++ b/plugins/git4idea/src/git4idea/GitUtil.java @@ -29,6 +29,7 @@ import com.intellij.openapi.vfs.VfsUtil; import com.intellij.openapi.vfs.VirtualFile; import com.intellij.util.Consumer; import com.intellij.util.ObjectUtils; +import com.intellij.util.ThrowableRunnable; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.containers.Convertor; import com.intellij.util.containers.OpenTHashSet; @@ -1023,6 +1024,22 @@ public class GitUtil { VcsImplUtil.proposeUpdateIgnoreFile(project, GitVcs.getInstance(project), ignoreFileRoot); } + public static void tryRunOrClose(@NotNull AutoCloseable closeable, + @NotNull ThrowableRunnable runnable) throws T { + try { + runnable.run(); + } + catch (Throwable e) { + try { + closeable.close(); + } + catch (Throwable e2) { + e.addSuppressed(e2); + } + throw e; + } + } + private static class GitRepositoryNotFoundException extends VcsException { private static final String MESSAGE = "Can't find configured git repository for %s"; diff --git a/plugins/git4idea/src/git4idea/commands/GitHandlerAuthenticationManager.java b/plugins/git4idea/src/git4idea/commands/GitHandlerAuthenticationManager.java index 0506ad6f3d75..6e53b720adda 100644 --- a/plugins/git4idea/src/git4idea/commands/GitHandlerAuthenticationManager.java +++ b/plugins/git4idea/src/git4idea/commands/GitHandlerAuthenticationManager.java @@ -8,10 +8,10 @@ import com.intellij.openapi.util.Key; import com.intellij.openapi.util.SystemInfo; import com.intellij.openapi.util.registry.Registry; import com.intellij.openapi.util.text.StringUtil; -import com.intellij.util.ThrowableRunnable; import com.intellij.util.containers.ContainerUtil; import com.intellij.util.io.URLUtil; import com.intellij.util.net.HttpConfigurable; +import git4idea.GitUtil; import git4idea.config.GitVcsApplicationSettings; import git4idea.config.GitVersionSpecialty; import org.jetbrains.annotations.NonNls; @@ -54,7 +54,7 @@ public class GitHandlerAuthenticationManager implements AutoCloseable { @NotNull public static GitHandlerAuthenticationManager prepare(@NotNull Project project, @NotNull GitLineHandler handler) throws IOException { GitHandlerAuthenticationManager manager = new GitHandlerAuthenticationManager(project, handler); - tryRunOrClose(manager, () -> { + GitUtil.tryRunOrClose(manager, () -> { manager.prepareHttpAuth(); if (GitVcsApplicationSettings.getInstance().isUseIdeaSsh()) { manager.prepareSshAuth(); @@ -212,19 +212,4 @@ public class GitHandlerAuthenticationManager implements AutoCloseable { return httpConfigurable.isProxyException(host); }); } - - private static void tryRunOrClose(@NotNull AutoCloseable closeable, @NotNull ThrowableRunnable runnable) throws IOException { - try { - runnable.run(); - } - catch (Throwable e) { - try { - closeable.close(); - } - catch (Throwable e2) { - e.addSuppressed(e2); - } - throw e; - } - } } diff --git a/plugins/git4idea/src/git4idea/commands/GitImpl.java b/plugins/git4idea/src/git4idea/commands/GitImpl.java index 98cee5207d34..cbfc9424a849 100644 --- a/plugins/git4idea/src/git4idea/commands/GitImpl.java +++ b/plugins/git4idea/src/git4idea/commands/GitImpl.java @@ -16,10 +16,7 @@ import com.intellij.vcsUtil.VcsFileUtil; import git4idea.branch.GitRebaseParams; import git4idea.config.GitVersionSpecialty; import git4idea.push.GitPushParams; -import git4idea.rebase.GitInteractiveRebaseEditorHandler; -import git4idea.rebase.GitRebaseEditorHandler; -import git4idea.rebase.GitRebaseEditorService; -import git4idea.rebase.GitRebaseResumeMode; +import git4idea.rebase.*; import git4idea.repo.GitRemote; import git4idea.repo.GitRepository; import git4idea.reset.GitResetMode; @@ -711,17 +708,12 @@ public class GitImpl extends GitImplBase { @NotNull private GitRebaseCommandResult runWithEditor(@NotNull GitLineHandler handler, @NotNull GitRebaseEditorHandler editorHandler) { - GitRebaseEditorService service = GitRebaseEditorService.getInstance(); - service.configureHandler(handler, editorHandler.getHandlerNo()); - try { + try (GitHandlerRebaseEditorManager ignored = GitHandlerRebaseEditorManager.prepareEditor(handler, editorHandler)) { GitCommandResult result = runCommand(handler); if (editorHandler.wasCommitListEditorCancelled()) return GitRebaseCommandResult.cancelledInCommitList(result); if (editorHandler.wasUnstructuredEditorCancelled()) return GitRebaseCommandResult.cancelledInCommitMessage(result); return GitRebaseCommandResult.normal(result); } - finally { - service.unregisterHandler(editorHandler.getHandlerNo()); - } } @VisibleForTesting @@ -730,7 +722,7 @@ public class GitImpl extends GitImplBase { @NotNull VirtualFile root, @NotNull GitLineHandler handler, boolean commitListAware) { - GitInteractiveRebaseEditorHandler editor = new GitInteractiveRebaseEditorHandler(GitRebaseEditorService.getInstance(), project, root); + GitInteractiveRebaseEditorHandler editor = new GitInteractiveRebaseEditorHandler(project, root); if (!commitListAware) { editor.setRebaseEditorShown(); } diff --git a/plugins/git4idea/src/git4idea/rebase/GitAutomaticRebaseEditor.kt b/plugins/git4idea/src/git4idea/rebase/GitAutomaticRebaseEditor.kt index bf4b43bd7192..ee6954c060ad 100644 --- a/plugins/git4idea/src/git4idea/rebase/GitAutomaticRebaseEditor.kt +++ b/plugins/git4idea/src/git4idea/rebase/GitAutomaticRebaseEditor.kt @@ -27,7 +27,7 @@ internal class GitAutomaticRebaseEditor(private val project: Project, private val root: VirtualFile, private val entriesEditor: (List) -> List, private val plainTextEditor: (String) -> String -) : GitInteractiveRebaseEditorHandler(GitRebaseEditorService.getInstance(), project, root) { +) : GitInteractiveRebaseEditorHandler(project, root) { val LOG = logger() override fun editCommits(path: String): Int { diff --git a/plugins/git4idea/src/git4idea/rebase/GitHandlerRebaseEditorManager.java b/plugins/git4idea/src/git4idea/rebase/GitHandlerRebaseEditorManager.java new file mode 100644 index 000000000000..61f79ae0bd8a --- /dev/null +++ b/plugins/git4idea/src/git4idea/rebase/GitHandlerRebaseEditorManager.java @@ -0,0 +1,50 @@ +// Copyright 2000-2019 JetBrains s.r.o. Use of this source code is governed by the Apache 2.0 license that can be found in the LICENSE file. +package git4idea.rebase; + +import git4idea.GitUtil; +import git4idea.commands.GitCommand; +import git4idea.commands.GitHandler; +import org.jetbrains.annotations.NotNull; +import org.jetbrains.annotations.Nullable; + +import java.util.UUID; + +public class GitHandlerRebaseEditorManager implements AutoCloseable { + @NotNull private final GitHandler myHandler; + @NotNull private final GitRebaseEditorHandler myEditorHandler; + @NotNull private final GitRebaseEditorService myService; + + @Nullable private UUID myHandlerId; + + /** + * Configure handler with editor + */ + @NotNull + public static GitHandlerRebaseEditorManager prepareEditor(GitHandler h, @NotNull GitRebaseEditorHandler editorHandler) { + GitHandlerRebaseEditorManager manager = new GitHandlerRebaseEditorManager(h, editorHandler); + GitUtil.tryRunOrClose(manager, () -> { + manager.prepareEditor(); + }); + return manager; + } + + private GitHandlerRebaseEditorManager(@NotNull GitHandler handler, @NotNull GitRebaseEditorHandler editorHandler) { + myHandler = handler; + myEditorHandler = editorHandler; + myService = GitRebaseEditorService.getInstance(); + } + + private void prepareEditor() { + myHandlerId = myService.registerHandler(myEditorHandler); + myHandler.addCustomEnvironmentVariable(GitCommand.GIT_EDITOR_ENV, myService.getEditorCommand()); + myHandler.addCustomEnvironmentVariable(GitRebaseEditorMain.IDEA_REBASE_HANDER_NO, myHandlerId.toString()); + } + + @Override + public void close() { + if (myHandlerId != null) { + myService.unregisterHandler(myHandlerId); + myHandlerId = null; + } + } +} diff --git a/plugins/git4idea/src/git4idea/rebase/GitInteractiveRebaseEditorHandler.java b/plugins/git4idea/src/git4idea/rebase/GitInteractiveRebaseEditorHandler.java index 0c594c961229..42e60c013d36 100644 --- a/plugins/git4idea/src/git4idea/rebase/GitInteractiveRebaseEditorHandler.java +++ b/plugins/git4idea/src/git4idea/rebase/GitInteractiveRebaseEditorHandler.java @@ -15,11 +15,9 @@ import one.util.streamex.StreamEx; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; -import java.io.Closeable; import java.io.File; import java.io.IOException; import java.util.List; -import java.util.UUID; import static com.intellij.CommonBundle.getCancelButtonText; import static com.intellij.CommonBundle.getOkButtonText; @@ -34,13 +32,10 @@ import static git4idea.rebase.GitRebaseEditorMain.ERROR_EXIT_CODE; * dialog with the specified file. If user accepts the changes, it saves file and returns 0, * otherwise it just returns error code. */ -public class GitInteractiveRebaseEditorHandler implements Closeable, GitRebaseEditorHandler { +public class GitInteractiveRebaseEditorHandler implements GitRebaseEditorHandler { private final static Logger LOG = Logger.getInstance(GitInteractiveRebaseEditorHandler.class); - private final GitRebaseEditorService myService; private final Project myProject; private final VirtualFile myRoot; - @NotNull private final UUID myHandlerNo; - private boolean myIsClosed; /** * If interactive rebase editor (with the list of commits) was shown, this is true. @@ -51,16 +46,13 @@ public class GitInteractiveRebaseEditorHandler implements Closeable, GitRebaseEd private boolean myCommitListCancelled; private boolean myUnstructuredEditorCancelled; - public GitInteractiveRebaseEditorHandler(@NotNull GitRebaseEditorService service, @NotNull Project project, @NotNull VirtualFile root) { - myService = service; + public GitInteractiveRebaseEditorHandler(@NotNull Project project, @NotNull VirtualFile root) { myProject = project; myRoot = root; - myHandlerNo = service.registerHandler(this, project); } @Override public int editCommits(@NotNull String path) { - ensureOpen(); try { if (myRebaseEditorShown) { myUnstructuredEditorCancelled = !handleUnstructuredEditor(path); @@ -170,28 +162,6 @@ public class GitInteractiveRebaseEditorHandler implements Closeable, GitRebaseEd myRebaseEditorShown = true; } - /** - * Check that handler has not yet been closed - */ - private void ensureOpen() { - if (myIsClosed) { - throw new IllegalStateException("The handler was already closed"); - } - } - - @Override - public void close() { - ensureOpen(); - myIsClosed = true; - myService.unregisterHandler(myHandlerNo); - } - - @Override - @NotNull - public UUID getHandlerNo() { - return myHandlerNo; - } - @Override public boolean wasCommitListEditorCancelled() { return myCommitListCancelled; diff --git a/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorHandler.java b/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorHandler.java index b48e30a409e8..755c4528d0eb 100644 --- a/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorHandler.java +++ b/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorHandler.java @@ -17,8 +17,6 @@ package git4idea.rebase; import org.jetbrains.annotations.NotNull; -import java.util.UUID; - /** *

Serves as the GIT_EDITOR during interactive rebase: it is called by Git instead of vim, * and allows to edit the list of rebased commits, and to reword commit messages.

@@ -35,12 +33,6 @@ public interface GitRebaseEditorHandler { */ int editCommits(@NotNull String path); - /** - * Unique number of the handler registered in the {@link com.intellij.ide.XmlRpcServer} - */ - @NotNull - UUID getHandlerNo(); - /** * Tells if the interactive rebase editor (with the list of commits to rebase) was cancelled by user. * @see #wasUnstructuredEditorCancelled() diff --git a/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorService.java b/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorService.java index 39a14d10b250..ddc7750c600a 100644 --- a/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorService.java +++ b/plugins/git4idea/src/git4idea/rebase/GitRebaseEditorService.java @@ -16,12 +16,8 @@ package git4idea.rebase; import com.intellij.ide.XmlRpcServer; -import com.intellij.openapi.Disposable; import com.intellij.openapi.components.ServiceManager; -import com.intellij.openapi.util.Disposer; import com.intellij.util.containers.ContainerUtil; -import git4idea.commands.GitCommand; -import git4idea.commands.GitLineHandler; import org.apache.commons.codec.DecoderException; import org.apache.xmlrpc.XmlRpcClientLite; import org.jetbrains.annotations.NonNls; @@ -101,17 +97,11 @@ public class GitRebaseEditorService { * @return the handler identifier */ @NotNull - public UUID registerHandler(@NotNull GitRebaseEditorHandler handler, @NotNull Disposable parentDisposable) { + public UUID registerHandler(@NotNull GitRebaseEditorHandler handler) { addInternalHandler(); synchronized (myHandlersLock) { UUID key = UUID.randomUUID(); myHandlers.put(key, handler); - Disposer.register(parentDisposable, new Disposable() { - @Override - public void dispose() { - myHandlers.remove(key); - } - }); return key; } } @@ -145,18 +135,6 @@ public class GitRebaseEditorService { } } - /** - * Configure handler with editor - * - * @param h the handler to configure - * @param editorNo the editor number - */ - public void configureHandler(GitLineHandler h, @NotNull UUID editorNo) { - h.addCustomEnvironmentVariable(GitCommand.GIT_EDITOR_ENV, getEditorCommand()); - h.addCustomEnvironmentVariable(GitRebaseEditorMain.IDEA_REBASE_HANDER_NO, editorNo.toString()); - } - - /** * The internal xml rcp handler */ diff --git a/plugins/git4idea/src/git4idea/rebase/GitRebaser.java b/plugins/git4idea/src/git4idea/rebase/GitRebaser.java index cf29edf6c105..53dd2db39858 100644 --- a/plugins/git4idea/src/git4idea/rebase/GitRebaser.java +++ b/plugins/git4idea/src/git4idea/rebase/GitRebaser.java @@ -131,20 +131,14 @@ public class GitRebaser { final GitRebaseProblemDetector rebaseConflictDetector = new GitRebaseProblemDetector(); rh.addLineListener(rebaseConflictDetector); - makeContinueRebaseInteractiveEditor(root, rh); - - final GitTask rebaseTask = new GitTask(myProject, rh, "git rebase " + startOperation); - rebaseTask.setProgressAnalyzer(new GitStandardProgressAnalyzer()); - rebaseTask.setProgressIndicator(myProgressIndicator); - return executeRebaseTaskInBackground(root, rh, rebaseConflictDetector, rebaseTask); - } - - protected void makeContinueRebaseInteractiveEditor(VirtualFile root, GitLineHandler rh) { - GitRebaseEditorService rebaseEditorService = GitRebaseEditorService.getInstance(); // TODO If interactive rebase with commit rewording was invoked, this should take the reworded message - GitRebaser.TrivialEditor editor = new GitRebaser.TrivialEditor(rebaseEditorService, myProject, root); - UUID rebaseEditorNo = editor.getHandlerNo(); - rebaseEditorService.configureHandler(rh, rebaseEditorNo); + GitRebaser.TrivialEditor editor = new GitRebaser.TrivialEditor(myProject, root); + try (GitHandlerRebaseEditorManager ignored = GitHandlerRebaseEditorManager.prepareEditor(rh, editor)) { + final GitTask rebaseTask = new GitTask(myProject, rh, "git rebase " + startOperation); + rebaseTask.setProgressAnalyzer(new GitStandardProgressAnalyzer()); + rebaseTask.setProgressIndicator(myProgressIndicator); + return executeRebaseTaskInBackground(root, rh, rebaseConflictDetector, rebaseTask); + } } /** @@ -176,27 +170,17 @@ public class GitRebaser { final GitLineHandler h = new GitLineHandler(myProject, root, GitCommand.REBASE); h.setStdoutSuppressed(false); - UUID rebaseEditorNo = null; - GitRebaseEditorService rebaseEditorService = GitRebaseEditorService.getInstance(); - try { - h.addParameters("-i", "-m", "-v"); - h.addParameters(parentCommit); + h.addParameters("-i", "-m", "-v"); + h.addParameters(parentCommit); - final GitRebaseProblemDetector rebaseConflictDetector = new GitRebaseProblemDetector(); - h.addLineListener(rebaseConflictDetector); - - final PushRebaseEditor pushRebaseEditor = new PushRebaseEditor(rebaseEditorService, root, olderCommits, false); - rebaseEditorNo = pushRebaseEditor.getHandlerNo(); - rebaseEditorService.configureHandler(h, rebaseEditorNo); + final GitRebaseProblemDetector rebaseConflictDetector = new GitRebaseProblemDetector(); + h.addLineListener(rebaseConflictDetector); + final PushRebaseEditor pushRebaseEditor = new PushRebaseEditor(root, olderCommits, false); + try (GitHandlerRebaseEditorManager ignored = GitHandlerRebaseEditorManager.prepareEditor(h, pushRebaseEditor)) { final GitTask rebaseTask = new GitTask(myProject, h, "Reordering commits"); rebaseTask.setProgressIndicator(myProgressIndicator); return executeRebaseTaskInBackground(root, h, rebaseConflictDetector, rebaseTask); - } finally { // TODO should be unregistered in the task.success - // unregistering rebase service - if (rebaseEditorNo != null) { - rebaseEditorService.unregisterHandler(rebaseEditorNo); - } } } @@ -288,10 +272,8 @@ public class GitRebaser { } public static class TrivialEditor extends GitInteractiveRebaseEditorHandler{ - public TrivialEditor(@NotNull GitRebaseEditorService service, - @NotNull Project project, - @NotNull VirtualFile root) { - super(service, project, root); + public TrivialEditor(@NotNull Project project, @NotNull VirtualFile root) { + super(project, root); } @Override @@ -374,11 +356,10 @@ public class GitRebaser { * @param commits the reordered commits * @param hasMerges if true, the vcs root has merges */ - PushRebaseEditor(GitRebaseEditorService rebaseEditorService, - final VirtualFile root, - List commits, - boolean hasMerges) { - super(rebaseEditorService, myProject, root); + PushRebaseEditor(final VirtualFile root, + List commits, + boolean hasMerges) { + super(myProject, root); myCommits = commits; myHasMerges = hasMerges; } diff --git a/plugins/git4idea/tests/git4idea/test/TestGit.kt b/plugins/git4idea/tests/git4idea/test/TestGit.kt index 27c8022b16de..5718d8761d4c 100644 --- a/plugins/git4idea/tests/git4idea/test/TestGit.kt +++ b/plugins/git4idea/tests/git4idea/test/TestGit.kt @@ -9,7 +9,6 @@ import git4idea.branch.GitRebaseParams import git4idea.commands.* import git4idea.push.GitPushParams import git4idea.rebase.GitInteractiveRebaseEditorHandler -import git4idea.rebase.GitRebaseEditorService import git4idea.repo.GitRepository import java.io.File @@ -80,8 +79,7 @@ class TestGitImpl : GitImpl() { commitListAware: Boolean): GitInteractiveRebaseEditorHandler { if (interactiveRebaseEditor == null) return super.createEditor(project, root, handler, commitListAware) - val service = GitRebaseEditorService.getInstance() - val editor = object: GitInteractiveRebaseEditorHandler(service, project, root) { + val editor = object : GitInteractiveRebaseEditorHandler(project, root) { override fun handleUnstructuredEditor(path: String): Boolean { val plainTextEditor = interactiveRebaseEditor!!.plainTextEditor return if (plainTextEditor != null) handleEditor(path, plainTextEditor) else super.handleUnstructuredEditor(path) @@ -97,13 +95,12 @@ class TestGitImpl : GitImpl() { val file = File(path) FileUtil.writeToFile(file, editor(FileUtil.loadFile(file))) } - catch(e: Exception) { + catch (e: Exception) { LOG.error(e) } return true } } - service.configureHandler(handler, editor.handlerNo) return editor }