| msg58332 - (view) |
Author: Ingemar Nilsson (init) |
Date: 2007-12-09 23:23 |
| If you use shutil.move(src, dst) to move a file src to a directory dst, the file will not be renamed into the dst directory, but rather copied-then-removed, even if src and dst is on the same filesystem. There are several ways to fix this: * (The easiest) Fix the documentation for shutil.move() so that this is mentioned. * Fix shutil.move() so that it rename a file into a new directory. * Change os.rename() to accept a directory as a destination for a file. The reason for this problem is that shutil.move() tries to use os.rename(), but fails since the destination is a directory. It then proceeds to the copy-then-remove method, even though the documentation gives the impression that this only happens when src and dst are on different filesystems. |
|
|
| msg58343 - (view) |
Author: Facundo Batista (facundobatista) *  |
Date: 2007-12-10 12:01 |
| I have another way: * Check if the destination is a directory, and in such case make an os.path.join(destination, originfile), and then use os.rename() with this new destination. What do you think? Do you think this way suffers from any multiplatform issue? More important: how can this be tested? |
|
|
| msg58356 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-10 18:15 |
| We should first decide what should happen. While for command line tools "mv FILE DIR" is established syntax for "mv FILE DIR/`basename FILE`", I'm not at all sure that shutil.move(src, dst) should do the same. I think it should be closer to the UNIX os.rename() semantics which promises that src *replaces* dst. If dst is an empty dir, it is removed using rmdir(); if it is a non-empty dir, the removal fails and hence the move fails. I think this is what shutil.move() should do too. (Even if it can't make the atomicity promises that UNIX makes.) |
|
|
| msg58368 - (view) |
Author: Ingemar Nilsson (init) |
Date: 2007-12-10 20:20 |
| Well, if that's what you want, I suggest documenting it in the manual. From reading the manual, I thought that shutil.move() would behave like mv, and I was surprised that it doesn't. To me the big issue isn't how it is solved, it's rather that its behavior is clearly documented in the manual. |
|
|
| msg58431 - (view) |
Author: Raghuram Devarakonda (draghuram)  |
Date: 2007-12-11 14:53 |
| Since we already have os.rename, wouldn't it be better for shutil.move() to be closer to command line 'mv'? I think Facundo's approach should work. |
|
|
| msg58441 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2007-12-11 17:24 |
| > Since we already have os.rename, wouldn't it be better for shutil.move() > to be closer to command line 'mv'? I think Facundo's approach should work. I'd rather not do this. It might cause disasters for code that expects the old semantics. If you want a way to do the "mv" semantics, propose a new API. shutil.move() still offers several things over os.rename(): deleting the target even if os.rename() doesn't (it doesn't on Windows); cross-filesystem moves implemented as copy. |
|
|
| msg58494 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2007-12-12 14:37 |
| Then, a small change in the doc might clarify the usage (as OP suggested); "If the destination is on our current filesystem, then simply use rename." can be replaced with: "If the destination is a fiile and is on same filesystem as that of src, then simply use rename." |
|
|
| msg58495 - (view) |
Author: Ingemar Nilsson (init) |
Date: 2007-12-12 15:08 |
| > If you want a way to do the "mv" semantics, propose > a new API. shutil.mv()? |
|
|
| msg61539 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-22 22:05 |
| I don't see where there is a change in semantics. Today, `shutil.move(source_file, destination_dir)` already moves source_file inside the destination_dir (it doesn't replace the latter with the former). Is there a misunderstanding? |
|
|
| msg61540 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-01-22 22:19 |
| `shutil.move(source_file, destination_dir)` does move source_file to destination_dir but the point is that even when source and destinations are on the same file system, it still "copies" the data when there is no need for it. The reason is explained in the very first comment. |
|
|
| msg61541 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-22 22:26 |
| Raghuram, I had understood that, I was answering Guido's objection :-) |
|
|
| msg61544 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2008-01-22 22:33 |
| Antoine: please just ignore me, even though I may once have written this code, I clearly don't understand it any more. :-) All we need now is a patch and someone to review it, right? |
|
|
| msg61584 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-23 14:46 |
| Before tackling this, I'd like a precision on os.rename(src, dst) semantics. The documentation says "If dst is a directory, OSError will be raised". However, under Linux, if src is a directory and dst is an empty directory, dst is overwritten with src: $ mkdir src dst $ touch dst/t $ ./python -c "import os; os.rename('src', 'dst')" Traceback (most recent call last): File "", line 1, in OSError: [Errno 39] Directory not empty $ rm dst/t $ ./python -c "import os; os.rename('src', 'dst')" $ Is this a bug, a misfeature, or just an imprecision in the documentation? |
|
|
| msg61585 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-01-23 15:17 |
| > Before tackling this, I'd like a precision on os.rename(src, dst) > semantics. The documentation says "If dst is a directory, OSError will > be raised". However, under Linux, if src is a directory and dst is an > empty directory, dst is overwritten with src: I think the doc is incorrect. The rename() man page doesn't explicitly say that destination can not be directory. In fact, a small C program directly calling rename() (on SuSE 10) behaves exactly as your python script. |
|
|
| msg61586 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-23 15:29 |
| Does this mean we should preserve this behaviour for shutil.move() as well? Right now, shutil.move(src_dir, dst_dir) replaces dst_dir with src_dir if dst_dir is empty, but moves src_dir inside dst_dir otherwise. |
|
|
| msg61588 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-01-23 16:07 |
| > Does this mean we should preserve this behaviour for shutil.move() as well? I don't think so. The key is to remember that shutil.move() is os.rename() with some additional benefits (as stated by Guido in an earlier comment). Also, changing the behaviour of shutil.move() may break backwards compatibility. I thought this issue has reached a conclusion that all one need is a doc update. Some thing like (copying from my previous comment): ""If the destination is a fiile and is on same filesystem as that of src, then simply use rename." In fact, even the issue's component has been changed to "Documentation". |
|
|
| msg61606 - (view) |
Author: Guido van Rossum (gvanrossum) * |
Date: 2008-01-23 21:28 |
| > Before tackling this, I'd like a precision on os.rename(src, dst) > semantics. The documentation says "If dst is a directory, OSError will > be raised". However, under Linux, if src is a directory and dst is an > empty directory, dst is overwritten with src: > > $ mkdir src dst > $ touch dst/t > $ ./python -c "import os; os.rename('src', 'dst')" > Traceback (most recent call last): > File "", line 1, in > OSError: [Errno 39] Directory not empty > $ rm dst/t > $ ./python -c "import os; os.rename('src', 'dst')" > $ > > Is this a bug, a misfeature, or just an imprecision in the documentation? To be honest, I wasn't aware of this behavior of the rename() system call for directories on Unix. It is most certainly a feature (of the system call) that should be maintained (for the system call wrapper). shutil.move() should not mimic this though -- it should more closely approximate the "mv" utility's behavior, which doesn't differentiate between empty and non-empty destination directories, and always moves *into* the target when it is a directory (and complains if the resulting destination path already exists). |
|
|
| msg61656 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-24 23:19 |
| Here is a patch, which also contains additional unit tests for shutil.move(). It would be nice if people tested it under various configurations, to see if there are any problems. |
|
|
| msg61800 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-01-28 21:56 |
| Hi Antoine, You stated the following in a previous comment: "Right now, shutil.move(src_dir, dst_dir) replaces dst_dir with src_dir if dst_dir is empty, but moves src_dir inside dst_dir otherwise." But my test shows differently. If dst_dir doesn't exist or if it is empty, dst_dir is simply replaced with src_dir. If dst_dir is non-empty, shutil.move() raises error. In which case did you find src_dir being moved to dst_dir? |
|
|
| msg61817 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-29 11:51 |
| Hi Raghuram, I'm confused, because I can't reproduce it either. I'm afraid I had drunk too much or too little coffee when typing that. Perhaps the patch's semantics should be reconsidered then... What do you think? |
|
|
| msg61828 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-01-29 20:53 |
| We are back to square 1 :-). Your patch incorporates Facundo's suggestion which is 'rename(src_file, dst_dir/`basename src_file`). It is not clear to me from rereading the earlier comments whether Guido rejected this approach or not. I would personally prefer this approach. If this change is not going to be considered, we will be left with making a doc change; something to the effect: "If the destination is a file and is on same file system as that of src, then simply use rename." An additional doc change will be needed to correct the statement "If dst is a directory, OSError will be raised". As you found out, this is not entirely true on Linux and perhaps on other versions of unix. I don't think any other changes are needed. I did not go through the test cases in the patch but I think we can certainly use some of them even if no code change is done. |
|
|
| msg61829 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2008-01-29 21:07 |
| By the way, if we really wanted to be complete in the unit tests, we should also test the case where either src or dst is a symlink to a directory. But it's still much better than originally - there was hardly any unit test for shutil.move(). |
|
|
| msg63946 - (view) |
Author: Sean Reifschneider (jafo) * |
Date: 2008-03-18 17:24 |
| After reviewing the discussion I'm going to accept this because: Guido seemed to me to say "figure it out among yourselves". We're talking about shutil, so mimicing the shell move (mv) semantics is not entirely unreasonable. The current semantics are the same as what this patch implements, this just optimizes it from a copy to a rename if possible. Committed into trunk as rev61527. |
|
|