Skip to content

Commit

Permalink
settings: propagate error from UserSettings::from_config()
Browse files Browse the repository at this point in the history
All variables parsed here are debug options, but it would be annoying if
timestamp options were silently ignored because of a typo.
  • Loading branch information
yuja committed Dec 18, 2024
1 parent 8fc0669 commit 4f08c62
Show file tree
Hide file tree
Showing 10 changed files with 36 additions and 30 deletions.
2 changes: 1 addition & 1 deletion cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion cli/src/complete.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down
8 changes: 4 additions & 4 deletions cli/src/merge_tools/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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)
};
Expand Down Expand Up @@ -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)
};
Expand Down
2 changes: 1 addition & 1 deletion lib/src/git_backend.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
}
29 changes: 16 additions & 13 deletions lib/src/settings.rs
Original file line number Diff line number Diff line change
Expand Up @@ -124,28 +124,31 @@ impl SignSettings {
}
}

fn get_timestamp_config(config: &StackedConfig, key: &'static str) -> Option<Timestamp> {
fn to_timestamp(value: ConfigValue) -> Result<Timestamp, Box<dyn std::error::Error + Send + Sync>> {
// 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,
if let Some(s) = value.as_str() {
Ok(Timestamp::from_datetime(DateTime::parse_from_rfc3339(s)?))
} else {
let ty = value.type_name();
Err(format!("invalid type: {ty}, expected a date-time string").into())
}
}

impl UserSettings {
pub fn from_config(config: StackedConfig) -> Self {
let commit_timestamp = get_timestamp_config(&config, "debug.commit-timestamp");
let operation_timestamp = get_timestamp_config(&config, "debug.operation-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,
commit_timestamp,
operation_timestamp,
rng: Arc::new(JJRng::new(rng_seed)),
}
})
}

// TODO: Reconsider UserSettings/RepoSettings abstraction. See
Expand Down
15 changes: 9 additions & 6 deletions lib/tests/test_commit_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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)
Expand All @@ -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()
Expand All @@ -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)
Expand All @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/test_init.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/test_operations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,7 +431,7 @@ fn stable_op_id_settings() -> UserSettings {
)
.unwrap(),
);
UserSettings::from_config(config)
UserSettings::from_config(config).unwrap()
}

#[test]
Expand Down
2 changes: 1 addition & 1 deletion lib/tests/test_signing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
2 changes: 1 addition & 1 deletion lib/testutils/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down

0 comments on commit 4f08c62

Please sign in to comment.