Rename check files #459

jelly · 2024-05-16T16:25:07Z

No description provided.

src/fileActions.jsx

allisonkarlitskaya

This is currently a bit rough around the edge (commented out code and such) but it's pretty much exactly what I was hoping for!

Are you happy with it?

allisonkarlitskaya · 2024-05-22T17:21:20Z

src/files-rename.py

+dest = sys.argv[2]
+# overwrite = sys.argv[3]
+
+if libc6.renameat2(AT_FDCWD, source.encode(), AT_FDCWD, dest.encode(), RENAME_NOREPLACE) == -1:


encode() isn't quite right here. To match what Python normally does with filesystem encoding (which is what the bridge does with the filenames in fsinfo) you need to use surrogate escapes.

It's actually kinda funny since the string we have in sys.argv was actually decoded from the commandline (after being encoded for it, from a string, by the bridge).

In all of those cases, though, I think errors='surrogateescape' is what's in play.

Thanks! I kinda wish we could spawn with native types auto converted like dbus but eh :-) (dream on, dreaaam on)

src/files-rename.py

allisonkarlitskaya · 2024-05-22T17:24:24Z

src/files-rename.py

+# overwrite = sys.argv[3]
+
+if libc6.renameat2(AT_FDCWD, source.encode(), AT_FDCWD, dest.encode(), RENAME_NOREPLACE) == -1:
+    sys.stderr.write(os.strerror(ctypes.get_errno()))


I feel like I'd like to see some sort of structured json output similar to the thing that fsinfo reports on errors...

At the least, though: we should have some way of uniquely identifying the EEXIST case, no?

I was considering returning a different exit code, json feels a bit too overengineered. There are only two scenario's:

All Good = 0

General error = 1

File exists = 2

I keep thinking that I Want to implement some kind of a strerror mechanism in javascript...

I agree, not sure how I get errno.EEXISTS converted to str(). I feel this warrants a bigger discussion and some more thinking outside this PR. I do like the idea!

allisonkarlitskaya · 2024-05-22T17:32:07Z

btw: I feel like this could become a template for more such operations, and once we collect the lot of them it could form the basis of the fsmodify channel we keep discussing...

jelly · 2024-05-23T09:46:52Z

nd such) but it's pretty much exactly what I was hoping for!

Are you happy with it?

I want to move into this direction, then also do cut and copy and paste like this but in a different Python file.

jelly · 2024-05-27T13:36:34Z

We need a fallback for when renameate2 is not supported for example on a nfs mount

As we allow renaming symlinks, file and directories just mention the name.

jelly · 2024-05-28T11:31:29Z

I dropped renameat2 as obviously folks are going to want to rename files in a NFS share or something else, which renameat2 does not support. So we would need a fallback there.

allisonkarlitskaya · 2024-05-28T11:36:04Z

src/fileActions.jsx

    } else if (val.includes("/")) {
-        setNameError(_("Directory name cannot include a /."));
+        setNameError(_("Name cannot include a /."));
+    } else if (cwdInfo?.entries[val]) {


So no "Rename and overwrite" option, at present?

Not presently, I have supported it in renameat2 but not here as this was a completely new scenario and when I made UI for it I did not receive positive feedback.

allisonkarlitskaya · 2024-05-28T11:36:14Z

src/fileActions.jsx

@@ -436,16 +438,18 @@ const EditPermissionsModal = ({ selected, path }) => {
    );
 };

