[RFC] On windows don't check for exec bit in is_executable()

[sethjackson]

In Windows there is not equivalent to the filesystem executable bit,
the documentation states that for Windows :executable() returns
1 for all files. But this behaviour was broken because is_executable()
checked for the UNIX bit.

When WIN32 is set we now skip the S_IXUSR check.

Cherry picked from #810.

[justinmk]

@equalsraf is it certain that libuv doesn't fix this already?

[equalsraf]

Last time I checked libuv does not return the exec bit as set (nor should it). We can also change the macro to always return true.

Some context: is_executable is only used in is_executable_in_path. In windows all files that match $PATHEXT are executable (db17263 in #810 did that in is_executable_in_path).

[justinmk]

In windows all files that match $PATHEXT are executable

Even in the NT kernel? I was thinking this was left over from Vim windows 95 assumptions.

[equalsraf]

Even in the NT kernel? I was thinks this was left over from Vim windows 95 assumptions.

Don't know, but PATHEXT is still defined in my system and contains .COM,.BAT,.EXE and also scripts extensions like .rb for ruby.

[justinmk]

Well I'll be a kitten's mitten. I just created a text file in Windows 7, invoked it it in cmd.exe, and it tried to execute every line of the text file.

If I disable the "Read & Execute" setting in the file's "Security" properties, then "Read" is also disabled. From MSDN:

Read is the only permission needed to run scripts. Execute permission doesn't matter.

[justinmk]

Could do this instead:

return os_access(name, F_OK);

Then os_getperm is not needed for the WIN32 branch. This will require defining F_OK in win_defs.h:

#  define F_OK 0

See justinmk@b1e18dc

[justinmk]

Oops we don't have os_access, we use uv_fs_access directly. So never mind I guess.

[sethjackson]

Hmm the Vim docs say this:

On MS-DOS and MS-Windows it only checks if the file exists and
is not a directory, not if it's really executable.

So the original diff should suffice no?

Eg: even if I remove Read & Execute (and therefore Read) permissions
and then do this in Vim:

:echom executable("test.bat")

Vim will still return 1 even though I can't run said Batch script from a Windows shell.

[justinmk]

Vim will still return 1 even though I can't run said Batch script from a Windows shell.

If we can make executable() do something actually useful on Windows, then it doesn't matter what Vim's behavior is. So os_file_is_readable() seems like a good idea to me.

[sethjackson]

Ok. Works for me. Will update the docs too.

[justinmk]

remove MS-DOS

[justinmk]

@sethjackson The commits in #810 might be in WIP state, so please note the commit guidelines at https://github.com/neovim/neovim/blob/master/CONTRIBUTING.md#commit-guidelines

[justinmk]

Sorry, I should have read the docs more carefully. We do need to use S_ISREG because that checks that the file is not a directory.

[sethjackson]

Yea. I just realized that after I pushed. :/

Also there appears to be this weird problem with uv_fs_access on Windows where if you request anything but W_OK it short circuits and returns 0 without actually checking the file attributes which means os_is_file_readable always returns true even if the user doesn't have permission to access the given file. :(

Personally I feel that's a bug in the libuv implementation so perhaps a libuv PR is in order?
See: https://github.com/libuv/libuv/blob/df62b54aa2b3d916ca442ee84f0678129a848c4f/src/win/fs.c#L1346

[justinmk]

@sethjackson It would be worth opening an issue at libuv at least to get confirmation. They are very responsive.

[justinmk]

Check this issue first though: libuv/libuv#316

[justinmk]

I think the logic is ok, can you give a scenario where it would be wrong? The call to GetFileAttributes() above that will fail if the file doesn't exist.

[justinmk]

Merged f34a264. Thanks for helping with #810!

Now we actually need to define S_ISREG ;)

[sethjackson]

@justinmk ok. Will probably open an issue but I might check what CPython does first. ;)

And yea I'm hoping that all the stuff for win_defs.h can be squashed into one commit. There is a bunch of stuff in there...

[justinmk]

I'm hoping that all the stuff for win_defs.h can be squashed into one commit.

Most of it is here: equalsraf@d02f0d6

But I am not sure all of it is needed, it would be better to include each part with the relevant function changes. For example, R_OK and W_OK were needed for the os_file_is_readable PR, so they were grouped into #3166.