Skip to content

Conversation

@BlakeWilliams
Copy link

@BlakeWilliams BlakeWilliams commented Oct 12, 2024

Currently lazygit looks for its config file in XDG_CONFIG_HOME if it's available, but if not it falls back to the defaults defined by the xdg package. Unfortunately the defaults the package falls back to isn't what CLI applications commonly fall back to on macOS. Specifically, it looks in ~/Library/Application Support instead of ~/.config.

This updates the app config logic to:

  • Look for ~/.config/lazygit first if XDG_CONFIG_HOME is not set and we're on macOS.
  • Fallback to the existing xdg package location if the configuration file exists there.
  • Default to ~/.config/lazygit/config.yml if XDG_CONFIG_HOME is not set, we're on macOS, and there is no existing configuration file.

This change did feel a bit like having to thread a needle and I didn't see any existing tests for this behavior (which is reasonable, since it's complicated and OS dependent) so I did test a few variations of the configuration locally by building with this change included and comparing against a brew installed lazygit.

It seemed to work properly, falling back to the existing location when XDG_CONFIG_HOME isn't set, using ~/.config/lazygit when config.yml is present, and creating ~/.config/lazygit/config.yml when it's not.

I think this should resolve #1341

  • Cheatsheets are up-to-date (run go generate ./...)
  • Code has been formatted (see here)
  • Tests have been added/updated (see here for the integration test guide)
  • Text is internationalised (see here)
  • If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
  • Docs have been updated if necessary
  • You've read through your own file changes for silly mistakes etc

@BlakeWilliams
Copy link
Author

I took a quick pass at updating the docs, but I didn't want to change too much without getting thoughts on how we might want to communicate this change, if at all. Happy to update that if you have any preferences there!

@BlakeWilliams BlakeWilliams marked this pull request as ready for review October 12, 2024 19:35
return true, legacyConfigPath
}

// if on macOs, default to looking in ~/.config/lazygit first since that's
Copy link
Author

@BlakeWilliams BlakeWilliams Oct 12, 2024

Choose a reason for hiding this comment

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

I generally don't like lengthy comments in the middle of methods, but I thought that this was obtuse enough to warrant a description of why we have to implement this behavior instead of using xdg.

Copy link
Contributor

Choose a reason for hiding this comment

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

macOs typo?

Copy link
Owner

@jesseduffield jesseduffield left a comment

Choose a reason for hiding this comment

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

LGTM. Just so there's no ambiguity: if I've got my current config file in ~/Library/Application\ Support/lazygit/config.yml and I don't have the xdg env var set, lazygit will continue using the config from ~/Library/Application\ Support/lazygit/config.yml?

@BlakeWilliams
Copy link
Author

LGTM. Just so there's no ambiguity: if I've got my current config file in ~/Library/Application\ Support/lazygit/config.yml and I don't have the xdg env var set, lazygit will continue using the config from ~/Library/Application\ Support/lazygit/config.yml?

Just double checked this and output some debug logs for additional confidence:

With XDG_CONFIG_HOME unset, and ~/.config/lazygit/config.yml exists:

lazygit ❯ go run .
&{/Users/Blake/.config/lazygit/config.yml 0 {339739848 63870478900 0x105f1f020} true}

With XDG_CONFIG_HOME unset, and ~/.config/lazygit/config.yml does not exist:

lazygit ❯ go run .
&{/Users/Blake/Library/Application Support/lazygit/config.yml 0 {339739848 63870478900 0x101f87020} true}

So yes, you're correct that it will fall back to ~/Library/Application\ Support/lazygit/config.yml when XDG_CONFIG_HOME is unset and ~/.config/lazygit/config.yml does not exist.

@jesseduffield jesseduffield added the enhancement New feature or request label Dec 31, 2024
@jesseduffield
Copy link
Owner

Cool, I'm happy to merge this, but can I get you to squash your changes into a single commit? Given you've merged the master branch into your branch, this might be messy. Here's a script I use to disentangle things in that state:

#!/bin/bash

# Set the base branch
BASE_BRANCH="master"

# Check if the working tree is dirty
if [[ -n $(git status --porcelain) ]]; then
    echo "Error: Working tree is dirty. Please commit or stash your changes before running this script."
    exit 1
fi

# Get the merge base commit
merge_base=$(git merge-base $BASE_BRANCH HEAD)

# Get the first commit hash, message, and author details
first_commit_hash=$(git rev-list --reverse $merge_base..HEAD | head -n 1)
first_commit_message=$(git log -1 --format=%B $first_commit_hash)

# Reset to the merge base
git reset $merge_base

# Stage all changes
git add -A

# Create a new commit with all the changes, using the first commit's message and author
GIT_AUTHOR_NAME="$(git log -1 --format='%an' $first_commit_hash)" \
GIT_AUTHOR_EMAIL="$(git log -1 --format='%ae' $first_commit_hash)" \
git commit -m "$first_commit_message"

# Rebase onto the base branch
git rebase $BASE_BRANCH

