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

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

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|.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: Changing the flags in File.copy

Comment 3

[Nathan Froyd [:froydnj]]

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.

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(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 :)

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: Changing the flags in File.copy

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Companion testsuite

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=dc08d840e41c

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #8)
> Try: https://tbpl.mozilla.org/?tree=Try&rev=dc08d840e41c

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dabd0af858d7
https://hg.mozilla.org/integration/mozilla-inbound/rev/4280a881fef0

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/dabd0af858d7
https://hg.mozilla.org/mozilla-central/rev/4280a881fef0