From c235fafcc12f34618d0b37d9a9b90a80bfe8af37 Mon Sep 17 00:00:00 2001 From: Geoff Lamrock Date: Wed, 19 Mar 2025 08:34:21 +1100 Subject: [PATCH] Fix issues with detecting when to re-parent during rebase --- .../Commands/Helpers/StackHelpersTests.cs | 124 +++++++++++++++++- .../Helpers/TestGitRepositoryBuilder.cs | 5 + src/Stack/Commands/Helpers/StackHelpers.cs | 12 +- src/Stack/Git/GitClient.cs | 14 ++ 4 files changed, 152 insertions(+), 3 deletions(-) diff --git a/src/Stack.Tests/Commands/Helpers/StackHelpersTests.cs b/src/Stack.Tests/Commands/Helpers/StackHelpersTests.cs index 8b06e43d..97f4adea 100644 --- a/src/Stack.Tests/Commands/Helpers/StackHelpersTests.cs +++ b/src/Stack.Tests/Commands/Helpers/StackHelpersTests.cs @@ -135,7 +135,7 @@ public void UpdateStackUsingRebase_WhenARemoteBranchIsDeleted_RebasesOntoThePare // sourceBranch: A commit containing the changes that were made in branch1 but with a different hash e.g. a squash merge // branch1: A commit containing the original changes to the file // branch2: A second commit that changes the file again, building on the one from branch1 - var repo = new TestGitRepositoryBuilder() + using var repo = new TestGitRepositoryBuilder() .WithBranch(b => b .WithName(sourceBranch) .PushToRemote()) @@ -176,4 +176,126 @@ public void UpdateStackUsingRebase_WhenARemoteBranchIsDeleted_RebasesOntoThePare var fileContents = File.ReadAllText(Path.Join(repo.LocalDirectoryPath, changedFilePath)); fileContents.Should().Be(commit2ChangedFileContents); } + + [Fact] + public void UpdateStackUsingRebase_WhenARemoteBranchIsDeleted_ButTheTargetBranchHasAlreadyHadAdditionalCommitsMergedInto_DoesNotRebaseOntoTheParentBranch() + { + // Arrange + var sourceBranch = Some.BranchName(); + var branch1 = Some.BranchName(); + var branch2 = Some.BranchName(); + var changedFilePath = Some.Name(); + var commit1ChangedFileContents = "These are the changes in the first commit"; + var commit2ChangedFileContents = "These are the changes in the first commit, with some additional changes in the second commit"; + + // We have three branches in this scenario: + // + // sourceBranch: A commit containing the changes that were made in branch1 but with a different hash e.g. a squash merge + // branch1: A commit containing the original changes to the file + // branch2: A second commit that changes the file again, building on the one from branch1 + using var repo = new TestGitRepositoryBuilder() + .WithBranch(b => b + .WithName(sourceBranch) + .PushToRemote()) + .WithBranch(b => b + .WithName(branch1) + .FromSourceBranch(sourceBranch) + .WithCommit(c => c.WithChanges(changedFilePath, commit1ChangedFileContents)) + .PushToRemote()) + .WithBranch(b => b + .WithName(branch2) + .FromSourceBranch(branch1) + .WithCommit(c => c.WithChanges(changedFilePath, commit2ChangedFileContents)) + .PushToRemote()) + .Build(); + + var inputProvider = Substitute.For(); + var gitClient = new GitClient(new TestLogger(testOutputHelper), repo.GitClientSettings); + var logger = new TestLogger(testOutputHelper); + var gitHubClient = Substitute.For(); + + // "Merge the PR" by committing the changes into the source branch and deleting the remote branch + gitClient.ChangeBranch(sourceBranch); + File.WriteAllText(Path.Join(repo.LocalDirectoryPath, changedFilePath), commit1ChangedFileContents); + repo.Stage(changedFilePath); + repo.Commit(); + repo.Push(sourceBranch); + + // Rebase the target branch onto the source branch to simulate the update after PR is merged + repo.DeleteRemoteTrackingBranch(branch1); + + gitClient.ChangeBranch(branch2); + gitClient.RebaseOntoNewParent(sourceBranch, branch1); + + // Make another to the source branch + var filePath = Some.Name(); + var newContents = "Here are some new changes."; + gitClient.ChangeBranch(sourceBranch); + File.WriteAllText(Path.Join(repo.LocalDirectoryPath, filePath), newContents); + repo.Stage(filePath); + repo.Commit(); + repo.Push(sourceBranch); + + var stack = new Config.Stack("Stack1", Some.HttpsUri().ToString(), sourceBranch, [branch1, branch2]); + var stackStatus = StackHelpers.GetStackStatus(stack, branch1, logger, gitClient, gitHubClient, false); + + // Act: Even though the parent branch (branch1) has been deleted on the remote, + // we should not explicitly re-parent the target branch (branch2) onto the source branch + // because we've already done that in the past and doing so can cause conflicts. + StackHelpers.UpdateStackUsingRebase(stack, stackStatus, gitClient, inputProvider, logger); + + // Assert + gitClient.ChangeBranch(branch2); + var fileContents = File.ReadAllText(Path.Join(repo.LocalDirectoryPath, filePath)); + fileContents.Should().Be(newContents); + } + + [Fact] + public void UpdateStackUsingRebase_WhenARemoteBranchIsDeleted_AndLocalBranchIsDeleted_DoesNotRebaseOntoTheParentBranch() + { + // Arrange + var sourceBranch = Some.BranchName(); + var branch1 = Some.BranchName(); + var branch2 = Some.BranchName(); + var changedFile1Path = Some.Name(); + var changedFile2Path = Some.Name(); + var changedFile1Contents = "Here are some changes."; + var changedFile2Contents = "Here are some different changes."; + + using var repo = new TestGitRepositoryBuilder() + .WithBranch(b => b.WithName(sourceBranch)) + .WithBranch(b => b.WithName(branch1).FromSourceBranch(sourceBranch)) + .WithBranch(b => b.WithName(branch2).FromSourceBranch(branch1)) + .Build(); + + var inputProvider = Substitute.For(); + var gitClient = new GitClient(new TestLogger(testOutputHelper), repo.GitClientSettings); + var logger = new TestLogger(testOutputHelper); + var gitHubClient = Substitute.For(); + + gitClient.ChangeBranch(sourceBranch); + File.WriteAllText(Path.Join(repo.LocalDirectoryPath, changedFile1Path), changedFile1Contents); + repo.Stage(changedFile1Path); + repo.Commit(); + + repo.DeleteLocalBranch(branch1); + + gitClient.ChangeBranch(branch2); + File.WriteAllText(Path.Join(repo.LocalDirectoryPath, changedFile2Path), changedFile2Contents); + repo.Stage(changedFile2Path); + repo.Commit(); + + var stack = new Config.Stack("Stack1", Some.HttpsUri().ToString(), sourceBranch, [branch1, branch2]); + var stackStatus = StackHelpers.GetStackStatus(stack, branch1, logger, gitClient, gitHubClient, false); + + gitClient.Fetch(true); + + // Act + StackHelpers.UpdateStackUsingRebase(stack, stackStatus, gitClient, inputProvider, logger); + + // Assert + gitClient.ChangeBranch(branch2); + var fileContents = File.ReadAllText(Path.Join(repo.LocalDirectoryPath, changedFile2Path)); + fileContents.Should().Be(changedFile2Contents); + } } \ No newline at end of file diff --git a/src/Stack.Tests/Helpers/TestGitRepositoryBuilder.cs b/src/Stack.Tests/Helpers/TestGitRepositoryBuilder.cs index 3968fdba..e8810539 100644 --- a/src/Stack.Tests/Helpers/TestGitRepositoryBuilder.cs +++ b/src/Stack.Tests/Helpers/TestGitRepositoryBuilder.cs @@ -380,6 +380,11 @@ public void DeleteRemoteTrackingBranch(string branchName) LocalRepository.Branches.Remove(remoteBranchName); } + public void DeleteLocalBranch(string branchName) + { + LocalRepository.Branches.Remove(branchName); + } + public void Push(string branchName) { LocalRepository.Network.Push(LocalRepository.Branches[branchName]); diff --git a/src/Stack/Commands/Helpers/StackHelpers.cs b/src/Stack/Commands/Helpers/StackHelpers.cs index 1cebf346..4bb9e21f 100644 --- a/src/Stack/Commands/Helpers/StackHelpers.cs +++ b/src/Stack/Commands/Helpers/StackHelpers.cs @@ -618,9 +618,17 @@ public static void UpdateStackUsingRebase( if (branchDetail.IsActive) { - if (lowestInactiveBranchToReParentFrom is not null) + var lowestInactiveBranchToReParentFromDetail = lowestInactiveBranchToReParentFrom is not null ? status.Branches[lowestInactiveBranchToReParentFrom] : null; + var shouldRebaseOntoParent = lowestInactiveBranchToReParentFromDetail is not null && lowestInactiveBranchToReParentFromDetail.Status.ExistsLocally; + + if (shouldRebaseOntoParent) + { + shouldRebaseOntoParent = gitClient.IsAncestor(branchToRebaseFrom, lowestInactiveBranchToReParentFrom!); + } + + if (shouldRebaseOntoParent) { - RebaseOntoNewParent(branchToRebaseFrom, branchToRebaseOnto, lowestInactiveBranchToReParentFrom, gitClient, inputProvider, logger); + RebaseOntoNewParent(branchToRebaseFrom, branchToRebaseOnto, lowestInactiveBranchToReParentFrom!, gitClient, inputProvider, logger); } else { diff --git a/src/Stack/Git/GitClient.cs b/src/Stack/Git/GitClient.cs index 6aff14c5..effa7046 100644 --- a/src/Stack/Git/GitClient.cs +++ b/src/Stack/Git/GitClient.cs @@ -50,6 +50,7 @@ public interface IGitClient string GetRootOfRepository(); void OpenFileInEditorAndWaitForClose(string path); string? GetConfigValue(string key); + bool IsAncestor(string ancestor, string descendant); } public class GitClient(ILogger logger, GitClientSettings settings) : IGitClient @@ -283,6 +284,19 @@ public string GetRootOfRepository() return string.IsNullOrEmpty(configValue) ? null : configValue; } + public bool IsAncestor(string ancestor, string descendant) + { + var isAncestor = false; + + ExecuteGitCommand($"merge-base --is-ancestor {ancestor} {descendant}", false, exitCode => + { + isAncestor = exitCode == 0; + return null; + }); + + return isAncestor; + } + public void OpenFileInEditorAndWaitForClose(string path) { var editor = GetConfiguredEditor();