preserve mtime when copying files

[raslop]

Fixes #869
Relates to #779

[gokcehan]

@raslop Thank you for the patch but I don't think it is a good idea to preseve mtime for all copy operations. Note cp also do not preserve mtimes by default. cp -a preserve mtimes along with many other attributes. It could have been a better default for lf but it is much harder to implement so lf comes with a simple default and you are expected to use custom commands if you don't want the default.

[raslop]

Thanks for the feedback.

I agree that this is a breaking change and that's not a good idea.

This change was not meant to implement cp -a but more cp --preserve timestamps (although it only preserves the mtime, and that was the goal).

The next best thing is to add a configuration option, say preserve_mtime with default off, and do os.Chtimes() conditionally on that. (This also leaves room to later add other preserve_... options, if desired by someone.)

It could have been a better default for lf but it is much harder to implement so lf comes with a simple default

Well, not so hard (in my opinion), see above.

you are expected to use custom commands if you don't want the default.

Hmm, I've tried that, but I didn't want to re-implement progress indication, which I like very much, in my custom command, just to get the mtimes preserved.

I think adding a configuration option to preserve mtimes when copying (see above) is a good trade-off.
What do you think? Is that acceptable?

[gokcehan]

@raslop There seems to be an example in the tips wiki page to show progress bar using rsync though I'm not sure how it works in practice.

I understand the benefit of preserving attributes in the builtin copy but I would rather have them all or at least a reasonable set of them implemented at the same time and keep them under a single option (e.g. set preserve "mode:ownership:timestamps:links"). Currently this patch does not even seem to implement a single attribute to a full extent (replaces atime with mtime and ignores ctime and btime) so it feels quite opinionated. I'm sure someone will complain about this and I doubt you will be around to respond.

If you like, we can keep the PR open for a while to attract others and maybe you or someone else can complete the work to a reasonable extent.

[raslop]

There seems to be an example in the tips wiki page to show progress bar using rsync though I'm not sure how it works in practice.

Indeed, somehow I have missed that. This works indeed quite well.
But still, it requires an external tool and a modification to the configuration file. And there are other things, one has to think about when using a custom-command, the progress indicator was just an example, how to handle existing files is another one.

but I would rather have them all or at least a reasonable set of them implemented at the same time and keep them under a single option (e.g. set preserve "mode:ownership:timestamps:links")

Agreed.

Currently this patch does not even seem to implement a single attribute to a full extent (replaces atime with mtime and ignores ctime and btime) so it feels quite opinionated.

I have to admit that's true. But unfortunately I'm not sure if that's possible with go. The os.Chtimes() only takes atime and mtime, but not ctime or btime. And from the FileInfo object (passed to the copyFile() function) one can only query mtime, but not atime. That's why in my patch atime is replaced with mtime, which I find somehow ok-ish, as atime is not really used nowadays and most people disable it anyway in their filesystems (i.e. mount with >"noatime").

Preserving file-mode should be easy and straightforward (for what I have seen so far in the go library).

Preserving ownership is only possible as super-user, but is not exposed by FileInfo.

Preserving links is only applicable when copying multiple files where some of them are hard-linked (against others in the group to be copied). This is not possible to achieve in the copyFile() function alone, but must be handled somewhere up in the call-stack.
As preserving links is not trivial and somehow esoteric and most probably quite seldom used, I would suggest to not implement it, at least not now, but only if someone really wants to have it.

If you like, we can keep the PR open for a while to attract others and maybe you or someone else can complete the work to a reasonable extent.

Fair enough.
I think, I will read up on the go library functions around filemode, filetimes, ownership and superuser-detection and come back later.

[gokcehan]

@raslop If you are going to work on this, feel free to take a look at times library which we already use in the code elsewhere. That can be useful for the timestamps if I'm not mistaken.

Mode preserving might already be the case as we create the new file with the source file mode. If that's the case, we might think about implementing the opposite.

If you get in doubt, feel free to use cp as the reference behavior as I will probably do the same to review the patch.

[raslop]

I've rebased to the current master and force-pushed the branch.

I've added the suggested preserve option, but only with mode and timestamps values for now (and only mtime and atime for timestamps).
Preserving btime/ctime is not portably possible with go (as far as I understand). One can retrieve btime/ctime with the mentioned times library, but not set them (neither with that lib, nor with the go standard library).
Ownership is vice versa, one can set uid/gid with os.Chown(), but there seems to be no portable way to retrieve them from a file.

The basis is there and can be enhanced in the future, once it's portably possible.

[raslop]

NB: The default value for the new preserve option (i.e. mode) reflects the current behaviour of lf. Setting it to the empty value, will not preserve anything (i.e. mode is set to a hard-coded default (0600), following your idea from the last comment).

@gokcehan: If you have some time, maybe you can have another look the the pull-request.
(and sorry that I come back only after such a long time).

[gokcehan]

@raslop Sorry, I had looked at the PR but I forgot to drop a reply. It looks good in general but I'm not sure 0600 is a proper default. For example cp --no-preserve=all comes up with 0644 on my system. os.Create follows a similar convention (i.e. 0666+umask where umask is commonly 0022). If you use 0666 and call os.OpenFile, I think maybe it applies umask automatically?

[raslop]

@gokcehan I've chosen the 0600 default to not make user-only readable files accidentally world-readable when mode is not preserved. But now I remember you said to use cp for orientation if in doubt.
Please see the FIXUP commit, which changes the default to 0666. Some quick tests show same behaviour as cp --no-preserve=all.

[gokcehan]

@raslop Looks good, thanks for the patch.