WIP fix possible path issues #62

asdf · 2019-10-27T12:37:54Z

asdf commented

2019-10-27 12:37:54 +00:00

This PR is intended to fix #52. It's a work in progress while checking.

This will be dependent on #60, and will need to be rebased once it is merged(??? - still trying to understand how different branches work together).

There are two examples of things I've found that can be addressed:

An actual file path issue - line 64 of main.go had a file path constructed using a fixed path separator. It has been replaced by filepath.Join() to build the file path.
A space saving change - lines 852-853 of client.go get the filename of a path by splitting and trimming. This has been replaced by path.Base.

This PR is intended to fix #52. It's a work in progress while checking. This will be dependent on #60, and will need to be rebased once it is merged(??? - still trying to understand how different branches work together). There are two examples of things I've found that can be addressed: 1. An actual file path issue - line 64 of main.go had a file path constructed using a fixed path separator. It has been replaced by `filepath.Join()` to build the file path. 2. A space saving change - lines 852-853 of client.go get the filename of a path by splitting and trimming. This has been replaced by `path.Base`.

asdf added this to the 2.0.0 milestone 2019-10-27 12:38:18 +00:00

asdf added the

non-functional

label 2019-10-27 12:38:45 +00:00

asdf self-assigned this 2019-10-27 12:38:49 +00:00

sloum was assigned by asdf

2019-10-27 12:38:49 +00:00

asdf commented

2019-10-27 13:10:13 +00:00

There are a few operations with variations on the following:

