libgit2 safe checkout demo

Cargo.toml

@@ -222,6 +222,7 @@ gix-testtools = { version = "0.19.0", git = "https://github.com/GitoxideLabs/git
 
 insta = { version = "1.47.2", features = ["json"] }
 git2 = { version = "0.21.0", features = [
+    "unstable-sha256",
     "vendored-openssl",
     "vendored-libgit2",
 ] }
@@ -316,6 +317,7 @@ smallvec = "1.15.1"
 [patch.crates-io]
 # Keep workspace crates that use the `gix 0.84` line on the same git revision.
 gix = { git = "https://github.com/GitoxideLabs/gitoxide", rev = "66f70f3063724b4145b21c6691c4f57892c7a74e" }
+git2 = { git = "https://github.com/jonathantanmy2/git2-rs", rev = "9e2ec81c8d37ba5142e5fb97c5afbc2f43c6d7eb" }
 
 [workspace.lints.clippy]
 # Note that `all` doesn't include everything, so extra lints should be added here.

crates/but-api/src/commit/create.rs

@@ -65,6 +65,7 @@ pub(crate) fn commit_create_only_impl(
         rebase,
         commit_selector,
         rejected_specs,
+        ..
     } = but_workspace::commit::commit_create(
         editor,
         changes,

crates/but-api/src/legacy/workspace.rs

@@ -302,6 +302,7 @@ pub fn stash_into_branch(
             rebase,
             commit_selector,
             rejected_specs,
+            ..
         } = but_workspace::commit::commit_create(
             editor,
             worktree_changes,

crates/but-core/src/worktree/checkout/function.rs

@@ -13,6 +13,35 @@ use crate::update_head_reference;
 
 use super::{Options, Outcome, utils::merge_worktree_changes_into_destination_or_keep_snapshot};
 
+/// Update the index and working directory (but not any refs). If
+/// `baseline_treeish` is None, `HEAD^{tree}` is used instead.
+pub fn checkout_tree(
+    repo: &gix::Repository,
+    treeish: gix::ObjectId,
+    baseline_treeish: Option<gix::ObjectId>,
+) -> anyhow::Result<()> {
+    let git2_repo = git2::Repository::open(repo.git_dir())?;
+
+    // eprintln!("HEAD is {:?}, treeish is {:?}", repo.head_id(), treeish);
+    if let Some(baseline_treeish) = baseline_treeish {
+        let baseline = git2_repo
+            .find_object(baseline_treeish.to_git2(), None)?
+            .peel_to_tree()?;
+        git2_repo.index()?.read_tree(&baseline)?;
+        // eprintln!("baseline tree is {:?}", baseline.id());
+        let mut opts = git2::build::CheckoutBuilder::new();
+        opts.baseline(&baseline);
+        git2_repo.checkout_tree(
+            &git2_repo.find_object(treeish.to_git2(), None)?,
+            Some(&mut opts),
+        )?;
+    } else {
+        // eprintln!("baseline tree not given");
+        git2_repo.checkout_tree(&git2_repo.find_object(treeish.to_git2(), None)?, None)?;
+    }
+    Ok(())
+}
+
 /// Like [`safe_checkout()`], but the current tree will always be fetched from
 pub fn safe_checkout_from_head(
     new_head_id: gix::ObjectId,

crates/but-core/src/worktree/mod.rs

@@ -4,7 +4,7 @@ pub mod checkout;
 use std::{io::Read, path::Path};
 
 use bstr::BStr;
-pub use checkout::function::{safe_checkout, safe_checkout_from_head};
+pub use checkout::function::{checkout_tree, safe_checkout, safe_checkout_from_head};
 use gix::filter::plumbing::pipeline::convert::ToGitOutcome;
 
 /// Read a worktree file into `buf` after converting it to what Git *would* store.

crates/but-rebase/src/graph_rebase/materialize.rs

@@ -1,12 +1,6 @@
 //! Functions for materializing a rebase
 use anyhow::{Context, Result, bail};
-use but_core::{
-    ObjectStorageExt as _, RefMetadata,
-    worktree::{
-        checkout::{Options, UncommitedWorktreeChanges},
-        safe_checkout_from_head,
-    },
-};
+use but_core::{ObjectStorageExt as _, RefMetadata, worktree::checkout_tree};
 use gix::refs::{
     Target,
     transaction::{Change, LogChange, PreviousValue, RefEdit, RefLog},
@@ -51,16 +45,17 @@ impl<'ws, 'graph, M: RefMetadata> SuccessfulRebase<'ws, 'graph, M> {
 
                     // If the head has changed (which means it's in the
                     // commit mapping), perform a safe checkout.
-                    safe_checkout_from_head(
-                        new_head,
-                        &repo,
-                        Options {
-                            uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict,
-                            skip_head_update: true,
-                            merge_base_override,
-                            allow_conflicted_commit_checkout: true,
-                        },
-                    )?;
+                    // safe_checkout_from_head(
+                    //     new_head,
+                    //     &repo,
+                    //     Options {
+                    //         uncommitted_changes: UncommitedWorktreeChanges::KeepAndAbortOnConflict,
+                    //         skip_head_update: true,
+                    //         merge_base_override,
+                    //         allow_conflicted_commit_checkout: true,
+                    //     },
+                    // )?;
+                    checkout_tree(&repo, new_head, merge_base_override)?;
                 }
             }
         }

crates/but-transaction/src/lib.rs

@@ -480,6 +480,7 @@ where
         side: InsertSide,
         changes: Vec<DiffSpec>,
         message: String,
+        existing_consumed: Vec<DiffSpec>,
     ) -> anyhow::Result<IntermediateCommitCreateResult> {
         let context_lines = self.inner.context_lines;
         self.rebase(|editor, commit_mappings, _| {
@@ -492,13 +493,15 @@ where
                 rebase,
                 commit_selector,
                 rejected_specs,
-            } = but_workspace::commit::commit_create(
+                consumed,
+            } = but_workspace::commit::commit_create_ex(
                 editor,
                 changes,
                 relative_to,
                 side,
                 &message,
                 context_lines,
+                existing_consumed,
             )?;
 
             let new_commit = commit_selector
@@ -509,6 +512,7 @@ where
                 IntermediateCommitCreateResult {
                     new_commit,
                     rejected_specs,
+                    consumed,
                 },
                 rebase,
             ))
@@ -560,6 +564,7 @@ where
                 IntermediateCommitCreateResult {
                     new_commit,
                     rejected_specs,
+                    consumed: Vec::new(),
                 },
                 rebase,
             ))
