Web Workflow: Renaming a folder to be inside itself removes it

[makermelissa]

CircuitPython version

Adafruit CircuitPython 8.0.0-beta.0

Code/REPL

N/A

Behavior

If you go into the offline editor http://circuitpython.local/fs/ and rename a folder, it shows the full path. If you set it so it is inside itself (for instance you rename /test/ to /test/test2/) then the folder disappears with no error. I would expect an error message.

Description

No response

Additional information

No response

[makermelissa]

I looked into this a bit and it appears renaming just uses f_rename, which appears to be part of tinyusb (https://github.com/hathach/tinyusb/blob/master/lib/fatfs/ff.c#L3500), so this issue may be more pervasive than I thought and possibly in the wrong place.

[dhalbert]

I think it is using the f_rename() in circuitpython/lib/oofatfs/ff.c. TinyUsb has a version too, but we don't use that implementation of fatfs.

[makermelissa]

Ah, ok. Thanks.

[makermelissa]

Ok, so the way it looks like it works is:

1. It checks that the new location exists and there isn't another entry of the same name
2. It creates a copy of the folder inside there
3. It removes the original folder

Perhaps a check needs to be added to see if the parent of the new location is the same as the old location.

[dhalbert]

This was actually fixed years ago in #1645, but that fix is in fat_vfs_rename() in extmod/vfs_fat.c, and the web workflow code is calling f_rename() directly, which bypasses the fix. We need to study a bit more about whether to move the check into f_rename() or call fat_vfs_rename() instead.

[makermelissa]

Oh nice. Yeah, I may take a look at the code. I was looking at the f_rename() function last week, but with trying to add a fix, I was starting to feel a bit out of my depth, so I put it aside.

However, I think I'm familiar enough and comfortable enough with the current Web Workflow code that I could probably see if changing to fat_vfs_rename() would just work or if it would with minor modifications and make those modifications and test it out, but probably after I finish my guide.

[makermelissa]

It looks like Scott used f_rename() instead of fat_vfs_rename() so that he could get some status information about specific failures and relay that info back via HTTP error codes. Adding some additional checks from fat_vfs_rename() to Web Workflow should be pretty straightforward.