Bug 225307 – Restore pre-r276879 behavior for FileSystem::moveFile()

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Chris Dumez]

Restore pre-r276879 behavior for FileSystem::moveFile(). In particular, FileSystem::moveFile() used to support moving a file across different volumes.

[Chris Dumez]

Created attachment 427562 [details]
Patch

[Darin Adler]

Comment on attachment 427562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427562&action=review

> Source/WTF/wtf/FileSystem.cpp:545
> +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);

I think this may still be different behavior from the old code on Apple platforms. Does it copy the metadata too, or just the data in the file?

> Source/WTF/wtf/FileSystem.cpp:548
> +    return std::filesystem::remove(fsOldPath, ec);

This could be a non-empty directory, so then I think we’d need remove_all, right?

Is it desired behavior that if the remove fails, we leave the second copy behind and return false, but there is no indication that there are now two copies? We could attempt to delete the copy, if that’s the semantic we want. But then we might delete all of the copy and part of the original maybe?

[Chris Dumez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 427562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427562&action=review
> 
> > Source/WTF/wtf/FileSystem.cpp:545
> > +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
> 
> I think this may still be different behavior from the old code on Apple
> platforms. Does it copy the metadata too, or just the data in the file?

All I see in the Apple documentation is:
"""
If the source and destination of the move operation are not on the same volume, this method copies the item first and then removes it from its current location. This behavior may trigger additional delegate notifications related to copying and removing individual items.
"""

I will try it out and check.

> 
> > Source/WTF/wtf/FileSystem.cpp:548
> > +    return std::filesystem::remove(fsOldPath, ec);
> 
> This could be a non-empty directory, so then I think we’d need remove_all,
> right?

Indeed, that is a good point. I'll fix.

> Is it desired behavior that if the remove fails, we leave the second copy
> behind and return false, but there is no indication that there are now two
> copies? We could attempt to delete the copy, if that’s the semantic we want.
> But then we might delete all of the copy and part of the original maybe?

This is what I *thought* was the best behavior. This is very edge-casy but we could indeed also try and restore original state if deleting the original file fails.

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to Darin Adler from comment #2)
> > Comment on attachment 427562 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427562&action=review
> > 
> > > Source/WTF/wtf/FileSystem.cpp:545
> > > +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
> > 
> > I think this may still be different behavior from the old code on Apple
> > platforms. Does it copy the metadata too, or just the data in the file?

What kind of metadata were you concerned about? Things like the files permissions (readable/writeable..) or other things too? (maybe creation time / last modified?).

> 
> All I see in the Apple documentation is:
> """
> If the source and destination of the move operation are not on the same
> volume, this method copies the item first and then removes it from its
> current location. This behavior may trigger additional delegate
> notifications related to copying and removing individual items.
> """
> 
> I will try it out and check.
> 
> > 
> > > Source/WTF/wtf/FileSystem.cpp:548
> > > +    return std::filesystem::remove(fsOldPath, ec);
> > 
> > This could be a non-empty directory, so then I think we’d need remove_all,
> > right?
> 
> Indeed, that is a good point. I'll fix.
> 
> > Is it desired behavior that if the remove fails, we leave the second copy
> > behind and return false, but there is no indication that there are now two
> > copies? We could attempt to delete the copy, if that’s the semantic we want.
> > But then we might delete all of the copy and part of the original maybe?
> 
> This is what I *thought* was the best behavior. This is very edge-casy but
> we could indeed also try and restore original state if deleting the original
> file fails.

For what it's worth [NSFileManager moveItemAtURL:], which is what we were using on macOS, does not remove the copy if the removal of the source fails. It merely reports an error in this case. So the behavior seems similar to what I implemented.

[Darin Adler]

Comment on attachment 427562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427562&action=review

>>>> Source/WTF/wtf/FileSystem.cpp:545
>>>> +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
>>> 
>>> I think this may still be different behavior from the old code on Apple platforms. Does it copy the metadata too, or just the data in the file?
>> 
>> All I see in the Apple documentation is:
>> """
>> If the source and destination of the move operation are not on the same volume, this method copies the item first and then removes it from its current location. This behavior may trigger additional delegate notifications related to copying and removing individual items.
>> """
>> 
>> I will try it out and check.
> 
> What kind of metadata were you concerned about? Things like the files permissions (readable/writeable..) or other things too? (maybe creation time / last modified?).

Since I’m a such an old school Mac person I think of likely-long-obsolete things like "resource forks", file type and creator. There is this general "extended file attributes" category that is what the xattr tool manipulates. Not sure off hand what purposes this is used for.

[Darin Adler]

Comment on attachment 427562 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=427562&action=review

>>>>> Source/WTF/wtf/FileSystem.cpp:545
>>>>> +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
>>>> 
>>>> I think this may still be different behavior from the old code on Apple platforms. Does it copy the metadata too, or just the data in the file?
>>> 
>>> All I see in the Apple documentation is:
>>> """
>>> If the source and destination of the move operation are not on the same volume, this method copies the item first and then removes it from its current location. This behavior may trigger additional delegate notifications related to copying and removing individual items.
>>> """
>>> 
>>> I will try it out and check.
>> 
>> What kind of metadata were you concerned about? Things like the files permissions (readable/writeable..) or other things too? (maybe creation time / last modified?).
> 
> Since I’m a such an old school Mac person I think of likely-long-obsolete things like "resource forks", file type and creator. There is this general "extended file attributes" category that is what the xattr tool manipulates. Not sure off hand what purposes this is used for.

On example is probably the download metadata that is used by Gatekeeper to tell people where a previously unopened file was downloaded from. But some of this is done automatically by the OS with no way to bypass I think.

Trying to get all these things right is the reason programmers use the provided OS level file copying function instead of writing our own. The POSIX level one is copyfile, I think. It has flags to control this: COPYFILE_ACL, COPYFILE_STAT, and COPYFILE_XATTR.

I have no idea if these matter for the cases we use.

[Chris Dumez]

(In reply to Darin Adler from comment #6)
> Comment on attachment 427562 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427562&action=review
> 
> >>>>> Source/WTF/wtf/FileSystem.cpp:545
> >>>>> +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
> >>>> 
> >>>> I think this may still be different behavior from the old code on Apple platforms. Does it copy the metadata too, or just the data in the file?
> >>> 
> >>> All I see in the Apple documentation is:
> >>> """
> >>> If the source and destination of the move operation are not on the same volume, this method copies the item first and then removes it from its current location. This behavior may trigger additional delegate notifications related to copying and removing individual items.
> >>> """
> >>> 
> >>> I will try it out and check.
> >> 
> >> What kind of metadata were you concerned about? Things like the files permissions (readable/writeable..) or other things too? (maybe creation time / last modified?).
> > 
> > Since I’m a such an old school Mac person I think of likely-long-obsolete things like "resource forks", file type and creator. There is this general "extended file attributes" category that is what the xattr tool manipulates. Not sure off hand what purposes this is used for.
> 
> On example is probably the download metadata that is used by Gatekeeper to
> tell people where a previously unopened file was downloaded from. But some
> of this is done automatically by the OS with no way to bypass I think.
> 
> Trying to get all these things right is the reason programmers use the
> provided OS level file copying function instead of writing our own. The
> POSIX level one is copyfile, I think. It has flags to control this:
> COPYFILE_ACL, COPYFILE_STAT, and COPYFILE_XATTR.
> 
> I have no idea if these matter for the cases we use.

I see, there is a std::filesystem::copy_file() which does copy the attributes over. However, it only copies a single regular file.

[Chris Dumez]

Created attachment 427598 [details]
Patch

[Chris Dumez]

(In reply to Chris Dumez from comment #7)
> (In reply to Darin Adler from comment #6)
> > Comment on attachment 427562 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427562&action=review
> > 
> > >>>>> Source/WTF/wtf/FileSystem.cpp:545
> > >>>>> +    std::filesystem::copy(fsOldPath, fsNewPath, std::filesystem::copy_options::overwrite_existing, ec);
> > >>>> 
> > >>>> I think this may still be different behavior from the old code on Apple platforms. Does it copy the metadata too, or just the data in the file?
> > >>> 
> > >>> All I see in the Apple documentation is:
> > >>> """
> > >>> If the source and destination of the move operation are not on the same volume, this method copies the item first and then removes it from its current location. This behavior may trigger additional delegate notifications related to copying and removing individual items.
> > >>> """
> > >>> 
> > >>> I will try it out and check.
> > >> 
> > >> What kind of metadata were you concerned about? Things like the files permissions (readable/writeable..) or other things too? (maybe creation time / last modified?).
> > > 
> > > Since I’m a such an old school Mac person I think of likely-long-obsolete things like "resource forks", file type and creator. There is this general "extended file attributes" category that is what the xattr tool manipulates. Not sure off hand what purposes this is used for.
> > 
> > On example is probably the download metadata that is used by Gatekeeper to
> > tell people where a previously unopened file was downloaded from. But some
> > of this is done automatically by the OS with no way to bypass I think.
> > 
> > Trying to get all these things right is the reason programmers use the
> > provided OS level file copying function instead of writing our own. The
> > POSIX level one is copyfile, I think. It has flags to control this:
> > COPYFILE_ACL, COPYFILE_STAT, and COPYFILE_XATTR.
> > 
> > I have no idea if these matter for the cases we use.
> 
> I see, there is a std::filesystem::copy_file() which does copy the
> attributes over. However, it only copies a single regular file.

Reading the documentation for std::filesystem::copy() more closely, I see that it will end up calling std::filesystem::copy_file() for each file so it will indeed copy both the content and the attributes.

[EWS]

Committed r276936 (237272@main): <https://commits.webkit.org/237272@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427598 [details].

[Radar WebKit Bug Importer]

<rdar://problem/77478629>