Skip to content

Commit

Permalink
cli: add --config-file=PATH argument
Browse files Browse the repository at this point in the history
This would be useful for scripting purpose. Maybe we can also replace the
current --config-toml=<TOML> use cases by --config-file=<PATH> and simpler
--config=<KEY>=<VALUE>.

#4926 (comment)

If we want to add more source variants (such as fd number), it might be better
to add --config-from=<type>:<path|fd|..>. In any case, we'll probably want
--config=<KEY>=<VALUE>, and therefore, we'll need to merge more than one
--config* arguments.
  • Loading branch information
yuja committed Dec 13, 2024
1 parent c29b5a2 commit fca92f1
Show file tree
Hide file tree
Showing 8 changed files with 188 additions and 8 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
`snapshot.max-new-file-size` config option. It will print a warning and large
files will be left untracked.

* New command option `--config-file=PATH` to load additional configuration from
files.

* Templates now support the `>=`, `>`, `<=`, and `<` relational operators for
`Integer` types.

Expand Down
88 changes: 87 additions & 1 deletion cli/src/cli_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ use crate::complete;
use crate::config::config_from_environment;
use crate::config::parse_config_args;
use crate::config::CommandNameAndArgs;
use crate::config::ConfigArg;
use crate::config::ConfigEnv;
use crate::diff_util;
use crate::diff_util::DiffFormat;
Expand Down Expand Up @@ -3102,6 +3103,26 @@ pub struct EarlyArgs {
// cases, designed so that `--config ui.color=auto` works
#[arg(long, value_name = "TOML", global = true)]
pub config_toml: Vec<String>,
/// Additional configuration files (can be repeated)
#[arg(long, value_name = "PATH", global = true, value_hint = clap::ValueHint::FilePath)]
pub config_file: Vec<String>,
}

impl EarlyArgs {
fn merged_config_args(&self, matches: &ArgMatches) -> Vec<ConfigArg> {
merge_args_with(
matches,
&[
("config_toml", &self.config_toml),
("config_file", &self.config_file),
],
|id, value| match id {
"config_toml" => ConfigArg::Toml(value.clone()),
"config_file" => ConfigArg::File(value.clone()),
_ => unreachable!("unexpected id {id:?}"),
},
)
}
}

/// Wrapper around revset expression argument.
Expand Down Expand Up @@ -3143,6 +3164,29 @@ impl ValueParserFactory for RevisionArg {
}
}

/// Merges multiple clap args in order of appearance.
///
/// The `id_values` is a list of `(id, values)` pairs, where `id` is the name of
/// the clap `Arg`, and `values` are the parsed values for that arg. The
/// `convert` function transforms each `(id, value)` pair to e.g. an enum.
///
/// This is a workaround for <https://github.com/clap-rs/clap/issues/3146>.
pub fn merge_args_with<'k, 'v, T, U>(
matches: &ArgMatches,
id_values: &[(&'k str, &'v [T])],
mut convert: impl FnMut(&'k str, &'v T) -> U,
) -> Vec<U> {
let mut pos_values: Vec<(usize, U)> = Vec::new();
for (id, values) in id_values {
pos_values.extend(itertools::zip_eq(
matches.indices_of(id).into_iter().flatten(),
values.iter().map(|v| convert(id, v)),
));
}
pos_values.sort_unstable_by_key(|&(pos, _)| pos);
pos_values.into_iter().map(|(_, value)| value).collect()
}

fn get_string_or_array(
config: &StackedConfig,
key: &'static str,
Expand Down Expand Up @@ -3279,7 +3323,7 @@ fn handle_early_args(
let args = EarlyArgs::from_arg_matches(&early_matches).unwrap();

let old_layers_len = config.layers().len();
config.extend_layers(parse_config_args(&args.config_toml)?);
config.extend_layers(parse_config_args(&args.merged_config_args(&early_matches))?);

// Command arguments overrides any other configuration including the
// variables loaded from --config* arguments.
Expand Down Expand Up @@ -3711,3 +3755,45 @@ fn format_template_aliases_hint(template_aliases: &TemplateAliasesMap) -> String
);
hint
}

