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

Add a 'target' field for symlinkattributes #76

Closed
wants to merge 4 commits into from
Closed

Conversation

hishamhm
Copy link
Member

It returns the resolved path of the symlink.

Since this adds functionality, this should be merged in the 1.7 branch. PRing for review.

elikosan and others added 3 commits June 1, 2016 17:52
* Make lfs export luaopen_lfs under Windows
It returns the resolved path of the symlink.
@coveralls
Copy link
Collaborator

coveralls commented Jun 20, 2016

Coverage Status

Coverage increased (+1.04%) to 80.102% when pulling 2e068eb on symlink_target into 81e5b16 on 1.7.

@hishamhm
Copy link
Member Author

(I don't know why these other commits 6e0dc7b and 7dae11c ended up in this PR too, but I guess they should be in 1.7 too)

@coveralls
Copy link
Collaborator

coveralls commented Jun 20, 2016

Coverage Status

Coverage increased (+1.09%) to 80.151% when pulling a1015fe on symlink_target into 81e5b16 on 1.7.

*/
static int push_link_target(lua_State *L) {
#ifdef _WIN32
errno = ENOSYS;

Choose a reason for hiding this comment

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

Why not implement on windows?

Copy link
Member Author

Choose a reason for hiding this comment

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

Be my guest! :)

Choose a reason for hiding this comment

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

So that implies it was just a lack of time rather than a technical obstacle?

Copy link
Member Author

Choose a reason for hiding this comment

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

...and interest — I usually don't do Windows coding for free :)

@hishamhm
Copy link
Member Author

For cross-referencing: I noticed that this closes #43.

@mpeterv
Copy link
Contributor

mpeterv commented Jun 21, 2016

(I don't know why these other commits 6e0dc7b and 7dae11c ended up in this PR too, but I guess they should be in 1.7 too)

Probably because I haven't merged master into 1.7 yet and this PR's branch was created from master.

@mpeterv
Copy link
Contributor

mpeterv commented Jun 21, 2016

Not sure what is going on with LuaJIT failures, see mpeterv/hererocks#27.

@mpeterv
Copy link
Contributor

mpeterv commented Jun 25, 2016

@hishamhm I don't like that to getting size for readlink requires indexing a table that was just built. Maybe could return stat struct (using pointer argument) to make post-processing easier?

@mpeterv
Copy link
Contributor

mpeterv commented Jun 25, 2016

Alternatively, it should be possible to use existing mechanism for declaring functions pushing fields. But then there has to be a way to use different field lists for stat and lstat, and also these pushing functions should be able to signal failure.

@n1tehawk
Copy link
Contributor

n1tehawk commented Jul 15, 2016

I'd like to suggest a different approach for determining the buffer size:

Start with a reasonable initial value (say 256 or 512), and check the readlink() return value against it. If that suggests that the buffer may have been filled completely, realloc() it to a bigger size and retry. See n1tehawk@3497406#diff-f6b7bcefa5f97fba6fa9e82b43357724R865 ...

If you wish, I'll create a pull request for this.
Regards, NiteHawk

[EDIT] P.S.: I realized that size - 1 isn't needed, and fixed my branch accordingly: n1tehawk@4185914

@hishamhm
Copy link
Member Author

@n1tehawk yes, this PR would be welcome!

@n1tehawk
Copy link
Contributor

See #78.

Compared to my previous suggestion, I have added proper readlink() error handling. Also: The size restriction was too conservative/narrow without need - since we'll only write target[tsize] if (tsize < size), it's safe to use size for the readlink() call directly (instead of size - 1).

Regards, NiteHawk

@n1tehawk
Copy link
Contributor

This PR was superseded by merging #78, and should be closed.

Regards, NiteHawk

@mpeterv mpeterv closed this Oct 14, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants