implement symlink functionality with filepath.EvalSymlinks()

[shibumi]

Please fill in the fields below to submit a pull request. The more
information that is provided, the better.

Fixes issue #:
#32
as well as PR #37

Description of pull request:
We are using filepath.EvalSymlinks() here, because it will
give us the last element in a symlink chain.
Therefore we don't need to worry about stack size
or recursion cycles.

Please verify and check that the pull request fulfills the following
requirements:

• Tests have been added for the bug fix or new feature
• Docs have been added for the bug fix or new feature

[shibumi]

mh ok this is weird.. when I run these tests locally they work...:

=== RUN   TestSymlinkToFile
--- PASS: TestSymlinkToFile (0.00s)
=== RUN   TestSymlinkToFolder
--- PASS: TestSymlinkToFolder (0.00s)

is this again some Windows related stuff?

[shibumi]

mhh looks like an error in path translation between Linux paths and Windows paths?!

json: unsupported type: func(*testing.T)
open bad/path: The system cannot find the path specified.
--- FAIL: TestSymlinkToFolder (0.00s)
    runlib_test.go:82: RecordArtifacts returned '(map[symTest\symTest2\symTmpfile:map[sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad]], %!s(<nil>))', expected '(map[symTest/symTest2/symTmpfile:map[sha256:ba7816bf8f01cfea414140de5dae2223b00361a396177a9cb410ff61f20015ad]], nil)'

I have also no idea where this "%!s(<nil>)" comes from

[shibumi]

Doesn't windows support SymLinks?

EDIT: no.. then the symlink wouldn't work at all.. right now it's calculating the hash correct

[shibumi]

I think I fixed the file path matching via using: filepath.FromSlash().. I am just wondering why this works for all the other tests.. lol.

[shibumi]

Mh I think I will add 1-2 more tests to the symlink code. I am not convinced yet, that it's secure without a max recursion parameter

-> draft

[shibumi]

I've added a test for this PR.. seems I were wrong and filepath.EvalSym() can not detect symlinks. :S
The newest test proofs this.

I could re-implement the recursion counter from the other PR or create a history or something.

[shibumi]

@lukpueh Can you have a look on this PR, too? I have reimplemented most of the PR #37.
The only difference here is that we return an error if detect a symlink cycle instead of going further.
I really think there should be no valid reason why a software project or any other project should build cycles with symlinks

[shibumi]

@lukpueh I have also added our first custom error. The error handling got some nice changes in go 1.13, we can use these changes and return our own library specific errors instead. This makes developers life easier, because they don't need to compare strings anymore.
They can just do:

if err == errors.Is(err, OurCustomErr){ ..}

[shibumi]

Shall I squash this PR into one commit? would make it easier, if we encounter problems to revert it.

[lukpueh]

Many thanks for the great work, @shibumi. I left a few minor suggestions and questions.

[lukpueh]

Thanks for coming up with and implementing this new approach, @shibumi! It looks a lot better than what we had before. Will this also work if we called RecordArtifacts twice? I think you need to reset visitedSymlinks at some point.

Thanks for addressing my review comment regarding line width. Is there a reason why you now sometimes wrap quite a bit before 80 chars? IMO you can use the full 80 chars for code comments.

[shibumi]

I will squash the last 2 nonsense commits from github UI to one 'refactor comments' commit. Shouldn't use github for applying changes directly .. sigh

[shibumi]

Thanks for coming up with and implementing this new approach, @shibumi! It looks a lot better than what we had before. Will this also work if we called RecordArtifacts twice? I think you need to reset visitedSymlinks at some point.

We could pass a set as argument, that way we could control the passed set and on recursion depth 0 devs would need to pass an empty set as initialization. It's the only solution I come up with, but I will think about this till Friday afternoon.

[lukpueh]

💯 Thanks, that's what I usually do. I like the feature from a reviewer's perspective, but I also think there should be no "Update XYZ" commits.

[lukpueh]

We could pass a set as argument, that way we could control the passed set and on recursion depth 0 devs would need to pass an empty set as initialization.

Sounds like a good idea. Or we add a wrapper like so:

# pseudo-code
function RecordArtifacts(paths):
  setup global var
  _recordArtifacts(paths) # recurse but don't visit symlinks more than once
  tear down global var

[shibumi]

I like the wrapper variant. This is better than passing a map. We could do it like the other big Golang libraries and just write recordArtifacts, for example os.Rename(): https://golang.org/src/os/file.go?s=9921:9963#L313

[shibumi]

@lukpueh I am not sure if visitedSymlinks = nil is enough for 'removing' the variable and tearing it down correctly. This should work, if we want to be sure I could also implement a RemoveAll function for our Set and call that one. 5min of Google said setting it nil should be enough.

[lukpueh]

Many thanks for implementing the wrapper approach and for adding extensive testing. I have one question left (inline). Let me know what you think.

[shibumi]

@lukpueh I've fixed the nil check for the visitedSymlinks hashset. Ping me if you find something else :)

[shibumi]

@lukpueh checks passed, ready to merge. When merged, I will use the current master as base for the intoto-run implementation.

[lukpueh]

Thanks for the quality contribution, @shibumi!