search

sindresorhus / trash

Move files and directories to the trash
MIT License
2.57k stars 78 forks source link

linux: fix to comply with XDG, and show correct name in GNOME Trash #86

Closed sourcejedi closed 4 years ago

sourcejedi commented 5 years ago

When trashing a file or directory, the implementation MUST create the corresponding file in $trash/info first. Moreover, it MUST try to do this in an atomic fashion, so that if two processes try to trash files with the same filename this will result in two different trash files. On Unix-line systems this is done by generating a filename, and then opening with O_EXCL.

https://specifications.freedesktop.org/trash-spec/trashspec-latest.html

It looks like GnomeVFS was sanity-checking the timestamps. So if you break the rule and don't create the trashinfo file first, then GnomeVFS will ignore it :-).

Fixes #56

sindresorhus commented 5 years ago

Have to manually tested that this fixes the problem on Ubuntu and that you can now correctly restore trashed files?

sindresorhus commented 5 years ago

And how about:

I see a discrepancy in the date format within the .trashinfo files that trash generates. That date format breaks the spec, so it should be fixed, but it is unsure whether this date format discrepancy is related to the larger problem. - https://github.com/sindresorhus/trash/issues/56#issuecomment-465103904

?

sourcejedi commented 5 years ago

Ah. I was ambiguous in both cases :-).

Have to manually tested that this fixes the problem on Ubuntu and that you can now correctly restore trashed files?

I saw issue #56 happen on Gnome in Fedora 30 (gvfs-1.40.2-1.fc30.x86_64). I tested this PR fixes the problem. The trashed files now show up with the correct filename in Gnome Trash. Gnome Trash is also able to restore these trashed files, to the correct location & filename.

I haven't bothered to test Gnome in Ubuntu as well. It should just be the same.

I see a discrepancy in the date format within the .trashinfo files that trash generates. That date format breaks the spec, so it should be fixed, but it is unsure whether this date format discrepancy is related to the larger problem. - https://github.com/sindresorhus/trash/issues/56#issuecomment-465103904

It turns out that is not "related to the larger problem". #56 is caused by the modified time on the info file being later than the modified time on the trashed file. It is not caused by the format of DeletionDate= in the info file.

IMO this PR is more important, and can be merged on its own.

I agree it would be good to look at the discrepancy in the format of DeletionDate=. I think the xdg-trash spec is a mess here

Therefore, if you change the format of DeletionDate=, it might be an idea to look a bit further than Gnome. Just in case there is something else non-obvious that we missed. It is probably fine, it seems such a simple change that I don't have an idea where it could go wrong :-).

sourcejedi commented 5 years ago

56 is caused by the modified time on the info file being later than the modified time on the trashed file.

Sorry, that's what I had thought, but now I'm not sure it makes sense. Because simply rename()'ing the file into the trash doesn't modify it. I expect file timestamps are involved somehow though.

sindresorhus commented 4 years ago

I'm confused by your last comment. Are you saying this should be merged or not?

sindresorhus commented 4 years ago

@stroncium Any thoughts on this change?

stroncium commented 4 years ago

@sindresorhus TLDR: feels like OP isn't sure it actually works(Personally, I would be slightly surprised if it did.), we should probably wait for him as it would take some time for me to get into all the details. The code change itself is dead simple though.

Longer version:

We use random ids long enough for chance of files colliding to be astronomically smaller than chance of HW failure, so this rule won't have effect on us. Following specification is good, but as we skip a lot of parts of that specification, doesn't feel like we should care about this particular thing(especially when it's gonna slow implementation down and won't have any effect realistically).

Moreover, the spec only explicitly mentions creation but not modification, so it isn't even obvious that someone should expect modification time to be in some fixed relationship with original file modification time. However, if it is the case for some (widespread) implementations we should probably support it(though would be cool to also find clarification on specs and maybe report a bug against their implementation).

I haven't bothered to test Gnome in Ubuntu as well. It should just be the same.

Ubuntu is well known for messing everything up, and as it is about bells and whistles, chances they have some custom stuff by default are somewhat high. Better test it there too, but we could probably ask original reporter to do it(I don't have Ubuntu VMs and am not too eager to configure them).

@sourcejedi re: ramblings

Why does it use local time, without specifying the timezone? That's not how any filesystem works, apart from FAT, and that is regarded as undesirable :-). It would be better for trash times to work more consistently with filesystem timestamps. The spec does not provide a rationale. (It also mentions the filesystem's local time - I have no idea what that means).

FS local time would be the time which is used to write to write timestamps in said FS. User local time thing is for the cases when source FS doesn't have timestamps(or they can't be read) or the file is trashed to another FS, which got different timezone from the source FS(so there is no single timestamp which would be correct for both). (I don't think our implementation checks for any of this though. >_<)

The grammar for rfc3339 requires either a Z or an offset (timezone) at the end. It is not optional! The RFC has a section dedicated to rejecting the idea of "Unqualified Local Time".

rfc3339 is only used as reference for format parts, format as a whole is explicitly defined as YYYY-MM-DDThh:mm:ss. (And requiring timezone would be a problem actually, since not all systems have time zones set up.)

sindresorhus commented 4 years ago

I'm closing this for now as the change is not proven to be correct.