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

[RDY] Replace `struct stat` with `FileInfo` #619

Merged
merged 10 commits into from May 9, 2014

Conversation

Projects
None yet
6 participants
@stefan991
Copy link
Contributor

stefan991 commented Apr 25, 2014

mch_stat is used very often in the codebase. I think it is the best to refactor all uses of mch_stat and struct stat into functions in os/fs.c. This pull request won't probably refactor every use of mch_stat, but it is a good start.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Apr 25, 2014

At the very least it makes things clearer, and that gets a big thumbs up, +1!

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented Apr 27, 2014

Link to the now hidden discussion about the type of file_sizes: stefan991@6b60262#src-os-fs-c-P4

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented May 3, 2014

This got bigger than I thought it would be, so for now this replaces only the usage of struct stat with FileInfo. FileInfo.stat usage outside of 'src/os/' should be refactored in other pull requests.
Making this [RFC] now.

@stefan991 stefan991 changed the title [WIP] Cleanup usage of mch_stat() [RFC] Replace struct stat with FileInfo May 3, 2014

@stefan991 stefan991 changed the title [RFC] Replace struct stat with FileInfo [RFC] Replace `struct stat` with `FileInfo` May 3, 2014

@lslah

This comment has been minimized.

Copy link
Contributor

lslah commented May 3, 2014

Apart from the two missing negations this LGTM. Great job @stefan991! 👍

