-
Notifications
You must be signed in to change notification settings - Fork 427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix Windows filename issue, update Y/n Prompt, update readme #918
base: master
Are you sure you want to change the base?
Conversation
Fix path separator bug. The command line converts path into local filesystem paths, then uses that to build server-side paths. This fixes the slashes to the format used on the server, even if the client uses a different format .(E.g. Windows uses a backslash.)
Removed two known issues: Doesn't work on Windows. But it does, though there may be some bugs. It does handle files which have the same name on the server. In a crude if functional fashion (errors out, or renames server-side).
Various yes/no promptes use Y/N but don't specify that the default is Y (yes). Change them to indicate that in the customary manner for command line programs, also used elsewhere in this program.
Thank you @matthewblain for the PR and welcome to drive! |
@@ -1420,8 +1420,6 @@ Background sync is not just hard, it is stupid. Here are my technical and philos | |||
|
|||
## Known Issues | |||
|
|||
* It probably doesn't work on Windows. | |||
* Google Drive allows a directory to contain files/directories with the same name. Client doesn't handle these cases yet. We don't recommend you to use `drive` if you have such files/directories to avoid data loss. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather leave this statement in there because drive has to ask folks to fix name clashes(or it can do it automatically). There is also a force mode that just overwrites.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added it back with a link to learn more.
@@ -1420,8 +1420,6 @@ Background sync is not just hard, it is stupid. Here are my technical and philos | |||
|
|||
## Known Issues | |||
|
|||
* It probably doesn't work on Windows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah unfortunately I personally don't have Windows machines nor would I be able to verify that it works properly on Windows so I am hesitant about removing this statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added back a modified statement: that it's unsupported but works somewhat.
// This function appears to be meant to return server-side (/-separated) paths. | ||
// But its input is client side (maybe / or \) separted paths. | ||
// filepath.ToSlash fixes that. | ||
relPath = filepath.ToSlash(relPath) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you mean to instead replace:
- relPath = "/" + relPath
+ relPath = filepath.ToSlash(relPath)
instead of keeping both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It's been a while so I forget exactly what happened (see issue#917) , but it was something like on Windows the input might be something like "photos\myalbum". Which should then be converted into "/photos/myalbum" for use on the remove side, but instead was being turned in to "/photos\myalbum". So both the ToSlash (convert "photos\myalbum" into "photos/myalbum") and the "/" prepend are necessary.
@@ -299,7 +299,7 @@ func (c *Context) DeInitialize(prompter func(...interface{}) bool, returnOnAnyEr | |||
} | |||
|
|||
for _, p := range pathsToRemove { | |||
if !prompter("remove: ", p, ". This operation is permanent (Y/N) ") { | |||
if !prompter("remove: ", p, ". This operation is permanent (Y/n) ") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How come the change of Y/n
? I was thinking it is representative of Yes/No
instead of Yes/no
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The capitla letter represents the default. https://unix.stackexchange.com/questions/15918/whats-the-standard-used-by-yum-prompt-is-this-ok-y-n
prompt: add a space in "Proceed with the changes? [Y/n]:"
README: fix s/hypen/hyphen/g plus deinitialization spelling
Ping @matthewblain. |
Ensure that nil is properly sent back after remote change/file resolution. This regression was caused by forgetting to send back that nil. This regression was caused by PR odeke-em#741. The consequence of the bug was that trying to push from non-existent folders would erraneously give back ```shell Resolving... Everything is up-to-date. ``` since the remotesChan channel would get closed after resolution without sending notifying whoever was resolving that the file or parent didn't exist remotely. Fixes odeke-em#933
Fix path separator bug. The command line converts path into local filesystem paths, then uses that to build server-side paths. This fixes the slashes to the format used on the server, even if the client uses a different format .(E.g. Windows uses a backslash.)
Removed two known issues: Doesn't work on Windows. But it does, though there may be some bugs. It does handle files which have the same name on the server. In a crude if functional fashion (errors out, or renames server-side).
Various yes/no promptes use Y/N but don't specify that the default is Y (yes). Change them to indicate that in the customary manner for command line programs, also used elsewhere in this program.
Update readme to be more accurate.
# Conflicts: # README.md
Sorry I dropped this for a long time; see comments and changes. |
This request contains three independent changes.