settings: propagate error from UserSettings::from_config()

cli/src/cli_util.rs

@@ -3704,7 +3704,7 @@ impl CliRunner {
             }
         }
 
-        let settings = UserSettings::from_config(config);
+        let settings = UserSettings::from_config(config)?;
         let command_helper_data = CommandHelperData {
             app: self.app,
             cwd,

cli/src/complete.rs

@@ -753,7 +753,7 @@ fn get_jj_command() -> Result<(JjBuilder, UserSettings), CommandError> {
         cmd: current_exe,
         args: cmd_args,
     };
-    let settings = UserSettings::from_config(config);
+    let settings = UserSettings::from_config(config)?;
 
     Ok((builder, settings))
 }

cli/src/merge_tools/mod.rs

@@ -373,7 +373,7 @@ mod tests {
     fn test_get_diff_editor_with_name() {
         let get = |name, config_text| {
             let config = config_from_string(config_text);
-            let settings = UserSettings::from_config(config);
+            let settings = UserSettings::from_config(config).unwrap();
             DiffEditor::with_name(
                 name,
                 &settings,
@@ -442,7 +442,7 @@ mod tests {
         let get = |text| {
             let config = config_from_string(text);
             let ui = Ui::with_config(&config).unwrap();
-            let settings = UserSettings::from_config(config);
+            let settings = UserSettings::from_config(config).unwrap();
             DiffEditor::from_settings(
                 &ui,
                 &settings,
@@ -614,7 +614,7 @@ mod tests {
     fn test_get_merge_editor_with_name() {
         let get = |name, config_text| {
             let config = config_from_string(config_text);
-            let settings = UserSettings::from_config(config);
+            let settings = UserSettings::from_config(config).unwrap();
             MergeEditor::with_name(name, &settings, ConflictMarkerStyle::Diff)
                 .map(|editor| editor.tool)
         };
@@ -666,7 +666,7 @@ mod tests {
         let get = |text| {
             let config = config_from_string(text);
             let ui = Ui::with_config(&config).unwrap();
-            let settings = UserSettings::from_config(config);
+            let settings = UserSettings::from_config(config).unwrap();
             MergeEditor::from_settings(&ui, &settings, ConflictMarkerStyle::Diff)
                 .map(|editor| editor.tool)
         };

cli/tests/test_revset_output.rs

@@ -740,7 +740,7 @@ fn test_revset_committer_date_with_time_zone() {
     test_env.jj_cmd_ok(
         &repo_path,
         &[
-            "--config=debug.commit-timestamp='2023-01-25T11:30:00-05:00'",
+            "--config=debug.commit-timestamp=2023-01-25T11:30:00-05:00",
             "describe",
             "-m",
             "first",
@@ -749,7 +749,7 @@ fn test_revset_committer_date_with_time_zone() {
     test_env.jj_cmd_ok(
         &repo_path,
         &[
-            "--config=debug.commit-timestamp='2023-01-25T12:30:00-05:00'",
+            "--config=debug.commit-timestamp=2023-01-25T12:30:00-05:00",
             "new",
             "-m",
             "second",
@@ -758,7 +758,7 @@ fn test_revset_committer_date_with_time_zone() {
     test_env.jj_cmd_ok(
         &repo_path,
         &[
-            "--config=debug.commit-timestamp='2023-01-25T13:30:00-05:00'",
+            "--config=debug.commit-timestamp=2023-01-25T13:30:00-05:00",
             "new",
             "-m",
             "third",
@@ -768,7 +768,7 @@ fn test_revset_committer_date_with_time_zone() {
     let mut log_commits_before_and_after =
         |committer_date: &str, now: &str, tz: &str| -> (String, String) {
             test_env.add_env_var("TZ", tz);
-            let config = format!("debug.commit-timestamp='{now}'");
+            let config = format!("debug.commit-timestamp={now}");
             let before_log = test_env.jj_cmd_success(
                 &repo_path,
                 &[

lib/src/git_backend.rs

@@ -2222,6 +2222,6 @@ mod tests {
     // our UserSettings type comes from jj_lib (1).
     fn user_settings() -> UserSettings {
         let config = StackedConfig::empty();
-        UserSettings::from_config(config)
+        UserSettings::from_config(config).unwrap()
     }
 }

lib/src/settings.rs

@@ -42,7 +42,8 @@ use crate::signing::SignBehavior;
 #[derive(Debug, Clone)]
 pub struct UserSettings {
     config: StackedConfig,
-    timestamp: Option<Timestamp>,
+    commit_timestamp: Option<Timestamp>,
+    operation_timestamp: Option<Timestamp>,
     rng: Arc<JJRng>,
 }
 
@@ -123,26 +124,36 @@ impl SignSettings {
     }
 }
 
-fn get_timestamp_config(config: &StackedConfig, key: &'static str) -> Option<Timestamp> {
-    // TODO: Maybe switch to native TOML date-time type?
-    match config.get::<String>(key) {
-        Ok(timestamp_str) => match DateTime::parse_from_rfc3339(&timestamp_str) {
-            Ok(datetime) => Some(Timestamp::from_datetime(datetime)),
-            Err(_) => None,
-        },
-        Err(_) => None,
+fn to_timestamp(value: ConfigValue) -> Result<Timestamp, Box<dyn std::error::Error + Send + Sync>> {
+    // Since toml_edit::Datetime isn't the date-time type used across our code
+    // base, we accept both string and date-time types.
+    if let Some(s) = value.as_str() {
+        Ok(Timestamp::from_datetime(DateTime::parse_from_rfc3339(s)?))
+    } else if let Some(d) = value.as_datetime() {
+        // It's easier to re-parse the TOML date-time expression.
+        let s = d.to_string();
+        Ok(Timestamp::from_datetime(DateTime::parse_from_rfc3339(&s)?))
+    } else {
+        let ty = value.type_name();
+        Err(format!("invalid type: {ty}, expected a date-time").into())
     }
 }
 
 impl UserSettings {
-    pub fn from_config(config: StackedConfig) -> Self {
-        let timestamp = get_timestamp_config(&config, "debug.commit-timestamp");
-        let rng_seed = config.get::<u64>("debug.randomness-seed").ok();
-        UserSettings {
+    pub fn from_config(config: StackedConfig) -> Result<Self, ConfigGetError> {
+        let commit_timestamp = config
+            .get_value_with("debug.commit-timestamp", to_timestamp)
+            .optional()?;
+        let operation_timestamp = config
+            .get_value_with("debug.operation-timestamp", to_timestamp)
+            .optional()?;
+        let rng_seed = config.get::<u64>("debug.randomness-seed").optional()?;
+        Ok(UserSettings {
             config,
-            timestamp,
+            commit_timestamp,
+            operation_timestamp,
             rng: Arc::new(JJRng::new(rng_seed)),
-        }
+        })
     }
 
     // TODO: Reconsider UserSettings/RepoSettings abstraction. See
@@ -176,11 +187,11 @@ impl UserSettings {
     pub const USER_EMAIL_PLACEHOLDER: &'static str = "(no email configured)";
 
     pub fn commit_timestamp(&self) -> Option<Timestamp> {
-        self.timestamp
+        self.commit_timestamp
     }
 
     pub fn operation_timestamp(&self) -> Option<Timestamp> {
-        get_timestamp_config(&self.config, "debug.operation-timestamp")
+        self.operation_timestamp
     }
 
     pub fn operation_hostname(&self) -> String {
@@ -212,7 +223,7 @@ impl UserSettings {
     }
 
     pub fn signature(&self) -> Signature {
-        let timestamp = self.timestamp.unwrap_or_else(Timestamp::now);
+        let timestamp = self.commit_timestamp.unwrap_or_else(Timestamp::now);
         Signature {
             name: self.user_name(),
             email: self.user_email(),

lib/tests/test_commit_builder.rs

@@ -177,7 +177,7 @@ fn test_rewrite(backend: TestRepoBackend) {
         )
         .unwrap(),
     );
-    let rewrite_settings = UserSettings::from_config(config);
+    let rewrite_settings = UserSettings::from_config(config).unwrap();
     let mut tx = repo.start_transaction(&settings);
     let rewritten_commit = tx
         .repo_mut()
@@ -221,7 +221,7 @@ fn test_rewrite(backend: TestRepoBackend) {
 #[test_case(TestRepoBackend::Local ; "local backend")]
 #[test_case(TestRepoBackend::Git ; "git backend")]
 fn test_rewrite_update_missing_user(backend: TestRepoBackend) {
-    let missing_user_settings = UserSettings::from_config(StackedConfig::empty());
+    let missing_user_settings = UserSettings::from_config(StackedConfig::empty()).unwrap();
     let test_repo = TestRepo::init_with_backend(backend);
     let repo = &test_repo.repo;
 
@@ -251,7 +251,7 @@ fn test_rewrite_update_missing_user(backend: TestRepoBackend) {
         )
         .unwrap(),
     );
-    let settings = UserSettings::from_config(config);
+    let settings = UserSettings::from_config(config).unwrap();
     let rewritten_commit = tx
         .repo_mut()
         .rewrite_commit(&settings, &initial_commit)
@@ -278,7 +278,8 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {
 
     // Create discardable commit
     let initial_timestamp = "2001-02-03T04:05:06+07:00";
-    let settings = UserSettings::from_config(config_with_commit_timestamp(initial_timestamp));
+    let settings =
+        UserSettings::from_config(config_with_commit_timestamp(initial_timestamp)).unwrap();
     let mut tx = repo.start_transaction(&settings);
     let initial_commit = tx
         .repo_mut()
@@ -297,7 +298,8 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {
 
     // Rewrite discardable commit to no longer be discardable
     let new_timestamp_1 = "2002-03-04T05:06:07+08:00";
-    let settings = UserSettings::from_config(config_with_commit_timestamp(new_timestamp_1));
+    let settings =
+        UserSettings::from_config(config_with_commit_timestamp(new_timestamp_1)).unwrap();
     let rewritten_commit_1 = tx
         .repo_mut()
         .rewrite_commit(&settings, &initial_commit)
@@ -315,7 +317,8 @@ fn test_rewrite_resets_author_timestamp(backend: TestRepoBackend) {
 
     // Rewrite non-discardable commit
     let new_timestamp_2 = "2003-04-05T06:07:08+09:00";
-    let settings = UserSettings::from_config(config_with_commit_timestamp(new_timestamp_2));
+    let settings =
+        UserSettings::from_config(config_with_commit_timestamp(new_timestamp_2)).unwrap();
     let rewritten_commit_2 = tx
         .repo_mut()
         .rewrite_commit(&settings, &rewritten_commit_1)

lib/tests/test_init.rs

@@ -143,7 +143,7 @@ fn test_init_no_config_set(backend: TestRepoBackend) {
     // Test that we can create a repo without setting any config
     // TODO: Perhaps, StackedConfig::empty() will be replaced with ::default()
     // or something that includes the minimal configuration variables.
-    let settings = UserSettings::from_config(StackedConfig::empty());
+    let settings = UserSettings::from_config(StackedConfig::empty()).unwrap();
     let test_workspace = TestWorkspace::init_with_backend(&settings, backend);
     let repo = &test_workspace.repo;
     let wc_commit_id = repo

lib/tests/test_operations.rs

@@ -427,11 +427,11 @@ fn stable_op_id_settings() -> UserSettings {
     config.add_layer(
         ConfigLayer::parse(
             ConfigSource::User,
-            "debug.operation-timestamp = '2001-02-03T04:05:06+07:00'",
+            "debug.operation-timestamp = 2001-02-03T04:05:06+07:00",
         )
         .unwrap(),
     );
-    UserSettings::from_config(config)
+    UserSettings::from_config(config).unwrap()
 }
 
 #[test]

lib/tests/test_signing.rs

@@ -30,7 +30,7 @@ fn user_settings(sign_all: bool) -> UserSettings {
         )
         .unwrap(),
     );
-    UserSettings::from_config(config)
+    UserSettings::from_config(config).unwrap()
 }
 
 fn someone_else() -> Signature {

lib/testutils/src/lib.rs

@@ -123,7 +123,7 @@ pub fn base_user_config() -> StackedConfig {
 /// Returns new immutable settings object that includes fake user configuration
 /// needed to run basic operations.
 pub fn user_settings() -> UserSettings {
-    UserSettings::from_config(base_user_config())
+    UserSettings::from_config(base_user_config()).unwrap()
 }
 
 #[derive(Debug)]