Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add workspace config #3670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+97 −11
Open

Add workspace config #3670

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
address unconditionally. Only ASCII case folding is currently implemented,
but this will likely change in the future.

* Workspaces may have an additional layered configuration, located at
`.jj/config.toml`. `jj config` subcommands which took layer options like
`--repo` now also support `--workspace`.

### Fixed bugs

## [0.19.0] - 2024-07-03
Expand Down
12 changes: 11 additions & 1 deletion cli/src/cli_util.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2133,6 +2133,11 @@ pub fn get_new_config_file_path(
new_config_path()?.ok_or_else(|| user_error("No repo config path found to edit"))?
}
ConfigSource::Repo => command.workspace_loader()?.repo_path().join("config.toml"),
ConfigSource::Workspace => command
.workspace_loader()?
.workspace_root()
.join(".jj")
.join("config.toml"),
_ => {
return Err(user_error(format!(
"Can't get path for config source {config_source:?}"
Expand Down Expand Up @@ -2836,14 +2841,18 @@ impl CliRunner {
.map_err(|err| map_workspace_load_error(err, None));
layered_configs.read_user_config()?;
let mut repo_config_path = None;
let mut workspace_config_path = None;
if let Ok(loader) = &maybe_cwd_workspace_loader {
layered_configs.read_repo_config(loader.repo_path())?;
layered_configs.read_workspace_config(loader.workspace_root())?;
repo_config_path = Some(layered_configs.repo_config_path(loader.repo_path()));
workspace_config_path =
Some(layered_configs.workspace_config_path(loader.workspace_root()));
}
let config = layered_configs.merge();
ui.reset(&config).map_err(|e| {
let user_config_path = layered_configs.user_config_path().unwrap_or(None);
let paths = [repo_config_path, user_config_path]
let paths = [repo_config_path, user_config_path, workspace_config_path]
.into_iter()
.flatten()
.map(|path| format!("- {}", path.display()))
Expand All @@ -2869,6 +2878,7 @@ impl CliRunner {
let loader = WorkspaceLoader::init(&cwd.join(path))
.map_err(|err| map_workspace_load_error(err, Some(path)))?;
layered_configs.read_repo_config(loader.repo_path())?;
layered_configs.read_workspace_config(loader.workspace_root())?;
Ok(loader)
} else {
maybe_cwd_workspace_loader
Expand Down
6 changes: 6 additions & 0 deletions cli/src/commands/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ pub(crate) struct ConfigLevelArgs {
/// Target the repo-level config
#[arg(long, group = "config_level")]
repo: bool,

/// Target the workspace-level config
#[arg(long, group = "config_level")]
workspace: bool,
}

impl ConfigLevelArgs {
Expand All @@ -48,6 +52,8 @@ impl ConfigLevelArgs {
Some(ConfigSource::User)
} else if self.repo {
Some(ConfigSource::Repo)
} else if self.workspace {
Some(ConfigSource::Workspace)
} else {
None
}
Expand Down
21 changes: 20 additions & 1 deletion cli/src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ pub enum ConfigSource {
// TODO: Track explicit file paths, especially for when user config is a dir.
User,
Repo,
Workspace,
CommandArg,
}

Expand All @@ -194,7 +195,7 @@ pub struct AnnotatedValue {
/// 2. Base environment variables
/// 3. [User config](https://github.com/martinvonz/jj/blob/main/docs/config.md#configuration)
/// 4. Repo config `.jj/repo/config.toml`
/// 5. TODO: Workspace config `.jj/config.toml`
/// 5. Workspace config `.jj/config.toml`
Copy link
Contributor

@ilyagr ilyagr May 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether .jj/working_copy/config.toml might be a better location for this. Otherwise, this emphasizes the working-copy config over the repo-level config, and I'm not sure that's desired (people might assume this is the ONLY config file, and that it's therefore repo-level).

@yuja , @martinvonz , WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether .jj/working_copy/config.toml might be a better location for this.

My feeling is that .jj/working_copy is the store directory dedicated for the WorkingCopy implementation, so better to not add arbitrary files in it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

I suppose, if need be, we could later move .jj/working_copy to .jj/workspace/working_copy and then .jj/config.toml to .jj/workspace/config.toml.

Copy link
Contributor

@ilyagr ilyagr Jul 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still find myself a little worried about people thinking that workspaces are broken if they end up using .jj/config.toml instead of .jj/repo/config.toml.

WDYT of putting this file in .jj/workspace/config.toml now, and then moving working_copy to that dir later?

This is made better by jj config edit and jj config path existing, so I don't think it's absolutely critical, but it would be nice to not have to move the config file later (with all the doc changes and backwards compatibility work that involves)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the file can be renamed to .jj/workspace-config.toml, then?

BTW, do we have any practical use case of workspace-specific configuration? If we don't, and if the existence of .jj/config.toml is confusing, it might be better to not support workspace config at all. Not having workspace config might help simplify IncludeIf-like conditional. #616

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that the config for not ignoring the executable bit in the working copy (#4478) could be stored in a workspace-level config

Yes, but I assumed it's practically okay to set it to repo/config.toml. (Maybe I should say "immediate need", not "real use case".)

Copy link
Contributor

@ilyagr ilyagr Sep 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we go in the direction of supporting "IncludeIf" and conditional config, it might make sense to put workspace config inside repo config under a key like [if.workspace.default] (assuming if.something is how other conditional config can be setup; I'd want these to have similar syntax if possible, thought it's not absolutely necessary).

For something like executable bits, we could eventually have a fancier conditionals like "Config for all workspaces under /mnt/c" (or whatever the WSL mount for the NTFS C: drive is).

At the same time, I'm not sure whether the conditional config idea is more practical for now (or in general). It might be easier to have workspace-config.toml for now (or even permanently).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For context, the reason I went ahead and implemented the workspace config is because I had a complex repository scenario where multiple related-but-divergent trunks shared a lot of history, and wanted to use multiple workspaces to manage the different related codebases with different jj templates and revsets for each.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, if there's a real need, we shouldn't be too nitpicky. Let's land .jj/config.toml or .jj/workspace-config.toml. I think @ilyagr is okay with .jj/workspace-config.toml.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm OK with .jj/workspace-config.toml. I appreciate Eidolon explaining one more use-case; I don't have a clear opinion on what the conclusion from that or from Martin's use-case should be.

/// 6. Override environment variables
/// 7. Command-line arguments `--config-toml`
#[derive(Clone, Debug)]
Expand All @@ -203,6 +204,7 @@ pub struct LayeredConfigs {
env_base: config::Config,
user: Option<config::Config>,
repo: Option<config::Config>,
workspace: Option<config::Config>,
env_overrides: config::Config,
arg_overrides: Option<config::Config>,
}
Expand All @@ -215,6 +217,7 @@ impl LayeredConfigs {
env_base: env_base(),
user: None,
repo: None,
workspace: None,
env_overrides: env_overrides(),
arg_overrides: None,
}
Expand All @@ -238,10 +241,22 @@ impl LayeredConfigs {
Ok(())
}

#[instrument]
pub fn read_workspace_config(&mut self, workspace_root: &Path) -> Result<(), ConfigError> {
self.workspace = Some(read_config_file(
&self.workspace_config_path(workspace_root),
)?);
Ok(())
}

pub fn repo_config_path(&self, repo_path: &Path) -> PathBuf {
repo_path.join("config.toml")
}

pub fn workspace_config_path(&self, workspace_root: &Path) -> PathBuf {
workspace_root.join(".jj").join("config.toml")
}

pub fn parse_config_args(&mut self, toml_strs: &[String]) -> Result<(), ConfigError> {
let config = toml_strs
.iter()
Expand Down Expand Up @@ -271,6 +286,7 @@ impl LayeredConfigs {
(ConfigSource::Env, Some(&self.env_base)),
(ConfigSource::User, self.user.as_ref()),
(ConfigSource::Repo, self.repo.as_ref()),
(ConfigSource::Workspace, self.workspace.as_ref()),
(ConfigSource::Env, Some(&self.env_overrides)),
(ConfigSource::CommandArg, self.arg_overrides.as_ref()),
];
Expand Down Expand Up @@ -880,6 +896,7 @@ mod tests {
env_base: empty_config.to_owned(),
user: None,
repo: None,
workspace: None,
env_overrides: empty_config,
arg_overrides: None,
};
Expand Down Expand Up @@ -911,6 +928,7 @@ mod tests {
env_base: env_base_config,
user: None,
repo: Some(repo_config),
workspace: None,
env_overrides: empty_config,
arg_overrides: None,
};
Expand Down Expand Up @@ -1034,6 +1052,7 @@ mod tests {
env_base: empty_config.to_owned(),
user: Some(user_config),
repo: Some(repo_config),
workspace: None,
env_overrides: empty_config,
arg_overrides: None,
};
Expand Down
10 changes: 7 additions & 3 deletions cli/tests/[email protected]
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,12 +473,13 @@ Start an editor on a jj config file.

Creates the file if it doesn't already exist regardless of what the editor does.

**Usage:** `jj config edit <--user|--repo>`
**Usage:** `jj config edit <--user|--repo|--workspace>`

###### **Options:**

* `--user` — Target the user-level config
* `--repo` — Target the repo-level config
* `--workspace` — Target the workspace-level config



Expand Down Expand Up @@ -518,6 +519,7 @@ List variables set in config file, along with their values
* `--include-overridden` — Allow printing overridden values
* `--user` — Target the user-level config
* `--repo` — Target the repo-level config
* `--workspace` — Target the workspace-level config
* `-T`, `--template <TEMPLATE>` — Render each variable using the given template

The following keywords are defined:
Expand All @@ -538,20 +540,21 @@ A config file at that path may or may not exist.

See `jj config edit` if you'd like to immediately edit the file.

**Usage:** `jj config path <--user|--repo>`
**Usage:** `jj config path <--user|--repo|--workspace>`

###### **Options:**

* `--user` — Target the user-level config
* `--repo` — Target the repo-level config
* `--workspace` — Target the workspace-level config



## `jj config set`

Update config file to set the given option to a given value

**Usage:** `jj config set <--user|--repo> <NAME> <VALUE>`
**Usage:** `jj config set <--user|--repo|--workspace> <NAME> <VALUE>`

###### **Arguments:**

Expand All @@ -562,6 +565,7 @@ Update config file to set the given option to a given value

* `--user` — Target the user-level config
* `--repo` — Target the repo-level config
* `--workspace` — Target the workspace-level config



Expand Down
51 changes: 45 additions & 6 deletions cli/tests/test_config_command.rs
View file
Open in desktop
HybridEidolon marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -203,10 +203,22 @@ fn test_config_list_layer() {
],
);

test_env.jj_cmd_ok(
&repo_path,
&[
"config",
"set",
"--user",
"test-layered-workspace-key",
"test-original-val",
],
);

let stdout = test_env.jj_cmd_success(&repo_path, &["config", "list", "--user"]);
insta::assert_snapshot!(stdout, @r###"
test-key = "test-val"
test-layered-key = "test-original-val"
test-layered-workspace-key = "test-original-val"
"###);

// Repo
Expand All @@ -221,6 +233,18 @@ fn test_config_list_layer() {
],
);

// Workspace
test_env.jj_cmd_ok(
&repo_path,
&[
"config",
"set",
"--workspace",
"test-layered-workspace-key",
"test-layered-val",
],
);

let stdout = test_env.jj_cmd_success(&repo_path, &["config", "list", "--user"]);
insta::assert_snapshot!(stdout, @r###"
test-key = "test-val"
Expand All @@ -230,6 +254,11 @@ fn test_config_list_layer() {
insta::assert_snapshot!(stdout, @r###"
test-layered-key = "test-layered-val"
"###);

let stdout = test_env.jj_cmd_success(&repo_path, &["config", "list", "--workspace"]);
insta::assert_snapshot!(stdout, @r###"
test-layered-workspace-key = "test-layered-val"
"###);
}

#[test]
Expand Down Expand Up @@ -408,16 +437,26 @@ fn test_config_layer_workspace() {
// Repo
std::fs::write(
main_path.join(".jj/repo/config.toml"),
format!("{config_key} = {value:?}\n", value = "main-repo"),
format!(
"ui.merge-editor = \"foo\"\n{config_key} = {value:?}\n",
value = "main-repo"
),
)
.unwrap();
// Secondary workspace
std::fs::write(
secondary_path.join(".jj/config.toml"),
format!("{config_key} = {value:?}\n", value = "main-workspace"),
)
.unwrap();

let stdout = test_env.jj_cmd_success(&main_path, &["config", "list", config_key]);
insta::assert_snapshot!(stdout, @r###"
ui.editor = "main-repo"
"###);
let stdout = test_env.jj_cmd_success(&secondary_path, &["config", "list", config_key]);
insta::assert_snapshot!(stdout, @r###"
ui.editor = "main-repo"
ui.editor = "main-workspace"
"###);
}

Expand All @@ -427,11 +466,11 @@ fn test_config_set_bad_opts() {
let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "set"]);
insta::assert_snapshot!(stderr, @r###"
error: the following required arguments were not provided:
<--user|--repo>
<--user|--repo|--workspace>
<NAME>
<VALUE>

Usage: jj config set <--user|--repo> <NAME> <VALUE>
Usage: jj config set <--user|--repo|--workspace> <NAME> <VALUE>

For more information, try '--help'.
"###);
Expand Down Expand Up @@ -608,9 +647,9 @@ fn test_config_edit_missing_opt() {
let stderr = test_env.jj_cmd_cli_error(test_env.env_root(), &["config", "edit"]);
insta::assert_snapshot!(stderr, @r###"
error: the following required arguments were not provided:
<--user|--repo>
<--user|--repo|--workspace>

Usage: jj config edit <--user|--repo>
Usage: jj config edit <--user|--repo|--workspace>

For more information, try '--help'.
"###);
Expand Down
4 changes: 4 additions & 0 deletions docs/config.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ config path --user`.
- The repo settings. These can be edited with `jj config edit --repo` and are
located in `.jj/repo/config.toml`.

- The workspace settings. These can be edited with `jj config edit --workspace`
and are located in `.jj/config.toml` in the workspace root. The first working
directory created in a fresh `jj` repo is itself a workspace.

- Settings [specified in the command-line](#specifying-config-on-the-command-line).

These are listed in the order they are loaded; the settings from earlier items
Expand Down