cli: Add `--show-origin` option to `config list` #3023

wfchandler · 2024-02-11T05:36:48Z

Add a new --show-origin option to jj config list to display the
source type and file path (when present) of config values.

Related to #1047

Checklist

If applicable:

I have updated CHANGELOG.md
I have updated the documentation (README.md, docs/, demos/)
~~I have updated the config schema (cli/src/config-schema.json)~~ Not applicable
I have added tests to cover my changes

google-cla · 2024-02-11T05:36:52Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

jonathantanmy · 2024-02-12T02:44:00Z

CHANGELOG.md

@@ -23,6 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `jj git fetch` now accepts `-b` as a shorthand for `--branch`, making it more
  consistent with other commands that accept a branch

+* `jj config list` gained a `--show-sources` flag to display the origin of


Thanks, this looks like a useful feature. The equivalent option in Git is called --show-origin, and here and in other places, you also describe the information displayed as an "origin", so it might be better to use that term. (You can hold off making that change until other people have chimed in on the name.)

I should have referenced #1047 in the PR, will add that now. That has a task to add --show-sources, but I'm happy to switch to --show-origin if preferred.

Mercurial calls it hg config --source. I don't have much opinion about the name. I'm inclined to copy Git's name for familiarity if no one has any arguments against that name.

Updated flag name to --show-origin.

jonathantanmy · 2024-02-12T02:46:07Z

cli/src/config.rs

    User,
    Repo,
    CommandArg,
 }