@@ -1017,4 +1022,6 @@ pub struct IntermediateCommitCreateResult {
     pub new_commit: Option<gix::ObjectId>,
     /// Any specs that failed to be committed.
     pub rejected_specs: Vec<(RejectionReason, DiffSpec)>,
+    ///
+    pub consumed: Vec<DiffSpec>,
 }

crates/but-workspace/src/commit/commit_create.rs

@@ -25,6 +25,8 @@ pub struct CommitCreateOutcome<'ws, 'meta, M: RefMetadata> {
     /// Rejected diff specs from commit creation. See [`create_commit`] for
     /// more details.
     pub rejected_specs: Vec<(but_core::tree::create_tree::RejectionReason, DiffSpec)>,
+    ///
+    pub consumed: Vec<DiffSpec>,
 }
 
 /// Create a commit from `changes` and insert it relative to `relative_to` on `side`.
@@ -52,6 +54,27 @@ pub fn commit_create<'ws, 'meta, M: RefMetadata>(
     side: InsertSide,
     message: &str,
     context_lines: u32,
+) -> Result<CommitCreateOutcome<'ws, 'meta, M>> {
+    commit_create_ex(
+        editor,
+        changes,
+        relative_to,
+        side,
+        message,
+        context_lines,
+        Vec::new(),
+    )
+}
+
+///
+pub fn commit_create_ex<'ws, 'meta, M: RefMetadata>(
+    mut editor: Editor<'ws, 'meta, M>,
+    changes: Vec<DiffSpec>,
+    relative_to: impl ToSelector,
+    side: InsertSide,
+    message: &str,
+    context_lines: u32,
+    existing_consumed: Vec<DiffSpec>,
 ) -> Result<CommitCreateOutcome<'ws, 'meta, M>> {
     let relative_to_selector = relative_to.to_selector(&editor)?;
     let parent_commit_id =
@@ -76,6 +99,7 @@ pub fn commit_create<'ws, 'meta, M: RefMetadata>(
             rebase: editor.rebase()?,
             commit_selector: None,
             rejected_specs: create_out.rejected_specs,
+            consumed: Vec::new(),
         });
     };
 
@@ -86,12 +110,15 @@ pub fn commit_create<'ws, 'meta, M: RefMetadata>(
         .iter()
         .map(|(_, spec)| &spec.path)
         .collect();
-    let consumed: Vec<_> = all_changes
-        .into_iter()
-        .filter(|spec| !rejected_paths.contains(&spec.path))
-        .collect();
+    let mut consumed = existing_consumed;
+    consumed.extend(
+        all_changes
+            .into_iter()
+            .filter(|spec| !rejected_paths.contains(&spec.path)),
+    );
     if !consumed.is_empty() {
-        let merge_base = compute_merge_base_override(editor.repo(), consumed, context_lines)?;
+        let merge_base =
+            compute_merge_base_override(editor.repo(), consumed.clone(), context_lines)?;
         editor.set_merge_base_override(merge_base);
     }
 
@@ -105,6 +132,7 @@ pub fn commit_create<'ws, 'meta, M: RefMetadata>(
         rebase: editor.rebase()?,
         commit_selector: Some(commit_selector),
         rejected_specs: create_out.rejected_specs,
+        consumed,
     })
 }
 