#[cfg(test)]
mod tests {
use clap::CommandFactory as _;

use super::*;

#[derive(clap::Parser, Clone, Debug)]
pub struct TestArgs {
#[arg(long)]
pub foo: Vec<u32>,
#[arg(long)]
pub bar: Vec<u32>,
#[arg(long)]
pub baz: bool,
}

#[test]
fn test_merge_args_with() {
let command = TestArgs::command();
let parse = |args: &[&str]| -> Vec<(&'static str, u32)> {
let matches = command.clone().try_get_matches_from(args).unwrap();
let args = TestArgs::from_arg_matches(&matches).unwrap();
merge_args_with(
&matches,
&[("foo", &args.foo), ("bar", &args.bar)],
|id, value| (id, *value),
)
};

assert_eq!(parse(&["jj"]), vec![]);
assert_eq!(parse(&["jj", "--foo=1"]), vec![("foo", 1)]);
assert_eq!(
parse(&["jj", "--foo=1", "--bar=2"]),
vec![("foo", 1), ("bar", 2)]
);
assert_eq!(
parse(&["jj", "--foo=1", "--baz", "--bar=2", "--foo", "3"]),
vec![("foo", 1), ("bar", 2), ("foo", 3)]
);
}
}
19 changes: 16 additions & 3 deletions cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -319,7 +319,7 @@ impl ConfigEnv {
/// 4. Repo config `.jj/repo/config.toml`
/// 5. TODO: Workspace config `.jj/config.toml`
/// 6. Override environment variables
/// 7. Command-line arguments `--config-toml`
/// 7. Command-line arguments `--config-toml`, `--config-file`
///
/// This function sets up 1, 2, and 6.
pub fn config_from_environment(
Expand Down Expand Up @@ -401,15 +401,28 @@ fn env_overrides_layer() -> ConfigLayer {
layer
}

/// Configuration source/data provided as command-line argument.
#[derive(Clone, Debug, Eq, PartialEq)]
pub enum ConfigArg {
/// `--config-toml=TOML`
Toml(String),
/// `--config-file=PATH`
File(String),
}

/// Parses `--config-toml` arguments.
pub fn parse_config_args(toml_strs: &[String]) -> Result<Vec<ConfigLayer>, ConfigLoadError> {
pub fn parse_config_args(toml_strs: &[ConfigArg]) -> Result<Vec<ConfigLayer>, ConfigLoadError> {
// It might look silly that a layer is constructed per argument, but
// --config-toml argument can contain a full TOML document, and it makes
// sense to preserve line numbers within the doc. If we add
// --config=KEY=VALUE, multiple values might be loaded into one layer.
let source = ConfigSource::CommandArg;
toml_strs
.iter()
.map(|text| ConfigLayer::parse(ConfigSource::CommandArg, text))
.map(|arg| match arg {
ConfigArg::Toml(text) => ConfigLayer::parse(source, text),
ConfigArg::File(path) => ConfigLayer::load_from_file(source, path.into()),
})
.try_collect()
}

Expand Down
1 change: 1 addition & 0 deletions cli/tests/[email protected]
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ To get started, see the tutorial at https://martinvonz.github.io/jj/latest/tutor
Warnings and errors will still be printed.
* `--no-pager` — Disable the pager
* `--config-toml <TOML>` — Additional configuration options (can be repeated)
* `--config-file <PATH>` — Additional configuration files (can be repeated)
Expand Down
1 change: 1 addition & 0 deletions cli/tests/test_completion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ fn test_bookmark_names() {
--quiet Silence non-primary command output
--no-pager Disable the pager
--config-toml Additional configuration options (can be repeated)
--config-file Additional configuration files (can be repeated)
--help Print help (see more with '--help')
");

Expand Down
69 changes: 69 additions & 0 deletions cli/tests/test_global_opts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@

use std::ffi::OsString;

use indoc::indoc;

use crate::common::get_stderr_string;
use crate::common::strip_last_line;
use crate::common::TestEnvironment;
Expand Down Expand Up @@ -595,6 +597,72 @@ fn test_early_args() {
);
}

#[test]
fn test_config_args() {
let test_env = TestEnvironment::default();
let list_config = |args: &[&str]| {
test_env.jj_cmd_success(
test_env.env_root(),
&[&["config", "list", "--include-overridden", "test"], args].concat(),
)
};

std::fs::write(
test_env.env_root().join("file1.toml"),
indoc! {"
test.key1 = 'file1'
test.key2 = 'file1'
"},
)
.unwrap();
std::fs::write(
test_env.env_root().join("file2.toml"),
indoc! {"
test.key3 = 'file2'
"},
)
.unwrap();

let stdout = list_config(&["--config-toml=test.key1='arg1'"]);
insta::assert_snapshot!(stdout, @"test.key1 = 'arg1'");
let stdout = list_config(&["--config-file=file1.toml"]);
insta::assert_snapshot!(stdout, @r"
test.key1 = 'file1'
test.key2 = 'file1'
");

// --config* arguments are processed in order of appearance
let stdout = list_config(&[
"--config-toml=test.key1='arg1'",
"--config-file=file1.toml",
"--config-toml=test.key2='arg3'",
"--config-file=file2.toml",
]);
insta::assert_snapshot!(stdout, @r"
# test.key1 = 'arg1'
test.key1 = 'file1'
# test.key2 = 'file1'
test.key2 = 'arg3'
test.key3 = 'file2'
");

let stderr = test_env.jj_cmd_failure(
test_env.env_root(),
&["config", "list", "--config-file=unknown.toml"],
);
insta::with_settings!({
filters => [("(?m)^([2-9]): .*", "$1: <redacted>")],
}, {
insta::assert_snapshot!(stderr, @r"
Config error: Failed to read configuration file
Caused by:
1: Cannot access unknown.toml
2: <redacted>
For help, see https://martinvonz.github.io/jj/latest/config/.
");
});
}

#[test]
fn test_invalid_config() {
// Test that we get a reasonable error if the config is invalid (#55)
Expand Down Expand Up @@ -708,6 +776,7 @@ fn test_help() {
--quiet Silence non-primary command output
--no-pager Disable the pager
--config-toml <TOML> Additional configuration options (can be repeated)
--config-file <PATH> Additional configuration files (can be repeated)
");
}

Expand Down
12 changes: 9 additions & 3 deletions docs/config.md
Original file line number Diff line number Diff line change
Expand Up @@ -1203,9 +1203,9 @@ env JJ_CONFIG=/dev/null jj log # Ignores any settings specified in the con

### Specifying config on the command-line

You can use one or more `--config-toml` options on the command line to specify
additional configuration settings. This overrides settings defined in config
files or environment variables. For example,
You can use one or more `--config-toml`/`--config-file` options on the command
line to specify additional configuration settings. This overrides settings
defined in config files or environment variables. For example,

```shell
jj --config-toml='ui.color="always"' --config-toml='ui.diff-editor="kdiff3"' split
Expand All @@ -1221,3 +1221,9 @@ files with the config specified in `.jjconfig.toml`:
```shell
jj --config-toml="$(cat extra-config.toml)" log
```

This is equivalent to

```shell
jj --config-file=extra-config.toml log
```
3 changes: 2 additions & 1 deletion lib/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,8 @@ impl ConfigLayer {
Ok(Self::with_data(source, data.into_mut()))
}

fn load_from_file(source: ConfigSource, path: PathBuf) -> Result<Self, ConfigLoadError> {
/// Loads TOML file from the specified `path`.
pub fn load_from_file(source: ConfigSource, path: PathBuf) -> Result<Self, ConfigLoadError> {
let text = fs::read_to_string(&path)
.context(&path)
.map_err(ConfigLoadError::Read)?;
Expand Down

0 comments on commit fca92f1

Please sign in to comment.