amended commits are re-signed

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

@@ -238,7 +238,7 @@ pub fn create_commit(
             .collect(),
     };
 
-    if parents.len() > 1 {
+    if !matches!(destination, Destination::AmendCommit(_)) && parents.len() > 1 {
         bail!("cannot currently handle more than 1 parent")
     }
 
@@ -266,7 +266,8 @@ pub fn create_commit(
                     .decode()?
                     .to_owned();
                 commit.tree = new_tree;
-                Some(repo.write_object(commit)?.detach())
+                let (new_commit, _ref_edit) = plumbing::create_given_commit(repo, None, commit)?;
+                Some(new_commit)
             }
         }
     } else {

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/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"
+

crates/but-workspace/tests/workspace/commit_engine/amend_commit.rs

@@ -1,15 +1,13 @@
 use crate::commit_engine::utils::{
-    commit_whole_files_and_all_hunks_from_workspace, read_only_in_memory_scenario, stable_env,
-    visualize_tree,
+    assure_stable_env, commit_from_outcome, commit_whole_files_and_all_hunks_from_workspace,
+    read_only_in_memory_scenario, visualize_tree, writable_scenario, writable_scenario_execute,
+    write_sequence,
 };
 use but_workspace::commit_engine::Destination;
-use gix::prelude::ObjectIdExt;
-use serial_test::serial;
 
 #[test]
-#[serial]
 fn all_changes_and_renames_to_topmost_commit_no_parent() -> anyhow::Result<()> {
-    let _env = stable_env();
+    assure_stable_env();
 
     let repo = read_only_in_memory_scenario("all-file-types-renamed-and-modified")?;
     let head_commit = repo.rev_parse_single("HEAD")?;
@@ -69,29 +67,111 @@ fn all_changes_and_renames_to_topmost_commit_no_parent() -> anyhow::Result<()> {
 }
 
 #[test]
-#[serial]
-#[ignore = "TBD"]
-fn all_aspects_of_amended_commit_are_copied_including_extra_headers() -> anyhow::Result<()> {
+fn all_aspects_of_amended_commit_are_copied() -> anyhow::Result<()> {
+    assure_stable_env();
+
+    let (repo, _tmp) = writable_scenario("merge-with-two-branches-line-offset");
+    // Rewrite the entire file, which is fine as we rewrite/amend the base-commit itself.
+    write_sequence(&repo, "file", [(40, 70)])?;
+    let outcome = commit_whole_files_and_all_hunks_from_workspace(
+        &repo,
+        Destination::AmendCommit(repo.rev_parse_single("merge")?.detach()),
+    )?;
+    let tree = visualize_tree(&repo, &outcome)?;
+    insta::assert_snapshot!(tree, @r#"
+    5bbee6d
+    └── file:100644:1c9325b "40\n41\n42\n43\n44\n45\n46\n47\n48\n49\n50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n60\n61\n62\n63\n64\n65\n66\n67\n68\n69\n70\n"
+    "#);
+    insta::assert_debug_snapshot!(commit_from_outcome(&repo, &outcome)?, @r#"
+    Commit {
+        tree: Sha1(5bbee6d0219923e795f7b0818dda2f33f16278b4),
+        parents: [
+            Sha1(91ef6f6fc0a8b97fb456886c1cc3b2a3536ea2eb),
+            Sha1(7f389eda1b366f3d56ecc1300b3835727c3309b6),
+        ],
+        author: Signature {
+            name: "author",
+            email: "[email protected]",
+            time: Time {
+                seconds: 946684800,
+                offset: 0,
+                sign: Plus,
+            },
+        },
+        committer: Signature {
+            name: "committer",
+            email: "[email protected]",
+            time: Time {
+                seconds: 946771200,
+                offset: 0,
+                sign: Plus,
+            },
+        },
+        encoding: None,
+        message: "Merge branch \'A\' into merge\n",
+        extra_headers: [],
+    }
+    "#);
     Ok(())
 }
 
 #[test]
-#[serial]
-#[ignore = "TBD"]
 fn signatures_are_redone() -> anyhow::Result<()> {
+    assure_stable_env();
+
+    let (mut repo, _tmp) = writable_scenario_execute("two-signed-commits-with-line-offset");
+
+    let head_id = repo.head_id()?;
+    let head_commit = head_id.object()?.into_commit().decode()?.to_owned();
+    let head_id = head_id.detach();
+    let previous_signature = head_commit
+        .extra_headers()
+        .pgp_signature()
+        .expect("it's signed by default");
+
+    // Rewrite everything for amending on top.
+    write_sequence(&repo, "file", [(40, 60)])?;
+    let outcome =
+        commit_whole_files_and_all_hunks_from_workspace(&repo, Destination::AmendCommit(head_id))?;
+
+    let new_commit = commit_from_outcome(&repo, &outcome)?;
+    let new_signature = new_commit
+        .extra_headers()
+        .pgp_signature()
+        .expect("signing config is respected");
+    assert_ne!(
+        previous_signature, new_signature,
+        "signatures are recreated as the commit is changed"
+    );
+    assert_eq!(
+        new_commit
+            .extra_headers()
+            .find_all(gix::objs::commit::SIGNATURE_FIELD_NAME)
+            .count(),
+        1,
+        "it doesn't leave outdated signatures on top of the updated one"
+    );
+
+    repo.config_snapshot_mut()
+        .set_raw_value(&"gitbutler.signCommits", "false")?;
+    write_local_config(&repo)?;
+    let outcome =
+        commit_whole_files_and_all_hunks_from_workspace(&repo, Destination::AmendCommit(head_id))?;
+    let new_commit = commit_from_outcome(&repo, &outcome)?;
+    assert!(
+        new_commit.extra_headers().pgp_signature().is_none(),
+        "If signing commits is disabled, \
+    it will drop the signature (instead of leaving an invalid one)"
+    );
     Ok(())
 }
 
-fn commit_from_outcome(
-    repo: &gix::Repository,
-    outcome: &but_workspace::commit_engine::CreateCommitOutcome,
-) -> anyhow::Result<gix::objs::Commit> {
-    Ok(outcome
-        .new_commit
-        .expect("the amended commit was created")
-        .attach(repo)
-        .object()?
-        .peel_to_commit()?
-        .decode()?
-        .into())
+// In-memory config changes aren't enough as we still only have snapshots, without the ability to keep
+// the entire configuration fresh.
+fn write_local_config(repo: &gix::Repository) -> anyhow::Result<()> {
+    repo.config_snapshot().write_to_filter(
+        &mut std::fs::File::create(repo.path().join("config"))?,
+        |section| section.meta().source == gix::config::Source::Local,
+    )?;
+    Ok(())
 }