Add FileSystemHandle::move() method by a-sully · Pull Request #317 · WICG/file-system-access

a-sully · 2021-08-06T15:44:46Z

See MoveAndRename.md

a-sully · 2021-08-06T15:46:55Z

Note that at this point the changes to the explainer are still very much WIP.

Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab

pwnall · 2021-08-16T15:52:11Z

@a-sully Could you please hash out an alternative where the API throws on non-atomic moves?

I think it's worth discussing the benefits (no performance cliff, more flexibility in handling errors for the app, less complexity for browser implementers translating to less room for implementation-specific behavior) versus the costs (potential for errors not caught in development, apps need code to handle the cross-filesystem case).

Also, we should discuss doing different things for moving files vs directories. Moving a large directory across the OPFS -> file system boundary means locking and possibly performing checks (Safe Browsing in Chrome) for an unbounded number of files.

a-sully · 2021-08-16T18:00:53Z

@pwnall I've updated the description to talk about the things you mentioned. To be clear, are you talking about throwing on all moves which we cannot guarantee to be atomic (moves to non-local filesystems) or only when we attempt said move and it fails?

Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab

pwnall

The explainer and spec changes LGTM, given the idea we're proposing.

Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab

a-sully · 2021-09-14T23:14:46Z

Just a quick drive-by review, since I will need to document this on https://web.dev/file-system-access/. Do you have a Chromium bug for me to subscribe to and see when this ships (or can be tested) and thus needs documentation?

@tomayac Just created a launch bug: https://crbug.com/1249662

…temHandle::rename() methods, a=testonly Automatic update from web-platform-tests Add FileSystemHandle::move() and FileSystemHandle::rename() methods Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2984739 Commit-Queue: Austin Sullivan <asully@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/main@{#919810} -- wpt-commits: 72023078617ed82a4c707c7966fffe42b0f0e66a wpt-pr: 29686

szewai · 2021-10-19T21:03:50Z

Hello @a-sully @pwnall @mikewest @mkruisselbrink @tomayac, is there any update on this? Can this be merged?

Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2984739 Commit-Queue: Austin Sullivan <asully@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/main@{#919810}

hcldan · 2022-01-13T16:28:11Z

@a-sully
The https://web.dev/file-system-access/
article makes it sound like you can only rename files and move directories... at least that's how I read it. Maybe change the wording? Unless that's how it works... in which case... why?

Files and folders can be renamed or moved to a new location by calling rename() or move() on the FileSystemFileHandle or FileSystemDirectoryHandle respectively.

tomayac · 2022-01-13T16:40:23Z

This is what's in the article:

Files and folders can be renamed or moved to a new location by calling rename() or move() on the FileSystemFileHandle or FileSystemDirectoryHandle respectively. For move(), the first parameter is a FileSystemDirectoryHandle.

What do you want to change?

hcldan · 2022-01-13T16:49:40Z

Files and folders can be renamed or moved to a new location by calling move() on the FileSystemHandle.

a-sully · 2022-01-13T18:32:53Z

@tomayac we actually just landed a change which consolidates the move() and rename() methods into one overloaded move() method. I've uploaded a new patch on this PR with the changes.

hcldan · 2022-01-13T19:01:16Z

@tomayac I updated my answer in light of the new api. There might be better ways to phrase it.
Actually, the reduction to 1 api removes the ambiguity of the "respectively" ending.

tomayac · 2022-01-14T10:09:06Z

Please see GoogleChrome/web.dev#7172 with these changes.

hcldan · 2022-01-14T17:05:06Z

--- nevermind ---

Actually... if you check this comment revision history (I don't know how to get it back), I realize what I'm doing wrong...
but the error message could use some love.

hcldan · 2022-01-14T17:24:43Z

Not sure I understand this one though...

hcldan · 2022-01-14T17:42:40Z

can you not move dirs with things in them?

a-sully · 2022-01-14T18:10:32Z

@hcldan it looks like there's been some miscommunication on our side. Due to all of the open questions regarding cross-file-system moves, move() is temporarily disabled for directories (hence the abort errors) and moves outside of the Origin-Private File System. This feature is currently only available in the OPFS to those with the AccessHandles origin trial enabled.

@tomayac would you mind updating web.dev to reflect this? (see the launch bug.)

We would like to support this feature more broadly eventually, but to do so we need to address the open questions mentioned in this PR. Apologies for the confusion.

hcldan · 2022-01-14T18:18:19Z

But both of these dirs are within the OPFS.. they aren't cross filesystem... Even those dir moves are disabled?

a-sully · 2022-01-14T18:38:17Z

Yes, directory moves within OPFS are (annoyingly) not atomic so those have been temporarily blocked, as well.

hcldan · 2022-01-14T20:10:48Z

temporarily blocked

How temporarily (how long)?

tomayac · 2022-01-17T12:11:41Z

@tomayac would you mind updating web.dev to reflect this? (see the launch bug.)

Done: GoogleChrome/web.dev@0f2f161.

hcldan · 2022-01-19T15:26:12Z

@hcldan it looks like there's been some miscommunication on our side. Due to all of the open questions regarding cross-file-system moves, move() is temporarily disabled for directories (hence the abort errors) and moves outside of the Origin-Private File System. This feature is currently only available in the OPFS to those with the AccessHandles origin trial enabled.

@tomayac would you mind updating web.dev to reflect this? (see the launch bug.)

We would like to support this feature more broadly eventually, but to do so we need to address the open questions mentioned in this PR. Apologies for the confusion.

@a-sully Thank you for this statement, but it brings up another question for me.
When chrome 99 ships there won't be a flag anymore to rope off AccessHandles. Will this limitation ship in chrome 99, or do you expect there will be a fix for this "soon" (before chrome 99 ships)?

annevk · 2022-02-11T10:57:35Z

@a-sully could you please rebase these commits against https://github.com/whatwg/fs and create a PR there?

a-sully · 2022-03-07T22:57:08Z

This issue has been ported to whatwg/fs#10

…temHandle::rename() methods, a=testonly Automatic update from web-platform-tests Add FileSystemHandle::move() and FileSystemHandle::rename() methods Currently, it is not possible to move or rename a file or directory without creating a new file/directory, copying over data (recursively, in the case of a directory), and removing the original. This CL allows for the atomic moving of a file or directory on the local file system without needing to duplicate data. Moves to non-local file systems will are not guaranteed to be atomic and will involve duplicating data. PR: WICG/file-system-access#317 Bug: 1140805 Change-Id: I774ed1d9616249b6ecc80783db48a7bfee915aab Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2984739 Commit-Queue: Austin Sullivan <asully@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Victor Costan <pwnall@chromium.org> Reviewed-by: Marijn Kruisselbrink <mek@chromium.org> Cr-Commit-Position: refs/heads/main@{#919810} -- wpt-commits: 72023078617ed82a4c707c7966fffe42b0f0e66a wpt-pr: 29686

outline of FileSystemHandle::move()

5ba280f

a-sully requested review from mkruisselbrink and pwnall August 6, 2021 15:44

chromium-wpt-export-bot mentioned this pull request Aug 12, 2021

Add FileSystemHandle::move() and FileSystemHandle::rename() methods web-platform-tests/wpt#29686

Merged

jimmywarting mentioned this pull request Aug 29, 2021

Add move jimmywarting/native-file-system-adapter#18

Closed

a-sully changed the title ~~Add FileSystemHandle::move() method~~ Add FileSystemHandle::move() and method Sep 7, 2021

a-sully changed the title ~~Add FileSystemHandle::move() and method~~ Add FileSystemHandle::move() and FileSystemHandle::rename() methods Sep 7, 2021

a-sully added 3 commits September 8, 2021 16:32

Update EXPLAINER.md

c447973

Update index.bs

1d68183

Update index.bs

c716ead

pwnall approved these changes Sep 8, 2021

View reviewed changes

Comment thread EXPLAINER.md

move discussion to md file

f7bc984

tomayac reviewed Sep 9, 2021

View reviewed changes

Comment thread MoveAndRename.md Outdated

tomayac reviewed Sep 9, 2021

View reviewed changes

Comment thread MoveAndRename.md Outdated

engedy reviewed Sep 14, 2021

View reviewed changes

Comment thread MoveAndRename.md Outdated

a-sully added 2 commits January 13, 2022 18:25

consolidate rename and move

60eba92

consolidate rename and move

250cdba

a-sully changed the title ~~Add FileSystemHandle::move() and FileSystemHandle::rename() methods~~ Add FileSystemHandle::move() method Jan 13, 2022

a-sully mentioned this pull request Mar 7, 2022

Add FileSystemHandle.move method whatwg/fs#10

Open

3 tasks

a-sully closed this Mar 7, 2022

a-sully deleted the move-handle branch June 14, 2023 05:22

Conversation

a-sully commented Aug 6, 2021 • edited by pr-preview Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-sully commented Aug 6, 2021

Uh oh!

pwnall commented Aug 16, 2021

Uh oh!

a-sully commented Aug 16, 2021

Uh oh!

pwnall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

a-sully commented Sep 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szewai commented Oct 19, 2021

Uh oh!

hcldan commented Jan 13, 2022

Uh oh!

tomayac commented Jan 13, 2022

Uh oh!

hcldan commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-sully commented Jan 13, 2022

Uh oh!

hcldan commented Jan 13, 2022

Uh oh!

tomayac commented Jan 14, 2022

Uh oh!

hcldan commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hcldan commented Jan 14, 2022

Uh oh!

hcldan commented Jan 14, 2022

Uh oh!

a-sully commented Jan 14, 2022

Uh oh!

hcldan commented Jan 14, 2022

Uh oh!

a-sully commented Jan 14, 2022

Uh oh!

hcldan commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomayac commented Jan 17, 2022

Uh oh!

hcldan commented Jan 19, 2022

Uh oh!

annevk commented Feb 11, 2022

Uh oh!

a-sully commented Mar 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

a-sully commented Aug 6, 2021 •

edited by pr-preview Bot

Loading

a-sully commented Sep 14, 2021 •

edited

Loading

hcldan commented Jan 13, 2022 •

edited

Loading

hcldan commented Jan 14, 2022 •

edited

Loading

hcldan commented Jan 14, 2022 •

edited

Loading