Make sure we resolve the symlink of the cwd #3017

chmouel · 2023-09-20T13:02:58Z

Make sure we resolve the current directory symlink or it would fail
getting confused about the repo root.

Cheatsheets are up-to-date (run go run scripts/cheatsheet/main.go generate)
Code has been formatted (see here)
Tests have been added/updated (see here for the integration test guide)

Make sure we resolve the current directory symlink or it would fail getting confused about the repo root. Signed-off-by: Chmouel Boudjnah <[email protected]>

chmouel · 2023-09-20T13:05:42Z

@jesseduffield i'd like to add tests but it seems that MemFs driver of aerofs does not support Symlinking, would that be okay to create a temporary dir and create a repo in there with symlink ? that would be in the integration test isnt it ? let me know if you have any suggestions on how to do this..

jesseduffield · 2023-09-25T02:03:07Z

@chmouel yep it is annoying that aeofs doesn't cover symlinking. I say in that case it's best to create an integration test for this. Should be simple enough to verify that you can start lazygit inside the symlinked repo without it exiting with an error. There's an example of such a test at pkg/integration/tests/config/remote_named_star.go

mark2185 · 2023-10-18T14:09:54Z

This works only if lazygit is launched from a folder that is a symlink, it doesn't work if it's e.g. one level deeper.

E.g.:

$> file ~/.config/foo
/home/mark/.config/foo: symbolic link to /home/mark/workspace/gits/dotfiles/.config/foo
$> cd ~/.config/foo
$> lazygit
(works)
$> cd foo-deeper/
$> lazygit
2023/10/18 16:06:57 An error occurred! Please create an issue at: https://github.com/jesseduffield/lazygit/issues

*errors.errorString Must open lazygit in a git repository
/home/mark/workspace/gits/lazygit/pkg/commands/git.go:236 (0x985906)
	findWorktreeRoot: return "", errors.New("Must open lazygit in a git repository")
/home/mark/workspace/gits/lazygit/pkg/commands/git.go:90 (0x984488)
	NewGitCommand: rootDirectory, err := findWorktreeRoot(cmn.Fs, currentPath)