filename = filepath.Base(...)
...strings.Trim(filename, " 	
vfa")

Could some of these become a new function?

There are a few operations with variations on the following: ```` filename = filepath.Base(...) ...strings.Trim(filename, " vfa") ```` Could some of these become a new function?

sloum reviewed 2019-10-27 14:43:38 +00:00

client.go Outdated

						
				@ -851,3 +853,2 @@

						if u.DownloadOnly {

							nameSplit := strings.Split(u.Resource, "/")

							filename := nameSplit[len(nameSplit)-1]

							filename := path.Base(u.Resource)

sloum commented

2019-10-27 14:43:26 +00:00

This has a slight problem. path.Base, when given something like gopher://colorfield.space/1/~sloum/ will return ~sloum. When really we want it to return an empty string so that we can enter the following if block and give a default name (gophermap). However, this is still a problem even without the terminal slash. Which is what the previous logic was based around.

We may want to remove attempts to use a default filename as it seems more complicated than initially thought to figure out when that is the appropriate action. Part of me thinks: look for a . in the last path segment. If it is there it is a file name. If it isn't then use a default. But lots of files, particularly for unix based systems do not have file extensions (since they are convention and dont mean a lot to the system like they seem to on windows).

I definitely think using path.Base is probably the way to go. We just need to make some decisions on how to handle getting filenames. I think it possible that these decisions could simplify logic in a few places, which would be nice.

This has a slight problem. `path.Base`, when given something like `gopher://colorfield.space/1/~sloum/` will return `~sloum`. When really we want it to return an empty string so that we can enter the following if block and give a default name (gophermap). However, this is still a problem even without the terminal slash. Which is what the previous logic was based around. We may want to remove attempts to use a default filename as it seems more complicated than initially thought to figure out when that is the appropriate action. Part of me thinks: look for a `.` in the last path segment. If it is there it is a file name. If it isn't then use a default. But lots of files, particularly for unix based systems do not have file extensions (since they are convention and dont mean a lot to the system like they seem to on windows). I definitely think using path.Base is probably the way to go. We just need to make some decisions on how to handle getting filenames. I think it possible that these decisions could simplify logic in a few places, which would be nice.

main.go Outdated

						
				@ -62,3 +63,3 @@

					opts.WriteString(certs)

					return ioutil.WriteFile(bombadillo.Options["configlocation"]+"/.bombadillo.ini", []byte(opts.String()), 0644)

					return ioutil.WriteFile(filepath.Join(bombadillo.Options["configlocation"], ".bombadillo.ini"), []byte(opts.String()), 0644)

sloum commented

2019-10-27 14:38:42 +00:00

This is good 👍

This is good :thumbsup:

asdf commented

2019-10-28 12:27:14 +00:00

I've merged this with the new changes from develop and reverted the changes to client.go

I didn't see the issue with file names you had identified, and thought it was less complicated and might save some space.

We could take a look at how the file name is determined, but I'm not sure it will add much value. Even in the example you gave, ~sloum seems acceptable.

All paths used by ioutil.WriteFile are now created using filepath.Join. It also seems any errors arising (like if the directory doesn't exist, or a write error) will be displayed to the user.

Not sure there is much more to this one.

I've merged this with the new changes from develop and reverted the changes to client.go I didn't see the issue with file names you had identified, and thought it was less complicated and might save some space. We could take a look at how the file name is determined, but I'm not sure it will add much value. Even in the example you gave, ~sloum seems acceptable. All paths used by `ioutil.WriteFile` are now created using `filepath.Join`. It also seems any errors arising (like if the directory doesn't exist, or a write error) will be displayed to the user. Not sure there is much more to this one.

sloum commented

2019-10-28 14:35:11 +00:00

Ok. You are probably right that ~sloum would be fine. Is ~ an allowable file character on all of our target systems? Are there other characters that could be in a filename that are not valid? Should we url encode the filename just to be safe? What happens (I can test this in a bit when I get to work) if the file is the root of a directory? Will it save as hostname or as empty sting or as slash?

Ok. You are probably right that `~sloum` would be fine. Is `~` an allowable file character on all of our target systems? Are there other characters that could be in a filename that are not valid? Should we url encode the filename just to be safe? What happens (I can test this in a bit when I get to work) if the file is the root of a directory? Will it save as hostname or as empty sting or as slash?

asdf commented

2019-10-29 03:02:18 +00:00

Any file errors should occur within ioutil.WriteFile, and the client handles these nicely by aborting the write and printing the actual file error message so the user can interpret it.

With that said, we should avoid situations where the client suggests a file name that will cause an error. This can caused by:

How we determine the file name
Any instance of characters not allowed by the current OS.

The existing approach might be OK, with an expansion of strings.Trim(filename, " vfa") to also include characters not supported by the current operating system. We can improve file naming though, but we should think about some good inputs and expected output like:

Input	Expected
"https://rawtext.club"	rawtext.club.html
"gopher://colorfield.space:70/1"	gophermap
"gopher://colorfield.space:70/1/projects"	gophermap
"https://tildegit.org:443/sloum/stubb"	stubb.html
"gemini://gemini.conman.org:1965/"	gemini.conman.org
"gemini://gemini.conman.org:1965/news.txt"	news.txt

(I guess I am just suggesting TDD, I'm just not sure how else to be reasonably sure that it works as we expect)

Also, I just noticed that we are not handling overwrites. If a file exists and is writable, and the same name is used, it is overwritten without warning.

Any file errors should occur within `ioutil.WriteFile`, and the client handles these nicely by aborting the write and printing the actual file error message so the user can interpret it. With that said, we should avoid situations where the client suggests a file name that will cause an error. This can caused by: - How we determine the file name - Any instance of characters not allowed by the current OS. The existing approach might be OK, with an expansion of `strings.Trim(filename, " vfa")` to also include characters not supported by the current operating system. We can improve file naming though, but we should think about some good inputs and expected output like: Input | Expected --- | --- "https://rawtext.club" | rawtext.club.html "gopher://colorfield.space:70/1" | gophermap "gopher://colorfield.space:70/1/projects" | gophermap "https://tildegit.org:443/sloum/stubb" | stubb.html "gemini://gemini.conman.org:1965/" | gemini.conman.org "gemini://gemini.conman.org:1965/news.txt" | news.txt (I guess I am just suggesting TDD, I'm just not sure how else to be reasonably sure that it works as we expect) Also, I just noticed that we are not handling overwrites. If a file exists and is writable, and the same name is used, it is overwritten without warning.

sloum commented

2019-10-29 03:35:01 +00:00

Yeah. File overwrites are something I had been planning on fixing at some point and had never gotten around to (for my use case using Bombadillo I almost never save files, so when it was jsut a browser for me to use it wasnt high on the priority list). It would be nice to have.

I agree, tests to cover the various naming cases and url possibilities would be good.

this has some good discussion of character usage in filenames. Apparently anything but / is valid...sort of. But there are lots of good reasons to not allow a good many other characters.

Also, I just noticed: this PR doesn't seem to include adjustments to writing to files. Did things get reverted?

Yeah. File overwrites are something I had been planning on fixing at some point and had never gotten around to (for my use case using Bombadillo I almost never save files, so when it was jsut a browser for me to use it wasnt high on the priority list). It would be nice to have. I agree, tests to cover the various naming cases and url possibilities would be good. [this](https://stackoverflow.com/questions/457994/what-characters-should-be-restricted-from-a-unix-file-name) has some good discussion of character usage in filenames. Apparently anything but `/` is valid...sort of. But there are lots of good reasons to not allow a good many other characters. Also, I just noticed: this PR doesn't seem to include adjustments to writing to files. Did things get reverted?

asdf commented

2019-10-29 04:07:11 +00:00

I reverted the changes that used path.Base() because of the issues you pointed out. Another change was not required once mailcap was removed. Using filepath.Join() when saving the configuration file is the only other item I've found requiring attention.

I reverted the changes that used `path.Base()` because of the issues you pointed out. Another change was not required once mailcap was removed. Using `filepath.Join()` when saving the configuration file is the only other item I've found requiring attention.

sloum commented

2019-10-29 04:59:31 +00:00

Ok. I definitely think the join usage for the config works great. Do you think we should try to make file save adjustments as a part of this PR? Or merge this in?

Ok. I definitely think the `join` usage for the config works great. Do you think we should try to make file save adjustments as a part of this PR? Or merge this in?

asdf commented

2019-10-29 10:03:16 +00:00

I'll merge this in. It would be good to think about other changes a bit more, but I'll raise an issue for now.

asdf closed this pull request

2019-10-29 10:03:55 +00:00

asdf deleted branch fix-path-issues

2019-10-29 10:04:06 +00:00

Sign in to join this conversation.

No reviewers