Closed Bug 793949 Opened 9 years ago Closed 9 years ago

[OS.File] File.copy should not append data

Categories

(Toolkit :: OS.File, defect)

All
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files, 2 obsolete files)

Currently, the pump-based implementation of File.copy opens file with flags |write: true|. Consequently, copying on top of an existing file appends to this file. It should be |trunc: true|.
Attached patch Companion testsuite (obsolete) — Splinter Review
Assignee: nobody → dteller
Attachment #664347 - Flags: review?(nfroyd)
Comment on attachment 664347 [details] [diff] [review]
Companion testsuite

Review of attachment 664347 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/osfile/tests/mochi/worker_test_osfile_front.js
@@ +407,5 @@
> +  // Copy a second time to ensure that we are overwriting the destination file,
> +  // as expected
> +  OS.File.copy(src_file_name, tmp_file_name);
> +
> +  compare_files("test_copy_existing 2", src_file_name, tmp_file_name);

Please choose a different file to copy for this second test, just to ensure that the second copy is actually copying and not just no-op'ing or something.
Attachment #664347 - Flags: review?(nfroyd) → review+
Comment on attachment 664350 [details] [diff] [review]
Changing the flags in File.copy

Review of attachment 664350 [details] [diff] [review]:
-----------------------------------------------------------------

I'm assuming we don't have this problem on Windows because copy is implemented with an OS primitive rather than hand-rolling it ourselves?

::: toolkit/components/osfile/osfile_unix_front.jsm
@@ +517,5 @@
>             source = File.open(sourcePath);
>             if (options.noOverwrite) {
>               dest = File.open(destPath, {create:true});
>             } else {
> +             dest = File.open(destPath, {trunc:true});

Doh.
Attachment #664350 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 664350 [details] [diff] [review]
> Changing the flags in File.copy
> 
> Review of attachment 664350 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm assuming we don't have this problem on Windows because copy is
> implemented with an OS primitive rather than hand-rolling it ourselves?

That's the idea.
> 
> ::: toolkit/components/osfile/osfile_unix_front.jsm
> @@ +517,5 @@
> >             source = File.open(sourcePath);
> >             if (options.noOverwrite) {
> >               dest = File.open(destPath, {create:true});
> >             } else {
> > +             dest = File.open(destPath, {trunc:true});
> 
> Doh.

Yeah, I know :)
Attachment #664347 - Attachment is obsolete: true
Attachment #665187 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/dabd0af858d7
https://hg.mozilla.org/mozilla-central/rev/4280a881fef0
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.