-const setDirectoryName = (val, setName, setNameError, setErrorMessage) => {
+const setDirectoryName = (val, setName, setNameError, setErrorMessage, cwdInfo) => {


This function should be renamed now.

Note also, the mkdir PR stops using this function (since it creates its own copy in a separate file).

I'd say we refactor it there then.

I mean, this function has always had the wrong name (since it was used from the rename dialog as well) but you've fixed the messages here, but not the function name...

Changed it, but imo it is out of scope for this review

allisonkarlitskaya · 2024-05-28T11:36:44Z

src/fileActions.jsx

@@ -185,6 +186,7 @@ const CreateDirectoryModal = ({ currentPath }) => {

 const RenameItemModal = ({ path, selected }) => {


I wonder if you'd like to move this to a new tsx file in src/dialogs/ like the mkdir PR does...

No, this PR has been open for 2 weeks now, and then it will conflict again.

This slightly improves the situation where one can rename an existing file but there is still an obvious race. The validation only happens when one changes the input, if you touch something via for example an ssh session you will still overwrite the file. For that we can't use `--no-clobber` as that does not fail with an informative error.

martinpitt

Thanks! I like this. It's a gradual improvement, simple, and robust. Not landing myself yet, as there was lots of discussion with @allisonkarlitskaya and she may want the last word here.

allisonkarlitskaya

This is pretty trivial now, and it's a step in the right direction.

Strictly speaking, the suggested change would be nice, but it's no big deal if we want to handle that later.

allisonkarlitskaya · 2024-05-31T07:17:23Z

src/fileActions.jsx

@@ -436,7 +438,7 @@ const EditPermissionsModal = ({ selected, path }) => {
    );
 };

-const setDirectoryName = (val, setName, setNameError, setErrorMessage) => {
+const setInputName = (val, setName, setNameError, setErrorMessage, cwdInfo) => {


minor nag: this change landed in the wrong commit. The chosen name is also a bit weird, so if you're gonna move it to the other commit (which makes all the other changes to this function) maybe call it something like setFileName() instead?

And it's a completely separate topic, but I dislike the structure of this function: I reworked it for my mkdir patch to be a simple function which took the proposed filename and returned a string describing an error (in case of error) or undefined for "no error". I kept the state-modification logic in the component itself, which reduced the number of arguments to the error-checking function.

jelly requested a review from allisonkarlitskaya May 16, 2024 16:25

jelly force-pushed the rename-check-files branch from 1da05f9 to 19c2b6b Compare May 16, 2024 16:45

allisonkarlitskaya reviewed May 21, 2024

View reviewed changes

src/fileActions.jsx Outdated Show resolved Hide resolved

jelly requested a review from allisonkarlitskaya May 22, 2024 09:10

jelly mentioned this pull request May 22, 2024

Don't move a file under a directory when renaming #404

Closed

jelly force-pushed the rename-check-files branch from 0acb191 to 7a2c535 Compare May 22, 2024 16:21

allisonkarlitskaya reviewed May 22, 2024

View reviewed changes

jelly force-pushed the rename-check-files branch from 7a2c535 to 78ca82b Compare May 23, 2024 09:40

jelly requested a review from allisonkarlitskaya May 23, 2024 09:47

Drop Directory from rename/create modal error texts

c3b1570

As we allow renaming symlinks, file and directories just mention the name.

jelly force-pushed the rename-check-files branch from 78ca82b to 7e0b2e7 Compare May 28, 2024 11:28

allisonkarlitskaya requested changes May 28, 2024

View reviewed changes

jelly force-pushed the rename-check-files branch from 7e0b2e7 to 7270461 Compare May 28, 2024 12:00

jelly requested a review from allisonkarlitskaya May 28, 2024 12:19

jelly force-pushed the rename-check-files branch from 7270461 to 5ce02ac Compare May 29, 2024 06:56

martinpitt approved these changes May 29, 2024

View reviewed changes

allisonkarlitskaya approved these changes May 31, 2024

View reviewed changes

jelly merged commit 19cdfa7 into cockpit-project:main May 31, 2024
12 checks passed

jelly deleted the rename-check-files branch May 31, 2024 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename check files #459

Rename check files #459

jelly commented May 16, 2024

allisonkarlitskaya left a comment

allisonkarlitskaya May 22, 2024

allisonkarlitskaya May 22, 2024

jelly May 23, 2024

allisonkarlitskaya May 22, 2024

jelly May 23, 2024

allisonkarlitskaya May 23, 2024

jelly May 23, 2024

allisonkarlitskaya commented May 22, 2024

jelly commented May 23, 2024

jelly commented May 27, 2024

jelly commented May 28, 2024

allisonkarlitskaya May 28, 2024

jelly May 28, 2024

allisonkarlitskaya May 28, 2024

jelly May 28, 2024

allisonkarlitskaya May 28, 2024

jelly May 29, 2024

allisonkarlitskaya May 28, 2024

jelly May 28, 2024

martinpitt left a comment

allisonkarlitskaya left a comment

allisonkarlitskaya May 31, 2024

allisonkarlitskaya May 31, 2024

		@@ -185,6 +186,7 @@ const CreateDirectoryModal = ({ currentPath }) => {

		const RenameItemModal = ({ path, selected }) => {

Rename check files #459

Rename check files #459

Conversation

jelly commented May 16, 2024

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonkarlitskaya commented May 22, 2024

jelly commented May 23, 2024

jelly commented May 27, 2024

jelly commented May 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martinpitt left a comment

Choose a reason for hiding this comment

allisonkarlitskaya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment