classification
Title: distinct error type if shutil.copyfile() fails because of src and dst are the same file
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BreamoreBoy, anthonybaxter, eric.araujo, gennad, georg.brandl, hynek, ishimoto, nnorwitz, pitrou, python-dev, tarek, zooko
Priority: normal Keywords: easy, needs review, patch

Created on 2006-05-22 03:08 by zooko, last changed 2014-03-10 22:11 by python-dev. This issue is now closed.

Files
File name Uploaded Description Edit
patch.txt zooko, 2006-05-22 03:08
1492704.diff gennad, 2011-08-10 14:33 review
new_patch.diff gennad, 2011-09-07 15:14 review
issue1492704_new.patch ishimoto, 2012-07-16 10:28 review
issue1492704_new_2.patch ishimoto, 2012-07-16 16:03 review
issue1492704_new_3.patch ishimoto, 2012-07-16 23:03 review
Messages (42)
msg50324 - (view) Author: Zooko O'Whielacronx (zooko) Date: 2006-05-22 03:08
I need to call shutil.move() and be able to tell the
difference between an error such as access denied and
an error due to the two arguments being the same file.


--- old-dw/src/Lib/shutil.py    2006-05-22
00:06:02.000000000 -0300
+++ new-dw/src/Lib/shutil.py    2006-05-22
00:06:02.000000000 -0300
@@ -16,6 +16,9 @@
 class Error(exceptions.EnvironmentError):
     pass

+class SameFileError(Error):
+    pass
+
 def copyfileobj(fsrc, fdst, length=16*1024):
     """copy data from file-like object fsrc to
file-like object fdst"""
     while 1:
@@ -39,7 +42,7 @@
 def copyfile(src, dst):
     """Copy data from src to dst"""
     if _samefile(src, dst):
-        raise Error, "`%s` and `%s` are the same file"
% (src, dst)
+        raise SameFileError, "`%s` and `%s` are the
same file" % (src, dst)

     fsrc = None
     fdst = None

msg50325 - (view) Author: Zooko O'Whielacronx (zooko) Date: 2006-07-06 22:53
Logged In: YES 
user_id=52562

Please apply.  This patch is completely backwards-compatible
and makes possible some uses of shutil.move().
msg50326 - (view) Author: Zooko O'Whielacronx (zooko) Date: 2006-07-06 22:55
Logged In: YES 
user_id=52562

I'm not sure how to draw attention to my patch, so I will
try assigning it to anthonybaxter.  That ought to get attention.
msg50327 - (view) Author: Zooko O'Whielacronx (zooko) Date: 2007-06-28 15:08
Dear Pythonistas: please apply this patch.  There is no reason not to, and it enables programmers to use shutil more cleanly.
msg50328 - (view) Author: Neal Norwitz (nnorwitz) * (Python committer) Date: 2007-07-06 05:25
In order for patches to be applied, they require tests and doc updates as appropriate.  Can you supply these updates and attach a patch with all the changes?
msg114664 - (view) Author: Mark Lawrence (BreamoreBoy) * Date: 2010-08-22 10:24
@Zooko are you interested in taking this forward?
msg114723 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2010-08-23 01:27
Antoine, is this obsoleted by PEP 3151?
msg141825 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-09 15:10
I have re-read PEP 3151 and think it has no bearing on this bug: the PEP is about adding exception classes that map to errno values, and this report is about a library function that returns a custom exception unrelated to errnos.

I’m willing to review a patch with tests and docs updates for this, or write one myself.
msg141864 - (view) Author: Gennadiy Zlobin (gennad) * Date: 2011-08-10 14:33
This patch should fix the issue
msg142473 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-08-19 16:08
Thanks for the patch.  I made a review on Rietveld; a mail should have been sent.
msg143688 - (view) Author: Gennadiy Zlobin (gennad) * Date: 2011-09-07 15:14
Thanks for the comments! Here'a a new patch.
msg143690 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-07 15:26
Thanks, looks good.  There are a few minor cosmetic things I’ll change before committing.  I assume you have tested it on Windows?
msg143724 - (view) Author: Gennadiy Zlobin (gennad) * Date: 2011-09-08 09:59
My fault. I tested it only partially, relying on the documentation that says

"""
On Windows, if dst already exists, OSError will be raised even if it is a file
"""
Actually it does not (at least at my Windows 7):
(Pdb) os.path.isfile(src)
True
(Pdb) p os.rename(src, src)
None

Am I doing something wrong?
msg143774 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2011-09-09 17:18
By testing, I mean running ./python -m test test_shutil
msg143781 - (view) Author: Gennadiy Zlobin (gennad) * Date: 2011-09-09 17:39
Yes, I got Windows 7, downloaded VS 2008 express, compiled, ran
python python_d.exe -m test test_shutil 
and tests failed. I found out that os.rename does not raise OSError, according to my previous comment...
msg165586 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 10:28
Cleaned up the patch.