/home/mark/workspace/gits/lazygit/pkg/gui/gui.go:271 (0xabf68b)
	(*Gui).onNewRepo: gui.git, err = commands.NewGitCommand(
/home/mark/workspace/gits/lazygit/pkg/gui/gui.go:655 (0xac223b)
	(*Gui).Run: if err := gui.onNewRepo(startArgs, context.NO_CONTEXT); err != nil {
/home/mark/workspace/gits/lazygit/pkg/gui/gui.go:674 (0xac2925)
	(*Gui).RunAndHandleError.func1: if err := gui.Run(startArgs); err != nil {
/home/mark/workspace/gits/lazygit/pkg/utils/utils.go:111 (0x810bdc)
	SafeWithError: err := f()
/home/mark/workspace/gits/lazygit/pkg/gui/gui.go:673 (0xac286e)
	(*Gui).RunAndHandleError: return utils.SafeWithError(func() error {
/home/mark/workspace/gits/lazygit/pkg/app/app.go:265 (0xaeea1d)
	Run: err := app.Gui.RunAndHandleError(startArgs)
/home/mark/workspace/gits/lazygit/pkg/app/app.go:48 (0xaee9b2)
	Run: err = app.Run(startArgs)
/home/mark/workspace/gits/lazygit/pkg/app/entry_point.go:150 (0xaf0baa)
	Start: Run(appConfig, common, appTypes.NewStartArgs(cliArgs.FilterPath, parsedGitArg, integrationTest))
/home/mark/workspace/gits/lazygit/main.go:23 (0xaf2338)
	main: app.Start(ldFlagsBuildInfo, nil)
/usr/lib/go/src/runtime/internal/atomic/types.go:194 (0x43d8bb)
	(*Uint32).Load: return Load(&u.value)
/usr/lib/go/src/runtime/asm_amd64.s:1650 (0x46d961)
	goexit: BYTE	$0x90	// NOP

JL102 · 2023-10-20T18:19:15Z

Before discovering that it was a known issue (and before finding this PR), I created a minimal repo to reproduce the same symlink issue The README has simple reproduction instructions. Feel free to use it if it helps in any way. https://github.com/JL102/lazygit-submodule-symlink-test/tree/main

mark2185 · 2023-10-22T06:07:24Z

pkg/commands/git_commands/repo_paths.go

@@ -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)


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.

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

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.

hboetes · 2023-10-22T07:05:00Z

Works for me. 👍

jesseduffield/lazygit#3017

mark2185 · 2023-12-01T22:17:48Z

@chmouel are you still up for this? It would be great to have an integration test for this, and like Jesse said:

I say in that case it's best to create an integration test for this. Should be simple enough to verify that you can start lazygit inside > the symlinked repo without it exiting with an error. There's an example of such a test at pkg/integration/tests/config/remote_named_star.go

chmouel · 2023-12-02T07:46:26Z

@mark2185 you are right, i am still up for it, let me find some time this week-end or i'll close this PR

mark2185 · 2023-12-02T08:46:56Z

No need to close it, someone else can just build on top of it!

chmouel · 2023-12-04T08:57:18Z

So I have tried to build an integration and I have some difficulty changing the directory of where the test run:

//cat pkg/integration/tests/misc/symlinks_repo.go
package misc
 
import (
    "fmt"
    "os"
    "path/filepath"
 
    "github.com/jesseduffield/lazygit/pkg/config"
    . "github.com/jesseduffield/lazygit/pkg/integration/components"
)
 
var SymlinkRepo = NewIntegrationTest(NewIntegrationTestArgs{
    Description:  "Make sure lazygit open on a symlink into a repository",
    ExtraCmdArgs: []string{},
    Skip:         false,
    SetupConfig:  func(config *config.AppConfig) {},
    SetupRepo: func(shell *Shell) {
        shell.NewBranch("mybranch")
        shell.CreateDir("symlinked")
        shell.CreateFileAndAdd("symlinked/blah.txt", "blah")
        shell.Commit("initial commit")
    },
    Run: func(t *TestDriver, _ config.KeybindingConfig) {
        tempdir, _ := os.MkdirTemp("", "lazygit-symlink")
        t.Shell().RunShellCommand(fmt.Sprintf("ln -s $PWD/symlinked %s", tempdir))
        _ := os.Chdir(filepath.Join(tempdir, "symlinked"))
         t.Views().Commits()
    },
})

It need to be outside of where the test run currently since that bug would not reproduce otherwise so using a tempdir, but when I run that test against master it doesn't fail, i can't figure out why the Chdir is not taken into account, if someone has an idea that would be great,

(i haven't defer the os.RemoveAll of tempdir because i think it was bringing other issues)

jwhitley · 2024-01-01T17:59:20Z

FYI, this PR would be obsoleted by PR #3183, which replaces lazygit's pathfinding code by delegating that work to git (via git rev-parse). I've confirmed this by following @JL102's reproduction steps in the test repo linked above. In short, since git already correctly handles many edge cases of this ilk, delegating to git is ultimately a more robust approach.

chmouel · 2024-01-01T19:24:14Z

@jwhitley perfect thank you, i may close this PR now since it was stalled for too long!

Make sure we resolve the symlink of the cwd

e34354e

Make sure we resolve the current directory symlink or it would fail getting confused about the repo root. Signed-off-by: Chmouel Boudjnah <[email protected]>

mark2185 reviewed Oct 22, 2023

View reviewed changes

This was referenced Oct 22, 2023

lazygit no longer works in a symlinked directory #2958

Closed

an error occured! under a soft link dir #2932

Closed

Under a soft link dir it should be work! #2933

Closed

hboetes added a commit to hboetes/sports that referenced this pull request Oct 24, 2023

Add PR that resolves CWD

b19e2af

jesseduffield/lazygit#3017

mark2185 mentioned this pull request Oct 31, 2023

[Bug] Lazygit crashes when inside a link to a subdirectory of a git repo #3102

Closed

mark2185 added the bug Something isn't working label Dec 1, 2023

chmouel closed this Jan 1, 2024

chmouel deleted the resolve-symlinks branch January 1, 2024 19:24

jwhitley mentioned this pull request Jan 2, 2024

Use git rev-parse to obtain repository and worktree paths #3183

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure we resolve the symlink of the cwd #3017

Make sure we resolve the symlink of the cwd #3017

chmouel commented Sep 20, 2023 •

edited

Loading

chmouel commented Sep 20, 2023

jesseduffield commented Sep 25, 2023

mark2185 commented Oct 18, 2023

JL102 commented Oct 20, 2023 •

edited

Loading

mark2185 Oct 22, 2023

jesseduffield Oct 23, 2023

mark2185 Oct 23, 2023 •

edited

Loading

hboetes commented Oct 22, 2023

mark2185 commented Dec 1, 2023

chmouel commented Dec 2, 2023

mark2185 commented Dec 2, 2023

chmouel commented Dec 4, 2023 •

edited

Loading

jwhitley commented Jan 1, 2024

chmouel commented Jan 1, 2024

Make sure we resolve the symlink of the cwd #3017

Make sure we resolve the symlink of the cwd #3017

Conversation

chmouel commented Sep 20, 2023 • edited Loading

chmouel commented Sep 20, 2023

jesseduffield commented Sep 25, 2023

mark2185 commented Oct 18, 2023

JL102 commented Oct 20, 2023 • edited Loading

mark2185 Oct 22, 2023

Choose a reason for hiding this comment

jesseduffield Oct 23, 2023

Choose a reason for hiding this comment

mark2185 Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

hboetes commented Oct 22, 2023

mark2185 commented Dec 1, 2023

chmouel commented Dec 2, 2023

mark2185 commented Dec 2, 2023

chmouel commented Dec 4, 2023 • edited Loading

jwhitley commented Jan 1, 2024

chmouel commented Jan 1, 2024

chmouel commented Sep 20, 2023 •

edited

Loading

JL102 commented Oct 20, 2023 •

edited

Loading

mark2185 Oct 23, 2023 •

edited

Loading

chmouel commented Dec 4, 2023 •

edited

Loading