jesseduffield / lazygit

simple terminal UI for git commands
MIT License
48.78k stars 1.74k forks source link

Support per-repo config files #3293

Open stefanhaller opened 5 months ago

stefanhaller commented 5 months ago

For some of lazygit's config options it would be good to have different settings for different repos; some examples are:

It would also allow us to get rid of the weird git.commitPrefixes map and have a single git.commitPrefix setting.

The obvious way to support this is to allow a local config file in the git repo that overrides the global one, much like git itself does it with ~/.gitconfig vs. .git/config.

There are two possible options where we could put the file:

  1. in the .git folder (.git/lazygit.yml)
  2. in the working copy (./.lazygit.yml)

Some considerations for each:

  1. makes it per-repo, so it applies to all workspaces that are attached to the repo.
    • it feels a bit strange to put third-party things into git's private folder, but it seems to be ok, judging from the responses to this mailing list thread
    • if users throw away their working copy and re-clone, the settings are lost. But so are the ones of git itself, so I guess that's ok.
  2. makes it per-workspace, which I suppose is less desirable than per-repo in most cases
    • users can decide whether they want to version it (which makes it per-repo again...), or add it to .gitignore
    • it is similar to .vscode/settings.json
    • creates a security vulnerability unless we make sure that custom commands or editor configs can't be read from this file

If we don't want to make the decision, we could also read files from both locations and let the user decide where to put it. But then we at least need to define the precedence.

Another alternative might be to put custom lazygit configs into git's config file; this is the approach that git gui takes. Since git uses the init format, they would have to be translated to yaml from there, which I'm not sure is always possible. I find this approach less promising for us.

jwhitley commented 5 months ago

In practice, I think option 2 is preferable. Yes, ./.lazygit.yml is technically per-worktree, but in practice it’s going to mostly be a one-and-done configuration process, intended to share and standardize these settings across all clones of a repository. This is analogous to other versioned configs ala .editorconfig or .vscode/. Putting this under .git/ makes these settings throwaway-per-clone rather than a point of standardization, and I’m simply not understanding any use case for that.

If a location is needed outside the worktree, e.g. because the dev wants to make a per-repo setting but doesn’t have control of .gitignore or consensus to version .lazygit.yml, then I’d make a counter-proposal: allow the user’s .config/lazygit/config.yml to contain blocks of per-repository settings. e.g. “for all repos matching /path/to/repo/x, override global settings with these settings”. Some simple globbing in repo match strings might be helpful here. A quick sketch:

gui:
  […]
git:
  […]
repos:
  “/path/to/repo/x*”:
    git:
      commitPrefix: wombat
      signOff: true
  “/path/to/repo/y”:
    git:
      […]

That said, I’d leave that off entirely lacking a concrete user need/use case to drive it since I think just versioning repo-local .lazygit.yml is probably enough.

Beyond that, I do agree that this is overall a great feature, esp. re: how you note it relates to git.commitPrefixes.

stefanhaller commented 5 months ago

in practice it’s going to mostly be a one-and-done configuration process, intended to share and standardize these settings across all clones of a repository.

That's an assumption that may be true for some of the examples mentioned above, but maybe not for others (e.g. the git.log.order example). In general, I see most of lazygit's configs to be personal preferences, and as such they should not be versioned, similar to how .vscode/settings.json is usually git-ignored rather than versioned.

This makes me think that we should actually support both locations, so that users can have both versioned configs and personal ones. And the versioned one should probably be read first so that users can override it with their private one.

I’d make a counter-proposal: allow the user’s .config/lazygit/config.yml to contain blocks of per-repository settings.

I'd be pretty strongly against that, since paths or even repo names can change and things become stale. Git itself uses this approach for keeping track of which repos are under git maintenance, and this has actually bitten me in the past when I renamed some clones (or maybe the directory they lived in, I don't remember) and git maintenance stopped working for those without me noticing. (On the other hand, the ability to provide settings for a bunch of repos at once by using globbing does seem attractive.)

afhoffman commented 5 months ago

If a location is needed outside the worktree, e.g. because the dev wants to make a per-repo setting but doesn’t have control of .gitignore or consensus to version .lazygit.yml

Please correct me if there's a better way, but I think adding the local config to .git/info/exclude would work in that case?

This feature does sound great; I would use it a lot for commit prefixes.

What about the possibility of having a .config/lazygit/per_repo_template.yml that lazy git could copy into the active repo on command? I think my per-repo settings would have similar structure but different values from repo to repo. I hate to add things just for the sake of it, but it could be nice to be able to quickly bootstrap the config for a new clone.

jwhitley commented 5 months ago

That's an assumption that may be true for some of the examples mentioned above, but maybe not for others (e.g. the git.log.order example).

Good example, point taken.

I'd be pretty strongly against that, since paths or even repo names can change and things become stale.

Agreed. Likewise, I think my idea is moot in light of supporting a hierarchy involving both .lazygit.yml and .git/lazygit.yml. If a user needs persistence for local overrides, it's not hard to copy in a custom .git/lazygit.yml as needed. In a work environment, I'd whip up a shell function and call it a day.

jwhitley commented 5 months ago

Please correct me if there's a better way, but I think adding the local config to .git/info/exclude would work in that case?

Completely true. I even have a git helper script to expedite this use case: git-embargo. Still, there are a lot of devs for whom needing familiarity with a somewhat obscure git feature to get something done amounts to "undiscoverable" in the context of lazygit UX.

What about the possibility of having a .config/lazygit/per_repo_template.yml that lazy git could copy into the active repo on command? I think my per-repo settings would have similar structure but different values from repo to repo. I hate to add things just for the sake of it, but it could be nice to be able to quickly bootstrap the config for a new clone.

Users can already provide their chosen defaults in ~/.config/lazygit/config.yml, so a template like this would only help in an "argument" between the user's defaults and a versioned .lazygit.yml in the repo. If that's needed, it gives me pause about the overall design.

jwhitley commented 5 months ago

@stefanhaller I just noted that lazygit config can specify full user commands. That suggests that recognized .lazygit.yml keys must be in an allow-list which includes no configs which can specify a command, for security reasons. That, or lazygit must also fall down the rabbit-hole of "do you trust this repository's people? ?? ???"

afhoffman commented 5 months ago

Completely true. I even have a git helper script to expedite this use case: git-embargo. Still, there are a lot of devs for whom needing familiarity with a somewhat obscure git feature to get something done amounts to "undiscoverable" in the context of lazygit UX.

Yes I agree w/ that in terms of discoverability. I had no idea what .git/info/exclude was until I saw it in the ignore menu in lazygit and googled what it was.

Users can already provide their chosen defaults in ~/.config/lazygit/config.yml, so a template like this would only help in an "argument" between the user's defaults and a versioned .lazygit.yml in the repo. If that's needed, it gives me pause about the overall design.

I think I failed to explain my envisioned use-case. I have my global defaults, but I'm contributing to a repository which has a different commit prefix convention than I normally like to use. I want to quickly set up a config for my clone of that repo because I don't want to add those prefixes to my global config.

Or if I'm working with a couple different teams and Team 1 has one convention, Team 2 has a different convention. I don't want to have too many prefix presets in my global config, and I don't want to have to try and remember which prefixes belong to which team if I see them all in a list together. If neither team consents to a tracked .lazygit.yml or adding it to the .gitignore, but I want to be able to quickly set up my cloned repo with a local config.

So I think what I was intending was more something that helps automate setting up a local config in a repo which doesn't already have one. I totally agree, my idea has no utility in a situation where the repo you're working in already has a tracked .lazygit.yml.

I think if lazygit adds the option to have a local config (and I think it should), it would be nice if it could help you stub one out when you need one instead of making the user go copy/paste it out of the docs.

stefanhaller commented 5 months ago

@stefanhaller I just noted that lazygit config can specify full user commands. That suggests that recognized .lazygit.yml keys must be in an allow-list which includes no configs which can specify a command, for security reasons. That, or lazygit must also fall down the rabbit-hole of "do you trust this repository's people? ?? ???"

Yes, this was brought up in the mailing list thread I cited above. It was careless of me not to mention it in the PR description, I added it now.

Unfortunately this would make the implementation a lot less straightforward. I had hoped we could implement the feature by just adding a few lines of code to unmarshal the local config file into the config struct after the global one.

duckbrain commented 5 months ago

It seems to me like there may only be a few options that make sense to have project-specific overrides. We could have a "different" config format that describes just those fields. Anything related to scripts/keybindings/gui seem in appropriate for project-specific overrides.

Maybe the git section of the config is appropriate as a whole? I don't see anything that looks problematic on a cursory glance. That'd make the implementation and the documentation easy enough. ".lazygit.yml contains only a git section that matches the git section in the base config)

This can be done relatively sanely in the decoding logic by building up a struct that has pointers to the primary struct and unmarshalling into that.

type RepoConfig struct {
    Git *GitConfig `yaml:"git"`
}

...

var repoConfig = RepoConfig{
    Git: &base.Git,
}
yaml.Unmarshal(content, &repoConfig)
jesseduffield commented 2 weeks ago

I worry @duckbrain that if we only allow a subset of the config to be used that: 1) some users will be annoyed at the constraint 2) some configs could still end up being abused by bad actors

Another option is to add a popup when lazygit encounters a new config file on startup that gets the user to confirm that they trust the file: then we can add the path to an array so that next time the user opens lazygit the config is merged with the global one. I think this would work great: it's a minor inconvenience the first time you open lazygit with that config, but it's only a one-time cost. Vscode does similar things (it's always asking me if I trust something)

I think having a file in the workspace that the user can choose to either ignore or exclude should work.

For the case of having one config apply to many repos, perhaps we could also look for files in parent directories, all the way to the root? Not sure how well that gels with your setup @afhoffman .

stefanhaller commented 2 weeks ago

I think the best option for the security concern is to filter out all those config options that could be abused (all that can be set to a command, basically). This is not very hard to do, technically, and the only challenge is that we need to carefully audit the set of configs that need to be filtered out.

  1. some users will be annoyed at the constraint

If these are users who want to use their own local config file, then they can simply use the one in .git/lazygit.yml (as opposed to putting it in the tree and git-ignoring it), that one is not restricted. For a versioned config file, I think the restriction makes sense and shouldn't be surprising.

  1. some configs could still end up being abused by bad actors

I don't understand, can you give an example?

Another option is to add a popup when lazygit encounters a new config file on startup

Personally I hate all this trust stuff that VS Code has started to ask me about a while ago, so I globally turned that off. So I'm not in favor of this option. Also, we would probably have to remember a hash of the file (or something like that), so that we can ask again whenever it has changed.


I had started to work on this topic quite a while ago, but then paused it again when I ran into issues that I didn't anticipate. The biggest issue is that we need to reload config files while lazygit is running, but some configs are only read once at program start. Examples are properties that are set on views, e.g. gui.border, or things like refresher.fetchInterval. We'd have to find a way to reconfigure these upon reloading a config file. This seems brittle and error-prone to me, it's so easy to forget one.

The upside is that once we have solved that somehow, we could reload the config file not only when switching repos, but any time it changed. We'd just have to remember the modification date(s), and check them each time our terminal window receives focus. I would find this very attractive.

jesseduffield commented 2 weeks ago

I think the best option for the security concern is to filter out all those config options that could be abused (all that can be set to a command, basically). This is not very hard to do, technically, and the only challenge is that we need to carefully audit the set of configs that need to be filtered out.

What concerns me is that some of the things we would want to configure on a per-repo basis are exactly the things that pose security risks e.g. having repo-specific custom commands or configuring a repo-specific editor (that doesn't use a preset)

I don't understand, can you give an example?

My argument is that we could do an audit now but later on forget to exclude some new config key that involves executing code and then we'd be back to having a security vulnerability again.

The upside is that once we have solved that somehow, we could reload the config file not only when switching repos, but any time it changed. We'd just have to remember the modification date(s), and check them each time our terminal window receives focus. I would find this very attractive.

I agree that would be a dream.

jesseduffield commented 2 weeks ago

Another thing that just came to mind on the topic of excluding keys vs requiring user confirmation: vscode's incessant trust checks can be annoying but the 99% typical case is that a user creates one or two config files for themselves and has to confirm them once. Maybe a workplace has a lazygit config that's in version control. But it's going to be rare. I would much prefer the trust check if it means I have full control over what I put in my per-repo config.

stefanhaller commented 2 weeks ago

Maybe I wasn't clear enough on that, but my proposal is to support both ./.lazygit.yml and .git/lazygit.yml. The former could be used either as a local config file (when git-ignored), or a versioned one, and it would be restricted for that reason; the latter can only be used as a local config file (obviously), and thus doesn't need to be restricted. Doesn't that address the concern?

jesseduffield commented 2 weeks ago

I'm still uneasy about it. Putting everything on the table: Restricted config:

Trust popup:

Thoughts?

jwhitley commented 2 weeks ago

Going back to Stefan's original use-cases, what about ./.lazygit.yml only permitting keys in a short allow list? The idea would be to surface a limited initial selection of config settings that are useful to standardize across multiple contributors. Lazygit is a pretty "personal" development tool, and I'm not offhand convinced of the need to allow any repository to override certain settings (e.g. git.paging.pager just. no.). If any useful settings are missed on the first pass, those are easy PRs. Likewise, any PR adding a setting to an allow list is also going to jump out at review time.

That completely avoids the problem of block lists being default-allow for newly introduced config settings. It may also avoid the need to think (yet) about trusted/untrusted repos, since I suspect there really aren't use cases for standardizing any setting that specifies a command.

stefanhaller commented 2 weeks ago

Going back to Stefan's original use-cases, what about ./.lazygit.yml only permitting keys in a short allow list?

@jesseduffield made it pretty clear that he is against that, and actually I agree now. I find it totally plausible that someone has a repo containing files of a certain special kind, and provides a bunch of custom lazygit commands for working with them. So yes, I think the trust popup is the way to go.

There are still some questions around that, but I think they can all be solved in some way. For example, a repo might start out without a .lazygit.yml file, a user creates a local, git-ignored one, and answers the trust question once. Now someone git-adds a versioned config file, which will clobber the user's local one, and won't put up the prompt again because the user answered it already before; security hole. (Unless we remember the hash of the file and ask again whenever it changes.)

But actually we can tell whether a file is versioned or not, so we could put up the prompt only if it is; that would solve it. (I think -- security is tricky, it's so easy to miss cases.)