config: also use XDG_CONFIG_HOME on MacOS #3466

ckoehler · 2024-04-06T05:14:40Z

This replaces the dirs crate with the more lightweight and flexible etcetera crate, which also lets us use XDG config on MacOS.

Fixes #3434.

~~I'm not entirely sure how do prevent the line break in the doc table between "Application" and "Support" 😅 ...~~

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)
I have added tests to cover my changes

docs/config.md

cli/src/config.rs

noahmayr · 2024-04-06T13:26:13Z

I'm not entirely sure how do prevent the line break in the doc table between "Application" and "Support" 😅 ...

I don't think there's a line break, it's just Github's wrapping (check the line numbers on the left, the second line doesn't have one)

cli/src/config.rs

CHANGELOG.md

ckoehler · 2024-04-12T03:20:11Z

Sooo I'm finding the current behavior of the config file handling really convoluted. As a result, I am making bigger changes than I thought I'd have to. Before I go too far (maybe it's too late already!), I'd like some feedback:

Is this okay? Too much?
Do the tests describe the wanted behaviors well?

The commits themselves are kind of a mess (I want to keep more history with so many changes), so don't look at those yet. I'll clean them up before I merge anything.

Thanks!

ilyagr · 2024-04-12T03:38:31Z

Before I go too far (maybe it's too late already!), I'd like some feedback:

Is this okay? Too much?

Do the tests describe the wanted behaviors well?

The commits themselves are kind of a mess

I'm a little confused -- what are the changes (especially if the commits are too raw to be looked at)?

Didn't we decide that we should allow all three locations? At least the commit descriptions seem to suggest the PR still forbids the Application Support location.

hypatian · 2024-04-12T03:54:57Z

I'm very confused here—from reading through the changes, it looks to me like despite the documentation and the commit message claiming that on macOS it will use XDG_CONFIG_DIR by default, it will actually use the system location by default. And, of course, it gives errors on multiple locations not on any specific location. (Edit: to be clear: it will read from the system location by default. Newly created files go to ~/.jjconfig.toml, it looks like.)

Has the documentation (including commit message) not been updated to match the code changes?

ckoehler · 2024-04-12T04:12:44Z

Didn't we decide that we should allow all three locations? At least the commit descriptions seem to suggest the PR still forbids the Application Support location.

Yes, all three are allowed. I’ll clean it up later. Sounds like the most helpful thing would be to describe the behavior this introduces.

I'm very confused here—from reading through the changes, it looks to me like despite the documentation and the commit message claiming that on macOS it will use XDG_CONFIG_DIR by default, it will actually use the system location by default. And, of course, it gives errors on multiple locations not on any specific location. (Edit: to be clear: it will read from the system location by default. Newly created files go to ~/.jjconfig.toml, it looks like.)

Yes, if there is no configuration yet and jj has to create it, it goes into $HOME. If more than one exists, it shows an error. If only one exists, it uses that. For Mac, that can be Application Support, $XDG_CONFIG_HOME, or $HOME/.jjconfig.toml.

Has the documentation (including commit message) not been updated to match the code changes?

Not yet. I’ll do that.

gulbanana · 2024-04-12T08:49:56Z

I don't think it's desirable to put the new config in $HOME instead of the OS-specific location by default, basically for the reasons here #3466 (comment). The problem with the existing config implementation was that it didn't support XDG_CONFIG_HOME; adding that support is good, but we shouldn't break the use of AppData/Application Support for people who aren't using XDG. Which will be most people.

ckoehler · 2024-04-12T13:16:24Z

I don't think it's desirable to put the new config in $HOME instead of the OS-specific location by default, basically for the reasons here #3466 (comment). The problem with the existing config implementation was that it didn't support XDG_CONFIG_HOME; adding that support is good, but we shouldn't break the use of AppData/Application Support for people who aren't using XDG. Which will be most people.

I based that on a comment by ~~Martin~~ @ilyagr on Discord (maybe I misunderstood), but it’s easy enough to change.

ckoehler · 2024-04-15T04:13:02Z

I updated all the docs and changelog and cleaned up the commit. The behavior of some of the config commands creating config files if none exist is really separate from this, so it can be its own PR.

martinvonz

I find this hard to review. It feels like it does many things at once. Is it possible to split it up something like the following?

change from dirs to etcetera
refactor tests
refactor config_path()
add support for XDG_CONFIG_HOME

ckoehler · 2024-04-17T17:41:00Z

I find this hard to review. It feels like it does many things at once. Is it possible to split it up something like the following?
* change from `dirs` to `etcetera`

* refactor tests

* refactor `config_path()`

* add support for XDG_CONFIG_HOME

Sure! Those individual commits just won't work by themselves. I interpreted atomic as "need to build and work independently".

martinvonz · 2024-04-17T17:42:10Z

I interpreted atomic as "need to build and work independently".

Yes, that's correct.

Those individual commits just won't work by themselves.

Why not?

ckoehler · 2024-04-17T17:59:00Z

Those individual commits just won't work by themselves.

Why not?

Eh maybe I spoke too soon. Let me give it a shot. Thanks for the feedback :)

ckoehler · 2024-04-18T03:45:08Z

Alright, I hope this is easier to review.

martinvonz

Thanks, it was a lot easier to review now, but I'm still quite confused about the third commit.

cli/src/config.rs

martinvonz · 2024-04-20T01:23:25Z

cli/src/config.rs

+            if handle_missing == HandleMissingConfig::Create && !path.exists() {
+                create_config_file(&path)?;
+            }


Was this required because of some other changes? Or does this represent a change in behavior?

I combined the two functions into one, with an argument to create the file when needed. This is existing behavior.

Perhaps, create_config_file() can be moved to caller? It's generally easier to test if the function had no side effect.

cli/src/config.rs

Add a cfg to make etctera use the correct config directory on MacOS (which etcetera calls the `data_dir`).

Rename the test enum and `new_config_path` function to reflect that we never have just "new": we either look for existing or create new, or just look for existing.

* rename it to make clearer what it does * it returns an existing config or None if there isn't one * if told to create missing, it does so * as a result, `ConfigPath` is no longer necessary and removed

cli/src/config.rs

yuja · 2024-04-20T04:24:15Z

cli/src/config.rs

-                Want::New(want) => (None, Some(want)),
-                Want::ExistingAndNew(want) => (Some(want), Some(want)),
+            match self.want {
+                Want::None => {}


Doesn't it remove the assertion for Want::None case?

I don't remember the original expectation of these tests, but it doesn't make sense to run a test with no assertion.

Thanks for pointing that out, you're exactly right! The Want::None test doesn't really make sense, because we need to either call the function that gives us a config path without creating it or with creating it.

What we want is a way to tell the test to run one or the other, and what to expect out of each. I'm changing it accordingly.

It looks like this was added in #1861, so maybe @qfel can clarify?

It has been a while, so I don't remember well, but..

It looks like Want::None used to declare both new_config_path and existing_config_path were expected to return None. In this PR test_config_path_home_dir_doesnt_create_new uses Want::None. What do you want to assert for that test? That both existing_config_path and new_or_existing_config_path return None, or something else?

I didn't understand the behavior too well before. In this case it should be None with existing_config_path and Some("<path>") with new_or_existing_config_path.

yuja · 2024-04-20T04:26:19Z

cli/src/config.rs

+            if handle_missing == HandleMissingConfig::Create && !path.exists() {
+                create_config_file(&path)?;
+            }


Perhaps, create_config_file() can be moved to caller? It's generally easier to test if the function had no side effect.

yuja · 2024-04-20T04:28:39Z

cli/src/config.rs

@@ -31,6 +31,8 @@ pub enum ConfigError {
    ConfigReadError(#[from] config::ConfigError),
    #[error("Both {0} and {1} exist. Please consolidate your configs in one of them.")]
    AmbiguousSource(PathBuf, PathBuf),
+    #[error("Configs found in {0}, {1}, and {2}. Please consolidate your configs in one of them.")]
+    AmbiguousSource3(PathBuf, PathBuf, PathBuf),


Perhaps, these two error variants can be combined as AmbiguousSource(Vec<PathBuf>). The error message could be formatted as something like "Configs found in multiple locations: {}", .0.iter().map(|path| path.display()).join(", ") (untested)

ckoehler · 2024-04-20T21:17:28Z

I'm going to take a break from this PR, it's been taking a lot more time than I expected. Someone else is free to take it over.

ckoehler force-pushed the push-kkwmmvvtprtr branch 5 times, most recently from 8c1371c to 1012f40 Compare April 6, 2024 05:40

noahmayr reviewed Apr 6, 2024

View reviewed changes

docs/config.md Outdated Show resolved Hide resolved

cli/src/config.rs Outdated Show resolved Hide resolved

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from 036a3cc to 85c4e91 Compare April 6, 2024 17:12

noahmayr reviewed Apr 6, 2024

View reviewed changes

cli/src/config.rs Outdated Show resolved Hide resolved

ilyagr reviewed Apr 6, 2024

View reviewed changes

cli/src/config.rs Outdated Show resolved Hide resolved

ckoehler force-pushed the push-kkwmmvvtprtr branch 5 times, most recently from 19ecfcc to 8f5a300 Compare April 7, 2024 00:54

ckoehler changed the title ~~config: use XDG_CONFIG_HOME on MacOS as a fallback~~ config: use XDG_CONFIG_HOME on MacOS by default Apr 9, 2024

chriskrycho reviewed Apr 9, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

ckoehler force-pushed the push-kkwmmvvtprtr branch from 8f5a300 to cb93ef5 Compare April 12, 2024 03:16

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from 8d58b3c to 885eb2c Compare April 12, 2024 22:53

ckoehler changed the title ~~config: use XDG_CONFIG_HOME on MacOS by default~~ config: also use XDG_CONFIG_HOME on MacOS Apr 13, 2024

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from a47ffe2 to 80bfe37 Compare April 15, 2024 01:43

ckoehler force-pushed the push-kkwmmvvtprtr branch 3 times, most recently from ca05b21 to 8c57c05 Compare April 15, 2024 02:11

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from 2af6237 to f748470 Compare April 17, 2024 13:39

martinvonz requested changes Apr 17, 2024

View reviewed changes

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from 1f2b361 to 8d7a13b Compare April 18, 2024 03:42

martinvonz reviewed Apr 18, 2024

View reviewed changes

cli/src/config.rs Show resolved Hide resolved

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from d6d6dae to 354cceb Compare April 19, 2024 02:30

martinvonz reviewed Apr 19, 2024

View reviewed changes

cli/src/config.rs Show resolved Hide resolved

cli/src/config.rs Outdated Show resolved Hide resolved

cli/src/config.rs Show resolved Hide resolved

cli/src/config.rs Outdated Show resolved Hide resolved

ckoehler force-pushed the push-kkwmmvvtprtr branch 2 times, most recently from 62c3e72 to 7e2dac9 Compare April 19, 2024 18:39

martinvonz reviewed Apr 20, 2024

View reviewed changes

ckoehler added 4 commits April 19, 2024 20:11

config: switch from dirs to etcetera

86cbfac

Add a cfg to make etctera use the correct config directory on MacOS (which etcetera calls the `data_dir`).

config: refactor tests to reflect our desired behavior

ca1ab15

Rename the test enum and `new_config_path` function to reflect that we never have just "new": we either look for existing or create new, or just look for existing.

config: refactor config_path()

8107d8c

* rename it to make clearer what it does * it returns an existing config or None if there isn't one * if told to create missing, it does so * as a result, `ConfigPath` is no longer necessary and removed

config: add XDG_CONFIG_HOME config location for MacOS

6e79eb8

ckoehler force-pushed the push-kkwmmvvtprtr branch from 7e2dac9 to 6e79eb8 Compare April 20, 2024 02:12

yuja reviewed Apr 20, 2024

View reviewed changes

bnjmnt4n mentioned this pull request May 18, 2024

jujutsu: update module to respect XDG config placement nix-community/home-manager#5207

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: also use XDG_CONFIG_HOME on MacOS #3466

config: also use XDG_CONFIG_HOME on MacOS #3466

ckoehler commented Apr 6, 2024 •

edited

Loading

noahmayr commented Apr 6, 2024

ckoehler commented Apr 12, 2024

ilyagr commented Apr 12, 2024 •

edited

Loading

hypatian commented Apr 12, 2024 •

edited

Loading

ckoehler commented Apr 12, 2024

gulbanana commented Apr 12, 2024

ckoehler commented Apr 12, 2024 •

edited

Loading

ckoehler commented Apr 15, 2024

martinvonz left a comment

ckoehler commented Apr 17, 2024

martinvonz commented Apr 17, 2024

ckoehler commented Apr 17, 2024

ckoehler commented Apr 18, 2024

martinvonz left a comment

martinvonz Apr 20, 2024

ckoehler Apr 20, 2024

yuja Apr 20, 2024

yuja Apr 20, 2024

ckoehler Apr 20, 2024

martinvonz Apr 20, 2024

qfel Apr 24, 2024

ckoehler Apr 24, 2024

yuja Apr 20, 2024

yuja Apr 20, 2024

ckoehler commented Apr 20, 2024

config: also use XDG_CONFIG_HOME on MacOS #3466

Are you sure you want to change the base?

config: also use XDG_CONFIG_HOME on MacOS #3466

Conversation

ckoehler commented Apr 6, 2024 • edited Loading

Checklist

noahmayr commented Apr 6, 2024

ckoehler commented Apr 12, 2024

ilyagr commented Apr 12, 2024 • edited Loading

hypatian commented Apr 12, 2024 • edited Loading

ckoehler commented Apr 12, 2024

gulbanana commented Apr 12, 2024

ckoehler commented Apr 12, 2024 • edited Loading

ckoehler commented Apr 15, 2024

martinvonz left a comment

Choose a reason for hiding this comment

ckoehler commented Apr 17, 2024

martinvonz commented Apr 17, 2024

ckoehler commented Apr 17, 2024

ckoehler commented Apr 18, 2024

martinvonz left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ckoehler commented Apr 20, 2024

ckoehler commented Apr 6, 2024 •

edited

Loading

ilyagr commented Apr 12, 2024 •

edited

Loading

hypatian commented Apr 12, 2024 •

edited

Loading

ckoehler commented Apr 12, 2024 •

edited

Loading