Currently lazygit looks for its config file in `XDG_CONFIG_HOME` if it's
available, but if not it falls back to the defaults defined by the
[xdg](https://github.com/adrg/xdg) package. Unfortunately the defaults
the package falls back to isn't what CLI applications commonly fall back
to on macOS. Specifically, it looks in `~/Library/Application Support`
instead of `~/.config`.

This updates the app config logic to:

- Look for `~/.config/lazygit` first if `XDG_CONFIG_HOME` is not set
  and we're on macOS.
- Fallback to the existing `xdg` package location if the configuration
  file exists there.
- Default to `~/.config/lazygit/config.yml` if `XDG_CONFIG_HOME` is not
  set, we're on macOS, and there is no existing configuration file.

This change did feel a bit like having to thread a needle and I didn't
see any existing tests for this behavior (which is reasonable, since it's
complicated and OS dependent) so I did test a few variations of the
configuration locally by building with this change included and running
a `brew` installed lazygit.

It seemed to work properly, falling back to the existing location when
`XDG_CONFIG_HOME` isn't set, using `~/.config/lazygit` when `config.yml`
is present, and creating `~/.config/lazygit/config.yml` when it's not.

I think this should resolve jesseduffield#1341
@BlakeWilliams
Copy link
Author

@jesseduffield Sorry about that, work habit. Just squashed and pushed 👍

michelegera added a commit to michelegera/dotfiles that referenced this pull request Feb 23, 2025
Theme will be correctly applied once jesseduffield/lazygit#3989 is merged and released.
@stefanhaller
Copy link
Collaborator

@BlakeWilliams Sorry for the long delay in dealing with this. I haven't been involved in the first round of reviews, and it seems Jesse simply forgot to merge this. And he's very busy these days, so I'm happy to take over now that it came up again.

I have two questions/concerns about this:

  • looks like the default location has changed to ~/.config/lazygit/config.yml (in the case that no config file exists yet in either location). Is this really a good change, or should we rather keep creating it in Application Support in that case and only use ~/.config/lazygit/config.yml if it exists? The reason I'm thinking about this is that for me as a developer it won't be a good idea to move my config file to ~/.config, because checking out older versions is a very common thing for me (e.g. to check when a bug was introduced or fixed, or to remind myself how a certain feature worked back in version x.y), so it would be annoying if those older versions wouldn't find my config file. Maybe it's just me, and I would just keep my config file in Application Support and all is well, but I'm wondering if this could also be annoying for other people. Sometimes when people report issues they might check out older versions too to see where the bug was introduced. I'm happy to be convinced that the change is a good one, I just wanted to mention this as a concern.
  • if we do make this change, then shouldn't we also change the default location of state.yml? It looks like it is still created in Application Support in this branch. From what I can tell, the corresponding new location should be ~/.local/state/lazygit/state.yml then.

And finally a question about the implementation: I'm not very familiar with the xdg library we are using, and didn't have time to have a closer look now, but I wonder if it has been explored to configure the library to search in the directories we want, instead of adding code on our side to do that? For example, it looks like it should be possible to set xdg.ConfigHome and/or xdg.ConfigDirs to the directories we want to use.

@BlakeWilliams
Copy link
Author

  • looks like the default location has changed to ~/.config/lazygit/config.yml (in the case that no config file exists yet in either location). Is this really a good change, or should we rather keep creating it in Application Support in that case and only use ~/.config/lazygit/config.yml if it exists? The reason I'm thinking about this is that for me as a developer it won't be a good idea to move my config file to ~/.config, because checking out older versions is a very common thing for me (e.g. to check when a bug was introduced or fixed, or to remind myself how a certain feature worked back in version x.y), so it would be annoying if those older versions wouldn't find my config file. Maybe it's just me, and I would just keep my config file in Application Support and all is well, but I'm wondering if this could also be annoying for other people. Sometimes when people report issues they might check out older versions too to see where the bug was introduced. I'm happy to be convinced that the change is a good one, I just wanted to mention this as a concern.

Totally reasonable concern to bring up! I had two main motivations with this change: enable LazyGit to use the common and expected XDG path ~/.config and to make the default less surprising and in-line with the majority of other CLI programs. Both without breaking backwards compatibility 🙂

To your point about backwards compatibility, I could see an argument for keeping the default as-is for a few releases (or some period of time, whatever makes most sense) before swapping the default location. That way we have some level of backwards compatibility across versions if/when we do make the swap. I'm curious how much of a concern that is here since I can't make that call, that's up to y'all as maintainers.

  • if we do make this change, then shouldn't we also change the default location of state.yml? It looks like it is still created in Application Support in this branch. From what I can tell, the corresponding new location should be ~/.local/state/lazygit/state.yml then.

I think we could, but due to users generally not editing that file manually, it felt less important to move.

@stefanhaller
Copy link
Collaborator

To your point about backwards compatibility, I could see an argument for keeping the default as-is for a few releases (or some period of time, whatever makes most sense) before swapping the default location.

I like this suggestion, let's do that. I'll set myself a reminder to switch the default in a few months.

  • if we do make this change, then shouldn't we also change the default location of state.yml? It looks like it is still created in Application Support in this branch. From what I can tell, the corresponding new location should be ~/.local/state/lazygit/state.yml then.

I think we could, but due to users generally not editing that file manually, it felt less important to move.

While that's true, I'd prefer the two to use consistent locations, so I do think it should move too. But we can take care of that once we switch the default.

I'd also like to come back to my "And finally a question about the implementation" question above; did you explore this? I think this could save us some lines of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support .config/XDG configuration location in MacOSX

4 participants