From bcf94bd70ea78924230de4e848383d308dd40e6f Mon Sep 17 00:00:00 2001
From: Martin von Zweigbergk <martinvonz@google.com>
Date: Sat, 4 Jun 2022 16:18:03 -0700
Subject: [PATCH] CommitBuilder: when rewriting commit, replace placeholder
 user/email

If a commit's author field has the placeholder user/email values
(i.e. "(no name configured)" and "(no email configured)"), and they
have now configured their email and username, they probably want us to
update the author field with the new information, so that's what this
patch does. Thanks to durin42@ for the suggestion on #322.
---
 CHANGELOG.md                     |  3 +++
 lib/src/commit_builder.rs        |  8 ++++++
 lib/src/settings.rs              | 12 +++++++--
 lib/tests/test_commit_builder.rs | 46 ++++++++++++++++++++++++++++++++
 4 files changed, 67 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c731e29b1..464b981b0 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -99,6 +99,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `jj log` now accepts a `--reversed` option, which will show older commits
   first.
 
+* The "(no name/email configured)" placeholder value for name/email will now be
+  replaced if once you modify a commit after having configured your name/email.
+
 ### Fixed bugs
 
 * When rebasing a conflict where one side modified a file and the other side
diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs
index f20273918..df20183c4 100644
--- a/lib/src/commit_builder.rs
+++ b/lib/src/commit_builder.rs
@@ -66,6 +66,14 @@ impl CommitBuilder {
         let mut commit = predecessor.store_commit().clone();
         commit.predecessors = vec![predecessor.id().clone()];
         commit.committer = settings.signature();
+        // If the user had not configured a name and email before but now they have,
+        // update the the author fields with the new information.
+        if commit.author.name == UserSettings::user_name_placeholder() {
+            commit.author.name = commit.committer.name.clone();
+        }
+        if commit.author.email == UserSettings::user_email_placeholder() {
+            commit.author.email = commit.committer.email.clone();
+        }
         CommitBuilder {
             store: store.clone(),
             commit,
diff --git a/lib/src/settings.rs b/lib/src/settings.rs
index 062efa7c7..cd2d0f83b 100644
--- a/lib/src/settings.rs
+++ b/lib/src/settings.rs
@@ -56,13 +56,21 @@ impl UserSettings {
     pub fn user_name(&self) -> String {
         self.config
             .get_string("user.name")
-            .unwrap_or_else(|_| "(no name configured)".to_string())
+            .unwrap_or_else(|_| Self::user_name_placeholder().to_string())
+    }
+
+    pub fn user_name_placeholder() -> &'static str {
+        "(no name configured)"
     }
 
     pub fn user_email(&self) -> String {
         self.config
             .get_string("user.email")
-            .unwrap_or_else(|_| "(no email configured)".to_string())
+            .unwrap_or_else(|_| Self::user_email_placeholder().to_string())
+    }
+
+    pub fn user_email_placeholder() -> &'static str {
+        "(no email configured)"
     }
 
     pub fn push_branch_prefix(&self) -> String {
diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs
index 607e39b79..c958fab89 100644
--- a/lib/tests/test_commit_builder.rs
+++ b/lib/tests/test_commit_builder.rs
@@ -153,6 +153,52 @@ fn test_rewrite(use_git: bool) {
     );
 }
 
+// An author field with the placeholder name/email should get filled in on
+// rewrite
+#[test_case(false ; "local backend")]
+#[test_case(true ; "git backend")]
+fn test_rewrite_update_missing_user(use_git: bool) {
+    let missing_user_settings =
+        UserSettings::from_config(config::Config::builder().build().unwrap());
+    let test_repo = TestRepo::init(use_git);
+    let repo = &test_repo.repo;
+    let store = repo.store().clone();
+
+    let mut tx = repo.start_transaction("test");
+    let initial_commit = CommitBuilder::for_new_commit(
+        &missing_user_settings,
+        &store,
+        repo.store().empty_tree_id().clone(),
+    )
+    .write_to_repo(tx.mut_repo());
+    assert_eq!(initial_commit.author().name, "(no name configured)");
+    assert_eq!(initial_commit.author().email, "(no email configured)");
+    assert_eq!(initial_commit.committer().name, "(no name configured)");
+    assert_eq!(initial_commit.committer().email, "(no email configured)");
+
+    let config = config::Config::builder()
+        .set_override("user.name", "Configured User")
+        .unwrap()
+        .set_override("user.email", "configured.user@example.com")
+        .unwrap()
+        .build()
+        .unwrap();
+    let settings = UserSettings::from_config(config);
+    let rewritten_commit = CommitBuilder::for_rewrite_from(&settings, &store, &initial_commit)
+        .write_to_repo(tx.mut_repo());
+
+    assert_eq!(rewritten_commit.author().name, "Configured User");
+    assert_eq!(
+        rewritten_commit.author().email,
+        "configured.user@example.com"
+    );
+    assert_eq!(rewritten_commit.committer().name, "Configured User");
+    assert_eq!(
+        rewritten_commit.committer().email,
+        "configured.user@example.com"
+    );
+}
+
 #[test_case(false ; "local backend")]
 // #[test_case(true ; "git backend")]
 fn test_commit_builder_descendants(use_git: bool) {