Skip to content
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

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

matthewblain
Copy link

This request contains three independent changes.

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.
@odeke-em
Copy link
Owner

odeke-em commented Jun 5, 2017

Thank you @matthewblain for the PR and welcome to drive!
My apologies for the late reply, I've been quite busy.
I've posted some comments on your PR. Unfortunately I don't have Windows machines in my
possession currently nor do I use Windows but I'll look at getting one when I return home in a couple of days.

@@ -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.
Copy link
Owner

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.

Copy link
Author

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.
Copy link
Owner

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.

Copy link
Author

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)
Copy link
Owner

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?

Copy link
Author

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) ") {
Copy link
Owner

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

devinalvaro and others added 4 commits July 2, 2017 17:48
prompt: add a space in "Proceed with the changes? [Y/n]:"
README: fix s/hypen/hyphen/g plus deinitialization spelling
@odeke-em
Copy link
Owner

Ping @matthewblain.

odeke-em and others added 6 commits July 23, 2017 23:56
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.
@matthewblain
Copy link
Author

Sorry I dropped this for a long time; see comments and changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants