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.

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

Recognize circular links #3593

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 35 additions & 1 deletion internal/smartlink/smartlink.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,15 @@ func LinkContents(src, dest string) error {
// Link creates a link from src to target. MS decided to support Symlinks but only if you opt into developer mode (go figure),
// which we cannot reasonably force on our users. So on Windows we will instead create dirs and hardlinks.
func Link(src, dest string) error {
var err error
isCircular, err := isCircularLink(src)
if err != nil {
return errs.Wrap(err, "could not check for circular link")
}
if isCircular {
logging.Warning("Skipping linking '%s' to '%s' as it is a circular link", src, dest)
return nil
}

src, dest, err = resolvePaths(src, dest)
if err != nil {
return errs.Wrap(err, "Could not resolve src and dest paths")
Expand Down Expand Up @@ -80,6 +88,32 @@ func Link(src, dest string) error {
return nil
}

func isCircularLink(src string) (bool, error) {
if !fileutils.IsSymlink(src) || !fileutils.IsDir(src) {
return false, nil
}

target, err := os.Readlink(src)
if err != nil {
return false, errs.Wrap(err, "could not read symlink %s", src)
}

if !filepath.IsAbs(target) {
resolved, err := fileutils.ResolveUniquePath(src)
if err != nil {
return false, errs.Wrap(err, "Could not resolve src path")
}
target = resolved
}

// If the target points at the parent directory, we have a circular link
if filepath.Dir(src) == target {
return true, nil
}

return false, nil
}
Comment on lines +91 to +115
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't feel right. It's totally valid to have a link like this:

- dir1/file1
- dir1/dir2 > ../dir1

The problem is that our Linking mechanism doesn't understand this and will continue to recurse forever. We need to fix that, whilst still supporting a link that goes back up the path.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this library doesn't link directories I'm not sure how we can support this beyond what this PR is doing. If we encounter a circular directory we skip it, all of the contents of the directory will still be linked.

I've moved the check to make it more clear. I think that in the end, all we can do is ensure all of the files in the directories and subdirectories have been linked with the same layout as the source.


// UnlinkContents will unlink the contents of src to dest if the links exist
// WARNING: on windows smartlinks are hard links, and relating hard links back to their source is non-trivial, so instead
// we just delete the target path. If the user modified the target in any way their changes will be lost.
Expand Down
67 changes: 67 additions & 0 deletions internal/smartlink/smartlink_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package smartlink

import (
"os"
"path/filepath"
"testing"

"github.com/ActiveState/cli/internal/fileutils"
"github.com/stretchr/testify/require"
)

func TestLinkContentsWithCircularLink(t *testing.T) {
srcDir, err := os.MkdirTemp("", "src")
require.NoError(t, err)
defer os.RemoveAll(srcDir)

destDir, err := os.MkdirTemp("", "dest")
require.NoError(t, err)
defer os.RemoveAll(destDir)

// Create test file structure:
// src/
// ├── regular.txt
// └── subdir/
// ├── circle -> subdir (circular link)
// └── subfile.txt

testFile := filepath.Join(srcDir, "regular.txt")
err = os.WriteFile(testFile, []byte("test content"), 0644)
require.NoError(t, err)

subDir := filepath.Join(srcDir, "subdir")
err = os.Mkdir(subDir, 0755)
require.NoError(t, err)

subFile := filepath.Join(subDir, "subfile.txt")
err = os.WriteFile(subFile, []byte("sub content"), 0644)
require.NoError(t, err)

circularLink := filepath.Join(subDir, "circle")
err = os.Symlink(subDir, circularLink)
require.NoError(t, err)

err = LinkContents(srcDir, destDir)
require.NoError(t, err)

// Verify file structure:
// dest/
// ├── regular.txt
// └── subdir/
// └── subfile.txt

destFile := filepath.Join(destDir, "regular.txt")
require.FileExists(t, destFile)
content, err := os.ReadFile(destFile)
require.NoError(t, err)
require.Equal(t, "test content", string(content))

destSubFile := filepath.Join(destDir, "subdir", "subfile.txt")
require.FileExists(t, destSubFile)
subContent, err := os.ReadFile(destSubFile)
require.NoError(t, err)
require.Equal(t, "sub content", string(subContent))

destCircular := filepath.Join(destDir, "subdir", "circle")
require.False(t, fileutils.TargetExists(destCircular), "Circular link should not have been created")
}
Loading