Don't suggest deleting slid-out branch when it has children

VirtusLab · Jan 10, 2025 · 6ea2f03 · 6ea2f03
1 parent 53a2ceb
commit 6ea2f03
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 12 deletions.
diff --git a/CHANGE-NOTES.md b/CHANGE-NOTES.md
@@ -1,6 +1,7 @@
 # Changelog
 
 ## v5.1.2
+- Fixed: don't suggest deleting slid-out branch when it has children (to make work easier for the fork point algorithm)
 
 ## v5.1.1
 - Fixed: compatibility issues with latest 2024.3 EAPs

diff --git a/...tend/actions/src/main/java/com/virtuslab/gitmachete/frontend/actions/common/SlideOut.java b/...tend/actions/src/main/java/com/virtuslab/gitmachete/frontend/actions/common/SlideOut.java
@@ -69,13 +69,31 @@ public void run() {
 
   @ContinuesInBackground
   public void run(@UI Runnable doInUIThreadWhenReady) {
-    val slideOutBranchIsCurrent = branchToSlideOutName.equals(gitRepository.getCurrentBranchName());
-    if (slideOutBranchIsCurrent) {
-      LOG.debug("Skipping (optional) local branch deletion because it is equal to current branch");
+    val branchToSlideOutIsCurrent = branchToSlideOutName.equals(gitRepository.getCurrentBranchName());
+    val branchToSlideOut = branchLayout.getEntryByName(branchToSlideOutName);
+    if (branchToSlideOut == null) {
+      // Unlikely, let's handle this case to calm down Checker Framework.
+      return;
+    }
+    // If a branch has children in machete layout,
+    // it's better to NOT delete it so that fork points for the children are still computed correctly.
+    // See the point on "too many commits taken into rebase" in https://github.com/VirtusLab/git-machete#faq
+    val branchToSlideOutHasChildren = branchToSlideOut.getChildren().nonEmpty();
+    if (branchToSlideOutIsCurrent) {
+      LOG.debug("Skipping (optional) local branch deletion because it is current branch");
+      slideOutBranchAndRunPostSlideOutHookIfPresent(() -> {
+        VcsNotifier.getInstance(project).notifySuccess(/* displayId */ null,
+            /* title */ "",
+            getString("action.GitMachete.SlideOut.notification.title.slide-out-success.is-current.HTML").fmt(
+                branchToSlideOutName));
+        ModalityUiUtil.invokeLaterIfNeeded(ModalityState.NON_MODAL, doInUIThreadWhenReady);
+      });
+    } else if (branchToSlideOutHasChildren) {
+      LOG.debug("Skipping (optional) local branch deletion because it has children");
       slideOutBranchAndRunPostSlideOutHookIfPresent(() -> {
         VcsNotifier.getInstance(project).notifySuccess(/* displayId */ null,
             /* title */ "",
-            getString("action.GitMachete.SlideOut.notification.title.slide-out-success.of-current.HTML").fmt(
+            getString("action.GitMachete.SlideOut.notification.title.slide-out-success.has-children.HTML").fmt(
                 branchToSlideOutName));
         ModalityUiUtil.invokeLaterIfNeeded(ModalityState.NON_MODAL, doInUIThreadWhenReady);
       });

diff --git a/frontend/resourcebundles/src/main/resources/GitMacheteBundle.properties b/frontend/resourcebundles/src/main/resources/GitMacheteBundle.properties
@@ -189,7 +189,8 @@ action.GitMachete.BaseSlideOutAction.description=Slide out ''{0}''
 action.GitMachete.SlideOut.notification.message.slide-out-info.canceled.HTML=<html>Action canceled. Branch <b>{0}</b> has not been slid out</html>
 action.GitMachete.SlideOut.notification.title.slide-out-fail.HTML=<html>Slide out of <b>{0}</b> failed</html>
 action.GitMachete.SlideOut.notification.title.slide-out-info.canceled=Slide out failed
-action.GitMachete.SlideOut.notification.title.slide-out-success.of-current.HTML=<html>Branch <b>{0}</b> has been slid out but not deleted as it is currently checked out</html>
+action.GitMachete.SlideOut.notification.title.slide-out-success.has-children.HTML=<html>Branch <b>{0}</b> has been slid out but not deleted because it had child branches</html>
+action.GitMachete.SlideOut.notification.title.slide-out-success.is-current.HTML=<html>Branch <b>{0}</b> has been slid out but not deleted because it is currently checked out</html>
 action.GitMachete.SlideOut.notification.title.slide-out-success.with-delete.HTML=<html>Branch <b>{0}</b> has been slid out and deleted</html>
 action.GitMachete.SlideOut.notification.title.slide-out-success.without-delete.HTML=<html>Branch <b>{0}</b> has been slid out but not deleted</html>
 string.GitMachete.SlideOut.post-slide-out-hook-task.title=Running machete-post-slide-out hook\u2026

diff --git a/src/uiTest/resources/project.rhino.js b/src/uiTest/resources/project.rhino.js
@@ -571,6 +571,10 @@ function Project(underlyingProject) {
     return hash.asString();
   };
 
+  this.doesBranchExist = function (branchName) {
+    return this.getHashOfCommitPointedByBranch(branchName) != null;
+  }
+
   this.getSyncToParentStatus = function (child) {
     const snapshot = getGraphTable().getGitMacheteRepositorySnapshot();
     const managedBranch = snapshot.getManagedBranchByName(child);

diff --git a/src/uiTest/scala/com/virtuslab/gitmachete/uitest/RunningIntelliJFixtureExtension.scala b/src/uiTest/scala/com/virtuslab/gitmachete/uitest/RunningIntelliJFixtureExtension.scala
@@ -225,6 +225,10 @@ trait RunningIntelliJFixtureExtension extends RobotPluginExtension { this: IdePr
         }
       }
 
+      def doesBranchExist(branch: String): Boolean = {
+        callJs[java.lang.Boolean](s"project.doesBranchExist('$branch')")
+      }
+
       def getCurrentBranchName(): String = {
         callJs[String]("project.getCurrentBranchName()")
       }
@@ -297,10 +301,6 @@ trait RunningIntelliJFixtureExtension extends RobotPluginExtension { this: IdePr
         callJs("project.refreshGraphTableModel().getRowCount()")
       }
 
-      def rejectBranchDeletionOnSlideOut(): Unit = doAndAwait {
-        runJs("project.rejectBranchDeletionOnSlideOut()")
-      }
-
       def resetCurrentToRemote(): Unit = doAndAwait {
         runJs(s"project.resetCurrentToRemote()")
       }

diff --git a/src/uiTest/scala/com/virtuslab/gitmachete/uitest/UITestSuite.scala b/src/uiTest/scala/com/virtuslab/gitmachete/uitest/UITestSuite.scala
@@ -2,7 +2,14 @@ package com.virtuslab.gitmachete.uitest
 
 import com.virtuslab.gitmachete.testcommon.SetupScripts.SETUP_WITH_SINGLE_REMOTE
 import com.virtuslab.gitmachete.testcommon.TestGitRepository
-import org.junit.jupiter.api.Assertions.assertEquals
+import org.junit.jupiter.api.Assertions.{
+  assertEquals,
+  assertFalse,
+  assertNotEquals,
+  assertNotNull,
+  assertNull,
+  assertTrue
+}
 import org.junit.jupiter.api.extension.{ExtendWith, ExtensionContext, TestWatcher}
 import org.junit.jupiter.api.{AfterEach, BeforeEach, Test}
 import org.virtuslab.ideprobe.Extensions._
@@ -108,15 +115,19 @@ class UITestSuite extends TestGitRepository(SETUP_WITH_SINGLE_REMOTE) {
 
     // Let's slide out a root branch now
     project.slideOutSelected("develop")
-    project.acceptBranchDeletionOnSlideOut()
+    // There shouldn't be a branch deletion dialog as `develop` has children (so it will be slid out but not deleted)
+    assertTrue(project.doesBranchExist("develop"))
     branchAndCommitRowsCount = project.refreshModelAndGetRowCount()
     // 6 branch rows (`develop` is no longer there) + 7 commit rows
     // (1 commit of `allow-ownership-link` and 3 commits of `call-ws` are all gone)
     assertEquals(13, branchAndCommitRowsCount)
 
     project.checkoutBranch("master")
+    assertTrue(project.doesBranchExist("call-ws"))
     project.slideOutSelected("call-ws")
-    project.rejectBranchDeletionOnSlideOut()
+    project.acceptBranchDeletionOnSlideOut()
+    assertFalse(project.doesBranchExist("call-ws"))
+
     val managedBranchesAfterSlideOut = project.refreshModelAndGetManagedBranches()
     // Non-existent branches should be skipped while causing no error (only a low-severity notification).
     assertEquals(