Gennadiy added a test of shutil.move(), but SameFileError 
will be raised only if shutil.copy() was called. 

Am I missing something?
msg165599 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-16 13:10
Thanks.  Can you clarify what behavior changed compared to the previous patch?  From the doc it looks like shutil.move now always raises SameFileError, whereas the previous patch said that on Unix it depended on the semantics of os.rename.

The patch has some very minor issues like extraneous whitespace in the docs, but they can be fixed by the person committing.
msg165604 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 13:47
Behavior is not changed at all.

I fixed test_shutil.py to test if SameFileError is raised in
shutil.copy() instead of shutil.move().
msg165606 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-16 13:54
If nobody else, I’ll review it tomorrow at the latest.

On a first glance I realized our shutil Exceptions are all derived from EnvironmentError which is just a compatibility alias for OSError since 3.3.

Quick poll before I open a dedicated ticket, change that for 3.4?
msg165607 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 13:56
So, the title of this issue is misleading.

The patch originally proposed by Zooko does not raise SameFileError in
shutil.move(). If source and destination is same file, shutil.move() may
raise exception, but the exception is NOT SameFileError but OSError or something.

shutil.copy() raises SameFileError if source and destination is same file.
msg165608 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-16 14:00
Well, then fix is for shutil.move() too please, otherwise we can’t close
this ticket which is a pity after over 6 years. :)
msg165609 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 14:09
Well, I happy to improve patch. 

But, on Linux and Windows, shutil.move() does not raise any exception if source and destination are identical. If we change the behavior, I'm afraid we would break a lot of existing applications.
msg165610 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-16 14:19
Sorry, I didn’t look at zooko’s patch which was also just about copyfile. I presume it’s all about the copy fallback that happens when os.rename didn’t work out.

Will look at your code more closely later.
msg165611 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 14:19
Ooops, shutil.move() will raise SameFileError if destination is directory. I'll investigate the patch further more.
msg165629 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 16:03
Patch updated.

- SameFileError is now derived from EnvironmentError.
- Fixed documentation.
- Fixed test method name.

I investigated this patch:

- shutil.copyfile() and shutil.copy() raises SameFileError if source and destination are same file.

- shutil.move() never raises SameFileError. shutil.move() may raise exception if source and dest are same file, but it depends on underlying implementation of platform. Linux and Windows don't raise exception in this case.

- I am opposed to change shutil.move() to raise SameFileError, because such incompatible change can break existing applilcations.
msg165635 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-16 16:42
> - SameFileError is now derived from EnvironmentError.

Why?
msg165638 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 16:55
>> - SameFileError is now derived from EnvironmentError.
>
>Why?

oh, sorry, I misunderstood you suggested to do so.
msg165639 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-16 17:07
No, sorry if my ramblings confused you. I'm pondering about deriving Error from OSError in 3.4. That has nothing to do with this ticket. I just saw it while glancing over your patch.
msg165663 - (view) Author: Atsuo Ishimoto (ishimoto) * Date: 2012-07-16 23:03
Patch updated.

- SameFileError is reverted to be derived from shutil.Error as original patch.
msg165847 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-19 18:26
New changeset 9e94eb39aaad by Hynek Schlawack in branch 'default':
#1492704: Make shutil.copyfile() raise a distinct SameFileError
http://hg.python.org/cpython/rev/9e94eb39aaad
msg165848 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-19 18:30
As beta2 has been postponed I have already committed it with some slight modifications.

Re: deriving from Error: It doesn't make any sense to do so but this way we're mostly backward compatible. Changing it to EnvironmentError uncovered the two extra changes I added.

