Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we resolve the symlink of the cwd #3017

Closed
wants to merge 1 commit into from
Closed

Make sure we resolve the symlink of the cwd #3017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pkg/commands/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ func NewGitCommand(
// converting to forward slashes for the sake of windows (which uses backwards slashes). We want everything
// to have forward slashes internally
currentPath = filepath.ToSlash(currentPath)
if currentPath, err = git_commands.ResolveSymlink(currentPath); err != nil {
return nil, utils.WrapError(err)
}

gitDir := env.GetGitDirEnv()
if gitDir != "" {
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/git_commands/repo_paths.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func GetRepoPaths(
fs afero.Fs,
currentPath string,
) (*RepoPaths, error) {
return getRepoPathsAux(afero.NewOsFs(), resolveSymlink, currentPath)
return getRepoPathsAux(afero.NewOsFs(), ResolveSymlink, currentPath)
}

func getRepoPathsAux(
Expand Down Expand Up @@ -228,7 +228,7 @@ func getCurrentRepoGitDirPath(
}

// takes a path containing a symlink and returns the true path
func resolveSymlink(path string) (string, error) {
func ResolveSymlink(path string) (string, error) {
l, err := os.Lstat(path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we even need this check if it's os.ModeSymlink?

In order to be able to call lazygit from a folder whose path contains a symlink this check fails, and lazygit fails to start, but if we just return filepath.EvalSymlinks(path), all works as expected.

The example is what I've written in the conversation for this PR.

@jesseduffield could this have been done for potential performance reasons? Tried blaming it, but it went through two refactors so I stopped.

Copy link
Owner

Choose a reason for hiding this comment

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

I've never made any performance-related decisions around symlinks. If filepath.EvalSymlinks(path) works I'm happy for us to use it

Copy link
Collaborator

@mark2185 mark2185 Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The docs say:

// EvalSymlinks returns the path name after the evaluation of any symbolic
// links.
// If path is relative the result will be relative to the current directory,
// unless one of the components is an absolute symbolic link.
// EvalSymlinks calls Clean on the result.

That would suggest nothing changes if the given path isn't a symlink/contains a symlink, so I see no reason to if-else on it being a symlink for this function at all.

But I'd wait for tests that confirm it.

if err != nil {
return "", err
Expand Down