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

gideonthomas · 2014-10-20T23:01:04Z

No description provided.

humphd · 2014-10-20T23:42:00Z

client/src/index.js

@@ -117,6 +118,9 @@ function createFS(options) {
  var autoSync;
  var pathCache;

+  // Path needed to be upstreamed during a downstream sync


I don't fully understand this comment. I think it needs more context. It seems odd, on its face, to be talking about a path to be upstreamed during a downstream.

humphd · 2014-10-20T23:44:42Z

client/src/index.js

+        upstreamPath = resolvePath(needUpstream);
+        requestSync(upstreamPath);
+      } else {
+        upstreamPath = null;


Question: it looks like needUpstream and upstreamPath are in play at the same time as one another; could we just use upstreamPath to indicate that we need to upstream, and not have a second variable? If not possible, fine. Just wondering. The fewer variables and states to manage, the fewer bugs.

https://github.com/mozilla/makedrive/pull/443/files#diff-c3ede5b5250ec9a7181630da3090839aR211 uses it to manage error states...its kinda equivalent to pathCache in a way

humphd · 2014-10-21T00:10:52Z

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 · 2014-10-21T05:24:23Z

@alicoding would you be able to test this?

alicoding · 2014-10-21T15:01:15Z

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

STR:

Connect to MakeDrive
Create file /path/to/file.txt
Write something to the file Hello World
Quit the browser before sync will happen
Reopen the browser and connect to MakeDrive again
Check /p/path/to/file.txt (Expected to see the file, but result we don't see it)
Try to MakeDrive.fs().sync.request() just to see if we can force sync the file
Checked again and no sync happened.

humphd · 2014-10-21T15:02:12Z

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 · 2014-10-21T15:06:04Z

@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 · 2014-10-21T15:07:05Z

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 · 2014-10-21T15:08:35Z

@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 · 2014-10-21T23:20:08Z

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

gideonthomas · 2014-10-22T20:26:32Z

@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 #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 · 2014-10-25T18:37:56Z

client/src/index.js

-    pathCache = fs.pathToSync;
-    fs.pathToSync = null;
-    manager.syncPath(pathCache);
+    // We do not expose a prototype that allows


What does this comment mean? That we need to do this in the future? If so, file a bug. If not, clarify the comment.

humphd · 2014-10-25T18:51:03Z

lib/rsync/patch.js

+        // If the file was modified before the
+        // diffNode's modified time, the file is outdated
+        // and needs to be patched
+        if(stats.mtime <= diffNode.modified) {


I wonder how we should deal with the NOMTIME flag being set for the filesystem, see https://github.com/filerjs/filer#filerfilesystemoptions-callback-constructor. It would break your expectations here.

we can address it in #447

humphd · 2014-10-25T18:56:16Z

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 · 2014-10-25T19:19:56Z

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

alicoding · 2014-10-29T16:11:45Z

r+!

… patch

… newer version of a file, instead upstream that file

humphd reviewed Oct 20, 2014
View reviewed changes

gideonthomas force-pushed the issue372 branch from 61d03fd to 019b527 Compare October 20, 2014 23:42

humphd reviewed Oct 20, 2014
View reviewed changes

gideonthomas mentioned this pull request Oct 22, 2014

Develop a solution for allowing persistence of information about the file system between sessions #450

Open

humphd reviewed Oct 25, 2014
View reviewed changes

gideonthomas force-pushed the issue372 branch from da60fba to 0d3d864 Compare October 25, 2014 19:18

gideonthomas added 2 commits October 29, 2014 12:12

Do not delete conflicted copies when removing delete nodes in rsync's…

2e7fecb

… patch

Fix mozilla#372 - Do not create conflicted copies if the client has a…

f64388e

… newer version of a file, instead upstream that file

gideonthomas force-pushed the issue372 branch from 0d3d864 to f64388e Compare October 29, 2014 16:13

gideonthomas merged commit f64388e into mozilla:master Oct 29, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gideonthomas commented Oct 20, 2014

humphd Oct 20, 2014

humphd Oct 20, 2014

gideonthomas Oct 20, 2014

humphd commented Oct 21, 2014

gideonthomas commented Oct 21, 2014

alicoding commented Oct 21, 2014

humphd commented Oct 21, 2014

alicoding commented Oct 21, 2014

humphd commented Oct 21, 2014

alicoding commented Oct 21, 2014

gideonthomas commented Oct 21, 2014

gideonthomas commented Oct 22, 2014

humphd Oct 25, 2014

humphd Oct 25, 2014

gideonthomas Oct 25, 2014

humphd commented Oct 25, 2014

gideonthomas commented Oct 25, 2014

alicoding commented Oct 29, 2014

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

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

Conversation

gideonthomas commented Oct 20, 2014

humphd Oct 20, 2014

Choose a reason for hiding this comment

humphd Oct 20, 2014

Choose a reason for hiding this comment

gideonthomas Oct 20, 2014

Choose a reason for hiding this comment

humphd commented Oct 21, 2014

gideonthomas commented Oct 21, 2014

alicoding commented Oct 21, 2014

humphd commented Oct 21, 2014

alicoding commented Oct 21, 2014

humphd commented Oct 21, 2014

alicoding commented Oct 21, 2014

gideonthomas commented Oct 21, 2014

gideonthomas commented Oct 22, 2014

humphd Oct 25, 2014

Choose a reason for hiding this comment

humphd Oct 25, 2014

Choose a reason for hiding this comment

gideonthomas Oct 25, 2014

Choose a reason for hiding this comment

humphd commented Oct 25, 2014

gideonthomas commented Oct 25, 2014

alicoding commented Oct 29, 2014