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

Added ability to move and rename files #36

Closed
wants to merge 21 commits into from
Closed

Added ability to move and rename files #36

wants to merge 21 commits into from

Conversation

dylanwulf
Copy link

I added the ability to move and rename files using grive without needing to reupload or redownload. It works like this:
grive -m [--move] source destination
I thought it would be useful to people who often deal with large files or have slow internet connections. It should also be fairly simple to implement the ability to copy or move multiple files at once.

@@ -138,6 +138,7 @@ template <> struct Val::Type2Enum<double> { static const TypeEnum type = double
template <> struct Val::Type2Enum<std::string> { static const TypeEnum type = string_type ; } ;
template <> struct Val::Type2Enum<Val::Array> { static const TypeEnum type = array_type ; } ;
template <> struct Val::Type2Enum<Val::Object> { static const TypeEnum type = object_type ; } ;
template <> struct Val::Type2Enum<std::vector<std::string> > { static const TypeEnum type = string_list_type ; } ;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't like this thing... array of strings should be implemented via array Val of string Vals, like in json: [ str1, str2, ... ]

Copy link
Author

Choose a reason for hiding this comment

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

I did it this way because boost accepts program arguments with multiple values as a vector of strings, and I'm not quite sure how I could convert it to an array Val of string Vals

Copy link
Owner

Choose a reason for hiding this comment

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

in fact I think it doesn't really matter because the proper place for move operation arguments is just in Drive::Move() function arguments :-) and it should also be simpler to implement :-)

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, you're right. I'll change this

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

Overall, thanks for this feature :-)
By the way, is it capable of moving directories?
I think the best way would be to try and detect renames automatically... but that's probably something that I will try to implement later.

@dylanwulf
Copy link
Author

Yes, it works on directories just fine. I'm still learning C++, so the reason I did this was mostly to get more experience with it. Sorry for any noobish mistakes I made :[

@dylanwulf
Copy link
Author

I think if you want to automatically detect moves and renames, you would have to store every file's hash in a file, so you would know which file was renamed/moved and where it came from. But that would get problematic if the user has two files with the exact same contents...

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

Yes, it works on directories just fine. I'm still learning C++, so the reason I did this was mostly to get more experience with it. Sorry for any noobish mistakes I made :[

I'm also not a very big C++ guru so everything's ok =)

you would have to store every file's hash in a file

That's what I want to implement anyway, because grive currently scans all files in the working directory and calculates md5 hashes on every invocation, and it gets very slow if there are a lot of files. Grive definitely needs some sort of "index" (like the similar structure is called in git). File ctimes (inode change times) should also be stored there. That's how git tracks changes in the working directory: when file ctime is the same it knows file is unchanged, and when file size changes it knows the file is obviously modified. And only if ctime is changed, but the file size isn't, it checks the modification status by comparing the hash.

...Hm... In fact it's not necessary for rename detection to store hashes, because they're recalculated every time, i.e. they're already available at the moment of downloading/uploading to/from the server...

@dylanwulf
Copy link
Author

I made all the changes you suggested.
Also, I don't think renaming files changes the ctimes, not sure if that would be a problem.

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

ctime is changed by the filesystem on every inode change - it's basically a non-fakeable mtime. :-) the only way to fake it is to change block device directly while it's unmounted. that's very useful for change tracking :)

@dylanwulf
Copy link
Author

Ooh, I see. Does Google Drive have something similar, or does it only track mtime?

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

I think server-side mtime is also non-fakeable.

@dylanwulf
Copy link
Author

https://developers.google.com/drive/v2/reference/files/update
It accepts a parameter called "modifiedDateBehavior" which can be set to now, noChange, or any date from the request body. Also, local ctime changes when a file is moved or renamed, but server mtime does not change when a file is moved or renamed from the web UI.

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

In fact I have some more comments on code:

  1. There's some mess with indents and curly braces in Syncer2::Move()
  2. It does not handle the case when the new parent is not yet synced with server
  3. It segfaults when I run it with grive -p dir -m dir/from dir/to and does not work when I run it with grive -p dir -m from to
    :)

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

Ok, so it's possible to change server-side mtime ... but I see there's 'version' field for which the doc says "A monotonically increasing version number for the file. This reflects every change made to the file on the server, even those not visible to the requesting user."

@dylanwulf
Copy link
Author

I'll work on fixing that. What's the difference between -p and -s options?

@vitalif
Copy link
Owner

vitalif commented Oct 12, 2015

-p specifies sync directory different from current working directory
-s select one subdir to sync
i.e. grive -p dir is similar to cd dir; grive

@dylanwulf
Copy link
Author

Okay, should be all fixed now.

  1. Fixed the indents and curly braces
  2. I changed it so it always syncs before moving, to make sure everything is up to date. The move will check to make sure the source file and destination folder are up to date, and fail if they are not.
  3. It should work with the -p option now, and move accepts both absolute paths and paths relative to current directory as valid arguments

@vitalif
Copy link
Owner

vitalif commented Nov 10, 2015

OK, I've just merged your code.
For cleaner version history I've squashed all your commits into a single commit with preserved authorship.
Thanks :)

@vitalif vitalif closed this Nov 10, 2015
@dylanwulf
Copy link
Author

Thank you for all your helpful feedback :)

@vitalif
Copy link
Owner

vitalif commented Jan 3, 2016

Look at the latest commits, I've implemented automatic rename detection along with a local "index" structure :)

@dylanwulf
Copy link
Author

Ah, very nice!

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

Successfully merging this pull request may close these issues.

None yet

2 participants