/* if original file does exist throw away the copy */
if (mch_stat((char *)fname, &st) >= 0)
if (!os_file_exists(fname)) {

This comment has been minimized.

@Hinidu

Hinidu May 3, 2014

Contributor

This should be negated

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

{
struct stat st;
FileInfo file_info;

This comment has been minimized.

@Hinidu

Hinidu May 3, 2014

Contributor

You can declare file_info in if (file_info_p == NULL) scope

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

It seems that accessing file_info through a pointer outside of the scope results in undefined behavior. See http://stackoverflow.com/questions/2759371/in-c-do-braces-act-as-a-stack-frame
I would leave it as it is just to be sure.

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

Thanks! I will keep it in mind.

@@ -1575,7 +1570,8 @@ void write_viminfo(char_u *file, int forceit)
* Check if tempfile already exists. Never overwrite an
* existing file!
*/
if (mch_stat((char *)tempname, &st_new) == 0) {
FileInfo new_info; // FileInfo of potential new file
if (os_get_file_info((char *)tempname, &new_info)) {

This comment has been minimized.

@Hinidu

Hinidu May 3, 2014

Contributor

new_info is not used so os_get_file_info can be replaced with os_file_exists

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

if ((url && fnamecmp(vp->ffv_fname, ff_expand_buffer) == 0)
|| (vp->ffv_dev_valid
&& vp->ffv_dev == file_info.stat.st_dev
&& vp->ffv_ino == file_info.stat.st_ino)) {

This comment has been minimized.

@Hinidu

Hinidu May 3, 2014

Contributor

This condition does not have the same meaning as before. Now if url == true and fnamecmp(vp->ffv_fname, ff_expand_buffer) != 0 then (vp->ffv_dev_valid && vp->ffv_dev == file_info.stat.st_dev && vp->ffv_ino == file_info.stat.st_ino) would also be checked. In the ternary operator version just one of these subconditions can be checked.

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍 added !url && to the RHS of the or.

int lstat_res;

lstat_res = mch_lstat((char *)fname, &st);
bool file_info_link_ok = os_get_file_info((char *)fname, &file_info);

This comment has been minimized.

@Hinidu

Hinidu May 3, 2014

Contributor

Should we use here os_get_file_info_link?

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

@@ -189,6 +189,16 @@ int os_file_is_writable(const char *name)
return 0;
}

bool os_get_file_size(const char *name, off_t *size)

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

Is there any reason why you did not use off_t &size? (The same for other functions' file_info parameter)

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

C dosn't support references, they were added in c++.

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

What an unfortunate mistake... Sorry :-)

src/os/os.h Outdated
@@ -58,6 +58,12 @@ bool os_file_is_readonly(const char *name);
/// @return `2` for a directory which we have rights to write into.
int os_file_is_writable(const char *name);

/// Get the size of a file in bytes.
///
/// @param size pointer to an off_t to put the size into.

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

Doxygen have additional flags to specify direction of the parameter: http://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdparam
For example in this case you can write @param[out] size
I think it is good to clarify direction on pointer arguments.

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

src/os/os.h Outdated

/// Get the file information for a given path without following links
///
/// @param file_descriptor File descriptor of the file.

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

Perhaps copy-paste error: os_get_file_info_link has path(not the file_descriptor) parameter.

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

describe 'os_get_file_info', ->
it 'returns false if given an non-existing file', ->
file_info = fileinfo_new!
assert.is_false (fs.os_get_file_info '/non-existent', file_info)

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

I think eq false is better for consistency with other tests.

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

see comment from @lslah on another pull request: #313 (comment)
I agree with @lslah about this.

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

I'm ok with both variants. I think we must choose one and specify that choice somewhere.


it 'returns true if given an existing file and fills file_info', ->
file_info = fileinfo_new!
file_info[0].stat.st_ino = 0

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

Maybe better to do it in fileinfo_new?(To avoid repetitions)

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

file_info[0].stat.st_ino = 0
path = 'unit-test-directory/test.file'
assert.is_true (fs.os_get_file_info path, file_info)
assert.is_true file_info[0].stat.st_ino > 0

This comment has been minimized.

@Hinidu

Hinidu May 4, 2014

Contributor

What do you think about incapsulating this line in the function is_file_info_filled and perhaps adding to it some other checks(st_dev for example)?

This comment has been minimized.

@stefan991

stefan991 May 4, 2014

Contributor

👍

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented May 4, 2014

LGTM 👍 Good work!

*size = statbuf.st_size;
return true;
}
return false;

This comment has been minimized.

@jszakmeister

jszakmeister May 7, 2014

Member

There seems to be a lot of true/false happening in these os functions. Wouldn't it be better to let the actual error propagate back to the caller? It may not be necessary for the current code paths, but it seems generally more useful. Otherwise we can't inform the user what went wrong... only that something failed. :-(

Thoughts?

This comment has been minimized.

@aktau

aktau May 7, 2014

Member

I'm all for getting more info to the user, especially if it doesn't complicate calling patterns. Since we seem to be exposing libuv anyway (through os_stat for example), perhaps it's not such a big problem to bring in libuv'isms into this.

Then again, I've always had a weak spot for how to linux kernel does it: return a signed number where negative values indicate an error, and positive ones mean success (like read()).

This comment has been minimized.

@stefan991

stefan991 May 7, 2014

Contributor

This pull request exposes uv_stat_t, but this should be private in the future. I did expose this to make the conversation to libuv easier and to split the refactorings to later pull requests. I don't think we should expose libuv outside of /src/os/.
Libuv doesn't return error codes directly, but instead you have to call uv_last_error(loop) (see: http://nikhilm.github.io/uvbook/basics.html#error-handling). I think we should implement error handling, but I don't think we should use libuv error codes directly, because we should be able to handle errors in a consistent way, even if they are not from libuv.

This comment has been minimized.

@aktau

aktau May 7, 2014

Member

I think we should implement error handling, but I don't think we should use libuv error codes directly, because we should be able to handle errors in a consistent way, even if they are not from libuv.

That should be preferable of course (if it's not too complex).

This comment has been minimized.

@stefan991

stefan991 May 7, 2014

Contributor

I created a separate Issue for this: #698

@stefan991

This comment has been minimized.

Copy link
Contributor

stefan991 commented May 7, 2014

Rebased, marking [RDY] now.

@stefan991 stefan991 changed the title [RFC] Replace `struct stat` with `FileInfo` [RDY] Replace `struct stat` with `FileInfo` May 7, 2014

stefan991 added some commits May 3, 2014

justinmk added a commit that referenced this pull request May 9, 2014

Merge pull request #619 from stefan991/mch_stat-cleanup
Replace `struct stat` with `FileInfo`

@justinmk justinmk merged commit 1a3ee71 into neovim:master May 9, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@justinmk

This comment has been minimized.

Copy link
Member

justinmk commented May 9, 2014

LGTM. Great work @stefan991

if ((filesize = lseek(fileno(fp),
(off_t)0L, SEEK_END)) <= 0)
// Get the tag file size.
if ((filesize = lseek(fileno(fp), (off_t)0L, SEEK_END)) <= 0) {

This comment has been minimized.

@justinmk

justinmk May 10, 2014

Member

@stefan991 The comment above indicates that lseek was being used for portability. Any reason to continue doing it that way, instead of os_get_file_size()?

This comment has been minimized.

@stefan991

stefan991 May 10, 2014

Contributor

@justinmk os_get_file_size() wants a path to a file instead of a file descriptor.
os_file_get_info_fd() could be used instead, but then this would take 3 LOC.
Also I'm not sure if seek and stat on a file descriptor are consistent with each other.
So I decided to leave this lseek()-version because this should be simple and clean enough.

Grimy pushed a commit to Grimy/neovim that referenced this pull request Jan 7, 2015

Clarify error about Vim without python 2 support
Should be less confusing to some users. Related to issue neovim#619.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment