Merge pull request #7271 from gitbutlerapp/advanced-committing

advanced committing

Cargo.toml

@@ -5,7 +5,7 @@ resolver = "2"
 [workspace.dependencies]
 bstr = "1.11.1"
 # Add the `tracing` or `tracing-detail` features to see more of gitoxide in the logs. Useful to see which programs it invokes.
-gix = { git = "https://github.com/GitoxideLabs/gitoxide", rev = "5c327bbfeb7c685a93962e087f72d1083a768b2d", default-features = false, features = [
+gix = { git = "https://github.com/GitoxideLabs/gitoxide", branch = "improvements", default-features = false, features = [
 ] }
 gix-testtools = "0.15.0"
 insta = "1.41.1"

crates/but-core/tests/core/diff/worktree_changes.rs

@@ -234,6 +234,17 @@ fn added_in_unborn() -> Result<()> {
     Ok(())
 }
 
+#[test]
+fn sparse() -> Result<()> {
+    let repo = repo_in("sparse", "non-cone")?;
+    let err = diff::worktree_changes(&repo).unwrap_err();
+    assert!(
+        err.to_string().contains("sparse"),
+        "Currently status doesn't run on sparse indices, but it could if it would unsparse it"
+    );
+    Ok(())
+}
+
 #[test]
 fn submodule_added_in_unborn() -> Result<()> {
     let repo = repo("submodule-added-unborn")?;
@@ -1222,16 +1233,20 @@ fn unified_diffs(
         .collect()
 }
 
-pub fn repo(fixture_name: &str) -> anyhow::Result<gix::Repository> {
-    let root = gix_testtools::scripted_fixture_read_only("worktree-changes.sh")
+pub fn repo_in(fixture_name: &str, name: &str) -> anyhow::Result<gix::Repository> {
+    let root = gix_testtools::scripted_fixture_read_only(format!("{}.sh", fixture_name))
         .map_err(anyhow::Error::from_boxed)?;
-    let worktree_root = root.join(fixture_name);
-    Ok(gix::open(worktree_root)?)
+    let worktree_root = root.join(name);
+    Ok(gix::open_opts(
+        worktree_root,
+        gix::open::Options::isolated(),
+    )?)
+}
+
+pub fn repo(fixture_name: &str) -> anyhow::Result<gix::Repository> {
+    repo_in("worktree-changes", fixture_name)
 }
 
 pub fn repo_unix(fixture_name: &str) -> anyhow::Result<gix::Repository> {
-    let root = gix_testtools::scripted_fixture_read_only("worktree-changes-unix.sh")
-        .map_err(anyhow::Error::from_boxed)?;
-    let worktree_root = root.join(fixture_name);
-    Ok(gix::open(worktree_root)?)
+    repo_in("worktree-changes-unix", fixture_name)
 }

crates/but-core/tests/fixtures/sparse.sh

@@ -0,0 +1,19 @@
+#!/bin/bash
+
+set -eu -o pipefail
+
+git init -q non-cone
+(cd non-cone
+  touch a b
+  mkdir c1
+  (cd c1 && touch a b && mkdir c2 && cd c2 && touch a b)
+  (cd c1 && mkdir c3 && cd c3 && touch a b)
+  mkdir d
+  (cd d && touch a b && mkdir c4 && cd c4 && touch a b c5)
+
+  git add .
+  git commit -m "init"
+
+  git sparse-checkout set c1/c2 --sparse-index
+)
+

crates/but-workspace/Cargo.toml

@@ -40,5 +40,4 @@ md5 = "0.7.0"
 gix-testtools = "0.15.0"
 gitbutler-testsupport.workspace = true
 insta = "1.42.1"
-but-core = { workspace = true, features = ["testing"] }
-serial_test = "3.2.0"
+but-core = { workspace = true, features = ["testing"] }

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

@@ -20,16 +20,20 @@ mod plumbing;
 /// The place to apply the [change-specifications](DiffSpec) to.
 ///
 /// Note that any commit this instance points to will be the basis to apply all changes to.
-#[derive(Debug, Clone, Copy)]
+#[derive(Debug, Clone)]
 pub enum Destination {
-    /// Create a new commit on top of the given `Some(commit)`, so it will be the sole parent
+    /// Create a new commit on top of the given `parent_commit_id`, so it will be the sole parent
     /// of the newly created commit, making it its ancestor.
-    /// To create a commit at the position of the first commit of a branch, the parent has to be the merge-base with the *target branch*.
-    ///
-    /// If the commit is `None`, the base-state for the new commit will be an empty tree and the new commit will be the first one
-    /// (i.e. have no parent). This is the case when `HEAD` is unborn. If `HEAD` is detached, this is a failure.
-    ParentForNewCommit(Option<gix::ObjectId>),
-    /// Amend the given commit.
+    NewCommit {
+        /// If `None`, the base-state for the new commit will be an empty tree and the new commit will be the first one
+        /// (i.e. have no parent). This is the case when `HEAD` is unborn. If `HEAD` is detached, this is a failure.
+        ///
+        /// To create a commit at the position of the first commit of a branch, the parent has to be the merge-base with the *target branch*.
+        parent_commit_id: Option<gix::ObjectId>,
+        /// Use `message` as commit message for the new commit.
+        message: String,
+    },
+    /// Amend all changes to the given commit, leaving all other aspects of the commit unchanged.
     AmendCommit(gix::ObjectId),
 }
 
@@ -91,8 +95,6 @@ pub struct CreateCommitOutcome {
     pub rejected_specs: Vec<DiffSpec>,
     /// The newly created commit, or `None` if no commit could be created as all changes-requests were rejected.
     pub new_commit: Option<gix::ObjectId>,
-    /// Only set when `HEAD` was updated as it was unborn, and we created the first commit.
-    pub ref_edit: Option<gix::refs::transaction::RefEdit>,
 }
 
 /// Additional information about the outcome of a [`create_tree()`] call.
@@ -109,7 +111,7 @@ pub struct CreateTreeOutcome {
 /// Like [`create_commit()`], but lower-level and only returns a new tree, without finally associating it with a commit.
 pub fn create_tree(
     repo: &gix::Repository,
-    destination: Destination,
+    destination: &Destination,
     origin_commit: Option<gix::ObjectId>,
     changes: Vec<DiffSpec>,
     context_lines: u32,
@@ -119,8 +121,14 @@ pub fn create_tree(
     }
 
     let target_tree = match destination {
-        Destination::ParentForNewCommit(None) => gix::ObjectId::empty_tree(repo.object_hash()),
-        Destination::ParentForNewCommit(Some(base_commit))
+        Destination::NewCommit {
+            parent_commit_id: None,
+            ..
+        } => gix::ObjectId::empty_tree(repo.object_hash()),
+        Destination::NewCommit {
+            parent_commit_id: Some(base_commit),
+            ..
+        }
         | Destination::AmendCommit(base_commit) => {
             but_core::Commit::from_id(base_commit.attach(repo))?
                 .tree_id()?
@@ -194,19 +202,9 @@ pub fn create_tree(
     })
 }
 
-/// Control how [`create_commit()`] alters references after creating the new commit.
-#[derive(Debug, Copy, Clone)]
-pub enum RefHandling {
-    /// If the commit is created on top of a commit that the `HEAD` ref is currently pointing to, then update it to point to the new commit.
-    UpdateHEADRefForTipCommits,
-    /// Do not touch any ref, only create a commit.
-    None,
-}
-
 /// Alter the single `destination` in a given `frame` with as many `changes` as possible and write new objects into `repo`,
 /// but only if the commit succeeds.
 /// If `origin_commit` is `Some(commit)`, all changes are considered to originate from the given commit, otherwise they originate from the worktree.
-/// Use `message` as commit message.
 /// `context_lines` is the amount of lines of context included in each [`HunkHeader`], and the value that will be used to recover the existing hunks,
 /// so that the hunks can be matched.
 ///
@@ -220,63 +218,64 @@ pub fn create_commit(
     destination: Destination,
     origin_commit: Option<gix::ObjectId>,
     changes: Vec<DiffSpec>,
-    message: &str,
     context_lines: u32,
-    ref_handling: RefHandling,
 ) -> anyhow::Result<CreateCommitOutcome> {
-    let parents = match destination {
-        Destination::ParentForNewCommit(None) => Vec::new(),
-        Destination::ParentForNewCommit(Some(parent)) => vec![parent],
-        Destination::AmendCommit(_) => {
-            todo!("get parents of the given commit ")
-        }
+    let parents = match &destination {
+        Destination::NewCommit {
+            parent_commit_id: None,
+            ..
+        } => Vec::new(),
+        Destination::NewCommit {
+            parent_commit_id: Some(parent),
+            ..
+        } => vec![*parent],
+        Destination::AmendCommit(commit_id) => commit_id
+            .attach(repo)
+            .object()?
+            .peel_to_commit()?
+            .parent_ids()
+            .map(|id| id.detach())
+            .collect(),
     };
 
-    if parents.len() > 1 {
+    if !matches!(destination, Destination::AmendCommit(_)) && parents.len() > 1 {
         bail!("cannot currently handle more than 1 parent")
     }
 
-    let ref_name_to_update = repo
-        .head_name()?
-        .context("Refusing to commit into a detached HEAD")?;
-    let ref_name_to_update = match ref_handling {
-        RefHandling::UpdateHEADRefForTipCommits => {
-            if let &[parent] = &parents[..] {
-                new_commit_is_on_top_of_tip(repo, ref_name_to_update.as_ref(), parent)?
-                    .then_some(ref_name_to_update)
-            } else if repo.head()?.is_unborn() {
-                Some(ref_name_to_update)
-            } else {
-                None
-            }
-        }
-        RefHandling::None => None,
-    };
-
     let CreateTreeOutcome {
         rejected_specs,
         new_tree,
-    } = create_tree(repo, destination, origin_commit, changes, context_lines)?;
-    let (new_commit, ref_edit) = if let Some(new_tree) = new_tree {
-        let (author, committer) = repo.commit_signatures()?;
-        let (new_commit, ref_edit) = plumbing::create_commit(
-            repo,
-            ref_name_to_update,
-            author,
-            committer,
-            message,
-            new_tree,
-            parents,
-            None,
-        )?;
-        (Some(new_commit), ref_edit)
+    } = create_tree(repo, &destination, origin_commit, changes, context_lines)?;
+    let new_commit = if let Some(new_tree) = new_tree {
+        match destination {
+            Destination::NewCommit {
+                message,
+                parent_commit_id: _,
+            } => {
+                let (author, committer) = repo.commit_signatures()?;
+                let (new_commit, _ref_edit) = plumbing::create_commit(
+                    repo, None, author, committer, &message, new_tree, parents, None,
+                )?;
+                Some(new_commit)
+            }
+            Destination::AmendCommit(commit_id) => {
+                let mut commit = commit_id
+                    .attach(repo)
+                    .object()?
+                    .peel_to_commit()?
+                    .decode()?
+                    .to_owned();
+                commit.tree = new_tree;
+                let (new_commit, _ref_edit) = plumbing::create_given_commit(repo, None, commit)?;
+                Some(new_commit)
+            }
+        }
     } else {
-        (None, None)
+        None
     };
     Ok(CreateCommitOutcome {
         rejected_specs,
         new_commit,
-        ref_edit,
     })
 }
 
@@ -288,17 +287,6 @@ fn into_err_spec(input: &mut PossibleChange) {
     };
 }
 
-fn new_commit_is_on_top_of_tip(
-    repo: &gix::Repository,
-    name: &gix::refs::FullNameRef,
-    parent: gix::ObjectId,
-) -> anyhow::Result<bool> {
-    let Some(head_ref) = repo.try_find_reference(name)? else {
-        return Ok(true);
-    };
-    Ok(head_ref.id() == *parent)
-}
-
 type PossibleChange = Result<DiffSpec, DiffSpec>;
 
 /// Apply `changes` to `changes_base_tree` and return the newly written tree.

crates/but-workspace/src/commit_engine/plumbing.rs

@@ -23,7 +23,7 @@ pub fn create_commit(
     parents: impl IntoIterator<Item = impl Into<gix::ObjectId>>,
     commit_headers: Option<but_core::commit::HeadersV2>,
 ) -> anyhow::Result<(gix::ObjectId, Option<gix::refs::transaction::RefEdit>)> {
-    let mut commit = gix::objs::Commit {
+    let commit = gix::objs::Commit {
         message: message.into(),
         tree,
         author,
@@ -32,23 +32,43 @@ pub fn create_commit(
         parents: parents.into_iter().map(Into::into).collect(),
         extra_headers: commit_headers.unwrap_or_default().into(),
     };
+    create_given_commit(repo, update_ref, commit)
+}
 
+/// Use the given commit and possibly sign it, replacing a possibly existing signature,
+/// or removing the signature if GitButler is not configured to keep it.
+///
+/// Signatures will be removed automatically if signing is disabled to prevent an amended commit
+/// to use the old signature. They will also be added or replace existing signatures.
+#[allow(clippy::too_many_arguments)]
+pub fn create_given_commit(
+    repo: &gix::Repository,
+    update_ref: Option<gix::refs::FullName>,
+    mut commit: gix::objs::Commit,
+) -> anyhow::Result<(gix::ObjectId, Option<gix::refs::transaction::RefEdit>)> {
+    if let Some(pos) = commit
+        .extra_headers()
+        .find_pos(gix::objs::commit::SIGNATURE_FIELD_NAME)
+    {
+        commit.extra_headers.remove(pos);
+    }
     if repo.git_settings()?.gitbutler_sign_commits.unwrap_or(false) {
         let mut buf = Vec::new();
         commit.write_to(&mut buf)?;
-        let signature = sign_buffer(repo, &buf);
-        match signature {
+        match sign_buffer(repo, &buf) {
             Ok(signature) => {
-                commit.extra_headers.push(("gpgsig".into(), signature));
+                commit
+                    .extra_headers
+                    .push((gix::objs::commit::SIGNATURE_FIELD_NAME.into(), signature));
             }
-            Err(e) => {
+            Err(err) => {
                 // If signing fails, turn off signing automatically and let everyone know,
                 repo.set_git_settings(&GitConfigSettings {
                     gitbutler_sign_commits: Some(false),
                     ..GitConfigSettings::default()
                 })?;
                 return Err(
-                    anyhow!("Failed to sign commit: {}", e).context(Code::CommitSigningFailed)
+                    anyhow!("Failed to sign commit: {}", err).context(Code::CommitSigningFailed)
                 );
             }
         }
@@ -58,8 +78,8 @@ pub fn create_commit(
     let refedit = if let Some(refname) = update_ref {
         // TODO:(ST) should this be something more like what Git does (also in terms of reflog message)?
         //       Probably should support making a commit in full with `gix`.
-        let log_message = message;
-        let edit = update_reference(repo, refname, new_commit_id, log_message.into())?;
+        let log_message = commit.message;
+        let edit = update_reference(repo, refname, new_commit_id, log_message)?;
         Some(edit)
     } else {
         None

crates/but-workspace/src/commit_engine/ui.rs

@@ -50,7 +50,6 @@ impl From<super::CreateCommitOutcome> for CreateCommitOutcome {
         super::CreateCommitOutcome {
             rejected_specs,
             new_commit,
-            ref_edit: _,
         }: super::CreateCommitOutcome,
     ) -> Self {
         CreateCommitOutcome {

crates/but-workspace/tests/fixtures/scenario/two-signed-commits-with-line-offset.sh

@@ -0,0 +1,26 @@
+#!/usr/bin/env bash
+
+### Description
+# A single branch with two signed commits. The first commit has 10 lines, the second adds
+# another 10 lines to the top of the file.
+# Large numbers are used make fuzzy-patch application harder.
+set -eu -o pipefail
+
+ssh-keygen -t rsa -b 2048 -C "[email protected]" -N "" -f signature.key
+
+git init
+git config gpg.format ssh
+git config user.signingKey "$PWD/signature.key"
+git config GitButler.signCommits true
+
+export "GIT_CONFIG_COUNT=2"
+export "GIT_CONFIG_KEY_0=commit.gpgsign"
+export "GIT_CONFIG_VALUE_0=true"
+export "GIT_CONFIG_KEY_1=init.defaultBranch"
+export "GIT_CONFIG_VALUE_1=main"
+
+seq 10 20 >file
+git add file && git commit -m init && git tag first-commit
+
+seq 20  >file && git commit -am "insert 10 lines to the top"
+