crates/but-workspace/src/commit/mod.rs

@@ -22,21 +22,22 @@ fn compute_merge_base_override(
 ) -> anyhow::Result<gix::ObjectId> {
     let head_tree = repo.head_tree_id_or_empty()?;
     let workdir = repo.workdir().context("non-bare repository")?;
-    let mut specs: Vec<_> = consumed
-        .into_iter()
-        .filter(|spec| {
-            if spec.hunk_headers.is_empty() {
-                return workdir
-                    .join(gix::path::from_bstr(spec.path.as_bstr()))
-                    .exists();
-            }
-            true
-        })
-        .map(|mut spec| {
-            spec.previous_path = None;
-            Ok(spec)
-        })
-        .collect();
+    let mut specs: Vec<_> = consumed.into_iter().map(|mut spec| Ok(spec)).collect();
+    // let mut specs: Vec<_> = consumed
+    //     .into_iter()
+    //     .filter(|spec| {
+    //         if spec.hunk_headers.is_empty() {
+    //             return workdir
+    //                 .join(gix::path::from_bstr(spec.path.as_bstr()))
+    //                 .exists();
+    //         }
+    //         true
+    //     })
+    //     .map(|mut spec| {
+    //         spec.previous_path = None;
+    //         Ok(spec)
+    //     })
+    //     .collect();
     if specs.is_empty() {
         return Ok(head_tree.detach());
     }
@@ -48,7 +49,7 @@ fn compute_merge_base_override(
 pub mod reword;
 pub use reword::reword;
 pub mod commit_create;
-pub use commit_create::{CommitCreateOutcome, commit_create};
+pub use commit_create::{CommitCreateOutcome, commit_create, commit_create_ex};
 pub mod commit_amend;
 pub use commit_amend::{CommitAmendOutcome, commit_amend};
 pub mod insert_blank_commit;

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

@@ -685,13 +685,16 @@ pub(crate) fn commit_batch(
         DryRun::No,
         |mut tx| {
             let mut new_commits = Vec::with_capacity(planned_commit_count);
+            let mut consumed = Vec::new();
             for planned_commit in planned_commits {
                 let outcome = tx.create_commit(
                     position.0.clone(),
                     position.1,
                     planned_commit.diff_specs,
                     planned_commit.message,
+                    consumed.clone(),
                 )?;
+                consumed.extend(outcome.consumed);
 
                 if !outcome.rejected_specs.is_empty() {
                     return Err(anyhow::anyhow!(

crates/but/src/command/legacy/status/tui/mod.rs

@@ -2349,6 +2349,7 @@ impl App {
                     insert_side,
                     changes_to_commit,
                     String::new(),
+                    Vec::new(),
                 )?;
 
                 if commit_create_result.rejected_specs.is_empty() {

crates/but/src/command/legacy/status/tui/tests/commit_tests.rs

@@ -553,13 +553,15 @@ fn commit_moved_and_modified_file() {
     tui.env().append_file("moved-test.txt", "new content\n");
     tui.reload();
 
+    eprintln!("about to commit");
     // commit the moved file
     tui.input_then_render('c');
     tui.input_then_render(KeyCode::Down);
     tui.input_then_render('i');
     tui.input_then_render(KeyCode::Enter);
     tui.input_then_render("move test.txt to moved-test.txt");
     tui.input_then_render(KeyCode::Enter);
+    // tui.debug();
 
     // there should be no more changes to commit
     tui.reload()

crates/but/src/command/legacy/status/tui/tests/utils.rs

@@ -162,6 +162,10 @@ impl TestTui {
         self.env.as_ref().unwrap()
     }
 
+    pub(super) fn debug(mut self) -> ! {
+        std::mem::take(&mut self.env).unwrap().debug()
+    }
+
     #[track_caller]
     pub(super) fn reload(&mut self) -> TestTuiInputThenRenderResult<'_> {
         self.render_with_messages(

crates/but/tests/but/command/commit.rs

@@ -1561,6 +1561,25 @@ Error: Multiple branches found. Specify a branch to commit to using the branch a
 "#]]);
 }
 
+#[test]
+fn commit_does_not_needlessly_touch_file() -> anyhow::Result<()> {
+    let env = Sandbox::init_scenario_with_target_and_default_settings("one-stack");
+    env.setup_metadata(&["A"]);
+
+    env.file("A", "new content");
+
+    let old_time = std::fs::metadata(env.projects_root().join("A"))?.modified()?;
+    env.but("commit -m test").assert().success();
+    let new_time = std::fs::metadata(env.projects_root().join("A"))?.modified()?;
+
+    assert_eq!(
+        new_time, old_time,
+        "time should be the same, because file should not have been modified"
+    );
+
+    Ok(())
+}
+
 /// Helper to build an isolated `std::process::Command` for `but` with the same
 /// environment as the Sandbox test harness.
 /// That way it can be spawned, which isn't possible in the [`Sandbox`] version.