Safe upload #1026
Merged
Safe upload #1026
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
shanalikhan
requested changes
Sep 2, 2019
Instead of comparing the gist's latestUpload time against the local latestUpload time, this function should compare the gist time against the local latestDownload time in order to determine if the cloud settings are newer than the local settings. Also, on an upload, set the local latestDownload time to the same time as the upload since the settings are effectively "downloaded" on an upload. This protects against the following scenario: - Machine A uploads settings to cloud - Machine B downloads these settings - Machine B changes settings and uploads them to the cloud - Machine A downloads the settings - Machine A changes settings and tries to upload them The last upload attempt fails because A's last upload time is before the cloud's last upload time, even though A downloaded the settings. A forced upload is needed in order to upload the settings. It therefore makes sense to compare against the local download time.
When checking for the existance of files in the gist, use the settings file "gistName" to lookup the file in the gist instead of "fileName" in order to find any files in a subdir. The gist uses "|" as a separator in the "gistName" for files in a subdir and so using a plain filename does not match. This fixes the case where a user has snippets and does an upload with no changes to settings or snippets. The snippets are already up in the gist. The code that checks for files that are local and not in the gist fails to match the fileName to the gistName and incorrectly concludes that the file is not in the gist. This causes the upload code to upload all the files even if there were no changes, resulting in an "empty" gist revision with only the upload timestamp changed.
|
Rebased and fixed up the snippet-related commit. Ready for review. |
shanalikhan
approved these changes
Sep 5, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Short description of what this resolves:
Allows uploading settings without forcing when multiple machines are uploading settings.
Example failing scenario:
The last upload attempt fails because A's last upload time is before the cloud's last upload time, even though A downloaded the settings. A forced upload is needed in order to upload the settings. It, therefore, makes sense to compare against the local download time.
Changes proposed in this pull request:
Fixes: #1016
How Has This Been Tested?
Screenshots (if appropriate):
n/a
Checklist: