101527 - moveTo function on windows doesn't remove old directory (works on mac, linux)

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Doug Turner (:dougt)]

Shane Caraveo wrote:

> Seems I cannot rename directories with the moveTo function on Windows
> (haven't tried other platforms), it copies the directory tree instead.
> Renaming files works fine.  Here is sample code to repro via xpcshell.
> Thought I would get comments before posting a bug report.
> Thanks
> Shane Caraveo
> ActiveState
> 
> 
> var file =
> Components.classes["@mozilla.org/file/local;1"].createInstance(Components.in
> terfaces.nsILocalFile);
> 
> // assumes test.txt exists already
> // this WORKS
> var fname = "c:\\temp\\test.txt";
> var newfname = "c:\\temp\\test2.txt";
> 
> file.initWithUnicodePath(fname);
> file.moveToUnicode(null, newfname);
> 
> // assumes that c:\temp\test directory exists already
> // FAILES to rename directory, COPIES it instead, along with
> files/subdirectories
> var path = "c:\\temp\\test";
> var newname = "c:\\temp\\testing";
> file.initWithUnicodePath(path);
> file.moveToUnicode(null, newname);



It's a bug. If newParentDir is null and newName is non-null, it's supposed to be
renamed in place whether it's a file or a dir. The Mac impl does that and I'm
pretty sure the Unix impl does that. So, I think the Windows impl should be
fixed and the comment in the .idl should be clarified.

Comment 1

[Pete Collins (MDG)]

Yes, moveTo works fine on unix. Even across logical drives. ;-)
 
--pete

Comment 2

[Doug Turner (:dougt)]

no fire under my feet for this.  sorry.

Comment 3

[Pete Collins (MDG)]

Assign it to me Doug. I'll try to get to it for 0.9.7.

The only problem i have is I don't own a windows box. 
Anyone want to donate or even lend me a box. 

;-)

--pete

Comment 4

[Luke Stone]

The description above is not exactly the behavior I'm seeing.  I've got a recent
build (Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.9+)
Gecko/20020307) on Win2000.  

When I move a directory, the old directory structure stays, but all files are
moved.  So the final effect is that the dir is copied to a new name, and all
files are removed from the original.  

I'll attach a test file for xpcshell (the sample code above doesn't work for me)
to show what I did.

-Luke

Comment 5

[Luke Stone]

Attachment: test code for xpcshell

This is a short test for moving (renaming) a directory.

It's meant to be fed to xpcshell like
xpcshell> load("test.js");

Comment 6

[Pete Collins (MDG)]

What if you don't use the Unicode API's?

Does it work then or same thing?

I'll take this since i now have a windows box. 

--pete

Comment 7

[Luke Stone]

Attachment: patch to remove directories after moving their contents

this is a trivial patch that adds a remove() after copying all the contents of
a directory. so it gets rid of the empty source directory.  

 on my windows box this makes MoveTo do what it's supposed to.

- Luke

Comment 8

[(not reading, please use [e-mail] instead)]

this MoveTo() bug is blocking an adt1 bug (#137886)

I'll test and drive Luke's patch in.

Comment 9

[(not reading, please use [e-mail] instead)]

Attachment: updated patch against trunk

similar patch to what luke provided, just up to date.
this fixes the problem for me.

Comment 10

[(not reading, please use [e-mail] instead)]

taking, and moving in, since I have an immediate need for this.

Comment 11

[(not reading, please use [e-mail] instead)]

accepting.

Comment 12

[(not reading, please use [e-mail] instead)]

Attachment: updated patch, more comments.

Comment 13

[David :Bienvenu]

this looks like we remove the source in both the  move and copy cases, if I'm
reading the diff correctly. Shouldn't we be checking move?

Comment 14

[(not reading, please use [e-mail] instead)]

Attachment: whoops, only do the remove on move, thanks bienvenu

Comment 15

[David :Bienvenu]

Comment on attachment 80966 [details] [diff] [review]
whoops, only do the remove on move, thanks bienvenu

sr=bienvenu

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 80966 [details] [diff] [review]
whoops, only do the remove on move, thanks bienvenu

r=dougt

Comment 17

[(not reading, please use [e-mail] instead)]

fixed on the trunk.  I'll seek driver approval for the branch.

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 80966 [details] [diff] [review]
whoops, only do the remove on move, thanks bienvenu

a=asa (on behalf of drivers) for checkin to the 1.0 branch

Comment 19

[(not reading, please use [e-mail] instead)]

fixed on both branch and trunk.

Comment 20

[Asa Dotzler [:asa]]

adding fixed1.0.0 keyword (branch resolution). This bug has comments saying it
was fixed on the 1.0 branch and a bonsai checkin comment that agrees. To verify
the bug has been fixed on the 1.0 branch please replace the fixed1.0.0 keyword
with verified1.0.0.