diff --git a/apps/desktop/src/components/commit/CommitFailedFileEntry.svelte b/apps/desktop/src/components/commit/CommitFailedFileEntry.svelte index 8a54e9d828d..f50b628d708 100644 --- a/apps/desktop/src/components/commit/CommitFailedFileEntry.svelte +++ b/apps/desktop/src/components/commit/CommitFailedFileEntry.svelte @@ -21,7 +21,11 @@ let isFolded = $state(true); function reasonRelatedToDependencyInfo(reason: RejectionReason): boolean { - return reason === "cherryPickMergeConflict" || reason === "workspaceMergeConflict"; + return ( + reason === "cherryPickMergeConflict" || + reason === "incompatibleBase" || + reason === "workspaceMergeConflict" + ); } diff --git a/apps/desktop/src/lib/stacks/stackEndpoints.ts b/apps/desktop/src/lib/stacks/stackEndpoints.ts index efb8331cab3..ab762783168 100644 --- a/apps/desktop/src/lib/stacks/stackEndpoints.ts +++ b/apps/desktop/src/lib/stacks/stackEndpoints.ts @@ -85,6 +85,7 @@ export const REJECTTION_REASONS = [ "workspaceMergeConflict", "workspaceMergeConflictOfUnrelatedFile", "cherryPickMergeConflict", + "incompatibleBase", "noEffectiveChanges", "worktreeFileMissingForObjectConversion", "fileToLargeOrBinary", @@ -105,6 +106,8 @@ export function readableRejectionReason(reason: RejectionReason): string { switch (reason) { case "cherryPickMergeConflict": return "Cherry-pick merge conflict"; + case "incompatibleBase": + return "Incompatible branch base"; case "noEffectiveChanges": return "No effective changes"; case "workspaceMergeConflict": diff --git a/apps/desktop/src/lib/state/uiState.svelte.ts b/apps/desktop/src/lib/state/uiState.svelte.ts index 10011541d8e..a9522dca3c1 100644 --- a/apps/desktop/src/lib/state/uiState.svelte.ts +++ b/apps/desktop/src/lib/state/uiState.svelte.ts @@ -27,6 +27,7 @@ export type RejectionReason = | "workspaceMergeConflict" | "workspaceMergeConflictOfUnrelatedFile" | "cherryPickMergeConflict" + | "incompatibleBase" | "noEffectiveChanges" | "worktreeFileMissingForObjectConversion" | "fileToLargeOrBinary" diff --git a/apps/lite/ui/src/operations/rejectedChangesToastOptions.tsx b/apps/lite/ui/src/operations/rejectedChangesToastOptions.tsx index d8acb89b157..8aa057911ac 100644 --- a/apps/lite/ui/src/operations/rejectedChangesToastOptions.tsx +++ b/apps/lite/ui/src/operations/rejectedChangesToastOptions.tsx @@ -19,6 +19,8 @@ const readableRejectionReason = (reason: RejectionReason): string => { switch (reason) { case "cherryPickMergeConflict": return "Cherry-pick merge conflict"; + case "incompatibleBase": + return "Incompatible branch base"; case "noEffectiveChanges": return "No effective changes"; case "workspaceMergeConflict": diff --git a/crates/but-core/src/tree/mod.rs b/crates/but-core/src/tree/mod.rs index e754eb95265..d577c920120 100644 --- a/crates/but-core/src/tree/mod.rs +++ b/crates/but-core/src/tree/mod.rs @@ -22,6 +22,9 @@ pub mod create_tree { NoEffectiveChanges, /// The final cherry-pick to bring the new tree down onto the target tree (merge it in) failed with a conflict. CherryPickMergeConflict, + /// The file at this path has a different version in the workspace base tree (`HEAD^{tree}`) + /// than in the target stack's tree, and this base divergence caused the cherry-pick to fail. + IncompatibleBase, /// The final merge of the workspace commit failed with a conflict. WorkspaceMergeConflict, /// The final merge of the workspace commit failed with a conflict, @@ -107,7 +110,11 @@ pub fn create_tree( // Some rejections are OK, and we want to create a commit anyway. || !matches!( c, - Err((RejectionReason::CherryPickMergeConflict,_)) + Err(( + RejectionReason::CherryPickMergeConflict + | RejectionReason::IncompatibleBase, + _ + )) ) }) { changes @@ -142,12 +149,35 @@ pub fn create_tree( }) .collect(); if !unresolved_conflicts.is_empty() { + let base_tree = base.attach(repo).object()?.peel_to_tree()?; + let ours_tree = ours.attach(repo).object()?.peel_to_tree()?; for change in changes.iter_mut().filter(|c| { c.as_ref().ok().is_some_and(|change| { unresolved_conflicts.contains(&change.path.as_bstr()) }) }) { - into_err_spec(change, RejectionReason::CherryPickMergeConflict); + let path = change + .as_ref() + .ok() + .map(|c| c.path.as_bstr()) + .expect("filter above ensures Ok"); + let path_ref = gix::path::from_bstr(path); + let base_entry_id = base_tree + .lookup_entry_by_path(path_ref.as_ref()) + .ok() + .flatten() + .map(|e| e.object_id()); + let ours_entry_id = ours_tree + .lookup_entry_by_path(path_ref.as_ref()) + .ok() + .flatten() + .map(|e| e.object_id()); + let reason = if base_entry_id != ours_entry_id { + RejectionReason::IncompatibleBase + } else { + RejectionReason::CherryPickMergeConflict + }; + into_err_spec(change, reason); } continue 'retry; } diff --git a/crates/but-workspace/tests/fixtures/scenario/two-stacks-modifying-shared-file.sh b/crates/but-workspace/tests/fixtures/scenario/two-stacks-modifying-shared-file.sh new file mode 100755 index 00000000000..439253334fb --- /dev/null +++ b/crates/but-workspace/tests/fixtures/scenario/two-stacks-modifying-shared-file.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash + +source "${BASH_SOURCE[0]%/*}/shared.sh" + +git init +echo "two stacks with different bases, upstream modified file, uncommitted changes" >.git/description + +# Initial base with a file +echo "original" >file.txt +git add file.txt +commit "initial" + +# Advance main: upstream modifies file.txt +echo "upstream-changed" >file.txt +git add file.txt +commit "upstream changes file" +setup_target_to_match_main + +# Stack A: fork from main~1 (old base, before upstream changed file.txt) +git checkout -b A main~1 +echo "A-content" >a-only.txt +git add a-only.txt +commit "A adds a-only" + +# Stack B: fork from main (new base, includes upstream's change to file.txt) +git checkout -b B main +echo "B-content" >b-only.txt +git add b-only.txt +commit "B adds b-only" + +# Create workspace merge commit from both stacks +# B is current (has upstream-changed file.txt), merge A in +# The merge base between A and B is main~1 where file.txt="original" +# A has file.txt="original" (unchanged from its base), B has file.txt="upstream-changed" +# So the merge picks B's (upstream) version of file.txt for the workspace tree. +create_workspace_commit_once B A + +# Uncommitted worktree change: modify file.txt. +# This change is relative to the workspace merge tree (which has "upstream-changed"). +# When committing to stack A, the cherry-pick does: +# base = HEAD^{tree} (workspace: file.txt="upstream-changed") +# ours = A-tip^{tree} (A: file.txt="original") +# theirs = HEAD^{tree} + changes (file.txt="worktree-modified") +# The cherry-pick sees (base→ours) changed "upstream-changed" to "original" +# and (base→theirs) changed "upstream-changed" to "worktree-modified" → CONFLICT +echo "worktree-modified" >file.txt diff --git a/crates/but-workspace/tests/workspace/commit/commit_create.rs b/crates/but-workspace/tests/workspace/commit/commit_create.rs index 60617b655bc..9e17a748cd6 100644 --- a/crates/but-workspace/tests/workspace/commit/commit_create.rs +++ b/crates/but-workspace/tests/workspace/commit/commit_create.rs @@ -6,7 +6,10 @@ use but_rebase::graph_rebase::{ }; use but_workspace::commit::commit_create; -use crate::ref_info::with_workspace_commit::utils::named_writable_scenario_with_description_and_graph as writable_scenario; +use crate::ref_info::with_workspace_commit::utils::{ + StackState, add_stack_with_segments, + named_writable_scenario_with_description_and_graph as writable_scenario, +}; fn worktree_changes_as_specs(repo: &gix::Repository) -> Result> { Ok(but_core::diff::worktree_changes(repo)? @@ -234,3 +237,53 @@ fn commit_all_rejected_is_noop() -> Result<()> { Ok(()) } + +/// When stacks have different bases (e.g. one forked before an upstream +/// change and another after), the workspace merge tree (`HEAD^{tree}`) +/// contains the upstream version of a file while the older stack's tree +/// does not. +/// +/// The cherry-pick in `create_tree` correctly detects this as a conflict: +/// base (`HEAD^{tree}`) has the upstream content, ours (old stack parent) +/// has the pre-upstream content, and theirs has the worktree edit — a +/// genuine three-way conflict. +#[test] +fn commit_to_wrong_base_rejects_conflicting_file() -> Result<()> { + let (_tmp, graph, repo, mut meta, _description) = + writable_scenario("two-stacks-modifying-shared-file", |meta| { + add_stack_with_segments(meta, 0, "A", StackState::InWorkspace, &[]); + add_stack_with_segments(meta, 1, "B", StackState::InWorkspace, &[]); + })?; + + // Stack A has the OLD base (before upstream changed file.txt). + // Committing a worktree change to file.txt on A should be rejected. + let a_ref = repo.find_reference("A")?; + + let mut ws = graph.into_workspace()?; + let editor = Editor::create(&mut ws, &mut meta, &repo)?; + let outcome = commit_create( + editor, + worktree_changes_as_specs(&repo)?, + RelativeToRef::Reference(a_ref.name()), + InsertSide::Above, + "commit worktree change to stack A", + 0, + )?; + + assert!( + !outcome.rejected_specs.is_empty(), + "file.txt change should be rejected on the old-base stack" + ); + assert!( + outcome.rejected_specs.iter().any(|(reason, spec)| { + matches!( + reason, + but_core::tree::create_tree::RejectionReason::IncompatibleBase + ) && spec.path == "file.txt" + }), + "expected IncompatibleBase for file.txt, got: {:?}", + outcome.rejected_specs + ); + + Ok(()) +} diff --git a/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs b/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs index 8349563dbb7..21fc33ceae6 100644 --- a/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs +++ b/crates/but-workspace/tests/workspace/commit_engine/new_commit.rs @@ -960,7 +960,7 @@ fn commit_whole_file_to_conflicting_position() -> anyhow::Result<()> { insta::assert_debug_snapshot!(outcome.rejected_specs, @r#" [ ( - CherryPickMergeConflict, + IncompatibleBase, DiffSpec { previous_path: None, path: "file", @@ -1017,7 +1017,7 @@ fn commit_whole_file_to_conflicting_position_one_unconflicting_file_remains() -> insta::assert_debug_snapshot!(outcome.rejected_specs, @r#" [ ( - CherryPickMergeConflict, + IncompatibleBase, DiffSpec { previous_path: None, path: "file", diff --git a/crates/but/src/command/legacy/status/tui/mod.rs b/crates/but/src/command/legacy/status/tui/mod.rs index 686b59e0488..a31f53f44ff 100644 --- a/crates/but/src/command/legacy/status/tui/mod.rs +++ b/crates/but/src/command/legacy/status/tui/mod.rs @@ -1610,6 +1610,7 @@ impl App { .map(|(rejection_reason, diff_spec)| { let human_reason = match rejection_reason { RejectionReason::NoEffectiveChanges => "Changes were a no-op", + RejectionReason::IncompatibleBase => "Incompatible branch base, rebase target", RejectionReason::CherryPickMergeConflict | RejectionReason::WorkspaceMergeConflict | RejectionReason::WorkspaceMergeConflictOfUnrelatedFile => { diff --git a/e2e/playwright/scripts/project-with-different-base-stacks.sh b/e2e/playwright/scripts/project-with-different-base-stacks.sh new file mode 100755 index 00000000000..4481c0daeff --- /dev/null +++ b/e2e/playwright/scripts/project-with-different-base-stacks.sh @@ -0,0 +1,65 @@ +#!/bin/bash + +echo "GIT CONFIG $GIT_CONFIG_GLOBAL" +echo "DATA DIR $E2E_TEST_APP_DATA_DIR" +echo "BUT $BUT" + +# Setup a remote project with two branches that fork from DIFFERENT master +# commits. This causes stacks to have different merge bases with the target, +# which triggers IncompatibleBase rejections when committing a shared file +# to the wrong stack. +# +# master: init ──── upstream-change +# │ │ +# old-stack new-stack +# (doesn't have (has the upstream +# upstream change) change) + +mkdir remote-project +pushd remote-project +git init -b master --object-format=sha1 + +# Initial commit with a shared file +cat > shared_file << 'CONTENT' +alpha +bravo +charlie +delta +echo +foxtrot +golf +hotel +india +juliet +CONTENT +git add shared_file +git commit -m "Initial commit with shared_file" + +# Create old-stack branch from here (before the upstream change) +git checkout -b old-stack +echo "old-stack work" >> other_file +git add other_file +git commit -m "old-stack: add other_file" +git checkout master + +# Now advance master with a change to shared_file +sed 's/juliet/JULIET-UPSTREAM/' shared_file > shared_file.tmp && mv shared_file.tmp shared_file +git commit -am "Upstream change to shared_file" + +# Create new-stack branch from updated master (has the upstream change) +git checkout -b new-stack +echo "new-stack work" >> another_file +git add another_file +git commit -m "new-stack: add another_file" +git checkout master +popd + +# Clone the remote and set up GitButler +git clone remote-project local-clone +pushd local-clone + git checkout master + target_branch="$(git rev-parse --symbolic-full-name @{u})" + target_branch="${target_branch#refs/remotes/}" + "$BUT" setup + "$BUT" config target "$target_branch" +popd diff --git a/e2e/playwright/tests/incompatibleBase.spec.ts b/e2e/playwright/tests/incompatibleBase.spec.ts new file mode 100644 index 00000000000..8e8ab633ec0 --- /dev/null +++ b/e2e/playwright/tests/incompatibleBase.spec.ts @@ -0,0 +1,126 @@ +import { getBaseURL, type GitButler, startGitButler } from "../src/setup.ts"; +import { test } from "../src/test.ts"; +import { + clickByTestId, + dragAndDropByLocator, + fillByTestId, + getByTestId, + textEditorFillByTestId, + waitForTestId, +} from "../src/util.ts"; +import { expect, type Page } from "@playwright/test"; +import { writeFileSync } from "fs"; + +let gitbutler: GitButler; + +test.use({ + baseURL: getBaseURL(), +}); + +test.afterEach(async () => { + await gitbutler?.destroy(); +}); + +const SHARED_FILE_CONTENT = [ + "alpha", + "bravo", + "charlie", + "delta", + "echo", + "foxtrot", + "golf", + "hotel", + "india", + "JULIET-MODIFIED-BY-USER", +] + .join("\n") + .concat("\n"); + +/** + * Set up two stacks with different merge bases and write a worktree change + * to shared_file. Returns the file locator for the uncommitted change. + */ +async function setupDifferentBaseStacks(gitbutler: GitButler, page: Page) { + await gitbutler.runScript("project-with-different-base-stacks.sh"); + await gitbutler.runScript("apply-upstream-branch.sh", ["old-stack", "local-clone"]); + await gitbutler.runScript("apply-upstream-branch.sh", ["new-stack", "local-clone"]); + + await page.goto("/"); + await waitForTestId(page, "workspace-view"); + + const stacks = getByTestId(page, "stack"); + await expect(stacks).toHaveCount(2); + + // Write a change to shared_file. old-stack's base has "juliet", + // new-stack's has "JULIET-UPSTREAM". + const filePath = gitbutler.pathInWorkdir("local-clone/shared_file"); + writeFileSync(filePath, SHARED_FILE_CONTENT); + + const fileLocator = page + .getByTestId("uncommitted-changes-file-list") + .getByTestId("file-list-item") + .filter({ hasText: "shared_file" }); + await expect(fileLocator).toBeVisible(); + + return { stacks, fileLocator }; +} + +test("amend: should show incompatible-base rejection when amending wrong-base stack", async ({ + page, + context, +}, testInfo) => { + const workdir = testInfo.outputPath("workdir"); + const configdir = testInfo.outputPath("config"); + gitbutler = await startGitButler(workdir, configdir, context); + + const { fileLocator } = await setupDifferentBaseStacks(gitbutler, page); + + // Drag the file onto old-stack's commit to amend it. + // shared_file has a different base version there → IncompatibleBase. + const oldStackCommit = getByTestId(page, "commit-row").filter({ + hasText: "old-stack: add other_file", + }); + await expect(oldStackCommit).toBeVisible(); + + await dragAndDropByLocator(page, fileLocator, oldStackCommit); + + // The commit-failed modal should appear with "Incompatible branch base" + const modal = getByTestId(page, "global-modal-commit-failed"); + await expect(modal).toBeVisible(); + await expect(modal).toContainText("Incompatible branch base"); + await expect(modal).not.toContainText("Cherry-pick merge conflict"); + + await clickByTestId(page, "global-modal-action-button"); + await expect(modal).not.toBeVisible(); +}); + +test("commit: should show incompatible-base rejection when creating commit on wrong-base stack", async ({ + page, + context, +}, testInfo) => { + const workdir = testInfo.outputPath("workdir"); + const configdir = testInfo.outputPath("config"); + gitbutler = await startGitButler(workdir, configdir, context); + + const { stacks } = await setupDifferentBaseStacks(gitbutler, page); + + // Click the commit button on old-stack specifically. + const oldStack = stacks.filter({ hasText: "old-stack" }); + await expect(oldStack).toBeVisible(); + await oldStack.getByTestId("start-commit-button").click(); + + // Fill in a commit message and submit + await waitForTestId(page, "new-commit-view"); + await fillByTestId(page, "commit-drawer-title-input", "Test commit on wrong stack"); + await textEditorFillByTestId(page, "commit-drawer-description-input", "This should be rejected"); + await clickByTestId(page, "commit-drawer-action-button"); + + // The commit-failed modal should appear with "Incompatible branch base" + const modal = getByTestId(page, "global-modal-commit-failed"); + await expect(modal).toBeVisible(); + await expect(modal).toContainText("Incompatible branch base"); + await expect(modal).not.toContainText("Cherry-pick merge conflict"); + + await clickByTestId(page, "global-modal-action-button"); + await expect(modal).not.toBeVisible(); +}); diff --git a/packages/but-sdk/src/generated/index.d.ts b/packages/but-sdk/src/generated/index.d.ts index 6ca1630be53..987d444351d 100644 --- a/packages/but-sdk/src/generated/index.d.ts +++ b/packages/but-sdk/src/generated/index.d.ts @@ -1625,7 +1625,7 @@ export type RejectedChange = { }; /** Provide a description of why a [`crate::DiffSpec`] was rejected for application to the tree of a commit. */ -export type RejectionReason = "noEffectiveChanges" | "cherryPickMergeConflict" | "workspaceMergeConflict" | "workspaceMergeConflictOfUnrelatedFile" | "worktreeFileMissingForObjectConversion" | "fileToLargeOrBinary" | "pathNotFoundInBaseTree" | "unsupportedDirectoryEntry" | "unsupportedTreeEntry" | "missingDiffSpecAssociation"; +export type RejectionReason = "noEffectiveChanges" | "cherryPickMergeConflict" | "incompatibleBase" | "workspaceMergeConflict" | "workspaceMergeConflictOfUnrelatedFile" | "worktreeFileMissingForObjectConversion" | "fileToLargeOrBinary" | "pathNotFoundInBaseTree" | "unsupportedDirectoryEntry" | "unsupportedTreeEntry" | "missingDiffSpecAssociation"; /** * Specifies a location, usually used to either have something inserted