otiai10 / copy

Go copy directory recursively
https://pkg.go.dev/github.com/otiai10/copy
MIT License
713 stars 115 forks source link

Fix symlink relative path resolution #80

Closed LorisFriedel closed 8 months ago

LorisFriedel commented 2 years ago

fixing #26

codecov-commenter commented 2 years ago

Codecov Report

Merging #80 (0b1f27b) into main (528e2c9) will decrease coverage by 0.12%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #80      +/-   ##
==========================================
- Coverage   77.40%   77.27%   -0.13%     
==========================================
  Files          13       13              
  Lines         177      176       -1     
==========================================
- Hits          137      136       -1     
  Misses         20       20              
  Partials       20       20              
Impacted Files Coverage Δ
test_setup.go 100.00% <ø> (ø)
copy.go 75.51% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 528e2c9...0b1f27b. Read the comment docs.

otiai10 commented 2 years ago

Can you please write a new test case that fails with previous version using os.Readlink then we can make sure the exact problem your code will fix?

LorisFriedel commented 2 years ago

Can you please write a new test case that fails with previous version using os.Readlink then we can make sure the exact problem your code will fix?

Hello @otiai10 ! I'm not sure to understand what you want me to do?

The fix here is that os.Readlink doesn't take into account the symlink file location and the test was not representative of a real use case because the dynamically creating symlink was relative to where the test was executed and not the symlink itself.

PS: for the test coverage verification to pass, do I need to increase it by creating some more tests or is there another way?

Thank you!

otiai10 commented 2 years ago

I want you to add/fix test cases to let it fail correctly.

Then, fix the source code.

That's my request. Is it possible?

(please don't care about the coverage ;)

LorisFriedel commented 2 years ago

I want you to add/fix test cases to let it fail correctly.

Then, fix the source code.

That's my request. Is it possible?

(please don't care about the coverage ;)

@otiai10 I don't know how (without duplicating the code) I could include in the PR a test case that test the old implementation with os.ReadLink AND fix it at the same time?

I'm a bit confused here :/

otiai10 commented 2 years ago

Separate commits, for example

millerlogic commented 1 year ago

Can we please have this fix? Symlink deep copy is broken. Does the new test not fail on the existing code?