classification
Title: shutil.move() does not use os.rename() if dst is a directory
Type: behavior Stage:
Components: Documentation Versions: Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: jafo Nosy List: draghuram, facundobatista, gvanrossum, init, jafo, pitrou
Priority: normal Keywords: patch

Created on 2007-12-09 23:23 by init, last changed 2008-03-21 20:28 by draghuram. This issue is now closed.

Files
File name Uploaded Description Edit
shutilmove.patch pitrou, 2008-01-24 23:19
Messages (23)
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) * (Python committer) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) Date: 2008-01-22 22:26
Raghuram, I had understood that, I was answering Guido's objection :-)
msg61544 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) 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) * (Python committer) 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 "<string>", line 1, in <module>
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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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 "<string>", line 1, in <module>
> 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) (Python triager) 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) * (Python committer) 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) * (Python committer) 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.
History
Date User Action Args
2008-03-21 20:28:47draghuramsetstatus: open -> closed
resolution: accepted -> fixed
2008-03-18 17:24:37jafosetnosy: + jafo
messages: + msg63946
priority: normal
assignee: jafo
keywords: + patch
resolution: accepted
2008-01-29 21:07:31pitrousetmessages: + msg61829
2008-01-29 20:53:21draghuramsetmessages: + msg61828
2008-01-29 11:51:46pitrousetmessages: + msg61817
2008-01-28 21:56:51draghuramsetmessages: + msg61800
2008-01-24 23:19:09pitrousetfiles: + shutilmove.patch
messages: + msg61656
2008-01-23 21:28:20gvanrossumsetmessages: + msg61606
2008-01-23 16:07:01draghuramsetmessages: + msg61588
2008-01-23 15:29:07pitrousetmessages: + msg61586
2008-01-23 15:17:08draghuramsetmessages: + msg61585
2008-01-23 14:46:24pitrousetmessages: + msg61584
2008-01-22 22:33:06gvanrossumsetmessages: + msg61544
2008-01-22 22:26:12pitrousetmessages: + msg61541
2008-01-22 22:19:19draghuramsetmessages: + msg61540
2008-01-22 22:05:10pitrousetnosy: + pitrou
messages: + msg61539
versions: + Python 2.6, - Python 2.5
2008-01-21 15:29:44draghuramsetcomponents: + Documentation, - Library (Lib)
2007-12-12 15:08:30initsetmessages: + msg58495
2007-12-12 14:37:36draghuramsetmessages: + msg58494
2007-12-11 17:24:04gvanrossumsetmessages: + msg58441
2007-12-11 14:53:45draghuramsetnosy: + draghuram
messages: + msg58431
2007-12-10 20:20:40initsetmessages: + msg58368
2007-12-10 18:15:06gvanrossumsetnosy: + gvanrossum
messages: + msg58356
2007-12-10 12:01:10facundobatistasetnosy: + facundobatista
messages: + msg58343
2007-12-09 23:23:24initcreate