From 3f8ac2198d87729be2bce8ac8b7e41a4fe74746f Mon Sep 17 00:00:00 2001 From: Emily Fox Date: Wed, 16 Aug 2023 16:10:54 -0500 Subject: [PATCH] commits: use empty strings instead of placeholders for missing name or email This commit replaces the functions `UserSettings::user_name_placeholder()`` and `UserSettings::user_email_placeholder()` with `const` `&str`s to emphasize that the placeholder strings must not be changed to support commits without names or email addresses made before this change. --- CHANGELOG.md | 5 +++++ cli/src/cli_util.rs | 4 +--- cli/src/commands/git.rs | 12 ++++++++---- cli/tests/test_git_push.rs | 10 +++++----- lib/src/commit_builder.rs | 8 ++++++-- lib/src/settings.rs | 19 +++++++------------ lib/tests/test_commit_builder.rs | 11 +++++------ lib/tests/test_init.rs | 17 ++++------------- 8 files changed, 41 insertions(+), 45 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 22da5bb60..c0d81ffec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 * `jj` will fail attempts to snapshot new files larger than 1MiB by default. This behavior can be customized with the `snapshot.max-new-file-size` config option. +* Author and committer signatures now use empty strings to represent unset + names and email addresses. + Older binaries may not warn user when attempting to `git push` commits + with such signatures. + ### New features diff --git a/cli/src/cli_util.rs b/cli/src/cli_util.rs index 3bf7c2e53..ac2633c6e 100644 --- a/cli/src/cli_util.rs +++ b/cli/src/cli_util.rs @@ -1397,9 +1397,7 @@ See https://github.com/martinvonz/jj/blob/main/docs/working-copy.md#stale-workin self.update_working_copy(ui, maybe_old_commit.as_ref())?; } let settings = &self.settings; - if settings.user_name() == UserSettings::user_name_placeholder() - || settings.user_email() == UserSettings::user_email_placeholder() - { + if settings.user_name().is_empty() || settings.user_email().is_empty() { writeln!( ui.warning(), r#"Name and email not configured. Until configured, your commits will be created with the empty identity, and can't be pushed to remotes. To configure, run: diff --git a/cli/src/commands/git.rs b/cli/src/commands/git.rs index ca3738c6e..05e45cca3 100644 --- a/cli/src/commands/git.rs +++ b/cli/src/commands/git.rs @@ -905,10 +905,14 @@ fn cmd_git_push( if commit.description().is_empty() { reasons.push("it has no description"); } - if commit.author().name == UserSettings::user_name_placeholder() - || commit.author().email == UserSettings::user_email_placeholder() - || commit.committer().name == UserSettings::user_name_placeholder() - || commit.committer().email == UserSettings::user_email_placeholder() + if commit.author().name.is_empty() + || commit.author().name == UserSettings::USER_NAME_PLACEHOLDER + || commit.author().email.is_empty() + || commit.author().email == UserSettings::USER_EMAIL_PLACEHOLDER + || commit.committer().name.is_empty() + || commit.committer().name == UserSettings::USER_NAME_PLACEHOLDER + || commit.committer().email.is_empty() + || commit.committer().email == UserSettings::USER_EMAIL_PLACEHOLDER { reasons.push("it has no author and/or committer set"); } diff --git a/cli/tests/test_git_push.rs b/cli/tests/test_git_push.rs index c3ab6ccbf..fbbe58b04 100644 --- a/cli/tests/test_git_push.rs +++ b/cli/tests/test_git_push.rs @@ -483,14 +483,14 @@ fn test_git_push_missing_author() { &["git", "push", "--branch", "missing-name"], ); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit 574dffd73428 since it has no author and/or committer set + Error: Won't push commit 944313939bbd since it has no author and/or committer set "###); run_without_var("JJ_EMAIL", &["checkout", "root", "-m=initial"]); run_without_var("JJ_EMAIL", &["branch", "create", "missing-email"]); let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch=missing-email"]); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit e6c50f13f197 since it has no author and/or committer set + Error: Won't push commit 59354714f789 since it has no author and/or committer set "###); } @@ -509,7 +509,7 @@ fn test_git_push_missing_committer() { let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch=missing-name"]); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit e009726caa4a since it has no author and/or committer set + Error: Won't push commit 4fd190283d1a since it has no author and/or committer set "###); test_env.jj_cmd_success(&workspace_root, &["checkout", "root"]); test_env.jj_cmd_success(&workspace_root, &["branch", "create", "missing-email"]); @@ -517,7 +517,7 @@ fn test_git_push_missing_committer() { let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch=missing-email"]); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit 27ec5f0793e6 since it has no author and/or committer set + Error: Won't push commit eab97428a6ec since it has no author and/or committer set "###); // Test message when there are multiple reasons (missing committer and @@ -526,7 +526,7 @@ fn test_git_push_missing_committer() { let stderr = test_env.jj_cmd_failure(&workspace_root, &["git", "push", "--branch=missing-email"]); insta::assert_snapshot!(stderr, @r###" - Error: Won't push commit f73024ee65ec since it has no description and it has no author and/or committer set + Error: Won't push commit 1143ed607f54 since it has no description and it has no author and/or committer set "###); } diff --git a/lib/src/commit_builder.rs b/lib/src/commit_builder.rs index 26c3e4d4d..164f0ecd7 100644 --- a/lib/src/commit_builder.rs +++ b/lib/src/commit_builder.rs @@ -70,10 +70,14 @@ impl CommitBuilder<'_> { commit.committer = settings.signature(); // If the user had not configured a name and email before but now they have, // update the author fields with the new information. - if commit.author.name == UserSettings::user_name_placeholder() { + if commit.author.name.is_empty() + || commit.author.name == UserSettings::USER_NAME_PLACEHOLDER + { commit.author.name = commit.committer.name.clone(); } - if commit.author.email == UserSettings::user_email_placeholder() { + if commit.author.email.is_empty() + || commit.author.email == UserSettings::USER_EMAIL_PLACEHOLDER + { commit.author.email = commit.committer.email.clone(); } CommitBuilder { diff --git a/lib/src/settings.rs b/lib/src/settings.rs index fcf3c0500..bceea1bab 100644 --- a/lib/src/settings.rs +++ b/lib/src/settings.rs @@ -98,19 +98,14 @@ impl UserSettings { } pub fn user_name(&self) -> String { - self.config - .get_string("user.name") - .unwrap_or_else(|_| Self::user_name_placeholder().to_string()) + self.config.get_string("user.name").unwrap_or_default() } - pub fn user_name_placeholder() -> &'static str { - "(no name configured)" - } + // Must not be changed to avoid git pushing older commits with no set name + pub const USER_NAME_PLACEHOLDER: &str = "(no name configured)"; pub fn user_email(&self) -> String { - self.config - .get_string("user.email") - .unwrap_or_else(|_| Self::user_email_placeholder().to_string()) + self.config.get_string("user.email").unwrap_or_default() } pub fn fsmonitor_kind(&self) -> Result, config::ConfigError> { @@ -121,9 +116,9 @@ impl UserSettings { } } - pub fn user_email_placeholder() -> &'static str { - "(no email configured)" - } + // Must not be changed to avoid git pushing older commits with no set email + // address + pub const USER_EMAIL_PLACEHOLDER: &str = "(no email configured)"; pub fn operation_timestamp(&self) -> Option { get_timestamp_config(&self.config, "debug.operation-timestamp") diff --git a/lib/tests/test_commit_builder.rs b/lib/tests/test_commit_builder.rs index d1e654810..43a336f73 100644 --- a/lib/tests/test_commit_builder.rs +++ b/lib/tests/test_commit_builder.rs @@ -188,8 +188,7 @@ fn test_rewrite(use_git: bool) { ); } -// An author field with the placeholder name/email should get filled in on -// rewrite +// An author field with an empty 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) { @@ -208,10 +207,10 @@ fn test_rewrite_update_missing_user(use_git: bool) { ) .write() .unwrap(); - 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)"); + assert_eq!(initial_commit.author().name, ""); + assert_eq!(initial_commit.author().email, ""); + assert_eq!(initial_commit.committer().name, ""); + assert_eq!(initial_commit.committer().email, ""); let config = config::Config::builder() .set_override("user.name", "Configured User") diff --git a/lib/tests/test_init.rs b/lib/tests/test_init.rs index 949e496bd..8acc4474d 100644 --- a/lib/tests/test_init.rs +++ b/lib/tests/test_init.rs @@ -105,19 +105,10 @@ fn test_init_no_config_set(use_git: bool) { .get_wc_commit_id(&WorkspaceId::default()) .unwrap(); let wc_commit = repo.store().get_commit(wc_commit_id).unwrap(); - assert_eq!(wc_commit.author().name, "(no name configured)".to_string()); - assert_eq!( - wc_commit.author().email, - "(no email configured)".to_string() - ); - assert_eq!( - wc_commit.committer().name, - "(no name configured)".to_string() - ); - assert_eq!( - wc_commit.committer().email, - "(no email configured)".to_string() - ); + assert_eq!(wc_commit.author().name, "".to_string()); + assert_eq!(wc_commit.author().email, "".to_string()); + assert_eq!(wc_commit.committer().name, "".to_string()); + assert_eq!(wc_commit.committer().email, "".to_string()); } #[test_case(false ; "local backend")]