Merge pull request #13150 from Byron/multi-process-safety

Re-enable modern-but testing

Author: Byron

.github/workflows/push.yaml

@@ -220,6 +220,7 @@ jobs:
           cargo check -p but-workspace --all-targets
           cargo check -p but-api --all-targets
           cargo check -p but --all-targets
+          make check-modern-but
         name: Special `cargo check` runs
         env:
           RUSTFLAGS: "--deny warnings"
@@ -302,6 +303,7 @@ jobs:
       - run: cargo test --workspace --exclude gitbutler-tauri --exclude but-server --exclude but-api-macros-tests
       # It's intentional to use 'name equals run-script' so it's easy to re-run locally on failure.
       - run: cargo test -p but
+      - run: make test-modern-but
       - run: cargo test -p but-server
       - name: test vendored
         run: |

Makefile

@@ -28,9 +28,20 @@ check:
 clippy:
 	cargo clippy --workspace --all-targets -- -D warnings
 
+# An alias for nextest
 .PHONY: test
 test: nextest
 
+# CI executes this to build a version of `but` without any legacy code.
+.PHONY: check-modern-but
+check-modern-but:
+	cargo check -p but --all-targets --no-default-features
+
+# CI executes this to test a version of `but` without any legacy code.
+.PHONY: test-modern-but
+test-modern-but:
+	cargo test -p but --no-default-features
+
 # Run all tests in the entire workspace and show all failures in the end.
 .PHONY: nextest
 nextest:

crates/but/src/args/tests.rs

@@ -1,6 +1,8 @@
 use super::*;
 
+#[cfg(feature = "legacy")]
 mod commit;
+#[cfg(feature = "legacy")]
 mod reword;
 
 mod config_ai {

crates/but/src/command/commit/file.rs

@@ -1,11 +1,9 @@
+use crate::utils::OutputChannel;
 use anyhow::{Context as _, Result};
 use bstr::BStr;
 use bstr::ByteSlice;
 use but_core::{DiffSpec, diff::tree_changes, sync::RepoExclusive};
 use but_ctx::Context;
-use gitbutler_branch_actions::update_workspace_commit;
-
-use crate::utils::OutputChannel;
 
 pub fn commited_file_to_another_commit(
     ctx: &mut Context,
@@ -43,7 +41,7 @@ pub fn commited_file_to_another_commit_with_perm(
         perm,
     )?;
 
-    update_workspace_commit(ctx, false)?;
+    legacy_update_workspace_commit(ctx)?;
 
     if let Some(out) = out.for_human() {
         writeln!(out, "Moved files between commits!")?;
@@ -92,7 +90,7 @@ pub fn uncommit_file_with_perm(
         perm,
     )?;
 
-    update_workspace_commit(ctx, false)?;
+    legacy_update_workspace_commit(ctx)?;
 
     if let Some(out) = out.for_human() {
         writeln!(out, "Uncommitted changes")?;
@@ -140,10 +138,12 @@ pub fn uncommit_file_and_discard_with_perm(
         perm,
     )?;
 
-    let repo = ctx.repo.get()?;
-    let dropped = but_workspace::discard_workspace_changes(&repo, relevant_changes, context_lines)?;
+    let dropped = {
+        let repo = ctx.repo.get()?;
+        but_workspace::discard_workspace_changes(&repo, relevant_changes, context_lines)?
+    };
 
-    update_workspace_commit(ctx, false)?;
+    legacy_update_workspace_commit(ctx)?;
 
     if emit_output {
         if let Some(out) = out.for_human() {
@@ -197,3 +197,11 @@ fn find_stack_id_for_branch_with_perm(
         .and_then(|(stack, _)| stack.id);
     Ok(assign_to)
 }
+
+/// Refresh the workspace commit when legacy workspace state is available.
+/// TODO: remove this as it shouldn't be needed - all the functions it calls use the rebase engine which takes care of that.
+fn legacy_update_workspace_commit(_ctx: &mut Context) -> Result<()> {
+    #[cfg(feature = "legacy")]
+    gitbutler_branch_actions::update_workspace_commit(_ctx, false)?;
+    Ok(())
+}

crates/but/src/lib.rs

@@ -37,8 +37,9 @@ use but_settings::AppSettings;
 use colored::Colorize;
 use gix::date::time::CustomFormat;
 
+#[cfg(feature = "legacy")]
+use crate::command::legacy::ShowDiffInEditor;
 use crate::{
-    command::legacy::ShowDiffInEditor,
     setup::{BackgroundSync, InitCtxOptions},
     utils::{
         OneshotMetricsContext, OutputChannel, ResultErrorExt, ResultJsonExt, ResultMetricsExt,
@@ -1488,6 +1489,16 @@ async fn maybe_run_status_after(
     }
 }
 
+/// Ignore `--status-after` in non-legacy builds until a non-legacy status command exists.
+#[cfg(not(feature = "legacy"))]
+async fn maybe_run_status_after(
+    _status_after: bool,
+    _result: &anyhow::Result<()>,
+    _ctx: &mut but_ctx::Context,
+    _out: &mut OutputChannel,
+) {
+}
+
 /// Run workspace status output after a mutation command completes.
 ///
 /// In human mode, prints a blank line then full status.

crates/but/tests/but/command/branch/mod.rs

@@ -1,6 +1,5 @@
 mod apply;
 mod move_branch;
-#[cfg(feature = "legacy")]
 mod new;
 mod tear_off;
 mod unapply;

crates/but/tests/but/command/config.rs

@@ -37,6 +37,7 @@ fn ai_openai_defaults_to_global_config() -> anyhow::Result<()> {
 fn ai_ollama_local_writes_repo_config() -> anyhow::Result<()> {
     let env = Sandbox::empty()?;
     env.invoke_bash("git init repo");
+    #[cfg(feature = "legacy")]
     env.but("-C repo setup").assert().success();
 
     env.but("-C repo config ai --local ollama --endpoint localhost:11434 --model llama3.1")
@@ -113,6 +114,7 @@ fn ai_show_outputs_current_global_configuration_json() -> anyhow::Result<()> {
 fn ai_show_outputs_current_local_configuration_json() -> anyhow::Result<()> {
     let env = Sandbox::empty()?;
     env.invoke_bash("git init repo");
+    #[cfg(feature = "legacy")]
     env.but("-C repo setup").assert().success();
 
     env.but("-C repo config ai --local ollama --endpoint localhost:11434 --model llama3.1")

crates/but/tests/but/command/mod.rs

@@ -4,6 +4,7 @@
 #[cfg(feature = "legacy")]
 mod absorb;
 mod alias;
+#[cfg(feature = "legacy")]
 mod branch;
 #[cfg(feature = "legacy")]
 mod claude;