internal/fs: handle symlinks in copyFile() #657

ibrasho · 2017-05-26T11:31:07Z

fs.copyFile() currently fails when src is a symlink to a directory.

This PR adds 2 functions:

fs.IsSymlink() to check if a path is a symbolic link.
fs.copySymlink() to copy a symlink (preserves relative symlinks)

And update fs.copyFile() to handle symlinks correctly.

Should solve #651 .

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho · 2017-05-26T12:03:20Z

I borrowed some functions from #641 . 😉

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

sdboyer · 2017-05-26T12:29:05Z

internal/fs/fs.go

@@ -242,6 +242,13 @@ func CopyDir(src, dst string) error {
 // of the source file. The file mode will be copied from the source and
 // the copied data is synced/flushed to stable storage.
 func copyFile(src, dst string) (err error) {
+	if isSymlink, err := IsSymlink(src); err != nil {


For a short function like this, using a shorter variable name - e.g., is or sym instead of isSymlink - is more idiomatic.

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

davecheney · 2017-05-27T12:38:47Z

internal/fs/fs_test.go

+	}
+
+	for path, want := range tests {
+		if runtime.GOOS == "windows" {


Do this at the top of the test before doing any setup work

davecheney · 2017-05-27T12:39:37Z

internal/fs/fs_test.go

+		got, err := IsSymlink(path)
+		if err != nil {
+			if !want.err {
+				t.Fatalf("expected no error, got %v", err)


use t.Errorf(); continue

so that you get all the test failures, not just the first

davecheney · 2017-05-27T12:40:09Z

internal/fs/fs.go

@@ -242,6 +242,13 @@ func CopyDir(src, dst string) error {
 // of the source file. The file mode will be copied from the source and
 // the copied data is synced/flushed to stable storage.
 func copyFile(src, dst string) (err error) {
+	if sym, err := IsSymlink(src); err != nil {
+		return errors.Wrapf(err, "could not lstat %s", src)


just return the error, it's already wrapped

davecheney · 2017-05-27T12:40:57Z

internal/fs/fs.go

+	}
+
+	err = os.Symlink(resolved, dst)
+	if err != nil {


you can write this

return errors.Wrapf(err, "...")

if err == nil, errors.Wrapf returns nil.

This might be too magical.

davecheney · 2017-05-27T12:41:33Z

internal/fs/fs.go

@@ -337,3 +360,13 @@ func IsRegular(name string) (bool, error) {
 	}
 	return true, nil
 }
+
+// IsSymlink determines if the given path is a symbolic link.


Please add a note to describe the behaviour of this function on Windows.

It should behave in the same way. os.Lstat() call GetFileAttributesEx on Windows and FileStat.Mode() will convert the attribute syscall.FILE_ATTRIBUTE_REPARSE_POINT to os.ModeSymlink. (see os/stat_windows.go and os/types_windows.go).

Unless I'm missing something else. 😄

The reason why they the tests for this function are skipped is that creating a symlink (using os.Symlink()) is not supported on Windows. But if the symlink already exists, it will be detected by this function.

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

sdboyer

small style nits, then LGTM

sdboyer · 2017-05-30T02:34:55Z

internal/fs/fs.go

+
+	err = os.Symlink(resolved, dst)
+
+	return errors.Wrapf(err, "failed to create symlink %s to %s", src, resolved)


This is awkwardly halfway between a terse form and the standard form. Either combine this and the preceding line into one (errors.Wrapf(os.Symlink(resolved, dst), ...)), or do the more standard if err != nil check.

sdboyer · 2017-05-30T02:39:28Z

internal/fs/fs_test.go

+		}
+
+		inaccessibleSymlink = filepath.Join(dir, "symlink")
+		err = os.Symlink(inaccessibleFile, inaccessibleSymlink)


just return this final call directly, no need to assign into var

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho · 2017-05-30T06:47:52Z

Done.

sdboyer · 2017-05-30T12:58:47Z

great, thanks! 🎉

internal/fs: add tests for fs.copyFile() on symlinks

1dd2d58

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

googlebot added the cla: yes label May 26, 2017

ibrasho added 3 commits May 26, 2017 14:31

internal/fs: add IsSymlink() and copySymlink() to fix copyFile()

b614fbf

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

internal/fs: add tests for IsSymlink()

e8fb51d

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

internal/fs: simplify statement

fc0c4b7

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

internal/fs: skip inaccessible file tests on windows

4af7780

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

ibrasho force-pushed the fix-symlink-copying branch from 489e562 to 4af7780 Compare May 26, 2017 13:35

sdboyer reviewed May 26, 2017

View reviewed changes

internal/fs: rename var in copyFile

f82ed6b

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

davecheney suggested changes May 27, 2017

View reviewed changes

internal/fs: nits here and there

e568774

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

sdboyer suggested changes May 30, 2017

View reviewed changes

internal/fs: more nits

4d224bd

Signed-off-by: Ibrahim AshShohail <ibra.sho@gmail.com>

sdboyer approved these changes May 30, 2017

View reviewed changes

sdboyer merged commit 37fe793 into golang:master May 30, 2017

ibrasho deleted the fix-symlink-copying branch May 30, 2017 14:05

ibrasho mentioned this pull request May 30, 2017

dep init failing for repositories with sub-directories #651

Closed

ibrasho mentioned this pull request Jun 20, 2017

Symlink issue on Windows: A required privilege is not held by the client. #773

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

internal/fs: handle symlinks in copyFile() #657

internal/fs: handle symlinks in copyFile() #657

ibrasho commented May 26, 2017

ibrasho commented May 26, 2017

sdboyer May 26, 2017

davecheney May 27, 2017

davecheney May 27, 2017

davecheney May 27, 2017

davecheney May 27, 2017

davecheney May 27, 2017

ibrasho May 27, 2017

ibrasho May 27, 2017 •

edited

Loading

sdboyer left a comment

sdboyer May 30, 2017

sdboyer May 30, 2017

ibrasho commented May 30, 2017 •

edited

Loading

sdboyer commented May 30, 2017


		err = os.Symlink(resolved, dst)

		return errors.Wrapf(err, "failed to create symlink %s to %s", src, resolved)

internal/fs: handle symlinks in copyFile() #657

internal/fs: handle symlinks in copyFile() #657

Conversation

ibrasho commented May 26, 2017

ibrasho commented May 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho May 27, 2017 • edited Loading

Choose a reason for hiding this comment

sdboyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ibrasho commented May 30, 2017 • edited Loading

sdboyer commented May 30, 2017

ibrasho May 27, 2017 •

edited

Loading

ibrasho commented May 30, 2017 •

edited

Loading