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

Jump to bottom

Add workspace config #3670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Add workspace config #3670

wants to merge 1 commit into from

Conversation

HybridEidolon
Copy link
Contributor

This adds support for per-workspace configuration as was described as a TODO in the LayeredConfig struct. Like repo configuration in .jj/repo/config.toml, the workspace config goes in .jj/config.toml in the workspace root, and its settings take precedence over repo, user and default configs.

The config subcommands that take --user and --repo are additionally given --workspace which behaves as one would expect.

Checklist

If applicable:

  • I have updated CHANGELOG.md
  • I have updated the documentation (README.md, docs/, demos/)
  • I have added tests to cover my changes

PhilipMetzger
PhilipMetzger reviewed May 12, 2024
View reviewed changes
Copy link
Collaborator

@PhilipMetzger PhilipMetzger left a comment

Choose a reason for hiding this comment

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

You missed to cargo +nightly fmt the code and a additional test for the workspace level feature. If you fix that LGTM.

yuja
yuja reviewed May 12, 2024
View reviewed changes
cli/src/commands/config.rs Outdated Show resolved Hide resolved
cli/tests/test_config_command.rs Show resolved Hide resolved
@HybridEidolon HybridEidolon force-pushed the workspace-config branch 3 times, most recently from 9737caa to 01b8970 Compare May 12, 2024 18:26
ilyagr
ilyagr reviewed May 12, 2024
View reviewed changes
cli/src/config.rs
@@ -59,7 +60,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
Collaborator

@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
Collaborator

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
Collaborator

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
Collaborator

@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
Collaborator

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
Collaborator

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
Collaborator

@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
Collaborator

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
Collaborator

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.

yuja
yuja approved these changes May 13, 2024
View reviewed changes
cli/tests/test_config_command.rs Outdated Show resolved Hide resolved
cli/tests/test_config_command.rs Show resolved Hide resolved
cli/src/config.rs
@@ -59,7 +60,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
Collaborator

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.

@ilyagr
Copy link
Collaborator

ilyagr commented May 13, 2024

Just FYI, you had a flaky test failure, I re-ran the testing job for you. You should be able to re-run it as well, I think, in the Actions tab.

@PhilipMetzger
Copy link
Collaborator

@HybridEidolon Any intent to land this soon? It probably just needs a rebase and solving the conflict in CHANGELOG.md.

@HybridEidolon
Copy link
Contributor Author

Yep, I can look at this again after I return from my trip next week.

@HybridEidolon HybridEidolon force-pushed the workspace-config branch 2 times, most recently from e0c65f5 to 9bcbe94 Compare July 10, 2024 18:10
@HybridEidolon
Copy link
Contributor Author

Rebased on upstream with conflicts resolved

@HybridEidolon
Add workspace config
0f7ff5b 
This adds support for per-workspace configuration as was
described as a TODO in the LayeredConfig struct. Like repo
configuration in `.jj/repo/config.toml`, the workspace config
goes in `.jj/config.toml` in the workspace root, and its
settings take precedence over repo, user and default configs.

The config subcommands that take `--user` and `--repo` are
additionally given `--workspace` which behaves as one would
expect.
@ilyagr ilyagr mentioned this pull request Sep 3, 2024
Merged
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilipMetzger PhilipMetzger PhilipMetzger left review comments

@ilyagr ilyagr ilyagr left review comments

@martinvonz martinvonz martinvonz left review comments

@yuja yuja yuja approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HybridEidolon @ilyagr @PhilipMetzger @yuja @martinvonz