martinvonz / jj

A Git-compatible VCS that is both simple and powerful
https://martinvonz.github.io/jj/
Apache License 2.0
8.97k stars 312 forks source link

jjconfig.toml should be opened recursively from the current dir #616

Open berkus opened 2 years ago

berkus commented 2 years ago

Description

The email/user name configuration does not have to be global, there are several repos where I commit from different email addresses.

It would make sense to traverse directories up looking for a config file, before looking in the ~/.config/ etc.

Steps to Reproduce the Problem

  1. Try to configure a different user/email combination for a single repository, different from the globally configured default.
martinvonz commented 2 years ago

I've meant to add support for repo-level and workspace-level configs , as well as system-level configs. I just haven't gotten around to it yet. That's what Git and Mercurial do anyway.

What you're suggesting seems a little different in that it you could create a hierarchy of projects, so you might have this:

~/
    .jjconfig.toml
    work/
        .jjconfig.toml
        work-project1/
            .jj/
                jjconfig.toml
        work-project2/
    personal/
        .jjconfig.toml
        personal-project1/
        personal-project2/

That's an interesting idea. Did you get that setup working with Git? It looks like it has support for include statements in config files.

martinvonz commented 2 years ago

Apparently the same question was just asked on the Git mailing list and on Git for Windows.

berkus commented 2 years ago

This is just similar to how git finds its .git directory and I think it makes some sense for the config file. I.e. I can have a directory for Work projects and use one email for those, but a separate directory for hobby projects and commit there using a public github email, for example.

martinvonz commented 2 years ago

Yes, understood. If we add this feature, we'll have to remember to take both performance and security into account. It would mean that we're always walking up to the root directory, which is probably fine. It would also mean that we trust users who have write access to parent directories, which seems fine to me, but note that the Git-for-Windows devs considered that a security vulnerability.

berkus commented 2 years ago

note that the Git-for-Windows devs considered that a security vulnerability.

Seems like a valid concern on systems where random users can write anywhere. Probably windows build should disallow searching in the root directory (and if you think a bit, probably on other systems too - i do not see a useful case where .git should be created in the root directory at all, unless you try to version your entire files tree across all mounted filesystems).

martinvonz commented 2 years ago

To clarify, the reason I thought it doesn't seem bad to trust files in parent directories is that I thought permissions are almost always set up so people who can write to the root directory can probably write to your system configs anyway. But maybe it's not unusual for users to have write access to C:\ on Windows but still not have write access to C:\Programs\jj\config (or wherever we'd read system-wide configs from - I haven't used Windows in 11 years).

dbarnett commented 1 year ago

FWIW, I was wanting the same (config for different email addresses per repo), but am totally fine with a non-recursive solution just like in git, like /.jj/config.toml. Could that part be implemented first?

martinvonz commented 1 year ago

Yes, I think it's very likely that we'll add support for per-repo configs first. I'm not sure if I'll have time to prioritize it myself soon, but that depends on what we'll end up needing internally (because then I get to work on it during the week).

dbarnett commented 1 year ago

Had a look through some of the code. There's a bit of a snag with the sequence of operations, that currently UserSettings and Config is constructed first in the code and then passed to code that finds the workspace root, but to load config from the repo you kinda need to do the reverse and resolve the workspace root before fully resolving the config.

I think instead the sequence of events needs to be:

  1. first resolve CWD
  2. then resolve the workspace root
  3. then check for any repo-local config
  4. then construct the full config from all config sources (with precedence: env vars, local repo config, system config)
  5. then finish loading UserSettings and Ui, taking the paths and configs as input
martinvonz commented 1 year ago

Thanks! I think what you propose makes sense. I think that was also my conclusion when I looked into it a bit a while ago.

For step 2, we'll need to do some early parsing of the command line arguments, so we can resolve the -R argument and read configs from there (addressing this TODO).

Git had a recent security bug that they fixed (IIRC) by adding a config that's read before step 2 above (the config controls how the workspace root is resolved). I don't think we need to support early reading like that yet, but it might be good to keep in mind.

We may also want to insert another level of configs later, namely for workspaces. It seems like it wouldn't be much that you'd want to configure per workspace, but it might be good to keep that, too, in mind. In particular, I think the repo-level config should live in .jj/repo/ (e.g. .jj/repo/config.toml).

By the way, I'm not sure that the Ui object is the right place to store the configs. We currently store a copy of the UserSettings object in WorkspaceCommandHelper. It seems obviously bad to store it in both places.

The difference between UserSettings and RepoSettings is very subtle and I keep forgetting why I did it that way. I think it had something to do with cases like where you fetch from one local repo into another. You would probably not want the current repo's configs to affect the other repo in that case. If I'm right about that, which I'm not sure I am, we should somehow keep the repo-level configs out of the UserSettings object until we've created the RepoSettings object. However, that sounds quite annoying to do, and I don't want that to get in the way of getting a useful feature implemented. We should also check how Git and Mercurial deal with that. I suspect they both let the current repo affect the other repo.

