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.

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

cli: load configs from .jj/repo/config.toml #982

Merged
merged 9 commits into from
Jan 7, 2023
Merged

cli: load configs from .jj/repo/config.toml #982

merged 9 commits into from
Jan 7, 2023

Conversation

yuja
Copy link
Contributor

@yuja yuja commented Jan 6, 2023

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

Copy link
Member

@martinvonz martinvonz left a comment

Choose a reason for hiding this comment

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

Thank you! Very useful feature, and very nicely done.

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
yuja added 9 commits January 7, 2023 11:12
UserSettings will be instantiated after both user and repo configs are
loaded. We might want to add a wrapper for CLI settings, but I have no idea
how that should be structured. Let's use bare config::Config until then.
…nfig

For the same reason as the previous commit. -R/--repository can't be parsed
until the real command name gets available.
The next commit will add a struct that keeps umerged configs, which will be
used to insert repo config between user and override configs.
Thinking of config precedence, we'll probably need to store "base" (default +
env_base + user) config and "override" (env_overrides + --config-toml) config
separately to support repo config. Suppose env_overrides() is a temporary
value, it's better to override any file-derived values with $JJ_ env.
Since CliRunner knows both defaults and envs, it makes sense to feed them
to initialize ui.
It's unclear whether parse_args() or its caller should update LayeredConfigs.
--config-toml is processed by callee to apply --color early. -R/--repository
will be processed by caller since it will instantiate WorkspaceLoader.
Maybe --config-toml can be removed from EarlyArgs, and handle_early_args()
just updates ui state based on --color argument?
Since per-repo config may contain CLI settings, it must be visible to CLI.
Therefore, UserSettings::with_repo() -> RepoSettings isn't used, and its
implementation is nullified by this commit.

#616
@yuja yuja merged commit ea96ea3 into jj-vcs:main Jan 7, 2023
@yuja yuja deleted the push-37da19a747e54949a19f8e8c15da57f4 branch January 7, 2023 02:33
@ppwwyyxx
Copy link

Could you explain how precedence is expected to work? I'm running jj at its github HEAD, and I set a per-repo alias inside .jj/repo/config.toml to overwrite the global one, but it ignores my repo config and still uses the one I had in ~/.config/jj/config.toml

@martinvonz
Copy link
Member

Could you explain how precedence is expected to work? I'm running jj at its github HEAD, and I set a per-repo alias inside .jj/repo/config.toml to overwrite the global one, but it ignores my repo config and still uses the one I had in ~/.config/jj/config.toml

Example (let us know if that doesn't work for you):

$ jj config list aliases.foo
aliases.foo=["st"]

$ cd $(mktemp -d); jj init --git
Initialized repo in "."

$ echo 'aliases.foo = ["diff"]' > .jj/repo/config.toml

$ jj config list aliases.foo
aliases.foo=["diff"]

@ppwwyyxx
Copy link

This is what I got:

❯ 14:18 cd $(mktemp -d) ; jj init --git
Initialized repo in "."
❯ 14:18 echo 'aliases.foo = ["diff"]' > .jj/repo/config.toml
❯ 14:18 jj config list aliases.foo
❯ 14:18 jj config --no-pager list aliases.foo
aliases.foo=["diff"]
❯ 14:18 jj foo
error: unrecognized subcommand 'foo'

Usage: jj [OPTIONS] [COMMAND]

For more information, try '--help'.
❯ 14:19 jj --version
jj 0.10.0-8dbe12da2a6d771d3409b19b76bf0f8781e6b218

@martinvonz
Copy link
Member

This is what I got:

This PR has been closed for a while, so I moved this to a new bug report instead: #2414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants