Ticket #4720: delete resource forks on macOS last #4722

zyv · 2025-06-09T05:41:37Z

Proposed changes

Resolves: #4720

…delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

zyv · 2025-06-09T05:42:39Z

Could you please test this, @ollixx77?

@aborodin I'm still not sure if I'm handling return codes correctly.

ossilator · 2025-06-09T06:45:01Z

lib/fs.h

@@ -108,6 +108,11 @@
 #define DIR_IS_DOT(x)     ((x)[0] == '.' && (x)[1] == '\0')
 #define DIR_IS_DOTDOT(x)  ((x)[0] == '.' && (x)[1] == '.' && (x)[2] == '\0')

+/* On macOS, if a file system doesn't support metadata, it is automatically stored in shadow files
+ * called resource forks. The names of these files begin with `._`. They are managed automatically


that's kinda misleading. see https://en.wikipedia.org/wiki/Resource_fork .

So, what exactly is misleading?

it's not the files that are called resource forks. resource (and data) forks are stored in these magic shadow files if the fs doesn't support forks natively.
technically, it's also not metadata ... paradata would fit better, if that was a thing.

Okay, I've revised my comment.

ossilator · 2025-06-09T06:48:39Z

src/filemanager/file.c

        else
-            return_status = erase_file (ctx, tmp_vpath);
+        {
+            if (!FILE_IS_RESOURCE_FORK (next->d_name) || delete_resource_forks)


the conditions should be swapped for efficiency in the negative (== normal) case.

One would think that AI-powered compilers running on laptops suspended from quadcopters would optimize for that. But you are right; they do not.

ossilator · 2025-06-09T06:58:41Z

src/filemanager/file.c

@@ -1439,9 +1439,14 @@ try_erase_dir (file_op_context_t *ctx, const vfs_path_t *vpath)
  abort -> cancel stack
  ignore -> warn every level, gets default
  ignore_all -> remove as much as possible
+
+  This function should either be called with delete_resource_forks = TRUE always, or called twice,


this feels complex and dirty.
my first thought would be to just put files that look like forks into a list, and delete them in a second loop. it's kinda like your sorting idea, just more targeted.

I thought about it, but this looks much more complicated to me. I'm not ready to implement anything like this in the current code base.

I thought the functions would retrieve a list of items to delete from the panels. However, it's a recursive call tree connected with callbacks to the progress dialog. The easiest and safest method seems to be simply calling the recursion twice.

easiest, i guess. but safest ...? this looks like an invitation for double accounting, dialogs asking twice for the same error, etc. it's much harder to guarantee that it works in every case, and that it keeps working.

i'm not sure why you think the separate loop would be much more complicated. it would be a little bit more code (part of the loop body would have to be duplicated or factored out), but seems rather straight-forward.

I think it's complicated because it's difficult for me to do. Maybe someone can help.

ollixx77 · 2025-06-09T07:30:08Z

Could you please test this, @ollixx77?

@aborodin I'm still not sure if I'm handling return codes correctly.

The original change worked so far.
But took me while to get this all setup to install.

…ursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

…oid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

mc-worker · 2025-06-09T19:07:30Z

src/filemanager/file.c

-            return_status = recursive_erase (ctx, tmp_vpath);
+        {
+            return_status = recursive_erase (ctx, tmp_vpath, FALSE);
+            if (return_status != FILE_ABORT)


Please make this compiled conditionally. Users of OSes other than macOS must not feel bad.

I don't think it will put users of other operating systems at a disadvantage, but if you think this is the right approach, I can compile it conditionally. I was concerned that it would make the code more difficult to maintain.

i also had the thought to make this os-specific. but then, what if it's an nfs mount from a mac?

i also had the thought to make this os-specific. but then, what if it's an nfs mount from a mac?

Yes, this is a good argument.

But what about non-macOS users? What is the disadvantage for them? Only if there are files beginning with ._ in the tree will anything be different; in all other cases, there will be no change.

I think this is extremely rare on other systems, and when it does happen, the impact will be negligible, so I would rather drop my fixups; they don't look good.

mc-worker · 2025-06-09T19:08:34Z

src/filemanager/file.c

        else
-            return_status = erase_file (ctx, tmp_vpath);
+        {
+            if (delete_resource_forks || !FILE_IS_RESOURCE_FORK (next->d_name))


Please make this compiled conditionally.

mc-worker · 2025-06-09T19:08:52Z

src/filemanager/file.c

-            error = recursive_erase (ctx, vpath);
+        {
+            error = recursive_erase (ctx, vpath, FALSE);
+            if (error != FILE_ABORT)


Please make this compiled conditionally.

…t to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

…cOS last to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

…s on macOS last to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

ossilator · 2025-06-10T07:30:58Z

now i wonder ...

given that these are "shadow" files that always go along with "real" files, and we're talking about a situation of actually running on macos (otherwise the case of the files magically disappearing would be irrelevant), why aren't the files hidden by the os? this looks like a shortcoming that would cause all kinds of problems.

which brings me to the next point: shouldn't for example move/rename also have related code? that would suggest that this should be handled at the vfs level.

but then there is copy. how would copying a file with forks even work on an fs that supports them natively, when they are invisible to regular unix apis? or does the posix layer actually inject the shadow files?

i guess this should be properly researched before going any further with this.

Ticket #4720: delete resource forks on macOS last to avoid recursive …

a009062

…delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

zyv requested a review from aborodin June 9, 2025 05:41

zyv self-assigned this Jun 9, 2025

github-actions bot added the needs triage Needs triage by maintainers label Jun 9, 2025

github-actions bot added this to the Future Releases milestone Jun 9, 2025

github-actions bot added the prio: medium Has the potential to affect progress label Jun 9, 2025

zyv added area: core Issues not related to a specific subsystem and removed needs triage Needs triage by maintainers labels Jun 9, 2025

zyv modified the milestones: Future Releases, 4.8.34 Jun 9, 2025

ossilator suggested changes Jun 9, 2025

View reviewed changes

zyv added 2 commits June 9, 2025 10:06

fixup! Ticket #4720: delete resource forks on macOS last to avoid rec…

6d1544c

…ursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

fixup! fixup! Ticket #4720: delete resource forks on macOS last to av…

e563773

…oid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

mc-worker reviewed Jun 9, 2025

View reviewed changes

zyv added 3 commits June 10, 2025 07:34

fixup! fixup! fixup! Ticket #4720: delete resource forks on macOS las…

e7aa5ec

…t to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

fixup! fixup! fixup! fixup! Ticket #4720: delete resource forks on ma…

f39514d

…cOS last to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

fixup! fixup! fixup! fixup! fixup! Ticket #4720: delete resource fork…

b2363ba

…s on macOS last to avoid recursive delete failures Signed-off-by: Yury V. Zaytsev <[email protected]>

Uh oh!

Ticket #4720: delete resource forks on macOS last #4722

Are you sure you want to change the base?

Ticket #4720: delete resource forks on macOS last #4722

Uh oh!

Conversation

zyv commented Jun 9, 2025

Proposed changes

Uh oh!

zyv commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ollixx77 commented Jun 9, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ossilator commented Jun 10, 2025

Uh oh!

Uh oh!