Added write permissions feature to Dir section

jskrnbindra commented 5 years ago

Implement write permissions feature in the Dir section which shows a lock symbol (🔒) when:

The current directory is write protected; or
The current user does not have permissions enough to write to the directory

Note: Requires Powerline patched fonts to be installed.

Motivation and Context

Closes #75

Types of changes

[ ] Bug fix (non-breaking change which fixes an issue)
[x] New feature (non-breaking change which adds functionality)
[ ] Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

[x] I have tested using MacOS
[ ] I have tested using Linux

Checklist:

[x] I have checked that no other PR duplicates mine
[x] My code follows the code style of this project.
[x] My change requires a change to the documentation.
[x] I have updated the documentation accordingly.
[x] I have updated the tests accordingly.

jskrnbindra commented 5 years ago

@matchai A few tests are failing in AppVeyor but pass in my local machine. Is this because I'm doing chmod anyhow? 🤔

matchai commented 5 years ago

@jskrnbindra Hmm... Strange indeed. 🤔 Could it be because of Cygwin user groups? https://stackoverflow.com/a/19779410

Snuggle commented 5 years ago

Just going off your screenshot, are there two spaces here or is it just your terminal font having weird kerning for emoji?

jskrnbindra commented 5 years ago

@matchai About the AppVeyor's chmod thing. I tried:

chown :Users testDir

and:

setfacl -s user::r-x,group::---,other::--- testDir

from here as AppVeyor is using Cygwin v2.889. If I try digging further it adds dirty commits to the PR. (Need two remove all commits after f07ae94)

The issue seems to be with Cygwin because the TavisCI build passes smoothly. Also, why are we using two CI systems? 🤔 Can we make either of the builds passing as accepted (i.e. not forcing both builds to pass, if any of these passes it's okay)?

jskrnbindra commented 5 years ago

Just going off your screenshot, are there two spaces here or is it just your terminal font having weird kerning for emoji?

No these are not two spaces. It's my terminal displaying this Powerline font as such:

The lock emoji in the screenshot above is just the emoji between the quotes (and no space). The space after the lock symbol is within this emoji.

Is there a fix/workaround for this issue? @Snuggle

matchai commented 5 years ago

The reason we have two CI systems is to cover all supported OSes. Travis tests Linux and macOS while AppVeyor tests Windows. 🍎🐧

Don't worry about dirtying the commit history. We squash-merge, so this PR will be recorded as one commit. 👍

With regards to your terminal showing extra space, I personally use iTerm 2. Under Profiles > Text > Unicode, you can enable Use Unicode version 9 widths.

jskrnbindra commented 5 years ago

@matchai I tried a lot of ways. (chgrp, chown, setfacl, etc.) But AppVeyor's filesystem (NTFS) does not seems to make a directory write protected. I tried modifying users, owners and groups but no luck.

A simple chmod was able to update the directory permissions as expected but only superficially. For example, running

chmod 400 testDir

or

chmod -w testDir

would show: dr-x------+ 1 appveyor Users 0 Oct 27 09:44 testDir as output of ls -lahrt. But when I cd into testDir and try to create a file, it creates it. and even echo ([ -w . ]; and echo "writable") ran inside testDir (after running the above chmod) prints 'writable' to the screen.

For Windows (Cygwin) we need to find out a way to:

make the dir read-only
see if a directory is read-only.

Cygwin docs suggest using setfacl command line tool to change permissions for files but I was unable to run this command in AppVeyor's environment.

jskrnbindra commented 5 years ago

I think we should put a "help-wanted" label on this.

jskrnbindra commented 5 years ago

Should we use some hacky way of doing this? Like looking at the output of ls -lahrt to see if the directory is write protected or not?

Or should we drop this feature off on Windows for now?

matchai commented 5 years ago

I'd like to do a little bit of testing on my personal Cygwin machine to see if I could maybe get this working before we decide to have features only enabled for some terminal emulators.

I'll try to get back to this PR in the next few days after I'm done traveling ✈️

Snuggle commented 5 years ago

Why Cygwin? Isn't WSL the standard for running Linux stuff on Windows now?

matchai commented 5 years ago

I'm personally running Windows 10 LTSB, which has no support yet for WSL yet; so a little bit for selfish reasons 😉.

EDIT: Whoops! Looks like I'm wrong: https://docs.microsoft.com/en-us/windows/wsl/install-manual It would still be nice to keep Cygwin supported for users that aren't on Win10, but if it's a headache, I suppose we can drop those tests.

Snuggle commented 5 years ago

Windows Subsystem for Linux seems much much nicer. I just don't know of a way to test for it, do you?

jskrnbindra commented 5 years ago

Considering WSL, testing shouldn't be any different from testing on Linux. So if a test passes on linux it will pass in WSL.

Snuggle commented 5 years ago

In theory, yup! WSL seems much more important than Cygwin though.

matchai commented 5 years ago

I don't believe that any CI providers at the moment support nested virtualization for free for OSS: https://github.com/appveyor/ci/issues/1295#issuecomment-412597732

kulabun commented 5 years ago

I think that since it is a cygwin bug, the best way is just to ignore cygwin.

if test (uname -s) != "CYGWIN"
   echo $SPACEFISH_DIR_LOCK_SYMBOL
end

matchai commented 5 years ago

I've been holding up this PR for too long. I suppose it's fine if we ignore Cygwin when testing this section for the time being. @jskrnbindra if you can get this to a passing state, I'll go ahead and give it another look over. 👍

matchai commented 5 years ago

It passes! 🙌 Thank you for taking the time to get this working @jskrnbindra 😄 You may just want to rebase master since your branch is out of date.

I'll get started with reviewing it.

jskrnbindra commented 5 years ago

🎉🙃

matchai commented 5 years ago

Thanks again @jskrnbindra! I know this took a bit of a while, but I'm glad you stuck with it. 😄

Snuggle commented 5 years ago

Oh gosh, this PR got merged seconds before I managed to get my review in time, heh. 😅 Battle plan?

matchai commented 5 years ago

Whoops! I'll quickly commit your suggested changes to master. 😅

jskrnbindra commented 5 years ago

Lost all the permissions to modify on this PR now. ⚓️

matchai / spacefish