mozilla / makedrive

[RETIRED] Webmaker Filesystem
Mozilla Public License 2.0
351 stars 33 forks source link

Fix #372 - Do not create conflicted copies if the client has a newer version of a file, and upstream that file #443

Closed gideonthomas closed 9 years ago

humphd commented 9 years ago

I think code looks pretty reasonable, and I've done one pass through it. I think the proof is in the testing, though, so it would be good to see what @alicoding sees when running this patch with Mobile Webmaker.

This was a pretty big change, thanks for getting it done so quickly.

gideonthomas commented 9 years ago

@alicoding would you be able to test this?

alicoding commented 9 years ago

File created before sync happen never get synced when connected again.

STR:

  1. Connect to MakeDrive
  2. Create file /path/to/file.txt
  3. Write something to the file Hello World
  4. Quit the browser before sync will happen
  5. Reopen the browser and connect to MakeDrive again
  6. Check /p/path/to/file.txt (Expected to see the file, but result we don't see it)
  7. Try to MakeDrive.fs().sync.request() just to see if we can force sync the file
  8. Checked again and no sync happened.
humphd commented 9 years ago

Why do you expect to see the file if you haven't synced yet? How could the server have a file that is only local, and has never been synced?

alicoding commented 9 years ago

@humphd I actually did MakeDrive.fs().sync.request(); and expected to see the file, but I'm not really sure if this is just how Nimble was setup and no file is being sync after I did that?

humphd commented 9 years ago

If you did MakeDrive.fs().sync.request() then you did do a sync, right? You say above then you never synced. Can you clarify exactly what you are doing. Your steps are not detailed enough.

alicoding commented 9 years ago

@humphd Sorry, I meant after my second reconnect I was trying to see why my local file is not being sync and even I have tried MakeDrive.fs().sync.request() and it doesn't seem like syncing it up to the upstream.

gideonthomas commented 9 years ago

@alicoding I might need more info because I tried to reproduce the error in the demo and am not getting it

gideonthomas commented 9 years ago

@humphd so @alicoding and I were discussing today about this issue that he found. It doesn't really seem directly related to my patch but it needs to be fixed (filed in https://github.com/mozilla/makedrive/issues/450).

The thing is, triggering a sync relies on fs.pathToSync which is undefined until changes are made to files. So, when Ali makes the change to the file, it gets recorded in pathToSync, but he closes the tab (disconnects) before the sync can happen. Consequently, fs and in turn fs.pathToSync goes out of scope and gets destroyed. So when Ali opens a new tab again, and calls sync.request(), pathToSync is obviously undefined as it is initialized with a new fs instance and thus a sync cannot happen UNTIL Ali makes a change to that file (which causes pathToSync to update) and trigger a sync.

One way we thought about fixing this is by creating the /etc directory to hold stuff that we want to persist about the fs between sessions e.g. pathToSync. However, we need to discuss this and come back to it later.

More importantly, would you be able to quickly review my code again when you get time so that we can land this code before the milestone ends?

humphd commented 9 years ago

A few comments. Once those are fixed, make sure travis passes, and get @alicoding to do a functional test with Mobile Webmaker. I don't need to review this again unless it changes a lot.

gideonthomas commented 9 years ago

@alicoding would you be able to test this on mobile webmaker?

alicoding commented 9 years ago

r+!