| msg62193 - (view) |
Author: André Fritzsche (computercrustie) |
Date: 2008-02-08 06:15 |
| shutil.destinsrc(src,dst) Checks if 'dst' starts with 'src', which can return a wrong result if 'dst' even starts with 'scr' but isn't really a subdirector of it. E.g. (src=r'C:\data', dst=r'C:\data.old') returned true, although dst isn't a subdirectory of src. I tried to fix this creating the absolute paths of 'dst' and 'src' appended the path seperator, if there wasn't one. Then did the check again and now the result is correct. See the diff file I've appended (and hopefully created correctly) |
|
|
| msg62200 - (view) |
Author: Raghuram Devarakonda (draghuram)  |
Date: 2008-02-08 17:08 |
| Can you write a test to catch this problem? The patch should preferably be against the latest svn source. |
|
|
| msg62202 - (view) |
Author: Christian Heimes (christian.heimes) *  |
Date: 2008-02-08 18:56 |
| The fix may get into 2.5 but definitely not into 2.4 because it's not a security fix. |
|
|
| msg62205 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-02-08 20:37 |
| I added couple of test cases. Please see the patch shutil_destinsrc.patch. |
|
|
| msg62206 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-02-08 20:38 |
| Christian, do you mind testing on windows? I tested only on Linux. |
|
|
| msg62272 - (view) |
Author: André Fritzsche (computercrustie) |
Date: 2008-02-11 05:50 |
| Raghuram, you've been too fast ;-) Your test matches the problem. I don't know if it happens on Linux, but on my Win32 Installation the test 'test_destinsrc_2' fails with an AssertionError 'destinsrc() wrongly concluded that dst (@test\srcdir.new) is in src (@test\srcdir)'. Using the submitted patch everything comes out with 'Ok'. I've appended the failing test output. |
|
|
| msg62288 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-02-11 14:23 |
| > Raghuram, you've been too fast ;-) Sorry about that :-) and thanks for validating the test cases. |
|
|
| msg63155 - (view) |
Author: Joseph Armbruster (JosephArmbruster) |
Date: 2008-03-01 01:37 |
| Using: http://svn.python.org/projects/python/trunk @ 61127 OS Name: Microsoft Windows XP Professional OS Version: 5.1.2600 Service Pack 2 Build 2600 test_shutil 1 test OK. |
|
|
| msg63156 - (view) |
Author: Joseph Armbruster (JosephArmbruster) |
Date: 2008-03-01 02:21 |
| On another note, I just completed building the docs in windows and shutil.destinsrc does not appear to be documented. I did notice this description for shutil: "The shutil module offers a number of high-level operations on files and collections of files. In particular, functions are provided which support file copying and removal." I would have expected to find a function like this in os.path, not shutil. |
|
|
| msg63157 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-03-01 03:16 |
| On Fri, Feb 29, 2008 at 9:21 PM, Joseph Armbruster <report@bugs.python.org> wrote: > On another note, I just completed building the docs in windows and > shutil.destinsrc does not appear to be documented. I did notice this > description for shutil: destinsrc() is an internal function used only in shutil.move(). |
|
|
| msg63158 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-03-01 03:48 |
| Should it get a _ prepended to it then? |
|
|
| msg63159 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-03-01 03:53 |
| > Should it get a _ prepended to it then? Probably yes. |
|
|
| msg63163 - (view) |
Author: Christian Heimes (christian.heimes) * |
Date: 2008-03-01 15:51 |
| The function is just a one liner in 2.6 and it's used in one place only. Let's move it into move(). |
|
|
| msg63166 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2008-03-01 16:31 |
| Here's a patch that moves destinsrc into move. |
|
|
| msg63168 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-03-01 17:38 |
| > The function is just a one liner in 2.6 and it's used in one place only. > Let's move it into move(). Isn't it clear from this issue's original description that there is a bug in the one liner that needs fix ? |
|
|
| msg65681 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2008-04-22 19:43 |
| Can some one with commit privileges check in shutil_destinsrc.patch? The change is rather simple and there is no point for issues such as these to remain open for long time. |
|
|
| msg80660 - (view) |
Author: James Cooper (jamescooper) |
Date: 2009-01-27 17:35 |
| Our company recently rediscovered this bug in 2.5.2. After a couple hours of debugging, we realized the error message was incorrect and found the bug in the destinsrc function. This may not be a show-stopping bug, but it's non-obvious, annoying, unnecessary, and very easy to fix. Any chance of getting this into a release? |
|
|
| msg80661 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2009-01-27 17:39 |
| On Tue, Jan 27, 2009 at 12:35 PM, James Cooper <report@bugs.python.org> wrote: > This may not be a show-stopping bug, but it's non-obvious, annoying, > unnecessary, and very easy to fix. Any chance of getting this into a > release? Considering that the issue is idle for such a long time, I am not sure if any one will check this in. Your best bet may be to bring this up on python-dev. As you say, the problem is obvious and the fix is simple. |
|
|
| msg80729 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-01-28 22:13 |
| Adding a test as well would be nice. |
|
|
| msg80772 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2009-01-29 16:00 |
| On Wed, Jan 28, 2009 at 5:13 PM, Antoine Pitrou <report@bugs.python.org> wrote: shutil_destinsrc.patch has both the code chage and two test cases. Actually test cases are much longer than the code itself :-). |
|
|
| msg80775 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-01-29 18:32 |
| Sorry, I had only given a quick look at Benjamin's patch, not yours. Actually Benjamin's patch does not seem to address anything, which makes things more confusing. |
|
|
| msg80776 - (view) |
Author: Raghuram Devarakonda (draghuram) |
Date: 2009-01-29 18:45 |
| On Thu, Jan 29, 2009 at 1:32 PM, Antoine Pitrou <report@bugs.python.org> wrote: > Sorry, I had only given a quick look at Benjamin's patch, not yours. > Actually Benjamin's patch does not seem to address anything, which makes > things more confusing. True. It is all a bit unfortunate for such a small change to get bogged down like this. I specifically asked about the patch proposed by Christian and implemented by Benjamin (in ) but no one bothered to reply. Please look at my patch and see if you can apply it. In fact, I even submitted it when Guido asked for some patches to test codereview tool initially. It is at http://codereview.appspot.com/841/show. |
|
|
| msg80784 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2009-01-29 20:38 |
| Committed, thanks! |
|
|
| msg81343 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2009-02-07 18:42 |
| Sorry to bring this fixed-closed issue back again. I see that this was committed in a hurry. Either, shutil.destinsrc should be Documented, there currently does not exists any documentation to explain what destinsrc is supposed to do, or the function should be made _destinsrc to be internal only. I vote for the second approach of making it _destinsrc as it is used only in the shutil.move(). If no action can be taken on a closed bug, I shall open a new one and also attach a patch to make the method private. |
|
|
| msg81344 - (view) |
Author: Benjamin Peterson (benjamin.peterson) * |
Date: 2009-02-07 19:09 |
| Made private in r69415. |
|
|
| msg81345 - (view) |
Author: Senthil Kumaran (orsenthil) * |
Date: 2009-02-07 19:12 |
| Thanks for the quick action. Really nice. |
|
|