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

Check if localModifiedTime >= item.mtime: avoid re-upload #1131

Merged
merged 3 commits into from
Nov 7, 2020
Merged

Check if localModifiedTime >= item.mtime: avoid re-upload #1131

merged 3 commits into from
Nov 7, 2020

Conversation

mp1994
Copy link
Contributor

@mp1994 mp1994 commented Nov 7, 2020

In file sync.d, in function private void applyDifference(JSONValue driveItem, string driveId, bool isRoot), check if localModifiedTime is greater or equal to the item modified time (item.mtime). This avoid the re-upload in case another process has modified the file in the local disk and that change is already synchronized with remote.

This happens when local file time = remote file time > local database time

Example (that requires no fix): remote file time > local file time = local database time, if the file is modified online. The sync correctly updates the local file with no issues.
Example (that requires this fix): local file is changed using a Windows Virtual Machine with OneDrive, that updates also the remote file. When onedrive --synchronize runs on linux, the local file time is equal to the remote time, and both are greater than the local database time. This fix avoids the safeRename while updating the local database entry.

This may be very specific to my configuration, but it should be completely transparent to "conventional" configurations. Thus, I hope this PR will be merged!

@abraunegg
Copy link
Owner

@mp1994
Thanks for the PR. There are some issues with it which I need to fix up, but also will make some comments in your code changes as well to assist with some understanding before I commit some changes here.

src/sync.d Outdated
@@ -131,11 +131,12 @@ private Item makeItem(const ref JSONValue driveItem)
// https://github.com/abraunegg/onedrive/issues/11
if (isItemRemote(driveItem)) {
item.mtime = SysTime.fromISOExtString(driveItem["remoteItem"]["fileSystemInfo"]["lastModifiedDateTime"].str);
log.vdebug("Remote item time: ", item.mtime);
Copy link
Owner

Choose a reason for hiding this comment

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

remote item here is not 'local' or 'remote', but a OneDrive object that resides on another drive id which is not the account default - typically this is onedrive shared folder

src/sync.d Outdated
@@ -2146,6 +2147,7 @@ final class SyncEngine
if (cached && item.eTag != oldItem.eTag) {
// Is the item in the local database
if (itemdb.idInLocalDatabase(item.driveId, item.id)){
log.vdebug(">> Item in local database.\n");
Copy link
Owner

Choose a reason for hiding this comment

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

this will need to be cleaned up

src/sync.d Outdated
//log.vdebug("localModifiedTime (local file): ", localModifiedTime);
//log.vdebug("item.mtime (OneDrive item): ", item.mtime);
log.vlog("local file modified time: ", localModifiedTime);
log.vlog("OneDrive database item time (item.mtime): ", item.mtime);
Copy link
Owner

Choose a reason for hiding this comment

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

item.mtime is not the database item - but the item provided by OneDrive via the API

* Update PR
@abraunegg
Copy link
Owner

@mp1994
Please can you review the updated PR before this gets merged into 'master'

@mp1994
Copy link
Contributor Author

mp1994 commented Nov 7, 2020

@abraunegg thank you for working on this, I have tested the PR with the following output

$ ./onedrive --synchronize --verbose --verbose

Output of the log relative to the modified file:

[DEBUG] OneDrive item ID is present in local database
The local item has a different modified time 2020-Nov-07 20:57:23Z when compared to database modified time 2020-Nov-07 15:03:14Z
The local item has a different hash when compared to database item hash
[DEBUG] localModifiedTime (local file): 2020-Nov-07 20:57:23Z
[DEBUG] item.mtime (OneDrive item):     2020-Nov-07 20:57:23Z
Local item modified time is equal to OneDrive item modified time based on UTC time conversion - keeping local item
[DEBUG] Skipping OneDrive change as this is determined to be unwanted due to local item modified time being newer than OneDrive item

It worked as intended, the local file is kept without safeRename being called (i.e., no copy of the file is created/uploaded).
The database entry appears to be updated correctly.
Thank you also for improving the comments on the source code, which was already very well commented and documented.

* tweak debug output messaging
@abraunegg
Copy link
Owner

abraunegg commented Nov 7, 2020

@mp1994
Thanks for validating. I have updated that final debug line output to be cleaner in messaging, and will commit this to master

@mp1994
Copy link
Contributor Author

mp1994 commented Nov 7, 2020

@mp1994
Thanks for validating. I have updated that final debug line output to be cleaner in messaging, and will commit this to master

@abraunegg that's great, thank you!

@abraunegg abraunegg self-requested a review November 7, 2020 21:23
@abraunegg abraunegg changed the title (sync.d) Check if localModifiedTime >= item.mtime: avoid re-upload Check if localModifiedTime >= item.mtime: avoid re-upload Nov 7, 2020
@abraunegg abraunegg merged commit 7a4abff into abraunegg:master Nov 7, 2020
@abraunegg abraunegg added this to the v2.4.7 milestone Nov 8, 2020
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked and limited conversation to collaborators May 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants