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: 509fead23a

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 <T extends Throwable> void tryRunOrClose(@NotNull AutoCloseable closeable,
+                                                         @NotNull ThrowableRunnable<T> 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";
 

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<IOException> runnable) throws IOException {
-    try {
-      runnable.run();
-    }
-    catch (Throwable e) {
-      try {
-        closeable.close();
-      }
-      catch (Throwable e2) {
-        e.addSuppressed(e2);
-      }
-      throw e;
-    }
-  }
 }

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();
     }

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<GitRebaseEntry>) -> List<GitRebaseEntry>,
                                         private val plainTextEditor: (String) -> String
-) : GitInteractiveRebaseEditorHandler(GitRebaseEditorService.getInstance(), project, root) {
+) : GitInteractiveRebaseEditorHandler(project, root) {
   val LOG = logger<GitAutomaticRebaseEditor>()
 
   override fun editCommits(path: String): Int {

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;
+    }
+  }
+}

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;

plugins/git4idea/src/git4idea/rebase/GitRebaseEditorHandler.java

@@ -17,8 +17,6 @@ package git4idea.rebase;
 
 import org.jetbrains.annotations.NotNull;
 
-import java.util.UUID;
-
 /**
  * <p>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.</p>
@@ -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()

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
    */

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<String> commits,
-                            boolean hasMerges) {
-      super(rebaseEditorService, myProject, root);
+    PushRebaseEditor(final VirtualFile root,
+                     List<String> commits,
+                     boolean hasMerges) {
+      super(myProject, root);
       myCommits = commits;
       myHasMerges = hasMerges;
     }

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
   }