Fixes: When saving files, it's easy to overwrite an existing file #73

sloum · 2019-11-03T03:23:01Z

sloum commented

2019-11-03 03:23:01 +00:00

Relates to, and would hopefully close, #68

This checks if a file already exists with the save name. If it does it adds .# where # is an int that defaults to one and iterates until it is able to make a file without overwriting anything. The code looks a little gross, but works well. I had a version where I tried to look for a file extension and split that off and put the number before the extension... but laziness won out.

I also do not love the code duplication. I could probably move it into a function. You know what, I will do that... but I have to feed a baby first. Sooooo I;m gonna make the pull request then come back and move it into a function. In the meantime if you try it out and think another approach would be better, let me know.

Let me know if you think this works alright.

Relates to, and would hopefully close, #68 This checks if a file already exists with the save name. If it does it adds `.#` where `#` is an int that defaults to one and iterates until it is able to make a file without overwriting anything. The code looks a little gross, but works well. I had a version where I tried to look for a file extension and split that off and put the number before the extension... but laziness won out. I also do not love the code duplication. I could probably move it into a function. You know what, I will do that... but I have to feed a baby first. Sooooo I;m gonna make the pull request then come back and move it into a function. In the meantime if you try it out and think another approach would be better, let me know. Let me know if you think this works alright.

asdf was assigned by sloum

2019-11-03 03:23:01 +00:00

sloum self-assigned this 2019-11-03 03:23:01 +00:00

sloum added the

enhancement

label 2019-11-03 03:23:01 +00:00

asdf commented

2019-11-03 03:50:29 +00:00

Thanks for taking care of this one.

Is it possible for this to loop indefinitely due to another error like permissions?

The suffix seems to only be used in the loop, should it be declared like:

for suffix := 1; err == nil {
...
}

Also I agree with a new function.

Thanks for taking care of this one. Is it possible for this to loop indefinitely due to another error like permissions? The suffix seems to only be used in the loop, should it be declared like: ```go for suffix := 1; err == nil { ... } ``` Also I agree with a new function.

sloum commented

2019-11-03 04:27:55 +00:00

I think the answer is yes to both. I'll try to check for the specific error and I'll move the iterator into the function. I had been thinking of it like a while loop, but really I can just treat it as a regular for loop.

sloum commented

2019-11-03 05:18:22 +00:00

I have updated the code to:

Use a function
Use a better for loop in that function
Check for other types of errors

Of note is that I am actually ignoring the errors generated by the function since they will be raised immediately afterward by the attempt to write to the file. As such, it seemed silly to write messaging that is the same as the messaging that would be done anyway immediately after. I added a code comment to this effect.

I kept the error return on the function though in case it is useful elsewhere at some point. It feels like it should return an error, that error just isnt that useful in the specific context of where the function is currently being called.

I have updated the code to: - Use a function - Use a better for loop in that function - Check for other types of errors Of note is that I am actually ignoring the errors generated by the function since they will be raised immediately afterward by the attempt to write to the file. As such, it seemed silly to write messaging that is the same as the messaging that would be done anyway immediately after. I added a code comment to this effect. I kept the error return on the function though in case it is useful elsewhere at some point. It feels like it _should_ return an error, that error just isnt that useful in the specific context of where the function is currently being called.

asdf commented

2019-11-04 02:12:59 +00:00

This seems to work pretty well. I tried the following:

test	result
save to a file that exists	increments successfully
save to a file that doesn't exist	saves file
save to a file that is read only	increments successfully
save to a directory that is read only	permission denied

There is some interesting behaviour, partially related to this but more related to how the download path is constructed:

test	command	result
save to root of filesystem	`:w . /`	saves file as `~/.1`
save to /root directory	`:w . /root`	saves `~/root.1`
save to /root directory	`:w . /root/`	tries to save to `~/root/.1`, fails on write as there is no directory

The directory thing makes sense, as download paths are all relative. I'll raise another issue for that. Should we allow an empty file name though?

This seems to work pretty well. I tried the following: test | result --- | --- save to a file that exists | increments successfully save to a file that doesn't exist | saves file save to a file that is read only | increments successfully save to a directory that is read only | permission denied There is some interesting behaviour, partially related to this but more related to how the download path is constructed: test | command | result --- | --- | --- save to root of filesystem | `:w . /` | saves file as `~/.1` save to /root directory | `:w . /root` | saves `~/root.1` save to /root directory | `:w . /root/` | tries to save to `~/root/.1`, fails on write as there is no directory The directory thing makes sense, as download paths are all relative. I'll raise another issue for that. Should we allow an empty file name though?

sloum commented

2019-11-04 05:57:33 +00:00

We are set up to allow empty filename. We are not set up for pathing of any kind. The third param there is meant to be a file name, not a file path. The man page explicitly states to not include a leading /. That said, we are not doing anything to prevent that from being passed. I have not built out any checks to make sure it is a valid filename.

At the VERY least we should be checking for path elements and returning an error. A filename cannot contain a /, so a simple check for the string containing one is a simple solution. That said, it seems like a bandaid given the implications of issue #75 .

Let me know what you think we should do with this PR (separate from issue #75 , which will garner its own comments/ideas).

We are set up to allow empty filename. We are not set up for pathing of any kind. The third param there is meant to be a **file name**, _not_ a file path. The man page explicitly states to not include a leading `/`. That said, we are not doing anything to prevent that from being passed. I have not built out any checks to make sure it is a valid filename. At the VERY least we should be checking for path elements and returning an error. A filename cannot contain a `/`, so a simple check for the string containing one is a simple solution. That said, it seems like a bandaid given the implications of issue #75 . Let me know what you think we should do with this PR (separate from issue #75 , which will garner its own comments/ideas).

asdf commented

2019-11-04 20:53:59 +00:00

I'm going to recommend #75 work toward disallowing paths. It also seems like the better place to look at preventing empty file names. If you agree, then this can be merged in.

sloum commented

2019-11-05 01:26:39 +00:00

Yes. I agree. This solves the basic case of iterating the filename as necessary; #75 can take care of pathing concerns. I'll merge this in now and add a comment to #75 that this is merged in.

sloum closed this pull request

2019-11-05 01:26:45 +00:00

asdf deleted branch dont-overwrite-files

2019-11-08 05:37:27 +00:00

Sign in to join this conversation.

No reviewers