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

Fix #749, #620: implement lchown() #752

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrewkoung
Copy link
Contributor

Let me know if my logic was wrong, but I believe this was the correct flow.

@codecov-io
Copy link

codecov-io commented Apr 7, 2019

Codecov Report

Merging #752 into master will increase coverage by 0.03%.
The diff coverage is 87.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #752      +/-   ##
==========================================
+ Coverage   86.88%   86.92%   +0.03%     
==========================================
  Files          16       16              
  Lines        1739     1751      +12     
==========================================
+ Hits         1511     1522      +11     
- Misses        228      229       +1
Impacted Files Coverage Δ
src/filesystem/interface.js 93.29% <ø> (ø) ⬆️
src/filesystem/implementation.js 84.21% <87.09%> (+0.08%) ⬆️

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 4aae538...34434b0. Read the comment docs.

Copy link
Member

@modeswitch modeswitch left a comment

Choose a reason for hiding this comment

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

Quick first pass. @humphd I haven't reviewed the implementation itself.

src/filesystem/implementation.js Outdated Show resolved Hide resolved
src/filesystem/implementation.js Outdated Show resolved Hide resolved
src/filesystem/implementation.js Outdated Show resolved Hide resolved
src/filesystem/interface.js Outdated Show resolved Hide resolved
src/filesystem/implementation.js Outdated Show resolved Hide resolved
@humphd humphd changed the title implementing lchown() Fix #749, #620: implement lchown() Apr 10, 2019
@humphd
Copy link
Contributor

humphd commented Apr 10, 2019

Updated the title of this PR, since it will fix #749, #620.

Copy link
Contributor

@humphd humphd left a comment

Choose a reason for hiding this comment

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

This is looking really good. One thing it lacks is any kind of testing for the central difference between this and chown, namely, tests that work with symlinks and make sure that when you all chown it changes the referenced file, and when you call lchown, it changes the symlink node instead. I'd add some tests that basically do this:

  • create a file named /file
  • create a symlink to /file called /link
  • chown /link` to 500 500 or something
  • lchown /link to 600 600 or something (different from the first)
  • call stat on /link and expect the uid/gid to match 500
  • call lstat on /link and expect the uid/gid to match 600

I'd break all that up into two separate tests, one for stat/chown and the other for lstat/lchown. This way we'll know that they work as expected on the proper node.

Great work on this.

src/filesystem/interface.js Outdated Show resolved Hide resolved
@andrewkoung
Copy link
Contributor Author

There happens to be a test for chown() already, but I've created the test for lchown()!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants