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

"These X file(s) would be overwritten" error should not be thrown when updating unchanged files #114

Closed
l3iggs opened this issue Mar 21, 2015 · 19 comments

Comments

@l3iggs
Copy link

l3iggs commented Mar 21, 2015

How to reproduce:
Step 1: use drive pull to pull down a file
Step 2: edit the file locally
Step 3: use drive push to push the changes of that file back up to Google

When I do this, I get an error on Step 3 with the message:
"These 1 file(s) would be overwritten. Use -ignore-conflict to override this behaviour"

The fact is, nothing gets overwritten by pushing an updated file over an already existing file. 30 days of revisions are stored by Google Drive. I think the right thing to do here is to probably warn the user with a message along the lines of:

This push will update the following files:
{list of files to be updated}
Revisions of these files will be stored for 30 days and can be reverted in the web interface.

By the way, behaviour is spelled behavior :-P

@odeke-em
Copy link
Owner

It is 5AM here I'll jump onto this over the weekend.
Haha @ behaviour vs behavior <==> tabs vs spaces

@odeke-em
Copy link
Owner

Also please take a look at #57

@l3iggs
Copy link
Author

l3iggs commented Mar 21, 2015

I just had a look at that. It seems that issue #57 was concerning overwriting locally edited files with a possibly outdated cloud version when the user issues a drive pull. Preventing that from happening seems like a perfect time to require the -ignore-conflict switch. I guess that's why you implemented it in the first place.

My suggestion here is that you should probably not apply the same logic when the user issues a drive push for files that already exist in the cloud since Google keeps a history of all the file versions there for us, there's no chance of ever losing data on a push (unless the user waits 30 days and Google's revision times out, hence the wording of the warning message I've suggested).

@odeke-em
Copy link
Owner

Except that we are not tracking changes nor using versions for files already on the cloud.

@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

I was under the impression that the version management was all done server side.

A quick test I've just done confirms that the Google Drive web interface is indeed showing me all of the versions of my files just as I expect as I update them with drive push even when i push over files already existing in my Drive. Am I missing something?

@odeke-em
Copy link
Owner

By we, I mean drive's end on your computer. Yes Google Drive maintains a whole bunch of versions and even keeps track of changes to the respective files. Doing this on the client side requires a whole bunch of changes which I don't think we are ready for yet. For now while we pushing to the latest version of the file, I would rather stick to the behaviour that mitigates data loss. As you know, the README specifies that drive does push and pull, but even still users expect protection from their own mistakes. I would rather play it safe than sorry.

@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

This issue is only related to the drive push command.

My statement here is that it's impossible to lose data with drive push (since Google is managing revisions in the cloud) and so the "These 1 file(s) would be overwritten. Use -ignore-conflict to override this behaviour" error and associated logic should be removed from the codebase for the drive push command.

@odeke-em
Copy link
Owner

I got your point the first time. Except that every file on the cloud every file is versioned and can even be untrashed but how come users will complain of data loss?
Remember that drive even supports untrash-ing

@l3iggs l3iggs changed the title -ignore-conflict requirement on push is too aggressive "These X file(s) would be overwritten" error makes no sense when pushing Mar 23, 2015
@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

Well I think #57 was complaining about data loss on a pull, which is legitimate since his/her pulls seemed to be overwriting local files that had not been saved anywhere else.

It seems to me that any complaints of data loss on a push are not legitimate and should be disregarded while educating the user on how to use the file versioning scheme in Google Drive.

@odeke-em
Copy link
Owner

It was with a data loss on both pull and push. What you seem to be missing is that drive has not yet started managing and tracking versions, if it were doing that, then I would willingly accept this feature.
I think I have a hunch on how to get this to happy. However, I will do that in about 11 hours when I get back home.

@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

I just read #57 again and I still don't see where data was lost on a push, in all the examples there a pull caused the data loss. Can you give me a quick example of how this is possible? Because I currently don't believe it is and I believe that's the crux of our misunderstanding.

We're definitely somehow not communicating properly on this issue because I fail to see how drive not having any version tracking features is relevant here.

@odeke-em
Copy link
Owner

You and two classmates collaborate on a text document, and are behind terminals. Everyone edits the doc and pushes up one after the other. With your suggestion of ignoring protection just because Google Drive maintains versions, I will have about three irrate users posting issues or asking to re-open #57. With the guards, you can know what is up, do a drive diff, find out what's going on, find out what to merge, push up.
If drive had the ability to pull or push specific versions (this will be coming sometime in the future), you could know the version you are on, push to that ETag then even be able to merge up because we can track which exact copy of the doc you are on. A pull-to-merge can then auto fix any edits and conflicts(shhh, this is how I'll get it to allow for auto-merging).
Make sense?

@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

I don't think anyone from #57 expects drive to turn into a real-time collaboration&merge tool (I sure hope it doesn't). In the above example, each student's changes are neatly saved by Google Drive waiting for a human with a proper merge tool to come along and download the three revisions, merge them and upload the result. I stand by my claim that a push will never cause data loss (outside of the 30 day revision timeout). What drive should do in this case, is throw a warning during the push: "Hey, someone else has edited the file you're pushing to between the time you last pulled it and when you're pushing it now! You should probably investigate this and maybe do a merge to incorporate their changes if needed." Going back to #89, drive could then even suggest the user run the proper drive diff command which would generate a diff in a format that patch understands so that the user can then use the robustness of that utility's conflict handling to fix up the situation.

I understand the users in #57 were complaining about locally edited files being clobbered during pulls by old versions or changes made by their friends, which you solved by throwing an error and stopping the dangerous pull from being done and that makes total sense. However, you're also throwing the same exact error every time I'm pushing to an existing file which nobody has edited, which really makes no sense.

Consider another use case (mine): I'm working alone on the files in my drive (my hunch is that this is a more frequent use case than the real-time-overlapping-push-collaboration noted above). I pull a file down, make a quick edit and 30 seconds later attempt to push it up. I'm greeted with an error message on push that says "X files will be overwritten" (which is a flat out lie, since I know Google is keeping revisions). The message tells me I need to add "-ignore-conflict" to my command line. Currently, the only way I can get drive to work is to add "-ignore-conflict" to every single one of my pushes, which is obviously ridiculous.

$ echo somecontent > file.txt
$ drive push file.txt
Resolving...
+ /file.txt
Addition count 1 src: 12.00B
Proceed with the changes? [Y/n]: y
1 / 1 [========] 100.00 % 1s
$ echo morecontent >> file.txt
$ drive push file.txt
Resolving...
These 1 file(s) would be overwritten. Use -ignore-conflict to override this behaviour
/file.txt
conflicts have prevented a push operation
$ drive push -ignore-conflict file.txt
Resolving...
X /file.txt
Clashing modification count 1 src: 24.00B dest: 12.00B
Proceed with the changes? [Y/n]: y
1 / 1 [========] 100.00 % 1s

@odeke-em
Copy link
Owner

This is from a comment I left 3 hours ago

I think I have a hunch on how to get this to happy. However, I will do that in about 11 hours when I get back home.

In 8 hours I'll try to show you with a patch that removing this protection might not be necessary and that you'll be able to push a file.

@l3iggs
Copy link
Author

l3iggs commented Mar 23, 2015

:-) fingers crossed!
I'm still not convinced we're communicating though, so I'm a little worried that it's wasted effort.

@odeke-em
Copy link
Owner

Actually your third last and second last points made a lot sense, of which basically drive should confirm first with the mother ship that the file wasn't changed -- this is not happening right now. However in the event that the file is actually modified, I am still leaving the protection in.

@l3iggs l3iggs changed the title "These X file(s) would be overwritten" error makes no sense when pushing "These X file(s) would be overwritten" error should not be thrown when updating unchanged files Mar 23, 2015
@odeke-em
Copy link
Owner

See if your advocacy had started with, I cannot push to remote which is basically the bug that we are trying to solve, I could have gotten a PR in yesterday, but the suggestion got too broad that's why we have gone back and forth. Basically the protection never properly covered the case for push.

@odeke-em
Copy link
Owner

PR #121 is what I was talking about. Please take a look at it. Thank you.

@odeke-em
Copy link
Owner

Addressed by 8f515bc

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

No branches or pull requests

2 participants