+impl fmt::Display for ConfigSource {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {


I would use a custom name for this function (instead of implementing a trait), since we may not always want to display a config source in the same way (for example, we could represent each source type by a single letter). But I'll leave it to people more experienced in the jj project than I to decide.

Hmm, I see the benefit keeping our options open in regards to formatting. Searching around in cli/src I generally see fmt::Display being used, e.g. RemoteBranchName, though there are exceptions like CommitOrChangeId.

Let's see what others say.

Other than Error types, I think fmt::Display is used where there's one reasonable string representation.

For ConfigSource enum, I'm okay with either way. I personally would avoid implementing fmt::Display because the output (like "command line") seems more verbose than I would expect from fmt::Display of enums, though.

I've added a description method in lieu of the fmt::Display implementation.

wfchandler · 2024-02-12T04:55:29Z

I should mention that on Windows fs::canonicalize() returns the somewhat unusual UNC format:

usercfg:\\?\C:\Users\myuser\AppData\Roaming\jj\config.toml       ui.color="auto"

The dunce crate could be used to convert to the more commonly used simple format that users will be more familiar with.

martinvonz · 2024-02-12T04:52:52Z

CHANGELOG.md

@@ -23,6 +23,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 * `jj git fetch` now accepts `-b` as a shorthand for `--branch`, making it more
  consistent with other commands that accept a branch

+* `jj config list` gained a `--show-sources` flag to display the origin of


Mercurial calls it hg config --source. I don't have much opinion about the name. I'm inclined to copy Git's name for familiarity if no one has any arguments against that name.

martinvonz · 2024-02-12T04:59:44Z

cli/tests/test_config_command.rs

+    usercfg:$TEST_ENV/config/config0002.toml	user-key-1="user-val-1"
+    usercfg:$TEST_ENV/config/config0003.toml	user-key-2="user-val-2"
+    repocfg:$TEST_ENV/repo/.jj/repo/config.toml	repo-key="repo-val"


It seems more consistent and easier to parse if the colon always comes after the full source, so I would prefer repocfg $TEST_ENV/repo/.jj/repo/config.toml: repo-key="repo-val".

Mercurial doesn't show any prefix at all. Git seems to show a file: prefix for all configs that come from a file without distinguishing between user config, repo config, etc. I wonder why they did it that way. Can you think of a reason?

Not sure if the colon placement will make it easier to parse - there is already a tab there that parsers can split on.

In Git, looking at the commit that introduced this feature (70bd879), I don't think there was an explicit decision to unify all the files - it was just about exposing what the parsing mechanism already knew. Support for --show-scope (to differentiate between the different types of files) was added later.

Not sure if the colon placement will make it easier to parse - there is already a tab there that parsers can split on.

True. I think it still helps humans parse it if the colon consistently comes after the source/origin.

I've switched to the proposed syntax. To my eye it's a hard to tell where the path ends and the value starts on longer lines.

env: ui.pager="/usr/bin/less" usercfg /home/user/.config/jj/config.toml: ui.editor="nvim" usercfg /home/user/.config/jj/config.toml: user.email="[email protected]" usercfg /home/user/.config/jj/config.toml: user.name="Will Chandler" repocfg /home/user/devel/jj/.jj/repo/config.toml: colors.commit_id prefix.bold=true repocfg /home/user/devel/jj/.jj/repo/config.toml: revset-aliases.immutable_heads()="main@origin | gh-pages@origin" cmdline: ui.paginate="never"

Also changed command line to cmdline for easier splitting.

To my eye it's a hard to tell where the path ends and the value starts on longer lines.

Oh, we should use the formatter to label the different parts of the output so it can be styled differently, but that's out of scope for this PR.

Makes sense 👍

yuja · 2024-02-12T05:42:27Z

cli/tests/test_config_command.rs

+    For help, see https://github.com/martinvonz/jj/blob/main/docs/config.md.
+    "###);
+    #[cfg(windows)]
+    insta::assert_snapshot!(stdout, @r###"


better to normalize the output by .replace() or insta::with_settings!({filters => ..}). It would be annoying if we had to run tests on both platforms to update the snapshot.

Good call, I added a filter.

cli/src/commands/config.rs

yuja · 2024-02-12T05:55:07Z

cli/src/config.rs

    User,
    Repo,
    CommandArg,
 }

+impl fmt::Display for ConfigSource {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {


Other than Error types, I think fmt::Display is used where there's one reasonable string representation.

For ConfigSource enum, I'm okay with either way. I personally would avoid implementing fmt::Display because the output (like "command line") seems more verbose than I would expect from fmt::Display of enums, though.

Update `config` to the latest minor version. The latest version of `config` adds more detailed error information and platform-specific file paths that require updates to snapshot tests. More significantly, environment config variables are no longer case-sensitive[1]. Internally they are down-cased, so we must adjust `test_command_args` to set lowercase environment variables. In addition, the newly added `config::Value::origin()` method exposes the path of config files. [1] rust-cli/config-rs#354

Add a new `--show-origin` option to `jj config list` to display the source type and file path (when present) of config values. In cases where the user config path is a directory, the individual file containing a given value will be displayed. The method that captures config file paths, `config::FileSourceFile::resolve()`, converts them to relative paths. These may be difficult to recognize when the current working directory is distant from a config file's location. We therefore attempt to canonicalize all file paths. Should this fail, we use the path as-is.

yuja · 2024-02-13T13:28:13Z

cli/src/config.rs

-                    "KEY1".to_string() => "value1".to_string(),
-                    "KEY2".to_string() => "value2".to_string(),
+                    "key1".to_string() => "value1".to_string(),
+                    "key2".to_string() => "value2".to_string(),


environment config variables are no longer case-sensitive

Oops, it probably means the structured command config no longer works. Environment variables are case sensitive on Unix, and we do use the TOML table syntax to override them.
[EDIT] Alias definitions, merge tools, etc. will also get broken.

Maybe it's time to roll our own config module? I think it will help implement #616 and/or handle colors/revset-aliases overrides better.

Sorry for the late reply. I didn't notice this yesterday.

Hmm, I think my summary of what changed in config v0.14.0 is not quite right. Prior to v0.14.0 environment variable keys were already being downcased, see rust-cli/config-rs#340, but the internal keys tracked by config were not. For example, an internal PAGER key would not be set by environment variable PAGER because the latter had been converted to pager.

The change in rust-cli/config-rs#354 was to downcase the internal keys as well. Looking at the config schema I don't see any mixed case settings, do you have an example of what you would expect to break so I can be sure I understand?

Prior to v0.14.0 environment variable keys were already being downcased

I think you're talking about the config loaded from environment variables, but this CommandNameAndArgs stuff is data configured in a config file, and passed to the external process. So the case matters.

do you have an example of what you would expect to break

In addition to this, [aliases], [revset-aliases], [template-aliases], [merge-tools.<NAME>] would break if keys are downcased. They are free-form dictionary, and users may define revset-aliases.HEAD = "@-" for example.

@wfchandler: To clarify, I think Yuya is talking about

jj/cli/src/config/misc.toml

Line 9 in 2652809

pager = { command = ["less", "-FRX"], env = { LESSCHARSET = "utf-8" } }

(the user can have their own similar config). (Correct me if I'm wrong, @yuja.)

Perhaps, this is the upstream issue. It doesn't describe the exact problem we have, but the root cause would be the same.
rust-cli/config-rs#531

Thanks for the explanation, I see what you mean. I'll go ahead and close out this PR given the problem with config v0.14.0.

Thank you @yuja @martinvonz and @jonathantanmy for the thorough review, much better to have caught this issue now.

I hope the problem will be addressed at config-rs, and this PR can be revisited later. Thanks anyways.

Yes, thanks for trying, and sorry about the trouble. And thanks to Yuya for noticing the problem!

jonathantanmy reviewed Feb 12, 2024

View reviewed changes

wfchandler force-pushed the wc/config-origin branch from 2c29cdc to 8e1b7ed Compare February 12, 2024 03:24

martinvonz reviewed Feb 12, 2024

View reviewed changes

yuja reviewed Feb 12, 2024

View reviewed changes

wfchandler force-pushed the wc/config-origin branch 2 times, most recently from 774953d to 96afa7e Compare February 13, 2024 04:49

wfchandler changed the title ~~cli: Add --show-sources option to config list~~ cli: Add --show-origin option to config list Feb 13, 2024

wfchandler force-pushed the wc/config-origin branch from 96afa7e to a7d637f Compare February 13, 2024 04:51

Will Chandler added 2 commits February 12, 2024 23:58

wfchandler force-pushed the wc/config-origin branch from a7d637f to 90bb450 Compare February 13, 2024 04:58

yuja requested changes Feb 13, 2024

View reviewed changes

wfchandler requested a review from yuja February 14, 2024 04:47

wfchandler closed this Feb 15, 2024

yuja mentioned this pull request Mar 29, 2024

invalid config makes it hard to fix the config #3317

Closed

primeos-work mentioned this pull request Nov 15, 2024

bug: config 0.14.0 only supports lowercase fields (regression since 0.13.4) rust-cli/config-rs#531

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli: Add `--show-origin` option to `config list` #3023

cli: Add `--show-origin` option to `config list` #3023

wfchandler commented Feb 11, 2024 •

edited

Loading

google-cla bot commented Feb 11, 2024

jonathantanmy Feb 12, 2024

wfchandler Feb 12, 2024

martinvonz Feb 12, 2024

wfchandler Feb 13, 2024

jonathantanmy Feb 12, 2024

wfchandler Feb 12, 2024

yuja Feb 12, 2024

wfchandler Feb 13, 2024

wfchandler commented Feb 12, 2024

martinvonz Feb 12, 2024

martinvonz Feb 12, 2024

jonathantanmy Feb 12, 2024

martinvonz Feb 12, 2024

wfchandler Feb 13, 2024

martinvonz Feb 13, 2024

wfchandler Feb 14, 2024

yuja Feb 12, 2024

wfchandler Feb 13, 2024

yuja Feb 12, 2024

yuja Feb 13, 2024 •

edited

Loading

wfchandler Feb 14, 2024

yuja Feb 14, 2024

martinvonz Feb 14, 2024

yuja Feb 14, 2024

wfchandler Feb 15, 2024

yuja Feb 15, 2024

martinvonz Feb 15, 2024

cli: Add --show-origin option to config list #3023

cli: Add --show-origin option to config list #3023

Conversation

wfchandler commented Feb 11, 2024 • edited Loading

Checklist

google-cla bot commented Feb 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wfchandler commented Feb 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuja Feb 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cli: Add `--show-origin` option to `config list` #3023

cli: Add `--show-origin` option to `config list` #3023

wfchandler commented Feb 11, 2024 •

edited

Loading

yuja Feb 13, 2024 •

edited

Loading