@yuja (who's also spent a lot of time on VCS development), do you have any thoughts on any of the above?

yuja commented 1 year ago

I think some early config object is needed at step 2 to resolve command aliases. jj -R <path> could be parsed without aliases, but jj <cmd> [cmdargs].. -R <path> couldn't, and sneaking -R from env::args() would be horrible.

I don't think aliases (and some other global options) need to be set in .jj/repo/config.toml, so the early config can be loaded without the workspace root.

By the way, I'm not sure that the Ui object is the right place to store the configs.

IMO, Ui should be just a consumer of UserSettings. It's annoying that we can't ui.write() while ui.settings() is borrowed, for example.

We should also check how Git and Mercurial deal with that. I suspect they both let the current repo affect the other repo.

Mercurial stores baseui to isolate repo-specific configs, but I don't know if that's needed for jj at this point. We don't have native push/pull things yet.

https://www.mercurial-scm.org/repo/hg/file/6.3.1/mercurial/hg.py#l1489

martinvonz commented 1 year ago

IMO, Ui should be just a consumer of UserSettings. It's annoying that we can't ui.write() while ui.settings() is borrowed, for example.

That's a good argument for removing it from there. I guess we'll have the same problem if the canonical place is in WorkspaceCommandHelper. Maybe we just need to pass it around separately.

Mercurial stores baseui to isolate repo-specific configs,

Interesting, that code doesn't even look familiar to me (but I am very forgetful). The config has good support for layering configs, so we could probably take advantage of that if we wanted to support it later.

but I don't know if that's needed for jj at this point. We don't have native push/pull things yet.

Yes, no need for it yet, just trying to keep it mind.

jennings commented 1 year ago

Although .jj/repo/config.toml exists now, I would still find it helpful to automatically use a different config based on where the repository is on disk.

For the recursive solution, perhaps jj could do what EditorConfig does to mitigate the security vulnerability: Recursion stops when an .editorconfig is found that contains root = true.

I would also be happy with the non-recursive Git-style solution, because that's what I do in Git:

# ~/.gitconfig
[includeIf "gitdir/i:C:/src/personal/**"]
    path = ~/.gitconfig.personal
# ~/.gitconfig.personal
[user]
    email = jennings@example.com

In TOML, maybe that could use an array of tables?

user.email = "default@example.com"

# read ~/.jjconfig.personal.toml for any repo under C:\src\personal\
[[include]]
path-regex = "^c:/src/personal/"
file       = "~/.jjconfig.personal.toml"

# read ~/.jjconfig.work.toml for any repo under C:\src\work\
[[include]]
path-regex = "^c:/src/work/"
file       = "~/.jjconfig.work.toml"

Or, since this project seems to prefer dotted notation, it could be a table where the name is ignored:

include.personal.path-regex = "^c:/src/personal/"
include.personal.file       = "~/.jjconfig.personal.toml"
include.work.path-regex = "^c:/src/work/"
include.work.file       = "~/.jjconfig.work.toml"

Either syntax could allow for adding different criteria in the future, like how git has gitdir, onbranch:<name>, and hasconfig:remote.*.url:

# always include ~/.jjconfig.local.toml if it exists
[[include]]
file = "~/.jjconfig.local.toml"

# include if the repo uses a git backing store
[[include]]
backing-store = "git"
file          = "~/.jjconfig.git.toml"
sunshowers commented 11 months ago

I like something like the git includeIf approach.

yuja commented 8 months ago

Just dropping an idea. In addition to includeIf-like stuff, inline syntax would also be useful:

[[scoped]]
scoped.path = "~/work"
user.email = "my-work-email@example.com"

[[scoped]]
scoped.path = "~/oss"
user.email = "my-private-email@example.com"
sean-c-johnston commented 4 months ago

This is one of the first things that I looked for when jj prompted me to set up my user name and email - with git I have the same sort of setup as described here in my ~/work and ~/personal dirs.

Per-repo config is a solution in the meantime but I'm an idiot and will often forget to set it up in newly cloned or created repos.

kfkonrad commented 3 months ago

I like something like the git includeIf approach.

fwiw I've implemented a generic tool called relconf (GitHub repo, crate on crates.io) that generates config based on the current $PWD similar to what git does with includeIf. It also supports uncoditional (always) importing like git does with include.

The main caveat there is that relconf needs a separate config file and can only merge entire files, not insert and allow overrides at a later point in the same file like include and includeif do. I.e. the following is not possible with relconf:

Personally I don't like overriding my config like that anyway so it doesn't bother me. I realize it's just another workaround but for me it does the trick until jj eventually adds native support

Here's the relconf config I use right now, the rest in documented in the relconf README:

tools:
- name: jj
  format: toml
  rootconfig: ~/.jjconfig.toml
  inject:
    - env-name: JJ_CONFIG
      path: ~/.config/jj/merged.toml
  subconfigs:
    - path: ~/.config/jj/always.toml
    - path: ~/.config/jj/customer.toml
      when:
        - directory: ~/workspace/customer-gitlab
          match-subdirectories: true