Allow more fine tuning of the move_storage, or at least make it safer

[GoogleCodeExporter]

This is in continuation of the improvements requested here: 
https://code.google.com/p/libtorrent/issues/detail?id=444&start=100

Consider this scenario:
1. You finish a torrent in path X.
2. You re-add the torrent and set it to download in path Y this time.(let's 
just say you forgot that you already have it)
3. The torrent downloads some data, let's say 13%, and you realize that you 
already have it in path X and you want to seed it instead.
4. You use move_storage on the torrent where you enter the path X
5. libtorrent will move all the files from Y to X without checking if they 
already exist in X, and it overwrites them. You most probably will lose only a 
few files from a multifile torrent if you do a recheck afterwards.

It would be awesome if libtorrent could do a check before actually making the 
move. It also would be awesome if it allowed multiple behavior.

Some ideas:
1. Make move_storage accept a flag.
2. The flag could be {overwrite, recheck, ask}
3. In case of 'ask' an alert should be posted, which the program could use to 
display a dialog to the user to decide what to do.
4. Provide a way to abort the move in case the user doesn't want to either 
overwrite or recheck the data.
5. In case of a failed recheck, the user should be asked again because the 
files ARE different but have the same names. This doesn't mean, however, that 
they aren't actual and usable files to the user. (a rare occurence nevertheless)
6. Of course in a partial failed recheck, libtorrent should check if the "to be 
moved" data are usable in the final destination.

Original issue reported on code.google.com by hammered...@gmail.com on 5 May 2013 at 9:43

[GoogleCodeExporter]

I think it could be simplified. How about this scheme:

move_storage takes a flag argument of:
  1. always overwrite any file in the destination folder
  2. fail the move if any file exist
  3. silently leave existing files (and delete the source), if any of the files already exist in the destination

I believe with these options you could implement your scheme by first issue a 
fail-if-exist. If that operation fails, you can ask the user, and then re-issue 
with either overwrite or use existing files.

Does that sound reasonable?

Original comment by arvid.no...@gmail.com on 5 May 2013 at 10:06

[GoogleCodeExporter]

Yeap it sounds way simpler your way. But a 2 questions:
1. Do 2) and 3) check only the filenames or do they do a hash check too? IMO, 
2) should do a namecheck only, while 3) should do a hash recheck, unless 
specified otherwise(extra flag, skip rechec, assume whole data on disk, speed 
optimization).
2. What happens if only you download in path X 80% percent of a torrent. Then 
in path Y you download 5% of the torrent. From the 2% percent in Y 1% percent 
is part of the 20% percent missing in X and the other 4% is duplicate. Do you 
merge? I acknowledge that this could introduce code complexity without many 
uses in real situations. So it is better left unhandled. 

Original comment by hammered...@gmail.com on 5 May 2013 at 10:35

[GoogleCodeExporter]

Number 2 question correction:
2. What happens if only you download in path X 80% percent of a torrent. Then 
in path Y you download 5% of the torrent. From the 5% percent in Y, 1% percent 
is part of the 20% percent missing in X and the other 4% is duplicate. Do you 
merge? I acknowledge that this could introduce code complexity without many 
uses in real situations. So it is better left unhandled. 

Original comment by hammered...@gmail.com on 5 May 2013 at 10:36

[GoogleCodeExporter]

I would prefer not merging files. The hash checking in case any new files are 
taken could be a full check, right?

ideally it would only check the new files, but just issuing a full check would 
be a lot simpler.

Original comment by arvid.no...@gmail.com on 5 May 2013 at 10:53

[GoogleCodeExporter]

Are you refering to "3. silently leave existing files (and delete the source), 
if any of the files already exist in the destination"? Then yes, a full check, 
I don't see how you could implement a partial check....

Original comment by hammered...@gmail.com on 5 May 2013 at 11:03

[GoogleCodeExporter]

right.. well.. you could get away with only checking the new files, and not the 
ones you copied.

Original comment by arvid.no...@gmail.com on 6 May 2013 at 12:14

[GoogleCodeExporter]

Hmm, I now understand the use case better.
Well wouldn't it be trivial to implement partial check since you know that the 
data you copied is verified? Only the existing data(in the new location) should 
be verified. Something like extending the current hash checking routine and 
telling it "here take this list of torrent pieces and consider them verified, 
check all else".

Original comment by hammered...@gmail.com on 6 May 2013 at 12:26

[GoogleCodeExporter]

Hmm, there seems to be a very old duplicate issue about this from qBittorrent 
also: https://code.google.com/p/libtorrent/issues/detail?id=8

Original comment by hammered...@gmail.com on 6 May 2013 at 12:31

[GoogleCodeExporter]

would you mind testing this patch? It's against trunk and adds the feature I 
outlined above to move_storage, including documentation.

Original comment by arvid.no...@gmail.com on 8 May 2013 at 4:13

• Changed state: Started

Attachments:

• move_storage.diff

[GoogleCodeExporter]

I tried move_storage(path, fail_if_exist) but it still overwrites the files in 
the destination path.

For the documentation: You should indicate what error_code the 
storage_move_failed_alert will contain in 'fail_if_exist'. (an alert on failure 
will be thrown right?)

Clarification: Does 'dont_replace' trigger a full recheck? The docs doesn't 
mention anything on this.

Original comment by hammered...@gmail.com on 8 May 2013 at 10:14

[GoogleCodeExporter]

I even tried with r8379 but fail_if_exist seems to be ignored. It still 
overwrites the files.

Original comment by hammered...@gmail.com on 9 May 2013 at 12:37

[GoogleCodeExporter]

Issue 8 has been merged into this issue.

Original comment by arvid.no...@gmail.com on 10 May 2013 at 2:22

[GoogleCodeExporter]

thanks. that turned out to be a silly mistake in my patch. I've checked this in 
as well as a fix to the faile_if_exist bug.

I believe this fully takes care of the use cases we care about. If there's 
still something that doesn't work or if there's some use case it doesn't cover. 
Please comment.

Original comment by arvid.no...@gmail.com on 10 May 2013 at 2:24

• Changed state: Fixed

[GoogleCodeExporter]

by the way, I put this in trunk. It will be released in the next major release, 
not in 0.16.x.

Original comment by arvid.no...@gmail.com on 10 May 2013 at 2:25

[GoogleCodeExporter]

fail_if_exist seems to work now, but not always. I can reproduce the failure 
consistently, but I am not sure it isn't a qbittorrent yet. I need to test some 
things. I am tired now, expect some update tomorrow on this.

In the meantime I have 2 suggestions:
1. If I understand the code correctly, dont_replace triggers a full recheck 
afterwards, this should be documented
2. You need to define a new error code for fail_if_exist and document it. 
Currently, when it fails storage_moved_failed_alert::error::value() returns 0 
(ie no_error) which isn't helpful. We need a new error_code for this.

Original comment by hammered...@gmail.com on 10 May 2013 at 10:24

[GoogleCodeExporter]

I've made the error code be boost::system::errc::file_exists when it fail 
because a file exist. While adding this I found an error in trunk where the 
storage's save path wouldn't be updated in the case of "need_full_check" (when 
moving storage with dont_replace). Maybe that caused your test to fail?

Original comment by arvid.no...@gmail.com on 11 May 2013 at 4:02

[GoogleCodeExporter]

error.value() still returns 0.

fail_if_exist fails in this case: When the destination path is just a root 
folder of a drive eg "H:\" or "G:\". The files get overwritten even though they 
exist there.

Original comment by hammered...@gmail.com on 11 May 2013 at 3:23

[GoogleCodeExporter]

there was a silly typo related to the error handling. Fixed now, with a unit 
test :)

Original comment by arvid.no...@gmail.com on 11 May 2013 at 9:37

[GoogleCodeExporter]

I believe I fixed the second error as well. I was simply looking if the new 
save path existed at all, and apparently you can't stat "c:\" to know if it 
exists. It should be fixed in trunk in [8396].

Original comment by arvid.no...@gmail.com on 11 May 2013 at 9:42

[GoogleCodeExporter]

fail_if_exist seems to work correctly now.

error.value() gives me 17 and error.message() gives me(on Windows): "The system 
cannot move the file to a different disk drive." 
->http://msdn.microsoft.com/en-us/library/ms837428.aspx

I get the feeling this isn't the same as boost::system::errc::file_exists. Am I 
correct?

Original comment by hammered...@gmail.com on 11 May 2013 at 11:33