That said, thank you for your contribution and please fill out and send in a contribution form so you get a neat little star next to your name in the bug tracker: http://www.python.org/psf/contrib/
msg165856 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-07-19 19:14
Did you get an exception from the release manager for this new feature?
msg165863 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-19 19:22
Oh my understanding was that it was pushed to 3.4 only because of the then imminent beta2. Georg, is it okay to keep it?
msg165868 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-07-19 19:34
Sorry, looks like a feature to me.  Please wait for 3.4 with it.
msg165871 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-07-19 19:43
Ok sorry, backing out.
msg165872 - (view) Author: Roundup Robot (python-dev) Date: 2012-07-19 19:43
New changeset 3adb4ee4b794 by Hynek Schlawack in branch 'default':
#1492704: Backout and wait for 3.4
http://hg.python.org/cpython/rev/3adb4ee4b794
msg172290 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-07 10:53
New changeset e11642068f85 by Hynek Schlawack in branch 'default':
Closes #1492704: Make shutil.copyfile() raise a distinct SameFileError
http://hg.python.org/cpython/rev/e11642068f85
msg173943 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-10-27 17:02
I think it should be documented and tested that this change is backward-compatible, as the new error class inherits from the one previously used.
msg174047 - (view) Author: Roundup Robot (python-dev) Date: 2012-10-28 13:00
New changeset e59e274551e0 by Hynek Schlawack in branch 'default':
#1492704: Ensure and document backward compatibility of the change
http://hg.python.org/cpython/rev/e59e274551e0
msg174074 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-10-28 20:26
Thank you!
msg174103 - (view) Author: Hynek Schlawack (hynek) * (Python committer) Date: 2012-10-29 06:00
You're welcome. :)
msg213105 - (view) Author: Roundup Robot (python-dev) Date: 2014-03-10 22:11
New changeset 276227a93f6f by R David Murray in branch 'default':
whatsnew: shutil copyfile SameFileError (#1492704)
http://hg.python.org/cpython/rev/276227a93f6f
History
Date User Action Args
2014-03-10 22:11:28python-devsetmessages: + msg213105
2012-10-29 06:00:09hyneksetmessages: + msg174103
2012-10-28 20:26:13eric.araujosetmessages: + msg174074
2012-10-28 13:00:36python-devsetmessages: + msg174047
2012-10-27 17:02:01eric.araujosetmessages: + msg173943
2012-10-07 10:53:26python-devsetstatus: open -> closed
resolution: fixed
messages: + msg172290

stage: resolved
2012-07-19 19:43:39python-devsetmessages: + msg165872
2012-07-19 19:43:22hyneksetstatus: closed -> open
versions: + Python 3.4, - Python 3.3
messages: + msg165871

resolution: fixed -> (no value)
stage: resolved -> (no value)
2012-07-19 19:34:27georg.brandlsetmessages: + msg165868
2012-07-19 19:22:42hyneksetmessages: + msg165863
2012-07-19 19:14:54eric.araujosetnosy: + georg.brandl
messages: + msg165856
2012-07-19 18:30:06hyneksetstatus: open -> closed
versions: + Python 3.3, - Python 3.4
messages: + msg165848

resolution: fixed
stage: patch review -> resolved
2012-07-19 18:26:37python-devsetnosy: + python-dev
messages: + msg165847
2012-07-16 23:03:39ishimotosetfiles: + issue1492704_new_3.patch

messages: + msg165663
2012-07-16 17:07:09hyneksetmessages: + msg165639
2012-07-16 16:55:12ishimotosetmessages: + msg165638
2012-07-16 16:42:38hyneksetmessages: + msg165635
2012-07-16 16:03:21ishimotosetfiles: + issue1492704_new_2.patch

messages: + msg165629
2012-07-16 14:19:42ishimotosetmessages: + msg165611
2012-07-16 14:19:21hyneksetmessages: + msg165610
title: distinct error type from shutil.move() -> distinct error type if shutil.copyfile() fails because of src and dst are the same file
2012-07-16 14:09:53ishimotosetmessages: + msg165609
2012-07-16 14:00:36hyneksetmessages: + msg165608
2012-07-16 13:56:13ishimotosetmessages: + msg165607
2012-07-16 13:54:59hyneksetmessages: + msg165606
2012-07-16 13:47:24ishimotosetmessages: + msg165604
2012-07-16 13:10:24eric.araujosetversions: + Python 3.4, - Python 3.3
messages: + msg165599

assignee: eric.araujo ->
keywords: + needs review
stage: test needed -> patch review
2012-07-16 12:14:20pitrousetnosy: + hynek
2012-07-16 10:28:56ishimotosetfiles: + issue1492704_new.patch
nosy: + ishimoto
messages: + msg165586

2011-09-09 17:39:22gennadsetmessages: + msg143781
2011-09-09 17:18:45eric.araujosetmessages: + msg143774
2011-09-08 09:59:52gennadsetmessages: + msg143724
2011-09-07 15:26:29eric.araujosetmessages: + msg143690
2011-09-07 15:15:00gennadsetfiles: + new_patch.diff

messages: + msg143688
2011-08-19 16:08:32eric.araujosetmessages: + msg142473
2011-08-10 14:33:46gennadsetfiles: + 1492704.diff
nosy: + gennad
messages: + msg141864

2011-08-09 15:10:39eric.araujosetassignee: tarek -> eric.araujo
messages: + msg141825
versions: + Python 3.3, - Python 3.2
2010-08-23 01:27:36eric.araujosetnosy: + pitrou
messages: + msg114723
2010-08-22 10:24:53BreamoreBoysetnosy: + BreamoreBoy
messages: + msg114664
2010-06-12 09:27:13eric.araujosetassignee: anthonybaxter -> tarek
versions: + Python 3.2, - Python 3.1, Python 2.7
nosy: + tarek, eric.araujo
2009-04-22 12:47:19ajaksu2setkeywords: + easy
2009-03-21 02:46:38ajaksu2setstage: test needed
type: enhancement
versions: + Python 3.1, Python 2.7, - Python 2.6
2006-05-22 03:08:36zookocreate