Utilize canonicalize() to expand . and .. characters in Paths

[rbisewski]

As per issue #34, this pull request uses the canonicalize() function to handle and resolve . and .. characters

The canonicalize functionality of Rust is a bit tricky and has (possibly?) differing behavior between the OSes and requires a number of careful checks to ensure that errors do not normally occur (since canonicalize() will throw errors if a folder or file does not exist, etc), along with some testing to confirm that even complicated paths like /tmp/./../tmp/abc.png work safely.

As a last resort if some or all of the canonalization logic causes errors or empty strings, then the default is to try the original non-canonicalized path.

Please note I only really tested this on Linux, so feel free to test it on MacOS or Windows if you have them.

[rbisewski]

Fair enough, your code is simpler in a number of ways so we can use it, though I'd prefer to keep the early return for the ~ path; but up to cloudhead ultimately I suppose.

Now that said, part of this is due to the fact that I only learned Rust during Hacktober the other day and thus am unfamiliar with how to deal with unique Rust constructs like Option or Result; hence any help is much appreciated, so thanks for going above and beyond :smile:

Also my understanding is, unlike the Path module of Rust, canonicalize() is OS-level and thus will fail in all of the common ways a filesystem fails; e.g. if the directory doesn't exist or the file doesn't exist or the file has bad permissions.

Based on a quick test, the e and w commands will actually create a file if it doesn't exist, so we can't afford to let canonicalize() fail here if the user wants to open or save a file that doesn't yet exist. Ergo, we probably want to canonicalize the directory separate from the filename.

In fact, we may also simply want to return early if a directory cannot be canonicalized.

In addition, on the filename side of things, we probably want to check if the given path is simply a directory and not a file, and we will need to check that the filename is valid in other ways. Right now ~/abc.png/. will pass right through this function unimpeded (saving to ~/abc.png), but I think we'd both agree that ~/abc.png/. isn't a valid file and we ought to pass back an error to the end-user, no?

All of that said, there are still a number of edge cases the suggested logic won't handle and will cause rx to fail. A few of them that immediately come to mind...

• The command e /tmp/ will crash the program
• The command w ~/abc.png/. will save to ~/abc.png
• The command e ~/abc.png/.. will open a new file but be unable to save
• The command e ~/a.. will open a new file but will save the contents to ~/a..png

Anyway, I'll spend a bit of time to improve this later in the week since we really need to handle all of the edge cases this can fail due to user-error.

[roman-holovin]

Now that said, part of this is due to the fact that I only learned Rust during Hacktober the other day and thus am unfamiliar with how to deal with unique Rust constructs like Option or Result; hence any help is much appreciated, so thanks for going above and beyond smile

Awesome, keep at it :+1:

Based on a quick test, the e and w commands will actually create a file if it doesn't exist, so we can't afford to let canonicalize() fail here if the user wants to open or save a file that doesn't yet exist. Ergo, we probably want to canonicalize the directory separate from the filename.

I see, that makes sense.

In fact, we may also simply want to return early if a directory cannot be canonicalized.

Maybe providing error, in this case, is the good thing to do. vim, for example, is a little annoying in a way, that you can specify any path you want on :e, but it will fail to save a file later if some intermediate directory does not exist.

[rbisewski]

I've also found another issue with the e command, specifically opening a file without a .png extension will open a new view but ignore and existing file of the same name with a .png extension. Adding a check seems to solve this.

In the interest of keeping the changeset small, I think I am done for now, but I will note there are still two problems I'd like to handle as separate issues:

• The command e /tmp/ will crash the program; this seems to be a deep issue related to filesystem permissions and was failing even before the use of canonicalize(). I think making that a separate issue is probably a better idea than addressing it here.
• The command w ~/abc.png/. will save to ~/abc.png; this seems to be a feature not a bug of the file_name() function, specifically that is equates abc.png as being equal to abc.png/. in spite of this conflicting with Linux shells. Probably this can be a separate issue since it is really more a debate about filesystem or program philosophy more than anything else, and it doesn't cause any crashes.

Assuming that's alright with you Cloudhead, feel free to review my changeset when you have time and then merge it.

[rbisewski]

Thanks again for your code review :)

I think this resolves the issue, but let me know if you see anything else.

[cloudhead]

Looks much better!

[cloudhead]

Great, this is looking good - just two minor suggestions and it's good to go!

[rbisewski]

Thanks again, I have since committed the changes you requested.

Feel free to merge it once you think it's ready.

[cloudhead]

Perfect, thank you!